From 130a5268d2c258f0abbbf708deaf01d337f449c1 Mon Sep 17 00:00:00 2001 From: Gurdeep Atwal Date: Fri, 27 Dec 2024 15:47:16 +0000 Subject: [PATCH 1/9] add validator --- exporter/applications/forms/parties.py | 3 ++ exporter/core/validators.py | 12 +++++++ .../applications/forms/test_parties.py | 33 +++++++++++++++++++ 3 files changed, 48 insertions(+) diff --git a/exporter/applications/forms/parties.py b/exporter/applications/forms/parties.py index 836c9826f9..823ebfeed3 100644 --- a/exporter/applications/forms/parties.py +++ b/exporter/applications/forms/parties.py @@ -11,6 +11,7 @@ from core.forms.widgets import Autocomplete from exporter.core.constants import CaseTypes, FileUploadFileTypes from exporter.core.services import get_countries +from exporter.core.validators import SpecialCharacterStringValidator from lite_content.lite_exporter_frontend import strings from lite_content.lite_exporter_frontend.applications import PartyForm, PartyTypeForm from lite_forms.common import country_question @@ -222,6 +223,7 @@ class PartyNameForm(BaseForm): 80, f"End user name should be 80 characters or less", ), + SpecialCharacterStringValidator(), ], ) @@ -300,6 +302,7 @@ class PartyAddressForm(BaseForm): address = forms.CharField( widget=forms.Textarea(attrs={"rows": "10"}), error_messages={"required": "Enter an address"}, + validators=[SpecialCharacterStringValidator()], ) country = forms.ChoiceField( choices=[("", "Select a country")], error_messages={"required": "Select the country"} diff --git a/exporter/core/validators.py b/exporter/core/validators.py index f7960cad7b..2b0345e942 100644 --- a/exporter/core/validators.py +++ b/exporter/core/validators.py @@ -1,5 +1,6 @@ from datetime import date from dateutil.relativedelta import relativedelta +import re from django.core.exceptions import ValidationError from django.utils import timezone @@ -52,3 +53,14 @@ def __init__(self, message, **kwargs): def __call__(self, value): if value > (date.today() + self.relativedelta): raise ValidationError(self.message) + + +class SpecialCharacterStringValidator: + message = "There's a invalid special charactor in this field." + regex_string = r"^[\000-\031]" + + def __call__(self, value): + if value: + match_regex = re.compile(self.regex_string) + if bool(match_regex.match(value)): + raise ValidationError(self.message) diff --git a/unit_tests/exporter/applications/forms/test_parties.py b/unit_tests/exporter/applications/forms/test_parties.py index 723bec4b5d..df1548a2dd 100644 --- a/unit_tests/exporter/applications/forms/test_parties.py +++ b/unit_tests/exporter/applications/forms/test_parties.py @@ -94,6 +94,17 @@ def test_consignee_name_form(data, valid, errors): False, {"name": [f"End user name should be 80 characters or less"]}, ), + ( + {"name": "\x02 control chars not allowed"}, + False, + {"name": ["There's a invalid special charactor in this field."]}, + ), + ({"name": "control \x1A is allowed"}, True, None), + ( + {"name": "\x00 control chars not allowed"}, + False, + {"name": ["There's a invalid special charactor in this field.", "Null characters are not allowed."]}, + ), ), ) def test_end_user_name_form(data, valid, errors): @@ -168,6 +179,17 @@ def test_consignee_website_form(data, valid, errors): ({"address": "this\r\nis\r\ninvalid", "country": "aus"}, True, None), ({"address": "this_is_not", "country": "aus"}, True, None), ({"address": "this\w\ais\a\ainvalid", "country": "aus"}, True, None), + ( + {"address": "\x02 control chars not allowed", "country": "aus"}, + False, + {"address": ["There's a invalid special charactor in this field."]}, + ), + ({"address": "control \x1A is allowed", "country": "aus"}, True, None), + ( + {"address": "\x00 control chars not allowed", "country": "aus"}, + False, + {"address": ["There's a invalid special charactor in this field.", "Null characters are not allowed."]}, + ), ), ) @patch("exporter.applications.forms.parties.get_countries") @@ -193,6 +215,17 @@ class Request: ({"address": "", "country": ""}, False, {"address": ["Enter an address"], "country": ["Select the country"]}), ({"address": "This-is-a-valid-address", "country": "aus"}, True, None), ({"address": "this\r\nis\r\ninvalid", "country": "aus"}, True, None), + ( + {"address": "\x02 control chars not allowed", "country": "aus"}, + False, + {"address": ["There's a invalid special charactor in this field."]}, + ), + ({"address": "control \x1A is allowed", "country": "aus"}, True, None), + ( + {"address": "\x00 control chars not allowed", "country": "aus"}, + False, + {"address": ["There's a invalid special charactor in this field.", "Null characters are not allowed."]}, + ), ), ) @patch("exporter.applications.forms.parties.get_countries") From 84faff4a3b37990c83960068218239deef903cf2 Mon Sep 17 00:00:00 2001 From: Gurdeep Atwal Date: Mon, 30 Dec 2024 18:43:41 +0000 Subject: [PATCH 2/9] changed so it cleans data --- core/helpers.py | 4 + exporter/applications/forms/parties.py | 12 ++- exporter/core/validators.py | 12 --- .../applications/forms/test_parties.py | 78 +++++++++++-------- 4 files changed, 58 insertions(+), 48 deletions(-) diff --git a/core/helpers.py b/core/helpers.py index 44c7870567..baab6212fb 100644 --- a/core/helpers.py +++ b/core/helpers.py @@ -72,3 +72,7 @@ def stream_document_response(api_response): ]: response.headers[header_to_copy] = api_response.headers[header_to_copy] return response + + +def remove_non_printable_characters(str): + return "".join([c for c in str if ord(c) > 31 or ord(c) in [9, 10, 13]]) diff --git a/exporter/applications/forms/parties.py b/exporter/applications/forms/parties.py index 823ebfeed3..e7be51a22c 100644 --- a/exporter/applications/forms/parties.py +++ b/exporter/applications/forms/parties.py @@ -1,3 +1,4 @@ +from core.helpers import remove_non_printable_characters from crispy_forms_gds.choices import Choice from crispy_forms_gds.helper import FormHelper from crispy_forms_gds.layout import Layout, Submit, HTML @@ -11,7 +12,6 @@ from core.forms.widgets import Autocomplete from exporter.core.constants import CaseTypes, FileUploadFileTypes from exporter.core.services import get_countries -from exporter.core.validators import SpecialCharacterStringValidator from lite_content.lite_exporter_frontend import strings from lite_content.lite_exporter_frontend.applications import PartyForm, PartyTypeForm from lite_forms.common import country_question @@ -223,10 +223,13 @@ class PartyNameForm(BaseForm): 80, f"End user name should be 80 characters or less", ), - SpecialCharacterStringValidator(), ], ) + def clean_name(self): + name = self.cleaned_data["name"] + return remove_non_printable_characters(name) + def get_layout_fields(self): return ("name",) @@ -302,7 +305,6 @@ class PartyAddressForm(BaseForm): address = forms.CharField( widget=forms.Textarea(attrs={"rows": "10"}), error_messages={"required": "Enter an address"}, - validators=[SpecialCharacterStringValidator()], ) country = forms.ChoiceField( choices=[("", "Select a country")], error_messages={"required": "Select the country"} @@ -318,6 +320,10 @@ def __init__(self, *args, **kwargs): country_choices = [(country["id"], country["name"]) for country in countries] self.fields["country"].choices += country_choices + def clean_address(self): + address = self.cleaned_data["address"] + return remove_non_printable_characters(address) + def get_layout_fields(self): return ( "address", diff --git a/exporter/core/validators.py b/exporter/core/validators.py index 2b0345e942..f7960cad7b 100644 --- a/exporter/core/validators.py +++ b/exporter/core/validators.py @@ -1,6 +1,5 @@ from datetime import date from dateutil.relativedelta import relativedelta -import re from django.core.exceptions import ValidationError from django.utils import timezone @@ -53,14 +52,3 @@ def __init__(self, message, **kwargs): def __call__(self, value): if value > (date.today() + self.relativedelta): raise ValidationError(self.message) - - -class SpecialCharacterStringValidator: - message = "There's a invalid special charactor in this field." - regex_string = r"^[\000-\031]" - - def __call__(self, value): - if value: - match_regex = re.compile(self.regex_string) - if bool(match_regex.match(value)): - raise ValidationError(self.message) diff --git a/unit_tests/exporter/applications/forms/test_parties.py b/unit_tests/exporter/applications/forms/test_parties.py index df1548a2dd..aec1f5e8d0 100644 --- a/unit_tests/exporter/applications/forms/test_parties.py +++ b/unit_tests/exporter/applications/forms/test_parties.py @@ -84,6 +84,26 @@ def test_consignee_name_form(data, valid, errors): assert form.errors == errors +@pytest.mark.parametrize( + "data, expected", + ( + ({"name": "\x02 test1"}, " test1"), + ({"name": "\x02test2"}, "test2"), + ({"name": "this is \n test3"}, "this is \n test3"), + ({"name": "this is \t test4"}, "this is \t test4"), + ({"name": "this is \r test5"}, "this is \r test5"), + ({"name": "namé 6"}, "namé 6"), + ({"name": "namé's"}, "namé's"), + ({"name": "test 8"}, "test 8"), + ), +) +def test_consignee_name_removes_non_printable(data, expected): + form = parties.ConsigneeNameForm(data=data) + + form.is_valid() + assert form.cleaned_data["name"] == expected + + @pytest.mark.parametrize( "data, valid, errors", ( @@ -94,17 +114,6 @@ def test_consignee_name_form(data, valid, errors): False, {"name": [f"End user name should be 80 characters or less"]}, ), - ( - {"name": "\x02 control chars not allowed"}, - False, - {"name": ["There's a invalid special charactor in this field."]}, - ), - ({"name": "control \x1A is allowed"}, True, None), - ( - {"name": "\x00 control chars not allowed"}, - False, - {"name": ["There's a invalid special charactor in this field.", "Null characters are not allowed."]}, - ), ), ) def test_end_user_name_form(data, valid, errors): @@ -179,17 +188,6 @@ def test_consignee_website_form(data, valid, errors): ({"address": "this\r\nis\r\ninvalid", "country": "aus"}, True, None), ({"address": "this_is_not", "country": "aus"}, True, None), ({"address": "this\w\ais\a\ainvalid", "country": "aus"}, True, None), - ( - {"address": "\x02 control chars not allowed", "country": "aus"}, - False, - {"address": ["There's a invalid special charactor in this field."]}, - ), - ({"address": "control \x1A is allowed", "country": "aus"}, True, None), - ( - {"address": "\x00 control chars not allowed", "country": "aus"}, - False, - {"address": ["There's a invalid special charactor in this field.", "Null characters are not allowed."]}, - ), ), ) @patch("exporter.applications.forms.parties.get_countries") @@ -208,6 +206,31 @@ class Request: assert form.errors == errors +@pytest.mark.parametrize( + "data, expected", + ( + ({"address": "1 somewhere", "country": "aus"}, "1 somewhere"), + ({"address": "1 \x02 somewhere", "country": "aus"}, "1 somewhere"), + ({"address": "1 \x02 \n somewhere's", "country": "aus"}, "1 \n somewhere's"), + ({"address": "1 \x01somewhere", "country": "aus"}, "somewhere"), + ({"address": "1 \x03 \n somewhere", "country": "aus"}, "1 somewhere"), + ({"address": "1 \x03 \n ô somewhere", "country": "aus"}, "1 ô somewhere"), + ({"address": "1 \x02 \r somewhere's", "country": "aus"}, "1 \r somewhere's"), + ({"address": "1 \x02 \t somewhere's", "country": "aus"}, "1 \t somewhere's"), + ), +) +@patch("exporter.applications.forms.parties.get_countries") +def test_end_user_address_removes_non_printable(mock_get_countries, data, expected): + class Request: + csp_nonce = "test" + + request = Request() + mock_get_countries.return_value = [{"id": "aus", "name": "Austria"}, {"id": "fr", "name": "France"}] + form = parties.EndUserAddressForm(request=request, data=data) + + form.is_valid() + + @pytest.mark.parametrize( "data, valid, errors", ( @@ -215,17 +238,6 @@ class Request: ({"address": "", "country": ""}, False, {"address": ["Enter an address"], "country": ["Select the country"]}), ({"address": "This-is-a-valid-address", "country": "aus"}, True, None), ({"address": "this\r\nis\r\ninvalid", "country": "aus"}, True, None), - ( - {"address": "\x02 control chars not allowed", "country": "aus"}, - False, - {"address": ["There's a invalid special charactor in this field."]}, - ), - ({"address": "control \x1A is allowed", "country": "aus"}, True, None), - ( - {"address": "\x00 control chars not allowed", "country": "aus"}, - False, - {"address": ["There's a invalid special charactor in this field.", "Null characters are not allowed."]}, - ), ), ) @patch("exporter.applications.forms.parties.get_countries") From 3f7e25357a5e60a4443ea418ba71c8b3b168e14d Mon Sep 17 00:00:00 2001 From: Brendan Smith Date: Fri, 20 Dec 2024 11:53:51 +0000 Subject: [PATCH 3/9] Add line separator to multiple licence condition concatenation --- caseworker/advice/forms/approval.py | 6 +- .../views/test_give_approval_advice_view.py | 73 +++++++++++++++++++ 2 files changed, 78 insertions(+), 1 deletion(-) diff --git a/caseworker/advice/forms/approval.py b/caseworker/advice/forms/approval.py index 81330bff76..4aeea5b4c7 100644 --- a/caseworker/advice/forms/approval.py +++ b/caseworker/advice/forms/approval.py @@ -113,7 +113,11 @@ 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_checkboxes"]])} + return { + "proviso": "\n\n--------\n".join( + [cleaned_data[selected] for selected in cleaned_data["proviso_checkboxes"]] + ) + } def __init__(self, *args, **kwargs): proviso = kwargs.pop("proviso") diff --git a/unit_tests/caseworker/advice/views/test_give_approval_advice_view.py b/unit_tests/caseworker/advice/views/test_give_approval_advice_view.py index a92dac9b53..7beaebfd30 100644 --- a/unit_tests/caseworker/advice/views/test_give_approval_advice_view.py +++ b/unit_tests/caseworker/advice/views/test_give_approval_advice_view.py @@ -6,6 +6,7 @@ from caseworker.advice import services from caseworker.advice.constants import AdviceSteps +from core import client @pytest.fixture(autouse=True) @@ -212,6 +213,78 @@ def test_DESNZ_give_approval_advice_post_valid_add_conditional( assert add_instructions_response.status_code == 302 +@pytest.fixture +def mock_proviso_multiple(requests_mock): + url = client._build_absolute_uri("/picklist/?type=proviso&page=1&disable_pagination=True&show_deactivated=False") + data = { + "results": [ + {"name": "condition 1", "text": "condition 1 text"}, + {"name": "condition 2", "text": "condition 2 text"}, + {"name": "condition 3", "text": "condition 3 text"}, + ] + } + return requests_mock.get(url=url, json=data) + + +@mock.patch("caseworker.advice.views.mixins.get_gov_user") +def test_DESNZ_give_approval_advice_post_valid_multiple_conditions( + mock_get_gov_user, + authorized_client, + data_standard_case, + url, + mock_approval_reason, + mock_proviso_multiple, + mock_footnote_details, + mock_post_advice, + post_to_step, + beautiful_soup, +): + mock_get_gov_user.return_value = ( + { + "user": { + "team": { + "id": "56273dd4-4634-4ad7-a782-e480f85a85a9", + "name": "DESNZ Chemical", + "alias": services.DESNZ_CHEMICAL, + } + } + }, + None, + ) + + response = post_to_step( + AdviceSteps.RECOMMEND_APPROVAL, + {"approval_reasons": "reason", "add_licence_conditions": True}, + ) + assert response.status_code == 200 + soup = beautiful_soup(response.content) + # redirected to next form + header = soup.find("h1") + assert header.text == "Add licence conditions (optional)" + + add_LC_response = post_to_step( + AdviceSteps.LICENCE_CONDITIONS, + { + "proviso_checkboxes": ["condition_1", "condition_3"], + "condition_1": "condition 1 abc", + "condition_3": "condition 3 xyz", + }, + ) + assert add_LC_response.status_code == 200 + soup = beautiful_soup(add_LC_response.content) + # redirected to next form + header = soup.find("h1") + assert header.text == "Add instructions to the exporter, or a reporting footnote (optional)" + + add_instructions_response = post_to_step( + AdviceSteps.LICENCE_FOOTNOTES, + {}, + ) + assert add_instructions_response.status_code == 302 + assert len(mock_post_advice.request_history) == 1 + assert mock_post_advice.request_history[0].json()[0]["proviso"] == "condition 1 abc\n\n--------\ncondition 3 xyz" + + @mock.patch("caseworker.advice.views.mixins.get_gov_user") def test_DESNZ_give_approval_advice_post_valid_add_conditional_optional( mock_get_gov_user, From 5925dd89b75f4840475b0b289062244ca49e2295 Mon Sep 17 00:00:00 2001 From: Brendan Smith Date: Wed, 18 Dec 2024 16:05:08 +0000 Subject: [PATCH 4/9] Prevent 'other' radio option showing for advice picklist fields which have no picklist options --- caseworker/advice/forms/approval.py | 18 +----------------- caseworker/advice/forms/consolidate.py | 2 +- caseworker/advice/forms/forms.py | 6 ++++-- .../views/tests/test_consolidate_advice.py | 5 +---- 4 files changed, 7 insertions(+), 24 deletions(-) diff --git a/caseworker/advice/forms/approval.py b/caseworker/advice/forms/approval.py index 4aeea5b4c7..0e1bbebf20 100644 --- a/caseworker/advice/forms/approval.py +++ b/caseworker/advice/forms/approval.py @@ -3,8 +3,8 @@ from core.common.forms import BaseForm from crispy_forms_gds.helper import FormHelper from crispy_forms_gds.layout import Layout, Submit -from crispy_forms_gds.choices import Choice +from caseworker.advice.forms.forms import PicklistAdviceForm from core.forms.layouts import ( ConditionalCheckboxes, ConditionalCheckboxesQuestion, @@ -28,22 +28,6 @@ def __init__(self, *args, **kwargs): self.helper.add_input(Submit("submit", "Continue")) -class PicklistAdviceForm(forms.Form): - def _picklist_to_choices(self, picklist_data): - reasons_choices = [] - reasons_text = {"other": ""} - - for result in picklist_data["results"]: - key = "_".join(result.get("name").lower().split()) - choice = Choice(key, result.get("name")) - if result == picklist_data["results"][-1]: - choice = Choice(key, result.get("name"), divider="or") - reasons_choices.append(choice) - reasons_text[key] = result.get("text") - reasons_choices.append(Choice("other", "Other")) - return reasons_choices, reasons_text - - class MoveCaseForwardForm(forms.Form): def __init__(self, move_case_button_label="Move case forward", *args, **kwargs): super().__init__(*args, **kwargs) diff --git a/caseworker/advice/forms/consolidate.py b/caseworker/advice/forms/consolidate.py index fd85a27d8c..ccd42f4ebb 100644 --- a/caseworker/advice/forms/consolidate.py +++ b/caseworker/advice/forms/consolidate.py @@ -59,7 +59,7 @@ class ConsolidateApprovalForm(PicklistAdviceForm): def __init__(self, approval_reason, proviso, **kwargs): super().__init__(**kwargs) - approval_choices, approval_text = self._picklist_to_choices(approval_reason, include_other=False) + approval_choices, approval_text = self._picklist_to_choices(approval_reason) self.approval_text = approval_text self.fields["approval_radios"].choices = approval_choices diff --git a/caseworker/advice/forms/forms.py b/caseworker/advice/forms/forms.py index b4c0474327..d7cc103ee4 100644 --- a/caseworker/advice/forms/forms.py +++ b/caseworker/advice/forms/forms.py @@ -31,7 +31,7 @@ def get_approval_advice_form_factory(advice, approval_reason, proviso, footnote_ class PicklistAdviceForm(forms.Form): def _picklist_to_choices(self, picklist_data, include_other=True): reasons_choices = [] - reasons_text = {"other": ""} + reasons_text = {} for result in picklist_data["results"]: key = "_".join(result.get("name").lower().split()) @@ -40,7 +40,9 @@ def _picklist_to_choices(self, picklist_data, include_other=True): choice = Choice(key, result.get("name"), divider="or") reasons_choices.append(choice) reasons_text[key] = result.get("text") - if include_other: + picklist_choices = len(reasons_choices) > 0 + if include_other and picklist_choices: + reasons_text["other"] = "" reasons_choices.append(Choice("other", "Other")) return reasons_choices, reasons_text diff --git a/caseworker/advice/views/tests/test_consolidate_advice.py b/caseworker/advice/views/tests/test_consolidate_advice.py index f0bc4924f9..a20001afbd 100644 --- a/caseworker/advice/views/tests/test_consolidate_advice.py +++ b/caseworker/advice/views/tests/test_consolidate_advice.py @@ -401,10 +401,7 @@ def test_ConsolidateApproveView_GET_canned_snippets( soup = BeautifulSoup(response.content, "html.parser") assert "firearm serial numbers" in soup.find("div", {"id": "div_id_proviso_snippets"}).text assert soup.find("button", attrs={"data-snippet-key": "firearm_serial_numbers"}).text == "Add licence condition" - assert ( - soup.find("script", {"id": "proviso"}).text - == '{"other": "", "firearm_serial_numbers": "Firearm serial numbers text"}' - ) + assert soup.find("script", {"id": "proviso"}).text == '{"firearm_serial_numbers": "Firearm serial numbers text"}' def test_ConsolidateApproveView_POST_bad_input( From c14917b9c92b03768d50c26e32ef32aff4c13036 Mon Sep 17 00:00:00 2001 From: Brendan Smith Date: Thu, 19 Dec 2024 10:29:55 +0000 Subject: [PATCH 5/9] Ensure checkbox licence conditions collapse to single textbox when there are no picklist conditions --- caseworker/advice/constants.py | 2 -- caseworker/advice/forms/approval.py | 32 ++++++++++++++--------------- caseworker/advice/forms/edit.py | 20 ------------------ caseworker/advice/views/approval.py | 32 +++++++++++++++++++++-------- caseworker/advice/views/edit.py | 13 +++++++----- 5 files changed, 48 insertions(+), 51 deletions(-) delete mode 100644 caseworker/advice/forms/edit.py diff --git a/caseworker/advice/constants.py b/caseworker/advice/constants.py index 2a6cc02596..dc13a41352 100644 --- a/caseworker/advice/constants.py +++ b/caseworker/advice/constants.py @@ -30,7 +30,5 @@ class AdviceType: class AdviceSteps: RECOMMEND_APPROVAL = "recommend_approval" - DESNZ_APPROVAL = "desnz_approval" - FCDO_APPROVAL = "fcdo_approval" LICENCE_CONDITIONS = "licence_conditions" LICENCE_FOOTNOTES = "licence_footnotes" diff --git a/caseworker/advice/forms/approval.py b/caseworker/advice/forms/approval.py index 0e1bbebf20..d956bf0405 100644 --- a/caseworker/advice/forms/approval.py +++ b/caseworker/advice/forms/approval.py @@ -71,22 +71,10 @@ def get_layout_fields(self): ) -class LicenceConditionsForm(PicklistAdviceForm, BaseForm): +class PicklistLicenceConditionsForm(PicklistAdviceForm, BaseForm): class Layout: TITLE = "Add licence conditions (optional)" - proviso = forms.CharField( - widget=forms.Textarea(attrs={"rows": 7, "class": "govuk-!-margin-top-4"}), - label="", - required=False, - ) - - approval_radios = forms.ChoiceField( - label="What is your reason for approving?", - required=False, - widget=forms.RadioSelect, - choices=(), - ) proviso_checkboxes = forms.MultipleChoiceField( label="", required=False, @@ -96,7 +84,7 @@ class Layout: def clean(self): cleaned_data = super().clean() - # only return proviso (text) for selected radios, nothing else matters, join by 2 newlines + # only return proviso (text) for selected checkboxes, nothing else matters, join by 2 newlines return { "proviso": "\n\n--------\n".join( [cleaned_data[selected] for selected in cleaned_data["proviso_checkboxes"]] @@ -107,7 +95,6 @@ def __init__(self, *args, **kwargs): proviso = kwargs.pop("proviso") proviso_choices, proviso_text = self._picklist_to_choices(proviso) - self.proviso_text = proviso_text self.conditional_checkbox_choices = ( ConditionalCheckboxesQuestion(choices.label, choices.value) for choices in proviso_choices @@ -125,10 +112,23 @@ def __init__(self, *args, **kwargs): ) def get_layout_fields(self): - return (ConditionalCheckboxes("proviso_checkboxes", *self.conditional_checkbox_choices),) +class SimpleLicenceConditionsForm(BaseForm): + class Layout: + TITLE = "Add licence conditions (optional)" + + proviso = forms.CharField( + widget=forms.Textarea(attrs={"rows": 7, "class": "govuk-!-margin-top-4"}), + label="Licence condition", + required=False, + ) + + def get_layout_fields(self): + return ("proviso",) + + class FootnotesApprovalAdviceForm(PicklistAdviceForm, BaseForm): class Layout: TITLE = "Add instructions to the exporter, or a reporting footnote (optional)" diff --git a/caseworker/advice/forms/edit.py b/caseworker/advice/forms/edit.py deleted file mode 100644 index ef68bed300..0000000000 --- a/caseworker/advice/forms/edit.py +++ /dev/null @@ -1,20 +0,0 @@ -from django import forms - -from core.common.forms import BaseForm - - -class PicklistApprovalAdviceEditForm(BaseForm): - class Layout: - TITLE = "Add licence conditions (optional)" - - proviso = forms.CharField( - widget=forms.Textarea(attrs={"rows": 30, "class": "govuk-!-margin-top-4"}), - label="", - required=False, - ) - - def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) - - def get_layout_fields(self): - return ("proviso",) diff --git a/caseworker/advice/views/approval.py b/caseworker/advice/views/approval.py index b27d3e7fc9..29dee1a235 100644 --- a/caseworker/advice/views/approval.py +++ b/caseworker/advice/views/approval.py @@ -2,7 +2,8 @@ from caseworker.advice.conditionals import form_add_licence_conditions, is_desnz_team from caseworker.advice.forms.approval import ( FootnotesApprovalAdviceForm, - LicenceConditionsForm, + PicklistLicenceConditionsForm, + SimpleLicenceConditionsForm, RecommendAnApprovalForm, ) from caseworker.advice.payloads import GiveApprovalAdvicePayloadBuilder @@ -19,13 +20,7 @@ from core.decorators import expect_status -class GiveApprovalAdviceView(LoginRequiredMixin, CaseContextMixin, BaseSessionWizardView): - - form_list = [ - (AdviceSteps.RECOMMEND_APPROVAL, RecommendAnApprovalForm), - (AdviceSteps.LICENCE_CONDITIONS, LicenceConditionsForm), - (AdviceSteps.LICENCE_FOOTNOTES, FootnotesApprovalAdviceForm), - ] +class BaseApprovalAdviceView(LoginRequiredMixin, CaseContextMixin, BaseSessionWizardView): condition_dict = { AdviceSteps.RECOMMEND_APPROVAL: C(is_desnz_team), @@ -33,6 +28,12 @@ class GiveApprovalAdviceView(LoginRequiredMixin, CaseContextMixin, BaseSessionWi AdviceSteps.LICENCE_FOOTNOTES: C(form_add_licence_conditions(AdviceSteps.RECOMMEND_APPROVAL)), } + form_list = [ + (AdviceSteps.RECOMMEND_APPROVAL, RecommendAnApprovalForm), + (AdviceSteps.LICENCE_CONDITIONS, PicklistLicenceConditionsForm), + (AdviceSteps.LICENCE_FOOTNOTES, FootnotesApprovalAdviceForm), + ] + step_kwargs = { AdviceSteps.RECOMMEND_APPROVAL: approval_picklist, AdviceSteps.LICENCE_CONDITIONS: proviso_picklist, @@ -62,3 +63,18 @@ def done(self, form_list, form_dict, **kwargs): data = self.get_payload(form_dict) self.post_approval_advice(data) return redirect(self.get_success_url()) + + +class GiveApprovalAdviceView(BaseApprovalAdviceView): + + def get_form(self, step=None, data=None, files=None): + + if step == AdviceSteps.LICENCE_CONDITIONS: + picklist_form_kwargs = self.step_kwargs[AdviceSteps.LICENCE_CONDITIONS](self) + picklist_options_exist = len(picklist_form_kwargs["proviso"]["results"]) > 0 + if picklist_options_exist: + return PicklistLicenceConditionsForm(data=data, **picklist_form_kwargs) + else: + return SimpleLicenceConditionsForm(data=data) + + return super().get_form(step, data, files) diff --git a/caseworker/advice/views/edit.py b/caseworker/advice/views/edit.py index 58a71f099a..a419cfe237 100644 --- a/caseworker/advice/views/edit.py +++ b/caseworker/advice/views/edit.py @@ -1,16 +1,19 @@ -from caseworker.advice.forms.approval import FootnotesApprovalAdviceForm, RecommendAnApprovalForm -from caseworker.advice.forms.edit import PicklistApprovalAdviceEditForm -from caseworker.advice.views.approval import GiveApprovalAdviceView +from caseworker.advice.forms.approval import ( + FootnotesApprovalAdviceForm, + RecommendAnApprovalForm, + SimpleLicenceConditionsForm, +) +from caseworker.advice.views.approval import BaseApprovalAdviceView from caseworker.advice import services from caseworker.advice.constants import AdviceSteps from caseworker.advice.picklist_helpers import approval_picklist, footnote_picklist -class EditAdviceView(GiveApprovalAdviceView): +class EditAdviceView(BaseApprovalAdviceView): form_list = [ (AdviceSteps.RECOMMEND_APPROVAL, RecommendAnApprovalForm), - (AdviceSteps.LICENCE_CONDITIONS, PicklistApprovalAdviceEditForm), + (AdviceSteps.LICENCE_CONDITIONS, SimpleLicenceConditionsForm), (AdviceSteps.LICENCE_FOOTNOTES, FootnotesApprovalAdviceForm), ] From 8956e15989fee4320deb30f20986fa1bfaf7d91a Mon Sep 17 00:00:00 2001 From: Brendan Smith Date: Fri, 20 Dec 2024 16:41:58 +0000 Subject: [PATCH 6/9] Fix coverage --- .../views/test_give_approval_advice_view.py | 63 ++++++++++++++++++- 1 file changed, 60 insertions(+), 3 deletions(-) diff --git a/unit_tests/caseworker/advice/views/test_give_approval_advice_view.py b/unit_tests/caseworker/advice/views/test_give_approval_advice_view.py index 7beaebfd30..5a2a4504bd 100644 --- a/unit_tests/caseworker/advice/views/test_give_approval_advice_view.py +++ b/unit_tests/caseworker/advice/views/test_give_approval_advice_view.py @@ -251,7 +251,6 @@ def test_DESNZ_give_approval_advice_post_valid_multiple_conditions( }, None, ) - response = post_to_step( AdviceSteps.RECOMMEND_APPROVAL, {"approval_reasons": "reason", "add_licence_conditions": True}, @@ -261,7 +260,6 @@ def test_DESNZ_give_approval_advice_post_valid_multiple_conditions( # redirected to next form header = soup.find("h1") assert header.text == "Add licence conditions (optional)" - add_LC_response = post_to_step( AdviceSteps.LICENCE_CONDITIONS, { @@ -275,7 +273,6 @@ def test_DESNZ_give_approval_advice_post_valid_multiple_conditions( # redirected to next form header = soup.find("h1") assert header.text == "Add instructions to the exporter, or a reporting footnote (optional)" - add_instructions_response = post_to_step( AdviceSteps.LICENCE_FOOTNOTES, {}, @@ -285,6 +282,66 @@ def test_DESNZ_give_approval_advice_post_valid_multiple_conditions( assert mock_post_advice.request_history[0].json()[0]["proviso"] == "condition 1 abc\n\n--------\ncondition 3 xyz" +@pytest.fixture +def mock_no_provisos(requests_mock): + url = client._build_absolute_uri("/picklist/?type=proviso&page=1&disable_pagination=True&show_deactivated=False") + data = {"results": []} + return requests_mock.get(url=url, json=data) + + +@mock.patch("caseworker.advice.views.mixins.get_gov_user") +def test_DESNZ_give_approval_advice_post_valid_no_provisos( + mock_get_gov_user, + authorized_client, + data_standard_case, + url, + mock_approval_reason, + mock_no_provisos, + mock_footnote_details, + mock_post_advice, + post_to_step, + beautiful_soup, +): + mock_get_gov_user.return_value = ( + { + "user": { + "team": { + "id": "56273dd4-4634-4ad7-a782-e480f85a85a9", + "name": "DESNZ Chemical", + "alias": services.DESNZ_CHEMICAL, + } + } + }, + None, + ) + response = post_to_step( + AdviceSteps.RECOMMEND_APPROVAL, + {"approval_reasons": "reason", "add_licence_conditions": True}, + ) + assert response.status_code == 200 + soup = beautiful_soup(response.content) + # redirected to next form + header = soup.find("h1") + assert header.text == "Add licence conditions (optional)" + + add_LC_response = post_to_step( + AdviceSteps.LICENCE_CONDITIONS, + {"proviso": "proviso"}, + ) + assert add_LC_response.status_code == 200 + soup = beautiful_soup(add_LC_response.content) + # redirected to next form + header = soup.find("h1") + assert header.text == "Add instructions to the exporter, or a reporting footnote (optional)" + + add_instructions_response = post_to_step( + AdviceSteps.LICENCE_FOOTNOTES, + {"instructions_to_exporter": "instructions", "footnote_details": "footnotes"}, + ) + assert add_instructions_response.status_code == 302 + assert len(mock_post_advice.request_history) == 1 + + @mock.patch("caseworker.advice.views.mixins.get_gov_user") def test_DESNZ_give_approval_advice_post_valid_add_conditional_optional( mock_get_gov_user, From dd72b1f4f5a3a7eab9a04808e43e210aaa105623 Mon Sep 17 00:00:00 2001 From: Brendan Smith Date: Mon, 6 Jan 2025 12:37:19 +0000 Subject: [PATCH 7/9] Ensure data prefix passed to form in GiveApprovalAdviceView --- caseworker/advice/views/approval.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/caseworker/advice/views/approval.py b/caseworker/advice/views/approval.py index 29dee1a235..4565fb8cce 100644 --- a/caseworker/advice/views/approval.py +++ b/caseworker/advice/views/approval.py @@ -73,8 +73,8 @@ def get_form(self, step=None, data=None, files=None): picklist_form_kwargs = self.step_kwargs[AdviceSteps.LICENCE_CONDITIONS](self) picklist_options_exist = len(picklist_form_kwargs["proviso"]["results"]) > 0 if picklist_options_exist: - return PicklistLicenceConditionsForm(data=data, **picklist_form_kwargs) + return PicklistLicenceConditionsForm(data=data, prefix=step, **picklist_form_kwargs) else: - return SimpleLicenceConditionsForm(data=data) + return SimpleLicenceConditionsForm(data=data, prefix=step) return super().get_form(step, data, files) From 629e9c11e80e09a49eb2f77a69406e6c897a2b68 Mon Sep 17 00:00:00 2001 From: Brendan Smith Date: Mon, 6 Jan 2025 12:56:25 +0000 Subject: [PATCH 8/9] Remove unecessary margin styling --- caseworker/advice/forms/approval.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/caseworker/advice/forms/approval.py b/caseworker/advice/forms/approval.py index d956bf0405..27e18e22ec 100644 --- a/caseworker/advice/forms/approval.py +++ b/caseworker/advice/forms/approval.py @@ -40,7 +40,7 @@ class Layout: TITLE = "Recommend an approval" approval_reasons = forms.CharField( - widget=forms.Textarea(attrs={"rows": 7, "class": "govuk-!-margin-top-4"}), + widget=forms.Textarea(attrs={"rows": 7}), label="", error_messages={"required": "Enter a reason for approving"}, ) @@ -105,7 +105,7 @@ def __init__(self, *args, **kwargs): 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"}), + widget=forms.Textarea(attrs={"rows": 3}), label="Description", required=False, initial=proviso_text[choices.value], @@ -120,7 +120,7 @@ class Layout: TITLE = "Add licence conditions (optional)" proviso = forms.CharField( - widget=forms.Textarea(attrs={"rows": 7, "class": "govuk-!-margin-top-4"}), + widget=forms.Textarea(attrs={"rows": 7}), label="Licence condition", required=False, ) @@ -147,7 +147,7 @@ class Layout: choices=(), ) footnote_details = forms.CharField( - widget=forms.Textarea(attrs={"rows": 3, "class": "govuk-!-margin-top-4"}), + widget=forms.Textarea(attrs={"rows": 3}), label="", required=False, ) From 7363d75bc4b5380b70cc2b5abb5cd3634ede6417 Mon Sep 17 00:00:00 2001 From: Gurdeep Atwal Date: Mon, 6 Jan 2025 15:13:56 +0000 Subject: [PATCH 9/9] add comment --- core/helpers.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/helpers.py b/core/helpers.py index baab6212fb..a5d58a247f 100644 --- a/core/helpers.py +++ b/core/helpers.py @@ -75,4 +75,6 @@ def stream_document_response(api_response): def remove_non_printable_characters(str): + # Given a string this will remove all non_printable_characters from ascii table + # Chars 9(/h), 10 (/n), 13(/r) are maintained return "".join([c for c in str if ord(c) > 31 or ord(c) in [9, 10, 13]])