Opened 15 years ago

Closed 8 years ago

#11830 closed Bug (needsinfo)

save_model in ModelAdmin and inline model formsets

Reported by: Nils Fredrik Gjerull Owned by: nobody
Component: contrib.admin Version: dev
Severity: Normal Keywords: save_model save
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I tried to use the save_model in ModelAdmin to add and remove some related objects for the saved instance. But because save_formset is called after save_model a ValidationError is emitted. This happens because my code removes a related object expected by a BaseInlineFormSet. The reason I use save_model and not the save method on the model is because I need access to the request object. If, however, I did this in the save method of the model the error will still be the same. save_formset is called after save_model which call the save method on the model instance. If you remove a related object to a model instance expected by one of ModelAdmin's related formsets you get an error.

I have supplied a patch containing a test that trigger this error. I have also included a rater naive fix, which simply ignores related objects triggering ValidationError in save_existing_object. The exception is emitted in a context where it is not caught by the form handling code.

Attachments (3)

save_model_validation_error_tests_only.diff (7.2 KB ) - added by Tobias McNulty 14 years ago.
updated patch (with tests only) to apply to current trunk
save_model_validation_error_tests_only.2.diff (11.5 KB ) - added by Tobias McNulty 14 years ago.
updated patch (with tests only) to apply to current trunk
save_model_validation_error.diff (13.1 KB ) - added by Nils Fredrik Gjerull 14 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 by Nils Fredrik Gjerull, 15 years ago

Perhaps a hook like save_model should be added (post_save_model?). This would be called after the save_formsets have been called on the formsets. This will however not solve the problem of changing or deleting related objects from the model's save method.

I also discovered a problem with my naiv solution. It makes the data submitted in the form the master, data changed in the save method or the save_model method will be overwritten. It is a design decision whether the save method or the form data should be the master. Added a test for this in the patch. The test assumes the form data is the master.

comment:2 by Russell Keith-Magee, 14 years ago

milestone: 1.2
Triage Stage: UnreviewedAccepted

comment:3 by Tobias McNulty, 14 years ago

Owner: changed from nobody to Tobias McNulty
Status: newassigned

comment:4 by Tobias McNulty, 14 years ago

Owner: changed from Tobias McNulty to nobody
Patch needs improvement: set
Status: assignednew

With a little work I updated the tests to apply to trunk. At this point it still appears that two of the tests are breaking before the original author intended them to.

nfg could you take a look and try to update the patch to apply to current trunk, and/or indicate if this bug still exists?

by Tobias McNulty, 14 years ago

updated patch (with tests only) to apply to current trunk

by Tobias McNulty, 14 years ago

updated patch (with tests only) to apply to current trunk

by Nils Fredrik Gjerull, 14 years ago

comment:5 by Nils Fredrik Gjerull, 14 years ago

Version: 1.1SVN

I think I need to explain my use case. A client I worked for needed to update organization information from a central registry. I made a Organization model, and some related models, and a OrganizationAdmin class. I then added a button to the change_form view called 'Update from registry' (in addition to the save buttons). This allowed the client to just enter the official organization number and the rest would be retrieved from the registry. One of the many things I did in order to implement this was to change some related model instances in the OrganizationAdmin's save_model method. This, however, triggered a ValidationError. This happens because the formsets for the related models are validated after the model has been saved. So it was my intention that test_admin_save_model_method and test_model_save_method should fail with ValidationError('Select a valid choice. That choice is not one of the available choices.'). I managed to implement my use case by doing some minor trickery in the ModelAdmin save_formset method.

It is my opinion that changes made to the model instance, or to related model instances, in the Model save method or in the ModelAdmin save_model method should have precedence over the data in a Form. The test just expose some of the limitation in what you can do in the save and save_model method when django.contrib.admin is used. Unfortunately I have not found a good solution for this. I think this would require rewriting parts of the model-form validation code, and should probably not be included in the 1.2 release.

I have updated the patch to make my point clearer.

comment:6 by Tobias McNulty, 14 years ago

milestone: 1.21.3

bumping to 1.3 per nfg and kmtracey - re-writing model form validation is not going to happen in 1.2

comment:7 by Julien Phalip, 13 years ago

Severity: Normal
Type: Bug

comment:8 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

comment:11 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:12 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:13 by Tim Graham, 8 years ago

Resolution: needsinfo
Status: newclosed

This ticket hasn't seen any activity for 6 years and it's not obvious to me at this point how to move it forward.

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