From 8d7850ead1c367d635213c45269998b83868559c Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 7 Jun 2016 11:41:20 +0100 Subject: [PATCH] Use .csv extension not querystring parameter The link to download a CSV of notifications looks like `/endpoint?download=csv`. This not not very web idiomatic. The service manual recommends: > Only use query strings for URLs with unordered parameters like options > to search pages. The CSV is a different representation of the same data, it does not perform searching or filtering on the data. The proper way (as we do elsewhere in this app) is to put an extension on the endpoint to indicate an alternate representation, eg `/endpoint.csv` --- app/main/views/jobs.py | 53 ++++++++++++++++++-------- app/templates/views/notifications.html | 2 +- tests/app/main/views/test_jobs.py | 15 ++++++-- 3 files changed, 50 insertions(+), 20 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index de0e90d8f..e87450863 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -72,20 +72,6 @@ def view_job(service_id, job_id): version=job['template_version'])['data'] notifications = notification_api_client.get_notifications_for_service(service_id, job_id) finished = job['status'] == 'finished' - if 'download' in request.args and request.args['download'] == 'csv': - csv_content = generate_notifications_csv( - notification_api_client.get_notifications_for_service( - service_id, - job_id, - page_size=job['notification_count'] - )['notifications']) - return csv_content, 200, { - 'Content-Type': 'text/csv; charset=utf-8', - 'Content-Disposition': 'inline; filename="{} - {}.csv"'.format( - template['name'], - format_datetime_short(job['created_at']) - ) - } return render_template( 'views/jobs/job.html', notifications=notifications['notifications'], @@ -108,6 +94,36 @@ def view_job(service_id, job_id): ) +@main.route("/services//jobs/.csv") +@login_required +@user_has_permissions('view_activity', admin_override=True) +def view_job_csv(service_id, job_id): + job = job_api_client.get_job(service_id, job_id)['data'] + template = service_api_client.get_service_template( + service_id=service_id, + template_id=job['template'], + version=job['template_version'] + )['data'] + + return ( + generate_notifications_csv( + notification_api_client.get_notifications_for_service( + service_id, + job_id, + page_size=job['notification_count'] + )['notifications'] + ), + 200, + { + 'Content-Type': 'text/csv; charset=utf-8', + 'Content-Disposition': 'inline; filename="{} - {}.csv"'.format( + template['name'], + format_datetime_short(job['created_at']) + ) + } + ) + + @main.route("/services//jobs/.json") @login_required @user_has_permissions('view_activity', admin_override=True) @@ -140,6 +156,7 @@ def view_job_updates(service_id, job_id): @main.route('/services//notifications/') +@main.route('/services//notifications/.csv', endpoint="view_notifications_csv") @login_required @user_has_permissions('view_activity', admin_override=True) def view_notifications(service_id, message_type): @@ -181,7 +198,7 @@ def view_notifications(service_id, message_type): page + 1, 'Next page', 'page {}'.format(page + 1)) - if 'download' in request.args and request.args['download'] == 'csv': + if request.path.endswith('csv'): csv_content = generate_notifications_csv( notification_api_client.get_notifications_for_service( service_id=service_id, @@ -202,6 +219,12 @@ def view_notifications(service_id, message_type): next_page=next_page, request_args=request.args, message_type=message_type, + download_link=url_for( + '.view_notifications_csv', + service_id=current_service['id'], + message_type=message_type, + status=request.args.get('status') + ), status_filters=[ [item[0], item[1], url_for( '.view_notifications', diff --git a/app/templates/views/notifications.html b/app/templates/views/notifications.html index ef465dcc8..ec544443b 100644 --- a/app/templates/views/notifications.html +++ b/app/templates/views/notifications.html @@ -35,7 +35,7 @@ {% if notifications %}

- Download as a CSV file + Download as a CSV file   Delivery information is available for 7 days

diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index cdd1aa3e3..70123fb2d 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -131,6 +131,12 @@ def test_can_show_notifications( assert 'csv' in content page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert page_title in page.h1.text.strip() + assert url_for( + '.view_notifications_csv', + service_id=service_one['id'], + message_type=message_type, + status=status_argument + ) == page.findAll("a", {"download": "download"})[0]['href'] mock_get_notifications.assert_called_with( limit_days=7, @@ -141,7 +147,7 @@ def test_can_show_notifications( ) csv_response = client.get(url_for( - 'main.view_notifications', + 'main.view_notifications_csv', service_id=service_one['id'], message_type='email', download='csv' @@ -190,12 +196,13 @@ def test_should_download_notifications_for_a_job(app_, with app_.test_client() as client: client.login(api_user_active) response = client.get(url_for( - 'main.view_job', + 'main.view_job_csv', service_id=fake_uuid, job_id=fake_uuid, - download='csv')) + )) csv_content = generate_notifications_csv( - mock_get_notifications(fake_uuid, job_id=fake_uuid)['notifications']) + mock_get_notifications(fake_uuid, job_id=fake_uuid)['notifications'] + ) assert response.status_code == 200 assert response.get_data(as_text=True) == csv_content assert 'text/csv' in response.headers['Content-Type']