Code review comment for lp:~mhall119/django-openid-auth/fixes-642132

Revision history for this message
James Henstridge (jamesh) wrote :

This should do the check to see if anyone else has the same user name, and should ideally use the same name checking code as create_user_from_openid() does (that way we only need to update the algorithm once if we want to change it).

I would suggest only changing the user name if the existing user name does not have the sreg nickname as a prefix. This should prevent us from trying to change the nickname on every login if there is a duplicate nickname.

With respect to the tests, how about factoring out the body of test_login_update_details() into a helper method, from the first client.post() call and returning the response object returned by client.get('/getuser/'). That helper could then be used to run both tests. I'd also like to see a test of when there is a conflict for the new nickname.

Lastly, a more basic question: do we want to have a setting for this behaviour? If it has minimal overhead and is something most people would want, is there any reason not to always do this?

review: Needs Fixing

« Back to merge proposal