From 2c652ea121c4b2fa662cb67c7fd0b8f457906c5b Mon Sep 17 00:00:00 2001 From: Dale Cannon Date: Mon, 18 Sep 2023 12:58:33 +0100 Subject: [PATCH 01/13] Add MeasuresEditGeographicalArea form --- measures/forms.py | 103 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/measures/forms.py b/measures/forms.py index 7faf482a0..2ce9fc059 100644 --- a/measures/forms.py +++ b/measures/forms.py @@ -1588,3 +1588,106 @@ def clean(self): validate_duties(duties, measure.valid_between.lower) return cleaned_data + + +class MeasuresEditGeographicalAreaForm( + MeasureGeoAreaInitialDataMixin, + BindNestedFormMixin, + forms.Form, +): + """Used in `MeasureEditWizard` to allow editing the geographical area and + exclusions of multiple measures.""" + + geo_area = RadioNested( + label="", + help_text=( + "Choose the geographical area to which the measures apply. " + "This can be a specific country or a group of countries, and exclusions can be specified. " + "The measures will only apply to imports from or exports to the selected area." + ), + choices=constants.GeoAreaType.choices, + nested_forms={ + constants.GeoAreaType.ERGA_OMNES.value: [ErgaOmnesExclusionsFormSet], + constants.GeoAreaType.GROUP.value: [ + GeoGroupForm, + GeoGroupExclusionsFormSet, + ], + constants.GeoAreaType.COUNTRY.value: [CountryRegionForm], + }, + error_messages={"required": "A geographical area must be selected"}, + ) + + @property + def geo_area_field_name(self): + return f"{self.prefix}-geo_area" + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + + geographical_area_fields = { + constants.GeoAreaType.ERGA_OMNES: self.erga_omnes_instance, + constants.GeoAreaType.GROUP: self.data.get( + f"{self.prefix}-geographical_area_group", + ), + constants.GeoAreaType.COUNTRY: self.data.get( + f"{self.prefix}-geographical_area_country_or_region", + ), + } + + self.fields["geo_area"].initial = self.data.get(f"{self.prefix}-geo_area") + + nested_forms_initial = {} + + if self.fields["geo_area"].initial: + nested_forms_initial["geographical_area"] = geographical_area_fields[ + self.fields["geo_area"].initial + ] + + geo_area_initial_data = self.get_geo_area_initial() + nested_forms_initial.update(geo_area_initial_data) + kwargs.pop("initial", None) + self.bind_nested_forms(*args, initial=nested_forms_initial, **kwargs) + + self.helper = FormHelper(self) + self.helper.form_tag = False + self.helper.legend_size = Size.SMALL + self.helper.layout = Layout( + "geo_area", + Submit( + "submit", + "Save", + data_module="govuk-button", + data_prevent_double_click="true", + ), + ) + + @property + def erga_omnes_instance(self): + return GeographicalArea.objects.current().erga_omnes().get() + + def clean(self): + cleaned_data = super().clean() + + geographical_area_fields = { + constants.GeoAreaType.ERGA_OMNES: self.erga_omnes_instance, + constants.GeoAreaType.GROUP: cleaned_data.get("geographical_area_group"), + constants.GeoAreaType.COUNTRY: cleaned_data.get( + "geographical_area_country_or_region", + ), + } + + if cleaned_data.get("geo_area"): + geo_area_choice = cleaned_data.get("geo_area") + cleaned_data["geographical_area"] = geographical_area_fields[ + geo_area_choice + ] + exclusions = cleaned_data.get( + constants.FORMSET_PREFIX_MAPPING[geo_area_choice], + ) + if exclusions: + cleaned_data["exclusions"] = [ + exclusion[constants.FIELD_NAME_MAPPING[geo_area_choice]] + for exclusion in exclusions + ] + + return cleaned_data From 4cbd47a5bc575b528e68bf8c68000608de77d9c1 Mon Sep 17 00:00:00 2001 From: Dale Cannon Date: Mon, 18 Sep 2023 13:00:00 +0100 Subject: [PATCH 02/13] Add geo area step to MeasureEditWizard --- measures/conditions.py | 9 +++++ measures/constants.py | 1 + measures/views.py | 81 ++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 89 insertions(+), 2 deletions(-) diff --git a/measures/conditions.py b/measures/conditions.py index e8a3b0c32..54ac858a1 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(wizard): + cleaned_data = wizard.get_cleaned_data_for_step(START) + if cleaned_data: + return MeasureEditSteps.GEOGRAPHICAL_AREA.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: show_step_geographical_area, } diff --git a/measures/constants.py b/measures/constants.py index fa05e96a0..1fe850c9f 100644 --- a/measures/constants.py +++ b/measures/constants.py @@ -9,3 +9,4 @@ class MeasureEditSteps(models.TextChoices): QUOTA_ORDER_NUMBER = ("quota_order_number", "Quota order number") REGULATION = ("regulation", "Regulation") DUTIES = ("duties", "Duties") + GEOGRAPHICAL_AREA = ("geographical_area", "Geographical area") diff --git a/measures/views.py b/measures/views.py index 71531e6ec..72369d206 100644 --- a/measures/views.py +++ b/measures/views.py @@ -34,6 +34,7 @@ from common.views import TamatoListView from common.views import TrackedModelDetailMixin from common.views import TrackedModelDetailView +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 +45,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,6 +269,7 @@ class MeasureEditWizard( (MeasureEditSteps.QUOTA_ORDER_NUMBER, forms.MeasureQuotaOrderNumberForm), (MeasureEditSteps.REGULATION, forms.MeasureRegulationForm), (MeasureEditSteps.DUTIES, forms.MeasureDutiesForm), + (MeasureEditSteps.GEOGRAPHICAL_AREA, forms.MeasuresEditGeographicalAreaForm), ] templates = { @@ -293,6 +296,10 @@ class MeasureEditWizard( MeasureEditSteps.DUTIES: { "title": "Edit the duties", }, + MeasureEditSteps.GEOGRAPHICAL_AREA: { + "title": "Edit the geographical area", + "link_text": "Geographical area", + }, } def get_template_names(self): @@ -313,7 +320,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, + ]: kwargs["selected_measures"] = self.get_queryset() if step == MeasureEditSteps.DUTIES: @@ -328,6 +339,62 @@ def get_form_kwargs(self, step): return kwargs + def update_measure_excluded_geographical_areas( + self, + measure, + exclusions, + workbasket, + ): + """Updates a measure's excluded geographical areas.""" + existing_exclusions = measure.exclusions.current() + + if not exclusions: + for exclusion in existing_exclusions: + exclusion.new_version( + update_type=UpdateType.DELETE, + 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 done(self, form_list, **kwargs): cleaned_data = self.get_all_cleaned_data() selected_measures = self.get_queryset() @@ -349,6 +416,8 @@ 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_geo_area = cleaned_data.get("geographical_area", None) + new_exclusions = cleaned_data.get("exclusions", None) for measure in selected_measures: new_measure = measure.new_version( workbasket=workbasket, @@ -365,6 +434,14 @@ def done(self, form_list, **kwargs): generating_regulation=new_generating_regulation if new_generating_regulation else measure.generating_regulation, + geographical_area=new_geo_area + if new_geo_area + else measure.geographical_area, + ) + self.update_measure_excluded_geographical_areas( + new_measure, + new_exclusions, + workbasket, ) if new_duties: diff_components( @@ -383,7 +460,7 @@ def done(self, form_list, **kwargs): update_type=UpdateType.UPDATE, footnoted_measure=new_measure, ) - self.session_store.clear() + self.session_store.clear() return redirect(reverse("workbaskets:review-workbasket")) From ff10ea670be4f683dd9f90c6a09bc8ba6794a2c4 Mon Sep 17 00:00:00 2001 From: Dale Cannon Date: Mon, 18 Sep 2023 13:00:41 +0100 Subject: [PATCH 03/13] Add form test --- measures/tests/test_forms.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/measures/tests/test_forms.py b/measures/tests/test_forms.py index 27ec2bf76..f10a69f86 100644 --- a/measures/tests/test_forms.py +++ b/measures/tests/test_forms.py @@ -1494,3 +1494,25 @@ 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_measures_edit_geographical_area_form_cleaned_data(erga_omnes): + """Tests that `MeasuresEditGeographicalArea` sets `cleaned_data` when passed + valid geographical area form data.""" + area_group = factories.GeoGroupFactory.create() + excluded_area = factories.CountryFactory.create() + + data = { + f"{GEO_AREA_FORM_PREFIX}-geo_area": constants.GeoAreaType.GROUP, + f"{GEO_AREA_FORM_PREFIX}-geographical_area_group": area_group.pk, + "geo_group_exclusions_formset-0-geo_group_exclusion": excluded_area.pk, + } + + with override_current_transaction(Transaction.objects.last()): + form = forms.MeasuresEditGeographicalAreaForm( + data=data, + prefix=GEO_AREA_FORM_PREFIX, + ) + assert form.is_valid() + assert form.cleaned_data["geographical_area"] == area_group + assert form.cleaned_data["exclusions"] == [excluded_area] From f47722a46ad6416f2a183e20fcfe6ab5c764a9de Mon Sep 17 00:00:00 2001 From: Dale Cannon Date: Mon, 18 Sep 2023 13:01:10 +0100 Subject: [PATCH 04/13] Add view test --- measures/tests/test_views.py | 91 +++++++++++++++++++++++++++++++++--- 1 file changed, 84 insertions(+), 7 deletions(-) diff --git a/measures/tests/test_views.py b/measures/tests/test_views.py index 344e90735..c7628eadf 100644 --- a/measures/tests/test_views.py +++ b/measures/tests/test_views.py @@ -1307,10 +1307,9 @@ def test_measure_form_wizard_create_measures( # Create measures returns a list of created measures measure_data = wizard.create_measures(form_data) measures = Measure.objects.filter(goods_nomenclature__in=[commodity1, commodity2]) - """ - In this implementation goods_nomenclature is a FK of Measure, so there is one measure - for each commodity specified in formset-commodities. + In this implementation goods_nomenclature is a FK of Measure, so there is + one measure for each commodity specified in formset-commodities. Verify that the expected measures were created. """ @@ -1535,10 +1534,7 @@ def test_measure_form_wizard_create_measures_with_tariff_suspension_action( # Create measures returns a list of created measures measure_data = wizard.create_measures(form_data) measures = Measure.objects.filter(goods_nomenclature=commodity1) - - """ - Verify that the expected measures were created. - """ + """Verify that the expected measures were created.""" assert len(measure_data) == 1 # Each created measure contains the supplied condition codes where DELETE=False @@ -2376,6 +2372,87 @@ def test_multiple_measure_edit_preserves_footnote_associations( assert footnote in expected_footnotes +def test_multiple_measure_edit_geographical_area_and_exclusions( + valid_user_client, + session_workbasket, + erga_omnes, +): + """Tests that the geographical area and exclusions of multiple measures can + be edited in `MeasureEditWizard`.""" + measure_1 = factories.MeasureFactory.create(with_exclusion=True) + measure_2 = factories.MeasureFactory.create(with_exclusion=True) + new_geo_area = erga_omnes + 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], + }, + "next_step": MeasureEditSteps.GEOGRAPHICAL_AREA, + }, + { + "data": { + STEP_KEY: MeasureEditSteps.GEOGRAPHICAL_AREA, + "geographical_area-geo_area": "ERGA_OMNES", + "erga_omnes_exclusions_formset-0-erga_omnes_exclusion": 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.geographical_area == new_geo_area + 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 From e615da9b37193bd47a2533899ed79022ec6f8e63 Mon Sep 17 00:00:00 2001 From: Dale Cannon Date: Wed, 20 Sep 2023 16:37:15 +0100 Subject: [PATCH 05/13] Allow only exclusions to be edited --- measures/conditions.py | 6 +- measures/constants.py | 6 +- measures/forms.py | 102 +++------------ .../measures/edit-multiple-formset.jinja | 58 +++++++++ measures/views.py | 118 +++++++++++++----- 5 files changed, 168 insertions(+), 122 deletions(-) create mode 100644 measures/jinja2/measures/edit-multiple-formset.jinja diff --git a/measures/conditions.py b/measures/conditions.py index 54ac858a1..e63d8988a 100644 --- a/measures/conditions.py +++ b/measures/conditions.py @@ -34,10 +34,10 @@ def show_step_duties(wizard): return MeasureEditSteps.DUTIES.value in cleaned_data.get("fields_to_edit") -def show_step_geographical_area(wizard): +def show_step_geographical_area_exclusions(wizard): cleaned_data = wizard.get_cleaned_data_for_step(START) if cleaned_data: - return MeasureEditSteps.GEOGRAPHICAL_AREA.value in cleaned_data.get( + return MeasureEditSteps.GEOGRAPHICAL_AREA_EXCLUSIONS.value in cleaned_data.get( "fields_to_edit", ) @@ -48,5 +48,5 @@ def show_step_geographical_area(wizard): MeasureEditSteps.QUOTA_ORDER_NUMBER: show_step_quota_order_number, MeasureEditSteps.REGULATION: show_step_regulation, MeasureEditSteps.DUTIES: show_step_duties, - MeasureEditSteps.GEOGRAPHICAL_AREA: show_step_geographical_area, + MeasureEditSteps.GEOGRAPHICAL_AREA_EXCLUSIONS: show_step_geographical_area_exclusions, } diff --git a/measures/constants.py b/measures/constants.py index 1fe850c9f..231607282 100644 --- a/measures/constants.py +++ b/measures/constants.py @@ -1,6 +1,7 @@ from django.db import models START = "start" +GEOGRAPHICAL_AREA_EXCLUSIONS = "geographical_area_exclusions" class MeasureEditSteps(models.TextChoices): @@ -9,4 +10,7 @@ class MeasureEditSteps(models.TextChoices): QUOTA_ORDER_NUMBER = ("quota_order_number", "Quota order number") REGULATION = ("regulation", "Regulation") DUTIES = ("duties", "Duties") - GEOGRAPHICAL_AREA = ("geographical_area", "Geographical area") + GEOGRAPHICAL_AREA_EXCLUSIONS = ( + "geographical_area_exclusions", + "Geographical area exclusions", + ) diff --git a/measures/forms.py b/measures/forms.py index 2ce9fc059..1506f7a0c 100644 --- a/measures/forms.py +++ b/measures/forms.py @@ -1590,104 +1590,36 @@ def clean(self): return cleaned_data -class MeasuresEditGeographicalAreaForm( - MeasureGeoAreaInitialDataMixin, - BindNestedFormMixin, - forms.Form, -): - """Used in `MeasureEditWizard` to allow editing the geographical area and - exclusions of multiple measures.""" - - geo_area = RadioNested( +class MeasureGeographicalAreaExclusionsForm(forms.Form): + excluded_area = forms.ModelChoiceField( label="", - help_text=( - "Choose the geographical area to which the measures apply. " - "This can be a specific country or a group of countries, and exclusions can be specified. " - "The measures will only apply to imports from or exports to the selected area." - ), - choices=constants.GeoAreaType.choices, - nested_forms={ - constants.GeoAreaType.ERGA_OMNES.value: [ErgaOmnesExclusionsFormSet], - constants.GeoAreaType.GROUP.value: [ - GeoGroupForm, - GeoGroupExclusionsFormSet, - ], - constants.GeoAreaType.COUNTRY.value: [CountryRegionForm], - }, - error_messages={"required": "A geographical area must be selected"}, + queryset=GeographicalArea.objects.current() + .with_latest_description() + .as_at_today_and_beyond() + .order_by("description"), + help_text="Select a geographical area to be excluded", + required=False, ) - @property - def geo_area_field_name(self): - return f"{self.prefix}-geo_area" - def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - geographical_area_fields = { - constants.GeoAreaType.ERGA_OMNES: self.erga_omnes_instance, - constants.GeoAreaType.GROUP: self.data.get( - f"{self.prefix}-geographical_area_group", - ), - constants.GeoAreaType.COUNTRY: self.data.get( - f"{self.prefix}-geographical_area_country_or_region", - ), - } - - self.fields["geo_area"].initial = self.data.get(f"{self.prefix}-geo_area") - - nested_forms_initial = {} - - if self.fields["geo_area"].initial: - nested_forms_initial["geographical_area"] = geographical_area_fields[ - self.fields["geo_area"].initial - ] - - geo_area_initial_data = self.get_geo_area_initial() - nested_forms_initial.update(geo_area_initial_data) - kwargs.pop("initial", None) - self.bind_nested_forms(*args, initial=nested_forms_initial, **kwargs) + 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( - "geo_area", - Submit( - "submit", - "Save", - data_module="govuk-button", - data_prevent_double_click="true", + Fieldset( + "excluded_area", ), ) - @property - def erga_omnes_instance(self): - return GeographicalArea.objects.current().erga_omnes().get() - - def clean(self): - cleaned_data = super().clean() - - geographical_area_fields = { - constants.GeoAreaType.ERGA_OMNES: self.erga_omnes_instance, - constants.GeoAreaType.GROUP: cleaned_data.get("geographical_area_group"), - constants.GeoAreaType.COUNTRY: cleaned_data.get( - "geographical_area_country_or_region", - ), - } - if cleaned_data.get("geo_area"): - geo_area_choice = cleaned_data.get("geo_area") - cleaned_data["geographical_area"] = geographical_area_fields[ - geo_area_choice - ] - exclusions = cleaned_data.get( - constants.FORMSET_PREFIX_MAPPING[geo_area_choice], - ) - if exclusions: - cleaned_data["exclusions"] = [ - exclusion[constants.FIELD_NAME_MAPPING[geo_area_choice]] - for exclusion in exclusions - ] +class MeasureGeographicalAreaExclusionsFormSet(FormSet): + """Allows editing the geographical area exclusions of multiple measures in + `MeasureEditWizard`.""" - return cleaned_data + 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/views.py b/measures/views.py index 72369d206..342dfd7a0 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,8 +35,10 @@ 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 GEOGRAPHICAL_AREA_EXCLUSIONS from measures.constants import START from measures.constants import MeasureEditSteps from measures.filters import MeasureFilter @@ -269,11 +272,15 @@ class MeasureEditWizard( (MeasureEditSteps.QUOTA_ORDER_NUMBER, forms.MeasureQuotaOrderNumberForm), (MeasureEditSteps.REGULATION, forms.MeasureRegulationForm), (MeasureEditSteps.DUTIES, forms.MeasureDutiesForm), - (MeasureEditSteps.GEOGRAPHICAL_AREA, forms.MeasuresEditGeographicalAreaForm), + ( + MeasureEditSteps.GEOGRAPHICAL_AREA_EXCLUSIONS, + forms.MeasureGeographicalAreaExclusionsFormSet, + ), ] templates = { START: "measures/edit-multiple-start.jinja", + GEOGRAPHICAL_AREA_EXCLUSIONS: "measures/edit-multiple-formset.jinja", } step_metadata = { @@ -296,9 +303,8 @@ class MeasureEditWizard( MeasureEditSteps.DUTIES: { "title": "Edit the duties", }, - MeasureEditSteps.GEOGRAPHICAL_AREA: { - "title": "Edit the geographical area", - "link_text": "Geographical area", + MeasureEditSteps.GEOGRAPHICAL_AREA_EXCLUSIONS: { + "title": "Edit the geographical area exclusions", }, } @@ -323,7 +329,7 @@ def get_form_kwargs(self, step): if step not in [ START, MeasureEditSteps.QUOTA_ORDER_NUMBER, - MeasureEditSteps.GEOGRAPHICAL_AREA, + MeasureEditSteps.GEOGRAPHICAL_AREA_EXCLUSIONS, ]: kwargs["selected_measures"] = self.get_queryset() @@ -339,15 +345,53 @@ 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, - measure, - exclusions, - workbasket, + edited: bool, + measure: Measure, + exclusions: List[GeographicalArea], + workbasket: WorkBasket, ): - """Updates a measure's excluded geographical areas.""" + """Updates the excluded geographical areas associated to the measure.""" existing_exclusions = measure.exclusions.current() + if not edited: + for exclusion in existing_exclusions: + exclusion.new_version( + modified_measure=measure, + workbasket=workbasket, + ) + return + if not exclusions: for exclusion in existing_exclusions: exclusion.new_version( @@ -395,6 +439,17 @@ def update_measure_excluded_geographical_areas( 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() @@ -416,8 +471,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_geo_area = cleaned_data.get("geographical_area", None) - new_exclusions = cleaned_data.get("exclusions", 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, @@ -434,32 +491,27 @@ def done(self, form_list, **kwargs): generating_regulation=new_generating_regulation if new_generating_regulation else measure.generating_regulation, - geographical_area=new_geo_area - if new_geo_area - else measure.geographical_area, + ) + self.update_measure_components( + measure=new_measure, + duties=new_duties, + workbasket=workbasket, + ) + self.update_measure_condition_components( + measure=new_measure, + workbasket=workbasket, ) self.update_measure_excluded_geographical_areas( - new_measure, - new_exclusions, - workbasket, + edited="geographical_area_exclusions" + in cleaned_data.get("fields_to_edit", []), + measure=new_measure, + exclusions=new_exclusions, + workbasket=workbasket, ) - 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_footnote_associations( + measure=new_measure, + workbasket=workbasket, ) - for fa in footnote_associations: - fa.new_version( - workbasket=workbasket, - update_type=UpdateType.UPDATE, - footnoted_measure=new_measure, - ) self.session_store.clear() return redirect(reverse("workbaskets:review-workbasket")) From 86b2b24b9824a9ab20b93d942f88cf9c36e2005a Mon Sep 17 00:00:00 2001 From: Dale Cannon Date: Wed, 20 Sep 2023 16:37:41 +0100 Subject: [PATCH 06/13] Update tests --- measures/tests/test_forms.py | 38 ++++++++++++++++++++---------------- measures/tests/test_views.py | 20 ++++++++----------- 2 files changed, 29 insertions(+), 29 deletions(-) diff --git a/measures/tests/test_forms.py b/measures/tests/test_forms.py index f10a69f86..e0981550b 100644 --- a/measures/tests/test_forms.py +++ b/measures/tests/test_forms.py @@ -1496,23 +1496,27 @@ def test_measure_review_form_validates_components_applicability_exclusivity( ) -def test_measures_edit_geographical_area_form_cleaned_data(erga_omnes): - """Tests that `MeasuresEditGeographicalArea` sets `cleaned_data` when passed - valid geographical area form data.""" - area_group = factories.GeoGroupFactory.create() - excluded_area = factories.CountryFactory.create() - +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 = { - f"{GEO_AREA_FORM_PREFIX}-geo_area": constants.GeoAreaType.GROUP, - f"{GEO_AREA_FORM_PREFIX}-geographical_area_group": area_group.pk, - "geo_group_exclusions_formset-0-geo_group_exclusion": excluded_area.pk, + "excluded_area": geo_area.pk, } - - with override_current_transaction(Transaction.objects.last()): - form = forms.MeasuresEditGeographicalAreaForm( - data=data, - prefix=GEO_AREA_FORM_PREFIX, - ) + with override_current_transaction(geo_area.transaction): + form = forms.MeasureGeographicalAreaExclusionsForm(data) assert form.is_valid() - assert form.cleaned_data["geographical_area"] == area_group - assert form.cleaned_data["exclusions"] == [excluded_area] + 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 c7628eadf..e7c3f14ae 100644 --- a/measures/tests/test_views.py +++ b/measures/tests/test_views.py @@ -2372,16 +2372,14 @@ def test_multiple_measure_edit_preserves_footnote_associations( assert footnote in expected_footnotes -def test_multiple_measure_edit_geographical_area_and_exclusions( +def test_multiple_measure_edit_geographical_area_exclusions( valid_user_client, session_workbasket, - erga_omnes, ): - """Tests that the geographical area and exclusions of multiple measures can - be edited in `MeasureEditWizard`.""" + """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(with_exclusion=True) - new_geo_area = erga_omnes + measure_2 = factories.MeasureFactory.create() new_excluded_area = factories.CountryFactory.create() url = reverse("measure-ui-edit-multiple") @@ -2405,15 +2403,14 @@ def test_multiple_measure_edit_geographical_area_and_exclusions( { "data": { STEP_KEY: START, - "start-fields_to_edit": [MeasureEditSteps.GEOGRAPHICAL_AREA], + "start-fields_to_edit": [MeasureEditSteps.GEOGRAPHICAL_AREA_EXCLUSIONS], }, - "next_step": MeasureEditSteps.GEOGRAPHICAL_AREA, + "next_step": MeasureEditSteps.GEOGRAPHICAL_AREA_EXCLUSIONS, }, { "data": { - STEP_KEY: MeasureEditSteps.GEOGRAPHICAL_AREA, - "geographical_area-geo_area": "ERGA_OMNES", - "erga_omnes_exclusions_formset-0-erga_omnes_exclusion": new_excluded_area.pk, + STEP_KEY: MeasureEditSteps.GEOGRAPHICAL_AREA_EXCLUSIONS, + "form-0-excluded_area": new_excluded_area.pk, }, "next_step": "complete", }, @@ -2446,7 +2443,6 @@ def test_multiple_measure_edit_geographical_area_and_exclusions( with override_current_transaction(Transaction.objects.last()): for measure in workbasket_measures: assert measure.update_type == UpdateType.UPDATE - assert measure.geographical_area == new_geo_area assert ( measure.exclusions.current().first().excluded_geographical_area == new_excluded_area From 21b8e6d5d00b9e8cefad37c6daffa9b134876627 Mon Sep 17 00:00:00 2001 From: Dale Cannon Date: Wed, 20 Sep 2023 17:04:03 +0100 Subject: [PATCH 07/13] Set queryset in __init__ to fix failing CI/CD tests --- measures/forms.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/measures/forms.py b/measures/forms.py index 3b03f1a5c..0e561192f 100644 --- a/measures/forms.py +++ b/measures/forms.py @@ -1639,10 +1639,7 @@ def clean(self): class MeasureGeographicalAreaExclusionsForm(forms.Form): excluded_area = forms.ModelChoiceField( label="", - queryset=GeographicalArea.objects.current() - .with_latest_description() - .as_at_today_and_beyond() - .order_by("description"), + queryset=None, help_text="Select a geographical area to be excluded", required=False, ) @@ -1650,6 +1647,13 @@ class MeasureGeographicalAreaExclusionsForm(forms.Form): 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}" From 00120ce180e86d83c3fb6416a398dc381e53c958 Mon Sep 17 00:00:00 2001 From: Dale Cannon Date: Wed, 20 Sep 2023 17:20:36 +0100 Subject: [PATCH 08/13] Set default queryset to fix tests --- measures/forms.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/measures/forms.py b/measures/forms.py index 0e561192f..7b02ef6dd 100644 --- a/measures/forms.py +++ b/measures/forms.py @@ -1639,7 +1639,7 @@ def clean(self): class MeasureGeographicalAreaExclusionsForm(forms.Form): excluded_area = forms.ModelChoiceField( label="", - queryset=None, + queryset=GeographicalArea.objects.all(), help_text="Select a geographical area to be excluded", required=False, ) From aba8daed3252d4dc849e403f015e42df24f908d9 Mon Sep 17 00:00:00 2001 From: Dale Cannon Date: Wed, 20 Sep 2023 17:48:51 +0100 Subject: [PATCH 09/13] Remove trailing comma --- measures/forms.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/measures/forms.py b/measures/forms.py index 7b02ef6dd..c3dc0778f 100644 --- a/measures/forms.py +++ b/measures/forms.py @@ -1651,7 +1651,7 @@ def __init__(self, *args, **kwargs): GeographicalArea.objects.current() .with_latest_description() .as_at_today_and_beyond() - .order_by("description"), + .order_by("description") ) self.fields[ From f126bb26492b10eed07a3558d437cc9d91fcf768 Mon Sep 17 00:00:00 2001 From: Dale Cannon Date: Wed, 20 Sep 2023 18:15:05 +0100 Subject: [PATCH 10/13] Mock diff_components for multiple measures edit tests --- measures/tests/test_views.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/measures/tests/test_views.py b/measures/tests/test_views.py index 3d1375312..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.""" @@ -2378,6 +2394,7 @@ def test_multiple_measure_edit_preserves_footnote_associations( 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`.""" From 4d3e70dd391652a49fde1f6222ad525986263af1 Mon Sep 17 00:00:00 2001 From: Dale Cannon Date: Fri, 22 Sep 2023 09:09:14 +0100 Subject: [PATCH 11/13] Remove superfluous constant --- measures/constants.py | 1 - measures/views.py | 3 +-- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/measures/constants.py b/measures/constants.py index 231607282..afaf7318b 100644 --- a/measures/constants.py +++ b/measures/constants.py @@ -1,7 +1,6 @@ from django.db import models START = "start" -GEOGRAPHICAL_AREA_EXCLUSIONS = "geographical_area_exclusions" class MeasureEditSteps(models.TextChoices): diff --git a/measures/views.py b/measures/views.py index 7cdfe82d8..d0591e824 100644 --- a/measures/views.py +++ b/measures/views.py @@ -38,7 +38,6 @@ 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 GEOGRAPHICAL_AREA_EXCLUSIONS from measures.constants import START from measures.constants import MeasureEditSteps from measures.filters import MeasureFilter @@ -280,7 +279,7 @@ class MeasureEditWizard( templates = { START: "measures/edit-multiple-start.jinja", - GEOGRAPHICAL_AREA_EXCLUSIONS: "measures/edit-multiple-formset.jinja", + MeasureEditSteps.GEOGRAPHICAL_AREA_EXCLUSIONS: "measures/edit-multiple-formset.jinja", } step_metadata = { From 4d960d94d75ba9fbc87870c8291896900f2eb135 Mon Sep 17 00:00:00 2001 From: Dale Cannon Date: Fri, 22 Sep 2023 09:25:37 +0100 Subject: [PATCH 12/13] Add comment --- measures/views.py | 1 + 1 file changed, 1 insertion(+) diff --git a/measures/views.py b/measures/views.py index d0591e824..30421922a 100644 --- a/measures/views.py +++ b/measures/views.py @@ -383,6 +383,7 @@ def update_measure_excluded_geographical_areas( """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( From 937f32169b6c27f0f6e31422bb1fdabe59e5f24e Mon Sep 17 00:00:00 2001 From: Dale Cannon Date: Fri, 22 Sep 2023 09:32:42 +0100 Subject: [PATCH 13/13] Remove unneeded if block --- measures/views.py | 9 --------- 1 file changed, 9 deletions(-) diff --git a/measures/views.py b/measures/views.py index 30421922a..ff7423c8f 100644 --- a/measures/views.py +++ b/measures/views.py @@ -392,15 +392,6 @@ def update_measure_excluded_geographical_areas( ) return - if not exclusions: - for exclusion in existing_exclusions: - exclusion.new_version( - update_type=UpdateType.DELETE, - modified_measure=measure, - workbasket=workbasket, - ) - return - new_excluded_areas = get_all_members_of_geo_groups( validity=measure.valid_between, geo_areas=exclusions,