From 002b58a062af22e2a4687d1111fc2d7821f02ef3 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 11 Aug 2016 12:32:38 +0100 Subject: [PATCH 1/4] Make service API client do partial updates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The service API client was updating every attribute of a service. Which, while kinda clunky, is fine… …until something calling it doesn’t pass in every attribute of the current service. It was then defaulting optional parameters to `None`. Which resulted in a bug whereby every time a service was set to live, its `reply_to_address` and `sms_sender_name` got overwritten to be empty. This commit changes the `update` method to only require the service ID, and pass whatever other named arguments it received straight through to the API. The API handles partial updates just fine (I think). --- app/main/views/service_settings.py | 48 +++++------------- app/notify_client/service_api_client.py | 32 +++--------- tests/__init__.py | 21 ++++---- tests/app/main/views/test_service_settings.py | 49 +++++++++---------- tests/conftest.py | 37 +++++++------- 5 files changed, 72 insertions(+), 115 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 25a2e4a42..4e26adb1b 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -66,17 +66,12 @@ def service_name_change_confirm(service_id): form = ConfirmPasswordForm(_check_password) if form.validate_on_submit(): - current_service['name'] = session['service_name_change'] - current_service['email_from'] = email_safe(session['service_name_change']) try: service_api_client.update_service( current_service['id'], - current_service['name'], - current_service['active'], - current_service['message_limit'], - current_service['restricted'], - current_service['users'], - current_service['email_from']) + name=session['service_name_change'], + email_from=email_safe(session['service_name_change']) + ) except HTTPError as e: error_msg = "Duplicate service name '{}'".format(session['service_name_change']) if e.status_code == 400 and error_msg in e.message['name']: @@ -143,14 +138,11 @@ def service_request_to_go_live(service_id): def service_switch_live(service_id): service_api_client.update_service( current_service['id'], - current_service['name'], - current_service['active'], # TODO This limit should be set depending on the agreement signed by # with Notify. - 250000 if current_service['restricted'] else 50, - False if current_service['restricted'] else True, - current_service['users'], - current_service['email_from']) + message_limit=250000 if current_service['restricted'] else 50, + restricted=(not current_service['restricted']) + ) return redirect(url_for('.service_settings', service_id=service_id)) @@ -188,15 +180,10 @@ def service_status_change_confirm(service_id): form = ConfirmPasswordForm(_check_password) if form.validate_on_submit(): - current_service['active'] = True service_api_client.update_service( current_service['id'], - current_service['name'], - current_service['active'], - current_service['message_limit'], - current_service['restricted'], - current_service['users'], - current_service['email_from']) + active=True + ) return redirect(url_for('.service_settings', service_id=service_id)) return render_template( 'views/service-settings/confirm.html', @@ -249,13 +236,8 @@ def service_set_reply_to_email(service_id): message = 'Reply to email set to {}'.format(form.email_address.data) service_api_client.update_service( current_service['id'], - current_service['name'], - current_service['active'], - current_service['message_limit'], - current_service['restricted'], - current_service['users'], - current_service['email_from'], - reply_to_email_address=form.email_address.data) + reply_to_email_address=form.email_address.data + ) flash(message, 'default_with_tick') return redirect(url_for('.service_settings', service_id=service_id)) return render_template( @@ -277,14 +259,8 @@ def service_set_sms_sender(service_id): message = 'Text message sender removed' service_api_client.update_service( current_service['id'], - current_service['name'], - current_service['active'], - current_service['message_limit'], - current_service['restricted'], - current_service['users'], - current_service['email_from'], - current_service['reply_to_email_address'], - sms_sender=form.sms_sender.data if form.sms_sender.data else None) + sms_sender=form.sms_sender.data or None + ) flash(message, 'default_with_tick') return redirect(url_for('.service_settings', service_id=service_id)) return render_template( diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index d1134406f..b6a689659 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -73,38 +73,20 @@ class ServiceAPIClient(NotificationsAPIClient): """ return self.get('/service', *params) - def update_service(self, - service_id, - service_name, - active, - message_limit, - restricted, - users, - email_from, - reply_to_email_address=None, - sms_sender=None): + def update_service( + self, + service_id, + **kwargs + ): """ Update a service. """ - data = { - "id": service_id, - "name": service_name, - "active": active, - "message_limit": message_limit, - "restricted": restricted, - "users": users, - "email_from": email_from, - "reply_to_email_address": reply_to_email_address, - "sms_sender": sms_sender - } - _attach_current_user(data) + _attach_current_user(kwargs) endpoint = "/service/{0}".format(service_id) return self.post(endpoint, data) def update_service_with_properties(self, service_id, properties): - _attach_current_user(properties) - endpoint = "/service/{0}".format(service_id) - return self.post(endpoint, properties) + return self.update_service(service_id, **properties) def remove_user_from_service(self, service_id, user_id): """ diff --git a/tests/__init__.py b/tests/__init__.py index 7841a02e7..855abfe6a 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -40,15 +40,17 @@ def created_by_json(id_, name='', email_address=''): def service_json( - id_, - name, - users, - message_limit=1000, - active=False, - restricted=True, - email_from=None, - reply_to_email_address=None, - research_mode=False): + id_, + name, + users, + message_limit=1000, + active=False, + restricted=True, + email_from=None, + reply_to_email_address=None, + sms_sender=None, + research_mode=False +): return { 'id': id_, 'name': name, @@ -58,6 +60,7 @@ def service_json( 'restricted': restricted, 'email_from': email_from, 'reply_to_email_address': reply_to_email_address, + 'sms_sender': sms_sender, 'research_mode': research_mode } diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 1793211dd..bf769297b 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -80,7 +80,11 @@ def test_switch_service_to_live(app_, assert response.location == url_for( 'main.service_settings', service_id=service_one['id'], _external=True) - mock_update_service.assert_called_with(ANY, ANY, ANY, 250000, False, ANY, ANY) + mock_update_service.assert_called_with( + service_one['id'], + message_limit=250000, + restricted=False + ) def test_switch_service_to_restricted(app_, @@ -100,7 +104,11 @@ def test_switch_service_to_restricted(app_, assert response.location == url_for( 'main.service_settings', service_id=service_one['id'], _external=True) - mock_update_service.assert_called_with(ANY, ANY, ANY, 50, True, ANY, ANY) + mock_update_service.assert_called_with( + service_one['id'], + message_limit=50, + restricted=True + ) def test_should_not_allow_duplicate_names(app_, @@ -159,13 +167,11 @@ def test_should_redirect_after_service_name_confirmation(app_, assert response.status_code == 302 settings_url = url_for('main.service_settings', service_id=service_id, _external=True) assert settings_url == response.location - mock_update_service.assert_called_once_with(service_id, - service_new_name, - service_one['active'], - service_one['message_limit'], - service_one['restricted'], - service_one['users'], - email_safe(service_new_name)) + mock_update_service.assert_called_once_with( + service_id, + name=service_new_name, + email_from=email_safe(service_new_name) + ) assert mock_verify_password.called @@ -564,14 +570,10 @@ def test_set_reply_to_email_address( data=data, follow_redirects=True) assert response.status_code == 200 - mock_update_service.assert_called_with(service_one['id'], - service_one['name'], - service_one['active'], - service_one['message_limit'], - service_one['restricted'], - ANY, - service_one['email_from'], - "test@someservice.gov.uk") + mock_update_service.assert_called_with( + service_one['id'], + reply_to_email_address="test@someservice.gov.uk" + ) def test_if_reply_to_email_address_set_then_form_populated(app_, @@ -710,15 +712,10 @@ def test_set_text_message_sender( follow_redirects=True) assert response.status_code == 200 - mock_update_service.assert_called_with(service_one['id'], - service_one['name'], - service_one['active'], - service_one['message_limit'], - service_one['restricted'], - service_one['users'], - service_one['email_from'], - service_one['reply_to_email_address'], - "elevenchars") + mock_update_service.assert_called_with( + service_one['id'], + sms_sender="elevenchars" + ) def test_if_sms_sender_set_then_form_populated(app_, diff --git a/tests/conftest.py b/tests/conftest.py index 0bcef48af..5aee6f55e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -125,18 +125,20 @@ def mock_create_service(mocker): @pytest.fixture(scope='function') def mock_update_service(mocker): - def _update(service_id, - service_name, - active, - message_limit, - restricted, - users, - email_from, - reply_to_email_address=None, - sms_sender=None): + def _update(service_id, **kwargs): service = service_json( - service_id, service_name, users, message_limit=message_limit, - active=active, restricted=restricted, email_from=email_from, reply_to_email_address=reply_to_email_address) + service_id, + **{key: kwargs.get(key) for key in [ + 'name', + 'users', + 'message_limit', + 'active', + 'restricted', + 'email_from', + 'reply_to_email_address', + 'sms_sender' + ]} + ) return {'data': service} return mocker.patch( @@ -145,14 +147,11 @@ def mock_update_service(mocker): @pytest.fixture(scope='function') def mock_update_service_raise_httperror_duplicate_name(mocker): - def _update(service_id, - service_name, - active, - limit, - restricted, - users, - email_from): - json_mock = Mock(return_value={'message': {'name': ["Duplicate service name '{}'".format(service_name)]}}) + def _update( + service_id, + **kwargs + ): + json_mock = Mock(return_value={'message': {'name': ["Duplicate service name '{}'".format(kwargs.get('name'))]}}) resp_mock = Mock(status_code=400, json=json_mock) http_error = HTTPError(response=resp_mock, message="Default message") raise http_error From 0cfe10639a0018553080885569b4ba125341122d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 11 Aug 2016 13:55:42 +0100 Subject: [PATCH 2/4] Only allow update service to modify named attrs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To prevent typos and inadvertently updating something we shouldn’t, this adds some filtering to the update_service method to make sure it is only allowed to update certain attributes of a service. --- app/notify_client/service_api_client.py | 15 +++++++++++++++ .../app/notify_client/test_service_api_client.py | 6 ++++++ 2 files changed, 21 insertions(+) diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index b6a689659..a9b855e0d 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -81,6 +81,21 @@ class ServiceAPIClient(NotificationsAPIClient): """ Update a service. """ + disallowed_attributes = set(kwargs.keys()) - { + 'name', + 'users', + 'message_limit', + 'active', + 'restricted', + 'email_from', + 'reply_to_email_address', + 'sms_sender' + } + if disallowed_attributes: + raise TypeError('Not allowed to update service attributes: {}'.format( + ", ".join(disallowed_attributes) + )) + _attach_current_user(kwargs) endpoint = "/service/{0}".format(service_id) return self.post(endpoint, data) diff --git a/tests/app/notify_client/test_service_api_client.py b/tests/app/notify_client/test_service_api_client.py index 57557936e..95a6e8e50 100644 --- a/tests/app/notify_client/test_service_api_client.py +++ b/tests/app/notify_client/test_service_api_client.py @@ -37,3 +37,9 @@ def test_client_gets_service(mocker, function, params): function(client, 'foo') mock_get.assert_called_once_with('/service/foo', params=params) + + +def test_client_only_updates_allowed_attributes(mocker): + with pytest.raises(TypeError) as error: + ServiceAPIClient().update_service('service_id', foo='bar') + assert str(error.value) == 'Not allowed to update service attributes: foo' From da1fa2e61c97fc2e6baf656af4f4c7c25d78e345 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 11 Aug 2016 14:20:43 +0100 Subject: [PATCH 3/4] Make `_attach_current_user` a pure function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mutating dictionaries is gross and doesn’t work as you’d expect. Better to have the function return a new dictionary instead. Means we can be explicit that `created_by` is one of the allowed params when updating a service. --- app/notify_client/__init__.py | 6 ++++-- app/notify_client/api_key_api_client.py | 2 +- app/notify_client/invite_api_client.py | 4 ++-- app/notify_client/job_api_client.py | 2 +- app/notify_client/provider_client.py | 2 +- app/notify_client/service_api_client.py | 15 ++++++++------- tests/app/notify_client/test_job_client.py | 7 +++---- .../app/notify_client/test_service_api_client.py | 6 +++--- 8 files changed, 23 insertions(+), 21 deletions(-) diff --git a/app/notify_client/__init__.py b/app/notify_client/__init__.py index 0ff7b47c3..69b374978 100644 --- a/app/notify_client/__init__.py +++ b/app/notify_client/__init__.py @@ -2,5 +2,7 @@ from flask.ext.login import current_user def _attach_current_user(data): - data['created_by'] = current_user.id - return data + return dict( + created_by=current_user.id, + **data + ) diff --git a/app/notify_client/api_key_api_client.py b/app/notify_client/api_key_api_client.py index 797678541..ade506c38 100644 --- a/app/notify_client/api_key_api_client.py +++ b/app/notify_client/api_key_api_client.py @@ -30,7 +30,7 @@ class ApiKeyApiClient(BaseAPIClient): 'name': key_name, 'key_type': key_type } - _attach_current_user(data) + data = _attach_current_user(data) key = self.post(url='/service/{}/api-key'.format(service_id), data=data) return key['data'] diff --git a/app/notify_client/invite_api_client.py b/app/notify_client/invite_api_client.py index 874f6b78c..0ac65acb8 100644 --- a/app/notify_client/invite_api_client.py +++ b/app/notify_client/invite_api_client.py @@ -23,7 +23,7 @@ class InviteApiClient(BaseAPIClient): 'from_user': invite_from_id, 'permissions': permissions } - _attach_current_user(data) + data = _attach_current_user(data) resp = self.post(url='/service/{}/invite'.format(service_id), data=data) return InvitedUser(**resp['data']) @@ -40,7 +40,7 @@ class InviteApiClient(BaseAPIClient): def cancel_invited_user(self, service_id, invited_user_id): data = {'status': 'cancelled'} - _attach_current_user(data) + data = _attach_current_user(data) self.post(url='/service/{0}/invite/{1}'.format(service_id, invited_user_id), data=data) diff --git a/app/notify_client/job_api_client.py b/app/notify_client/job_api_client.py index c3d45c5f5..1111f8c9e 100644 --- a/app/notify_client/job_api_client.py +++ b/app/notify_client/job_api_client.py @@ -32,6 +32,6 @@ class JobApiClient(BaseAPIClient): "original_file_name": original_file_name, "notification_count": notification_count } - _attach_current_user(data) + data = _attach_current_user(data) resp = self.post(url='/service/{}/job'.format(service_id), data=data) return resp['data'] diff --git a/app/notify_client/provider_client.py b/app/notify_client/provider_client.py index 47ed15aab..b2f07a5c9 100644 --- a/app/notify_client/provider_client.py +++ b/app/notify_client/provider_client.py @@ -29,5 +29,5 @@ class ProviderClient(BaseAPIClient): data = { "priority": priority } - _attach_current_user(data) + data = _attach_current_user(data) return self.post(url='/provider-details/{}'.format(provider_id), data=data) diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index a9b855e0d..90fb6e2f4 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -30,7 +30,7 @@ class ServiceAPIClient(NotificationsAPIClient): "restricted": restricted, "email_from": email_from } - _attach_current_user(data) + data = _attach_current_user(data) return self.post("/service", data)['data']['id'] def delete_service(self, service_id): @@ -81,7 +81,8 @@ class ServiceAPIClient(NotificationsAPIClient): """ Update a service. """ - disallowed_attributes = set(kwargs.keys()) - { + data = _attach_current_user(kwargs) + disallowed_attributes = set(data.keys()) - { 'name', 'users', 'message_limit', @@ -89,14 +90,14 @@ class ServiceAPIClient(NotificationsAPIClient): 'restricted', 'email_from', 'reply_to_email_address', - 'sms_sender' + 'sms_sender', + 'created_by' } if disallowed_attributes: raise TypeError('Not allowed to update service attributes: {}'.format( ", ".join(disallowed_attributes) )) - _attach_current_user(kwargs) endpoint = "/service/{0}".format(service_id) return self.post(endpoint, data) @@ -127,7 +128,7 @@ class ServiceAPIClient(NotificationsAPIClient): data.update({ 'subject': subject }) - _attach_current_user(data) + data = _attach_current_user(data) endpoint = "/service/{0}/template".format(service_id) return self.post(endpoint, data) @@ -146,7 +147,7 @@ class ServiceAPIClient(NotificationsAPIClient): data.update({ 'subject': subject }) - _attach_current_user(data) + data = _attach_current_user(data) endpoint = "/service/{0}/template/{1}".format(service_id, id_) return self.post(endpoint, data) @@ -187,7 +188,7 @@ class ServiceAPIClient(NotificationsAPIClient): data = { 'archived': True } - _attach_current_user(data) + data = _attach_current_user(data) return self.post(endpoint, data=data) def find_all_service_email_from(self, user_id=None): diff --git a/tests/app/notify_client/test_job_client.py b/tests/app/notify_client/test_job_client.py index cf0634df6..442997135 100644 --- a/tests/app/notify_client/test_job_client.py +++ b/tests/app/notify_client/test_job_client.py @@ -7,20 +7,19 @@ def test_client_creates_job_data_correctly(mocker, fake_uuid): template_id = fake_uuid original_file_name = 'test.csv' notification_count = 1 + mocker.patch('app.notify_client.current_user', id='1') expected_data = { "id": job_id, "template": template_id, "original_file_name": original_file_name, - "notification_count": 1 + "notification_count": 1, + "created_by": '1' } expected_url = '/service/{}/job'.format(service_id) client = JobApiClient() - mock_attach_user = mocker.patch( - 'app.notify_client.job_api_client._attach_current_user', - return_value={'created_by': fake_uuid}) mock_post = mocker.patch('app.notify_client.job_api_client.JobApiClient.post') client.create_job(job_id, service_id, template_id, original_file_name, notification_count) diff --git a/tests/app/notify_client/test_service_api_client.py b/tests/app/notify_client/test_service_api_client.py index 95a6e8e50..9fa6cd4d9 100644 --- a/tests/app/notify_client/test_service_api_client.py +++ b/tests/app/notify_client/test_service_api_client.py @@ -7,17 +7,16 @@ from tests.conftest import fake_uuid def test_client_posts_archived_true_when_deleting_template(mocker): service_id = fake_uuid template_id = fake_uuid + mocker.patch('app.notify_client.current_user', id='1') expected_data = { 'archived': True, - 'created_by': fake_uuid + 'created_by': '1' } expected_url = '/service/{}/template/{}'.format(service_id, template_id) client = ServiceAPIClient() mock_post = mocker.patch('app.notify_client.service_api_client.ServiceAPIClient.post') - mock_attach_user = mocker.patch('app.notify_client.service_api_client._attach_current_user', - side_effect=lambda x: x.update({'created_by': fake_uuid})) client.delete_service_template(service_id, template_id) mock_post.assert_called_once_with(expected_url, data=expected_data) @@ -40,6 +39,7 @@ def test_client_gets_service(mocker, function, params): def test_client_only_updates_allowed_attributes(mocker): + mocker.patch('app.notify_client.current_user', id='1') with pytest.raises(TypeError) as error: ServiceAPIClient().update_service('service_id', foo='bar') assert str(error.value) == 'Not allowed to update service attributes: foo' From 955566b127e129df9ce9bab48002a9a77fd744a6 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 11 Aug 2016 17:10:10 +0100 Subject: [PATCH 4/4] =?UTF-8?q?Don=E2=80=99t=20allow=20changing=20service?= =?UTF-8?q?=20users=20on=20update?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/notify_client/service_api_client.py | 1 - 1 file changed, 1 deletion(-) diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index 90fb6e2f4..718ef3946 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -84,7 +84,6 @@ class ServiceAPIClient(NotificationsAPIClient): data = _attach_current_user(kwargs) disallowed_attributes = set(data.keys()) - { 'name', - 'users', 'message_limit', 'active', 'restricted',