#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

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)

in reply to:  description comment:1 by Mariusz Felisiak, 21 months ago

Easy pickings: unset
Keywords: Gotcha removed
Summary: filter_horizontal and filter_vertical don't work with through - limitation not documentedfilter_horizontal and filter_vertical don't work with through
Type: BugCleanup/optimization

Neither of

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 (see also #12203.)

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

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

Last edited 21 months ago by Mariusz Felisiak (previous) (diff)

comment:2 by Mariusz Felisiak, 21 months ago

Component: Documentationcontrib.admin
Summary: filter_horizontal and filter_vertical don't work with throughAdd system check for filter_horizontal/filter_vertical on ManyToManyFields with intermediary models.
Triage Stage: UnreviewedAccepted

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

comment:3 by David Pratten, 21 months ago

Thanks. Sounds like a good outcome.

in reply to:  3 ; comment:4 by Mariusz Felisiak, 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:  
    533533                return must_be(
    534534                    "a many-to-many field", option=label, obj=obj, id="admin.E020"
    535535                )
     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                ]
    536546            else:
    537547                return []
    538548

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

comment:5 by David Pratten, 21 months ago

Owner: changed from nobody to David Pratten
Status: newassigned

Ok I'll take this up.

in reply to:  4 ; comment:6 by David Pratten, 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:  
    533533                return must_be(
    534534                    "a many-to-many field", option=label, obj=obj, id="admin.E020"
    535535                )
     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                ]
    536546            else:
    537547                return []
    538548

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

in reply to:  6 comment:7 by Mariusz Felisiak, 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 Mariusz Felisiak, 20 months ago

Cc: Natalia Bidart added

comment:9 by Natalia Bidart, 18 months ago

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

comment:10 by Hrushikesh Vaidya, 17 months ago

Owner: changed from David Pratten to Hrushikesh Vaidya

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

comment:11 by Hrushikesh Vaidya, 17 months ago

PR ready for review

comment:12 by Mariusz Felisiak, 17 months ago

Has patch: set

comment:13 by Mariusz Felisiak, 17 months ago

Triage Stage: AcceptedReady for checkin

comment:14 by Mariusz Felisiak <felisiak.mariusz@…>, 17 months ago

Resolution: fixed
Status: assignedclosed

In 10786578:

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

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