diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 443000e6b..d1b78c27a 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/app/models.py b/app/models.py index f8d3d6a88..ba667f599 100644 --- a/app/models.py +++ b/app/models.py @@ -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" @@ -679,6 +694,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/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/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') diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 6d1842da1..38705ed42 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -16,6 +16,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 @@ -357,28 +358,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): diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 126e9b948..cbf3eb1b2 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( @@ -1731,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