-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Put view under login #6193
Put view under login #6193
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The title of this PR needs some work -- it doesn't explain what view or what this PR is doing :)
@@ -211,7 +210,7 @@ def account_advertising(request): | |||
) | |||
|
|||
|
|||
class TokenMixin: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the actual bugfix here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, otherwise an anonymous user could hit that page, it was returning 500. There was a discussion on slack, sorry for the lack of context.
Co-Authored-By: Manuel Kaufmann <[email protected]>
…org into put-page-under-login
Just for clarification, this PR fix this problem https://readthedocs.org/accounts/tokens/ (enter without being loged in) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good refactor 👍
Also, we were implementing our own LoginRequiredMixin, but we already have this on django.