Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#16115 closed New feature (fixed)

New hook to allow change objects after all inline formsets have been saved

Reported by: Igor Sobreira Owned by: Jannis Leidel
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: 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

Today we have save_model() and save_formset() in ModelAdmin , but there is no default place where we can change objects after the object and all related formsets have been saved.

You can override response_change() and response_add() , but I don't think that's a good place to deal with model objects, since the purpose of these methods is to send messages to user and return the correct HttpResponse object.

I'm attaching a patch with the proposed hook, I think it's better to have a method that wraps save_model() and save_formet() than create an empty method in the end of add_view() and change_view() . But it's just a suggestion.

I can write docs if you like the idea.

Attachments (5)

new_hook_save_model_and_relations.diff (2.2 KB) - added by Igor Sobreira 5 years ago.
new_hook_save_model_and_related_with_tests.diff (7.6 KB) - added by Igor Sobreira 5 years ago.
new_hook_save_model_and_relations_with_tests_and_docs.diff (8.6 KB) - added by Igor Sobreira 5 years ago.
16115.modeladmin-save-related.diff (7.8 KB) - added by Julien Phalip 5 years ago.
16115.modeladmin-save-related.2.diff (7.6 KB) - added by Julien Phalip 5 years ago.
Patch updated to latest trunk

Download all attachments as: .zip

Change History (19)

Changed 5 years ago by Igor Sobreira

comment:1 Changed 5 years ago by Igor Sobreira

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Version: 1.3SVN

comment:2 Changed 5 years ago by Luke Plant

Type: Cleanup/optimizationNew feature

comment:3 Changed 5 years ago by Julien Phalip

Needs documentation: set
Needs tests: set
Triage Stage: UnreviewedAccepted

This makes sense and was also requested in #16016 (closed as dupe). "save_model_and_relations" feels quite verbose but I can't think of a better name right this minute. It'd be useful if you could provide tests and doc so this ticket can be moved along.

comment:4 Changed 5 years ago by Igor Sobreira

Owner: changed from nobody to Igor Sobreira
Status: newassigned

Changed 5 years ago by Igor Sobreira

comment:5 Changed 5 years ago by Igor Sobreira

Thanks for the feedback julien. I agree with you that the method name is too long, specially because it has an "and". I'm not very good on giving names. What about "save_all", or "save_all_objects"?

I've added a new patch with tests, there is a unit test mocking external calls to make sure it calls the correct methods, and a more high level test, to make sure I can do what I want, that is edit all related objects on add_view and change_view.

I'll work on some docs for it later.

comment:6 Changed 5 years ago by Igor Sobreira

Needs documentation: unset
Needs tests: unset

Changed 5 years ago by Julien Phalip

comment:7 Changed 5 years ago by Julien Phalip

Thank you for your patch. I've made a number of modifications to it:

  • Renamed the hook to save_related().
  • Took the call to save_model() out of that hook so that it focuses solely on form/formset saving business.
  • Removed the test that was in regressiontests.modeladmin as it was testing internal implementation details, which is best to avoid doing. The other test verifies the external behaviour, which is enough here.
  • Tweaked the doc and added release notes.

Please take a look and see whether this addresses your original request. Thanks.

comment:8 Changed 5 years ago by Igor Sobreira

Great julien, your changes make a lot of sense. I was really in doubt about that unit test since i havent's seen other like that. Thanks!

comment:9 Changed 5 years ago by Igor Sobreira

Owner: changed from Igor Sobreira to Julien Phalip
Status: assignednew

julien, i'm reassigning the ticket to you, I think you pretty much finished it. (actually I don't have permission :)

Last edited 5 years ago by Igor Sobreira (previous) (diff)

comment:10 Changed 5 years ago by Julien Phalip

Owner: Julien Phalip deleted

Igors, no need to assign it to me ;)

I too feel that the work is pretty much done, but since I came up with a new API in the latest patch I'd prefer to get at least another person to cast their eyes on it before RFC'ing this ticket, just in case we missed something.

Changed 5 years ago by Julien Phalip

Patch updated to latest trunk

comment:11 Changed 5 years ago by Preston Timmons

Triage Stage: AcceptedReady for checkin
UI/UX: unset

I applied and reviewed the patch at r16452. It looks good to me. Updating to RFC.

comment:12 Changed 5 years ago by Jannis Leidel

Owner: set to Jannis Leidel
Resolution: fixed
Status: newclosed

In [16498]:

Fixed #16115 -- Added ModelAdmin.save_related method to be able to do pre- or post-save operations for objects related to the parent object currently displayed. Thanks, Julien Phalip.

comment:13 Changed 5 years ago by nivhaa@…

Thanks guys, save_related is a great thing I've been waiting for a long time. But it seems there's something missing. Assuming that the inlines are part of the object, let's say for example, that I've edited the object and the inlines through the admin page and hit the "save" button. now I want to do some processing on the object itself after the inlines were saved. calling save_model won't help, as the inlines haven't been saved yet. wouldn't it be very useful to also pass "obj" to save_realted arguments?

comment:14 Changed 5 years ago by Julien Phalip

@nivhaa: You should be able to access the main object via form.instance from inside save_related().

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