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

password_change_done #3428

Closed
wants to merge 3 commits into from
Closed

password_change_done #3428

wants to merge 3 commits into from

Conversation

amirreza8002
Copy link

i added a page so when you change password it redirects to that instead of staying in the password change view

it's my first time doing this so go easy on me :)

pls let me know what is missing

@pennersr
Copy link
Owner

Currently, when you change your password you indeed stay on the page, and from the user's point of view that looks like this:

image

With the changes made here, you end up on a separate page. This raises several questions:

  • The page content will state "Passwordsuccessfully changed.", yet, that exact content is already in posted as a message. So, if you are aiming for something like this, shouldn't the message (as in thedjango.contrib.messages` one) be dropped?

  • Given that the success page is just a GET request away on its own URL, you can easily scare someone by navigating to that page while they are fetching coffee.

So I have some doubts on whether or not this is truly an improvement over the existing situation. Now, I definitely understand why people would want to navigate away from the password change form, so perhaps that should be made easier by means of adding some form of get_password_change_redirect_url() to the adapter. Then, overriding the default behavior is easier, and people could redirect to e.g. the main dashboard/profile page or something like that.

@pennersr
Copy link
Owner

See 088a4f1 -- that introduces a get_password_change_redirect_url() adapter method. Opting for this so that projects are free to do whatever they see fit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants