Update service permissions to ensure state in sync

app/dao/services_dao.py

@@ -31,7 +31,9 @@ from app.models import (
     TEMPLATE_TYPES,
     JobStatistics,
     SMS_TYPE,
-    EMAIL_TYPE
+    EMAIL_TYPE,
+    INTERNATIONAL_SMS_TYPE,
+    LETTER_TYPE
 )
 from app.service.statistics import format_monthly_template_notification_stats
 from app.statsd_decorators import statsd
@@ -136,10 +138,28 @@ def dao_create_service(service, user, service_id=None, service_permissions=[SMS_
     service.active = True
     service.research_mode = False
 
-    for permission in service_permissions:
+    def process_deprecated_service_permissions():
-        service_permission = ServicePermission(service_id=service.id, permission=permission)
+        for permission in service_permissions:
-        service.permissions.append(service_permission)
+            service_permission = ServicePermission(service_id=service.id, permission=permission)
+            service.permissions.append(service_permission)
 
+            if permission == INTERNATIONAL_SMS_TYPE:
+                service.can_send_international_sms = True
+            if permission == LETTER_TYPE:
+                service.can_send_letters = True
+
+        def sync_flags(flag, notify_type):
+            if flag and notify_type not in service_permissions:
+                service_permission = ServicePermission(service_id=service.id, permission=notify_type)
+                service.permissions.append(service_permission)
+            elif flag is False and notify_type in service_permissions:
+                service_permission = ServicePermission(service_id=service.id, permission=notify_type)
+                service.permissions.remove(service_permission)
+
+        sync_flags(service.can_send_international_sms, INTERNATIONAL_SMS_TYPE)
+        sync_flags(service.can_send_letters, LETTER_TYPE)
+
+    process_deprecated_service_permissions()
     db.session.add(service)
 
 

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
+from app.models import ServicePermission, INTERNATIONAL_SMS_TYPE, SMS_TYPE, LETTER_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
@@ -184,6 +184,7 @@ class ServiceSchema(BaseSchema):
 
     def service_permissions(self, service):
         permissions = []
+        str_permissions = []
 
         perms = dao_fetch_service_permissions(service.id)
         for p in perms:
@@ -192,6 +193,28 @@ class ServiceSchema(BaseSchema):
                 "permission": p.permission
             }
             permissions.append(permission)
+            str_permissions.append(p.permission)
+
+        def process_deprecated_permission_flags():
+            def sync_flags(flag, notify_type):
+                if flag and notify_type not in str_permissions:
+                    permission = {
+                        "service_id": service.id,
+                        "permission": notify_type
+                    }
+                    permissions.append(permission)
+                elif flag is False and notify_type in str_permissions:
+                    permission = {
+                        "service_id": service.id,
+                        "permission": notify_type
+                    }
+                    permissions.remove(permission)
+
+            sync_flags(service.can_send_international_sms, INTERNATIONAL_SMS_TYPE)
+            sync_flags(service.can_send_letters, LETTER_TYPE)
+
+        process_deprecated_permission_flags()
+
         return permissions
 
     class Meta:
@@ -232,7 +255,7 @@ class ServiceSchema(BaseSchema):
     def format_permissions_for_data_model(self, in_data):
         if isinstance(in_data, dict) and 'permissions' in in_data:
             permissions = []
-            for p in in_data.get('permissions'):
+            for p in in_data['permissions']:
                 permission = models.ServicePermission(service_id=in_data["id"], permission=p)
                 permissions.append(permission)
             in_data['permissions'] = permissions
@@ -240,7 +263,7 @@ class ServiceSchema(BaseSchema):
     @post_dump
     def format_permissions_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.get('permissions')]
+            in_data['permissions'] = [p.get("permission") for p in in_data['permissions']]
         return in_data
 
 

tests/app/conftest.py

@@ -24,7 +24,7 @@ from app.models import (
     NotificationStatistics,
     ServiceWhitelist,
     KEY_TYPE_NORMAL, KEY_TYPE_TEST, KEY_TYPE_TEAM,
-    MOBILE_TYPE, EMAIL_TYPE, LETTER_TYPE, NOTIFICATION_STATUS_TYPES_COMPLETED, ScheduledNotification)
+    MOBILE_TYPE, EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, NOTIFICATION_STATUS_TYPES_COMPLETED, ScheduledNotification)
 from app.dao.users_dao import (create_user_code, create_secret_code)
 from app.dao.services_dao import (dao_create_service, dao_add_user_to_service)
 from app.dao.templates_dao import dao_create_template
@@ -124,7 +124,8 @@ def sample_service(
     restricted=False,
     limit=1000,
     email_from=None,
-    can_send_international_sms=False
+    can_send_international_sms=False,
+    permissions=[SMS_TYPE, EMAIL_TYPE]
 ):
     if user is None:
         user = create_user()
@@ -142,7 +143,7 @@ def sample_service(
     service = Service.query.filter_by(name=service_name).first()
     if not service:
         service = Service(**data)
-        dao_create_service(service, user)
+        dao_create_service(service, user, service_permissions=permissions)
     else:
         if user not in service.users:
             dao_add_user_to_service(service, user)

tests/app/service/test_rest.py

@@ -21,6 +21,7 @@ from tests.app.conftest import (
     sample_notification_with_job
 )
 from app.models import (
+    ServicePermission,
     KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE
 )
 
@@ -157,7 +158,7 @@ def test_get_service_list_has_default_permissions(client, service_factory):
     assert response.status_code == 200
     json_resp = json.loads(response.get_data(as_text=True))
     assert len(json_resp['data']) == 3
-    assert all([set(json['permissions']) & set([EMAIL_TYPE, SMS_TYPE]) for json in json_resp['data']])
+    assert all([set(json['permissions']) == set([EMAIL_TYPE, SMS_TYPE]) for json in json_resp['data']])
 
 
 def test_get_service_by_id_has_default_permissions(client, sample_service):
@@ -490,18 +491,41 @@ def test_update_service_flags_will_add_service_permissions(client, sample_servic
     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 all(set(result['data']['permissions']) & set([SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE]))
+    assert set(result['data']['permissions']) == set([SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE])
+
+
+def test_update_service_flags_will_remove_service_permissions(client, notify_db, notify_db_session):
+    auth_header = create_authorization_header()
+
+    service = create_service(notify_db, notify_db_session, can_send_international_sms=True)
+
+    assert service.can_send_international_sms is True
+    assert set([p.permission for p in service.permissions]) == set([SMS_TYPE, EMAIL_TYPE, INTERNATIONAL_SMS_TYPE])
+
+    data = {
+        'can_send_international_sms': False,
+    }
+
+    resp = client.post(
+        '/service/{}'.format(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 == 200
+    assert result['data']['can_send_international_sms'] is False
+    assert set(result['data']['permissions']) == set([SMS_TYPE, EMAIL_TYPE])
 
 
 def test_update_permissions_can_add_service_permissions(client, sample_service):
-    from app.models import ServicePermission
     auth_header = create_authorization_header()
 
     data = {
         'research_mode': True,
-        'can_send_letters': True,
         'can_send_international_sms': True,
-        'permissions': [EMAIL_TYPE, SMS_TYPE, LETTER_TYPE]
+        'can_send_letters': True,
+        'permissions': [EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE]
     }
 
     resp = client.post(
@@ -515,7 +539,7 @@ def test_update_permissions_can_add_service_permissions(client, sample_service):
     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 all(set(result['data']['permissions']) & set([SMS_TYPE, EMAIL_TYPE, LETTER_TYPE]))
+    assert set(result['data']['permissions']) == set([SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE])
 
 
 def test_update_permissions_with_an_invalid_permission_will_raise_error(client, sample_service):
@@ -523,9 +547,6 @@ def test_update_permissions_with_an_invalid_permission_will_raise_error(client,
     invalid_permission = 'invalid_permission'
 
     data = {
-        'research_mode': True,
-        'can_send_letters': True,
-        'can_send_international_sms': True,
         'permissions': [EMAIL_TYPE, SMS_TYPE, invalid_permission]
     }
 
@@ -579,40 +600,6 @@ def test_update_permissions_with_international_sms_without_sms_permissions_will_
     assert "International SMS must have SMS enabled" == result['message']
 
 
-def test_update_service_flags_can_remove_service_permissions(client, sample_service):
-    auth_header = create_authorization_header()
-    initial_data = {
-        'research_mode': True,
-        'can_send_letters': True,
-        'can_send_international_sms': True,
-        'permissions': [EMAIL_TYPE, SMS_TYPE]
-    }
-
-    resp = client.post(
-        '/service/{}'.format(sample_service.id),
-        data=json.dumps(initial_data),
-        headers=[('Content-Type', 'application/json'), auth_header]
-    )
-
-    remove_permission_data = {
-        'can_send_letters': True,
-        'can_send_international_sms': False,
-        'permissions': [EMAIL_TYPE]
-    }
-
-    resp = client.post(
-        '/service/{}'.format(sample_service.id),
-        data=json.dumps(remove_permission_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']['can_send_letters'] is True
-    assert result['data']['can_send_international_sms'] is False
-    assert result['data']['permissions'] == [EMAIL_TYPE]
-
-
 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: