Opened 4 years ago

Last modified 2 months ago

#17208 assigned Cleanup/optimization

Dogfood class-based views in contrib.admin

Reported by: melinath Owned by: auvipy
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@… 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 (21)

comment:1 Changed 4 years ago by julien

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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 3 years ago by seldon

  • Cc seldon added

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

comment:4 Changed 3 years ago by julien

  • Owner changed from nobody to julien

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

comment:7 Changed 3 years ago by julien

  • 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 3 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 3 years ago by rasca

  • Cc rasca added

comment:10 follow-up: Changed 3 years ago 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?

comment:11 Changed 3 years ago by net147

  • Cc net147 added

comment:12 Changed 3 years ago by vzima

  • Cc vlastimil.zima@… added

comment:13 Changed 3 years ago by tswicegood

  • Cc travis@… added

comment:14 Changed 3 years ago by julien

  • Owner julien deleted

comment:15 Changed 3 years ago by abki

  • Cc amirouche.boubekki@… added

comment:16 Changed 19 months ago by bruno@…

  • Cc bruno@… added

comment:17 Changed 19 months ago by bruno@…

Anyone still working on this ?

comment:18 Changed 19 months ago by robin

  • Cc robinchew@… added

comment:19 Changed 17 months ago by mjumbewu@…

  • Cc mjumbewu@… added

comment:20 Changed 4 months ago by auvipy

  • Owner set to auvipy
  • Status changed from new to assigned

I'm gonna work on this and propose my proposal for google summer of code this year.

comment:21 in reply to: ↑ 10 Changed 2 months ago 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?

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