From 0dcf04def91bb796f4e1e8e753c3ee47dcb8132e Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Tue, 21 Aug 2018 16:45:10 +0100 Subject: [PATCH 1/3] Add notification status for returned letters We need to update letter notifications with a new status when DVLA gives us a list of references for returned letters. This adds the new status to the models and the DB. DVLA call this 'returned mail', so I'm using it as the status name since it seems less ambiguous than 'returned'. --- app/models.py | 8 +++++++- .../versions/0224_returned_letter_status.py | 20 +++++++++++++++++++ .../notification_dao/test_notification_dao.py | 12 +++++++++++ .../notifications/test_get_notifications.py | 2 +- .../test_notification_schemas.py | 4 ++-- 5 files changed, 42 insertions(+), 4 deletions(-) create mode 100644 migrations/versions/0224_returned_letter_status.py diff --git a/app/models.py b/app/models.py index 353b6e56c..f27c8f1e8 100644 --- a/app/models.py +++ b/app/models.py @@ -1050,12 +1050,14 @@ NOTIFICATION_TEMPORARY_FAILURE = 'temporary-failure' NOTIFICATION_PERMANENT_FAILURE = 'permanent-failure' NOTIFICATION_PENDING_VIRUS_CHECK = 'pending-virus-check' NOTIFICATION_VIRUS_SCAN_FAILED = 'virus-scan-failed' +NOTIFICATION_RETURNED_LETTER = 'returned-letter' NOTIFICATION_STATUS_TYPES_FAILED = [ NOTIFICATION_TECHNICAL_FAILURE, NOTIFICATION_TEMPORARY_FAILURE, NOTIFICATION_PERMANENT_FAILURE, NOTIFICATION_VIRUS_SCAN_FAILED, + NOTIFICATION_RETURNED_LETTER, ] NOTIFICATION_STATUS_TYPES_COMPLETED = [ @@ -1065,6 +1067,7 @@ NOTIFICATION_STATUS_TYPES_COMPLETED = [ NOTIFICATION_TECHNICAL_FAILURE, NOTIFICATION_TEMPORARY_FAILURE, NOTIFICATION_PERMANENT_FAILURE, + NOTIFICATION_RETURNED_LETTER, ] NOTIFICATION_STATUS_SUCCESS = [ @@ -1079,6 +1082,7 @@ NOTIFICATION_STATUS_TYPES_BILLABLE = [ NOTIFICATION_FAILED, NOTIFICATION_TEMPORARY_FAILURE, NOTIFICATION_PERMANENT_FAILURE, + NOTIFICATION_RETURNED_LETTER, ] NOTIFICATION_STATUS_TYPES = [ @@ -1094,6 +1098,7 @@ NOTIFICATION_STATUS_TYPES = [ NOTIFICATION_PERMANENT_FAILURE, NOTIFICATION_PENDING_VIRUS_CHECK, NOTIFICATION_VIRUS_SCAN_FAILED, + NOTIFICATION_RETURNED_LETTER, ] NOTIFICATION_STATUS_TYPES_NON_BILLABLE = list(set(NOTIFICATION_STATUS_TYPES) - set(NOTIFICATION_STATUS_TYPES_BILLABLE)) @@ -1286,7 +1291,8 @@ class Notification(db.Model): 'technical-failure': 'Technical failure', 'sending': 'Accepted', 'created': 'Accepted', - 'delivered': 'Received' + 'delivered': 'Received', + 'returned-letter': 'Returned', } }[self.template.template_type].get(self.status, self.status) diff --git a/migrations/versions/0224_returned_letter_status.py b/migrations/versions/0224_returned_letter_status.py new file mode 100644 index 000000000..29707c271 --- /dev/null +++ b/migrations/versions/0224_returned_letter_status.py @@ -0,0 +1,20 @@ +""" + +Revision ID: 0224_returned_letter_status +Revises: 0223_add_domain_constraint +Create Date: 2018-08-21 14:44:04.203480 + +""" +from alembic import op + + +revision = '0224_returned_letter_status' +down_revision = '0223_add_domain_constraint' + + +def upgrade(): + op.execute("INSERT INTO notification_status_types (name) VALUES ('returned-letter')") + + +def downgrade(): + op.execute("DELETE FROM notification_status_types WHERE name='returned-letter'") diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 3a0b4e324..5aa53b87a 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -1683,6 +1683,18 @@ def test_dao_update_notifications_by_reference_returns_zero_when_no_notification assert updated_count == 0 +def test_dao_update_notifications_by_reference_set_returned_letter_status(sample_letter_template): + notification = create_notification(template=sample_letter_template, reference='ref') + + updated_count = dao_update_notifications_by_reference( + references=['ref'], + update_dict={"status": "returned-letter"} + ) + + assert updated_count == 1 + assert Notification.query.get(notification.id).status == 'returned-letter' + + def test_dao_get_notification_by_reference_with_one_match_returns_notification(sample_letter_template, notify_db): create_notification(template=sample_letter_template, reference='REF1') notification = dao_get_notification_by_reference('REF1') diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index ef12a0fd0..18e2b72dc 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -443,7 +443,7 @@ def test_get_all_notifications_filter_by_status_invalid_status(client, sample_no assert len(json_response['errors']) == 1 assert json_response['errors'][0]['message'] == "status elephant is not one of [cancelled, created, sending, " \ "sent, delivered, pending, failed, technical-failure, temporary-failure, permanent-failure, " \ - "pending-virus-check, virus-scan-failed, accepted, received]" + "pending-virus-check, virus-scan-failed, returned-letter, accepted, received]" def test_get_all_notifications_filter_by_multiple_statuses(client, sample_template): diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py index db0b72490..cc2765789 100644 --- a/tests/app/v2/notifications/test_notification_schemas.py +++ b/tests/app/v2/notifications/test_notification_schemas.py @@ -44,7 +44,7 @@ def test_get_notifications_request_invalid_statuses( partial_error_status = "is not one of " \ "[cancelled, created, sending, sent, delivered, pending, failed, " \ "technical-failure, temporary-failure, permanent-failure, pending-virus-check, " \ - "virus-scan-failed, accepted, received]" + "virus-scan-failed, returned-letter, accepted, received]" with pytest.raises(ValidationError) as e: validate({'status': invalid_statuses + valid_statuses}, get_notifications_request) @@ -92,7 +92,7 @@ def test_get_notifications_request_invalid_statuses_and_template_types(): for invalid_status in ["elephant", "giraffe"]: assert "status {} is not one of [cancelled, created, sending, sent, delivered, " \ "pending, failed, technical-failure, temporary-failure, permanent-failure, " \ - "pending-virus-check, virus-scan-failed, accepted, received]".format( + "pending-virus-check, virus-scan-failed, returned-letter, accepted, received]".format( invalid_status ) in error_messages From 18ab7f33378b655af80e53ccbd7b0328c1436d90 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Thu, 30 Aug 2018 14:27:57 +0100 Subject: [PATCH 2/3] Add updated history count to dao_update_notifications_by_reference For returned letter updates most notifications won't exist in the notifications table, so in order to find out whether the reference matches any known letters we need to check the count of updated history records. --- app/celery/letters_pdf_tasks.py | 2 +- app/celery/tasks.py | 6 ++-- app/dao/notifications_dao.py | 4 +-- .../notification_dao/test_notification_dao.py | 28 +++++++++++++------ 4 files changed, 25 insertions(+), 15 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 8a65ba333..2df7d680e 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -288,7 +288,7 @@ def update_letter_pdf_status(reference, status): update_dict={ 'status': status, 'updated_at': datetime.utcnow() - }) + })[0] def replay_letters_in_error(filename=None): diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 3c2089942..bab249eea 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -339,7 +339,7 @@ def update_letter_notifications_to_sent_to_dvla(self, notification_references): # This task will be called by the FTP app to update notifications as sent to DVLA provider = get_current_provider(LETTER_TYPE) - updated_count = dao_update_notifications_by_reference( + updated_count, _ = dao_update_notifications_by_reference( notification_references, { 'status': NOTIFICATION_SENDING, @@ -357,7 +357,7 @@ def update_letter_notifications_to_sent_to_dvla(self, notification_references): def update_letter_notifications_to_error(self, notification_references): # This task will be called by the FTP app to update notifications as sent to DVLA - updated_count = dao_update_notifications_by_reference( + updated_count, _ = dao_update_notifications_by_reference( notification_references, { 'status': NOTIFICATION_TECHNICAL_FAILURE, @@ -465,7 +465,7 @@ def update_letter_notification(filename, temporary_failures, update): status = NOTIFICATION_TEMPORARY_FAILURE temporary_failures.append(update.reference) - updated_count = dao_update_notifications_by_reference( + updated_count, _ = dao_update_notifications_by_reference( references=[update.reference], update_dict={"status": status, "billable_units": update.page_count, diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 27d609562..1bc683813 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -465,14 +465,14 @@ def dao_update_notifications_by_reference(references, update_dict): synchronize_session=False ) - NotificationHistory.query.filter( + updated_history_count = NotificationHistory.query.filter( NotificationHistory.reference.in_(references) ).update( update_dict, synchronize_session=False ) - return updated_count + return updated_count, updated_history_count @statsd(namespace="dao") diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 5aa53b87a..00333179c 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -1653,10 +1653,13 @@ def test_dao_update_notifications_by_reference_updated_notificaitons_and_history notification_1 = create_notification(template=sample_template, reference='ref') notification_2 = create_notification(template=sample_template, reference='ref') - updated_count = dao_update_notifications_by_reference(references=['ref'], - update_dict={"status": "delivered", - "billable_units": 2} - ) + updated_count, updated_history_count = dao_update_notifications_by_reference( + references=['ref'], + update_dict={ + "status": "delivered", + "billable_units": 2 + } + ) assert updated_count == 2 updated_1 = Notification.query.get(notification_1.id) assert updated_1.billable_units == 2 @@ -1665,6 +1668,7 @@ def test_dao_update_notifications_by_reference_updated_notificaitons_and_history assert updated_2.billable_units == 2 assert updated_2.status == 'delivered' + assert updated_history_count == 2 updated_history_1 = NotificationHistory.query.get(notification_1.id) assert updated_history_1.billable_units == 2 assert updated_history_1.status == 'delivered' @@ -1676,22 +1680,28 @@ def test_dao_update_notifications_by_reference_updated_notificaitons_and_history def test_dao_update_notifications_by_reference_returns_zero_when_no_notifications_to_update(notify_db): - updated_count = dao_update_notifications_by_reference(references=['ref'], - update_dict={"status": "delivered", - "billable_units": 2} - ) + updated_count, updated_history_count = dao_update_notifications_by_reference( + references=['ref'], + update_dict={ + "status": "delivered", + "billable_units": 2 + } + ) + assert updated_count == 0 + assert updated_history_count == 0 def test_dao_update_notifications_by_reference_set_returned_letter_status(sample_letter_template): notification = create_notification(template=sample_letter_template, reference='ref') - updated_count = dao_update_notifications_by_reference( + updated_count, updated_history_count = dao_update_notifications_by_reference( references=['ref'], update_dict={"status": "returned-letter"} ) assert updated_count == 1 + assert updated_history_count == 1 assert Notification.query.get(notification.id).status == 'returned-letter' From 3787e2954bfcd39fe24382e54ce811581bed81fc Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Fri, 31 Aug 2018 16:49:06 +0100 Subject: [PATCH 3/3] Add a task to process returned letter lists Adds an API endpoint `/letters/returned` that accepts a list of notification references and creates a task to update their status. Adds a new task that uses the list of references to update the status of notifications to 'returned-letter'. The update is currently done using a single query and logs the number of changed records (including notification history records). This could potentially be done within the `/letters/returned` endpoint, but creating a job right away allows us to extend this more easily in the future (e.g. logging missing notifications or adding callbacks). The job is using the database tasks queue. --- app/celery/tasks.py | 16 +++++++++++++++ app/letters/letter_schemas.py | 19 ++++++++++++++++++ app/letters/rest.py | 12 ++++++++++- app/models.py | 2 +- tests/app/celery/test_tasks.py | 12 +++++++++++ tests/app/letters/test_returned_letters.py | 23 ++++++++++++++++++++++ 6 files changed, 82 insertions(+), 2 deletions(-) create mode 100644 tests/app/letters/test_returned_letters.py diff --git a/app/celery/tasks.py b/app/celery/tasks.py index bab249eea..7ab2b88a5 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -64,6 +64,7 @@ from app.models import ( NOTIFICATION_SENDING, NOTIFICATION_TEMPORARY_FAILURE, NOTIFICATION_TECHNICAL_FAILURE, + NOTIFICATION_RETURNED_LETTER, SMS_TYPE, DailySortedLetter, ) @@ -591,3 +592,18 @@ def process_incomplete_job(job_id): process_row(row, template, job, job.service) job_complete(job, resumed=True) + + +@notify_celery.task(name='process-returned-letters-list') +@statsd(namespace="tasks") +def process_returned_letters_list(notification_references): + updated, updated_history = dao_update_notifications_by_reference( + notification_references, + {"status": NOTIFICATION_RETURNED_LETTER} + ) + + current_app.logger.info( + "Updated {} letter notifications ({} history notifications, from {} references) to returned-letter".format( + updated, updated_history, len(notification_references) + ) + ) diff --git a/app/letters/letter_schemas.py b/app/letters/letter_schemas.py index 3827e4f4e..9a4d28958 100644 --- a/app/letters/letter_schemas.py +++ b/app/letters/letter_schemas.py @@ -13,3 +13,22 @@ letter_job_ids = { }, "required": ["job_ids"] } + + +letter_references = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "list of letter notification references", + "type": "object", + "title": "references", + "properties": { + "references": { + "type": "array", + "items": { + "type": "string", + "pattern": "^[0-9A-Z]{16}$" + }, + "minItems": 1 + }, + }, + "required": ["references"] +} diff --git a/app/letters/rest.py b/app/letters/rest.py index 611d5132d..37a9d6a2e 100644 --- a/app/letters/rest.py +++ b/app/letters/rest.py @@ -2,11 +2,12 @@ from flask import Blueprint, jsonify from flask import request from app import notify_celery +from app.celery.tasks import process_returned_letters_list from app.config import QueueNames, TaskNames from app.dao.jobs_dao import dao_get_all_letter_jobs from app.schemas import job_schema from app.v2.errors import register_errors -from app.letters.letter_schemas import letter_job_ids +from app.letters.letter_schemas import letter_job_ids, letter_references from app.schema_validation import validate letter_job = Blueprint("letter-job", __name__) @@ -27,3 +28,12 @@ def get_letter_jobs(): data = job_schema.dump(letter_jobs, many=True).data return jsonify(data=data), 200 + + +@letter_job.route('/letters/returned', methods=['POST']) +def create_process_returned_letters_job(): + references = validate(request.get_json(), letter_references) + + process_returned_letters_list.apply_async([references['references']], queue=QueueNames.DATABASE) + + return jsonify(references=references['references']), 200 diff --git a/app/models.py b/app/models.py index f27c8f1e8..767857ed9 100644 --- a/app/models.py +++ b/app/models.py @@ -1309,7 +1309,7 @@ class Notification(db.Model): if self.status in [NOTIFICATION_CREATED, NOTIFICATION_SENDING]: return NOTIFICATION_STATUS_LETTER_ACCEPTED - elif self.status == NOTIFICATION_DELIVERED: + elif self.status in [NOTIFICATION_DELIVERED, NOTIFICATION_RETURNED_LETTER]: return NOTIFICATION_STATUS_LETTER_RECEIVED else: # Currently can only be technical-failure diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 873285e00..d024787d0 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -27,6 +27,7 @@ from app.celery.tasks import ( get_template_class, s3, send_inbound_sms_to_service, + process_returned_letters_list, ) from app.config import QueueNames from app.dao import jobs_dao, services_dao @@ -1551,3 +1552,14 @@ def test_process_incomplete_jobs_sets_status_to_in_progress_and_resets_processin assert job2.processing_started == datetime.utcnow() assert mock_process_incomplete_job.mock_calls == [call(str(job1.id)), call(str(job2.id))] + + +def test_process_returned_letters_list(mocker, sample_letter_template): + create_notification(sample_letter_template, reference='ref1') + create_notification(sample_letter_template, reference='ref2') + + process_returned_letters_list(['ref1', 'ref2', 'unknown-ref']) + + assert [ + n.status for n in Notification.query.all() + ] == ['returned-letter', 'returned-letter'] diff --git a/tests/app/letters/test_returned_letters.py b/tests/app/letters/test_returned_letters.py new file mode 100644 index 000000000..a5e6dc6f9 --- /dev/null +++ b/tests/app/letters/test_returned_letters.py @@ -0,0 +1,23 @@ +import pytest + + +@pytest.mark.parametrize('status, references', [ + (200, ["1234567890ABCDEF", "1234567890ABCDEG"]), + (400, ["1234567890ABCDEFG", "1234567890ABCDEG"]), + (400, ["1234567890ABCDE", "1234567890ABCDEG"]), + (400, ["1234567890ABCDE\u26d4", "1234567890ABCDEG"]), + (400, ["NOTIFY0001234567890ABCDEF", "1234567890ABCDEG"]), +]) +def test_process_returned_letters(status, references, admin_request, mocker): + mock_celery = mocker.patch("app.letters.rest.process_returned_letters_list.apply_async") + + response = admin_request.post( + 'letter-job.create_process_returned_letters_job', + _data={"references": references}, + _expected_status=status + ) + + if status != 200: + assert '{} does not match'.format(references[0]) in response['errors'][0]['message'] + else: + mock_celery.assert_called_once_with([references], queue='database-tasks')