some renaming, addressed a major issue with refusal and made comments…

… more explicit

caseworker/advice/conditionals.py

@@ -2,12 +2,14 @@
 
 
 def form_add_licence_conditions(step_name):
+    """Returns the boolean value for add_licence_conditions from the approval form"""
+
     def _get_form_field_boolean(wizard):
         cleaned_data = wizard.get_cleaned_data_for_step(step_name)
         return cleaned_data.get("add_licence_conditions")
 
     return _get_form_field_boolean
 
 
-def DESNZ_team(wizard):
+def is_desnz_team(wizard):
     return wizard.caseworker["team"]["alias"] in services.DESNZ_TEAMS

caseworker/advice/forms.py

@@ -540,7 +540,6 @@ class PicklistApprovalAdviceFormEdit(BaseForm):
     class Layout:
         TITLE = "Add licence conditions, instructions to exporter or footnotes (optional)"
 
-    DOCUMENT_TITLE = "Recommend approval for this case"
     proviso = forms.CharField(
         widget=forms.Textarea(attrs={"rows": 30, "class": "govuk-!-margin-top-4"}),
         label="",
@@ -557,13 +556,12 @@ def get_layout_fields(self):
         return ("proviso",)
 
 
-class PicklistApprovalAdviceForm(PicklistAdviceForm, BaseForm):
+class LicenceConditionsForm(PicklistAdviceForm, BaseForm):
     DOCUMENT_TITLE = "Recommend approval for this case"
 
     class Layout:
         TITLE = "Add licence conditions, instructions to exporter or footnotes (optional)"
 
-    DOCUMENT_TITLE = "Recommend approval for this case"
     proviso = forms.CharField(
         widget=forms.Textarea(attrs={"rows": 7, "class": "govuk-!-margin-top-4"}),
         label="",
@@ -576,7 +574,7 @@ class Layout:
         widget=forms.RadioSelect,
         choices=(),
     )
-    proviso_radios = forms.MultipleChoiceField(
+    proviso_checkboxes = forms.MultipleChoiceField(
         label="Add a licence condition (optional)",
         required=False,
         widget=forms.CheckboxSelectMultiple,
@@ -586,7 +584,7 @@ class Layout:
     def clean(self):
         cleaned_data = super().clean()
         # only return proviso (text) for selected radios, nothing else matters, join by 2 newlines
-        return {"proviso": "\r\n\r\n".join([cleaned_data[selected] for selected in cleaned_data["proviso_radios"]])}
+        return {"proviso": "\r\n\r\n".join([cleaned_data[selected] for selected in cleaned_data["proviso_checkboxes"]])}
 
     def __init__(self, *args, **kwargs):
         kwargs.pop("approval_reason")
@@ -602,7 +600,7 @@ def __init__(self, *args, **kwargs):
 
         super().__init__(*args, **kwargs)
 
-        self.fields["proviso_radios"].choices = proviso_choices
+        self.fields["proviso_checkboxes"].choices = proviso_choices
         for choices in proviso_choices:
             self.fields[choices.value] = forms.CharField(
                 widget=forms.Textarea(attrs={"rows": 3, "class": "govuk-!-margin-top-4"}),
@@ -613,7 +611,7 @@ def __init__(self, *args, **kwargs):
 
     def get_layout_fields(self):
 
-        return (ConditionalCheckboxes("proviso_radios", *self.conditional_checkbox_choices),)
+        return (ConditionalCheckboxes("proviso_checkboxes", *self.conditional_checkbox_choices),)
 
 
 class FootnotesApprovalAdviceForm(PicklistAdviceForm, BaseForm):

caseworker/advice/templates/advice/view_my_advice.html

@@ -35,7 +35,7 @@ <h2 class="govuk-error-summary__title" id="error-summary-title">
               <h1 class="govuk-heading-xl">View recommendation</h1>
               {% if my_advice %}
                 {% if buttons.edit_recommendation %}
-                  {% if caseworker|user_in_DESNZ_team%}
+                  {% if caseworker|is_desnz_team and case|is_approval %}
                     <a role="button" draggable="false" class="govuk-button govuk-button--secondary" href="{% url 'cases:edit_advice' queue_pk case.id %}">Edit recommendation</a>
                   {% else %}
                     <a role="button" draggable="false" class="govuk-button govuk-button--secondary" href="{% url 'cases:edit_advice_legacy' queue_pk case.id %}">Edit recommendation</a>

caseworker/advice/templatetags/advice_tags.py

@@ -225,5 +225,10 @@ def _add_team_decisions(grouped_advice):
 
 
 @register.filter
-def user_in_DESNZ_team(caseworker):
+def is_desnz_team(caseworker):
     return caseworker["team"]["alias"] in services.DESNZ_TEAMS
+
+
+@register.filter
+def is_approval(case):
+    return case["advice"][0]["type"]["key"] in ["proviso", "approve"]

caseworker/advice/urls.py

@@ -10,8 +10,8 @@
     path("approve-all/", views.GiveApprovalAdviceView.as_view(), name="approve_all"),
     path("refuse-all/", views.RefusalAdviceView.as_view(), name="refuse_all"),
     path("view-my-advice/", views.AdviceDetailView.as_view(), name="view_my_advice"),
-    path("edit-advice-legacy/", views.EditAdviceView.as_view(), name="edit_advice_legacy"),
-    path("edit-advice-desnz/", views.EditAdviceViewDESNZ.as_view(), name="edit_advice"),
+    path("edit-advice-legacy/", views.EditAdviceViewLegacy.as_view(), name="edit_advice_legacy"),
+    path("edit-advice/", views.EditAdviceView.as_view(), name="edit_advice"),
     path("delete-advice/", views.DeleteAdviceView.as_view(), name="delete_advice"),
     path("countersign/", views.CountersignAdviceView.as_view(), name="countersign_advice_view"),
     path("countersign/review-advice/", views.ReviewCountersignView.as_view(), name="countersign_review"),

caseworker/advice/views.py

@@ -1,5 +1,5 @@
 from http import HTTPStatus
-from caseworker.advice.conditionals import form_add_licence_conditions, DESNZ_team
+from caseworker.advice.conditionals import form_add_licence_conditions, is_desnz_team
 from core.wizard.views import BaseSessionWizardView
 from core.wizard.conditionals import C
 import sentry_sdk
@@ -283,7 +283,7 @@ def get_success_url(self):
         return reverse("queues:cases", kwargs={"queue_pk": self.kwargs["queue_pk"]})
 
 
-class EditAdviceView(LoginRequiredMixin, CaseContextMixin, FormView):
+class EditAdviceViewLegacy(LoginRequiredMixin, CaseContextMixin, FormView):
     """
     Form to edit given advice for all products on the application
     """
@@ -357,16 +357,15 @@ def get_context_data(self, **kwargs):
 
 
 class GiveApprovalAdviceView(LoginRequiredMixin, CaseContextMixin, BaseSessionWizardView):
-    # on save combine everything into a single field
 
     form_list = [
         (AdviceView.DESNZ_RECOMMEND_APPROVAL, forms.DESNZRecommendAnApproval),
-        (AdviceView.LICENCE_CONDITIONS, forms.PicklistApprovalAdviceForm),
+        (AdviceView.LICENCE_CONDITIONS, forms.LicenceConditionsForm),
         (AdviceView.LICENCE_FOOTNOTES, forms.FootnotesApprovalAdviceForm),
     ]
 
     condition_dict = {
-        AdviceView.DESNZ_RECOMMEND_APPROVAL: C(DESNZ_team),
+        AdviceView.DESNZ_RECOMMEND_APPROVAL: C(is_desnz_team),
         AdviceView.LICENCE_CONDITIONS: C(form_add_licence_conditions("desnz_recommend_approval")),
         AdviceView.LICENCE_FOOTNOTES: C(form_add_licence_conditions("desnz_recommend_approval")),
     }
@@ -387,15 +386,24 @@ def get_form_kwargs(self, step=None):
     def get_success_url(self):
         return reverse("cases:view_my_advice", kwargs=self.kwargs)
 
+    @expect_status(
+        HTTPStatus.OK,
+        "Error adding approval advice",
+        "Unexpected error adding approval advice",
+    )
+    def post_approval_advice(self, data):
+        return services.post_approval_advice(self.request, self.case, data)
+
     def done(self, form_list, form_dict, **kwargs):
         data = {}
+        # flattens the form_dict
         for form_data in form_dict.values():
             data.update(form_data.cleaned_data)
-        services.post_approval_advice(self.request, self.case, data)
+        self.post_approval_advice(data)
         return redirect(self.get_success_url())
 
 
-class EditAdviceViewDESNZ(GiveApprovalAdviceView):
+class EditAdviceView(GiveApprovalAdviceView):
 
     form_list = [
         (AdviceView.DESNZ_RECOMMEND_APPROVAL, forms.DESNZRecommendAnApproval),
@@ -407,7 +415,8 @@ def get_form_initial(self, step):
         my_advice = services.filter_current_user_advice(self.case.advice, self.caseworker_id)
         advice = my_advice[0]
 
-        # default to other so that they're not likely to reselect, approval_radios isn't stored.
+        # When the form is prepopulated in the edit flow,
+        # the radio values are set to other because only the textfield values are stored it's not possible to replay the selected radio.
         return {
             "add_licence_conditions": bool(advice.get("proviso")),
             "approval_reasons": advice.get("text", ""),

unit_tests/caseworker/advice/templatetags/test_advice_tags.py

@@ -7,7 +7,8 @@
     is_case_pv_graded,
     get_denial_reason_display_values,
     format_serial_numbers,
-    user_in_DESNZ_team,
+    is_desnz_team,
+    is_approval,
 )
 from caseworker.cases.objects import Case
 
@@ -469,7 +470,17 @@ def test_format_serial_numbersformat_serial_numbers(serial_numbers, quantity, ex
     "alias, expected_result",
     ([("DESNZ_CHEMICAL", True), ("GIBBERISH", False)]),
 )
-def test_user_in_DESNZ_team(alias, expected_result):
+def test_is_desnz_team(alias, expected_result):
     caseworker = {"team": {"alias": alias}}
-    result = user_in_DESNZ_team(caseworker)
+    result = is_desnz_team(caseworker)
+    assert result == expected_result
+
+
+@pytest.mark.parametrize(
+    "advice, expected_result",
+    ([("approve", True), ("proviso", True), ("refuse", False)]),
+)
+def test_is_approval(advice, expected_result):
+    case = {"advice": [{"type": {"key": advice}}]}
+    result = is_approval(case)
     assert result == expected_result

unit_tests/caseworker/advice/views/test_select_advice_view.py

@@ -45,7 +45,7 @@ def test_select_advice_post_desnz(mock_get_gov_user, authorized_client, url):
     )
     response = authorized_client.post(url, data={"recommendation": "approve_all"})
     assert response.status_code == 302
-    assert "approve-all-desnz" in response.url
+    assert "approve-all" in response.url
 
 
 def test_view_serial_numbers_for_firearm_product_in_select_advice_view(authorized_client, data_standard_case, url):