Merge pull request #3237 from alphagov/null-to-all

Make service_broadcast_settings.provider non-nullable

app/models.py

@@ -560,8 +560,7 @@ class Service(db.Model, Versioned):
 
     def get_available_broadcast_providers(self):
         # There may be future checks here if we add, for example, platform admin level provider killswitches.
-        # NOTE: We are in the middle of changing the value for all allowed_broadcast_provider from `None`to "all"
-        if self.allowed_broadcast_provider and self.allowed_broadcast_provider != ALL_BROADCAST_PROVIDERS:
+        if self.allowed_broadcast_provider != ALL_BROADCAST_PROVIDERS:
             return [x for x in current_app.config['ENABLED_CBCS'] if x == self.allowed_broadcast_provider]
         else:
             return current_app.config['ENABLED_CBCS']
@@ -2557,7 +2556,7 @@ class ServiceBroadcastSettings(db.Model):
     channel = db.Column(
         db.String(255), db.ForeignKey('broadcast_channel_types.name'), nullable=False
     )
-    provider = db.Column(db.String, db.ForeignKey('broadcast_provider_types.name'), nullable=True)
+    provider = db.Column(db.String, db.ForeignKey('broadcast_provider_types.name'), nullable=False)
     created_at = db.Column(db.DateTime, nullable=False, default=datetime.datetime.utcnow)
     updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow)
 

app/service/service_broadcast_settings_schema.py

@@ -6,7 +6,7 @@ service_broadcast_settings_schema = {
     "properties": {
         "broadcast_channel": {"enum": ["test", "severe"]},
         "service_mode": {"enum": ["training", "live"]},
-        "provider_restriction": {"enum": [None, "three", "o2", "vodafone", "ee", "all"]}
+        "provider_restriction": {"enum": ["three", "o2", "vodafone", "ee", "all"]}
     },
     "required": ["broadcast_channel", "service_mode", "provider_restriction"]
 }

migrations/versions/0353_broadcast_provider_not_null.py

@@ -0,0 +1,22 @@
+"""
+
+Revision ID: 0353_broadcast_provider_not_null
+Revises: 0352_broadcast_provider_types
+Create Date: 2021-05-10 15:06:40.046786
+
+"""
+from alembic import op
+import sqlalchemy as sa
+
+revision = '0353_broadcast_provider_not_null'
+down_revision = '0352_broadcast_provider_types'
+
+
+def upgrade():
+    op.execute("UPDATE service_broadcast_settings SET provider = 'all' WHERE provider is null")
+    op.alter_column('service_broadcast_settings', 'provider', existing_type=sa.VARCHAR(), nullable=False)
+
+
+def downgrade():
+    op.alter_column('service_broadcast_settings', 'provider', existing_type=sa.VARCHAR(), nullable=True)
+    op.execute("UPDATE service_broadcast_settings SET provider = null WHERE provider = 'all'")

tests/app/celery/test_broadcast_message_tasks.py

@@ -30,14 +30,7 @@ from tests.app.db import (
 from tests.conftest import set_config
 
 
-@pytest.mark.parametrize('available_provider', [None, 'all'])
-def test_send_broadcast_event_queues_up_for_active_providers(
-    mocker,
-    notify_api,
-    sample_broadcast_service,
-    available_provider,
-):
-    sample_broadcast_service.allowed_broadcast_provider = available_provider
+def test_send_broadcast_event_queues_up_for_active_providers(mocker, notify_api, sample_broadcast_service):
     template = create_template(sample_broadcast_service, BROADCAST_TYPE)
     broadcast_message = create_broadcast_message(template, status=BroadcastStatusType.BROADCASTING)
     event = create_broadcast_event(broadcast_message)

tests/app/service/test_rest.py

@@ -292,8 +292,6 @@ def test_get_service_by_id(admin_request, sample_service):
 @pytest.mark.parametrize('broadcast_channel,allowed_broadcast_provider', (
     ('test', 'all'),
     ('severe', 'all'),
-    ('test', None),
-    ('severe', None),
     ('test', 'ee'),
     ('severe', 'three'),
 ))
@@ -3719,7 +3717,7 @@ def test_set_as_broadcast_service_sets_broadcast_channel(
     data = {
         'broadcast_channel': channel,
         'service_mode': 'live',
-        'provider_restriction': None,
+        'provider_restriction': "all",
     }
 
     result = admin_request.post(
@@ -3744,7 +3742,7 @@ def test_set_as_broadcast_service_updates_channel_for_broadcast_service(
     data = {
         'broadcast_channel': "test",
         'service_mode': 'training',
-        'provider_restriction': None,
+        'provider_restriction': "all",
     }
 
     result = admin_request.post(
@@ -3768,7 +3766,7 @@ def test_set_as_broadcast_service_rejects_unknown_channels(
     data = {
         'broadcast_channel': channel,
         'service_mode': 'live',
-        'provider_restriction': None,
+        'provider_restriction': "all",
     }
 
     admin_request.post(
@@ -3784,7 +3782,7 @@ def test_set_as_broadcast_service_rejects_if_no_channel(
 ):
     data = {
         'service_mode': 'training',
-        'provider_restriction': None,
+        'provider_restriction': "all",
     }
 
     admin_request.post(
@@ -3807,7 +3805,7 @@ def test_set_as_broadcast_service_gives_broadcast_permission_and_removes_other_c
     data = {
         'broadcast_channel': "severe",
         'service_mode': 'training',
-        'provider_restriction': None,
+        'provider_restriction': "all",
     }
 
     result = admin_request.post(
@@ -3838,7 +3836,7 @@ def test_set_as_broadcast_service_maintains_broadcast_permission_for_existing_br
     data = {
         'broadcast_channel': "severe",
         'service_mode': 'live',
-        'provider_restriction': None,
+        'provider_restriction': "all",
     }
 
     result = admin_request.post(
@@ -3860,7 +3858,7 @@ def test_set_as_broadcast_service_sets_count_as_live_to_false(
     data = {
         'broadcast_channel': "severe",
         'service_mode': 'live',
-        'provider_restriction': None,
+        'provider_restriction': "all",
     }
     result = admin_request.post(
         'service.set_as_broadcast_service',
@@ -3881,7 +3879,7 @@ def test_set_as_broadcast_service_sets_service_org_to_broadcast_org(
     data = {
         'broadcast_channel': "severe",
         'service_mode': 'training',
-        'provider_restriction': None,
+        'provider_restriction': "all",
     }
     result = admin_request.post(
         'service.set_as_broadcast_service',
@@ -3900,7 +3898,7 @@ def test_set_as_broadcast_service_does_not_error_if_run_on_a_service_that_is_alr
     data = {
         'broadcast_channel': "severe",
         'service_mode': "live",
-        'provider_restriction': None,
+        'provider_restriction': "all",
     }
     for _ in range(2):
         admin_request.post(
@@ -3922,7 +3920,7 @@ def test_set_as_broadcast_service_sets_service_to_live_mode(
     data = {
         'broadcast_channel': 'severe',
         'service_mode': 'live',
-        'provider_restriction': None,
+        'provider_restriction': "all",
     }
 
     result = admin_request.post(
@@ -3947,7 +3945,7 @@ def test_set_as_broadcast_service_doesnt_override_existing_go_live_at(
     data = {
         'broadcast_channel': 'severe',
         'service_mode': 'live',
-        'provider_restriction': None,
+        'provider_restriction': "all",
     }
 
     result = admin_request.post(
@@ -3973,7 +3971,7 @@ def test_set_as_broadcast_service_sets_service_to_training_mode(
     data = {
         'broadcast_channel': 'severe',
         'service_mode': 'training',
-        'provider_restriction': None,
+        'provider_restriction': "all",
     }
 
     result = admin_request.post(
@@ -3993,7 +3991,7 @@ def test_set_as_broadcast_service_rejects_unknown_service_mode(
     data = {
         'broadcast_channel': 'severe',
         'service_mode': service_mode,
-        'provider_restriction': None,
+        'provider_restriction': "all",
     }
 
     admin_request.post(
@@ -4009,7 +4007,7 @@ def test_set_as_broadcast_service_rejects_if_no_service_mode(
 ):
     data = {
         'broadcast_channel': 'severe',
-        'provider_restriction': None,
+        'provider_restriction': "all",
     }
 
     admin_request.post(
@@ -4020,7 +4018,7 @@ def test_set_as_broadcast_service_rejects_if_no_service_mode(
     )
 
 
-@pytest.mark.parametrize('provider', [None, "all", "three", "ee", "vodafone", "o2"])
+@pytest.mark.parametrize('provider', ["all", "three", "ee", "vodafone", "o2"])
 def test_set_as_broadcast_service_sets_mobile_provider_restriction(
     admin_request, sample_service, broadcast_organisation, provider
 ):
@@ -4045,7 +4043,7 @@ def test_set_as_broadcast_service_sets_mobile_provider_restriction(
     assert records[0].provider == provider
 
 
-@pytest.mark.parametrize('provider', [None, "vodafone"])
+@pytest.mark.parametrize('provider', ["all", "vodafone"])
 def test_set_as_broadcast_service_updates_mobile_provider_restriction(
     admin_request, notify_db, sample_broadcast_service, provider
 ):
@@ -4116,7 +4114,7 @@ def test_set_as_broadcast_service_updates_services_history(
     data = {
         'broadcast_channel': 'test',
         'service_mode': 'live',
-        'provider_restriction': None,
+        'provider_restriction': "all",
     }
 
     admin_request.post(