Ability to use plain ForeignKeys with InlinePanels in modeladmin

awhileback commented 3 years ago

Found a bug? Please fill out the sections below. 👍

Issue Summary

Imagine a drip email marketing functionality, in which non-developers are able to build query sets from dropdown menus and append them as inline panels on an email form. That's what this is ;). Of course, people tend to forget things, and the first thing that needs to be included in an email marketing rule by default which the content author would otherwise forget is to exclude people who have unsubscribed. So this is an attempt to automatically append "is_subscribed = True" to the first inline rule of a marketing email form.

But, the conditional default modeladmin inline property seems to be duplicated under certain circumstances. When defining a custom CreateView with an inline form pre-filled, the form is properly appended upon create. But upon edit of the just-created item, the item seems to have been duplicated, there are then two identical entries.

After removing one of the duplicates and saving the item, the duplicate doesn't come back a second time after going back and forth between the edit/list page, so this seems to be specific to the initial create + save, perhaps, in the CreateView that was specified.

Steps to Reproduce

Install wagtail
Install Django drip campaigns (straight from git is fine)
Migrate, integrate the drip admin into wagtail via wagtail_hooks like so....

(You will of course not have an is_subscribed boolean field, change to some other django user model boolean field to see the result, you'll also not have the django-summernote editor widget unless you install it. Otherwise I think these should work as described.)

from drip.models import Drip, SentDrip, QuerySetRule
from wagtail.contrib.modeladmin.views import CreateView, EditView

class DripAdminCreateView(CreateView):
    def get_instance(self):
        instance = super().get_instance()
        # default values for first query rules InlinePanel to only match subscribed users
        if instance.queryset_rules.exists():
            pass
        else:
            instance.queryset_rules = [
                QuerySetRule(method_type='filter', field_name='is_subscribed', lookup_type='exact', rule_type='and', field_value=1, sort_order=0)
            ]
        return instance

class DripAdmin(ModelAdmin):
    model = Drip
    menu_icon = 'fa-tint'
    menu_label = 'Drip Email'
    list_display = ('id', 'name', 'subject_template', 'enabled', 'lastchanged')
    create_view_class = DripAdminCreateView

    panels = [
        MultiFieldPanel(
            [
                FieldPanel('name', widget=SubjectField),
                FieldPanel('enabled'),
                FieldPanel('from_email'),
                FieldPanel('from_email_name', widget=SubjectField),
                FieldPanel('subject_template', widget=SubjectField),
                FieldPanel('body_html_template', widget=SummernoteInplaceWidget),
                InlinePanel('queryset_rules', label='Query Set Rules'),
            ],
            _('Drip Admin'),
        )
    ]

class SentDripAdmin(ModelAdmin):
    model = SentDrip
    menu_icon = 'fa-envelope-open'
    menu_label = 'Drip Email Logs'
    list_display = ('id', 'drip', 'subject', 'user', 'date')
    permission_helper_class = DripLogPermissionHelper

    panels = [
        MultiFieldPanel(
            [
                FieldPanel('subject', widget=SubjectField),
                FieldPanel('body', widget=SummernoteInplaceWidget),
                FieldPanel('from_email', widget=SubjectField),
                FieldPanel('from_email_name', widget=SubjectField),
            ],
            _('Sent Drip Admin'),
        )
    ]

class EmailGroup(ModelAdminGroup):
    menu_label = 'Email'
    menu_icon = 'fa-envelope'
    menu_order = 500
    items = (DripAdmin, SentDripAdmin)

modeladmin_register(EmailGroup)

To further test this I put some print statements in my conditionals along with an EditView specification and got the following...

class DripAdminEditView(EditView):
    def get_instance(self):
        instance = super().get_instance()
        if instance.queryset_rules.exists():
            print('true')
        else:
            print('false')
        return instance

Upon edit, I get...

true
true
true
true
[11/May/2021 02:09:02] "GET /admin/drip/drip/edit/10/ HTTP/1.1" 200 51466

Upon create, with a print of ('false') added to the default inline...

class DripAdminCreateView(CreateView):
    def get_instance(self):
        instance = super().get_instance()
        # default values for first query rules InlinePanel to only match subscribed users
        if instance.queryset_rules.exists():
            print('true')
        else:
            instance.queryset_rules = [
                    QuerySetRule(method_type='filter', field_name='is_subscribed', lookup_type='exact', field_value=1, sort_order=0)
            ]
            print('false')

        return instance

I got...

false
false
false
false
[11/May/2021 02:10:07] "GET /admin/drip/drip/create/ HTTP/1.1" 200 42503

So the conditionals seem to be working as expected.

This doesn't appear to be some weird browser caching behavior, the database shows two identical entries for the first inline, here's an SQL dump of one after 'create':

INSERT INTO "main"."drip_querysetrule" ("id", "sort_order", "date", "last_changed", "method_type", "field_name", "lookup_type", "field_value", "drip_id") VALUES ('32', '0', '2021-05-11 07:00:01.843932', '2021-05-11 07:00:01.940603', 'filter', 'is_subscribed', 'exact', '1', '10');
INSERT INTO "main"."drip_querysetrule" ("id", "sort_order", "date", "last_changed", "method_type", "field_name", "lookup_type", "field_value", "drip_id") VALUES ('33', '0', '2021-05-11 07:00:01.985938', '2021-05-11 07:00:01.985988', 'filter', 'is_subscribed', 'exact', '1', '10');
INSERT INTO "main"."drip_querysetrule" ("id", "sort_order", "date", "last_changed", "method_type", "field_name", "lookup_type", "field_value", "drip_id") VALUES ('34', '1', '2021-05-11 07:00:02.045635', '2021-05-11 07:00:02.045719', 'filter', 'last_login', 'gt', 'now-1 day', '10');

Technical details

Python version: 3.8.5
Django version: 3.1
Wagtail version: 2.11.7
Browser version: Safari 14

gasman commented 3 years ago

Hi @awhileback, I think this is because the queryset_rules relation in django-drip-campaigns - being not specifically built for Wagtail - is a ForeignKey rather than a ParentalKey. InlinePanels are only designed for ParentalKey relations - these have different behaviours regarding how forms are processed and the order in which models are saved, and these differences are key to various bits of Wagtail functionality like previewing and revision history.

Having said that, those aspects don't apply to ModelAdmin, and since ModelAdmin is intended to work with plain Django models, I think it's reasonable to expect plain foreign keys to work there. So while I don't think this is an actual bug, I'd say it's a legitimate feature request.

awhileback commented 3 years ago

Hey @gasman, thanks for the reply, now that you mention it I remember why there was a need to fork the drip app. Sorry, it's been awhile since I got this working initially a couple of years ago, I've just come back to it for the purpose of upgrading versions.

I did sub-class the InlinePanel model and change it to a ParentalKey to get it to work. That was done before I posted the bug report.

https://github.com/awhileback/django-drip-campaigns/blob/e708f3d55bef4bca1ac167d43223f028da852fd3/drip/models.py#L365

wagtail-nest / wagtail-modeladmin