From 30f4da0ea226be1e3e56455d0b47a792f731d221 Mon Sep 17 00:00:00 2001
From: Dale Cannon <118175145+dalecannon@users.noreply.github.com>
Date: Tue, 3 Oct 2023 16:55:40 +0100
Subject: [PATCH] TP2000-1067  Fix measure edit footnote save (#1050)

* Tidy up template

* Move constants, rename variable, re-order code

* Catch footnote submitted directly on saving

* Add test

* Amend comment

* Remove check for POST in request
---
 measures/constants.py                         |  4 +
 measures/forms.py                             | 26 ++++--
 .../measures/create-footnotes-formset.jinja   | 87 +++++++++----------
 measures/tests/conftest.py                    |  2 +-
 measures/tests/test_forms.py                  |  6 +-
 measures/tests/test_views.py                  | 25 ++++++
 measures/views.py                             | 16 ++--
 7 files changed, 100 insertions(+), 66 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 fd4608f28..969d7f010 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):
@@ -606,10 +605,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()
@@ -685,10 +682,21 @@ def save(self, commit=True):
                 "component_measure",
             )
 
+        # Footnotes added via "Add another 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
+        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)
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 -%}
-                <a class="govuk-link govuk-!-font-weight-bold" href="{{ footnote.get_url() }}">{{ footnote|string }}</a>
-                {%- endset %}
-                {% set remove_button -%}
-                    <button formaction={{ form.instance.get_url('edit-footnotes') }} class="govuk-link govuk-!-font-weight-bold fake-link" name="remove" value={{ footnote.pk }}>Remove</a>    
-                {%- endset %}
-                {{ table_rows.append([
-                {"html": footnote_link},
-                {"text": footnote.get_description().description|default("")},
-                {"html": remove_button}
-                ]) or "" }}
-            {% endif %}
-        {% endfor %}
-        {{ govukTable({
-            "head": [
-            {"text": "ID"},
-            {"text": "Description"},
-            {"text": ""}
-            ],
-            "rows": table_rows,
-            "caption": "Footnotes currently assigned to the measure",
-            "captionClasses": "govuk-table__caption--m"
-        }) }}
-    {% else %}
-        <p class="govuk-label">No footnotes found.</p>
-        <br></br>
-    {% endif %}
+{% with measure_footnotes = form.request.session["instance_footnotes_" ~ form.instance.sid] %}
+  {% if measure_footnotes %}
+    {% set table_rows = [] %}
+    {% for footnote in footnotes %}
+      {% if footnote.pk in measure_footnotes %}
+        {% set footnote_link -%}
+          <a class="govuk-link govuk-!-font-weight-bold" href="{{ footnote.get_url() }}">{{ footnote|string }}</a>
+        {%- endset %}
+        {% set remove_button -%}
+          <button formaction={{ form.instance.get_url('edit-footnotes') }} class="govuk-link govuk-!-font-weight-bold fake-link" name="remove" value={{ footnote.pk }}>Remove</a>
+        {%- endset %}
+        {{ table_rows.append([
+          {"html": footnote_link},
+          {"text": footnote.get_description().description|default("")},
+          {"html": remove_button}
+        ]) or "" }}
+      {% endif %}
+    {% endfor %}
+    {{ govukTable({
+      "head": [
+        {"text": "ID"},
+        {"text": "Description"},
+        {"text": ""}
+      ],
+        "rows": table_rows,
+        "caption": "Footnotes currently assigned to the measure",
+        "captionClasses": "govuk-table__caption--m"
+      }) }}
+  {% endif %}
 {% endwith %}
 
 {{ crispy(footnotes_formset.management_form, no_form_tags) }}
-    <label class="govuk-label">Add a footnote</label>
-    <div id="sort-code-hint" class="govuk-hint">Start typing the ID of the footnote or terms used in the description</div>
-    {% for form in footnotes_formset %}
-        {{ crispy(form) }}
-    {% endfor %}
 
-    {% if footnotes_formset.data[footnotes_formset.prefix ~ "-ADD"] %}
-        {{ crispy(footnotes_formset.empty_form)|replace("__prefix__", footnotes_formset.forms|length)|safe }}
-    {% endif %}
+{% for form in footnotes_formset %}
+  {{ crispy(form) }}
+{% endfor %}
+
+{% if footnotes_formset.data[footnotes_formset.prefix ~ "-ADD"] %}
+  {{ crispy(footnotes_formset.empty_form)|replace("__prefix__", footnotes_formset.forms|length)|safe }}
+{% endif %}
 
-    {% call govukFieldset({"legend": {}, "classes": "govuk-!-padding-top-3"}) %}
-        <button class="govuk-button govuk-button--secondary" data-module="govuk-button" value=1, name={{ footnotes_formset.prefix ~ "-ADD" }}, formaction={{ object.get_url("edit-footnotes") }}>
-            Add another footnote
-        </button
-       
-    {% endcall %}
\ No newline at end of file
+{% call govukFieldset({"legend": {}, "classes": "govuk-!-padding-top-3"}) %}
+  <button class="govuk-button govuk-button--secondary" data-module="govuk-button" value=1, name={{ footnotes_formset.prefix ~ "-ADD" }}, formaction={{ object.get_url("edit-footnotes") }}>
+    Add another footnote
+  </button
+{% endcall %}
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 5ac2b5a5e..e9d41d00d 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
@@ -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)
diff --git a/measures/tests/test_views.py b/measures/tests/test_views.py
index d92ecd0b3..469509c6e 100644
--- a/measures/tests/test_views.py
+++ b/measures/tests/test_views.py
@@ -592,6 +592,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
diff --git a/measures/views.py b/measures/views.py
index 577ad05a5..13df16d98 100644
--- a/measures/views.py
+++ b/measures/views.py
@@ -39,11 +39,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
@@ -1014,20 +1014,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,
@@ -1036,12 +1036,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