From b19f556ac9df098cf86b258a9e592b59e40928aa Mon Sep 17 00:00:00 2001 From: Dale Cannon <118175145+dalecannon@users.noreply.github.com> Date: Tue, 19 Sep 2023 13:41:35 +0100 Subject: [PATCH] TP2000-885 Use quota origins and exclusions to populate geo data in create measure journey (#1032) * Refactor MeasureGeographicalAreaForm * Set up cleaned_data to use quota origins and exclusions * Conditionally show geo areas form * Update form tests * Update view tests * Add help text * Sort cleaned_data in test * Remove duplicate fixture * Simplify dictionary get * Use active origins only * Fix if statement --- measures/forms.py | 74 ++++++++++++++----- .../measures/create-geo-areas-formset.jinja | 57 ++++++++++++++ measures/jinja2/measures/create-review.jinja | 21 +++--- .../jinja2/measures/create-wizard-step.jinja | 6 +- measures/tests/test_forms.py | 62 ++++++++++++++-- measures/tests/test_views.py | 39 +++++----- measures/views.py | 43 +++++++++-- 7 files changed, 237 insertions(+), 65 deletions(-) create mode 100644 measures/jinja2/measures/create-geo-areas-formset.jinja diff --git a/measures/forms.py b/measures/forms.py index 7faf482a0..3b321d851 100644 --- a/measures/forms.py +++ b/measures/forms.py @@ -977,6 +977,7 @@ class Meta: help_text=( "Search for a quota using its order number. " "You can then select the correct quota from the dropdown list. " + "Selecting a quota will automatically populate the appropriate geographical areas on the next page." ), queryset=QuotaOrderNumber.objects.all(), required=False, @@ -1034,6 +1035,10 @@ class MeasureGeographicalAreaForm( error_messages={"required": "A Geographical area must be selected"}, ) + @property + def erga_omnes_instance(self): + return GeographicalArea.objects.current().erga_omnes().get() + @property def geo_area_field_name(self): return f"{self.prefix}-geo_area" @@ -1062,9 +1067,7 @@ def get_countries_initial(self): return initial - def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) - + def get_initial_data(self): geographical_area_fields = { constants.GeoAreaType.ERGA_OMNES: self.erga_omnes_instance, constants.GeoAreaType.GROUP: self.data.get( @@ -1088,9 +1091,20 @@ def __init__(self, *args, **kwargs): countries_initial_data = self.get_countries_initial() nested_forms_initial.update(geo_area_initial_data) nested_forms_initial.update(countries_initial_data) - kwargs.pop("initial") - self.bind_nested_forms(*args, initial=nested_forms_initial, **kwargs) + return nested_forms_initial + + def init_fields(self): + if ( + self.order_number + and self.order_number.quotaordernumberorigin_set.current() + .as_at_today_and_beyond() + .exists() + ): + self.fields["geo_area"].required = False + self.fields["geo_area"].disabled = True + + def init_layout(self): self.helper = FormHelper(self) self.helper.label_size = Size.SMALL self.helper.legend_size = Size.SMALL @@ -1104,13 +1118,31 @@ def __init__(self, *args, **kwargs): ), ) - @property - def erga_omnes_instance(self): - return GeographicalArea.objects.current().erga_omnes().get() + def __init__(self, *args, **kwargs): + self.order_number = kwargs.pop("order_number", None) + super().__init__(*args, **kwargs) + self.init_fields() + nested_forms_initial = self.get_initial_data() if not self.order_number else {} + kwargs.pop("initial", None) + self.bind_nested_forms(*args, initial=nested_forms_initial, **kwargs) + self.init_layout() def clean(self): cleaned_data = super().clean() + # Use quota order number origins and exclusions to set cleaned_data + if self.order_number: + origins = self.order_number.quotaordernumberorigin_set.current() + cleaned_data["geo_areas_and_exclusions"] = [ + { + "geo_area": origin.geographical_area, + "exclusions": list(origin.excluded_areas.current()), + } + for origin in origins + ] + return cleaned_data + + # Otherwise take geographical data from form geo_area_choice = self.cleaned_data.get("geo_area") geographical_area_fields = { @@ -1121,29 +1153,35 @@ def clean(self): if geo_area_choice: if not self.formset_submitted: if geo_area_choice == constants.GeoAreaType.ERGA_OMNES: - cleaned_data["geo_area_list"] = [self.erga_omnes_instance] + cleaned_data["geo_areas_and_exclusions"] = [ + {"geo_area": self.erga_omnes_instance}, + ] elif geo_area_choice == constants.GeoAreaType.GROUP: data_key = constants.SUBFORM_PREFIX_MAPPING[geo_area_choice] - cleaned_data["geo_area_list"] = [cleaned_data[data_key]] + cleaned_data["geo_areas_and_exclusions"] = [ + {"geo_area": cleaned_data[data_key]}, + ] elif geo_area_choice == constants.GeoAreaType.COUNTRY: field_name = geographical_area_fields[geo_area_choice] data_key = constants.SUBFORM_PREFIX_MAPPING[geo_area_choice] - cleaned_data["geo_area_list"] = [ - geo_area[field_name] for geo_area in cleaned_data[data_key] + cleaned_data["geo_areas_and_exclusions"] = [ + {"geo_area": geo_area[field_name]} + for geo_area in cleaned_data[data_key] ] - exclusions = cleaned_data.get( + geo_area_exclusions = cleaned_data.get( constants.EXCLUSIONS_FORMSET_PREFIX_MAPPING[geo_area_choice], ) - if exclusions: - cleaned_data["geo_area_exclusions"] = [ + if geo_area_exclusions: + exclusions = [ exclusion[constants.FIELD_NAME_MAPPING[geo_area_choice]] - for exclusion in cleaned_data[ - constants.EXCLUSIONS_FORMSET_PREFIX_MAPPING[geo_area_choice] - ] + for exclusion in geo_area_exclusions ] + cleaned_data["geo_areas_and_exclusions"][0][ + "exclusions" + ] = exclusions return cleaned_data diff --git a/measures/jinja2/measures/create-geo-areas-formset.jinja b/measures/jinja2/measures/create-geo-areas-formset.jinja new file mode 100644 index 000000000..5bbd1786c --- /dev/null +++ b/measures/jinja2/measures/create-geo-areas-formset.jinja @@ -0,0 +1,57 @@ +{% extends "measures/create-wizard-step.jinja" %} +{% from "components/details/macro.njk" import govukDetails %} +{% from "components/summary-list/macro.njk" import govukSummaryList %} + +{% block breadcrumb %} + {% if request.path != "/" %} + {{ breadcrumbs(request, [{"text": "Review the geographical areas"}]) }} + {% endif %} + {% endblock %} + +{% block page_title_heading %}Review the geographical areas{% endblock %} + +{% block form %} + {% if order_number and origins_and_exclusions %} + + {{ govukDetails({ + "summaryText": "What can I do if the geographical areas are incorrect?", + "text": "The geographical areas are determined by the quota you entered on the previous page. If the geographical areas are incorrect, you will need to edit the quota. You will then need to re-enter the quota to reflect the changes." + }) }} + + {% set row_data = [] %} + {% for data in origins_and_exclusions %} + {% set origin %} + {{ data["origin"].area_id }} - {{ data["origin"].get_description().description }} + {% endset %} + + {{ row_data.append({ + "key": {"text": "Geographical area"}, + "value": {"text": origin}, + "actions": {"items": []} + }) or "" }} + + {% if data.get("exclusions") %} + {% set exclusions %} + {% for exclusion in data["exclusions"] %} + {{ exclusion.area_id }} - {{ exclusion.get_description().description }}{% if not loop.last %}, {% endif %} + {% endfor %} + {% endset %} + + {{ row_data.append({ + "key": {"text": "Geographical area exclusions"}, + "value": {"text": exclusions if data["exclusions"] else "-"}, + "actions": {"items": []} + }) or "" }} + {% endif %} + {% endfor %} + + {{ govukSummaryList({ + "rows": row_data, + })}} + + {{ govukButton({"text": "Continue"}) }} + + {% else %} + {{ crispy(form) }} + {% endif %} +{% endblock %} diff --git a/measures/jinja2/measures/create-review.jinja b/measures/jinja2/measures/create-review.jinja index ca9335d18..a8f3cd18e 100644 --- a/measures/jinja2/measures/create-review.jinja +++ b/measures/jinja2/measures/create-review.jinja @@ -74,19 +74,18 @@ {% endcall %} {% call(rows, data) review_step("geographical_area") %} - {% for item in data.geo_area_list %} - {{ rows.extend([ - ("Geographical area", item.get_description().description|safe), - ]) }} - {% endfor %} - {% if data.geo_area_exclusions %} - {% for item in data.geo_area_exclusions %} + {% for geo_data in data.geo_areas_and_exclusions %} {{ rows.extend([ - ("Excluded geographical area", item.get_description().description|safe), + ("Geographical area", geo_data["geo_area"].get_description().description|safe), ]) }} - {% endfor %} - {% endif %} - + {% if geo_data.get("exclusions") %} + {% for exclusion in geo_data["exclusions"] %} + {{ rows.extend([ + ("Excluded geographical area", exclusion.get_description().description|safe), + ]) }} + {% endfor %} + {% endif %} + {% endfor %} {% endcall %} {% call(rows, data) review_step("commodities") %} diff --git a/measures/jinja2/measures/create-wizard-step.jinja b/measures/jinja2/measures/create-wizard-step.jinja index d991eb293..37d86038e 100644 --- a/measures/jinja2/measures/create-wizard-step.jinja +++ b/measures/jinja2/measures/create-wizard-step.jinja @@ -9,7 +9,11 @@
{{ page_subtitle|default("") }} -

{{ page_title }}

+

+ {% block page_title_heading %} + {{ page_title }} + {% endblock %} +

{% if step_metadata[wizard.steps.current].info %}

{{ step_metadata[wizard.steps.current].info }}

diff --git a/measures/tests/test_forms.py b/measures/tests/test_forms.py index 27ec2bf76..09c9f2818 100644 --- a/measures/tests/test_forms.py +++ b/measures/tests/test_forms.py @@ -173,7 +173,9 @@ def test_measure_forms_geo_area_valid_data_erga_omnes(erga_omnes): prefix=GEO_AREA_FORM_PREFIX, ) assert form.is_valid() - assert form.cleaned_data["geo_area_list"] == [erga_omnes] + assert ( + form.cleaned_data["geo_areas_and_exclusions"][0]["geo_area"] == erga_omnes + ) def test_measure_forms_geo_area_valid_data_erga_omnes_exclusions(erga_omnes): @@ -202,7 +204,10 @@ def test_measure_forms_geo_area_valid_data_erga_omnes_exclusions(erga_omnes): prefix=GEO_AREA_FORM_PREFIX, ) assert form.is_valid() - assert form.cleaned_data["geo_area_exclusions"] == [geo_area1, geo_area2] + assert form.cleaned_data["geo_areas_and_exclusions"][0]["exclusions"] == [ + geo_area1, + geo_area2, + ] def test_measure_forms_geo_area_valid_data_erga_omnes_exclusions_delete(erga_omnes): @@ -250,7 +255,9 @@ def test_measure_forms_geo_area_valid_data_geo_group_exclusions(erga_omnes): prefix=GEO_AREA_FORM_PREFIX, ) assert form.is_valid() - assert form.cleaned_data["geo_area_exclusions"] == [geo_area1] + assert form.cleaned_data["geo_areas_and_exclusions"][0]["exclusions"] == [ + geo_area1, + ] def test_measure_forms_geo_area_valid_data_geo_group_exclusions_delete(erga_omnes): @@ -312,8 +319,7 @@ def test_measure_forms_geo_area_valid_data_geo_group(erga_omnes): prefix=GEO_AREA_FORM_PREFIX, ) assert form.is_valid() - # https://uktrade.atlassian.net/browse/TP2000-437 500 error where object instead of a list of objects - assert form.cleaned_data["geo_area_list"] == [geo_group] + assert form.cleaned_data["geo_areas_and_exclusions"][0]["geo_area"] == geo_group def test_measure_forms_geo_area_valid_data_countries_submit(erga_omnes): @@ -336,9 +342,13 @@ def test_measure_forms_geo_area_valid_data_countries_submit(erga_omnes): prefix=GEO_AREA_FORM_PREFIX, ) assert form.is_valid() - assert form.cleaned_data["geo_area_list"] == [geo_area1, geo_area2] + cleaned_data = [ + data["geo_area"] for data in form.cleaned_data["geo_areas_and_exclusions"] + ] + assert cleaned_data == [geo_area1, geo_area2] # specific country selection should have empty exclusions - assert not form.cleaned_data.get("geo_area_exclusions") + for data in form.cleaned_data["geo_areas_and_exclusions"]: + assert not data.get("exclusions") def test_measure_forms_geo_area_valid_data_countries_delete(erga_omnes): @@ -461,6 +471,44 @@ def test_measure_forms_geo_area_invalid_data_error_messages(data, error, erga_om assert error in form.errors["geo_area"] +def test_measure_geographical_area_form_quota_order_number(): + """Tests that `MeasureGeographicalAreaForm` sets `cleaned_data` with + geographical data from quota order number if one is passed in `kwargs`.""" + origin_1 = factories.QuotaOrderNumberOriginFactory.create() + origin_2 = factories.QuotaOrderNumberOriginFactory.create( + order_number=origin_1.order_number, + ) + origin_1_exclusions = factories.QuotaOrderNumberOriginExclusionFactory.create_batch( + 2, + origin=origin_1, + ) + order_number = origin_1.order_number + + with override_current_transaction(Transaction.objects.last()): + form = forms.MeasureGeographicalAreaForm( + data={}, + initial={}, + order_number=order_number, + prefix=GEO_AREA_FORM_PREFIX, + ) + assert form.is_valid() + + expected_cleaned_data = [ + { + "geo_area": origin_1.geographical_area, + "exclusions": list(origin_1.excluded_areas.all()), + }, + { + "geo_area": origin_2.geographical_area, + "exclusions": [], + }, + ] + assert sorted( + form.cleaned_data["geo_areas_and_exclusions"], + key=lambda d: sorted(d.items()), + ) == sorted(expected_cleaned_data, key=lambda d: sorted(d.items())) + + def test_measure_forms_details_invalid_data(): data = { "measure_type": "foo", diff --git a/measures/tests/test_views.py b/measures/tests/test_views.py index 344e90735..e09a28733 100644 --- a/measures/tests/test_views.py +++ b/measures/tests/test_views.py @@ -1252,7 +1252,10 @@ def test_measure_form_wizard_create_measures( form_data = { "measure_type": measure_type, "generating_regulation": regulation, - "geo_area_list": [geo_area1, geo_area2], + "geo_areas_and_exclusions": [ + {"geo_area": geo_area1}, + {"geo_area": geo_area2}, + ], "order_number": None, "valid_between": date_ranges.normal, "formset-commodities": [ @@ -1307,10 +1310,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. """ @@ -1509,7 +1511,9 @@ def test_measure_form_wizard_create_measures_with_tariff_suspension_action( form_data = { "measure_type": measure_type, "generating_regulation": regulation, - "geo_area_list": [geo_area1], + "geo_areas_and_exclusions": [ + {"geo_area": geo_area1}, + ], "order_number": None, "valid_between": date_ranges.normal, "formset-commodities": [ @@ -1535,10 +1539,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 @@ -1587,27 +1588,28 @@ def test_measure_form_wizard_create_measures_with_tariff_suspension_action( ) -@pytest.mark.parametrize("step", ["commodities", "conditions"]) +@pytest.mark.parametrize("step", ["geographical_area", "commodities", "conditions"]) def test_measure_create_wizard_get_form_kwargs( step, session_request, measure_type, - regulation, - erga_omnes, + quota_order_number, ): details_data = { "measure_create_wizard-current_step": "measure_details", "measure_details-measure_type": [measure_type.pk], - "measure_details-generating_regulation": [regulation.pk], - "measure_details-geo_area_type": ["ERGA_OMNES"], "measure_details-start_date_0": [2], "measure_details-start_date_1": [4], "measure_details-start_date_2": [2021], "measure_details-min_commodity_count": [2], } + quota_data = {"quota_order_number-order_number": [quota_order_number]} + storage = MeasureCreateSessionStorage(request=session_request, prefix="") storage.set_step_data("measure_details", details_data) + storage.set_step_data("quota_order_number", quota_data) storage._set_current_step(step) + wizard = MeasureCreateWizard( request=session_request, storage=storage, @@ -1617,17 +1619,14 @@ def test_measure_create_wizard_get_form_kwargs( wizard.form_list = OrderedDict(wizard.form_list) form_kwargs = wizard.get_form_kwargs(step) - if step == "commodities": - assert "measure_start_date" in form_kwargs - assert "min_commodity_count" in form_kwargs - assert "measure_type" in form_kwargs["form_kwargs"] + if step == "geographical_area": + assert form_kwargs["order_number"] == quota_order_number + elif step == "commodities": assert form_kwargs["measure_start_date"] == date(2021, 4, 2) assert form_kwargs["min_commodity_count"] == 2 assert form_kwargs["form_kwargs"]["measure_type"] == measure_type else: # conditions - assert "measure_start_date" in form_kwargs["form_kwargs"] - assert "measure_type" in form_kwargs["form_kwargs"] assert form_kwargs["form_kwargs"]["measure_start_date"] == date(2021, 4, 2) assert form_kwargs["form_kwargs"]["measure_type"] == measure_type diff --git a/measures/views.py b/measures/views.py index 71531e6ec..ff13f6daa 100644 --- a/measures/views.py +++ b/measures/views.py @@ -433,7 +433,7 @@ class MeasureCreateWizard( MEASURE_DETAILS: "measures/create-wizard-step.jinja", REGULATION_ID: "measures/create-wizard-step.jinja", QUOTA_ORDER_NUMBER: "measures/create-wizard-step.jinja", - GEOGRAPHICAL_AREA: "measures/create-wizard-step.jinja", + GEOGRAPHICAL_AREA: "measures/create-geo-areas-formset.jinja", COMMODITIES: "measures/create-comm-codes-formset.jinja", ADDITIONAL_CODE: "measures/create-wizard-step.jinja", CONDITIONS: "measures/create-formset.jinja", @@ -597,11 +597,11 @@ def create_measures(self, data): for commodity_data in data.get("formset-commodities", []): if not commodity_data.get("DELETE"): - for geo_area in data["geo_area_list"]: + for geo_data in data["geo_areas_and_exclusions"]: measure_data = { "measure_type": data["measure_type"], - "geographical_area": geo_area, - "exclusions": data.get("geo_area_exclusions", None) or [], + "geographical_area": geo_data["geo_area"], + "exclusions": geo_data.get("exclusions", []), "goods_nomenclature": commodity_data["commodity"], "additional_code": data["additional_code"], "order_number": data["order_number"], @@ -682,7 +682,7 @@ def get_cleaned_data_for_step(self, step): Note: This patched version of `super().get_cleaned_data_for_step` temporarily saves the cleaned_data to provide quick retrieval should another call for it be made in the same request (as happens in - `get_form_kwargs()` and qtemplate for summary page) to avoid revalidating forms unnecessarily. + `get_form_kwargs()` and template for summary page) to avoid revalidating forms unnecessarily. """ self.cleaned_data = getattr(self, "cleaned_data", {}) if step in self.cleaned_data: @@ -691,6 +691,12 @@ def get_cleaned_data_for_step(self, step): self.cleaned_data[step] = super().get_cleaned_data_for_step(step) return self.cleaned_data[step] + @property + def quota_order_number(self): + cleaned_data = self.get_cleaned_data_for_step(self.QUOTA_ORDER_NUMBER) + order_number = cleaned_data.get("order_number") if cleaned_data else None + return order_number + def get_context_data(self, form, **kwargs): context = super().get_context_data(form=form, **kwargs) context["step_metadata"] = self.step_metadata @@ -698,11 +704,32 @@ def get_context_data(self, form, **kwargs): context["form"].is_bound = False context["no_form_tags"] = FormHelper() context["no_form_tags"].form_tag = False + + if self.steps.current == self.GEOGRAPHICAL_AREA: + origins_and_exclusions = None + if self.quota_order_number: + origins_and_exclusions = [ + { + "origin": origin.geographical_area, + "exclusions": list(origin.excluded_areas.current()), + } + for origin in self.quota_order_number.quotaordernumberorigin_set.current().as_at_today_and_beyond() + ] + context.update( + { + "order_number": self.quota_order_number, + "origins_and_exclusions": origins_and_exclusions, + }, + ) + return context def get_form_kwargs(self, step): kwargs = {} - if step == self.COMMODITIES: + if step == self.GEOGRAPHICAL_AREA: + kwargs["order_number"] = self.quota_order_number + + elif step == self.COMMODITIES: measure_start_date = None measure_type = None min_commodity_count = 0 @@ -723,7 +750,7 @@ def get_form_kwargs(self, step): "measure_type": measure_type, } - if step == self.CONDITIONS: + elif step == self.CONDITIONS: measure_start_date = None measure_type = None measure_details = self.get_cleaned_data_for_step(self.MEASURE_DETAILS) @@ -735,7 +762,7 @@ def get_form_kwargs(self, step): "measure_type": measure_type, } - if step == self.SUMMARY: + elif step == self.SUMMARY: measure_details = self.get_cleaned_data_for_step(self.MEASURE_DETAILS) measure_type = ( measure_details.get("measure_type") if measure_details else None