[Django] #17208: Dogfood class-based views in contrib.admin
Django
--------------------------------------+------------------------------------
Reporter: melinath | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: SVN
Severity: Normal | Keywords: class-based views
Triage Stage: Unreviewed | admin
Easy pickings: 0 | Has patch: 0
| UI/UX: 0
--------------------------------------+------------------------------------
This is something that I believe was discussed at DjangoCon, as well as in
the django-dev mailing list and on IRC. It would be good to do a) on
principle, b) to make the admin views easier to extend, and c) to
potentially find and correct issues in the generic class-based views.
Naturally, any solution would need to be backwards-compatible.
--
Ticket URL: <https://code.djangoproject.com/ticket/17208>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: melinath | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: SVN
Component: contrib.admin | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: class-based views | Needs documentation: 0
admin | Patch needs improvement: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by julien):
* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
This has been discussed at least here recently:
http://groups.google.com/group/django-
developers/browse_thread/thread/54f51dbf63d242d1/
Admin views are already quite extensively tested, so it should be fairly
easy to track any backwards-incompatibility issues. My only concern at
this stage is that, although admin views would become more extensible,
there's also a risk that they'd become harder to understand and
manipulate. However, I think this is well worth trying and see where that
leads us.
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:1>
Django
Reporter: melinath | Owner: nobody
Cleanup/optimization | Version: SVN
Component: contrib.admin | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: class-based views | Needs documentation: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: seldon (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:2>
Django
Reporter: melinath | Owner: nobody
Cleanup/optimization | Version: SVN
Component: contrib.admin | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: class-based views | Needs documentation: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by julien):
I've started (experimental) work on this, so I'm assigning to me. That
shouldn't stop anyone from looking into this, though. I'll try to post an
update soon.
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:3>
Django
Reporter: melinath | Owner: julien
Cleanup/optimization | Version: SVN
Component: contrib.admin | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: class-based views | Needs documentation: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* owner: nobody => julien
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:4>
Django
Reporter: melinath | Owner: julien
Type: | Status: new
Severity: Normal | Triage Stage: Accepted
Keywords: class-based views | Needs documentation: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: zbyte64 (added)
Comment:
Using class based views it should be possible for the admin to allow for
3rd party libraries to provide other data store interfaces. I am working
on a project that builds on top of a re-factored class-based admin views
for document databases: https://github.com/zbyte64/django-
dockit/blob/master/dockit/admin/documentadmin.py
If there is an interest in discussing api endpoints to help with such
efforts let me know.
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:5>
Django
Reporter: melinath | Owner: julien
Type: | Status: new
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: class-based views | Needs documentation: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by julien):
I'm nearly done with the patch converting admin views to CBVs, so far
fully backwards compatible. I've mostly just got the changelist view left
to wrap up. Sorry I've got busy lately and haven't got around to getting
the patch ready. This is one of my priorities and I'll hopefully get to it
soon.
Besides that, I agree that CBVs will open the door to many opportunities
like the one you suggest. I'm also hopeful it'll make it easier to turn
the admin into a RESTful API.
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:6>
Django
Reporter: melinath | Owner: julien
Type: | Status: new
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution:
Severity: Normal | Triage Stage: Accepted
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_docs: 0 => 1
* has_patch: 0 => 1
Comment:
So, I have rewritten the admin views into CBVs. The code can be viewed
here: https://github.com/jphalip/django/compare/master...features%2Fadmin-
cbv
This is only the first pass, the main aim as this stage being to remain
completely backwards compatible. Thus, the whole test suite currently
passes unchanged, except for 2 failures which are apparently due to some
generic CBVs executing one more database query than needed — this could be
somewhat related to #18354.
{{{
======================================================================
FAIL: test_group_permission_performance
(regressiontests.admin_views.tests.GroupAdminTest)
----------------------------------------------------------------------
Traceback (most recent call last):
File
"/Users/julien/Documents/Development/eclipse/workspace/DjangoCore/django/tests/regressiontests/admin_views/tests.py",
line 3314, in test_group_permission_performance
self.assertEqual(response.status_code, 200)
File "/Users/julien/.virtualenvs/djangotests2.6/lib/python2.6/site-
packages/django/test/testcases.py", line 273, in __exit__
executed, self.num
AssertionError: 7 != 6 : 7 queries executed, 6 expected
======================================================================
FAIL: test_user_permission_performance
(regressiontests.admin_views.tests.UserAdminTest)
----------------------------------------------------------------------
Traceback (most recent call last):
File
"/Users/julien/Documents/Development/eclipse/workspace/DjangoCore/django/tests/regressiontests/admin_views/tests.py",
line 3276, in test_user_permission_performance
self.assertEqual(response.status_code, 200)
File "/Users/julien/.virtualenvs/djangotests2.6/lib/python2.6/site-
packages/django/test/testcases.py", line 273, in __exit__
executed, self.num
AssertionError: 9 != 8 : 9 queries executed, 8 expected
}}}
The immediate benefit is that we're getting more hooks, as through this
refactoring process some of the views' code has been broken into smaller
pieces.
Some opportunities that can now be considered are:
- Add even more hooks, by breaking down some of the remaining long chunks
of code into smaller methods, where it makes sense.
- Finally get rid of the `ChangeList` class, which is a private API and
has always been a PITA to work with. It could potentially be merged into
the change list CBV.
- Write more generic views for handling some use cases that are needed in
the admin and that could potentially be needed in other contexts (e.g.
inline formsets: #16256).
- As part of future work, we could provide a RESTful API for the admin so
it can be manipulated via Ajax requests.
So, again, I see this work as the initial step towards making the admin
more flexible and customizable. That said, I'm still not entirely sold on
the approach. Using CBVs does provide a lot more flexibility, but it comes
at the cost of increasing the code's complexity. While the admin is
arguably the Django app that needs the most flexibility in the world, we
still want to keep things manageable and not too hard to maintain.
As far as backwards compatibility is concerned, we also need to decide how
compatible we want to remain in 1.5 and beyond? If breaking compatibility
is necessary at some stage to ensure that we start off with a manageable
and maintainable codebase, then I'd personally be ok as long as the
upgrade path is made simple.
I'd love to get feedback on the work done so far and hear some ideas on
the next steps. Since this is a pretty massive effort which would impact a
lot of people, I'll also probably bring this topic up on the mailing list.
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:7>
Django
Reporter: melinath | Owner: julien
Type: | Status: new
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: class-based views | Needs documentation: 1
admin | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by rasca):
I disagree that the code is more complex now. At least for me, being used
to the generic CBV I think the code is much clearer and easy to understand
than before, being split in familiar methods.
Said that, I think there's much room for improvement. For example joining
all the formsets construction and prefix checking:
https://github.com/rasca/django/commit/3c1c28329ff2310191aa7ba29acfadc020771516
I don't know why it won't let me pull request to your fork (it lets me
pull request any other fork). Please cherry pick...
I would love to get rid of the `ChangeList` class too. It will be hard
work tough, and might be considered BC.
As you said, much of the code could live in a generic CBV from #16256. One
of the things we need to decide in that ticket is for example if we are
aiming to use those views here or not. I don't know how much of those can
be used without breaking BC or without making the API based on not
breaking the admin BC. If we decide to break BC (for example in a new-new-
admin) there's many, many things that can be improved and dogfood other
tickets as well..
I agree with julien (talked with him in IRC) that the best thing is to
tackle the problems separately and then see if we can merge them at a
latter step.
Another thought on future work (maybe 2.0?) is to get rid of the
ModelAdmin and just subclass the admin views directly, let's say we have
#16213 with some sugar on top for example.
As for the immediate work, I think we really need to look closer at the
failing tests, it seems like a deeper problem in the generic CBV than in
this refactor. Otherwise this looks good for me.
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:8>
Django
Reporter: melinath | Owner: julien
Type: | Status: new
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: class-based views | Needs documentation: 1
admin | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: rasca (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:9>
Django
Reporter: melinath | Owner: julien
Type: | Status: new
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: class-based views | Needs documentation: 1
admin | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by julien):
Your code for `FormSetsMixin` looks good and clean, thank you. It makes
sense to merge it if that makes further refactoring easier. BTW, have you
figured out the issue with the github pull requests?
I propose we consider an accelerated deprecation path for the `ChangeList`
class: give warning in 1.5 and remove in 1.6. At least let's see where
that takes us and see if that class doesn't become too much of a burden.
I'm also wondering about some methods which currently are in `ModelAdmin`
and which could potentially be moved over to the CBVs:
`response_change()`, `response_add()`, `response_action()`,
`construct_change_message()`, `get_changelist_form()`,
`get_changelist_formset()`, `get_object()`, `get_form()`,
`get_fieldsets()`, `get_inline_instances()`, `formfield_for_*()`. Is that
something you had considered in previous work?
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:10>
Django
Reporter: melinath | Owner: julien
Type: | Status: new
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: class-based views | Needs documentation: 1
admin | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: net147 (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:11>
Django
Reporter: melinath | Owner: julien
Type: | Status: new
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: class-based views | Needs documentation: 1
admin | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: vlastimil.zima@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:12>
Django
Reporter: melinath | Owner: julien
Type: | Status: new
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: class-based views | Needs documentation: 1
admin | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: travis@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:13>
Django
Reporter: melinath | Owner:
Type: | Status: new
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: class-based views | Needs documentation: 1
admin | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* owner: julien =>
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:14>
Django
Reporter: melinath | Owner:
Type: | Status: new
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: class-based views | Needs documentation: 1
admin | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: amirouche.boubekki@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:15>
Django
Reporter: melinath | Owner:
Type: | Status: new
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: class-based views | Needs documentation: 1
admin | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: bruno@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:16>
Django
Reporter: melinath | Owner:
Type: | Status: new
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: class-based views | Needs documentation: 1
admin | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by bruno@…):
Anyone still working on this ?
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:17>
Django
Reporter: melinath | Owner:
Type: | Status: new
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: class-based views | Needs documentation: 1
admin | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: robinchew@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:18>
Django
Reporter: melinath | Owner:
Type: | Status: new
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: class-based views | Needs documentation: 1
admin | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: mjumbewu@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:19>
Django
Reporter: melinath | Owner: auvipy
Type: | Status: assigned
Cleanup/optimization |
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: class-based views | Triage Stage: Accepted
admin |
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by auvipy):
* owner: => auvipy
* status: new => assigned
Comment:
I'm gonna work on this and propose my proposal for google summer of code
this year.
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:20>
Django
Reporter: melinath | Owner: auvipy
Type: | Status: assigned
Cleanup/optimization |
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: class-based views | Triage Stage: Accepted
admin |
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by auvipy):
Hi Julian,
I've decided to work on dog fooding django auth and admin. I have seen the
old patches and going to implement them freshly. any other suggestions you
have for the changes?
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:21>
Django
Reporter: melinath | Owner:
Type: | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: class-based views | Triage Stage: Accepted
admin |
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: auvipy =>
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:22>
Django
Reporter: melinath | Owner:
Type: | Status: new
Cleanup/optimization |
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: class-based views | Triage Stage: Accepted
admin |
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: yoongkang.lim@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:23>
Django
Reporter: melinath | Owner: yoongkang
Type: | Status: assigned
Cleanup/optimization |
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: class-based views | Triage Stage: Accepted
admin |
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by yoongkang):
* owner: => yoongkang
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:24>
Django
Reporter: melinath | Owner: yoongkang
Type: | Status: assigned
Cleanup/optimization |
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: class-based views | Triage Stage: Accepted
admin |
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: zachborboa@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:25>
Django
-------------------------------------+-------------------------------------
Reporter: melinath | Owner: yoongkang
Type: | Status: assigned
Cleanup/optimization |
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: class-based views | Triage Stage: Accepted
admin |
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
A [https://github.com/django/django/pull/6045 PR] by yoongkang took an
alternate approach but I didn't see if there was a reason to reject the
approach used in earlier versions of the patch that subclassed more
specific class-based views like `UpdateView`, `CreateView`, `DeleteView`?
I have mixed feelings about this conversion in general. I guess it makes
the views easier to customize, but on the other hand, I think it makes the
code more difficult to follow (however I haven't used CBGVs a lot, so it
might just be me).
--
Ticket URL: <https://code.djangoproject.com/ticket/17208#comment:26>