diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 256e02985..d7bd33f9c 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -150,7 +150,8 @@ def view_job_csv(service_id, job_id): job_id=job_id, status=filter_args.get('status'), page=request.args.get('page', 1), - page_size=5000 + page_size=5000, + format_for_csv=True ) ), mimetype='text/csv', diff --git a/app/notify_client/notification_api_client.py b/app/notify_client/notification_api_client.py index 0c3d018df..7ea6831e9 100644 --- a/app/notify_client/notification_api_client.py +++ b/app/notify_client/notification_api_client.py @@ -20,7 +20,8 @@ class NotificationApiClient(NotifyAdminAPIClient): page_size=None, limit_days=None, include_jobs=None, - include_from_test_key=None + include_from_test_key=None, + format_for_csv=None ): params = {} if page is not None: @@ -35,6 +36,8 @@ class NotificationApiClient(NotifyAdminAPIClient): params['include_jobs'] = include_jobs if include_from_test_key is not None: params['include_from_test_key'] = include_from_test_key + if format_for_csv is not None: + params['format_for_csv'] = format_for_csv if job_id: return self.get( url='/service/{}/job/{}/notifications'.format(service_id, job_id), diff --git a/app/utils.py b/app/utils.py index d6e8703f3..788847aa0 100644 --- a/app/utils.py +++ b/app/utils.py @@ -126,73 +126,33 @@ def get_errors_for_csv(recipients, template_type): def generate_notifications_csv(**kwargs): from app import notification_api_client - csvfile = StringIO() - csvwriter = csv.DictWriter( - csvfile, - fieldnames=['Row number', 'Recipient', 'Template', 'Type', 'Job', 'Status', 'Time'] - ) - - def read_current_row(): - csvfile.seek(0) - line = csvfile.read() - return line - - def clear_file_buffer(): - csvfile.seek(0) - csvfile.truncate() - - # Initiate download quicker by returning the headers first - csvwriter.writeheader() - line = read_current_row() - clear_file_buffer() - yield line - - # get_notifications_for_service is always paginated by the api, so set a page param if not specified if 'page' not in kwargs: kwargs['page'] = 1 + fieldnames = ['Row number', 'Recipient', 'Template', 'Type', 'Job', 'Status', 'Time'] + yield ','.join(fieldnames) + '\n' while kwargs['page']: - current_app.logger.info('Current kwargs: {}'.format(kwargs)) notifications_resp = notification_api_client.get_notifications_for_service(**kwargs) - current_app.logger.info('Total notifications for csv job: {}'.format(notifications_resp['total'])) - notifications_list = notifications_resp['notifications'] - for x in notifications_list: - csvwriter.writerow(format_notification_for_csv(x)) - - line = read_current_row() - clear_file_buffer() + notifications = notifications_resp['notifications'] + 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 if notifications_resp['links'].get('next'): - current_app.logger.info('Next link exists') - current_app.logger.info('Notification response links: {}'.format(notifications_resp['links'])) - current_app.logger.info('Current page {}'.format(kwargs['page'])) kwargs['page'] += 1 else: - current_app.logger.info('No next link exists') - current_app.logger.info('Notification response links: {}'.format(notifications_resp['links'])) - current_app.logger.info('Current page {}'.format(kwargs['page'])) return -def format_notification_for_csv(notification): - from app import format_datetime_24h, format_notification_status - # row_number can be 0 so we need to check for it - row_num = notification.get('job_row_number') - row_num = '' if row_num is None else row_num + 1 - current_app.logger.info('Row number for job: {}'.format(row_num)) - - return { - 'Row number': row_num, - 'Recipient': notification['to'], - 'Template': notification['template']['name'], - 'Type': notification['template']['template_type'], - 'Job': notification['job']['original_file_name'] if notification['job'] else '', - 'Status': format_notification_status(notification['status'], notification['template']['template_type']), - 'Time': format_datetime_24h(notification['created_at']) - } - - def get_page_from_request(): if 'page' in request.args: try: diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 1f7d58da0..440e7d519 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -14,25 +14,6 @@ from tests import notification_json from freezegun import freeze_time -def _csv_notifications(notifications_json): - csvfile = StringIO() - csvwriter = csv.writer(csvfile) - from app import format_datetime_24h, format_notification_status - csvwriter.writerow(['Row number', 'Recipient', 'Template', 'Type', 'Job', 'Status', 'Time']) - - for x in notifications_json: - csvwriter.writerow([ - int(x['job_row_number']) + 2 if 'job_row_number' in x and x['job_row_number'] else '', - x['to'], - x['template']['name'], - x['template']['template_type'], - x['job']['original_file_name'] if x['job'] else '', - format_notification_status(x['status'], x['template']['template_type']), - format_datetime_24h(x['created_at']) - ]) - return csvfile.getvalue() - - def test_get_jobs_should_return_list_of_all_real_jobs( logged_in_client, service_one, diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index 224e2a244..1e7d36108 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -10,13 +10,61 @@ from app.utils import ( generate_notifications_csv, generate_previous_dict, generate_next_dict, - Spreadsheet, - format_notification_for_csv + Spreadsheet ) from tests import notification_json, single_notification_json, template_json +def _get_notifications_csv( + service_id, + page=1, + row_number=1, + recipient='foo@bar.com', + template_name='foo', + template_type='sms', + job_name='bar.csv', + status='Delivered', + created_at='Thursday 19 April at 12:00', + rows=1, + with_links=False +): + links = {} + if with_links: + links = { + 'prev': '/service/{}/notifications?page=0'.format(service_id), + 'next': '/service/{}/notifications?page=1'.format(service_id), + 'last': '/service/{}/notifications?page=2'.format(service_id) + } + + data = { + 'notifications': [{ + "row_number": row_number, + "recipient": recipient, + "template_name": template_name, + "template_type": template_type, + "job_name": job_name, + "status": status, + "created_at": created_at + } for i in range(rows)], + 'total': rows, + 'page_size': 50, + 'links': links + } + return data + + +@pytest.fixture(scope='function') +def _get_notifications_csv_mock( + mocker, + api_user_active +): + return mocker.patch( + 'app.notification_api_client.get_notifications_for_service', + side_effect=_get_notifications_csv + ) + + @pytest.mark.parametrize('service_name, safe_email', [ ('name with spaces', 'name.with.spaces'), ('singleword', 'singleword'), @@ -58,56 +106,23 @@ def test_can_create_spreadsheet_from_large_excel_file(): assert ret.as_csv_data -@pytest.mark.parametrize( - "status, template_type, expected_status", - [ - ('temporary-failure', 'email', 'Inbox not accepting messages right now'), - ('permanent-failure', 'email', 'Email address doesn’t exist'), - ('temporary-failure', 'sms', 'Phone not accepting messages right now'), - ('permanent-failure', 'sms', 'Phone number doesn’t exist') - ] -) -def test_format_notification_for_csv_formats_status( - status, - template_type, - expected_status -): - json_row = single_notification_json( - '1234', - template=template_json(service_id='1234', id_='5678', type_=template_type), - status=status - ) - csv_line = format_notification_for_csv(json_row) - - assert csv_line['Status'] == expected_status - - -@freeze_time("2016-01-01 15:09:00.061258") -def test_format_notification_for_csv_formats_time(): - json_row = single_notification_json('1234') - - csv_line = format_notification_for_csv(json_row) - - assert csv_line['Time'] == 'Friday 01 January 2016 at 15:09' - - -def test_generate_notifications_csv_returns_correct_csv_file(mock_get_notifications): +def test_generate_notifications_csv_returns_correct_csv_file(_get_notifications_csv_mock): csv_content = generate_notifications_csv(service_id='1234') - csv_file = DictReader(StringIO('\n'.join(csv_content))) assert csv_file.fieldnames == ['Row number', 'Recipient', 'Template', 'Type', 'Job', 'Status', 'Time'] -def test_generate_notifications_csv_only_calls_once_if_no_next_link(mock_get_notifications): +def test_generate_notifications_csv_only_calls_once_if_no_next_link(_get_notifications_csv_mock): list(generate_notifications_csv(service_id='1234')) - assert mock_get_notifications.call_count == 1 + assert _get_notifications_csv_mock.call_count == 1 def test_generate_notifications_csv_calls_twice_if_next_link(mocker): service_id = '1234' - response_with_links = notification_json(service_id, rows=5, with_links=True) - response_with_no_links = notification_json(service_id, rows=2, with_links=False) + 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) + mock_get_notifications = mocker.patch( 'app.notification_api_client.get_notifications_for_service', side_effect=[ @@ -119,36 +134,12 @@ def test_generate_notifications_csv_calls_twice_if_next_link(mocker): csv_content = generate_notifications_csv(service_id=service_id) csv = DictReader(StringIO('\n'.join(csv_content))) - assert len(list(csv)) == 7 + assert len(list(csv)) == 10 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 assert mock_get_notifications.mock_calls[1][2]['page'] == 2 -@pytest.mark.parametrize('row_number, expected_result', [ - (None, ''), - (0, '1'), - (1, '2'), - (300, '301') -]) -def test_generate_notifications_csv_formats_row_number_correctly(mocker, row_number, expected_result): - service_id = '1234' - response_with_job_row_zero = notification_json(service_id, rows=1, with_links=True, job_row_number=row_number) - mocker.patch( - 'app.notification_api_client.get_notifications_for_service', - side_effect=[ - response_with_job_row_zero - ] - ) - - csv_content = generate_notifications_csv(service_id=service_id) - csv = DictReader(StringIO('\n'.join(csv_content))) - csv_rows = list(csv) - - assert len(csv_rows) == 1 - assert csv_rows[0].get('Row number') == expected_result - - def normalize_spaces(string): return ' '.join(string.split())