From 09348438154a289e336b48b64876645fce1443dd Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 16 Feb 2018 11:35:36 +0000 Subject: [PATCH] Add original file data to job downloads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When downloading a report of a which messages from a job have been delivered and which have failed we currently only include the Notify data. This makes it hard to reconcile or do analysis on these reports, because often the thing that people want to reconcile on is in the data theyโ€™ve uploaded (eg a reference number). Hereโ€™s an example of a user talking about this problem: > It would also be helpful if the format of the delivery and failure > reports could include the fields from the recipient's file. While I > can, of course, cross-reference one report with the other it would be > easier if I did not have to. We send emails to individuals within > organisations and it is not always easy to establish the organisation > from a recipient's email address. This is particularly important when > emails fail to be delivered as we need to contact the organisation to > establish a new contact. โ€“ ticket 677 Weโ€™ve also seen it when doing research with a local council. This commit takes the original file, the data from the API, and munges them together. --- app/main/views/jobs.py | 3 +- app/utils.py | 41 ++++++++++++-------- requirements.txt | 2 +- tests/app/test_utils.py | 83 +++++++++++++++++++++++++++++++++++------ 4 files changed, 101 insertions(+), 28 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index bd4f1320b..017cd4f0b 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -126,7 +126,8 @@ def view_job_csv(service_id, job_id): status=filter_args.get('status'), page=request.args.get('page', 1), page_size=5000, - format_for_csv=True + format_for_csv=True, + template_type=template['template_type'], ) ), mimetype='text/csv', diff --git a/app/utils.py b/app/utils.py index 3db2785d9..761b2f608 100644 --- a/app/utils.py +++ b/app/utils.py @@ -17,6 +17,7 @@ import pytz import yaml from flask import abort, current_app, redirect, request, session, url_for from flask_login import current_user +from notifications_utils.recipients import RecipientCSV from notifications_utils.template import ( EmailPreviewTemplate, LetterImageTemplate, @@ -121,33 +122,43 @@ def get_errors_for_csv(recipients, template_type): def generate_notifications_csv(**kwargs): from app import notification_api_client + from app.main.s3_client import s3download if 'page' not in kwargs: kwargs['page'] = 1 - if kwargs['job_id']: - fieldnames = ['Row number', 'Recipient', 'Template', 'Type', 'Job', 'Status', 'Time'] + if kwargs.get('job_id'): + original_file_contents = s3download(kwargs['service_id'], kwargs['job_id']) + original_upload = RecipientCSV( + original_file_contents, + template_type=kwargs['template_type'], + ) + original_column_headers = original_upload.column_headers + fieldnames = ['Row number'] + original_column_headers + ['Template', 'Type', 'Job', 'Status', 'Time'] else: fieldnames = ['Recipient', 'Template', 'Type', 'Job', 'Status', 'Time'] yield ','.join(fieldnames) + '\n' while kwargs['page']: - # if job_id then response looks different notifications_resp = notification_api_client.get_notifications_for_service(**kwargs) notifications = notifications_resp['notifications'] - if kwargs['job_id']: + + if kwargs.get('job_id'): for notification in notifications: - values = [ - notification['row_number'], - notification['recipient'], - notification['template_name'], - notification['template_type'], - notification['job_name'], - notification['status'], - notification['created_at'] - ] - line = ','.join(str(i) for i in values) + '\n' - yield line + original_row = original_upload[notification['row_number'] - 1] + values = [ + notification['row_number'], + ] + [ + original_row.get(header) + for header in original_column_headers + ] + [ + notification['template_name'], + notification['template_type'], + notification['job_name'], + notification['status'], + notification['created_at'] + ] + yield ','.join(map(str, values)) + '\n' else: # Change here for notification in notifications: diff --git a/requirements.txt b/requirements.txt index 5fb0ddf16..31dba0fc9 100644 --- a/requirements.txt +++ b/requirements.txt @@ -18,4 +18,4 @@ notifications-python-client==4.7.2 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@23.6.2#egg=notifications-utils==23.6.2 +git+https://github.com/alphagov/notifications-utils.git@23.8.0#egg=notifications-utils==23.8.0 diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index 2427ab68f..8c0959554 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -43,7 +43,7 @@ def _get_notifications_csv( data = { 'notifications': [{ - "row_number": row_number, + "row_number": row_number + i, "to": recipient, "recipient": recipient, "template_name": template_name, @@ -128,23 +128,78 @@ def test_can_create_spreadsheet_from_dict_with_filename(): assert Spreadsheet.from_dict({}, filename='empty.csv').as_dict['file_name'] == "empty.csv" -def test_generate_notifications_csv_returns_correct_csv_file(_get_notifications_csv_mock): - csv_content = generate_notifications_csv(service_id='1234', job_id=fake_uuid) +@pytest.mark.parametrize('original_file_contents, expected_column_headers, expected_1st_row', [ + ( + """ + phone_number + 07700900123 + """, + ['Row number', 'phone_number', 'Template', 'Type', 'Job', 'Status', 'Time'], + ['1', '07700900123', 'foo', 'sms', 'bar.csv', 'Delivered', 'Thursday 19 April at 12:00'], + ), + ( + """ + phone_number, a, b, c + 07700900123, ๐Ÿœ,๐Ÿ,๐Ÿฆ€ + """, + ['Row number', 'phone_number', 'a', 'b', 'c', 'Template', 'Type', 'Job', 'Status', 'Time'], + ['1', '07700900123', '๐Ÿœ', '๐Ÿ', '๐Ÿฆ€', 'foo', 'sms', 'bar.csv', 'Delivered', 'Thursday 19 April at 12:00'], + ), +]) +def test_generate_notifications_csv_returns_correct_csv_file( + app_, + mocker, + _get_notifications_csv_mock, + original_file_contents, + expected_column_headers, + expected_1st_row, +): + mocker.patch( + 'app.main.s3_client.s3download', + return_value=original_file_contents, + ) + csv_content = generate_notifications_csv(service_id='1234', job_id=fake_uuid, template_type='sms') csv_file = DictReader(StringIO('\n'.join(csv_content))) - assert csv_file.fieldnames == ['Row number', 'Recipient', 'Template', 'Type', 'Job', 'Status', 'Time'] + assert csv_file.fieldnames == expected_column_headers + assert next(csv_file) == dict(zip(expected_column_headers, expected_1st_row)) -def test_generate_notifications_csv_only_calls_once_if_no_next_link(_get_notifications_csv_mock): - list(generate_notifications_csv(service_id='1234', job_id=fake_uuid)) +def test_generate_notifications_csv_only_calls_once_if_no_next_link( + app_, + _get_notifications_csv_mock, +): + list(generate_notifications_csv(service_id='1234')) assert _get_notifications_csv_mock.call_count == 1 @pytest.mark.parametrize("job_id", ["some", None]) -def test_generate_notifications_csv_calls_twice_if_next_link(mocker, job_id): +def test_generate_notifications_csv_calls_twice_if_next_link( + app_, + mocker, + job_id, +): + + mocker.patch( + 'app.main.s3_client.s3download', + return_value=""" + phone_number + 07700900000 + 07700900001 + 07700900002 + 07700900003 + 07700900004 + 07700900005 + 07700900006 + 07700900007 + 07700900008 + 07700900009 + """ + ) + service_id = '1234' response_with_links = _get_notifications_csv(service_id, rows=7, with_links=True) - response_with_no_links = _get_notifications_csv(service_id, rows=3, with_links=False) + response_with_no_links = _get_notifications_csv(service_id, rows=3, row_number=8, with_links=False) mock_get_notifications = mocker.patch( 'app.notification_api_client.get_notifications_for_service', @@ -154,10 +209,16 @@ def test_generate_notifications_csv_calls_twice_if_next_link(mocker, job_id): ] ) - csv_content = generate_notifications_csv(service_id=service_id, job_id=fake_uuid if job_id else None) - csv = DictReader(StringIO('\n'.join(csv_content))) + csv_content = generate_notifications_csv( + service_id=service_id, + job_id=job_id or fake_uuid, + template_type='sms', + ) + csv = list(DictReader(StringIO('\n'.join(csv_content)))) - assert len(list(csv)) == 10 + assert len(csv) == 10 + assert csv[0]['phone_number'] == '07700900000' + assert csv[9]['phone_number'] == '07700900009' assert mock_get_notifications.call_count == 2 # mock_calls[0][2] is the kwargs from first call assert mock_get_notifications.mock_calls[0][2]['page'] == 1