Customizable LoginRequiredMixin #32

Closed
wants to merge 3 commits into
from

Projects

None yet

4 participants

@foxbunny

The LoginRequiredMixin was lacking the ability to customize login_url, redirect
field, or redirect path per view because it was using a decorator. Since these
things need to change based on instance, we are using the redirect_to_login
view directly instead of the login_required decorator, and we are adding
methods and properties which allow us to micro-manage the view instances.

Branko Vukelic Customizable LoginRequiredMixin
The LoginRequiredMixin was lacking the ability to customize login_url, redirect
field, or redirect path per view because it was using a decorator. Since these
things need to change based on _instance_, we are using the redirect_to_login
view directly instead of the login_required decorator, and we are adding
methods and properties which allow us to micro-manage the view instances.
0713986
@rafales
Contributor
rafales commented Feb 22, 2013

I agree that LoginRequiredMixin should allow for more customization, but all access mixins (LoginRequiredMixin, StaffuserRequiredMixin etc.) should have consistent interface.

@rafales rafales and 1 other commented on an outdated diff Feb 22, 2013
braces/views.py
def dispatch(self, request, *args, **kwargs):
- return super(LoginRequiredMixin, self).dispatch(request,
- *args, **kwargs)
+ if not request.user.is_authenticated():
+ return redirect_to_login(
+ self.get_reidrect_path(request),
@rafales
rafales Feb 22, 2013 Contributor

Looks like typo to me (reidirect -> redirect).

@foxbunny

FTR, I don't actually use braces all that much, because in most cases it lacked the customization interface I needed. This is just a quick copy/edit/paste from a piece of code I usually use myself. So consider this a draft of what is possible with Django rather than a proper pull request.

Branko Vukelic added some commits Feb 27, 2013
Branko Vukelic Fixed typo
Thanks to @rafales for catching it
bf410db
Branko Vukelic Merge branch 'master' into custom_login_required 5449f25
@chrisjones-brack3t
Member

I'm actually working on a rewrite of this mixin right now. LoginRequiredMixin

@chrisjones-brack3t
Member

Closing this pull request. It has been addressed in the upcoming 1.0 release.

@foxbunny

@chrisjones-brack3t Sorry about that, didn't know you had it in works. Anyway, is there a reason why you don't use methods to configure these things? It's quite unlike Django's built-in CVBs and it's also not very flexible that way (e.g., cannot use reverse, or cannot have more than one login URL, etc).

@kennethlove
Member

@foxbunny what we currently do is following the pattern Django set up in their login_required decorator and how our other access-related mixins work. It's likely worth changing them to provide those methods but we're not planning to do that for this 1.0 release. If you'd like to get them in there, we welcome a patch (but it would need to address all the access-related mixins, not just LoginRequiredMixin).

@foxbunny

Sure thing.

@chrisjones-brack3t
Member

We went back and added override-able get_login_url and get_redirect_field_name methods to all access based mixins.

@foxbunny
foxbunny commented Mar 2, 2013

👍

@foxbunny foxbunny deleted the foxbunny:custom_login_required branch Mar 2, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment