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)