ModelAdmin form_fields_exclude is ignored on models with a panels definition

[mixxorz]

Issue Summary

According to the docs and PR wagtail/wagtail#3295, you should be able to add form_fields_exclude or get_form_fields_exclude on a ModelAdmin to exclude certain fields from rendering on the admin. When I tried it, it didn't work. Upon further investigation, I found out that it has never actually worked. Issue #6 is still open.

The issue is rooted in extract_panel_definitions_from_model_class function. When you have a model that has panels, it returns the panels as is, without filtering the excluded fields.

So a couple things that stand out to me:

1. Maybe mention in the docs that this doesn't actually work
2. Find a way to make it work for all panel types

Steps to Reproduce

1. Create a ModelAdmin in wagtail_hooks.py
2. Set form_fields_exclude = ['some_field'] as a class attribute
3. Create a new instance of that model in the Wagtail admin
4. Look in confusion at the still present some_field

Workaround

I'm currently working around the issue by creating a custom CreateView for my ModelAdmins and overriding get_edit_handlers to remove panels I don't want.

class PatchedCreateView(CreateView):

    def get_edit_handler_class(self):
        if hasattr(self.model, 'edit_handler'):
            edit_handler = self.model.edit_handler
        else:
            fields_to_exclude = self.model_admin.get_form_fields_exclude(
                request=self.request
            )
            panels = extract_panel_definitions_from_model_class(
                self.model,
                exclude=fields_to_exclude
            )

            # Filter out fields in fields_to_exclude
            panels = [
                panel for panel in panels
                if getattr(panel, 'field_name', None) not in fields_to_exclude
            ]

            edit_handler = ObjectList(panels)
        return edit_handler.bind_to_model(self.model)

Only problem is this doesn't work for panels without field names. But in my particular case that's okay.

Technical details

• Python version: 3.6.4
• Django version: 1.11.10
• Wagtail version: 1.13.1

[lucasmoeskops]

It's actually a rare use case I think, since you already have custom panels defined, but I guess you want to customise the panels per view. You can also do this by creating multiple edit handlers, but this way should technically also be possible. I'll see if I can add the functionality for this.

[lucasmoeskops]

wagtail/wagtail#4363

[ababic]

Hi guys. This feature wasn't ever meant to be used in combination with a panels attribute on the model. It's really meant to serve as a shortcut to removing a field or two from the admin form when needed (which shows all editable fields by default), without having to define panels.

If the decision has been made to now prefer form_fields_exclude over specific panel definitions, then as well intentioned the changes implemented in wagtail/wagtail#4363 are, they do introduce a degree of inconsistency that should really be remedied. The new behaviour only works when panels is a flat list, but not if the fields are grouped into groups (with MutliFieldPanel) or rows (with FieldRowPanel), or if edit_handler is set directly to create a custom TabbedInterface.

For consistency, I would propose that the changes in wagtail/wagtail#4363 be reverted, and instead we document that the form_fields_exclude is meant to bypass the need for defining a panels attribute on the model. And if panels or edit_handler is set, the form_fields_exclude value will be ignored.

In a perfect world, we'd have a lot more flexibility baked into modeladmin for controlling the form representation (and actively encourage it a place to define such representation, like Django's version does). But, I think we have some significant hurdles to jump before we get there. Right now, such changes would only affect the UI for models that don't subclass Page, and wouldn't affect the snippets UI if the model were also registered as a snippet.

[ababic]

@lucasmoeskops / @mixxorz please see my comment above.

[mixxorz]

tbh I just thought the documentation for form_fields_excludes was misleading and thought it needed updating. Something like "this does not override custom panel definitions on the model" plus some information on how to do that (through a custom Create/Edit/FormView).

That being said, I don't see the point in reverting wagtail/wagtail#4363 because covering one common use-case is better than nothing. The inconsistency can be mitigated, for now at least, by documenting what does and doesn't work. Hopefully, we'll eventually have a solution that fits all use-cases.

[ababic]

That being said, I don't see the point in reverting wagtail/wagtail#4363 because covering one common use-case is better than nothing. The inconsistency can be mitigated, for now at least, by documenting what does and doesn't work. Hopefully, we'll eventually have a solution that fits all use-cases.

I have to politely disagree here. I think consistency should be of more importance to a project like wagtail, and consistency with model representation is something we need to be improving in wagtail (not the opposite).

I know the changes solve a problem in your case, but I do believe the way you're using those two things in combination is rare. A better approach to solving this problem would be to add a get_edit_handler() method to the ModelAdmin class, and have the views call it when needed. Something a little like:

def get_edit_handler(self, request, obj, is_create=False):
    if hasattr(self.model, 'edit_handler'):
        edit_handler = self.model.edit_handler
    else:
        panels = extract_panel_definitions_from_model_class(
            self.model, exclude=self.get_form_fields_exclude(request))
        edit_handler = ObjectList(panels)
    return edit_handler.bind_to_model(self.model)

This would allow you to customise both the interface for 'create', 'edit' or any custom views in one place, without having to resort to overriding views.

[mixxorz]

I think you bring up valid points.

So to summarize:

1. Move get_edit_handler out of ModelFormView and intoModelAdmin
2. Either remove form_fields_exclude entirely (breaking change) or document its intent and actual behavior.
3. Rollback ModelAdmin form_fields_exclude is ignored on models with a panels definition #14 in the process.

Does this sound good? If so, I'm willing to open a PR that implements this behavior.

By the way, I agree that once we've implemented a "right way" to customize a ModelAdmin's panels, that should override form_fields_exclude.

[ababic]

Thanks @mixxorz,

It may be better to await confirmation from a core team member (@BertrandBordage / @gasman), as they could have different ideas, but that's the approach I would suggest (with 'improving the documentation' for form_fields_exclude, rather than removing it).

get_edit_handler would need to stay on ModelFormView so that anyone already overriding that method doesn't get an error when they call super() - But, the method should be made to return the result of the ModelAdmin version (with is_create being determined by a class attribute of some description, which would be set to True on CreateView)

EDIT: The new method on the ModelAdmin class will also need documenting

[gasman]

Sorry for missing this discussion earlier. This is now something we need to resolve for the 2.1 final release, since the current merged version of wagtail/wagtail#4363 has broken the behaviour of MultiFieldPanel, as pointed out by @favoyang on wagtail/wagtail#4363 (comment) / wagtail/wagtail#4539.

It looks to me like the immediate bug is a simple logical error that can be fixed by reverting or adjusting c83a712d5926fbc148c8606bbc4eac7513d9ff0b - I guess the intent of the isinstance(panel, FieldPanel) check was to skip over non-FieldPanels, but it ended up dropping them from the list instead. However, fixing that just re-raises @ababic's point from #14 - to correctly apply form_fields_exclude to an existing panels list, we'd need to recurse into MultiFieldPanel, TabbedInterface etc.

Instead, I think there's a strong argument that the old behaviour - an explicit panel definition takes priority over form_fields_exclude - is the preferred behaviour. The panel definition is the lower-level construction, for when you need full control over the layout of the form; it makes sense to view form_fields_exclude as a shortcut to say "I don't care about defining panels myself - please do it for me automatically, based on this information". A similar situation in vanilla Django would be fields / exclude on ModelForm - use them if you're happy to entrust Django with translating model fields into form fields, but if you need more control, you should define form fields explicitly.

So, if there are no strong objections from @BertrandBordage / @lucasmoeskops, I propose reverting wagtail/wagtail#4363 in its entirety. If anyone's able to write a documentation patch in the next week or so to clarify the form_fields_exclude behaviour, we can include that in the 2.1 release; any further code refactoring is best left for 2.2 at this point.

[lucasmoeskops]

Hi,

Thanks for the suggestions. I agree with the point from Matt and ababic that form_fields_exclude was not meant for this and am for reverting it. Optionally it could generate a warning maybe, saying that the form_fields_exclude option was ignored because of the panel definition.

[gasman]

Now reverted in e84b4a0 (master) / 6ccd665 (stable/2.1.x).

[laymonage]

I believe the same issue/lack of documentation also exists in snippets. That said, this issue thread has been specific to ModelAdmin so I'm transferring this to the wagtail-modeladmin repository as per wagtail/rfcs#85. I'll raise a separate issue for Wagtail's snippets later.

[lb-]

Maybe the form field exclude behaviour is the cause/confusion underlying this issue. wagtail/wagtail#10625