From c6977b13a0f5565365468e71a829f229354d462f Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 26 May 2022 18:11:10 +0100 Subject: [PATCH 1/3] Simplify stubs in go-live tests This replaces multiple stubs with a single stub on the lower level API client method to return the desired set of templates. You can see this most clearly in the diff for the "_sms_sender_" test: - Add a stub for "get_service_templates" - Remove stubs for "all_templates" and "get_templates" In order to make the change, I had to separate the reply-to set of tests from the "_things_right" tests because "count_of_templates" was actually in conflict with "count_of_email_templates". To make the new test I copied the original and removed unnecessary stubs from both of them depending on what's being tested. I'm not sure what the "_things_right" test name means; the name of the new test is at least consistent with others in the file. Note: I also removed the "assert mock_templates.called is True" lines as they wasn't adding any value that I can see. --- .../service_settings/test_service_settings.py | 155 +++++++++--------- 1 file changed, 77 insertions(+), 78 deletions(-) diff --git a/tests/app/main/views/service_settings/test_service_settings.py b/tests/app/main/views/service_settings/test_service_settings.py index 40a4bf35a..065c8c3dc 100644 --- a/tests/app/main/views/service_settings/test_service_settings.py +++ b/tests/app/main/views/service_settings/test_service_settings.py @@ -36,6 +36,7 @@ from tests.conftest import ( create_platform_admin_user, create_reply_to_email_address, create_sms_sender, + create_template, normalize_spaces, ) @@ -836,20 +837,6 @@ def test_should_check_if_estimated_volumes_provided( ) -@pytest.mark.parametrize(( - 'count_of_users_with_manage_service,' - 'count_of_invites_with_manage_service,' - 'expected_user_checklist_item' -), [ - (1, 0, 'Add a team member who can manage settings, team and usage Not completed'), - (2, 0, 'Add a team member who can manage settings, team and usage Completed'), - (1, 1, 'Add a team member who can manage settings, team and usage Completed'), -]) -@pytest.mark.parametrize('count_of_templates, expected_templates_checklist_item', [ - (0, 'Add templates with examples of the content you plan to send Not completed'), - (1, 'Add templates with examples of the content you plan to send Completed'), - (2, 'Add templates with examples of the content you plan to send Completed'), -]) @pytest.mark.parametrize(( 'volume_email,' 'count_of_email_templates,' @@ -865,6 +852,66 @@ def test_should_check_if_estimated_volumes_provided( (1, 0, [], 'Add a reply-to email address Not completed'), (1, 0, [{}], 'Add a reply-to email address Completed'), ]) +def test_should_check_for_reply_to_on_go_live( + client_request, + mocker, + service_one, + fake_uuid, + single_sms_sender, + volume_email, + count_of_email_templates, + reply_to_email_addresses, + expected_reply_to_checklist_item, + mock_get_invites_for_service, + mock_get_users_by_service, +): + mocker.patch( + 'app.service_api_client.get_service_templates', + return_value={'data': [ + create_template(template_type='email') + for _ in range(0, count_of_email_templates) + ]} + ) + + mock_get_reply_to_email_addresses = mocker.patch( + 'app.main.views.service_settings.service_api_client.get_reply_to_email_addresses', + return_value=reply_to_email_addresses + ) + + for channel, volume in (('email', volume_email), ('sms', 0), ('letter', 1)): + mocker.patch( + 'app.models.service.Service.volume_{}'.format(channel), + create=True, + new_callable=PropertyMock, + return_value=volume, + ) + + page = client_request.get( + 'main.request_to_go_live', service_id=SERVICE_ONE_ID + ) + assert page.h1.text == 'Before you request to go live' + + checklist_items = page.select('.task-list .task-list-item') + assert normalize_spaces(checklist_items[3].text) == expected_reply_to_checklist_item + + if count_of_email_templates: + mock_get_reply_to_email_addresses.assert_called_once_with(SERVICE_ONE_ID) + + +@pytest.mark.parametrize(( + 'count_of_users_with_manage_service,' + 'count_of_invites_with_manage_service,' + 'expected_user_checklist_item' +), [ + (1, 0, 'Add a team member who can manage settings, team and usage Not completed'), + (2, 0, 'Add a team member who can manage settings, team and usage Completed'), + (1, 1, 'Add a team member who can manage settings, team and usage Completed'), +]) +@pytest.mark.parametrize('count_of_templates, expected_templates_checklist_item', [ + (0, 'Add templates with examples of the content you plan to send Not completed'), + (1, 'Add templates with examples of the content you plan to send Completed'), + (2, 'Add templates with examples of the content you plan to send Completed'), +]) def test_should_check_for_sending_things_right( client_request, mocker, @@ -876,19 +923,18 @@ def test_should_check_for_sending_things_right( expected_user_checklist_item, count_of_templates, expected_templates_checklist_item, - volume_email, - count_of_email_templates, - reply_to_email_addresses, - expected_reply_to_checklist_item, active_user_with_permissions, active_user_no_settings_permission, + single_reply_to_email_address, ): - def _templates_by_type(template_type): - return { - 'email': list(range(0, count_of_email_templates)), - 'sms': [], - }.get(template_type) - active_user_with_permissions, + mocker.patch( + 'app.service_api_client.get_service_templates', + return_value={'data': [ + create_template(template_type='sms') + for _ in range(0, count_of_templates) + ]} + ) + mock_get_users = mocker.patch( 'app.models.user.Users.client_method', return_value=( @@ -917,35 +963,6 @@ def test_should_check_for_sending_things_right( ) ) - mock_templates = mocker.patch( - 'app.models.service.Service.all_templates', - new_callable=PropertyMock, - return_value=list(range(0, count_of_templates)), - ) - - mocker.patch( - 'app.models.service.Service.get_templates', - side_effect=_templates_by_type, - ) - mocker.patch( - 'app.models.service.Service.organisation_id', - new_callable=PropertyMock, - return_value=None, - ) - - mock_get_reply_to_email_addresses = mocker.patch( - 'app.main.views.service_settings.service_api_client.get_reply_to_email_addresses', - return_value=reply_to_email_addresses - ) - - for channel, volume in (('email', volume_email), ('sms', 0), ('letter', 1)): - mocker.patch( - 'app.models.service.Service.volume_{}'.format(channel), - create=True, - new_callable=PropertyMock, - return_value=volume, - ) - page = client_request.get( 'main.request_to_go_live', service_id=SERVICE_ONE_ID ) @@ -954,14 +971,9 @@ def test_should_check_for_sending_things_right( checklist_items = page.select('.task-list .task-list-item') assert normalize_spaces(checklist_items[1].text) == expected_user_checklist_item assert normalize_spaces(checklist_items[2].text) == expected_templates_checklist_item - assert normalize_spaces(checklist_items[3].text) == expected_reply_to_checklist_item mock_get_users.assert_called_once_with(SERVICE_ONE_ID) mock_get_invites.assert_called_once_with(SERVICE_ONE_ID) - assert mock_templates.called is True - - if count_of_email_templates: - mock_get_reply_to_email_addresses.assert_called_once_with(SERVICE_ONE_ID) @pytest.mark.parametrize('checklist_completed, agreement_signed, expected_button', ( @@ -1149,34 +1161,23 @@ def test_should_check_for_sms_sender_on_go_live( ): service_one['organisation_type'] = organisation_type - def _templates_by_type(template_type): - return list(range(0, { - 'email': 0, - 'sms': count_of_sms_templates, - }.get(template_type, count_of_sms_templates))) + mocker.patch( + 'app.service_api_client.get_service_templates', + return_value={'data': [ + create_template(template_type='sms') + for _ in range(0, count_of_sms_templates) + ]} + ) mocker.patch( 'app.models.service.Service.has_team_members', return_value=True, ) - mock_templates = mocker.patch( - 'app.models.service.Service.all_templates', - new_callable=PropertyMock, - side_effect=partial(_templates_by_type, 'all'), - ) - mocker.patch( - 'app.models.service.Service.get_templates', - side_effect=_templates_by_type, - ) mock_get_sms_senders = mocker.patch( 'app.main.views.service_settings.service_api_client.get_sms_senders', return_value=sms_senders, ) - mocker.patch( - 'app.main.views.service_settings.service_api_client.get_reply_to_email_addresses', - return_value=[], - ) for channel, volume in (('email', 0), ('sms', estimated_sms_volume)): mocker.patch( @@ -1194,8 +1195,6 @@ def test_should_check_for_sms_sender_on_go_live( checklist_items = page.select('.task-list .task-list-item') assert normalize_spaces(checklist_items[3].text) == expected_sms_sender_checklist_item - assert mock_templates.called is True - mock_get_sms_senders.assert_called_once_with(SERVICE_ONE_ID) From 1b96a13565c6c6aa22e438a1f25ae3446f4ac8b3 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 26 May 2022 18:34:27 +0100 Subject: [PATCH 2/3] Add xfail tests for go-live reply-to/sender check This represents a bug where a user can request to go live without setting a reply-to email address or SMS sender despite the service having one or more email or SMS templates, respectively. We will make these tests pass in the next commit. --- tests/app/models/test_service.py | 44 +++++++++++++++++++++++++++++++- tests/conftest.py | 12 ++++++++- 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/tests/app/models/test_service.py b/tests/app/models/test_service.py index e143e2603..f7ea179d6 100644 --- a/tests/app/models/test_service.py +++ b/tests/app/models/test_service.py @@ -6,7 +6,7 @@ from app.models.organisation import Organisation from app.models.service import Service from app.models.user import User from tests import organisation_json, service_json -from tests.conftest import ORGANISATION_ID +from tests.conftest import ORGANISATION_ID, create_folder, create_template INV_PARENT_FOLDER_ID = '7e979e79-d970-43a5-ac69-b625a8d147b0' INV_CHILD_1_FOLDER_ID = '92ee1ee0-e4ee-4dcc-b1a7-a5da9ebcfa2b' @@ -265,3 +265,45 @@ def test_service_billing_details(purchase_order_number, expected_result): service = Service(service_json(purchase_order_number=purchase_order_number)) service._dict['purchase_order_number'] = purchase_order_number assert service.billing_details == expected_result + + +@pytest.mark.xfail +def test_has_email_templates_includes_folders( + mocker, + service_one, + mock_get_template_folders, +): + mocker.patch( + 'app.service_api_client.get_service_templates', + return_value={'data': [create_template( + folder='something', template_type='email' + )]} + ) + + mocker.patch( + 'app.template_folder_api_client.get_template_folders', + return_value=[create_folder(id='something')] + ) + + assert Service(service_one).has_email_templates + + +@pytest.mark.xfail +def test_has_sms_templates_includes_folders( + mocker, + service_one, + mock_get_template_folders, +): + mocker.patch( + 'app.service_api_client.get_service_templates', + return_value={'data': [create_template( + folder='something', template_type='sms' + )]} + ) + + mocker.patch( + 'app.template_folder_api_client.get_template_folders', + return_value=[create_folder(id='something')] + ) + + assert Service(service_one).has_sms_templates diff --git a/tests/conftest.py b/tests/conftest.py index 5f01240c8..305e6f873 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3982,6 +3982,14 @@ def create_notifications( ) +def create_folder(id): + return { + 'id': id, + 'parent_id': None, + 'name': 'My folder' + } + + def create_template( service_id=SERVICE_ONE_ID, template_id=None, @@ -3990,7 +3998,8 @@ def create_template( content='Template content', subject='Template subject', redact_personalisation=False, - postage=None + postage=None, + folder=None ): return template_json( service_id=service_id, @@ -4001,6 +4010,7 @@ def create_template( subject=subject, redact_personalisation=redact_personalisation, postage=postage, + folder=folder, ) From 32efb9a03d065d271b91a6bc66e269c0bd973863 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 27 May 2022 10:31:06 +0100 Subject: [PATCH 3/3] Fix go-live checks ignoring nested templates This is a very low impact bug since a user can always create such templates after their service is live and not be subject to checks we do before that point. Still, we may as well fix it. The main benefit of this change is actually to contribute towards moving methods like "get_templates" out of the Service class so we can simplify and cache their results more effectively. Note: I wanted to simplify the Service class further as the two "has_" properties are only used once in the app code. Unfortunately they are tightly coupled in one of the tests as well [^1]. [^1]: https://github.com/alphagov/notifications-admin/blob/bef0382cca16ba87d04430f8cd391092b1dc91ef/tests/app/main/views/service_settings/test_service_settings.py#L1961-L1962 --- app/models/service.py | 10 ++++++++-- tests/app/models/test_service.py | 26 ++------------------------ 2 files changed, 10 insertions(+), 26 deletions(-) diff --git a/app/models/service.py b/app/models/service.py index 9d1ca937b..3694d8487 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -284,13 +284,19 @@ class Service(JSONModel, SortByNameMixin): )) ) + def has_templates_of_type(self, template_type): + return any( + template for template in self.all_templates + if template['template_type'] == template_type + ) + @property def has_email_templates(self): - return len(self.get_templates('email')) > 0 + return self.has_templates_of_type('email') @property def has_sms_templates(self): - return len(self.get_templates('sms')) > 0 + return self.has_templates_of_type('sms') @property def intending_to_send_email(self): diff --git a/tests/app/models/test_service.py b/tests/app/models/test_service.py index f7ea179d6..908b49037 100644 --- a/tests/app/models/test_service.py +++ b/tests/app/models/test_service.py @@ -267,29 +267,7 @@ def test_service_billing_details(purchase_order_number, expected_result): assert service.billing_details == expected_result -@pytest.mark.xfail -def test_has_email_templates_includes_folders( - mocker, - service_one, - mock_get_template_folders, -): - mocker.patch( - 'app.service_api_client.get_service_templates', - return_value={'data': [create_template( - folder='something', template_type='email' - )]} - ) - - mocker.patch( - 'app.template_folder_api_client.get_template_folders', - return_value=[create_folder(id='something')] - ) - - assert Service(service_one).has_email_templates - - -@pytest.mark.xfail -def test_has_sms_templates_includes_folders( +def test_has_templates_of_type_includes_folders( mocker, service_one, mock_get_template_folders, @@ -306,4 +284,4 @@ def test_has_sms_templates_includes_folders( return_value=[create_folder(id='something')] ) - assert Service(service_one).has_sms_templates + assert Service(service_one).has_templates_of_type('sms')