From c9fbdbe05f656b0bee5009b91d5ea3bcf619b22a Mon Sep 17 00:00:00 2001 From: A Gleeson Date: Thu, 21 Sep 2023 10:36:59 +0000 Subject: [PATCH 1/4] fixed get packaged workbasket bug (#1047) * fixed bug and unit tested * incorrect reference fix, comments and another test * Pr comments * removed unecessary variables * updated test tasks --- common/tests/factories.py | 24 +++ conftest.py | 38 +++++ notifications/models.py | 6 +- notifications/tests/conftest.py | 127 ++++++++++++++-- notifications/tests/test_models.py | 141 +++++++++++++++--- notifications/tests/test_tasks.py | 42 ++++-- .../models/crown_dependencies_envelope.py | 1 - 7 files changed, 327 insertions(+), 52 deletions(-) diff --git a/common/tests/factories.py b/common/tests/factories.py index da8f952cd..696275669 100644 --- a/common/tests/factories.py +++ b/common/tests/factories.py @@ -1449,6 +1449,20 @@ class Meta: model = "notifications.EnvelopeReadyForProcessingNotification" +class EnvelopeAcceptedNotificationFactory(factory.django.DjangoModelFactory): + """This is a factory for an envelope that has been accepted notification.""" + + class Meta: + model = "notifications.EnvelopeAcceptedNotification" + + +class EnvelopeRejectedNotificationFactory(factory.django.DjangoModelFactory): + """This is a factory for an envelope that has been rejected notification.""" + + class Meta: + model = "notifications.EnvelopeRejectedNotification" + + class CrownDependenciesEnvelopeSuccessNotificationFactory( factory.django.DjangoModelFactory, ): @@ -1457,3 +1471,13 @@ class CrownDependenciesEnvelopeSuccessNotificationFactory( class Meta: model = "notifications.CrownDependenciesEnvelopeSuccessNotification" + + +class CrownDependenciesEnvelopeFailedNotificationFactory( + factory.django.DjangoModelFactory, +): + """This is a factory for a crown dependencies envelope failed + notification.""" + + class Meta: + model = "notifications.CrownDependenciesEnvelopeFailedNotification" diff --git a/conftest.py b/conftest.py index 170f98c0e..ec6785447 100644 --- a/conftest.py +++ b/conftest.py @@ -1937,6 +1937,44 @@ def factory_method(**kwargs): return factory_method +@pytest.fixture(scope="function") +def failed_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. + + 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_failed() + 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): """ diff --git a/notifications/models.py b/notifications/models.py index c9faad050..59ebf4069 100644 --- a/notifications/models.py +++ b/notifications/models.py @@ -283,7 +283,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 ) @@ -393,9 +393,9 @@ def __init__(self, notified_object_pk: int): ) def get_personalisation(self) -> dict: - self.notified_object() + crown_dependicies_envelope = self.notified_object() personalisation = { - "envelope_id": self.packagedworkbaskets.last().envelope.envelope_id, + "envelope_id": crown_dependicies_envelope.packagedworkbaskets.last().envelope.envelope_id, } return personalisation diff --git a/notifications/tests/conftest.py b/notifications/tests/conftest.py index d771afd97..9849cb7a4 100644 --- a/notifications/tests/conftest.py +++ b/notifications/tests/conftest.py @@ -2,17 +2,20 @@ from common.tests import factories from importer.models import ImportBatchStatus +from publishing.models import PackagedWorkBasket @pytest.fixture() def goods_report_notification(): + present_email = f"goods_report@email.co.uk" # /PS-IGNORE + not_present_email = f"no_goods_report@email.co.uk" # /PS-IGNORE factories.NotifiedUserFactory( - email="goods_report@email.co.uk", # /PS-IGNORE + email=present_email, enrol_packaging=False, enrol_goods_report=True, ) factories.NotifiedUserFactory( - email="no_goods_report@email.co.uk", # /PS-IGNORE + email=not_present_email, ) import_batch = factories.ImportBatchFactory.create( status=ImportBatchStatus.SUCCEEDED, @@ -20,37 +23,137 @@ def goods_report_notification(): taric_file="goods.xml", ) - return factories.GoodsSuccessfulImportNotificationFactory( - notified_object_pk=import_batch.id, + return ( + factories.GoodsSuccessfulImportNotificationFactory( + notified_object_pk=import_batch.id, + ), + present_email, + not_present_email, ) @pytest.fixture() def ready_for_packaging_notification(published_envelope_factory): + present_email = f"packaging@email.co.uk" # /PS-IGNORE + not_present_email = f"no_packaging@email.co.uk" # /PS-IGNORE factories.NotifiedUserFactory( - email="packaging@email.co.uk", # /PS-IGNORE + email=present_email, ) factories.NotifiedUserFactory( - email="no_packaging@email.co.uk", # /PS-IGNORE + email=not_present_email, enrol_packaging=False, ) packaged_wb = published_envelope_factory() - return factories.EnvelopeReadyForProcessingNotificationFactory( - notified_object_pk=packaged_wb.id, + return ( + factories.EnvelopeReadyForProcessingNotificationFactory( + notified_object_pk=packaged_wb.id, + ), + present_email, + not_present_email, ) @pytest.fixture() def successful_publishing_notification(crown_dependencies_envelope_factory): + present_email = f"publishing@email.co.uk" # /PS-IGNORE + not_present_email = f"no_publishing@email.co.uk" # /PS-IGNORE factories.NotifiedUserFactory( - email="publishing@email.co.uk", # /PS-IGNORE + email=present_email, enrol_packaging=False, enrol_api_publishing=True, ) factories.NotifiedUserFactory( - email="no_publishing@email.co.uk", # /PS-IGNORE + email=not_present_email, ) cde = crown_dependencies_envelope_factory() - return factories.CrownDependenciesEnvelopeSuccessNotificationFactory( - notified_object_pk=cde.id, + return ( + factories.CrownDependenciesEnvelopeSuccessNotificationFactory( + notified_object_pk=cde.id, + ), + present_email, + not_present_email, + ) + + +@pytest.fixture() +def failed_publishing_notification(successful_envelope_factory, settings): + present_email = f"publishing@email.co.uk" # /PS-IGNORE + not_present_email = f"no_publishing@email.co.uk" # /PS-IGNORE + factories.NotifiedUserFactory( + email=present_email, + enrol_packaging=False, + enrol_api_publishing=True, + ) + factories.NotifiedUserFactory( + email=not_present_email, + ) + + settings.ENABLE_PACKAGING_NOTIFICATIONS = False + successful_envelope_factory() + pwb = PackagedWorkBasket.objects.get_unpublished_to_api().last() + crown_dependencies_envelope = factories.CrownDependenciesEnvelopeFactory( + packaged_work_basket=pwb, + ) + + crown_dependencies_envelope.publishing_failed() + return ( + factories.CrownDependenciesEnvelopeFailedNotificationFactory( + notified_object_pk=crown_dependencies_envelope.id, + ), + present_email, + not_present_email, + ) + + +@pytest.fixture() +def accepted_packaging_notification(successful_envelope_factory, settings): + present_email = f"packaging@email.co.uk" # /PS-IGNORE + not_present_email = f"no_packaging@email.co.uk" # /PS-IGNORE + factories.NotifiedUserFactory( + email=present_email, + ) + factories.NotifiedUserFactory( + email=not_present_email, + enrol_packaging=False, + ) + + ### disable so it doesn't create it's own notification + settings.ENABLE_PACKAGING_NOTIFICATIONS = False + envelope = successful_envelope_factory() + packaged_wb = PackagedWorkBasket.objects.get( + envelope=envelope, + ) + return ( + factories.EnvelopeAcceptedNotificationFactory( + notified_object_pk=packaged_wb.id, + ), + present_email, + not_present_email, + ) + + +@pytest.fixture() +def rejected_packaging_notification(failed_envelope_factory, settings): + present_email = f"packaging@email.co.uk" # /PS-IGNORE + not_present_email = f"no_packaging@email.co.uk" # /PS-IGNORE + factories.NotifiedUserFactory( + email=present_email, + ) + factories.NotifiedUserFactory( + email=not_present_email, + enrol_packaging=False, + ) + + ### disable so it doesn't create it's own notification + settings.ENABLE_PACKAGING_NOTIFICATIONS = False + envelope = failed_envelope_factory() + packaged_wb = PackagedWorkBasket.objects.get( + envelope=envelope, + ) + return ( + factories.EnvelopeRejectedNotificationFactory( + notified_object_pk=packaged_wb.id, + ), + present_email, + not_present_email, ) diff --git a/notifications/tests/test_models.py b/notifications/tests/test_models.py index 145981eb2..04ec867d2 100644 --- a/notifications/tests/test_models.py +++ b/notifications/tests/test_models.py @@ -14,16 +14,19 @@ 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 + ( + notification, + expected_present_email, + expected_not_present_email, + ) = goods_report_notification - users = goods_report_notification.notified_users() + users = notification.notified_users() for user in users: assert user.email == expected_present_email assert user.email != expected_not_present_email - import_batch = goods_report_notification.notified_object() + import_batch = notification.notified_object() assert isinstance(import_batch, ImportBatch) return_value = { @@ -36,7 +39,7 @@ def test_create_goods_report_notification(goods_report_notification): "notifications.models.prepare_link_to_file", return_value=return_value, ) as mocked_prepare_link_to_file: - personalisation = goods_report_notification.get_personalisation() + personalisation = notification.get_personalisation() assert personalisation == { "tgb_id": import_batch.name, @@ -47,21 +50,24 @@ def test_create_goods_report_notification(goods_report_notification): 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 + ( + notification, + expected_present_email, + expected_not_present_email, + ) = ready_for_packaging_notification - users = ready_for_packaging_notification.notified_users() + users = 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(), + notification.notified_object(), PackagedWorkBasket, ) - content = ready_for_packaging_notification.get_personalisation() + content = notification.get_personalisation() assert content == { "envelope_id": "230001", "description": "", @@ -73,30 +79,123 @@ def test_create_packaging_notification(ready_for_packaging_notification): } +def test_create_accepted_envelope(accepted_packaging_notification): + """Test that the creating a notification correctly assigns users.""" + + ( + notification, + expected_present_email, + expected_not_present_email, + ) = accepted_packaging_notification + + users = notification.notified_users() + + for user in users: + assert user.email == expected_present_email + assert user.email != expected_not_present_email + + assert isinstance( + notification.notified_object(), + PackagedWorkBasket, + ) + + content = notification.get_personalisation() + assert "envelope_id" in content and content["envelope_id"] == "230001" + assert "transaction_count" in content and content["transaction_count"] == 1 + assert ( + "loading_report_message" in content + and content["loading_report_message"] + == "Loading report(s): REPORT_DBT23000.html" + ) + assert "comments" in content + + +def test_create_rejected_envelope(rejected_packaging_notification): + """Test that the creating a notification correctly assigns users.""" + + ( + notification, + expected_present_email, + expected_not_present_email, + ) = rejected_packaging_notification + + users = notification.notified_users() + + for user in users: + assert user.email == expected_present_email + assert user.email != expected_not_present_email + + assert isinstance( + notification.notified_object(), + PackagedWorkBasket, + ) + + content = notification.get_personalisation() + assert "envelope_id" in content and content["envelope_id"] == "230001" + assert "transaction_count" in content and content["transaction_count"] == 1 + assert ( + "loading_report_message" in content + and content["loading_report_message"] + == "Loading report(s): REPORT_DBT23001.html" + ) + assert "comments" in content + + 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 + ( + notification, + expected_present_email, + expected_not_present_email, + ) = successful_publishing_notification - users = successful_publishing_notification.notified_users() + users = 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(), + notification.notified_object(), CrownDependenciesEnvelope, ) - content = successful_publishing_notification.get_personalisation() + content = notification.get_personalisation() + assert content == {"envelope_id": "230001"} + + +def test_create_failed_publishing_notification(failed_publishing_notification): + """Test that the creating a notification correctly assigns users.""" + + ( + notification, + expected_present_email, + expected_not_present_email, + ) = failed_publishing_notification + + users = notification.notified_users() + + for user in users: + assert user.email == expected_present_email + assert user.email != expected_not_present_email + + assert isinstance( + notification.notified_object(), + CrownDependenciesEnvelope, + ) + + content = notification.get_personalisation() assert content == {"envelope_id": "230001"} 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 + ( + notification, + expected_present_email, + expected_not_present_email, + ) = ready_for_packaging_notification + with patch( "notifications.models.send_emails", return_value={ @@ -105,11 +204,11 @@ def test_send_notification_emails(ready_for_packaging_notification): "failed_recipients": "", }, ) as mocked_send_emails: - ready_for_packaging_notification.send_notification_emails() + notification.send_notification_emails() mocked_send_emails.assert_called_once() log_success = NotificationLog.objects.get( - notification=ready_for_packaging_notification, + notification=notification, recipients=expected_present_email, success=True, ) @@ -124,11 +223,11 @@ def test_send_notification_emails(ready_for_packaging_notification): "failed_recipients": " \n".join([expected_present_email]), }, ) as mocked_send_emails: - ready_for_packaging_notification.send_notification_emails() + notification.send_notification_emails() mocked_send_emails.assert_called_once() log_fail = NotificationLog.objects.get( - notification=ready_for_packaging_notification, + notification=notification, success=False, ) diff --git a/notifications/tests/test_tasks.py b/notifications/tests/test_tasks.py index a0e82bd00..6b2a37284 100644 --- a/notifications/tests/test_tasks.py +++ b/notifications/tests/test_tasks.py @@ -14,8 +14,11 @@ def test_send_emails_goods_report_notification( ): """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 + ( + notification, + expected_present_email, + expected_not_present_email, + ) = goods_report_notification return_value = { "file": "VGVzdA==", @@ -37,19 +40,20 @@ def test_send_emails_goods_report_notification( ) as mocked_send_emails: tasks.send_emails_task.apply( kwargs={ - "notification_pk": goods_report_notification.id, + "notification_pk": 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, + notification=notification, recipients=expected_present_email, success=True, ) - assert expected_unenrolled_email not in log.recipients + assert expected_present_email in log.recipients + assert expected_not_present_email not in log.recipients def test_send_emails_packaging_notification( @@ -58,8 +62,11 @@ def test_send_emails_packaging_notification( """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 = "packaging@email.co.uk" # /PS-IGNORE - expected_unenrolled_email = "no_packaging@email.co.uk" # /PS-IGNORE + ( + notification, + expected_present_email, + expected_not_present_email, + ) = ready_for_packaging_notification with patch( "notifications.models.send_emails", @@ -71,18 +78,19 @@ def test_send_emails_packaging_notification( ) as mocked_send_emails: tasks.send_emails_task.apply( kwargs={ - "notification_pk": ready_for_packaging_notification.id, + "notification_pk": notification.id, }, ) mocked_send_emails.assert_called_once() log = models.NotificationLog.objects.get( - notification=ready_for_packaging_notification, + notification=notification, recipients=expected_present_email, success=True, ) - assert expected_unenrolled_email not in log.recipients + assert expected_present_email in log.recipients + assert expected_not_present_email not in log.recipients def test_send_emails_publishing_notification( @@ -92,8 +100,11 @@ def test_send_emails_publishing_notification( """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 + ( + notification, + expected_present_email, + expected_not_present_email, + ) = successful_publishing_notification with patch( "notifications.models.send_emails", @@ -105,15 +116,16 @@ def test_send_emails_publishing_notification( ) as mocked_send_emails: tasks.send_emails_task.apply( kwargs={ - "notification_pk": successful_publishing_notification.id, + "notification_pk": notification.id, }, ) mocked_send_emails.assert_called_once() log = models.NotificationLog.objects.get( - notification=successful_publishing_notification, + notification=notification, recipients=expected_present_email, success=True, ) - assert expected_unenrolled_email not in log.recipients + assert expected_present_email in log.recipients + assert expected_not_present_email not in log.recipients diff --git a/publishing/models/crown_dependencies_envelope.py b/publishing/models/crown_dependencies_envelope.py index 487e58dee..aab4640c7 100644 --- a/publishing/models/crown_dependencies_envelope.py +++ b/publishing/models/crown_dependencies_envelope.py @@ -158,7 +158,6 @@ def notify_publishing_success(self): notification = CrownDependenciesEnvelopeSuccessNotification( notified_object_pk=self.pk, ) - print(notification) notification.save() notification.schedule_send_emails() From 3c75451231d10fdcec588657dcb8ea02bda2b570 Mon Sep 17 00:00:00 2001 From: Dale Cannon <118175145+dalecannon@users.noreply.github.com> Date: Fri, 22 Sep 2023 10:24:49 +0100 Subject: [PATCH 2/4] TP2000-1048 Add geo area exclusions step to multiple measure edit journey (#1039) * Add MeasuresEditGeographicalArea form * Add geo area step to MeasureEditWizard * Add form test * Add view test * Allow only exclusions to be edited * Update tests * Set queryset in __init__ to fix failing CI/CD tests * Set default queryset to fix tests * Remove trailing comma * Mock diff_components for multiple measures edit tests * Remove superfluous constant * Add comment * Remove unneeded if block --- measures/conditions.py | 9 + measures/constants.py | 4 + measures/forms.py | 39 +++++ .../measures/edit-multiple-formset.jinja | 58 +++++++ measures/tests/test_forms.py | 26 +++ measures/tests/test_views.py | 94 +++++++++++ measures/views.py | 156 ++++++++++++++++-- 7 files changed, 368 insertions(+), 18 deletions(-) create mode 100644 measures/jinja2/measures/edit-multiple-formset.jinja diff --git a/measures/conditions.py b/measures/conditions.py index e8a3b0c32..e63d8988a 100644 --- a/measures/conditions.py +++ b/measures/conditions.py @@ -34,10 +34,19 @@ def show_step_duties(wizard): return MeasureEditSteps.DUTIES.value in cleaned_data.get("fields_to_edit") +def show_step_geographical_area_exclusions(wizard): + cleaned_data = wizard.get_cleaned_data_for_step(START) + if cleaned_data: + return MeasureEditSteps.GEOGRAPHICAL_AREA_EXCLUSIONS.value in cleaned_data.get( + "fields_to_edit", + ) + + measure_edit_condition_dict = { MeasureEditSteps.START_DATE: show_step_start_date, MeasureEditSteps.END_DATE: show_step_end_date, MeasureEditSteps.QUOTA_ORDER_NUMBER: show_step_quota_order_number, MeasureEditSteps.REGULATION: show_step_regulation, MeasureEditSteps.DUTIES: show_step_duties, + MeasureEditSteps.GEOGRAPHICAL_AREA_EXCLUSIONS: show_step_geographical_area_exclusions, } diff --git a/measures/constants.py b/measures/constants.py index fa05e96a0..afaf7318b 100644 --- a/measures/constants.py +++ b/measures/constants.py @@ -9,3 +9,7 @@ class MeasureEditSteps(models.TextChoices): QUOTA_ORDER_NUMBER = ("quota_order_number", "Quota order number") REGULATION = ("regulation", "Regulation") DUTIES = ("duties", "Duties") + GEOGRAPHICAL_AREA_EXCLUSIONS = ( + "geographical_area_exclusions", + "Geographical area exclusions", + ) diff --git a/measures/forms.py b/measures/forms.py index 1468561bb..c3dc0778f 100644 --- a/measures/forms.py +++ b/measures/forms.py @@ -1634,3 +1634,42 @@ def clean(self): validate_duties(duties, measure.valid_between.lower) return cleaned_data + + +class MeasureGeographicalAreaExclusionsForm(forms.Form): + excluded_area = forms.ModelChoiceField( + label="", + queryset=GeographicalArea.objects.all(), + help_text="Select a geographical area to be excluded", + required=False, + ) + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + + self.fields["excluded_area"].queryset = ( + GeographicalArea.objects.current() + .with_latest_description() + .as_at_today_and_beyond() + .order_by("description") + ) + + self.fields[ + "excluded_area" + ].label_from_instance = lambda obj: f"{obj.area_id} - {obj.description}" + + self.helper = FormHelper(self) + self.helper.form_tag = False + self.helper.legend_size = Size.SMALL + self.helper.layout = Layout( + Fieldset( + "excluded_area", + ), + ) + + +class MeasureGeographicalAreaExclusionsFormSet(FormSet): + """Allows editing the geographical area exclusions of multiple measures in + `MeasureEditWizard`.""" + + form = MeasureGeographicalAreaExclusionsForm diff --git a/measures/jinja2/measures/edit-multiple-formset.jinja b/measures/jinja2/measures/edit-multiple-formset.jinja new file mode 100644 index 000000000..797cdbfb3 --- /dev/null +++ b/measures/jinja2/measures/edit-multiple-formset.jinja @@ -0,0 +1,58 @@ +{% extends "measures/edit-wizard-step.jinja" %} + +{% from "components/fieldset/macro.njk" import govukFieldset %} +{% from "components/error-summary/macro.njk" import govukErrorSummary %} + +{% block form %} + {% set formset = form %} + {{ crispy(formset.management_form, no_form_tags) }} + + {% if formset.non_form_errors() %} + {% set error_list = [] %} + {% for error in formset.non_form_errors() %} + {{ error_list.append({ + "html": '

' ~ error ~ '

', + }) or "" }} + {% endfor %} + + {{ govukErrorSummary({ + "titleText": "There is a problem", + "errorList": error_list + }) }} + {% endif %} + + {% for form in formset %} + {% if form.non_field_errors() %} + {% set error_list = [] %} + {% for error in form.non_field_errors() %} + {{ error_list.append({ + "html": '

' ~ error ~ '

', + }) or "" }} + {% endfor %} + + {{ govukErrorSummary({ + "titleText": "There is a problem", + "errorList": error_list + }) }} + {% endif %} + {{ crispy(form) }} + {% endfor %} + + {% if formset.data[formset.prefix ~ "-ADD"] %} + {{ crispy(formset.empty_form)|replace("__prefix__", formset.forms|length)|safe }} + {% endif %} + +
+ {{ govukButton({"text": "Save"}) }} + + {% if formset.total_form_count() + 1 < formset.max_num %} + {{ govukButton({ + "text": "Add another", + "attributes": {"id": "add-new"}, + "classes": "govuk-button--secondary", + "value": "1", + "name": formset.prefix ~ "-ADD", + }) }} + {% endif %} +
+{% endblock %} diff --git a/measures/tests/test_forms.py b/measures/tests/test_forms.py index 85081fd57..a518f1e71 100644 --- a/measures/tests/test_forms.py +++ b/measures/tests/test_forms.py @@ -1582,3 +1582,29 @@ def test_measure_review_form_validates_components_applicability_exclusivity( "A duty cannot be specified on both commodities and conditions" in form.errors["__all__"] ) + + +def test_measure_geographical_area_exclusions_form_valid_choice(): + """Tests that `MeasureGeographicalAreaExclusionsForm` is valid when an + available geographical area is selected.""" + geo_area = factories.GeographicalAreaFactory.create() + data = { + "excluded_area": geo_area.pk, + } + with override_current_transaction(geo_area.transaction): + form = forms.MeasureGeographicalAreaExclusionsForm(data) + assert form.is_valid() + assert form.cleaned_data["excluded_area"] == geo_area + + +def test_measure_geographical_area_exclusions_form_invalid_choice(): + """Tests that `MeasureGeographicalAreaExclusionsForm` raises an raises an + invalid choice error when an unavailable geographical area is selected.""" + data = { + "excluded_area": "geo_area", + } + form = forms.MeasureGeographicalAreaExclusionsForm(data) + assert not form.is_valid() + assert form.errors["excluded_area"] == [ + "Select a valid choice. That choice is not one of the available choices.", + ] diff --git a/measures/tests/test_views.py b/measures/tests/test_views.py index e09a28733..99469d3ed 100644 --- a/measures/tests/test_views.py +++ b/measures/tests/test_views.py @@ -47,6 +47,17 @@ pytestmark = pytest.mark.django_db +@pytest.fixture() +def mocked_diff_components(): + """Mocks `diff_components()` inside `update_measure_components()` in + `MeasureEditWizard` to prevent parsing errors where test measures lack a + duty sentence.""" + with patch( + "measures.views.MeasureEditWizard.update_measure_components", + ) as update_measure_components: + yield update_measure_components + + def test_measure_footnotes_update_get_delete_key(): footnote_key = "form-0-footnote" expected = "form-0-DELETE" @@ -1718,6 +1729,7 @@ def test_measuretype_api_list_view(valid_user_client): def test_multiple_measure_start_and_end_date_edit_functionality( valid_user_client, session_workbasket, + mocked_diff_components, ): """Tests that MeasureEditWizard takes a list of measures, and sets their update type to update, updates their end dates and start dates, and clears @@ -1841,6 +1853,7 @@ def test_multiple_measure_edit_single_form_functionality( data, valid_user_client, session_workbasket, + mocked_diff_components, ): """Tests that MeasureEditWizard takes a list of measures, and sets their update type to update, updates their end dates and start dates, and clears @@ -1913,6 +1926,7 @@ def test_multiple_measure_edit_single_form_functionality( def test_multiple_measure_edit_only_regulation( valid_user_client, session_workbasket, + mocked_diff_components, ): """Tests the regulation step in MeasureEditWizard.""" measure_1 = factories.MeasureFactory.create() @@ -2147,6 +2161,7 @@ def test_measure_list_selected_measures_list(valid_user_client): def test_multiple_measure_edit_only_quota_order_number( valid_user_client, session_workbasket, + mocked_diff_components, ): """Tests the regulation step in MeasureEditWizard.""" measure_1 = factories.MeasureFactory.create() @@ -2298,6 +2313,7 @@ def test_multiple_measure_edit_only_duties( def test_multiple_measure_edit_preserves_footnote_associations( valid_user_client, session_workbasket, + mocked_diff_components, ): """Tests that footnote associations are preserved in MeasureEditWizard.""" @@ -2375,6 +2391,84 @@ def test_multiple_measure_edit_preserves_footnote_associations( assert footnote in expected_footnotes +def test_multiple_measure_edit_geographical_area_exclusions( + valid_user_client, + session_workbasket, + mocked_diff_components, +): + """Tests that the geographical area exclusions of multiple measures can be + edited in `MeasureEditWizard`.""" + measure_1 = factories.MeasureFactory.create(with_exclusion=True) + measure_2 = factories.MeasureFactory.create() + new_excluded_area = factories.CountryFactory.create() + + url = reverse("measure-ui-edit-multiple") + session = valid_user_client.session + session.update( + { + "workbasket": { + "id": session_workbasket.pk, + }, + "MULTIPLE_MEASURE_SELECTIONS": { + measure_1.pk: 1, + measure_2.pk: 1, + }, + }, + ) + session.save() + + STEP_KEY = "measure_edit_wizard-current_step" + + wizard_data = [ + { + "data": { + STEP_KEY: START, + "start-fields_to_edit": [MeasureEditSteps.GEOGRAPHICAL_AREA_EXCLUSIONS], + }, + "next_step": MeasureEditSteps.GEOGRAPHICAL_AREA_EXCLUSIONS, + }, + { + "data": { + STEP_KEY: MeasureEditSteps.GEOGRAPHICAL_AREA_EXCLUSIONS, + "form-0-excluded_area": new_excluded_area.pk, + }, + "next_step": "complete", + }, + ] + for step_data in wizard_data: + url = reverse( + "measure-ui-edit-multiple", + kwargs={"step": step_data["data"][STEP_KEY]}, + ) + response = valid_user_client.get(url) + assert response.status_code == 200 + + response = valid_user_client.post(url, step_data["data"]) + assert response.status_code == 302 + + assert response.url == reverse( + "measure-ui-edit-multiple", + kwargs={"step": step_data["next_step"]}, + ) + + complete_response = valid_user_client.get(response.url) + assert complete_response.status_code == 302 + assert valid_user_client.session["MULTIPLE_MEASURE_SELECTIONS"] == {} + + workbasket_measures = Measure.objects.filter( + transaction__workbasket=session_workbasket, + ) + assert workbasket_measures + + with override_current_transaction(Transaction.objects.last()): + for measure in workbasket_measures: + assert measure.update_type == UpdateType.UPDATE + assert ( + measure.exclusions.current().first().excluded_geographical_area + == new_excluded_area + ) + + def test_measure_list_redirects_to_search_with_no_params(valid_user_client): response = valid_user_client.get(reverse("measure-ui-list")) assert response.status_code == 302 diff --git a/measures/views.py b/measures/views.py index ff13f6daa..ff7423c8f 100644 --- a/measures/views.py +++ b/measures/views.py @@ -3,6 +3,7 @@ from itertools import groupby from operator import attrgetter from typing import Any +from typing import List from typing import Type from urllib.parse import urlencode @@ -34,6 +35,8 @@ from common.views import TamatoListView from common.views import TrackedModelDetailMixin from common.views import TrackedModelDetailView +from geo_areas.models import GeographicalArea +from geo_areas.utils import get_all_members_of_geo_groups from measures import forms from measures.constants import START from measures.constants import MeasureEditSteps @@ -44,6 +47,7 @@ from measures.models import Measure from measures.models import MeasureActionPair from measures.models import MeasureConditionComponent +from measures.models import MeasureExcludedGeographicalArea from measures.models import MeasureType from measures.pagination import MeasurePaginator from measures.parsers import DutySentenceParser @@ -267,10 +271,15 @@ class MeasureEditWizard( (MeasureEditSteps.QUOTA_ORDER_NUMBER, forms.MeasureQuotaOrderNumberForm), (MeasureEditSteps.REGULATION, forms.MeasureRegulationForm), (MeasureEditSteps.DUTIES, forms.MeasureDutiesForm), + ( + MeasureEditSteps.GEOGRAPHICAL_AREA_EXCLUSIONS, + forms.MeasureGeographicalAreaExclusionsFormSet, + ), ] templates = { START: "measures/edit-multiple-start.jinja", + MeasureEditSteps.GEOGRAPHICAL_AREA_EXCLUSIONS: "measures/edit-multiple-formset.jinja", } step_metadata = { @@ -293,6 +302,9 @@ class MeasureEditWizard( MeasureEditSteps.DUTIES: { "title": "Edit the duties", }, + MeasureEditSteps.GEOGRAPHICAL_AREA_EXCLUSIONS: { + "title": "Edit the geographical area exclusions", + }, } def get_template_names(self): @@ -313,7 +325,11 @@ def get_context_data(self, form, **kwargs): def get_form_kwargs(self, step): kwargs = {} - if step not in [START, MeasureEditSteps.QUOTA_ORDER_NUMBER]: + if step not in [ + START, + MeasureEditSteps.QUOTA_ORDER_NUMBER, + MeasureEditSteps.GEOGRAPHICAL_AREA_EXCLUSIONS, + ]: kwargs["selected_measures"] = self.get_queryset() if step == MeasureEditSteps.DUTIES: @@ -328,6 +344,103 @@ def get_form_kwargs(self, step): return kwargs + def update_measure_components( + self, + measure: Measure, + duties: str, + workbasket: WorkBasket, + ): + """Updates the measure components associated to the measure.""" + diff_components( + instance=measure, + duty_sentence=duties if duties else measure.duty_sentence, + start_date=measure.valid_between.lower, + workbasket=workbasket, + transaction=workbasket.current_transaction, + ) + + def update_measure_condition_components( + self, + measure: Measure, + workbasket: WorkBasket, + ): + """Updates the measure condition components associated to the + measure.""" + conditions = measure.conditions.current() + for condition in conditions: + condition.new_version( + dependent_measure=measure, + workbasket=workbasket, + ) + + def update_measure_excluded_geographical_areas( + self, + edited: bool, + measure: Measure, + exclusions: List[GeographicalArea], + workbasket: WorkBasket, + ): + """Updates the excluded geographical areas associated to the measure.""" + existing_exclusions = measure.exclusions.current() + + # Update any exclusions to new measure version + if not edited: + for exclusion in existing_exclusions: + exclusion.new_version( + modified_measure=measure, + workbasket=workbasket, + ) + return + + new_excluded_areas = get_all_members_of_geo_groups( + validity=measure.valid_between, + geo_areas=exclusions, + ) + + for geo_area in new_excluded_areas: + existing_exclusion = existing_exclusions.filter( + excluded_geographical_area=geo_area, + ).first() + if existing_exclusion: + existing_exclusion.new_version( + modified_measure=measure, + workbasket=workbasket, + ) + else: + MeasureExcludedGeographicalArea.objects.create( + modified_measure=measure, + excluded_geographical_area=geo_area, + update_type=UpdateType.CREATE, + transaction=workbasket.new_transaction(), + ) + + removed_excluded_areas = { + e.excluded_geographical_area for e in existing_exclusions + }.difference(set(exclusions)) + + exclusions_to_remove = [ + existing_exclusions.get(excluded_geographical_area__id=geo_area.id) + for geo_area in removed_excluded_areas + ] + + for exclusion in exclusions_to_remove: + exclusion.new_version( + update_type=UpdateType.DELETE, + modified_measure=measure, + workbasket=workbasket, + ) + + def update_measure_footnote_associations(self, measure, workbasket): + """Updates the footnotes associated to the measure.""" + footnote_associations = FootnoteAssociationMeasure.objects.current().filter( + footnoted_measure__sid=measure.sid, + ) + for fa in footnote_associations: + fa.new_version( + footnoted_measure=measure, + workbasket=workbasket, + ) + def done(self, form_list, **kwargs): cleaned_data = self.get_all_cleaned_data() selected_measures = self.get_queryset() @@ -349,6 +462,10 @@ def done(self, form_list, **kwargs): new_quota_order_number = cleaned_data.get("order_number", None) new_generating_regulation = cleaned_data.get("generating_regulation", None) new_duties = cleaned_data.get("duties", None) + new_exclusions = [ + e["excluded_area"] + for e in cleaned_data.get("formset-geographical_area_exclusions", []) + ] for measure in selected_measures: new_measure = measure.new_version( workbasket=workbasket, @@ -366,24 +483,27 @@ def done(self, form_list, **kwargs): if new_generating_regulation else measure.generating_regulation, ) - if new_duties: - diff_components( - instance=new_measure, - duty_sentence=new_duties, - start_date=new_measure.valid_between.lower, - workbasket=workbasket, - transaction=workbasket.current_transaction, - ) - footnote_associations = FootnoteAssociationMeasure.objects.current().filter( - footnoted_measure=measure, + self.update_measure_components( + measure=new_measure, + duties=new_duties, + workbasket=workbasket, ) - for fa in footnote_associations: - fa.new_version( - workbasket=workbasket, - update_type=UpdateType.UPDATE, - footnoted_measure=new_measure, - ) - self.session_store.clear() + self.update_measure_condition_components( + measure=new_measure, + workbasket=workbasket, + ) + self.update_measure_excluded_geographical_areas( + edited="geographical_area_exclusions" + in cleaned_data.get("fields_to_edit", []), + measure=new_measure, + exclusions=new_exclusions, + workbasket=workbasket, + ) + self.update_measure_footnote_associations( + measure=new_measure, + workbasket=workbasket, + ) + self.session_store.clear() return redirect(reverse("workbaskets:review-workbasket")) From fd99858e5db243b5c8d35e2ebb402a106253eaee Mon Sep 17 00:00:00 2001 From: Charlie Prichard <46421052+CPrich905@users.noreply.github.com> Date: Fri, 22 Sep 2023 12:19:56 +0100 Subject: [PATCH 3/4] TP2000-1043 Final implementation changes (#1038) * applyinng ticket changes * code review fixes * tidy up --- common/static/common/js/application.js | 4 +++- common/static/common/js/openCloseAccordion.js | 16 ++++++++++++++++ common/static/common/scss/_accordion.scss | 9 +++++++++ measures/forms.py | 14 +++++++++++--- measures/jinja2/measures/list.jinja | 4 ---- measures/jinja2/measures/search.jinja | 1 - 6 files changed, 39 insertions(+), 9 deletions(-) create mode 100644 common/static/common/js/openCloseAccordion.js diff --git a/common/static/common/js/application.js b/common/static/common/js/application.js index 5fd28c541..499632ab2 100644 --- a/common/static/common/js/application.js +++ b/common/static/common/js/application.js @@ -12,6 +12,7 @@ import { initAll } from 'govuk-frontend'; import initCheckboxes from './checkboxes'; import initConditionalMeasureConditions from './conditionalMeasureConditions'; import initFilterDisabledToggleForComCode from './conditionalDisablingFilters' +import initOpenCloseAccordionSection from './openCloseAccordion'; showHideCheckboxes(); // Initialise accessible-autocomplete components without a `name` attr in order // to avoid the "dummy" autocomplete field being submitted as part of the form @@ -24,4 +25,5 @@ initAll(); initCheckboxes(); initConditionalMeasureConditions(); initAutocompleteProgressiveEnhancement(); -initFilterDisabledToggleForComCode(); \ No newline at end of file +initFilterDisabledToggleForComCode(); +initOpenCloseAccordionSection(); \ No newline at end of file diff --git a/common/static/common/js/openCloseAccordion.js b/common/static/common/js/openCloseAccordion.js new file mode 100644 index 000000000..df2e08989 --- /dev/null +++ b/common/static/common/js/openCloseAccordion.js @@ -0,0 +1,16 @@ +// TODO: adjust the pathname filter to account for all objects search/filter pages +const initOpenCloseAccordionSection = () => { + document.addEventListener('DOMContentLoaded', function() { + + let expandedSection = document.getElementById('accordion-open-close-section') + let pathname = window.location.pathname + if(expandedSection){ + if (!pathname.includes('search')){ + expandedSection.classList.remove('govuk-accordion__section--expanded'); + } + } + } + ) +} + +export default initOpenCloseAccordionSection; \ No newline at end of file diff --git a/common/static/common/scss/_accordion.scss b/common/static/common/scss/_accordion.scss index 7665693e7..29fd9d7b1 100644 --- a/common/static/common/scss/_accordion.scss +++ b/common/static/common/scss/_accordion.scss @@ -5,3 +5,12 @@ .govuk-accordion__section-content > :last-child { @extend .govuk-\!-margin-bottom-6; } + +.black-label--no-button { + .govuk-accordion__section-button { + color: black !important; + } + .govuk-accordion__icon { + display: none !important; + } +} \ No newline at end of file diff --git a/measures/forms.py b/measures/forms.py index c3dc0778f..0a17e6b10 100644 --- a/measures/forms.py +++ b/measures/forms.py @@ -777,13 +777,17 @@ def __init__(self, *args, **kwargs): self.helper.layout = Layout( Accordion( AccordionSection( - "Select one or more options to search", + "Search and filter", + HTML( + '

Select one or more options to search

', + ), Div( Div( Div( "goods_nomenclature", Field.text("sid", field_width=Fluid.TWO_THIRDS), "regulation", + "footnote", css_class="govuk-grid-column-one-third", ), Div( @@ -813,6 +817,9 @@ def __init__(self, *args, **kwargs): ), Div( "modc", + HTML( + "

To use the 'Include inherited measures' filter, enter a valid commodity code in the 'Select commodity code' filter above

", + ), css_class="govuk-grid-column-full form-group-margin-bottom-2", ), css_class="govuk-grid-row govuk-!-margin-top-6", @@ -840,7 +847,7 @@ def __init__(self, *args, **kwargs): "end_date", css_class="govuk-grid-column-one-half form-group-margin-bottom-2", ), - css_class="govuk-grid-row govuk-!-padding-top-6 filter-layout__filters", + css_class="govuk-grid-row govuk-!-padding-top-6", ), Div( Div( @@ -857,7 +864,8 @@ def __init__(self, *args, **kwargs): css_class="govuk-grid-row govuk-!-padding-top-3", ), ), - css_class="govuk-grid-row govuk-!-padding-3", + css_class="govuk-grid-row govuk-!-padding-3 black-label--no-button govuk-accordion__section--expanded", + id="accordion-open-close-section", ), ), ) diff --git a/measures/jinja2/measures/list.jinja b/measures/jinja2/measures/list.jinja index 3c79c71f5..663bf38a3 100644 --- a/measures/jinja2/measures/list.jinja +++ b/measures/jinja2/measures/list.jinja @@ -15,10 +15,6 @@
-

Search and filter

-

- Select one or more options to search -

{{ crispy(filter.form) }}
diff --git a/measures/jinja2/measures/search.jinja b/measures/jinja2/measures/search.jinja index bf280f75f..3c6334149 100644 --- a/measures/jinja2/measures/search.jinja +++ b/measures/jinja2/measures/search.jinja @@ -18,7 +18,6 @@
-

Search and filter

{{ crispy(filter.form) }}
From 037876194557b8e444c0be4703a2e264e2d56b7a Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 26 Sep 2023 10:02:21 +0100 Subject: [PATCH 4/4] Bump gevent from 21.12.0 to 23.9.1 (#1053) Bumps [gevent](https://github.com/gevent/gevent) from 21.12.0 to 23.9.1. - [Release notes](https://github.com/gevent/gevent/releases) - [Changelog](https://github.com/gevent/gevent/blob/master/docs/changelog_pre.rst) - [Commits](https://github.com/gevent/gevent/compare/21.12.0...23.9.1) --- updated-dependencies: - dependency-name: gevent dependency-type: direct:production ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index cd109a217..5a3798ba8 100644 --- a/requirements.txt +++ b/requirements.txt @@ -39,7 +39,7 @@ elastic-apm==6.7.2 factory-boy==3.2.0 flower==1.2.0 freezegun==1.1.0 -gevent==21.12.0 +gevent==23.9.1 govuk-frontend-jinja @ git+https://github.com/alphagov/govuk-frontend-jinja.git@15845e4cca3a05df72c6e13ec6a7e35acc682f52 govuk-tech-docs-sphinx-theme==1.0.0 gunicorn==20.1.0