Skip to content

Fixed #12203 -- Allow using ManyToManyField with through in Admin.#18612

Open
RosanaRufer wants to merge 1 commit intodjango:mainfrom
RosanaRufer:12203
Open

Fixed #12203 -- Allow using ManyToManyField with through in Admin.#18612
RosanaRufer wants to merge 1 commit intodjango:mainfrom
RosanaRufer:12203

Conversation

@RosanaRufer
Copy link
Copy Markdown
Contributor

@RosanaRufer RosanaRufer commented Sep 23, 2024

Trac ticket number

ticket-12203

Branch description

Closing ticket 12203 - Making it possible to define custom through models as Admin fields.
Extended description

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • [N/A] I have attached screenshots in both light and dark modes for any UI changes.

Context

Took @collinanderson 's work, tested the solution and works great with a basic through model.
This solution removes the check that prevents setting an m2m field in ModelAdmin.fields, when the field was configured with a custom through model. We wondered if it'd make sense to remove this check only when the through model isn't exactly like the one that would be autogenerated by Django but we don't think it's really possible for the checks to figure out if it's going to be an issue or not without executing the operation, if you know a way, please share.

@RosanaRufer RosanaRufer changed the title Fixed django#12203 -- Allow using ManyToManyField with through in Admin. [Not ready yet] Fixed django#12203 -- Allow using ManyToManyField with through in Admin. Sep 23, 2024
@collinanderson
Copy link
Copy Markdown
Contributor

collinanderson commented Sep 23, 2024

instead of completely removing the check we'd want to do it when:
a) if there are other fields in the through model these are either nullable or have a default value

The description of https://code.djangoproject.com/ticket/9475 says:

Similarly, if the attributes are calculated in the custom save() method or if they have a default value, those methods should be supported.
Maybe, simply removing the extra checks will do. So is it is the values are not sufficient, a db error will be raised anyway.

So a non-null field could theoretically be fine as long as it gets filled by the model's save() (or somehow). For example, there could be a non-null "user" field on the model that somehow gets auto-filled in to the current user before it gets saved to the database.

Similar to ticket 9475, I'd suggest just letting the db error be raised at runtime. I don't think it's really possible for the checks to figure out if it's going to be an issue or not without looking at the code. But I might be wrong.

@RosanaRufer RosanaRufer changed the title [Not ready yet] Fixed django#12203 -- Allow using ManyToManyField with through in Admin. Fixed django#12203 -- Allow using ManyToManyField with through in Admin. Sep 24, 2024
@RosanaRufer RosanaRufer marked this pull request as ready for review September 24, 2024 13:57
@agas0077
Copy link
Copy Markdown

is there a way to know when this is going to be merged?

@nessita
Copy link
Copy Markdown
Contributor

nessita commented Jan 8, 2025

is there a way to know when this is going to be merged?

Hello @agas0077, thank you for your interest in this PR, and thank you @RosanaRufer and @collinanderson for getting started in this work. I will add some comments after a quick initial review pass:

  1. In order for this PR to be listed as "needing review" in the Django Development Dashboard, please remember to set the proper Trac flags in the ticket as described in the PR checklist when this is ready for review. So the "has patch" flag has to be set in the ticket (and the flags for needs docs/needs tests/needs improvements needs to be unset).
  2. Django is proud of its stability, both in APIs and functionality, ensuring that upgrades from one version to the next are seamless. This stability is maintained through a strict deprecation policy, which ensures that no backward-incompatible changes are introduced without a deprecation warning being issued during a two-release cycle. This gives users the opportunity to upgrade their systems properly.
    Since the proposed changes are backward-incompatible, they must be handled according to the guidelines outlined in these docs. Specifically, I think this work needs a deprecation warning that the system check is going to be removed.
  3. We need to add a robust set of tests for the admin interface, to demonstrate and ensure the correct behavior under various scenarios. This is particularly important in light of your comment, @collinanderson, regarding the possibility of data being removed in ways that were previously not possible.
  4. Lastly, this needs to include a release note.

Because of the above, I'll set the ticket as needs docs, needs tests and needs improvement. When the changes above are addressed, each flag should be unset in the ticket.

Once the above is completed, members of the Review and Triage team will provide a more in depth review of the proposal.

@nessita nessita changed the title Fixed django#12203 -- Allow using ManyToManyField with through in Admin. Fixed #12203 -- Allow using ManyToManyField with through in Admin. Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants