From 6b33690cd3306878c2027ccfcbfc19f23eba8cce Mon Sep 17 00:00:00 2001 From: Sage Abdullah Date: Thu, 12 Dec 2024 11:54:17 +0000 Subject: [PATCH] Refactor pages revisions_revert view to be a subclass of EditView (#12690) Currently, the code that handles the POST request for reverting a revision lives in the EditView class, while the revisions_revert view is a smaller view that tries to "mimic" the EditView for rendering the view as part of a GET request. The view injects the revision ID into the form, which has the action URL hardcoded to the EditView. Including the revision ID in the form allows the EditView to tell whether it's in a "reverting" mode or not, and adjust the POST logic accordingly. However, this results in possible inconsistencies in both views. Whenever we want to change EditView code or template, we need to make sure to also update the revisions_revert view. The fact that the revisions_revert view is a function-based view doesn't help. Instead of copying the view code and reusing the template with the addition of injecting the revision ID in the form, turn it into a proper subclass of the EditView, and make use of Django's URL patterns to retrieve the revision ID in the EditView. This approach is similar to how reverting revisions is handled for snippets. Ideally, all the code for handling revisions revert should live in the RevisionsRevertView, and the EditView shouldn't know about it at all. This is how it's done for snippets: all the revisions revert-related code is put in RevisionsRevertMixin. However, this is currently not possible for pages without more significant refactoring, so this commit does the minimal change needed to achieve the goal of keeping the revisions_revert view in sync with the EditView. --- CHANGELOG.txt | 1 + docs/releases/6.4.md | 1 + .../templates/wagtailadmin/pages/edit.html | 6 +- wagtail/admin/tests/pages/test_revisions.py | 15 +- wagtail/admin/tests/test_audit_log.py | 6 +- wagtail/admin/urls/pages.py | 2 +- wagtail/admin/views/pages/edit.py | 67 +++++---- wagtail/admin/views/pages/revisions.py | 142 +++++------------- 8 files changed, 91 insertions(+), 149 deletions(-) diff --git a/CHANGELOG.txt b/CHANGELOG.txt index de62f786f6..dddb50f856 100644 --- a/CHANGELOG.txt +++ b/CHANGELOG.txt @@ -100,6 +100,7 @@ Changelog * Maintenance: Use built-in `venv` instead of `pipenv` in CircleCI (Sage Abdullah) * Maintenance: Add a new Stimulus `FormsetController` (`w-formset`) to support dynamic formset insertion/deletion behavior (LB (Ben) Johnston) * Maintenance: Enable breadcrumbs by default on admin views using generic templates (Sage Abdullah) + * Maintenance: Refactor pages `revisions_revert` view to be a subclass of `EditView` (Sage Abdullah) 6.3.2 (xx.xx.xxxx) - IN DEVELOPMENT diff --git a/docs/releases/6.4.md b/docs/releases/6.4.md index 5ef765a5ad..588051f9a9 100644 --- a/docs/releases/6.4.md +++ b/docs/releases/6.4.md @@ -118,6 +118,7 @@ depth: 1 * Use built-in `venv` instead of `pipenv` in CircleCI (Sage Abdullah) * Add a new Stimulus `FormsetController` (`w-formset`) to support dynamic formset insertion/deletion behavior (LB (Ben) Johnston) * Enable breadcrumbs by default on admin views using generic templates (Sage Abdullah) + * Refactor pages `revisions_revert` view to be a subclass of `EditView` (Sage Abdullah) ## Upgrade considerations - changes affecting all projects diff --git a/wagtail/admin/templates/wagtailadmin/pages/edit.html b/wagtail/admin/templates/wagtailadmin/pages/edit.html index d3381d4c8e..4f8dc7d355 100644 --- a/wagtail/admin/templates/wagtailadmin/pages/edit.html +++ b/wagtail/admin/templates/wagtailadmin/pages/edit.html @@ -14,7 +14,7 @@
{{ edit_handler.render_form_content }} - {% if is_revision %} - - {% endif %} - {% block footer %} {{ block.super }} {# Reuse the footer container, but redefine the actions content down below to use component classes #} diff --git a/wagtail/admin/tests/pages/test_revisions.py b/wagtail/admin/tests/pages/test_revisions.py index b1bee5ba02..712157702c 100644 --- a/wagtail/admin/tests/pages/test_revisions.py +++ b/wagtail/admin/tests/pages/test_revisions.py @@ -120,12 +120,17 @@ class TestRevisions(WagtailTestUtils, TestCase): # Form should show the content of the revision, not the current draft self.assertContains(response, "Last Christmas I gave you my heart") - # Form should include a hidden 'revision' field - revision_field = ( - """""" - % self.last_christmas_revision.id + # Form should use the revisions revert URL as the action + soup = self.get_soup(response.content) + form = soup.select_one("form[data-edit-form]") + self.assertIsNotNone(form) + self.assertEqual( + form.get("action"), + reverse( + "wagtailadmin_pages:revisions_revert", + args=(self.christmas_event.id, self.last_christmas_revision.id), + ), ) - self.assertContains(response, revision_field) # Buttons should be relabelled self.assertContains(response, "Replace current draft") diff --git a/wagtail/admin/tests/test_audit_log.py b/wagtail/admin/tests/test_audit_log.py index 8c189b54a5..4644c0d749 100644 --- a/wagtail/admin/tests/test_audit_log.py +++ b/wagtail/admin/tests/test_audit_log.py @@ -374,12 +374,14 @@ class TestAuditLogAdmin(AdminTemplateTestUtils, WagtailTestUtils, TestCase): self.login(user=self.administrator) response = self.client.post( - reverse("wagtailadmin_pages:edit", args=(self.hello_page.id,)), + reverse( + "wagtailadmin_pages:revisions_revert", + args=(self.hello_page.id, revision.id), + ), { "title": "Hello World!", "content": "another hello", "slug": "hello-world", - "revision": revision.id, "action-publish": "action-publish", }, follow=True, diff --git a/wagtail/admin/urls/pages.py b/wagtail/admin/urls/pages.py index fd8b4821df..162b0b0ed6 100644 --- a/wagtail/admin/urls/pages.py +++ b/wagtail/admin/urls/pages.py @@ -106,7 +106,7 @@ urlpatterns = [ ), path( "/revisions//revert/", - revisions.revisions_revert, + revisions.RevisionsRevertView.as_view(), name="revisions_revert", ), path( diff --git a/wagtail/admin/views/pages/edit.py b/wagtail/admin/views/pages/edit.py index 5dfe4dab48..6f9c9fa6ca 100644 --- a/wagtail/admin/views/pages/edit.py +++ b/wagtail/admin/views/pages/edit.py @@ -290,9 +290,7 @@ class EditView(WagtailAdminTemplateMixin, HookResponseMixin, View): reply.log_delete(page_revision=revision, user=self.request.user) def get_edit_message_button(self): - return messages.button( - reverse("wagtailadmin_pages:edit", args=(self.page.id,)), _("Edit") - ) + return messages.button(self.get_edit_url(), _("Edit")) def get_view_draft_message_button(self): return messages.button( @@ -320,7 +318,13 @@ class EditView(WagtailAdminTemplateMixin, HookResponseMixin, View): else: return self.page - def dispatch(self, request, page_id): + def get_object(self): + return self.real_page_record.get_latest_revision_as_object() + + def get_edit_url(self): + return reverse("wagtailadmin_pages:edit", args=(self.page.id,)) + + def dispatch(self, request, page_id, **kwargs): self.real_page_record = get_object_or_404( Page.objects.prefetch_workflow_states(), id=page_id ) @@ -340,7 +344,14 @@ class EditView(WagtailAdminTemplateMixin, HookResponseMixin, View): "back to a branch where the model class is still present." ) - self.page = self.real_page_record.get_latest_revision_as_object() + self.revision_id = kwargs.get("revision_id") + self.is_reverting = bool(self.revision_id) + self.previous_revision = None + if self.is_reverting: + self.previous_revision = get_object_or_404( + self.real_page_record.revisions, id=self.revision_id + ) + self.page = self.get_object() self.parent = self.page.get_parent() self.scheduled_page = self.real_page_record.get_scheduled_revision_as_object() @@ -393,9 +404,9 @@ class EditView(WagtailAdminTemplateMixin, HookResponseMixin, View): self.errors_debug = None - return super().dispatch(request) + return super().dispatch(request, page_id, **kwargs) - def get(self, request): + def get(self, request, *args, **kwargs): if self.lock: lock_message = self.lock.get_message(self.request.user) if lock_message: @@ -454,7 +465,7 @@ class EditView(WagtailAdminTemplateMixin, HookResponseMixin, View): ], ) - def post(self, request): + def post(self, request, *args, **kwargs): # Don't allow POST requests if the page is an alias if self.page.alias_of_id: # Return 405 "Method Not Allowed" response @@ -491,14 +502,6 @@ class EditView(WagtailAdminTemplateMixin, HookResponseMixin, View): return self.workflow_action in available_action_names def form_valid(self, form): - self.is_reverting = bool(self.request.POST.get("revision")) - # If a revision ID was passed in the form, get that revision so its - # date can be referenced in notification messages - if self.is_reverting: - self.previous_revision = get_object_or_404( - self.page.revisions, id=self.request.POST.get("revision") - ) - self.has_content_changes = self.form.has_changed() if self.request.POST.get("action-publish") and self.page_perms.can_publish(): @@ -533,7 +536,7 @@ class EditView(WagtailAdminTemplateMixin, HookResponseMixin, View): revision = self.page.save_revision( user=self.request.user, log_action=True, # Always log the new revision on edit - previous_revision=(self.previous_revision if self.is_reverting else None), + previous_revision=self.previous_revision, ) self.add_save_confirmation_message() @@ -558,7 +561,7 @@ class EditView(WagtailAdminTemplateMixin, HookResponseMixin, View): revision = self.page.save_revision( user=self.request.user, log_action=True, # Always log the new revision on edit - previous_revision=(self.previous_revision if self.is_reverting else None), + previous_revision=self.previous_revision, ) # store submitted go_live_at for messaging below @@ -572,7 +575,7 @@ class EditView(WagtailAdminTemplateMixin, HookResponseMixin, View): revision, user=self.request.user, changed=self.has_content_changes, - previous_revision=(self.previous_revision if self.is_reverting else None), + previous_revision=self.previous_revision, ) action.execute(skip_permission_checks=True) @@ -654,7 +657,7 @@ class EditView(WagtailAdminTemplateMixin, HookResponseMixin, View): revision = self.page.save_revision( user=self.request.user, log_action=True, # Always log the new revision on edit - previous_revision=(self.previous_revision if self.is_reverting else None), + previous_revision=self.previous_revision, ) if self.has_content_changes and "comments" in self.form.formsets: @@ -701,7 +704,7 @@ class EditView(WagtailAdminTemplateMixin, HookResponseMixin, View): revision = self.page.save_revision( user=self.request.user, log_action=True, # Always log the new revision on edit - previous_revision=(self.previous_revision if self.is_reverting else None), + previous_revision=self.previous_revision, ) if self.has_content_changes and "comments" in self.form.formsets: @@ -783,7 +786,7 @@ class EditView(WagtailAdminTemplateMixin, HookResponseMixin, View): revision = self.page.save_revision( user=self.request.user, log_action=True, # Always log the new revision on edit - previous_revision=(self.previous_revision if self.is_reverting else None), + previous_revision=self.previous_revision, ) if self.has_content_changes and "comments" in self.form.formsets: @@ -810,7 +813,7 @@ class EditView(WagtailAdminTemplateMixin, HookResponseMixin, View): return redirect("wagtailadmin_explore", self.page.get_parent().id) def redirect_and_remain(self): - target_url = reverse("wagtailadmin_pages:edit", args=[self.page.id]) + target_url = self.get_edit_url() if self.next_url: # Ensure the 'next' url is passed through again if present target_url += "?next=%s" % quote(self.next_url) @@ -905,19 +908,22 @@ class EditView(WagtailAdminTemplateMixin, HookResponseMixin, View): self.page.latest_revision_id, ) - def get_context_data(self, **kwargs): - context = super().get_context_data(**kwargs) - user_perms = self.page.permissions_for_user(self.request.user) - bound_panel = self.edit_handler.get_bound_panel( - instance=self.page, request=self.request, form=self.form - ) - action_menu = PageActionMenu( + def get_action_menu(self): + return PageActionMenu( self.request, view="edit", page=self.page, lock=self.lock, locked_for_user=self.locked_for_user, ) + + def get_context_data(self, **kwargs): + context = super().get_context_data(**kwargs) + user_perms = self.page.permissions_for_user(self.request.user) + bound_panel = self.edit_handler.get_bound_panel( + instance=self.page, request=self.request, form=self.form + ) + action_menu = self.get_action_menu() side_panels = self.get_side_panels() media = MediaContainer([bound_panel, self.form, action_menu, side_panels]).media @@ -933,6 +939,7 @@ class EditView(WagtailAdminTemplateMixin, HookResponseMixin, View): "side_panels": side_panels, "form": self.form, "next": self.next_url, + "action_url": self.get_edit_url(), "history_url": self.get_history_url(), "has_unsaved_changes": self.has_unsaved_changes, "page_locked": self.locked_for_user, diff --git a/wagtail/admin/views/pages/revisions.py b/wagtail/admin/views/pages/revisions.py index fdfc1ea06d..06a44731a0 100644 --- a/wagtail/admin/views/pages/revisions.py +++ b/wagtail/admin/views/pages/revisions.py @@ -1,9 +1,6 @@ -from django.conf import settings -from django.contrib.contenttypes.models import ContentType from django.core.exceptions import PermissionDenied from django.shortcuts import get_object_or_404, redirect from django.template.loader import render_to_string -from django.template.response import TemplateResponse from django.urls import reverse from django.utils.decorators import method_decorator from django.utils.safestring import mark_safe @@ -12,18 +9,12 @@ from django.utils.translation import gettext as _ from wagtail.admin import messages from wagtail.admin.action_menu import PageActionMenu from wagtail.admin.auth import user_has_any_page_permission, user_passes_test -from wagtail.admin.ui.components import MediaContainer -from wagtail.admin.ui.side_panels import ( - ChecksSidePanel, - CommentsSidePanel, - PageStatusSidePanel, - PreviewSidePanel, -) from wagtail.admin.views.generic.models import ( RevisionsCompareView, RevisionsUnscheduleView, ) from wagtail.admin.views.generic.preview import PreviewRevision +from wagtail.admin.views.pages.edit import EditView from wagtail.admin.views.pages.utils import GenericPageBreadcrumbsMixin from wagtail.models import Page from wagtail.utils.timestamps import render_timestamp @@ -33,116 +24,55 @@ def revisions_index(request, page_id): return redirect("wagtailadmin_pages:history", page_id) -def revisions_revert(request, page_id, revision_id): - # TODO: refactor this into a class-based view that extends the EditView - page = get_object_or_404(Page, id=page_id).specific - page_perms = page.permissions_for_user(request.user) - if not page_perms.can_edit(): - raise PermissionDenied +class RevisionsRevertView(EditView): + revisions_revert_url_name = "wagtailadmin_pages:revisions_revert" - revision = get_object_or_404(page.revisions, id=revision_id) - revision_page = revision.as_object() + def get_action_menu(self): + return PageActionMenu( + self.request, + view="revisions_revert", + is_revision=True, + page=self.page, + lock=self.lock, + locked_for_user=self.locked_for_user, + ) - scheduled_page = page.get_scheduled_revision_as_object() + def get(self, request, *args, **kwargs): + self._add_warning_message() + return super().get(request, *args, **kwargs) - content_type = ContentType.objects.get_for_model(page) - page_class = content_type.model_class() + def _add_warning_message(self): + messages.warning(self.request, self.get_warning_message()) - if getattr(settings, "WAGTAIL_I18N_ENABLED", False): - locale = page.locale - translations = [ - { - "locale": translation.locale, - "url": reverse("wagtailadmin_pages:edit", args=[translation.id]), - } - for translation in page.get_translations() - .only("id", "locale", "depth") - .select_related("locale") - if translation.permissions_for_user(request.user).can_edit() - ] - else: - locale = None - translations = [] + def get_object(self): + return self.previous_revision.as_object() - edit_handler = page_class.get_edit_handler() - form_class = edit_handler.get_form_class() + def get_revisions_revert_url(self): + return reverse( + self.revisions_revert_url_name, + args=[self.page.pk, self.revision_id], + ) - form = form_class(instance=revision_page, for_user=request.user) - edit_handler = edit_handler.get_bound_panel( - instance=revision_page, request=request, form=form - ) + def get_warning_message(self): + user_avatar = render_to_string( + "wagtailadmin/shared/user_avatar.html", + {"user": self.previous_revision.user}, + ) - preview_url = reverse("wagtailadmin_pages:preview_on_edit", args=[page.id]) - lock = page.get_lock() - - action_menu = PageActionMenu( - request, - view="revisions_revert", - is_revision=True, - page=page, - lock=lock, - locked_for_user=lock is not None and lock.for_user(request.user), - ) - side_panels = [ - PageStatusSidePanel( - revision_page, - request, - show_schedule_publishing_toggle=form.show_schedule_publishing_toggle, - live_object=page, - scheduled_object=scheduled_page, - locale=locale, - translations=translations, - ), - ] - if page.is_previewable(): - side_panels.append(PreviewSidePanel(page, request, preview_url=preview_url)) - side_panels.append(ChecksSidePanel(page, request)) - if form.show_comments_toggle: - side_panels.append(CommentsSidePanel(page, request)) - side_panels = MediaContainer(side_panels) - - media = MediaContainer([edit_handler, form, action_menu, side_panels]).media - - user_avatar = render_to_string( - "wagtailadmin/shared/user_avatar.html", {"user": revision.user} - ) - - messages.warning( - request, - mark_safe( + return mark_safe( _( "You are viewing a previous version of this page from %(created_at)s by %(user)s" ) % { - "created_at": render_timestamp(revision.created_at), + "created_at": render_timestamp(self.previous_revision.created_at), "user": user_avatar, } - ), - ) + ) - page_title = _("Editing %(page_type)s") % { - "page_type": page_class.get_verbose_name() - } - page_subtitle = page.get_admin_display_title() - header_title = f"{page_title}: {page_subtitle}" - - return TemplateResponse( - request, - "wagtailadmin/pages/edit.html", - { - "page": page, - "revision": revision, - "is_revision": True, - "content_type": content_type, - "edit_handler": edit_handler, - "errors_debug": None, - "action_menu": action_menu, - "side_panels": side_panels, - "header_title": header_title, - "form": form, # Used in unit tests - "media": media, - }, - ) + def get_context_data(self, **kwargs): + context = super().get_context_data(**kwargs) + context["action_url"] = self.get_revisions_revert_url() + return context @method_decorator(user_passes_test(user_has_any_page_permission), name="dispatch")