From e8a94422e5658008560c52d3f8413aece351e94f Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 3 Nov 2017 16:44:33 +0000 Subject: [PATCH 1/3] 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}) From 4eaa482b2d5f1ea8c68f6c69570ad56e4e534efe Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 10 Nov 2017 08:42:00 +0000 Subject: [PATCH 2/3] Rewrite existing test to use admin_request fixture Makes it consistent with the other, related, test in this PR. --- tests/app/service/test_rest.py | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 5e3fdddb6..2df3ef0da 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1553,25 +1553,18 @@ def test_prefixing_messages_based_on_prefix_sms( (False, False, False), ]) def test_set_sms_prefixing_for_service( + admin_request, client, sample_service, posted_value, stored_value, returned_value, ): - data = { - 'prefix_sms': posted_value, - } - - auth_header = create_authorization_header() - - resp = client.post( - '/service/{}'.format(sample_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header] + result = admin_request.post( + 'service.update_service', + service_id=sample_service.id, + _data={'prefix_sms': posted_value}, ) - result = json.loads(resp.get_data(as_text=True)) - assert resp.status_code == 200 assert result['data']['prefix_sms'] == stored_value # This derived value will go away eventually, once we’ve done a migration assert result['data']['prefix_sms_with_service_name'] == returned_value From 492462d4f9b132afee969285f328723ce83c72aa Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 10 Nov 2017 08:49:48 +0000 Subject: [PATCH 3/3] Make getting a service return `prefix_sms` column This gives us the confidence to remove references to the computed column in https://github.com/alphagov/notifications-admin/pull/1619 --- tests/app/service/test_rest.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 2df3ef0da..1d4ffe233 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -159,6 +159,7 @@ def test_get_service_by_id(client, sample_service): assert json_resp['data']['dvla_organisation'] == '001' assert json_resp['data']['sms_sender'] == current_app.config['FROM_NUMBER'] assert json_resp['data']['prefix_sms_with_service_name'] is True + assert json_resp['data']['prefix_sms'] is True def test_get_service_by_id_returns_free_sms_limit(client, sample_service):