[Django] #33375: Admin changelist_formset does not use the Admin queryset
Django
--------------------------------------------+------------------------
Reporter: François Freitag | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: dev
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
--------------------------------------------+------------------------
I’m implementing soft-deletion for `models.Model`s 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33375>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: François Freitag | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: dev
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by François Freitag):
* Attachment "failing-test.patch" added.
A failing test to reproduce the issue. The response status can be
commented out to verify the objects in the DB are actually not updated.
Django
Reporter: François Freitag | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: dev
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by François Freitag):
* cc: François Freitag (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/33375#comment:1>
Django
Reporter: François Freitag | Owner: nobody
Component: contrib.admin | Version: dev
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: new => closed
* resolution: => needsinfo
Comment:
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():
}}}
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, the form
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33375#comment:2>
Django
Reporter: François Freitag | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: dev
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by François Freitag):
* status: closed => new
* resolution: needsinfo =>
Comment:
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.']}, {}]
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33375#comment:3>
Django
Reporter: François Freitag | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: dev
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by François Freitag):
* Attachment "failing-test.patch" removed.
A failing test to reproduce the issue. The response status can be
commented out to verify the objects in the DB are actually not updated.
--
Django
Reporter: François Freitag | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: dev
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by François Freitag):
* Attachment "failing-test.patch" added.
Failing test v2 (fixed _save and management form missing from POST data)
Django
Reporter: François Freitag | Owner: nobody
Component: contrib.admin | Version: dev
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: new => closed
* resolution: => wontfix
Comment:
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
[https://github.com/django/django/blob/33401cba9317de2487c31ffdd1a5a51ecfbc0248/django/forms/models.py#L859-L862
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
[https://docs.djangoproject.com/en/4.0/ref/contrib/admin/#django.contrib.admin.ModelAdmin.get_changelist_form
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... :)
--
Ticket URL: <https://code.djangoproject.com/ticket/33375#comment:4>
Django
Reporter: François Freitag | Owner: nobody
Component: contrib.admin | Version: dev
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by François Freitag):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33375#comment:5>