From 7a412873d3802370976cc0508f10bece4f0faec3 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 3 Apr 2017 11:51:47 +0100 Subject: [PATCH 1/4] Refactor for better string concatenation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Using `+` to concatenate strings isn’t very memory efficient. Not sure if there are real-world implications for how it’s being used here, but can’t hurt to use `.join` instead. Rewriting it as a generator also lets us remove some unneeded variable assignment. --- app/celery/tasks.py | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index a868a76dc..96fbc1d53 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -267,18 +267,22 @@ def persist_letter( def build_dvla_file(self, job_id): try: if all_notifications_are_created_for_job(job_id): - notifications = dao_get_all_notifications_for_job(job_id) - file = "" - for n in notifications: - t = {"content": n.template.content, "subject": n.template.subject} - # This unique id is a 7 digits requested by DVLA, not known if this number needs to be sequential. - unique_id = int(''.join(map(str, random.sample(range(9), 7)))) - template = LetterDVLATemplate(t, n.personalisation, unique_id) - file = file + str(template) + "\n" - s3upload(filedata=file, - region=current_app.config['AWS_REGION'], - bucket_name=current_app.config['DVLA_UPLOAD_BUCKET_NAME'], - file_location="{}-dvla-job.text".format(job_id)) + file_contents = '\n'.join( + 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=int(''.join(map(str, random.sample(range(9), 7)))), + )) + for notification in dao_get_all_notifications_for_job(job_id) + ) + s3upload( + filedata=file_contents + '\n', + region=current_app.config['AWS_REGION'], + bucket_name=current_app.config['DVLA_UPLOAD_BUCKET_NAME'], + file_location="{}-dvla-job.text".format(job_id) + ) else: current_app.logger.info("All notifications for job {} are not persisted".format(job_id)) self.retry(queue="retry", exc="All notifications for job {} are not persisted".format(job_id)) From c4c3268dd9ce94a397c07cc028c56d33e7756b56 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 3 Apr 2017 11:55:30 +0100 Subject: [PATCH 2/4] Use a simpler method of generating random number MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rather than generating each digit of the number, generate the whole random number in one go. Should be faster, and is a bit easier to read. Don’t need to worry about it not being zero-padded because the `Template` class handles this here: https://github.com/alphagov/notifications-utils/blob/6ddd2ff352b48aa534eba023574bb89dd037fde6/notifications_utils/template.py#L410 --- app/celery/tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 96fbc1d53..f746512be 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -273,7 +273,7 @@ def build_dvla_file(self, job_id): notification.personalisation, # This unique id is a 7 digits requested by DVLA, not known # if this number needs to be sequential. - numeric_id=int(''.join(map(str, random.sample(range(9), 7)))), + numeric_id=random.randint(1, int('9' * 7)), )) for notification in dao_get_all_notifications_for_job(job_id) ) From eb5f9421dd301b14a88c7721ac7d623a2f61e5df Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 3 Apr 2017 12:20:49 +0100 Subject: [PATCH 3/4] Check for what DVLALetterTemplate is called with Extends the test to make sure that the thing that builds each line of the file is getting called with the right template, personalisation and numeric ID. Will be helpful the more complicated the call to the template gets. --- tests/app/celery/test_tasks.py | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index eae030386..80b83756c 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -971,17 +971,29 @@ def test_build_dvla_file(sample_letter_template, mocker): create_notification(template=job.template, job=job) create_notification(template=job.template, job=job) - mocked = mocker.patch("app.celery.tasks.s3upload") - mocker.patch("app.celery.tasks.LetterDVLATemplate.__str__", return_value="dvla|string") + mocker.patch("app.celery.tasks.random.randint", return_value=999) + mocked_upload = mocker.patch("app.celery.tasks.s3upload") + 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" build_dvla_file(job.id) - file = "dvla|string\ndvla|string\n" + mocked_upload.assert_called_once_with( + filedata="dvla|string\ndvla|string\n", + region=current_app.config['AWS_REGION'], + bucket_name=current_app.config['DVLA_UPLOAD_BUCKET_NAME'], + file_location="{}-dvla-job.text".format(job.id) + ) - assert mocked.called - mocked.assert_called_once_with(filedata=file, - region=current_app.config['AWS_REGION'], - bucket_name=current_app.config['DVLA_UPLOAD_BUCKET_NAME'], - file_location="{}-dvla-job.text".format(job.id)) + # 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.' + + # Personalisation + assert mocked_letter_template.call_args[0][1] is None + + # Named arguments + assert mocked_letter_template.call_args[1]['numeric_id'] == 999 def test_build_dvla_file_retries_if_all_notifications_are_not_created(sample_letter_template, mocker): From e47a008dd9ec5479e3d74f98f6f28e6d3d7c6ab3 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 3 Apr 2017 12:26:28 +0100 Subject: [PATCH 4/4] Pass through contact block from service to letter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Whatever a user has entered for their service’s contact block should appear in the right place in the file we give to DVLA. The work to output in the right fields in the DVLA file has already been done. We just weren’t passing it through. This commit passes it through. --- app/celery/tasks.py | 1 + tests/app/celery/test_tasks.py | 1 + tests/app/conftest.py | 3 ++- 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index f746512be..cdf5dc7d2 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -274,6 +274,7 @@ def build_dvla_file(self, job_id): # 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, )) for notification in dao_get_all_notifications_for_job(job_id) ) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 80b83756c..671d52189 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -994,6 +994,7 @@ def test_build_dvla_file(sample_letter_template, mocker): # Named arguments assert mocked_letter_template.call_args[1]['numeric_id'] == 999 + assert mocked_letter_template.call_args[1]['contact_block'] == 'London,\nSW1A 1AA' def test_build_dvla_file_retries_if_all_notifications_are_not_created(sample_letter_template, mocker): diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 387bb33eb..0805242b7 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -134,7 +134,8 @@ def sample_service( 'message_limit': limit, 'restricted': restricted, 'email_from': email_from, - 'created_by': user + 'created_by': user, + 'letter_contact_block': 'London,\nSW1A 1AA', } service = Service.query.filter_by(name=service_name).first() if not service: