Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added social auth... #405

Open
wants to merge 40 commits into
base: develop
Choose a base branch
from
Open

Added social auth... #405

wants to merge 40 commits into from

Conversation

BeOleg
Copy link

@BeOleg BeOleg commented Feb 7, 2014

2 current issues:

  1. The account_detail view takes another full page refresh to update after adding or removing a social network.
  2. Needs some serious redesigning.

@BeOleg BeOleg mentioned this pull request Feb 10, 2014
@@ -206,6 +206,7 @@ def account_login(request, template_name='registration/login.html',
redirect_field_name: redirect_to,
'site': current_site,
'site_name': current_site.name,
'messages': get_messages(request),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use the messages context processor here? It's already enabled in the settings (

'django.contrib.messages',
)


)

try:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why you wrap all of these in a try/except.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ydaniv asked me to catch an import error there, in case some developers
don't have the file with the social media tokens & secret keys.

On Fri, Mar 7, 2014 at 8:08 AM, Paul Walsh [email protected] wrote:

In openbudgets/settings/init.py:

)

+SOCIAL_AUTH_PIPELINE = (

  • 'social_auth.backends.pipeline.social.social_auth_user',
    
  • 'openbudgets.apps.accounts.social_pipeline.associate_by_email', # only difference from default
    
  • 'social_auth.backends.pipeline.user.get_username',
    
  • # 'social_auth.backends.pipeline.user.create_user',
    
  • 'openbudgets.apps.accounts.social_pipeline.create_user',#custom pipeline
    
  • 'social_auth.backends.pipeline.social.associate_user',
    
  • 'social_auth.backends.pipeline.social.load_extra_data',
    
  • 'social_auth.backends.pipeline.user.update_user_details'
    
  • )
    +
    +try:

I don't understand why you wrap all of these in a try/except.

Reply to this email directly or view it on GitHubhttps://github.com//pull/405/files#r10374253
.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, shouldn't this stuff be in the said file then, and not here at all? Seeing as it all strictly depends on that file? Which file are you referring to?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it these are project's defaults and it makes sense to have them here then they should be left here, if not then they can be removed and we only need import social_config in a try-catch clause.

@pwalsh
Copy link
Member

pwalsh commented Mar 7, 2014

Hi @BeOleg there is no need for us to hold on merging this because of the design issues. I'd rather merge it so it is available when we do the next design round.

However, there are some things that need to be done before considering merging it with the codebase:

  1. All of @yprez 's points need to be addressed.
  2. We need basic unit tests that confirm the functionality - we can't merge code related to user registration/login, which is a critical component of the app, without some test coverage, especially around our our case of matching email addresses to "merge" a new social registration/login with existing accounts when there is a match.

<form method="POST" action="{% url "socialauth_disconnect_individual" provider account.id %}"
class="social-revoke-form">
{% csrf_token %}
<input type="submit" class="social-revoke-control" value="-"> {{ provider }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BeOleg <input> is a non-content element. Perhaps it would be better to replace it with a <button> tag and put the {{ provider }} as its content?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to have a - sign side by side with the network's name, to symbolize the revoke of the credentials of this certain network.

Same thing can be achieved by placing {{provider}} in the 'value' attribute of the input element, a button element will work as well, as long as it's has a type='submit' attribute, since I only accept POST request to the credentials revoke resource to prevent XSS.

Wanted to enhance the design of this entire thing, but frankly could not figure out which less file applies to this section :(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry abut UI behaviour, until we do new UI work with Ran.

@pwalsh pwalsh added this to the Future milestone Jun 28, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants