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

Fix LOGOUT_ON_PASSWORD_CHANGE settings not tested #3441

Closed

Conversation

SebCorbin
Copy link
Contributor

@SebCorbin SebCorbin commented Sep 21, 2023

  • Make sure you use semantic commit messages.
    Examples: "fix(google): Fixed foobar bug", "feat(accounts): Added foobar feature".
  • All Python code must formatted using Black, and clean from pep8 and isort issues.
  • JavaScript code should adhere to StandardJS.
  • If your changes are significant, please update ChangeLog.rst.
  • If your change is substantial, feel free to add yourself to AUTHORS.

Let's test this and find out that it is bugged, then I'll ad a new commit to fix (TDD style)

@SebCorbin SebCorbin marked this pull request as ready for review September 21, 2023 09:32
)
return HttpResponseRedirect(redirect_url)
elif not self.request.user.has_usable_password():
if not self.request.user.has_usable_password():
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you alter this code? AFAIK, Django's AnonymousUser does not have a has_usable_password() method.

Copy link
Contributor Author

@SebCorbin SebCorbin Sep 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK the comment We end up here when ACCOUNT_LOGOUT_ON_PASSWORD_CHANGE = True is not valid (this is why I'm adding a test), which means :

  • if we end up on this view as anonymous, we should be redirected somewhere, LOGIN_URL maybe? And I would suggest doing this in dispatch not after the form is loaded
  • we're connected and if we submit this view and successfully change the password:
    • we have LOGOUT_ON_PASSWORD_CHANGE to True and should be logged out
    • LOGOUT_ON_PASSWORD_CHANGE is False and we just redirect to success_url

Copy link
Owner

@pennersr pennersr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes are sound. Though, the PasswordSetView() has similar logic, can you give it the same treatment?

@pennersr
Copy link
Owner

Refactored via 088a4f1

@pennersr pennersr closed this Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants