From 28ef9a185322ae683dcbeb6a3ae50d3099acd9ef Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 22 May 2017 10:58:41 +0100 Subject: [PATCH 01/11] Refactored service permissisons data model --- app/models.py | 39 +++++++++++-------- .../0085_update_incoming_to_inbound.py | 28 +++++++++++++ tests/app/dao/test_service_permissions_dao.py | 6 +-- 3 files changed, 54 insertions(+), 19 deletions(-) create mode 100644 migrations/versions/0085_update_incoming_to_inbound.py diff --git a/app/models.py b/app/models.py index 5fff42027..de42129f0 100644 --- a/app/models.py +++ b/app/models.py @@ -3,13 +3,14 @@ import uuid import datetime from flask import url_for, current_app +from sqlalchemy.ext.associationproxy import association_proxy from sqlalchemy.ext.hybrid import hybrid_property from sqlalchemy.dialects.postgresql import ( UUID, JSON ) from sqlalchemy import UniqueConstraint, and_ -from sqlalchemy.orm import foreign, remote +from sqlalchemy.orm import backref, foreign, remote from notifications_utils.recipients import ( validate_email_address, validate_phone_number, @@ -144,9 +145,9 @@ class DVLAOrganisation(db.Model): INTERNATIONAL_SMS_TYPE = 'international_sms' -INCOMING_SMS_TYPE = 'incoming_sms' +INBOUND_SMS_TYPE = 'inbound_sms' -SERVICE_PERMISSION_TYPES = [EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE] +SERVICE_PERMISSION_TYPES = [EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INBOUND_SMS_TYPE] class ServicePermissionTypes(db.Model): @@ -155,18 +156,6 @@ class ServicePermissionTypes(db.Model): name = db.Column(db.String(255), primary_key=True) -class ServicePermission(db.Model): - __tablename__ = "service_permissions" - - service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), - primary_key=True, index=True, nullable=False) - service = db.relationship('Service') - permission = db.Column(db.String(255), db.ForeignKey('service_permission_types.name'), - index=True, primary_key=True, nullable=False) - created_at = db.Column(db.DateTime, default=datetime.datetime.utcnow, nullable=False) - updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) - - class Service(db.Model, Versioned): __tablename__ = 'services' @@ -217,7 +206,8 @@ class Service(db.Model, Versioned): nullable=False, default=BRANDING_GOVUK ) - permissions = db.relationship('ServicePermission') + + association_proxy('permissions', 'service_permission_types') # This is only for backward compatibility and will be dropped when the columns are removed from the data model def set_permissions(self): @@ -226,6 +216,23 @@ class Service(db.Model, Versioned): self.can_send_international_sms = INTERNATIONAL_SMS_TYPE in [p.permission for p in self.permissions] +class ServicePermission(db.Model): + __tablename__ = "service_permissions" + + service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), + primary_key=True, index=True, nullable=False) + permission = db.Column(db.String(255), db.ForeignKey('service_permission_types.name'), + index=True, primary_key=True, nullable=False) + service = db.relationship("Service", foreign_keys=[service_id]) + created_at = db.Column(db.DateTime, default=datetime.datetime.utcnow, nullable=False) + + service_permission_types = db.relationship( + Service, backref=backref("permissions", cascade="all, delete-orphan")) + + def __repr__(self): + return '<{} has service permission: {}>'.format(self.service_id, self.permission) + + MOBILE_TYPE = 'mobile' EMAIL_TYPE = 'email' diff --git a/migrations/versions/0085_update_incoming_to_inbound.py b/migrations/versions/0085_update_incoming_to_inbound.py new file mode 100644 index 000000000..e5d15f064 --- /dev/null +++ b/migrations/versions/0085_update_incoming_to_inbound.py @@ -0,0 +1,28 @@ +"""empty message + +Revision ID: 0085_update_incoming_to_inbound +Revises: 0084_add_job_stats +Create Date: 2017-05-22 10:23:43.939050 + +""" + +# revision identifiers, used by Alembic. +revision = '0085_update_incoming_to_inbound' +down_revision = '0084_add_job_stats' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_column('service_permissions', 'updated_at') + op.execute("UPDATE service_permission_types SET name='inbound_sms' WHERE name='incoming_sms'") + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.add_column('service_permissions', sa.Column('updated_at', postgresql.TIMESTAMP(), autoincrement=False, nullable=True)) + op.execute("UPDATE service_permission_types SET name='incoming_sms' WHERE name='inbound_sms'") + # ### end Alembic commands ### diff --git a/tests/app/dao/test_service_permissions_dao.py b/tests/app/dao/test_service_permissions_dao.py index c41c891f4..50b5559f8 100644 --- a/tests/app/dao/test_service_permissions_dao.py +++ b/tests/app/dao/test_service_permissions_dao.py @@ -1,7 +1,7 @@ import pytest from app.dao.service_permissions_dao import dao_fetch_service_permissions, dao_remove_service_permission -from app.models import EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE +from app.models import EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INBOUND_SMS_TYPE from tests.app.db import create_service_permission, create_service @@ -34,11 +34,11 @@ def test_fetch_service_permissions_gets_service_permissions(service_without_perm def test_remove_service_permission(service_without_permissions): create_service_permission(service_id=service_without_permissions.id, permission=EMAIL_TYPE) - create_service_permission(service_id=service_without_permissions.id, permission=INCOMING_SMS_TYPE) + create_service_permission(service_id=service_without_permissions.id, permission=INBOUND_SMS_TYPE) dao_remove_service_permission(service_without_permissions.id, EMAIL_TYPE) permissions = dao_fetch_service_permissions(service_without_permissions.id) assert len(permissions) == 1 - assert permissions[0].permission == INCOMING_SMS_TYPE + assert permissions[0].permission == INBOUND_SMS_TYPE assert permissions[0].service_id == service_without_permissions.id From 052004bef0e4bbebe305fc399504d2442af80f11 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 22 May 2017 13:33:43 +0100 Subject: [PATCH 02/11] Refactored data model to remove cascade --- app/models.py | 7 +++---- .../versions/0085_update_incoming_to_inbound.py | 4 ---- tests/app/dao/test_services_dao.py | 17 ++++++++++++++++- 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/app/models.py b/app/models.py index de42129f0..378c45b3e 100644 --- a/app/models.py +++ b/app/models.py @@ -10,7 +10,7 @@ from sqlalchemy.dialects.postgresql import ( JSON ) from sqlalchemy import UniqueConstraint, and_ -from sqlalchemy.orm import backref, foreign, remote +from sqlalchemy.orm import foreign, remote from notifications_utils.recipients import ( validate_email_address, validate_phone_number, @@ -223,11 +223,10 @@ class ServicePermission(db.Model): primary_key=True, index=True, nullable=False) permission = db.Column(db.String(255), db.ForeignKey('service_permission_types.name'), index=True, primary_key=True, nullable=False) - service = db.relationship("Service", foreign_keys=[service_id]) + service = db.relationship("Service") created_at = db.Column(db.DateTime, default=datetime.datetime.utcnow, nullable=False) - service_permission_types = db.relationship( - Service, backref=backref("permissions", cascade="all, delete-orphan")) + service_permission_types = db.relationship(Service, backref=db.backref("permissions")) def __repr__(self): return '<{} has service permission: {}>'.format(self.service_id, self.permission) diff --git a/migrations/versions/0085_update_incoming_to_inbound.py b/migrations/versions/0085_update_incoming_to_inbound.py index e5d15f064..ae4388c27 100644 --- a/migrations/versions/0085_update_incoming_to_inbound.py +++ b/migrations/versions/0085_update_incoming_to_inbound.py @@ -15,14 +15,10 @@ import sqlalchemy as sa from sqlalchemy.dialects import postgresql def upgrade(): - # ### commands auto generated by Alembic - please adjust! ### op.drop_column('service_permissions', 'updated_at') op.execute("UPDATE service_permission_types SET name='inbound_sms' WHERE name='incoming_sms'") - # ### end Alembic commands ### def downgrade(): - # ### commands auto generated by Alembic - please adjust! ### op.add_column('service_permissions', sa.Column('updated_at', postgresql.TIMESTAMP(), autoincrement=False, nullable=True)) op.execute("UPDATE service_permission_types SET name='incoming_sms' WHERE name='inbound_sms'") - # ### end Alembic commands ### diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 2dc6f199f..0a682ad54 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -44,6 +44,8 @@ from app.models import ( User, InvitedUser, Service, + ServicePermission, + ServicePermissionTypes, BRANDING_GOVUK, DVLA_ORG_HM_GOVERNMENT, KEY_TYPE_NORMAL, @@ -52,7 +54,8 @@ from app.models import ( EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, - INTERNATIONAL_SMS_TYPE + INTERNATIONAL_SMS_TYPE, + SERVICE_PERMISSION_TYPES ) from tests.app.db import create_user, create_service @@ -286,6 +289,14 @@ def test_remove_permission_from_service_by_id_returns_service_with_correct_permi assert service.permissions[0].permission == EMAIL_TYPE +def test_remove_service_does_not_remove_service_permission_types(sample_service): + delete_service_and_all_associated_db_objects(sample_service) + + services = dao_fetch_all_services() + assert len(services) == 0 + assert set([p.name for p in ServicePermissionTypes.query.all()]) == set(SERVICE_PERMISSION_TYPES) + + def test_create_service_by_id_adding_and_removing_letter_returns_service_without_letter(service_factory): service = service_factory.get('testing', email_from='testing') @@ -392,6 +403,9 @@ def test_delete_service_and_associated_objects(notify_db, sample_invited_user, sample_permission, sample_provider_statistics): + # Default service permissions of Email and SMS + assert ServicePermission.query.count() == 2 + delete_service_and_all_associated_db_objects(sample_service) assert NotificationStatistics.query.count() == 0 assert TemplateStatistics.query.count() == 0 @@ -408,6 +422,7 @@ def test_delete_service_and_associated_objects(notify_db, assert InvitedUser.query.count() == 0 assert Service.query.count() == 0 assert Service.get_history_model().query.count() == 0 + assert ServicePermission.query.count() == 0 def test_add_existing_user_to_another_service_doesnot_change_old_permissions(sample_user): From f5d3eed7e0dc0c0998eebaa0e5471c1b9ee706fa Mon Sep 17 00:00:00 2001 From: kentsanggds Date: Mon, 22 May 2017 16:05:31 +0100 Subject: [PATCH 03/11] Revert "Refactored service permissisons data model" --- app/models.py | 36 ++++++++----------- .../0085_update_incoming_to_inbound.py | 24 ------------- tests/app/dao/test_service_permissions_dao.py | 6 ++-- tests/app/dao/test_services_dao.py | 17 +-------- 4 files changed, 19 insertions(+), 64 deletions(-) delete mode 100644 migrations/versions/0085_update_incoming_to_inbound.py diff --git a/app/models.py b/app/models.py index 378c45b3e..5fff42027 100644 --- a/app/models.py +++ b/app/models.py @@ -3,7 +3,6 @@ import uuid import datetime from flask import url_for, current_app -from sqlalchemy.ext.associationproxy import association_proxy from sqlalchemy.ext.hybrid import hybrid_property from sqlalchemy.dialects.postgresql import ( UUID, @@ -145,9 +144,9 @@ class DVLAOrganisation(db.Model): INTERNATIONAL_SMS_TYPE = 'international_sms' -INBOUND_SMS_TYPE = 'inbound_sms' +INCOMING_SMS_TYPE = 'incoming_sms' -SERVICE_PERMISSION_TYPES = [EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INBOUND_SMS_TYPE] +SERVICE_PERMISSION_TYPES = [EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE] class ServicePermissionTypes(db.Model): @@ -156,6 +155,18 @@ class ServicePermissionTypes(db.Model): name = db.Column(db.String(255), primary_key=True) +class ServicePermission(db.Model): + __tablename__ = "service_permissions" + + service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), + primary_key=True, index=True, nullable=False) + service = db.relationship('Service') + permission = db.Column(db.String(255), db.ForeignKey('service_permission_types.name'), + index=True, primary_key=True, nullable=False) + created_at = db.Column(db.DateTime, default=datetime.datetime.utcnow, nullable=False) + updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) + + class Service(db.Model, Versioned): __tablename__ = 'services' @@ -206,8 +217,7 @@ class Service(db.Model, Versioned): nullable=False, default=BRANDING_GOVUK ) - - association_proxy('permissions', 'service_permission_types') + permissions = db.relationship('ServicePermission') # This is only for backward compatibility and will be dropped when the columns are removed from the data model def set_permissions(self): @@ -216,22 +226,6 @@ class Service(db.Model, Versioned): self.can_send_international_sms = INTERNATIONAL_SMS_TYPE in [p.permission for p in self.permissions] -class ServicePermission(db.Model): - __tablename__ = "service_permissions" - - service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), - primary_key=True, index=True, nullable=False) - permission = db.Column(db.String(255), db.ForeignKey('service_permission_types.name'), - index=True, primary_key=True, nullable=False) - service = db.relationship("Service") - created_at = db.Column(db.DateTime, default=datetime.datetime.utcnow, nullable=False) - - service_permission_types = db.relationship(Service, backref=db.backref("permissions")) - - def __repr__(self): - return '<{} has service permission: {}>'.format(self.service_id, self.permission) - - MOBILE_TYPE = 'mobile' EMAIL_TYPE = 'email' diff --git a/migrations/versions/0085_update_incoming_to_inbound.py b/migrations/versions/0085_update_incoming_to_inbound.py deleted file mode 100644 index ae4388c27..000000000 --- a/migrations/versions/0085_update_incoming_to_inbound.py +++ /dev/null @@ -1,24 +0,0 @@ -"""empty message - -Revision ID: 0085_update_incoming_to_inbound -Revises: 0084_add_job_stats -Create Date: 2017-05-22 10:23:43.939050 - -""" - -# revision identifiers, used by Alembic. -revision = '0085_update_incoming_to_inbound' -down_revision = '0084_add_job_stats' - -from alembic import op -import sqlalchemy as sa -from sqlalchemy.dialects import postgresql - -def upgrade(): - op.drop_column('service_permissions', 'updated_at') - op.execute("UPDATE service_permission_types SET name='inbound_sms' WHERE name='incoming_sms'") - - -def downgrade(): - op.add_column('service_permissions', sa.Column('updated_at', postgresql.TIMESTAMP(), autoincrement=False, nullable=True)) - op.execute("UPDATE service_permission_types SET name='incoming_sms' WHERE name='inbound_sms'") diff --git a/tests/app/dao/test_service_permissions_dao.py b/tests/app/dao/test_service_permissions_dao.py index 50b5559f8..c41c891f4 100644 --- a/tests/app/dao/test_service_permissions_dao.py +++ b/tests/app/dao/test_service_permissions_dao.py @@ -1,7 +1,7 @@ import pytest from app.dao.service_permissions_dao import dao_fetch_service_permissions, dao_remove_service_permission -from app.models import EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INBOUND_SMS_TYPE +from app.models import EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE from tests.app.db import create_service_permission, create_service @@ -34,11 +34,11 @@ def test_fetch_service_permissions_gets_service_permissions(service_without_perm def test_remove_service_permission(service_without_permissions): create_service_permission(service_id=service_without_permissions.id, permission=EMAIL_TYPE) - create_service_permission(service_id=service_without_permissions.id, permission=INBOUND_SMS_TYPE) + create_service_permission(service_id=service_without_permissions.id, permission=INCOMING_SMS_TYPE) dao_remove_service_permission(service_without_permissions.id, EMAIL_TYPE) permissions = dao_fetch_service_permissions(service_without_permissions.id) assert len(permissions) == 1 - assert permissions[0].permission == INBOUND_SMS_TYPE + assert permissions[0].permission == INCOMING_SMS_TYPE assert permissions[0].service_id == service_without_permissions.id diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 0a682ad54..2dc6f199f 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -44,8 +44,6 @@ from app.models import ( User, InvitedUser, Service, - ServicePermission, - ServicePermissionTypes, BRANDING_GOVUK, DVLA_ORG_HM_GOVERNMENT, KEY_TYPE_NORMAL, @@ -54,8 +52,7 @@ from app.models import ( EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, - INTERNATIONAL_SMS_TYPE, - SERVICE_PERMISSION_TYPES + INTERNATIONAL_SMS_TYPE ) from tests.app.db import create_user, create_service @@ -289,14 +286,6 @@ def test_remove_permission_from_service_by_id_returns_service_with_correct_permi assert service.permissions[0].permission == EMAIL_TYPE -def test_remove_service_does_not_remove_service_permission_types(sample_service): - delete_service_and_all_associated_db_objects(sample_service) - - services = dao_fetch_all_services() - assert len(services) == 0 - assert set([p.name for p in ServicePermissionTypes.query.all()]) == set(SERVICE_PERMISSION_TYPES) - - def test_create_service_by_id_adding_and_removing_letter_returns_service_without_letter(service_factory): service = service_factory.get('testing', email_from='testing') @@ -403,9 +392,6 @@ def test_delete_service_and_associated_objects(notify_db, sample_invited_user, sample_permission, sample_provider_statistics): - # Default service permissions of Email and SMS - assert ServicePermission.query.count() == 2 - delete_service_and_all_associated_db_objects(sample_service) assert NotificationStatistics.query.count() == 0 assert TemplateStatistics.query.count() == 0 @@ -422,7 +408,6 @@ def test_delete_service_and_associated_objects(notify_db, assert InvitedUser.query.count() == 0 assert Service.query.count() == 0 assert Service.get_history_model().query.count() == 0 - assert ServicePermission.query.count() == 0 def test_add_existing_user_to_another_service_doesnot_change_old_permissions(sample_user): From 111e9df000cbcf13c34c29b627ad730c63b0305d Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 22 May 2017 16:56:43 +0100 Subject: [PATCH 04/11] Removed drop columns from service_permissions table --- migrations/versions/0085_update_incoming_to_inbound.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/migrations/versions/0085_update_incoming_to_inbound.py b/migrations/versions/0085_update_incoming_to_inbound.py index ae4388c27..ae2f9ae16 100644 --- a/migrations/versions/0085_update_incoming_to_inbound.py +++ b/migrations/versions/0085_update_incoming_to_inbound.py @@ -15,10 +15,8 @@ import sqlalchemy as sa from sqlalchemy.dialects import postgresql def upgrade(): - op.drop_column('service_permissions', 'updated_at') op.execute("UPDATE service_permission_types SET name='inbound_sms' WHERE name='incoming_sms'") def downgrade(): - op.add_column('service_permissions', sa.Column('updated_at', postgresql.TIMESTAMP(), autoincrement=False, nullable=True)) op.execute("UPDATE service_permission_types SET name='incoming_sms' WHERE name='inbound_sms'") From 3744463296997cad7c4292f39239661bff090eca Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 22 May 2017 15:58:19 +0100 Subject: [PATCH 05/11] treat 40604 and GOVUK as not having a sender ID in prep for removing the 40604-as-default, first we need to make sure that if you either have GOVUK or None as your sms sender, then we send GOVUK through to the provider --- app/config.py | 2 +- app/delivery/send_to_providers.py | 5 ++- tests/app/delivery/test_send_to_providers.py | 40 ++++++++++++++++++-- 3 files changed, 40 insertions(+), 7 deletions(-) diff --git a/app/config.py b/app/config.py index b3528cb49..4bdec85b6 100644 --- a/app/config.py +++ b/app/config.py @@ -287,7 +287,7 @@ class Live(Config): NOTIFY_ENVIRONMENT = 'live' CSV_UPLOAD_BUCKET_NAME = 'live-notifications-csv-upload' STATSD_ENABLED = True - FROM_NUMBER = '40604' + FROM_NUMBER = 'GOVUK' FUNCTIONAL_TEST_PROVIDER_SERVICE_ID = '6c1d81bb-dae2-4ee9-80b0-89a4aae9f649' FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID = 'ba9e1789-a804-40b8-871f-cc60d4c1286f' PERFORMANCE_PLATFORM_ENABLED = True diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 0a9aa22f0..df4c664f9 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -23,6 +23,7 @@ from app.celery.statistics_tasks import record_initial_job_statistics, create_in def send_sms_to_provider(notification): service = notification.service + if not service.active: technical_failure(notification=notification) return @@ -37,7 +38,7 @@ def send_sms_to_provider(notification): template_model.__dict__, values=notification.personalisation, prefix=service.name, - sender=service.sms_sender + sender=service.sms_sender not in {None, current_app.config['FROM_NUMBER']} ) if service.research_mode or notification.key_type == KEY_TYPE_TEST: @@ -50,7 +51,7 @@ def send_sms_to_provider(notification): to=validate_and_format_phone_number(notification.to, international=notification.international), content=str(template), reference=str(notification.id), - sender=service.sms_sender + sender=service.sms_sender or current_app.config['FROM_NUMBER'] ) except Exception as e: dao_toggle_sms_provider(provider.name) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 1d243b3e5..5cb467ac3 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -5,6 +5,7 @@ from unittest.mock import ANY, call import pytest from notifications_utils.recipients import validate_and_format_phone_number +from flask import current_app import app from app import mmg_client, firetext_client @@ -73,7 +74,7 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist( to=validate_and_format_phone_number("+447234123123"), content="Sample service: Hello Jo\nHere is some HTML & entities", reference=str(db_notification.id), - sender=None + sender=current_app.config['FROM_NUMBER'] ) stats_mock.assert_called_once_with(db_notification) @@ -175,7 +176,7 @@ def test_send_sms_should_use_template_version_from_notification_not_latest( to=validate_and_format_phone_number("+447234123123"), content="Sample service: This is a template:\nwith a newline", reference=str(db_notification.id), - sender=None + sender=current_app.config['FROM_NUMBER'] ) persisted_notification = notifications_dao.get_notification_by_id(db_notification.id) @@ -549,7 +550,7 @@ def test_should_send_sms_to_international_providers( to="447234123999", content=ANY, reference=str(db_notification_uk.id), - sender=None + sender=current_app.config['FROM_NUMBER'] ) send_to_providers.send_sms_to_provider( @@ -560,7 +561,7 @@ def test_should_send_sms_to_international_providers( to="447234123111", content=ANY, reference=str(db_notification_international.id), - sender=None + sender=current_app.config['FROM_NUMBER'] ) notification_uk = Notification.query.filter_by(id=db_notification_uk.id).one() @@ -619,3 +620,34 @@ def test_should_set_international_phone_number_to_sent_status( ) assert notification.status == 'sent' + + +@pytest.mark.parametrize('sms_sender, expected_sender, expected_content', [ + ('foo', 'foo', 'bar'), + # if 40604 is actually in DB then treat that as if entered manually + ('40604', '40604', 'bar'), + # 'testing' is the FROM_NUMBER during unit tests + (None, 'testing', 'Sample service: bar'), + ('testing', 'testing', 'Sample service: bar'), +]) +def test_should_handle_sms_sender_and_prefix_message( + sample_service, + mocker, + sms_sender, + expected_sender, + expected_content +): + mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.delivery.send_to_providers.create_initial_notification_statistic_tasks') + sample_service.sms_sender = sms_sender + template = create_template(sample_service, content='bar') + notification = create_notification(template) + + send_to_providers.send_sms_to_provider(notification) + + mmg_client.send_sms.assert_called_once_with( + content=expected_content, + sender=expected_sender, + to=ANY, + reference=ANY, + ) From 2535a7fe9839fc8222fa4b0ac4f321e1630fc690 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 22 May 2017 14:43:46 +0100 Subject: [PATCH 06/11] set sms_sender to be 'GOVUK' if not otherwise specified this is a precursor to making the column non-nullable --- app/models.py | 2 +- tests/app/service/test_rest.py | 2 ++ tests/app/v2/notifications/test_post_notifications.py | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/models.py b/app/models.py index 5fff42027..0fe4b0a6e 100644 --- a/app/models.py +++ b/app/models.py @@ -199,7 +199,7 @@ class Service(db.Model, Versioned): created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False) reply_to_email_address = db.Column(db.Text, index=False, unique=False, nullable=True) letter_contact_block = db.Column(db.Text, index=False, unique=False, nullable=True) - sms_sender = db.Column(db.String(11), nullable=True) + sms_sender = db.Column(db.String(11), nullable=True, default='GOVUK') organisation_id = db.Column(UUID(as_uuid=True), db.ForeignKey('organisation.id'), index=True, nullable=True) organisation = db.relationship('Organisation') dvla_organisation_id = db.Column( diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index c12ca4eec..7ab0ba793 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -144,6 +144,7 @@ def test_get_service_by_id(client, sample_service): assert json_resp['data']['organisation'] is None assert json_resp['data']['branding'] == 'govuk' assert json_resp['data']['dvla_organisation'] == '001' + assert json_resp['data']['sms_sender'] == 'GOVUK' def test_get_service_by_id_should_404_if_no_service(notify_api, notify_db): @@ -213,6 +214,7 @@ def test_create_service(client, sample_user): assert json_resp['data']['email_from'] == 'created.service' assert not json_resp['data']['research_mode'] assert json_resp['data']['dvla_organisation'] == '001' + assert json_resp['data']['sms_sender'] == 'GOVUK' auth_header_fetch = create_authorization_header() diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 14f8d6d2e..606f0ed42 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -34,7 +34,7 @@ def test_post_sms_notification_returns_201(notify_api, sample_template_with_plac assert resp_json['reference'] == reference assert resp_json['content']['body'] == sample_template_with_placeholders.content.replace("(( Name))", "Jo") # conftest fixture service does not have a sms sender, use config default - assert resp_json['content']['from_number'] == notify_api.config["FROM_NUMBER"] + assert resp_json['content']['from_number'] == 'GOVUK' assert 'v2/notifications/{}'.format(notification_id) in resp_json['uri'] assert resp_json['template']['id'] == str(sample_template_with_placeholders.id) assert resp_json['template']['version'] == sample_template_with_placeholders.version From 88215f406203d30e12d76dbacf08f6e4360503a1 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 22 May 2017 16:56:43 +0100 Subject: [PATCH 07/11] Removed drop columns from service_permissions table --- .../0085_update_incoming_to_inbound.py | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 migrations/versions/0085_update_incoming_to_inbound.py diff --git a/migrations/versions/0085_update_incoming_to_inbound.py b/migrations/versions/0085_update_incoming_to_inbound.py new file mode 100644 index 000000000..ae2f9ae16 --- /dev/null +++ b/migrations/versions/0085_update_incoming_to_inbound.py @@ -0,0 +1,22 @@ +"""empty message + +Revision ID: 0085_update_incoming_to_inbound +Revises: 0084_add_job_stats +Create Date: 2017-05-22 10:23:43.939050 + +""" + +# revision identifiers, used by Alembic. +revision = '0085_update_incoming_to_inbound' +down_revision = '0084_add_job_stats' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +def upgrade(): + op.execute("UPDATE service_permission_types SET name='inbound_sms' WHERE name='incoming_sms'") + + +def downgrade(): + op.execute("UPDATE service_permission_types SET name='incoming_sms' WHERE name='inbound_sms'") From de7ad6fb95c8684ecacabe2d44e2cac29647cff3 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 22 May 2017 17:25:58 +0100 Subject: [PATCH 08/11] Refactored service_permissions data model --- app/models.py | 36 +++++++++++-------- tests/app/dao/test_service_permissions_dao.py | 6 ++-- tests/app/dao/test_services_dao.py | 17 ++++++++- 3 files changed, 40 insertions(+), 19 deletions(-) diff --git a/app/models.py b/app/models.py index 5fff42027..378c45b3e 100644 --- a/app/models.py +++ b/app/models.py @@ -3,6 +3,7 @@ import uuid import datetime from flask import url_for, current_app +from sqlalchemy.ext.associationproxy import association_proxy from sqlalchemy.ext.hybrid import hybrid_property from sqlalchemy.dialects.postgresql import ( UUID, @@ -144,9 +145,9 @@ class DVLAOrganisation(db.Model): INTERNATIONAL_SMS_TYPE = 'international_sms' -INCOMING_SMS_TYPE = 'incoming_sms' +INBOUND_SMS_TYPE = 'inbound_sms' -SERVICE_PERMISSION_TYPES = [EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE] +SERVICE_PERMISSION_TYPES = [EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INBOUND_SMS_TYPE] class ServicePermissionTypes(db.Model): @@ -155,18 +156,6 @@ class ServicePermissionTypes(db.Model): name = db.Column(db.String(255), primary_key=True) -class ServicePermission(db.Model): - __tablename__ = "service_permissions" - - service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), - primary_key=True, index=True, nullable=False) - service = db.relationship('Service') - permission = db.Column(db.String(255), db.ForeignKey('service_permission_types.name'), - index=True, primary_key=True, nullable=False) - created_at = db.Column(db.DateTime, default=datetime.datetime.utcnow, nullable=False) - updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) - - class Service(db.Model, Versioned): __tablename__ = 'services' @@ -217,7 +206,8 @@ class Service(db.Model, Versioned): nullable=False, default=BRANDING_GOVUK ) - permissions = db.relationship('ServicePermission') + + association_proxy('permissions', 'service_permission_types') # This is only for backward compatibility and will be dropped when the columns are removed from the data model def set_permissions(self): @@ -226,6 +216,22 @@ class Service(db.Model, Versioned): self.can_send_international_sms = INTERNATIONAL_SMS_TYPE in [p.permission for p in self.permissions] +class ServicePermission(db.Model): + __tablename__ = "service_permissions" + + service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), + primary_key=True, index=True, nullable=False) + permission = db.Column(db.String(255), db.ForeignKey('service_permission_types.name'), + index=True, primary_key=True, nullable=False) + service = db.relationship("Service") + created_at = db.Column(db.DateTime, default=datetime.datetime.utcnow, nullable=False) + + service_permission_types = db.relationship(Service, backref=db.backref("permissions")) + + def __repr__(self): + return '<{} has service permission: {}>'.format(self.service_id, self.permission) + + MOBILE_TYPE = 'mobile' EMAIL_TYPE = 'email' diff --git a/tests/app/dao/test_service_permissions_dao.py b/tests/app/dao/test_service_permissions_dao.py index c41c891f4..50b5559f8 100644 --- a/tests/app/dao/test_service_permissions_dao.py +++ b/tests/app/dao/test_service_permissions_dao.py @@ -1,7 +1,7 @@ import pytest from app.dao.service_permissions_dao import dao_fetch_service_permissions, dao_remove_service_permission -from app.models import EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE +from app.models import EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INBOUND_SMS_TYPE from tests.app.db import create_service_permission, create_service @@ -34,11 +34,11 @@ def test_fetch_service_permissions_gets_service_permissions(service_without_perm def test_remove_service_permission(service_without_permissions): create_service_permission(service_id=service_without_permissions.id, permission=EMAIL_TYPE) - create_service_permission(service_id=service_without_permissions.id, permission=INCOMING_SMS_TYPE) + create_service_permission(service_id=service_without_permissions.id, permission=INBOUND_SMS_TYPE) dao_remove_service_permission(service_without_permissions.id, EMAIL_TYPE) permissions = dao_fetch_service_permissions(service_without_permissions.id) assert len(permissions) == 1 - assert permissions[0].permission == INCOMING_SMS_TYPE + assert permissions[0].permission == INBOUND_SMS_TYPE assert permissions[0].service_id == service_without_permissions.id diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 2dc6f199f..0a682ad54 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -44,6 +44,8 @@ from app.models import ( User, InvitedUser, Service, + ServicePermission, + ServicePermissionTypes, BRANDING_GOVUK, DVLA_ORG_HM_GOVERNMENT, KEY_TYPE_NORMAL, @@ -52,7 +54,8 @@ from app.models import ( EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, - INTERNATIONAL_SMS_TYPE + INTERNATIONAL_SMS_TYPE, + SERVICE_PERMISSION_TYPES ) from tests.app.db import create_user, create_service @@ -286,6 +289,14 @@ def test_remove_permission_from_service_by_id_returns_service_with_correct_permi assert service.permissions[0].permission == EMAIL_TYPE +def test_remove_service_does_not_remove_service_permission_types(sample_service): + delete_service_and_all_associated_db_objects(sample_service) + + services = dao_fetch_all_services() + assert len(services) == 0 + assert set([p.name for p in ServicePermissionTypes.query.all()]) == set(SERVICE_PERMISSION_TYPES) + + def test_create_service_by_id_adding_and_removing_letter_returns_service_without_letter(service_factory): service = service_factory.get('testing', email_from='testing') @@ -392,6 +403,9 @@ def test_delete_service_and_associated_objects(notify_db, sample_invited_user, sample_permission, sample_provider_statistics): + # Default service permissions of Email and SMS + assert ServicePermission.query.count() == 2 + delete_service_and_all_associated_db_objects(sample_service) assert NotificationStatistics.query.count() == 0 assert TemplateStatistics.query.count() == 0 @@ -408,6 +422,7 @@ def test_delete_service_and_associated_objects(notify_db, assert InvitedUser.query.count() == 0 assert Service.query.count() == 0 assert Service.get_history_model().query.count() == 0 + assert ServicePermission.query.count() == 0 def test_add_existing_user_to_another_service_doesnot_change_old_permissions(sample_user): From 3d2c12128b9fb829dbb50b04407f45a692c70dc2 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 22 May 2017 17:27:26 +0100 Subject: [PATCH 09/11] Update services test --- tests/app/dao/test_services_dao.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 0a682ad54..ff7b3b54b 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -294,7 +294,7 @@ def test_remove_service_does_not_remove_service_permission_types(sample_service) services = dao_fetch_all_services() assert len(services) == 0 - assert set([p.name for p in ServicePermissionTypes.query.all()]) == set(SERVICE_PERMISSION_TYPES) + assert set([p.name for p in ServicePermissionTypes.query.all()]) & set(SERVICE_PERMISSION_TYPES) def test_create_service_by_id_adding_and_removing_letter_returns_service_without_letter(service_factory): From 86c9600b04443e9307cbf203fd11579081b5689c Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 23 May 2017 10:35:13 +0100 Subject: [PATCH 10/11] use config to get default sender rather than hardcoding this means that on non-prod envs, it reflects that environment. it needs to be a lamdba, because the column object is created at import time, when current_app.config won't have been loaded - this means that when you create a Service object, that lambda executes and grabs the correct default value --- app/models.py | 2 +- tests/app/service/test_rest.py | 6 +++--- tests/app/service/test_sender.py | 3 +-- tests/app/v2/notifications/test_post_notifications.py | 7 ++++--- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/models.py b/app/models.py index 0fe4b0a6e..3fce77c60 100644 --- a/app/models.py +++ b/app/models.py @@ -199,7 +199,7 @@ class Service(db.Model, Versioned): created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False) reply_to_email_address = db.Column(db.Text, index=False, unique=False, nullable=True) letter_contact_block = db.Column(db.Text, index=False, unique=False, nullable=True) - sms_sender = db.Column(db.String(11), nullable=True, default='GOVUK') + sms_sender = db.Column(db.String(11), nullable=True, default=lambda: current_app.config['FROM_NUMBER']) organisation_id = db.Column(UUID(as_uuid=True), db.ForeignKey('organisation.id'), index=True, nullable=True) organisation = db.relationship('Organisation') dvla_organisation_id = db.Column( diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 7ab0ba793..126e9b948 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -5,7 +5,7 @@ import uuid from unittest.mock import ANY import pytest -from flask import url_for +from flask import url_for, current_app from freezegun import freeze_time from app.dao.users_dao import save_model_user @@ -144,7 +144,7 @@ def test_get_service_by_id(client, sample_service): assert json_resp['data']['organisation'] is None assert json_resp['data']['branding'] == 'govuk' assert json_resp['data']['dvla_organisation'] == '001' - assert json_resp['data']['sms_sender'] == 'GOVUK' + assert json_resp['data']['sms_sender'] == current_app.config['FROM_NUMBER'] def test_get_service_by_id_should_404_if_no_service(notify_api, notify_db): @@ -214,7 +214,7 @@ def test_create_service(client, sample_user): assert json_resp['data']['email_from'] == 'created.service' assert not json_resp['data']['research_mode'] assert json_resp['data']['dvla_organisation'] == '001' - assert json_resp['data']['sms_sender'] == 'GOVUK' + assert json_resp['data']['sms_sender'] == current_app.config['FROM_NUMBER'] auth_header_fetch = create_authorization_header() diff --git a/tests/app/service/test_sender.py b/tests/app/service/test_sender.py index 3223e9e71..cb6ee93fd 100644 --- a/tests/app/service/test_sender.py +++ b/tests/app/service/test_sender.py @@ -109,5 +109,4 @@ def test_send_notification_to_service_users_sends_to_active_users_only( assert Notification.query.count() == 2 - assert notifications[0].to == first_active_user.email_address - assert notifications[1].to == second_active_user.email_address + assert pending_user.email_address not in [n.to for n in notifications] diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 606f0ed42..4e721f15f 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -1,6 +1,8 @@ import uuid + import pytest -from flask import json +from flask import json, current_app + from app.models import Notification from app.v2.errors import RateLimitError from tests import create_authorization_header @@ -33,8 +35,7 @@ def test_post_sms_notification_returns_201(notify_api, sample_template_with_plac assert resp_json['id'] == str(notification_id) assert resp_json['reference'] == reference assert resp_json['content']['body'] == sample_template_with_placeholders.content.replace("(( Name))", "Jo") - # conftest fixture service does not have a sms sender, use config default - assert resp_json['content']['from_number'] == 'GOVUK' + assert resp_json['content']['from_number'] == current_app.config['FROM_NUMBER'] assert 'v2/notifications/{}'.format(notification_id) in resp_json['uri'] assert resp_json['template']['id'] == str(sample_template_with_placeholders.id) assert resp_json['template']['version'] == sample_template_with_placeholders.version From d745df3f44b4ef1c65633a9188453096ab255f64 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 22 May 2017 15:10:05 +0100 Subject: [PATCH 11/11] Fix test that doesn't account for notifications being created in a different order --- tests/app/service/test_sender.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/app/service/test_sender.py b/tests/app/service/test_sender.py index cb6ee93fd..ca042cc67 100644 --- a/tests/app/service/test_sender.py +++ b/tests/app/service/test_sender.py @@ -106,7 +106,9 @@ def test_send_notification_to_service_users_sends_to_active_users_only( send_notification_to_service_users(service_id=service.id, template_id=template.id) notifications = Notification.query.all() + notifications_recipients = [notification.to for notification in notifications] assert Notification.query.count() == 2 - - assert pending_user.email_address not in [n.to for n in notifications] + assert pending_user.email_address not in notifications_recipients + assert first_active_user.email_address in notifications_recipients + assert second_active_user.email_address in notifications_recipients