Merge pull request #1580 from alphagov/update-dvla-response-process

Process the new post printing response file from DVLA

app/celery/tasks.py

@@ -45,6 +45,7 @@ from app.dao.notifications_dao import (
     dao_update_notifications_for_job_to_sent_to_dvla,
     dao_update_notifications_by_reference,
     dao_get_last_notification_added_for_job_id,
+    dao_get_notification_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
@@ -63,6 +64,7 @@ from app.models import (
     LETTER_TYPE,
     NOTIFICATION_DELIVERED,
     NOTIFICATION_SENDING,
+    NOTIFICATION_TEMPORARY_FAILURE,
     NOTIFICATION_TECHNICAL_FAILURE,
     SMS_TYPE,
 )
@@ -475,8 +477,10 @@ def update_letter_notifications_statuses(self, filename):
         raise
     else:
         for update in notification_updates:
+            check_billable_units(update)
+
             status = NOTIFICATION_DELIVERED if update.status == DVLA_RESPONSE_STATUS_SENT \
-                else NOTIFICATION_TECHNICAL_FAILURE
+                else NOTIFICATION_TEMPORARY_FAILURE
             updated_count = dao_update_notifications_by_reference(
                 references=[update.reference],
                 update_dict={"status": status,
@@ -502,6 +506,16 @@ def process_updates_from_file(response_file):
     return notification_updates
 
 
+def check_billable_units(notification_update):
+    notification = dao_get_notification_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(
+            notification.id, notification.billable_units, notification_update.page_count)
+
+        current_app.logger.error(msg)
+
+
 @notify_celery.task(bind=True, name="send-inbound-sms", max_retries=5, default_retry_delay=300)
 @statsd(namespace="tasks")
 def send_inbound_sms_to_service(self, inbound_sms_id, service_id):

app/dao/notifications_dao.py

@@ -471,6 +471,13 @@ def dao_get_notifications_by_to_field(service_id, search_term, statuses=None):
     return results
 
 
+@statsd(namespace="dao")
+def dao_get_notification_by_reference(reference):
+    return Notification.query.filter(
+        Notification.reference == reference
+    ).one()
+
+
 @statsd(namespace="dao")
 def dao_get_notifications_by_references(references):
     return Notification.query.filter(

app/notifications/notifications_letter_callback.py

@@ -54,7 +54,8 @@ def process_letter_response():
         filename = message['Records'][0]['s3']['object']['key']
         current_app.logger.info('Received file from DVLA: {}'.format(filename))
 
-        if 'rs.txt' in filename.lower():
+        if filename.lower().endswith('rs.txt') or filename.lower().endswith('rsp.txt'):
+            current_app.logger.info('DVLA callback: Calling task to update letter notifications')
             update_letter_notifications_statuses.apply_async([filename], queue=QueueNames.NOTIFY)
 
     return jsonify(

tests/app/celery/test_ftp_update_tasks.py

@@ -1,3 +1,4 @@
+from collections import namedtuple
 from datetime import datetime
 
 import pytest
@@ -10,9 +11,11 @@ from app.models import (
     NOTIFICATION_CREATED,
     NOTIFICATION_DELIVERED,
     NOTIFICATION_SENDING,
+    NOTIFICATION_TEMPORARY_FAILURE,
     NOTIFICATION_TECHNICAL_FAILURE
 )
 from app.celery.tasks import (
+    check_billable_units,
     process_updates_from_file,
     update_dvla_job_to_error,
     update_job_to_sent_to_dvla,
@@ -25,6 +28,15 @@ from tests.app.db import create_notification, create_service_callback_api
 from tests.conftest import set_config
 
 
+@pytest.fixture
+def notification_update():
+    """
+    Returns a namedtuple to use as the argument for the check_billable_units function
+    """
+    NotificationUpdate = namedtuple('NotificationUpdate', ['reference', 'status', 'page_count', 'cost_threshold'])
+    return NotificationUpdate('REFERENCE_ABC', 'sent', '1', 'cost')
+
+
 def test_update_job_to_sent_to_dvla(sample_letter_template, sample_letter_job):
     create_notification(template=sample_letter_template, job=sample_letter_job)
     create_notification(template=sample_letter_template, job=sample_letter_job)
@@ -107,7 +119,7 @@ def test_update_letter_notifications_statuses_persisted(notify_api, mocker, samp
     assert sent_letter.status == NOTIFICATION_DELIVERED
     assert sent_letter.billable_units == 1
     assert sent_letter.updated_at
-    assert failed_letter.status == NOTIFICATION_TECHNICAL_FAILURE
+    assert failed_letter.status == NOTIFICATION_TEMPORARY_FAILURE
     assert failed_letter.billable_units == 2
     assert failed_letter.updated_at
 
@@ -162,3 +174,37 @@ def test_update_letter_notifications_to_error_updates_based_on_notification_refe
     assert first.sent_at is None
     assert first.updated_at == dt
     assert second.status == NOTIFICATION_CREATED
+
+
+def test_check_billable_units_when_billable_units_matches_page_count(
+    client,
+    sample_letter_template,
+    mocker,
+    notification_update
+):
+    mock_logger = mocker.patch('app.celery.tasks.current_app.logger.error')
+
+    notification = create_notification(sample_letter_template, reference='REFERENCE_ABC')
+    notification.billable_units = 1
+
+    check_billable_units(notification_update)
+
+    mock_logger.assert_not_called()
+
+
+def test_check_billable_units_when_billable_units_does_not_match_page_count(
+    client,
+    sample_letter_template,
+    mocker,
+    notification_update
+):
+    mock_logger = mocker.patch('app.celery.tasks.current_app.logger.error')
+
+    notification = create_notification(sample_letter_template, reference='REFERENCE_ABC')
+    notification.billable_units = 3
+
+    check_billable_units(notification_update)
+
+    mock_logger.assert_called_once_with(
+        'Notification with id {} had 3 billable_units but a page count of 1'.format(notification.id)
+    )

tests/app/dao/notification_dao/test_notification_dao.py

@@ -31,6 +31,7 @@ from app.dao.notifications_dao import (
     set_scheduled_notification_to_processed,
     update_notification_status_by_id,
     update_notification_status_by_reference,
+    dao_get_notification_by_reference,
     dao_get_notifications_by_references
 )
 from app.dao.services_dao import dao_update_service
@@ -1994,6 +1995,26 @@ def test_dao_update_notifications_by_reference_returns_zero_when_no_notification
     assert updated_count == 0
 
 
+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')
+
+    assert notification.reference == 'REF1'
+
+
+def test_dao_get_notification_by_reference_with_multiple_matches_raises_error(sample_letter_template, notify_db):
+    create_notification(template=sample_letter_template, reference='REF1')
+    create_notification(template=sample_letter_template, reference='REF1')
+
+    with pytest.raises(SQLAlchemyError):
+        dao_get_notification_by_reference('REF1')
+
+
+def test_dao_get_notification_by_reference_with_no_matches_raises_error(notify_db):
+    with pytest.raises(SQLAlchemyError):
+        dao_get_notification_by_reference('REF1')
+
+
 def test_dao_get_notifications_by_reference(sample_template):
     create_notification(template=sample_template, reference='noref')
     notification_1 = create_notification(template=sample_template, reference='ref')

tests/app/notifications/rest/test_callbacks.py

@@ -69,21 +69,43 @@ def test_dvla_callback_autoconfirm_does_not_call_update_letter_notifications_tas
     assert not update_task.called
 
 
-def test_dvla_callback_calls_update_letter_notifications_task(client, mocker):
+def test_dvla_callback_calls_does_not_update_letter_notifications_task_with_invalid_file_type(client, mocker):
     update_task = \
         mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async')
-    data = _sample_sns_s3_dvla_response_callback()
+
+    data = _sample_sns_s3_callback("bar.txt")
+    response = dvla_post(client, data)
+
+    assert response.status_code == 200
+    assert not update_task.called
+
+
+def test_dvla_rs_txt_file_callback_calls_update_letter_notifications_task(client, mocker):
+    update_task = \
+        mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async')
+    data = _sample_sns_s3_callback('Notify-20170411153023-rs.txt')
     response = dvla_post(client, data)
 
     assert response.status_code == 200
     assert update_task.called
-    update_task.assert_called_with(['bar.rs.txt'], queue='notify-internal-tasks')
+    update_task.assert_called_with(['Notify-20170411153023-rs.txt'], queue='notify-internal-tasks')
+
+
+def test_dvla_rsp_txt_file_callback_calls_update_letter_notifications_task(client, mocker):
+    update_task = \
+        mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async')
+    data = _sample_sns_s3_callback('NOTIFY.20170823160812.RSP.TXT')
+    response = dvla_post(client, data)
+
+    assert response.status_code == 200
+    assert update_task.called
+    update_task.assert_called_with(['NOTIFY.20170823160812.RSP.TXT'], queue='notify-internal-tasks')
 
 
 def test_dvla_ack_calls_does_not_call_letter_notifications_task(client, mocker):
     update_task = \
         mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async')
-    data = _sample_sns_s3_dvla_ack()
+    data = _sample_sns_s3_callback('bar.ack.txt')
     response = dvla_post(client, data)
 
     assert response.status_code == 200
@@ -462,7 +484,9 @@ def test_firetext_callback_should_record_statsd(client, notify_db, notify_db_ses
         app.statsd_client.incr.assert_any_call("callback.firetext.delivered")
 
 
-def _sample_sns_s3_dvla_ack():
+def _sample_sns_s3_callback(filename):
+    message_contents = '''{"Records":[{"eventVersion":"2.0","eventSource":"aws:s3","awsRegion":"eu-west-1","eventTime":"2017-05-16T11:38:41.073Z","eventName":"ObjectCreated:Put","userIdentity":{"principalId":"some-p-id"},"requestParameters":{"sourceIPAddress":"8.8.8.8"},"responseElements":{"x-amz-request-id":"some-r-id","x-amz-id-2":"some-x-am-id"},"s3":{"s3SchemaVersion":"1.0","configurationId":"some-c-id","bucket":{"name":"some-bucket","ownerIdentity":{"principalId":"some-p-id"},"arn":"some-bucket-arn"},
+            "object":{"key":"%s"}}}]}''' % (filename)  # noqa
     return json.dumps({
         "SigningCertURL": "foo.pem",
         "UnsubscribeURL": "bar",
@@ -473,22 +497,7 @@ def _sample_sns_s3_dvla_ack():
         "MessageId": "6adbfe0a-d610-509a-9c47-af894e90d32d",
         "Subject": "Amazon S3 Notification",
         "TopicArn": "sample-topic-arn",
-        "Message": '{"Records":[{"eventVersion":"2.0","eventSource":"aws:s3","awsRegion":"eu-west-1","eventTime":"2017-05-16T11:38:41.073Z","eventName":"ObjectCreated:Put","userIdentity":{"principalId":"some-p-id"},"requestParameters":{"sourceIPAddress":"8.8.8.8"},"responseElements":{"x-amz-request-id":"some-r-id","x-amz-id-2":"some-x-am-id"},"s3":{"s3SchemaVersion":"1.0","configurationId":"some-c-id","bucket":{"name":"some-bucket","ownerIdentity":{"principalId":"some-p-id"},"arn":"some-bucket-arn"},"object":{"key":"bar.ack.txt","size":200,"eTag":"some-e-tag","versionId":"some-v-id","sequencer":"some-seq"}}}]}'  # noqa
-    })
-
-
-def _sample_sns_s3_dvla_response_callback():
-    return json.dumps({
-        "SigningCertURL": "foo.pem",
-        "UnsubscribeURL": "bar",
-        "Signature": "some-signature",
-        "Type": "Notification",
-        "Timestamp": "2016-05-03T08:35:12.884Z",
-        "SignatureVersion": "1",
-        "MessageId": "6adbfe0a-d610-509a-9c47-af894e90d32d",
-        "Subject": "Amazon S3 Notification",
-        "TopicArn": "sample-topic-arn",
-        "Message": '{"Records":[{"eventVersion":"2.0","eventSource":"aws:s3","awsRegion":"eu-west-1","eventTime":"2017-05-16T11:38:41.073Z","eventName":"ObjectCreated:Put","userIdentity":{"principalId":"some-p-id"},"requestParameters":{"sourceIPAddress":"8.8.8.8"},"responseElements":{"x-amz-request-id":"some-r-id","x-amz-id-2":"some-x-am-id"},"s3":{"s3SchemaVersion":"1.0","configurationId":"some-c-id","bucket":{"name":"some-bucket","ownerIdentity":{"principalId":"some-p-id"},"arn":"some-bucket-arn"},"object":{"key":"bar.rs.txt","size":200,"eTag":"some-e-tag","versionId":"some-v-id","sequencer":"some-seq"}}}]}'  # noqa
+        "Message": message_contents
     })