Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#33375 closed Bug (wontfix)

Admin changelist_formset does not use the Admin queryset

Reported by: François Freitag Owned by: nobody
Component: contrib.admin Version: dev
Severity: Normal Keywords:
Cc: François Freitag Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I’m implementing soft-deletion for models.Models with a BooleanField named deleted. For the entire system (except Django admin), an object deleted=True no longer exist. That is implemented by setting the default manager to an objects manager that ignores deleted=True rows.
For the admin, I’m overriding the queryset to include deleted=True.

When using list_editable = ["deleted"], the soft-deleted objects cannot be re-activated. The reason is that the id field in the changelist_formset uses the _default_manager (objects).
I do not want to make include_deleted the default manager, as any model field in the system would use that instead of the objects, unless overridden.

IMO, the admin should be using the result of get_queryset() for the changelist_formset id field Queryset.
Happy to attempt patching if the ticket is accepted.

Attachments (1)

failing-test.patch (3.5 KB ) - added by François Freitag 3 years ago.
Failing test v2 (fixed _save and management form missing from POST data)

Download all attachments as: .zip

Change History (6)

comment:1 by François Freitag, 3 years ago

Cc: François Freitag added

comment:2 by Carlton Gibson, 3 years ago

Resolution: needsinfo
Status: newclosed

Hi François.

Can I ask you to dig a bit deeper, I'm not sure the test is right...

The relevant section from the changelist_view:

        # Handle POSTed bulk-edit data.
        if request.method == 'POST' and cl.list_editable and '_save' in request.POST:
            if not self.has_change_permission(request):
                raise PermissionDenied
            FormSet = self.get_changelist_formset(request)
            modified_objects = self._get_list_editable_queryset(request, FormSet.get_default_prefix())
            formset = cl.formset = FormSet(request.POST, request.FILES, queryset=modified_objects)
            if formset.is_valid():

src

Adding the required _save to the POST data, we enter the block here, but despite having the right queryset, we don't have any forms, and the formset isn't valid:

(Pdb) modified_objects
<QuerySet [<SoftDeletable: SoftDeletable object (1)>, <SoftDeletable: SoftDeletable object (2)>]>
(Pdb) formset.queryset
<QuerySet [<SoftDeletable: SoftDeletable object (1)>, <SoftDeletable: SoftDeletable object (2)>]>
(Pdb) formset.is_valid()
False
(Pdb) formset.forms
[]
(Pdb) 

I guess this is the missing management form data (but I've run out of time to dig as it is).

If you look into _get_list_editable_queryset(), you'll see that self.get_queryset() is used, which gives us the hoped for modified_objects queryset.

No doubt you're seeing something, but there's not enough quite yet to see what it is (with available time).
I'll mark as needsinfo. Please reopen when you can add more.
Thanks.

Last edited 3 years ago by Carlton Gibson (previous) (diff)

comment:3 by François Freitag, 3 years ago

Resolution: needsinfo
Status: closednew

Hi Carlton,

Thanks for looking into it! The ManagementForm was indeed missing from my patch. It was also missing _save=on.
With these changes, the issue triggers in the correct code path. I’ll update the attached patch immediately.

modified_objects is indeed the views queryset, including soft-deleted objects. However, the formset id field used the default queryset. Here’s the behavior I’m seeing while step-debugging the code:

> /home/freitafr/dev/django/django/contrib/admin/options.py(1765)changelist_view()
-> if formset.is_valid():
(Pdb) l
1760                    raise PermissionDenied
1761                FormSet = self.get_changelist_formset(request)
1762                breakpoint()
1763                modified_objects = self._get_list_editable_queryset(request, FormSet.get_default_prefix())
1764                formset = cl.formset = FormSet(request.POST, request.FILES, queryset=modified_objects)
1765 ->             if formset.is_valid():
1766                    changecount = 0
1767                    for form in formset.forms:
1768                        if form.has_changed():
1769                            obj = self.save_form(request, form, change=True)
1770                            self.save_model(request, obj, form, change=True)
(Pdb) n
> /home/freitafr/dev/django/django/contrib/admin/options.py(1795)changelist_view()
-> if formset:
(Pdb) p formset.is_valid()
False
(Pdb) p formset.errors
[{'id': ['Select a valid choice. That choice is not one of the available choices.']}, {}]

by François Freitag, 3 years ago

Attachment: failing-test.patch added

Failing test v2 (fixed _save and management form missing from POST data)

comment:4 by Carlton Gibson, 3 years ago

Resolution: wontfix
Status: newclosed

Thanks for the update François.

[{'id': ['Select a valid choice. That choice is not one of the available choices.']}, {}]

So just looking at that, you have a ModelChoiceField with the wrong QuerySet, and sure enough...

(Pdb) form.fields
{'deleted': <django.forms.fields.BooleanField object at 0x104a136a0>, 'id': <django.forms.models.ModelChoiceField object at 0x104a13a90>}
(Pdb) form.fields['id'].queryset
<QuerySet [<SoftDeletable: SoftDeletable object (2)>]>

Why? Because in BaseModelFormSet.add_fields() we add a just such a field using the default manager.

I suspect that's untouchable, and just a limitation of the _soft delete_ approach.

I'd look at overriding ModelAdmin.get_changelist_form (or maybe ModelAdmin.get_changelist_formset) to provide the base form class declaring the id ModelChoiceField with the correct (for your case) QuerySet.

I'm going to mark as wontfix. If you come up with a suggestion, happy to take a look (but... :)

comment:5 by François Freitag, 3 years ago

Thanks for the answer!
I suspected it would be quite involved to override the auto-generated form field queryset. If no-one feels strongly it should be done, I’m fine implementing workarounds in my project.

Note: See TracTickets for help on using tickets.
Back to Top