From 3a7e3ca4ce14dc82593ad078423d3166d8258947 Mon Sep 17 00:00:00 2001 From: zerolab Date: Thu, 14 Nov 2024 16:51:33 +0000 Subject: [PATCH 01/13] Add bulk of the Bundles work from Alpha tidied up a bit, linted and type hinted f init f init --- cms/bundles/__init__.py | 0 cms/bundles/admin_forms.py | 33 ++ cms/bundles/admin_urls.py | 8 + cms/bundles/apps.py | 8 + cms/bundles/enums.py | 18 + cms/bundles/forms.py | 85 +++++ cms/bundles/management/__init__.py | 0 cms/bundles/management/commands/__init__.py | 0 .../management/commands/publish_bundles.py | 121 +++++++ .../publish_scheduled_without_bundles.py | 102 ++++++ cms/bundles/migrations/0001_initial.py | 88 +++++ cms/bundles/migrations/__init__.py | 0 cms/bundles/models.py | 201 ++++++++++++ cms/bundles/notifications.py | 104 ++++++ cms/bundles/panels.py | 50 +++ .../bundles/wagtailadmin/add_to_bundle.html | 29 ++ .../templates/bundles/wagtailadmin/edit.html | 14 + .../bundles/wagtailadmin/inspect.html | 5 + .../wagtailadmin/panels/latest_bundles.html | 59 ++++ cms/bundles/views.py | 104 ++++++ cms/bundles/viewsets.py | 308 ++++++++++++++++++ cms/bundles/wagtail_hooks.py | 199 +++++++++++ cms/core/wagtail_hooks.py | 1 + cms/jinja2/assets/icons/boxes-stacked.svg | 1 + cms/settings/base.py | 4 + poetry.lock | 16 +- pyproject.toml | 1 + 27 files changed, 1558 insertions(+), 1 deletion(-) create mode 100644 cms/bundles/__init__.py create mode 100644 cms/bundles/admin_forms.py create mode 100644 cms/bundles/admin_urls.py create mode 100644 cms/bundles/apps.py create mode 100644 cms/bundles/enums.py create mode 100644 cms/bundles/forms.py create mode 100644 cms/bundles/management/__init__.py create mode 100644 cms/bundles/management/commands/__init__.py create mode 100644 cms/bundles/management/commands/publish_bundles.py create mode 100644 cms/bundles/management/commands/publish_scheduled_without_bundles.py create mode 100644 cms/bundles/migrations/0001_initial.py create mode 100644 cms/bundles/migrations/__init__.py create mode 100644 cms/bundles/models.py create mode 100644 cms/bundles/notifications.py create mode 100644 cms/bundles/panels.py create mode 100644 cms/bundles/templates/bundles/wagtailadmin/add_to_bundle.html create mode 100644 cms/bundles/templates/bundles/wagtailadmin/edit.html create mode 100644 cms/bundles/templates/bundles/wagtailadmin/inspect.html create mode 100644 cms/bundles/templates/bundles/wagtailadmin/panels/latest_bundles.html create mode 100644 cms/bundles/views.py create mode 100644 cms/bundles/viewsets.py create mode 100644 cms/bundles/wagtail_hooks.py create mode 100644 cms/jinja2/assets/icons/boxes-stacked.svg diff --git a/cms/bundles/__init__.py b/cms/bundles/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/cms/bundles/admin_forms.py b/cms/bundles/admin_forms.py new file mode 100644 index 00000000..99ead3db --- /dev/null +++ b/cms/bundles/admin_forms.py @@ -0,0 +1,33 @@ +from typing import Any + +from django import forms +from django.core.exceptions import ValidationError +from django.utils.translation import gettext as _ + +from .models import Bundle +from .viewsets import BundleChooserWidget + + +class AddToBundleForm(forms.Form): + """Administrative form used in the 'add to bundle' view.""" + + def __init__(self, *args: Any, **kwargs: Any) -> None: + self.page_to_add = kwargs.pop("page_to_add") + + super().__init__(*args, **kwargs) + + self.fields["bundle"] = forms.ModelChoiceField( + queryset=Bundle.objects.editable(), + widget=BundleChooserWidget(), + label=_("Bundle"), + help_text=_("Select a bundle for this page."), + ) + + def clean(self) -> None: + super().clean() + + bundle = self.cleaned_data.get("bundle") + if bundle and bundle.bundled_pages.filter(page=self.page_to_add).exists(): + raise ValidationError( + {"bundle": f"Page {self.page_to_add.get_admin_display_title()} is already in bundle '{bundle}'"} + ) diff --git a/cms/bundles/admin_urls.py b/cms/bundles/admin_urls.py new file mode 100644 index 00000000..872ceae2 --- /dev/null +++ b/cms/bundles/admin_urls.py @@ -0,0 +1,8 @@ +from django.urls import path + +from . import views + +app_name = "bundles" +urlpatterns = [ + path("add//", views.add_to_bundle, name="add_to_bundle"), +] diff --git a/cms/bundles/apps.py b/cms/bundles/apps.py new file mode 100644 index 00000000..2e984971 --- /dev/null +++ b/cms/bundles/apps.py @@ -0,0 +1,8 @@ +from django.apps import AppConfig + + +class BundlesAppConfig(AppConfig): + """The bundles app config.""" + + default_auto_field = "django.db.models.AutoField" + name = "cms.bundles" diff --git a/cms/bundles/enums.py b/cms/bundles/enums.py new file mode 100644 index 00000000..4090a914 --- /dev/null +++ b/cms/bundles/enums.py @@ -0,0 +1,18 @@ +from django.db import models +from django.utils.translation import gettext_lazy as _ + + +class BundleStatus(models.TextChoices): + """The bundle statuses.""" + + PENDING = "PENDING", _("Pending") + IN_REVIEW = "IN_REVIEW", _("In Review") + APPROVED = "APPROVED", _("Approved") + RELEASED = "RELEASED", _("Released") + + +ACTIVE_BUNDLE_STATUSES = [BundleStatus.PENDING, BundleStatus.IN_REVIEW, BundleStatus.APPROVED] +ACTIVE_BUNDLE_STATUS_CHOICES = [ + (BundleStatus[choice].value, BundleStatus[choice].label) for choice in ACTIVE_BUNDLE_STATUSES +] +EDITABLE_BUNDLE_STATUSES = [BundleStatus.PENDING, BundleStatus.IN_REVIEW] diff --git a/cms/bundles/forms.py b/cms/bundles/forms.py new file mode 100644 index 00000000..aebb6c18 --- /dev/null +++ b/cms/bundles/forms.py @@ -0,0 +1,85 @@ +from typing import TYPE_CHECKING, Any + +from django import forms +from django.core.exceptions import ValidationError +from django.utils import timezone +from wagtail.admin.forms import WagtailAdminModelForm + +from cms.bundles.enums import ACTIVE_BUNDLE_STATUS_CHOICES, EDITABLE_BUNDLE_STATUSES, BundleStatus + +if TYPE_CHECKING: + from .models import Bundle + + +class BundleAdminForm(WagtailAdminModelForm): + """The Bundle admin form used in the add/edit interface.""" + + instance: "Bundle" + + def __init__(self, *args: Any, **kwargs: Any) -> None: + """Helps the form initialisation. + + - Hides the "Released" status choice as that happens on publish + - disabled/hide the approved at/by fields + """ + super().__init__(*args, **kwargs) + # hide the "Released" status choice + if self.instance.status in EDITABLE_BUNDLE_STATUSES: + self.fields["status"].choices = ACTIVE_BUNDLE_STATUS_CHOICES + elif self.instance.status == BundleStatus.APPROVED.value: + for field_name in self.fields: + if field_name != "status": + self.fields[field_name].disabled = True + + # fully hide and disable the approved_at/by fields to prevent form tampering + self.fields["approved_at"].disabled = True + self.fields["approved_at"].widget = forms.HiddenInput() + self.fields["approved_by"].disabled = True + self.fields["approved_by"].widget = forms.HiddenInput() + + self.original_status = self.instance.status + + def _validate_bundled_pages(self) -> None: + """Validates and tidies up related pages. + + - if we have an empty page reference, remove it form the form data + - ensure the selected page is not in another active bundle. + """ + for idx, form in enumerate(self.formsets["bundled_pages"].forms): + if not form.is_valid(): + continue + + page = form.clean().get("page") + if page is None: + # tidy up in case the page reference is empty + self.formsets["bundled_pages"].forms[idx].cleaned_data["DELETE"] = True + else: + page = page.specific + if page.in_active_bundle and page.active_bundle != self.instance and not form.cleaned_data["DELETE"]: + raise ValidationError(f"{page} is already in an active bundle ({page.active_bundle})") + + def clean(self) -> dict[str, Any] | None: + """Validates the form. + + - the bundle cannot be self-approved. That is, someone other than the bundle creator must approve it. + - tidies up/ populates approved at/by + """ + cleaned_data: dict[str, Any] = super().clean() + + self._validate_bundled_pages() + + status = cleaned_data["status"] + if self.instance.status != status: + # the status has changed, let's check + if status == BundleStatus.APPROVED: + if self.instance.created_by_id == self.for_user.pk: + cleaned_data["status"] = self.instance.status + self.add_error("status", ValidationError("You cannot self-approve your own bundle!")) + + cleaned_data["approved_at"] = timezone.now() + cleaned_data["approved_by"] = self.for_user + elif self.instance.status == BundleStatus.APPROVED: + cleaned_data["approved_at"] = None + cleaned_data["approved_by"] = None + + return cleaned_data diff --git a/cms/bundles/management/__init__.py b/cms/bundles/management/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/cms/bundles/management/commands/__init__.py b/cms/bundles/management/commands/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/cms/bundles/management/commands/publish_bundles.py b/cms/bundles/management/commands/publish_bundles.py new file mode 100644 index 00000000..a79ad034 --- /dev/null +++ b/cms/bundles/management/commands/publish_bundles.py @@ -0,0 +1,121 @@ +import logging +import time +import uuid +from typing import TYPE_CHECKING, Any + +from django.conf import settings +from django.core.management.base import BaseCommand +from django.db import transaction +from django.urls import reverse +from django.utils import timezone +from wagtail.log_actions import log + +from cms.bundles.enums import BundleStatus +from cms.bundles.models import Bundle +from cms.bundles.notifications import notify_slack_of_publication_start, notify_slack_of_publish_end +from cms.release_calendar.enums import ReleaseStatus + +logger = logging.getLogger(__name__) + +if TYPE_CHECKING: + from django.core.management.base import CommandParser + + +class Command(BaseCommand): + """The management command class for bundled publishing.""" + + base_url: str = "" + + def add_arguments(self, parser: "CommandParser") -> None: + parser.add_argument( + "--dry-run", + action="store_true", + dest="dry_run", + default=False, + help="Dry run -- don't change anything.", + ) + + def _update_related_release_calendar_page(self, bundle: Bundle) -> None: + """Updates the release calendar page related to the bundle with the pages in the bundle.""" + content = [] + pages = [] + for page in bundle.get_bundled_pages(): + pages.append( + { + "id": uuid.uuid4(), + "type": "item", + "value": {"page": page.pk, "title": "", "description": "", "external_url": ""}, + } + ) + if pages: + content.append({"type": "release_content", "value": {"title": "Publications", "links": pages}}) + + page = bundle.release_calendar_page + page.content = content + page.status = ReleaseStatus.PUBLISHED + revision = page.save_revision(log_action=True) + revision.publish() + + @transaction.atomic + def handle_bundle(self, bundle: Bundle) -> None: + """Manages the bundle publication. + + - published related pages + - updates the release calendar entry + """ + # only provide a URL if we can generate a full one + inspect_url = self.base_url + reverse("bundle:inspect", args=(bundle.pk,)) if self.base_url else None + + logger.info("Publishing bundle=%d", bundle.id) + start_time = time.time() + notify_slack_of_publication_start(bundle, url=inspect_url) + for page in bundle.get_bundled_pages(): + if (revision := page.scheduled_revision) is None: + continue + # just run publish for the revision -- since the approved go + # live datetime is before now it will make the object live + revision.publish(log_action="wagtail.publish.scheduled") + + # update the related release calendar and publish + if bundle.release_calendar_page_id: + self._update_related_release_calendar_page(bundle) + + bundle.status = BundleStatus.RELEASED + bundle.save() + publish_duration = time.time() - start_time + logger.info("Published bundle=%d duration=%.3fms", bundle.id, publish_duration * 1000) + + notify_slack_of_publish_end(bundle, publish_duration, url=inspect_url) + + log(action="wagtail.publish.scheduled", instance=bundle) + + def handle(self, *args: Any, **options: dict[str, Any]) -> None: + """The management command handler.""" + dry_run = False + if options["dry_run"]: + self.stdout.write("Will do a dry run.") + dry_run = True + + self.base_url = getattr(settings, "WAGTAILADMIN_BASE_URL", "") + + bundles_to_publish = Bundle.objects.filter(status=BundleStatus.APPROVED, release_date__lte=timezone.now()) + if dry_run: + self.stdout.write("\n---------------------------------") + if bundles_to_publish: + self.stdout.write("Bundles to be published:") + for bundle in bundles_to_publish: + self.stdout.write(f"- {bundle.name}") + bundled_pages = [ + f"{page.get_admin_display_title()} ({page.__class__.__name__})" + for page in bundle.get_bundled_pages().specific() + ] + self.stdout.write(f' Pages: {"\n\t ".join(bundled_pages)}') + + else: + self.stdout.write("No bundles to go live.") + else: + for bundle in bundles_to_publish: + try: + self.handle_bundle(bundle) + except Exception: # pylint: disable=broad-exception-caught + logger.exception("Publish failed bundle=%d", bundle.id) diff --git a/cms/bundles/management/commands/publish_scheduled_without_bundles.py b/cms/bundles/management/commands/publish_scheduled_without_bundles.py new file mode 100644 index 00000000..ed44c986 --- /dev/null +++ b/cms/bundles/management/commands/publish_scheduled_without_bundles.py @@ -0,0 +1,102 @@ +from typing import TYPE_CHECKING, Any + +from django.apps import apps +from django.core.management.base import BaseCommand +from django.utils import timezone +from wagtail.models import DraftStateMixin, Page, Revision + +from cms.bundles.models import BundledPageMixin + +if TYPE_CHECKING: + from django.core.management.base import CommandParser + + +class Command(BaseCommand): + """A copy of Wagtail's publish_scheduled management command that excludes bundled objects.""" + + def add_arguments(self, parser: "CommandParser") -> None: + parser.add_argument( + "--dry-run", + action="store_true", + dest="dry-run", + default=False, + help="Dry run -- don't change anything.", + ) + + def handle(self, *args: Any, **options: dict[str, Any]) -> None: + dry_run = False + if options["dry-run"]: + self.stdout.write("Will do a dry run.") + dry_run = True + + self._unpublish_expired(dry_run) + self._publish_scheduled_without_bundles(dry_run) + + def _unpublish_expired(self, dry_run: bool) -> None: + models = [Page] + models += [ + model for model in apps.get_models() if issubclass(model, DraftStateMixin) and not issubclass(model, Page) + ] + # 1. get all expired objects with live = True + expired_objects = [] + for model in models: + expired_objects += [model.objects.filter(live=True, expire_at__lt=timezone.now()).order_by("expire_at")] + if dry_run: + self.stdout.write("\n---------------------------------") + if expired_objects: + self.stdout.write("Expired objects to be deactivated:") + self.stdout.write("Expiry datetime\t\tModel\t\tSlug\t\tName") + self.stdout.write("---------------\t\t-----\t\t----\t\t----") + for queryset in expired_objects: + if queryset.model is Page: + for obj in queryset: + self.stdout.write( + f'{obj.expire_at.strftime("%Y-%m-%d %H:%M")}\t' + f"{obj.specific_class.__name__}\t{obj.slug}\t{obj.title}" + ) + else: + for obj in queryset: + self.stdout.write( + f'{obj.expire_at.strftime("%Y-%m-%d %H:%M")}\t' + f"{queryset.model.__name__}\t\t\t{obj!s}" + ) + else: + self.stdout.write("No expired objects to be deactivated found.") + else: + # Unpublish the expired objects + for queryset in expired_objects: + # Cast to list to make sure the query is fully evaluated + # before unpublishing anything + for obj in list(queryset): + obj.unpublish(set_expired=True, log_action="wagtail.unpublish.scheduled") + + def _publish_scheduled_without_bundles(self, dry_run: bool) -> None: + # 2. get all revisions that need to be published + preliminary_revs_for_publishing = Revision.objects.filter(approved_go_live_at__lt=timezone.now()).order_by( + "approved_go_live_at" + ) + revs_for_publishing = [] + for rev in preliminary_revs_for_publishing: + content_object = rev.as_object() + if not isinstance(content_object, BundledPageMixin) or not content_object.in_active_bundle: + revs_for_publishing.append(rev) + if dry_run: + self.stdout.write("\n---------------------------------") + if revs_for_publishing: + self.stdout.write("Revisions to be published:") + self.stdout.write("Go live datetime\tModel\t\tSlug\t\tName") + self.stdout.write("----------------\t-----\t\t----\t\t----") + for rp in revs_for_publishing: + model = rp.content_type.model_class() + rev_data = rp.content + self.stdout.write( + f'{rp.approved_go_live_at.strftime("%Y-%m-%d %H:%M")}\t' + f'{model.__name__}\t{rev_data.get("slug", "")}\t\t{rev_data.get("title", rp.object_str)}' + ) + else: + self.stdout.write("No objects to go live.") + else: + for rp in revs_for_publishing: + # just run publish for the revision -- since the approved go + # live datetime is before now it will make the object live + rp.publish(log_action="wagtail.publish.scheduled") diff --git a/cms/bundles/migrations/0001_initial.py b/cms/bundles/migrations/0001_initial.py new file mode 100644 index 00000000..ed323099 --- /dev/null +++ b/cms/bundles/migrations/0001_initial.py @@ -0,0 +1,88 @@ +# Generated by Django 5.1.3 on 2024-11-14 12:49 + +import django.db.models.deletion +import modelcluster.fields +import wagtail.search.index +from django.conf import settings +from django.db import migrations, models + + +class Migration(migrations.Migration): + initial = True + + dependencies = [ + ("release_calendar", "0002_create_releasecalendarindex"), + ("wagtailcore", "0094_alter_page_locale"), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.CreateModel( + name="Bundle", + fields=[ + ("id", models.AutoField(auto_created=True, primary_key=True, serialize=False)), + ("name", models.CharField(max_length=255)), + ("created_at", models.DateTimeField(auto_now_add=True)), + ("approved_at", models.DateTimeField(blank=True, null=True)), + ("publication_date", models.DateTimeField(blank=True, null=True)), + ("status", models.CharField(default="PENDING", max_length=32)), + ( + "approved_by", + models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + related_name="approved_bundles", + to=settings.AUTH_USER_MODEL, + ), + ), + ( + "created_by", + models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + related_name="bundles", + to=settings.AUTH_USER_MODEL, + ), + ), + ( + "release_calendar_page", + models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + related_name="bundles", + to="release_calendar.releasecalendarpage", + ), + ), + ], + options={ + "abstract": False, + }, + bases=(wagtail.search.index.Indexed, models.Model), + ), + migrations.CreateModel( + name="BundlePage", + fields=[ + ("id", models.AutoField(auto_created=True, primary_key=True, serialize=False)), + ("sort_order", models.IntegerField(blank=True, editable=False, null=True)), + ( + "page", + models.ForeignKey( + blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, to="wagtailcore.page" + ), + ), + ( + "parent", + modelcluster.fields.ParentalKey( + on_delete=django.db.models.deletion.CASCADE, related_name="bundled_pages", to="bundles.bundle" + ), + ), + ], + options={ + "ordering": ["sort_order"], + "abstract": False, + }, + ), + ] diff --git a/cms/bundles/migrations/__init__.py b/cms/bundles/migrations/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/cms/bundles/models.py b/cms/bundles/models.py new file mode 100644 index 00000000..b8a20f24 --- /dev/null +++ b/cms/bundles/models.py @@ -0,0 +1,201 @@ +from typing import TYPE_CHECKING, Any, ClassVar, Optional, Self + +from django.db import models +from django.db.models import F, QuerySet +from django.db.models.functions import Coalesce +from django.utils.functional import cached_property +from django.utils.timezone import now +from django.utils.translation import gettext_lazy as _ +from modelcluster.fields import ParentalKey +from modelcluster.models import ClusterableModel +from wagtail.admin.panels import FieldPanel, FieldRowPanel, InlinePanel +from wagtail.models import Orderable, Page +from wagtail.search import index + +from .enums import ACTIVE_BUNDLE_STATUSES, EDITABLE_BUNDLE_STATUSES, BundleStatus +from .forms import BundleAdminForm +from .panels import BundleNotePanel, PageChooserWithStatusPanel + +if TYPE_CHECKING: + import datetime + + from wagtail.admin.panels import Panel + + +class BundlePage(Orderable): + parent = ParentalKey("Bundle", related_name="bundled_pages", on_delete=models.CASCADE) + page = models.ForeignKey( # type: ignore[var-annotated] + "wagtailcore.Page", blank=True, null=True, on_delete=models.SET_NULL + ) + + panels: ClassVar[list["Panel"]] = [ + PageChooserWithStatusPanel("page", ["analysis.AnalysisPage"]), + ] + + def __str__(self) -> str: + return f"BundlePage: page {self.page_id} in bundle {self.parent_id}" + + +class BundlesQuerySet(QuerySet): + def active(self) -> Self: + """Provides a pre-filtered queryset for active bundles. Usage: Bundle.objects.active().""" + return self.filter(status__in=ACTIVE_BUNDLE_STATUSES) + + def editable(self) -> Self: + """Provides a pre-filtered queryset for editable bundles. Usage: Bundle.objects.editable().""" + return self.filter(status__in=EDITABLE_BUNDLE_STATUSES) + + +# note: mypy doesn't cope with dynamic base classes and fails with: +# 'Unsupported dynamic base class "models.Manager.from_queryset" [misc]' +# @see https://github.com/python/mypy/issues/2477 +class BundleManager(models.Manager.from_queryset(BundlesQuerySet)): # type: ignore[misc] + def get_queryset(self) -> BundlesQuerySet: + """Augments the queryset to order it by the publication date, then name, then reverse id.""" + queryset: BundlesQuerySet = super().get_queryset() + queryset = queryset.annotate( + release_date=Coalesce("publication_date", "release_calendar_page__release_date") + ).order_by(F("release_date").desc(nulls_last=True), "name", "-pk") + return queryset + + +class Bundle(index.Indexed, ClusterableModel, models.Model): # type: ignore[django-manager-missing] + base_form_class = BundleAdminForm + + name = models.CharField(max_length=255) + created_at = models.DateTimeField(auto_now_add=True) + created_by = models.ForeignKey( + "users.User", + null=True, + blank=True, + on_delete=models.SET_NULL, + related_name="bundles", + ) + # @see https://docs.wagtail.org/en/stable/advanced_topics/reference_index.html + created_by.wagtail_reference_index_ignore = True # type: ignore[attr-defined] + + approved_at = models.DateTimeField(blank=True, null=True) + approved_by = models.ForeignKey( + "users.User", + null=True, + blank=True, + on_delete=models.SET_NULL, + related_name="approved_bundles", + ) + approved_by.wagtail_reference_index_ignore = True # type: ignore[attr-defined] + + publication_date = models.DateTimeField(blank=True, null=True) + release_calendar_page = models.ForeignKey( + "release_calendar.ReleaseCalendarPage", + null=True, + blank=True, + on_delete=models.SET_NULL, + related_name="bundles", + ) + status = models.CharField(choices=BundleStatus.choices, default=BundleStatus.PENDING, max_length=32) + + objects = BundleManager() + + panels: ClassVar[list["Panel"]] = [ + FieldPanel("name"), + FieldRowPanel( + [ + FieldPanel("release_calendar_page", heading="Release Calendar page"), + FieldPanel("publication_date", heading="or Publication date"), + ], + heading=_("Scheduling"), + icon="calendar", + ), + FieldPanel("status"), + InlinePanel("bundled_pages", heading=_("Bundled pages"), icon="doc-empty", label=_("Page")), + # these are handled by the form + FieldPanel("approved_by", classname="hidden w-hidden"), + FieldPanel("approved_at", classname="hidden w-hidden"), + ] + + search_fields: ClassVar[list[index.SearchField | index.AutocompleteField]] = [ + index.SearchField("name"), + index.AutocompleteField("name"), + ] + + def __str__(self) -> str: + return str(self.name) + + @cached_property + def scheduled_publication_date(self) -> Optional["datetime.datetime"]: + """Returns the direct publication date or the linked release calendar page, if set.""" + date: datetime.datetime | None = self.publication_date + if not date and self.release_calendar_page_id: + date = self.release_calendar_page.release_date # type: ignore[union-attr] + return date + + @property + def can_be_approved(self) -> bool: + """Determines + Note: strictly speaking, the bundle should in "in review" in order for it to be approved. + """ + return self.status in [BundleStatus.PENDING, BundleStatus.IN_REVIEW] + + def get_bundled_pages(self) -> QuerySet[Page]: + """Convenience method to return all bundled pages.""" + pages: QuerySet[Page] = Page.objects.filter(pk__in=self.bundled_pages.values_list("page__pk", flat=True)) + return pages + + def save(self, **kwargs: Any) -> None: # type: ignore[override] + """Adds additional behaviour on bundle saving. + + For non-released bundles, we update the publication date for related pages if needed. + """ + super().save(**kwargs) + + if self.status == BundleStatus.RELEASED: + return + + if self.scheduled_publication_date and self.scheduled_publication_date >= now(): + # Schedule publishing for related pages. + # ignoring [attr-defined] because Wagtail is not fully typed and mypy is confused. + for bundled_page in self.get_bundled_pages().defer_streamfields().specific(): # type: ignore[attr-defined] + if bundled_page.go_live_at == self.scheduled_publication_date: + continue + + # note: this could use a custom log action for history + bundled_page.go_live_at = self.scheduled_publication_date + revision = bundled_page.save_revision() + revision.publish() + + +class BundledPageMixin: + """A helper page mixin for bundled content. + + Add it to Page classes that should be in bundles. + """ + + panels: ClassVar[list["Panel"]] = [BundleNotePanel(heading="Bundle", icon="boxes-stacked")] + + @cached_property + def bundles(self) -> QuerySet[Bundle]: + """Return all bundles this instance belongs to.""" + queryset: QuerySet[Bundle] = Bundle.objects.none() + if self.pk: # type: ignore[attr-defined] + queryset = Bundle.objects.filter( + pk__in=self.bundlepage_set.all().values_list("parent", flat=True) # type: ignore[attr-defined] + ) + return queryset + + @cached_property + def active_bundles(self) -> QuerySet[Bundle]: + """Returns the active bundles this instance belongs to. In theory, it should be only one.""" + return self.bundles.filter(status__in=ACTIVE_BUNDLE_STATUSES) + + @cached_property + def in_active_bundle(self) -> bool: + """Determines whether this instance is in an active bundle (that is not yet released).""" + exists: bool = self.bundlepage_set.filter( # type: ignore[attr-defined] + parent__status__in=ACTIVE_BUNDLE_STATUSES + ).exists() + return exists + + @property + def active_bundle(self) -> Bundle | None: + """Helper to return the active bundle this instance is in.""" + return self.active_bundles.first() diff --git a/cms/bundles/notifications.py b/cms/bundles/notifications.py new file mode 100644 index 00000000..df98f60f --- /dev/null +++ b/cms/bundles/notifications.py @@ -0,0 +1,104 @@ +import logging +from http import HTTPStatus +from typing import TYPE_CHECKING, Optional + +from django.conf import settings +from slack_sdk.webhook import WebhookClient + +from cms.bundles.models import Bundle + +if TYPE_CHECKING: + from cms.users.models import User + + +logger = logging.getLogger("cms.bundles") + + +def notify_slack_of_status_change( + bundle: Bundle, old_status: str, user: Optional["User"] = None, url: str | None = None +) -> None: + """Sends a Slack notification for Bundle status changes.""" + if (webhook_url := settings.SLACK_NOTIFICATIONS_WEBHOOK_URL) is None: + return + + client = WebhookClient(webhook_url) + + fields = [ + {"title": "Title", "value": bundle.name, "short": True}, + {"title": "Changed by", "value": user.get_full_name() if user else "System", "short": True}, + {"title": "Old status", "value": old_status, "short": True}, + {"title": "New status", "value": bundle.get_status_display(), "short": True}, + ] + if url: + fields.append( + {"title": "Link", "value": url, "short": False}, + ) + + response = client.send( + text="Bundle status changed", + attachments=[{"color": "good", "fields": fields}], + unfurl_links=False, + unfurl_media=False, + ) + + if response.status_code != HTTPStatus.OK: + logger.error("Unable to notify Slack of bundle status change: %s", response.body) + + +def notify_slack_of_publication_start(bundle: Bundle, user: Optional["User"] = None, url: str | None = None) -> None: + """Sends a Slack notification for Bundle publication start.""" + if (webhook_url := settings.SLACK_NOTIFICATIONS_WEBHOOK_URL) is None: + return + + client = WebhookClient(webhook_url) + + fields = [ + {"title": "Title", "value": bundle.name, "short": True}, + {"title": "User", "value": user.get_full_name() if user else "System", "short": True}, + {"title": "Pages", "value": bundle.get_bundled_pages().count(), "short": True}, + ] + if url: + fields.append( + {"title": "Link", "value": url, "short": False}, + ) + + response = client.send( + text="Starting bundle publication", + attachments=[{"color": "good", "fields": fields}], + unfurl_links=False, + unfurl_media=False, + ) + + if response.status_code != HTTPStatus.OK: + logger.error("Unable to notify Slack of bundle status change: %s", response.body) + + +def notify_slack_of_publish_end( + bundle: Bundle, elapsed: float, user: Optional["User"] = None, url: str | None = None +) -> None: + """Sends a Slack notification for Bundle publication end.""" + if (webhook_url := settings.SLACK_NOTIFICATIONS_WEBHOOK_URL) is None: + return + + client = WebhookClient(webhook_url) + + fields = [ + {"title": "Title", "value": bundle.name, "short": True}, + {"title": "User", "value": user.get_full_name() if user else "System", "short": True}, + {"title": "Pages", "value": bundle.get_bundled_pages().count(), "short": True}, + {"title": "Total time", "value": f"{elapsed:.3f} seconds"}, + ] + if url: + fields.append( + {"title": "Link", "value": url, "short": False}, + ) + + response = client.send( + text="Finished bundle publication", + attachments=[{"color": "good", "fields": fields}], + unfurl_links=False, + unfurl_media=False, + ) + + if response.status_code != HTTPStatus.OK: + logger.error("Unable to notify Slack of bundle status change: %s", response.body) diff --git a/cms/bundles/panels.py b/cms/bundles/panels.py new file mode 100644 index 00000000..3450398e --- /dev/null +++ b/cms/bundles/panels.py @@ -0,0 +1,50 @@ +from typing import TYPE_CHECKING, Any, Union + +from django.utils.html import format_html, format_html_join +from wagtail.admin.panels import HelpPanel, PageChooserPanel + +if TYPE_CHECKING: + from django.db.models import Model + from django.utils.safestring import SafeString + + +class BundleNotePanel(HelpPanel): + """An extended HelpPanel class.""" + + class BoundPanel(HelpPanel.BoundPanel): + def __init__(self, **kwargs: Any) -> None: + super().__init__(**kwargs) + self.content = self._content_for_instance(self.instance) + + def _content_for_instance(self, instance: "Model") -> Union[str, "SafeString"]: + if not hasattr(instance, "bundles"): + return "" + + if bundles := instance.bundles: + content_html = format_html_join( + "\n", + "
  • {} (Status: {})
    • ", + ( + ( + bundle.name, + bundle.get_status_display(), + ) + for bundle in bundles + ), + ) + + content = format_html("

    This page is in the following bundle(s):

    ", content_html) + else: + content = format_html("

    This page is not part of any bundles

    ") + return content + + +class PageChooserWithStatusPanel(PageChooserPanel): + """A custom page chooser panel that includes the page workflow status.""" + + class BoundPanel(PageChooserPanel.BoundPanel): + def __init__(self, **kwargs: Any) -> None: + """Sets the panel heading to the page verbose name to help differentiate page types.""" + super().__init__(**kwargs) + if page := self.instance.page: + self.heading = page.specific.get_verbose_name() diff --git a/cms/bundles/templates/bundles/wagtailadmin/add_to_bundle.html b/cms/bundles/templates/bundles/wagtailadmin/add_to_bundle.html new file mode 100644 index 00000000..1342a4ee --- /dev/null +++ b/cms/bundles/templates/bundles/wagtailadmin/add_to_bundle.html @@ -0,0 +1,29 @@ +{% extends "wagtailadmin/base.html" %} +{% load i18n wagtailadmin_tags %} +{% block titletag %}{% blocktrans trimmed with title=page_to_move.specific_deferred.get_admin_display_title %}Add {{ title }} to a bundle{% endblocktrans %}{% endblock %} +{% block content %} + {% include "wagtailadmin/shared/header.html" with title=_("Add") subtitle=page_to_move.specific_deferred.get_admin_display_title icon="boxes-stacked" %} + +
    +
    + {% csrf_token %} + {% if next %}{% endif %} + +
      +
    • {% formattedfield form.bundle %}
      • +
    + + +
    +
    +{% endblock %} + +{% block extra_js %} + {{ block.super }} + {{ form.media.js }} +{% endblock %} + +{% block extra_css %} + {{ block.super }} + {{ form.media.css }} +{% endblock %} diff --git a/cms/bundles/templates/bundles/wagtailadmin/edit.html b/cms/bundles/templates/bundles/wagtailadmin/edit.html new file mode 100644 index 00000000..87a17f61 --- /dev/null +++ b/cms/bundles/templates/bundles/wagtailadmin/edit.html @@ -0,0 +1,14 @@ +{% extends "wagtailadmin/generic/form.html" %} +{% load i18n %} + +{% block actions %} + {{ block.super }} + {% if show_save_and_approve %} + + {% elif show_publish %} + + {% endif %} + {% if delete_url %} + {{ delete_item_label }} + {% endif %} +{% endblock %} diff --git a/cms/bundles/templates/bundles/wagtailadmin/inspect.html b/cms/bundles/templates/bundles/wagtailadmin/inspect.html new file mode 100644 index 00000000..140d6860 --- /dev/null +++ b/cms/bundles/templates/bundles/wagtailadmin/inspect.html @@ -0,0 +1,5 @@ +{% extends "wagtailadmin/generic/inspect.html" %} +{% load i18n %} + +{% block footer %} +{% endblock %} diff --git a/cms/bundles/templates/bundles/wagtailadmin/panels/latest_bundles.html b/cms/bundles/templates/bundles/wagtailadmin/panels/latest_bundles.html new file mode 100644 index 00000000..1a8cde48 --- /dev/null +++ b/cms/bundles/templates/bundles/wagtailadmin/panels/latest_bundles.html @@ -0,0 +1,59 @@ +{% load i18n wagtailcore_tags wagtailadmin_tags %} +{% if is_shown %} + {% panel id="latest-bundles" heading=_("Latest active bundles") %} + {% help_block status="info" %} +

    Bundles are collections of pages and datasets to publish together.

    + {% endhelp_block %} +

    + {% icon name="plus" wrapped=1 %}{% trans "Add bundle" %} + View all bundles +

    + {% if bundles %} + + + + + + + {# add class="w-sr-only" to make this visible for screen readers only #} + + + + + + + + + + {% for bundle in bundles %} + + + + + + + + {% endfor %} + +
    {% trans "Title" %}{% trans "Status" %}{% trans "Scheduled publication date" %}{% trans "Added" %}{% trans "Added by" %}
    +
    + {{ bundle.name }} +
    + +     		+ {{ bundle.get_status_display }} + + {{ bundle.scheduled_publication_date|default_if_none:"" }} + {% human_readable_date bundle.created_at %}{% include "wagtailadmin/shared/user_avatar.html" with user=bundle.created_by username=bundle.created_by.get_full_name|default:bundle.created_by.get_username %}
    + {% else %} +

    There are currently no active bundles.

    + {% endif %} + {% endpanel %} +{% endif %} diff --git a/cms/bundles/views.py b/cms/bundles/views.py new file mode 100644 index 00000000..604084fa --- /dev/null +++ b/cms/bundles/views.py @@ -0,0 +1,104 @@ +from typing import TYPE_CHECKING, Any + +from django.core.exceptions import PermissionDenied +from django.http import Http404 +from django.shortcuts import get_object_or_404, redirect +from django.urls import reverse +from django.utils.http import url_has_allowed_host_and_scheme +from django.utils.translation import gettext as _ +from django.views.generic import FormView +from wagtail.admin import messages +from wagtail.models import Page +from wagtail.permission_policies import ModelPermissionPolicy + +from .admin_forms import AddToBundleForm +from .models import Bundle, BundledPageMixin, BundlePage + +if TYPE_CHECKING: + from django.http import HttpRequest, HttpResponseBase, HttpResponseRedirect + + +class AddToBundleView(FormView): + form_class = AddToBundleForm + template_name = "bundles/wagtailadmin/add_to_bundle.html" + + page_to_add: Page = None + goto_next: str | None = None + + def dispatch(self, request: "HttpRequest", *args: Any, **kwargs: Any) -> "HttpResponseBase": + self.page_to_add = get_object_or_404( + Page.objects.specific().defer_streamfields(), id=self.kwargs["page_to_add_id"] + ) + + if not isinstance(self.page_to_add, BundledPageMixin): + raise Http404(_("Cannot add this page type to a bundle")) + + page_perms = self.page_to_add.permissions_for_user(request.user) # type: ignore[attr-defined] + # TODO: add the relevant permission checks + if not (page_perms.can_edit() or page_perms.can_publish()): + raise PermissionDenied + + permission_policy = ModelPermissionPolicy(Bundle) + if not permission_policy.user_has_permission(request.user, "change"): + raise PermissionDenied + + self.goto_next = None + redirect_to = request.GET.get("next", "") + if url_has_allowed_host_and_scheme(url=redirect_to, allowed_hosts={self.request.get_host()}): + self.goto_next = redirect_to + + if self.page_to_add.in_active_bundle: + bundles = ", ".join(list(self.page_to_add.active_bundles.values_list("name", flat=True))) + messages.warning( + request, + _("Page %(title)s is already in a bundle ('%(bundles)s')") + % { + "title": self.page_to_add.get_admin_display_title(), # type: ignore[attr-defined] + "bundles": bundles, + }, + ) + if self.goto_next: + return redirect(self.goto_next) + + return redirect("wagtailadmin_home") + + return super().dispatch(request, *args, **kwargs) + + def get_form_kwargs(self) -> dict[str, Any]: + kwargs = super().get_form_kwargs() + kwargs.update({"page_to_add": self.page_to_add}) + return kwargs + + def get_context_data(self, **kwargs: Any) -> dict[str, Any]: + context_data = super().get_context_data(**kwargs) + context_data.update( + { + "page_to_add": self.page_to_add, + "next": self.goto_next, + } + ) + return context_data + + def form_valid(self, form: AddToBundleForm) -> "HttpResponseRedirect": + bundle: Bundle = form.cleaned_data["bundle"] # the 'bundle' field is required in the form. + bundle.bundled_pages.add(BundlePage(page=self.page_to_add)) + bundle.save() + + messages.success( + self.request, + f"Page '{self.page_to_add.get_admin_display_title()}' added to bundle '{bundle}'", + buttons=[ + messages.button( + reverse("wagtailadmin_pages:edit", args=(self.page_to_add.id,)), + _("Edit"), + ) + ], + ) + redirect_to = self.request.POST.get("next", "") + if url_has_allowed_host_and_scheme(url=redirect_to, allowed_hosts={self.request.get_host()}): + return redirect(redirect_to) + + return redirect("wagtailadmin_explore", self.page_to_add.get_parent().id) + + +add_to_bundle = AddToBundleView.as_view() diff --git a/cms/bundles/viewsets.py b/cms/bundles/viewsets.py new file mode 100644 index 00000000..e49e1483 --- /dev/null +++ b/cms/bundles/viewsets.py @@ -0,0 +1,308 @@ +import time +from functools import cached_property +from typing import TYPE_CHECKING, Any, ClassVar + +from django.db.models import QuerySet +from django.http import HttpRequest +from django.shortcuts import redirect +from django.urls import reverse +from django.utils import timezone +from django.utils.html import format_html, format_html_join +from django.utils.translation import gettext as _ +from wagtail.admin.ui.tables import Column, DateColumn, UpdatedAtColumn, UserColumn +from wagtail.admin.views.generic import CreateView, EditView, IndexView, InspectView +from wagtail.admin.views.generic.chooser import ChooseView +from wagtail.admin.viewsets.chooser import ChooserViewSet +from wagtail.admin.viewsets.model import ModelViewSet +from wagtail.log_actions import log + +from .enums import BundleStatus +from .models import Bundle +from .notifications import notify_slack_of_publication_start, notify_slack_of_publish_end, notify_slack_of_status_change + +if TYPE_CHECKING: + from django.db.models.fields import Field + from django.http import HttpResponseBase + from django.utils.safestring import SafeString + + +class BundleCreateView(CreateView): + """The Bundle create view class.""" + + def save_instance(self) -> Bundle: + """Automatically set the creating user on Bundle creation.""" + instance: Bundle = super().save_instance() + instance.created_by = self.request.user + instance.save(update_fields=["created_by"]) + return instance + + +class BundleEditView(EditView): + """The Bundle edit view class.""" + + actions: ClassVar[list[str]] = ["edit", "save-and-approve", "publish"] + template_name = "bundles/wagtailadmin/edit.html" + has_content_changes: bool = False + start_time: float | None = None + + def dispatch(self, request: HttpRequest, *args: Any, **kwargs: Any) -> "HttpResponseBase": + if (instance := self.get_object()) and instance.status == BundleStatus.RELEASED: + return redirect(self.index_url_name) + + response: HttpResponseBase = super().dispatch(request, *args, **kwargs) + return response + + def get_form_kwargs(self) -> dict: + kwargs: dict = super().get_form_kwargs() + if self.request.method == "POST": + data = self.request.POST.copy() + if "action-save-and-approve" in self.request.POST: + data["status"] = BundleStatus.APPROVED.value + data["approved_at"] = timezone.now() + data["approved_by"] = self.request.user + kwargs["data"] = data + elif "action-publish" in self.request.POST: + data["status"] = BundleStatus.RELEASED.value + kwargs["data"] = data + return kwargs + + def save_instance(self) -> Bundle: + instance: Bundle = self.form.save() + self.has_content_changes = self.form.has_changed() + + if self.has_content_changes: + log(action="wagtail.edit", instance=instance, content_changed=True, data={"fields": self.form.changed_data}) + + if "status" in self.form.changed_data: + kwargs: dict = {"content_changed": self.has_content_changes} + original_status = BundleStatus[self.form.original_status].label + url = self.request.build_absolute_uri(reverse("bundle:inspect", args=(instance.pk,))) + + if instance.status == BundleStatus.APPROVED: + action = "bundles.approve" + kwargs["data"] = {"old": original_status} + notify_slack_of_status_change(instance, original_status, user=self.request.user, url=url) + elif instance.status == BundleStatus.RELEASED.value: + action = "wagtail.publish" + self.start_time = time.time() + else: + action = "bundles.update_status" + kwargs["data"] = { + "old": original_status, + "new": instance.get_status_display(), + } + notify_slack_of_status_change(instance, original_status, user=self.request.user, url=url) + + # now log the status change + log( + action=action, + instance=instance, + **kwargs, + ) + + return instance + + def run_after_hook(self) -> None: + """This method allows calling hooks or additional logic after an action has been executed. + + In our case, we want to send a Slack notification if manually published, and approve any of the + related pages that are in a Wagtail workflow. + """ + if self.action == "publish" or (self.action == "edit" and self.object.status == BundleStatus.RELEASED): + notify_slack_of_publication_start(self.object, user=self.request.user) + start_time = self.start_time or time.time() + for page in self.object.get_bundled_pages(): + if page.current_workflow_state: + page.current_workflow_state.current_task_state.approve(user=self.request.user) + + notify_slack_of_publish_end(self.object, time.time() - start_time, user=self.request.user) + + def get_context_data(self, **kwargs: Any) -> dict: + """Updates the template context. + + Show the "save and approve" button if the bundle has the right status, and we have a different user + than the creator + """ + context: dict = super().get_context_data(**kwargs) + + context["show_save_and_approve"] = ( + self.object.can_be_approved and self.form.for_user.pk != self.object.created_by_id + ) + context["show_publish"] = ( + self.object.status == BundleStatus.APPROVED and not self.object.scheduled_publication_date + ) + + return context + + +class BundleInspectView(InspectView): + """The Bundle inspect view class.""" + + template_name = "bundles/wagtailadmin/inspect.html" + + def get_fields(self) -> list[str]: + """Returns the list of fields to include in the inspect view.""" + return ["name", "status", "created_at", "created_by", "approved", "scheduled_publication", "pages"] + + def get_field_label(self, field_name: str, field: "Field") -> Any: + match field_name: + case "approved": + return _("Approval status") + case "scheduled_publication": + return _("Scheduled publication") + case "pages": + return _("Pages") + case _: + return super().get_field_label(field_name, field) + + def get_field_display_value(self, field_name: str, field: "Field") -> Any: + """Allows customising field display in the inspect class. + + This allows us to use get_FIELDNAME_display_value methods. + """ + value_func = getattr(self, f"get_{field_name}_display_value", None) + if value_func is not None: + return value_func() + + return super().get_field_display_value(field_name, field) + + def get_approved_display_value(self) -> str: + """Custom approved by formatting. Varies based on status, and approver/time of approval.""" + if self.object.status in [BundleStatus.APPROVED, BundleStatus.RELEASED]: + if self.object.approved_by_id and self.object.approved_at: + return f"{self.object.approved_by} on {self.object.approved_at}" + return _("Unknown approval data") + return _("Pending approval") + + def get_scheduled_publication_display_value(self) -> str: + """Displays the scheduled publication date, if set.""" + return self.object.scheduled_publication_date or _("No scheduled publication") + + def get_pages_display_value(self) -> "SafeString": + """Returns formatted markup for Pages linked to the Bundle.""" + pages = self.object.get_bundled_pages().specific() + data = ( + ( + reverse("wagtailadmin_pages:edit", args=(page.pk,)), + page.get_admin_display_title(), + page.get_verbose_name(), + ( + page.current_workflow_state.current_task_state.task.name + if page.current_workflow_state + else "not in a workflow" + ), + reverse("wagtailadmin_pages:view_draft", args=(page.pk,)), + ) + for page in pages + ) + + page_data = format_html_join( + "\n", + '{}{}{} ' + 'Preview', + data, + ) + + return format_html( + "" + "{}
    TitleTypeStatusActions
    ", + page_data, + ) + + +class BundleIndexView(IndexView): + """The Bundle index view class. + + We adjust the queryset and change the edit URL based on the bundle status. + """ + + model = Bundle + + def get_queryset(self) -> QuerySet[Bundle]: + """Modifies the Bundle queryset with the related created_by ForeignKey selected to avoid N+1 queries.""" + queryset: QuerySet[Bundle] = super().get_queryset() + + return queryset.select_related("created_by") + + def get_edit_url(self, instance: Bundle) -> str | None: + """Override the default edit url to disable the edit URL for released bundles.""" + if instance.status != BundleStatus.RELEASED: + edit_url: str | None = super().get_edit_url(instance) + return edit_url + return None + + def get_copy_url(self, instance: Bundle) -> str | None: + """Disables the bundle copy.""" + return None + + @cached_property + def columns(self) -> list[Column]: + """Defines the list of desired columns in the listing.""" + return [ + self._get_title_column("__str__"), + Column("scheduled_publication_date"), + Column("get_status_display", label=_("Status")), + UpdatedAtColumn(), + DateColumn(name="created_at", label=_("Added"), sort_key="created_at"), + UserColumn("created_by", label=_("Added by")), + DateColumn(name="approved_at", label=_("Approved at"), sort_key="approved_at"), + UserColumn("approved_by"), + ] + + +class BundleChooseView(ChooseView): + """The Bundle choose view class. Used in choosers.""" + + icon = "boxes-stacked" + + @property + def columns(self) -> list[Column]: + """Defines the list of desired columns in the chooser.""" + return [ + *super().columns, + Column("scheduled_publication_date"), + UserColumn("created_by"), + ] + + def get_object_list(self) -> QuerySet[Bundle]: + """Overrides the default object list to only fetch the fields we're using.""" + queryset: QuerySet[Bundle] = Bundle.objects.select_related("created_by").only("name", "created_by") + return queryset + + +class BundleChooserViewSet(ChooserViewSet): + """Defines the chooser viewset for Bundles.""" + + model = Bundle + icon = "boxes-stacked" + choose_view_class = BundleChooseView + + def get_object_list(self) -> QuerySet[Bundle]: + """Only return editable bundles.""" + queryset: QuerySet[Bundle] = self.model.objects.editable() + return queryset + + +class BundleViewSet(ModelViewSet): + """The viewset class for Bundle. + + We extend the generic ModelViewSet to add our customisations. + @see https://docs.wagtail.org/en/stable/reference/viewsets.html#modelviewset + """ + + model = Bundle + icon = "boxes-stacked" + add_view_class = BundleCreateView + edit_view_class = BundleEditView + inspect_view_class = BundleInspectView + index_view_class = BundleIndexView + chooser_viewset_class = BundleChooserViewSet + list_filter: ClassVar[list[str]] = ["status", "created_by"] + add_to_admin_menu = True + inspect_view_enabled = True + + +bundle_viewset = BundleViewSet("bundle") +bundle_chooser_viewset = BundleChooserViewSet("bundle_chooser") + +BundleChooserWidget = bundle_chooser_viewset.widget_class diff --git a/cms/bundles/wagtail_hooks.py b/cms/bundles/wagtail_hooks.py new file mode 100644 index 00000000..dd74190b --- /dev/null +++ b/cms/bundles/wagtail_hooks.py @@ -0,0 +1,199 @@ +from functools import cached_property +from typing import TYPE_CHECKING, Any, Union + +from django.db.models import QuerySet +from django.urls import include, path +from django.utils.timezone import now +from django.utils.translation import gettext_lazy as _ +from wagtail import hooks +from wagtail.admin.ui.components import Component +from wagtail.admin.widgets import PageListingButton +from wagtail.log_actions import LogFormatter +from wagtail.permission_policies import ModelPermissionPolicy + +from . import admin_urls +from .models import Bundle, BundledPageMixin +from .viewsets import bundle_chooser_viewset, bundle_viewset + +if TYPE_CHECKING: + from django.http import HttpRequest + from django.urls import URLPattern + from django.urls.resolvers import URLResolver + from laces.typing import RenderContext + from wagtail.log_actions import LogActionRegistry + from wagtail.models import ModelLogEntry, Page + + from cms.users.models import User + + +@hooks.register("register_admin_viewset") +def register_viewset() -> list: + """Registers the bundle viewsets. + + @see https://docs.wagtail.org/en/stable/reference/hooks.html#register-admin-viewset + """ + return [bundle_viewset, bundle_chooser_viewset] + + +class PageAddToBundleButton(PageListingButton): + """Defines the 'Add to Bundle' button to use in different contexts in the admin.""" + + label = _("Add to Bundle") + icon_name = "boxes-stacked" + aria_label_format = _("Add '%(title)s' to a bundle") + url_name = "bundles:add_to_bundle" + + @property + def permission_policy(self) -> ModelPermissionPolicy: + """Informs the permission policy to use Bundle-derived model permissions.""" + return ModelPermissionPolicy(Bundle) + + @property + def show(self) -> bool: + """Determines whether the button should be shown. + + We only want it for pages inheriting from BundledPageMixin that are not in an active bundle. + """ + if not isinstance(self.page, BundledPageMixin): + return False + + if self.page.in_active_bundle: + return False + + # Note: limit to pages that are not in an active bundle + can_show: bool = ( + self.page_perms.can_edit() or self.page_perms.can_publish() + ) and self.permission_policy.user_has_any_permission(self.user, ["add", "change", "delete"]) + return can_show + + +@hooks.register("register_page_header_buttons") +def page_header_buttons(page: "Page", user: "User", view_name: str, next_url: str | None = None) -> PageListingButton: # pylint: disable=unused-argument + """Registers the add to bundle button in the buttons shown in the page add/edit header. + + @see https://docs.wagtail.org/en/stable/reference/hooks.html#register-page-header-buttons. + """ + yield PageAddToBundleButton(page=page, user=user, priority=10, next_url=next_url) + + +@hooks.register("register_page_listing_buttons") +def page_listing_buttons(page: "Page", user: "User", next_url: str | None = None) -> PageListingButton: + """Registers the add to bundle button in the buttons shown in the page listing. + + @see https://docs.wagtail.org/en/stable/reference/hooks.html#register_page_listing_buttons. + """ + yield PageAddToBundleButton(page=page, user=user, priority=10, next_url=next_url) + + +@hooks.register("register_admin_urls") +def register_admin_urls() -> list[Union["URLPattern", "URLResolver"]]: + """Registers the admin urls for Bundles. + + @see https://docs.wagtail.org/en/stable/reference/hooks.html#register-admin-urls. + """ + return [path("bundles/", include(admin_urls))] + + +@hooks.register("before_edit_page") +def preset_golive_date(request: "HttpRequest", page: "Page") -> None: + """Implements the before_edit_page to preset the golive date on pages in active bundles. + + @see https://docs.wagtail.org/en/stable/reference/hooks.html#before-edit-page. + """ + if not isinstance(page, BundledPageMixin): + return + + if not page.in_active_bundle: + return + + scheduled_publication_date = page.active_bundle.scheduled_publication_date # type: ignore[union-attr] + # note: ignoring union-attr because we already check that the page is in an active bundle. + if not scheduled_publication_date: + return + + # note: ignoring + # - attr-defined because mypy thinks page is only a BundledPageMixin class, rather than Page and BundledPageMixin. + # - union-attr because active_bundle can be none, but we check for that above + if now() < scheduled_publication_date != page.go_live_at: # type: ignore[attr-defined] + # pre-set the scheduled publishing time + page.go_live_at = scheduled_publication_date # type: ignore[attr-defined] + + +class LatestBundlesPanel(Component): + """The admin dashboard panel for showing the latest bundles.""" + + name = "latest_bundles" + order = 150 + template_name = "bundles/wagtailadmin/panels/latest_bundles.html" + + def __init__(self, request: "HttpRequest") -> None: + self.request = request + self.permission_policy = ModelPermissionPolicy(Bundle) + + @cached_property + def is_shown(self) -> bool: + """Determine if the panel is shown based on whether the user can modify it.""" + has_permission: bool = self.permission_policy.user_has_any_permission( + self.request.user, {"add", "change", "delete"} + ) + return has_permission + + def get_latest_bundles(self) -> QuerySet[Bundle]: + """Returns the latest 10 bundles if the panel is shown.""" + queryset: QuerySet[Bundle] = Bundle.objects.none() + if self.is_shown: + queryset = Bundle.objects.active().select_related("created_by")[:10] + + return queryset + + def get_context_data(self, parent_context: "RenderContext") -> "RenderContext": + """Adds the request, the latest bundles and whether the panel is shown to the panel context.""" + context = super().get_context_data(parent_context) + context["request"] = self.request + context["bundles"] = self.get_latest_bundles() + context["is_shown"] = self.is_shown + return context + + +@hooks.register("construct_homepage_panels") +def add_latest_bundles_panel(request: "HttpRequest", panels: list[Component]) -> None: + """Adds the LatestBundlesPanel to the list of Wagtail admin dashboard panels. + + @see https://docs.wagtail.org/en/stable/reference/hooks.html#construct-homepage-panels + """ + panels.append(LatestBundlesPanel(request)) + + +@hooks.register("register_log_actions") +def register_bundle_log_actions(actions: "LogActionRegistry") -> None: + """Registers custom logging actions. + + @see https://docs.wagtail.org/en/stable/extending/audit_log.html + @see https://docs.wagtail.org/en/stable/reference/hooks.html#register-log-actions + """ + + @actions.register_action("bundles.update_status") + class ChangeBundleStatus(LogFormatter): # pylint: disable=unused-variable + """LogFormatter class for the bundle status change actions.""" + + label = _("Change bundle status") + + def format_message(self, log_entry: "ModelLogEntry") -> Any: + """Returns the formatted log message.""" + try: + return _(f"Changed the bundle status from '{log_entry.data["old"]}' to '{log_entry.data["new"]}'") + except KeyError: + return _("Changed the bundle status") + + @actions.register_action("bundles.approve") + class ApproveBundle(LogFormatter): # pylint: disable=unused-variable + """LogFormatter class for the bundle approval actions.""" + + label = _("Approve bundle") + + def format_message(self, log_entry: "ModelLogEntry") -> Any: + """Returns the formatted log message.""" + try: + return _(f"Approved the bundle. (Old status: '{log_entry.data["old"]}')") + except KeyError: + return _("Approved the bundle") diff --git a/cms/core/wagtail_hooks.py b/cms/core/wagtail_hooks.py index 59543867..2ae12adb 100644 --- a/cms/core/wagtail_hooks.py +++ b/cms/core/wagtail_hooks.py @@ -14,6 +14,7 @@ def register_icons(icons: list[str]) -> list[str]: """ return [ *icons, + "boxes-stacked.svg", "data-analysis.svg", "identity.svg", "news.svg", diff --git a/cms/jinja2/assets/icons/boxes-stacked.svg b/cms/jinja2/assets/icons/boxes-stacked.svg new file mode 100644 index 00000000..bd09df78 --- /dev/null +++ b/cms/jinja2/assets/icons/boxes-stacked.svg @@ -0,0 +1 @@ + diff --git a/cms/settings/base.py b/cms/settings/base.py index 9511f869..607a802e 100644 --- a/cms/settings/base.py +++ b/cms/settings/base.py @@ -55,6 +55,7 @@ INSTALLED_APPS = [ "cms.analysis", + "cms.bundles", "cms.core", "cms.documents", "cms.home", @@ -787,3 +788,6 @@ # ONS Cookie banner settings ONS_COOKIE_BANNER_SERVICE_NAME = env.get("ONS_COOKIE_BANNER_SERVICE_NAME", "www.ons.gov.uk") MANAGE_COOKIE_SETTINGS_URL = env.get("MANAGE_COOKIE_SETTINGS_URL", "https://www.ons.gov.uk/cookies") + + +SLACK_NOTIFICATIONS_WEBHOOK_URL = env.get("SLACK_NOTIFICATIONS_WEBHOOK_URL") diff --git a/poetry.lock b/poetry.lock index 354c6513..42315e9f 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1696,6 +1696,20 @@ files = [ {file = "six-1.16.0.tar.gz", hash = "sha256:1e61c37477a1626458e36f7b1d82aa5c9b094fa4802892072e49de9c60c4c926"}, ] +[[package]] +name = "slack-sdk" +version = "3.33.3" +description = "The Slack API Platform SDK for Python" +optional = false +python-versions = ">=3.6" +files = [ + {file = "slack_sdk-3.33.3-py2.py3-none-any.whl", hash = "sha256:0515fb93cd03b18de61f876a8304c4c3cef4dd3c2a3bad62d7394d2eb5a3c8e6"}, + {file = "slack_sdk-3.33.3.tar.gz", hash = "sha256:4cc44c9ffe4bb28a01fbe3264c2f466c783b893a4eca62026ab845ec7c176ff1"}, +] + +[package.extras] +optional = ["SQLAlchemy (>=1.4,<3)", "aiodns (>1.0)", "aiohttp (>=3.7.3,<4)", "boto3 (<=2)", "websocket-client (>=1,<2)", "websockets (>=9.1,<14)"] + [[package]] name = "soupsieve" version = "2.6" @@ -2058,4 +2072,4 @@ wand = ["Wand (>=0.6,<1.0)"] [metadata] lock-version = "2.0" python-versions = "^3.12" -content-hash = "4d93ff1acdcb3aafa0f83b5b2ff87f89449975de8bb838b93e27b630e64160c4" +content-hash = "298be357df9647135f9ac1b14c5fddb61e2f61aeb87b59f14f7d66baac6a3529" diff --git a/pyproject.toml b/pyproject.toml index e1f2f6fe..003ac5fa 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -27,6 +27,7 @@ apscheduler = "^3.10.4" wagtail-storages = "~2.0" wagtailmath = "~1.3.0" whitenoise = "~6.8" +slack-sdk = "^3.33.3" [tool.poetry.group.dev.dependencies] # :TODO: Remove pylint when ruff supports all pylint rules From b259ba5d340317962f873f0a96fb729a0e075b08 Mon Sep 17 00:00:00 2001 From: zerolab Date: Thu, 14 Nov 2024 16:52:02 +0000 Subject: [PATCH 02/13] Make Analysis page bundle-able --- cms/analysis/models.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cms/analysis/models.py b/cms/analysis/models.py index 158dbf53..4ead55a5 100644 --- a/cms/analysis/models.py +++ b/cms/analysis/models.py @@ -14,6 +14,7 @@ from wagtail.search import index from cms.analysis.blocks import AnalysisStoryBlock +from cms.bundles.models import BundledPageMixin from cms.core.blocks import HeadlineFiguresBlock from cms.core.fields import StreamField from cms.core.models import BasePage @@ -75,7 +76,7 @@ def previous_releases(self, request: "HttpRequest") -> "TemplateResponse": return response -class AnalysisPage(BasePage): # type: ignore[django-manager-missing] +class AnalysisPage(BundledPageMixin, BasePage): # type: ignore[django-manager-missing] """The analysis page model.""" parent_page_types: ClassVar[list[str]] = ["AnalysisSeries"] From e13045b3489f1e9bb41b097ea46cc861f632a475 Mon Sep 17 00:00:00 2001 From: zerolab Date: Mon, 18 Nov 2024 12:22:31 +0000 Subject: [PATCH 03/13] Re-enable the scheduler --- .docker/Procfile | 1 + heroku.yml | 10 +++++----- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/.docker/Procfile b/.docker/Procfile index e386ef56..02ccf946 100644 --- a/.docker/Procfile +++ b/.docker/Procfile @@ -1,2 +1,3 @@ web: python manage.py runserver 0.0.0.0:8000 +scheduler: python manage.py scheduler frontend: npm run start:reload diff --git a/heroku.yml b/heroku.yml index 6497029b..83df8d5c 100644 --- a/heroku.yml +++ b/heroku.yml @@ -7,8 +7,8 @@ release: image: web command: - django-admin check --deploy && django-admin createcachetable && django-admin migrate --noinput -#run: -# scheduler: -# image: web -# command: -# - django-admin scheduler +run: + scheduler: + image: web + command: + - django-admin scheduler From d2b3ea61385bac62d9c8d65f5e4b69558333d7a1 Mon Sep 17 00:00:00 2001 From: zerolab Date: Mon, 18 Nov 2024 16:44:22 +0000 Subject: [PATCH 04/13] Add tests --- cms/bundles/tests/__init__.py | 0 cms/bundles/tests/factories.py | 77 ++++++++++++ cms/bundles/tests/test_models.py | 123 ++++++++++++++++++ cms/bundles/tests/test_panels.py | 51 ++++++++ cms/bundles/tests/test_viewsets.py | 193 +++++++++++++++++++++++++++++ cms/bundles/tests/utils.py | 46 +++++++ cms/users/tests/__init__.py | 0 cms/users/tests/factories.py | 59 +++++++++ 8 files changed, 549 insertions(+) create mode 100644 cms/bundles/tests/__init__.py create mode 100644 cms/bundles/tests/factories.py create mode 100644 cms/bundles/tests/test_models.py create mode 100644 cms/bundles/tests/test_panels.py create mode 100644 cms/bundles/tests/test_viewsets.py create mode 100644 cms/bundles/tests/utils.py create mode 100644 cms/users/tests/__init__.py create mode 100644 cms/users/tests/factories.py diff --git a/cms/bundles/tests/__init__.py b/cms/bundles/tests/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/cms/bundles/tests/factories.py b/cms/bundles/tests/factories.py new file mode 100644 index 00000000..16d314fd --- /dev/null +++ b/cms/bundles/tests/factories.py @@ -0,0 +1,77 @@ +import factory +from django.utils import timezone + +from cms.bundles.enums import BundleStatus +from cms.bundles.models import Bundle, BundlePage +from cms.users.tests.factories import UserFactory + + +class BundleFactory(factory.django.DjangoModelFactory): + """Factory for Bundle model.""" + + class Meta: + model = Bundle + + name = factory.Faker("sentence", nb_words=4) + created_at = factory.LazyFunction(timezone.now) + created_by = factory.SubFactory(UserFactory) + status = BundleStatus.PENDING + + class Params: + """Defines custom factory traits. + + Usage: BundlFactory(approved=True) or BundlFactory(released=True) + """ + + in_review = factory.Trait( + status=BundleStatus.IN_REVIEW, + publication_date=factory.LazyFunction(lambda: timezone.now() + timezone.timedelta(days=1)), + ) + + # Trait for approved bundles + approved = factory.Trait( + status=BundleStatus.APPROVED, + approved_at=factory.LazyFunction(timezone.now), + approved_by=factory.SubFactory(UserFactory), + publication_date=factory.LazyFunction(lambda: timezone.now() + timezone.timedelta(days=1)), + ) + + # Trait for released bundles + released = factory.Trait( + status=BundleStatus.RELEASED, + approved_at=factory.LazyFunction(lambda: timezone.now() - timezone.timedelta(days=1)), + approved_by=factory.SubFactory(UserFactory), + publication_date=factory.LazyFunction(timezone.now), + ) + + @factory.post_generation + def bundled_pages(self, create, extracted, **kwargs): + """Creates BundlePage instances for the bundle. + + Usage: + # Create a bundle with no pages + bundle = BundleFactory() + + # Create a bundle with specific pages + bundle = BundleFactory(bundled_pages=[page1, page2]) + + # Create an approved bundle with pages + bundle = BundleFactory(approved=True, bundled_pages=[page1, page2]) + """ + if not create: + return + + if extracted: + for page in extracted: + BundlePageFactory(parent=self, page=page) + + +class BundlePageFactory(factory.django.DjangoModelFactory): + """Factory for BundlePage orderable model.""" + + class Meta: + model = BundlePage + + parent = factory.SubFactory(BundleFactory) + page = factory.SubFactory("cms.analysis.tests.factories.AnalysisPageFactory") + sort_order = factory.Sequence(lambda n: n) diff --git a/cms/bundles/tests/test_models.py b/cms/bundles/tests/test_models.py new file mode 100644 index 00000000..ae87f219 --- /dev/null +++ b/cms/bundles/tests/test_models.py @@ -0,0 +1,123 @@ +from datetime import timedelta + +from django.test import TestCase +from django.utils import timezone + +from cms.analysis.tests.factories import AnalysisPageFactory +from cms.bundles.enums import BundleStatus +from cms.bundles.tests.factories import BundleFactory, BundlePageFactory +from cms.release_calendar.tests.factories import ReleaseCalendarPageFactory + + +class BundleModelTestCase(TestCase): + """Test Bundle model properties and methods.""" + + def setUp(self): + self.bundle = BundleFactory(name="The bundle") + self.analysis_page = AnalysisPageFactory(title="PSF") + + def test_str(self): + """Test string representation.""" + self.assertEqual(str(self.bundle), self.bundle.name) + + def test_scheduled_publication_date__direct(self): + """Test scheduled_publication_date returns direct publication date.""" + del self.bundle.scheduled_publication_date # clear the cached property + now = timezone.now() + self.bundle.publication_date = now + self.assertEqual(self.bundle.scheduled_publication_date, now) + + def test_scheduled_publication_date__via_release(self): + """Test scheduled_publication_date returns release calendar date.""" + release_page = ReleaseCalendarPageFactory() + self.bundle.release_calendar_page = release_page + self.assertEqual(self.bundle.scheduled_publication_date, release_page.release_date) + + def test_can_be_approved(self): + """Test can_be_approved property.""" + test_cases = [ + (BundleStatus.PENDING, True), + (BundleStatus.IN_REVIEW, True), + (BundleStatus.APPROVED, False), + (BundleStatus.RELEASED, False), + ] + + for status, expected in test_cases: + with self.subTest(status=status): + self.bundle.status = status + self.assertEqual(self.bundle.can_be_approved, expected) + + def test_get_bundled_pages(self): + """Test get_bundled_pages returns correct queryset.""" + BundlePageFactory(parent=self.bundle, page=self.analysis_page) + page_ids = self.bundle.get_bundled_pages().values_list("pk", flat=True) + self.assertEqual(len(page_ids), 1) + self.assertEqual(page_ids[0], self.analysis_page.pk) + + def test_save_updates_page_publication_dates(self): + """Test save method updates page publication dates.""" + BundlePageFactory(parent=self.bundle, page=self.analysis_page) + del self.bundle.scheduled_publication_date # clear the cached property + future_date = timezone.now() + timedelta(days=1) + self.bundle.publication_date = future_date + self.bundle.save() + + self.analysis_page.refresh_from_db() + self.assertEqual(self.analysis_page.go_live_at, future_date) + + def test_save_doesnt_update_dates_when_released(self): + """Test save method doesn't update dates for released bundles.""" + future_date = timezone.now() + timedelta(days=1) + self.bundle.status = BundleStatus.RELEASED + self.bundle.publication_date = future_date + BundlePageFactory(parent=self.bundle, page=self.analysis_page) + + self.bundle.save() + self.analysis_page.refresh_from_db() + + self.assertNotEqual(self.analysis_page.go_live_at, future_date) + + +class BundledPageMixinTestCase(TestCase): + """Test BundledPageMixin properties and methods.""" + + def setUp(self): + self.page = AnalysisPageFactory() + self.bundle = BundleFactory() + self.bundle_page = BundlePageFactory(parent=self.bundle, page=self.page) + + def test_bundles_property(self): + """Test bundles property returns correct queryset.""" + self.assertEqual(self.page.bundles.count(), 1) + self.assertEqual(self.page.bundles.first(), self.bundle) + + def test_active_bundles_property(self): + """Test active_bundles property returns correct queryset.""" + self.bundle.status = BundleStatus.RELEASED + self.bundle.save(update_fields=["status"]) + + self.assertEqual(self.page.active_bundles.count(), 0) + + self.bundle.status = BundleStatus.PENDING + self.bundle.save(update_fields=["status"]) + + self.assertEqual(self.page.active_bundles.count(), 1) + + def test_in_active_bundle_property(self): + """Test in_active_bundle property.""" + self.assertTrue(self.page.in_active_bundle) + + self.bundle.status = BundleStatus.RELEASED + self.bundle.save(update_fields=["status"]) + + del self.page.in_active_bundle # clear the cached property + self.assertFalse(self.page.in_active_bundle) + + def test_active_bundle_property(self): + """Test active_bundle property returns most recent active bundle.""" + self.assertEqual(self.page.active_bundle, self.bundle) + + self.bundle.status = BundleStatus.RELEASED + self.bundle.save(update_fields=["status"]) + + self.assertIsNone(self.page.active_bundle) diff --git a/cms/bundles/tests/test_panels.py b/cms/bundles/tests/test_panels.py new file mode 100644 index 00000000..f78b95e9 --- /dev/null +++ b/cms/bundles/tests/test_panels.py @@ -0,0 +1,51 @@ +from typing import TYPE_CHECKING + +from django.test import TestCase +from wagtail.test.utils import WagtailTestUtils + +from cms.analysis.tests.factories import AnalysisPageFactory +from cms.bundles.enums import BundleStatus +from cms.bundles.panels import BundleNotePanel +from cms.bundles.tests.factories import BundleFactory, BundlePageFactory + +if TYPE_CHECKING: + from wagtail.models import Page + + +class BundleNotePanelTestCase(WagtailTestUtils, TestCase): + """Test BundleNotePanel functionality.""" + + @classmethod + def setUpTestData(cls): + cls.superuser = cls.create_superuser(username="admin") + cls.page = AnalysisPageFactory() + cls.bundle = BundleFactory(name="Test Bundle", status=BundleStatus.PENDING) + cls.panel = BundleNotePanel() + + def get_bound_panel(self, page: "Page") -> BundleNotePanel.BoundPanel: + """Binds the panel to the given page.""" + return self.panel.bind_to_model(page._meta.model).get_bound_panel(instance=page) + + def test_panel_content_without_bundles(self): + """Test panel content when page is not in any bundles.""" + self.assertIn("This page is not part of any bundles", self.get_bound_panel(self.page).content) + + def test_panel_content_with_bundles(self): + """Test panel content when page is in bundles.""" + BundlePageFactory(parent=self.bundle, page=self.page) + + content = self.get_bound_panel(self.page).content + + self.assertIn("This page is in the following bundle(s):", content) + self.assertIn("Test Bundle", content) + self.assertIn("Status: Pending", content) + + def test_panel_content_non_bundled_model(self): + """Test panel content for non-bundled models.""" + + class DummyModel: + pass + + panel = self.panel.bind_to_model(DummyModel) + bound_panel = panel.get_bound_panel(instance=DummyModel()) + self.assertEqual(bound_panel.content, "") diff --git a/cms/bundles/tests/test_viewsets.py b/cms/bundles/tests/test_viewsets.py new file mode 100644 index 00000000..2c5502b4 --- /dev/null +++ b/cms/bundles/tests/test_viewsets.py @@ -0,0 +1,193 @@ +from http import HTTPStatus +from unittest import mock + +from django.test import TestCase +from django.urls import reverse +from wagtail.test.utils import WagtailTestUtils + +from cms.analysis.tests.factories import AnalysisPageFactory +from cms.bundles.enums import BundleStatus +from cms.bundles.models import Bundle +from cms.bundles.tests.factories import BundleFactory +from cms.bundles.tests.utils import grant_all_bundle_permissions, make_bundle_viewer +from cms.users.tests.factories import GroupFactory, UserFactory + + +class BundleViewSetTestCase(WagtailTestUtils, TestCase): + """Test Bundle viewset functionality.""" + + @classmethod + def setUpTestData(cls): + cls.superuser = cls.create_superuser(username="admin") + + cls.publishing_group = GroupFactory(name="Publishing Officers", access_admin=True) + grant_all_bundle_permissions(cls.publishing_group) + cls.publishing_officer = UserFactory(username="publishing_officer") + cls.publishing_officer.groups.add(cls.publishing_group) + + cls.bundle_viewer = UserFactory(username="bundle.viewer", access_admin=True) + make_bundle_viewer(cls.bundle_viewer) + + # a regular generic_user that can only access the Wagtail admin + cls.generic_user = UserFactory(username="generic.generic_user", access_admin=True) + + cls.bundle_index_url = reverse("bundle:index") + cls.bundle_add_url = reverse("bundle:add") + + def setUp(self): + self.bundle = BundleFactory(name="Original bundle", created_by=self.publishing_officer) + self.analysis_page = AnalysisPageFactory(title="PSF") + + self.edit_url = reverse("bundle:edit", args=[self.bundle.id]) + + def test_bundle_index__unhappy_paths(self): + """Test bundle list view permissions.""" + response = self.client.get(self.bundle_index_url) + self.assertEqual(response.status_code, HTTPStatus.FOUND) + self.assertRedirects(response, f"/admin/login/?next={self.bundle_index_url}") + + self.client.force_login(self.generic_user) + response = self.client.get(self.bundle_index_url, follow=True) + self.assertRedirects(response, "/admin/") + self.assertContains(response, "Sorry, you do not have permission to access this area.") + + def test_bundle_index__happy_path(self): + """Users with bundle permissions can see the index.""" + for user in [self.bundle_viewer, self.publishing_officer, self.superuser]: + with self.subTest(user=user): + self.client.force_login(user) + response = self.client.get(self.bundle_index_url) + self.assertEqual(response.status_code, HTTPStatus.OK) + + def test_bundle_add_view(self): + """Test bundle creation.""" + self.client.force_login(self.publishing_officer) + response = self.client.post( + self.bundle_add_url, + { + "name": "A New Bundle", + "status": BundleStatus.PENDING, + "bundled_pages-TOTAL_FORMS": "1", + "bundled_pages-INITIAL_FORMS": "0", + "bundled_pages-MIN_NUM_FORMS": "0", + "bundled_pages-MAX_NUM_FORMS": "1000", + "bundled_pages-0-page": str(self.analysis_page.id), + "bundled_pages-0-ORDER": "0", + }, + ) + + self.assertEqual(response.status_code, 302) + self.assertTrue(Bundle.objects.filter(name="A New Bundle").exists()) + + def test_bundle_add_view__with_page_already_in_a_bundle(self): + """Test bundle creation.""" + self.client.force_login(self.publishing_officer) + response = self.client.post( + self.bundle_add_url, + { + "name": "A New Bundle", + "status": BundleStatus.PENDING, + "bundled_pages-TOTAL_FORMS": "1", + "bundled_pages-INITIAL_FORMS": "0", + "bundled_pages-MIN_NUM_FORMS": "0", + "bundled_pages-MAX_NUM_FORMS": "1000", + "bundled_pages-0-page": str(self.analysis_page.id), + "bundled_pages-0-ORDER": "0", + }, + ) + + self.assertEqual(response.status_code, 302) + self.assertTrue(Bundle.objects.filter(name="A New Bundle").exists()) + + def test_bundle_add_view__without_permissions(self): + """Checks that users without permission cannot access the add bundle page.""" + for user in [self.generic_user, self.bundle_viewer]: + with self.subTest(user=user): + self.client.force_login(user) + response = self.client.get(self.bundle_add_url, follow=True) + self.assertRedirects(response, "/admin/") + self.assertContains(response, "Sorry, you do not have permission to access this area.") + + def test_bundle_edit_view(self): + """Test bundle editing.""" + url = reverse("bundle:edit", args=[self.bundle.id]) + + self.client.force_login(self.publishing_officer) + response = self.client.post( + url, + { + "name": "Updated Bundle", + "status": self.bundle.status, + "bundled_pages-TOTAL_FORMS": "1", + "bundled_pages-INITIAL_FORMS": "1", + "bundled_pages-MIN_NUM_FORMS": "0", + "bundled_pages-MAX_NUM_FORMS": "1000", + "bundled_pages-0-id": "", + "bundled_pages-0-page": str(self.analysis_page.id), + "bundled_pages-0-ORDER": "0", + }, + ) + + self.assertEqual(response.status_code, 302) + self.bundle.refresh_from_db() + self.assertEqual(self.bundle.name, "Updated Bundle") + + @mock.patch("cms.bundles.viewsets.notify_slack_of_status_change") + def test_bundle_approval__happy_path(self, mock_notify_slack): + """Test bundle approval workflow.""" + self.client.force_login(self.superuser) + response = self.client.post( + self.edit_url, + { + "name": self.bundle.name, + "status": BundleStatus.APPROVED, + "bundled_pages-TOTAL_FORMS": "1", + "bundled_pages-INITIAL_FORMS": "1", + "bundled_pages-MIN_NUM_FORMS": "0", + "bundled_pages-MAX_NUM_FORMS": "1000", + "bundled_pages-0-id": "", + "bundled_pages-0-page": str(self.analysis_page.id), + "bundled_pages-0-ORDER": "0", + }, + ) + + self.assertEqual(response.status_code, 302) + self.bundle.refresh_from_db() + self.assertEqual(self.bundle.status, BundleStatus.APPROVED) + self.assertIsNotNone(self.bundle.approved_at) + self.assertEqual(self.bundle.approved_by, self.superuser) + + self.assertTrue(mock_notify_slack.called) + + @mock.patch("cms.bundles.viewsets.notify_slack_of_status_change") + def test_bundle_approval__cannot__self_approve(self, mock_notify_slack): + """Test bundle approval workflow.""" + self.client.force_login(self.publishing_officer) + original_status = self.bundle.status + + response = self.client.post( + self.edit_url, + { + "name": self.bundle.name, + "status": BundleStatus.APPROVED, + "bundled_pages-TOTAL_FORMS": "1", + "bundled_pages-INITIAL_FORMS": "1", + "bundled_pages-MIN_NUM_FORMS": "0", + "bundled_pages-MAX_NUM_FORMS": "1000", + "bundled_pages-0-id": "", + "bundled_pages-0-page": str(self.analysis_page.id), + "bundled_pages-0-ORDER": "0", + }, + follow=True, + ) + + self.assertEqual(response.status_code, HTTPStatus.OK) + self.assertEqual(response.context["request"].path, self.edit_url) + self.assertContains(response, "You cannot self-approve your own bundle!") + + self.bundle.refresh_from_db() + self.assertEqual(self.bundle.status, original_status) + self.assertIsNone(self.bundle.approved_at) + self.assertIsNone(self.bundle.approved_by) + + self.assertFalse(mock_notify_slack.called) diff --git a/cms/bundles/tests/utils.py b/cms/bundles/tests/utils.py new file mode 100644 index 00000000..1450d5bc --- /dev/null +++ b/cms/bundles/tests/utils.py @@ -0,0 +1,46 @@ +from typing import TYPE_CHECKING + +from django.contrib.auth.models import Permission +from django.db.models import QuerySet + +if TYPE_CHECKING: + from django.contrib.auth.models import Group + + from cms.users.models import User + + +def get_all_bundle_permissions() -> QuerySet[Permission]: + """Gets all bundle permissions.""" + return Permission.objects.filter( + codename__in=[ + "add_bundle", + "change_bundle", + "delete_bundle", + "view_bundle", + ] + ) + + +def get_view_bundle_permission() -> Permission: + """Returns the view bundle permission.""" + return Permission.objects.get(codename="view_bundle") + + +def make_bundle_manager(user: "User") -> None: + """Givess all the bundle permissions to the given user.""" + user.user_permissions.add(*get_all_bundle_permissions()) + + +def make_bundle_viewer(user: "User") -> None: + """Gives the view bundle permission to the given user.""" + user.user_permissions.add(get_view_bundle_permission()) + + +def grant_all_bundle_permissions(group: "Group") -> None: + """Adds all the bundle permissions to the given group.""" + group.permissions.add(*get_all_bundle_permissions()) + + +def grant_view_bundle_permissions(group: "Group") -> None: + """Adds the view bundle permission to the given group.""" + group.permissions.add(get_view_bundle_permission()) diff --git a/cms/users/tests/__init__.py b/cms/users/tests/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/cms/users/tests/factories.py b/cms/users/tests/factories.py new file mode 100644 index 00000000..0b4afd43 --- /dev/null +++ b/cms/users/tests/factories.py @@ -0,0 +1,59 @@ +import factory +from django.contrib.auth.hashers import make_password +from django.contrib.auth.models import Group, Permission +from factory.django import DjangoModelFactory + +from cms.users.models import User + + +class GroupFactory(DjangoModelFactory): + class Meta: + model = Group + django_get_or_create = ("name",) + + @factory.post_generation + def access_admin(self, create, extracted, **kwargs): + """Creates BundlePage instances for the bundle. + + Usage: + # Create a Django generic_user group + group = GroupFactory() + + # Create a Django generic_user group with Wagtail admin access + group = GroupFactory(access_admin=True) + """ + if not create: + return + + if extracted: + admin_permission = Permission.objects.get(content_type__app_label="wagtailadmin", codename="access_admin") + self.permissions.add(admin_permission) + + +class UserFactory(DjangoModelFactory): + username = factory.Faker("user_name") + email = factory.Faker("email") + password = factory.LazyFunction(lambda: make_password("password")) + first_name = factory.Faker("first_name") + last_name = factory.Faker("last_name") + + class Meta: + model = User + + @factory.post_generation + def access_admin(self, create, extracted, **kwargs): + """Creates BundlePage instances for the bundle. + + Usage: + # Create a Django generic_user group + generic_user = UserFactory() + + # Create a Django generic_user group with Wagtail admin access + generic_user = UserFactory(access_admin=True) + """ + if not create: + return + + if extracted: + admin_permission = Permission.objects.get(content_type__app_label="wagtailadmin", codename="access_admin") + self.user_permissions.add(admin_permission) From 096dfa6de85c1d663686a59a43ec8f88b791cdb8 Mon Sep 17 00:00:00 2001 From: zerolab Date: Thu, 21 Nov 2024 10:36:52 +0000 Subject: [PATCH 05/13] Expand test coverage --- cms/bundles/admin_forms.py | 22 +- cms/bundles/forms.py | 9 +- cms/bundles/notifications.py | 4 +- cms/bundles/tests/test_forms.py | 141 ++++++++++++ cms/bundles/tests/test_management_commands.py | 201 ++++++++++++++++++ cms/bundles/tests/test_models.py | 6 + cms/bundles/tests/test_notifications.py | 176 +++++++++++++++ cms/bundles/tests/test_views.py | 87 ++++++++ cms/bundles/tests/test_viewsets.py | 70 ++++-- cms/bundles/tests/test_wagtail_hooks.py | 183 ++++++++++++++++ cms/bundles/tests/utils.py | 10 + cms/bundles/wagtail_hooks.py | 2 +- cms/settings/test.py | 4 + 13 files changed, 888 insertions(+), 27 deletions(-) create mode 100644 cms/bundles/tests/test_forms.py create mode 100644 cms/bundles/tests/test_management_commands.py create mode 100644 cms/bundles/tests/test_notifications.py create mode 100644 cms/bundles/tests/test_views.py create mode 100644 cms/bundles/tests/test_wagtail_hooks.py diff --git a/cms/bundles/admin_forms.py b/cms/bundles/admin_forms.py index 99ead3db..291b8a9d 100644 --- a/cms/bundles/admin_forms.py +++ b/cms/bundles/admin_forms.py @@ -1,18 +1,21 @@ -from typing import Any +from typing import TYPE_CHECKING, Any from django import forms from django.core.exceptions import ValidationError from django.utils.translation import gettext as _ -from .models import Bundle +from .models import Bundle, BundledPageMixin from .viewsets import BundleChooserWidget +if TYPE_CHECKING: + from wagtail.models import Page + class AddToBundleForm(forms.Form): """Administrative form used in the 'add to bundle' view.""" def __init__(self, *args: Any, **kwargs: Any) -> None: - self.page_to_add = kwargs.pop("page_to_add") + self.page_to_add: Page = kwargs.pop("page_to_add") super().__init__(*args, **kwargs) @@ -26,8 +29,15 @@ def __init__(self, *args: Any, **kwargs: Any) -> None: def clean(self) -> None: super().clean() + if not isinstance(self.page_to_add, BundledPageMixin): + # While this form is used the "add to bundle" view which already checks for this, + # it doesn't hurt to trust but verify. + raise ValidationError(_("Pages of this type cannot be added.")) + bundle = self.cleaned_data.get("bundle") if bundle and bundle.bundled_pages.filter(page=self.page_to_add).exists(): - raise ValidationError( - {"bundle": f"Page {self.page_to_add.get_admin_display_title()} is already in bundle '{bundle}'"} - ) + message = _("Page '%(page)s' is already in bundle '%(bundle)s'") % { + "page": self.page_to_add.get_admin_display_title(), # type: ignore[attr-defined] + "bundle": bundle, + } + raise ValidationError({"bundle": message}) diff --git a/cms/bundles/forms.py b/cms/bundles/forms.py index aebb6c18..62f80249 100644 --- a/cms/bundles/forms.py +++ b/cms/bundles/forms.py @@ -3,6 +3,7 @@ from django import forms from django.core.exceptions import ValidationError from django.utils import timezone +from django.utils.translation import gettext as _ from wagtail.admin.forms import WagtailAdminModelForm from cms.bundles.enums import ACTIVE_BUNDLE_STATUS_CHOICES, EDITABLE_BUNDLE_STATUSES, BundleStatus @@ -56,7 +57,13 @@ def _validate_bundled_pages(self) -> None: else: page = page.specific if page.in_active_bundle and page.active_bundle != self.instance and not form.cleaned_data["DELETE"]: - raise ValidationError(f"{page} is already in an active bundle ({page.active_bundle})") + raise ValidationError( + _("'%(page)s' is already in an active bundle (%(bundle)s)") + % { + "page": page, + "bundle": page.active_bundle, + } + ) def clean(self) -> dict[str, Any] | None: """Validates the form. diff --git a/cms/bundles/notifications.py b/cms/bundles/notifications.py index df98f60f..cfea1b31 100644 --- a/cms/bundles/notifications.py +++ b/cms/bundles/notifications.py @@ -70,7 +70,7 @@ def notify_slack_of_publication_start(bundle: Bundle, user: Optional["User"] = N ) if response.status_code != HTTPStatus.OK: - logger.error("Unable to notify Slack of bundle status change: %s", response.body) + logger.error("Unable to notify Slack of bundle publication start: %s", response.body) def notify_slack_of_publish_end( @@ -101,4 +101,4 @@ def notify_slack_of_publish_end( ) if response.status_code != HTTPStatus.OK: - logger.error("Unable to notify Slack of bundle status change: %s", response.body) + logger.error("Unable to notify Slack of bundle publication finish: %s", response.body) diff --git a/cms/bundles/tests/test_forms.py b/cms/bundles/tests/test_forms.py new file mode 100644 index 00000000..92f1459a --- /dev/null +++ b/cms/bundles/tests/test_forms.py @@ -0,0 +1,141 @@ +from typing import Any + +from django import forms +from django.test import TestCase +from wagtail.admin.panels import get_edit_handler +from wagtail.test.utils.form_data import inline_formset, nested_form_data + +from cms.analysis.tests.factories import AnalysisPageFactory +from cms.bundles.admin_forms import AddToBundleForm +from cms.bundles.enums import ACTIVE_BUNDLE_STATUS_CHOICES, BundleStatus +from cms.bundles.models import Bundle +from cms.bundles.tests.factories import BundleFactory, BundlePageFactory +from cms.bundles.viewsets import BundleChooserWidget +from cms.release_calendar.tests.factories import ReleaseCalendarPageFactory +from cms.users.tests.factories import UserFactory + + +class AddToBundleFormTestCase(TestCase): + @classmethod + def setUpTestData(cls): + cls.bundle = BundleFactory(name="First Bundle") + cls.non_editable_bundle = BundleFactory(approved=True) + cls.page = AnalysisPageFactory(title="The Analysis") + + def test_form_init(self): + """Checks the form gets a bundle form field on init.""" + form = AddToBundleForm(page_to_add=self.page) + self.assertIn("bundle", form.fields) + self.assertIsInstance(form.fields["bundle"].widget, BundleChooserWidget) + self.assertQuerySetEqual( + form.fields["bundle"].queryset, + Bundle.objects.filter(pk=self.bundle.pk), + ) + + def test_form_clean__validates_page_not_in_bundle(self): + """Checks that we cannot add the page to a new bundle if already in an active one.""" + BundlePageFactory(parent=self.bundle, page=self.page) + form = AddToBundleForm(page_to_add=self.page, data={"bundle": self.bundle.pk}) + + self.assertFalse(form.is_valid()) + self.assertDictEqual( + form.errors, + {"bundle": [f"Page '{self.page.get_admin_display_title()}' is already in bundle 'First Bundle'"]}, + ) + + def test_form_clean__validates_page_is_bundleable(self): + """Checks the given page inherits from BundlePageMixin.""" + form = AddToBundleForm(page_to_add=ReleaseCalendarPageFactory(), data={"bundle": self.bundle.pk}) + self.assertFalse(form.is_valid()) + self.assertDictEqual(form.errors, {"__all__": ["Pages of this type cannot be added."]}) + + +class BundleAdminFormTestCase(TestCase): + @classmethod + def setUpTestData(cls): + cls.bundle = BundleFactory(name="First Bundle") + cls.page = AnalysisPageFactory(title="The Analysis") + cls.form_class = get_edit_handler(Bundle).get_form_class() + + def setUp(self): + self.form_data = nested_form_data(self.raw_form_data()) + + def raw_form_data(self) -> dict[str, Any]: + """Returns raw form data.""" + return { + "name": "First Bundle", + "status": BundleStatus.IN_REVIEW, + "bundled_pages": inline_formset([]), + } + + def test_form_init__status_choices(self): + """Checks status choices variation.""" + cases = [ + (BundleStatus.PENDING, ACTIVE_BUNDLE_STATUS_CHOICES), + (BundleStatus.IN_REVIEW, ACTIVE_BUNDLE_STATUS_CHOICES), + (BundleStatus.APPROVED, BundleStatus.choices), + (BundleStatus.RELEASED, BundleStatus.choices), + ] + for status, choices in cases: + with self.subTest(status=status, choices=choices): + self.bundle.status = status + form = self.form_class(instance=self.bundle) + self.assertEqual(form.fields["status"].choices, choices) + + def test_form_init__approved_by_at_are_disabled(self): + """Checks that approved_at and approved_by are disabled. They are programmatically set.""" + form = self.form_class(instance=self.bundle) + self.assertTrue(form.fields["approved_at"].disabled) + self.assertTrue(form.fields["approved_by"].disabled) + + self.assertIsInstance(form.fields["approved_by"].widget, forms.HiddenInput) + self.assertIsInstance(form.fields["approved_at"].widget, forms.HiddenInput) + + def test_form_init__fields_disabled_if_status_is_approved(self): + """Checks that all but the status field are disabled once approved to prevent further editing.""" + self.bundle.status = BundleStatus.APPROVED + form = self.form_class(instance=self.bundle) + fields = {field: True for field in form.fields} + fields["status"] = False + + for field, expected in fields.items(): + with self.subTest(field=field, expected=expected): + self.assertEqual(form.fields[field].disabled, expected) + + def test_clean__removes_deleted_page_references(self): + """Checks that we clean up references to pages that may have been deleted since being added to the bundle.""" + raw_data = self.raw_form_data() + raw_data["bundled_pages"] = inline_formset([{"page": ""}]) + data = nested_form_data(raw_data) + + form = self.form_class(instance=self.bundle, data=data) + + self.assertTrue(form.is_valid()) + formset = form.formsets["bundled_pages"] + self.assertTrue(formset.forms[0].cleaned_data["DELETE"]) + + def test_clean__validates_added_page_not_in_another_bundle(self): + """Should validate that the page is not in the active bundle.""" + another_bundle = BundleFactory(name="Another Bundle") + BundlePageFactory(parent=another_bundle, page=self.page) + + raw_data = self.raw_form_data() + raw_data["bundled_pages"] = inline_formset([{"page": self.page.id}]) + data = nested_form_data(raw_data) + + form = self.form_class(instance=self.bundle, data=data) + self.assertFalse(form.is_valid()) + self.assertDictEqual( + form.errors, {"__all__": ["'The Analysis' is already in an active bundle (Another Bundle)"]} + ) + + def test_clean__sets_approved_by_and_approved_at(self): + """Checks that Bundle gets the approver and approval time set.""" + approver = UserFactory() + + data = self.form_data + data["status"] = BundleStatus.APPROVED + form = self.form_class(instance=self.bundle, data=data, for_user=approver) + + self.assertTrue(form.is_valid()) + self.assertEqual(form.cleaned_data["approved_by"], approver) diff --git a/cms/bundles/tests/test_management_commands.py b/cms/bundles/tests/test_management_commands.py new file mode 100644 index 00000000..a5fb3628 --- /dev/null +++ b/cms/bundles/tests/test_management_commands.py @@ -0,0 +1,201 @@ +from datetime import timedelta +from io import StringIO +from unittest.mock import patch + +from django.core.management import call_command +from django.test import TestCase, override_settings +from django.utils import timezone +from wagtail.models import ModelLogEntry, PageLogEntry + +from cms.analysis.tests.factories import AnalysisPageFactory +from cms.bundles.enums import BundleStatus +from cms.bundles.tests.factories import BundleFactory, BundlePageFactory +from cms.home.models import HomePage +from cms.release_calendar.enums import ReleaseStatus +from cms.release_calendar.tests.factories import ReleaseCalendarPageFactory + + +class PublishBundlesCommandTestCase(TestCase): + def setUp(self): + self.stdout = StringIO() + self.stderr = StringIO() + + self.publication_date = timezone.now() - timedelta(minutes=1) + self.analysis_page = AnalysisPageFactory(title="Test Analysis", live=False) + self.analysis_page.save_revision(approved_go_live_at=self.publication_date) + + self.bundle = BundleFactory(approved=True, name="Test Bundle", publication_date=self.publication_date) + + def call_command(self, *args, **kwargs): + """Helper to call the management command.""" + call_command( + "publish_bundles", + *args, + stdout=self.stdout, + stderr=self.stderr, + **kwargs, + ) + + def test_dry_run_with_no_bundles(self): + """Test dry run output when there are no bundles to publish.""" + self.bundle.publication_date = timezone.now() + timedelta(minutes=10) + self.bundle.save(update_fields=["publication_date"]) + + self.call_command(dry_run=True) + + output = self.stdout.getvalue() + self.assertIn("Will do a dry run", output) + self.assertIn("No bundles to go live", output) + + def test_dry_run_with_bundles(self): + """Test dry run output when there are bundles to publish.""" + BundlePageFactory(parent=self.bundle, page=self.analysis_page) + + self.call_command(dry_run=True) + + output = self.stdout.getvalue() + self.assertIn("Will do a dry run", output) + self.assertIn("Bundles to be published:", output) + self.assertIn(f"- {self.bundle.name}", output) + self.assertIn( + f"{self.analysis_page.get_admin_display_title()} ({self.analysis_page.__class__.__name__})", output + ) + + @override_settings(SLACK_NOTIFICATIONS_WEBHOOK_URL="https://slack.example.com") + @patch("cms.bundles.management.commands.publish_bundles.notify_slack_of_publication_start") + @patch("cms.bundles.management.commands.publish_bundles.notify_slack_of_publish_end") + def test_publish_bundle(self, mock_notify_end, mock_notify_start): + """Test publishing a bundle.""" + # Sanity checks + self.assertFalse(self.analysis_page.live) + self.assertFalse(ModelLogEntry.objects.filter(action="wagtail.publish.scheduled").exists()) + self.assertFalse(PageLogEntry.objects.filter(action="wagtail.publish.scheduled").exists()) + + # add another page, but publish in the meantime. + another_page = AnalysisPageFactory(title="Test Analysis", live=False) + another_page.save_revision().publish() + BundlePageFactory(parent=self.bundle, page=self.analysis_page) + BundlePageFactory(parent=self.bundle, page=another_page) + + self.call_command() + + self.bundle.refresh_from_db() + self.assertEqual(self.bundle.status, BundleStatus.RELEASED) + + self.analysis_page.refresh_from_db() + self.assertTrue(self.analysis_page.live) + + # Check notifications were sent + self.assertTrue(mock_notify_start.called) + self.assertTrue(mock_notify_end.called) + + # Check that we have a log entry + self.assertEqual(ModelLogEntry.objects.filter(action="wagtail.publish.scheduled").count(), 1) + self.assertEqual(PageLogEntry.objects.filter(action="wagtail.publish.scheduled").count(), 1) + + def test_publish_bundle_with_release_calendar(self): + """Test publishing a bundle with an associated release calendar page.""" + release_page = ReleaseCalendarPageFactory(release_date=self.publication_date) + BundlePageFactory(parent=self.bundle, page=self.analysis_page) + self.bundle.publication_date = None + self.bundle.release_calendar_page = release_page + self.bundle.save(update_fields=["publication_date", "release_calendar_page"]) + + self.call_command() + + # Check release calendar was updated + release_page.refresh_from_db() + self.assertEqual(release_page.status, ReleaseStatus.PUBLISHED) + + content = release_page.content[0].value + self.assertEqual(content["title"], "Publications") + self.assertEqual(len(content["links"]), 1) + self.assertEqual(content["links"][0]["page"].pk, self.analysis_page.pk) + + @override_settings(SLACK_NOTIFICATIONS_WEBHOOK_URL="https://slack.ons.gov.uk") + @patch("cms.bundles.management.commands.publish_bundles.logger") + def test_publish_bundle_error_handling(self, mock_logger): + """Test error handling during bundle publication.""" + BundlePageFactory(parent=self.bundle, page=self.analysis_page) + + # Mock an error during publication + with patch( + "cms.bundles.management.commands.publish_bundles.notify_slack_of_publication_start", + side_effect=Exception("Test error"), + ): + self.call_command() + + # Check error was logged + mock_logger.exception.assert_called_with("Publish failed bundle=%d", self.bundle.id) + + # Check bundle status wasn't changed due to error + self.bundle.refresh_from_db() + self.assertEqual(self.bundle.status, BundleStatus.APPROVED) + + @override_settings(WAGTAILADMIN_BASE_URL="https://test.ons.gov.uk") + @override_settings(SLACK_NOTIFICATIONS_WEBHOOK_URL="https://slack.ons.gov.uk") + @patch("cms.bundles.management.commands.publish_bundles.notify_slack_of_publication_start") + def test_publish_bundle_with_base_url(self, mock_notify): + """Test publishing with a configured base URL.""" + self.call_command() + + # Verify notification was called with correct URL + mock_notify.assert_called_once() + call_kwargs = mock_notify.call_args[1] + self.assertTrue(call_kwargs["url"].startswith("https://test.ons.gov.uk")) + self.assertIn(str(self.bundle.pk), call_kwargs["url"]) + + +class PublishScheduledWithoutBundlesCommandTestCase(TestCase): + @classmethod + def setUpTestData(cls): + cls.home = HomePage.objects.first() + cls.analysis_page = AnalysisPageFactory(title="Test Analysis", live=False) + cls.bundle = BundleFactory(name="Test Bundle", bundled_pages=[cls.analysis_page]) + + cls.publication_date = timezone.now() - timedelta(minutes=1) + cls.analysis_page.save_revision(approved_go_live_at=cls.publication_date) + + def setUp(self): + self.stdout = StringIO() + self.stderr = StringIO() + + def call_command(self, *args, **kwargs): + """Helper to call the management command.""" + call_command( + "publish_scheduled_without_bundles", + *args, + stdout=self.stdout, + stderr=self.stderr, + **kwargs, + ) + + def test_dry_run(self): + """Test dry run doesn't include our bundled page.""" + self.call_command(dry_run=True) + + output = self.stdout.getvalue() + self.assertIn("Will do a dry run.", output) + self.assertIn("No objects to go live.", output) + + def test_dry_run__with_a_scheduled_page(self): + """Test dry run doesn't include our bundled page.""" + self.home.save_revision(approved_go_live_at=self.publication_date) + + self.call_command(dry_run=True) + + output = self.stdout.getvalue() + self.assertIn("Will do a dry run.", output) + + self.assertIn("Revisions to be published:", output) + self.assertIn(self.home.title, output) + + self.assertFalse(PageLogEntry.objects.filter(action="wagtail.publish.scheduled").exists()) + + def test_publish_scheduled_without_bundles__happy_path(self): + """Checks only a scheduled non-bundled page has been published.""" + self.home.save_revision(approved_go_live_at=self.publication_date) + + self.call_command() + + self.assertEqual(PageLogEntry.objects.filter(action="wagtail.publish.scheduled").count(), 1) diff --git a/cms/bundles/tests/test_models.py b/cms/bundles/tests/test_models.py index ae87f219..8fbfa782 100644 --- a/cms/bundles/tests/test_models.py +++ b/cms/bundles/tests/test_models.py @@ -77,6 +77,12 @@ def test_save_doesnt_update_dates_when_released(self): self.assertNotEqual(self.analysis_page.go_live_at, future_date) + def test_bundlepage_orderable_str(self): + """Checks the BundlePage model __str__ method.""" + bundle_page = BundlePageFactory(parent=self.bundle, page=self.analysis_page) + + self.assertEqual(str(bundle_page), f"BundlePage: page {self.analysis_page.pk} in bundle {self.bundle.id}") + class BundledPageMixinTestCase(TestCase): """Test BundledPageMixin properties and methods.""" diff --git a/cms/bundles/tests/test_notifications.py b/cms/bundles/tests/test_notifications.py new file mode 100644 index 00000000..92b91044 --- /dev/null +++ b/cms/bundles/tests/test_notifications.py @@ -0,0 +1,176 @@ +from http import HTTPStatus +from unittest.mock import Mock, patch + +from django.test import RequestFactory, TestCase, override_settings +from django.urls import reverse + +from cms.analysis.tests.factories import AnalysisPageFactory +from cms.bundles.enums import BundleStatus +from cms.bundles.notifications import ( + notify_slack_of_publication_start, + notify_slack_of_publish_end, + notify_slack_of_status_change, +) +from cms.bundles.tests.factories import BundleFactory +from cms.users.tests.factories import UserFactory + + +class SlackNotificationsTestCase(TestCase): + @classmethod + def setUpTestData(cls): + cls.bundle = BundleFactory(name="First Bundle", bundled_pages=[AnalysisPageFactory()]) + cls.user = UserFactory(first_name="Publishing", last_name="Officer") + request = RequestFactory().get("/") + cls.inspect_url = request.build_absolute_uri(reverse("bundle:inspect", args=(cls.bundle.pk,))) + + def setUp(self): + self.mock_response = Mock() + self.mock_response.status_code = HTTPStatus.OK + self.mock_response.body = "" + + @override_settings(SLACK_NOTIFICATIONS_WEBHOOK_URL="https://slack.ons.gov.uk") + @patch("cms.bundles.notifications.WebhookClient") + def test_notify_slack_of_status_change__happy_path(self, mock_client): + """Should send notification with correct fields.""" + mock_client.return_value.send.return_value = self.mock_response + + self.bundle.status = BundleStatus.IN_REVIEW + notify_slack_of_status_change(self.bundle, BundleStatus.PENDING.label, self.user) + + mock_client.return_value.send.assert_called_once() + call_kwargs = mock_client.return_value.send.call_args[1] + + self.assertEqual(call_kwargs["text"], "Bundle status changed") + + self.assertListEqual( + call_kwargs["attachments"][0]["fields"], + [ + {"title": "Title", "value": "First Bundle", "short": True}, + {"title": "Changed by", "value": "Publishing Officer", "short": True}, + {"title": "Old status", "value": BundleStatus.PENDING.label, "short": True}, + {"title": "New status", "value": BundleStatus.IN_REVIEW.label, "short": True}, + ], + ) + + @patch("cms.bundles.notifications.WebhookClient") + def test_notify_slack_of_status_change__no_webhook_url(self, mock_client): + """Should return early if no webhook URL is configured.""" + notify_slack_of_status_change(self.bundle, BundleStatus.PENDING, self.user) + mock_client.assert_not_called() + + @override_settings(SLACK_NOTIFICATIONS_WEBHOOK_URL="https://slack.ons.gov.uk") + @patch("cms.bundles.notifications.WebhookClient") + def test_notify_slack_of_status_change__error_logging(self, mock_client): + """Should log error if Slack request fails.""" + with patch("cms.bundles.notifications.logger") as mock_logger: + self.mock_response.status_code = HTTPStatus.BAD_REQUEST + self.mock_response.body = "Error message" + mock_client.return_value.send.return_value = self.mock_response + + self.bundle.status = BundleStatus.IN_REVIEW + notify_slack_of_status_change(self.bundle, BundleStatus.PENDING.label, self.user, self.inspect_url) + call_kwargs = mock_client.return_value.send.call_args[1] + self.assertListEqual( + call_kwargs["attachments"][0]["fields"], + [ + {"title": "Title", "value": "First Bundle", "short": True}, + {"title": "Changed by", "value": "Publishing Officer", "short": True}, + {"title": "Old status", "value": BundleStatus.PENDING.label, "short": True}, + {"title": "New status", "value": BundleStatus.IN_REVIEW.label, "short": True}, + {"title": "Link", "value": self.inspect_url, "short": False}, + ], + ) + + mock_logger.error.assert_called_once_with( + "Unable to notify Slack of bundle status change: %s", "Error message" + ) + + @override_settings(SLACK_NOTIFICATIONS_WEBHOOK_URL="https://slack.ons.gov.uk") + @patch("cms.bundles.notifications.WebhookClient") + def test_notify_slack_of_publication_start__happy_path(self, mock_client): + """Should send notification with correct fields.""" + mock_client.return_value.send.return_value = self.mock_response + + notify_slack_of_publication_start(self.bundle, self.user, self.inspect_url) + + mock_client.return_value.send.assert_called_once() + call_kwargs = mock_client.return_value.send.call_args[1] + + self.assertEqual(call_kwargs["text"], "Starting bundle publication") + self.assertListEqual( + call_kwargs["attachments"][0]["fields"], + [ + {"title": "Title", "value": "First Bundle", "short": True}, + {"title": "User", "value": "Publishing Officer", "short": True}, + {"title": "Pages", "value": 1, "short": True}, + {"title": "Link", "value": self.inspect_url, "short": False}, + ], + ) + + def test_notify_slack_of_publication_start__no_webhook_url(self): + """Should return early if no webhook URL is configured.""" + with patch("slack_sdk.webhook.WebhookClient") as mock_client: + notify_slack_of_publication_start(self.bundle, self.user) + mock_client.assert_not_called() + + @override_settings(SLACK_NOTIFICATIONS_WEBHOOK_URL="https://slack.ons.gov.uk") + @patch("cms.bundles.notifications.WebhookClient") + def test_notify_slack_of_publish_start__error_logging(self, mock_client): + """Should log error if Slack request fails.""" + with patch("cms.bundles.notifications.logger") as mock_logger: + self.mock_response.status_code = HTTPStatus.BAD_REQUEST + self.mock_response.body = "Error message" + mock_client.return_value.send.return_value = self.mock_response + + notify_slack_of_publication_start(self.bundle, self.user) + + mock_logger.error.assert_called_once_with( + "Unable to notify Slack of bundle publication start: %s", "Error message" + ) + + @override_settings(SLACK_NOTIFICATIONS_WEBHOOK_URL="https://slack.ons.gov.uk") + @patch("cms.bundles.notifications.WebhookClient") + def test_notify_slack_of_publish_end__happy_path(self, mock_client): + """Should send notification with correct fields.""" + mock_client.return_value.send.return_value = self.mock_response + + notify_slack_of_publish_end(self.bundle, 1.234, self.user, self.inspect_url) + + mock_client.return_value.send.assert_called_once() + call_kwargs = mock_client.return_value.send.call_args[1] + + self.assertEqual(call_kwargs["text"], "Finished bundle publication") + fields = call_kwargs["attachments"][0]["fields"] + + self.assertListEqual( + fields, + [ + {"title": "Title", "value": "First Bundle", "short": True}, + {"title": "User", "value": "Publishing Officer", "short": True}, + {"title": "Pages", "value": 1, "short": True}, + {"title": "Total time", "value": "1.234 seconds"}, + {"title": "Link", "value": self.inspect_url, "short": False}, + ], + ) + self.assertEqual(len(fields), 5) # Including URL and elapsed time + + def test_notify_slack_of_publish_end__no_webhook_url(self): + """Should return early if no webhook URL is configured.""" + with patch("slack_sdk.webhook.WebhookClient") as mock_client: + notify_slack_of_publish_end(self.bundle, 1.234, self.user) + mock_client.assert_not_called() + + @override_settings(SLACK_NOTIFICATIONS_WEBHOOK_URL="https://slack.ons.gov.uk") + @patch("cms.bundles.notifications.WebhookClient") + def test_notify_slack_of_publish_end__error_logging(self, mock_client): + """Should log error if Slack request fails.""" + with patch("cms.bundles.notifications.logger") as mock_logger: + self.mock_response.status_code = HTTPStatus.BAD_REQUEST + self.mock_response.body = "Error message" + mock_client.return_value.send.return_value = self.mock_response + + notify_slack_of_publish_end(self.bundle, 1.234, self.user, self.inspect_url) + + mock_logger.error.assert_called_once_with( + "Unable to notify Slack of bundle publication finish: %s", "Error message" + ) diff --git a/cms/bundles/tests/test_views.py b/cms/bundles/tests/test_views.py new file mode 100644 index 00000000..0c15a24e --- /dev/null +++ b/cms/bundles/tests/test_views.py @@ -0,0 +1,87 @@ +from http import HTTPStatus + +from django.test import TestCase +from django.urls import reverse +from wagtail.test.utils.wagtail_tests import WagtailTestUtils + +from cms.analysis.tests.factories import AnalysisPageFactory +from cms.bundles.admin_forms import AddToBundleForm +from cms.bundles.models import Bundle +from cms.bundles.tests.factories import BundleFactory, BundlePageFactory +from cms.bundles.tests.utils import grant_all_bundle_permissions, grant_all_page_permissions, make_bundle_viewer +from cms.users.tests.factories import GroupFactory, UserFactory + + +class AddToBundleViewTestCase(WagtailTestUtils, TestCase): + @classmethod + def setUpTestData(cls): + cls.superuser = cls.create_superuser(username="admin") + + cls.publishing_group = GroupFactory(name="Publishing Officers", access_admin=True) + grant_all_bundle_permissions(cls.publishing_group) + grant_all_page_permissions(cls.publishing_group) + cls.publishing_officer = UserFactory(username="publishing_officer") + cls.publishing_officer.groups.add(cls.publishing_group) + + cls.bundle_viewer = UserFactory(username="bundle.viewer", access_admin=True) + make_bundle_viewer(cls.bundle_viewer) + + def setUp(self): + self.bundle = BundleFactory(name="First Bundle", created_by=self.publishing_officer) + self.analysis_page = AnalysisPageFactory(title="November 2024", parent__title="PSF") + self.add_url = reverse("bundles:add_to_bundle", args=[self.analysis_page.id]) + self.bundle_index_url = reverse("bundle:index") + + self.client.force_login(self.publishing_officer) + + def test_dispatch__happy_path(self): + """Dispatch should not complain about anything.""" + response = self.client.get(f"{self.add_url}?next={self.bundle_index_url}") + self.assertEqual(response.status_code, HTTPStatus.OK) + self.assertTemplateUsed(response, "bundles/wagtailadmin/add_to_bundle.html") + + self.assertEqual(response.context["page_to_add"], self.analysis_page) + self.assertEqual(response.context["next"], self.bundle_index_url) + self.assertIsInstance(response.context["form"], AddToBundleForm) + self.assertEqual(response.context["form"].page_to_add, self.analysis_page) + + def test_dispatch__returns_404_for_wrong_page_id(self): + """The page must exist in the first place.""" + url = reverse("bundles:add_to_bundle", args=[99999]) + response = self.client.get(url) + self.assertEqual(response.status_code, HTTPStatus.NOT_FOUND) + + def test_dispatch__returns_404_for_non_bundleable_page(self): + """Only pages with BundledPageMixin can be added to a bundle.""" + url = reverse("bundles:add_to_bundle", args=[self.analysis_page.get_parent().id]) + response = self.client.get(url) + self.assertEqual(response.status_code, HTTPStatus.NOT_FOUND) + + def test_dispatch__returns_404_if_user_doesnt_have_access(self): + """Only users that can edit see the page are allowed to add it to the bundle.""" + self.client.force_login(self.bundle_viewer) + response = self.client.get(self.add_url, follow=True) + self.assertRedirects(response, "/admin/") + self.assertContains(response, "Sorry, you do not have permission to access this area.") + + def test_dispatch__doesnt_allow_adding_page_already_in_active_bundle(self): + """Tests that we get redirected away with a corresponding message when the page we try to add to the bundle is + already in a different bundle. + """ + another_bundle = BundleFactory(name="Another Bundle") + BundlePageFactory(parent=another_bundle, page=self.analysis_page) + response = self.client.get(self.add_url, follow=True) + self.assertRedirects(response, "/admin/") + self.assertContains(response, "PSF: November 2024 is already in a bundle") + + def test_post__successful(self): + """Checks that on successful post, the page is added to the bundle and + we get redirected to the valid next URL. + """ + response = self.client.post( + f"{self.add_url}?next={self.bundle_index_url}", data={"bundle": self.bundle.id}, follow=True + ) + + self.assertEqual(response.status_code, HTTPStatus.OK) + self.assertContains(response, "Page 'PSF: November 2024' added to bundle 'First Bundle'") + self.assertQuerySetEqual(self.analysis_page.bundles, Bundle.objects.all()) diff --git a/cms/bundles/tests/test_viewsets.py b/cms/bundles/tests/test_viewsets.py index 2c5502b4..29dbc5f0 100644 --- a/cms/bundles/tests/test_viewsets.py +++ b/cms/bundles/tests/test_viewsets.py @@ -4,6 +4,7 @@ from django.test import TestCase from django.urls import reverse from wagtail.test.utils import WagtailTestUtils +from wagtail.test.utils.form_data import inline_formset, nested_form_data from cms.analysis.tests.factories import AnalysisPageFactory from cms.bundles.enums import BundleStatus @@ -34,14 +35,23 @@ def setUpTestData(cls): cls.bundle_index_url = reverse("bundle:index") cls.bundle_add_url = reverse("bundle:add") + cls.released_bundle = BundleFactory(released=True, name="Release Bundle") + cls.released_bundle_edit_url = reverse("bundle:edit", args=[cls.released_bundle.id]) + + cls.approved_bundle = BundleFactory(approved=True, name="Approve Bundle") + cls.approved_bundle_edit_url = reverse("bundle:edit", args=[cls.approved_bundle.id]) + def setUp(self): self.bundle = BundleFactory(name="Original bundle", created_by=self.publishing_officer) self.analysis_page = AnalysisPageFactory(title="PSF") self.edit_url = reverse("bundle:edit", args=[self.bundle.id]) + self.client.force_login(self.publishing_officer) + def test_bundle_index__unhappy_paths(self): """Test bundle list view permissions.""" + self.client.logout() response = self.client.get(self.bundle_index_url) self.assertEqual(response.status_code, HTTPStatus.FOUND) self.assertRedirects(response, f"/admin/login/?next={self.bundle_index_url}") @@ -61,7 +71,6 @@ def test_bundle_index__happy_path(self): def test_bundle_add_view(self): """Test bundle creation.""" - self.client.force_login(self.publishing_officer) response = self.client.post( self.bundle_add_url, { @@ -81,7 +90,6 @@ def test_bundle_add_view(self): def test_bundle_add_view__with_page_already_in_a_bundle(self): """Test bundle creation.""" - self.client.force_login(self.publishing_officer) response = self.client.post( self.bundle_add_url, { @@ -110,28 +118,45 @@ def test_bundle_add_view__without_permissions(self): def test_bundle_edit_view(self): """Test bundle editing.""" - url = reverse("bundle:edit", args=[self.bundle.id]) - - self.client.force_login(self.publishing_officer) response = self.client.post( - url, - { - "name": "Updated Bundle", - "status": self.bundle.status, - "bundled_pages-TOTAL_FORMS": "1", - "bundled_pages-INITIAL_FORMS": "1", - "bundled_pages-MIN_NUM_FORMS": "0", - "bundled_pages-MAX_NUM_FORMS": "1000", - "bundled_pages-0-id": "", - "bundled_pages-0-page": str(self.analysis_page.id), - "bundled_pages-0-ORDER": "0", - }, + self.edit_url, + nested_form_data( + { + "name": "Updated Bundle", + "status": self.bundle.status, + "bundled_pages": inline_formset([{"page": self.analysis_page.id}]), + } + ), ) self.assertEqual(response.status_code, 302) self.bundle.refresh_from_db() self.assertEqual(self.bundle.name, "Updated Bundle") + def test_bundle_edit_view__redirects_to_index_for_released_bundles(self): + """Released bundles should no longer be editable.""" + response = self.client.get(self.released_bundle_edit_url) + self.assertRedirects(response, self.bundle_index_url) + + def test_bundle_edit_view__updates_approved_fields_on_save_and_approve(self): + """Checks the fields are populated if the user clicks the 'Save and approve' button.""" + self.client.force_login(self.superuser) + self.client.post( + self.edit_url, + nested_form_data( + { + "name": "Updated Bundle", + "status": self.bundle.status, # correct. "save and approve" should update the status directly + "bundled_pages": inline_formset([]), + "action-save-and-approve": "save-and-approve", + } + ), + ) + self.bundle.refresh_from_db() + self.assertEqual(self.bundle.status, BundleStatus.APPROVED) + self.assertIsNotNone(self.bundle.approved_at) + self.assertEqual(self.bundle.approved_by, self.superuser) + @mock.patch("cms.bundles.viewsets.notify_slack_of_status_change") def test_bundle_approval__happy_path(self, mock_notify_slack): """Test bundle approval workflow.""" @@ -191,3 +216,14 @@ def test_bundle_approval__cannot__self_approve(self, mock_notify_slack): self.assertIsNone(self.bundle.approved_by) self.assertFalse(mock_notify_slack.called) + + def test_index_view(self): + """Checks the content of the index page.""" + response = self.client.get(self.bundle_index_url) + self.assertContains(response, self.edit_url) + self.assertContains(response, self.approved_bundle_edit_url) + self.assertNotContains(response, self.released_bundle_edit_url) + + self.assertContains(response, "Pending", 2) # status + status filter + self.assertContains(response, "Released", 2) # status + status filter + self.assertContains(response, "Approved", 5) # status + status filter, approved at/by diff --git a/cms/bundles/tests/test_wagtail_hooks.py b/cms/bundles/tests/test_wagtail_hooks.py new file mode 100644 index 00000000..25d908c6 --- /dev/null +++ b/cms/bundles/tests/test_wagtail_hooks.py @@ -0,0 +1,183 @@ +from datetime import timedelta + +from django.test import TestCase +from django.urls import reverse +from django.utils import timezone +from wagtail.test.utils.wagtail_tests import WagtailTestUtils + +from cms.analysis.tests.factories import AnalysisPageFactory +from cms.bundles.models import Bundle +from cms.bundles.tests.factories import BundleFactory, BundlePageFactory +from cms.bundles.tests.utils import grant_all_bundle_permissions, grant_all_page_permissions, make_bundle_viewer +from cms.bundles.wagtail_hooks import LatestBundlesPanel +from cms.release_calendar.tests.factories import ReleaseCalendarPageFactory +from cms.users.tests.factories import GroupFactory, UserFactory + + +class WagtailHooksTestCase(WagtailTestUtils, TestCase): + @classmethod + def setUpTestData(cls): + cls.superuser = cls.create_superuser(username="admin") + + cls.publishing_group = GroupFactory(name="Publishing Officers", access_admin=True) + grant_all_page_permissions(cls.publishing_group) + grant_all_bundle_permissions(cls.publishing_group) + cls.publishing_officer = UserFactory(username="publishing_officer") + cls.publishing_officer.groups.add(cls.publishing_group) + + cls.bundle_viewer = UserFactory(username="bundle.viewer", access_admin=True) + make_bundle_viewer(cls.bundle_viewer) + + # a regular generic_user that can only access the Wagtail admin + cls.generic_user = UserFactory(username="generic.generic_user", access_admin=True) + + cls.bundle_index_url = reverse("bundle:index") + cls.bundle_add_url = reverse("bundle:add") + cls.dashboard_url = reverse("wagtailadmin_home") + + cls.pending_bundle = BundleFactory(name="Pending Bundle", created_by=cls.bundle_viewer) + cls.in_review_bundle = BundleFactory(in_review=True, name="Bundle In review", created_by=cls.superuser) + cls.approved_bundle = BundleFactory(name="Approved Bundle", approved=True, created_by=cls.publishing_officer) + cls.released_bundle = BundleFactory(released=True, name="Released Bundle") + + cls.analysis_page = AnalysisPageFactory(title="November 2024", parent__title="PSF") + cls.analysis_edit_url = reverse("wagtailadmin_pages:edit", args=[cls.analysis_page.id]) + cls.analysis_parent_url = reverse("wagtailadmin_explore", args=[cls.analysis_page.get_parent().id]) + cls.add_to_bundle_url = reverse("bundles:add_to_bundle", args=[cls.analysis_page.id]) + + def test_latest_bundles_panel_is_shown(self): + """Checks that the latest bundles dashboard panel is shown to relevant users.""" + cases = [ + (self.generic_user, 0), + (self.bundle_viewer, 1), + (self.publishing_officer, 1), + (self.superuser, 1), + ] + for user, shown in cases: + with self.subTest(user=user.username, shown=shown): + self.client.force_login(user) + response = self.client.get(self.dashboard_url) + self.assertContains(response, "Latest active bundles", count=shown) + + def test_latest_bundles_panel_content(self): + """Checks that the latest bundles dashboard panel content only shows active bundles.""" + self.client.force_login(self.publishing_officer) + response = self.client.get(self.dashboard_url) + + panels = response.context["panels"] + self.assertIsInstance(panels[0], LatestBundlesPanel) + self.assertIs(panels[0].permission_policy.model, Bundle) + + self.assertContains(response, self.bundle_add_url) + self.assertContains(response, self.bundle_index_url) + self.assertContains(response, "View all bundles") + + for bundle in [self.pending_bundle, self.in_review_bundle, self.approved_bundle]: + self.assertContains(response, bundle.name) + self.assertContains(response, bundle.created_by.get_full_name()) + self.assertContains(response, bundle.status.label) + self.assertNotContains(response, self.released_bundle.status.label) + + def test_preset_golive_date__happy_path(self): + """Checks we update the page go live at on page edit , if in the future and doesn't match the bundle date.""" + self.client.force_login(self.publishing_officer) + + BundlePageFactory(parent=self.pending_bundle, page=self.analysis_page) + + # set to +15 minutes as the check is on now() < scheduled_publication_date & page.go_live_at != scheduled + nowish = timezone.now() + timedelta(minutes=15) + bundle_date = nowish + timedelta(hours=1) + + cases = [ + # bundle publication date, page go_live_at, expected change, case description + (nowish - timedelta(hours=1), nowish, nowish, "Go live unchanged as bundle date in the past"), + (bundle_date, bundle_date, bundle_date, "Go live unchanged as it matches bundle"), + (bundle_date, nowish + timedelta(days=1), bundle_date, "Go live updated to match bundle"), + ] + for bundle_publication_date, go_live_at, expected, case in cases: + with self.subTest(go_live_at=go_live_at, expected=expected, case=case): + self.pending_bundle.publication_date = bundle_publication_date + self.pending_bundle.save(update_fields=["publication_date"]) + + self.analysis_page.go_live_at = go_live_at + self.analysis_page.save(update_fields=["go_live_at"]) + + response = self.client.get(self.analysis_edit_url) + context_page = response.context["page"] + self.assertEqual(context_page.go_live_at, expected) + + def test_preset_golive_date__updates_only_if_page_in_active_bundle(self): + """Checks the go live at update only happens if the page is in active bundle.""" + self.client.force_login(self.publishing_officer) + + nowish = timezone.now() + timedelta(minutes=15) + self.pending_bundle.publication_date = nowish + timedelta(hours=1) + self.pending_bundle.save(update_fields=["publication_date"]) + + self.analysis_page.go_live_at = nowish + self.analysis_page.save(update_fields=["go_live_at"]) + + response = self.client.get(self.analysis_edit_url) + context_page = response.context["page"] + self.assertEqual(context_page.go_live_at, nowish) + + def test_preset_golive_date__updates_only_if_page_is_bundleable(self): + """Checks the go live at change happens only for bundleable pages..""" + self.client.force_login(self.publishing_officer) + + nowish = timezone.now() + timedelta(minutes=15) + self.pending_bundle.publication_date = nowish + timedelta(hours=1) + self.pending_bundle.save(update_fields=["publication_date"]) + + page = ReleaseCalendarPageFactory() + page.go_live_at = nowish + page.save(update_fields=["go_live_at"]) + + response = self.client.get(reverse("wagtailadmin_pages:edit", args=[page.id])) + context_page = response.context["page"] + self.assertEqual(context_page.go_live_at, nowish) + + def test_add_to_bundle_buttons(self): + """Tests that the 'Add to Bundle' button appears in appropriate contexts.""" + # Test both header and listing contexts + contexts = [(self.analysis_edit_url, "header"), (self.analysis_parent_url, "listing")] + + for user in [self.generic_user, self.bundle_viewer]: + for url, context in contexts: + with self.subTest(user=user.username, context=context): + self.client.force_login(user) + response = self.client.get(url, follow=True) + self.assertEqual(response.context["request"].path, reverse("wagtailadmin_home")) + self.assertContains(response, "Sorry, you do not have permission to access this area.") + + cases_with_access = [ + (self.publishing_officer, 1), # Has all permissions, should see button + (self.superuser, 1), # Has all permissions, should see button + ] + for user, expected_count in cases_with_access: + for url, context in contexts: + with self.subTest(user=user.username, context=context, expected=expected_count): + self.client.force_login(user) + response = self.client.get(url) + + # Check if button appears in response + self.assertContains(response, "Add to Bundle", count=expected_count) + self.assertContains(response, "boxes-stacked") # icon name + self.assertContains(response, self.add_to_bundle_url) + + def test_add_to_bundle_buttons__doesnt_show_for_pages_in_bundle(self): + """Checks that the button doesn't appear for pages already in a bundle.""" + release_calendar_page = ReleaseCalendarPageFactory() + contexts = [ + (reverse("wagtailadmin_pages:edit", args=[release_calendar_page.id]), "header"), + (reverse("wagtailadmin_explore", args=[release_calendar_page.get_parent().id]), "listing"), + ] + BundlePageFactory(parent=self.pending_bundle, page=self.analysis_page) + + self.client.force_login(self.publishing_officer) + + for url, context in contexts: + with self.subTest(msg=f"Non-bundleable page - {context}"): + response = self.client.get(url) + self.assertNotContains(response, "Add to Bundle") + self.assertNotContains(response, self.add_to_bundle_url) diff --git a/cms/bundles/tests/utils.py b/cms/bundles/tests/utils.py index 1450d5bc..08baed0b 100644 --- a/cms/bundles/tests/utils.py +++ b/cms/bundles/tests/utils.py @@ -2,6 +2,9 @@ from django.contrib.auth.models import Permission from django.db.models import QuerySet +from wagtail.models import GroupPagePermission + +from cms.home.models import HomePage if TYPE_CHECKING: from django.contrib.auth.models import Group @@ -44,3 +47,10 @@ def grant_all_bundle_permissions(group: "Group") -> None: def grant_view_bundle_permissions(group: "Group") -> None: """Adds the view bundle permission to the given group.""" group.permissions.add(get_view_bundle_permission()) + + +def grant_all_page_permissions(group: "Group") -> None: + """Adds all the page permissions to the given group.""" + home = HomePage.objects.first() + for permission_type in ["add", "change", "delete", "view"]: + GroupPagePermission.objects.create(group=group, page=home, permission_type=permission_type) diff --git a/cms/bundles/wagtail_hooks.py b/cms/bundles/wagtail_hooks.py index dd74190b..7ce2e8b3 100644 --- a/cms/bundles/wagtail_hooks.py +++ b/cms/bundles/wagtail_hooks.py @@ -134,7 +134,7 @@ def __init__(self, request: "HttpRequest") -> None: def is_shown(self) -> bool: """Determine if the panel is shown based on whether the user can modify it.""" has_permission: bool = self.permission_policy.user_has_any_permission( - self.request.user, {"add", "change", "delete"} + self.request.user, {"add", "change", "delete", "view"} ) return has_permission diff --git a/cms/settings/test.py b/cms/settings/test.py index 3f227d55..96ae94ea 100644 --- a/cms/settings/test.py +++ b/cms/settings/test.py @@ -40,3 +40,7 @@ DEFENDER_DISABLE_USERNAME_LOCKOUT = True DEFENDER_DISABLE_IP_LOCKOUT = True + + +# Silence Slack notifications by default +SLACK_NOTIFICATIONS_WEBHOOK_URL = None From c576e8547c624a28a32078c22f4a3914a9e191d0 Mon Sep 17 00:00:00 2001 From: zerolab Date: Thu, 21 Nov 2024 14:42:09 +0000 Subject: [PATCH 06/13] Tidy up --- cms/analysis/models.py | 1 + cms/bundles/tests/test_management_commands.py | 6 +++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/cms/analysis/models.py b/cms/analysis/models.py index 4ead55a5..0fc597ab 100644 --- a/cms/analysis/models.py +++ b/cms/analysis/models.py @@ -125,6 +125,7 @@ class AnalysisPage(BundledPageMixin, BasePage): # type: ignore[django-manager-m show_cite_this_page = models.BooleanField(default=True) content_panels: ClassVar[list["Panel"]] = [ + *BundledPageMixin.panels, MultiFieldPanel( [ TitleFieldPanel("title", help_text=_("Also known as the release edition. e.g. 'November 2024'.")), diff --git a/cms/bundles/tests/test_management_commands.py b/cms/bundles/tests/test_management_commands.py index a5fb3628..24845291 100644 --- a/cms/bundles/tests/test_management_commands.py +++ b/cms/bundles/tests/test_management_commands.py @@ -4,6 +4,7 @@ from django.core.management import call_command from django.test import TestCase, override_settings +from django.urls import reverse from django.utils import timezone from wagtail.models import ModelLogEntry, PageLogEntry @@ -142,7 +143,10 @@ def test_publish_bundle_with_base_url(self, mock_notify): # Verify notification was called with correct URL mock_notify.assert_called_once() call_kwargs = mock_notify.call_args[1] - self.assertTrue(call_kwargs["url"].startswith("https://test.ons.gov.uk")) + + self.assertEqual( + call_kwargs["url"], "https://test.ons.gov.uk" + reverse("bundle:inspect", args=(self.bundle.pk,)) + ) self.assertIn(str(self.bundle.pk), call_kwargs["url"]) From a3ebb5dfc77f77929bea4bc58abf5e67bc510b90 Mon Sep 17 00:00:00 2001 From: zerolab Date: Thu, 21 Nov 2024 18:09:47 +0000 Subject: [PATCH 07/13] Tweaks based on feedback --- cms/bundles/models.py | 22 ++++++++---------- cms/bundles/panels.py | 8 +++++-- cms/bundles/tests/test_forms.py | 11 ++++----- cms/bundles/tests/test_models.py | 2 ++ cms/bundles/tests/test_notifications.py | 18 +++++---------- cms/bundles/views.py | 6 +++-- cms/bundles/wagtail_hooks.py | 8 +++---- cms/release_calendar/tests/test_forms.py | 29 +++++++++++++----------- 8 files changed, 52 insertions(+), 52 deletions(-) diff --git a/cms/bundles/models.py b/cms/bundles/models.py index b8a20f24..8f172d2b 100644 --- a/cms/bundles/models.py +++ b/cms/bundles/models.py @@ -53,10 +53,10 @@ class BundleManager(models.Manager.from_queryset(BundlesQuerySet)): # type: ign def get_queryset(self) -> BundlesQuerySet: """Augments the queryset to order it by the publication date, then name, then reverse id.""" queryset: BundlesQuerySet = super().get_queryset() - queryset = queryset.annotate( + queryset = queryset.alias( release_date=Coalesce("publication_date", "release_calendar_page__release_date") ).order_by(F("release_date").desc(nulls_last=True), "name", "-pk") - return queryset + return queryset # note: not returning directly to placate no-any-return class Bundle(index.Indexed, ClusterableModel, models.Model): # type: ignore[django-manager-missing] @@ -71,7 +71,7 @@ class Bundle(index.Indexed, ClusterableModel, models.Model): # type: ignore[dja on_delete=models.SET_NULL, related_name="bundles", ) - # @see https://docs.wagtail.org/en/stable/advanced_topics/reference_index.html + # See https://docs.wagtail.org/en/stable/advanced_topics/reference_index.html created_by.wagtail_reference_index_ignore = True # type: ignore[attr-defined] approved_at = models.DateTimeField(blank=True, null=True) @@ -131,7 +131,8 @@ def scheduled_publication_date(self) -> Optional["datetime.datetime"]: @property def can_be_approved(self) -> bool: - """Determines + """Determines whether the bundle can be approved (i.e. is not already approved or released). + Note: strictly speaking, the bundle should in "in review" in order for it to be approved. """ return self.status in [BundleStatus.PENDING, BundleStatus.IN_REVIEW] @@ -188,14 +189,11 @@ def active_bundles(self) -> QuerySet[Bundle]: return self.bundles.filter(status__in=ACTIVE_BUNDLE_STATUSES) @cached_property - def in_active_bundle(self) -> bool: - """Determines whether this instance is in an active bundle (that is not yet released).""" - exists: bool = self.bundlepage_set.filter( # type: ignore[attr-defined] - parent__status__in=ACTIVE_BUNDLE_STATUSES - ).exists() - return exists - - @property def active_bundle(self) -> Bundle | None: """Helper to return the active bundle this instance is in.""" return self.active_bundles.first() + + @cached_property + def in_active_bundle(self) -> bool: + """Determines whether this instance is in an active bundle (that is not yet released).""" + return self.active_bundle is not None diff --git a/cms/bundles/panels.py b/cms/bundles/panels.py index 3450398e..8810c6b3 100644 --- a/cms/bundles/panels.py +++ b/cms/bundles/panels.py @@ -1,6 +1,8 @@ from typing import TYPE_CHECKING, Any, Union from django.utils.html import format_html, format_html_join +from django.utils.safestring import mark_safe +from django.utils.translation import gettext as _ from wagtail.admin.panels import HelpPanel, PageChooserPanel if TYPE_CHECKING: @@ -33,9 +35,11 @@ def _content_for_instance(self, instance: "Model") -> Union[str, "SafeString"]: ), ) - content = format_html("

    This page is in the following bundle(s):

      {}
    ", content_html) + content = format_html( + "

    {}

      {}
    ", _("This page is in the following bundle(s):"), content_html + ) else: - content = format_html("

    This page is not part of any bundles

    ") + content = mark_safe(f"

    {_('This page is not part of any bundles.')}

    ") # noqa: S308 return content diff --git a/cms/bundles/tests/test_forms.py b/cms/bundles/tests/test_forms.py index 92f1459a..cfa491e2 100644 --- a/cms/bundles/tests/test_forms.py +++ b/cms/bundles/tests/test_forms.py @@ -38,16 +38,15 @@ def test_form_clean__validates_page_not_in_bundle(self): form = AddToBundleForm(page_to_add=self.page, data={"bundle": self.bundle.pk}) self.assertFalse(form.is_valid()) - self.assertDictEqual( - form.errors, - {"bundle": [f"Page '{self.page.get_admin_display_title()}' is already in bundle 'First Bundle'"]}, + self.assertFormError( + form, "bundle", [f"Page '{self.page.get_admin_display_title()}' is already in bundle 'First Bundle'"] ) def test_form_clean__validates_page_is_bundleable(self): """Checks the given page inherits from BundlePageMixin.""" form = AddToBundleForm(page_to_add=ReleaseCalendarPageFactory(), data={"bundle": self.bundle.pk}) self.assertFalse(form.is_valid()) - self.assertDictEqual(form.errors, {"__all__": ["Pages of this type cannot be added."]}) + self.assertFormError(form, None, ["Pages of this type cannot be added."]) class BundleAdminFormTestCase(TestCase): @@ -125,9 +124,7 @@ def test_clean__validates_added_page_not_in_another_bundle(self): form = self.form_class(instance=self.bundle, data=data) self.assertFalse(form.is_valid()) - self.assertDictEqual( - form.errors, {"__all__": ["'The Analysis' is already in an active bundle (Another Bundle)"]} - ) + self.assertFormError(form, None, ["'The Analysis' is already in an active bundle (Another Bundle)"]) def test_clean__sets_approved_by_and_approved_at(self): """Checks that Bundle gets the approver and approval time set.""" diff --git a/cms/bundles/tests/test_models.py b/cms/bundles/tests/test_models.py index 8fbfa782..b026c3bd 100644 --- a/cms/bundles/tests/test_models.py +++ b/cms/bundles/tests/test_models.py @@ -117,6 +117,7 @@ def test_in_active_bundle_property(self): self.bundle.save(update_fields=["status"]) del self.page.in_active_bundle # clear the cached property + del self.page.active_bundle # clear the cached property self.assertFalse(self.page.in_active_bundle) def test_active_bundle_property(self): @@ -126,4 +127,5 @@ def test_active_bundle_property(self): self.bundle.status = BundleStatus.RELEASED self.bundle.save(update_fields=["status"]) + del self.page.active_bundle # cleared cached property self.assertIsNone(self.page.active_bundle) diff --git a/cms/bundles/tests/test_notifications.py b/cms/bundles/tests/test_notifications.py index 92b91044..d70f19bc 100644 --- a/cms/bundles/tests/test_notifications.py +++ b/cms/bundles/tests/test_notifications.py @@ -62,7 +62,7 @@ def test_notify_slack_of_status_change__no_webhook_url(self, mock_client): @patch("cms.bundles.notifications.WebhookClient") def test_notify_slack_of_status_change__error_logging(self, mock_client): """Should log error if Slack request fails.""" - with patch("cms.bundles.notifications.logger") as mock_logger: + with self.assertLogs("cms.bundles") as logs_recorder: self.mock_response.status_code = HTTPStatus.BAD_REQUEST self.mock_response.body = "Error message" mock_client.return_value.send.return_value = self.mock_response @@ -81,9 +81,7 @@ def test_notify_slack_of_status_change__error_logging(self, mock_client): ], ) - mock_logger.error.assert_called_once_with( - "Unable to notify Slack of bundle status change: %s", "Error message" - ) + self.assertIn("Unable to notify Slack of bundle status change: Error message", logs_recorder.output[0]) @override_settings(SLACK_NOTIFICATIONS_WEBHOOK_URL="https://slack.ons.gov.uk") @patch("cms.bundles.notifications.WebhookClient") @@ -117,16 +115,14 @@ def test_notify_slack_of_publication_start__no_webhook_url(self): @patch("cms.bundles.notifications.WebhookClient") def test_notify_slack_of_publish_start__error_logging(self, mock_client): """Should log error if Slack request fails.""" - with patch("cms.bundles.notifications.logger") as mock_logger: + with self.assertLogs("cms.bundles") as logs_recorder: self.mock_response.status_code = HTTPStatus.BAD_REQUEST self.mock_response.body = "Error message" mock_client.return_value.send.return_value = self.mock_response notify_slack_of_publication_start(self.bundle, self.user) - mock_logger.error.assert_called_once_with( - "Unable to notify Slack of bundle publication start: %s", "Error message" - ) + self.assertIn("Unable to notify Slack of bundle publication start: Error message", logs_recorder.output[0]) @override_settings(SLACK_NOTIFICATIONS_WEBHOOK_URL="https://slack.ons.gov.uk") @patch("cms.bundles.notifications.WebhookClient") @@ -164,13 +160,11 @@ def test_notify_slack_of_publish_end__no_webhook_url(self): @patch("cms.bundles.notifications.WebhookClient") def test_notify_slack_of_publish_end__error_logging(self, mock_client): """Should log error if Slack request fails.""" - with patch("cms.bundles.notifications.logger") as mock_logger: + with self.assertLogs("cms.bundles") as logs_recorder: self.mock_response.status_code = HTTPStatus.BAD_REQUEST self.mock_response.body = "Error message" mock_client.return_value.send.return_value = self.mock_response notify_slack_of_publish_end(self.bundle, 1.234, self.user, self.inspect_url) - mock_logger.error.assert_called_once_with( - "Unable to notify Slack of bundle publication finish: %s", "Error message" - ) + self.assertIn("Unable to notify Slack of bundle publication finish: Error message", logs_recorder.output[0]) diff --git a/cms/bundles/views.py b/cms/bundles/views.py index 604084fa..98d9e4e4 100644 --- a/cms/bundles/views.py +++ b/cms/bundles/views.py @@ -5,6 +5,7 @@ from django.shortcuts import get_object_or_404, redirect from django.urls import reverse from django.utils.http import url_has_allowed_host_and_scheme +from django.utils.text import get_text_list from django.utils.translation import gettext as _ from django.views.generic import FormView from wagtail.admin import messages @@ -48,13 +49,14 @@ def dispatch(self, request: "HttpRequest", *args: Any, **kwargs: Any) -> "HttpRe self.goto_next = redirect_to if self.page_to_add.in_active_bundle: - bundles = ", ".join(list(self.page_to_add.active_bundles.values_list("name", flat=True))) messages.warning( request, _("Page %(title)s is already in a bundle ('%(bundles)s')") % { "title": self.page_to_add.get_admin_display_title(), # type: ignore[attr-defined] - "bundles": bundles, + "bundles": get_text_list( + list(self.page_to_add.active_bundles.values_list("name", flat=True)), last_word="and" + ), }, ) if self.goto_next: diff --git a/cms/bundles/wagtail_hooks.py b/cms/bundles/wagtail_hooks.py index 7ce2e8b3..12d9a7db 100644 --- a/cms/bundles/wagtail_hooks.py +++ b/cms/bundles/wagtail_hooks.py @@ -106,17 +106,17 @@ def preset_golive_date(request: "HttpRequest", page: "Page") -> None: if not page.in_active_bundle: return - scheduled_publication_date = page.active_bundle.scheduled_publication_date # type: ignore[union-attr] + scheduled_date = page.active_bundle.scheduled_publication_date # type: ignore[union-attr] # note: ignoring union-attr because we already check that the page is in an active bundle. - if not scheduled_publication_date: + if not scheduled_date: return # note: ignoring # - attr-defined because mypy thinks page is only a BundledPageMixin class, rather than Page and BundledPageMixin. # - union-attr because active_bundle can be none, but we check for that above - if now() < scheduled_publication_date != page.go_live_at: # type: ignore[attr-defined] + if now() < scheduled_date and scheduled_date != page.go_live_at: # type: ignore[attr-defined] # pre-set the scheduled publishing time - page.go_live_at = scheduled_publication_date # type: ignore[attr-defined] + page.go_live_at = scheduled_date # type: ignore[attr-defined] class LatestBundlesPanel(Component): diff --git a/cms/release_calendar/tests/test_forms.py b/cms/release_calendar/tests/test_forms.py index 63a967a5..5283db24 100644 --- a/cms/release_calendar/tests/test_forms.py +++ b/cms/release_calendar/tests/test_forms.py @@ -75,7 +75,7 @@ def test_form_clean__validates_notice(self): form = self.form_class(instance=self.page, data=data) self.assertFalse(form.is_valid()) - self.assertListEqual(form.errors["notice"], ["The notice field is required when the release is cancelled"]) + self.assertFormError(form, "notice", ["The notice field is required when the release is cancelled"]) def test_form_clean__validates_release_date_when_confirmed(self): """Validates that the release date must be set if the release is confirmed.""" @@ -95,8 +95,9 @@ def test_form_clean__validates_release_date_when_confirmed(self): self.assertEqual(form.is_valid(), is_valid) if not is_valid: - self.assertListEqual( - form.errors["release_date"], + self.assertFormError( + form, + "release_date", ["The release date field is required when the release is confirmed"], ) @@ -118,8 +119,9 @@ def test_form_clean__validates_release_date_text(self): self.assertEqual(form.is_valid(), is_valid) if not is_valid: - self.assertListEqual( - form.errors["release_date_text"], + self.assertFormError( + form, + "release_date_text", ["The release date text must be in the 'Month YYYY' or 'Month YYYY to Month YYYY' format."], ) @@ -130,7 +132,7 @@ def test_form_clean__validates_release_date_text_start_end_dates(self): form = self.form_class(instance=self.page, data=data) self.assertFalse(form.is_valid()) - self.assertListEqual(form.errors["release_date_text"], ["The end month must be after the start month."]) + self.assertFormError(form, "release_date_text", ["The end month must be after the start month."]) def test_form_clean__can_add_release_date_when_confirming(self): """Checks that we can set a new release date when the release is confirmed, if previously it was empty.""" @@ -157,8 +159,9 @@ def test_form_clean__validates_changes_to_release_date_must_be_filled(self): form = self.form_class(instance=self.page, data=data) self.assertFalse(form.is_valid()) - self.assertListEqual( - form.errors["changes_to_release_date"], + self.assertFormError( + form, + "changes_to_release_date", [ "If a confirmed calendar entry needs to be rescheduled, " "the 'Changes to release date' field must be filled out." @@ -192,8 +195,8 @@ def test_form_clean__validates_either_release_date_or_text(self): self.assertFalse(form.is_valid()) message = ["Please enter the release date or the release date text, not both."] - self.assertListEqual(form.errors["release_date"], message) - self.assertListEqual(form.errors["release_date_text"], message) + self.assertFormError(form, "release_date", message) + self.assertFormError(form, "release_date_text", message) def test_form_clean__validates_either_next_release_date_or_text(self): """Checks that editors can enter either the next release date or the text, not both.""" @@ -206,8 +209,8 @@ def test_form_clean__validates_either_next_release_date_or_text(self): self.assertFalse(form.is_valid()) message = ["Please enter the next release date or the next release text, not both."] - self.assertListEqual(form.errors["next_release_date"], message) - self.assertListEqual(form.errors["next_release_text"], message) + self.assertFormError(form, "next_release_date", message) + self.assertFormError(form, "next_release_text", message) def test_form_clean__validates_next_release_date_is_after_release_date(self): """Checks that editors enter a release that that is after the release date.""" @@ -219,4 +222,4 @@ def test_form_clean__validates_next_release_date_is_after_release_date(self): self.assertFalse(form.is_valid()) message = ["The next release date must be after the release date."] - self.assertListEqual(form.errors["next_release_date"], message) + self.assertFormError(form, "next_release_date", message) From 465b1c437f57bcb3b3b26140ab43282262f28a2e Mon Sep 17 00:00:00 2001 From: zerolab Date: Fri, 22 Nov 2024 13:05:21 +0000 Subject: [PATCH 08/13] Back to format_html --- cms/bundles/panels.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cms/bundles/panels.py b/cms/bundles/panels.py index 8810c6b3..bac515e8 100644 --- a/cms/bundles/panels.py +++ b/cms/bundles/panels.py @@ -1,7 +1,6 @@ from typing import TYPE_CHECKING, Any, Union from django.utils.html import format_html, format_html_join -from django.utils.safestring import mark_safe from django.utils.translation import gettext as _ from wagtail.admin.panels import HelpPanel, PageChooserPanel @@ -39,7 +38,7 @@ def _content_for_instance(self, instance: "Model") -> Union[str, "SafeString"]: "

    {}

      {}
    ", _("This page is in the following bundle(s):"), content_html ) else: - content = mark_safe(f"

    {_('This page is not part of any bundles.')}

    ") # noqa: S308 + content = format_html("

    {}

    ", _("This page is not part of any bundles.")) return content From 5b3783eb4b2c518fdba9375d95709fd32b926673 Mon Sep 17 00:00:00 2001 From: zerolab Date: Mon, 2 Dec 2024 10:51:48 +0000 Subject: [PATCH 09/13] Disable `missing-function-docstring` pylint rule globally --- .pylintrc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.pylintrc b/.pylintrc index ec09010b..aa563177 100644 --- a/.pylintrc +++ b/.pylintrc @@ -437,6 +437,7 @@ disable=raw-checker-failed, too-many-ancestors, too-few-public-methods, missing-class-docstring, + missing-function-docstring, fixme # note: @@ -445,6 +446,7 @@ disable=raw-checker-failed, # - too-many-ancestors: because of our and Wagtail's use of mixins # - too-few-public-methods: because of Django Meta classes # - missing-class-docstring: mostly because of Django classes. +# - missing-function-docstring: because we can't disable it for trivial functions. # - fixme: because we want to leave TODO notes for future features. # Enable the message, report, category or checker with the given id(s). You can From 4e87b796138336012a405c70a6f9c6d062688d24 Mon Sep 17 00:00:00 2001 From: zerolab Date: Mon, 2 Dec 2024 10:52:50 +0000 Subject: [PATCH 10/13] Make bundle name unique and validate release calendar page or pub date, not both --- cms/bundles/forms.py | 5 +++++ cms/bundles/migrations/0001_initial.py | 2 +- cms/bundles/models.py | 5 +---- cms/bundles/tests/test_forms.py | 14 ++++++++++++++ 4 files changed, 21 insertions(+), 5 deletions(-) diff --git a/cms/bundles/forms.py b/cms/bundles/forms.py index 62f80249..e3b6055a 100644 --- a/cms/bundles/forms.py +++ b/cms/bundles/forms.py @@ -89,4 +89,9 @@ def clean(self) -> dict[str, Any] | None: cleaned_data["approved_at"] = None cleaned_data["approved_by"] = None + if self.cleaned_data["release_calendar_page"] and self.cleaned_data["publication_date"]: + error = _("You must choose either a Release Calendar page or a Publication date, not both.") + self.add_error("release_calendar_page", error) + self.add_error("publication_date", error) + return cleaned_data diff --git a/cms/bundles/migrations/0001_initial.py b/cms/bundles/migrations/0001_initial.py index ed323099..2ec91320 100644 --- a/cms/bundles/migrations/0001_initial.py +++ b/cms/bundles/migrations/0001_initial.py @@ -21,7 +21,7 @@ class Migration(migrations.Migration): name="Bundle", fields=[ ("id", models.AutoField(auto_created=True, primary_key=True, serialize=False)), - ("name", models.CharField(max_length=255)), + ("name", models.CharField(max_length=255, unique=True)), ("created_at", models.DateTimeField(auto_now_add=True)), ("approved_at", models.DateTimeField(blank=True, null=True)), ("publication_date", models.DateTimeField(blank=True, null=True)), diff --git a/cms/bundles/models.py b/cms/bundles/models.py index 8f172d2b..467054ca 100644 --- a/cms/bundles/models.py +++ b/cms/bundles/models.py @@ -62,7 +62,7 @@ def get_queryset(self) -> BundlesQuerySet: class Bundle(index.Indexed, ClusterableModel, models.Model): # type: ignore[django-manager-missing] base_form_class = BundleAdminForm - name = models.CharField(max_length=255) + name = models.CharField(max_length=255, unique=True) created_at = models.DateTimeField(auto_now_add=True) created_by = models.ForeignKey( "users.User", @@ -138,7 +138,6 @@ def can_be_approved(self) -> bool: return self.status in [BundleStatus.PENDING, BundleStatus.IN_REVIEW] def get_bundled_pages(self) -> QuerySet[Page]: - """Convenience method to return all bundled pages.""" pages: QuerySet[Page] = Page.objects.filter(pk__in=self.bundled_pages.values_list("page__pk", flat=True)) return pages @@ -190,10 +189,8 @@ def active_bundles(self) -> QuerySet[Bundle]: @cached_property def active_bundle(self) -> Bundle | None: - """Helper to return the active bundle this instance is in.""" return self.active_bundles.first() @cached_property def in_active_bundle(self) -> bool: - """Determines whether this instance is in an active bundle (that is not yet released).""" return self.active_bundle is not None diff --git a/cms/bundles/tests/test_forms.py b/cms/bundles/tests/test_forms.py index cfa491e2..7e77d272 100644 --- a/cms/bundles/tests/test_forms.py +++ b/cms/bundles/tests/test_forms.py @@ -2,6 +2,7 @@ from django import forms from django.test import TestCase +from django.utils import timezone from wagtail.admin.panels import get_edit_handler from wagtail.test.utils.form_data import inline_formset, nested_form_data @@ -136,3 +137,16 @@ def test_clean__sets_approved_by_and_approved_at(self): self.assertTrue(form.is_valid()) self.assertEqual(form.cleaned_data["approved_by"], approver) + + def test_clean__validates_release_calendar_page_or_publication_date(self): + release_calendar_page = ReleaseCalendarPageFactory() + data = self.form_data + data["release_calendar_page"] = release_calendar_page.id + data["publication_date"] = timezone.now() + + form = self.form_class(data=data) + + self.assertFalse(form.is_valid()) + error = "You must choose either a Release Calendar page or a Publication date, not both." + self.assertFormError(form, "release_calendar_page", [error]) + self.assertFormError(form, "publication_date", [error]) From 5c4c7dba4f2f571056c80dc1bccc332291c8b6f2 Mon Sep 17 00:00:00 2001 From: zerolab Date: Mon, 2 Dec 2024 11:09:08 +0000 Subject: [PATCH 11/13] Further tweaks based on code review --- .../publish_scheduled_without_bundles.py | 5 +- cms/bundles/models.py | 2 +- cms/bundles/viewsets.py | 66 ++++++++++--------- 3 files changed, 40 insertions(+), 33 deletions(-) diff --git a/cms/bundles/management/commands/publish_scheduled_without_bundles.py b/cms/bundles/management/commands/publish_scheduled_without_bundles.py index ed44c986..26e150c5 100644 --- a/cms/bundles/management/commands/publish_scheduled_without_bundles.py +++ b/cms/bundles/management/commands/publish_scheduled_without_bundles.py @@ -12,7 +12,10 @@ class Command(BaseCommand): - """A copy of Wagtail's publish_scheduled management command that excludes bundled objects.""" + """A copy of Wagtail's publish_scheduled management command that excludes bundled objects. + + @see https://github.com/wagtail/wagtail/blob/main/wagtail/management/commands/publish_scheduled.py + """ def add_arguments(self, parser: "CommandParser") -> None: parser.add_argument( diff --git a/cms/bundles/models.py b/cms/bundles/models.py index 467054ca..425fde08 100644 --- a/cms/bundles/models.py +++ b/cms/bundles/models.py @@ -133,7 +133,7 @@ def scheduled_publication_date(self) -> Optional["datetime.datetime"]: def can_be_approved(self) -> bool: """Determines whether the bundle can be approved (i.e. is not already approved or released). - Note: strictly speaking, the bundle should in "in review" in order for it to be approved. + Note: strictly speaking, the bundle should be in "in review" in order for it to be approved. """ return self.status in [BundleStatus.PENDING, BundleStatus.IN_REVIEW] diff --git a/cms/bundles/viewsets.py b/cms/bundles/viewsets.py index e49e1483..8da0ce60 100644 --- a/cms/bundles/viewsets.py +++ b/cms/bundles/viewsets.py @@ -70,35 +70,39 @@ def save_instance(self) -> Bundle: instance: Bundle = self.form.save() self.has_content_changes = self.form.has_changed() - if self.has_content_changes: - log(action="wagtail.edit", instance=instance, content_changed=True, data={"fields": self.form.changed_data}) - - if "status" in self.form.changed_data: - kwargs: dict = {"content_changed": self.has_content_changes} - original_status = BundleStatus[self.form.original_status].label - url = self.request.build_absolute_uri(reverse("bundle:inspect", args=(instance.pk,))) - - if instance.status == BundleStatus.APPROVED: - action = "bundles.approve" - kwargs["data"] = {"old": original_status} - notify_slack_of_status_change(instance, original_status, user=self.request.user, url=url) - elif instance.status == BundleStatus.RELEASED.value: - action = "wagtail.publish" - self.start_time = time.time() - else: - action = "bundles.update_status" - kwargs["data"] = { - "old": original_status, - "new": instance.get_status_display(), - } - notify_slack_of_status_change(instance, original_status, user=self.request.user, url=url) - - # now log the status change - log( - action=action, - instance=instance, - **kwargs, - ) + if not self.has_content_changes: + return instance + + log(action="wagtail.edit", instance=instance, content_changed=True, data={"fields": self.form.changed_data}) + + if "status" not in self.form.changed_data: + return instance + + kwargs: dict = {"content_changed": self.has_content_changes} + original_status = BundleStatus[self.form.original_status].label + url = self.request.build_absolute_uri(reverse("bundle:inspect", args=(instance.pk,))) + + if instance.status == BundleStatus.APPROVED: + action = "bundles.approve" + kwargs["data"] = {"old": original_status} + notify_slack_of_status_change(instance, original_status, user=self.request.user, url=url) + elif instance.status == BundleStatus.RELEASED.value: + action = "wagtail.publish" + self.start_time = time.time() + else: + action = "bundles.update_status" + kwargs["data"] = { + "old": original_status, + "new": instance.get_status_display(), + } + notify_slack_of_status_change(instance, original_status, user=self.request.user, url=url) + + # now log the status change + log( + action=action, + instance=instance, + **kwargs, + ) return instance @@ -144,7 +148,7 @@ def get_fields(self) -> list[str]: """Returns the list of fields to include in the inspect view.""" return ["name", "status", "created_at", "created_by", "approved", "scheduled_publication", "pages"] - def get_field_label(self, field_name: str, field: "Field") -> Any: + def get_field_label(self, field_name: str, field: "Field") -> str: match field_name: case "approved": return _("Approval status") @@ -153,7 +157,7 @@ def get_field_label(self, field_name: str, field: "Field") -> Any: case "pages": return _("Pages") case _: - return super().get_field_label(field_name, field) + return super().get_field_label(field_name, field) # type: ignore[no-any-return] def get_field_display_value(self, field_name: str, field: "Field") -> Any: """Allows customising field display in the inspect class. From 1bd97c8ddc3832ac18bef2bc47944612d058a47e Mon Sep 17 00:00:00 2001 From: zerolab Date: Mon, 2 Dec 2024 15:38:23 +0000 Subject: [PATCH 12/13] Further tidy ups --- cms/bundles/forms.py | 7 ++++--- cms/bundles/management/commands/publish_bundles.py | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/cms/bundles/forms.py b/cms/bundles/forms.py index e3b6055a..8c54cd9b 100644 --- a/cms/bundles/forms.py +++ b/cms/bundles/forms.py @@ -82,9 +82,10 @@ def clean(self) -> dict[str, Any] | None: if self.instance.created_by_id == self.for_user.pk: cleaned_data["status"] = self.instance.status self.add_error("status", ValidationError("You cannot self-approve your own bundle!")) - - cleaned_data["approved_at"] = timezone.now() - cleaned_data["approved_by"] = self.for_user + else: + # the approver is different from the creator, so let's populate the relevant fields. + cleaned_data["approved_at"] = timezone.now() + cleaned_data["approved_by"] = self.for_user elif self.instance.status == BundleStatus.APPROVED: cleaned_data["approved_at"] = None cleaned_data["approved_by"] = None diff --git a/cms/bundles/management/commands/publish_bundles.py b/cms/bundles/management/commands/publish_bundles.py index a79ad034..75ccb008 100644 --- a/cms/bundles/management/commands/publish_bundles.py +++ b/cms/bundles/management/commands/publish_bundles.py @@ -56,6 +56,7 @@ def _update_related_release_calendar_page(self, bundle: Bundle) -> None: revision = page.save_revision(log_action=True) revision.publish() + # TODO: revisit after discussion. @transaction.atomic def handle_bundle(self, bundle: Bundle) -> None: """Manages the bundle publication. @@ -90,7 +91,6 @@ def handle_bundle(self, bundle: Bundle) -> None: log(action="wagtail.publish.scheduled", instance=bundle) def handle(self, *args: Any, **options: dict[str, Any]) -> None: - """The management command handler.""" dry_run = False if options["dry_run"]: self.stdout.write("Will do a dry run.") From 6c3afbeec60d6b0ea80fbf7c70b773232c5867ef Mon Sep 17 00:00:00 2001 From: zerolab Date: Mon, 2 Dec 2024 16:14:20 +0000 Subject: [PATCH 13/13] fixup! Further tidy ups --- .pre-commit-config.yaml | 2 +- cms/bundles/tests/test_forms.py | 11 ++++++++++- cms/bundles/tests/test_viewsets.py | 6 ++++++ poetry.lock | 2 +- pyproject.toml | 2 +- 5 files changed, 19 insertions(+), 4 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index e6d8f051..47d76399 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -5,7 +5,7 @@ default_language_version: repos: # Python linting and formatting - repo: https://github.com/astral-sh/ruff-pre-commit - rev: 'v0.7.1' # keep version in sync with pyproject.toml + rev: 'v0.7.4' # keep version in sync with pyproject.toml hooks: - id: ruff args: [--fix, --exit-non-zero-on-fix] diff --git a/cms/bundles/tests/test_forms.py b/cms/bundles/tests/test_forms.py index 7e77d272..7822a886 100644 --- a/cms/bundles/tests/test_forms.py +++ b/cms/bundles/tests/test_forms.py @@ -128,7 +128,6 @@ def test_clean__validates_added_page_not_in_another_bundle(self): self.assertFormError(form, None, ["'The Analysis' is already in an active bundle (Another Bundle)"]) def test_clean__sets_approved_by_and_approved_at(self): - """Checks that Bundle gets the approver and approval time set.""" approver = UserFactory() data = self.form_data @@ -138,6 +137,16 @@ def test_clean__sets_approved_by_and_approved_at(self): self.assertTrue(form.is_valid()) self.assertEqual(form.cleaned_data["approved_by"], approver) + def test_clean__doesnt_set_approved_by_and_approved_at_if_self_approving(self): + data = self.form_data + data["status"] = BundleStatus.APPROVED + form = self.form_class(instance=self.bundle, data=data, for_user=self.bundle.created_by) + + self.assertFalse(form.is_valid()) + self.assertFormError(form, "status", ["You cannot self-approve your own bundle!"]) + self.assertIsNone(form.cleaned_data["approved_by"]) + self.assertIsNone(form.cleaned_data["approved_at"]) + def test_clean__validates_release_calendar_page_or_publication_date(self): release_calendar_page = ReleaseCalendarPageFactory() data = self.form_data diff --git a/cms/bundles/tests/test_viewsets.py b/cms/bundles/tests/test_viewsets.py index 29dbc5f0..f093ec2d 100644 --- a/cms/bundles/tests/test_viewsets.py +++ b/cms/bundles/tests/test_viewsets.py @@ -210,6 +210,12 @@ def test_bundle_approval__cannot__self_approve(self, mock_notify_slack): self.assertEqual(response.context["request"].path, self.edit_url) self.assertContains(response, "You cannot self-approve your own bundle!") + form = response.context["form"] + self.assertIsNone(form.cleaned_data["approved_by"]) + self.assertIsNone(form.cleaned_data["approved_at"]) + self.assertIsNone(form.fields["approved_by"].initial) + self.assertIsNone(form.fields["approved_at"].initial) + self.bundle.refresh_from_db() self.assertEqual(self.bundle.status, original_status) self.assertIsNone(self.bundle.approved_at) diff --git a/poetry.lock b/poetry.lock index fbdd5ae3..03da1f8b 100644 --- a/poetry.lock +++ b/poetry.lock @@ -2368,4 +2368,4 @@ files = [ [metadata] lock-version = "2.0" python-versions = "^3.12" -content-hash = "9dc5660c05f70a25a8f1f1a67b1436f69fac5a50d0981c9ccce5345f306c9bfc" +content-hash = "d6f3cd9d5d16ec644f0e0abfe25470f9d0baa2897b861f71ddcafa7987095dd3" diff --git a/pyproject.toml b/pyproject.toml index ec8b2b1a..a7808c98 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -38,7 +38,7 @@ mypy = "^1.13.0" # keep version in sync with .pre-commit-config.yaml django-stubs = { version="^5.1.1", extras=["compatible-mypy"]} pylint = "^3.3.1" pylint-django = "^2.0.0" -ruff = "^0.7.1" # keep version in sync with .pre-commit-config.yaml +ruff = "0.7.4" # keep version in sync with .pre-commit-config.yaml wagtail-factories = "^4.1.0" coverage = "^7.6.4"