Delete related variants when deleting the segment (#183)

* Delete related variants when deleting the segment

Closes #155

* Fix typo

* Fix migration ordering

* Split metadata migrations

src/wagtail_personalisation/migrations/0021_personalisablepagemetadata_segment_set_on_delete_protect.py

@@ -0,0 +1,19 @@
+# Generated by Django 2.0.7 on 2018-07-04 15:26
+
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('wagtail_personalisation', '0020_rules_delete_relatedqueryname'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='personalisablepagemetadata',
+            name='segment',
+            field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.PROTECT, related_name='page_metadata', to='wagtail_personalisation.Segment'),
+        ),
+    ]

src/wagtail_personalisation/migrations/0021_personalisablepagemetadata_canonical_protect.py → src/wagtail_personalisation/migrations/0022_personalisablepagemetadata_canonical_protect.py

@@ -7,7 +7,7 @@
 class Migration(migrations.Migration):
 
     dependencies = [
-        ('wagtail_personalisation', '0020_rules_delete_relatedqueryname'),
+        ('wagtail_personalisation', '0021_personalisablepagemetadata_segment_set_on_delete_protect'),
     ]
 
     operations = [
@@ -16,9 +16,4 @@ class Migration(migrations.Migration):
             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/migrations/0023_personalisablepagemetadata_variant_cascade.py

@@ -0,0 +1,19 @@
+# Generated by Django 2.0.5 on 2018-07-19 09:57
+
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('wagtail_personalisation', '0022_personalisablepagemetadata_canonical_protect'),
+    ]
+
+    operations = [
+        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

@@ -216,10 +216,8 @@ class PersonalisablePageMetadata(ClusterableModel):
         null=True
     )
 
-    segment = models.ForeignKey(
-        Segment, related_name='page_metadata',
-        on_delete=models.SET_NULL,
-        null=True, blank=True)
+    segment = models.ForeignKey(Segment, models.PROTECT, null=True,
+                                related_name='page_metadata')
 
     @cached_property
     def has_variants(self):

src/wagtail_personalisation/templates/modeladmin/wagtail_personalisation/segment/delete.html

@@ -0,0 +1,49 @@
+{% extends "modeladmin/delete.html" %}
+
+{% load i18n modeladmin_tags %}
+
+{% block content_main %}
+    <div class="nice-padding">
+        {% if protected_error %}
+            <h2>{% blocktrans with view.verbose_name|capfirst as model_name %}{{ model_name }} could not be deleted{% endblocktrans %}</h2>
+            <p>{% blocktrans with instance as instance_name %}'{{ instance_name }}' is currently referenced by other objects, and cannot be deleted without jeopardising data integrity. To delete it successfully, first remove references from the following objects, then try again:{% endblocktrans %}</p>
+            <ul>
+                {% for obj in linked_objects %}<li><b>{{ obj|get_content_type_for_obj|title }}:</b> {{ obj }}</li>{% endfor %}
+            </ul>
+            <p><a href="{{ view.index_url }}" class="button">{% trans 'Go back to listing' %}</a></p>
+        {% elif cannot_delete_page_variants_error %}
+            <h2>{% blocktrans %}Cannot delete all the page variants{% endblocktrans %}</h2>
+            <p>{% blocktrans %}You need to have permissions to delete the page variants associated with this segment.{% endblocktrans %}
+        {% else %}
+            {% with page_variants=view.get_affected_page_objects %}
+                {% if page_variants %}
+                    <p>
+                        {% blocktrans %}Deleting the segment will also mean deleting all the page variants associated with it. Do you want to continue?{% endblocktrans %}
+                    </p>
+                    <p>
+                        {% blocktrans %}The page objects that <strong>will be deleted</strong> are:{% endblocktrans %}
+                    </p>
+                    <ul>
+                    {% for variant in page_variants %}
+                        <li>
+                            <a href="{% url 'wagtailadmin_explore' variant.pk %}">
+                                {{ variant }}
+                            </a>
+                        </li>
+                    {% endfor %}
+                    </ul>
+                    {% trans 'Yes, delete the segment and associated page variants' as submit_button_value %}
+                {% else %}
+                    <p>
+                        {% blocktrans %}Do you want to continue deleting this segment?{% endblocktrans %}
+                    </p>
+                    {% trans 'Yes, delete the segment' as submit_button_value %}
+                {% endif %}
+                <form action="{{ view.delete_url }}" method="POST">
+                    {% csrf_token %}
+                    <input type="submit" value="{{ submit_button_value }}" class="button serious" />
+                </form>
+            {% endwith %}
+        {% endif %}
+    </div>
+{% endblock %}

src/wagtail_personalisation/utils.py

@@ -109,3 +109,10 @@ def exclude_variants(pages):
         canonical_page_id=F('variant_id')
     ).values_list('variant_id')
     return pages.exclude(pk__in=excluded_variant_pages)
+
+
+def can_delete_pages(pages, user):
+    for variant in pages:
+        if not variant.permissions_for_user(user).can_delete():
+            return False
+    return True

src/wagtail_personalisation/views.py

@@ -3,16 +3,19 @@
 import csv
 
 from django import forms
+from django.core.exceptions import PermissionDenied
+from django.db import transaction
 from django.http import (
     HttpResponse, HttpResponseForbidden, HttpResponseRedirect)
 from django.shortcuts import get_object_or_404
 from django.urls import reverse
 from django.utils.translation import ugettext_lazy as _
 from wagtail.contrib.modeladmin.options import ModelAdmin, modeladmin_register
-from wagtail.contrib.modeladmin.views import IndexView
+from wagtail.contrib.modeladmin.views import DeleteView, IndexView
 from wagtail.core.models import Page
 
 from wagtail_personalisation.models import Segment
+from wagtail_personalisation.utils import can_delete_pages
 
 
 class SegmentModelIndexView(IndexView):
@@ -35,12 +38,49 @@ def get_template_names(self):
         ]
 
 
+class SegmentModelDeleteView(DeleteView):
+    def get_affected_page_objects(self):
+        return Page.objects.filter(pk__in=(
+            self.instance.get_used_pages().values_list('variant_id', flat=True)
+        ))
+
+    def get_template_names(self):
+        return [
+            'modeladmin/wagtail_personalisation/segment/delete.html',
+            'modeladmin/delete.html',
+        ]
+
+    def delete_instance(self):
+        page_variants = self.get_affected_page_objects()
+        if not can_delete_pages(page_variants, self.request.user):
+            raise PermissionDenied(
+                'User has no permission to delete variant page objects.'
+            )
+        # Deleting page objects triggers deletion of the personalisation
+        # metadata too because of models.CASCADE.
+        with transaction.atomic():
+            for variant in page_variants.iterator():
+                # Delete each one separately so signals are called.
+                variant.delete()
+            super().delete_instance()
+
+    def post(self, request, *args, **kwargs):
+        if not can_delete_pages(self.get_affected_page_objects(),
+                                self.request.user):
+            context = self.get_context_data(
+                cannot_delete_page_variants_error=True,
+            )
+            return self.render_to_response(context)
+        return super().post(request, *args, **kwargs)
+
+
 @modeladmin_register
 class SegmentModelAdmin(ModelAdmin):
     """The model admin for the Segments administration interface."""
     model = Segment
     index_view_class = SegmentModelIndexView
     dashboard_view_class = SegmentModelDashboardView
+    delete_view_class = SegmentModelDeleteView
     menu_icon = 'fa-snowflake-o'
     add_to_settings_menu = False
     list_display = ('name', 'persistent', 'match_any', 'status',

tests/unit/test_models.py

@@ -52,3 +52,11 @@ def test_canonical_page_deletion_is_protected(segmented_page):
     # an error. All variants should be deleted beforehand.
     with pytest.raises(ProtectedError):
         segmented_page.personalisation_metadata.canonical_page.delete()
+
+
+@pytest.mark.django_db
+def test_page_protection_when_deleting_segment(segmented_page):
+    segment = segmented_page.personalisation_metadata.segment
+    assert len(segment.get_used_pages())
+    with pytest.raises(ProtectedError):
+        segment.delete()

tests/unit/test_utils.py

@@ -1,7 +1,8 @@
 import pytest
 
 from tests.factories.page import ContentPageFactory
-from wagtail_personalisation.utils import impersonate_other_page
+from wagtail_personalisation.utils import (
+    can_delete_pages, impersonate_other_page)
 
 
 @pytest.fixture
@@ -24,3 +25,14 @@ def test_impersonate_other_page(page, otherpage):
     impersonate_other_page(page, otherpage)
     assert page.title == otherpage.title == 'Bye'
     assert page.path == otherpage.path
+
+
+@pytest.mark.django_db
+def test_can_delete_pages_with_superuser(rf, user, segmented_page):
+    user.is_superuser = True
+    assert can_delete_pages([segmented_page], user)
+
+
+@pytest.mark.django_db
+def test_cannot_delete_pages_with_standard_user(user, segmented_page):
+    assert not can_delete_pages([segmented_page], user)

tests/unit/test_views.py

@@ -1,8 +1,12 @@
 import pytest
+from django.core.exceptions import PermissionDenied
 from django.urls import reverse
+from wagtail.core.models import Page
 
 from wagtail_personalisation.models import Segment
 from wagtail_personalisation.rules import VisitCountRule
+from wagtail_personalisation.views import (
+    SegmentModelDeleteView, SegmentModelAdmin)
 
 
 @pytest.mark.django_db
@@ -51,3 +55,56 @@ def test_segment_user_data_view(site, client, mocker, django_user_model):
     assert data_lines[0] == 'Username,Visit count - Test page,Visit count - Regular page\r'
     assert data_lines[1] == 'first,3,9\r'
     assert data_lines[2] == 'second,0,1\r'
+
+
+@pytest.mark.django_db
+def test_segment_delete_view_delete_instance(rf, segmented_page, user):
+    user.is_superuser = True
+    user.save()
+    segment = segmented_page.personalisation_metadata.segment
+    canonical_page = segmented_page.personalisation_metadata.canonical_page
+    variants_metadata = segment.get_used_pages()
+    page_variants = Page.objects.filter(pk__in=(
+        variants_metadata.values_list('variant_id', flat=True)
+    ))
+
+    # Make sure all canonical page, variants and variants metadata exist
+    assert canonical_page
+    assert page_variants
+    assert variants_metadata
+
+    # Delete the segment via the method on the view.
+    request = rf.get('/'.format(segment.pk))
+    request.user = user
+    view = SegmentModelDeleteView(
+        instance_pk=str(segment.pk),
+        model_admin=SegmentModelAdmin()
+    )
+    view.request = request
+    view.delete_instance()
+
+    # Segment has been deleted.
+    with pytest.raises(segment.DoesNotExist):
+        segment.refresh_from_db()
+
+    # Canonical page stayed intact.
+    canonical_page.refresh_from_db()
+
+    # Variant pages and their metadata have been deleted.
+    assert not page_variants.all()
+    assert not variants_metadata.all()
+
+
+@pytest.mark.django_db
+def test_segment_delete_view_raises_permission_denied(rf, segmented_page, user):
+    segment = segmented_page.personalisation_metadata.segment
+    request = rf.get('/'.format(segment.pk))
+    request.user = user
+    view = SegmentModelDeleteView(
+        instance_pk=str(segment.pk),
+        model_admin=SegmentModelAdmin()
+    )
+    view.request = request
+    message = 'User have no permission to delete variant page objects.'
+    with pytest.raises(PermissionDenied, message=message):
+        view.delete_instance()