From 2abaee645afc1b51d6f8cda60fb19c52d3d80e1a Mon Sep 17 00:00:00 2001 From: Dale Cannon Date: Mon, 25 Sep 2023 14:07:46 +0100 Subject: [PATCH 1/6] Tidy up template --- .../measures/create-footnotes-formset.jinja | 87 +++++++++---------- 1 file changed, 41 insertions(+), 46 deletions(-) diff --git a/measures/jinja2/measures/create-footnotes-formset.jinja b/measures/jinja2/measures/create-footnotes-formset.jinja index 1229df6b6..b58867697 100644 --- a/measures/jinja2/measures/create-footnotes-formset.jinja +++ b/measures/jinja2/measures/create-footnotes-formset.jinja @@ -1,51 +1,46 @@ -{% with form_footnotes = form.request.session["instance_footnotes_" ~ form.instance.sid] %} - {% if form_footnotes %} - {% set table_rows = [] %} - {% for footnote in footnotes %} - {% if footnote.pk in form.request.session["instance_footnotes_"~form.instance.sid]%} - {% set footnote_link -%} - {{ footnote|string }} - {%- endset %} - {% set remove_button -%} - + Add another footnote + Date: Mon, 25 Sep 2023 14:17:31 +0100 Subject: [PATCH 2/6] Move constants, rename variable, re-order code --- measures/constants.py | 4 ++++ measures/forms.py | 5 ++--- measures/tests/conftest.py | 2 +- measures/tests/test_forms.py | 4 ++-- measures/views.py | 16 ++++++++-------- 5 files changed, 17 insertions(+), 14 deletions(-) diff --git a/measures/constants.py b/measures/constants.py index afaf7318b..5f1e6d495 100644 --- a/measures/constants.py +++ b/measures/constants.py @@ -13,3 +13,7 @@ class MeasureEditSteps(models.TextChoices): "geographical_area_exclusions", "Geographical area exclusions", ) + + +MEASURE_CONDITIONS_FORMSET_PREFIX = "measure-conditions-formset" +MEASURE_COMMODITIES_FORMSET_PREFIX = "measure_commodities_duties_formset" diff --git a/measures/forms.py b/measures/forms.py index 0a17e6b10..303701c17 100644 --- a/measures/forms.py +++ b/measures/forms.py @@ -45,6 +45,8 @@ from geo_areas.models import GeographicalArea from geo_areas.utils import get_all_members_of_geo_groups from measures import models +from measures.constants import MEASURE_COMMODITIES_FORMSET_PREFIX +from measures.constants import MEASURE_CONDITIONS_FORMSET_PREFIX from measures.constants import MeasureEditSteps from measures.models import MeasureExcludedGeographicalArea from measures.parsers import DutySentenceParser @@ -58,9 +60,6 @@ logger = logging.getLogger(__name__) -MEASURE_CONDITIONS_FORMSET_PREFIX = "measure-conditions-formset" -MEASURE_COMMODITIES_FORMSET_PREFIX = "measure_commodities_duties_formset" - class MeasureGeoAreaInitialDataMixin(FormSetSubmitMixin): def get_geo_area_initial(self): diff --git a/measures/tests/conftest.py b/measures/tests/conftest.py index b94e9bb01..0db3ba8fa 100644 --- a/measures/tests/conftest.py +++ b/measures/tests/conftest.py @@ -13,7 +13,7 @@ from common.validators import ApplicabilityCode from common.validators import UpdateType from geo_areas.validators import AreaCode -from measures.forms import MEASURE_CONDITIONS_FORMSET_PREFIX +from measures.constants import MEASURE_CONDITIONS_FORMSET_PREFIX from measures.forms import MeasureForm from measures.models import Measure from measures.models import MeasureAction diff --git a/measures/tests/test_forms.py b/measures/tests/test_forms.py index a518f1e71..eff5faef8 100644 --- a/measures/tests/test_forms.py +++ b/measures/tests/test_forms.py @@ -13,8 +13,8 @@ from geo_areas import constants from geo_areas.validators import AreaCode from measures import forms -from measures.forms import MEASURE_COMMODITIES_FORMSET_PREFIX -from measures.forms import MEASURE_CONDITIONS_FORMSET_PREFIX +from measures.constants import MEASURE_COMMODITIES_FORMSET_PREFIX +from measures.constants import MEASURE_CONDITIONS_FORMSET_PREFIX from measures.forms import MeasureConditionsFormSet from measures.forms import MeasureEndDateForm from measures.forms import MeasureForm diff --git a/measures/views.py b/measures/views.py index ff7423c8f..3b5fd0393 100644 --- a/measures/views.py +++ b/measures/views.py @@ -38,11 +38,11 @@ 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 MEASURE_CONDITIONS_FORMSET_PREFIX from measures.constants import START from measures.constants import MeasureEditSteps from measures.filters import MeasureFilter from measures.filters import MeasureTypeFilterBackend -from measures.forms import MEASURE_CONDITIONS_FORMSET_PREFIX from measures.models import FootnoteAssociationMeasure from measures.models import Measure from measures.models import MeasureActionPair @@ -972,20 +972,20 @@ def get_conditions(self, measure): def get_context_data(self, **kwargs): context = super().get_context_data(**kwargs) - initial = self.request.session.get( + context["no_form_tags"] = FormHelper() + context["no_form_tags"].form_tag = False + + formset_footnotes = self.request.session.get( f"formset_initial_{self.kwargs.get('sid')}", [], ) footnotes_formset = forms.MeasureUpdateFootnotesFormSet() - footnotes_formset.initial = initial + footnotes_formset.initial = formset_footnotes footnotes_formset.form_kwargs = {"path": self.request.path} context["footnotes_formset"] = footnotes_formset - context["no_form_tags"] = FormHelper() - context["no_form_tags"].form_tag = False context["footnotes"] = self.get_footnotes(context["measure"]) conditions_initial = [] - if self.request.POST: conditions_initial = unprefix_formset_data( MEASURE_CONDITIONS_FORMSET_PREFIX, @@ -994,12 +994,12 @@ def get_context_data(self, **kwargs): conditions_formset = forms.MeasureConditionsFormSet( self.request.POST, initial=conditions_initial, - prefix="measure-conditions-formset", + prefix=MEASURE_CONDITIONS_FORMSET_PREFIX, ) else: conditions_formset = forms.MeasureConditionsFormSet( initial=conditions_initial, - prefix="measure-conditions-formset", + prefix=MEASURE_CONDITIONS_FORMSET_PREFIX, ) conditions = self.get_conditions(context["measure"]) form_fields = conditions_formset.form.Meta.fields From 8fb1b6885df178f547761645bc12d91875073b93 Mon Sep 17 00:00:00 2001 From: Dale Cannon Date: Mon, 25 Sep 2023 14:29:25 +0100 Subject: [PATCH 3/6] Catch footnote submitted directly on saving --- measures/forms.py | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/measures/forms.py b/measures/forms.py index 303701c17..bbfb750c8 100644 --- a/measures/forms.py +++ b/measures/forms.py @@ -612,10 +612,8 @@ def clean(self): return cleaned_data def save(self, commit=True): - """Get the measure instance after form submission, get from session - storage any footnote pks created via the Footnote formset and any pks - not removed from the measure after editing and create footnotes via - FootnoteAssociationMeasure.""" + """Updates a measure instance's geographical area and exclusions, + duties, and footnote associations following form submission.""" instance = super().save(commit=False) if commit: instance.save() @@ -691,10 +689,22 @@ def save(self, commit=True): "component_measure", ) + # Footnotes added via "Add new footnote" button footnote_pks = [ - dct["footnote"] - for dct in self.request.session.get(f"formset_initial_{sid}", []) + form["footnote"] + for form in self.request.session.get(f"formset_initial_{sid}", []) ] + + # Footnote submitted directly via "Save" button + if hasattr(self.request, "POST"): + form_footnote = self.request.POST.get( + f"form-{len(footnote_pks)}-footnote", + None, + ) + if form_footnote: + footnote_pks.append(form_footnote) + + # Footnotes already on measure footnote_pks.extend(self.request.session.get(f"instance_footnotes_{sid}", [])) self.request.session.pop(f"formset_initial_{sid}", None) From a1f7bfb46ebfa1d6a0313c8cd5916b635e3fe911 Mon Sep 17 00:00:00 2001 From: Dale Cannon Date: Mon, 25 Sep 2023 14:30:27 +0100 Subject: [PATCH 4/6] Add test --- measures/tests/test_views.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/measures/tests/test_views.py b/measures/tests/test_views.py index 99469d3ed..b8a005420 100644 --- a/measures/tests/test_views.py +++ b/measures/tests/test_views.py @@ -591,6 +591,31 @@ def test_measure_update_get_footnotes(session_with_workbasket): assert len(footnotes) == 0 +def test_measure_update_form_creates_footnote_association( + measure_form, + valid_user_client, +): + """Test that editing a measure to add a new footnote doesn't require + pressing "Add another footnote" button before submitting (saving) the + form.""" + footnote = factories.FootnoteFactory.create() + measure = measure_form.instance + assert not measure.footnotes.exists() + + form_data = {k: v for k, v in measure_form.data.items() if v is not None} + # Add footnote to form data and not to "formset_initial" in session data (i.e not pressing "Add another footnote") + form_data["form-0-footnote"] = footnote.pk + + url = reverse("measure-ui-edit", kwargs={"sid": measure.sid}) + response = valid_user_client.post(url, form_data) + assert response.status_code == 302 + + assert FootnoteAssociationMeasure.objects.filter( + footnoted_measure__sid=measure.sid, + associated_footnote=footnote, + ).exists() + + # https://uktrade.atlassian.net/browse/TP2000-340 def test_measure_update_updates_footnote_association(measure_form, client, valid_user): """Tests that when updating a measure with an existing footnote the From 1aef61eb63ca80608aab0a63fbe238e5d49d563f Mon Sep 17 00:00:00 2001 From: Dale Cannon Date: Mon, 25 Sep 2023 15:38:57 +0100 Subject: [PATCH 5/6] Amend comment --- measures/forms.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/measures/forms.py b/measures/forms.py index bbfb750c8..cd57f9f93 100644 --- a/measures/forms.py +++ b/measures/forms.py @@ -689,7 +689,7 @@ def save(self, commit=True): "component_measure", ) - # Footnotes added via "Add new footnote" button + # Footnotes added via "Add another footnote" button footnote_pks = [ form["footnote"] for form in self.request.session.get(f"formset_initial_{sid}", []) From c312b6ca5e4469f7c973cad44f523bfc9f6848f6 Mon Sep 17 00:00:00 2001 From: Dale Cannon Date: Tue, 3 Oct 2023 13:20:08 +0100 Subject: [PATCH 6/6] Remove check for POST in request --- measures/forms.py | 13 ++++++------- measures/tests/test_forms.py | 2 ++ 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/measures/forms.py b/measures/forms.py index cd57f9f93..e359831ec 100644 --- a/measures/forms.py +++ b/measures/forms.py @@ -696,13 +696,12 @@ def save(self, commit=True): ] # Footnote submitted directly via "Save" button - if hasattr(self.request, "POST"): - form_footnote = self.request.POST.get( - f"form-{len(footnote_pks)}-footnote", - None, - ) - if form_footnote: - footnote_pks.append(form_footnote) + form_footnote = self.request.POST.get( + f"form-{len(footnote_pks)}-footnote", + None, + ) + if form_footnote: + footnote_pks.append(form_footnote) # Footnotes already on measure footnote_pks.extend(self.request.session.get(f"instance_footnotes_{sid}", [])) diff --git a/measures/tests/test_forms.py b/measures/tests/test_forms.py index eff5faef8..04980cfad 100644 --- a/measures/tests/test_forms.py +++ b/measures/tests/test_forms.py @@ -35,6 +35,7 @@ def test_diff_components_not_called( duty_sentence_parser, ): with override_current_transaction(Transaction.objects.last()): + measure_form.request.POST = {} measure_form.save(commit=False) assert diff_components.called == False @@ -42,6 +43,7 @@ def test_diff_components_not_called( @patch("measures.forms.diff_components") def test_diff_components_called(diff_components, measure_form, duty_sentence_parser): + measure_form.request.POST = {} measure_form.data.update(duty_sentence="6.000%") with override_current_transaction(Transaction.objects.last()): measure_form.save(commit=False)