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`
This commit is contained in:
Chris Hill-Scott
2016-06-07 11:41:20 +01:00
parent e1b2999371
commit 8d7850ead1
3 changed files with 50 additions and 20 deletions

View File

@@ -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/<service_id>/jobs/<job_id>.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/<service_id>/jobs/<job_id>.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/<service_id>/notifications/<message_type>')
@main.route('/services/<service_id>/notifications/<message_type>.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',

View File

@@ -35,7 +35,7 @@
{% if notifications %}
<p class="bottom-gutter">
<a href="{{ request.url }}&amp;download=csv" download class="heading-small">Download as a CSV file</a>
<a href="{{ download_link }}" download="download" class="heading-small">Download as a CSV file</a>
&emsp;
Delivery information is available for 7 days
</p>

View File

@@ -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']