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

Fixed #27891 -- Added PasswordResetConfirmView.post_reset_login_backend attribute. #8134

Merged
merged 1 commit into from
Mar 8, 2017

Conversation

camilonova
Copy link
Contributor

@camilonova camilonova commented Mar 2, 2017

@timgraham timgraham changed the title Fixed #27891 -- Added post_reset_login_backend argument to PasswordRe… Fixed #27891 -- Added PasswordResetConfirmView.post_reset_login_backend attribute. Mar 6, 2017
@timgraham
Copy link
Member

I pushed a commit with some cosmetic edits. The test needs some enhancement so that it works as a regression test if auth_login(self.request, user, self.post_reset_login_backend) is changed back to auth_login(self.request, user),

@timgraham
Copy link
Member

Is that really a regression test? I think that would pass before and after this patch. I think the regression test would be to use multiple auth backends in the original test case and verify that the one selected in the URLconf is used.

@camilonova
Copy link
Contributor Author

The test should not pass before this patch. That was exactly the error I was getting before working on this bug.

I'll make right now another test as you suggest.

@timgraham
Copy link
Member

test_confirm_login_post_reset_custom_backend_regresion doesn't use a custom backend, so that error happens before and after the patch. By the way, you can rebase and squash commits when updating. Thanks.

@camilonova
Copy link
Contributor Author

Tim, you were right, I just removed the old test and updated the old one to check the backend used.

Please tell me if you think is fine and I'll rebase and squash.

Thanks.

@timgraham
Copy link
Member

Yes, that's what I expected. I think it would be slightly clearer if the urlpattern used the second auth backend (AllowAllUsersModelBackend) instead.

@camilonova
Copy link
Contributor Author

Awesome. I just updated it. How about now?

@@ -336,7 +336,9 @@ def test_confirm_login_post_reset(self):
]
)
def test_confirm_login_post_reset_custom_backend(self):
backend = 'django.contrib.auth.backends.ModelBackend'
# Please note this backend is set at the url.
Copy link
Member

Choose a reason for hiding this comment

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

# This backend is specified in the url().

backend = 'django.contrib.auth.backends.ModelBackend'
# Please note this backend is set at the url.
backend = 'django.contrib.auth.backends.AllowAllUsersModelBackend'

Copy link
Member

Choose a reason for hiding this comment

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

skip the blank line

@camilonova
Copy link
Contributor Author

Looks like its ready

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