From a2b42194cdffa4e656b0d8c70e0becb2fcde0672 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 23 Oct 2017 15:57:00 +0100 Subject: [PATCH 1/7] 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 2/7] 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 3/7] 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 4/7] 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 5/7] 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 b40cab0c5dd77d697c8ea30cfef28aae4ad4c923 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 25 Oct 2017 15:38:58 +0100 Subject: [PATCH 6/7] 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 7/7] 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(