Opened 14 years ago

Closed 14 years ago

Last modified 12 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: dev
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: no UI/UX: no

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 14 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 14 years ago.
Correct tests failing on current trunk
12521.diff (18.9 KB ) - added by jkocherhans 14 years ago.
This combines my fixes, Honza fixes, and Ivan's tests. Almost there.
12521.2.diff (24.4 KB ) - added by jkocherhans 14 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 14 years ago.
Now with updated docs for model's validation methods.
12521.4.diff (42.1 KB ) - added by jkocherhans 14 years ago.
A rework based on conversations with Honza

Download all attachments as: .zip

Change History (22)

comment:1 by jkocherhans, 14 years ago

Has patch: set
Owner: changed from nobody to jkocherhans
Status: newassigned

comment:2 by jkocherhans, 14 years ago

Triage Stage: UnreviewedDesign decision needed

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

comment:3 by EmilStenstrom, 14 years ago

Cc: em@… added

comment:4 by Jannis Leidel, 14 years ago

Cc: Jannis Leidel added

comment:5 by Knut Nesheim, 14 years ago

Cc: knutin@… added

by Honza Král, 14 years ago

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

comment:6 by zbyte64, 14 years ago

Cc: zbyte64@… added

comment:7 by Ivan Sagalaev, 14 years ago

Cc: Maniac@… added

comment:8 by Ivan Sagalaev, 14 years ago

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?

in reply to:  8 comment:9 by Honza Král, 14 years ago

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 by Adrian Ribao, 14 years ago

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

comment:11 by Adrian Ribao, 14 years ago

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.

by Ivan Sagalaev, 14 years ago

Attachment: 12521.tests.diff added

Correct tests failing on current trunk

comment:12 by Ivan Sagalaev, 14 years ago

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

by jkocherhans, 14 years ago

Attachment: 12521.diff added

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

comment:13 by jkocherhans, 14 years ago

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.

by jkocherhans, 14 years ago

Attachment: 12521.2.diff added

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

by jkocherhans, 14 years ago

Attachment: 12521.3.diff added

Now with updated docs for model's validation methods.

comment:14 by Ivan Sagalaev, 14 years ago

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

by jkocherhans, 14 years ago

Attachment: 12521.4.diff added

A rework based on conversations with Honza

comment:15 by Luke Plant, 14 years ago

Resolution: fixed
Status: assignedclosed

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

comment:16 by Jacob, 12 years ago

milestone: 1.2

Milestone 1.2 deleted

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