Code

Opened 4 years ago

Closed 3 years ago

#14482 closed (duplicate)

better documentation of flow of save in admin

Reported by: jonathanmorgan Owned by: nobody
Component: Documentation Version: 1.2
Severity: Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

It would be good to better document how save in the admin works. Specifically, when you save a model with M2M fields the admin does the following:

  • stores the containing model instance in the database.
  • if new rows in associated table are created, stores them in the database.
  • clears all associated rows for the relation.
  • re-adds all that are in the request.
  • does not update the association objects that are stored in the instance of the object you are updating (so to look at results once all associations are added, you need to go to the database, can't look at the associations for the field in the containing instance).

When processing M2M, the admin does not compare what is in the request to what is in the database, remove those that have been removed, add any that are new, and ignore those that have not changed. Instead it clears all, then adds back all that are in request (not a bad way to do it in terms of performance). However, this causes the admin to never fire m2m_changed "remove" signals, and it causes the admin to always fire m2m_changed "add" signals, even when there are no changes to an association.

If this is the best way to do this for performance, OK, but it isn't intuitive (especially that the admin would be implemented in such a way as to just not emit a certain type of signal) and it took me a few hours of debugging to figure it out since I couldn't find these details documented anywhere.

For people using these hooks to get around problems they can't address in post-save overriding in the admin (which occurs after model instance is saved, but before M2M are processed), this will be confusing. And in terms of implementation best practices, "remove" signals could be fired in user-defined form processing code and so the signal is valid, but the documentation should describe how the admin works so one knows where the three pre-post pairs of signals occur there (or, in this case, so one can note that the "remove" pair of signals is never fired by the current implementation of the admin, and so you probably should implement a remove handler, but it won't get invoked in the admin, so you'll have to implement your "removed" handling in such a way that it doesn't actually rely on getting a "remove" signal and can be invoked by "add"s and "clear"s instead).

Attachments (0)

Change History (2)

comment:1 Changed 3 years ago by gabrielhurley

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

could you provide a test case for your statement about the admin not sending the appropriate m2m_changed signals? That sounds like a bug to me and I'd like to take a closer look at it. If writing a test case is too much, then at least point me to the relevant sections of the admin code. Thanks!

comment:2 Changed 3 years ago by gabrielhurley

  • Resolution set to duplicate
  • Status changed from new to closed

After further research, this is a known bug, reported similarly in #13962 and originally in #6707. Documenting it wouldn't hurt, but I think at this point #6707 needs to be fixed. If #6707 can't or won't be fixed, then the documentation should be the resolution to that ticket instead.

I've started a thread on Django Developers here: http://groups.google.com/group/django-developers/browse_frm/thread/dbb3e87e951f47fd

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.