From 0fd6d4d2e5c3bd459d0d635afa1c79a08a1ea821 Mon Sep 17 00:00:00 2001 From: Tomasz Knapik Date: Wed, 4 Jul 2018 17:22:04 +0100 Subject: [PATCH] Delete variants of a page that is being deleted --- .../migrations/0019_auto_20180526_1425.py | 2 +- .../0020_rules_delete_relatedqueryname.py | 2 +- ...nalisablepagemetadata_canonical_protect.py | 24 ++++++++ src/wagtail_personalisation/models.py | 13 +++-- .../confirm_delete.html | 57 +++++++++++++++++++ src/wagtail_personalisation/wagtail_hooks.py | 55 ++++++++++++++++++ tests/unit/test_models.py | 18 ++++++ tests/unit/test_wagtail_hooks.py | 52 +++++++++++++++++ 8 files changed, 216 insertions(+), 7 deletions(-) create mode 100644 src/wagtail_personalisation/migrations/0021_personalisablepagemetadata_canonical_protect.py create mode 100644 src/wagtail_personalisation/templates/wagtailadmin/pages/wagtail_personalisation/confirm_delete.html diff --git a/src/wagtail_personalisation/migrations/0019_auto_20180526_1425.py b/src/wagtail_personalisation/migrations/0019_auto_20180526_1425.py index 6e40b8d..492fc11 100644 --- a/src/wagtail_personalisation/migrations/0019_auto_20180526_1425.py +++ b/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): diff --git a/src/wagtail_personalisation/migrations/0020_rules_delete_relatedqueryname.py b/src/wagtail_personalisation/migrations/0020_rules_delete_relatedqueryname.py index af46c0e..88d811d 100644 --- a/src/wagtail_personalisation/migrations/0020_rules_delete_relatedqueryname.py +++ b/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): diff --git a/src/wagtail_personalisation/migrations/0021_personalisablepagemetadata_canonical_protect.py b/src/wagtail_personalisation/migrations/0021_personalisablepagemetadata_canonical_protect.py new file mode 100644 index 0000000..a033238 --- /dev/null +++ b/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'), + ), + ] diff --git a/src/wagtail_personalisation/models.py b/src/wagtail_personalisation/models.py index e0fd523..e59e6e5 100644 --- a/src/wagtail_personalisation/models.py +++ b/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', diff --git a/src/wagtail_personalisation/templates/wagtailadmin/pages/wagtail_personalisation/confirm_delete.html b/src/wagtail_personalisation/templates/wagtailadmin/pages/wagtail_personalisation/confirm_delete.html new file mode 100644 index 0000000..311d7a2 --- /dev/null +++ b/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" %} + +
+

+ {% 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 %} +

+ {% if variants %} +

+ {% 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 %} +

+ + {% endif %} + +
+ {% csrf_token %} + + {% if variants %} + {% trans 'Yes, delete the page and its variants' as submit_button_value %} + {% else %} + {% trans 'Yes, delete it' as submit_button_value %} + {% endif %} + + {% trans "No, don't delete it" %} +
+ + {% page_permissions page as page_perms %} + {% if page_perms.can_unpublish %} + {% url 'wagtailadmin_pages:unpublish' page.id as unpublish_url %} +

{% blocktrans %}Alternatively you can unpublish the page. This removes the page from public view and you can edit or publish it again later.{% endblocktrans %}

+ {% endif %} +
+{% endblock %} diff --git a/src/wagtail_personalisation/wagtail_hooks.py b/src/wagtail_personalisation/wagtail_hooks.py index 2a74c2b..f148872 100644 --- a/src/wagtail_personalisation/wagtail_hooks.py +++ b/src/wagtail_personalisation/wagtail_hooks.py @@ -3,11 +3,15 @@ from __future__ import absolute_import, unicode_literals 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') + ) + } + ) diff --git a/tests/unit/test_models.py b/tests/unit/test_models.py index 085da2f..4a15d9d 100644 --- a/tests/unit/test_models.py +++ b/tests/unit/test_models.py @@ -3,10 +3,12 @@ from __future__ import absolute_import, unicode_literals 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() diff --git a/tests/unit/test_wagtail_hooks.py b/tests/unit/test_wagtail_hooks.py index eb25387..367246a 100644 --- a/tests/unit/test_wagtail_hooks.py +++ b/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())