diff --git a/app/main/forms.py b/app/main/forms.py index b0cc3047c..350b34798 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -2190,13 +2190,6 @@ class ChooseBrandingForm(StripWhitespaceForm): FALLBACK_OPTION_VALUE = 'something_else' FALLBACK_OPTION = (FALLBACK_OPTION_VALUE, 'Something else') - @classmethod - def get_available_choices(cls, service, branding_type): - return ( - list(branding.get_available_choices(service, branding_type)) + - [cls.FALLBACK_OPTION] - ) - @property def something_else_is_only_option(self): return self.options.choices == (self.FALLBACK_OPTION,) @@ -2207,7 +2200,14 @@ class ChooseEmailBrandingForm(ChooseBrandingForm): def __init__(self, service): super().__init__() - self.options.choices = tuple(self.get_available_choices(service, 'email')) + self.options.choices = tuple(self.get_available_choices(service)) + + @classmethod + def get_available_choices(cls, service): + return ( + list(branding.get_email_choices(service)) + + [cls.FALLBACK_OPTION] + ) class ChooseLetterBrandingForm(ChooseBrandingForm): @@ -2216,7 +2216,12 @@ class ChooseLetterBrandingForm(ChooseBrandingForm): def __init__(self, service): super().__init__() - self.options.choices = tuple(self.get_available_choices(service, 'letter')) + + self.options.choices = tuple( + list(branding.get_letter_choices(service)) + + [self.FALLBACK_OPTION] + ) + if self.something_else_is_only_option: self.options.data = self.FALLBACK_OPTION_VALUE diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index ec2121478..6a95d2538 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -1179,7 +1179,7 @@ def email_branding_request(service_id): def check_email_branding_allowed_for_service(branding): allowed_branding_for_service = dict( - ChooseEmailBrandingForm.get_available_choices(current_service, branding_type='email') + ChooseEmailBrandingForm.get_available_choices(current_service) ) if branding not in allowed_branding_for_service: abort(404) diff --git a/app/utils/branding.py b/app/utils/branding.py index dfa9f1585..89e29dc41 100644 --- a/app/utils/branding.py +++ b/app/utils/branding.py @@ -1,21 +1,15 @@ 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 +def get_email_choices(service): + 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 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') @@ -24,7 +18,6 @@ def get_available_choices(service, branding_type): 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)) @@ -51,3 +44,33 @@ def get_available_choices(service, branding_type): ) ): yield ('organisation', service.organisation.name) + + +def get_letter_choices(service): + 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 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) diff --git a/tests/app/utils/test_branding.py b/tests/app/utils/test_branding.py index 6c528a669..5a7a9d0fb 100644 --- a/tests/app/utils/test_branding.py +++ b/tests/app/utils/test_branding.py @@ -3,11 +3,11 @@ from unittest.mock import PropertyMock import pytest from app.models.service import Service -from app.utils.branding import get_available_choices +from app.utils.branding import get_email_choices, get_letter_choices from tests import organisation_json -@pytest.mark.parametrize('branding_type', ['email', 'letter']) +@pytest.mark.parametrize('function', [get_email_choices, get_letter_choices]) @pytest.mark.parametrize('org_type, expected_options', [ ('central', []), ('local', []), @@ -17,20 +17,20 @@ from tests import organisation_json ('emergency_service', []), ('other', []), ]) -def test_get_available_choices_service_not_assigned_to_org( +def test_get_choices_service_not_assigned_to_org( service_one, - branding_type, + function, org_type, expected_options, ): service_one['organisation_type'] = org_type service = Service(service_one) - options = get_available_choices(service, branding_type=branding_type) + options = function(service) assert list(options) == expected_options -@pytest.mark.parametrize('branding_type', ['email', 'letter']) +@pytest.mark.parametrize('function', [get_email_choices, get_letter_choices]) @pytest.mark.parametrize('org_type, expected_options', [ ('local', [('organisation', 'Test Organisation')]), ('nhs_central', [('nhs', 'NHS')]), @@ -39,10 +39,10 @@ def test_get_available_choices_service_not_assigned_to_org( ('emergency_service', [('organisation', 'Test Organisation')]), ('other', [('organisation', 'Test Organisation')]), ]) -def test_get_available_choices_service_assigned_to_org( +def test_get_choices_service_assigned_to_org( mocker, service_one, - branding_type, + function, org_type, expected_options, mock_get_service_organisation, @@ -54,7 +54,7 @@ def test_get_available_choices_service_assigned_to_org( return_value=organisation_json(organisation_type=org_type) ) - options = get_available_choices(service, branding_type=branding_type) + options = function(service) assert list(options) == expected_options @@ -69,7 +69,7 @@ def test_get_available_choices_service_assigned_to_org( ('organisation', 'Test Organisation'), ]) ]) -def test_get_available_choices_email_branding_central_org( +def test_get_email_choices_central_org( mocker, service_one, service_branding, @@ -89,11 +89,11 @@ def test_get_available_choices_email_branding_central_org( return_value=service_branding, ) - options = get_available_choices(service, branding_type='email') + options = get_email_choices(service) assert list(options) == expected_options -def test_get_available_choices_letter_branding_set( +def test_get_letter_choices_branding_set( mocker, service_one, mock_get_service_organisation, @@ -111,7 +111,7 @@ def test_get_available_choices_letter_branding_set( return_value='some-random-branding', ) - options = get_available_choices(service, branding_type='letter') + options = get_letter_choices(service) assert list(options) == [ ('organisation', 'Test Organisation'), ]