Merge pull request #1236 from alphagov/fix-csv-download-timeout-issue

Fix CSV download timeout issue

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',

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),

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 = notifications_resp['notifications']
-        notifications_list = notifications_resp['notifications']
+        for notification in notifications:
-        for x in notifications_list:
+            values = [
-            csvwriter.writerow(format_notification_for_csv(x))
+                notification['row_number'],
-
+                notification['recipient'],
-            line = read_current_row()
+                notification['template_name'],
-            clear_file_buffer()
+                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:

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,

tests/app/test_utils.py

@@ -10,13 +10,61 @@ from app.utils import (
     generate_notifications_csv,
     generate_previous_dict,
     generate_next_dict,
-    Spreadsheet,
+    Spreadsheet
-    format_notification_for_csv
 )
 
 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(
+def test_generate_notifications_csv_returns_correct_csv_file(_get_notifications_csv_mock):
-    "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):
     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_links = _get_notifications_csv(service_id, rows=7, with_links=True)
-    response_with_no_links = notification_json(service_id, rows=2, with_links=False)
+    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())