Merge pull request #184 from wagtail/182-deleting-the-page-deletes-it…

…s-variants

Delete page variants when deleting the page

src/wagtail_personalisation/migrations/0019_auto_20180526_1425.py

@@ -1,7 +1,7 @@
 # Generated by Django 2.0.5 on 2018-05-26 14:25
 
-from django.db import migrations, models
 import django.db.models.deletion
+from django.db import migrations, models
 
 
 class Migration(migrations.Migration):

src/wagtail_personalisation/migrations/0020_rules_delete_relatedqueryname.py

@@ -1,8 +1,8 @@
 # Generated by Django 2.0.5 on 2018-05-30 18:51
 
-from django.db import migrations
 import django.db.models.deletion
 import modelcluster.fields
+from django.db import migrations
 
 
 class Migration(migrations.Migration):

src/wagtail_personalisation/migrations/0021_personalisablepagemetadata_canonical_protect.py

@@ -0,0 +1,24 @@
+# Generated by Django 2.0.7 on 2018-07-05 13:25
+
+import django.db.models.deletion
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('wagtail_personalisation', '0020_rules_delete_relatedqueryname'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='personalisablepagemetadata',
+            name='canonical_page',
+            field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.PROTECT, related_name='personalisable_canonical_metadata', to='wagtailcore.Page'),
+        ),
+        migrations.AlterField(
+            model_name='personalisablepagemetadata',
+            name='variant',
+            field=models.OneToOneField(null=True, on_delete=django.db.models.deletion.CASCADE, related_name='_personalisable_page_metadata', to='wagtailcore.Page'),
+        ),
+    ]

src/wagtail_personalisation/models.py

@@ -203,15 +203,18 @@ class PersonalisablePageMetadata(ClusterableModel):
     segments.
 
     """
+    # Canonical pages should not ever be deleted if they have variants
+    # because the variants will be orphaned.
     canonical_page = models.ForeignKey(
-        Page, related_name='personalisable_canonical_metadata',
-        on_delete=models.SET_NULL,
-        blank=True, null=True
+        Page, models.PROTECT, related_name='personalisable_canonical_metadata',
+        null=True
     )
 
+    # Delete metadata of the variant if its page gets deleted.
     variant = models.OneToOneField(
-        Page, related_name='_personalisable_page_metadata',
-        on_delete=models.CASCADE)
+        Page, models.CASCADE, related_name='_personalisable_page_metadata',
+        null=True
+    )
 
     segment = models.ForeignKey(
         Segment, related_name='page_metadata',

src/wagtail_personalisation/templates/wagtailadmin/pages/wagtail_personalisation/confirm_delete.html

@@ -0,0 +1,57 @@
+{% extends "wagtailadmin/base.html" %}
+
+{% load i18n wagtailadmin_tags %}
+
+{% block content %}
+    {% trans "Delete" as del_str %}
+    {% include "wagtailadmin/shared/header.html" with title=del_str subtitle=page.get_admin_display_title icon="doc-empty-inverse" %}
+
+    <div class="nice-padding">
+        <p>
+            {% trans 'Are you sure you want to delete this page?' %}
+            {% if descendant_count %}
+                {% blocktrans count counter=descendant_count %}
+                    This will also delete one more subpage.
+                {% plural %}
+                    This will also delete {{ descendant_count }} more subpages.
+                {% endblocktrans %}
+            {% endif %}
+        </p>
+        {% if variants %}
+            <p>
+                {% blocktrans count counter=variants|length %}
+                    This page is personalisable. Deleting it will delete its variant:
+                {% plural %}
+                    This page is personalisable. Deleting it will delete all of its variants:
+                {% endblocktrans %}
+            </p>
+            <ul>
+                {% for variant in variants %}
+                    <li>
+                        <a href="{% url 'wagtailadmin_explore' variant.pk %}">
+                            {{ variant }}
+                        </a>
+                    </li>
+                {% endfor %}
+            </ul>
+        {% endif %}
+
+        <form action="{% url 'wagtailadmin_pages:delete' page.id %}" method="POST">
+            {% csrf_token %}
+            <input type="hidden" name="next" value="{{ next }}">
+            {% if variants %}
+                {% trans 'Yes, delete the page and its variants' as submit_button_value %}
+            {% else %}
+                {% trans 'Yes, delete it' as submit_button_value %}
+            {% endif %}
+            <input type="submit" value="{{ submit_button_value }}" class="button serious">
+            <a href="{% if next %}{{ next }}{% else %}{% url 'wagtailadmin_explore' page.get_parent.id %}{% endif %}" class="button button-secondary">{% trans "No, don't delete it" %}</a>
+        </form>
+
+        {% page_permissions page as page_perms %}
+        {% if page_perms.can_unpublish %}
+            {% url 'wagtailadmin_pages:unpublish' page.id as unpublish_url %}
+            <p style="margin-top: 1em">{% blocktrans %}Alternatively you can <a href="{{ unpublish_url }}">unpublish the page</a>. This removes the page from public view and you can edit or publish it again later.{% endblocktrans %}</p>
+        {% endif %}
+    </div>
+{% endblock %}

src/wagtail_personalisation/wagtail_hooks.py

@@ -3,11 +3,15 @@
 import logging
 
 from django.conf.urls import include, url
+from django.db import transaction
+from django.shortcuts import redirect, render
 from django.template.defaultfilters import pluralize
 from django.urls import reverse
 from django.utils.safestring import mark_safe
 from django.utils.translation import ugettext_lazy as _
+from wagtail.admin import messages
 from wagtail.admin.site_summary import PagesSummaryItem, SummaryItem
+from wagtail.admin.views.pages import get_valid_next_url_from_request
 from wagtail.admin.widgets import Button, ButtonWithDropdownFromHook
 from wagtail.core import hooks
 from wagtail.core.models import Page
@@ -250,3 +254,54 @@ def add_personalisation_summary_panels(request, items):
     items.append(SegmentSummaryPanel(request))
     items.append(PersonalisedPagesSummaryPanel(request))
     items.append(VariantPagesSummaryPanel(request))
+
+
+@hooks.register('before_delete_page')
+def delete_related_variants(request, page):
+    if not isinstance(page, models.PersonalisablePageMixin) \
+            or not page.personalisation_metadata.is_canonical:
+        return
+    # Get a list of related personalisation metadata for all the related
+    # variants.
+    variants_metadata = (
+        page.personalisation_metadata.variants_metadata
+                                     .select_related('variant')
+    )
+    next_url = get_valid_next_url_from_request(request)
+
+    if request.method == 'POST':
+        parent_id = page.get_parent().id
+        variants_metadata = variants_metadata.select_related('variant')
+        with transaction.atomic():
+            for metadata in variants_metadata.iterator():
+                # Call delete() on objects to trigger any signals or hooks.
+                metadata.variant.delete()
+            # Delete the page's main variant and the page itself.
+            page.personalisation_metadata.delete()
+            page.delete()
+        msg = _("Page '{0}' and its variants deleted.")
+        messages.success(
+            request,
+            msg.format(page.get_admin_display_title())
+        )
+
+        for fn in hooks.get_hooks('after_delete_page'):
+            result = fn(request, page)
+            if hasattr(result, 'status_code'):
+                return result
+
+        if next_url:
+            return redirect(next_url)
+        return redirect('wagtailadmin_explore', parent_id)
+
+    return render(
+        request,
+        'wagtailadmin/pages/wagtail_personalisation/confirm_delete.html', {
+            'page': page,
+            'descendant_count': page.get_descendant_count(),
+            'next': next_url,
+            'variants': Page.objects.filter(
+                pk__in=variants_metadata.values_list('variant_id')
+            )
+        }
+    )

tests/unit/test_models.py

@@ -3,10 +3,12 @@
 import datetime
 
 import pytest
+from django.db.models import ProtectedError
 
 from tests.factories.page import ContentPageFactory
 from tests.factories.segment import SegmentFactory
 from tests.site.pages import models
+from wagtail_personalisation.models import PersonalisablePageMetadata
 from wagtail_personalisation.rules import TimeRule
 
 
@@ -34,3 +36,19 @@ def test_content_page_model():
     page = ContentPageFactory()
     qs = models.ContentPage.objects.all()
     assert page in qs
+
+
+@pytest.mark.django_db
+def test_variant_can_be_deleted_without_error(segmented_page):
+    segmented_page.delete()
+    # Make sure the metadata gets deleted because of models.CASCADE.
+    with pytest.raises(PersonalisablePageMetadata.DoesNotExist):
+        segmented_page._personalisable_page_metadata.refresh_from_db()
+
+
+@pytest.mark.django_db
+def test_canonical_page_deletion_is_protected(segmented_page):
+    # When deleting canonical page without deleting variants, it should return
+    # an error. All variants should be deleted beforehand.
+    with pytest.raises(ProtectedError):
+        segmented_page.personalisation_metadata.canonical_page.delete()

tests/unit/test_wagtail_hooks.py

@@ -1,4 +1,5 @@
 import pytest
+from wagtail.core.models import Page
 
 from tests.factories.segment import SegmentFactory
 from wagtail_personalisation import adapters, wagtail_hooks
@@ -60,3 +61,54 @@ def test_page_listing_more_buttons(site, rf, segmented_page):
     result = wagtail_hooks.page_listing_more_buttons(page, [])
     items = list(result)
     assert len(items) == 3
+
+
+@pytest.mark.django_db
+def test_custom_delete_page_view_does_not_trigger_for_variants(
+    rf,
+    segmented_page
+):
+    assert (
+        wagtail_hooks.delete_related_variants(rf.get('/'), segmented_page)
+    ) is None
+
+
+@pytest.mark.django_db
+def test_custom_delete_page_view_triggers_for_canonical_pages(
+    rf,
+    segmented_page
+):
+    assert (
+        wagtail_hooks.delete_related_variants(
+            rf.get('/'),
+            segmented_page.personalisation_metadata.canonical_page
+        )
+    ) is not None
+
+
+@pytest.mark.django_db
+def test_custom_delete_page_view_deletes_variants(rf, segmented_page, user):
+    post_request = rf.post('/')
+    user.is_superuser = True
+    rf.user = user
+    canonical_page = segmented_page.personalisation_metadata.canonical_page
+    canonical_page_variant = canonical_page.personalisation_metadata
+    assert canonical_page_variant
+
+    variants = Page.objects.filter(pk__in=(
+        canonical_page.personalisation_metadata.variants_metadata.values_list('variant_id', flat=True)
+    ))
+    variants_metadata = canonical_page.personalisation_metadata.variants_metadata
+    # Make sure there are variants that exist in the database.
+    assert len(variants.all())
+    assert len(variants_metadata.all())
+    wagtail_hooks.delete_related_variants(
+        post_request, segmented_page.personalisation_metadata.canonical_page
+    )
+    with pytest.raises(canonical_page.DoesNotExist):
+        canonical_page.refresh_from_db()
+    with pytest.raises(canonical_page_variant.DoesNotExist):
+        canonical_page_variant.refresh_from_db()
+    # Make sure all the variant pages have been deleted.
+    assert not len(variants.all())
+    assert not len(variants_metadata.all())