[Django] #34129: Admin list_editable failed to edit
Django
-----------------------------------------+------------------------
Reporter: dengjing1994 | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 3.2
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 |
-----------------------------------------+------------------------
1. I have two **Manager** for my **Model**, one shows only valid items and
another one shows all items, and then I change the model's **default
manager** to the previous one which shows only valid items.
2. I inherit the **ModelAdmin** and rewrite **get_queryset** method to
make the queryset use the manager which shows all items.
3. Add **valid** field to the **list_editable**.
4. Here comes the bug. When all items are valid, it's ok and I can change
item to invalid(valid=False) and save. But once there is an invalid item,
you can't edit it anymore. It just shows "Please correct the error below"
whithout any other message.
--
Ticket URL: <https://code.djangoproject.com/ticket/34129>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: dengjing1994 | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 3.2
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 dengjing1994):
* Attachment "first_all_valid.jpg" added.
1. all valid items
Django
-------------------------------+--------------------------------------
Reporter: dengjing1994 | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 3.2
Severity: Normal | Resolution:
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 dengjing1994):
* Attachment "second_change_to_invalid.jpg" added.
2.it's ok to edit item to invalid
Django
-------------------------------+--------------------------------------
Reporter: dengjing1994 | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 3.2
Severity: Normal | Resolution:
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 dengjing1994):
* Attachment "third_cannot_edit_anymore.jpg" added.
3. can't edit anymore once invalid item appear
Django
-------------------------------+--------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 3.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
* cc: Djing (added)
Comment:
The exception is DoesNotExist, and it is thrown by the **to_python**
method of django.forms.models.**ModelChoiceField**.
{{{
def to_python(self, value):
if value in self.empty_values:
return None
try:
key = self.to_field_name or 'pk'
if isinstance(value, self.queryset.model):
value = getattr(value, key)
value = self.queryset.get(**{key: value})
except (ValueError, TypeError, self.queryset.model.DoesNotExist):
raise ValidationError(self.error_messages['invalid_choice'],
code='invalid_choice')
return value
}}}
The **ModelChoiceField** and it's queryset come from the **add_fields**
method of django.forms.models.**BaseModelFormSet**. This method builds
field for pk according to the comment.
{{{
# Omit the preceding part...
if isinstance(pk, (ForeignKey, OneToOneField)):
qs = pk.remote_field.model._default_manager.get_queryset()
else:
qs = self.model._default_manager.get_queryset()
qs = qs.using(form.instance._state.db)
if form._meta.widgets:
widget = form._meta.widgets.get(self._pk_field.name, HiddenInput)
else:
widget = HiddenInput
form.fields[self._pk_field.name] = ModelChoiceField(qs,
initial=pk_value, required=False, widget=widget)
# Omit the latter part...
}}}
The qs here use the default manager of Model which only shows valid items,
so invalid item can't be found.
And I noticed that **BaseModelFormSet** has the correct queryset which
shows all items when it is created by **ModelAdmin**.
--
Ticket URL: <https://code.djangoproject.com/ticket/34129#comment:1>
Django
-------------------------------+--------------------------------------
Reporter: Djing | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 3.2
Severity: Normal | Resolution:
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 Josh Michael Karamuth):
Came across the same problem and you're right.
{{{
qs = self.model._default_manager.get_queryset()
}}}
Should be
{{{
qs = self.get_queryset()
}}}
Thanks for debugging! Will you submit a PR for this?
--
Ticket URL: <https://code.djangoproject.com/ticket/34129#comment:2>
Django
-------------------------------+--------------------------------------
Reporter: Djing | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 3.2
Severity: Normal | Resolution:
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 Djing):
Replying to [comment:2 Josh Michael Karamuth]:
> Came across the same problem and you're right.
>
>
> {{{
> qs = self.model._default_manager.get_queryset()
> }}}
>
> Should be
>
>
> {{{
> qs = self.get_queryset()
> }}}
>
> Thanks for debugging! Will you submit a PR for this?
Yes.
--
Ticket URL: <https://code.djangoproject.com/ticket/34129#comment:3>
Django
-------------------------------+--------------------------------------
Type: Bug | Status: assigned
Component: contrib.admin | Version: 3.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
* owner: nobody => Djing
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/34129#comment:4>
Django
-------------------------------+--------------------------------------
Reporter: Djing | Owner: Djing
Type: Bug | Status: assigned
Component: contrib.admin | Version: 3.2
Severity: Normal | Resolution:
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 David Sanders):
Please note the documentation warns about these situations where a default
queryset hide/restricts the data it retrieves:
https://docs.djangoproject.com/en/4.1/topics/db/managers/#default-managers
it’s a good idea to be careful in your choice of default manager in
order to avoid a situation where overriding get_queryset() results in an
inability to retrieve objects you’d like to work with.
It may be better to have the valid manager as the non-default and get the
admin to use this instead.
--
Ticket URL: <https://code.djangoproject.com/ticket/34129#comment:5>
Django
-------------------------------+--------------------------------------
Reporter: Djing | Owner: Djing
Type: Bug | Status: assigned
Component: contrib.admin | Version: 3.2
Severity: Normal | Resolution:
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 Djing):
Replying to [comment:2 Josh Michael Karamuth]:
This is my first time writing PR for django, and I follow the doc step by
step. Now I'm having some problems with the test.
After modifying the code as mentioned before, there are 2 existing tests
which located in **model_formsets.tests.ModelFormsetTest** failed.
{{{
def test_prevent_change_outer_model_and_create_invalid_data(self):
author = Author.objects.create(name="Charles")
other_author = Author.objects.create(name="Walt")
AuthorFormSet = modelformset_factory(Author, fields="__all__")
data = {
"form-TOTAL_FORMS": "2",
"form-INITIAL_FORMS": "2",
"form-MAX_NUM_FORMS": "",
"form-0-id": str(author.id),
"form-0-name": "Charles",
"form-1-id": str(other_author.id), # A model not in the
formset's queryset.
"form-1-name": "Changed name",
}
# This formset is only for Walt Whitman and shouldn't accept data
for
# other_author.
formset = AuthorFormSet(
data=data, queryset=Author.objects.filter(id__in=(author.id,))
)
self.assertTrue(formset.is_valid())
formset.save()
# The name of other_author shouldn't be changed and new models
aren't
# created.
self.assertSequenceEqual(Author.objects.all(), [author,
other_author])
def test_edit_only_object_outside_of_queryset(self):
charles = Author.objects.create(name="Charles Baudelaire")
walt = Author.objects.create(name="Walt Whitman")
data = {
"form-TOTAL_FORMS": "1",
"form-INITIAL_FORMS": "1",
"form-0-id": walt.pk,
"form-0-name": "Parth Patil",
}
AuthorFormSet = modelformset_factory(Author, fields="__all__",
edit_only=True)
formset = AuthorFormSet(data,
queryset=Author.objects.filter(pk=charles.pk))
self.assertIs(formset.is_valid(), True)
formset.save()
self.assertCountEqual(Author.objects.all(), [charles, walt])
}}}
They both give the formset a non-default queryset and pass data not in the
queryset, and see if it just work for data in the queryset.(It seems just
the opposite of mine).
So, only one of two situations can pass the test for
{{{self.assertTrue(formset.is_valid())}}}, either theirs or mine.
I don't know how to deal with this situation.
Or just as [comment:5 David Sanders] says:
>It may be better to have the valid manager as the non-default and get the
admin to use this instead
--
Ticket URL: <https://code.djangoproject.com/ticket/34129#comment:6>
Django
-------------------------------+--------------------------------------
Reporter: Djing | Owner: Djing
Component: contrib.admin | Version: 3.2
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
* status: assigned => closed
* resolution: => duplicate
Comment:
This is a duplicate of #33375: if you **really** need this pattern define
a form class with the id, using the desired queryset, so that
`BaseModelFormSet.add_fields()` doesn't add the field with the default
manager.
David is on track: you need to be extra careful with setting default
managers that filter the QuerySet.
--
Ticket URL: <https://code.djangoproject.com/ticket/34129#comment:7>