From b0e5062df2a9b774a355fe49cf51e98384c94744 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 12 Apr 2017 17:56:55 +0100 Subject: [PATCH] Added the random string reference to the letter - uses the reference field on the notifications table to store a 16char random string used to cross reference DVLA letters back to the notification - used as letter barcode does not have space for a UUID notification id Depends on https://github.com/alphagov/notifications-utils/pull/149 Renamed the numeric_id to notification_reference in utils and changed validation rules to match this Note also the persist_notification method set "reference" to be "client_reference" which is confusing and they are different things, so fixed this too. --- app/__init__.py | 6 +++++ app/celery/tasks.py | 10 ++++----- app/notifications/process_notifications.py | 4 +++- app/v2/notifications/post_notifications.py | 2 +- tests/app/celery/test_tasks.py | 22 ++++++++++++------- .../test_process_notification.py | 2 +- 6 files changed, 30 insertions(+), 16 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 2e962f79b..b64f1e745 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -1,4 +1,6 @@ import os +import random +import string import uuid from flask import Flask, _request_ctx_stack @@ -204,6 +206,10 @@ def create_uuid(): return str(uuid.uuid4()) +def create_random_identifier(): + return ''.join(random.choice(string.ascii_letters + string.digits) for _ in range(16)) + + def process_user_agent(user_agent_string): if user_agent_string and user_agent_string.lower().startswith("notify"): components = user_agent_string.split("/") diff --git a/app/celery/tasks.py b/app/celery/tasks.py index b43daaa70..167f1669c 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -10,6 +10,7 @@ from notifications_utils.template import SMSMessageTemplate, WithSubjectTemplate from sqlalchemy.exc import SQLAlchemyError from app import ( create_uuid, + create_random_identifier, DATETIME_FORMAT, notify_celery, encryption @@ -262,7 +263,8 @@ def persist_letter( created_at=created_at, job_id=notification['job'], job_row_number=notification['row_number'], - notification_id=notification_id + notification_id=notification_id, + reference=create_random_identifier() ) current_app.logger.info("Letter {} created at {}".format(saved_notification.id, created_at)) @@ -311,10 +313,8 @@ def create_dvla_file_contents(job_id): str(LetterDVLATemplate( notification.template.__dict__, notification.personalisation, - # This unique id is a 7 digits requested by DVLA, not known - # if this number needs to be sequential. - numeric_id=random.randint(1, int('9' * 7)), - contact_block=notification.service.letter_contact_block, + notification_reference=notification.reference, + contact_block=notification.service.letter_contact_block )) for notification in dao_get_all_notifications_for_job(job_id) ) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 3de694b0c..31d8b8c1b 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -36,6 +36,7 @@ def persist_notification(template_id, job_id=None, job_row_number=None, reference=None, + client_reference=None, notification_id=None, simulated=False): # if simulated create a Notification model to return but do not persist the Notification to the dB @@ -53,7 +54,8 @@ def persist_notification(template_id, created_at=created_at or datetime.utcnow(), job_id=job_id, job_row_number=job_row_number, - client_reference=reference + client_reference=client_reference, + reference=reference ) if not simulated: dao_create_notification(notification) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index c3cbfdabf..5f046edb0 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -47,7 +47,7 @@ def post_notification(notification_type): notification_type=notification_type, api_key_id=api_user.id, key_type=api_user.key_type, - reference=form.get('reference', None), + client_reference=form.get('reference', None), simulated=simulated) if not simulated: queue_name = 'priority' if template.process_type == PRIORITY else None diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 2dba7e4ca..818ad4fc4 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -907,6 +907,9 @@ def test_send_sms_does_not_send_duplicate_and_does_not_put_in_retry_queue(sample def test_persist_letter_saves_letter_to_database(sample_letter_job, mocker): + + mocker.patch('app.celery.tasks.create_random_identifier', return_value="this-is-random-in-real-life") + personalisation = { 'addressline1': 'Foo', 'addressline2': 'Bar', @@ -945,6 +948,7 @@ def test_persist_letter_saves_letter_to_database(sample_letter_job, mocker): assert notification_db.sent_at is None assert notification_db.sent_by is None assert notification_db.personalisation == personalisation + assert notification_db.reference == "this-is-random-in-real-life" def test_should_cancel_job_if_service_is_inactive(sample_service, @@ -1013,24 +1017,26 @@ def test_build_dvla_file_retries_if_all_notifications_are_not_created(sample_let def test_create_dvla_file_contents(sample_letter_template, mocker): - mocker.patch("app.celery.tasks.random.randint", return_value=999) job = create_job(template=sample_letter_template, notification_count=2) - create_notification(template=job.template, job=job) - create_notification(template=job.template, job=job) + create_notification(template=job.template, job=job, reference=1) + create_notification(template=job.template, job=job, reference=2) mocked_letter_template = mocker.patch("app.celery.tasks.LetterDVLATemplate") mocked_letter_template_instance = mocked_letter_template.return_value mocked_letter_template_instance.__str__.return_value = "dvla|string" + create_dvla_file_contents(job.id) + calls = mocked_letter_template.call_args_list # Template - assert mocked_letter_template.call_args[0][0]['subject'] == 'Template subject' - assert mocked_letter_template.call_args[0][0]['content'] == 'Dear Sir/Madam, Hello. Yours Truly, The Government.' + assert calls[0][0][0]['subject'] == 'Template subject' + assert calls[0][0][0]['content'] == 'Dear Sir/Madam, Hello. Yours Truly, The Government.' # Personalisation - assert mocked_letter_template.call_args[0][1] is None + assert not calls[0][0][1] # Named arguments - assert mocked_letter_template.call_args[1]['numeric_id'] == 999 - assert mocked_letter_template.call_args[1]['contact_block'] == 'London,\nSW1A 1AA' + assert calls[1][1]['contact_block'] == 'London,\nSW1A 1AA' + assert calls[0][1]['notification_reference'] == '1' + assert calls[1][1]['notification_reference'] == '2' @freeze_time("2017-03-23 11:09:00.061258") diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index caacde72b..9e428a0ab 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -179,7 +179,7 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker) created_at=created_at, job_id=sample_job.id, job_row_number=10, - reference="ref from client", + client_reference="ref from client", notification_id=n_id) assert Notification.query.count() == 1 assert NotificationHistory.query.count() == 1