From e8a94422e5658008560c52d3f8413aece351e94f Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Fri, 3 Nov 2017 16:44:33 +0000
Subject: [PATCH] Make SMS prefix setting non-nullable
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

We have now:
- defaulted new services to start with this column set to `true`
- migrated all preexisting services[1] to have either `true` or `false` set
  for this column

There is no way for a service to switch back from `true`/`false` to
`null`.

This means that we can safely enforce a non-nullable constraint on this
column now.

1. There is a little gotcha: the GOV.UK Notify service still relies on
   the `sms_sender` column. It doesn’t have a row in the
   `service_sms_senders` table. This means the previous migration
   never changed this service’s value for `prefix_sms` from `null`. So
   this commit also changes its value to `False`, so that the rest of
   the migration, of the whole column, is possible.
---
 .../versions/0140_sms_prefix_non_nullable.py  | 46 +++++++++++++++++++
 tests/app/conftest.py                         |  2 +-
 tests/app/db.py                               |  2 +-
 tests/app/service/test_rest.py                | 13 +++++-
 4 files changed, 60 insertions(+), 3 deletions(-)
 create mode 100644 migrations/versions/0140_sms_prefix_non_nullable.py

diff --git a/migrations/versions/0140_sms_prefix_non_nullable.py b/migrations/versions/0140_sms_prefix_non_nullable.py
new file mode 100644
index 000000000..f65e0a70e
--- /dev/null
+++ b/migrations/versions/0140_sms_prefix_non_nullable.py
@@ -0,0 +1,46 @@
+"""
+
+Revision ID: 0140_sms_prefix_non_nullable
+Revises: 0139_migrate_sms_allowance_data
+Create Date: 2017-11-07 13:04:04.077142
+
+"""
+from alembic import op
+from flask import current_app
+import sqlalchemy as sa
+from sqlalchemy.dialects import postgresql
+
+revision = '0140_sms_prefix_non_nullable'
+down_revision = '0139_migrate_sms_allowance_data'
+
+
+def upgrade():
+
+    op.execute("""
+        update services
+        set prefix_sms = false
+        where id = '{}'
+    """.format(current_app.config['NOTIFY_SERVICE_ID']))
+
+    op.alter_column(
+        'services',
+        'prefix_sms',
+        existing_type=sa.BOOLEAN(),
+        nullable=False,
+    )
+
+
+def downgrade():
+
+    op.alter_column(
+        'services',
+        'prefix_sms',
+        existing_type=sa.BOOLEAN(),
+        nullable=True,
+    )
+
+    op.execute("""
+        update services
+        set prefix_sms = null
+        where id = '{}'
+    """.format(current_app.config['NOTIFY_SERVICE_ID']))
diff --git a/tests/app/conftest.py b/tests/app/conftest.py
index cecfb5a27..948ba20cb 100644
--- a/tests/app/conftest.py
+++ b/tests/app/conftest.py
@@ -992,7 +992,7 @@ def notify_service(notify_db, notify_db_session):
             'active': True,
             'restricted': False,
             'email_from': 'notify.service',
-            'created_by': user
+            'created_by': user,
         }
         service = Service(**data)
         db.session.add(service)
diff --git a/tests/app/db.py b/tests/app/db.py
index 8a22a748c..0457b3c0a 100644
--- a/tests/app/db.py
+++ b/tests/app/db.py
@@ -68,7 +68,7 @@ def create_service(
     research_mode=False,
     active=True,
     email_from=None,
-    prefix_sms=None,
+    prefix_sms=True,
 ):
     service = Service(
         name=service_name,
diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py
index f7655be72..5e3fdddb6 100644
--- a/tests/app/service/test_rest.py
+++ b/tests/app/service/test_rest.py
@@ -1551,7 +1551,6 @@ def test_prefixing_messages_based_on_prefix_sms(
 @pytest.mark.parametrize('posted_value, stored_value, returned_value', [
     (True, True, True),
     (False, False, False),
-    (None, None, True),
 ])
 def test_set_sms_prefixing_for_service(
     client,
@@ -1581,6 +1580,18 @@ def test_set_sms_prefixing_for_service(
     assert result['data']['sms_sender'] == current_app.config['FROM_NUMBER']
 
 
+def test_set_sms_prefixing_for_service_cant_be_none(
+    admin_request,
+    sample_service,
+):
+    admin_request.post(
+        'service.update_service',
+        service_id=sample_service.id,
+        _data={'prefix_sms': None},
+        _expected_status=500,
+    )
+
+
 @pytest.mark.parametrize('today_only,stats', [
     ('False', {'requested': 2, 'delivered': 1, 'failed': 0}),
     ('True', {'requested': 1, 'delivered': 0, 'failed': 0})