diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index a2bb1b944..194cb0563 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -43,17 +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'] - - -def _set_template_filters(filter_args): - if not filter_args.get('template_type'): - filter_args['template_type'] = ['email', 'sms'] + filter_args['status'] = all_statuses @main.route("/services//jobs") @@ -77,20 +75,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'], @@ -113,6 +97,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) @@ -144,26 +158,31 @@ def view_job_updates(service_id, job_id): }) -@main.route('/services//notifications') +@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): +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( @@ -182,13 +201,13 @@ def view_notifications(service_id): 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, 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 +221,24 @@ 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, + 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', 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'], + ['Processed', 'sending,delivered,failed'], + ['Sending', 'sending'], + ['Delivered', 'delivered'], ['Failed', 'failed'], - ['Both', 'delivered,failed'] ] ] ) diff --git a/app/templates/views/dashboard/today.html b/app/templates/views/dashboard/today.html index 1b34d6e76..d13cfe2bc 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='sending,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='sending,delivered,failed') ) }}
diff --git a/app/templates/views/notifications.html b/app/templates/views/notifications.html index e811692f2..ec544443b 100644 --- a/app/templates/views/notifications.html +++ b/app/templates/views/notifications.html @@ -3,65 +3,39 @@ {% 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 %}

- 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_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..473367424 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,143 +79,42 @@ 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): - 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', - status='delivered,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.string.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): - 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', - template_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.string.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): - 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')) - 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() == 'Failed emails and 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=['email', 'sms']) # noqa - - -def test_can_view_failed_combination_of_notification_type_and_status( +@pytest.mark.parametrize( + "message_type,page_title", [ + ('email', 'Emails'), + ('sms', 'Text messages') + ] +) +@pytest.mark.parametrize( + "status_argument, expected_api_call", [ + ( + 'processed', + ['sending', 'delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] + ), + ( + 'sending', + ['sending'] + ), + ( + 'delivered', + ['delivered'] + ), + ( + 'failed', + ['failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] + ) + ] +) +def test_can_show_notifications( 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: @@ -222,13 +122,46 @@ def test_can_view_failed_combination_of_notification_type_and_status( response = client.get(url_for( 'main.view_notifications', service_id=service_one['id'], - status='failed', - template_type='sms')) + message_type=message_type, + status=status_argument)) assert response.status_code == 200 - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert page.h1.string.strip() == 'Failed text messages' + content = response.get_data(as_text=True) - 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 + 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_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, + page=1, + service_id=service_one['id'], + status=expected_api_call, + template_type=[message_type] + ) + + csv_response = client.get(url_for( + 'main.view_notifications_csv', + 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_, @@ -236,36 +169,21 @@ def test_should_show_notifications_for_a_service_with_next_previous(app_, active_user_with_permissions, mock_get_notifications_with_previous_next, 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'], 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 'Previous page' in content - 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'], - download='csv')) - csv_content = generate_notifications_csv( - mock_get_notifications(service_one['id'])['notifications']) + message_type='sms', + page=2 + )) assert response.status_code == 200 - assert response.get_data(as_text=True) == csv_content - assert 'text/csv' in response.headers['Content-Type'] + content = response.get_data(as_text=True) + 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 @freeze_time("2016-01-01 11:09:00.061258") @@ -282,12 +200,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']