Opened 7 years ago

Closed 7 years ago

Last modified 5 years ago

#12521 closed (fixed)

ModelAdmin save_model request.user idiom is broken by model validation

Reported by: simon Owned by: jkocherhans
Component: contrib.admin Version: master
Severity: Keywords:
Cc: em@…, Jannis Leidel, knutin@…, zbyte64@…, Maniac@…, aribao@… Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

Description

The common (documented) pattern for automatically setting the "author" field on an object to request.user by over-riding the save_model method on a ModelAdmin subclass breaks with the new model validation code. Here are the example models.py and admin.py files:

from django.db import models

class Entry(models.Model):
    title = models.CharField(max_length = 255)
    body = models.TextField()
    published = models.DateTimeField(auto_now_add = True)
    author = models.ForeignKey('auth.User')
    
    def __unicode__(self):
        return self.title

admin.py:

from models import Entry
from django.contrib import admin

class EntryAdmin(admin.ModelAdmin):
    list_display = ('title', 'published', 'author')
    exclude = ('author',)
    
    def save_model(self, request, obj, form, change):
        obj.author = request.user
        obj.save()

admin.site.register(Entry, EntryAdmin)

And here's the exception:

Traceback:
File "/Users/simon/Development/playing-with-django-1.2/ve/lib/python2.6/site-packages/django/core/handlers/base.py" in get_response
  99.                     response = callback(request, *callback_args, **callback_kwargs)
File "/Users/simon/Development/playing-with-django-1.2/ve/lib/python2.6/site-packages/django/contrib/admin/options.py" in wrapper
  237.                 return self.admin_site.admin_view(view)(*args, **kwargs)
File "/Users/simon/Development/playing-with-django-1.2/ve/lib/python2.6/site-packages/django/utils/decorators.py" in __call__
  36.         return self.decorator(self.func)(*args, **kwargs)
File "/Users/simon/Development/playing-with-django-1.2/ve/lib/python2.6/site-packages/django/utils/decorators.py" in _wrapped_view
  86.                     response = view_func(request, *args, **kwargs)
File "/Users/simon/Development/playing-with-django-1.2/ve/lib/python2.6/site-packages/django/utils/decorators.py" in __call__
  36.         return self.decorator(self.func)(*args, **kwargs)
File "/Users/simon/Development/playing-with-django-1.2/ve/lib/python2.6/site-packages/django/views/decorators/cache.py" in _wrapped_view_func
  70.         response = view_func(request, *args, **kwargs)
File "/Users/simon/Development/playing-with-django-1.2/ve/lib/python2.6/site-packages/django/contrib/admin/sites.py" in inner
  187.             return view(request, *args, **kwargs)
File "/Users/simon/Development/playing-with-django-1.2/ve/lib/python2.6/site-packages/django/utils/decorators.py" in _wrapped_view
  86.                     response = view_func(request, *args, **kwargs)
File "/Users/simon/Development/playing-with-django-1.2/ve/lib/python2.6/site-packages/django/db/transaction.py" in _commit_on_success
  295.                     res = func(*args, **kw)
File "/Users/simon/Development/playing-with-django-1.2/ve/lib/python2.6/site-packages/django/contrib/admin/options.py" in add_view
  760.             if form.is_valid():
File "/Users/simon/Development/playing-with-django-1.2/ve/lib/python2.6/site-packages/django/forms/forms.py" in is_valid
  120.         return self.is_bound and not bool(self.errors)
File "/Users/simon/Development/playing-with-django-1.2/ve/lib/python2.6/site-packages/django/forms/forms.py" in _get_errors
  111.             self.full_clean()
File "/Users/simon/Development/playing-with-django-1.2/ve/lib/python2.6/site-packages/django/forms/forms.py" in full_clean
  286.             self.cleaned_data = self.clean()
File "/Users/simon/Development/playing-with-django-1.2/ve/lib/python2.6/site-packages/django/forms/models.py" in clean
  270.                 raise UnresolvableValidationError(e.message_dict)

Exception Type: UnresolvableValidationError at /admin/blog/entry/add/
Exception Value: 

The validation error is "This field cannot be null".

This pattern is documented here: http://docs.djangoproject.com/en/dev/ref/contrib/admin/#modeladmin-methods - we should at the very least update the documentation, but even having done that we'd still break existing code.

Attachments (6)

only_validate_fields_on_modelform_not_entire_model.diff (6.6 KB) - added by Honza Král 7 years ago.
Simple quick patch implementing desired behavior, just a case study, still needs work
12521.tests.diff (7.0 KB) - added by Ivan Sagalaev 7 years ago.
Correct tests failing on current trunk
12521.diff (18.9 KB) - added by jkocherhans 7 years ago.
This combines my fixes, Honza fixes, and Ivan's tests. Almost there.
12521.2.diff (24.4 KB) - added by jkocherhans 7 years ago.
Similar to the last patch, but splits full_validate into validate_fields, validate_unique, validate.
12521.3.diff (27.5 KB) - added by jkocherhans 7 years ago.
Now with updated docs for model's validation methods.
12521.4.diff (42.1 KB) - added by jkocherhans 7 years ago.
A rework based on conversations with Honza

Download all attachments as: .zip

Change History (22)

comment:1 Changed 7 years ago by jkocherhans

Has patch: set
Needs documentation: unset
Needs tests: unset
Owner: changed from nobody to jkocherhans
Patch needs improvement: unset
Status: newassigned

comment:2 Changed 7 years ago by jkocherhans

Triage Stage: UnreviewedDesign decision needed

See #12513 and this thread on django-dev for more info.

comment:3 Changed 7 years ago by EmilStenstrom

Cc: em@… added

comment:4 Changed 7 years ago by Jannis Leidel

Cc: Jannis Leidel added

comment:5 Changed 7 years ago by Knut Nesheim

Cc: knutin@… added

Changed 7 years ago by Honza Král

Simple quick patch implementing desired behavior, just a case study, still needs work

comment:6 Changed 7 years ago by zbyte64

Cc: zbyte64@… added

comment:7 Changed 7 years ago by Ivan Sagalaev

Cc: Maniac@… added

comment:8 Changed 7 years ago by Ivan Sagalaev

Needs tests: set
Owner: changed from jkocherhans to Ivan Sagalaev
Status: assignednew

I'm about to add some tests to jkocherhans' patch. Honza, your patch is supposedly does the same thing, can you comment if you want yours as a replacement or not?

comment:9 in reply to:  8 Changed 7 years ago by Honza Král

Replying to isagalaev:

I'm about to add some tests to jkocherhans' patch. Honza, your patch is supposedly does the same thing, can you comment if you want yours as a replacement or not?

yes, the only problem with my patch, afaik, i sthat it only looks at exclude and not in at fields in django/forms/models.py line 252. As I said earlier, it's a quick patch (part of it copy paste, which is plain wrong) just to show the direction to go.

comment:10 Changed 7 years ago by Adrian Ribao

Cc: aribao@… added
Owner: changed from Ivan Sagalaev to Adrian Ribao
Status: newassigned

comment:11 Changed 7 years ago by Adrian Ribao

Owner: changed from Adrian Ribao to Ivan Sagalaev
Status: assignednew

What happened? I didn't change the owner nor the status! I'll try to set it back.

Changed 7 years ago by Ivan Sagalaev

Attachment: 12521.tests.diff added

Correct tests failing on current trunk

comment:12 Changed 7 years ago by Ivan Sagalaev

Owner: changed from Ivan Sagalaev to jkocherhans

Added a couple of tests that fail with current trunk. Some comments:

  • admin_views tests are testing UserCreationForm from contrib.auth. It used not to break after model-validation merge because it was correcting model instance as a side effect of validation. It's not rewritten in more idiomatic way applying its non-form data in save() (and currently it correctly fails due to missing "password" field in the form)
  • model_forms tests are exercising different ways to set field on a ModelForm and also make sure that the form still provides an incomplete and unsaved instance of a model

Changed 7 years ago by jkocherhans

Attachment: 12521.diff added

This combines my fixes, Honza fixes, and Ivan's tests. Almost there.

comment:13 Changed 7 years ago by jkocherhans

Status: newassigned

My current patch is almost there I think. I still need to talk to Honza about where the model.validate() hook should be called, but other than that I thinks it's ready. It also fixes #12520, #12537, and #12552.

Changed 7 years ago by jkocherhans

Attachment: 12521.2.diff added

Similar to the last patch, but splits full_validate into validate_fields, validate_unique, validate.

Changed 7 years ago by jkocherhans

Attachment: 12521.3.diff added

Now with updated docs for model's validation methods.

comment:14 Changed 7 years ago by Ivan Sagalaev

Confirming that current patch fixes my use-case with PreviewForm.

Changed 7 years ago by jkocherhans

Attachment: 12521.4.diff added

A rework based on conversations with Honza

comment:15 Changed 7 years ago by Luke Plant

Resolution: fixed
Status: assignedclosed

This was fixed in [12206] by Joseph Kocherhans (typo in commit message says #12512 instead of #12521).

comment:16 Changed 5 years ago by Jacob

milestone: 1.2

Milestone 1.2 deleted

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