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

4 files changed, 226 insertions, 108 deletions

diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py
index 697ac0eff8..3128de4af7 100644
--- a/django/contrib/admin/options.py
+++ b/django/contrib/admin/options.py
@@ -452,37 +452,53 @@ 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
-        object is being changed, and False if it's being added.
+        Given a ModelForm return an unsaved instance. ``change`` is True if
+        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):
+    def render_change_form(self, request, context, add=False, change=False, form_url='', obj=None):
+        opts = self.model._meta
+        app_label = opts.app_label
+        ordered_objects = opts.get_ordered_objects()
+        context.update({
+            'add': add,
+            'change': change,
+            'has_add_permission': self.has_add_permission(request),
+            'has_change_permission': self.has_change_permission(request, obj),
+            'has_delete_permission': self.has_delete_permission(request, obj),
+            'has_file_field': True, # FIXME - this should check if form or formsets have a FileField,
+            'has_absolute_url': hasattr(self.model, 'get_absolute_url'),
+            'ordered_objects': ordered_objects,
+            'form_url': mark_safe(form_url),
+            'opts': opts,
+            'content_type_id': ContentType.objects.get_for_model(self.model).id,
+            'save_as': self.save_as,
+            'save_on_top': self.save_on_top,
+            'root_path': self.admin_site.root_path,
+        })
+        return render_to_response(self.change_form_template or [
+            "admin/%s/%s/change_form.html" % (app_label, opts.object_name.lower()),
+            "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/'):
         """
-        Saves the object in the "add" stage and returns an HttpResponseRedirect.
-
-        `form` is a bound Form instance that's verified to be valid.
+        Determines the HttpResponse for the add_view stage.
         """
-        opts = self.model._meta
+        opts = obj._meta
+        pk_value = obj._get_pk_val()
         
-        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)}
+        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"):
@@ -490,11 +506,11 @@ class ModelAdmin(BaseModelAdmin):
             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)))
+                (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)
@@ -509,28 +525,15 @@ class ModelAdmin(BaseModelAdmin):
             else:
                 post_url = '../../../'
             return HttpResponseRedirect(post_url)
-    save_add = transaction.commit_on_success(save_add)
-
-    def save_change(self, request, form, formsets=None):
+    
+    def response_change(self, request, obj):
         """
-        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.
+        Determines the HttpResponse for the change_view stage.
         """
-        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)
+        opts = obj._meta
+        pk_value = obj._get_pk_val()
         
-        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)}
+        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'):
@@ -538,7 +541,7 @@ class ModelAdmin(BaseModelAdmin):
             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}
+            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"):
@@ -547,33 +550,6 @@ class ModelAdmin(BaseModelAdmin):
         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
-        app_label = opts.app_label
-        ordered_objects = opts.get_ordered_objects()
-        context.update({
-            'add': add,
-            'change': change,
-            'has_add_permission': self.has_add_permission(request),
-            'has_change_permission': self.has_change_permission(request, obj),
-            'has_delete_permission': self.has_delete_permission(request, obj),
-            'has_file_field': True, # FIXME - this should check if form or formsets have a FileField,
-            'has_absolute_url': hasattr(self.model, 'get_absolute_url'),
-            'ordered_objects': ordered_objects,
-            'form_url': mark_safe(form_url),
-            'opts': opts,
-            'content_type_id': ContentType.objects.get_for_model(self.model).id,
-            'save_as': self.save_as,
-            'save_on_top': self.save_on_top,
-            'root_path': self.admin_site.root_path,
-        })
-        return render_to_response(self.change_form_template or [
-            "admin/%s/%s/change_form.html" % (app_label, opts.object_name.lower()),
-            "admin/%s/change_form.html" % app_label,
-            "admin/change_form.html"
-        ], context, context_instance=template.RequestContext(request))
 
     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 = []
-        obj = self.model()
+        formsets = []
         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,
-                    instance=obj, save_as_new=request.POST.has_key("_saveasnew"))
-                inline_formsets.append(inline_formset)
-            if all_valid(inline_formsets) and form.is_valid():
-                return self.save_add(request, form, inline_formsets, '../%s/')
+                formset = FormSet(data=request.POST, files=request.FILES,
+                                  instance=new_object,
+                                  save_as_new=request.POST.has_key("_saveasnew"))
+                formsets.append(formset)
+            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)
-                inline_formsets.append(inline_formset)
+                formset = FormSet(instance=self.model())
+                formsets.append(formset)
 
         adminForm = AdminForm(form, list(self.get_fieldsets(request)), self.prepopulated_fields)
         media = self.media + adminForm.media
-        for fs in inline_formsets:
-            media = media + fs.media
+        for formset in formsets:
+            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):
-                inline_formset = FormSet(request.POST, request.FILES, instance=obj)
-                inline_formsets.append(inline_formset)
+            if form.is_valid():
+                form_validated = True
+                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():
-                return self.save_change(request, form, inline_formsets)
+            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=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)
-                inline_formsets.append(inline_formset)
+                formset = FormSet(instance=obj)
+                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."
diff --git a/docs/admin.txt b/docs/admin.txt
index 6efef41da3..d9b3523c04 100644
--- a/docs/admin.txt
+++ b/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
 --------------------------------
 
diff --git a/tests/regressiontests/admin_views/models.py b/tests/regressiontests/admin_views/models.py
index a8fc9467ba..98f99a6d00 100644
--- a/tests/regressiontests/admin_views/models.py
+++ b/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)
diff --git a/tests/regressiontests/admin_views/tests.py b/tests/regressiontests/admin_views/tests.py
index cc91494bba..2bdce7c07d 100644
--- a/tests/regressiontests/admin_views/tests.py
+++ b/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"&lt;p&gt;test content&lt;/p&gt;",
+            "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.