Simplify adding new views to ModelAdmin

[JamesRamm]

When using ModelAdmin, if you wish to add a new view alongside inspect/create/edit/delete, you need to do the following:

• Inherit from ButtonHelper and add a new function for your view link button, which follows the same template as cancel_button()/inspect_button() etc..

• Override get_buttons_for_obj to add your custom button...which basically involves reimplementing it exactly the same then tagging your button on to the end of the btns array. (we can call super and tag it on the end of the returned array, although this still involve repeating some of the logic within the base function)

• In your custom ModelAdmin child class, override get_admin_urls_for_registration to add the url for your new view onto the return urls array.

• In the same class, implement a new function to return your view (i.e. it should do the same as ModelAdmin.inspect_view but return your own view class).

• Implement the actual View class by e.g. inheriting from InspectView

This is quite a lot of boilerplate. Similarly if you wish to remove a view/button (e.g. preventing models from being deleted), you have to reimplement a function to do exactly the same thing but without the button you wish to remove.

I wonder if there is way to easily, dynamically register views/buttons in ModelAdmin?

I am not sure how this could work yet, but perhaps a class attribute which will take an array of tuples with the view class and button name? We would then need the necessary logic to register these views.
Allowing new views to be registered like this would not only make it more generic, but allow a lot of the repeated boilerplate within the base classes to be condensed (e.g. functions like index_view and create_view on ModelAdmin, or create_button, index_button etc on ButtonHelper which share much of the same logic)

I will happily do the work to this effect, but I think it needs some discussion and consensus before beginning.

Another interesting possibility would be to allow buttons to be registered which are not attached to a view ..this could be achieved by simply setting the url parameter to '', but it would be useful to be able to specify the html id of the created <a> element.
(e.g. to allow javascript actions etc...)

• Wagtail version: v1.7

[ababic]

@JamesRamm thanks for posting. Some really good points here! I've faced a similar frustrations myself (particularly with ButtonHelper), and would love to help refactor things to make things easier to extend. Not having attributes or methods on the ModelAdmin class to add a layer of control to ButtonHelper seems like a huge missed opportunity (my bad). I did propose some changes a while ago (#2758), but I was waiting for at least some feedback before starting work, as these things are kind of critical to a developers experience of working with Wagtail, and so are important to get right. I know it's far from the fully generic approach you hinted at above, but would you agree that is a step in the right direction?

In regards to what you said here:

Similarly if you wish to remove a view/button (e.g. preventing models from being deleted), you have to reimplement a function to do exactly the same thing but without the button you wish to remove.

Hiding features by editing the ButtonHelper would never be an approach I'd recommend, because whether the ButtonHelper shows the button or not, the URLs for those actions (and the views to perform them) are still registered, and likely still accessible by going to the URL directly. The recommended approach would be to simply not grant users the relevant permissions required for those actions (ButtonHelper only shows buttons for actions that the current user has permission to perform)

[ababic]

I am not sure how this could work yet, but perhaps a class attribute which will take an array of tuples with the view class and button name? We would then need the necessary logic to register these views.

I think we can safely split actions into 4 groups:

1. Those that aren't related to a specific instance, can have a simple URL pattern that doesn't contain any params, and the view only really needs to take a HttpRequest instance. e.g. 'index' and (in a most cases) 'create'.
2. Those related to a specific instance, and the URL just needs to contain an identifier to specify which instance in involved. e.g. 'edit', 'inspect', 'delete', 'publish', 'unpublish'.

And less common:

3. Those that aren't related to a specific instance but need to accept one or more parameters in their URL pattern that need to be passed to the view. e.g. a 'create' view for a tree-based model, where you need to pass additional information to specify where in the tree the new instance needs to be added.
4. Those related to a specific instance, and the URL needs to contain an identifier, plus one or more additional params that need to be passed to the view. e.g. a 'move' view for a tree-based model that takes a primary key, plus other arguments to specify where that instance should be moved to.

So long as a more generic solution wouldn't lock out support for the last two, I think it would be worthwhile pursuing this. Even if the end-result wasn't quite as simple as defining a few tuples, cutting out some of the repetition throughout, and reducing the number of manual steps for adding custom views would definitely be positive.

[ababic]

@JamesRamm what would you think to introducing a class similar to the one below for defining all of the things related to a specific action? I think most things could be introspected from that (urls, button stuff, permission checks etc). And if it had URL-related methods that could be overridden for types 3 and 4 (above), that should be pretty much everything covered, right?

ModelAdminAction(
    handle='edit',
    instance_specific=True,
    button_text='Edit',
    button_classnames=['edit'],
    button_extra_attributes={}: 
    view: EditView,
    permissions_required: ['can_edit'],
)

[ababic]

Put together a quick proof of concept for this class here:

https://github.com/rkhleics/wagtail/blob/modeladminactions/wagtail/contrib/modeladmin/actions.py

I'm thinking that the actions themselves would be defined as a list of tuples, where the 'codename' (e.g add/edit/delete) would be the first value, and the second would be a dictionary of args to be used to initialise a ModelAdminAction instance. The ModelAdmin class itself would then turn those into a dictionary of real ModelAdminAction instances on initialisation, with the 'codename' as the key, and the instance as a value. That way, all the various helpers could easily refer back to individual ModelAdminAction instances using the codename.

Other ideas welcomed :)

[JamesRamm]

@ababic I agree with your 4 'groups' of actions, although I might also add a 5th group where you have an action not tied to a specific view? This would support, essentially, buttons with javascript handlers (ajax updates and whatnot - I would also allow any generic button attributes (like id) to be specified).
A class to handle each model action is a good idea - am I correct in thinking the intended interface would be something like below?

MyModelAdmin(ModelAdmin):

    modeladmin_actions = (('codename', {...}))

Ideally, the existing default actions - inspect, edit, create, delete, would all be implemented through the same interface (as ModelAdminActions in this case) and the default behaviour would be to have the existing actions... actions.py could provide a list of default actions so that it can be easily extended/overridden, e.g

in actions.py:

INSPECT = ('Inspect', {...})
EDIT = ('Edit', {...})
...
DEFAULTS = [INSPECT, EDIT, ...]

So you can then do:

from .actions import DEFAULTS

MyModelAdmin(ModelAdmin):

    modeladmin_actions = DEFAULTS += [('MyAction', {...})]

Or

from .actions import INSPECT

MyModelAdmin(ModelAdmin):
    modeladmin_actions = [INSPECT]

It looks like it would nicely reduce adding an action to just filling in some parameters and creating a view.

[ababic]

@JamesRamm I envisaged the ModelAdminAction class to be smart enough to support that use case too. I think by setting register_url=False and providing a button_url value could be enough to support that (I'm wide open to suggestions on better names for those / how we make that option clearer).

I did envisage the existing actions being migrated to the new interface too... the ModelAdmin class could then cycle through them to create the URLs list etc, and we could probably drop all of the get_codename_template and codename_view methods from the ModelAdmin itself, making things much cleaner. However, that would probably introduce a lot of backwards compatibility issues. It might be that we need to phase some changes in later.

Separate sets of actions would need to be defined for page-type models and non page-type models too, as there are differences between the two.

[ababic]

@JamesRamm Love your idea of having the default options configured in the same file so that they can be imported more easily. One thing I neglected to explain before, but I'd probably suggest that 'codenames' be made to conform to something like '[a-z0-9_]'. I think it would help avoid collisions, and also communicates better how it will be used by the other components.

[JamesRamm]

Im onboard with all that. Would it be possible to keep all the existing functionality with deprecation warnings on functions, while still merging the new method in full? Then provide deprecation warnings for the next release or so (whatever the wagtail convention is).

On conventions...a small note; on your ModelAdminAction you are using both @property and get... functions. Feel free to strike me down (I dont know the wagtail conventions), but you could replace get... with properties for a more consistent implementation? (Although I can see liberal use of get... throughout wagtail so maybe its the accepted style)

A regex and collision check is a great idea yes. Your '[a-z0-9_]' suggestion is liberal enough to allow most namings I can think of...I would also do a specific collision check against the previously registered actions (if possible), to prevent stuff like modeladmin_actions = DEFAULTS += [('name_clash', {...}), ('name_clash', {...})]

[ababic]

I think we will likely need to take some steer from the core team RE deprecation a little later down the line, as I feel it could probably be the biggest challenge here as things pan out.

Great point on conventions on use of @property VS get_something. would gratefully accept some steer there!

And good point about collision checking. I feel like a nicely-worded 'ImproperlyConfigured' exception to notify users about those on ModelAdmin.__init__() would be very useful.

[ababic]

@JamesRamm In regards to something you said about the 5th group of actions:

This would support, essentially, buttons with javascript handlers (ajax updates and whatnot - I would also allow any generic button attributes (like id) to be specified).

I'm beginning to think it will be difficult to fully support this with just a single definition on the model class. E.g. in the case of adding an 'id' attribute, you wouldn't ever want the same ID to be added to all buttons on that type. In almost all examples I can think of, you'd want to have unique values for each instance of the button, likely using field values from the relevant model instance. I feel like the only solution for buttons like that will be to create a method that accepts a model instance, like the methods that currently need to be defined on the ButtonHelper class. We can make things a little bit easier by allowing that method to be defined on the ModelAdmin class OR the ButtonHelper, but I can't think of a nice way to support them via a set of static values - unless anyone else has any ideas?

[JamesRamm]

@ababic, thats true. I was thinking you would probably also want to set some data attributes to be able to do anything particularly useful with javascript (at a minimum the pk of the model, at best whatever attributes the use wishes to have set.)

Yes, best would be a method along the lines of get_button_attributes(self, instance) which by default does nothing. Such a method could return a dict of attributes to set, which would then cover just about anything you would want to do with a button....
This should perhaps be a future addition, to prevent this issue from getting too big?

[ababic]

Yeah that's probably better as a future thing. Although, in the meantime, we can still support the definition of individual 'button definition' methods, and there's also the get_extra_attrs_for_row() method on the ModelAdmin, which can be utilised to add data attributes and whatever else to the row (<tr> element) for each result in IndexView, so things can be made available to js.

[bobslee]

+1 (great enhancement)

[ababic]

Just wanted to flag up a few related ModelAdmin issues.

Ideally, I'd like the following to precede this work, because it will help keep the view-related stuff simple, and I think ButtonHelper is easily weakest part of modeladmin, so whatever we can do to improve that in the meantime will help:

• Improve consistency of how views in ModelAdmin are initialised Improve consistency of how views in ModelAdmin are initialised #12
• Refactor ButtonHelper component of modeladmin to offer drop-down/sub-menu structuring of actions Refactor ButtonHelper component of modeladmin to offer drop-down/sub-menu structuring of actions #7

And possibly the following too, because it will remove some unnecessary cruft from options.py

• Move modeladmin.options.ThumbnailMixin to modeladmin.mixins.ThumbnailMixin Move modeladmin.options.ThumbnailMixin to modeladmin.mixins.ThumbnailMixin wagtail/wagtail#3339

I also never really considered this before, but I feel like this work could make something like the following easier, by letting you flag certain actions as 'bulk actions' or something:

• ModelAdmin: select multiple for mass-action Add bulk actions support for ModelAdmin listings #8

[JamesRamm]

Would #7 not initially complicate resolving this issue, since, if I understand, it is about adding new features...new features would be easier to add once this refactor is done, since this should make extension/future maintenance a bit easier?
There is some overlap with the proposal to tackle #7 with what is proposed here - namely allowing additional button methods to be added to ModelAdmin , which would be superseded by the ModelAdminAction class and simple interface for adding them to ModelAdmin proposed here...

Once this issue tackled, then #7 becomes a slightly simpler question of 'how can we do nested ModelAdminAction's', right?

[ababic]

@JamesRamm

Once this issue tackled, then #7 becomes a slightly simpler question of 'how can we do nested ModelAdminAction's', right?

I'm thinking #7 should probably be de-scoped, so that we can at least have index_view_buttons and inspect_view_buttons attributes on ModelAdmin that let you define which buttons you want to show on those pages, and in what order (just supporting a flat list for now). Without that functionality to complement ModelAdminAction, we're still kinda stuck in a place where developers will need to override various methods on ButtonHelper to make their new actions/buttons appear in the right places, which kind of defeats the object of simplifying it.

There is some overlap with the proposal to tackle #7 with what is proposed here - namely allowing additional button methods to be added to ModelAdmin , which would be superseded by the ModelAdminAction class and simple interface for adding them to ModelAdmin proposed here.

I think we've already agreed that it will be possible to support simple button definitions within the modeladmin_actions definition on a ModelAdmin class, but we still need a way to support more complex buttons that don't always just have the same text / url / might need to incorporate values from the current object. Developers need a way to be able to define methods to generate those more complex buttons, and currently, the only place that's supported is on the ButtonHelper class, which means you'd have to create a custom ButtonHelper just to add a new button definition method to it. Being able to define that method on the ModelAdmin class instead makes that a lot easier, and I think is something we need to have in place to complement this work.

[JamesRamm]

I'm thinking #7 should probably be de-scoped, so that we can at least have index_view_buttons and inspect_view_buttons attributes on ModelAdmin that let you define which buttons you want to show on those pages, and in what order (just supporting a flat list for now). Without that functionality to complement ModelAdminAction, we're still kinda stuck in a place where developers will need to override various methods on ButtonHelper to make their new actions/buttons appear in the right places, which kind of defeats the object of simplifying it.

Right, yes I see where you are coming from now. 👍

[ababic]

Hey @JamesRamm. Quick status update: The work I suggested should precede this work is coming along:

• Move modeladmin.options.ThumbnailMixin to modeladmin.mixins.ThumbnailMixin wagtail/wagtail#3339 is now done
• Improve consistency of how views in ModelAdmin are initialised #12 is being catered for by PR Improve consistency of modeladmin view initialisation and reduce general inconsistency / poor practice between views wagtail/wagtail#3542, which i'm waiting for feedback on
• Refactor ButtonHelper component of modeladmin to offer drop-down/sub-menu structuring of actions #7 is quite far along my own fork, but it's too big a change to go in as a single PR - it needs breaking up into smaller ones to stand any chance of being reviewed and merged.

I've begun working on this particular feature as a (not yet working) third-party app, bundled together with the new ButtonHelper stuff I've done (the two ideas complement each other incredibly well), so that It will be easier to demo and collect feedback on (https://github.com/ababic/waddleadmin/). But, I do kinda need wagtail/wagtail#3542 to be merged before I can get that working properly.

Do please check out actions.py and options.py if you'd like to see how it might all work / fit together.

[JamesRamm]

@ababic
Taken a quick look at actions.py and options.py; looks great. I think you have pretty much captured most of what we discussed which is awesome.
Being able to look through the file and see the action definitions for the default actions has the add on effect of increasing readability IMO - it is much easier to figure out how to create new actions from reading this code.

[unexceptable]

Any progress on this?

[ababic]

Hi @Adrian-Turjak. There's certainly progress, but this was always going to be a challenge. I'm trying to tackle a few related aspects of this functionality separately, in smaller, more focussed PRs, in an aid to make things easier/quicker to review. But they haven't been moving as quickly as I'd hoped.

wagtail/wagtail#3542 had to be cancelled and has been replaced with wagtail/wagtail#3626, which I hope will be reviewed soon. I think the focus has then got to be on getting https://github.com/ababic/waddleadmin/ into a state that will show all of the various components working together as intended. That's probably the best way to gather input from other developers, and get things into a shape that the core team will be happy with / want to prioritise.

[unexceptable]

@ababic Fantastic! I'll try and keep an eye on this will be happy to review and play with https://github.com/ababic/waddleadmin/ when you've got it in a state you're happy with. :)

As much as I dislike the default django admin, the ability to add actions to it was easy, and useful, so I'm really hoping for something just as nice for the wagtail admin! Because as the OP has said, the existing process is a little roundabout. :(