Opened 13 years ago

Closed 13 years ago

#14482 closed (duplicate)

better documentation of flow of save in admin

Reported by: Jonathan Morgan 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: no UI/UX: no

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

Change History (2)

comment:1 by Gabriel Hurley, 13 years ago

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 by Gabriel Hurley, 13 years ago

Resolution: duplicate
Status: newclosed

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

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