Dogfood class-based views in contrib.admin

[Stephen Burrows]

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.

[Julien Phalip]

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.

[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.

[zbyte64]

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.

[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.

[Julien Phalip]

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.

[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.

[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?

[bruno@…]

Anyone still working on this ?

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