save_model in ModelAdmin and inline model formsets

[Nils Fredrik Gjerull]

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.

[Nils Fredrik Gjerull]

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.

[Tobias McNulty]

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?

[Tobias McNulty]

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

[Tobias McNulty]

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

[Nils Fredrik Gjerull]

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.

[Tobias McNulty]

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

[Jacob]

Milestone 1.3 deleted

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Aymeric Augustin]

Change Easy pickings from NULL to False.

[Tim Graham]

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.