From 5ddbe80ea9afea6166364cf6f8221c5560f14495 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 26 Jul 2017 10:23:39 +0100 Subject: [PATCH 1/7] Fix calls to API client which now takes fewer args The Notify API client changed in version 4 to take two arguments, not three (service ID was removed in favour of the combined API key). This gets a bit gnarly because the API key has to be at least a certain length so it can be substringed internally. --- app/notify_client/api_key_api_client.py | 8 +++++--- app/notify_client/events_api_client.py | 8 +++++--- app/notify_client/invite_api_client.py | 8 +++++--- app/notify_client/job_api_client.py | 8 +++++--- app/notify_client/letter_jobs_client.py | 8 +++++--- app/notify_client/notification_api_client.py | 8 +++++--- app/notify_client/organisations_client.py | 8 +++++--- app/notify_client/provider_client.py | 8 +++++--- app/notify_client/service_api_client.py | 8 +++++--- app/notify_client/status_api_client.py | 8 +++++--- app/notify_client/template_statistics_api_client.py | 8 +++++--- app/notify_client/user_api_client.py | 8 +++++--- .../notify_client/test_notify_admin_api_client.py | 13 ++++++++----- 13 files changed, 68 insertions(+), 41 deletions(-) diff --git a/app/notify_client/api_key_api_client.py b/app/notify_client/api_key_api_client.py index e8429b650..0e79a137d 100644 --- a/app/notify_client/api_key_api_client.py +++ b/app/notify_client/api_key_api_client.py @@ -8,12 +8,14 @@ KEY_TYPE_TEST = 'test' class ApiKeyApiClient(NotifyAdminAPIClient): def __init__(self): - super().__init__("a", "b", "c") + super().__init__("a" * 73, "b") def init_app(self, app): self.base_url = app.config['API_HOST_NAME'] - self.service_id = app.config['ADMIN_CLIENT_USER_NAME'] - self.api_key = app.config['ADMIN_CLIENT_SECRET'] + self.api_key = '{}-{}'.format( + app.config['ADMIN_CLIENT_USER_NAME'], + app.config['ADMIN_CLIENT_SECRET'], + ) def get_api_keys(self, service_id, key_id=None): if key_id: diff --git a/app/notify_client/events_api_client.py b/app/notify_client/events_api_client.py index 8c2e99539..a6fcd8a5d 100644 --- a/app/notify_client/events_api_client.py +++ b/app/notify_client/events_api_client.py @@ -3,12 +3,14 @@ from app.notify_client import NotifyAdminAPIClient class EventsApiClient(NotifyAdminAPIClient): def __init__(self): - super().__init__("a", "b", "c") + super().__init__("a" * 73, "b") def init_app(self, app): self.base_url = app.config['API_HOST_NAME'] - self.service_id = app.config['ADMIN_CLIENT_USER_NAME'] - self.api_key = app.config['ADMIN_CLIENT_SECRET'] + self.api_key = '{}-{}'.format( + app.config['ADMIN_CLIENT_USER_NAME'], + app.config['ADMIN_CLIENT_SECRET'], + ) def create_event(self, event_type, event_data): data = { diff --git a/app/notify_client/invite_api_client.py b/app/notify_client/invite_api_client.py index 4fca1e4e6..96116c32f 100644 --- a/app/notify_client/invite_api_client.py +++ b/app/notify_client/invite_api_client.py @@ -5,12 +5,14 @@ from app.notify_client.models import InvitedUser class InviteApiClient(NotifyAdminAPIClient): def __init__(self): - super().__init__("a", "b", "c") + super().__init__("a" * 73, "b") def init_app(self, app): self.base_url = app.config['API_HOST_NAME'] - self.service_id = app.config['ADMIN_CLIENT_USER_NAME'] - self.api_key = app.config['ADMIN_CLIENT_SECRET'] + self.api_key = '{}-{}'.format( + app.config['ADMIN_CLIENT_USER_NAME'], + app.config['ADMIN_CLIENT_SECRET'], + ) def create_invite(self, invite_from_id, service_id, email_address, permissions): data = { diff --git a/app/notify_client/job_api_client.py b/app/notify_client/job_api_client.py index 8072a8bf9..8aa8c38a8 100644 --- a/app/notify_client/job_api_client.py +++ b/app/notify_client/job_api_client.py @@ -17,12 +17,14 @@ class JobApiClient(NotifyAdminAPIClient): } def __init__(self): - super().__init__("a", "b", "c") + super().__init__("a" * 73, "b") def init_app(self, app): self.base_url = app.config['API_HOST_NAME'] - self.service_id = app.config['ADMIN_CLIENT_USER_NAME'] - self.api_key = app.config['ADMIN_CLIENT_SECRET'] + self.api_key = '{}-{}'.format( + app.config['ADMIN_CLIENT_USER_NAME'], + app.config['ADMIN_CLIENT_SECRET'], + ) @staticmethod def __convert_statistics(job): diff --git a/app/notify_client/letter_jobs_client.py b/app/notify_client/letter_jobs_client.py index a3337917b..b37b68375 100644 --- a/app/notify_client/letter_jobs_client.py +++ b/app/notify_client/letter_jobs_client.py @@ -4,12 +4,14 @@ from app.notify_client import NotifyAdminAPIClient class LetterJobsClient(NotifyAdminAPIClient): def __init__(self): - super().__init__("a", "b", "c") + super().__init__("a" * 73, "b") def init_app(self, app): self.base_url = app.config['API_HOST_NAME'] - self.service_id = app.config['ADMIN_CLIENT_USER_NAME'] - self.api_key = app.config['ADMIN_CLIENT_SECRET'] + self.api_key = '{}-{}'.format( + app.config['ADMIN_CLIENT_USER_NAME'], + app.config['ADMIN_CLIENT_SECRET'], + ) def get_letter_jobs(self): return self.get(url='/letter-jobs')['data'] diff --git a/app/notify_client/notification_api_client.py b/app/notify_client/notification_api_client.py index ed476a930..917756af1 100644 --- a/app/notify_client/notification_api_client.py +++ b/app/notify_client/notification_api_client.py @@ -3,12 +3,14 @@ from app.notify_client import _attach_current_user, NotifyAdminAPIClient class NotificationApiClient(NotifyAdminAPIClient): def __init__(self): - super().__init__("a", "b", "c") + super().__init__("a" * 73, "b") def init_app(self, app): self.base_url = app.config['API_HOST_NAME'] - self.service_id = app.config['ADMIN_CLIENT_USER_NAME'] - self.api_key = app.config['ADMIN_CLIENT_SECRET'] + self.api_key = '{}-{}'.format( + app.config['ADMIN_CLIENT_USER_NAME'], + app.config['ADMIN_CLIENT_SECRET'], + ) def get_notifications_for_service( self, diff --git a/app/notify_client/organisations_client.py b/app/notify_client/organisations_client.py index d18b9e66f..b5fab3810 100644 --- a/app/notify_client/organisations_client.py +++ b/app/notify_client/organisations_client.py @@ -4,12 +4,14 @@ from app.notify_client import NotifyAdminAPIClient class OrganisationsClient(NotifyAdminAPIClient): def __init__(self): - super().__init__("a", "b", "c") + super().__init__("a" * 73, "b") def init_app(self, app): self.base_url = app.config['API_HOST_NAME'] - self.service_id = app.config['ADMIN_CLIENT_USER_NAME'] - self.api_key = app.config['ADMIN_CLIENT_SECRET'] + self.api_key = '{}-{}'.format( + app.config['ADMIN_CLIENT_USER_NAME'], + app.config['ADMIN_CLIENT_SECRET'], + ) def get_organisation(self, id): return self.get(url='/organisation/{}'.format(id)) diff --git a/app/notify_client/provider_client.py b/app/notify_client/provider_client.py index 2f34f7a98..464219b38 100644 --- a/app/notify_client/provider_client.py +++ b/app/notify_client/provider_client.py @@ -4,12 +4,14 @@ from app.notify_client import _attach_current_user, NotifyAdminAPIClient class ProviderClient(NotifyAdminAPIClient): def __init__(self): - super().__init__("a", "b", "c") + super().__init__("a" * 73, "b") def init_app(self, app): self.base_url = app.config['API_HOST_NAME'] - self.service_id = app.config['ADMIN_CLIENT_USER_NAME'] - self.api_key = app.config['ADMIN_CLIENT_SECRET'] + self.api_key = '{}-{}'.format( + app.config['ADMIN_CLIENT_USER_NAME'], + app.config['ADMIN_CLIENT_SECRET'], + ) def get_all_providers(self): return self.get( diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index 5ee8f7562..db54c91d3 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -10,12 +10,14 @@ class ServiceAPIClient(NotifyAdminAPIClient): # Fudge assert in the super __init__ so # we can set those variables later. def __init__(self): - super().__init__("a", "b", "c") + super().__init__("a" * 73, "b") def init_app(self, application): self.base_url = application.config['API_HOST_NAME'] - self.service_id = application.config['ADMIN_CLIENT_USER_NAME'] - self.api_key = application.config['ADMIN_CLIENT_SECRET'] + self.api_key = '{}-{}'.format( + application.config['ADMIN_CLIENT_USER_NAME'], + application.config['ADMIN_CLIENT_SECRET'], + ) def create_service(self, service_name, message_limit, restricted, user_id, email_from): """ diff --git a/app/notify_client/status_api_client.py b/app/notify_client/status_api_client.py index fc6c233c0..a37d06e86 100644 --- a/app/notify_client/status_api_client.py +++ b/app/notify_client/status_api_client.py @@ -4,12 +4,14 @@ from app.notify_client import NotifyAdminAPIClient class StatusApiClient(NotifyAdminAPIClient): def __init__(self): - super().__init__("a", "b", "c") + super().__init__("a" * 73, "b") def init_app(self, app): self.base_url = app.config['API_HOST_NAME'] - self.service_id = app.config['ADMIN_CLIENT_USER_NAME'] - self.api_key = app.config['ADMIN_CLIENT_SECRET'] + self.api_key = '{}-{}'.format( + app.config['ADMIN_CLIENT_USER_NAME'], + app.config['ADMIN_CLIENT_SECRET'], + ) def get_status(self, *params): return self.get(url='/_status', *params) diff --git a/app/notify_client/template_statistics_api_client.py b/app/notify_client/template_statistics_api_client.py index fc44fa7b6..0d2788d1d 100644 --- a/app/notify_client/template_statistics_api_client.py +++ b/app/notify_client/template_statistics_api_client.py @@ -3,12 +3,14 @@ from app.notify_client import NotifyAdminAPIClient class TemplateStatisticsApiClient(NotifyAdminAPIClient): def __init__(self): - super().__init__("a", "b", "c") + super().__init__("a" * 73, "b") def init_app(self, app): self.base_url = app.config['API_HOST_NAME'] - self.service_id = app.config['ADMIN_CLIENT_USER_NAME'] - self.api_key = app.config['ADMIN_CLIENT_SECRET'] + self.api_key = '{}-{}'.format( + app.config['ADMIN_CLIENT_USER_NAME'], + app.config['ADMIN_CLIENT_SECRET'], + ) def get_template_statistics_for_service(self, service_id, limit_days=None): params = {} diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index e99b7ec3b..be14e1f61 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -15,12 +15,14 @@ ALLOWED_ATTRIBUTES = { class UserApiClient(NotifyAdminAPIClient): def __init__(self): - super().__init__("a", "b", "c") + super().__init__("a" * 73, "b") def init_app(self, app): self.base_url = app.config['API_HOST_NAME'] - self.service_id = app.config['ADMIN_CLIENT_USER_NAME'] - self.api_key = app.config['ADMIN_CLIENT_SECRET'] + self.api_key = '{}-{}'.format( + app.config['ADMIN_CLIENT_USER_NAME'], + app.config['ADMIN_CLIENT_SECRET'], + ) self.max_failed_login_count = app.config["MAX_FAILED_LOGIN_COUNT"] def register_user(self, name, email_address, mobile_number, password): diff --git a/tests/app/notify_client/test_notify_admin_api_client.py b/tests/app/notify_client/test_notify_admin_api_client.py index 2e0fe9ef5..53bc61d30 100644 --- a/tests/app/notify_client/test_notify_admin_api_client.py +++ b/tests/app/notify_client/test_notify_admin_api_client.py @@ -9,6 +9,9 @@ from tests.conftest import api_user_active, platform_admin_user from app.notify_client import NotifyAdminAPIClient +SAMPLE_API_KEY = '{}-{}'.format('a' * 36, 's' * 36) + + @pytest.mark.parametrize('method', [ 'put', 'post', @@ -23,7 +26,7 @@ from app.notify_client import NotifyAdminAPIClient None ], ids=['active_service', 'no_service']) def test_active_service_can_be_modified(app_, method, user, service): - api_client = NotifyAdminAPIClient('api_key', 'base_url', 'service_id') + api_client = NotifyAdminAPIClient(SAMPLE_API_KEY, 'base_url') with app_.test_request_context() as request_context, app_.test_client() as client: client.login(user) @@ -42,7 +45,7 @@ def test_active_service_can_be_modified(app_, method, user, service): 'delete' ]) def test_inactive_service_cannot_be_modified_by_normal_user(app_, api_user_active, method): - api_client = NotifyAdminAPIClient('api_key', 'base_url', 'service_id') + api_client = NotifyAdminAPIClient(SAMPLE_API_KEY, 'base_url') with app_.test_request_context() as request_context, app_.test_client() as client: client.login(api_user_active) @@ -61,7 +64,7 @@ def test_inactive_service_cannot_be_modified_by_normal_user(app_, api_user_activ 'delete' ]) def test_inactive_service_can_be_modified_by_platform_admin(app_, platform_admin_user, method): - api_client = NotifyAdminAPIClient('api_key', 'base_url', 'service_id') + api_client = NotifyAdminAPIClient(SAMPLE_API_KEY, 'base_url') with app_.test_request_context() as request_context, app_.test_client() as client: client.login(platform_admin_user) @@ -75,7 +78,7 @@ def test_inactive_service_can_be_modified_by_platform_admin(app_, platform_admin def test_generate_headers_sets_standard_headers(): - api_client = NotifyAdminAPIClient('api_key', 'base_url', 'service_id') + api_client = NotifyAdminAPIClient(SAMPLE_API_KEY, 'base_url') # with patch('app.notify_client.has_request_context', return_value=False): headers = api_client.generate_headers('api_token') @@ -87,7 +90,7 @@ def test_generate_headers_sets_standard_headers(): def test_generate_headers_sets_request_id_if_in_request_context(app_): - api_client = NotifyAdminAPIClient('api_key', 'base_url', 'service_id') + api_client = NotifyAdminAPIClient(SAMPLE_API_KEY, 'base_url') with app_.test_request_context() as request_context: headers = api_client.generate_headers('api_token') From 9f9c2d5e87012471da2787d3323896533e0b20b1 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 26 Jul 2017 10:24:14 +0100 Subject: [PATCH 2/7] Fix test which looks at text of radio buttons Looks like `radio_button.text` no longer works. Probably a BeautifulSoup change. More robust to look at the text of the label and the value of the input anyway. --- tests/app/main/views/test_letters.py | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/tests/app/main/views/test_letters.py b/tests/app/main/views/test_letters.py index 629651d08..3ade48787 100644 --- a/tests/app/main/views/test_letters.py +++ b/tests/app/main/views/test_letters.py @@ -55,8 +55,14 @@ def test_letters_lets_in_without_permission( @pytest.mark.parametrize('permissions, choices', [ - (['email', 'sms', 'letter'], ['Email', 'Text message', 'Letter']), - (['email', 'sms'], ['Email', 'Text message']) + ( + ['email', 'sms', 'letter'], + ['Email', 'Text message', 'Letter'] + ), + ( + ['email', 'sms'], + ['Email', 'Text message'] + ), ]) def test_given_option_to_add_letters_if_allowed( logged_in_client, @@ -73,8 +79,13 @@ def test_given_option_to_add_letters_if_allowed( assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') radios = page.select('input[type=radio]') + labels = page.select('label') assert len(radios) == len(choices) + assert len(labels) == len(choices) - for index, choice in enumerate(choices): - assert radios[index].text.strip() == choice + for index, choice in enumerate(permissions): + assert radios[index]['value'] == choice + + for index, label in enumerate(choices): + assert labels[index].text.strip() == label From 68a1426e58475965a052d511c61e4ca2230b0e17 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 26 Jul 2017 10:46:39 +0100 Subject: [PATCH 3/7] More Python client weirdness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There’s some weird interaction between the message attribute of the exception and mocking. Luckily there is an internal attribute – `_message` which doesn’t go through all the magic. --- app/main/views/send.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 557faf0ba..07b837101 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -668,11 +668,11 @@ def _check_notification(service_id, template_id, exception=None): def get_template_error_dict(exception): # TODO: Make API return some computer-friendly identifier as well as the end user error messages - if 'service is in trial mode' in exception.message: + if 'service is in trial mode' in exception._message: error = 'not-allowed-to-send-to' - elif 'Exceeded send limits' in exception.message: + elif 'Exceeded send limits' in exception._message: error = 'too-many-messages' - elif 'Content for template has a character count greater than the limit of' in exception.message: + elif 'Content for template has a character count greater than the limit of' in exception._message: error = 'message-too-long' else: raise exception From 5fdbbda0226127b190186291c92e000ee9c1fa51 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 26 Jul 2017 10:47:14 +0100 Subject: [PATCH 4/7] =?UTF-8?q?For=20some=20reason=20we=E2=80=99re=20gener?= =?UTF-8?q?ating=20XHTML=20
s=20now?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ¯\_(ツ)_/¯ --- tests/app/main/views/test_service_settings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index b5f4473fc..606d4c2c8 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -206,7 +206,7 @@ def test_escapes_letter_contact_block( page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') div = str(page.find_all('tr')[5].find_all('td')[1].div) - assert 'foo
bar' in div + assert 'foo
bar' in div assert '