Add link/unlink to common/djangoapps/third_party_auth. - #3408
Add link/unlink to common/djangoapps/third_party_auth.#3408johncox-google wants to merge 18 commits into
Conversation
There was a problem hiding this comment.
Can you change <a href="{logout_url}">log out</a> to look like
{link_start}log out{link_end} where link_start=<a href="logout_url"> and link_end=</a>? This makes it a lot easier for the translators.
There was a problem hiding this comment.
Done (though used %s rather than .format to calculate the args to .format, as with the strong tag).
|
@sarina Review comments addressed in 351917f. |
|
@nedbat, @jzoldak, @marcotuts: ping on this review request. |
There was a problem hiding this comment.
Asserts should be about what cannot possibly happen, due to the structure of the code. This function is only called from the dashboard view, and accepts data from the request (I think?) Is an assert the right thing here, or a test and a raise?
There was a problem hiding this comment.
This function does not accept data from the request. messages is populated by python-social-auth's exception middleware's process_exception() call (https://github.com/omab/python-social-auth/blob/master/social/apps/django_app/middleware.py).
An invariant of that code is one call produces one message, and if that invariant were violated this code could silently begin returning incorrect values. The most likely cause of invariant violation would be bumping the version of python-social-auth to a version with a different implementation of process_exception(). In that case, this assert would cause tests to fail loudly with a localized error.
Since there's no way for callers to recover from such an error, raising a more specific exception is not useful.
|
@johncox-google I won't be able to get to this (and 3450) until next week. |
|
Looks like this pull request has failing tests, and it needs to be rebased. |
|
Sure, but rebasing is best done immediately before a merge, right? |
|
@johncox-google in this case, the pull request has conflicts with master -- doing the rebase now will surface those conflicts, so they can be resolved cleanly during the code review process. If we wait until just before merge to do the rebase, then there wouldn't be any review of how those conflicts were resolved. |
|
@singingwolfboy Understood -- we're planning to merge https://github.com/edx/edx-platform/pull/3450 instead of this PR since it's a superset; I'm rebasing/repushing that now. |
|
@johncox-google Ah, OK. Do you want to close this PR, then? |
|
@singingwolfboy Yup, this one and 2994 once 3450 is in. |
|
@singingwolfboy Working with @nedbat on the merge of 3450 now, so hopefully this won't be long-lived. :) |
|
OK, merged as part of https://github.com/edx/edx-platform/pull/3450. Thanks, all (and shout out to @nedbat for all his help with the final merge). |
5 participants
@nedbat, @jzoldak, @marcotuts: Here's the account link/unlink part of the third-party-auth feature set. @marcotuts has reviewed the UX already.
This PR depends upon (and must not be merged before) https://github.com/edx/edx-platform/pull/2994, but I wanted to start the review now. It's 2994 up to 2a69765; please have a look at 87c90f5 and after.
@psimakov, @mgainer, @jbau, @alexmdavis, @version101, @AnuragRamdasan: just tagging you to keep you in the loop; nothing you need to do.