From b16c855f56fb563874b849bf21332112e860806f Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Mon, 23 Oct 2017 17:48:27 +0100 Subject: [PATCH 01/13] Update notifications-python-client from 4.4.0 to 4.5.0 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 386828209..d7534f6db 100644 --- a/requirements.txt +++ b/requirements.txt @@ -20,7 +20,7 @@ SQLAlchemy-Utils==0.32.19 SQLAlchemy==1.1.14 statsd==3.2.1 -notifications-python-client==4.4.0 +notifications-python-client==4.5.0 # PaaS awscli>=1.11,<1.12 From 2e43044a0f950c3a889712fcea0bbb8e9abb542d Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Tue, 24 Oct 2017 01:17:28 +0100 Subject: [PATCH 02/13] Update marshmallow-sqlalchemy from 0.13.1 to 0.13.2 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 386828209..608012069 100644 --- a/requirements.txt +++ b/requirements.txt @@ -11,7 +11,7 @@ Flask==0.12.2 gunicorn==19.7.1 iso8601==0.1.12 jsonschema==2.6.0 -marshmallow-sqlalchemy==0.13.1 +marshmallow-sqlalchemy==0.13.2 marshmallow==2.13.6 monotonic==1.3 psycopg2==2.7.3.1 From 8d6c38e1fea3189642eac043a41def3f880a8251 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 24 Oct 2017 13:37:17 +0100 Subject: [PATCH 03/13] Now that we allow multiple SMS senders for a service, update the sender for SMS to be the default SMS sender, regardless if it is inbound number or not --- app/delivery/send_to_providers.py | 2 +- app/models.py | 2 - app/v2/notifications/post_notifications.py | 2 +- tests/app/delivery/test_send_to_providers.py | 47 +++++++++++++++---- tests/app/test_model.py | 10 +--- .../notifications/test_post_notifications.py | 19 +++++--- 6 files changed, 55 insertions(+), 27 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 808cd2cdb..c253748ad 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -73,7 +73,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.get_inbound_number() + sender=service.get_default_sms_sender() ) except Exception as e: dao_toggle_sms_provider(provider.name) diff --git a/app/models.py b/app/models.py index dfe6cabe7..645b5490f 100644 --- a/app/models.py +++ b/app/models.py @@ -275,8 +275,6 @@ class Service(db.Model, Versioned): def get_inbound_number(self): if self.inbound_number and self.inbound_number.active: return self.inbound_number.number - else: - return self.get_default_sms_sender() def get_default_sms_sender(self): default_sms_sender = [x for x in self.service_sms_senders if x.is_default] diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 32b96cf8f..b4c837306 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -95,7 +95,7 @@ def post_notification(notification_type): if notification_type == SMS_TYPE: create_resp_partial = functools.partial( create_post_sms_response_from_notification, - from_number=authenticated_service.get_inbound_number() + from_number=authenticated_service.get_default_sms_sender() ) elif notification_type == EMAIL_TYPE: create_resp_partial = functools.partial( diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 37b5f2ffb..07b17eb0b 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -12,6 +12,7 @@ import app from app import mmg_client, firetext_client from app.dao import (provider_details_dao, notifications_dao) from app.dao.provider_details_dao import dao_switch_sms_provider_to_provider_with_identifier +from app.dao.service_sms_sender_dao import dao_add_sms_sender_for_service from app.delivery import send_to_providers from app.models import ( Notification, @@ -31,8 +32,8 @@ from tests.app.db import ( create_notification, create_inbound_number, create_reply_to_email, - create_reply_to_email_for_notification -) + create_reply_to_email_for_notification, + create_service_sms_sender) def test_should_return_highest_priority_active_provider(restore_provider_details): @@ -699,14 +700,18 @@ def test_should_handle_sms_sender_and_prefix_message( ) -def test_should_use_inbound_number_as_sender_if_set( - sample_service, - mocker +def test_should_use_inbound_number_as_sender_if_default_sms_sender( + notify_db_session, + mocker ): - sample_service.sms_sender = 'test sender' - template = create_template(sample_service, content='bar') + service = create_service(sms_sender='test sender') + inbound_number = create_inbound_number('1') + dao_add_sms_sender_for_service(service_id=service.id, + sms_sender=inbound_number.number, + is_default=True, + inbound_number_id=inbound_number.id) + template = create_template(service, content='bar') notification = create_notification(template) - inbound_number = create_inbound_number('1', service_id=sample_service.id) mocker.patch('app.mmg_client.send_sms') mocker.patch('app.delivery.send_to_providers.create_initial_notification_statistic_tasks') @@ -721,6 +726,32 @@ def test_should_use_inbound_number_as_sender_if_set( ) +def test_should_use_default_sms_sender( + notify_db_session, + mocker +): + service = create_service(sms_sender='test sender') + inbound_number = create_inbound_number('1') + dao_add_sms_sender_for_service(service_id=service.id, + sms_sender=inbound_number.number, + is_default=False, + inbound_number_id=inbound_number.id) + template = create_template(service, content='bar') + notification = create_notification(template) + + mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.delivery.send_to_providers.create_initial_notification_statistic_tasks') + + send_to_providers.send_sms_to_provider(notification) + + mmg_client.send_sms.assert_called_once_with( + to=ANY, + content=ANY, + reference=str(notification.id), + sender='test sender' + ) + + def test_send_email_to_provider_get_linked_email_reply_to_default_is_false( sample_service, sample_email_template, diff --git a/tests/app/test_model.py b/tests/app/test_model.py index a1bc1e343..5ae310a19 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -252,17 +252,11 @@ def test_inbound_number_returns_inbound_number(client, notify_db_session): assert service.get_inbound_number() == inbound_number.number -def test_inbound_number_returns_sms_sender(client, notify_db_session): - service = create_service(sms_sender='testing') - - assert service.get_inbound_number() == service.sms_sender - - -def test_inbound_number_returns_from_number_config(client, notify_db_session): +def test_inbound_number_returns_none_when_no_inbound_number(client, notify_db_session): with set_config(client.application, 'FROM_NUMBER', 'test'): service = create_service(sms_sender=None) - assert service.get_inbound_number() == 'test' + assert not service.get_inbound_number() def test_service_get_default_reply_to_email_address(sample_service): diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 94fea27e5..9d63c6bfb 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -8,7 +8,6 @@ from app.models import ( ScheduledNotification, SCHEDULE_NOTIFICATIONS, EMAIL_TYPE, - INTERNATIONAL_SMS_TYPE, SMS_TYPE ) from flask import json, current_app @@ -25,7 +24,11 @@ from tests.app.conftest import ( sample_template_without_sms_permission ) -from tests.app.db import create_inbound_number, create_service, create_template, create_reply_to_email +from tests.app.db import ( + create_service, + create_template, + create_reply_to_email +) @pytest.mark.parametrize("reference", [None, "reference_from_client"]) @@ -64,15 +67,16 @@ def test_post_sms_notification_returns_201(client, sample_template_with_placehol assert mocked.called -def test_post_sms_notification_uses_inbound_number_as_sender(client, sample_template_with_placeholders, mocker): +def test_post_sms_notification_uses_inbound_number_as_sender(client, notify_db_session, mocker): + service = create_service(sms_sender='1', do_create_inbound_number=True) + template = create_template(service=service, content="Hello (( Name))\nYour thing is due soon") mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') data = { 'phone_number': '+447700900855', - 'template_id': str(sample_template_with_placeholders.id), + 'template_id': str(template.id), 'personalisation': {' Name': 'Jo'} } - inbound_number = create_inbound_number('1', service_id=sample_template_with_placeholders.service_id) - auth_header = create_authorization_header(service_id=sample_template_with_placeholders.service_id) + auth_header = create_authorization_header(service_id=service.id) response = client.post( path='/v2/notifications/sms', @@ -85,7 +89,8 @@ def test_post_sms_notification_uses_inbound_number_as_sender(client, sample_temp assert len(notifications) == 1 notification_id = notifications[0].id assert resp_json['id'] == str(notification_id) - assert resp_json['content']['from_number'] == inbound_number.number + assert resp_json['content']['from_number'] == '1' + mocked.assert_called_once_with([str(notification_id)], queue='send-sms-tasks') @pytest.mark.parametrize("notification_type, key_send_to, send_to", From f519c2a4d9a39ed589326a138cb3ae3a1061b585 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 24 Oct 2017 14:53:02 +0100 Subject: [PATCH 04/13] fix import --- tests/app/delivery/test_send_to_providers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 07b17eb0b..6733c5846 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -32,8 +32,8 @@ from tests.app.db import ( create_notification, create_inbound_number, create_reply_to_email, - create_reply_to_email_for_notification, - create_service_sms_sender) + create_reply_to_email_for_notification +) def test_should_return_highest_priority_active_provider(restore_provider_details): From a0de1d0698cf5277a96f50af2f10f51b0a5ac7d9 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Tue, 24 Oct 2017 17:32:05 +0100 Subject: [PATCH 05/13] Update psycopg2 from 2.7.3.1 to 2.7.3.2 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 386828209..5b477d254 100644 --- a/requirements.txt +++ b/requirements.txt @@ -14,7 +14,7 @@ jsonschema==2.6.0 marshmallow-sqlalchemy==0.13.1 marshmallow==2.13.6 monotonic==1.3 -psycopg2==2.7.3.1 +psycopg2==2.7.3.2 PyJWT==1.5.3 SQLAlchemy-Utils==0.32.19 SQLAlchemy==1.1.14 From a2b42194cdffa4e656b0d8c70e0becb2fcde0672 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 23 Oct 2017 15:57:00 +0100 Subject: [PATCH 06/13] Add letter status received to data model - in order to reduce the number of statuses in the database the letter status `received` will be mapped to `delivered` internally --- app/models.py | 17 ++++++++++++++++- tests/app/test_model.py | 10 +++++++--- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/app/models.py b/app/models.py index dfe6cabe7..04decb50a 100644 --- a/app/models.py +++ b/app/models.py @@ -876,6 +876,9 @@ NOTIFICATION_STATUS_TYPES_ENUM = db.Enum(*NOTIFICATION_STATUS_TYPES, name='notif NOTIFICATION_STATUS_LETTER_ACCEPTED = 'accepted' NOTIFICATION_STATUS_LETTER_ACCEPTED_PRETTY = 'Accepted' +NOTIFICATION_STATUS_LETTER_RECEIVED = 'received' + +DVLA_STATUS_SENT = 'Sent' class NotificationStatusTypes(db.Model): @@ -986,6 +989,14 @@ class Notification(db.Model): ['technical-failure', 'temporary-failure', 'permanent-failure', 'created', 'sending'] + - + + > IN + 'delivered' + + < OUT + ['received'] + :param status_or_statuses: a single status or list of statuses :return: a single status or list with the current failure statuses substituted for 'failure' """ @@ -994,6 +1005,7 @@ class Notification(db.Model): return ( NOTIFICATION_STATUS_TYPES_FAILED if _status == NOTIFICATION_FAILED else [NOTIFICATION_CREATED, NOTIFICATION_SENDING] if _status == NOTIFICATION_STATUS_LETTER_ACCEPTED else + NOTIFICATION_DELIVERED if _status == NOTIFICATION_STATUS_LETTER_RECEIVED else [_status] ) @@ -1044,6 +1056,7 @@ class Notification(db.Model): 'technical-failure': 'Technical failure', 'sending': NOTIFICATION_STATUS_LETTER_ACCEPTED_PRETTY, 'created': NOTIFICATION_STATUS_LETTER_ACCEPTED_PRETTY, + 'delivered': 'Received' } }[self.template.template_type].get(self.status, self.status) @@ -1058,8 +1071,10 @@ class Notification(db.Model): # get the two code flows mixed up at all assert self.notification_type == LETTER_TYPE - if self.status == NOTIFICATION_CREATED or NOTIFICATION_SENDING: + if self.status in [NOTIFICATION_CREATED, NOTIFICATION_SENDING]: return NOTIFICATION_STATUS_LETTER_ACCEPTED + elif self.status == NOTIFICATION_DELIVERED: + return NOTIFICATION_STATUS_LETTER_RECEIVED else: # Currently can only be technical-failure return self.status diff --git a/tests/app/test_model.py b/tests/app/test_model.py index a1bc1e343..f7d3ef6fd 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -10,12 +10,14 @@ from app.models import ( MOBILE_TYPE, EMAIL_TYPE, NOTIFICATION_CREATED, + NOTIFICATION_DELIVERED, NOTIFICATION_SENDING, NOTIFICATION_PENDING, NOTIFICATION_FAILED, - NOTIFICATION_TECHNICAL_FAILURE, + NOTIFICATION_STATUS_LETTER_ACCEPTED, + NOTIFICATION_STATUS_LETTER_RECEIVED, NOTIFICATION_STATUS_TYPES_FAILED, - NOTIFICATION_STATUS_LETTER_ACCEPTED + NOTIFICATION_TECHNICAL_FAILURE ) from tests.app.conftest import ( sample_template as create_sample_template, @@ -71,6 +73,7 @@ def test_should_not_build_service_whitelist_from_invalid_contact(recipient_type, ([NOTIFICATION_FAILED], NOTIFICATION_STATUS_TYPES_FAILED), ([NOTIFICATION_CREATED], [NOTIFICATION_CREATED]), ([NOTIFICATION_TECHNICAL_FAILURE], [NOTIFICATION_TECHNICAL_FAILURE]), + (NOTIFICATION_STATUS_LETTER_RECEIVED, NOTIFICATION_DELIVERED), # passing in lists containing multiple statuses ([NOTIFICATION_FAILED, NOTIFICATION_CREATED], NOTIFICATION_STATUS_TYPES_FAILED + [NOTIFICATION_CREATED]), ([NOTIFICATION_CREATED, NOTIFICATION_PENDING], [NOTIFICATION_CREATED, NOTIFICATION_PENDING]), @@ -132,7 +135,8 @@ def test_notification_for_csv_returns_correct_job_row_number(notify_db, notify_d ('sms', 'sent', 'Sent internationally'), ('letter', 'created', 'Accepted'), ('letter', 'sending', 'Accepted'), - ('letter', 'technical-failure', 'Technical failure') + ('letter', 'technical-failure', 'Technical failure'), + ('letter', 'delivered', 'Received') ]) def test_notification_for_csv_returns_formatted_status( notify_db, From 8bdc999818f2c69f4ffc9a746b168a485e6ed4a2 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 23 Oct 2017 16:23:36 +0100 Subject: [PATCH 07/13] Update notification schema to include received letter status --- app/v2/notifications/notification_schemas.py | 10 ++++++++-- .../app/v2/notifications/test_notification_schemas.py | 4 ++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py index 2eeddc532..1689a7319 100644 --- a/app/v2/notifications/notification_schemas.py +++ b/app/v2/notifications/notification_schemas.py @@ -1,4 +1,9 @@ -from app.models import NOTIFICATION_STATUS_TYPES, NOTIFICATION_STATUS_LETTER_ACCEPTED, TEMPLATE_TYPES +from app.models import ( + NOTIFICATION_STATUS_TYPES, + NOTIFICATION_STATUS_LETTER_ACCEPTED, + NOTIFICATION_STATUS_LETTER_RECEIVED, + TEMPLATE_TYPES +) from app.schema_validation.definitions import (uuid, personalisation, letter_personalisation) @@ -59,7 +64,8 @@ get_notifications_request = { "status": { "type": "array", "items": { - "enum": NOTIFICATION_STATUS_TYPES + [NOTIFICATION_STATUS_LETTER_ACCEPTED] + "enum": NOTIFICATION_STATUS_TYPES + + [NOTIFICATION_STATUS_LETTER_ACCEPTED + ', ' + NOTIFICATION_STATUS_LETTER_RECEIVED] } }, "template_type": { diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py index c667def11..6b51b195d 100644 --- a/tests/app/v2/notifications/test_notification_schemas.py +++ b/tests/app/v2/notifications/test_notification_schemas.py @@ -26,7 +26,7 @@ def test_get_notifications_request_invalid_statuses( ): partial_error_status = "is not one of " \ "[created, sending, sent, delivered, pending, failed, " \ - "technical-failure, temporary-failure, permanent-failure, accepted]" + "technical-failure, temporary-failure, permanent-failure, accepted, received]" with pytest.raises(ValidationError) as e: validate({'status': invalid_statuses + valid_statuses}, get_notifications_request) @@ -73,7 +73,7 @@ def test_get_notifications_request_invalid_statuses_and_template_types(): error_messages = [error['message'] for error in errors] for invalid_status in ["elephant", "giraffe"]: assert "status {} is not one of [created, sending, sent, delivered, " \ - "pending, failed, technical-failure, temporary-failure, permanent-failure, accepted]".format( + "pending, failed, technical-failure, temporary-failure, permanent-failure, accepted, received]".format( invalid_status ) in error_messages From 98d54737884f39afbba92cbd5696a4cfec1b9370 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 23 Oct 2017 16:24:55 +0100 Subject: [PATCH 08/13] Update test to to test letter status received --- .../notifications/test_get_notifications.py | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index 926c8c1d8..d4306ba93 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -393,7 +393,7 @@ def test_get_all_notifications_filter_by_status_invalid_status(client, sample_no assert json_response['status_code'] == 400 assert len(json_response['errors']) == 1 assert json_response['errors'][0]['message'] == "status elephant is not one of [created, sending, sent, " \ - "delivered, pending, failed, technical-failure, temporary-failure, permanent-failure, accepted]" + "delivered, pending, failed, technical-failure, temporary-failure, permanent-failure, accepted, received]" def test_get_all_notifications_filter_by_multiple_statuses(client, sample_template): @@ -583,14 +583,25 @@ def test_get_all_notifications_renames_letter_statuses( pytest.fail() -def test_get_notifications_renames_letter_statuses(client, sample_letter_notification): - auth_header = create_authorization_header(service_id=sample_letter_notification.service_id) +@pytest.mark.parametrize('db_status,expected_status', [ + ('created', 'accepted'), + ('sending', 'accepted'), + ('delivered', 'received'), + ('pending', 'pending'), + ('technical-failure', 'technical-failure') +]) +def test_get_notifications_renames_letter_statuses(client, sample_letter_template, db_status, expected_status): + letter_noti = create_notification( + sample_letter_template, + status=db_status, + personalisation={'address_line_1': 'Mr Foo', 'address_line_2': '1 Bar Street', 'postcode': 'N1'} + ) + auth_header = create_authorization_header(service_id=letter_noti.service_id) response = client.get( - path=url_for('v2_notifications.get_notification_by_id', id=sample_letter_notification.id), + path=url_for('v2_notifications.get_notification_by_id', id=letter_noti.id), headers=[('Content-Type', 'application/json'), auth_header] ) json_response = json.loads(response.get_data(as_text=True)) assert response.status_code == 200 - - assert json_response['status'] == 'accepted' + assert json_response['status'] == expected_status From 5da119f8244ca1ea523a822eafe7f6208917961a Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 23 Oct 2017 17:01:53 +0100 Subject: [PATCH 09/13] Update letter noti status based on dvla response file --- app/celery/tasks.py | 21 +++++++++++++++++++-- tests/app/celery/test_ftp_update_tasks.py | 20 +++++++++++++++++++- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index bece59f7c..52a5741f3 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -36,6 +36,7 @@ from app.dao.jobs_dao import ( dao_update_job_status) from app.dao.notifications_dao import ( get_notification_by_id, + update_notification_status_by_reference, dao_update_notifications_for_job_to_sent_to_dvla, dao_update_notifications_by_reference, dao_get_last_notification_added_for_job_id) @@ -46,6 +47,7 @@ from app.dao.templates_dao import dao_get_template_by_id from app.models import ( Job, Notification, + DVLA_STATUS_SENT, EMAIL_TYPE, JOB_STATUS_CANCELLED, JOB_STATUS_FINISHED, @@ -55,6 +57,8 @@ from app.models import ( JOB_STATUS_SENT_TO_DVLA, JOB_STATUS_ERROR, KEY_TYPE_NORMAL, LETTER_TYPE, + NOTIFICATION_DELIVERED, + NOTIFICATION_FAILED, NOTIFICATION_SENDING, NOTIFICATION_TECHNICAL_FAILURE, SMS_TYPE, @@ -443,8 +447,21 @@ def update_letter_notifications_statuses(self, filename): raise else: for update in notification_updates: - current_app.logger.info('DVLA update: {}'.format(str(update))) - # TODO: Update notifications with desired status + status = NOTIFICATION_DELIVERED if update.status == DVLA_STATUS_SENT else NOTIFICATION_FAILED + notification = update_notification_status_by_reference( + update.reference, + status + ) + + if not notification: + msg = "Update letter notification file {filename} failed: notification either not found " \ + "or already updated from delivered. Status {status} for notification reference {reference}".format( + filename=filename, status=status, reference=update.reference) + current_app.logger.error(msg) + else: + current_app.logger.info( + 'DVLA file: {filename}, notification updated to {status}: {reference}'.format( + filename=filename, status=status, reference=str(update.reference))) def process_updates_from_file(response_file): diff --git a/tests/app/celery/test_ftp_update_tasks.py b/tests/app/celery/test_ftp_update_tasks.py index dc96ac91d..6239d9285 100644 --- a/tests/app/celery/test_ftp_update_tasks.py +++ b/tests/app/celery/test_ftp_update_tasks.py @@ -7,10 +7,14 @@ from flask import current_app from app.models import ( Job, Notification, + NOTIFICATION_DELIVERED, + NOTIFICATION_FAILED, NOTIFICATION_SENDING, NOTIFICATION_CREATED, - NOTIFICATION_TECHNICAL_FAILURE + NOTIFICATION_TECHNICAL_FAILURE, + NOTIFICATION_STATUS_LETTER_RECEIVED ) +from app.dao.notifications_dao import dao_update_notifications_by_reference from app.celery.tasks import ( update_job_to_sent_to_dvla, update_dvla_job_to_error, @@ -91,6 +95,20 @@ def test_update_letter_notifications_statuses_builds_updates_list(notify_api, mo assert updates[1].cost_threshold == 'Sorted' +def test_update_letter_notifications_statuses_persisted(notify_api, mocker, sample_letter_template): + sent_letter = create_notification(sample_letter_template, reference='ref-foo', status=NOTIFICATION_SENDING) + failed_letter = create_notification(sample_letter_template, reference='ref-bar', status=NOTIFICATION_SENDING) + + valid_file = '{}|Sent|1|Unsorted\n{}|Failed|2|Sorted'.format( + sent_letter.reference, failed_letter.reference) + mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file) + + update_letter_notifications_statuses(filename='foo.txt') + + assert sent_letter.status == NOTIFICATION_DELIVERED + assert failed_letter.status == NOTIFICATION_FAILED + + def test_update_letter_notifications_to_sent_to_dvla_updates_based_on_notification_references( client, sample_letter_template From ab55650871eee80990394fdba93ff5779a97615e Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 24 Oct 2017 14:32:07 +0100 Subject: [PATCH 10/13] Reordered imports --- tests/app/celery/test_ftp_update_tasks.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/app/celery/test_ftp_update_tasks.py b/tests/app/celery/test_ftp_update_tasks.py index 6239d9285..639e18c9e 100644 --- a/tests/app/celery/test_ftp_update_tasks.py +++ b/tests/app/celery/test_ftp_update_tasks.py @@ -7,21 +7,21 @@ from flask import current_app from app.models import ( Job, Notification, + NOTIFICATION_CREATED, NOTIFICATION_DELIVERED, NOTIFICATION_FAILED, NOTIFICATION_SENDING, - NOTIFICATION_CREATED, - NOTIFICATION_TECHNICAL_FAILURE, - NOTIFICATION_STATUS_LETTER_RECEIVED + NOTIFICATION_STATUS_LETTER_RECEIVED, + NOTIFICATION_TECHNICAL_FAILURE ) from app.dao.notifications_dao import dao_update_notifications_by_reference from app.celery.tasks import ( - update_job_to_sent_to_dvla, + process_updates_from_file, update_dvla_job_to_error, + update_job_to_sent_to_dvla, update_letter_notifications_statuses, - update_letter_notifications_to_sent_to_dvla, update_letter_notifications_to_error, - process_updates_from_file + update_letter_notifications_to_sent_to_dvla ) from tests.app.db import create_notification From a8adf4d7d71337f793dc86149eb734ac648ead76 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 25 Oct 2017 11:58:54 +0100 Subject: [PATCH 11/13] This PR is to retain current behaviour when we allocate an inbound number for a service. When a service is allocated an inbound number and they only have one SMS sender, then update that SMS sender to the inbound number. That way they will not have more than one SMS sender and will not have to choose to use either one. --- app/dao/service_sms_sender_dao.py | 8 +++++ app/service/rest.py | 18 ++++++++-- tests/app/dao/test_service_sms_sender_dao.py | 31 ++++++++++++++-- tests/app/service/test_rest.py | 38 +++++++++++++++++--- 4 files changed, 86 insertions(+), 9 deletions(-) diff --git a/app/dao/service_sms_sender_dao.py b/app/dao/service_sms_sender_dao.py index 69397f441..4b89b7e8d 100644 --- a/app/dao/service_sms_sender_dao.py +++ b/app/dao/service_sms_sender_dao.py @@ -83,6 +83,14 @@ def dao_update_service_sms_sender(service_id, service_sms_sender_id, is_default, return sms_sender_to_update +@transactional +def update_existing_sms_sender_with_inbound_number(service_sms_sender, sms_sender, inbound_number_id): + service_sms_sender.sms_sender = sms_sender + service_sms_sender.inbound_number_id = inbound_number_id + db.session.add(service_sms_sender) + return service_sms_sender + + def _get_existing_default(service_id): sms_senders = dao_get_sms_senders_by_service_id(service_id=service_id) if sms_senders: diff --git a/app/service/rest.py b/app/service/rest.py index 3d2dc2827..005dbd8e9 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -28,8 +28,8 @@ from app.dao.service_sms_sender_dao import ( dao_add_sms_sender_for_service, dao_update_service_sms_sender, dao_get_service_sms_senders_by_id, - dao_get_sms_senders_by_service_id -) + dao_get_sms_senders_by_service_id, + update_existing_sms_sender_with_inbound_number) from app.dao.services_dao import ( dao_fetch_service_by_id, dao_fetch_all_services, @@ -654,10 +654,22 @@ def add_service_sms_sender(service_id): form = validate(request.get_json(), add_service_sms_sender_request) inbound_number_id = form.get('inbound_number_id', None) sms_sender = form.get('sms_sender') + if inbound_number_id: updated_number = dao_allocate_number_for_service(service_id=service_id, inbound_number_id=inbound_number_id) - # the sms_sender in the form is the inbound_number_id from client, use number from table. + # the sms_sender in the form is not set, use the inbound number sms_sender = updated_number.number + existing_sms_sender = dao_get_sms_senders_by_service_id(service_id) + # we don't want to create a new sms sender for the service if we are allocating an inbound number. + if len(existing_sms_sender) == 1: + update_existing_sms_sender = existing_sms_sender[0] + new_sms_sender = update_existing_sms_sender_with_inbound_number( + service_sms_sender=update_existing_sms_sender, + sms_sender=sms_sender, + inbound_number_id=inbound_number_id) + + return jsonify(new_sms_sender.serialize()), 201 + new_sms_sender = dao_add_sms_sender_for_service(service_id=service_id, sms_sender=sms_sender, is_default=form['is_default'], diff --git a/tests/app/dao/test_service_sms_sender_dao.py b/tests/app/dao/test_service_sms_sender_dao.py index 3dcec1469..c38ebe2bc 100644 --- a/tests/app/dao/test_service_sms_sender_dao.py +++ b/tests/app/dao/test_service_sms_sender_dao.py @@ -6,9 +6,12 @@ from sqlalchemy.exc import SQLAlchemyError from app.dao.service_sms_sender_dao import ( insert_or_update_service_sms_sender, dao_add_sms_sender_for_service, - dao_update_service_sms_sender, dao_get_service_sms_senders_by_id, dao_get_sms_senders_by_service_id) + dao_update_service_sms_sender, + dao_get_service_sms_senders_by_id, + dao_get_sms_senders_by_service_id, + update_existing_sms_sender_with_inbound_number) from app.models import ServiceSmsSender -from tests.app.db import create_service +from tests.app.db import create_service, create_inbound_number def test_update_service_sms_sender_updates_existing_row(notify_db_session): @@ -145,3 +148,27 @@ def test_dao_update_service_sms_sender_raises_exception_when_no_default_after_up service_sms_sender_id=sms_sender.id, is_default=False, sms_sender="updated") + + +def test_update_existing_sms_sender_with_inbound_number(notify_db_session): + service = create_service(sms_sender='testing') + inbound_number = create_inbound_number(number='12345', service_id=service.id) + + existing_sms_sender = ServiceSmsSender.query.filter_by(service_id=service.id).one() + sms_sender = update_existing_sms_sender_with_inbound_number( + service_sms_sender=existing_sms_sender, sms_sender=inbound_number.number, inbound_number_id=inbound_number.id) + + assert sms_sender.inbound_number_id == inbound_number.id + assert sms_sender.sms_sender == inbound_number.number + assert sms_sender.is_default + + +def test_update_existing_sms_sender_with_inbound_number_raises_exception_if_inbound_number_does_not_exist( + notify_db_session +): + service = create_service(sms_sender='testing') + existing_sms_sender = ServiceSmsSender.query.filter_by(service_id=service.id).one() + with pytest.raises(expected_exception=SQLAlchemyError): + update_existing_sms_sender_with_inbound_number(service_sms_sender=existing_sms_sender, + sms_sender='blah', + inbound_number_id=uuid.uuid4()) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 3e1483d1a..0ced56ec3 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2602,12 +2602,13 @@ def test_add_service_sms_sender_can_add_multiple_senders(client, notify_db_sessi assert len(senders) == 2 -def test_add_service_sms_sender_when_it_is_an_inbound_number(client, notify_db_session): - service = create_service() +def test_add_service_sms_sender_when_it_is_an_inbound_number_updates_the_only_existing_sms_sender( + client, notify_db_session): + service = create_service(sms_sender='GOVUK') inbound_number = create_inbound_number(number='12345') data = { "sms_sender": str(inbound_number.id), - "is_default": False, + "is_default": True, "inbound_number_id": str(inbound_number.id) } response = client.post('/service/{}/sms-sender'.format(service.id), @@ -2620,7 +2621,36 @@ def test_add_service_sms_sender_when_it_is_an_inbound_number(client, notify_db_s resp_json = json.loads(response.get_data(as_text=True)) assert resp_json['sms_sender'] == inbound_number.number assert resp_json['inbound_number_id'] == str(inbound_number.id) - assert not resp_json['is_default'] + assert resp_json['is_default'] + + senders = ServiceSmsSender.query.all() + assert len(senders) == 1 + + +def test_add_service_sms_sender_when_it_is_an_inbound_number_inserts_new_sms_sender_when_more_than_one( + client, notify_db_session): + service = create_service(sms_sender='GOVUK') + create_service_sms_sender(service=service, sms_sender="second", is_default=False) + inbound_number = create_inbound_number(number='12345') + data = { + "sms_sender": str(inbound_number.id), + "is_default": True, + "inbound_number_id": str(inbound_number.id) + } + response = client.post('/service/{}/sms-sender'.format(service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + assert response.status_code == 201 + updated_number = InboundNumber.query.get(inbound_number.id) + assert updated_number.service_id == service.id + resp_json = json.loads(response.get_data(as_text=True)) + assert resp_json['sms_sender'] == inbound_number.number + assert resp_json['inbound_number_id'] == str(inbound_number.id) + assert resp_json['is_default'] + + senders = ServiceSmsSender.query.filter_by(service_id=service.id).all() + assert len(senders) == 3 def test_add_service_sms_sender_switches_default(client, notify_db_session): From b40cab0c5dd77d697c8ea30cfef28aae4ad4c923 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 25 Oct 2017 15:38:58 +0100 Subject: [PATCH 12/13] Refactored models to make const consistent --- app/models.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/app/models.py b/app/models.py index 04decb50a..2eda29da8 100644 --- a/app/models.py +++ b/app/models.py @@ -875,10 +875,9 @@ NOTIFICATION_STATUS_TYPES_NON_BILLABLE = list(set(NOTIFICATION_STATUS_TYPES) - s NOTIFICATION_STATUS_TYPES_ENUM = db.Enum(*NOTIFICATION_STATUS_TYPES, name='notify_status_type') NOTIFICATION_STATUS_LETTER_ACCEPTED = 'accepted' -NOTIFICATION_STATUS_LETTER_ACCEPTED_PRETTY = 'Accepted' NOTIFICATION_STATUS_LETTER_RECEIVED = 'received' -DVLA_STATUS_SENT = 'Sent' +DVLA_RESPONSE_STATUS_SENT = 'Sent' class NotificationStatusTypes(db.Model): @@ -1054,8 +1053,8 @@ class Notification(db.Model): }, 'letter': { 'technical-failure': 'Technical failure', - 'sending': NOTIFICATION_STATUS_LETTER_ACCEPTED_PRETTY, - 'created': NOTIFICATION_STATUS_LETTER_ACCEPTED_PRETTY, + 'sending': 'Accepted', + 'created': 'Accepted', 'delivered': 'Received' } }[self.template.template_type].get(self.status, self.status) From 204f170de2d689e50f066461f990a287e23cf6c1 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 25 Oct 2017 15:39:54 +0100 Subject: [PATCH 13/13] Use NOTIFICATION_TECHNICAL_FAILURE not NOTIFICATION_FAILED --- app/celery/tasks.py | 6 +++--- tests/app/celery/test_ftp_update_tasks.py | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 52a5741f3..a042d7f0a 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -47,7 +47,7 @@ from app.dao.templates_dao import dao_get_template_by_id from app.models import ( Job, Notification, - DVLA_STATUS_SENT, + DVLA_RESPONSE_STATUS_SENT, EMAIL_TYPE, JOB_STATUS_CANCELLED, JOB_STATUS_FINISHED, @@ -58,7 +58,6 @@ from app.models import ( KEY_TYPE_NORMAL, LETTER_TYPE, NOTIFICATION_DELIVERED, - NOTIFICATION_FAILED, NOTIFICATION_SENDING, NOTIFICATION_TECHNICAL_FAILURE, SMS_TYPE, @@ -447,7 +446,8 @@ def update_letter_notifications_statuses(self, filename): raise else: for update in notification_updates: - status = NOTIFICATION_DELIVERED if update.status == DVLA_STATUS_SENT else NOTIFICATION_FAILED + status = NOTIFICATION_DELIVERED if update.status == DVLA_RESPONSE_STATUS_SENT \ + else NOTIFICATION_TECHNICAL_FAILURE notification = update_notification_status_by_reference( update.reference, status diff --git a/tests/app/celery/test_ftp_update_tasks.py b/tests/app/celery/test_ftp_update_tasks.py index 639e18c9e..eef0c8c36 100644 --- a/tests/app/celery/test_ftp_update_tasks.py +++ b/tests/app/celery/test_ftp_update_tasks.py @@ -9,7 +9,7 @@ from app.models import ( Notification, NOTIFICATION_CREATED, NOTIFICATION_DELIVERED, - NOTIFICATION_FAILED, + NOTIFICATION_TECHNICAL_FAILURE, NOTIFICATION_SENDING, NOTIFICATION_STATUS_LETTER_RECEIVED, NOTIFICATION_TECHNICAL_FAILURE @@ -106,7 +106,7 @@ def test_update_letter_notifications_statuses_persisted(notify_api, mocker, samp update_letter_notifications_statuses(filename='foo.txt') assert sent_letter.status == NOTIFICATION_DELIVERED - assert failed_letter.status == NOTIFICATION_FAILED + assert failed_letter.status == NOTIFICATION_TECHNICAL_FAILURE def test_update_letter_notifications_to_sent_to_dvla_updates_based_on_notification_references(