From bdcdfa6d0fc1fe1e5073dd9d252e316aa8900e28 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 2 Mar 2022 16:04:05 +0000 Subject: [PATCH 01/13] Standardise branding tests to start with endpoint This makes it easier to find all the tests that are relevant and is consistent with how we name tests elsewhere in the app. --- tests/app/main/views/test_service_settings.py | 50 +++++++++---------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 274f777cc..6f44f6226 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -4778,7 +4778,7 @@ def test_update_service_organisation_does_not_update_if_same_value( ('something_else', 'Something else'), ]) )) -def test_show_email_branding_request_page_when_no_branding_is_set( +def test_email_branding_request_page_when_no_branding_is_set( service_one, client_request, mocker, @@ -4835,7 +4835,7 @@ def test_show_email_branding_request_page_when_no_branding_is_set( ('emergency_service', None), ('other', None), )) -def test_letter_show_branding_request_page_when_no_branding_is_set( +def test_letter_branding_request_page_when_no_branding_is_set( service_one, client_request, mock_get_email_branding, @@ -4902,7 +4902,7 @@ def test_letter_show_branding_request_page_when_no_branding_is_set( ('something_else', 'Something else'), ]), )) -def test_show_branding_request_page_when_no_branding_is_set_but_organisation_exists( +def test_branding_request_page_when_no_branding_is_set_but_organisation_exists( mocker, service_one, client_request, @@ -4953,7 +4953,7 @@ def test_show_branding_request_page_when_no_branding_is_set_but_organisation_exi ('something_else', 'Something else'), ], 'letter'), )) -def test_show_branding_request_page_when_no_branding_is_set_but_organisation_exists_central_org( +def test_branding_request_page_when_no_branding_is_set_but_organisation_exists_central_org( mocker, service_one, client_request, @@ -4986,7 +4986,7 @@ def test_show_branding_request_page_when_no_branding_is_set_but_organisation_exi ] == expected_options -def test_show_email_branding_request_page_when_email_branding_is_set( +def test_email_branding_request_page_when_email_branding_is_set( mocker, service_one, client_request, @@ -5022,7 +5022,7 @@ def test_show_email_branding_request_page_when_email_branding_is_set( ] -def test_show_letter_branding_request_page_when_letter_branding_is_set( +def test_letter_branding_request_page_when_letter_branding_is_set( mocker, service_one, client_request, @@ -5051,7 +5051,7 @@ def test_show_letter_branding_request_page_when_letter_branding_is_set( ] -def test_back_link_on_email_branding_request_page( +def test_email_branding_request_page_back_link( client_request, ): page = client_request.get( @@ -5066,7 +5066,7 @@ def test_back_link_on_email_branding_request_page( (None, '/services/{}/service-settings'.format(SERVICE_ONE_ID),), (TEMPLATE_ONE_ID, '/services/{}/templates/{}'.format(SERVICE_ONE_ID, TEMPLATE_ONE_ID),) ]) -def test_back_link_on_letter_branding_request_page( +def test_letter_branding_request_page_back_link( client_request, from_template, back_link_url, @@ -5084,7 +5084,7 @@ def test_back_link_on_letter_branding_request_page( assert back_link[0].attrs['href'] == back_link_url -def test_show_letter_branding_request_page_when_branding_is_same_as_org( +def test_letter_branding_request_page_when_branding_is_same_as_org( mocker, service_one, client_request, @@ -5155,7 +5155,7 @@ def test_show_letter_branding_request_page_when_branding_is_same_as_org( 'main.email_branding_nhs', ), )) -def test_submit_email_branding_request( +def test_email_branding_request_submit( client_request, service_one, mocker, @@ -5187,7 +5187,7 @@ def test_submit_email_branding_request( ) -def test_submit_email_branding_request_when_no_radio_button_is_selected( +def test_email_branding_request_submit_when_no_radio_button_is_selected( client_request, service_one, mock_get_email_branding, @@ -5207,7 +5207,7 @@ def test_submit_email_branding_request_when_no_radio_button_is_selected( (None, 'Can’t tell (domain is user.gov.uk)'), ('Test Organisation', 'Test Organisation'), )) -def test_submit_letter_branding_request( +def test_letter_branding_request_submit( client_request, service_one, mocker, @@ -5277,7 +5277,7 @@ def test_submit_letter_branding_request( ({'options': 'something_else'}, 'Cannot be empty'), # no data in 'something_else' textbox ({'options': ''}, 'Select an option'), # no radio button selected )) -def test_submit_letter_branding_request_when_form_has_missing_data( +def test_letter_branding_request_submit_when_form_has_missing_data( client_request, mocker, service_one, @@ -5306,7 +5306,7 @@ def test_submit_letter_branding_request_when_form_has_missing_data( None, TEMPLATE_ONE_ID ]) -def test_submit_letter_branding_request_redirects_if_from_template_is_set( +def test_letter_branding_request_submit_redirects_if_from_template_is_set( client_request, service_one, mocker, @@ -5333,7 +5333,7 @@ def test_submit_letter_branding_request_redirects_if_from_template_is_set( ) -def test_submit_letter_branding_when_something_else_is_only_option( +def test_letter_branding_submit_when_something_else_is_only_option( client_request, service_one, mocker, @@ -5366,7 +5366,7 @@ def test_submit_letter_branding_when_something_else_is_only_option( ('main.email_branding_govuk_and_org', 'Before you request new branding'), ('main.email_branding_organisation', 'When you request new branding'), ]) -def test_get_email_branding_description_pages_for_org_branding( +def test_email_branding_description_pages_for_org_branding( client_request, mocker, service_one, @@ -5395,7 +5395,7 @@ def test_get_email_branding_description_pages_for_org_branding( ('main.email_branding_govuk', 'central', '__NONE__'), ('main.email_branding_nhs', 'nhs_local', NHS_BRANDING_ID), ]) -def test_get_email_branding_govuk_and_nhs_pages( +def test_email_branding_govuk_and_nhs_pages( client_request, mocker, service_one, @@ -5424,7 +5424,7 @@ def test_get_email_branding_govuk_and_nhs_pages( assert normalize_spaces(page.select_one('.page-footer button').text) == 'Use this branding' -def test_get_email_branding_something_else_page(client_request, service_one): +def test_email_branding_something_else_page(client_request, service_one): # expect to have a "NHS" option as well as the # fallback, so back button goes to choices page service_one['organisation_type'] = 'nhs_central' @@ -5461,7 +5461,7 @@ def test_get_email_branding_something_else_page_is_only_option(client_request, s ('main.email_branding_nhs'), ('main.email_branding_organisation'), ]) -def test_get_email_branding_description_pages_give_404_if_selected_branding_not_allowed( +def test_email_branding_pages_give_404_if_selected_branding_not_allowed( client_request, endpoint, ): @@ -5474,7 +5474,7 @@ def test_get_email_branding_description_pages_give_404_if_selected_branding_not_ ) -def test_update_email_branding_from_govuk_preview_page( +def test_email_branding_govuk_submit( mocker, client_request, service_one, @@ -5510,7 +5510,7 @@ def test_update_email_branding_from_govuk_preview_page( assert normalize_spaces(page.select_one('.banner-default').text) == 'You’ve updated your email branding' -def test_submit_email_branding_request_from_govuk_and_org_description_page( +def test_email_branding_govuk_and_org_submit( mocker, client_request, service_one, @@ -5569,7 +5569,7 @@ def test_submit_email_branding_request_from_govuk_and_org_description_page( ) -def test_update_email_branding_from_nhs_preview_page( +def test_email_branding_nhs_submit( mocker, client_request, service_one, @@ -5597,7 +5597,7 @@ def test_update_email_branding_from_nhs_preview_page( assert normalize_spaces(page.select_one('.banner-default').text) == 'You’ve updated your email branding' -def test_submit_email_branding_request_from_organisation_description_page( +def test_email_branding_organisation_submit( mocker, client_request, service_one, @@ -5656,7 +5656,7 @@ def test_submit_email_branding_request_from_organisation_description_page( ) -def test_submit_email_branding_something_else_page( +def test_email_branding_something_else_submit( client_request, mocker, service_one, @@ -5708,7 +5708,7 @@ def test_submit_email_branding_something_else_page( ) -def test_submit_email_branding_something_else_page_shows_error_if_textbox_is_empty( +def test_email_branding_something_else_submit_shows_error_if_textbox_is_empty( client_request, ): page = client_request.post( From 26d1222f1c73d552dcdf837acba45a8775b68a86 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 3 Mar 2022 11:40:03 +0000 Subject: [PATCH 02/13] Rename branding forms to clarify who they're for I've often struggled to find the form associated with a particular page due to the overlapping names e.g. "SetEmailBranding" sounds more like the radio button form a user sees than "BrandingOptions". Almost every form in forms.py also ends with "Form", so this also makes the branding forms consistent with that naming convention. --- app/main/forms.py | 16 ++++++++-------- app/main/views/email_branding.py | 6 +++--- app/main/views/letter_branding.py | 6 +++--- app/main/views/organisations.py | 14 +++++++------- app/main/views/service_settings.py | 14 +++++++------- 5 files changed, 28 insertions(+), 28 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 5eac807b5..2a13171a9 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1876,7 +1876,7 @@ class ServiceSwitchChannelForm(ServiceOnOffSettingForm): super().__init__(name, *args, **kwargs) -class SetEmailBranding(StripWhitespaceForm): +class AdminSetEmailBrandingForm(StripWhitespaceForm): branding_style = GovukRadiosFieldWithNoneOption( 'Branding style', @@ -1900,17 +1900,17 @@ class SetEmailBranding(StripWhitespaceForm): ) -class SetLetterBranding(SetEmailBranding): +class AdminSetLetterBrandingForm(AdminSetEmailBrandingForm): # form is the same, but instead of GOV.UK we have None as a valid option DEFAULT = (FieldWithNoneOption.NONE_OPTION_VALUE, 'None') -class PreviewBranding(StripWhitespaceForm): +class AdminPreviewBrandingForm(StripWhitespaceForm): branding_style = HiddenFieldWithNoneOption('branding_style') -class ServiceUpdateEmailBranding(StripWhitespaceForm): +class AdminEditEmailBrandingForm(StripWhitespaceForm): name = GovukTextInputField('Name of brand') text = GovukTextInputField('Text') colour = GovukTextInputField( @@ -1939,6 +1939,10 @@ class ServiceUpdateEmailBranding(StripWhitespaceForm): raise ValidationError('This field is required') +class AdminEditLetterBrandingForm(StripWhitespaceForm): + name = GovukTextInputField('Name of brand', validators=[DataRequired()]) + + class SVGFileUpload(StripWhitespaceForm): file = FileField_wtf( 'Upload an SVG logo', @@ -1951,10 +1955,6 @@ class SVGFileUpload(StripWhitespaceForm): ) -class ServiceLetterBrandingDetails(StripWhitespaceForm): - name = GovukTextInputField('Name of brand', validators=[DataRequired()]) - - class PDFUploadForm(StripWhitespaceForm): file = FileField_wtf( 'Upload a letter in PDF format', diff --git a/app/main/views/email_branding.py b/app/main/views/email_branding.py index 16e5adec6..c2c00c1e7 100644 --- a/app/main/views/email_branding.py +++ b/app/main/views/email_branding.py @@ -2,7 +2,7 @@ from flask import current_app, redirect, render_template, session, url_for from app import email_branding_client from app.main import main -from app.main.forms import SearchByNameForm, ServiceUpdateEmailBranding +from app.main.forms import AdminEditEmailBrandingForm, SearchByNameForm from app.s3_client.s3_logo_client import ( TEMP_TAG, delete_email_temp_file, @@ -32,7 +32,7 @@ def email_branding(): def update_email_branding(branding_id, logo=None): email_branding = email_branding_client.get_email_branding(branding_id)['email_branding'] - form = ServiceUpdateEmailBranding( + form = AdminEditEmailBrandingForm( name=email_branding['name'], text=email_branding['text'], colour=email_branding['colour'], @@ -86,7 +86,7 @@ def update_email_branding(branding_id, logo=None): @main.route("/email-branding/create/", methods=['GET', 'POST']) @user_is_platform_admin def create_email_branding(logo=None): - form = ServiceUpdateEmailBranding(brand_type='org') + form = AdminEditEmailBrandingForm(brand_type='org') if form.validate_on_submit(): if form.file.data: diff --git a/app/main/views/letter_branding.py b/app/main/views/letter_branding.py index 08f0332eb..9446e5591 100644 --- a/app/main/views/letter_branding.py +++ b/app/main/views/letter_branding.py @@ -12,8 +12,8 @@ from notifications_python_client.errors import HTTPError from app import letter_branding_client from app.main import main from app.main.forms import ( + AdminEditLetterBrandingForm, SearchByNameForm, - ServiceLetterBrandingDetails, SVGFileUpload, ) from app.s3_client.s3_logo_client import ( @@ -48,7 +48,7 @@ def update_letter_branding(branding_id, logo=None): letter_branding = letter_branding_client.get_letter_branding(branding_id) file_upload_form = SVGFileUpload() - letter_branding_details_form = ServiceLetterBrandingDetails( + letter_branding_details_form = AdminEditLetterBrandingForm( name=letter_branding['name'], ) @@ -123,7 +123,7 @@ def update_letter_branding(branding_id, logo=None): @user_is_platform_admin def create_letter_branding(logo=None): file_upload_form = SVGFileUpload() - letter_branding_details_form = ServiceLetterBrandingDetails() + letter_branding_details_form = AdminEditLetterBrandingForm() file_upload_form_submitted = file_upload_form.file.data details_form_submitted = request.form.get('operation') == 'branding-details' diff --git a/app/main/views/organisations.py b/app/main/views/organisations.py index edcc60f98..8808800a8 100644 --- a/app/main/views/organisations.py +++ b/app/main/views/organisations.py @@ -19,6 +19,9 @@ from app.main import main from app.main.forms import ( AddGPOrganisationForm, AddNHSLocalOrganisationForm, + AdminPreviewBrandingForm, + AdminSetEmailBrandingForm, + AdminSetLetterBrandingForm, BillingDetailsForm, EditNotesForm, GoLiveNotesForm, @@ -28,12 +31,9 @@ from app.main.forms import ( OrganisationCrownStatusForm, OrganisationDomainsForm, OrganisationOrganisationTypeForm, - PreviewBranding, RenameOrganisationForm, SearchByNameForm, SearchUsersForm, - SetEmailBranding, - SetLetterBranding, ) from app.main.views.dashboard import ( get_tuples_of_financial_years, @@ -405,7 +405,7 @@ def edit_organisation_email_branding(org_id): email_branding = email_branding_client.get_all_email_branding() - form = SetEmailBranding( + form = AdminSetEmailBrandingForm( all_branding_options=get_branding_as_value_and_label(email_branding), current_branding=current_organisation.email_branding_id, ) @@ -430,7 +430,7 @@ def organisation_preview_email_branding(org_id): branding_style = request.args.get('branding_style', None) - form = PreviewBranding(branding_style=branding_style) + form = AdminPreviewBrandingForm(branding_style=branding_style) if form.validate_on_submit(): current_organisation.update( @@ -451,7 +451,7 @@ def organisation_preview_email_branding(org_id): def edit_organisation_letter_branding(org_id): letter_branding = letter_branding_client.get_all_letter_branding() - form = SetLetterBranding( + form = AdminSetLetterBrandingForm( all_branding_options=get_branding_as_value_and_label(letter_branding), current_branding=current_organisation.letter_branding_id, ) @@ -475,7 +475,7 @@ def edit_organisation_letter_branding(org_id): def organisation_preview_letter_branding(org_id): branding_style = request.args.get('branding_style') - form = PreviewBranding(branding_style=branding_style) + form = AdminPreviewBrandingForm(branding_style=branding_style) if form.validate_on_submit(): current_organisation.update( diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index e8403acae..d887db905 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -38,6 +38,9 @@ from app.extensions import zendesk_client from app.formatters import email_safe from app.main import main from app.main.forms import ( + AdminPreviewBrandingForm, + AdminSetEmailBrandingForm, + AdminSetLetterBrandingForm, BillingDetailsForm, BrandingOptions, EditNotesForm, @@ -45,7 +48,6 @@ from app.main.forms import ( FreeSMSAllowance, LinkOrganisationsForm, MessageLimit, - PreviewBranding, RateLimit, RenameServiceForm, SearchByNameForm, @@ -62,8 +64,6 @@ from app.main.forms import ( ServiceReplyToEmailForm, ServiceSmsSenderForm, ServiceSwitchChannelForm, - SetEmailBranding, - SetLetterBranding, SMSPrefixForm, SomethingElseBrandingForm, ) @@ -1022,7 +1022,7 @@ def set_rate_limit(service_id): def service_set_email_branding(service_id): email_branding = email_branding_client.get_all_email_branding() - form = SetEmailBranding( + form = AdminSetEmailBrandingForm( all_branding_options=get_branding_as_value_and_label(email_branding), current_branding=current_service.email_branding_id, ) @@ -1046,7 +1046,7 @@ def service_set_email_branding(service_id): def service_preview_email_branding(service_id): branding_style = request.args.get('branding_style', None) - form = PreviewBranding(branding_style=branding_style) + form = AdminPreviewBrandingForm(branding_style=branding_style) if form.validate_on_submit(): current_service.update( @@ -1067,7 +1067,7 @@ def service_preview_email_branding(service_id): def service_set_letter_branding(service_id): letter_branding = letter_branding_client.get_all_letter_branding() - form = SetLetterBranding( + form = AdminSetLetterBrandingForm( all_branding_options=get_branding_as_value_and_label(letter_branding), current_branding=current_service.letter_branding_id, ) @@ -1091,7 +1091,7 @@ def service_set_letter_branding(service_id): def service_preview_letter_branding(service_id): branding_style = request.args.get('branding_style') - form = PreviewBranding(branding_style=branding_style) + form = AdminPreviewBrandingForm(branding_style=branding_style) if form.validate_on_submit(): current_service.update( From a04ed3eca5a7d58f244ea87978d6f47c2bfef603 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 3 Mar 2022 12:05:44 +0000 Subject: [PATCH 03/13] Use separate classes for branding option forms These are about to become a lot less similar to each other when we add email branding pools. Note that the optional *args and *kwargs weren't used anywhere. --- app/main/forms.py | 18 ++++++++++++++---- app/main/views/service_settings.py | 28 +++++++++++++++------------- 2 files changed, 29 insertions(+), 17 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 2a13171a9..f7c08c1f2 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -2180,7 +2180,7 @@ class LinkOrganisationsForm(StripWhitespaceForm): ) -class BrandingOptions(StripWhitespaceForm): +class ChooseBrandingForm(StripWhitespaceForm): FALLBACK_OPTION_VALUE = 'something_else' FALLBACK_OPTION = (FALLBACK_OPTION_VALUE, 'Something else') @@ -2188,8 +2188,8 @@ class BrandingOptions(StripWhitespaceForm): options = RadioField('Choose your new branding') something_else = TextAreaField('Describe the branding you want') - def __init__(self, service, *args, branding_type="email", **kwargs): - super().__init__(*args, **kwargs) + def __init__(self, service, branding_type): + super().__init__() self.branding_type = branding_type self.options.choices = tuple(self.get_available_choices(service, branding_type)) self.options.label.text = 'Choose your new {} branding'.format(branding_type) @@ -2248,7 +2248,7 @@ class BrandingOptions(StripWhitespaceForm): ): yield ('organisation', service.organisation.name) - yield BrandingOptions.FALLBACK_OPTION + yield ChooseBrandingForm.FALLBACK_OPTION @property def something_else_is_only_option(self): @@ -2266,6 +2266,16 @@ class BrandingOptions(StripWhitespaceForm): field.data = '' +class ChooseEmailBrandingForm(ChooseBrandingForm): + def __init__(self, service_id): + ChooseBrandingForm.__init__(self, service_id, branding_type='email') + + +class ChooseLetterBrandingForm(ChooseBrandingForm): + def __init__(self, service_id): + ChooseBrandingForm.__init__(self, service_id, branding_type='letter') + + class SomethingElseBrandingForm(StripWhitespaceForm): something_else = GovukTextareaField( 'Describe the branding you want', diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index d887db905..eb2ca5dee 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -42,7 +42,9 @@ from app.main.forms import ( AdminSetEmailBrandingForm, AdminSetLetterBrandingForm, BillingDetailsForm, - BrandingOptions, + ChooseBrandingForm, + ChooseEmailBrandingForm, + ChooseLetterBrandingForm, EditNotesForm, EstimateUsageForm, FreeSMSAllowance, @@ -89,7 +91,7 @@ def service_settings(service_id): return render_template( 'views/service-settings.html', service_permissions=PLATFORM_ADMIN_SERVICE_PERMISSIONS, - email_branding_options=BrandingOptions(current_service, branding_type='email') + email_branding_options=ChooseBrandingForm(current_service, branding_type='email') ) @@ -1135,7 +1137,7 @@ def link_service_to_organisation(service_id): def create_email_branding_zendesk_ticket(form_option_selected, detail=None): - form = BrandingOptions(current_service) + form = ChooseEmailBrandingForm(current_service) ticket_message = render_template( 'support-tickets/branding-request.txt', @@ -1159,7 +1161,7 @@ def create_email_branding_zendesk_ticket(form_option_selected, detail=None): @main.route("/services//service-settings/email-branding", methods=['GET', 'POST']) @user_has_permissions('manage_service') def email_branding_request(service_id): - form = BrandingOptions(current_service, branding_type='email') + form = ChooseEmailBrandingForm(current_service) branding_name = current_service.email_branding_name if form.validate_on_submit(): return redirect( @@ -1176,9 +1178,9 @@ def email_branding_request(service_id): ) -def check_branding_allowed_for_service(branding): +def check_email_branding_allowed_for_service(branding): allowed_branding_for_service = dict( - BrandingOptions.get_available_choices(current_service, branding_type='email') + ChooseEmailBrandingForm.get_available_choices(current_service, branding_type='email') ) if branding not in allowed_branding_for_service: abort(404) @@ -1187,7 +1189,7 @@ def check_branding_allowed_for_service(branding): @main.route("/services//service-settings/email-branding/govuk", methods=['GET', 'POST']) @user_has_permissions('manage_service') def email_branding_govuk(service_id): - check_branding_allowed_for_service('govuk') + check_email_branding_allowed_for_service('govuk') if request.method == 'POST': current_service.update(email_branding=None) @@ -1201,7 +1203,7 @@ def email_branding_govuk(service_id): @main.route("/services//service-settings/email-branding/govuk-and-org", methods=['GET', 'POST']) @user_has_permissions('manage_service') def email_branding_govuk_and_org(service_id): - check_branding_allowed_for_service('govuk_and_org') + check_email_branding_allowed_for_service('govuk_and_org') if request.method == 'POST': create_email_branding_zendesk_ticket('govuk_and_org') @@ -1215,7 +1217,7 @@ def email_branding_govuk_and_org(service_id): @main.route("/services//service-settings/email-branding/nhs", methods=['GET', 'POST']) @user_has_permissions('manage_service') def email_branding_nhs(service_id): - check_branding_allowed_for_service('nhs') + check_email_branding_allowed_for_service('nhs') if request.method == 'POST': current_service.update(email_branding=NHS_BRANDING_ID) @@ -1229,7 +1231,7 @@ def email_branding_nhs(service_id): @main.route("/services//service-settings/email-branding/organisation", methods=['GET', 'POST']) @user_has_permissions('manage_service') def email_branding_organisation(service_id): - check_branding_allowed_for_service('organisation') + check_email_branding_allowed_for_service('organisation') if request.method == 'POST': create_email_branding_zendesk_ticket('organisation') @@ -1243,7 +1245,7 @@ def email_branding_organisation(service_id): @main.route("/services//service-settings/email-branding/something-else", methods=['GET', 'POST']) @user_has_permissions('manage_service') def email_branding_something_else(service_id): - check_branding_allowed_for_service('something_else') + check_email_branding_allowed_for_service('something_else') form = SomethingElseBrandingForm() @@ -1256,14 +1258,14 @@ def email_branding_something_else(service_id): return render_template( 'views/service-settings/branding/email-branding-something-else.html', form=form, - branding_options=BrandingOptions(current_service, branding_type='email') + branding_options=ChooseBrandingForm(current_service, branding_type='email') ) @main.route("/services//service-settings/letter-branding", methods=['GET', 'POST']) @user_has_permissions('manage_service') def letter_branding_request(service_id): - form = BrandingOptions(current_service, branding_type='letter') + form = ChooseLetterBrandingForm(current_service) from_template = request.args.get('from_template') branding_name = current_service.letter_branding_name if form.validate_on_submit(): From 2fc0a105f427a7cfe378257993df510837fca7da Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 3 Mar 2022 12:51:38 +0000 Subject: [PATCH 04/13] Move branding choices logic into utility module This was a lot of code to be in a form and it's going to get even more complicated with email branding pools. Moving it out means we can also simplify the tests that target this code. --- app/main/forms.py | 57 ++++--------------------------------------- app/utils/branding.py | 53 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 52 deletions(-) create mode 100644 app/utils/branding.py diff --git a/app/main/forms.py b/app/main/forms.py index f7c08c1f2..007393b9a 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -67,7 +67,7 @@ from app.main.validators import ( ) from app.models.feedback import PROBLEM_TICKET_TYPE, QUESTION_TICKET_TYPE from app.models.organisation import Organisation -from app.utils import merge_jsonlike +from app.utils import branding, merge_jsonlike from app.utils.user import distinct_email_addresses from app.utils.user_permissions import ( all_ui_permissions, @@ -2198,57 +2198,10 @@ class ChooseBrandingForm(StripWhitespaceForm): @staticmethod def get_available_choices(service, branding_type): - if branding_type == "email": - organisation_branding_id = service.organisation.email_branding_id if service.organisation else None - service_branding_id = service.email_branding_id - service_branding_name = service.email_branding_name - elif branding_type == "letter": - organisation_branding_id = service.organisation.letter_branding_id if service.organisation else None - service_branding_id = service.letter_branding_id - service_branding_name = service.letter_branding_name - - if ( - service.organisation_type == Organisation.TYPE_CENTRAL - and organisation_branding_id is None - and service_branding_id is not None - and branding_type == "email" - ): - yield ('govuk', 'GOV.UK') - - if ( - service.organisation_type == Organisation.TYPE_CENTRAL - and service.organisation - and organisation_branding_id is None - and service_branding_name.lower() != 'GOV.UK and {}'.format(service.organisation.name).lower() - and branding_type == "email" - ): - yield ('govuk_and_org', 'GOV.UK and {}'.format(service.organisation.name)) - - if ( - service.organisation_type in { - Organisation.TYPE_NHS_CENTRAL, - Organisation.TYPE_NHS_LOCAL, - Organisation.TYPE_NHS_GP, - } - and service_branding_name != 'NHS' - ): - yield ('nhs', 'NHS') - - if ( - service.organisation - and service.organisation_type not in { - Organisation.TYPE_NHS_LOCAL, - Organisation.TYPE_NHS_CENTRAL, - Organisation.TYPE_NHS_GP, - } - and ( - service_branding_id is None - or service_branding_id != organisation_branding_id - ) - ): - yield ('organisation', service.organisation.name) - - yield ChooseBrandingForm.FALLBACK_OPTION + return ( + list(branding.get_available_choices(service, branding_type)) + + [ChooseBrandingForm.FALLBACK_OPTION] + ) @property def something_else_is_only_option(self): diff --git a/app/utils/branding.py b/app/utils/branding.py new file mode 100644 index 000000000..dfa9f1585 --- /dev/null +++ b/app/utils/branding.py @@ -0,0 +1,53 @@ +from app.models.organisation import Organisation + + +def get_available_choices(service, branding_type): + if branding_type == "email": + organisation_branding_id = service.organisation.email_branding_id if service.organisation else None + service_branding_id = service.email_branding_id + service_branding_name = service.email_branding_name + elif branding_type == "letter": + organisation_branding_id = service.organisation.letter_branding_id if service.organisation else None + service_branding_id = service.letter_branding_id + service_branding_name = service.letter_branding_name + + if ( + service.organisation_type == Organisation.TYPE_CENTRAL + and organisation_branding_id is None + and service_branding_id is not None + and branding_type == "email" + ): + yield ('govuk', 'GOV.UK') + + if ( + service.organisation_type == Organisation.TYPE_CENTRAL + and service.organisation + and organisation_branding_id is None + and service_branding_name.lower() != 'GOV.UK and {}'.format(service.organisation.name).lower() + and branding_type == "email" + ): + yield ('govuk_and_org', 'GOV.UK and {}'.format(service.organisation.name)) + + if ( + service.organisation_type in { + Organisation.TYPE_NHS_CENTRAL, + Organisation.TYPE_NHS_LOCAL, + Organisation.TYPE_NHS_GP, + } + and service_branding_name != 'NHS' + ): + yield ('nhs', 'NHS') + + if ( + service.organisation + and service.organisation_type not in { + Organisation.TYPE_NHS_LOCAL, + Organisation.TYPE_NHS_CENTRAL, + Organisation.TYPE_NHS_GP, + } + and ( + service_branding_id is None + or service_branding_id != organisation_branding_id + ) + ): + yield ('organisation', service.organisation.name) From a2d471eff4b5a6bd91a9180baf7f67d4acdf4a7a Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 3 Mar 2022 12:53:59 +0000 Subject: [PATCH 05/13] Add missing assertion for letter button text This is the same as the test below, which we will be removing soon. --- tests/app/main/views/test_service_settings.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 6f44f6226..f26fcf125 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -4853,6 +4853,9 @@ def test_letter_branding_request_page_when_no_branding_is_set( assert mock_get_email_branding.called is False assert mock_get_letter_branding_by_id.called is False + button_text = normalize_spaces(page.select_one('.page-footer button').text) + assert button_text == 'Request new branding' + if expected_options: assert [ ( From 1946d3c92809ba43066edb649dd5fe96aba11071 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 3 Mar 2022 12:54:57 +0000 Subject: [PATCH 06/13] Migrate two branding tests to utils module This is much simpler than trying to test the function via the page, although there are still two scenarios to test there: - The page with radio buttons (using NHS as an example). - The page with a text form (using "other" as an example). In future work we could split this test in two to make it clearer what it's trying to test. For now, this keeps the diff simple. --- tests/app/main/views/test_service_settings.py | 28 ------------------- tests/app/utils/test_branding.py | 28 +++++++++++++++++++ 2 files changed, 28 insertions(+), 28 deletions(-) create mode 100644 tests/app/utils/test_branding.py diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index f26fcf125..ac0ca5cf8 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -4753,27 +4753,10 @@ def test_update_service_organisation_does_not_update_if_same_value( @pytest.mark.parametrize('organisation_type, expected_options', ( - ('central', [ - ('something_else', 'Something else'), - ]), - ('local', [ - ('something_else', 'Something else'), - ]), ('nhs_central', [ ('nhs', 'NHS'), ('something_else', 'Something else'), ]), - ('nhs_local', [ - ('nhs', 'NHS'), - ('something_else', 'Something else'), - ]), - ('nhs_gp', [ - ('nhs', 'NHS'), - ('something_else', 'Something else'), - ]), - ('emergency_service', [ - ('something_else', 'Something else'), - ]), ('other', [ ('something_else', 'Something else'), ]) @@ -4818,21 +4801,10 @@ def test_email_branding_request_page_when_no_branding_is_set( @pytest.mark.parametrize('organisation_type, expected_options', ( - ('central', None), - ('local', None), ('nhs_central', [ ('nhs', 'NHS'), ('something_else', 'Something else'), ]), - ('nhs_local', [ - ('nhs', 'NHS'), - ('something_else', 'Something else'), - ]), - ('nhs_gp', [ - ('nhs', 'NHS'), - ('something_else', 'Something else'), - ]), - ('emergency_service', None), ('other', None), )) def test_letter_branding_request_page_when_no_branding_is_set( diff --git a/tests/app/utils/test_branding.py b/tests/app/utils/test_branding.py new file mode 100644 index 000000000..2dfab64f3 --- /dev/null +++ b/tests/app/utils/test_branding.py @@ -0,0 +1,28 @@ +import pytest + +from app.models.service import Service +from app.utils.branding import get_available_choices + + +@pytest.mark.parametrize('branding_type', ['email', 'letter']) +@pytest.mark.parametrize('org_type, existing_branding, expected_options', [ + ('central', None, []), + ('local', None, []), + ('nhs_central', None, [('nhs', 'NHS')]), + ('nhs_local', None, [('nhs', 'NHS')]), + ('nhs_gp', None, [('nhs', 'NHS')]), + ('emergency_service', None, []), + ('other', None, []), +]) +def test_get_available_choices_no_org( + service_one, + branding_type, + org_type, + existing_branding, + expected_options, +): + service_one['organisation_type'] = org_type + service = Service(service_one) + + options = get_available_choices(service, branding_type=branding_type) + assert list(options) == expected_options From 5a39e310aae0d3d869e17b0cdfd8300cbfc33540 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 3 Mar 2022 12:58:44 +0000 Subject: [PATCH 07/13] Migrate two more tests to the branding utility All of the mock / UI assertions in these tests are covered by the tests above them - these tests were mostly targetting which options were being shown, which we can check at a lower level. --- tests/app/main/views/test_service_settings.py | 111 ------------------ tests/app/utils/test_branding.py | 57 +++++++++ 2 files changed, 57 insertions(+), 111 deletions(-) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index ac0ca5cf8..bba4aa4bc 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -4850,117 +4850,6 @@ def test_letter_branding_request_page_when_no_branding_is_set( assert not page.select('.conditional-radios-panel') -@pytest.mark.parametrize('branding_type', ['email', 'letter']) -@pytest.mark.parametrize('organisation_type, expected_options', ( - ('local', [ - ('organisation', 'Test Organisation'), - ('something_else', 'Something else'), - ]), - ('nhs_central', [ - ('nhs', 'NHS'), - ('something_else', 'Something else'), - ]), - ('nhs_local', [ - ('nhs', 'NHS'), - ('something_else', 'Something else'), - ]), - ('nhs_gp', [ - ('nhs', 'NHS'), - ('something_else', 'Something else'), - ]), - ('emergency_service', [ - ('organisation', 'Test Organisation'), - ('something_else', 'Something else'), - ]), - ('other', [ - ('organisation', 'Test Organisation'), - ('something_else', 'Something else'), - ]), -)) -def test_branding_request_page_when_no_branding_is_set_but_organisation_exists( - mocker, - service_one, - client_request, - mock_get_email_branding, - mock_get_letter_branding_by_id, - mock_get_service_organisation, - organisation_type, - expected_options, - branding_type -): - service_one['{}_branding'.format(branding_type)] = None - mocker.patch( - 'app.organisations_client.get_organisation', - return_value=organisation_json(organisation_type=organisation_type), - ) - - page = client_request.get( - f'.{branding_type}_branding_request', service_id=SERVICE_ONE_ID - ) - - assert mock_get_email_branding.called is False - assert mock_get_letter_branding_by_id.called is False - - assert [ - ( - radio['value'], - page.select_one('label[for={}]'.format(radio['id'])).text.strip() - ) - for radio in page.select('input[type=radio]') - ] == expected_options - - button_text = normalize_spaces(page.select_one('.page-footer button').text) - - if branding_type == 'email': - assert button_text == 'Continue' - else: - assert button_text == 'Request new branding' - - -@pytest.mark.parametrize('organisation_type, expected_options, branding_type', ( - ('central', [ - ('govuk_and_org', 'GOV.UK and Test Organisation'), - ('organisation', 'Test Organisation'), - ('something_else', 'Something else'), - ], 'email'), - ('central', [ - ('organisation', 'Test Organisation'), - ('something_else', 'Something else'), - ], 'letter'), -)) -def test_branding_request_page_when_no_branding_is_set_but_organisation_exists_central_org( - mocker, - service_one, - client_request, - mock_get_email_branding, - mock_get_letter_branding_by_id, - mock_get_service_organisation, - organisation_type, - expected_options, - branding_type -): - service_one['{}_branding'.format(branding_type)] = None - mocker.patch( - 'app.organisations_client.get_organisation', - return_value=organisation_json(organisation_type=organisation_type), - ) - - page = client_request.get( - f'.{branding_type}_branding_request', service_id=SERVICE_ONE_ID - ) - - assert mock_get_email_branding.called is False - assert mock_get_letter_branding_by_id.called is False - - assert [ - ( - radio['value'], - page.select_one('label[for={}]'.format(radio['id'])).text.strip() - ) - for radio in page.select('input[type=radio]') - ] == expected_options - - def test_email_branding_request_page_when_email_branding_is_set( mocker, service_one, diff --git a/tests/app/utils/test_branding.py b/tests/app/utils/test_branding.py index 2dfab64f3..7a8a6f168 100644 --- a/tests/app/utils/test_branding.py +++ b/tests/app/utils/test_branding.py @@ -2,6 +2,7 @@ import pytest from app.models.service import Service from app.utils.branding import get_available_choices +from tests import organisation_json @pytest.mark.parametrize('branding_type', ['email', 'letter']) @@ -26,3 +27,59 @@ def test_get_available_choices_no_org( options = get_available_choices(service, branding_type=branding_type) assert list(options) == expected_options + + +@pytest.mark.parametrize('branding_type', ['email', 'letter']) +@pytest.mark.parametrize('org_type, existing_branding, expected_options', [ + ('local', None, [('organisation', 'Test Organisation')]), + ('nhs_central', None, [('nhs', 'NHS')]), + ('nhs_local', None, [('nhs', 'NHS')]), + ('nhs_gp', None, [('nhs', 'NHS')]), + ('emergency_service', None, [('organisation', 'Test Organisation')]), + ('other', None, [('organisation', 'Test Organisation')]), +]) +def test_get_available_choices_with_org( + mocker, + service_one, + branding_type, + org_type, + existing_branding, + expected_options, + mock_get_service_organisation, +): + service = Service(service_one) + + mocker.patch( + 'app.organisations_client.get_organisation', + return_value=organisation_json(organisation_type=org_type) + ) + + options = get_available_choices(service, branding_type=branding_type) + assert list(options) == expected_options + + +@pytest.mark.parametrize('branding_type, expected_options', [ + ('email', [ + ('govuk_and_org', 'GOV.UK and Test Organisation'), + ('organisation', 'Test Organisation'), + ]), + ('letter', [ + ('organisation', 'Test Organisation'), + ]) +]) +def test_get_available_choices_with_central_org( + mocker, + service_one, + branding_type, + expected_options, + mock_get_service_organisation, +): + service = Service(service_one) + + mocker.patch( + 'app.organisations_client.get_organisation', + return_value=organisation_json(organisation_type='central'), + ) + + options = get_available_choices(service, branding_type=branding_type) + assert list(options) == expected_options From 857b8b04b07068771d9435b57213cfbabde9e59a Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 3 Mar 2022 13:10:04 +0000 Subject: [PATCH 08/13] Duplicate tests for options with branding set This is a step towards further refactoring of the page tests, which don't need to check in this level of detail anymore. --- tests/app/utils/test_branding.py | 47 ++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/tests/app/utils/test_branding.py b/tests/app/utils/test_branding.py index 7a8a6f168..da048e04a 100644 --- a/tests/app/utils/test_branding.py +++ b/tests/app/utils/test_branding.py @@ -1,3 +1,5 @@ +from unittest.mock import PropertyMock + import pytest from app.models.service import Service @@ -83,3 +85,48 @@ def test_get_available_choices_with_central_org( options = get_available_choices(service, branding_type=branding_type) assert list(options) == expected_options + + +def test_get_available_choices_email_branding_set( + mocker, + service_one, + mock_get_service_organisation, + mock_get_email_branding, +): + service = Service(service_one) + + mocker.patch( + 'app.organisations_client.get_organisation', + return_value=organisation_json() + ) + mocker.patch( + 'app.models.service.Service.email_branding_id', + new_callable=PropertyMock, + return_value='1234-abcd', + ) + + options = get_available_choices(service, branding_type='email') + assert list(options) == [ + ('govuk', 'GOV.UK'), + ('govuk_and_org', 'GOV.UK and Test Organisation'), + ('organisation', 'Test Organisation'), + ] + + +def test_get_available_choices_letter_branding_set( + mocker, + service_one, + mock_get_service_organisation, + mock_get_letter_branding_by_id, +): + service = Service(service_one) + + mocker.patch( + 'app.organisations_client.get_organisation', + return_value=organisation_json() + ) + + options = get_available_choices(service, branding_type='letter') + assert list(options) == [ + ('organisation', 'Test Organisation'), + ] From fa3e6435a6fb001bc495b28814fba41f8257c192 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 15 Mar 2022 11:37:45 +0000 Subject: [PATCH 09/13] Fix small issues identified in PR review In response to: [^1], [^2], [^3], [^4], [^5] and [^6]. [^1]: https://github.com/alphagov/notifications-admin/pull/4182#discussion_r825824485 [^2]: https://github.com/alphagov/notifications-admin/pull/4182#discussion_r825824805 [^3]: https://github.com/alphagov/notifications-admin/pull/4182#discussion_r825857745 [^4]: https://github.com/alphagov/notifications-admin/pull/4182#discussion_r825859850 [^5]: https://github.com/alphagov/notifications-admin/pull/4182#discussion_r825859982 [^6]: https://github.com/alphagov/notifications-admin/pull/4182#discussion_r826001823 --- app/main/forms.py | 10 ++++---- tests/app/utils/test_branding.py | 41 +++++++++++++++++--------------- 2 files changed, 27 insertions(+), 24 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 007393b9a..1a904a420 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -2196,11 +2196,11 @@ class ChooseBrandingForm(StripWhitespaceForm): if self.something_else_is_only_option: self.options.data = self.FALLBACK_OPTION_VALUE - @staticmethod - def get_available_choices(service, branding_type): + @classmethod + def get_available_choices(cls, service, branding_type): return ( list(branding.get_available_choices(service, branding_type)) + - [ChooseBrandingForm.FALLBACK_OPTION] + [cls.FALLBACK_OPTION] ) @property @@ -2221,12 +2221,12 @@ class ChooseBrandingForm(StripWhitespaceForm): class ChooseEmailBrandingForm(ChooseBrandingForm): def __init__(self, service_id): - ChooseBrandingForm.__init__(self, service_id, branding_type='email') + super().__init__(service_id, branding_type='email') class ChooseLetterBrandingForm(ChooseBrandingForm): def __init__(self, service_id): - ChooseBrandingForm.__init__(self, service_id, branding_type='letter') + super().__init__(service_id, branding_type='letter') class SomethingElseBrandingForm(StripWhitespaceForm): diff --git a/tests/app/utils/test_branding.py b/tests/app/utils/test_branding.py index da048e04a..b42a9da76 100644 --- a/tests/app/utils/test_branding.py +++ b/tests/app/utils/test_branding.py @@ -8,20 +8,19 @@ from tests import organisation_json @pytest.mark.parametrize('branding_type', ['email', 'letter']) -@pytest.mark.parametrize('org_type, existing_branding, expected_options', [ - ('central', None, []), - ('local', None, []), - ('nhs_central', None, [('nhs', 'NHS')]), - ('nhs_local', None, [('nhs', 'NHS')]), - ('nhs_gp', None, [('nhs', 'NHS')]), - ('emergency_service', None, []), - ('other', None, []), +@pytest.mark.parametrize('org_type, expected_options', [ + ('central', []), + ('local', []), + ('nhs_central', [('nhs', 'NHS')]), + ('nhs_local', [('nhs', 'NHS')]), + ('nhs_gp', [('nhs', 'NHS')]), + ('emergency_service', []), + ('other', []), ]) -def test_get_available_choices_no_org( +def test_get_available_choices_service_not_assigned_to_org( service_one, branding_type, org_type, - existing_branding, expected_options, ): service_one['organisation_type'] = org_type @@ -32,20 +31,19 @@ def test_get_available_choices_no_org( @pytest.mark.parametrize('branding_type', ['email', 'letter']) -@pytest.mark.parametrize('org_type, existing_branding, expected_options', [ - ('local', None, [('organisation', 'Test Organisation')]), - ('nhs_central', None, [('nhs', 'NHS')]), - ('nhs_local', None, [('nhs', 'NHS')]), - ('nhs_gp', None, [('nhs', 'NHS')]), - ('emergency_service', None, [('organisation', 'Test Organisation')]), - ('other', None, [('organisation', 'Test Organisation')]), +@pytest.mark.parametrize('org_type, expected_options', [ + ('local', [('organisation', 'Test Organisation')]), + ('nhs_central', [('nhs', 'NHS')]), + ('nhs_local', [('nhs', 'NHS')]), + ('nhs_gp', [('nhs', 'NHS')]), + ('emergency_service', [('organisation', 'Test Organisation')]), + ('other', [('organisation', 'Test Organisation')]), ]) -def test_get_available_choices_with_org( +def test_get_available_choices_service_assigned_to_org( mocker, service_one, branding_type, org_type, - existing_branding, expected_options, mock_get_service_organisation, ): @@ -125,6 +123,11 @@ def test_get_available_choices_letter_branding_set( 'app.organisations_client.get_organisation', return_value=organisation_json() ) + mocker.patch( + 'app.models.service.Service.letter_branding_id', + new_callable=PropertyMock, + return_value='1234-abcd', + ) options = get_available_choices(service, branding_type='letter') assert list(options) == [ From f02c2b0b1dc8827eb2451136a7f1c7b2d93ccb9a Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 15 Mar 2022 10:50:18 +0000 Subject: [PATCH 10/13] Rename other "Admin" forms consistently I've also tweaked some of the names to make them clearer e.g. that the form is used to apply a change to a service. I've constrained the scope of this change to avoid forms that may be accessible by non-admins in the future. --- app/main/forms.py | 34 ++++++++++++++-------------- app/main/views/find_users.py | 4 ++-- app/main/views/organisations.py | 20 ++++++++--------- app/main/views/platform_admin.py | 8 +++---- app/main/views/providers.py | 6 ++--- app/main/views/service_settings.py | 36 +++++++++++++++--------------- 6 files changed, 54 insertions(+), 54 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 1a904a420..c72a20eb4 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1264,7 +1264,7 @@ class OrganisationAgreementSignedForm(StripWhitespaceForm): ) -class OrganisationDomainsForm(StripWhitespaceForm): +class AdminOrganisationDomainsForm(StripWhitespaceForm): def populate(self, domains_list): for index, value in enumerate(domains_list): @@ -1302,7 +1302,7 @@ class CreateNhsServiceForm(CreateServiceForm): ) -class NewOrganisationForm( +class AdminNewOrganisationForm( RenameOrganisationForm, OrganisationOrganisationTypeForm, OrganisationCrownStatusForm, @@ -1313,7 +1313,7 @@ class NewOrganisationForm( self.crown_status.choices = self.crown_status.choices[:-1] -class FreeSMSAllowance(StripWhitespaceForm): +class AdminServiceSMSAllowanceForm(StripWhitespaceForm): free_sms_allowance = GovukIntegerField( 'Numbers of text message fragments per year', validators=[ @@ -1322,7 +1322,7 @@ class FreeSMSAllowance(StripWhitespaceForm): ) -class MessageLimit(StripWhitespaceForm): +class AdminServiceMessageLimitForm(StripWhitespaceForm): message_limit = GovukIntegerField( 'Number of messages the service is allowed to send each day', validators=[ @@ -1331,7 +1331,7 @@ class MessageLimit(StripWhitespaceForm): ) -class RateLimit(StripWhitespaceForm): +class AdminServiceRateLimitForm(StripWhitespaceForm): rate_limit = GovukIntegerField( 'Number of messages the service can send in a rolling 60 second window', validators=[ @@ -1734,13 +1734,13 @@ class EstimateUsageForm(StripWhitespaceForm): return super().validate(*args, **kwargs) -class ProviderForm(StripWhitespaceForm): +class AdminProviderForm(StripWhitespaceForm): priority = GovukIntegerField( 'Priority', [validators.NumberRange(min=1, max=100, message="Must be between 1 and 100")] ) -class ProviderRatioForm(StripWhitespaceForm): +class AdminProviderRatioForm(StripWhitespaceForm): ratio = GovukRadiosField(choices=[ (str(value), '{}% / {}%'.format(value, 100 - value)) @@ -1823,11 +1823,11 @@ class ServiceEditInboundNumberForm(StripWhitespaceForm): is_default = GovukCheckboxField("Make this text message sender the default") -class EditNotesForm(StripWhitespaceForm): +class AdminNotesForm(StripWhitespaceForm): notes = TextAreaField(validators=[]) -class BillingDetailsForm(StripWhitespaceForm): +class AdminBillingDetailsForm(StripWhitespaceForm): billing_contact_email_addresses = GovukTextInputField('Contact email addresses') billing_contact_names = GovukTextInputField('Contact names') billing_reference = GovukTextInputField('Reference') @@ -2035,7 +2035,7 @@ class SearchByNameForm(StripWhitespaceForm): ) -class SearchUsersByEmailForm(StripWhitespaceForm): +class AdminSearchUsersByEmailForm(StripWhitespaceForm): search = GovukSearchField( 'Search by name or email address', @@ -2083,7 +2083,7 @@ class PlaceholderForm(StripWhitespaceForm): pass -class ServiceInboundNumberForm(StripWhitespaceForm): +class AdminServiceInboundNumberForm(StripWhitespaceForm): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self.inbound_number.choices = kwargs['inbound_number_choices'] @@ -2166,7 +2166,7 @@ class SetTemplateSenderForm(StripWhitespaceForm): sender = GovukRadiosField() -class LinkOrganisationsForm(StripWhitespaceForm): +class AdminSetOrganisationForm(StripWhitespaceForm): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) @@ -2245,7 +2245,7 @@ class SomethingElseBrandingForm(StripWhitespaceForm): ) -class ServiceDataRetentionForm(StripWhitespaceForm): +class AdminServiceAddDataRetentionForm(StripWhitespaceForm): notification_type = GovukRadiosField( 'What notification type?', @@ -2262,14 +2262,14 @@ class ServiceDataRetentionForm(StripWhitespaceForm): ) -class ServiceDataRetentionEditForm(StripWhitespaceForm): +class AdminServiceEditDataRetentionForm(StripWhitespaceForm): days_of_retention = GovukIntegerField( label="Days of retention", validators=[validators.NumberRange(min=3, max=90, message="Must be between 3 and 90")], ) -class ReturnedLettersForm(StripWhitespaceForm): +class AdminReturnedLettersForm(StripWhitespaceForm): references = TextAreaField( u'Letter references', validators=[ @@ -2421,7 +2421,7 @@ class TemplateAndFoldersSelectionForm(Form): ], required_message='Select the type of template you want to add') -class ClearCacheForm(StripWhitespaceForm): +class AdminClearCacheForm(StripWhitespaceForm): model_type = GovukCheckboxesField( 'What do you want to clear today', ) @@ -2431,7 +2431,7 @@ class ClearCacheForm(StripWhitespaceForm): raise ValidationError('Select at least one option') -class GoLiveNotesForm(StripWhitespaceForm): +class AdminOrganisationGoLiveNotesForm(StripWhitespaceForm): request_to_go_live_notes = TextAreaField( 'Go live notes', filters=[lambda x: x or None], diff --git a/app/main/views/find_users.py b/app/main/views/find_users.py index d3857ee45..d01d2c6be 100644 --- a/app/main/views/find_users.py +++ b/app/main/views/find_users.py @@ -5,7 +5,7 @@ from notifications_python_client.errors import HTTPError from app import user_api_client from app.event_handlers import create_archive_user_event from app.main import main -from app.main.forms import AuthTypeForm, SearchUsersByEmailForm +from app.main.forms import AdminSearchUsersByEmailForm, AuthTypeForm from app.models.user import User from app.utils.user import user_is_platform_admin @@ -13,7 +13,7 @@ from app.utils.user import user_is_platform_admin @main.route("/find-users-by-email", methods=['GET', 'POST']) @user_is_platform_admin def find_users_by_email(): - form = SearchUsersByEmailForm() + form = AdminSearchUsersByEmailForm() users_found = None if form.validate_on_submit(): users_found = user_api_client.find_users_by_full_or_partial_email(form.search.data)['data'] diff --git a/app/main/views/organisations.py b/app/main/views/organisations.py index 8808800a8..13b8a21ae 100644 --- a/app/main/views/organisations.py +++ b/app/main/views/organisations.py @@ -19,17 +19,17 @@ from app.main import main from app.main.forms import ( AddGPOrganisationForm, AddNHSLocalOrganisationForm, + AdminBillingDetailsForm, + AdminNewOrganisationForm, + AdminNotesForm, + AdminOrganisationDomainsForm, + AdminOrganisationGoLiveNotesForm, AdminPreviewBrandingForm, AdminSetEmailBrandingForm, AdminSetLetterBrandingForm, - BillingDetailsForm, - EditNotesForm, - GoLiveNotesForm, InviteOrgUserForm, - NewOrganisationForm, OrganisationAgreementSignedForm, OrganisationCrownStatusForm, - OrganisationDomainsForm, OrganisationOrganisationTypeForm, RenameOrganisationForm, SearchByNameForm, @@ -60,7 +60,7 @@ def organisations(): @main.route("/organisations/add", methods=['GET', 'POST']) @user_is_platform_admin def add_organisation(): - form = NewOrganisationForm() + form = AdminNewOrganisationForm() if form.validate_on_submit(): try: @@ -495,7 +495,7 @@ def organisation_preview_letter_branding(org_id): @user_is_platform_admin def edit_organisation_domains(org_id): - form = OrganisationDomainsForm() + form = AdminOrganisationDomainsForm() if form.validate_on_submit(): try: @@ -530,7 +530,7 @@ def edit_organisation_domains(org_id): @user_is_platform_admin def edit_organisation_go_live_notes(org_id): - form = GoLiveNotesForm() + form = AdminOrganisationGoLiveNotesForm() if form.validate_on_submit(): organisations_client.update_organisation( @@ -551,7 +551,7 @@ def edit_organisation_go_live_notes(org_id): @main.route("/organisations//settings/notes", methods=['GET', 'POST']) @user_is_platform_admin def edit_organisation_notes(org_id): - form = EditNotesForm(notes=current_organisation.notes) + form = AdminNotesForm(notes=current_organisation.notes) if form.validate_on_submit(): @@ -572,7 +572,7 @@ def edit_organisation_notes(org_id): @main.route("/organisations//settings/edit-billing-details", methods=['GET', 'POST']) @user_is_platform_admin def edit_organisation_billing_details(org_id): - form = BillingDetailsForm( + form = AdminBillingDetailsForm( billing_contact_email_addresses=current_organisation.billing_contact_email_addresses, billing_contact_names=current_organisation.billing_contact_names, billing_reference=current_organisation.billing_reference, diff --git a/app/main/views/platform_admin.py b/app/main/views/platform_admin.py index 23018ee94..fddba4a62 100644 --- a/app/main/views/platform_admin.py +++ b/app/main/views/platform_admin.py @@ -18,11 +18,11 @@ from app import ( from app.extensions import redis_client from app.main import main from app.main.forms import ( + AdminClearCacheForm, + AdminReturnedLettersForm, BillingReportDateFilterForm, - ClearCacheForm, DateFilterForm, RequiredDateFilterForm, - ReturnedLettersForm, ) from app.statistics_utils import ( get_formatted_percentage, @@ -341,7 +341,7 @@ def platform_admin_list_complaints(): @main.route("/platform-admin/returned-letters", methods=["GET", "POST"]) @user_is_platform_admin def platform_admin_returned_letters(): - form = ReturnedLettersForm() + form = AdminReturnedLettersForm() if form.validate_on_submit(): references = [ @@ -419,7 +419,7 @@ def clear_cache(): ]), ]) - form = ClearCacheForm() + form = AdminClearCacheForm() form.model_type.choices = [ (key, key.replace('_', ' ').title()) for key in CACHE_KEYS diff --git a/app/main/views/providers.py b/app/main/views/providers.py index 977936cc9..0868f1a63 100644 --- a/app/main/views/providers.py +++ b/app/main/views/providers.py @@ -7,7 +7,7 @@ from werkzeug.utils import redirect from app import format_date_numeric, provider_client from app.main import main -from app.main.forms import ProviderForm, ProviderRatioForm +from app.main.forms import AdminProviderForm, AdminProviderRatioForm from app.utils.user import user_is_platform_admin PROVIDER_PRIORITY_MEANING_SWITCHOVER = datetime(2019, 11, 29, 11, 0).isoformat() @@ -48,7 +48,7 @@ def add_monthly_traffic(domestic_sms_providers): @user_is_platform_admin def edit_provider(provider_id): provider = provider_client.get_provider_by_id(provider_id)['provider_details'] - form = ProviderForm(active=provider['active'], priority=provider['priority']) + form = AdminProviderForm(active=provider['active'], priority=provider['priority']) if form.validate_on_submit(): provider_client.update_provider(provider_id, form.priority.data) @@ -67,7 +67,7 @@ def edit_sms_provider_ratio(): if provider['notification_type'] == 'sms' ], key=itemgetter('identifier'), reverse=True) - form = ProviderRatioForm(ratio=providers[0]['priority']) + form = AdminProviderRatioForm(ratio=providers[0]['priority']) if len(providers) < 2: abort(400) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index eb2ca5dee..ecf0eb537 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -38,29 +38,29 @@ from app.extensions import zendesk_client from app.formatters import email_safe from app.main import main from app.main.forms import ( + AdminBillingDetailsForm, + AdminNotesForm, AdminPreviewBrandingForm, + AdminServiceAddDataRetentionForm, + AdminServiceEditDataRetentionForm, + AdminServiceInboundNumberForm, + AdminServiceMessageLimitForm, + AdminServiceRateLimitForm, + AdminServiceSMSAllowanceForm, AdminSetEmailBrandingForm, AdminSetLetterBrandingForm, - BillingDetailsForm, + AdminSetOrganisationForm, ChooseBrandingForm, ChooseEmailBrandingForm, ChooseLetterBrandingForm, - EditNotesForm, EstimateUsageForm, - FreeSMSAllowance, - LinkOrganisationsForm, - MessageLimit, - RateLimit, RenameServiceForm, SearchByNameForm, ServiceBroadcastAccountTypeForm, ServiceBroadcastChannelForm, ServiceBroadcastNetworkForm, ServiceContactDetailsForm, - ServiceDataRetentionEditForm, - ServiceDataRetentionForm, ServiceEditInboundNumberForm, - ServiceInboundNumberForm, ServiceLetterContactBlockForm, ServiceOnOffSettingForm, ServiceReplyToEmailForm, @@ -650,7 +650,7 @@ def service_set_inbound_number(service_id): (number['id'], number['number']) for number in available_inbound_numbers['data'] ] no_available_numbers = available_inbound_numbers['data'] == [] - form = ServiceInboundNumberForm( + form = AdminServiceInboundNumberForm( inbound_number_choices=inbound_numbers_value_and_label ) @@ -972,7 +972,7 @@ def service_delete_sms_sender(service_id, sms_sender_id): @user_is_platform_admin def set_free_sms_allowance(service_id): - form = FreeSMSAllowance(free_sms_allowance=current_service.free_sms_fragment_limit) + form = AdminServiceSMSAllowanceForm(free_sms_allowance=current_service.free_sms_fragment_limit) if form.validate_on_submit(): billing_api_client.create_or_update_free_sms_fragment_limit(service_id, form.free_sms_allowance.data) @@ -989,7 +989,7 @@ def set_free_sms_allowance(service_id): @user_is_platform_admin def set_message_limit(service_id): - form = MessageLimit(message_limit=current_service.message_limit) + form = AdminServiceMessageLimitForm(message_limit=current_service.message_limit) if form.validate_on_submit(): current_service.update(message_limit=form.message_limit.data) @@ -1006,7 +1006,7 @@ def set_message_limit(service_id): @user_is_platform_admin def set_rate_limit(service_id): - form = RateLimit(rate_limit=current_service.rate_limit) + form = AdminServiceRateLimitForm(rate_limit=current_service.rate_limit) if form.validate_on_submit(): current_service.update(rate_limit=form.rate_limit.data) @@ -1115,7 +1115,7 @@ def link_service_to_organisation(service_id): all_organisations = organisations_client.get_organisations() - form = LinkOrganisationsForm( + form = AdminSetOrganisationForm( choices=convert_dictionary_to_wtforms_choices_format(all_organisations, 'id', 'name'), organisations=current_service.organisation_id ) @@ -1313,7 +1313,7 @@ def data_retention(service_id): @main.route("/services//data-retention/add", methods=['GET', 'POST']) @user_is_platform_admin def add_data_retention(service_id): - form = ServiceDataRetentionForm() + form = AdminServiceAddDataRetentionForm() if form.validate_on_submit(): service_api_client.create_service_data_retention(service_id, form.notification_type.data, @@ -1329,7 +1329,7 @@ def add_data_retention(service_id): @user_is_platform_admin def edit_data_retention(service_id, data_retention_id): data_retention_item = current_service.get_data_retention_item(data_retention_id) - form = ServiceDataRetentionEditForm(days_of_retention=data_retention_item['days_of_retention']) + form = AdminServiceEditDataRetentionForm(days_of_retention=data_retention_item['days_of_retention']) if form.validate_on_submit(): service_api_client.update_service_data_retention(service_id, data_retention_id, form.days_of_retention.data) return redirect(url_for('.data_retention', service_id=service_id)) @@ -1344,7 +1344,7 @@ def edit_data_retention(service_id, data_retention_id): @main.route("/services//notes", methods=['GET', 'POST']) @user_is_platform_admin def edit_service_notes(service_id): - form = EditNotesForm(notes=current_service.notes) + form = AdminNotesForm(notes=current_service.notes) if form.validate_on_submit(): @@ -1365,7 +1365,7 @@ def edit_service_notes(service_id): @main.route("/services//edit-billing-details", methods=['GET', 'POST']) @user_is_platform_admin def edit_service_billing_details(service_id): - form = BillingDetailsForm( + form = AdminBillingDetailsForm( billing_contact_email_addresses=current_service.billing_contact_email_addresses, billing_contact_names=current_service.billing_contact_names, billing_reference=current_service.billing_reference, From acd69b8d7de00afba152ab69954fc240879dec87 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 15 Mar 2022 12:10:13 +0000 Subject: [PATCH 11/13] Combine tests for central org email branding Central orgs have more options than others, including the option to revert back to GOV.UK once branding is set. Combining the tests together should make that a bit clearer. --- tests/app/utils/test_branding.py | 38 +++++++++----------------------- 1 file changed, 10 insertions(+), 28 deletions(-) diff --git a/tests/app/utils/test_branding.py b/tests/app/utils/test_branding.py index b42a9da76..a31779e74 100644 --- a/tests/app/utils/test_branding.py +++ b/tests/app/utils/test_branding.py @@ -58,57 +58,39 @@ def test_get_available_choices_service_assigned_to_org( assert list(options) == expected_options -@pytest.mark.parametrize('branding_type, expected_options', [ - ('email', [ +@pytest.mark.parametrize('service_branding, expected_options', [ + (None, [ ('govuk_and_org', 'GOV.UK and Test Organisation'), ('organisation', 'Test Organisation'), ]), - ('letter', [ + ('1234-abcd', [ + ('govuk', 'GOV.UK'), + ('govuk_and_org', 'GOV.UK and Test Organisation'), ('organisation', 'Test Organisation'), ]) ]) -def test_get_available_choices_with_central_org( +def test_get_available_choices_email_branding_central_org( mocker, service_one, - branding_type, + service_branding, expected_options, mock_get_service_organisation, -): - service = Service(service_one) - - mocker.patch( - 'app.organisations_client.get_organisation', - return_value=organisation_json(organisation_type='central'), - ) - - options = get_available_choices(service, branding_type=branding_type) - assert list(options) == expected_options - - -def test_get_available_choices_email_branding_set( - mocker, - service_one, - mock_get_service_organisation, mock_get_email_branding, ): service = Service(service_one) mocker.patch( 'app.organisations_client.get_organisation', - return_value=organisation_json() + return_value=organisation_json(organisation_type='central'), ) mocker.patch( 'app.models.service.Service.email_branding_id', new_callable=PropertyMock, - return_value='1234-abcd', + return_value=service_branding, ) options = get_available_choices(service, branding_type='email') - assert list(options) == [ - ('govuk', 'GOV.UK'), - ('govuk_and_org', 'GOV.UK and Test Organisation'), - ('organisation', 'Test Organisation'), - ] + assert list(options) == expected_options def test_get_available_choices_letter_branding_set( From e7aeb72fd97c9c6da6c9ad8d9a51e0c1cb1e95de Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 15 Mar 2022 12:18:27 +0000 Subject: [PATCH 12/13] Finish extracting UI tests for set branding In response to: [^1]. The behaviour of the letter branding page is unchanged with set branding, so we don't need an extra (and previously, broken [^2]) test for it. The email branding page shows a preview of existing branding if it's set, so it's worth keeping that part of the test. [^1]: https://github.com/alphagov/notifications-admin/pull/4182#discussion_r825998534 [^2]: https://github.com/alphagov/notifications-admin/pull/4182#discussion_r825999860 --- tests/app/main/views/test_service_settings.py | 48 +------------------ 1 file changed, 1 insertion(+), 47 deletions(-) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index bba4aa4bc..5141362eb 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -4850,18 +4850,13 @@ def test_letter_branding_request_page_when_no_branding_is_set( assert not page.select('.conditional-radios-panel') -def test_email_branding_request_page_when_email_branding_is_set( +def test_email_branding_request_page_shows_branding_if_set( mocker, service_one, client_request, mock_get_email_branding, mock_get_service_organisation, ): - service_one['email_branding'] = sample_uuid() - mocker.patch( - 'app.organisations_client.get_organisation', - return_value=organisation_json(), - ) mocker.patch( 'app.models.service.Service.email_branding_id', new_callable=PropertyMock, @@ -4872,47 +4867,6 @@ def test_email_branding_request_page_when_email_branding_is_set( '.email_branding_request', service_id=SERVICE_ONE_ID ) assert page.find('iframe')['src'] == url_for('main.email_template', branding_style='1234-abcd') - assert [ - ( - radio['value'], - page.select_one('label[for={}]'.format(radio['id'])).text.strip() - ) - for radio in page.select('input[type=radio]') - ] == [ - ('govuk', 'GOV.UK'), - ('govuk_and_org', 'GOV.UK and Test Organisation'), - ('organisation', 'Test Organisation'), - ('something_else', 'Something else'), - ] - - -def test_letter_branding_request_page_when_letter_branding_is_set( - mocker, - service_one, - client_request, - mock_get_letter_branding_by_id, - mock_get_service_organisation, - active_user_with_permissions, -): - service_one['letter_branding'] = sample_uuid() - mocker.patch( - 'app.organisations_client.get_organisation', - return_value=organisation_json(), - ) - - page = client_request.get( - '.letter_branding_request', service_id=SERVICE_ONE_ID - ) - assert [ - ( - radio['value'], - page.select_one('label[for={}]'.format(radio['id'])).text.strip() - ) - for radio in page.select('input[type=radio]') - ] == [ - ('organisation', 'Test Organisation'), - ('something_else', 'Something else'), - ] def test_email_branding_request_page_back_link( From 6e81d055049f907dd4212a184c4f0ad58e44d086 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 22 Mar 2022 13:02:39 +0000 Subject: [PATCH 13/13] Rename test values to clarify they're branding IDs In response to: [^1] [^1]: https://github.com/alphagov/notifications-admin/pull/4182#discussion_r832009706 --- tests/app/main/views/test_service_settings.py | 4 ++-- tests/app/utils/test_branding.py | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 5141362eb..2f52ba349 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -4860,13 +4860,13 @@ def test_email_branding_request_page_shows_branding_if_set( mocker.patch( 'app.models.service.Service.email_branding_id', new_callable=PropertyMock, - return_value='1234-abcd', + return_value='some-random-branding', ) page = client_request.get( '.email_branding_request', service_id=SERVICE_ONE_ID ) - assert page.find('iframe')['src'] == url_for('main.email_template', branding_style='1234-abcd') + assert page.find('iframe')['src'] == url_for('main.email_template', branding_style='some-random-branding') def test_email_branding_request_page_back_link( diff --git a/tests/app/utils/test_branding.py b/tests/app/utils/test_branding.py index a31779e74..6c528a669 100644 --- a/tests/app/utils/test_branding.py +++ b/tests/app/utils/test_branding.py @@ -63,8 +63,8 @@ def test_get_available_choices_service_assigned_to_org( ('govuk_and_org', 'GOV.UK and Test Organisation'), ('organisation', 'Test Organisation'), ]), - ('1234-abcd', [ - ('govuk', 'GOV.UK'), + ('some-random-branding', [ + ('govuk', 'GOV.UK'), # central orgs can switch back to GOV.UK ('govuk_and_org', 'GOV.UK and Test Organisation'), ('organisation', 'Test Organisation'), ]) @@ -108,7 +108,7 @@ def test_get_available_choices_letter_branding_set( mocker.patch( 'app.models.service.Service.letter_branding_id', new_callable=PropertyMock, - return_value='1234-abcd', + return_value='some-random-branding', ) options = get_available_choices(service, branding_type='letter')