From 700e3d2fa775031dac262b884cf99130ce8cef3a Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 12 May 2017 14:59:14 +0100 Subject: [PATCH 1/4] update delivery receipts for countries that return them some countries don't return delivery receipts some countries return delivery receipts when they reach the carrier these countries, we should keep the notifications in sent (aka sent_internatinally) for. However, for countries that have normal delivery receipts, we should update them as we do for UK numbers --- app/dao/notifications_dao.py | 10 +++++- tests/app/dao/test_notification_dao.py | 44 ++++++++++++++++++-------- 2 files changed, 40 insertions(+), 14 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index da81c348b..a81b6cad5 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -8,6 +8,7 @@ from flask import current_app from werkzeug.datastructures import MultiDict from sqlalchemy import (desc, func, or_, and_, asc) from sqlalchemy.orm import joinedload +from notifications_utils.international_billing_rates import INTERNATIONAL_BILLING_RATES from app import db, create_uuid from app.dao import days_ago @@ -163,6 +164,10 @@ def _decide_permanent_temporary_failure(current_status, status): return status +def country_records_delivery(phone_prefix): + return INTERNATIONAL_BILLING_RATES[phone_prefix]['attributes']['dlr'].lower() == 'yes' + + def _update_notification_status(notification, status): status = _decide_permanent_temporary_failure(current_status=notification.status, status=status) notification.status = status @@ -182,7 +187,10 @@ def update_notification_status_by_id(notification_id, status): Notification.status == NOTIFICATION_SENT )).first() - if not notification or notification.status == NOTIFICATION_SENT: + if not notification: + return None + + if notification.international and not country_records_delivery(notification.phone_prefix): return None return _update_notification_status( diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 95dc3e09d..c6c8d2b0b 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -15,6 +15,7 @@ from app.models import ( NOTIFICATION_STATUS_TYPES, NOTIFICATION_STATUS_TYPES_FAILED, NOTIFICATION_SENT, + NOTIFICATION_DELIVERED, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST) @@ -353,28 +354,45 @@ def test_should_update_status_by_id_if_created(notify_db, notify_db_session): assert updated.status == 'failed' -def test_should_not_update_status_by_reference_if_in_sent_status(notify_db, notify_db_session): - notification = sample_notification( - notify_db, - notify_db_session, +def test_should_not_update_status_by_reference_if_from_country_with_no_delivery_receipts(sample_template): + notification = create_notification( + sample_template, status=NOTIFICATION_SENT, reference='foo' ) - update_notification_status_by_reference('foo', 'failed') - assert Notification.query.get(notification.id).status == NOTIFICATION_SENT + res = update_notification_status_by_reference('foo', 'failed') + + assert res is None + assert notification.status == NOTIFICATION_SENT -def test_should_not_update_status_by_id_if_in_sent_status(notify_db, notify_db_session): - notification = sample_notification( - notify_db, - notify_db_session, - status=NOTIFICATION_SENT +def test_should_not_update_status_by_id_if_sent_to_country_with_no_delivery_receipts(sample_template): + notification = create_notification( + sample_template, + status=NOTIFICATION_SENT, + international=True, + phone_prefix='1' # americans only have carrier delivery receipts ) - update_notification_status_by_id(notification.id, 'failed') + res = update_notification_status_by_id(notification.id, 'delivered') - assert Notification.query.get(notification.id).status == NOTIFICATION_SENT + assert res is None + assert notification.status == NOTIFICATION_SENT + + +def test_should_not_update_status_by_id_if_sent_to_country_with_no_delivery_receipts(sample_template): + notification = create_notification( + sample_template, + status=NOTIFICATION_SENT, + international=True, + phone_prefix='7' # russians have full delivery receipts + ) + + res = update_notification_status_by_id(notification.id, 'delivered') + + assert res == notification + assert notification.status == NOTIFICATION_DELIVERED def test_should_not_update_status_by_reference_if_not_sending(notify_db, notify_db_session): From 24dfcd2128e7051f351b18012852c8fc50e6faaf Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 23 May 2017 10:43:48 +0100 Subject: [PATCH 2/4] Add normalised_to field to notification --- app/models.py | 1 + .../versions/0086_add_norm_to_notification.py | 21 +++++++++++++++++++ 2 files changed, 22 insertions(+) create mode 100644 migrations/versions/0086_add_norm_to_notification.py diff --git a/app/models.py b/app/models.py index 378c45b3e..7ba8ff22c 100644 --- a/app/models.py +++ b/app/models.py @@ -679,6 +679,7 @@ class Notification(db.Model): id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) to = db.Column(db.String, nullable=False) + normalised_to = db.Column(db.String, nullable=True) job_id = db.Column(UUID(as_uuid=True), db.ForeignKey('jobs.id'), index=True, unique=False) job = db.relationship('Job', backref=db.backref('notifications', lazy='dynamic')) job_row_number = db.Column(db.Integer, nullable=True) diff --git a/migrations/versions/0086_add_norm_to_notification.py b/migrations/versions/0086_add_norm_to_notification.py new file mode 100644 index 000000000..346d5b6dc --- /dev/null +++ b/migrations/versions/0086_add_norm_to_notification.py @@ -0,0 +1,21 @@ +""" + +Revision ID: 0086_add_norm_to_notification +Revises: 0085_update_incoming_to_inbound +Create Date: 2017-05-23 10:37:00.404087 + +""" + +from alembic import op +import sqlalchemy as sa + +revision = '0086_add_norm_to_notification' +down_revision = '0085_update_incoming_to_inbound' + + +def upgrade(): + op.add_column('notifications', sa.Column('normalised_to', sa.String(), nullable=True)) + + +def downgrade(): + op.drop_column('notifications', 'normalised_to') From 554a193cffc4ee0a0dd6b38778b74abe51e3266c Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 24 May 2017 16:27:12 +0100 Subject: [PATCH 3/4] separate service deserialization from validation Marshmallow validates and deserialises - BUT, when it deserialises, it explicitly sets `sms_sender=None`, even when you haven't passed sms_sender in. This is problematic, because we wanted to take advantage of sqlalchemy's default value to set sms_sender to `GOVUK` when the actual DB commit happens. Instead, still use marshmallow for validating, but manually carry out the json deserialisation in the model class. This fixes a bug that only manifested when the database was upgraded, but the code hadn't updated. :tada: --- app/models.py | 17 ++++++++++++++++- app/service/rest.py | 23 +++++++++++++++-------- tests/app/service/test_rest.py | 6 +++++- 3 files changed, 36 insertions(+), 10 deletions(-) diff --git a/app/models.py b/app/models.py index 8b3b8e053..e6cde1e28 100644 --- a/app/models.py +++ b/app/models.py @@ -188,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, default=lambda: current_app.config['FROM_NUMBER']) + sms_sender = db.Column(db.String(11), nullable=False, 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( @@ -215,6 +215,21 @@ class Service(db.Model, Versioned): self.can_send_letters = LETTER_TYPE in [p.permission for p in self.permissions] self.can_send_international_sms = INTERNATIONAL_SMS_TYPE in [p.permission for p in self.permissions] + @classmethod + def from_json(cls, data): + """ + Assumption: data has been validated appropriately. + + Returns a Service object based on the provided data. Deserialises created_by to created_by_id as marshmallow + would. + """ + # validate json with marshmallow + fields = data.copy() + + fields['created_by_id'] = fields.pop('created_by') + + return cls(**fields) + class ServicePermission(db.Model): __tablename__ = "service_permissions" diff --git a/app/service/rest.py b/app/service/rest.py index 180740063..3270b96c2 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -5,11 +5,12 @@ from datetime import datetime from flask import ( jsonify, request, - current_app + current_app, + Blueprint ) from sqlalchemy.orm.exc import NoResultFound -from app.dao import notification_usage_dao +from app.dao import notification_usage_dao, notifications_dao from app.dao.dao_utils import dao_rollback from app.dao.api_key_dao import ( save_model_api_key, @@ -39,11 +40,13 @@ from app.dao.service_whitelist_dao import ( dao_add_and_commit_whitelisted_contacts, dao_remove_service_whitelist ) -from app.dao import notifications_dao from app.dao.provider_statistics_dao import get_fragment_count from app.dao.users_dao import get_user_by_id from app.errors import ( - InvalidRequest, register_errors) + InvalidRequest, + register_errors +) +from app.models import Service from app.service import statistics from app.service.utils import get_whitelist_objects from app.service.sender import send_notification_to_service_users @@ -57,7 +60,6 @@ from app.schemas import ( detailed_service_schema ) from app.utils import pagination_links -from flask import Blueprint service_blueprint = Blueprint('service', __name__) @@ -108,9 +110,14 @@ def create_service(): errors = {'user_id': ['Missing data for required field.']} raise InvalidRequest(errors, status_code=400) - user = get_user_by_id(data['user_id']) - data.pop('user_id', None) - valid_service = service_schema.load(request.get_json()).data + # validate json with marshmallow + service_schema.load(request.get_json()) + + user = get_user_by_id(data.pop('user_id', None)) + + # unpack valid json into service object + valid_service = Service.from_json(data) + dao_create_service(valid_service, user) return jsonify(data=service_schema.dump(valid_service).data), 201 diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 126e9b948..e4929a005 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -20,7 +20,7 @@ from tests.app.conftest import ( sample_notification_history as create_notification_history, sample_notification_with_job ) -from app.models import KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST +from app.models import Service, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST from tests.app.db import create_user @@ -216,6 +216,10 @@ def test_create_service(client, sample_user): assert json_resp['data']['dvla_organisation'] == '001' assert json_resp['data']['sms_sender'] == current_app.config['FROM_NUMBER'] + service_db = Service.query.get(json_resp['data']['id']) + assert service_db.name == 'created service' + assert service_db.sms_sender == current_app.config['FROM_NUMBER'] + auth_header_fetch = create_authorization_header() resp = client.get( From 4b8b6ca91ee8b01c128c8a9f49897beb11c4b018 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 24 May 2017 17:33:22 +0100 Subject: [PATCH 4/4] add test to ensure that updating other things doesnt affect sms sender --- app/models.py | 2 +- tests/app/service/test_rest.py | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/app/models.py b/app/models.py index e6cde1e28..4e2b4777b 100644 --- a/app/models.py +++ b/app/models.py @@ -188,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=False, default=lambda: current_app.config['FROM_NUMBER']) + 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 e4929a005..cbf3eb1b2 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1735,3 +1735,19 @@ def test_update_service_does_not_call_send_notification_when_restricted_not_chan assert resp.status_code == 200 assert not send_notification_mock.called + + +def test_update_service_works_when_sms_sender_is_null(sample_service, client, mocker): + sample_service.sms_sender = None + data = {'name': 'new name'} + + resp = client.post( + 'service/{}'.format(sample_service.id), + data=json.dumps(data), + headers=[create_authorization_header()], + content_type='application/json' + ) + + assert resp.status_code == 200 + # make sure it wasn't changed to not-null under the hood + assert sample_service.sms_sender is None