Add system check for filter_horizontal/filter_vertical on ManyToManyFields with intermediary models.

[David Pratten]

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:

• ​https://docs.djangoproject.com/en/4.1/ref/models/fields/#django.db.models.ManyToManyField.through

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.

[Mariusz Felisiak]

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="")

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.

In the same way:

• ​https://docs.djangoproject.com/en/4.1/ref/models/fields/#django.db.models.ManyToManyField.through

doesn't call out the consequence that filter_horizontal and filter_vertical will stop working if one goes down the pathway of:

ManyToManyField(through="")

Models docs are not the right place to describe how contrib apps work.

[Mariusz Felisiak]

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."

[David Pratten]

Thanks. Sounds like a good outcome.

[Mariusz Felisiak]

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

@@ -533 +533 @@
                 return must_be(
                     "a many-to-many field", option=label, obj=obj, id="admin.E020"
                 )
+            elif not field.remote_field.through._meta.auto_created:
+                return [
+                    checks.Error(
+                        f"The value of '{label}' cannot include the ManyToManyField "
+                        f"'{field_name}', because that field manually specifies a "
+                        f"relationship model.",
+                        obj=obj.__class__,
+                        id="admin.E013",
+                    )
+                ]
             else:
                 return []
 

Tests and ​docs changes (in the admin.E013 description) are also required.

[David Pratten]

Ok I'll take this up.

[David Pratten]

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

@@ -533 +533 @@
                 return must_be(
                     "a many-to-many field", option=label, obj=obj, id="admin.E020"
                 )
+            elif not field.remote_field.through._meta.auto_created:
+                return [
+                    checks.Error(
+                        f"The value of '{label}' cannot include the ManyToManyField "
+                        f"'{field_name}', because that field manually specifies a "
+                        f"relationship model.",
+                        obj=obj.__class__,
+                        id="admin.E013",
+                    )
+                ]
             else:
                 return []
 

Tests and ​docs changes (in the admin.E013 description) are also required.

[Mariusz Felisiak]

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.

[Natalia Bidart]

Hello David, would you have time to work on this? Thanks!

[Hrushikesh Vaidya]

Since David seems to be busy, I'll try to take this up.

[Hrushikesh Vaidya]

​PR ready for review

[Mariusz Felisiak <felisiak.mariusz@…>]

In 10786578:

Fixed #34345 -- Added system check for ManyToManyFields with intermediate tables in ModelAdmin.filter_horizontal/vertical.