Opened 21 months ago
Closed 17 months ago
#34345 closed Cleanup/optimization (fixed)
Add system check for filter_horizontal/filter_vertical on ManyToManyFields with intermediary models.
Reported by: | David Pratten | Owned by: | Hrushikesh Vaidya |
---|---|---|---|
Component: | contrib.admin | Version: | 4.1 |
Severity: | Normal | Keywords: | |
Cc: | Natalia Bidart | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Hi team,
I'm a huge fan of Django and have been using it since 0.95 but I stumbled over this one.
Neither of
- https://docs.djangoproject.com/en/4.1/ref/contrib/admin/#django.contrib.admin.ModelAdmin.filter_horizontal and
- https://docs.djangoproject.com/en/4.1/ref/contrib/admin/#django.contrib.admin.ModelAdmin.filter_vertical
call out the requirement to not use
ManyToManyField(through="")
In the same way:
doesn't call out the consequence that filter_horizontal and filter_vertical will stop working if one goes down the pathway of:
ManyToManyField(through="")
I just wasted half a day chasing this down.
Change History (14)
comment:1 by , 21 months ago
Easy pickings: | unset |
---|---|
Keywords: | Gotcha removed |
Summary: | filter_horizontal and filter_vertical don't work with through - limitation not documented → filter_horizontal and filter_vertical don't work with through |
Type: | Bug → Cleanup/optimization |
comment:2 by , 21 months ago
Component: | Documentation → contrib.admin |
---|---|
Summary: | filter_horizontal and filter_vertical don't work with through → Add system check for filter_horizontal/filter_vertical on ManyToManyFields with intermediary models. |
Triage Stage: | Unreviewed → Accepted |
What do you think about raising admin.E013
in this case?
"fields[n]/fieldsets[n][m]/filter_vertical[n]/filter_horizontal[n] cannot include the ManyToManyField <field name>, because that field manually specifies a relationship model."
follow-up: 6 comment:4 by , 21 months ago
Replying to David Pratten:
Thanks. Sounds like a good outcome.
Would you like to prepare a patch via GitHub PR? The following should work:
-
django/contrib/admin/checks.py
diff --git a/django/contrib/admin/checks.py b/django/contrib/admin/checks.py index 27537d9614..a844b3f16f 100644
a b class BaseModelAdminChecks: 533 533 return must_be( 534 534 "a many-to-many field", option=label, obj=obj, id="admin.E020" 535 535 ) 536 elif not field.remote_field.through._meta.auto_created: 537 return [ 538 checks.Error( 539 f"The value of '{label}' cannot include the ManyToManyField " 540 f"'{field_name}', because that field manually specifies a " 541 f"relationship model.", 542 obj=obj.__class__, 543 id="admin.E013", 544 ) 545 ] 536 546 else: 537 547 return [] 538 548
Tests and docs changes (in the admin.E013
description) are also required.
follow-up: 7 comment:6 by , 21 months ago
Replying to Mariusz Felisiak:
I'm happy to work through this, but it won't be quick.
- Are we redefining
admin.E013
there seems to already be a description of this error? - Could you direct me to an explanation of where the documentation for the errors is held and how it is updated?
- Could you direct me to an explanation of how to add a test case?
Thanks
Replying to David Pratten:
Thanks. Sounds like a good outcome.
Would you like to prepare a patch via GitHub PR? The following should work:
django/contrib/admin/checks.py
diff --git a/django/contrib/admin/checks.py b/django/contrib/admin/checks.py index 27537d9614..a844b3f16f 100644
a b class BaseModelAdminChecks: 533 533 return must_be( 534 534 "a many-to-many field", option=label, obj=obj, id="admin.E020" 535 535 ) 536 elif not field.remote_field.through._meta.auto_created: 537 return [ 538 checks.Error( 539 f"The value of '{label}' cannot include the ManyToManyField " 540 f"'{field_name}', because that field manually specifies a " 541 f"relationship model.", 542 obj=obj.__class__, 543 id="admin.E013", 544 ) 545 ] 536 546 else: 537 547 return [] 538 548 Tests and docs changes (in the
admin.E013
description) are also required.
comment:7 by , 21 months ago
Replying to David Pratten:
- Are we redefining
admin.E013
there seems to already be a description of this error?- Could you direct me to an explanation of where the documentation for the errors is held and how it is updated?
We want to add filter_vertical[n]
and filter_horizontal[n]
to the existing error admin.E013
that is documented in docs/ref/checks.txt, so we need to update the message in docs to the:
"fields[n]/filter_horizontal[n]/filter_vertical[n]/fieldsets[n][m]
cannot include the ManyToManyField
<field name>
, because that field manually specifies a relationship model."
Docs are wrapped at 79 chars.
- Could you direct me to an explanation of how to add a test case?
I would add test methods to the tests.modeladmin.test_checks.FilterHorizontalCheckTests
and tests.modeladmin.test_checks.FilterVerticalCheckTests
.
comment:8 by , 20 months ago
Cc: | added |
---|
comment:10 by , 17 months ago
Owner: | changed from | to
---|
Since David seems to be busy, I'll try to take this up.
comment:12 by , 17 months ago
Has patch: | set |
---|
comment:13 by , 17 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
There is a separate section in the same docs that describes Working with many-to-many intermediary models. I don't think it is necessary to cross-refer this section in all places where
ManyToManyField
is mentioned (see also #12203.)Models docs are not the right place to describe how contrib apps work.