Opened 5 years ago

Closed 5 years ago

Last modified 4 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@…, jezdez, 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_Kral 5 years ago.
Simple quick patch implementing desired behavior, just a case study, still needs work
12521.tests.diff (7.0 KB) - added by isagalaev 5 years ago.
Correct tests failing on current trunk
12521.diff (18.9 KB) - added by jkocherhans 5 years ago.
This combines my fixes, Honza fixes, and Ivan's tests. Almost there.
12521.2.diff (24.4 KB) - added by jkocherhans 5 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 5 years ago.
Now with updated docs for model's validation methods.
12521.4.diff (42.1 KB) - added by jkocherhans 5 years ago.
A rework based on conversations with Honza

Download all attachments as: .zip

Change History (22)

comment:1 Changed 5 years ago by jkocherhans

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to jkocherhans
  • Patch needs improvement unset
  • Status changed from new to assigned

comment:2 Changed 5 years ago by jkocherhans

  • Triage Stage changed from Unreviewed to Design decision needed

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

comment:3 Changed 5 years ago by EmilStenstrom

  • Cc em@… added

comment:4 Changed 5 years ago by jezdez

  • Cc jezdez added

comment:5 Changed 5 years ago by knutin

  • Cc knutin@… added

Changed 5 years ago by Honza_Kral

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

comment:6 Changed 5 years ago by zbyte64

  • Cc zbyte64@… added

comment:7 Changed 5 years ago by isagalaev

  • Cc Maniac@… added

comment:8 follow-up: Changed 5 years ago by isagalaev

  • Needs tests set
  • Owner changed from jkocherhans to isagalaev
  • Status changed from assigned to new

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

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

  • Cc aribao@… added
  • Owner changed from isagalaev to tolano
  • Status changed from new to assigned

comment:11 Changed 5 years ago by tolano

  • Owner changed from tolano to isagalaev
  • Status changed from assigned to new

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

Changed 5 years ago by isagalaev

Correct tests failing on current trunk

comment:12 Changed 5 years ago by isagalaev

  • Owner changed from isagalaev 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 5 years ago by jkocherhans

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

comment:13 Changed 5 years ago by jkocherhans

  • Status changed from new to assigned

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

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

Changed 5 years ago by jkocherhans

Now with updated docs for model's validation methods.

comment:14 Changed 5 years ago by isagalaev

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

Changed 5 years ago by jkocherhans

A rework based on conversations with Honza

comment:15 Changed 5 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from assigned to closed

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

comment:16 Changed 4 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

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