Opened 5 years ago

Last modified 7 weeks ago

#17208 assigned Cleanup/optimization

Dogfood class-based views in contrib.admin

Reported by: melinath Owned by: Yoong Kang Lim
Component: contrib.admin Version: master
Severity: Normal Keywords: class-based views admin
Cc: seldon, zbyte64, rasca, net147, vlastimil.zima@…, travis@…, amirouche.boubekki@…, bruno@…, robinchew@…, mjumbewu@…, yoongkang.lim@…, zachborboa@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (24)

comment:1 Changed 5 years ago by Julien Phalip

Triage Stage: UnreviewedAccepted

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.

comment:2 Changed 5 years ago by seldon

Cc: seldon added

comment:3 Changed 5 years ago by Julien Phalip

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.

comment:4 Changed 5 years ago by Julien Phalip

Owner: changed from nobody to Julien Phalip

comment:5 Changed 5 years ago by zbyte64

Cc: zbyte64 added

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.

comment:6 Changed 5 years ago by Julien Phalip

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.

comment:7 Changed 5 years ago by Julien Phalip

Has patch: set
Needs documentation: set

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.

comment:8 Changed 5 years ago 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.

comment:9 Changed 5 years ago by rasca

Cc: rasca added

comment:10 Changed 5 years ago by Julien Phalip

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?

comment:11 Changed 4 years ago by net147

Cc: net147 added

comment:12 Changed 4 years ago by Vlastimil Zíma

Cc: vlastimil.zima@… added

comment:13 Changed 4 years ago by Travis Swicegood

Cc: travis@… added

comment:14 Changed 4 years ago by Julien Phalip

Owner: Julien Phalip deleted

comment:15 Changed 4 years ago by Amirouche

Cc: amirouche.boubekki@… added

comment:16 Changed 3 years ago by bruno@…

Cc: bruno@… added

comment:17 Changed 3 years ago by bruno@…

Anyone still working on this ?

comment:18 Changed 3 years ago by Robin

Cc: robinchew@… added

comment:19 Changed 3 years ago by mjumbewu@…

Cc: mjumbewu@… added

comment:22 Changed 11 months ago by Asif Saifuddin Auvi

comment:23 Changed 11 months ago by Yoong Kang Lim

Cc: yoongkang.lim@… added

comment:24 Changed 11 months ago by Yoong Kang Lim

Owner: set to Yoong Kang Lim
Status: newassigned

comment:25 Changed 10 months ago by Zach Borboa

Cc: zachborboa@… added

comment:26 Changed 3 months ago by Tim Graham

A 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).

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