Break-out password reset confirmation so that it is re-usable. #12

wants to merge 2 commits into


None yet
3 participants
+ user = None
+ return user, token_generator.check_token(user, token) if user else False

akaariai May 1, 2012


Whitespace error.


akaariai commented May 1, 2012

A couple of comments (apart of the trivial WS error).

  • Is a new file needed for this: the function is used only in the so maybe it should stay there.
  • I am not sure if we actually do want this - I don't know the code here too well. If that function is something that should stay strictly internal, then splitting it out for reuse isn't wise...
  • Nitpick: I think we are going to aim for at maximum 50 char summary lines in the commit message. Not that it is enforced at all currently, and I don't know how strong the consensus is on this.

So, I am not going to merge this. Somebody with more knowledge should check if that function should be reusable.

tvaughan commented May 1, 2012

I'm happy to adjust the whitespace (per PEP 8 I assume), move this into another file (please tell me where), and shorten the commit title. But what other way would you suggest someone come up with a new password reset view that, for example, uses flash-like messages or is a single-page app with a JSON API without having to worry about the details of how to reset the password? How is this any different than say "auth_logout"?

Tom Vaughan added some commits Apr 28, 2012

Create a re-usable password reset confirmation.
This addresses issues raised in the pull request comments.

 * Whitespace has been adjusted per PEP 8.

 * The commit summary above is less than 50 characters.

 * confirm_password_reset has been moved to django/auth/
   since this is where authenticate, login, and logout live.

tvaughan commented May 7, 2012

Provided this is now acceptable, I could rebase this into a single commit. For the purpose of discussing this pull request, I chose to keep the history as-is. Although I did rebase on top of a current master branch to ensure a clean merge.

lqc commented May 9, 2012

I don't think this is a good idea. The function as you propose it is very view specific, so if I wanted to make my own password_reset view that doesn't use base36 encoded pk in the URL I can't use it. IMHO, a better approach would be to make this a class-based view with proper hook methods.

Not sure what is the current status on auth2, but maybe turning those views to CBV could be a part of it.

tvaughan commented May 9, 2012

OK. Thanks @lqc. That makes sense. I hadn't considered that.

@tvaughan tvaughan closed this May 9, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment