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..9568aea23 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,36 +122,40 @@ 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']: - 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 - else: - # Change here - for notification in notifications: + for notification in notifications_resp['notifications']: + if kwargs.get('job_id'): + values = [ + notification['row_number'], + ] + [ + original_upload[notification['row_number'] - 1].get(header) + for header in original_column_headers + ] + [ + notification['template_name'], + notification['template_type'], + notification['job_name'], + notification['status'], + notification['created_at'], + ] + else: values = [ notification['to'], notification['template']['name'], @@ -160,8 +165,8 @@ def generate_notifications_csv(**kwargs): notification['created_at'], notification['updated_at'] ] - line = ','.join(str(i) for i in values) + '\n' - yield line + yield ','.join(map(str, values)) + '\n' + if notifications_resp['links'].get('next'): kwargs['page'] += 1 else: 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