Fixed #5780 -- Adjusted the ModelAdmin API to allow the created/updated objects

to be passed to the formsets prior to validation.

This is a backward incompatible change for anyone overridding save_add or
save_change. They have been removed in favor of more granular methods
introduced in [8266] and the new response_add and response_change nethods.
save_model has been renamed to save_form due to its slightly changed behavior.


git-svn-id: http://code.djangoproject.com/svn/django/trunk@8273 bcc190cf-cafb-0310-a4f2-bffc1f526a37

django/contrib/admin/options.py

@@ -452,102 +452,18 @@ class ModelAdmin(BaseModelAdmin):
         """
         request.user.message_set.create(message=message)
 
-    def save_model(self, request, form, change):
+    def save_form(self, request, form, change):
         """
-        Save and return a model given a ModelForm. ``change`` is True if the
+        Given a ModelForm return an unsaved instance. ``change`` is True if
-        object is being changed, and False if it's being added.
+        the object is being changed, and False if it's being added.
         """
-        return form.save(commit=True)
+        return form.save(commit=False)
 
     def save_formset(self, request, form, formset, change):
         """
-        Save an inline formset attached to the object.
+        Given an inline formset return unsaved instances.
         """
-        formset.save()
+        return formset.save(commit=False)
-
-    def save_add(self, request, form, formsets, post_url_continue):
-        """
-        Saves the object in the "add" stage and returns an HttpResponseRedirect.
-
-        `form` is a bound Form instance that's verified to be valid.
-        """
-        opts = self.model._meta
-        
-        new_object = self.save_model(request, form, change=False)
-        if formsets:
-            for formset in formsets:
-                formset.instance = new_object
-                self.save_formset(request, form, formset, change=False)
-
-        pk_value = new_object._get_pk_val()
-        self.log_addition(request, new_object)
-                
-        msg = _('The %(name)s "%(obj)s" was added successfully.') % {'name': force_unicode(opts.verbose_name), 'obj': force_unicode(new_object)}
-        # Here, we distinguish between different save types by checking for
-        # the presence of keys in request.POST.
-        if request.POST.has_key("_continue"):
-            self.message_user(request, msg + ' ' + _("You may edit it again below."))
-            if request.POST.has_key("_popup"):
-                post_url_continue += "?_popup=1"
-            return HttpResponseRedirect(post_url_continue % pk_value)
-
-        if request.POST.has_key("_popup"):
-            return HttpResponse('<script type="text/javascript">opener.dismissAddAnotherPopup(window, "%s", "%s");</script>' % \
-                # escape() calls force_unicode.
-                (escape(pk_value), escape(new_object)))
-        elif request.POST.has_key("_addanother"):
-            self.message_user(request, msg + ' ' + (_("You may add another %s below.") % force_unicode(opts.verbose_name)))
-            return HttpResponseRedirect(request.path)
-        else:
-            self.message_user(request, msg)
-
-            # Figure out where to redirect. If the user has change permission,
-            # redirect to the change-list page for this object. Otherwise,
-            # redirect to the admin index.
-            if self.has_change_permission(request, None):
-                post_url = '../'
-            else:
-                post_url = '../../../'
-            return HttpResponseRedirect(post_url)
-    save_add = transaction.commit_on_success(save_add)
-
-    def save_change(self, request, form, formsets=None):
-        """
-        Saves the object in the "change" stage and returns an HttpResponseRedirect.
-
-        `form` is a bound Form instance that's verified to be valid.
-
-        `formsets` is a sequence of InlineFormSet instances that are verified to be valid.
-        """
-        opts = self.model._meta
-        new_object = self.save_model(request, form, change=True)
-        pk_value = new_object._get_pk_val()
-
-        if formsets:
-            for formset in formsets:
-                self.save_formset(request, form, formset, change=True)
-        
-        change_message = self.construct_change_message(request, form, formsets)
-        self.log_change(request, new_object, change_message)        
-
-        msg = _('The %(name)s "%(obj)s" was changed successfully.') % {'name': force_unicode(opts.verbose_name), 'obj': force_unicode(new_object)}
-        if request.POST.has_key("_continue"):
-            self.message_user(request, msg + ' ' + _("You may edit it again below."))
-            if request.REQUEST.has_key('_popup'):
-                return HttpResponseRedirect(request.path + "?_popup=1")
-            else:
-                return HttpResponseRedirect(request.path)
-        elif request.POST.has_key("_saveasnew"):
-            msg = _('The %(name)s "%(obj)s" was added successfully. You may edit it again below.') % {'name': force_unicode(opts.verbose_name), 'obj': new_object}
-            self.message_user(request, msg)
-            return HttpResponseRedirect("../%s/" % pk_value)
-        elif request.POST.has_key("_addanother"):
-            self.message_user(request, msg + ' ' + (_("You may add another %s below.") % force_unicode(opts.verbose_name)))
-            return HttpResponseRedirect("../add/")
-        else:
-            self.message_user(request, msg)
-            return HttpResponseRedirect("../")
-    save_change = transaction.commit_on_success(save_change)
 
     def render_change_form(self, request, context, add=False, change=False, form_url='', obj=None):
         opts = self.model._meta
@@ -574,6 +490,66 @@ class ModelAdmin(BaseModelAdmin):
             "admin/%s/change_form.html" % app_label,
             "admin/change_form.html"
         ], context, context_instance=template.RequestContext(request))
+    
+    def response_add(self, request, obj, post_url_continue='../%s/'):
+        """
+        Determines the HttpResponse for the add_view stage.
+        """
+        opts = obj._meta
+        pk_value = obj._get_pk_val()
+        
+        msg = _('The %(name)s "%(obj)s" was added successfully.') % {'name': force_unicode(opts.verbose_name), 'obj': force_unicode(obj)}
+        # Here, we distinguish between different save types by checking for
+        # the presence of keys in request.POST.
+        if request.POST.has_key("_continue"):
+            self.message_user(request, msg + ' ' + _("You may edit it again below."))
+            if request.POST.has_key("_popup"):
+                post_url_continue += "?_popup=1"
+            return HttpResponseRedirect(post_url_continue % pk_value)
+        
+        if request.POST.has_key("_popup"):
+            return HttpResponse('<script type="text/javascript">opener.dismissAddAnotherPopup(window, "%s", "%s");</script>' % \
+                # escape() calls force_unicode.
+                (escape(pk_value), escape(obj)))
+        elif request.POST.has_key("_addanother"):
+            self.message_user(request, msg + ' ' + (_("You may add another %s below.") % force_unicode(opts.verbose_name)))
+            return HttpResponseRedirect(request.path)
+        else:
+            self.message_user(request, msg)
+
+            # Figure out where to redirect. If the user has change permission,
+            # redirect to the change-list page for this object. Otherwise,
+            # redirect to the admin index.
+            if self.has_change_permission(request, None):
+                post_url = '../'
+            else:
+                post_url = '../../../'
+            return HttpResponseRedirect(post_url)
+    
+    def response_change(self, request, obj):
+        """
+        Determines the HttpResponse for the change_view stage.
+        """
+        opts = obj._meta
+        pk_value = obj._get_pk_val()
+        
+        msg = _('The %(name)s "%(obj)s" was changed successfully.') % {'name': force_unicode(opts.verbose_name), 'obj': force_unicode(obj)}
+        if request.POST.has_key("_continue"):
+            self.message_user(request, msg + ' ' + _("You may edit it again below."))
+            if request.REQUEST.has_key('_popup'):
+                return HttpResponseRedirect(request.path + "?_popup=1")
+            else:
+                return HttpResponseRedirect(request.path)
+        elif request.POST.has_key("_saveasnew"):
+            msg = _('The %(name)s "%(obj)s" was added successfully. You may edit it again below.') % {'name': force_unicode(opts.verbose_name), 'obj': obj}
+            self.message_user(request, msg)
+            return HttpResponseRedirect("../%s/" % pk_value)
+        elif request.POST.has_key("_addanother"):
+            self.message_user(request, msg + ' ' + (_("You may add another %s below.") % force_unicode(opts.verbose_name)))
+            return HttpResponseRedirect("../add/")
+        else:
+            self.message_user(request, msg)
+            return HttpResponseRedirect("../")
 
     def add_view(self, request, form_url='', extra_context=None):
         "The 'add' admin view for this model."
@@ -592,29 +568,44 @@ class ModelAdmin(BaseModelAdmin):
             post_url = '../../../'
 
         ModelForm = self.get_form(request)
-        inline_formsets = []
+        formsets = []
-        obj = self.model()
         if request.method == 'POST':
             form = ModelForm(request.POST, request.FILES)
+            if form.is_valid():
+                form_validated = True
+                new_object = self.save_form(request, form, change=False)
+            else:
+                form_validated = False
+                new_object = self.model()
             for FormSet in self.get_formsets(request):
-                inline_formset = FormSet(data=request.POST, files=request.FILES,
+                formset = FormSet(data=request.POST, files=request.FILES,
-                    instance=obj, save_as_new=request.POST.has_key("_saveasnew"))
+                                  instance=new_object,
-                inline_formsets.append(inline_formset)
+                                  save_as_new=request.POST.has_key("_saveasnew"))
-            if all_valid(inline_formsets) and form.is_valid():
+                formsets.append(formset)
-                return self.save_add(request, form, inline_formsets, '../%s/')
+            if all_valid(formsets) and form_validated:
+                new_object.save()
+                form.save_m2m()
+                for formset in formsets:
+                    instances = self.save_formset(request, form, formset, change=False)
+                    for instance in instances:
+                        instance.save()
+                    formset.save_m2m()
+                
+                self.log_addition(request, new_object)
+                return self.response_add(request, new_object)
         else:
             form = ModelForm(initial=dict(request.GET.items()))
             for FormSet in self.get_formsets(request):
-                inline_formset = FormSet(instance=obj)
+                formset = FormSet(instance=self.model())
-                inline_formsets.append(inline_formset)
+                formsets.append(formset)
 
         adminForm = AdminForm(form, list(self.get_fieldsets(request)), self.prepopulated_fields)
         media = self.media + adminForm.media
-        for fs in inline_formsets:
+        for formset in formsets:
-            media = media + fs.media
+            media = media + formset.media
 
         inline_admin_formsets = []
-        for inline, formset in zip(self.inline_instances, inline_formsets):
+        for inline, formset in zip(self.inline_instances, formsets):
             fieldsets = list(inline.get_fieldsets(request))
             inline_admin_formset = InlineAdminFormSet(inline, formset, fieldsets)
             inline_admin_formsets.append(inline_admin_formset)
@@ -626,11 +617,12 @@ class ModelAdmin(BaseModelAdmin):
             'show_delete': False,
             'media': mark_safe(media),
             'inline_admin_formsets': inline_admin_formsets,
-            'errors': AdminErrorList(form, inline_formsets),
+            'errors': AdminErrorList(form, formsets),
             'root_path': self.admin_site.root_path,
         }
         context.update(extra_context or {})
         return self.render_change_form(request, context, add=True)
+    add_view = transaction.commit_on_success(add_view)
 
     def change_view(self, request, object_id, extra_context=None):
         "The 'change' admin view for this model."
@@ -656,26 +648,43 @@ class ModelAdmin(BaseModelAdmin):
             return self.add_view(request, form_url='../../add/')
 
         ModelForm = self.get_form(request, obj)
-        inline_formsets = []
+        formsets = []
         if request.method == 'POST':
             form = ModelForm(request.POST, request.FILES, instance=obj)
-            for FormSet in self.get_formsets(request, obj):
+            if form.is_valid():
-                inline_formset = FormSet(request.POST, request.FILES, instance=obj)
+                form_validated = True
-                inline_formsets.append(inline_formset)
+                new_object = self.save_form(request, form, change=True)
+            else:
+                form_validated = False
+                new_object = obj
+            for FormSet in self.get_formsets(request, new_object):
+                formset = FormSet(request.POST, request.FILES,
+                                  instance=new_object)
+                formsets.append(formset)
 
-            if all_valid(inline_formsets) and form.is_valid():
+            if all_valid(formsets) and form_validated:
-                return self.save_change(request, form, inline_formsets)
+                new_object.save()
+                form.save_m2m()
+                for formset in formsets:
+                    instances = self.save_formset(request, form, formset, change=True)
+                    for instance in instances:
+                        instance.save()
+                    formset.save_m2m()
+                
+                change_message = self.construct_change_message(request, form, formsets)
+                self.log_change(request, new_object, change_message)
+                return self.response_change(request, new_object)
         else:
             form = ModelForm(instance=obj)
             for FormSet in self.get_formsets(request, obj):
-                inline_formset = FormSet(instance=obj)
+                formset = FormSet(instance=obj)
-                inline_formsets.append(inline_formset)
+                formsets.append(formset)
 
         adminForm = AdminForm(form, self.get_fieldsets(request, obj), self.prepopulated_fields)
         media = self.media + adminForm.media
 
         inline_admin_formsets = []
-        for inline, formset in zip(self.inline_instances, inline_formsets):
+        for inline, formset in zip(self.inline_instances, formsets):
             fieldsets = list(inline.get_fieldsets(request, obj))
             inline_admin_formset = InlineAdminFormSet(inline, formset, fieldsets)
             inline_admin_formsets.append(inline_admin_formset)
@@ -689,11 +698,12 @@ class ModelAdmin(BaseModelAdmin):
             'is_popup': request.REQUEST.has_key('_popup'),
             'media': mark_safe(media),
             'inline_admin_formsets': inline_admin_formsets,
-            'errors': AdminErrorList(form, inline_formsets),
+            'errors': AdminErrorList(form, formsets),
             'root_path': self.admin_site.root_path,
         }
         context.update(extra_context or {})
         return self.render_change_form(request, context, change=True, obj=obj)
+    change_view = transaction.commit_on_success(change_view)
 
     def changelist_view(self, request, extra_context=None):
         "The 'change list' admin view for this model."

docs/admin.txt

@@ -521,6 +521,44 @@ with an operator:
     Performs a full-text match. This is like the default search method but uses
     an index. Currently this is only available for MySQL.
 
+``ModelAdmin`` methods
+----------------------
+
+``save_form(self, request, form, change)``
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The ``save_form`` method is given the ``HttpRequest``, a ``ModelForm``
+instance and a boolean value based on whether it is adding or changing the
+object.
+
+This method should return an unsaved instance. For example to attach
+``request.user`` to the object prior to saving::
+
+    class ArticleAdmin(admin.ModelAdmin):
+        def save_form(self, request, form, change):
+            instance = form.save(commit=False)
+            instance.user = request.user
+            return instance
+
+``save_formset(self, request, form, formset, change)``
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The ``save_formset`` method is given the ``HttpRequest``, the parent
+``ModelForm`` instance and a boolean value baesed on whether it is adding or
+changing the parent object.
+
+This method should return unsaved instances. These instances will later be
+saved to the database. By default the formset will only return instances that
+have changed. For example to attach ``request.user`` to each changed formset
+model instance::
+
+    class ArticleAdmin(admin.ModelAdmin):
+        def save_formset(self, request, form, formset, change):
+            instances = formset.save(commit=False)
+            for instance in instances:
+                instance.user = request.user
+            return instances
+
 ``ModelAdmin`` media definitions
 --------------------------------
 

tests/regressiontests/admin_views/models.py

@@ -20,6 +20,9 @@ class Article(models.Model):
     def __unicode__(self):
         return self.title
 
+class ArticleInline(admin.TabularInline):
+    model = Article
+
 class ArticleAdmin(admin.ModelAdmin):
     list_display = ('content', 'date')
     list_filter = ('date',)
@@ -61,5 +64,5 @@ class ModelWithStringPrimaryKey(models.Model):
 
 admin.site.register(Article, ArticleAdmin)
 admin.site.register(CustomArticle, CustomArticleAdmin)
-admin.site.register(Section)
+admin.site.register(Section, inlines=[ArticleInline])
 admin.site.register(ModelWithStringPrimaryKey)

tests/regressiontests/admin_views/tests.py

@@ -11,10 +11,90 @@ from django.utils.html import escape
 # local test models
 from models import Article, CustomArticle, Section, ModelWithStringPrimaryKey
 
+class AdminViewBasicTest(TestCase):
+    fixtures = ['admin-views-users.xml']
+    
+    def setUp(self):
+        self.client.login(username='super', password='secret')
+    
+    def tearDown(self):
+        self.client.logout()
+    
+    def testTrailingSlashRequired(self):
+        """
+        If you leave off the trailing slash, app should redirect and add it.
+        """
+        request = self.client.get('/test_admin/admin/admin_views/article/add')
+        self.assertRedirects(request,
+            '/test_admin/admin/admin_views/article/add/'
+        )
+    
+    def testBasicAddGet(self):
+        """
+        A smoke test to ensure GET on the add_view works.
+        """
+        response = self.client.get('/test_admin/admin/admin_views/section/add/')
+        self.failUnlessEqual(response.status_code, 200)
+    
+    def testBasicEditGet(self):
+        """
+        A smoke test to ensureGET on the change_view works.
+        """
+        response = self.client.get('/test_admin/admin/admin_views/section/1/')
+        self.failUnlessEqual(response.status_code, 200)
+    
+    def testBasicAddPost(self):
+        """
+        A smoke test to ensure POST on add_view works.
+        """
+        post_data = {
+            "name": u"Another Section",
+            # inline data
+            "article_set-TOTAL_FORMS": u"3",
+            "article_set-INITIAL_FORMS": u"0",
+        }
+        response = self.client.post('/test_admin/admin/admin_views/section/add/', post_data)
+        self.failUnlessEqual(response.status_code, 302) # redirect somewhere
+    
+    def testBasicEditPost(self):
+        """
+        A smoke test to ensure POST on edit_view works.
+        """
+        post_data = {
+            "name": u"Test section",
+            # inline data
+            "article_set-TOTAL_FORMS": u"4",
+            "article_set-INITIAL_FORMS": u"1",
+            "article_set-0-id": u"1",
+            # there is no title in database, give one here or formset
+            # will fail.
+            "article_set-0-title": u"Need a title.",
+            "article_set-0-content": u"<p>test content</p>",
+            "article_set-0-date_0": u"2008-03-18",
+            "article_set-0-date_1": u"11:54:58",
+            "article_set-1-id": u"",
+            "article_set-1-title": u"",
+            "article_set-1-content": u"",
+            "article_set-1-date_0": u"",
+            "article_set-1-date_1": u"",
+            "article_set-2-id": u"",
+            "article_set-2-title": u"",
+            "article_set-2-content": u"",
+            "article_set-2-date_0": u"",
+            "article_set-2-date_1": u"",
+            "article_set-3-id": u"",
+            "article_set-3-title": u"",
+            "article_set-3-content": u"",
+            "article_set-3-date_0": u"",
+            "article_set-3-date_1": u"",
+        }
+        response = self.client.post('/test_admin/admin/admin_views/section/1/', post_data)
+        self.failUnlessEqual(response.status_code, 302) # redirect somewhere
+
 def get_perm(Model, perm):
     """Return the permission object, for the Model"""
     ct = ContentType.objects.get_for_model(Model)
-    return Permission.objects.get(content_type=ct,codename=perm)
+    return Permission.objects.get(content_type=ct, codename=perm)
 
 class AdminViewPermissionsTest(TestCase):
     """Tests for Admin Views Permissions."""
@@ -77,19 +157,6 @@ class AdminViewPermissionsTest(TestCase):
                      'username': 'joepublic',
                      'password': 'secret'}
 
-    def testTrailingSlashRequired(self):
-        """
-        If you leave off the trailing slash, app should redirect and add it.
-        """
-        self.client.post('/test_admin/admin/', self.super_login)
-
-        request = self.client.get(
-            '/test_admin/admin/admin_views/article/add'
-        )
-        self.assertRedirects(request,
-            '/test_admin/admin/admin_views/article/add/'
-        )
-
     def testLogin(self):
         """
         Make sure only staff members can log in.