Code

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#16115 closed New feature (fixed)

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

Reported by: igors Owned by: jezdez
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 igors 3 years ago.
new_hook_save_model_and_related_with_tests.diff (7.6 KB) - added by igors 3 years ago.
new_hook_save_model_and_relations_with_tests_and_docs.diff (8.6 KB) - added by igors 3 years ago.
16115.modeladmin-save-related.diff (7.8 KB) - added by julien 3 years ago.
16115.modeladmin-save-related.2.diff (7.6 KB) - added by julien 3 years ago.
Patch updated to latest trunk

Download all attachments as: .zip

Change History (19)

Changed 3 years ago by igors

comment:1 Changed 3 years ago by igors

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Version changed from 1.3 to SVN

comment:2 Changed 3 years ago by lukeplant

  • Type changed from Cleanup/optimization to New feature

comment:3 Changed 3 years ago by julien

  • Needs documentation set
  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

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 3 years ago by igors

  • Owner changed from nobody to igors
  • Status changed from new to assigned

comment:5 Changed 3 years ago by igors

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 3 years ago by igors

  • Needs documentation unset
  • Needs tests unset

Changed 3 years ago by julien

comment:7 Changed 3 years ago by julien

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 3 years ago by igors

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 3 years ago by igors

  • Owner changed from igors to julien
  • Status changed from assigned to new

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

Last edited 3 years ago by igors (previous) (diff)

comment:10 Changed 3 years ago by julien

  • Owner julien 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 3 years ago by julien

Patch updated to latest trunk

comment:11 Changed 3 years ago by prestontimmons

  • Triage Stage changed from Accepted to Ready for checkin
  • UI/UX unset

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

comment:12 Changed 3 years ago by jezdez

  • Owner set to jezdez
  • Resolution set to fixed
  • Status changed from new to closed

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 3 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 3 years ago by julien

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

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.