Fix external_auth to properly use retfun for @ssl_login_shortcut() #1862
Conversation
There was a problem hiding this comment.
Code coverage suggests that this line isn't tested. Is this something easy to add a test for?
There was a problem hiding this comment.
@brianhw Both of our auth entry functions set retfun, so this can't be hit without calling _external_signup directly. Should I make an "artificial" test directly to _signup with retfun set to None to test, or be bold and remove the safety redirect?
There was a problem hiding this comment.
It's also called by openid_login_complete(), appearing above it. And in that case the retfun is not set. So don't remove the redirect, unless you move it up into openid_login_complete(). Which is a reasonable option (and remove the default value for retfun=None).
There was a problem hiding this comment.
I honestly hadn't thought of what it would look like to have those both turned on. I'll leave it in as it is worth handling either way.
|
Code looks good. Just checking on the diff test results, and noting its flags. |
There was a problem hiding this comment.
We don't typically provide docstrings for unittests (see http://stackoverflow.com/questions/12962772/how-to-stop-python-unittest-from-printing-test-docstring) -- only for helper functions for tests. NBD here but for future development, now you know.
There was a problem hiding this comment.
I'll keep that in mind for the future.
|
👍 once a few minor comments are addressed. |
|
@sarina I think clearer failure messages trump line coverage in this case. 👍 On the tests |
|
@wedaly thanks! @carsongee looks like there are conflicts today. Can you do another rebase? @brianhw can you look at this today or tomorrow so we can merge it and avoid having to make Carson continually rebase? :) |
I also removed conflicting implementation of SSL_AUTH_IMMEDIATE_SIGNUP and rebased on the current master.
|
Looks good. 👍 |
|
@carsongee once you get a clean build, go ahead and merge this. |
|
Cool, will do. Thanks everyone |
Fix external_auth to properly use retfun for @ssl_login_shortcut()
5 participants
The @ssl_login_shortcut() decorator has been broken in the edX codebase for some time, because it did not properly wrap the function being decorated and pass this along to be called after authentication is completed.
This PR fixes that bug.
The changes made have been in production in the residential MITx systems at MIT since spring 2013.
This is an update of PR #653 with tests and the removal of the IMMEDIATE_SIGNUP_FEATURE