Refactor personalisable pages

Instead of working with django model mixins it now uses a separate
model entity to keep track of the personalized pages (metadata).

The current downside of this approach is that the segment of an existing
variant is no longer easily adjustable for now.

sandbox/sandbox/apps/home/migrations/0001_initial.py

@@ -1,27 +1,26 @@
 # -*- coding: utf-8 -*-
-# Generated by Django 1.11.1 on 2017-05-31 10:20
+# Generated by Django 1.11.1 on 2017-05-31 16:59
 from __future__ import unicode_literals
 
 from django.db import migrations, models
 import django.db.models.deletion
+import modelcluster.fields
 
 
 class Migration(migrations.Migration):
 
     initial = True
 
     dependencies = [
-        ('wagtail_personalisation', '0009_auto_20170531_0428'),
+        ('wagtailcore', '0033_remove_golive_expiry_help_text'),
+        ('wagtail_personalisation', '0011_personalisablepagemetadata'),
     ]
 
     operations = [
         migrations.CreateModel(
             name='HomePage',
             fields=[
                 ('page_ptr', models.OneToOneField(auto_created=True, on_delete=django.db.models.deletion.CASCADE, parent_link=True, primary_key=True, serialize=False, to='wagtailcore.Page')),
-                ('is_segmented', models.BooleanField(default=False)),
-                ('canonical_page', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='variations', to='home.HomePage')),
-                ('segment', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.PROTECT, related_name='segments', to='wagtail_personalisation.Segment')),
             ],
             options={
                 'abstract': False,

sandbox/sandbox/apps/home/models.py

@@ -5,5 +5,5 @@
 from wagtail_personalisation.models import PersonalisablePageMixin
 
 
-class HomePage(Page, PersonalisablePageMixin):
+class HomePage(PersonalisablePageMixin, Page):
     pass

src/wagtail_personalisation/migrations/0011_personalisablepagemetadata.py

@@ -0,0 +1,30 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.1 on 2017-05-31 14:28
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('wagtailcore', '0033_remove_golive_expiry_help_text'),
+        ('wagtail_personalisation', '0010_auto_20170531_1101'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='PersonalisablePageMetadata',
+            fields=[
+                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
+                ('is_segmented', models.BooleanField(default=False)),
+                ('canonical_page', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='personalisable_canonical_metadata', to='wagtailcore.Page')),
+                ('segment', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, related_name='page_metadata', to='wagtail_personalisation.Segment')),
+                ('variant', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, related_name='_personalisable_page_metadata', to='wagtailcore.Page')),
+            ],
+            options={
+                'abstract': False,
+            },
+        ),
+    ]

src/wagtail_personalisation/models.py

@@ -1,11 +1,13 @@
 from __future__ import absolute_import, unicode_literals
 
-from django.db import models
+from django.db import models, transaction
 from django.template.defaultfilters import slugify
 from django.utils.encoding import python_2_unicode_compatible
 from django.utils.functional import cached_property
 from django.utils.translation import ugettext_lazy as _
+from modelcluster.fields import ParentalKey
 from modelcluster.models import ClusterableModel
+from wagtail.wagtailcore.models import Page
 from wagtail.utils.decorators import cached_classmethod
 from wagtail.wagtailadmin.edit_handlers import (
     FieldPanel, FieldRowPanel, InlinePanel, MultiFieldPanel, ObjectList,
@@ -96,31 +98,24 @@ def toggle(self, save=True):
             self.save()
 
 
-class PersonalisablePageMixin(models.Model):
+class PersonalisablePageMetadata(ClusterableModel):
     """The personalisable page model. Allows creation of variants with linked
     segments.
 
     """
-
-    class Meta:
-        abstract = True
-
     canonical_page = models.ForeignKey(
-        'self', related_name='variations', on_delete=models.SET_NULL,
+        Page, related_name='personalisable_canonical_metadata',
+        on_delete=models.SET_NULL,
         blank=True, null=True
     )
+
+    variant = models.OneToOneField(
+        Page, related_name='_personalisable_page_metadata')
+
     segment = models.ForeignKey(
-        Segment, related_name='pages', on_delete=models.PROTECT,
-        blank=True, null=True
-    )
-    is_segmented = models.BooleanField(default=False)
+        Segment, related_name='page_metadata', null=True, blank=True)
 
-    variation_panels = [
-        MultiFieldPanel([
-            FieldPanel('segment'),
-            PageChooserPanel('canonical_page', page_type=None),
-        ])
-    ]
+    is_segmented = models.BooleanField(default=False)
 
     base_form_class = AdminPersonalisablePageForm
 
@@ -136,6 +131,14 @@ def has_variations(self):
         """
         return self.variations.exists()
 
+    @cached_property
+    def variations(self):
+        return (
+            PersonalisablePageMetadata.objects
+            .filter(canonical_page_id=self.canonical_page_id)
+            .exclude(variant_id=self.variant_id)
+            .exclude(variant_id=self.canonical_page_id))
+
     @cached_property
     def is_canonical(self):
         """Return a boolean indicating whether or not the personalisable page
@@ -147,53 +150,56 @@ def is_canonical(self):
         :rtype: bool
 
         """
-        return self.canonical_page_id is None
-
-    def get_unused_segments(self):
-        if not hasattr(self, '_unused_segments'):
-            self._unused_segments = (
-                Segment.objects.exclude(pages__canonical_page=self)
-                if self.is_canonical else Segment.objects.none())
-        return self._unused_segments
+        return self.canonical_page_id == self.variant_id
 
     def copy_for_segment(self, segment):
-        slug = "{}-{}".format(self.slug, segment.encoded_name())
-        title = "{} ({})".format(self.title, segment.name)
+        page = self.canonical_page
+
+        slug = "{}-{}".format(page.slug, segment.encoded_name())
+        title = "{} ({})".format(page.title, segment.name)
         update_attrs = {
             'title': title,
             'slug': slug,
-            'segment': segment,
             'live': False,
-            'canonical_page': self,
-            'is_segmented': True,
         }
 
-        return self.copy(update_attrs=update_attrs, copy_revisions=False)
+        with transaction.atomic():
+            new_page = self.canonical_page.copy(
+                update_attrs=update_attrs, copy_revisions=False)
+
+            PersonalisablePageMetadata.objects.create(
+                canonical_page=page,
+                variant=new_page,
+                segment=segment,
+                is_segmented=True)
+        return new_page
 
     def variants_for_segments(self, segments):
-        return self.__class__.objects.filter(
-            canonical_page=self, segment__in=segments)
+        return (
+            self.__class__.objects
+            .filter(
+                canonical_page_id=self.canonical_page_id,
+                segment__in=segments))
+
+    def get_unused_segments(self):
+        if self.is_canonical:
+            return (
+                Segment.objects
+                .exclude(page_metadata__canonical_page_id=self.canonical_page_id))
+        return Segment.objects.none()
 
 
-@cached_classmethod
-def get_edit_handler(cls):
-    """Add additional edit handlers to pages that are allowed to have
-    variations.
+class PersonalisablePageMixin(object):
+    """The personalisable page model. Allows creation of variants with linked
+    segments.
 
     """
-    tabs = []
-    if cls.content_panels:
-        tabs.append(ObjectList(cls.content_panels, heading=_("Content")))
-    if cls.variation_panels:
-        tabs.append(ObjectList(cls.variation_panels, heading=_("Variations")))
-    if cls.promote_panels:
-        tabs.append(ObjectList(cls.promote_panels, heading=_("Promote")))
-    if cls.settings_panels:
-        tabs.append(ObjectList(cls.settings_panels, heading=_("Settings"),
-                               classname='settings'))
-
-    edit_handler = TabbedInterface(tabs, base_form_class=cls.base_form_class)
-    return edit_handler.bind_to_model(cls)
-
-
-PersonalisablePageMixin.get_edit_handler = get_edit_handler
+
+    @cached_property
+    def personalisable_metadata(self):
+        try:
+            metadata = self._personalisable_page_metadata
+        except AttributeError:
+            metadata = PersonalisablePageMetadata.objects.create(
+                canonical_page=self, variant=self)
+        return metadata

src/wagtail_personalisation/views.py

@@ -122,11 +122,13 @@ def copy_page_view(request, page_id, segment_id):
     if request.user.has_perm('wagtailadmin.access_admin'):
         segment = get_object_or_404(Segment, pk=segment_id)
         page = get_object_or_404(Page, pk=page_id).specific
-        variants = page.variants_for_segments([segment])
+
+        metadata = page.personalisable_metadata
+        variants = metadata.variants_for_segments([segment])
         if variants.exists():
             variant = variants.first()
         else:
-            variant = page.copy_for_segment(segment)
+            variant = metadata.copy_for_segment(segment)
         edit_url = reverse('wagtailadmin_pages:edit', args=[variant.id])
 
         return HttpResponseRedirect(edit_url)

src/wagtail_personalisation/wagtail_hooks.py

@@ -10,7 +10,7 @@
 from wagtail.wagtailadmin.widgets import Button, ButtonWithDropdownFromHook
 from wagtail.wagtailcore import hooks
 
-from wagtail_personalisation import admin_urls
+from wagtail_personalisation import admin_urls, models
 from wagtail_personalisation.adapters import get_segment_adapter
 from wagtail_personalisation.models import PersonalisablePageMixin, Segment
 from wagtail_personalisation.utils import impersonate_other_page
@@ -79,7 +79,8 @@ def serve_variation(page, request, serve_args, serve_kwargs):
     user_segments = adapter.get_segments()
 
     if user_segments:
-        variations = page.variants_for_segments(user_segments)
+        metadata = page.personalisable_metadata
+        variations = metadata.variants_for_segments(user_segments)
         if variations:
             variation = variations.first()
             impersonate_other_page(variation, page)
@@ -92,7 +93,11 @@ def page_listing_variant_buttons(page, page_perms, is_parent=False):
     the page (if any) and a 'Create a new variant' button.
 
     """
-    if isinstance(page, PersonalisablePageMixin) and page.get_unused_segments():
+    if not isinstance(page, models.PersonalisablePageMixin):
+        return
+
+    metadata = page.personalisable_metadata
+    if metadata.is_canonical and metadata.get_unused_segments():
         yield ButtonWithDropdownFromHook(
             _('Variants'),
             hook_name='register_page_listing_variant_buttons',
@@ -109,7 +114,11 @@ def page_listing_more_buttons(page, page_perms, is_parent=False):
     create a new variant for the selected segment.
 
     """
-    for segment in page.get_unused_segments():
+    if not isinstance(page, models.PersonalisablePageMixin):
+        return
+
+    metadata = page.personalisable_metadata
+    for segment in metadata.get_unused_segments():
         yield Button(segment.name,
                      reverse('segment:copy_page', args=[page.pk, segment.pk]),
                      attrs={"title": _('Create this variant')},

tests/fixtures.py

@@ -23,7 +23,7 @@ def site():
 def segmented_page(site):
     page = HomePageFactory(parent=site.root_page)
     segment = SegmentFactory()
-    return page.copy_for_segment(segment)
+    return page.personalisable_metadata.copy_for_segment(segment)
 
 
 @pytest.fixture()

tests/sandbox/pages/migrations/0003_auto_20170531_1428.py

@@ -0,0 +1,39 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.1 on 2017-05-31 14:28
+from __future__ import unicode_literals
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('pages', '0002_auto_20170531_0915'),
+    ]
+
+    operations = [
+        migrations.RemoveField(
+            model_name='homepage',
+            name='canonical_page',
+        ),
+        migrations.RemoveField(
+            model_name='homepage',
+            name='is_segmented',
+        ),
+        migrations.RemoveField(
+            model_name='homepage',
+            name='segment',
+        ),
+        migrations.RemoveField(
+            model_name='specialpage',
+            name='canonical_page',
+        ),
+        migrations.RemoveField(
+            model_name='specialpage',
+            name='is_segmented',
+        ),
+        migrations.RemoveField(
+            model_name='specialpage',
+            name='segment',
+        ),
+    ]

tests/unit/test_models.py

@@ -63,7 +63,9 @@ def test_page_copy_for_segment(self, segmented_page):
         assert segmented_page
 
     def test_page_has_variations(self, segmented_page):
-        assert not segmented_page.is_canonical
-        assert not segmented_page.has_variations
-        assert segmented_page.canonical_page.is_canonical
-        assert segmented_page.canonical_page.has_variations
+        assert not segmented_page.personalisable_metadata.is_canonical
+        assert not segmented_page.personalisable_metadata.has_variations
+
+        canonical = segmented_page.personalisable_metadata.canonical_page
+        assert canonical.personalisable_metadata.is_canonical
+        assert canonical.personalisable_metadata.has_variations