From fa3e6435a6fb001bc495b28814fba41f8257c192 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 15 Mar 2022 11:37:45 +0000 Subject: [PATCH] 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) == [