Merge branch 'master' into schedule-api-notification

Conflicts:
	tests/app/v2/notifications/test_post_notifications.py

app/config.py

@@ -292,7 +292,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

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)

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'
 
@@ -199,7 +188,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=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(
@@ -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'
 

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'")

migrations/versions/0086_scheduled_notifications.py

@@ -1,7 +1,7 @@
 """empty message
 
-Revision ID: 0085_scheduled_notifications
-Revises: 0084_add_job_stats
+Revision ID: 0086_scheduled_notifications
+Revises: 0085_update_incoming_to_inbound
 Create Date: 2017-05-15 12:50:20.041950
 
 """
@@ -9,8 +9,8 @@ from alembic import op
 import sqlalchemy as sa
 from sqlalchemy.dialects import postgresql
 
-revision = '0085_scheduled_notifications'
-down_revision = '0084_add_job_stats'
+revision = '0086_scheduled_notifications'
+down_revision = '0085_update_incoming_to_inbound'
 
 
 def upgrade():

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

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):

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 <em>some HTML</em> & 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,
+    )

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,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'] == current_app.config['FROM_NUMBER']
 
 
 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'] == current_app.config['FROM_NUMBER']
 
     auth_header_fetch = create_authorization_header()
 

tests/app/service/test_sender.py

@@ -106,8 +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 notifications[0].to == first_active_user.email_address
-    assert notifications[1].to == second_active_user.email_address
+    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

tests/app/v2/notifications/test_post_notifications.py

@@ -1,10 +1,12 @@
 import uuid
 
 import pytest
-from flask import json
 from freezegun import freeze_time
 
 from app.models import Notification, ScheduledNotification
+from flask import json, current_app
+
+from app.models import Notification
 from app.v2.errors import RateLimitError
 from tests import create_authorization_header
 from tests.app.conftest import sample_template as create_sample_template, sample_service
@@ -36,8 +38,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'] == notify_api.config["FROM_NUMBER"]
+            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