From 107767e97ab08d9980916b2ef4d58240b3a0498f Mon Sep 17 00:00:00 2001 From: Karl Hobley Date: Tue, 2 Dec 2014 12:55:22 +0000 Subject: [PATCH 1/5] Update page IDs in revision JSON when copying Fixes #837 When copying a page, all of its revisions are also copied. The revisions are JSON encoded and include ID fields which point back to the original page object. These IDs weren't being updated when the revision was copied to a new page causing the editor to break when that new page is edited. --- wagtail/wagtailcore/models.py | 20 ++++++++++++++++++++ wagtail/wagtailcore/tests/test_page_model.py | 7 +++++++ 2 files changed, 27 insertions(+) diff --git a/wagtail/wagtailcore/models.py b/wagtail/wagtailcore/models.py index 47b9305c46..ac610b861d 100644 --- a/wagtail/wagtailcore/models.py +++ b/wagtail/wagtailcore/models.py @@ -1,5 +1,6 @@ import logging import warnings +import json import six from six import StringIO @@ -770,6 +771,25 @@ class Page(six.with_metaclass(PageBase, MP_Node, ClusterableModel, index.Indexed revision.submitted_for_moderation = False revision.approved_go_live_at = None revision.page = page_copy + + # Update ID fields in content + revision_content = json.loads(revision.content_json) + revision_content['pk'] = page_copy.pk + + for child_relation in get_all_child_relations(specific_self): + try: + child_objects = revision_content[child_relation.get_accessor_name()] + except KeyError: + # KeyErrors are possible if the revision was created + # before this child relation was added to the database + continue + + for child_object in child_objects: + child_object[child_relation.field.name] = page_copy.pk + + revision.content_json = json.dumps(revision_content) + + # Save revision.save() # Log diff --git a/wagtail/wagtailcore/tests/test_page_model.py b/wagtail/wagtailcore/tests/test_page_model.py index 8e049129b4..b22b024581 100644 --- a/wagtail/wagtailcore/tests/test_page_model.py +++ b/wagtail/wagtailcore/tests/test_page_model.py @@ -1,5 +1,6 @@ import warnings import datetime +import json import pytz @@ -430,6 +431,12 @@ class TestCopyPage(TestCase): # Check that the revisions weren't removed from old page self.assertEqual(christmas_event.revisions.count(), 1, "Revisions were removed from the original page") + # Check that the ids within the revision were updated correctly + new_revision = new_christmas_event.revisions.first() + new_revision_content = json.loads(new_revision.content_json) + self.assertEqual(new_revision_content['pk'], new_christmas_event.id) + self.assertEqual(new_revision_content['speakers'][0]['page'], new_christmas_event.id) + def test_copy_page_copies_revisions_and_doesnt_submit_for_moderation(self): christmas_event = EventPage.objects.get(url_path='/home/events/christmas/') christmas_event.save_revision(submitted_for_moderation=True) From 7235c61fb71ecb10f82c6a59a7964ae23f6bd538 Mon Sep 17 00:00:00 2001 From: Karl Hobley Date: Tue, 2 Dec 2014 13:50:52 +0000 Subject: [PATCH 2/5] Create new revision when copying pages This serves two purposes: * It makes sure update_attrs gets applied to the latest revision so the changes are reflected in the editor * It bumps the last_revision_created_at value so the new page gets ordered as if it was just created --- wagtail/wagtailcore/models.py | 12 +++++++++++ wagtail/wagtailcore/tests/test_page_model.py | 21 ++++++++++++++------ 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/wagtail/wagtailcore/models.py b/wagtail/wagtailcore/models.py index ac610b861d..4b3b8ef535 100644 --- a/wagtail/wagtailcore/models.py +++ b/wagtail/wagtailcore/models.py @@ -792,6 +792,18 @@ class Page(six.with_metaclass(PageBase, MP_Node, ClusterableModel, index.Indexed # Save revision.save() + # Create a new revision + # This code serves two purposes: + # * It makes sure update_attrs gets applied to the latest revision so the changes are reflected in the editor + # * It bumps the last_revision_created_at value so the new page gets ordered as if it was just created + latest_revision = page_copy.get_latest_revision_as_page() + + if update_attrs: + for field, value in update_attrs.items(): + setattr(latest_revision, field, value) + + latest_revision.save_revision() + # Log logger.info("Page copied: \"%s\" id=%d from=%d", page_copy.title, page_copy.id, self.id) diff --git a/wagtail/wagtailcore/tests/test_page_model.py b/wagtail/wagtailcore/tests/test_page_model.py index b22b024581..b5df493e36 100644 --- a/wagtail/wagtailcore/tests/test_page_model.py +++ b/wagtail/wagtailcore/tests/test_page_model.py @@ -426,11 +426,17 @@ class TestCopyPage(TestCase): new_christmas_event = christmas_event.copy(update_attrs={'title': "New christmas event", 'slug': 'new-christmas-event'}) # Check that the revisions were copied - self.assertEqual(new_christmas_event.revisions.count(), 1, "Revisions weren't copied") + # Copying creates a new revision so we're expecting the new page to have two revisions + self.assertEqual(new_christmas_event.revisions.count(), 2) # Check that the revisions weren't removed from old page self.assertEqual(christmas_event.revisions.count(), 1, "Revisions were removed from the original page") + # Check that the attributes were updated in the latest revision + latest_revision = new_christmas_event.get_latest_revision_as_page() + self.assertEqual(latest_revision.title, "New christmas event") + self.assertEqual(latest_revision.slug, 'new-christmas-event') + # Check that the ids within the revision were updated correctly new_revision = new_christmas_event.revisions.first() new_revision_content = json.loads(new_revision.content_json) @@ -463,8 +469,8 @@ class TestCopyPage(TestCase): new_christmas_event = christmas_event.copy(update_attrs={'title': "New christmas event", 'slug': 'new-christmas-event'}) # Check that the created_at time is the same - christmas_event_created_at = christmas_event.get_latest_revision().created_at - new_christmas_event_created_at = new_christmas_event.get_latest_revision().created_at + christmas_event_created_at = christmas_event.revisions.first().created_at + new_christmas_event_created_at = new_christmas_event.revisions.first().created_at self.assertEqual(christmas_event_created_at, new_christmas_event_created_at) def test_copy_page_copies_revisions_and_doesnt_schedule(self): @@ -488,7 +494,8 @@ class TestCopyPage(TestCase): new_christmas_event = christmas_event.copy(update_attrs={'title': "New christmas event", 'slug': 'new-christmas-event'}, copy_revisions=False) # Check that the revisions weren't copied - self.assertEqual(new_christmas_event.revisions.count(), 0, "Revisions were copied") + # Copying creates a new revision so we're expecting the new page to have one revision + self.assertEqual(new_christmas_event.revisions.count(), 1) # Check that the revisions weren't removed from old page self.assertEqual(christmas_event.revisions.count(), 1, "Revisions were removed from the original page") @@ -551,7 +558,8 @@ class TestCopyPage(TestCase): new_christmas_event = new_events_index.get_children().filter(slug='christmas').first() # Check that the revisions were copied - self.assertEqual(new_christmas_event.specific.revisions.count(), 1, "Revisions weren't copied") + # Copying creates a new revision so we're expecting the new page to have two revisions + self.assertEqual(new_christmas_event.specific.revisions.count(), 2) # Check that the revisions weren't removed from old page self.assertEqual(old_christmas_event.specific.revisions.count(), 1, "Revisions were removed from the original page") @@ -568,7 +576,8 @@ class TestCopyPage(TestCase): new_christmas_event = new_events_index.get_children().filter(slug='christmas').first() # Check that the revisions weren't copied - self.assertEqual(new_christmas_event.specific.revisions.count(), 0, "Revisions were copied") + # Copying creates a new revision so we're expecting the new page to have one revision + self.assertEqual(new_christmas_event.specific.revisions.count(), 1) # Check that the revisions weren't removed from old page self.assertEqual(old_christmas_event.specific.revisions.count(), 1, "Revisions were removed from the original page") From ca7e285353589e7f45eac52b2041e3b0064398bc Mon Sep 17 00:00:00 2001 From: Karl Hobley Date: Tue, 2 Dec 2014 14:03:09 +0000 Subject: [PATCH 3/5] Added user attribute to Page.copy This is used for setting the owner of the new pages and the user for the new revision that gets created --- wagtail/wagtailadmin/views/pages.py | 4 +--- wagtail/wagtailcore/models.py | 12 ++++++++---- wagtail/wagtailcore/tests/test_page_model.py | 18 ++++++++++++++++++ 3 files changed, 27 insertions(+), 7 deletions(-) diff --git a/wagtail/wagtailadmin/views/pages.py b/wagtail/wagtailadmin/views/pages.py index a30548b15a..6390eeeb24 100644 --- a/wagtail/wagtailadmin/views/pages.py +++ b/wagtail/wagtailadmin/views/pages.py @@ -665,11 +665,9 @@ def copy(request, page_id): 'slug': form.cleaned_data['new_slug'], }, keep_live=(can_publish and form.cleaned_data.get('publish_copies')), + user=request.user, ) - # Assign user of this request as the owner of all the new pages - new_page.get_descendants(inclusive=True).update(owner=request.user) - # Give a success message back to the user if form.cleaned_data.get('copy_subpages'): messages.success(request, _("Page '{0}' and {1} subpages copied.").format(page.title, new_page.get_descendants().count())) diff --git a/wagtail/wagtailcore/models.py b/wagtail/wagtailcore/models.py index 4b3b8ef535..50a3269242 100644 --- a/wagtail/wagtailcore/models.py +++ b/wagtail/wagtailcore/models.py @@ -730,7 +730,7 @@ class Page(six.with_metaclass(PageBase, MP_Node, ClusterableModel, index.Indexed # Log logger.info("Page moved: \"%s\" id=%d path=%s", self.title, self.id, new_url_path) - def copy(self, recursive=False, to=None, update_attrs=None, copy_revisions=True, keep_live=True): + def copy(self, recursive=False, to=None, update_attrs=None, copy_revisions=True, keep_live=True, user=None): # Make a copy page_copy = Page.objects.get(id=self.id).specific page_copy.pk = None @@ -743,6 +743,9 @@ class Page(six.with_metaclass(PageBase, MP_Node, ClusterableModel, index.Indexed page_copy.live = False page_copy.has_unpublished_changes = True + if user: + page_copy.owner = user + if update_attrs: for field, value in update_attrs.items(): setattr(page_copy, field, value) @@ -793,16 +796,17 @@ class Page(six.with_metaclass(PageBase, MP_Node, ClusterableModel, index.Indexed revision.save() # Create a new revision - # This code serves two purposes: + # This code serves a few purposes: # * It makes sure update_attrs gets applied to the latest revision so the changes are reflected in the editor # * It bumps the last_revision_created_at value so the new page gets ordered as if it was just created + # * It sets the user of the new revision so it's possible to see who copied the page by looking at its history latest_revision = page_copy.get_latest_revision_as_page() if update_attrs: for field, value in update_attrs.items(): setattr(latest_revision, field, value) - latest_revision.save_revision() + latest_revision.save_revision(user=user) # Log logger.info("Page copied: \"%s\" id=%d from=%d", page_copy.title, page_copy.id, self.id) @@ -810,7 +814,7 @@ class Page(six.with_metaclass(PageBase, MP_Node, ClusterableModel, index.Indexed # Copy child pages if recursive: for child_page in self.get_children(): - child_page.specific.copy(recursive=True, to=page_copy, copy_revisions=copy_revisions, keep_live=keep_live) + child_page.specific.copy(recursive=True, to=page_copy, copy_revisions=copy_revisions, keep_live=keep_live, user=user) return page_copy diff --git a/wagtail/wagtailcore/tests/test_page_model.py b/wagtail/wagtailcore/tests/test_page_model.py index b5df493e36..52f44cbce3 100644 --- a/wagtail/wagtailcore/tests/test_page_model.py +++ b/wagtail/wagtailcore/tests/test_page_model.py @@ -8,6 +8,7 @@ from django.test import TestCase, Client from django.test.utils import override_settings from django.http import HttpRequest, Http404 from django.contrib.contenttypes.models import ContentType +from django.contrib.auth import get_user_model from wagtail.wagtailcore.models import Page, Site from wagtail.tests.models import EventPage, EventIndex, SimplePage, PageWithOldStyleRouteMethod, BusinessIndex, BusinessSubIndex, BusinessChild, StandardIndex @@ -582,6 +583,23 @@ class TestCopyPage(TestCase): # Check that the revisions weren't removed from old page self.assertEqual(old_christmas_event.specific.revisions.count(), 1, "Revisions were removed from the original page") + def test_copy_page_updates_user(self): + event_editor = get_user_model().objects.get(username='eventeditor') + christmas_event = EventPage.objects.get(url_path='/home/events/christmas/') + christmas_event.save_revision() + + # Copy it + new_christmas_event = christmas_event.copy( + update_attrs={'title': "New christmas event", 'slug': 'new-christmas-event'}, + user=event_editor, + ) + + # Check that the owner has been updated + self.assertEqual(new_christmas_event.owner, event_editor) + + # Check that the user on the last revision is correct + self.assertEqual(new_christmas_event.get_latest_revision().user, event_editor) + class TestSubpageTypeBusinessRules(TestCase): def test_allowed_subpage_types(self): From 1fcb94c0b9f74f0e72ef8400c4ec54f347644985 Mon Sep 17 00:00:00 2001 From: Karl Hobley Date: Tue, 2 Dec 2014 16:27:25 +0000 Subject: [PATCH 4/5] Order by created_at when getting first revision --- wagtail/wagtailcore/tests/test_page_model.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/wagtail/wagtailcore/tests/test_page_model.py b/wagtail/wagtailcore/tests/test_page_model.py index 52f44cbce3..2ce3e5c221 100644 --- a/wagtail/wagtailcore/tests/test_page_model.py +++ b/wagtail/wagtailcore/tests/test_page_model.py @@ -452,10 +452,10 @@ class TestCopyPage(TestCase): new_christmas_event = christmas_event.copy(update_attrs={'title': "New christmas event", 'slug': 'new-christmas-event'}) # Check that the old revision is still submitted for moderation - self.assertTrue(christmas_event.revisions.first().submitted_for_moderation) + self.assertTrue(christmas_event.revisions.order_by('created_at').first().submitted_for_moderation) # Check that the new revision is not submitted for moderation - self.assertFalse(new_christmas_event.revisions.first().submitted_for_moderation) + self.assertFalse(new_christmas_event.revisions.order_by('created_at').first().submitted_for_moderation) def test_copy_page_copies_revisions_and_doesnt_change_created_at(self): christmas_event = EventPage.objects.get(url_path='/home/events/christmas/') @@ -470,8 +470,8 @@ class TestCopyPage(TestCase): new_christmas_event = christmas_event.copy(update_attrs={'title': "New christmas event", 'slug': 'new-christmas-event'}) # Check that the created_at time is the same - christmas_event_created_at = christmas_event.revisions.first().created_at - new_christmas_event_created_at = new_christmas_event.revisions.first().created_at + christmas_event_created_at = christmas_event.revisions.order_by('created_at').first().created_at + new_christmas_event_created_at = new_christmas_event.revisions.order_by('created_at').first().created_at self.assertEqual(christmas_event_created_at, new_christmas_event_created_at) def test_copy_page_copies_revisions_and_doesnt_schedule(self): @@ -482,10 +482,10 @@ class TestCopyPage(TestCase): new_christmas_event = christmas_event.copy(update_attrs={'title': "New christmas event", 'slug': 'new-christmas-event'}) # Check that the old revision is still scheduled - self.assertEqual(christmas_event.revisions.first().approved_go_live_at, datetime.datetime(2014, 9, 16, 9, 12, 00, tzinfo=pytz.utc)) + self.assertEqual(christmas_event.revisions.order_by('created_at').first().approved_go_live_at, datetime.datetime(2014, 9, 16, 9, 12, 00, tzinfo=pytz.utc)) # Check that the new revision is not scheduled - self.assertEqual(new_christmas_event.revisions.first().approved_go_live_at, None) + self.assertEqual(new_christmas_event.revisions.order_by('created_at').first().approved_go_live_at, None) def test_copy_page_doesnt_copy_revisions_if_told_not_to_do_so(self): christmas_event = EventPage.objects.get(url_path='/home/events/christmas/') From 52581222e8f27a62755b3d090c8f0cbc27af2f86 Mon Sep 17 00:00:00 2001 From: Karl Hobley Date: Tue, 2 Dec 2014 17:14:22 +0000 Subject: [PATCH 5/5] Changed test to use eventmoderator user --- wagtail/wagtailcore/tests/test_page_model.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/wagtail/wagtailcore/tests/test_page_model.py b/wagtail/wagtailcore/tests/test_page_model.py index 2ce3e5c221..be549f502e 100644 --- a/wagtail/wagtailcore/tests/test_page_model.py +++ b/wagtail/wagtailcore/tests/test_page_model.py @@ -584,21 +584,21 @@ class TestCopyPage(TestCase): self.assertEqual(old_christmas_event.specific.revisions.count(), 1, "Revisions were removed from the original page") def test_copy_page_updates_user(self): - event_editor = get_user_model().objects.get(username='eventeditor') + event_moderator = get_user_model().objects.get(username='eventmoderator') christmas_event = EventPage.objects.get(url_path='/home/events/christmas/') christmas_event.save_revision() # Copy it new_christmas_event = christmas_event.copy( update_attrs={'title': "New christmas event", 'slug': 'new-christmas-event'}, - user=event_editor, + user=event_moderator, ) # Check that the owner has been updated - self.assertEqual(new_christmas_event.owner, event_editor) + self.assertEqual(new_christmas_event.owner, event_moderator) # Check that the user on the last revision is correct - self.assertEqual(new_christmas_event.get_latest_revision().user, event_editor) + self.assertEqual(new_christmas_event.get_latest_revision().user, event_moderator) class TestSubpageTypeBusinessRules(TestCase):