Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#27998 closed Bug (fixed)

LogEntry messages do not list m2m fields that were changed when an object is changed via ModelAdmin

Reported by: ljsjl Owned by: ljsjl
Component: contrib.admin Version: 1.10
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

What I expect to happen: m2m fields that are changed via a ModelAdmin in an update are listed in the appropriate change message in that object's history.
What happens: m2m fields are not listed in the generated change message

Reproduction: Create a vanilla Django install. Create a group. Assign a permission to that group. View the group's history, the message will say "No fields changed."

Cause: It looks like save_related() is called before form.has_changed() or form.changed_data are evaluated. save_related() calls form.save_m2m() which updates the relationship table. At this point it looks like querysets in the ModelForm form.initial have not been evaluated, so when they are evaluated as part of constructing the change message the database has already been modified by form.save_m2m(), and so the fields are left out of form.changed_data.

Fix: Probably call form.has_changed() at some point before save_related(). Happy to put together a patch for changelist_view and changeform_view if needed and attach to this ticket.

Change History (8)

comment:1 by Tim Graham, 8 years ago

Triage Stage: UnreviewedAccepted

If you're able to, submitting a pull request to GitHub is the best way to submit a patch.

comment:2 by ljsjl, 8 years ago

Owner: changed from nobody to ljsjl
Status: newassigned
Last edited 8 years ago by Tim Graham (previous) (diff)

comment:3 by ljsjl, 8 years ago

Has patch: set

comment:4 by Tim Graham <timograham@…>, 7 years ago

Resolution: fixed
Status: assignedclosed

In 15b465c:

Fixed #27998 -- Made ManyToManyField changes logged in admin's object history.

comment:5 by Tim Graham, 7 years ago

I found that this is a regression in Django 1.10 due to ded502024191053475bac811d365ac29dca1db61 and therefore should be fixed in the current stable branch, 1.11.x. The fix for #28543 fixes this issue in a better way (at the form level, rather than only in the admin) but I'm concerned that the approach I've proposed there, changing the return type of ManyToManyField.value_from_object(), is too risky for a stable release. I've proposed an alternate PR that modifies model_to_dict() instead. It restores the behavior of model_to_dict() returning a list for ManyToManyField from before 67d984413c9540074e4fe6aa033081a35cf192bc.

comment:6 by GitHub <noreply@…>, 7 years ago

In e5bd585:

Fixed #28543 -- Prevented ManyToManyField.value_from_object() from being lazy.

Previously, it was a QuerySet which could reevaluate to a new value if the
model's data changes. This is inconsistent with other Field.value_from_object()
methods.

This allows reverting the fix in the admin for refs #27998.

comment:7 by Tim Graham <timograham@…>, 7 years ago

In 20c0339:

[1.11.x] Fixed #27998, #28543 -- Restored logging of ManyToManyField changes in admin's object history.

And prevented ManyToManyField initial data in model forms from being affected
by subsequent model changes.

Regression in 56a55566a791a11420fe96f745b7489e756fc931.

Partial backport of e5bd585c6eb1e13e2f8aac030b33c077b0b70c05 and
15b465c584f49a1d43b6c18796f83521ee4ffc22 from master

comment:8 by ljsjl, 7 years ago

Huh, guess I should have followed my initial fix further down the rabbit hole to Field rather than ModelAdmin behaviour. Thanks to Wonder and Tim for a better fix.

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