From 5405c2e1be9576f36c98a9609d2a4e28b1fac9e6 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 5 Feb 2019 16:34:05 +0000 Subject: [PATCH] fix service settings letter branding tests some tests are now expanded to handle the fact that letter branding can be null --- tests/__init__.py | 2 +- .../test_service_setting_permissions.py | 4 +- tests/app/main/views/test_service_settings.py | 119 +++++++++++------- .../test_letter_branding_client.py | 37 +++++- tests/conftest.py | 44 +++++-- 5 files changed, 144 insertions(+), 62 deletions(-) diff --git a/tests/__init__.py b/tests/__init__.py index 458873f55..da3bf8c04 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -167,8 +167,8 @@ def service_json( 'email_branding': email_branding, 'branding': branding, 'created_at': created_at or str(datetime.utcnow()), + 'letter_branding': None, 'letter_contact_block': letter_contact_block, - 'dvla_organisation': '001', 'permissions': permissions, 'inbound_api': inbound_api, 'service_callback_api': service_callback_api, diff --git a/tests/app/main/views/service_settings/test_service_setting_permissions.py b/tests/app/main/views/service_settings/test_service_setting_permissions.py index c6a8e959d..76a540ca1 100644 --- a/tests/app/main/views/service_settings/test_service_setting_permissions.py +++ b/tests/app/main/views/service_settings/test_service_setting_permissions.py @@ -10,7 +10,7 @@ def get_service_settings_page( platform_admin_user, service_one, mock_get_inbound_number_for_service, - mock_get_letter_branding, + mock_get_all_letter_branding, mock_get_service_organisation, mock_get_free_sms_fragment_limit, no_reply_to_email_addresses, @@ -94,7 +94,7 @@ def test_normal_user_doesnt_see_any_toggle_buttons( no_letter_contact_blocks, mock_get_service_organisation, single_sms_sender, - mock_get_letter_branding, + mock_get_all_letter_branding, mock_get_inbound_number_for_service, mock_get_free_sms_fragment_limit, mock_get_service_data_retention diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 7d2da0a14..683db61d9 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -1,8 +1,7 @@ -import uuid from functools import partial from unittest.mock import ANY, PropertyMock, call from urllib.parse import parse_qs, urlparse -from uuid import uuid4 +from uuid import UUID, uuid4 import pytest from bs4 import BeautifulSoup @@ -38,7 +37,7 @@ from tests.conftest import ( @pytest.fixture def mock_get_service_settings_page_common( - mock_get_letter_branding, + mock_get_all_letter_branding, mock_get_inbound_number_for_service, mock_get_free_sms_fragment_limit, mock_get_service_data_retention, @@ -95,7 +94,7 @@ def mock_get_service_settings_page_common( 'Organisation type Central Change', 'Free text message allowance 250,000 Change', 'Email branding GOV.UK Change', - 'Letter branding HM Government Change', + 'Letter branding Change', 'Data retention email Change' ]), @@ -187,7 +186,7 @@ def test_should_show_overview( 'Label Value Action', 'Send letters On Change', 'Sender addresses 1 Example Street Manage', - 'Letter branding HM Government Change', + 'Letter branding None Change', ]), ]) @@ -1173,7 +1172,7 @@ def test_route_for_platform_admin_update_service( client, platform_admin_user, service_one, - mock_get_letter_branding, + mock_get_all_letter_branding, route, ): mocker.patch('app.service_api_client.archive_service') @@ -2033,7 +2032,7 @@ def test_set_letter_contact_block_redirects_to_template( mock_update_service, ): service_one['permissions'] = ['letter'] - fake_template_id = uuid.uuid4() + fake_template_id = uuid4() response = logged_in_client.post( url_for( 'main.service_set_letter_contact_block', @@ -2069,15 +2068,14 @@ def test_set_letter_contact_block_has_max_10_lines( def test_request_letter_branding( client_request, - mock_get_letter_branding, + mock_get_letter_branding_by_id, + service_one ): request_page = client_request.get( 'main.request_letter_branding', service_id=SERVICE_ONE_ID, ) - assert request_page.select_one('main p').text.strip() == ( - 'Your letters have the HM Government logo.' - ) + assert request_page.select_one('main p').text.strip() == 'Your letters have no logo.' link_href = request_page.select_one('main a')['href'] feedback_page = client_request.get_url(link_href) assert feedback_page.select_one('textarea').text.strip() == ( @@ -2085,70 +2083,97 @@ def test_request_letter_branding( ) -def test_set_letter_branding_platform_admin_only( +def test_request_letter_branding_if_already_have_branding( + client_request, + mock_get_letter_branding_by_id, + service_one, +): + service_one['letter_branding'] = uuid4() + + request_page = client_request.get( + 'main.request_letter_branding', + service_id=SERVICE_ONE_ID, + ) + + mock_get_letter_branding_by_id.assert_called_once_with(service_one['letter_branding']) + assert request_page.select_one('main p').text.strip() == 'Your letters have the HM Government logo.' + + +def test_service_set_letter_branding_platform_admin_only( logged_in_client, service_one, ): - response = logged_in_client.get(url_for('main.set_letter_branding', service_id=service_one['id'])) + response = logged_in_client.get(url_for('main.service_set_letter_branding', service_id=service_one['id'])) assert response.status_code == 403 -@pytest.mark.parametrize('current_dvla_org_id, expected_selected, expected_items', [ - (None, '001', ( - ('001', 'HM Government'), - ('999', 'Animal and Plant Health Agency'), - ('500', 'Land Registry'), +@pytest.mark.parametrize('letter_branding, expected_selected, expected_items', [ + # expected order: currently selected, then default, then rest alphabetically + (None, '__NONE__', ( + ('__NONE__', 'None'), + (str(UUID(int=2)), 'Animal and Plant Health Agency'), + (str(UUID(int=0)), 'HM Government'), + (str(UUID(int=1)), 'Land Registry'), )), - ('500', '500', ( - ('500', 'Land Registry'), - ('001', 'HM Government'), - ('999', 'Animal and Plant Health Agency'), + (str(UUID(int=1)), str(UUID(int=1)), ( + (str(UUID(int=1)), 'Land Registry'), + ('__NONE__', 'None'), + (str(UUID(int=2)), 'Animal and Plant Health Agency'), + (str(UUID(int=0)), 'HM Government'), )), - ('999', '999', ( - ('999', 'Animal and Plant Health Agency'), - ('001', 'HM Government'), - ('500', 'Land Registry'), + (str(UUID(int=2)), str(UUID(int=2)), ( + (str(UUID(int=2)), 'Animal and Plant Health Agency'), + ('__NONE__', 'None'), + (str(UUID(int=0)), 'HM Government'), + (str(UUID(int=1)), 'Land Registry'), )), ]) -def test_set_letter_branding_prepopulates( +def test_service_set_letter_branding_prepopulates( logged_in_platform_admin_client, service_one, - mock_get_letter_branding, - current_dvla_org_id, + mock_get_all_letter_branding, + letter_branding, expected_selected, expected_items, ): - if current_dvla_org_id: - service_one['dvla_organisation'] = current_dvla_org_id - response = logged_in_platform_admin_client.get(url_for('main.set_letter_branding', service_id=service_one['id'])) + service_one['letter_branding'] = letter_branding + response = logged_in_platform_admin_client.get( + url_for('main.service_set_letter_branding', service_id=service_one['id']) + ) assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - for element in {'label[for^=dvla_org_id]', 'input[type=radio]'}: - assert len(page.select(element)) == len(expected_items) - - for index, expected_item in enumerate(expected_items): - expected_value, expected_label = expected_item - assert normalize_spaces(page.select('label[for^=dvla_org_id]')[index].text) == expected_label - assert page.select('input[type=radio]')[index]['value'] == expected_value - assert len(page.select('input[checked]')) == 1 assert page.select('input[checked]')[0]['value'] == expected_selected + for element in {'label[for^=branding_style]', 'input[type=radio]'}: + assert len(page.select(element)) == len(expected_items) -def test_set_letter_branding_saves( + for index, expected_item in enumerate(expected_items): + expected_value, expected_label = expected_item + assert normalize_spaces(page.select('label[for^=branding_style]')[index].text) == expected_label + assert page.select('input[type=radio]')[index]['value'] == expected_value + + +@pytest.mark.parametrize('selected_letter_branding, expected_post_data', [ + (str(UUID(int=1)), str(UUID(int=1))), + ('__NONE__', None), +]) +def test_service_set_letter_branding_saves( logged_in_platform_admin_client, service_one, mock_update_service, - mock_get_letter_branding, + mock_get_all_letter_branding, + selected_letter_branding, + expected_post_data ): response = logged_in_platform_admin_client.post( - url_for('main.set_letter_branding', service_id=service_one['id']), - data={'dvla_org_id': '500'} + url_for('main.service_set_letter_branding', service_id=service_one['id']), + data={'branding_style': selected_letter_branding} ) assert response.status_code == 302 assert response.location == url_for('main.service_settings', service_id=service_one['id'], _external=True) - mock_update_service.assert_called_once_with(service_one['id'], dvla_organisation='500') + mock_update_service.assert_called_once_with(service_one['id'], letter_branding=expected_post_data) @pytest.mark.parametrize('current_branding, expected_values, expected_labels', [ @@ -3063,7 +3088,7 @@ def test_service_settings_when_inbound_number_is_not_set( mock_get_service_organisation, single_sms_sender, mocker, - mock_get_letter_branding, + mock_get_all_letter_branding, mock_get_free_sms_fragment_limit, mock_get_service_data_retention, ): @@ -3081,7 +3106,7 @@ def test_set_inbound_sms_when_inbound_number_is_not_set( single_reply_to_email_address, single_letter_contact_block, mocker, - mock_get_letter_branding, + mock_get_all_letter_branding, ): mocker.patch('app.inbound_number_client.get_inbound_sms_number_for_service', return_value={'data': {}}) diff --git a/tests/app/notify_client/test_letter_branding_client.py b/tests/app/notify_client/test_letter_branding_client.py index 23f962971..8e29510b6 100644 --- a/tests/app/notify_client/test_letter_branding_client.py +++ b/tests/app/notify_client/test_letter_branding_client.py @@ -1,11 +1,38 @@ from app.notify_client.letter_branding_client import LetterBrandingClient -def test_get_letter_branding(mocker): - mock_get = mocker.patch('app.notify_client.letter_branding_client.LetterBrandingClient.get') - LetterBrandingClient().get_letter_branding() - mock_get.assert_called_once_with( - url='/dvla_organisations' +def test_get_letter_branding(mocker, fake_uuid): + mock_get = mocker.patch( + 'app.notify_client.letter_branding_client.LetterBrandingClient.get', + return_value={'foo': 'bar'} + ) + mock_redis_get = mocker.patch('app.notify_client.RedisClient.get', return_value=None) + mock_redis_set = mocker.patch('app.notify_client.RedisClient.set') + + LetterBrandingClient().get_letter_branding(fake_uuid) + + mock_get.assert_called_once_with(url='/letter-branding/{}'.format(fake_uuid)) + mock_redis_get.assert_called_once_with('letter_branding-{}'.format(fake_uuid)) + mock_redis_set.assert_called_once_with( + 'letter_branding-{}'.format(fake_uuid), + '{"foo": "bar"}', + ex=604800, + ) + + +def test_get_all_letter_branding(mocker): + mock_get = mocker.patch('app.notify_client.letter_branding_client.LetterBrandingClient.get', return_value=[1, 2, 3]) + mock_redis_get = mocker.patch('app.notify_client.RedisClient.get', return_value=None) + mock_redis_set = mocker.patch('app.notify_client.RedisClient.set') + + LetterBrandingClient().get_all_letter_branding() + + mock_get.assert_called_once_with(url='/letter-branding') + mock_redis_get.assert_called_once_with('letter_branding') + mock_redis_set.assert_called_once_with( + 'letter_branding', + '[1, 2, 3]', + ex=604800, ) diff --git a/tests/conftest.py b/tests/conftest.py index 023b4a9c7..b37d4c478 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3,6 +3,7 @@ import os from contextlib import contextmanager from datetime import date, datetime, timedelta from unittest.mock import Mock +from uuid import UUID import pytest from bs4 import BeautifulSoup @@ -2548,16 +2549,45 @@ def mock_get_email_branding_that_can_fit_onscreen(mocker): @pytest.fixture(scope='function') -def mock_get_letter_branding(mocker): +def mock_get_all_letter_branding(mocker): def _get_letter_branding(): - return { - '001': 'HM Government', - '500': 'Land Registry', - '999': 'Animal and Plant Health Agency', - } + return [ + { + 'id': str(UUID(int=0)), + 'name': 'HM Government', + 'filename': 'hm-government', + 'domain': None, + }, + { + 'id': str(UUID(int=1)), + 'name': 'Land Registry', + 'filename': 'land-registry', + 'domain': None, + }, + { + 'id': str(UUID(int=2)), + 'name': 'Animal and Plant Health Agency', + 'filename': 'animal', + 'domain': None, + } + ] return mocker.patch( - 'app.letter_branding_client.get_letter_branding', side_effect=_get_letter_branding + 'app.letter_branding_client.get_all_letter_branding', side_effect=_get_letter_branding + ) + + +@pytest.fixture +def mock_get_letter_branding_by_id(mocker): + def _get_branding_by_id(_id): + return { + 'id': _id, + 'name': 'HM Government', + 'filename': 'hm-government', + 'domain': None, + } + return mocker.patch( + 'app.letter_branding_client.get_letter_branding', side_effect=_get_branding_by_id )