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

New Mixin: AnonymousUserOnlyMixin #89

Closed
wants to merge 4 commits into from

Conversation

epicbagel
Copy link
Contributor

New mixin redirect authenticated users to a specified URL. This can be useful for preventing authenticated users accessing signup pages or redirecting them to their dashboard if they arrive on the home page.

Jon Bolt added 3 commits November 1, 2013 09:56
View mixin which redirects to a specified URL if authenticated. Can be
useful if you wanted to prevent authenticated users from accessing
signup pages etc.
@yetty
Copy link

yetty commented Dec 1, 2013

+1

authenticated_redirect_url = "/dashboard/"
...
"""
authenticated_redirect_url = None # Default the authenticated redirect url to none
Copy link

Choose a reason for hiding this comment

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

Maybe it would be useful to use settings.LOGIN_REDIRECT_URL as default value instead of None.

Copy link
Member

Choose a reason for hiding this comment

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

@yetty: But it's for anonymous users, why would you redirect logged-in users to the login page?

Copy link

Choose a reason for hiding this comment

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

@kennethlove Look at https://docs.djangoproject.com/en/dev/ref/settings/#login-redirect-url . settings.LOGIN_REDIRECT_URL is NOT login page, but the page after successful log in.

Copy link
Member

Choose a reason for hiding this comment

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

@yetty OK, but even then, they're already authenticated and shouldn't be, for this mixin. Seems odd to redirect them somewhere other than a 'log out to see this page' view.

Ultimately, it's the contributor's job to tell us if we're thinking correctly about it :) So, @epicbagel, please shed some light on this.

Copy link

Choose a reason for hiding this comment

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

@kennethlove My most common usage for this mixin is for login and registration view. If users are already logged in I would suppose to redirect them to the same page to which they are redirected after log in / registration.

I would appreciate some default value - not raising exception if authenticated_redirect_url is missing. Imho even E403 would be better than exception.

Not really important for me, but I think it would be nice to be consistent with Django. Just an idea, how to improve it :) Thanks anyway for merging this mixin! 👍

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that use-case makes sense. And that use of the setting makes sense, too. Probably want to do it how the AccessMixin does, though, so it's overridable in your class. And, like the comment below, it should probably be renamed to AnonymousRequiredMixin.

Anyway, my plan is to put this into 1.4 in a month or two. Good work everyone.

@ergusto
Copy link

ergusto commented Dec 26, 2013

+1 Although I'd suggest renaming it to AnonymousRequiredMixin to keep in line with how the other mixins are named.

Tests for AnonymousUserOnlyMixin.
"""
view_class = AnonymousUserOnlyView
view_url = '/unauthanticated_view/'
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to see a test that uses reverse_lazy here, too, to verify that it works with this mixin.

Copy link
Member

Choose a reason for hiding this comment

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

also, authEnticated

@kennethlove
Copy link
Member

Address the spelling issues and needed extra test cases and this'll go into 1.4.

Thanks!

@epicbagel
Copy link
Contributor Author

Thanks everyone for the feedback.

@yetty, @kennethlove the mixin is now using LOGIN_REDIRECT_URL from settings.py. This makes sense to me as it's the default place you redirect authenticated users to. Can always be overwritten if required, or course.

@ergusto - I've also renamed to AnonymousRequiredMixin.

Also fixed the typos, added a test case for reverse_lazy. Look forward to seeing it in 1.4 :)

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.

None yet

5 participants