Opened 6 years ago

Last modified 4 years ago

#11830 new Bug

save_model in ModelAdmin and inline model formsets

Reported by: nfg Owned by: nobody
Component: contrib.admin Version: master
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


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 5 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 5 years ago.
updated patch (with tests only) to apply to current trunk
save_model_validation_error.diff (13.1 KB) - added by nfg 5 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 6 years ago by nfg

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 Changed 5 years ago by russellm

  • milestone set to 1.2
  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 5 years ago by tobias

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

comment:4 Changed 5 years ago by tobias

  • Owner changed from tobias to nobody
  • Patch needs improvement set
  • Status changed from assigned to new

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?

Changed 5 years ago by tobias

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

Changed 5 years ago by tobias

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

Changed 5 years ago by nfg

comment:5 Changed 5 years ago by nfg

  • Version changed from 1.1 to SVN

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 Changed 5 years ago by tobias

  • milestone changed from 1.2 to 1.3

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

comment:7 Changed 4 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:8 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:11 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:12 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

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