Fix regression on plain TaggableManagers used in ClusterForm (#78)

Only postpone instance.save() after save_m2m() if we really have to, as indicated by the presence of fields with the _need_commit_after_assignment flag set.

modelcluster/contrib/taggit.py

@@ -80,6 +80,8 @@ def clear(self):
 
 
 class ClusterTaggableManager(TaggableManager):
+    _need_commit_after_assignment = True
+
     def __get__(self, instance, model):
         # override TaggableManager's requirement for instance to have a primary key
         # before we can access its tags

modelcluster/fields.py

@@ -470,6 +470,7 @@ def child_object_manager_cls(self):
 
 class ParentalManyToManyField(ManyToManyField):
     related_accessor_class = ParentalManyToManyDescriptor
+    _need_commit_after_assignment = True
 
     def contribute_to_class(self, cls, name, **kwargs):
         # ManyToManyField does not (as of Django 1.10) respect related_accessor_class,

modelcluster/forms.py

@@ -274,22 +274,43 @@ def media(self):
         return media
 
     def save(self, commit=True):
-        # call ModelForm.save() with commit=False so that we can fix up the behaviour for
-        # M2M fields before saving
-        instance = super(ClusterForm, self).save(commit=False)
-
-        # Always call save_m2m(), even for commit=False. The M2M field types allowed in a
-        # ClusterForm (ParentalManyToManyField and ClusterTaggableManager) are designed to
-        # update the local instance when written, rather than updating the database directly,
-        # and this happens on save_m2m.
-        #
-        # To actually save to the database, we need an explicit 'commit' step, which happens on
-        # instance.save(). We therefore ensure that save_m2m() happens first, so that the
-        # updated relation data exists on the local in-memory instance at the point of saving.
-        self.save_m2m()
+        # do we have any fields that expect us to call save_m2m immediately?
+        save_m2m_now = False
+        exclude = self._meta.exclude
+        fields = self._meta.fields
+
+        for f in self.instance._meta.get_fields():
+            if fields and f.name not in fields:
+                continue
+            if exclude and f.name in exclude:
+                continue
+            if getattr(f, '_need_commit_after_assignment', False):
+                save_m2m_now = True
+                break
+
+        instance = super(ClusterForm, self).save(commit=(commit and not save_m2m_now))
+
+        # The M2M-like fields designed for use with ClusterForm (currently
+        # ParentalManyToManyField and ClusterTaggableManager) will manage their own in-memory
+        # relations, and not immediately write to the database when we assign to them.
+        # For these fields (identified by the _need_commit_after_assignment
+        # flag), save_m2m() is a safe operation that does not affect the database and is thus
+        # valid for commit=False. In the commit=True case, committing to the database happens
+        # in the subsequent instance.save (so this needs to happen after save_m2m to ensure
+        # we have the updated relation data in place).
+
+        # For annoying legacy reasons we sometimes need to accommodate 'classic' M2M fields
+        # (particularly taggit.TaggableManager) within ClusterForm. These fields
+        # generally do require our instance to exist in the database at the point we call
+        # save_m2m() - for this reason, we only proceed with the customisation described above
+        # (i.e. postpone the instance.save() operation until after save_m2m) if there's a
+        # _need_commit_after_assignment field on the form that demands it.
+
+        if save_m2m_now:
+            self.save_m2m()
 
-        if commit:
-            instance.save()
+            if commit:
+                instance.save()
 
         for formset in self.formsets.values():
             formset.instance = instance

tests/migrations/0004_auto_20170406_1734.py

@@ -0,0 +1,41 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.10.6 on 2017-04-06 22:34
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+import django.db.models.deletion
+import taggit.managers
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('taggit', '0002_auto_20150616_2121'),
+        ('tests', '0003_gallery_galleryimage'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='NonClusterPlace',
+            fields=[
+                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
+                ('name', models.CharField(max_length=255)),
+            ],
+        ),
+        migrations.CreateModel(
+            name='TaggedNonClusterPlace',
+            fields=[
+                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
+                ('content_object', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='tagged_items', to='tests.NonClusterPlace')),
+                ('tag', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='tests_taggednonclusterplace_items', to='taggit.Tag')),
+            ],
+            options={
+                'abstract': False,
+            },
+        ),
+        migrations.AddField(
+            model_name='nonclusterplace',
+            name='tags',
+            field=taggit.managers.TaggableManager(blank=True, help_text='A comma-separated list of tags.', through='tests.TaggedNonClusterPlace', to='taggit.Tag', verbose_name='Tags'),
+        ),
+    ]

tests/models.py

@@ -4,6 +4,7 @@
 from django.utils.encoding import python_2_unicode_compatible
 
 from modelcluster.contrib.taggit import ClusterTaggableManager
+from taggit.managers import TaggableManager
 from taggit.models import TaggedItemBase
 
 from modelcluster.fields import ParentalKey, ParentalManyToManyField
@@ -61,6 +62,24 @@ class Restaurant(Place):
     proprietor = models.ForeignKey('Chef', null=True, blank=True, on_delete=models.SET_NULL, related_name='restaurants')
 
 
+class TaggedNonClusterPlace(TaggedItemBase):
+    content_object = models.ForeignKey('NonClusterPlace', related_name='tagged_items')
+
+
+@python_2_unicode_compatible
+class NonClusterPlace(models.Model):
+    """
+    For backwards compatibility we need ClusterModel to work with
+    plain TaggableManagers (as opposed to ClusterTaggableManager), albeit
+    without the in-memory relation behaviour
+    """
+    name = models.CharField(max_length=255)
+    tags = TaggableManager(through=TaggedNonClusterPlace, blank=True)
+
+    def __str__(self):
+        return self.name
+
+
 @python_2_unicode_compatible
 class Dish(models.Model):
     name = models.CharField(max_length=255)

tests/tests/test_tag.py

@@ -6,7 +6,7 @@
 from taggit.models import Tag
 from modelcluster.forms import ClusterForm
 
-from tests.models import Place, TaggedPlace
+from tests.models import NonClusterPlace, Place, TaggedPlace
 
 
 class TagTest(TestCase):
@@ -94,6 +94,44 @@ class Meta:
         self.assertTrue(Tag.objects.get(name='fajita') in mission_burrito.tags.all())
         self.assertFalse(Tag.objects.get(name='mexican') in mission_burrito.tags.all())
 
+    def test_create_with_tags(self):
+        class PlaceForm(ClusterForm):
+            class Meta:
+                model = Place
+                exclude_formsets = ['tagged_items', 'reviews']
+                fields = ['name', 'tags']
+
+        form = PlaceForm({
+            'name': "Mission Burrito",
+            'tags': "burrito, fajita"
+        }, instance=Place())
+        self.assertTrue(form.is_valid())
+        mission_burrito = form.save()
+        reloaded_mission_burrito = Place.objects.get(pk=mission_burrito.pk)
+        self.assertEqual(
+            set(reloaded_mission_burrito.tags.all()),
+            set([Tag.objects.get(name='burrito'), Tag.objects.get(name='fajita')])
+        )
+
+    def test_create_with_tags_with_plain_taggable_manager(self):
+        class PlaceForm(ClusterForm):
+            class Meta:
+                model = NonClusterPlace
+                exclude_formsets = ['tagged_items', 'reviews']
+                fields = ['name', 'tags']
+
+        form = PlaceForm({
+            'name': "Mission Burrito",
+            'tags': "burrito, fajita"
+        }, instance=NonClusterPlace())
+        self.assertTrue(form.is_valid())
+        mission_burrito = form.save()
+        reloaded_mission_burrito = NonClusterPlace.objects.get(pk=mission_burrito.pk)
+        self.assertEqual(
+            set(reloaded_mission_burrito.tags.all()),
+            set([Tag.objects.get(name='burrito'), Tag.objects.get(name='fajita')])
+        )
+
     @override_settings(TAGGIT_CASE_INSENSITIVE=True)
     def test_case_insensitive_tags(self):
         mission_burrito = Place(name='Mission Burrito')