From 178fa2afc6d859c607aefe4f730cb90cdabd3c86 Mon Sep 17 00:00:00 2001 From: Paul Pepper Date: Tue, 29 Aug 2023 14:50:02 +0100 Subject: [PATCH 01/15] Introduce Notification base-class and specialising sub-classes. --- notifications/models.py | 120 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 118 insertions(+), 2 deletions(-) diff --git a/notifications/models.py b/notifications/models.py index 502c1ee1e..ca5c7a52a 100644 --- a/notifications/models.py +++ b/notifications/models.py @@ -1,7 +1,15 @@ +import logging + +from django.conf import settings from django.db import models +from django.db.models.query_utils import Q +from notifications_python_client.notifications import NotificationsAPIClient +from polymorphic.models import PolymorphicModel from common.models.mixins import TimestampedMixin +logger = logging.getLogger(__name__) + class NotifiedUser(models.Model): """A NotifiedUser stores a user email address and whether that user is @@ -17,10 +25,118 @@ def __str__(self): class NotificationLog(TimestampedMixin): """ - A NotificationLog records which email template a group of users received. + A NotificationLog records which Notification a group of users received. We create one each time a group of users receive an email. """ - template_id = models.CharField(max_length=100) recipients = models.TextField() + notification = models.ForeignKey( + "notifications.Notification", + default=None, + null=True, + ) + + +class Notification(PolymorphicModel): + """ + Base class to manage sending notifications. + + Subclasses specialise this class's behaviour for specific categories of + notification. + """ + + notify_template_id: str + """GOV.UK Notify template ID, assigned in concrete subclasses.""" + + def send_emails(self): + """ + Handle sending emails for this notification to all associated + NotifiedUser instances. + + Implement in concrete subclasses. + """ + + raise NotImplementedError + + def schedule_send_emails(self, countdown=1): + """Schedule a call to send a notification email, run as an asynchronous + background task.""" + + send_emails.apply_sync(args=[self.pk], countdown=countdown) + + def send_email_notifications(self, notified_users): + """Send the individual notification emails to users via GOV.UK + Notify.""" + + if not notified_users: + logger.error( + f"No notified users for {self.__class__.__name__} " + f"with pk={self.pk}", + ) + return + + notifications_client = NotificationsAPIClient(settings.NOTIFICATIONS_API_KEY) + recipients = "" + for user in notified_users: + try: + notifications_client.send_email_notification( + email_address=user.email, + template_id=self.notify_template_id, + # TODO: get personalisation data. + personalisation={}, + ) + + recipients += f"{user.email} \n" + except: + logger.error( + f"Failed to send email notification to {user.email}.", + ) + + NotificationLog.objects.create( + recipients=recipients, + notification=self, + ) + + +class EnvelopeReadyForProcessingNotification(Notification): + """Manage sending notifications when envelopes are ready for processing by + HMRC.""" + + notify_template_id = "todo:notify-template-id-goes-here" + + def send_emails(self): + notified_users = NotifiedUser.objects.filter( + Q(enrol_packaging=True) | Q(enrole_api_publishing=True), + ) + self.send_email_notifications(notified_users) + + +class EnvelopeAcceptedNotification(Notification): + """TODO.""" + + +class EnvelopeRejectedNotification(Notification): + """TODO.""" + + +class GoodsSuccessfulImportNotification(Notification): + """TODO.""" + + +# ========================== start ========================== +# Code between start and end is a rewrite of, and drop-in replacement for +# notifications.tasks.send_emails(). + +from celery import shared_task +from django.db.transaction import atomic + + +@shared_task +@atomic +def send_emails(notification_pk: int): + notification = Notification.objects.get(pk=notification_pk) + notification.send_emails() + + +# ========================== end ========================== From 18e5619e68e96fcc95e869539be99b2537f9efbf Mon Sep 17 00:00:00 2001 From: Paul Pepper Date: Tue, 29 Aug 2023 17:04:37 +0100 Subject: [PATCH 02/15] Change emphasis of sub-class implementations - get notify template ID and notified users. --- notifications/models.py | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/notifications/models.py b/notifications/models.py index ca5c7a52a..eafe5d549 100644 --- a/notifications/models.py +++ b/notifications/models.py @@ -46,17 +46,20 @@ class Notification(PolymorphicModel): notification. """ - notify_template_id: str - """GOV.UK Notify template ID, assigned in concrete subclasses.""" - - def send_emails(self): + def notify_template_id(self) -> str: """ - Handle sending emails for this notification to all associated - NotifiedUser instances. + GOV.UK Notify template ID specific to each Notification sub-class. Implement in concrete subclasses. """ + raise NotImplementedError + + def notified_users(self) -> models.QuerySet[NotifiedUser]: + """ + Returns the queryset of NotifiedUsers for a specific notifications. + Implement in concrete subclasses. + """ raise NotImplementedError def schedule_send_emails(self, countdown=1): @@ -65,10 +68,10 @@ def schedule_send_emails(self, countdown=1): send_emails.apply_sync(args=[self.pk], countdown=countdown) - def send_email_notifications(self, notified_users): - """Send the individual notification emails to users via GOV.UK - Notify.""" + def send_emails(self): + """Send the notification emails to users via GOV.UK Notify.""" + notified_users = self.notified_users() if not notified_users: logger.error( f"No notified users for {self.__class__.__name__} " @@ -82,7 +85,7 @@ def send_email_notifications(self, notified_users): try: notifications_client.send_email_notification( email_address=user.email, - template_id=self.notify_template_id, + template_id=self.notify_template_id(), # TODO: get personalisation data. personalisation={}, ) @@ -103,13 +106,13 @@ class EnvelopeReadyForProcessingNotification(Notification): """Manage sending notifications when envelopes are ready for processing by HMRC.""" - notify_template_id = "todo:notify-template-id-goes-here" + def notify_template_id(self) -> str: + return settings.READY_FOR_CDS_TEMPLATE_ID - def send_emails(self): - notified_users = NotifiedUser.objects.filter( + def notified_users(self): + return NotifiedUser.objects.filter( Q(enrol_packaging=True) | Q(enrole_api_publishing=True), ) - self.send_email_notifications(notified_users) class EnvelopeAcceptedNotification(Notification): @@ -125,7 +128,7 @@ class GoodsSuccessfulImportNotification(Notification): # ========================== start ========================== -# Code between start and end is a rewrite of, and drop-in replacement for +# Code between start and end is a rewrite of, and drop-in replacement for, # notifications.tasks.send_emails(). from celery import shared_task From d054d0917472e59f2243092e07114c414658ef9c Mon Sep 17 00:00:00 2001 From: Paul Pepper Date: Tue, 29 Aug 2023 17:34:40 +0100 Subject: [PATCH 03/15] Associate Notifications with their source notified object. --- notifications/models.py | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/notifications/models.py b/notifications/models.py index eafe5d549..5a3140b60 100644 --- a/notifications/models.py +++ b/notifications/models.py @@ -31,6 +31,8 @@ class NotificationLog(TimestampedMixin): """ recipients = models.TextField() + """Comma separated email addresses, as a single string, of the recipients of + the notification.""" notification = models.ForeignKey( "notifications.Notification", default=None, @@ -46,6 +48,14 @@ class Notification(PolymorphicModel): notification. """ + notified_object_pk: int + """The primary key of the.""" + + notified_object_pk = models.IntegerField( + default=None, + null=True, + ) + def notify_template_id(self) -> str: """ GOV.UK Notify template ID specific to each Notification sub-class. @@ -62,6 +72,14 @@ def notified_users(self) -> models.QuerySet[NotifiedUser]: """ raise NotImplementedError + def notified_object(self) -> models.Model: + """ + Returns the object instance that is being notified on. + + Implement in concrete subclasses. + """ + raise NotImplementedError + def schedule_send_emails(self, countdown=1): """Schedule a call to send a notification email, run as an asynchronous background task.""" @@ -114,6 +132,15 @@ def notified_users(self): Q(enrol_packaging=True) | Q(enrole_api_publishing=True), ) + def notified_object(self) -> models.Model: + from publishing.models import PackagedWorkBasket + + return ( + PackagedWorkBasket.objects.get(self.notified_object_pk) + if self.notified_object_pk + else None + ) + class EnvelopeAcceptedNotification(Notification): """TODO.""" From 36b79db8ff57af6be352fdecc0d42a4fef1d2d5e Mon Sep 17 00:00:00 2001 From: Paul Pepper Date: Wed, 30 Aug 2023 12:23:41 +0100 Subject: [PATCH 04/15] Use proxy model of inheritance in base classes. --- notifications/models.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/notifications/models.py b/notifications/models.py index 5a3140b60..0770f1dea 100644 --- a/notifications/models.py +++ b/notifications/models.py @@ -4,7 +4,6 @@ from django.db import models from django.db.models.query_utils import Q from notifications_python_client.notifications import NotificationsAPIClient -from polymorphic.models import PolymorphicModel from common.models.mixins import TimestampedMixin @@ -40,21 +39,25 @@ class NotificationLog(TimestampedMixin): ) -class Notification(PolymorphicModel): +class Notification(models.Model): """ Base class to manage sending notifications. Subclasses specialise this class's behaviour for specific categories of notification. + + Subclasses should specify the proxy model of inheritance: + https://docs.djangoproject.com/en/dev/topics/db/models/#proxy-models """ - notified_object_pk: int - """The primary key of the.""" + def __init__(self, notified_object_pk: int = None): + self.notified_object_pk = notified_object_pk notified_object_pk = models.IntegerField( default=None, null=True, ) + """The primary key of the object being notified on.""" def notify_template_id(self) -> str: """ @@ -124,6 +127,9 @@ class EnvelopeReadyForProcessingNotification(Notification): """Manage sending notifications when envelopes are ready for processing by HMRC.""" + class Meta: + proxy = True + def notify_template_id(self) -> str: return settings.READY_FOR_CDS_TEMPLATE_ID From c68bf1a4abad50e123e7d7a7ba190afccc71df3f Mon Sep 17 00:00:00 2001 From: Anthoni Gleeson Date: Fri, 8 Sep 2023 14:55:32 +0100 Subject: [PATCH 05/15] WIP mostly working fix notification choices --- .gitignore | 2 + .../jinja2/eu-importer/notify-success.jinja | 40 +++ .../send_goods_report_notification.py | 45 +++ importer/urls.py | 10 + importer/views.py | 37 ++ notifications/admin.py | 51 +++ notifications/forms.py | 1 + .../migrations/0003_auto_20230908_0850.py | 150 ++++++++ notifications/models.py | 334 +++++++++++++++--- notifications/notify.py | 86 +++++ notifications/tasks.py | 44 +-- notifications/utils.py | 0 .../models/crown_dependencies_envelope.py | 48 ++- publishing/models/packaged_workbasket.py | 84 ++--- sample.env | 1 + settings/common.py | 1 + .../jinja2/workbaskets/review-goods.jinja | 9 + .../workbaskets/summary-workbasket.jinja | 15 +- workbaskets/views/ui.py | 25 ++ 19 files changed, 814 insertions(+), 169 deletions(-) create mode 100644 importer/jinja2/eu-importer/notify-success.jinja create mode 100644 importer/management/commands/send_goods_report_notification.py create mode 100644 notifications/migrations/0003_auto_20230908_0850.py create mode 100644 notifications/notify.py create mode 100644 notifications/utils.py diff --git a/.gitignore b/.gitignore index 43109bc38..28d2a8cf7 100644 --- a/.gitignore +++ b/.gitignore @@ -165,3 +165,5 @@ _dumped_cache.pkl # Vim .swp + +tamato_*.sql diff --git a/importer/jinja2/eu-importer/notify-success.jinja b/importer/jinja2/eu-importer/notify-success.jinja new file mode 100644 index 000000000..17b495110 --- /dev/null +++ b/importer/jinja2/eu-importer/notify-success.jinja @@ -0,0 +1,40 @@ +{% extends "layouts/layout.jinja" %} +{% from "components/breadcrumbs/macro.njk" import govukBreadcrumbs %} +{% from "components/button/macro.njk" import govukButton %} +{% from "components/panel/macro.njk" import govukPanel %} + + +{% set page_title = "Notify Goods Report" %} + + +{% block breadcrumb %} + {{ govukBreadcrumbs({ + "items": [ + {"text": "Home", "href": url("home")}, + {"text": "Edit an existing workbasket", "href": url("workbaskets:workbasket-ui-list")}, + {"text": "Workbasket " ~ request.session.workbasket.id ~ " - Review goods", + "href": url("workbaskets:workbasket-ui-review-goods")}, + {"text": page_title} + ] + }) }} +{% endblock %} + + +{% block content %} +
+
+ {{ govukPanel({ + "titleText": "Notification email sent", + "text": "An email notification has been successfully sent to Channel Islands.", + "classes": "govuk-!-margin-bottom-7" + }) }} + +
+ + Back to Review goods list + +
+ +
+
+{% endblock %} \ No newline at end of file diff --git a/importer/management/commands/send_goods_report_notification.py b/importer/management/commands/send_goods_report_notification.py new file mode 100644 index 000000000..f08a03ddc --- /dev/null +++ b/importer/management/commands/send_goods_report_notification.py @@ -0,0 +1,45 @@ +from django.core.management import BaseCommand +from django.core.management.base import CommandError + +from importer.models import ImportBatch +from notifications.models import GoodsSuccessfulImportNotification +from notifications.models import NotificationTypeChoices + + +def send_notifcation( + import_id: int, +): + try: + import_batch = ImportBatch.objects.get( + pk=import_id, + ) + except ImportBatch.DoesNotExist: + raise CommandError( + f"No ImportBatch instance found with pk={import_id}", + ) + + notification = GoodsSuccessfulImportNotification( + notificaiton_type=NotificationTypeChoices.GOODS_REPORT, + notified_object_pk=import_batch.id, + ) + notification.save() + notification.synchronous_send_emails() + + +class Command(BaseCommand): + help = "Send a good report notifcation for a give Id" + + def add_arguments(self, parser): + parser.add_argument( + "--import-batch-id", + help=( + "The primary key ID of ImportBatch instance for which a report " + "should be generated." + ), + type=int, + ) + + def handle(self, *args, **options): + send_notifcation( + import_id=options["import_batch_id"], + ) diff --git a/importer/urls.py b/importer/urls.py index cbeae244e..6497ffca3 100644 --- a/importer/urls.py +++ b/importer/urls.py @@ -41,6 +41,16 @@ views.DownloadGoodsReportView.as_view(), name="goods-report-ui-download", ), + path( + "notify-goods-report//", + views.NotifyGoodsReportView.as_view(), + name="goods-report-notify", + ), + path( + "notify-goods-report-confirm//", + views.NotifyGoodsReportSuccessView.as_view(), + name="goods-report-notify-success", + ), ] urlpatterns = general_importer_urlpatterns + commodity_importer_urlpatterns diff --git a/importer/views.py b/importer/views.py index e40e948db..bdfad1855 100644 --- a/importer/views.py +++ b/importer/views.py @@ -19,6 +19,8 @@ from importer.filters import TaricImportFilter from importer.goods_report import GoodsReporter from importer.models import ImportBatchStatus +from notifications.models import GoodsSuccessfulImportNotification +from notifications.models import NotificationTypeChoices from workbaskets.validators import WorkflowStatus @@ -236,3 +238,38 @@ class DownloadGoodsReportView( def get(self, request, *args, **kwargs) -> HttpResponse: import_batch = self.get_object() return self.download_response(import_batch) + + +class NotifyGoodsReportView( + PermissionRequiredMixin, + DetailView, +): + """View used to notify an import report of goods changes in Excel format.""" + + permission_required = "common.add_trackedmodel" + model = models.ImportBatch + + def get(self, request, *args, **kwargs): + import_batch = self.get_object() + + # create notification + notification = GoodsSuccessfulImportNotification( + notificaiton_type=( + NotificationTypeChoices.GOODS_REPORT.value, + NotificationTypeChoices.GOODS_REPORT.label, + ), + notified_object_pk=import_batch.id, + ) + notification.save() + notification.synchronous_send_emails() + + return redirect( + reverse("goods-report-notify-success", kwargs={"pk": import_batch.id}), + ) + + +class NotifyGoodsReportSuccessView(DetailView): + """Goods Report notification success trigger view.""" + + template_name = "eu-importer/notify-success.jinja" + model = models.ImportBatch diff --git a/notifications/admin.py b/notifications/admin.py index bc6da4940..4ffa133f1 100644 --- a/notifications/admin.py +++ b/notifications/admin.py @@ -1,6 +1,7 @@ from django.contrib import admin from notifications.forms import NotifiedUserAdminForm +from notifications.models import Notification from notifications.models import NotificationLog from notifications.models import NotifiedUser @@ -13,12 +14,15 @@ class NotifiedUserAdmin(admin.ModelAdmin): "email", "enrol_packaging", "enrol_api_publishing", + "enrol_goods_report", ) actions = [ "set_enrol_packaging", "unset_enrol_packaging", "set_enrol_api_publishing", "unset_enrol_api_publishing", + "set_enrol_goods_report", + "unset_enrol_goods_report", ] def set_enrol_packaging(self, request, queryset): @@ -33,6 +37,12 @@ def set_enrol_api_publishing(self, request, queryset): def unset_enrol_api_publishing(self, request, queryset): queryset.update(enrol_api_publishing=False) + def set_enrol_goods_report(self, request, queryset): + queryset.update(enrol_goods_report=True) + + def unset_enrol_goods_report(self, request, queryset): + queryset.update(enrol_goods_report=False) + class NotificationLogAdmin(admin.ModelAdmin): """Class providing read-only list and detail views for notification logs.""" @@ -59,5 +69,46 @@ def changeform_view(self, request, object_id=None, form_url="", extra_context=No ) +class NotificationAdmin(admin.ModelAdmin): + """Class providing read-only list and detail views for notification.""" + + ordering = ["pk"] + list_display = ( + "pk", + "notificaiton_type", + "notified_object_pk", + # "display_users", + ) + + readonly_fields = [] + + def display_users(self, obj): + return ", ".join( + [user.email for user in obj.notified_users().all()], + ) + + display_users.short_description = "Recipients" + + def get_readonly_fields(self, request, obj=None): + return list(self.readonly_fields) + [field.name for field in obj._meta.fields] + + def has_add_permission(self, request): + return False + + def has_delete_permission(self, request, obj=None): + return False + + def changeform_view(self, request, object_id=None, form_url="", extra_context=None): + extra_context = extra_context or {} + extra_context["show_save_and_continue"] = False + extra_context["show_save"] = False + return super(NotificationAdmin, self).changeform_view( + request, + object_id, + extra_context=extra_context, + ) + + admin.site.register(NotifiedUser, NotifiedUserAdmin) admin.site.register(NotificationLog, NotificationLogAdmin) +admin.site.register(Notification, NotificationAdmin) diff --git a/notifications/forms.py b/notifications/forms.py index 6812a9dd1..164a048f7 100644 --- a/notifications/forms.py +++ b/notifications/forms.py @@ -10,4 +10,5 @@ class Meta: "email", "enrol_packaging", "enrol_api_publishing", + "enrol_goods_report", ] diff --git a/notifications/migrations/0003_auto_20230908_0850.py b/notifications/migrations/0003_auto_20230908_0850.py new file mode 100644 index 000000000..750fe6324 --- /dev/null +++ b/notifications/migrations/0003_auto_20230908_0850.py @@ -0,0 +1,150 @@ +# Generated by Django 3.2.20 on 2023-09-08 07:50 + +import django.db.models.deletion +from django.db import migrations +from django.db import models + + +class Migration(migrations.Migration): + dependencies = [ + ("notifications", "0002_notifieduser_enrol_api_publishing"), + ] + + operations = [ + migrations.CreateModel( + name="Notification", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ("notified_object_pk", models.IntegerField(default=None, null=True)), + ( + "notificaiton_type", + models.CharField( + choices=[ + ( + "", + "Goods Report", + ), + ( + "", + "Packaging Notify Ready", + ), + ( + "", + "Packaging Accepted", + ), + ( + "", # /PS-IGNORE + "Packaging Rejected", + ), + ( + "", + "Publishing Success", + ), + ( + "", + "Publishing Failed", + ), + ], + max_length=150, + ), + ), + ], + ), + migrations.RemoveField( + model_name="notificationlog", + name="template_id", + ), + migrations.AddField( + model_name="notificationlog", + name="response_ids", + field=models.TextField(default=None, null=True), + ), + migrations.AddField( + model_name="notificationlog", + name="success", + field=models.BooleanField(default=True), + ), + migrations.AddField( + model_name="notifieduser", + name="enrol_goods_report", + field=models.BooleanField(default=False), + ), + migrations.CreateModel( + name="CrownDependenciesEnvelopeFailedNotification", + fields=[], + options={ + "proxy": True, + "indexes": [], + "constraints": [], + }, + bases=("notifications.notification",), + ), + migrations.CreateModel( + name="CrownDependenciesEnvelopeSuccessNotification", + fields=[], + options={ + "proxy": True, + "indexes": [], + "constraints": [], + }, + bases=("notifications.notification",), + ), + migrations.CreateModel( + name="EnvelopeAcceptedNotification", + fields=[], + options={ + "proxy": True, + "indexes": [], + "constraints": [], + }, + bases=("notifications.notification",), + ), + migrations.CreateModel( + name="EnvelopeReadyForProcessingNotification", + fields=[], + options={ + "proxy": True, + "indexes": [], + "constraints": [], + }, + bases=("notifications.notification",), + ), + migrations.CreateModel( + name="EnvelopeRejectedNotification", + fields=[], + options={ + "proxy": True, + "indexes": [], + "constraints": [], + }, + bases=("notifications.notification",), + ), + migrations.CreateModel( + name="GoodsSuccessfulImportNotification", + fields=[], + options={ + "proxy": True, + "indexes": [], + "constraints": [], + }, + bases=("notifications.notification",), + ), + migrations.AddField( + model_name="notificationlog", + name="notification", + field=models.ForeignKey( + default=None, + null=True, + on_delete=django.db.models.deletion.PROTECT, + to="notifications.notification", + ), + ), + ] diff --git a/notifications/models.py b/notifications/models.py index 0770f1dea..a4bfb6b0c 100644 --- a/notifications/models.py +++ b/notifications/models.py @@ -3,9 +3,12 @@ from django.conf import settings from django.db import models from django.db.models.query_utils import Q -from notifications_python_client.notifications import NotificationsAPIClient +from django.urls import reverse from common.models.mixins import TimestampedMixin +from notifications.notify import prepare_link_to_file +from notifications.notify import send_emails +from notifications.tasks import send_emails_task logger = logging.getLogger(__name__) @@ -17,9 +20,10 @@ class NotifiedUser(models.Model): email = models.EmailField() enrol_packaging = models.BooleanField(default=True) enrol_api_publishing = models.BooleanField(default=False) + enrol_goods_report = models.BooleanField(default=False) - def __str__(self): - return self.email + def __str__(self) -> str: + return str(self.email) class NotificationLog(TimestampedMixin): @@ -29,6 +33,10 @@ class NotificationLog(TimestampedMixin): We create one each time a group of users receive an email. """ + response_ids = models.TextField( + default=None, + null=True, + ) recipients = models.TextField() """Comma separated email addresses, as a single string, of the recipients of the notification.""" @@ -36,6 +44,48 @@ class NotificationLog(TimestampedMixin): "notifications.Notification", default=None, null=True, + on_delete=models.PROTECT, + ) + success = models.BooleanField(default=True) + + +class CustomNotificationChoice: + def __init__(self, value, label, template_id): + self.value = value + self.label = label + self.template_id = template_id + + +class NotificationTypeChoices(models.TextChoices): + GOODS_REPORT = CustomNotificationChoice( + "goods_report", + "Goods Report", + settings.GOODS_REPORT_TEMPLATE_ID, + ) + PACKAGING_NOTIFY_READY = CustomNotificationChoice( + "packaging_notify_ready", + "Packaging Notify Ready", + settings.READY_FOR_CDS_TEMPLATE_ID, + ) + PACKAGING_ACCEPTED = CustomNotificationChoice( + "packaging_accepted", + "Packaging Accepted", + settings.CDS_ACCEPTED_TEMPLATE_ID, + ) + PACKAGING_REJECTED = CustomNotificationChoice( + "packaging_rejected", + "Packaging Rejected", + settings.CDS_REJECTED_TEMPLATE_ID, + ) + PUBLISHING_SUCCESS = CustomNotificationChoice( + "publishing_success", + "Publishing Successful", + settings.API_PUBLISH_SUCCESS_TEMPLATE_ID, + ) + PUBLISHING_FAILED = CustomNotificationChoice( + "publishing_failed", + "Publishing Failed", + settings.API_PUBLISH_FAILED_TEMPLATE_ID, ) @@ -50,8 +100,9 @@ class Notification(models.Model): https://docs.djangoproject.com/en/dev/topics/db/models/#proxy-models """ - def __init__(self, notified_object_pk: int = None): - self.notified_object_pk = notified_object_pk + # def __init__(self, notificaiton_type: NotificationTypeChoices, notified_object_pk: int = None, ): + # self.notified_object_pk = notified_object_pk + # self.notificaiton_type = notificaiton_type notified_object_pk = models.IntegerField( default=None, @@ -59,6 +110,19 @@ def __init__(self, notified_object_pk: int = None): ) """The primary key of the object being notified on.""" + notificaiton_type = models.CharField( + max_length=150, + choices=[(choice.value, choice.label) for choice in NotificationTypeChoices], + ) + + def get_personalisation(self) -> dict: + """ + Returns the personalisation of the notified object. + + Implement in concrete subclasses. + """ + raise NotImplementedError + def notify_template_id(self) -> str: """ GOV.UK Notify template ID specific to each Notification sub-class. @@ -83,11 +147,14 @@ def notified_object(self) -> models.Model: """ raise NotImplementedError + def synchronous_send_emails(self): + """Synchronously call to send a notification email.""" + send_emails_task(self.pk, type(self)) + def schedule_send_emails(self, countdown=1): """Schedule a call to send a notification email, run as an asynchronous background task.""" - - send_emails.apply_sync(args=[self.pk], countdown=countdown) + send_emails_task.apply_sync(args=[self.pk, type(self)], countdown=countdown) def send_emails(self): """Send the notification emails to users via GOV.UK Notify.""" @@ -100,28 +167,28 @@ def send_emails(self): ) return - notifications_client = NotificationsAPIClient(settings.NOTIFICATIONS_API_KEY) - recipients = "" - for user in notified_users: - try: - notifications_client.send_email_notification( - email_address=user.email, - template_id=self.notify_template_id(), - # TODO: get personalisation data. - personalisation={}, - ) - - recipients += f"{user.email} \n" - except: - logger.error( - f"Failed to send email notification to {user.email}.", - ) + personalisation = self.get_personalisation() + + result = send_emails( + self.notify_template_id(), + personalisation, + notified_users, + ) NotificationLog.objects.create( - recipients=recipients, + response_ids=result["response_ids"], + recipients=result["recipients"], notification=self, ) + # if any emails failed create a log for unsuccessful emails + if result["failed_recipients"]: + NotificationLog.objects.create( + recipients=result["failed_recipients"], + notification=self, + success=False, + ) + class EnvelopeReadyForProcessingNotification(Notification): """Manage sending notifications when envelopes are ready for processing by @@ -130,12 +197,33 @@ class EnvelopeReadyForProcessingNotification(Notification): class Meta: proxy = True + def get_personalisation(self) -> dict: + packaged_workbasket = self.notified_object() + eif = "Immediately" + if packaged_workbasket.eif: + eif = packaged_workbasket.eif.strftime("%d/%m/%Y") + + personalisation = { + "envelope_id": packaged_workbasket.envelope.envelope_id, + "description": packaged_workbasket.description, + "download_url": ( + settings.BASE_SERVICE_URL + reverse("publishing:envelope-queue-ui-list") + ), + "theme": packaged_workbasket.theme, + "eif": eif, + "embargo": packaged_workbasket.embargo + if packaged_workbasket.embargo + else "None", + "jira_url": packaged_workbasket.jira_url, + } + return personalisation + def notify_template_id(self) -> str: - return settings.READY_FOR_CDS_TEMPLATE_ID + return NotificationTypeChoices.PACKAGING_NOTIFY_READY.template_id def notified_users(self): return NotifiedUser.objects.filter( - Q(enrol_packaging=True) | Q(enrole_api_publishing=True), + Q(enrol_packaging=True), ) def notified_object(self) -> models.Model: @@ -149,30 +237,194 @@ def notified_object(self) -> models.Model: class EnvelopeAcceptedNotification(Notification): - """TODO.""" + """Manage sending notifications when envelopes have been accepted by + HMRC.""" + + class Meta: + proxy = True + + def get_personalisation(self) -> dict: + packaged_workbasket = self.notified_object() + loading_report_message = "Loading report: No loading report was provided." + loading_reports = packaged_workbasket.loadingreports.exclude( + file_name="", + ).values_list( + "file_name", + flat=True, + ) + if loading_reports: + file_names = ", ".join(loading_reports) + loading_report_message = f"Loading report(s): {file_names}" + + personalisation = { + "envelope_id": packaged_workbasket.envelope.envelope_id, + "transaction_count": packaged_workbasket.workbasket.transactions.count(), + "loading_report_message": loading_report_message, + "comments": packaged_workbasket.loadingreports.first().comments, + } + return personalisation + + def notify_template_id(self) -> str: + return NotificationTypeChoices.PACKAGING_ACCEPTED.template_id + + def notified_users(self): + return NotifiedUser.objects.filter( + Q(enrol_packaging=True), + ) + + def notified_object(self) -> models.Model: + from publishing.models import PackagedWorkBasket + + return ( + PackagedWorkBasket.objects.get(self.notified_object_pk) + if self.notified_object_pk + else None + ) class EnvelopeRejectedNotification(Notification): - """TODO.""" + """Manage sending notifications when envelopes have been rejected by + HMRC.""" + class Meta: + proxy = True -class GoodsSuccessfulImportNotification(Notification): - """TODO.""" + def get_personalisation(self) -> dict: + packaged_workbasket = self.notified_object() + loading_report_message = "Loading report: No loading report was provided." + loading_reports = packaged_workbasket.loadingreports.exclude( + file_name="", + ).values_list( + "file_name", + flat=True, + ) + if loading_reports: + file_names = ", ".join(loading_reports) + loading_report_message = f"Loading report(s): {file_names}" + + personalisation = { + "envelope_id": packaged_workbasket.envelope.envelope_id, + "transaction_count": packaged_workbasket.workbasket.transactions.count(), + "loading_report_message": loading_report_message, + "comments": packaged_workbasket.loadingreports.first().comments, + } + return personalisation + def notify_template_id(self) -> str: + return NotificationTypeChoices.PACKAGING_REJECTED.template_id -# ========================== start ========================== -# Code between start and end is a rewrite of, and drop-in replacement for, -# notifications.tasks.send_emails(). + def notified_users(self): + return NotifiedUser.objects.filter( + Q(enrol_packaging=True), + ) -from celery import shared_task -from django.db.transaction import atomic + def notified_object(self) -> models.Model: + from publishing.models import PackagedWorkBasket + return ( + PackagedWorkBasket.objects.get(self.notified_object_pk) + if self.notified_object_pk + else None + ) + + +class CrownDependenciesEnvelopeSuccessNotification(Notification): + """Manage sending notifications when envelopes have been successfully + published to the Crown Dependencies api.""" + + class Meta: + proxy = True + + def get_personalisation(self) -> dict: + crown_dependicies_envelope = self.notified_object() + personalisation = { + "envelope_id": crown_dependicies_envelope.packagedworkbaskets.last().envelope.envelope_id, + } + return personalisation + + def notify_template_id(self) -> str: + return NotificationTypeChoices.PUBLISHING_SUCCESS.template_id + + def notified_users(self): + return NotifiedUser.objects.filter( + Q(enrole_api_publishing=True), + ) + + def notified_object(self) -> models.Model: + from publishing.models import CrownDependenciesEnvelope + + return ( + CrownDependenciesEnvelope.objects.get(self.notified_object_pk) + if self.notified_object_pk + else None + ) + + +class CrownDependenciesEnvelopeFailedNotification(Notification): + """Manage sending notifications when envelopes have been failed being + published to the Crown Dependencies api.""" + + class Meta: + proxy = True + + def get_personalisation(self) -> dict: + self.notified_object() + personalisation = { + "envelope_id": self.packagedworkbaskets.last().envelope.envelope_id, + } + return personalisation + + def notify_template_id(self) -> str: + return NotificationTypeChoices.PUBLISHING_FAILED.template_id + + def notified_users(self): + return NotifiedUser.objects.filter( + Q(enrole_api_publishing=True), + ) + + def notified_object(self) -> models.Model: + from publishing.models import CrownDependenciesEnvelope + + return ( + CrownDependenciesEnvelope.objects.get(self.notified_object_pk) + if self.notified_object_pk + else None + ) + + +class GoodsSuccessfulImportNotification(Notification): + """Manage sending notifications when a goods report has been reviewed and + can be sent to Crown Dependencies.""" -@shared_task -@atomic -def send_emails(notification_pk: int): - notification = Notification.objects.get(pk=notification_pk) - notification.send_emails() + class Meta: + proxy = True + + def get_personalisation(self) -> dict: + import_batch = self.notified_object() + personalisation = ( + { + "tgb_id": import_batch.name, + "link_to_file": prepare_link_to_file( + import_batch.taric_file, + confirm_email_before_download=True, + ), + }, + ) + return personalisation + def notify_template_id(self) -> str: + return NotificationTypeChoices.GOODS_REPORT.template_id -# ========================== end ========================== + def notified_users(self): + return NotifiedUser.objects.filter( + Q(enrol_goods_report=True), + ) + + def notified_object(self) -> models.Model: + from importer.models import ImportBatch + + return ( + ImportBatch.objects.get(self.notified_object_pk) + if self.notified_object_pk + else None + ) diff --git a/notifications/notify.py b/notifications/notify.py new file mode 100644 index 000000000..643181268 --- /dev/null +++ b/notifications/notify.py @@ -0,0 +1,86 @@ +import logging +from tempfile import NamedTemporaryFile +from typing import List + +from django.conf import settings +from notifications_python_client import prepare_upload +from notifications_python_client.notifications import NotificationsAPIClient + +from importer.goods_report import GoodsReporter + +logger = logging.getLogger(__name__) + + +def get_notifications_client(): + return NotificationsAPIClient(settings.NOTIFICATIONS_API_KEY) + + +def prepare_link_to_file( + file, + is_csv=False, + confirm_email_before_download=None, + retention_period=None, +): + """ + Prepare importer file to upload. Improvement possibility have file pre + genreated and in s3 possibly. + + params: + file: file which to generate report from + is_csv: if the file being attached is a csv set to True, default False + confirm_email_before_download: security feature where user opening files must be on Gov Notify email list + retention_period: how long the file link is valid for, default 6 months + """ + + with NamedTemporaryFile(suffix=".xlsx") as tmp: + reporter = GoodsReporter(file) + goods_report = reporter.create_report() + goods_report.xlsx_file(tmp) + return prepare_upload( + tmp, + is_csv, + confirm_email_before_download, + retention_period, + ) + + +def send_emails(template_id, personalisation: dict, email_addresses: List[str]) -> dict: + """ + Generic send emails function which triggers a notification to Gov notify. + + params: + template_id: email template Id + personalisation: email personalisation + email_addresses: list of emails to send emails to + + returns: + dict( + "response_ids": string of successful email response ids + "recipients": string of successful emails recipients + "failed_recipients": string of unsuccessful emails recipients + ) + """ + notification_client = get_notifications_client() + recipients = "" + failed_recipients = "" + response_ids = "" + for email in email_addresses: + try: + response = notification_client.send_email_notification( + email_address=email, + template_id=template_id, + personalisation=personalisation, + ) + response_ids += f"{response['id']} \n" + recipients += f"{email} \n" + except Exception as e: + failed_recipients += f"{email} \n" + logger.error( + f"Failed to send email notification to {email}, with status code {e.status_code}.", + ) + + return { + "response_ids": response_ids, + "recipients": recipients, + "failed_recipients": failed_recipients, + } diff --git a/notifications/tasks.py b/notifications/tasks.py index 407f8e657..cc7d292f2 100644 --- a/notifications/tasks.py +++ b/notifications/tasks.py @@ -1,50 +1,18 @@ import logging -from uuid import uuid4 from celery import shared_task -from django.conf import settings -from django.db.models.query_utils import Q from django.db.transaction import atomic -from notifications_python_client.notifications import NotificationsAPIClient - -from notifications.models import NotificationLog -from notifications.models import NotifiedUser logger = logging.getLogger(__name__) -def get_notifications_client(): - return NotificationsAPIClient(settings.NOTIFICATIONS_API_KEY) - - +# def get_notifications_client(): +# return NotificationsAPIClient(settings.NOTIFICATIONS_API_KEY) @shared_task @atomic -def send_emails(template_id: uuid4, personalisation: dict, email_type: str = None): +def send_emails_task(notification_pk: int, notification_type: "Notification"): """Task for emailing all users signed up to receive packaging updates and creating a log to record which users received which email template.""" - user_filters = { - "packaging": Q(enrol_packaging=True), - "publishing": Q(enrol_api_publishing=True), - } - # Will get all users by default - users = NotifiedUser.objects.filter(user_filters.get(email_type, Q())) - if users.exists(): - notifications_client = get_notifications_client() - recipients = "" - for user in users: - try: - notifications_client.send_email_notification( - email_address=user.email, - template_id=template_id, - personalisation=personalisation, - ) - recipients += f"{user.email} \n" - except: - logger.error( - f"Failed to send email notification to {user.email}.", - ) - - NotificationLog.objects.create( - template_id=template_id, - recipients=recipients, - ) + print(notification_type) + notification = notification_type.objects.get(pk=notification_pk) + notification.send_emails() diff --git a/notifications/utils.py b/notifications/utils.py new file mode 100644 index 000000000..e69de29bb diff --git a/publishing/models/crown_dependencies_envelope.py b/publishing/models/crown_dependencies_envelope.py index b1e778659..4e49577ac 100644 --- a/publishing/models/crown_dependencies_envelope.py +++ b/publishing/models/crown_dependencies_envelope.py @@ -1,7 +1,6 @@ import logging from datetime import datetime -from django.conf import settings from django.db.models import DateTimeField from django.db.models import Manager from django.db.models import QuerySet @@ -10,7 +9,9 @@ from django_fsm import transition from common.models.mixins import TimestampedMixin -from notifications.tasks import send_emails +from notifications.models import CrownDependenciesEnvelopeFailedNotification +from notifications.models import CrownDependenciesEnvelopeSuccessNotification +from notifications.models import NotificationTypeChoices from publishing.models.decorators import save_after from publishing.models.decorators import skip_notifications_if_disabled from publishing.models.packaged_workbasket import PackagedWorkBasket @@ -79,11 +80,13 @@ class CrownDependenciesEnvelope(TimestampedMixin): """ Represents a crown dependencies envelope. - This model contains the Envelope upload status to the Channel islands API and it's publishing times. + This model contains the Envelope upload status to the Channel islands API + and it's publishing times. An Envelope contains one or more Transactions, listing changes to be applied to the tariff in the sequence defined by the transaction IDs. Contains - xml_file which is a reference to the envelope stored on s3. This can be found in the Envelope model. + xml_file which is a reference to the envelope stored on s3. This can be + found in the Envelope model. """ class Meta: @@ -149,43 +152,34 @@ def publishing_failed(self): with a failed outcome.""" self.notify_publishing_failed() - def notify_publishing_completed(self, template_id: str): - """ - Notify users that envelope publishing has completed (success or failure) - for this instance. - - `template_id` should be the ID of the Notify email template of either - the successfully published or failed publishing email. - """ - - personalisation = { - "envelope_id": self.packagedworkbaskets.last().envelope.envelope_id, - } - - send_emails.delay( - template_id=template_id, - personalisation=personalisation, - email_type="publishing", - ) - @skip_notifications_if_disabled def notify_publishing_success(self): """Notify users that an envelope has successfully publishing to api.""" - self.notify_publishing_completed(settings.API_PUBLISH_SUCCESS_TEMPLATE_ID) + notification = CrownDependenciesEnvelopeSuccessNotification( + notificaiton_type=NotificationTypeChoices.PUBLISHING_SUCCESS, + notified_object_pk=self.pk, + ) + notification.save() + notification.schedule_send_emails() @skip_notifications_if_disabled def notify_publishing_failed(self): """Notify users that an envelope has failed publishing to api.""" - self.notify_publishing_completed(settings.API_PUBLISH_FAILED_TEMPLATE_ID) + notification = CrownDependenciesEnvelopeFailedNotification( + notificaiton_type=NotificationTypeChoices.PUBLISHING_FAILED, + notified_object_pk=self.pk, + ) + notification.save() + notification.schedule_send_emails() @atomic def refresh_from_db(self, using=None, fields=None): """Reload instance from database but avoid writing to self.publishing_state directly in order to avoid the exception - 'AttributeError: Direct publishing_state modification is not allowed.' - """ + 'AttributeError: Direct publishing_state modification is not + allowed.'.""" if fields is None: refresh_state = True fields = [f.name for f in self._meta.concrete_fields] diff --git a/publishing/models/packaged_workbasket.py b/publishing/models/packaged_workbasket.py index 1c7bc3cc5..72783ffb1 100644 --- a/publishing/models/packaged_workbasket.py +++ b/publishing/models/packaged_workbasket.py @@ -1,7 +1,6 @@ import logging from datetime import datetime -from django.conf import settings from django.core.exceptions import ObjectDoesNotExist from django.db.models import PROTECT from django.db.models import SET_NULL @@ -20,13 +19,15 @@ from django.db.models import Value from django.db.models.functions import Coalesce from django.db.transaction import atomic -from django.urls import reverse from django_fsm import FSMField from django_fsm import transition from common.models.mixins import TimestampedMixin +from notifications.models import EnvelopeAcceptedNotification +from notifications.models import EnvelopeReadyForProcessingNotification +from notifications.models import EnvelopeRejectedNotification from notifications.models import NotificationLog -from notifications.tasks import send_emails +from notifications.models import NotificationTypeChoices from publishing import models as publishing_models from publishing.models.decorators import save_after from publishing.models.decorators import skip_notifications_if_disabled @@ -397,7 +398,7 @@ def create_envelope_for_top(cls): def next_expected_to_api(self) -> bool: """ - checks if previous envelope in sequence has been published to the API. + Checks if previous envelope in sequence has been published to the API. This check will check if the previous packaged workbasket has a CrownDependenciesEnvelope OR has published_to_tariffs_api set in the @@ -531,8 +532,8 @@ def abandon(self): def refresh_from_db(self, using=None, fields=None): """Reload instance from database but avoid writing to self.processing_state directly in order to avoid the exception - 'AttributeError: Direct processing_state modification is not allowed.' - """ + 'AttributeError: Direct processing_state modification is not + allowed.'.""" if fields is None: refresh_state = True fields = [f.name for f in self._meta.concrete_fields] @@ -563,71 +564,34 @@ def notify_ready_for_processing(self): therefore normally called when the process for doing that has completed (see `publishing.tasks.create_xml_envelope_file()`). """ - eif = "Immediately" - if self.eif: - eif = self.eif.strftime("%d/%m/%Y") - - personalisation = { - "envelope_id": self.envelope.envelope_id, - "description": self.description, - "download_url": ( - settings.BASE_SERVICE_URL + reverse("publishing:envelope-queue-ui-list") - ), - "theme": self.theme, - "eif": eif, - "embargo": self.embargo if self.embargo else "None", - "jira_url": self.jira_url, - } - - send_emails.delay( - template_id=settings.READY_FOR_CDS_TEMPLATE_ID, - personalisation=personalisation, - email_type="packaging", - ) - - def notify_processing_completed(self, template_id): - """ - Notify users that envelope processing has completed (accepted or - rejected) for this instance. - - `template_id` should be the ID of the Notify email template of either - the successfully processed email or failed processing. - """ - loading_report_message = "Loading report: No loading report was provided." - loading_reports = self.loadingreports.exclude(file_name="").values_list( - "file_name", - flat=True, - ) - if loading_reports: - file_names = ", ".join(loading_reports) - loading_report_message = f"Loading report(s): {file_names}" - - personalisation = { - "envelope_id": self.envelope.envelope_id, - "transaction_count": self.workbasket.transactions.count(), - "loading_report_message": loading_report_message, - "comments": self.loadingreports.first().comments, - } - - send_emails.delay( - template_id=template_id, - personalisation=personalisation, - email_type="packaging", + notification = EnvelopeReadyForProcessingNotification( + notificaiton_type=NotificationTypeChoices.PACKAGING_NOTIFY_READY, + notified_object_pk=self.pk, ) + notification.save() + notification.schedule_send_emails() @skip_notifications_if_disabled def notify_processing_succeeded(self): """Notify users that envelope processing has succeeded (i.e. the associated envelope was correctly ingested into HMRC systems).""" - - self.notify_processing_completed(settings.CDS_ACCEPTED_TEMPLATE_ID) + notification = EnvelopeAcceptedNotification( + notificaiton_type=NotificationTypeChoices.PACKAGING_ACCEPTED, + notified_object_pk=self.pk, + ) + notification.save() + notification.schedule_send_emails() @skip_notifications_if_disabled def notify_processing_failed(self): """Notify users that envelope processing has failed (i.e. HMRC systems rejected this instance's associated envelope file).""" - - self.notify_processing_completed(settings.CDS_REJECTED_TEMPLATE_ID) + notification = EnvelopeRejectedNotification( + notificaiton_type=NotificationTypeChoices.PACKAGING_REJECTED, + notified_object_pk=self.pk, + ) + notification.save() + notification.schedule_send_emails() @property def cds_notified_notification_log(self) -> NotificationLog: diff --git a/sample.env b/sample.env index d08a7e967..d363eecd8 100644 --- a/sample.env +++ b/sample.env @@ -58,6 +58,7 @@ CDS_ACCEPTED_TEMPLATE_ID=cds_accepted_template_id CDS_REJECTED_TEMPLATE_ID=cds_rejected_template_id API_PUBLISH_SUCCESS_TEMPLATE_ID=api_success_template_id API_PUBLISH_FAILED_TEMPLATE_ID=api_failed_template_id +GOODS_REPORT_TEMPLATE_ID=goods_report_template_id # Base service URL. BASE_SERVICE_URL=http://localhost:8000 diff --git a/settings/common.py b/settings/common.py index 69febc37f..aa667ea4f 100644 --- a/settings/common.py +++ b/settings/common.py @@ -755,6 +755,7 @@ CDS_REJECTED_TEMPLATE_ID = os.environ.get("CDS_REJECTED_TEMPLATE_ID") API_PUBLISH_SUCCESS_TEMPLATE_ID = os.environ.get("API_PUBLISH_SUCCESS_TEMPLATE_ID") API_PUBLISH_FAILED_TEMPLATE_ID = os.environ.get("API_PUBLISH_FAILED_TEMPLATE_ID") +GOODS_REPORT_TEMPLATE_ID = os.environ.get("GOODS_REPORT_TEMPLATE_ID") # Base service URL - required when constructing an absolute TAP URL to a page # from a Celery task where no HTTP request object is available. diff --git a/workbaskets/jinja2/workbaskets/review-goods.jinja b/workbaskets/jinja2/workbaskets/review-goods.jinja index 720cbb987..c39c982ea 100644 --- a/workbaskets/jinja2/workbaskets/review-goods.jinja +++ b/workbaskets/jinja2/workbaskets/review-goods.jinja @@ -37,6 +37,15 @@ "classes": "govuk-button--secondary", }) }} {% endif %} + + {% if unsent_notification %} + {{ govukButton({ + "text": "Notify Channel Islands", + "href": url("goods-report-notify", kwargs={"pk": import_batch_pk}), + "classes": "govuk-button--primary", + "preventDoubleClick": true, + }) }} + {% endif %}

{% if report_lines %} diff --git a/workbaskets/jinja2/workbaskets/summary-workbasket.jinja b/workbaskets/jinja2/workbaskets/summary-workbasket.jinja index c7d116e22..81f15cd6d 100644 --- a/workbaskets/jinja2/workbaskets/summary-workbasket.jinja +++ b/workbaskets/jinja2/workbaskets/summary-workbasket.jinja @@ -95,9 +95,18 @@ aria-labelledby="summary-title" role="region" data-module="govuk-notification-banner">
{{ rule_check_result_content() }} - - Send to packaging queue - + {% if unsent_notification %} +
+ For commodity code imports a channel islands notification must be sent from the review goods tab before packaging. +
+ + Send to packaging queue + + {% else %} + + Send to packaging queue + + {% endif %}
{% endset %} diff --git a/workbaskets/views/ui.py b/workbaskets/views/ui.py index 9f67951ca..72bd43398 100644 --- a/workbaskets/views/ui.py +++ b/workbaskets/views/ui.py @@ -38,6 +38,8 @@ from importer.goods_report import GoodsReportLine from measures.filters import MeasureFilter from measures.models import Measure +from notifications.models import Notification +from notifications.models import NotificationTypeChoices from workbaskets import forms from workbaskets.models import WorkBasket from workbaskets.session_store import SessionStore @@ -346,6 +348,15 @@ def get_context_data(self, *args, **kwargs): ] context["import_batch_pk"] = import_batch.pk + # notifications only relevant to a goods import + context["unsent_notification"] = ( + import_batch.goods_import + and not Notification.objects.filter( + notified_object_pk=import_batch.pk, + notificaiton_type=NotificationTypeChoices.GOODS_REPORT, + ).exists() + ) + return context @@ -499,6 +510,19 @@ def get_context_data(self, **kwargs): self.request.user.is_superuser or self.request.user.has_perm("workbaskets.delete_workbasket") ) + # set to true if there is an associated goods import batch with an unsent notification + try: + import_batch = self.workbasket.importbatch + unsent_notifcation = ( + import_batch + and import_batch.goods_import + and not Notification.objects.filter( + notified_object_pk=import_batch.pk, + notificaiton_type=NotificationTypeChoices.GOODS_REPORT, + ).exists() + ) + except ObjectDoesNotExist: + unsent_notifcation = False context.update( { "workbasket": self.workbasket, @@ -506,6 +530,7 @@ def get_context_data(self, **kwargs): "uploaded_envelope_dates": self.uploaded_envelope_dates, "rule_check_in_progress": False, "user_can_delete_workbasket": user_can_delete_workbasket, + "unsent_notification": unsent_notifcation, }, ) if self.workbasket.rule_check_task_id: From 30a5d759f9c4e16e3f7c8a88ad16279d6b570ab9 Mon Sep 17 00:00:00 2001 From: Anthoni Gleeson Date: Mon, 11 Sep 2023 16:51:20 +0100 Subject: [PATCH 06/15] address type error and serialisation issues --- .../send_goods_report_notification.py | 2 +- importer/views.py | 5 +- notifications/admin.py | 9 +- ...908_0850.py => 0003_auto_20230911_0924.py} | 36 ++------ notifications/models.py | 89 ++++++++++--------- notifications/tasks.py | 10 ++- .../models/crown_dependencies_envelope.py | 4 +- publishing/models/packaged_workbasket.py | 6 +- workbaskets/views/ui.py | 4 +- 9 files changed, 75 insertions(+), 90 deletions(-) rename notifications/migrations/{0003_auto_20230908_0850.py => 0003_auto_20230911_0924.py} (71%) diff --git a/importer/management/commands/send_goods_report_notification.py b/importer/management/commands/send_goods_report_notification.py index f08a03ddc..403a20d67 100644 --- a/importer/management/commands/send_goods_report_notification.py +++ b/importer/management/commands/send_goods_report_notification.py @@ -19,7 +19,7 @@ def send_notifcation( ) notification = GoodsSuccessfulImportNotification( - notificaiton_type=NotificationTypeChoices.GOODS_REPORT, + notification_type=NotificationTypeChoices.GOODS_REPORT, notified_object_pk=import_batch.id, ) notification.save() diff --git a/importer/views.py b/importer/views.py index bdfad1855..54c875c37 100644 --- a/importer/views.py +++ b/importer/views.py @@ -254,10 +254,7 @@ def get(self, request, *args, **kwargs): # create notification notification = GoodsSuccessfulImportNotification( - notificaiton_type=( - NotificationTypeChoices.GOODS_REPORT.value, - NotificationTypeChoices.GOODS_REPORT.label, - ), + notification_type=NotificationTypeChoices.GOODS_REPORT, notified_object_pk=import_batch.id, ) notification.save() diff --git a/notifications/admin.py b/notifications/admin.py index 4ffa133f1..077f33549 100644 --- a/notifications/admin.py +++ b/notifications/admin.py @@ -72,19 +72,22 @@ def changeform_view(self, request, object_id=None, form_url="", extra_context=No class NotificationAdmin(admin.ModelAdmin): """Class providing read-only list and detail views for notification.""" + # TODO add filters on notification_type + ordering = ["pk"] list_display = ( "pk", - "notificaiton_type", + "notification_type", "notified_object_pk", - # "display_users", + "display_users", ) readonly_fields = [] def display_users(self, obj): + subclass_obj = obj.return_subclass_instance() return ", ".join( - [user.email for user in obj.notified_users().all()], + [user.email for user in subclass_obj.notified_users().all()], ) display_users.short_description = "Recipients" diff --git a/notifications/migrations/0003_auto_20230908_0850.py b/notifications/migrations/0003_auto_20230911_0924.py similarity index 71% rename from notifications/migrations/0003_auto_20230908_0850.py rename to notifications/migrations/0003_auto_20230911_0924.py index 750fe6324..523c7bbed 100644 --- a/notifications/migrations/0003_auto_20230908_0850.py +++ b/notifications/migrations/0003_auto_20230911_0924.py @@ -1,4 +1,4 @@ -# Generated by Django 3.2.20 on 2023-09-08 07:50 +# Generated by Django 3.2.20 on 2023-09-11 08:24 import django.db.models.deletion from django.db import migrations @@ -25,35 +25,17 @@ class Migration(migrations.Migration): ), ("notified_object_pk", models.IntegerField(default=None, null=True)), ( - "notificaiton_type", + "notification_type", models.CharField( choices=[ - ( - "", - "Goods Report", - ), - ( - "", - "Packaging Notify Ready", - ), - ( - "", - "Packaging Accepted", - ), - ( - "", # /PS-IGNORE - "Packaging Rejected", - ), - ( - "", - "Publishing Success", - ), - ( - "", - "Publishing Failed", - ), + ("goods_report", "Goods Report"), + ("packaging_notify_ready", "Packaging Notify Ready"), + ("packaging_accepted", "Packaging Accepted"), + ("packaging_rejected", "Packaging Rejected"), + ("publishing_success", "Publishing Successful"), + ("publishing_failed", "Publishing Failed"), ], - max_length=150, + max_length=100, ), ), ], diff --git a/notifications/models.py b/notifications/models.py index a4bfb6b0c..1bd733b23 100644 --- a/notifications/models.py +++ b/notifications/models.py @@ -49,43 +49,33 @@ class NotificationLog(TimestampedMixin): success = models.BooleanField(default=True) -class CustomNotificationChoice: - def __init__(self, value, label, template_id): - self.value = value - self.label = label - self.template_id = template_id +# class NotificationManager(models.Manager): class NotificationTypeChoices(models.TextChoices): - GOODS_REPORT = CustomNotificationChoice( + GOODS_REPORT = ( "goods_report", "Goods Report", - settings.GOODS_REPORT_TEMPLATE_ID, ) - PACKAGING_NOTIFY_READY = CustomNotificationChoice( + PACKAGING_NOTIFY_READY = ( "packaging_notify_ready", "Packaging Notify Ready", - settings.READY_FOR_CDS_TEMPLATE_ID, ) - PACKAGING_ACCEPTED = CustomNotificationChoice( + PACKAGING_ACCEPTED = ( "packaging_accepted", "Packaging Accepted", - settings.CDS_ACCEPTED_TEMPLATE_ID, ) - PACKAGING_REJECTED = CustomNotificationChoice( + PACKAGING_REJECTED = ( "packaging_rejected", "Packaging Rejected", - settings.CDS_REJECTED_TEMPLATE_ID, ) - PUBLISHING_SUCCESS = CustomNotificationChoice( + PUBLISHING_SUCCESS = ( "publishing_success", "Publishing Successful", - settings.API_PUBLISH_SUCCESS_TEMPLATE_ID, ) - PUBLISHING_FAILED = CustomNotificationChoice( + PUBLISHING_FAILED = ( "publishing_failed", "Publishing Failed", - settings.API_PUBLISH_FAILED_TEMPLATE_ID, ) @@ -110,11 +100,23 @@ class Notification(models.Model): ) """The primary key of the object being notified on.""" - notificaiton_type = models.CharField( - max_length=150, - choices=[(choice.value, choice.label) for choice in NotificationTypeChoices], + notification_type = models.CharField( + max_length=100, + choices=NotificationTypeChoices.choices, ) + def return_subclass_instance(self) -> "Notification": + subclasses = { + NotificationTypeChoices.GOODS_REPORT: GoodsSuccessfulImportNotification, + NotificationTypeChoices.PACKAGING_ACCEPTED: EnvelopeAcceptedNotification, + NotificationTypeChoices.PACKAGING_NOTIFY_READY: EnvelopeReadyForProcessingNotification, + NotificationTypeChoices.PACKAGING_REJECTED: EnvelopeRejectedNotification, + NotificationTypeChoices.PUBLISHING_SUCCESS: CrownDependenciesEnvelopeSuccessNotification, + NotificationTypeChoices.PUBLISHING_FAILED: CrownDependenciesEnvelopeFailedNotification, + } + self.__class__ = subclasses[self.notification_type] + return self + def get_personalisation(self) -> dict: """ Returns the personalisation of the notified object. @@ -149,12 +151,12 @@ def notified_object(self) -> models.Model: def synchronous_send_emails(self): """Synchronously call to send a notification email.""" - send_emails_task(self.pk, type(self)) + send_emails_task(self.pk) def schedule_send_emails(self, countdown=1): """Schedule a call to send a notification email, run as an asynchronous background task.""" - send_emails_task.apply_sync(args=[self.pk, type(self)], countdown=countdown) + send_emails_task.apply_async(args=[self.pk], countdown=countdown) def send_emails(self): """Send the notification emails to users via GOV.UK Notify.""" @@ -219,7 +221,7 @@ def get_personalisation(self) -> dict: return personalisation def notify_template_id(self) -> str: - return NotificationTypeChoices.PACKAGING_NOTIFY_READY.template_id + return settings.READY_FOR_CDS_TEMPLATE_ID def notified_users(self): return NotifiedUser.objects.filter( @@ -230,7 +232,7 @@ def notified_object(self) -> models.Model: from publishing.models import PackagedWorkBasket return ( - PackagedWorkBasket.objects.get(self.notified_object_pk) + PackagedWorkBasket.objects.get(pk=self.notified_object_pk) if self.notified_object_pk else None ) @@ -265,7 +267,7 @@ def get_personalisation(self) -> dict: return personalisation def notify_template_id(self) -> str: - return NotificationTypeChoices.PACKAGING_ACCEPTED.template_id + return settings.CDS_ACCEPTED_TEMPLATE_ID def notified_users(self): return NotifiedUser.objects.filter( @@ -311,7 +313,7 @@ def get_personalisation(self) -> dict: return personalisation def notify_template_id(self) -> str: - return NotificationTypeChoices.PACKAGING_REJECTED.template_id + return settings.CDS_REJECTED_TEMPLATE_ID def notified_users(self): return NotifiedUser.objects.filter( @@ -322,7 +324,7 @@ def notified_object(self) -> models.Model: from publishing.models import PackagedWorkBasket return ( - PackagedWorkBasket.objects.get(self.notified_object_pk) + PackagedWorkBasket.objects.get(pk=self.notified_object_pk) if self.notified_object_pk else None ) @@ -343,18 +345,18 @@ def get_personalisation(self) -> dict: return personalisation def notify_template_id(self) -> str: - return NotificationTypeChoices.PUBLISHING_SUCCESS.template_id + return settings.API_PUBLISH_SUCCESS_TEMPLATE_ID def notified_users(self): return NotifiedUser.objects.filter( - Q(enrole_api_publishing=True), + Q(enrol_api_publishing=True), ) def notified_object(self) -> models.Model: from publishing.models import CrownDependenciesEnvelope return ( - CrownDependenciesEnvelope.objects.get(self.notified_object_pk) + CrownDependenciesEnvelope.objects.get(pk=self.notified_object_pk) if self.notified_object_pk else None ) @@ -375,18 +377,18 @@ def get_personalisation(self) -> dict: return personalisation def notify_template_id(self) -> str: - return NotificationTypeChoices.PUBLISHING_FAILED.template_id + return settings.API_PUBLISH_FAILED_TEMPLATE_ID def notified_users(self): return NotifiedUser.objects.filter( - Q(enrole_api_publishing=True), + Q(enrol_api_publishing=True), ) def notified_object(self) -> models.Model: from publishing.models import CrownDependenciesEnvelope return ( - CrownDependenciesEnvelope.objects.get(self.notified_object_pk) + CrownDependenciesEnvelope.objects.get(pk=self.notified_object_pk) if self.notified_object_pk else None ) @@ -401,19 +403,17 @@ class Meta: def get_personalisation(self) -> dict: import_batch = self.notified_object() - personalisation = ( - { - "tgb_id": import_batch.name, - "link_to_file": prepare_link_to_file( - import_batch.taric_file, - confirm_email_before_download=True, - ), - }, - ) + personalisation = { + "tgb_id": import_batch.name, + "link_to_file": prepare_link_to_file( + import_batch.taric_file, + confirm_email_before_download=True, + ), + } return personalisation def notify_template_id(self) -> str: - return NotificationTypeChoices.GOODS_REPORT.template_id + return settings.GOODS_REPORT_TEMPLATE_ID def notified_users(self): return NotifiedUser.objects.filter( @@ -423,8 +423,9 @@ def notified_users(self): def notified_object(self) -> models.Model: from importer.models import ImportBatch + print(self.notified_object_pk) return ( - ImportBatch.objects.get(self.notified_object_pk) + ImportBatch.objects.get(id=self.notified_object_pk) if self.notified_object_pk else None ) diff --git a/notifications/tasks.py b/notifications/tasks.py index cc7d292f2..da3a97d6b 100644 --- a/notifications/tasks.py +++ b/notifications/tasks.py @@ -10,9 +10,11 @@ # return NotificationsAPIClient(settings.NOTIFICATIONS_API_KEY) @shared_task @atomic -def send_emails_task(notification_pk: int, notification_type: "Notification"): +def send_emails_task(notification_pk: int): """Task for emailing all users signed up to receive packaging updates and creating a log to record which users received which email template.""" - print(notification_type) - notification = notification_type.objects.get(pk=notification_pk) - notification.send_emails() + from notifications.models import Notification + + notification = Notification.objects.get(pk=notification_pk) + sub_notification = notification.return_subclass_instance() + sub_notification.send_emails() diff --git a/publishing/models/crown_dependencies_envelope.py b/publishing/models/crown_dependencies_envelope.py index 4e49577ac..fd9647586 100644 --- a/publishing/models/crown_dependencies_envelope.py +++ b/publishing/models/crown_dependencies_envelope.py @@ -157,7 +157,7 @@ def notify_publishing_success(self): """Notify users that an envelope has successfully publishing to api.""" notification = CrownDependenciesEnvelopeSuccessNotification( - notificaiton_type=NotificationTypeChoices.PUBLISHING_SUCCESS, + notification_type=NotificationTypeChoices.PUBLISHING_SUCCESS, notified_object_pk=self.pk, ) notification.save() @@ -168,7 +168,7 @@ def notify_publishing_failed(self): """Notify users that an envelope has failed publishing to api.""" notification = CrownDependenciesEnvelopeFailedNotification( - notificaiton_type=NotificationTypeChoices.PUBLISHING_FAILED, + notification_type=NotificationTypeChoices.PUBLISHING_FAILED, notified_object_pk=self.pk, ) notification.save() diff --git a/publishing/models/packaged_workbasket.py b/publishing/models/packaged_workbasket.py index 72783ffb1..21be98a9d 100644 --- a/publishing/models/packaged_workbasket.py +++ b/publishing/models/packaged_workbasket.py @@ -565,7 +565,7 @@ def notify_ready_for_processing(self): (see `publishing.tasks.create_xml_envelope_file()`). """ notification = EnvelopeReadyForProcessingNotification( - notificaiton_type=NotificationTypeChoices.PACKAGING_NOTIFY_READY, + notification_type=NotificationTypeChoices.PACKAGING_NOTIFY_READY, notified_object_pk=self.pk, ) notification.save() @@ -576,7 +576,7 @@ def notify_processing_succeeded(self): """Notify users that envelope processing has succeeded (i.e. the associated envelope was correctly ingested into HMRC systems).""" notification = EnvelopeAcceptedNotification( - notificaiton_type=NotificationTypeChoices.PACKAGING_ACCEPTED, + notification_type=NotificationTypeChoices.PACKAGING_ACCEPTED, notified_object_pk=self.pk, ) notification.save() @@ -587,7 +587,7 @@ def notify_processing_failed(self): """Notify users that envelope processing has failed (i.e. HMRC systems rejected this instance's associated envelope file).""" notification = EnvelopeRejectedNotification( - notificaiton_type=NotificationTypeChoices.PACKAGING_REJECTED, + notification_type=NotificationTypeChoices.PACKAGING_REJECTED, notified_object_pk=self.pk, ) notification.save() diff --git a/workbaskets/views/ui.py b/workbaskets/views/ui.py index 72bd43398..3e74c9d49 100644 --- a/workbaskets/views/ui.py +++ b/workbaskets/views/ui.py @@ -353,7 +353,7 @@ def get_context_data(self, *args, **kwargs): import_batch.goods_import and not Notification.objects.filter( notified_object_pk=import_batch.pk, - notificaiton_type=NotificationTypeChoices.GOODS_REPORT, + notification_type=NotificationTypeChoices.GOODS_REPORT, ).exists() ) @@ -518,7 +518,7 @@ def get_context_data(self, **kwargs): and import_batch.goods_import and not Notification.objects.filter( notified_object_pk=import_batch.pk, - notificaiton_type=NotificationTypeChoices.GOODS_REPORT, + notification_type=NotificationTypeChoices.GOODS_REPORT, ).exists() ) except ObjectDoesNotExist: From 1b0ea79c116df5e5070bb29dd9955453e50d2d57 Mon Sep 17 00:00:00 2001 From: Anthoni Gleeson Date: Mon, 11 Sep 2023 16:52:41 +0100 Subject: [PATCH 07/15] remove comment add todo --- notifications/notify.py | 1 + notifications/tasks.py | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/notifications/notify.py b/notifications/notify.py index 643181268..5c0f9f95e 100644 --- a/notifications/notify.py +++ b/notifications/notify.py @@ -74,6 +74,7 @@ def send_emails(template_id, personalisation: dict, email_addresses: List[str]) response_ids += f"{response['id']} \n" recipients += f"{email} \n" except Exception as e: + # TODO extract error info? failed_recipients += f"{email} \n" logger.error( f"Failed to send email notification to {email}, with status code {e.status_code}.", diff --git a/notifications/tasks.py b/notifications/tasks.py index da3a97d6b..425bfdfa5 100644 --- a/notifications/tasks.py +++ b/notifications/tasks.py @@ -6,8 +6,6 @@ logger = logging.getLogger(__name__) -# def get_notifications_client(): -# return NotificationsAPIClient(settings.NOTIFICATIONS_API_KEY) @shared_task @atomic def send_emails_task(notification_pk: int): From ddd73345408dff7f5869c8aa99e8df6be5c49d80 Mon Sep 17 00:00:00 2001 From: Anthoni Gleeson Date: Mon, 11 Sep 2023 17:30:00 +0100 Subject: [PATCH 08/15] Refactor notification inits and fixed email list --- .../send_goods_report_notification.py | 2 - importer/views.py | 2 - notifications/models.py | 39 ++++++++++++++++++- .../models/crown_dependencies_envelope.py | 3 -- publishing/models/packaged_workbasket.py | 4 -- 5 files changed, 37 insertions(+), 13 deletions(-) diff --git a/importer/management/commands/send_goods_report_notification.py b/importer/management/commands/send_goods_report_notification.py index 403a20d67..8db3368eb 100644 --- a/importer/management/commands/send_goods_report_notification.py +++ b/importer/management/commands/send_goods_report_notification.py @@ -3,7 +3,6 @@ from importer.models import ImportBatch from notifications.models import GoodsSuccessfulImportNotification -from notifications.models import NotificationTypeChoices def send_notifcation( @@ -19,7 +18,6 @@ def send_notifcation( ) notification = GoodsSuccessfulImportNotification( - notification_type=NotificationTypeChoices.GOODS_REPORT, notified_object_pk=import_batch.id, ) notification.save() diff --git a/importer/views.py b/importer/views.py index 54c875c37..78f8e9c9e 100644 --- a/importer/views.py +++ b/importer/views.py @@ -20,7 +20,6 @@ from importer.goods_report import GoodsReporter from importer.models import ImportBatchStatus from notifications.models import GoodsSuccessfulImportNotification -from notifications.models import NotificationTypeChoices from workbaskets.validators import WorkflowStatus @@ -254,7 +253,6 @@ def get(self, request, *args, **kwargs): # create notification notification = GoodsSuccessfulImportNotification( - notification_type=NotificationTypeChoices.GOODS_REPORT, notified_object_pk=import_batch.id, ) notification.save() diff --git a/notifications/models.py b/notifications/models.py index 1bd733b23..6d142661a 100644 --- a/notifications/models.py +++ b/notifications/models.py @@ -174,7 +174,7 @@ def send_emails(self): result = send_emails( self.notify_template_id(), personalisation, - notified_users, + [user.email for user in notified_users], ) NotificationLog.objects.create( @@ -199,6 +199,12 @@ class EnvelopeReadyForProcessingNotification(Notification): class Meta: proxy = True + def __init__(self, notified_object_pk: int): + super(EnvelopeReadyForProcessingNotification, self).__init__( + notified_object_pk=notified_object_pk, + notification_type=NotificationTypeChoices.PACKAGING_NOTIFY_READY, + ) + def get_personalisation(self) -> dict: packaged_workbasket = self.notified_object() eif = "Immediately" @@ -245,6 +251,12 @@ class EnvelopeAcceptedNotification(Notification): class Meta: proxy = True + def __init__(self, notified_object_pk: int): + super(EnvelopeAcceptedNotification, self).__init__( + notified_object_pk=notified_object_pk, + notification_type=NotificationTypeChoices.PACKAGING_ACCEPTED, + ) + def get_personalisation(self) -> dict: packaged_workbasket = self.notified_object() loading_report_message = "Loading report: No loading report was provided." @@ -291,6 +303,12 @@ class EnvelopeRejectedNotification(Notification): class Meta: proxy = True + def __init__(self, notified_object_pk: int): + super(EnvelopeRejectedNotification, self).__init__( + notified_object_pk=notified_object_pk, + notification_type=NotificationTypeChoices.PACKAGING_REJECTED, + ) + def get_personalisation(self) -> dict: packaged_workbasket = self.notified_object() loading_report_message = "Loading report: No loading report was provided." @@ -337,6 +355,12 @@ class CrownDependenciesEnvelopeSuccessNotification(Notification): class Meta: proxy = True + def __init__(self, notified_object_pk: int): + super(CrownDependenciesEnvelopeSuccessNotification, self).__init__( + notified_object_pk=notified_object_pk, + notification_type=NotificationTypeChoices.PUBLISHING_SUCCESS, + ) + def get_personalisation(self) -> dict: crown_dependicies_envelope = self.notified_object() personalisation = { @@ -369,6 +393,12 @@ class CrownDependenciesEnvelopeFailedNotification(Notification): class Meta: proxy = True + def __init__(self, notified_object_pk: int): + super(CrownDependenciesEnvelopeFailedNotification, self).__init__( + notified_object_pk=notified_object_pk, + notification_type=NotificationTypeChoices.PUBLISHING_FAILED, + ) + def get_personalisation(self) -> dict: self.notified_object() personalisation = { @@ -401,6 +431,12 @@ class GoodsSuccessfulImportNotification(Notification): class Meta: proxy = True + def __init__(self, notified_object_pk: int): + super(GoodsSuccessfulImportNotification, self).__init__( + notified_object_pk=notified_object_pk, + notification_type=NotificationTypeChoices.GOODS_REPORT, + ) + def get_personalisation(self) -> dict: import_batch = self.notified_object() personalisation = { @@ -423,7 +459,6 @@ def notified_users(self): def notified_object(self) -> models.Model: from importer.models import ImportBatch - print(self.notified_object_pk) return ( ImportBatch.objects.get(id=self.notified_object_pk) if self.notified_object_pk diff --git a/publishing/models/crown_dependencies_envelope.py b/publishing/models/crown_dependencies_envelope.py index fd9647586..aab4640c7 100644 --- a/publishing/models/crown_dependencies_envelope.py +++ b/publishing/models/crown_dependencies_envelope.py @@ -11,7 +11,6 @@ from common.models.mixins import TimestampedMixin from notifications.models import CrownDependenciesEnvelopeFailedNotification from notifications.models import CrownDependenciesEnvelopeSuccessNotification -from notifications.models import NotificationTypeChoices from publishing.models.decorators import save_after from publishing.models.decorators import skip_notifications_if_disabled from publishing.models.packaged_workbasket import PackagedWorkBasket @@ -157,7 +156,6 @@ def notify_publishing_success(self): """Notify users that an envelope has successfully publishing to api.""" notification = CrownDependenciesEnvelopeSuccessNotification( - notification_type=NotificationTypeChoices.PUBLISHING_SUCCESS, notified_object_pk=self.pk, ) notification.save() @@ -168,7 +166,6 @@ def notify_publishing_failed(self): """Notify users that an envelope has failed publishing to api.""" notification = CrownDependenciesEnvelopeFailedNotification( - notification_type=NotificationTypeChoices.PUBLISHING_FAILED, notified_object_pk=self.pk, ) notification.save() diff --git a/publishing/models/packaged_workbasket.py b/publishing/models/packaged_workbasket.py index 21be98a9d..d6df65b07 100644 --- a/publishing/models/packaged_workbasket.py +++ b/publishing/models/packaged_workbasket.py @@ -27,7 +27,6 @@ from notifications.models import EnvelopeReadyForProcessingNotification from notifications.models import EnvelopeRejectedNotification from notifications.models import NotificationLog -from notifications.models import NotificationTypeChoices from publishing import models as publishing_models from publishing.models.decorators import save_after from publishing.models.decorators import skip_notifications_if_disabled @@ -565,7 +564,6 @@ def notify_ready_for_processing(self): (see `publishing.tasks.create_xml_envelope_file()`). """ notification = EnvelopeReadyForProcessingNotification( - notification_type=NotificationTypeChoices.PACKAGING_NOTIFY_READY, notified_object_pk=self.pk, ) notification.save() @@ -576,7 +574,6 @@ def notify_processing_succeeded(self): """Notify users that envelope processing has succeeded (i.e. the associated envelope was correctly ingested into HMRC systems).""" notification = EnvelopeAcceptedNotification( - notification_type=NotificationTypeChoices.PACKAGING_ACCEPTED, notified_object_pk=self.pk, ) notification.save() @@ -587,7 +584,6 @@ def notify_processing_failed(self): """Notify users that envelope processing has failed (i.e. HMRC systems rejected this instance's associated envelope file).""" notification = EnvelopeRejectedNotification( - notification_type=NotificationTypeChoices.PACKAGING_REJECTED, notified_object_pk=self.pk, ) notification.save() From 052db89bafc50785076452506cb88acb9b4713dd Mon Sep 17 00:00:00 2001 From: Anthoni Gleeson Date: Wed, 13 Sep 2023 08:15:59 +0100 Subject: [PATCH 09/15] tests, mocks broke & goods report not working --- common/tests/factories.py | 31 +++ conftest.py | 136 +++++++++++++ .../test_send_goods_report_notification.py | 52 +++++ importer/tests/test_views.py | 27 +++ notifications/tasks.py | 1 + notifications/tests/conftest.py | 81 ++++++++ notifications/tests/test_models.py | 76 ++++++++ notifications/tests/test_tasks.py | 116 ++++++----- publishing/tests/conftest.py | 126 ------------ publishing/tests/test_envelope_queue_views.py | 4 +- .../test_model_crown_dependencies_envelope.py | 26 +-- publishing/tests/test_models.py | 27 +-- workbaskets/tests/test_views.py | 182 ++++++++++++++++++ 13 files changed, 658 insertions(+), 227 deletions(-) create mode 100644 importer/tests/management/commands/test_send_goods_report_notification.py create mode 100644 notifications/tests/conftest.py create mode 100644 notifications/tests/test_models.py diff --git a/common/tests/factories.py b/common/tests/factories.py index 278cf4b3d..febc7c3f7 100644 --- a/common/tests/factories.py +++ b/common/tests/factories.py @@ -1404,3 +1404,34 @@ class CrownDependenciesPublishingOperationalStatusFactory( class Meta: model = "publishing.CrownDependenciesPublishingOperationalStatus" + + +class SucceededImportBatchFactory(ImportBatchFactory): + status = ImportBatchStatus.SUCCEEDED + goods_import = True + taric_file = "goods.xml" + + +class GoodsSuccessfulImportNotificationFactory(factory.django.DjangoModelFactory): + """This is a factory for a goods report notification, requires an import id + passed by notified_object_id.""" + + class Meta: + model = "notifications.GoodsSuccessfulImportNotification" + + +class EnvelopeReadyForProcessingNotificationFactory(factory.django.DjangoModelFactory): + """This is a factory for an envelope ready for processing notificaiton.""" + + class Meta: + model = "notifications.EnvelopeReadyForProcessingNotification" + + +class CrownDependenciesEnvelopeSuccessNotificationFactory( + factory.django.DjangoModelFactory, +): + """This is a factory for a crown dependencies envelope success + notification.""" + + class Meta: + model = "notifications.CrownDependenciesEnvelopeSuccessNotification" diff --git a/conftest.py b/conftest.py index 0f69c106b..0ed5a2f27 100644 --- a/conftest.py +++ b/conftest.py @@ -9,6 +9,7 @@ from typing import Optional from typing import Sequence from typing import Type +from unittest.mock import MagicMock from unittest.mock import patch import boto3 @@ -57,6 +58,7 @@ from importer.models import ImportBatchStatus from importer.nursery import get_nursery from importer.taric import process_taric_xml_stream +from publishing.models import PackagedWorkBasket from workbaskets.models import WorkBasket from workbaskets.models import get_partition_scheme from workbaskets.validators import WorkflowStatus @@ -1547,3 +1549,137 @@ def empty_goods_import_batch(): status=ImportBatchStatus.SUCCEEDED, workbasket_id=archived_workbasket.id, ) + + +@pytest.fixture() +def mocked_send_emails_apply_async(): + with patch( + "notifications.tasks.send_emails_task.apply_async", + return_value=MagicMock(id=factory.Faker("uuid4")), + ) as mocked_delay: + yield mocked_delay + + +@pytest.fixture() +def mocked_send_emails(): + with patch( + "notifications.tasks.send_emails_task", + return_value=MagicMock(id=factory.Faker("uuid4")), + ) as mocked_delay: + yield mocked_delay + + +@pytest.fixture(scope="function") +def packaged_workbasket_factory(queued_workbasket_factory): + """ + Factory fixture to create a packaged workbasket. + + params: + workbasket defaults to queued_workbasket_factory() which creates a + Workbasket in the state QUEUED with an approved transaction and tracked models + """ + + def factory_method(workbasket=None, **kwargs): + if not workbasket: + workbasket = queued_workbasket_factory() + with patch( + "publishing.tasks.create_xml_envelope_file.apply_async", + return_value=MagicMock(id=factory.Faker("uuid4")), + ): + packaged_workbasket = factories.QueuedPackagedWorkBasketFactory( + workbasket=workbasket, **kwargs + ) + return packaged_workbasket + + return factory_method + + +@pytest.fixture(scope="function") +def published_envelope_factory(packaged_workbasket_factory, envelope_storage): + """ + Factory fixture to create an envelope and update the packaged_workbasket + envelope field. + + params: + packaged_workbasket defaults to packaged_workbasket_factory() which creates a + Packaged workbasket with a Workbasket in the state QUEUED + with an approved transaction and tracked models + """ + + def factory_method(packaged_workbasket=None, **kwargs): + if not packaged_workbasket: + packaged_workbasket = packaged_workbasket_factory() + + with patch( + "publishing.storages.EnvelopeStorage.save", + wraps=MagicMock(side_effect=envelope_storage.save), + ) as mock_save: + envelope = factories.PublishedEnvelopeFactory( + packaged_work_basket=packaged_workbasket, + **kwargs, + ) + mock_save.assert_called_once() + + packaged_workbasket.envelope = envelope + packaged_workbasket.save() + return envelope + + return factory_method + + +@pytest.fixture(scope="function") +def successful_envelope_factory(published_envelope_factory): + """ + Factory fixture to create a successfully processed envelope and update the + packaged_workbasket envelope field. + + params: + packaged_workbasket defaults to packaged_workbasket_factory() which creates a + Packaged workbasket with a Workbasket in the state QUEUED + with an approved transaction and tracked models + """ + + def factory_method(**kwargs): + envelope = published_envelope_factory(**kwargs) + + packaged_workbasket = PackagedWorkBasket.objects.get( + envelope=envelope, + ) + + packaged_workbasket.begin_processing() + assert packaged_workbasket.position == 0 + assert ( + packaged_workbasket.pk + == PackagedWorkBasket.objects.currently_processing().pk + ) + factories.LoadingReportFactory.create(packaged_workbasket=packaged_workbasket) + packaged_workbasket.processing_succeeded() + packaged_workbasket.save() + assert packaged_workbasket.position == 0 + return envelope + + return factory_method + + +@pytest.fixture(scope="function") +def crown_dependencies_envelope_factory(successful_envelope_factory): + """ + Factory fixture to create a crown dependencies envelope. + + params: + packaged_workbasket defaults to packaged_workbasket_factory() which creates a + Packaged workbasket with a Workbasket in the state QUEUED + with an approved transaction and tracked models + """ + + def factory_method(**kwargs): + envelope = successful_envelope_factory(**kwargs) + + packaged_workbasket = PackagedWorkBasket.objects.get( + envelope=envelope, + ) + return factories.CrownDependenciesEnvelopeFactory( + packaged_work_basket=packaged_workbasket, + ) + + return factory_method diff --git a/importer/tests/management/commands/test_send_goods_report_notification.py b/importer/tests/management/commands/test_send_goods_report_notification.py new file mode 100644 index 000000000..e6364f197 --- /dev/null +++ b/importer/tests/management/commands/test_send_goods_report_notification.py @@ -0,0 +1,52 @@ +from unittest.mock import MagicMock +from unittest.mock import patch + +import pytest +from django.core.management import call_command +from django.core.management.base import CommandError + +pytestmark = pytest.mark.django_db + + +@pytest.mark.parametrize( + "args, exception_type, error_msg", + [ + ( + [""], + CommandError, + "Error: unrecognized arguments:", + ), + ( + ["--import-batch-id", "1234"], + CommandError, + "No ImportBatch instance found with pk=1234", + ), + ], +) +def test_send_goods_report_notification_required_arguments( + args, + exception_type, + error_msg, +): + """Test that `send_goods_report_notification` command raises errors when + invalid arguments are provided.""" + with pytest.raises(exception_type, match=error_msg): + call_command("send_goods_report_notification", *args) + + +def test_send_goods_report_notification( + completed_goods_import_batch, +): + """Test that `send_goods_report_notification` command triggers an email + notification.""" + + with patch( + "notifications.tasks.send_emails_task", + return_value=MagicMock(), + ) as mocked_email_task: + call_command( + "send_goods_report_notification", + "--import-batch-id", + str(completed_goods_import_batch.id), + ) + mocked_email_task.assert_called_once() diff --git a/importer/tests/test_views.py b/importer/tests/test_views.py index 31fdcd9ba..1ecb4f301 100644 --- a/importer/tests/test_views.py +++ b/importer/tests/test_views.py @@ -1,4 +1,5 @@ from os import path +from unittest.mock import MagicMock from unittest.mock import patch import pytest @@ -282,3 +283,29 @@ def test_import_list_filters_return_correct_imports( assert len(page.find_all(class_="status-badge")) == 1 assert page.find(class_="status-badge", text=expected_status_text) assert page.find("tbody").find("td", text=import_batch.name) + + +def test_notify_channel_islands_redirects( + valid_user_client, + completed_goods_import_batch, +): + """Tests that, when the notify button is clicked that it redirects to the + conformation page on successful notification trigger.""" + + with patch( + "notifications.tasks.send_emails_task", + return_value=MagicMock(), + ) as mocked_email_task: + response = valid_user_client.get( + reverse( + "goods-report-notify", + kwargs={"pk": completed_goods_import_batch.pk}, + ), + ) + + mocked_email_task.assert_called_once() + assert response.status_code == 302 + assert response.url == reverse( + "goods-report-notify-success", + kwargs={"pk": completed_goods_import_batch.pk}, + ) diff --git a/notifications/tasks.py b/notifications/tasks.py index 425bfdfa5..88f622d37 100644 --- a/notifications/tasks.py +++ b/notifications/tasks.py @@ -13,6 +13,7 @@ def send_emails_task(notification_pk: int): creating a log to record which users received which email template.""" from notifications.models import Notification + print("in here") notification = Notification.objects.get(pk=notification_pk) sub_notification = notification.return_subclass_instance() sub_notification.send_emails() diff --git a/notifications/tests/conftest.py b/notifications/tests/conftest.py new file mode 100644 index 000000000..86fa305e6 --- /dev/null +++ b/notifications/tests/conftest.py @@ -0,0 +1,81 @@ +from unittest.mock import patch + +import pytest + +from common.tests import factories +from importer.models import ImportBatchStatus + + +@pytest.fixture() +def goods_report_notification(): + factories.NotifiedUserFactory( + email="goods_report@email.co.uk", # /PS-IGNORE + enrol_packaging=False, + enrol_goods_report=True, + ) + factories.NotifiedUserFactory( + email="no_goods_report@email.co.uk", # /PS-IGNORE + ) + import_batch = factories.ImportBatchFactory.create( + status=ImportBatchStatus.SUCCEEDED, + goods_import=True, + taric_file="goods.xml", + ) + + return factories.GoodsSuccessfulImportNotificationFactory( + notified_object_pk=import_batch.id, + ) + + +@pytest.fixture() +def ready_for_packaging_notification(published_envelope_factory): + factories.NotifiedUserFactory( + email="packaging@email.co.uk", # /PS-IGNORE + ) + factories.NotifiedUserFactory( + email="no_packaging@email.co.uk", # /PS-IGNORE + enrol_packaging=False, + ) + packaged_wb = published_envelope_factory() + return factories.EnvelopeReadyForProcessingNotificationFactory( + notified_object_pk=packaged_wb.id, + ) + + +@pytest.fixture() +def successful_publishing_notification(crown_dependencies_envelope_factory): + factories.NotifiedUserFactory( + email="publishing@email.co.uk", # /PS-IGNORE + enrol_packaging=False, + enrol_api_publishing=True, + ) + factories.NotifiedUserFactory( + email="no_publishing@email.co.uk", # /PS-IGNORE + ) + cde = crown_dependencies_envelope_factory() + return factories.CrownDependenciesEnvelopeSuccessNotificationFactory( + notified_object_pk=cde.id, + ) + + +@pytest.fixture() +def mock_notify_send_emails(): + with patch( + "notifications.notify.send_emails", + ) as mocked_send_emails: + yield mocked_send_emails + + +@pytest.fixture() +def mock_prepare_link(): + return_value = { + "file": "VGVzdA==", + "is_csv": False, + "confirm_email_before_download": True, + "retention_period": None, + } + with patch( + "notifications.notify.prepare_link_to_file", + return_value=return_value, + ) as mocked_prepare_link_to_file: + yield mocked_prepare_link_to_file diff --git a/notifications/tests/test_models.py b/notifications/tests/test_models.py new file mode 100644 index 000000000..a318c5f44 --- /dev/null +++ b/notifications/tests/test_models.py @@ -0,0 +1,76 @@ +import pytest + +from importer.models import ImportBatch +from publishing.models import CrownDependenciesEnvelope +from publishing.models import PackagedWorkBasket + +pytestmark = pytest.mark.django_db + + +def test_create_goods_report_notification(goods_report_notification): + """Test that the creating a notification correctly assigns users.""" + + expected_present_email = f"goods_report@email.co.uk" # /PS-IGNORE + expected_not_present_email = f"no_goods_report@email.co.uk" # /PS-IGNORE + + users = goods_report_notification.notified_users() + + for user in users: + assert user.email == expected_present_email + assert user.email != expected_not_present_email + + assert isinstance(goods_report_notification.notified_object(), ImportBatch) + # TODO check goods_report_notification.get_personalisation() + + +def test_create_packaging_notification(ready_for_packaging_notification): + """Test that the creating a notification correctly assigns users.""" + + expected_present_email = f"packaging@email.co.uk" # /PS-IGNORE + expected_not_present_email = f"no_packaging@email.co.uk" # /PS-IGNORE + + users = ready_for_packaging_notification.notified_users() + + for user in users: + assert user.email == expected_present_email + assert user.email != expected_not_present_email + + assert isinstance( + ready_for_packaging_notification.notified_object(), + PackagedWorkBasket, + ) + + content = ready_for_packaging_notification.get_personalisation() + assert content == { + "envelope_id": "230001", + "description": "", + "download_url": "http://localhost/publishing/envelope-queue/", + "theme": "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", + "eif": "Immediately", + "embargo": "None", + "jira_url": "www.fakejiraticket.com", + } + + +def test_create_successful_publishing_notification(successful_publishing_notification): + """Test that the creating a notification correctly assigns users.""" + + expected_present_email = f"publishing@email.co.uk" # /PS-IGNORE + expected_not_present_email = f"no_publishing@email.co.uk" # /PS-IGNORE + + users = successful_publishing_notification.notified_users() + + for user in users: + assert user.email == expected_present_email + assert user.email != expected_not_present_email + + assert isinstance( + successful_publishing_notification.notified_object(), + CrownDependenciesEnvelope, + ) + + content = successful_publishing_notification.get_personalisation() + assert content == {"envelope_id": "230001"} + + +# TODO add test send_emails diff --git a/notifications/tests/test_tasks.py b/notifications/tests/test_tasks.py index b824cf6fc..53902b90b 100644 --- a/notifications/tests/test_tasks.py +++ b/notifications/tests/test_tasks.py @@ -1,93 +1,85 @@ -from unittest.mock import patch -from uuid import uuid4 - import pytest -from common.tests import factories from notifications import models from notifications import tasks pytestmark = pytest.mark.django_db -@pytest.mark.skip(reason="TODO mock Notify client correctly") -@patch("notifications.tasks.NotificationsAPIClient.send_email_notification") -def test_send_emails_cds(send_email_notification, settings): - """Tests that email notifications are only sent to users subscribed to - packaging emails and that a log is created with this user's email and - template id.""" - enrolled_user = factories.NotifiedUserFactory.create() - unenrolled_user = factories.NotifiedUserFactory.create(enrol_packaging=False) - template_id = uuid4() - - personalisation = { - "envelope_id": "220001", - "description": "description", - "download_url": "https://example.com", - "theme": "theme", - "eif": None, - "embargo": "None", - "jira_url": "https://example.com", - } - - tasks.send_emails.apply( +def test_send_emails_goods_report_notification( + goods_report_notification, + mock_notify_send_emails, +): + """Tests that email notifications are only sent to users subscribed to email + type and that a log is created with this user's email.""" + expected_present_email = "goods_report@email.co.uk" # /PS-IGNORE + expected_unenrolled_email = "no_goods_report@email.co.uk" # /PS-IGNORE + + tasks.send_emails_task.apply( kwargs={ - "template_id": template_id, - "personalisation": personalisation, - "email_type": "packaging", + "notification_pk": goods_report_notification.id, }, ) - send_email_notification.assert_called_once_with( - email_address=enrolled_user.email, - template_id=template_id, - personalisation=personalisation, - ) + recipients = f"{expected_present_email} \n" - recipients = f"{enrolled_user.email} \n" log = models.NotificationLog.objects.get( - template_id=template_id, + notification=goods_report_notification, recipients=recipients, + success=True, ) - assert unenrolled_user.email not in log.recipients + assert expected_unenrolled_email not in log.recipients -@pytest.mark.skip(reason="TODO mock Notify client correctly") -@patch("notifications.tasks.NotificationsAPIClient.send_email_notification") -def test_send_emails_api(send_email_notification, settings): - """Tests that email notifications are only sent to users subscribed to - packaging emails and that a log is created with this user's email and - template id.""" - enrolled_user = factories.NotifiedUserFactory.create( - enrol_packaging=False, - enrol_api_publishing=True, - ) - unenrolled_user = factories.NotifiedUserFactory.create(enrol_packaging=False) - template_id = uuid4() +def test_send_emails_packaging_notification( + ready_for_packaging_notification, + mock_notify_send_emails, +): + """Tests that email notifications are only sent to users subscribed to email + type and that a log is created with this user's email.""" - personalisation = { - "envelope_id": "220001", - } + expected_present_email = "packaging@email.co.uk" # /PS-IGNORE + expected_unenrolled_email = "no_packaging@email.co.uk" # /PS-IGNORE - tasks.send_emails.apply( + tasks.send_emails_task.apply( kwargs={ - "template_id": template_id, - "personalisation": personalisation, - "email_type": "publishing", + "notification_pk": ready_for_packaging_notification.id, }, ) + recipients = f"{expected_present_email} \n" - send_email_notification.assert_called_once_with( - email_address=enrolled_user.email, - template_id=template_id, - personalisation=personalisation, + log = models.NotificationLog.objects.get( + notification=ready_for_packaging_notification, + recipients=recipients, + success=True, ) - recipients = f"{enrolled_user.email} \n" + assert expected_unenrolled_email not in log.recipients + + +def test_send_emails_publishing_notification( + successful_publishing_notification, + mock_notify_send_emails, +): + """Tests that email notifications are only sent to users subscribed to email + type and that a log is created with this user's email.""" + + expected_present_email = "publishing@email.co.uk" # /PS-IGNORE + expected_unenrolled_email = "no_publishing@email.co.uk" # /PS-IGNORE + + tasks.send_emails_task.apply( + kwargs={ + "notification_pk": successful_publishing_notification.id, + }, + ) + + recipients = f"{expected_present_email} \n" + log = models.NotificationLog.objects.get( - template_id=template_id, + notification=successful_publishing_notification, recipients=recipients, + success=True, ) - assert unenrolled_user.email not in log.recipients + assert expected_unenrolled_email not in log.recipients diff --git a/publishing/tests/conftest.py b/publishing/tests/conftest.py index 6ee53dbc8..8b49dee1f 100644 --- a/publishing/tests/conftest.py +++ b/publishing/tests/conftest.py @@ -13,7 +13,6 @@ import pytest from common.tests import factories -from publishing.models import PackagedWorkBasket from publishing.models import QueueState from publishing.models.state import CrownDependenciesPublishingState @@ -50,15 +49,6 @@ def unpause_publishing(): ) -@pytest.fixture() -def mocked_publishing_models_send_emails_delay(): - with patch( - "notifications.tasks.send_emails.delay", - return_value=MagicMock(id=factory.Faker("uuid4")), - ) as mocked_delay: - yield mocked_delay - - @pytest.fixture(scope="module", autouse=True) def mocked_create_xml_envelope_file_apply_sync(): """Mock the Celery delay() function on @@ -70,119 +60,3 @@ def mocked_create_xml_envelope_file_apply_sync(): return_value=MagicMock(id=factory.Faker("uuid4")), ) as mocked_apply_sync: yield mocked_apply_sync - - -@pytest.fixture(scope="function") -def packaged_workbasket_factory(queued_workbasket_factory): - """ - Factory fixture to create a packaged workbasket. - - params: - workbasket defaults to queued_workbasket_factory() which creates a - Workbasket in the state QUEUED with an approved transaction and tracked models - """ - - def factory_method(workbasket=None, **kwargs): - if not workbasket: - workbasket = queued_workbasket_factory() - with patch( - "publishing.tasks.create_xml_envelope_file.apply_async", - return_value=MagicMock(id=factory.Faker("uuid4")), - ): - packaged_workbasket = factories.QueuedPackagedWorkBasketFactory( - workbasket=workbasket, **kwargs - ) - return packaged_workbasket - - return factory_method - - -@pytest.fixture(scope="function") -def published_envelope_factory(packaged_workbasket_factory, envelope_storage): - """ - Factory fixture to create an envelope and update the packaged_workbasket - envelope field. - - params: - packaged_workbasket defaults to packaged_workbasket_factory() which creates a - Packaged workbasket with a Workbasket in the state QUEUED - with an approved transaction and tracked models - """ - - def factory_method(packaged_workbasket=None, **kwargs): - if not packaged_workbasket: - packaged_workbasket = packaged_workbasket_factory() - - with patch( - "publishing.storages.EnvelopeStorage.save", - wraps=MagicMock(side_effect=envelope_storage.save), - ) as mock_save: - envelope = factories.PublishedEnvelopeFactory( - packaged_work_basket=packaged_workbasket, - **kwargs, - ) - mock_save.assert_called_once() - - packaged_workbasket.envelope = envelope - packaged_workbasket.save() - return envelope - - return factory_method - - -@pytest.fixture(scope="function") -def successful_envelope_factory(published_envelope_factory): - """ - Factory fixture to create a successfully processed envelope and update the - packaged_workbasket envelope field. - - params: - packaged_workbasket defaults to packaged_workbasket_factory() which creates a - Packaged workbasket with a Workbasket in the state QUEUED - with an approved transaction and tracked models - """ - - def factory_method(**kwargs): - envelope = published_envelope_factory(**kwargs) - - packaged_workbasket = PackagedWorkBasket.objects.get( - envelope=envelope, - ) - - packaged_workbasket.begin_processing() - assert packaged_workbasket.position == 0 - assert ( - packaged_workbasket.pk - == PackagedWorkBasket.objects.currently_processing().pk - ) - factories.LoadingReportFactory.create(packaged_workbasket=packaged_workbasket) - packaged_workbasket.processing_succeeded() - packaged_workbasket.save() - assert packaged_workbasket.position == 0 - return envelope - - return factory_method - - -@pytest.fixture(scope="function") -def crown_dependencies_envelope_factory(successful_envelope_factory): - """ - Factory fixture to create a crown dependencies envelope. - - params: - packaged_workbasket defaults to packaged_workbasket_factory() which creates a - Packaged workbasket with a Workbasket in the state QUEUED - with an approved transaction and tracked models - """ - - def factory_method(**kwargs): - envelope = successful_envelope_factory(**kwargs) - - packaged_workbasket = PackagedWorkBasket.objects.get( - envelope=envelope, - ) - return factories.CrownDependenciesEnvelopeFactory( - packaged_work_basket=packaged_workbasket, - ) - - return factory_method diff --git a/publishing/tests/test_envelope_queue_views.py b/publishing/tests/test_envelope_queue_views.py index 35b10f53c..c88ab3c03 100644 --- a/publishing/tests/test_envelope_queue_views.py +++ b/publishing/tests/test_envelope_queue_views.py @@ -146,7 +146,7 @@ def test_start_processing(valid_user_client, unpause_queue): def test_accept_envelope( packaged_workbasket_factory, published_envelope_factory, - mocked_publishing_models_send_emails_delay, + mocked_send_emails_apply_async, valid_user_client, ): packaged_work_basket = packaged_workbasket_factory() @@ -183,7 +183,7 @@ def test_accept_envelope( def test_reject_envelope( packaged_workbasket_factory, published_envelope_factory, - mocked_publishing_models_send_emails_delay, + mocked_send_emails_apply_async, superuser_client, ): packaged_work_basket_1 = packaged_workbasket_factory() diff --git a/publishing/tests/test_model_crown_dependencies_envelope.py b/publishing/tests/test_model_crown_dependencies_envelope.py index e25f92543..d9ed71d34 100644 --- a/publishing/tests/test_model_crown_dependencies_envelope.py +++ b/publishing/tests/test_model_crown_dependencies_envelope.py @@ -1,6 +1,6 @@ import pytest -from django.conf import settings +from notifications.models import Notification from publishing.models import ApiPublishingState from publishing.models import CrownDependenciesEnvelope @@ -32,7 +32,7 @@ def test_create_crown_dependencies_envelope( def test_notify_processing_succeeded( - mocked_publishing_models_send_emails_delay, + mocked_send_emails_apply_async, packaged_workbasket_factory, successful_envelope_factory, crown_dependencies_envelope_factory, @@ -45,18 +45,14 @@ def test_notify_processing_succeeded( cd_envelope.notify_publishing_success() - personalisation = { - "envelope_id": pwb.envelope.envelope_id, - } - mocked_publishing_models_send_emails_delay.assert_called_with( - template_id=settings.API_PUBLISH_SUCCESS_TEMPLATE_ID, - personalisation=personalisation, - email_type="publishing", + notification = Notification.objects.last() + mocked_send_emails_apply_async.assert_called_with( + notification_id=notification.id, ) def test_notify_processing_failed( - mocked_publishing_models_send_emails_delay, + mocked_send_emails_apply_async, packaged_workbasket_factory, successful_envelope_factory, crown_dependencies_envelope_factory, @@ -69,11 +65,7 @@ def test_notify_processing_failed( cd_envelope.notify_publishing_failed() - personalisation = { - "envelope_id": pwb.envelope.envelope_id, - } - mocked_publishing_models_send_emails_delay.assert_called_with( - template_id=settings.API_PUBLISH_FAILED_TEMPLATE_ID, - personalisation=personalisation, - email_type="publishing", + notification = Notification.objects.last() + mocked_send_emails_apply_async.assert_called_with( + notification_id=notification.id, ) diff --git a/publishing/tests/test_models.py b/publishing/tests/test_models.py index 78c188feb..914023635 100644 --- a/publishing/tests/test_models.py +++ b/publishing/tests/test_models.py @@ -5,7 +5,6 @@ import factory import freezegun import pytest -from django.conf import settings from django_fsm import TransitionNotAllowed from common.tests import factories @@ -68,7 +67,7 @@ def test_create_from_invalid_status(): def test_notify_ready_for_processing( packaged_workbasket_factory, published_envelope_factory, - mocked_publishing_models_send_emails_delay, + mocked_send_emails_apply_async, settings, ): packaged_wb = packaged_workbasket_factory() @@ -83,15 +82,11 @@ def test_notify_ready_for_processing( "embargo": str(packaged_wb.embargo), "jira_url": packaged_wb.jira_url, } - mocked_publishing_models_send_emails_delay.assert_called_once_with( - template_id=settings.READY_FOR_CDS_TEMPLATE_ID, - personalisation=personalisation, - email_type="packaging", - ) + mocked_send_emails_apply_async.assert_called_once() def test_notify_processing_succeeded( - mocked_publishing_models_send_emails_delay, + mocked_send_emails_apply_async, packaged_workbasket_factory, published_envelope_factory, ): @@ -110,15 +105,11 @@ def test_notify_processing_succeeded( "loading_report_message": f"Loading report(s): {loading_report.file_name}", "comments": packaged_wb.loadingreports.first().comments, } - mocked_publishing_models_send_emails_delay.assert_called_once_with( - template_id=settings.CDS_ACCEPTED_TEMPLATE_ID, - personalisation=personalisation, - email_type="packaging", - ) + mocked_send_emails_apply_async.assert_called_once() def test_notify_processing_failed( - mocked_publishing_models_send_emails_delay, + mocked_send_emails_apply_async, packaged_workbasket_factory, published_envelope_factory, ): @@ -141,18 +132,14 @@ def test_notify_processing_failed( "comments": packaged_wb.loadingreports.first().comments, } - mocked_publishing_models_send_emails_delay.assert_called_once_with( - template_id=settings.CDS_REJECTED_TEMPLATE_ID, - personalisation=personalisation, - email_type="packaging", - ) + mocked_send_emails_apply_async.assert_called_once() def test_success_processing_transition( packaged_workbasket_factory, published_envelope_factory, envelope_storage, - mocked_publishing_models_send_emails_delay, + mocked_send_emails_apply_async, settings, ): settings.ENABLE_PACKAGING_NOTIFICATIONS = False diff --git a/workbaskets/tests/test_views.py b/workbaskets/tests/test_views.py index 6a8559f80..ee6fea126 100644 --- a/workbaskets/tests/test_views.py +++ b/workbaskets/tests/test_views.py @@ -16,9 +16,12 @@ from common.tests.factories import GoodsNomenclatureFactory from common.tests.factories import MeasureFactory from exporter.tasks import upload_workbaskets +from importer.models import ImportBatch +from importer.models import ImportBatchStatus from measures.models import Measure from workbaskets import models from workbaskets.forms import SelectableObjectsForm +from workbaskets.tasks import check_workbasket_sync from workbaskets.validators import WorkflowStatus from workbaskets.views import ui @@ -663,6 +666,109 @@ def test_submit_for_packaging(valid_user_client, session_workbasket): assert response.url[: len(response_url)] == response_url +@pytest.fixture +def successful_business_rules_setup(session_workbasket, valid_user_client): + """Sets up data and runs business rules.""" + with session_workbasket.new_transaction() as transaction: + good = GoodsNomenclatureFactory.create(transaction=transaction) + measure = MeasureFactory.create(transaction=transaction) + geo_area = GeographicalAreaFactory.create(transaction=transaction) + objects = [good, measure, geo_area] + for obj in objects: + TrackedModelCheckFactory.create( + transaction_check__transaction=transaction, + model=obj, + successful=True, + ) + session = valid_user_client.session + session["workbasket"] = { + "id": session_workbasket.pk, + "status": session_workbasket.status, + "title": session_workbasket.title, + "error_count": session_workbasket.tracked_model_check_errors.count(), + } + session.save() + + # run rule checks so unchecked_or_errored_transactions is set + check_workbasket_sync(session_workbasket) + + +def import_batch_with_notification(): + import_batch = factories.ImportBatchFactory.create( + status=ImportBatchStatus.SUCCEEDED, + goods_import=True, + taric_file="goods.xml", + ) + + return factories.GoodsSuccessfulImportNotificationFactory( + notified_object_pk=import_batch.id, + ) + + +@pytest.mark.parametrize( + "import_batch_factory,disabled", + [ + ( + lambda: factories.ImportBatchFactory.create( + status=ImportBatchStatus.SUCCEEDED, + goods_import=True, + ), + True, + ), + ( + import_batch_with_notification, + False, + ), + ( + lambda: factories.ImportBatchFactory.create( + status=ImportBatchStatus.SUCCEEDED, + ), + False, + ), + ( + lambda: None, + False, + ), + ], + ids=( + "goods_import_no_notification", + "goods_import_with_notification", + "master_import", + "no_import", + ), +) +def test_submit_for_packaging_disabled( + successful_business_rules_setup, + valid_user_client, + session_workbasket, + import_batch_factory, + disabled, +): + """Test that the submit-for-packaging button is disabled when a notification + has not been sent for a commodity code import (goods)""" + + import_batch = import_batch_factory() + + if import_batch: + import_batch.workbasket_id = session_workbasket.id + if isinstance(import_batch, ImportBatch): + import_batch.save() + + response = valid_user_client.get( + reverse("workbaskets:current-workbasket"), + ) + + assert response.status_code == 200 + soup = BeautifulSoup(str(response.content), "html.parser") + + packaging_button = soup.find("a", href="/publishing/create/") + + if disabled: + assert packaging_button.has_attr("disabled") + else: + assert not packaging_button.has_attr("disabled") + + def test_terminate_rule_check(valid_user_client, session_workbasket): session_workbasket.rule_check_task_id = 123 @@ -1193,3 +1299,79 @@ def test_application_access_after_workbasket_delete( # workbasket deletion. assert response.status_code == 200 assert not page.select("header nav a.workbasket-link") + + +def make_goods_import_batch(importer_storage, **kwargs): + return factories.ImportBatchFactory.create( + status=ImportBatchStatus.SUCCEEDED, + goods_import=True, + taric_file="goods.xml", + **kwargs, + ) + + +@pytest.mark.parametrize( + "import_batch_factory,visable", + [ + ( + lambda: factories.ImportBatchFactory.create( + status=ImportBatchStatus.SUCCEEDED, + goods_import=True, + taric_file="goods.xml", + ), + True, + ), + ( + import_batch_with_notification, + False, + ), + ( + lambda: factories.ImportBatchFactory.create( + status=ImportBatchStatus.SUCCEEDED, + ), + False, + ), + ( + lambda: None, + False, + ), + ], + ids=( + "goods_import_no_notification", + "goods_import_with_notification", + "master_import", + "no_import", + ), +) +def test_review_goods_notification_button( + successful_business_rules_setup, + valid_user_client, + session_workbasket, + import_batch_factory, + visable, +): + """Test that the submit-for-packaging button is disabled when a notification + has not been sent for a commodity code import (goods)""" + + import_batch = import_batch_factory() + + if import_batch: + import_batch.workbasket_id = session_workbasket.id + if isinstance(import_batch, ImportBatch): + import_batch.save() + + response = valid_user_client.get( + reverse("workbaskets:workbasket-ui-review-goods"), + ) + + assert response.status_code == 200 + soup = BeautifulSoup(str(response.content), "html.parser") + + # notify_button = soup.find("a", href=f"/notify-goods-report/{import_batch.id}/") + notify_button = soup.select(".govuk-body") + + print(notify_button) + if visable: + assert notify_button + else: + assert not notify_button From 6b059fc941e82d2e6fccc018b50a24c8c1e3cfbf Mon Sep 17 00:00:00 2001 From: Anthoni Gleeson Date: Thu, 14 Sep 2023 15:38:00 +0100 Subject: [PATCH 10/15] Working tests! --- .../test_send_goods_report_notification.py | 2 +- importer/tests/test_views.py | 2 +- notifications/models.py | 2 +- notifications/notify.py | 1 - notifications/tasks.py | 3 +- notifications/tests/conftest.py | 24 ++++-- notifications/tests/test_tasks.py | 82 +++++++++++++------ .../models/crown_dependencies_envelope.py | 1 + .../test_model_crown_dependencies_envelope.py | 17 ++-- publishing/tests/test_models.py | 7 ++ workbaskets/tests/test_views.py | 22 ++++- 11 files changed, 116 insertions(+), 47 deletions(-) diff --git a/importer/tests/management/commands/test_send_goods_report_notification.py b/importer/tests/management/commands/test_send_goods_report_notification.py index e6364f197..a64425197 100644 --- a/importer/tests/management/commands/test_send_goods_report_notification.py +++ b/importer/tests/management/commands/test_send_goods_report_notification.py @@ -41,7 +41,7 @@ def test_send_goods_report_notification( notification.""" with patch( - "notifications.tasks.send_emails_task", + "notifications.models.send_emails_task", return_value=MagicMock(), ) as mocked_email_task: call_command( diff --git a/importer/tests/test_views.py b/importer/tests/test_views.py index 1ecb4f301..87bf0e0ba 100644 --- a/importer/tests/test_views.py +++ b/importer/tests/test_views.py @@ -293,7 +293,7 @@ def test_notify_channel_islands_redirects( conformation page on successful notification trigger.""" with patch( - "notifications.tasks.send_emails_task", + "notifications.models.send_emails_task", return_value=MagicMock(), ) as mocked_email_task: response = valid_user_client.get( diff --git a/notifications/models.py b/notifications/models.py index 6d142661a..c6c085470 100644 --- a/notifications/models.py +++ b/notifications/models.py @@ -158,7 +158,7 @@ def schedule_send_emails(self, countdown=1): background task.""" send_emails_task.apply_async(args=[self.pk], countdown=countdown) - def send_emails(self): + def send_notification_emails(self): """Send the notification emails to users via GOV.UK Notify.""" notified_users = self.notified_users() diff --git a/notifications/notify.py b/notifications/notify.py index 5c0f9f95e..643181268 100644 --- a/notifications/notify.py +++ b/notifications/notify.py @@ -74,7 +74,6 @@ def send_emails(template_id, personalisation: dict, email_addresses: List[str]) response_ids += f"{response['id']} \n" recipients += f"{email} \n" except Exception as e: - # TODO extract error info? failed_recipients += f"{email} \n" logger.error( f"Failed to send email notification to {email}, with status code {e.status_code}.", diff --git a/notifications/tasks.py b/notifications/tasks.py index 88f622d37..a771c71b3 100644 --- a/notifications/tasks.py +++ b/notifications/tasks.py @@ -13,7 +13,6 @@ def send_emails_task(notification_pk: int): creating a log to record which users received which email template.""" from notifications.models import Notification - print("in here") notification = Notification.objects.get(pk=notification_pk) sub_notification = notification.return_subclass_instance() - sub_notification.send_emails() + sub_notification.send_notification_emails() diff --git a/notifications/tests/conftest.py b/notifications/tests/conftest.py index 86fa305e6..2cc434884 100644 --- a/notifications/tests/conftest.py +++ b/notifications/tests/conftest.py @@ -1,5 +1,6 @@ from unittest.mock import patch +import factory import pytest from common.tests import factories @@ -59,11 +60,22 @@ def successful_publishing_notification(crown_dependencies_envelope_factory): @pytest.fixture() -def mock_notify_send_emails(): - with patch( - "notifications.notify.send_emails", - ) as mocked_send_emails: - yield mocked_send_emails +def notify_send_emails_return_value(): + """ + Factory fixture to create a mock for sending an email. This allows you to + override the response in the test. + + params: + response_ids: [list of uuids], + recipients: [list of strings], + failed_recipients: [list of strings] + """ + + return { + "response_ids": " \n".join([factory.Faker("uuid")]), + "recipients": " \n".join([str(factory.Faker("email"))]), + "failed_recipients": "", + } @pytest.fixture() @@ -75,7 +87,7 @@ def mock_prepare_link(): "retention_period": None, } with patch( - "notifications.notify.prepare_link_to_file", + "notifications.models.GoodsSuccessfulImportNotification.prepare_link_to_file", return_value=return_value, ) as mocked_prepare_link_to_file: yield mocked_prepare_link_to_file diff --git a/notifications/tests/test_tasks.py b/notifications/tests/test_tasks.py index 53902b90b..a0e82bd00 100644 --- a/notifications/tests/test_tasks.py +++ b/notifications/tests/test_tasks.py @@ -1,3 +1,6 @@ +from unittest.mock import patch + +import factory import pytest from notifications import models @@ -8,24 +11,41 @@ def test_send_emails_goods_report_notification( goods_report_notification, - mock_notify_send_emails, ): """Tests that email notifications are only sent to users subscribed to email type and that a log is created with this user's email.""" expected_present_email = "goods_report@email.co.uk" # /PS-IGNORE expected_unenrolled_email = "no_goods_report@email.co.uk" # /PS-IGNORE - tasks.send_emails_task.apply( - kwargs={ - "notification_pk": goods_report_notification.id, - }, - ) - - recipients = f"{expected_present_email} \n" + return_value = { + "file": "VGVzdA==", + "is_csv": False, + "confirm_email_before_download": True, + "retention_period": None, + } + with patch( + "notifications.models.prepare_link_to_file", + return_value=return_value, + ) as mocked_prepare_link_to_file: + with patch( + "notifications.models.send_emails", + return_value={ + "response_ids": " \n".join([str(factory.Faker("uuid"))]), + "recipients": " \n".join([expected_present_email]), + "failed_recipients": "", + }, + ) as mocked_send_emails: + tasks.send_emails_task.apply( + kwargs={ + "notification_pk": goods_report_notification.id, + }, + ) + mocked_send_emails.assert_called_once() + mocked_prepare_link_to_file.assert_called_once() log = models.NotificationLog.objects.get( notification=goods_report_notification, - recipients=recipients, + recipients=expected_present_email, success=True, ) @@ -34,7 +54,6 @@ def test_send_emails_goods_report_notification( def test_send_emails_packaging_notification( ready_for_packaging_notification, - mock_notify_send_emails, ): """Tests that email notifications are only sent to users subscribed to email type and that a log is created with this user's email.""" @@ -42,16 +61,24 @@ def test_send_emails_packaging_notification( expected_present_email = "packaging@email.co.uk" # /PS-IGNORE expected_unenrolled_email = "no_packaging@email.co.uk" # /PS-IGNORE - tasks.send_emails_task.apply( - kwargs={ - "notification_pk": ready_for_packaging_notification.id, + with patch( + "notifications.models.send_emails", + return_value={ + "response_ids": " \n".join([str(factory.Faker("uuid"))]), + "recipients": " \n".join([expected_present_email]), + "failed_recipients": "", }, - ) - recipients = f"{expected_present_email} \n" + ) as mocked_send_emails: + tasks.send_emails_task.apply( + kwargs={ + "notification_pk": ready_for_packaging_notification.id, + }, + ) + mocked_send_emails.assert_called_once() log = models.NotificationLog.objects.get( notification=ready_for_packaging_notification, - recipients=recipients, + recipients=expected_present_email, success=True, ) @@ -60,7 +87,7 @@ def test_send_emails_packaging_notification( def test_send_emails_publishing_notification( successful_publishing_notification, - mock_notify_send_emails, + # mock_notify_send_emails, ): """Tests that email notifications are only sent to users subscribed to email type and that a log is created with this user's email.""" @@ -68,17 +95,24 @@ def test_send_emails_publishing_notification( expected_present_email = "publishing@email.co.uk" # /PS-IGNORE expected_unenrolled_email = "no_publishing@email.co.uk" # /PS-IGNORE - tasks.send_emails_task.apply( - kwargs={ - "notification_pk": successful_publishing_notification.id, + with patch( + "notifications.models.send_emails", + return_value={ + "response_ids": " \n".join([str(factory.Faker("uuid"))]), + "recipients": " \n".join([expected_present_email]), + "failed_recipients": "", }, - ) - - recipients = f"{expected_present_email} \n" + ) as mocked_send_emails: + tasks.send_emails_task.apply( + kwargs={ + "notification_pk": successful_publishing_notification.id, + }, + ) + mocked_send_emails.assert_called_once() log = models.NotificationLog.objects.get( notification=successful_publishing_notification, - recipients=recipients, + recipients=expected_present_email, success=True, ) diff --git a/publishing/models/crown_dependencies_envelope.py b/publishing/models/crown_dependencies_envelope.py index aab4640c7..487e58dee 100644 --- a/publishing/models/crown_dependencies_envelope.py +++ b/publishing/models/crown_dependencies_envelope.py @@ -158,6 +158,7 @@ def notify_publishing_success(self): notification = CrownDependenciesEnvelopeSuccessNotification( notified_object_pk=self.pk, ) + print(notification) notification.save() notification.schedule_send_emails() diff --git a/publishing/tests/test_model_crown_dependencies_envelope.py b/publishing/tests/test_model_crown_dependencies_envelope.py index d9ed71d34..b2ba46820 100644 --- a/publishing/tests/test_model_crown_dependencies_envelope.py +++ b/publishing/tests/test_model_crown_dependencies_envelope.py @@ -36,7 +36,10 @@ def test_notify_processing_succeeded( packaged_workbasket_factory, successful_envelope_factory, crown_dependencies_envelope_factory, + settings, ): + settings.ENABLE_PACKAGING_NOTIFICATIONS = True + pwb = packaged_workbasket_factory() crown_dependencies_envelope_factory(packaged_workbasket=pwb) @@ -45,10 +48,8 @@ def test_notify_processing_succeeded( cd_envelope.notify_publishing_success() - notification = Notification.objects.last() - mocked_send_emails_apply_async.assert_called_with( - notification_id=notification.id, - ) + Notification.objects.last() + mocked_send_emails_apply_async.assert_called() def test_notify_processing_failed( @@ -56,7 +57,9 @@ def test_notify_processing_failed( packaged_workbasket_factory, successful_envelope_factory, crown_dependencies_envelope_factory, + settings, ): + settings.ENABLE_PACKAGING_NOTIFICATIONS = True pwb = packaged_workbasket_factory() crown_dependencies_envelope_factory(packaged_workbasket=pwb) @@ -65,7 +68,5 @@ def test_notify_processing_failed( cd_envelope.notify_publishing_failed() - notification = Notification.objects.last() - mocked_send_emails_apply_async.assert_called_with( - notification_id=notification.id, - ) + Notification.objects.last() + mocked_send_emails_apply_async.assert_called() diff --git a/publishing/tests/test_models.py b/publishing/tests/test_models.py index 914023635..287339f8a 100644 --- a/publishing/tests/test_models.py +++ b/publishing/tests/test_models.py @@ -70,6 +70,8 @@ def test_notify_ready_for_processing( mocked_send_emails_apply_async, settings, ): + settings.ENABLE_PACKAGING_NOTIFICATIONS = True + packaged_wb = packaged_workbasket_factory() envelope = published_envelope_factory(packaged_workbasket=packaged_wb) packaged_wb.notify_ready_for_processing() @@ -89,7 +91,10 @@ def test_notify_processing_succeeded( mocked_send_emails_apply_async, packaged_workbasket_factory, published_envelope_factory, + settings, ): + settings.ENABLE_PACKAGING_NOTIFICATIONS = True + packaged_wb = packaged_workbasket_factory() loading_report = factories.LoadingReportFactory.create( packaged_workbasket=packaged_wb, @@ -112,7 +117,9 @@ def test_notify_processing_failed( mocked_send_emails_apply_async, packaged_workbasket_factory, published_envelope_factory, + settings, ): + settings.ENABLE_PACKAGING_NOTIFICATIONS = True packaged_wb = packaged_workbasket_factory() loading_report1 = factories.LoadingReportFactory.create( packaged_workbasket=packaged_wb, diff --git a/workbaskets/tests/test_views.py b/workbaskets/tests/test_views.py index ee6fea126..64f192507 100644 --- a/workbaskets/tests/test_views.py +++ b/workbaskets/tests/test_views.py @@ -1,5 +1,7 @@ +import os import re from unittest.mock import MagicMock +from unittest.mock import mock_open from unittest.mock import patch import pytest @@ -1310,6 +1312,7 @@ def make_goods_import_batch(importer_storage, **kwargs): ) +@pytest.skip("Unable to mock s3 file read from within ET.parse currently") @pytest.mark.parametrize( "import_batch_factory,visable", [ @@ -1360,9 +1363,22 @@ def test_review_goods_notification_button( if isinstance(import_batch, ImportBatch): import_batch.save() - response = valid_user_client.get( - reverse("workbaskets:workbasket-ui-review-goods"), - ) + def mock_xlsx_open(filename, mode): + if os.path.basename(filename) == "goods.xlsx": + return mock_open().return_value + return open(filename, mode) + + with patch( + "importer.goods_report.GoodsReport.xlsx_file", + return_value="", + ) as mocked_xlsx_file: + # with patch( + # ".open", + # mock_xlsx_open, + # ): + response = valid_user_client.get( + reverse("workbaskets:workbasket-ui-review-goods"), + ) assert response.status_code == 200 soup = BeautifulSoup(str(response.content), "html.parser") From 387cd2ac3859426fcfedc21c59ebec8713073546 Mon Sep 17 00:00:00 2001 From: Anthoni Gleeson Date: Thu, 14 Sep 2023 16:23:08 +0100 Subject: [PATCH 11/15] removing unused fixtures --- notifications/tests/conftest.py | 37 --------------------------------- 1 file changed, 37 deletions(-) diff --git a/notifications/tests/conftest.py b/notifications/tests/conftest.py index 2cc434884..d771afd97 100644 --- a/notifications/tests/conftest.py +++ b/notifications/tests/conftest.py @@ -1,6 +1,3 @@ -from unittest.mock import patch - -import factory import pytest from common.tests import factories @@ -57,37 +54,3 @@ def successful_publishing_notification(crown_dependencies_envelope_factory): return factories.CrownDependenciesEnvelopeSuccessNotificationFactory( notified_object_pk=cde.id, ) - - -@pytest.fixture() -def notify_send_emails_return_value(): - """ - Factory fixture to create a mock for sending an email. This allows you to - override the response in the test. - - params: - response_ids: [list of uuids], - recipients: [list of strings], - failed_recipients: [list of strings] - """ - - return { - "response_ids": " \n".join([factory.Faker("uuid")]), - "recipients": " \n".join([str(factory.Faker("email"))]), - "failed_recipients": "", - } - - -@pytest.fixture() -def mock_prepare_link(): - return_value = { - "file": "VGVzdA==", - "is_csv": False, - "confirm_email_before_download": True, - "retention_period": None, - } - with patch( - "notifications.models.GoodsSuccessfulImportNotification.prepare_link_to_file", - return_value=return_value, - ) as mocked_prepare_link_to_file: - yield mocked_prepare_link_to_file From 7512f20115aa88b0fcceb0a1e435ec2410ba3b08 Mon Sep 17 00:00:00 2001 From: Anthoni Gleeson Date: Fri, 15 Sep 2023 08:49:12 +0100 Subject: [PATCH 12/15] test fixes and admin filter --- conftest.py | 5 ++- notifications/admin.py | 4 +- notifications/tests/test_models.py | 61 +++++++++++++++++++++++++++++- workbaskets/tests/test_views.py | 8 ++-- 4 files changed, 70 insertions(+), 8 deletions(-) diff --git a/conftest.py b/conftest.py index b30cbf8f8..170f98c0e 100644 --- a/conftest.py +++ b/conftest.py @@ -1901,7 +1901,10 @@ def factory_method(packaged_workbasket=None, **kwargs): @pytest.fixture(scope="function") -def successful_envelope_factory(published_envelope_factory): +def successful_envelope_factory( + published_envelope_factory, + mocked_send_emails_apply_async, +): """ Factory fixture to create a successfully processed envelope and update the packaged_workbasket envelope field. diff --git a/notifications/admin.py b/notifications/admin.py index 077f33549..e7f4e8a42 100644 --- a/notifications/admin.py +++ b/notifications/admin.py @@ -72,8 +72,6 @@ def changeform_view(self, request, object_id=None, form_url="", extra_context=No class NotificationAdmin(admin.ModelAdmin): """Class providing read-only list and detail views for notification.""" - # TODO add filters on notification_type - ordering = ["pk"] list_display = ( "pk", @@ -82,6 +80,8 @@ class NotificationAdmin(admin.ModelAdmin): "display_users", ) + list_filter = ("notification_type",) + readonly_fields = [] def display_users(self, obj): diff --git a/notifications/tests/test_models.py b/notifications/tests/test_models.py index a318c5f44..c3f26497e 100644 --- a/notifications/tests/test_models.py +++ b/notifications/tests/test_models.py @@ -1,6 +1,10 @@ +from unittest.mock import patch + +import factory import pytest from importer.models import ImportBatch +from notifications.models import NotificationLog from publishing.models import CrownDependenciesEnvelope from publishing.models import PackagedWorkBasket @@ -20,7 +24,23 @@ def test_create_goods_report_notification(goods_report_notification): assert user.email != expected_not_present_email assert isinstance(goods_report_notification.notified_object(), ImportBatch) - # TODO check goods_report_notification.get_personalisation() + + return_value = { + "file": "VGVzdA==", + "is_csv": False, + "confirm_email_before_download": True, + "retention_period": None, + } + with patch( + "notifications.models.prepare_link_to_file", + return_value=return_value, + ) as mocked_prepare_link_to_file: + personalisation = goods_report_notification.get_personalisation() + + assert personalisation == { + "tgb_id": "0", + "link_to_file": return_value, + } def test_create_packaging_notification(ready_for_packaging_notification): @@ -74,3 +94,42 @@ def test_create_successful_publishing_notification(successful_publishing_notific # TODO add test send_emails +def test_send_notification_emails(ready_for_packaging_notification): + expected_present_email = f"packaging@email.co.uk" # /PS-IGNORE + expected_not_present_email = f"no_packaging@email.co.uk" # /PS-IGNORE + with patch( + "notifications.models.send_emails", + return_value={ + "response_ids": " \n".join([str(factory.Faker("uuid"))]), + "recipients": " \n".join([expected_present_email]), + "failed_recipients": "", + }, + ) as mocked_send_emails: + ready_for_packaging_notification.send_notification_emails() + mocked_send_emails.assert_called_once() + + log_success = NotificationLog.objects.get( + notification=ready_for_packaging_notification, + recipients=expected_present_email, + success=True, + ) + + assert expected_present_email in log_success.recipients + + with patch( + "notifications.models.send_emails", + return_value={ + "response_ids": "", + "recipients": "", + "failed_recipients": " \n".join([expected_present_email]), + }, + ) as mocked_send_emails: + ready_for_packaging_notification.send_notification_emails() + mocked_send_emails.assert_called_once() + + log_fail = NotificationLog.objects.get( + notification=ready_for_packaging_notification, + success=False, + ) + + assert expected_present_email in log_fail.recipients diff --git a/workbaskets/tests/test_views.py b/workbaskets/tests/test_views.py index 12f09ea20..e7b974fde 100644 --- a/workbaskets/tests/test_views.py +++ b/workbaskets/tests/test_views.py @@ -669,9 +669,9 @@ def test_submit_for_packaging(valid_user_client, session_workbasket): def successful_business_rules_setup(session_workbasket, valid_user_client): """Sets up data and runs business rules.""" with session_workbasket.new_transaction() as transaction: - good = GoodsNomenclatureFactory.create(transaction=transaction) - measure = MeasureFactory.create(transaction=transaction) - geo_area = GeographicalAreaFactory.create(transaction=transaction) + good = factories.GoodsNomenclatureFactory.create(transaction=transaction) + measure = factories.MeasureFactory.create(transaction=transaction) + geo_area = factories.GeographicalAreaFactory.create(transaction=transaction) objects = [good, measure, geo_area] for obj in objects: TrackedModelCheckFactory.create( @@ -1404,7 +1404,7 @@ def make_goods_import_batch(importer_storage, **kwargs): ) -@pytest.skip("Unable to mock s3 file read from within ET.parse currently") +@pytest.mark.skip(reason="Unable to mock s3 file read from within ET.parse currently") @pytest.mark.parametrize( "import_batch_factory,visable", [ From 13849397f05e13178394a88d78bdbb7e8b118463 Mon Sep 17 00:00:00 2001 From: Anthoni Gleeson Date: Fri, 15 Sep 2023 09:30:32 +0100 Subject: [PATCH 13/15] test fix --- notifications/tests/test_models.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/notifications/tests/test_models.py b/notifications/tests/test_models.py index c3f26497e..145981eb2 100644 --- a/notifications/tests/test_models.py +++ b/notifications/tests/test_models.py @@ -23,7 +23,8 @@ def test_create_goods_report_notification(goods_report_notification): assert user.email == expected_present_email assert user.email != expected_not_present_email - assert isinstance(goods_report_notification.notified_object(), ImportBatch) + import_batch = goods_report_notification.notified_object() + assert isinstance(import_batch, ImportBatch) return_value = { "file": "VGVzdA==", @@ -38,7 +39,7 @@ def test_create_goods_report_notification(goods_report_notification): personalisation = goods_report_notification.get_personalisation() assert personalisation == { - "tgb_id": "0", + "tgb_id": import_batch.name, "link_to_file": return_value, } @@ -93,7 +94,6 @@ def test_create_successful_publishing_notification(successful_publishing_notific assert content == {"envelope_id": "230001"} -# TODO add test send_emails def test_send_notification_emails(ready_for_packaging_notification): expected_present_email = f"packaging@email.co.uk" # /PS-IGNORE expected_not_present_email = f"no_packaging@email.co.uk" # /PS-IGNORE From 341c5c5f6f46d0ee0bb33277bf9d40d4b62f7d61 Mon Sep 17 00:00:00 2001 From: Anthoni Gleeson Date: Mon, 18 Sep 2023 14:37:56 +0100 Subject: [PATCH 14/15] Confirm should be true by default --- notifications/models.py | 1 - notifications/notify.py | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/notifications/models.py b/notifications/models.py index c6c085470..36cb20ca8 100644 --- a/notifications/models.py +++ b/notifications/models.py @@ -443,7 +443,6 @@ def get_personalisation(self) -> dict: "tgb_id": import_batch.name, "link_to_file": prepare_link_to_file( import_batch.taric_file, - confirm_email_before_download=True, ), } return personalisation diff --git a/notifications/notify.py b/notifications/notify.py index 643181268..8d19c8e54 100644 --- a/notifications/notify.py +++ b/notifications/notify.py @@ -18,7 +18,7 @@ def get_notifications_client(): def prepare_link_to_file( file, is_csv=False, - confirm_email_before_download=None, + confirm_email_before_download=True, retention_period=None, ): """ @@ -28,7 +28,7 @@ def prepare_link_to_file( params: file: file which to generate report from is_csv: if the file being attached is a csv set to True, default False - confirm_email_before_download: security feature where user opening files must be on Gov Notify email list + confirm_email_before_download: security feature where user opening files must be on Gov Notify email list, True by default retention_period: how long the file link is valid for, default 6 months """ From 6cf46d96765ad06d0c793cbbaae3cba50c1cfb1a Mon Sep 17 00:00:00 2001 From: Anthoni Gleeson Date: Tue, 19 Sep 2023 08:22:45 +0100 Subject: [PATCH 15/15] PR comments --- notifications/models.py | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/notifications/models.py b/notifications/models.py index 36cb20ca8..c9faad050 100644 --- a/notifications/models.py +++ b/notifications/models.py @@ -22,7 +22,7 @@ class NotifiedUser(models.Model): enrol_api_publishing = models.BooleanField(default=False) enrol_goods_report = models.BooleanField(default=False) - def __str__(self) -> str: + def __str__(self): return str(self.email) @@ -49,9 +49,6 @@ class NotificationLog(TimestampedMixin): success = models.BooleanField(default=True) -# class NotificationManager(models.Manager): - - class NotificationTypeChoices(models.TextChoices): GOODS_REPORT = ( "goods_report", @@ -90,10 +87,6 @@ class Notification(models.Model): https://docs.djangoproject.com/en/dev/topics/db/models/#proxy-models """ - # def __init__(self, notificaiton_type: NotificationTypeChoices, notified_object_pk: int = None, ): - # self.notified_object_pk = notified_object_pk - # self.notificaiton_type = notificaiton_type - notified_object_pk = models.IntegerField( default=None, null=True,