From 06903d54be3bdd6c62ee0a73e95295eee0226f97 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 7 Jun 2016 11:17:33 +0100 Subject: [PATCH 1/5] Remove template type filter from activity This commit splits the activity page into two pages, one for emails and one for SMS. Technically this means moving from having template type in the querystring and putting in it the URL, eg: *Before*: `/services/abc/notifications/?template_type=sms` *After*: `/services/abc/notifications/sms` This commit changes the activity page to only have controls --- app/main/views/jobs.py | 38 +++----- app/templates/views/dashboard/today.html | 8 +- app/templates/views/notifications.html | 48 +++------- tests/app/main/views/test_dashboard.py | 6 +- tests/app/main/views/test_jobs.py | 116 ++++++++--------------- 5 files changed, 70 insertions(+), 146 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index a2bb1b944..de0e90d8f 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -51,11 +51,6 @@ def _set_status_filters(filter_args): filter_args['status'] = ['delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] -def _set_template_filters(filter_args): - if not filter_args.get('template_type'): - filter_args['template_type'] = ['email', 'sms'] - - @main.route("/services//jobs") @login_required @user_has_permissions('view_activity', admin_override=True) @@ -144,26 +139,30 @@ def view_job_updates(service_id, job_id): }) -@main.route('/services//notifications') +@main.route('/services//notifications/') @login_required @user_has_permissions('view_activity', admin_override=True) -def view_notifications(service_id): +def view_notifications(service_id, message_type): # TODO get the api to return count of pages as well. page = get_page_from_request() if page is None: abort(404, "Invalid page argument ({}) reverting to page 1.".format(request.args['page'], None)) + if message_type not in ['email', 'sms']: + abort(404) filter_args = _parse_filter_args(request.args) _set_status_filters(filter_args) - _set_template_filters(filter_args) notifications = notification_api_client.get_notifications_for_service( service_id=service_id, page=page, - template_type=filter_args.get('template_type'), + template_type=[message_type], status=filter_args.get('status'), limit_days=current_app.config['ACTIVITY_STATS_LIMIT_DAYS']) - view_dict = MultiDict(request.args) + view_dict = dict( + message_type=message_type, + status=request.args.get('status') + ) prev_page = None if notifications['links'].get('prev', None): prev_page = generate_previous_next_dict( @@ -188,7 +187,7 @@ def view_notifications(service_id): service_id=service_id, page=page, page_size=notifications['total'], - template_type=filter_args.get('template_type') if 'template_type' in filter_args else ['email', 'sms'], + template_type=[message_type], status=filter_args.get('status'), limit_days=current_app.config['ACTIVITY_STATS_LIMIT_DAYS'])['notifications']) return csv_content, 200, { @@ -202,28 +201,17 @@ def view_notifications(service_id): prev_page=prev_page, next_page=next_page, request_args=request.args, - type_filters=[ - [item[0], item[1], url_for( - '.view_notifications', - service_id=current_service['id'], - template_type=item[1], - status=request.args.get('status', 'delivered,failed') - )] for item in [ - ['Emails', 'email'], - ['Text messages', 'sms'], - ['Both', 'email,sms'] - ] - ], + message_type=message_type, status_filters=[ [item[0], item[1], url_for( '.view_notifications', service_id=current_service['id'], - template_type=request.args.get('template_type', 'email,sms'), + message_type=message_type, status=item[1] )] for item in [ ['Successful', 'delivered'], ['Failed', 'failed'], - ['Both', 'delivered,failed'] + ['', 'delivered,failed'] ] ] ) diff --git a/app/templates/views/dashboard/today.html b/app/templates/views/dashboard/today.html index 1b34d6e76..cb50dd08f 100644 --- a/app/templates/views/dashboard/today.html +++ b/app/templates/views/dashboard/today.html @@ -18,8 +18,8 @@ statistics.emails_failed, statistics.get('emails_failure_rate', 0.0), statistics.get('emails_failure_rate', 0)|float > 3, - failure_link=url_for(".view_notifications", service_id=current_service.id, template_type='email', status='failed'), - label_link=url_for(".view_notifications", service_id=current_service.id, template_type='email', status='delivered,failed') + failure_link=url_for(".view_notifications", service_id=current_service.id, message_type='email', status='failed'), + label_link=url_for(".view_notifications", service_id=current_service.id, message_type='email', status='delivered,failed') ) }}
@@ -29,8 +29,8 @@ statistics.sms_failed, statistics.get('sms_failure_rate', 0.0), statistics.get('sms_failure_rate', 0)|float > 3, - failure_link=url_for(".view_notifications", service_id=current_service.id, template_type='sms', status='failed'), - label_link=url_for(".view_notifications", service_id=current_service.id, template_type='sms', status='delivered,failed') + failure_link=url_for(".view_notifications", service_id=current_service.id, message_type='sms', status='failed'), + label_link=url_for(".view_notifications", service_id=current_service.id, message_type='sms', status='delivered,failed') ) }}
diff --git a/app/templates/views/notifications.html b/app/templates/views/notifications.html index e811692f2..ef465dcc8 100644 --- a/app/templates/views/notifications.html +++ b/app/templates/views/notifications.html @@ -3,60 +3,34 @@ {% from "components/previous-next-navigation.html" import previous_next_navigation %} {% from "components/page-footer.html" import page_footer %} {% from "components/pill.html" import pill %} +{% from "components/message-count-label.html" import message_count_label %} {% block page_title %} - Activity – GOV.UK Notify + {{ message_count_label(99, message_type, suffix='') | capitalize }} – GOV.UK Notify {% endblock %} {% block maincolumn_content %}

- {%- if (request_args.get('template_type', 'email,sms') == 'email,sms') and (request_args.get('status', 'delivered,failed') == 'delivered,failed') -%} - - Activity - - {%- else -%} + - {%- if request_args.get('template_type', 'email,sms') == 'email,sms' %} emails and text messages - {%- else -%} - - {%- for template_label, template_option, _ in type_filters -%} - {%- if request_args.get('template_type') == template_option -%} - {%- if request_args.get('status', 'delivered,failed') == 'delivered,failed' -%} - {{ template_label }} - {%- else -%} - {{ template_label | lower }} - {%- endif -%} - {%- endif -%} - {%- endfor -%} - - {%- endif -%} - - {%- endif -%} + {{- message_count_label(99, message_type, suffix='') | capitalize }}

-
-
- {{ pill( - 'Status', - status_filters, - request_args.get('status', '') - ) }} -
-
- {{ pill( - 'Type', - type_filters, - request_args.get('template_type', '') - ) }} -
+
+ {{ pill( + 'Status', + status_filters, + request_args.get('status', '') + ) }}
{% if notifications %} diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index 114ae5437..d143166d2 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -175,7 +175,6 @@ def test_menu_send_messages(mocker, 'main.choose_template', service_id=service_one['id'], template_type='sms')in page - assert url_for('main.view_notifications', service_id=service_one['id']) in page assert url_for('main.manage_users', service_id=service_one['id']) in page assert url_for('main.documentation') in page @@ -209,7 +208,6 @@ def test_menu_manage_service(mocker, 'main.choose_template', service_id=service_one['id'], template_type='sms') in page - assert url_for('main.view_notifications', service_id=service_one['id']) in page assert url_for('main.manage_users', service_id=service_one['id']) in page assert url_for('main.service_settings', service_id=service_one['id']) in page assert url_for('main.documentation') in page @@ -242,7 +240,6 @@ def test_menu_manage_api_keys(mocker, 'main.choose_template', service_id=service_one['id'], template_type='sms') in page - assert url_for('main.view_notifications', service_id=service_one['id']) in page assert url_for('main.manage_users', service_id=service_one['id']) in page assert url_for('main.service_settings', service_id=service_one['id']) not in page assert url_for('main.show_all_services') not in page @@ -270,7 +267,8 @@ def test_menu_all_services_for_platform_admin_user(mocker, assert url_for('main.choose_template', service_id=service_one['id'], template_type='email') in page assert url_for('main.manage_users', service_id=service_one['id']) in page assert url_for('main.service_settings', service_id=service_one['id']) in page - assert url_for('main.view_notifications', service_id=service_one['id']) in page + assert url_for('main.view_notifications', service_id=service_one['id'], message_type='email') in page + assert url_for('main.view_notifications', service_id=service_one['id'], message_type='sms') in page assert url_for('main.api_keys', service_id=service_one['id']) not in page # Should this be here?? diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 3e5b1268e..27cbee52f 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -78,41 +78,20 @@ def test_should_show_updates_for_one_job_as_json( assert 'Uploaded by Test User on 1 January at 11:09' in content['status'] -def test_should_show_notifications_for_a_service(app_, - service_one, - active_user_with_permissions, - mock_get_notifications, - mocker): - with app_.test_request_context(): - with app_.test_client() as client: - client.login(active_user_with_permissions, mocker, service_one) - response = client.get(url_for('main.view_notifications', service_id=service_one['id'])) - assert response.status_code == 200 - content = response.get_data(as_text=True) - notifications = notification_json(service_one['id']) - notification = notifications['notifications'][0] - assert notification['to'] in content - assert notification['status'] in content - assert notification['template']['name'] in content - assert 'csv' in content - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert page.h1.string.strip() == 'Activity' - - mock_get_notifications.assert_called_with(limit_days=7, page=1, service_id=service_one['id'], status=['delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure'], template_type=['email', 'sms']) # noqa - - -def test_can_view_only_sms_notifications_for_a_service(app_, - service_one, - active_user_with_permissions, - mock_get_notifications, - mocker): +def test_can_see_sms( + app_, + service_one, + active_user_with_permissions, + mock_get_notifications, + mocker +): with app_.test_request_context(): with app_.test_client() as client: client.login(active_user_with_permissions, mocker, service_one) response = client.get(url_for( 'main.view_notifications', service_id=service_one['id'], - template_type='sms', + message_type='sms', status='delivered,failed')) assert response.status_code == 200 content = response.get_data(as_text=True) @@ -124,16 +103,18 @@ def test_can_view_only_sms_notifications_for_a_service(app_, assert notification['template']['name'] in content assert 'csv' in content page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert page.h1.string.strip() == 'Text messages' + assert page.h1.text.strip() == 'Text messages' mock_get_notifications.assert_called_with(limit_days=7, page=1, service_id=service_one['id'], status=['delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure'], template_type=['sms']) # noqa -def test_can_view_only_email_notifications_for_a_service(app_, - service_one, - active_user_with_permissions, - mock_get_notifications, - mocker): +def test_can_see_emails( + app_, + service_one, + active_user_with_permissions, + mock_get_notifications, + mocker +): with app_.test_request_context(): with app_.test_client() as client: client.login(active_user_with_permissions, mocker, service_one) @@ -141,7 +122,7 @@ def test_can_view_only_email_notifications_for_a_service(app_, 'main.view_notifications', service_id=service_one['id'], status='delivered,failed', - template_type='email')) + message_type='email')) assert response.status_code == 200 content = response.get_data(as_text=True) @@ -152,48 +133,25 @@ def test_can_view_only_email_notifications_for_a_service(app_, assert notification['template']['name'] in content assert 'csv' in content page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert page.h1.string.strip() == 'Emails' + assert page.h1.text.strip() == 'Emails' mock_get_notifications.assert_called_with(limit_days=7, page=1, service_id=service_one['id'], status=['delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure'], template_type=['email']) # noqa -def test_can_view_successful_notifications_for_a_service(app_, - service_one, - active_user_with_permissions, - mock_get_notifications, - mocker): - with app_.test_request_context(): - with app_.test_client() as client: - client.login(active_user_with_permissions, mocker, service_one) - response = client.get(url_for( - 'main.view_notifications', - service_id=service_one['id'], - status='delivered')) - assert response.status_code == 200 - content = response.get_data(as_text=True) - notifications = notification_json(service_one['id']) - notification = notifications['notifications'][0] - assert notification['to'] in content - assert notification['status'] in content - assert notification['template']['name'] in content - assert 'csv' in content - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert page.h1.string.strip() == 'Successful emails and text messages' - - mock_get_notifications.assert_called_with(limit_days=7, page=1, service_id=service_one['id'], status=['delivered'], template_type=['email', 'sms']) # noqa - - -def test_can_view_failed_notifications_for_a_service(app_, - service_one, - active_user_with_permissions, - mock_get_notifications, - mocker): +def test_can_view_failed_emails( + app_, + service_one, + active_user_with_permissions, + mock_get_notifications, + mocker +): with app_.test_request_context(): with app_.test_client() as client: client.login(active_user_with_permissions, mocker, service_one) response = client.get(url_for( 'main.view_notifications', service_id=service_one['id'], + message_type='email', status='failed')) assert response.status_code == 200 content = response.get_data(as_text=True) @@ -204,12 +162,12 @@ def test_can_view_failed_notifications_for_a_service(app_, assert notification['template']['name'] in content assert 'csv' in content page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert page.h1.string.strip() == 'Failed emails and text messages' + assert page.h1.text.strip() == 'Failed Emails' - mock_get_notifications.assert_called_with(limit_days=7, page=1, service_id=service_one['id'], status=['failed', 'temporary-failure', 'permanent-failure', 'technical-failure'], template_type=['email', 'sms']) # noqa + mock_get_notifications.assert_called_with(limit_days=7, page=1, service_id=service_one['id'], status=['failed', 'temporary-failure', 'permanent-failure', 'technical-failure'], template_type=['email']) # noqa -def test_can_view_failed_combination_of_notification_type_and_status( +def test_can_view_failed_sms_messages( app_, service_one, active_user_with_permissions, @@ -223,10 +181,10 @@ def test_can_view_failed_combination_of_notification_type_and_status( 'main.view_notifications', service_id=service_one['id'], status='failed', - template_type='sms')) + message_type='sms')) assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert page.h1.string.strip() == 'Failed text messages' + assert page.h1.text.strip() == 'Failed Text messages' mock_get_notifications.assert_called_with(limit_days=7, page=1, service_id=service_one['id'], status=['failed', 'temporary-failure', 'permanent-failure', 'technical-failure'], template_type=['sms']) # noqa @@ -239,11 +197,16 @@ def test_should_show_notifications_for_a_service_with_next_previous(app_, with app_.test_request_context(): with app_.test_client() as client: client.login(active_user_with_permissions, mocker, service_one) - response = client.get(url_for('main.view_notifications', service_id=service_one['id'], page=2)) + response = client.get(url_for( + 'main.view_notifications', + service_id=service_one['id'], + message_type='sms', + page=2 + )) assert response.status_code == 200 content = response.get_data(as_text=True) - assert url_for('main.view_notifications', service_id=service_one['id'], page=3) in content - assert url_for('main.view_notifications', service_id=service_one['id'], page=1) in content + assert url_for('main.view_notifications', service_id=service_one['id'], message_type='sms', page=3) in content + assert url_for('main.view_notifications', service_id=service_one['id'], message_type='sms', page=1) in content assert 'Previous page' in content assert 'Next page' in content @@ -260,6 +223,7 @@ def test_should_download_notifications_for_a_service(app_, response = client.get(url_for( 'main.view_notifications', service_id=service_one['id'], + message_type='email', download='csv')) csv_content = generate_notifications_csv( mock_get_notifications(service_one['id'])['notifications']) From 9b099d78c73e856816e9a9419fffc0344d1c9a92 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 7 Jun 2016 11:25:22 +0100 Subject: [PATCH 2/5] Make test for activity page more thorough This commit paramaterizes the test for the activity page, so that it checks all combinations of template type and notification status. --- tests/app/main/views/test_jobs.py | 43 +++++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 5 deletions(-) diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 27cbee52f..4e1797efb 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -1,3 +1,4 @@ +import pytest from flask import url_for from bs4 import BeautifulSoup import json @@ -78,12 +79,38 @@ def test_should_show_updates_for_one_job_as_json( assert 'Uploaded by Test User on 1 January at 11:09' in content['status'] +@pytest.mark.parametrize( + "message_type,page_title", [ + ('email', 'Emails'), + ('sms', 'Text messages') + ] +) +@pytest.mark.parametrize( + "status_argument, expected_api_call", [ + ( + 'delivered', + ['delivered'] + ), + ( + 'failed', + ['failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] + ), + ( + 'delivered,failed', + ['delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] + ) + ] +) def test_can_see_sms( app_, service_one, active_user_with_permissions, mock_get_notifications, - mocker + mocker, + message_type, + page_title, + status_argument, + expected_api_call ): with app_.test_request_context(): with app_.test_client() as client: @@ -91,8 +118,8 @@ def test_can_see_sms( response = client.get(url_for( 'main.view_notifications', service_id=service_one['id'], - message_type='sms', - status='delivered,failed')) + message_type=message_type, + status=status_argument)) assert response.status_code == 200 content = response.get_data(as_text=True) @@ -103,9 +130,15 @@ def test_can_see_sms( assert notification['template']['name'] in content assert 'csv' in content page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert page.h1.text.strip() == 'Text messages' + assert page_title in page.h1.text.strip() - mock_get_notifications.assert_called_with(limit_days=7, page=1, service_id=service_one['id'], status=['delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure'], template_type=['sms']) # noqa + mock_get_notifications.assert_called_with( + limit_days=7, + page=1, + service_id=service_one['id'], + status=expected_api_call, + template_type=[message_type] + ) def test_can_see_emails( From e1b29993711b110c55d489a54b8609b8e3819a18 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 7 Jun 2016 11:28:23 +0100 Subject: [PATCH 3/5] Move test for downloads CSV of notifications MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Downloading a CSV of notifications is very similar to viewing them on a webpage. So I think it’s sensible to move the assertions about the CSV download link into the same test, rather than it being it’s own test. This means being able to reuse the parametrization introuced in this commit’s parent. --- tests/app/main/views/test_jobs.py | 115 ++++-------------------------- 1 file changed, 13 insertions(+), 102 deletions(-) diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 4e1797efb..cdd1aa3e3 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -101,7 +101,7 @@ def test_should_show_updates_for_one_job_as_json( ) ] ) -def test_can_see_sms( +def test_can_show_notifications( app_, service_one, active_user_with_permissions, @@ -140,86 +140,18 @@ def test_can_see_sms( template_type=[message_type] ) - -def test_can_see_emails( - app_, - service_one, - active_user_with_permissions, - mock_get_notifications, - mocker -): - with app_.test_request_context(): - with app_.test_client() as client: - client.login(active_user_with_permissions, mocker, service_one) - response = client.get(url_for( - 'main.view_notifications', - service_id=service_one['id'], - status='delivered,failed', - message_type='email')) - assert response.status_code == 200 - content = response.get_data(as_text=True) - - notifications = notification_json(service_one['id']) - notification = notifications['notifications'][0] - assert notification['to'] in content - assert notification['status'] in content - assert notification['template']['name'] in content - assert 'csv' in content - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert page.h1.text.strip() == 'Emails' - - mock_get_notifications.assert_called_with(limit_days=7, page=1, service_id=service_one['id'], status=['delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure'], template_type=['email']) # noqa - - -def test_can_view_failed_emails( - app_, - service_one, - active_user_with_permissions, - mock_get_notifications, - mocker -): - with app_.test_request_context(): - with app_.test_client() as client: - client.login(active_user_with_permissions, mocker, service_one) - response = client.get(url_for( - 'main.view_notifications', - service_id=service_one['id'], - message_type='email', - status='failed')) - assert response.status_code == 200 - content = response.get_data(as_text=True) - notifications = notification_json(service_one['id']) - notification = notifications['notifications'][0] - assert notification['to'] in content - assert notification['status'] in content - assert notification['template']['name'] in content - assert 'csv' in content - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert page.h1.text.strip() == 'Failed Emails' - - mock_get_notifications.assert_called_with(limit_days=7, page=1, service_id=service_one['id'], status=['failed', 'temporary-failure', 'permanent-failure', 'technical-failure'], template_type=['email']) # noqa - - -def test_can_view_failed_sms_messages( - app_, - service_one, - active_user_with_permissions, - mock_get_notifications, - mocker -): - with app_.test_request_context(): - with app_.test_client() as client: - client.login(active_user_with_permissions, mocker, service_one) - response = client.get(url_for( - 'main.view_notifications', - service_id=service_one['id'], - status='failed', - message_type='sms')) - assert response.status_code == 200 - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert page.h1.text.strip() == 'Failed Text messages' - - mock_get_notifications.assert_called_with(limit_days=7, page=1, service_id=service_one['id'], status=['failed', 'temporary-failure', 'permanent-failure', 'technical-failure'], template_type=['sms']) # noqa + csv_response = client.get(url_for( + 'main.view_notifications', + service_id=service_one['id'], + message_type='email', + download='csv' + )) + csv_content = generate_notifications_csv( + mock_get_notifications(service_one['id'])['notifications'] + ) + assert csv_response.status_code == 200 + assert csv_response.get_data(as_text=True) == csv_content + assert 'text/csv' in csv_response.headers['Content-Type'] def test_should_show_notifications_for_a_service_with_next_previous(app_, @@ -244,27 +176,6 @@ def test_should_show_notifications_for_a_service_with_next_previous(app_, assert 'Next page' in content -def test_should_download_notifications_for_a_service(app_, - service_one, - active_user_with_permissions, - mock_get_service_template, - mock_get_notifications, - mocker): - with app_.test_request_context(): - with app_.test_client() as client: - client.login(active_user_with_permissions, mocker, service_one) - response = client.get(url_for( - 'main.view_notifications', - service_id=service_one['id'], - message_type='email', - download='csv')) - csv_content = generate_notifications_csv( - mock_get_notifications(service_one['id'])['notifications']) - assert response.status_code == 200 - assert response.get_data(as_text=True) == csv_content - assert 'text/csv' in response.headers['Content-Type'] - - @freeze_time("2016-01-01 11:09:00.061258") def test_should_download_notifications_for_a_job(app_, api_user_active, From 8d7850ead1c367d635213c45269998b83868559c Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 7 Jun 2016 11:41:20 +0100 Subject: [PATCH 4/5] 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'] From 3d7f07493b303155929dd202ce08a94603ef1f8e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 7 Jun 2016 12:01:43 +0100 Subject: [PATCH 5/5] =?UTF-8?q?Add=20filters=20for=20=E2=80=98processed?= =?UTF-8?q?=E2=80=99=20and=20sending=20states?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - _Processed_ is all the notifications that we know about, ie sending, failed and delivered - _Sending_ is notifications that we have either put into a queue or are waiting to hear back from the provider about. The big numbers on the dashboard are a count of all the messages we’ve processed. So when you click them, the table of notifications you see on the dashboard should contain that number of notifications. This also gets the activity page one step closer to being like the job page: | Before | After ---------|----------------------------|--------------------------------- Activity | Sending, failed, both | Processed, sending, failed, delivered Job page | Sending, failed, delivered | Sending, failed, delivered --- app/main/views/jobs.py | 16 ++++++++++------ app/templates/views/dashboard/today.html | 4 ++-- tests/app/main/views/test_jobs.py | 12 ++++++++---- 3 files changed, 20 insertions(+), 12 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index e87450863..194cb0563 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -43,12 +43,15 @@ def _parse_filter_args(filter_dict): def _set_status_filters(filter_args): + all_failure_statuses = ['failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] + all_statuses = ['sending', 'delivered'] + all_failure_statuses if filter_args.get('status'): - if 'failed' in filter_args.get('status'): - filter_args['status'].extend(['temporary-failure', 'permanent-failure', 'technical-failure']) + if 'processed' in filter_args.get('status'): + filter_args['status'] = all_statuses + elif 'failed' in filter_args.get('status'): + filter_args['status'].extend(all_failure_statuses[1:]) else: - # default to everything - filter_args['status'] = ['delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] + filter_args['status'] = all_statuses @main.route("/services//jobs") @@ -232,9 +235,10 @@ def view_notifications(service_id, message_type): message_type=message_type, status=item[1] )] for item in [ - ['Successful', 'delivered'], + ['Processed', 'sending,delivered,failed'], + ['Sending', 'sending'], + ['Delivered', 'delivered'], ['Failed', 'failed'], - ['', 'delivered,failed'] ] ] ) diff --git a/app/templates/views/dashboard/today.html b/app/templates/views/dashboard/today.html index cb50dd08f..d13cfe2bc 100644 --- a/app/templates/views/dashboard/today.html +++ b/app/templates/views/dashboard/today.html @@ -19,7 +19,7 @@ statistics.get('emails_failure_rate', 0.0), statistics.get('emails_failure_rate', 0)|float > 3, failure_link=url_for(".view_notifications", service_id=current_service.id, message_type='email', status='failed'), - label_link=url_for(".view_notifications", service_id=current_service.id, message_type='email', status='delivered,failed') + label_link=url_for(".view_notifications", service_id=current_service.id, message_type='email', status='sending,delivered,failed') ) }}
@@ -30,7 +30,7 @@ statistics.get('sms_failure_rate', 0.0), statistics.get('sms_failure_rate', 0)|float > 3, failure_link=url_for(".view_notifications", service_id=current_service.id, message_type='sms', status='failed'), - label_link=url_for(".view_notifications", service_id=current_service.id, message_type='sms', status='delivered,failed') + label_link=url_for(".view_notifications", service_id=current_service.id, message_type='sms', status='sending,delivered,failed') ) }}
diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 70123fb2d..473367424 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -87,6 +87,14 @@ def test_should_show_updates_for_one_job_as_json( ) @pytest.mark.parametrize( "status_argument, expected_api_call", [ + ( + 'processed', + ['sending', 'delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] + ), + ( + 'sending', + ['sending'] + ), ( 'delivered', ['delivered'] @@ -94,10 +102,6 @@ def test_should_show_updates_for_one_job_as_json( ( 'failed', ['failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] - ), - ( - 'delivered,failed', - ['delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] ) ] )