Opened 12 years ago

Closed 8 years ago

Last modified 6 years ago

#17209 closed Cleanup/optimization (fixed)

Dogfood class-based views in contrib.auth

Reported by: Stephen Burrows Owned by: Claude Paroz <claude@…>
Component: contrib.auth Version: dev
Severity: Normal Keywords: class-based views admin auth
Cc: Donald Stufft, seldon, andrewsmedina@…, Baptiste Mispelon, vlastimil.zima@…, marc.tamlyn@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Right now, the views provided by contrib.auth are not very extensible, since they are still functional. It would be great to add class-based versions which could eventually replace the functional versions. This is related to #17208; when this change is made, the admin login view should be tweaked to extend the new view.

Attachments (1)

patch.diff (24.4 KB ) - added by japrogramer 10 years ago.
this patch changes the auth view functions to CBV, maintaining backwards compatibility and tests passing ;)

Download all attachments as: .zip

Change History (47)

comment:1 by Preston Holmes, 12 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Donald Stufft, 12 years ago

Cc: Donald Stufft added

I'm taking a crack at this. I've started working on it in my branch on github: https://github.com/dstufft/django/tree/feature/contrib-auth-cbv

So far i've gotten the LoginView switched to a CBV with a shim to translate login -> LoginView and switching the auth specific kwarg names to match the more generic kwarg names in FormView.

comment:3 by seldon, 12 years ago

Cc: seldon added

comment:4 by Andrews Medina, 12 years ago

Cc: andrewsmedina@… added
Owner: changed from nobody to Andrews Medina

comment:5 by Baptiste Mispelon, 12 years ago

Cc: Baptiste Mispelon added

I've made some pogress on this: https://gist.github.com/1851113

It still needs tests and documentation but I'm hoping to work on it during the sprints at djangocon europe next month.

comment:6 by Andrews Medina, 12 years ago

bmispelon I can help you with that. You could add me to your fork on github?

in reply to:  6 comment:7 by Baptiste Mispelon, 12 years ago

Replying to andrews:

bmispelon I can help you with that. You could add me to your fork on github?

I made a proper fork of django's new github repo: https://github.com/bmispelon/django and committed my changes.

comment:8 by Baptiste Mispelon, 12 years ago

After two minor modifications, I've got all the tests passing on my fork.

There are still a few things I'd like to improve, mostly around the redirection logic in LoginView and LogoutView (it's still messy).

I've had a hard time wrapping my head around the flow of the function-based logout view. I'd appreciate it if someone could make sure I did actually replicate it.

I also have a few questions:

  • LogoutView: For now, it inherits from TemplateView. Does it make more sense to inherit from RedirectView (RedirectView has the nice advantage of dispatching POST, OPTIONS, DELETE, and PUT to GET)? Or maybe simply View?
  • PasswordResetConfirmView: as mentionned in the comment on the code, there is a potential backwards-compatibility issue. It should not be too hard to correct it but is it really a problem?
  • Backwards-compatible stubs: the approach I took to ensure backwards-compatibility was to create function based wrappers around the class-based views. Is this a good approach?
  • How do I approach writing tests for these new class-based views? I'm having a hard time determining what should be tested and how.

comment:9 by Baptiste Mispelon, 12 years ago

I've made some more improvements and reached a point where I'm pretty satisfied with the code.

I'm going to attempt writing some documentation now, using the work done by pydanny in the pull request #144 (it hasn't been accepted yet as I'm writing this).

comment:10 by anonymous, 12 years ago

Also check out #16197 which has been closed as a duplicate of this ticket.

comment:11 by Vlastimil Zíma, 12 years ago

Cc: vlastimil.zima@… added

comment:12 by Marc Tamlyn, 12 years ago

Cc: marc.tamlyn@… added
Patch needs improvement: set

I've tried bringing the patch up to the current master. Most of the tests pass and it seems to be ok, but there's something weird going on with the way that the wrapper shim works.

On discussion with andrewgodwin we've come up with the following changes:

1) Remove the wrapper shim, and convert all of the keyword arguments to kwargs. We can handle extra_context by updating the context on the TemplateResponse object returned from the CBV.

2) Tests: the new tests should be entirely equivalent to the old tests, but also separate. So a new set of urls linked to the cbv views, and retest all the expected behaviour. We should then be able to deprecate things easily when the time comes.

3) Docs/Release notes. The functional views documentation can be removed, and we can convert each part of the docs to now document the new, class based version.

comment:13 by Marc Tamlyn, 12 years ago

Ok, I've got all of the tests working again and updated the wrapping of the new views with the old ones.

The code is visible here: https://github.com/mjtamlyn/django/compare/auth-cbv

The last thing to add to the list above is integrating these changes with the rest of django. The auth views are used by the admin, flatpages and other parts of auth and these should be updated to use the new cbv approach.

comment:14 by Preston Holmes, 11 years ago

an interesting related bit to note here - is making the password reset process more flexible for custom users via CBV views:

http://pypi.python.org/pypi/django-password-reset

via

https://groups.google.com/forum/?fromgroups=#!topic/django-developers/NNnDVzoNQbU

comment:15 by Baptiste Mispelon, 11 years ago

I just pulled master back into my branch:
https://github.com/bmispelon/django/compare/auth-cbv

I thought the new swappable user model would break a lot of things but it turned out much easier than expected.

I've been looking into moving contrib.admin to use the new CBV but what I have so far is pretty ugly because the admin uses a lot of the old extra_context parameter, which requires CBV to create a get_context_data method.

As for flatpages, it only uses redirect_to_login which we didn't touch.

comment:16 by Claude Paroz, 11 years ago

It looks nice. It would be nice to rebase/merge the commits, write some documentation and generate a pull request so we can comment on it.

comment:17 by Rocky Meza, 11 years ago

I opened a pull request for this ticket https://github.com/django/django/pull/1239

This pull request is different from https://github.com/bmispelon/django/compare/auth-cbv in a couple ways:

  1. There is no wrapper shim. The views extract the old parameters from kwargs. This has the benefit of subclasses still supporting the old view invocation.
  2. It uses the DecoratorMixin pattern described in https://groups.google.com/d/msg/django-developers/jrfbenCJYYU/6aiH2sebnlAJ. This is controversial, yes, but it does mean a subclass will never lose its decorators should someone override dispatch.
  3. It is rebased on master.


I was going to write some documentation for the class-based views, but I was wondering if we should maintain (and keep documented) the function view kwarg parameters or if they should be truly deprecated. All access to those function view kwarg parameters is regulated through one piece of code, so it would be possible to deprecate them easily. I'm in favor of deprecating the function view kwargs, 1) because they are terribly inconsistent (post_reset_redirect vs. post_change_redirect) and 2) to embrace the established class-based views APIs.

comment:18 by Marc Tamlyn, 11 years ago

I'm not particularly fond of the approach you've taken here. I think we should not be trying to support some of the more obscure customisation points which existed historically. I agreed with some of the other core devs that the best way to proceed is to leave the old code as is, but with deprecation warnings, and create new views which are well designed to the new way of doing things, and provide a more consistent feeling API to the main GCBVs.

Other points:
If we're going to include the DecoratorMixinthen we should include it in a more logical place in the code base
Patch is completely missing tests - the tests for auth views are not complete as it is, so they need expanding to cover all the branches as well. Also they need to check all the deprecated views still work.
There are lots of minor style points - for example the use of resolve_url rather than reverse

comment:19 by Baptiste Mispelon, 11 years ago

Hi,

I worked with Marc on this issue during the sprints at Djangocon Europe last month (and last year too).
Admitedly, I haven't made much progress on this since, but I haven't given up and I still work on it from time to time.

As it's often the case, writing the code is the easier part. I think the code we have is pretty good now and I don't see much advantage in tweaking it further (except maybe the idea of using the DecoratorMixin, see later).

As Marc mentionned, we've more or less settled on the idea of creating plain CBV, without any backwards-compatibility layer.
We would then keep the old function-based views around, with a deprecation warning as per the deprecation policy.
This allows us to start on a relative clean slate and get rid of the cruft that's been added over the years.

With the code settled, what's preventing this ticket from moving further has more to do with things like tests, documentation, and other parts of django that need to be cleaned up.

For example, there are a few places in django that assume the presence of function-based views inside contrib/auth/views.py so those need to be fixed (I'm currently working on this).
There's also the issue of the admin which makes heavy use of the auth views and their configuration options. In particular, it uses extra_context which doesn't map well to class-based views.

One thing to note is that with this rewrite, django.contrib.auth would become the first app in django that ships with non-generic class-based views.
This means that we need to come up with a way of both documenting and testing these views.
This is not completely straighforward, especially considering that whatever we come up with would probably be regarded as some sort of canon on how to test/document non-generic class-based views.

Personally, I'd rather take some more time and do this properly (clean up the code, tests and doc) rather than cut corners and ship something still half-broken with backwards-compatibility cruft.

Finally, regarding the DecoratorMixin:
I like the idea but I wonder if shouldn't be in a separate ticket.
I think the feature makes sense on its own and with the more litmited scope that it has,
it would stand a better chance of getting commited separately.

Thanks.

comment:20 by Baptiste Mispelon, 11 years ago

I just pushed all the things I had left from the sprints onto https://github.com/bmispelon/django/compare/master...auth-cbv (I also merged the latest changes from master).

Here's what needs to be done next:

  • Move all the contrib.auth.test_views to a new test_legacy_views file.
  • Come up with a way of testing the class-based views (2 things need to be tested separately: the fact that the view itself works as intended, and its extension points).
  • Document the new class-based views.
  • Fix contrib.admin's usage of the auth views. As I mentionned before, the usage of the old extra_context parameter is what's causing problems here.

Out of these 4 points, only the first one is fairly easy (it's just some heavy copy/pasting and moving files around). The other 3 will be more tricky and I worry that the fixing of contrib.admin might unveil some more complicated issues.

by japrogramer, 10 years ago

Attachment: patch.diff added

this patch changes the auth view functions to CBV, maintaining backwards compatibility and tests passing ;)

comment:21 by japrogramer, 10 years ago

(above) just attached a file with my improvements on the auth views, they are now CBV and have function gateways to maintain backwards compatability but users can easily import the CBV and override functions to extend or modify functionality :D

comment:22 by japrogramer, 10 years ago

Has patch: set
Needs tests: set

comment:23 by japrogramer, 10 years ago

Needs tests: unset

comment:24 by Asif Saifuddin Auvi, 9 years ago

hi willing to work on this related tickets for gsoc this year. willing to assigning myself for now. will be analyzing the related tickets for a good proposal. looking forward to you guys kind suggestions :)

comment:25 by Asif Saifuddin Auvi, 9 years ago

Owner: changed from Andrews Medina to Asif Saifuddin Auvi
Status: newassigned

comment:26 by Asif Saifuddin Auvi, 9 years ago

starting work on this. mainly cleanly implement the patch of japrogramer.

comment:27 by Asif Saifuddin Auvi, 8 years ago

Needs documentation: set

comment:28 by Asif Saifuddin Auvi, 8 years ago

Targeting this for 1.10

comment:29 by Claude Paroz, 8 years ago

auvipy, still working on this?

in reply to:  29 comment:30 by Asif Saifuddin Auvi, 8 years ago

Replying to claudep:

auvipy, still working on this?

If you are willing to take it over for your personal sprint then OK but if not then I would be happy to start work on it. been busy with other projects.

comment:31 by Claude Paroz, 8 years ago

If you think you'll be able to work on it before the feature freeze (May 16th), then fine. If not, then simply deassign yourself.

Last edited 8 years ago by Tim Graham (previous) (diff)

comment:32 by Claude Paroz, 8 years ago

Needs documentation: unset
Patch needs improvement: unset

I added a patch for only login and logout in this PR. We might consider doing the password reset views later.

comment:33 by Philip James, 8 years ago

Patch needs improvement: set

I think the code looks good, and includes appropriate test changes. However, the patch needs to be rebased, and the references to 1.10 should be set to the next version.

comment:34 by Claude Paroz, 8 years ago

Patch needs improvement: unset

Patch rebased.

comment:35 by Philip James, 8 years ago

Triage Stage: AcceptedReady for checkin

I think the code looks good, tests pass, and there is updated documentation. Marking "Ready For Checkin", happy to be corrected.

comment:36 by Tim Graham, 8 years ago

Triage Stage: Ready for checkinAccepted

I see a few things for improvement. Will review it soon.

comment:37 by Tim Graham, 8 years ago

Owner: Asif Saifuddin Auvi removed
Status: assignednew
Triage Stage: AcceptedReady for checkin

comment:38 by Claude Paroz <claude@…>, 8 years ago

In 7896349:

Refs #17209 -- Added LoginView and LogoutView class-based views

Thanks Tim Graham for the review.

comment:39 by Claude Paroz, 8 years ago

Has patch: unset
Triage Stage: Ready for checkinAccepted

Now to the password change/reset views...

comment:40 by Claude Paroz, 8 years ago

Has patch: set

PR for the password-related views.

comment:41 by Tim Graham, 8 years ago

Triage Stage: AcceptedReady for checkin

comment:42 by Claude Paroz <claude@…>, 8 years ago

Owner: set to Claude Paroz <claude@…>
Resolution: fixed
Status: newclosed

In 255fb992:

Fixed #17209 -- Added password reset/change class-based views

Thanks Tim Graham for the review.

comment:43 by Tim Graham <timograham@…>, 7 years ago

In 51eaff6d:

Refs #17209 -- Fixed token verification for PasswordResetConfirmView POST requests.

comment:44 by Markus Holtermann <info@…>, 7 years ago

In b9b35f9e:

Fixed #27840 -- Fixed KeyError in PasswordResetConfirmView.form_valid().

When a user is already logged in when submitting the password and
password confirmation to reset a password, a KeyError occurred while
removing the reset session token from the session.

Refs #17209

Thanks Quentin Marlats for the report and Florian Apolloner and Tim
Graham for the review.

comment:45 by Markus Holtermann <info@…>, 7 years ago

In f5ff5be2:

[1.11.x] Fixed #27840 -- Fixed KeyError in PasswordResetConfirmView.form_valid().

When a user is already logged in when submitting the password and
password confirmation to reset a password, a KeyError occurred while
removing the reset session token from the session.

Refs #17209

Thanks Quentin Marlats for the report and Florian Apolloner and Tim
Graham for the review.

comment:46 by Tim Graham <timograham@…>, 6 years ago

In 4f313e2:

Refs #17209 -- Removed login/logout and password reset/change function-based views.

Per deprecation timeline.

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