From e29a477eb1943b9f0c2d0dd59e69bed8113b4fbf Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 16 Jul 2020 17:21:27 +0100 Subject: [PATCH] Fix setting of broadcast permission MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This was broken because current_service doesn’t update itself after calling the `update` method of the API. So we thought we were changing the permissions like this: ``` {'email', 'sms', 'letter'} {'email', 'sms', 'letter', 'broadcast'} {'sms', 'letter', 'broadcast'} {'letter', 'broadcast'} {'broadcast'} ``` But actually we were doing this: ``` {'email', 'sms', 'letter'} {'email', 'sms', 'letter', 'broadcast'} {'sms', 'letter'} {'email', 'letter'} {'email', 'sms'} ``` This commit changes the code to update the permissions like this: ``` {'email', 'sms', 'letter'} {'broadcast'} ``` It does so by adding a new method to the service model which changes all the permissions in one API call, and updates the tests to mock the underlying API call, not the method on the model. --- app/main/views/service_settings.py | 5 +- app/models/service.py | 5 ++ .../test_service_setting_permissions.py | 90 ++++++++++++------- 3 files changed, 64 insertions(+), 36 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index c4e695a6d..043fc68c2 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -318,10 +318,7 @@ def service_set_broadcast_permission(service_id): if form.validate_on_submit(): if form.enabled.data: - current_service.force_permission('broadcast', on=True) - current_service.force_permission('email', on=False) - current_service.force_permission('sms', on=False) - current_service.force_permission('letter', on=False) + current_service.force_broadcast_permission_on() else: current_service.force_permission('broadcast', on=False) diff --git a/app/models/service.py b/app/models/service.py index 602357d38..876aa2a39 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -99,6 +99,11 @@ class Service(JSONModel): permissions | permission if on else permissions - permission, ) + def force_broadcast_permission_on(self): + return self.update_permissions( + set(self.permissions) - {'email', 'sms', 'letter'} | {'broadcast'} + ) + def update_permissions(self, permissions): return self.update(permissions=list(permissions)) 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 8c88d630b..d68c6bfd1 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 @@ -1,5 +1,4 @@ import functools -from unittest.mock import call import pytest from flask import url_for @@ -39,34 +38,55 @@ def test_service_set_permission_requires_platform_admin( ) -@pytest.mark.parametrize('permission, form_data, expected_calls', [ - ('inbound_sms', 'True', [ - call('inbound_sms', on=True), - ]), - ('inbound_sms', 'False', [ - call('inbound_sms', on=False), - ]), - ('email_auth', 'True', [ - call('email_auth', on=True), - ]), - ('email_auth', 'False', [ - call('email_auth', on=False), - ]), - ('international_letters', 'True', [ - call('international_letters', on=True), - ]), - ('international_letters', 'False', [ - call('international_letters', on=False), - ]), - ('broadcast', 'True', [ - call('broadcast', on=True), - call('email', on=False), - call('sms', on=False), - call('letter', on=False), - ]), - ('broadcast', 'False', [ - call('broadcast', on=False), - ]), +@pytest.mark.parametrize('initial_permissions, permission, form_data, expected_update', [ + ( + [], + 'inbound_sms', + 'True', + ['inbound_sms'], + ), + ( + ['inbound_sms'], + 'inbound_sms', + 'False', + [], + ), + ( + [], + 'email_auth', + 'True', + ['email_auth'], + ), + ( + ['email_auth'], + 'email_auth', + 'False', + [], + ), + ( + [], + 'international_letters', + 'True', + ['international_letters'], + ), + ( + ['international_letters'], + 'international_letters', + 'False', + [], + ), + ( + ['email', 'sms', 'letter', 'international_sms', 'international_letters'], + 'broadcast', + 'True', + ['international_sms', 'international_letters', 'broadcast'], + ), + ( + ['broadcast', 'international_sms', 'international_letters'], + 'broadcast', + 'False', + ['international_sms', 'international_letters'], + ), ]) def test_service_set_permission( mocker, @@ -74,17 +94,23 @@ def test_service_set_permission( service_one, mock_get_inbound_number_for_service, permission, + initial_permissions, form_data, - expected_calls, + expected_update, ): - force_permission = mocker.patch('app.models.service.Service.force_permission') + service_one['permissions'] = initial_permissions + mock_update_service = mocker.patch('app.service_api_client.update_service') response = platform_admin_client.post( url_for('main.service_set_permission', service_id=service_one['id'], permission=permission), data={'enabled': form_data} ) assert response.status_code == 302 assert response.location == url_for('main.service_settings', service_id=service_one['id'], _external=True) - assert force_permission.call_args_list == expected_calls + + assert mock_update_service.call_args[0][0] == service_one['id'] + new_permissions = mock_update_service.call_args[1]['permissions'] + assert len(new_permissions) == len(set(new_permissions)) + assert set(new_permissions) == set(expected_update) @pytest.mark.parametrize('service_fields, endpoint, kwargs, text', [