diff --git a/app/schemas.py b/app/schemas.py index d9d71c991..2045f9c31 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -25,7 +25,7 @@ from notifications_utils.recipients import ( from app import ma from app import models -from app.models import ServicePermission, INTERNATIONAL_SMS_TYPE, SMS_TYPE, LETTER_TYPE +from app.models import ServicePermission, INTERNATIONAL_SMS_TYPE, SMS_TYPE, LETTER_TYPE, EMAIL_TYPE from app.dao.permissions_dao import permission_dao from app.dao.service_permissions_dao import dao_fetch_service_permissions from app.utils import get_template_instance @@ -181,6 +181,7 @@ class ServiceSchema(BaseSchema): branding = field_for(models.Service, 'branding') dvla_organisation = field_for(models.Service, 'dvla_organisation') permissions = fields.Method("service_permissions") + override_flag = False def service_permissions(self, service): permissions = [] @@ -195,8 +196,8 @@ class ServiceSchema(BaseSchema): permissions.append(permission) str_permissions.append(p.permission) - def process_deprecated_permission_flags(): - def sync_flags(flag, notify_type): + def deprecate_convert_flags_to_permissions(): + def convert_flags(flag, notify_type): if flag and notify_type not in str_permissions: permission = { "service_id": service.id, @@ -210,10 +211,10 @@ class ServiceSchema(BaseSchema): } permissions.remove(permission) - sync_flags(service.can_send_international_sms, INTERNATIONAL_SMS_TYPE) - sync_flags(service.can_send_letters, LETTER_TYPE) + convert_flags(service.can_send_international_sms, INTERNATIONAL_SMS_TYPE) + convert_flags(service.can_send_letters, LETTER_TYPE) - process_deprecated_permission_flags() + deprecate_convert_flags_to_permissions() return permissions @@ -248,24 +249,47 @@ class ServiceSchema(BaseSchema): duplicates = list(set([x for x in permissions if permissions.count(x) > 1])) raise ValueError('Service Permission duplicated: {}'.format(duplicates)) - if INTERNATIONAL_SMS_TYPE in permissions and SMS_TYPE not in permissions: - raise ValueError('International SMS must have SMS enabled') - @pre_load() - def format_permissions_for_data_model(self, in_data): + def format_for_data_model(self, in_data): if isinstance(in_data, dict) and 'permissions' in in_data: + str_permissions = in_data['permissions'] permissions = [] for p in in_data['permissions']: - permission = models.ServicePermission(service_id=in_data["id"], permission=p) + permission = ServicePermission(service_id=in_data["id"], permission=p) permissions.append(permission) in_data['permissions'] = permissions + def deprecate_override_flags(): + in_data['can_send_letters'] = LETTER_TYPE in [p.permission for p in permissions] + in_data['can_send_international_sms'] = INTERNATIONAL_SMS_TYPE in [p.permission for p in permissions] + + def deprecate_convert_flags_to_permissions(): + def convert_flag(flag, notify_type): + if flag and notify_type not in str_permissions: + permission = ServicePermission(service_id=in_data['id'], permission=notify_type) + permissions.append(permission) + elif flag is False and notify_type in str_permissions: + for p in permissions: + if p.permission == notify_type: + permissions.remove(p) + + convert_flag(in_data["can_send_international_sms"], INTERNATIONAL_SMS_TYPE) + convert_flag(in_data["can_send_letters"], LETTER_TYPE) + + if self.override_flag: + deprecate_override_flags() + else: + deprecate_convert_flags_to_permissions() + @post_dump - def format_permissions_as_string_array(self, in_data): + def format_as_string_array(self, in_data): if isinstance(in_data, dict) and 'permissions' in in_data: in_data['permissions'] = [p.get("permission") for p in in_data['permissions']] return in_data + def set_override_flag(self, flag): + self.override_flag = flag + class DetailedServiceSchema(BaseSchema): statistics = fields.Dict() diff --git a/app/service/rest.py b/app/service/rest.py index de1450e09..b3977f2b8 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -128,6 +128,7 @@ def update_service(service_id): service_going_live = fetched_service.restricted and not request.get_json().get('restricted', True) current_data = dict(service_schema.dump(fetched_service).data.items()) + service_schema.set_override_flag(request.get_json().get('permissions') is not None) current_data.update(request.get_json()) try: update_dict = service_schema.load(current_data).data diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index be0948bed..4977223eb 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -22,7 +22,7 @@ from tests.app.conftest import ( ) from app.models import ( ServicePermission, - KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, + KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INBOUND_SMS_TYPE ) @@ -473,26 +473,29 @@ def test_update_service_flags(client, sample_service): assert result['data']['can_send_international_sms'] is True -def test_update_service_flags_will_add_service_permissions(client, sample_service): +@pytest.fixture(scope='function') +def service_with_no_permissions(notify_db, notify_db_session): + return create_service(notify_db, notify_db_session, permissions=[]) + + +def test_update_service_flags_with_service_without_default_service_permissions(client, service_with_no_permissions): auth_header = create_authorization_header() data = { - 'research_mode': True, 'can_send_letters': True, 'can_send_international_sms': True, } resp = client.post( - '/service/{}'.format(sample_service.id), + '/service/{}'.format(service_with_no_permissions.id), data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header] ) result = json.loads(resp.get_data(as_text=True)) assert resp.status_code == 200 - assert result['data']['research_mode'] is True assert result['data']['can_send_letters'] is True assert result['data']['can_send_international_sms'] is True - assert set(result['data']['permissions']) == set([SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE]) + assert set(result['data']['permissions']) == set([LETTER_TYPE, INTERNATIONAL_SMS_TYPE]) def test_update_service_flags_will_remove_service_permissions(client, notify_db, notify_db_session): @@ -504,7 +507,7 @@ def test_update_service_flags_will_remove_service_permissions(client, notify_db, assert set([p.permission for p in service.permissions]) == set([SMS_TYPE, EMAIL_TYPE, INTERNATIONAL_SMS_TYPE]) data = { - 'can_send_international_sms': False, + 'can_send_international_sms': False } resp = client.post( @@ -519,35 +522,31 @@ def test_update_service_flags_will_remove_service_permissions(client, notify_db, assert set(result['data']['permissions']) == set([SMS_TYPE, EMAIL_TYPE]) -def test_update_permissions_can_add_service_permissions(client, sample_service): +def test_update_permissions_will_override_permission_flags(client, service_with_no_permissions): auth_header = create_authorization_header() data = { - 'research_mode': True, - 'can_send_international_sms': True, - 'can_send_letters': True, - 'permissions': [EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE] + 'permissions': [LETTER_TYPE, INTERNATIONAL_SMS_TYPE] } resp = client.post( - '/service/{}'.format(sample_service.id), + '/service/{}'.format(service_with_no_permissions.id), data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header] ) result = json.loads(resp.get_data(as_text=True)) assert resp.status_code == 200 - assert result['data']['research_mode'] is True assert result['data']['can_send_letters'] is True assert result['data']['can_send_international_sms'] is True - assert set(result['data']['permissions']) == set([SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE]) + assert set(result['data']['permissions']) == set([LETTER_TYPE, INTERNATIONAL_SMS_TYPE]) -def test_add_inbound_permissions_will_add_service_permissions(client, sample_service): +def test_update_service_permissions_will_add_service_permissions(client, sample_service): auth_header = create_authorization_header() data = { - 'permissions': [EMAIL_TYPE, SMS_TYPE, INBOUND_SMS_TYPE] + 'permissions': [EMAIL_TYPE, SMS_TYPE, LETTER_TYPE] } resp = client.post( @@ -558,7 +557,40 @@ def test_add_inbound_permissions_will_add_service_permissions(client, sample_ser result = json.loads(resp.get_data(as_text=True)) assert resp.status_code == 200 - assert set(result['data']['permissions']) == set([SMS_TYPE, EMAIL_TYPE, INBOUND_SMS_TYPE]) + assert set(result['data']['permissions']) == set([SMS_TYPE, EMAIL_TYPE, LETTER_TYPE]) + + +@pytest.mark.parametrize( + 'permission_to_add', + [ + (EMAIL_TYPE), + (SMS_TYPE), + (INTERNATIONAL_SMS_TYPE), + (LETTER_TYPE), + (INBOUND_SMS_TYPE), + ] +) +def test_add_service_permission_will_add_permission(client, service_with_no_permissions, permission_to_add): + auth_header = create_authorization_header() + + data = { + 'permissions': [permission_to_add] + } + + resp = client.post( + '/service/{}'.format(service_with_no_permissions.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header] + ) + + resp = client.get( + '/service/{}'.format(service_with_no_permissions.id), + headers=[auth_header] + ) + result = json.loads(resp.get_data(as_text=True)) + + assert resp.status_code == 200 + assert result['data']['permissions'] == [permission_to_add] def test_update_permissions_with_an_invalid_permission_will_raise_error(client, sample_service): @@ -600,26 +632,6 @@ def test_update_permissions_with_duplicate_permissions_will_raise_error(client, assert "Service Permission duplicated: ['{}']".format(LETTER_TYPE) in result['message'] -def test_update_permissions_with_international_sms_without_sms_permissions_will_raise_error(client, sample_service): - auth_header = create_authorization_header() - - data = { - 'can_send_international_sms': True, - 'permissions': [EMAIL_TYPE, INTERNATIONAL_SMS_TYPE] - } - - resp = client.post( - '/service/{}'.format(sample_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header] - ) - result = json.loads(resp.get_data(as_text=True)) - - assert resp.status_code == 400 - assert result['result'] == 'error' - assert "International SMS must have SMS enabled" == result['message'] - - def test_update_service_research_mode_throws_validation_error(notify_api, sample_service): with notify_api.test_request_context(): with notify_api.test_client() as client: