From c474b2312bde5eaaa11cbb72c620b3fc126a4c58 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 1 Mar 2018 15:39:51 +0000 Subject: [PATCH] Process responses for letters even after the notification has been deleted. This will continue to update the notification history for letter notifications. We currently have an issue where the responses to letters from the provider is taking a long time. This is due to the manual nature of their process. Updating the status of the letter will still work if the notification has been purged. Also turned back on the purge letter notification scheduled task. --- app/celery/tasks.py | 4 +-- app/config.py | 11 ++++---- app/dao/notifications_dao.py | 7 +++++ tests/app/celery/test_ftp_update_tasks.py | 25 +++++++++++++---- .../notification_dao/test_notification_dao.py | 27 ++++++++++++++++++- 5 files changed, 60 insertions(+), 14 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 428989f66..23b4fbd63 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -40,8 +40,8 @@ from app.dao.notifications_dao import ( get_notification_by_id, dao_update_notifications_by_reference, dao_get_last_notification_added_for_job_id, - dao_get_notification_by_reference, update_notification_status_by_reference, + dao_get_notification_history_by_reference, ) from app.dao.provider_details_dao import get_current_provider from app.dao.service_inbound_api_dao import get_service_inbound_api_for_service @@ -449,7 +449,7 @@ def update_letter_notification(filename, temporary_failures, update): def check_billable_units(notification_update): - notification = dao_get_notification_by_reference(notification_update.reference) + notification = dao_get_notification_history_by_reference(notification_update.reference) if int(notification_update.page_count) != notification.billable_units: msg = 'Notification with id {} had {} billable_units but a page count of {}'.format( diff --git a/app/config.py b/app/config.py index 7b2a5a75e..f9b125be1 100644 --- a/app/config.py +++ b/app/config.py @@ -188,12 +188,11 @@ class Config(object): 'schedule': crontab(hour=0, minute=20), 'options': {'queue': QueueNames.PERIODIC} }, - # Suspending the scheduled job to delete letter notifications, because there is a problem with the provider. - # 'delete-letter-notifications': { - # 'task': 'delete-letter-notifications', - # 'schedule': crontab(hour=0, minute=40), - # 'options': {'queue': QueueNames.PERIODIC} - # }, + 'delete-letter-notifications': { + 'task': 'delete-letter-notifications', + 'schedule': crontab(hour=0, minute=40), + 'options': {'queue': QueueNames.PERIODIC} + }, 'delete-inbound-sms': { 'task': 'delete-inbound-sms', 'schedule': crontab(hour=1, minute=0), diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index b79f4d0d5..155616801 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -463,6 +463,13 @@ def dao_get_notification_by_reference(reference): ).one() +@statsd(namespace="dao") +def dao_get_notification_history_by_reference(reference): + return NotificationHistory.query.filter( + NotificationHistory.reference == reference + ).one() + + @statsd(namespace="dao") def dao_get_notifications_by_references(references): return Notification.query.filter( diff --git a/tests/app/celery/test_ftp_update_tasks.py b/tests/app/celery/test_ftp_update_tasks.py index 9a2829334..05b73329f 100644 --- a/tests/app/celery/test_ftp_update_tasks.py +++ b/tests/app/celery/test_ftp_update_tasks.py @@ -9,11 +9,12 @@ from app.exceptions import DVLAException from app.models import ( Job, Notification, + NotificationHistory, NOTIFICATION_CREATED, NOTIFICATION_DELIVERED, NOTIFICATION_SENDING, NOTIFICATION_TEMPORARY_FAILURE, - NOTIFICATION_TECHNICAL_FAILURE + NOTIFICATION_TECHNICAL_FAILURE, ) from app.celery.tasks import ( check_billable_units, @@ -59,6 +60,22 @@ def test_update_letter_notifications_statuses_raises_for_invalid_format(notify_a assert 'DVLA response file: {} has an invalid format'.format('foo.txt') in str(e) +def test_update_letter_notification_statuses_when_notification_does_not_exist_updates_notification_history( + sample_letter_template, + mocker +): + valid_file = 'ref-foo|Sent|1|Unsorted' + mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file) + notification = create_notification(sample_letter_template, reference='ref-foo', status=NOTIFICATION_SENDING, + billable_units=1) + Notification.query.filter_by(id=notification.id).delete() + + update_letter_notifications_statuses(filename="older_than_7_days.txt") + + updated_history = NotificationHistory.query.filter_by(id=notification.id).one() + assert updated_history.status == NOTIFICATION_DELIVERED + + def test_update_letter_notifications_statuses_raises_dvla_exception(notify_api, mocker, sample_letter_template): valid_file = 'ref-foo|Failed|1|Unsorted' mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file) @@ -191,8 +208,7 @@ def test_check_billable_units_when_billable_units_matches_page_count( ): mock_logger = mocker.patch('app.celery.tasks.current_app.logger.error') - notification = create_notification(sample_letter_template, reference='REFERENCE_ABC') - notification.billable_units = 1 + create_notification(sample_letter_template, reference='REFERENCE_ABC', billable_units=1) check_billable_units(notification_update) @@ -207,8 +223,7 @@ def test_check_billable_units_when_billable_units_does_not_match_page_count( ): mock_logger = mocker.patch('app.celery.tasks.current_app.logger.error') - notification = create_notification(sample_letter_template, reference='REFERENCE_ABC') - notification.billable_units = 3 + notification = create_notification(sample_letter_template, reference='REFERENCE_ABC', billable_units=3) check_billable_units(notification_update) diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index c70298f43..ac99e0607 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -31,7 +31,8 @@ from app.dao.notifications_dao import ( update_notification_status_by_id, update_notification_status_by_reference, dao_get_notification_by_reference, - dao_get_notifications_by_references + dao_get_notifications_by_references, + dao_get_notification_history_by_reference, ) from app.dao.services_dao import dao_update_service from app.models import ( @@ -1998,6 +1999,30 @@ def test_dao_get_notifications_by_reference(sample_template): assert notifications[1].id in [notification_1.id, notification_2.id] +def test_dao_get_notification_history_by_reference_with_one_match_returns_notification( + sample_letter_template +): + create_notification(template=sample_letter_template, reference='REF1') + notification = dao_get_notification_history_by_reference('REF1') + + assert notification.reference == 'REF1' + + +def test_dao_get_notification_history_by_reference_with_multiple_matches_raises_error( + sample_letter_template +): + create_notification(template=sample_letter_template, reference='REF1') + create_notification(template=sample_letter_template, reference='REF1') + + with pytest.raises(SQLAlchemyError): + dao_get_notification_history_by_reference('REF1') + + +def test_dao_get_notification_history_by_reference_with_no_matches_raises_error(notify_db): + with pytest.raises(SQLAlchemyError): + dao_get_notification_history_by_reference('REF1') + + @freeze_time("2017-12-18 17:50") def test_dao_get_count_of_letters_to_process_for_today(sample_letter_template): # expected