Opened 10 years ago

Closed 10 years ago

#22006 closed Cleanup/optimization (fixed)

DRY login_required method should be documented

Reported by: django@… Owned by: mockforest
Component: Documentation Version: 1.6
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

See https://code.djangoproject.com/ticket/16626 for the discussion.

Currently, the docs suggest decorating each class separately, they should also suggest the mixin as recommended in the ticket above, for setting login_required (or others) on multiple classes.

Note: Creating a ProtectedView superclass and subclassing doesn't work for the reason as specified in the Note in the current docs:
https://docs.djangoproject.com/en/dev/topics/class-based-views/intro/#decorating-the-class

Attachments (3)

ticket-22006.diff (811 bytes ) - added by chrismarceloph 10 years ago.
ticket-22006-fixed.diff (852 bytes ) - added by mockforest 10 years ago.
added @classmethod
ticket-22006.2.diff (856 bytes ) - added by mockforest 10 years ago.
lines cut to max length of 80

Download all attachments as: .zip

Change History (17)

comment:1 by Tim Graham, 10 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

comment:2 by avendael, 10 years ago

Owner: changed from nobody to anonymous
Status: newassigned

by chrismarceloph, 10 years ago

Attachment: ticket-22006.diff added

comment:3 by chrismarceloph@…, 10 years ago

Has patch: set

Submitted patch. (Sorry did not claim it first, first-timer here.)

comment:4 by avendael, 10 years ago

Owner: changed from anonymous to avendael

comment:5 by avendael, 10 years ago

Owner: avendael removed
Status: assignednew

comment:6 by avendael, 10 years ago

No worries :)

comment:7 by Nikolajus Krauklis, 10 years ago

@chrismarceloph in your patch method as_view needs to be decorated with @classmethod
see https://code.djangoproject.com/ticket/16626 last comment

i would suggest to put that example into https://docs.djangoproject.com/en/dev/topics/class-based-views/intro/#using-mixins and then from https://docs.djangoproject.com/en/dev/topics/auth/default/#applying-permissions-to-generic-views refer to that example which is written in mixins sections instead of decorating dispatch

#nlsprint14

comment:8 by Nikolajus Krauklis, 10 years ago

and if you are planning to finish this ticket, please register here and assign it to yourself

comment:9 by mockforest, 10 years ago

Owner: set to mockforest
Status: newassigned

@dzhibas: Thanks! I've registered and am planning to finish this ticket.

If I understand it correctly, you're suggesting I move the example to https://docs.djangoproject.com/en/dev/topics/class-based-views/intro/#using-mixins and refer to it from https://docs.djangoproject.com/en/dev/topics/auth/default/#applying-permissions-to-generic-views? Sounds good; I think that section needs an example too.

comment:10 by Tim Graham, 10 years ago

Patch needs improvement: set

comment:11 by django@…, 10 years ago

I think he just means to say you need to add @classmethod on line 12 of your diff.

I'd probably also clarify the following;

by mockforest, 10 years ago

Attachment: ticket-22006-fixed.diff added

added @classmethod

by mockforest, 10 years ago

Attachment: ticket-22006.2.diff added

lines cut to max length of 80

comment:12 by mockforest, 10 years ago

I didn't see the first line of @dzhibas's comment, sorry. Patch fixed.

And now I'm not so sure about the suggestions. I feel like the example belongs to three separate sections:

What would be the best way to do this?

Last edited 10 years ago by mockforest (previous) (diff)

comment:13 by mockforest, 10 years ago

Patch needs improvement: unset

comment:14 by Tim Graham, 10 years ago

Resolution: fixed
Status: assignedclosed

In 584044566414d3f4595337225aa23b52a089a680:

Fixed #22006 -- Documented how to write a login_required mixin for CBVs.

Thanks django at patjack.co.uk for the suggestion and mockforest
for the draft patch.

Note: See TracTickets for help on using tickets.
Back to Top