Merge pull request #3166 from alphagov/email-auth-broadcast-bug

Email auth broadcast bug

app/dao/broadcast_service_dao.py

@@ -3,7 +3,7 @@ from datetime import datetime
 from flask import current_app
 
 from app import db
-from app.models import ServiceBroadcastSettings, ServicePermission, Organisation, BROADCAST_TYPE
+from app.models import ServiceBroadcastSettings, ServicePermission, Organisation, BROADCAST_TYPE, EMAIL_AUTH_TYPE
 from app.dao.dao_utils import transactional
 
 
@@ -20,7 +20,11 @@ def set_broadcast_service_type(service, service_mode, broadcast_channel, provide
 
     ServicePermission.query.filter(
         ServicePermission.service_id == service.id,
-        ServicePermission.permission != BROADCAST_TYPE
+        ServicePermission.permission != BROADCAST_TYPE,
+        # Email auth is an exception to the other service permissions (which relate to what type
+        # of notifications a service can send) where a broadcast service is allowed to have the
+        # email auth permission (but doesn't have to)
+        ServicePermission.permission != EMAIL_AUTH_TYPE
     ).delete()
 
     # Refresh the service object as it has references to the service permissions but we don't yet

tests/app/service/test_rest.py

@@ -35,8 +35,10 @@ from app.models import (
     INTERNATIONAL_LETTERS,
     INTERNATIONAL_SMS_TYPE,
     INBOUND_SMS_TYPE,
+    EMAIL_AUTH_TYPE,
     NOTIFICATION_RETURNED_LETTER,
     UPLOAD_LETTERS,
+    SERVICE_PERMISSION_TYPES,
 )
 from tests import create_authorization_header
 from tests.app.db import (
@@ -948,6 +950,8 @@ def test_update_service_permissions_will_add_service_permissions(client, sample_
         (INTERNATIONAL_SMS_TYPE),
         (LETTER_TYPE),
         (INBOUND_SMS_TYPE),
+        (EMAIL_AUTH_TYPE),
+        (BROADCAST_TYPE),  # TODO: remove this ability to set broadcast permission this way
     ]
 )
 def test_add_service_permission_will_add_permission(client, service_with_no_permissions, permission_to_add):
@@ -3743,13 +3747,15 @@ def test_set_as_broadcast_service_rejects_if_no_channel(
     )
 
 
-def test_set_as_broadcast_service_gives_broadcast_permission_and_removes_other_permissions(
-    admin_request, sample_service, broadcast_organisation
+@pytest.mark.parametrize('starting_permissions, ending_permissions', (
+    ([], [BROADCAST_TYPE]),
+    ([EMAIL_AUTH_TYPE], [BROADCAST_TYPE, EMAIL_AUTH_TYPE]),
+    ([p for p in SERVICE_PERMISSION_TYPES if p != BROADCAST_TYPE], [BROADCAST_TYPE, EMAIL_AUTH_TYPE]),
+))
+def test_set_as_broadcast_service_gives_broadcast_permission_and_removes_other_channel_permissions(
+    admin_request, broadcast_organisation, starting_permissions, ending_permissions
 ):
-    current_permissions = [p.permission for p in sample_service.permissions]
-    assert len(current_permissions) > 0
-    assert BROADCAST_TYPE not in current_permissions
-
+    sample_service = create_service(service_permissions=starting_permissions)
     data = {
         'broadcast_channel': "severe",
         'service_mode': 'training',
@@ -3761,17 +3767,25 @@ def test_set_as_broadcast_service_gives_broadcast_permission_and_removes_other_p
         service_id=sample_service.id,
         _data=data,
     )
-    assert result['data']['permissions'] == [BROADCAST_TYPE]
+    assert set(result['data']['permissions']) == set(ending_permissions)
 
     permissions = ServicePermission.query.filter_by(service_id=sample_service.id).all()
-    assert [p.permission for p in permissions] == [BROADCAST_TYPE]
+    assert set([p.permission for p in permissions]) == set(ending_permissions)
 
 
+@pytest.mark.parametrize('has_email_auth, ending_permissions', (
+    (False, [BROADCAST_TYPE]),
+    (True, [BROADCAST_TYPE, EMAIL_AUTH_TYPE]),
+))
 def test_set_as_broadcast_service_maintains_broadcast_permission_for_existing_broadcast_service(
-    admin_request, sample_broadcast_service
+    admin_request, sample_broadcast_service, has_email_auth, ending_permissions
 ):
+    if has_email_auth:
+        service_permission = ServicePermission(service_id=sample_broadcast_service.id, permission=EMAIL_AUTH_TYPE)
+        sample_broadcast_service.permissions.append(service_permission)
+
     current_permissions = [p.permission for p in sample_broadcast_service.permissions]
-    assert current_permissions == [BROADCAST_TYPE]
+    assert set(current_permissions) == set(ending_permissions)
 
     data = {
         'broadcast_channel': "severe",
@@ -3784,10 +3798,10 @@ def test_set_as_broadcast_service_maintains_broadcast_permission_for_existing_br
         service_id=sample_broadcast_service.id,
         _data=data,
     )
-    assert result['data']['permissions'] == [BROADCAST_TYPE]
+    assert set(result['data']['permissions']) == set(ending_permissions)
 
     permissions = ServicePermission.query.filter_by(service_id=sample_broadcast_service.id).all()
-    assert [p.permission for p in permissions] == [BROADCAST_TYPE]
+    assert set([p.permission for p in permissions]) == set(ending_permissions)
 
 
 def test_set_as_broadcast_service_sets_count_as_live_to_false(