diff --git a/app/assets/stylesheets/components/big-number.scss b/app/assets/stylesheets/components/big-number.scss index f3a97af4a..3e785dcb6 100644 --- a/app/assets/stylesheets/components/big-number.scss +++ b/app/assets/stylesheets/components/big-number.scss @@ -21,7 +21,7 @@ @extend %big-number; .big-number-number { - @include bold-36($tabular-numbers: true); + @include bold-36(); } } @@ -39,31 +39,13 @@ .big-number-with-status { @extend %big-number; - background: $text-colour; - color: $white; position: relative; margin-bottom: $gutter-half; .big-number { - padding: 15px; + padding: $gutter-half; position: relative; - - } - - .big-number-overlay-link { - - background: transparent; - position: absolute; - top: 0; - left: 0; - background: transparent; - width: 100%; - height: 100%; - - &:hover { - background: rgba($text-colour, 0.2); - } - + cursor: pointer; } .big-number-label { @@ -72,9 +54,31 @@ &:link, &:visited { - color: $white; - text-decoration: none; - border-bottom: 1px solid $white; + color: $link-colour; + } + + } + + .big-number-link { + + text-decoration: none; + background: $link-colour; + color: $white; + display: block; + border: 2px solid $link-colour; + margin-bottom: 5px; + + &:hover { + color: $light-blue-25; + } + + &:active, + &:focus { + outline: 3px solid $yellow; + } + + .big-number-label { + text-decoration: underline; } } @@ -85,6 +89,7 @@ @include core-19; display: block; background: $green; + color: $white; padding: 15px; a { @@ -94,8 +99,13 @@ &:active, &:hover { color: $white; - text-decoration: none; - border-bottom: 1px solid $white; + text-decoration: underline; + } + + &:active, + &:focus { + color: $black; + border-bottom: 1px solid $black; } } diff --git a/app/assets/stylesheets/components/pill.scss b/app/assets/stylesheets/components/pill.scss index de839a37c..5d3b57ce5 100644 --- a/app/assets/stylesheets/components/pill.scss +++ b/app/assets/stylesheets/components/pill.scss @@ -5,9 +5,9 @@ a, span { display: block; - padding: 10px; + padding: 10px; flex-grow: 1; - text-align: center; + text-align: left; &:first-child { margin-left: 0; @@ -20,13 +20,25 @@ } a { - background: $panel-colour; - color: $link-colour; - border: 1px solid $panel-colour; + $background: $link-colour; + background: $background; + color: $white; + border: 2px solid $background; position: relative; + text-decoration: none; + cursor: pointer; + + .pill-label { + text-decoration: underline; + } + + &:link, + &:visited { + color: $white; + } &:hover { - color: $text-colour; + color: $light-blue-25; } &:active, @@ -36,7 +48,10 @@ } span { - border: 1px solid $grey-1; + border: 2px solid $black; + outline: 1px solid rgba($white, 0.1); + position: relative; + z-index: 1000; color: $text-colour; } } diff --git a/app/assets/stylesheets/views/dashboard.scss b/app/assets/stylesheets/views/dashboard.scss index 6069a8f20..946b78576 100644 --- a/app/assets/stylesheets/views/dashboard.scss +++ b/app/assets/stylesheets/views/dashboard.scss @@ -2,7 +2,7 @@ table { th { - @include core-16; + @include core-19; border-bottom: 0; } @@ -26,16 +26,16 @@ width: 100%; margin-bottom: $gutter-half; height: $gutter-half; - color: $govuk-blue; + color: $text-colour; span { box-sizing: border-box; - display: block; - background: $govuk-blue; - color: $white; - height: $gutter-half; - padding-left: 5px; - padding-right: 5px; + display: inline-block; + overflow: visible; + background: $panel-colour; + color: $black; + padding: 5px 5px 2px 5px; + text-indent: -2px; margin: 3px 0 5px 0; transition: width 0.6s ease-in-out; } @@ -49,7 +49,7 @@ color: $govuk-blue; max-width: 100%; background: $white; - margin-bottom: $gutter; + margin-bottom: $gutter-two-thirds; } } diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 194cb0563..6bbcb7b5d 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -18,6 +18,7 @@ from app import ( job_api_client, notification_api_client, service_api_client, + statistics_api_client, current_service, format_datetime_short) from app.main import main @@ -26,6 +27,7 @@ from app.utils import ( generate_previous_next_dict, user_has_permissions, generate_notifications_csv) +from app.statistics_utils import sum_of_statistics, statistics_by_state def _parse_filter_args(filter_dict): @@ -46,7 +48,7 @@ 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 'processed' in filter_args.get('status'): + if 'processed' in filter_args.get('status') or not filter_args.get('status'): filter_args['status'] = all_statuses elif 'failed' in filter_args.get('status'): filter_args['status'].extend(all_failure_statuses[1:]) @@ -73,7 +75,12 @@ def view_job(service_id, job_id): template = service_api_client.get_service_template(service_id=service_id, template_id=job['template'], version=job['template_version'])['data'] - notifications = notification_api_client.get_notifications_for_service(service_id, job_id) + + filter_args = _parse_filter_args(request.args) + _set_status_filters(filter_args) + notifications = notification_api_client.get_notifications_for_service( + service_id, job_id, status=filter_args.get('status'), + ) finished = job['status'] == 'finished' return render_template( 'views/jobs/job.html', @@ -93,7 +100,9 @@ def view_job(service_id, job_id): template=Template( template, prefix=current_service['name'] - ) + ), + counts=_get_job_counts(job, request.args.get('help', 0)), + status=request.args.get('status', '') ) @@ -107,12 +116,15 @@ def view_job_csv(service_id, job_id): template_id=job['template'], version=job['template_version'] )['data'] + filter_args = _parse_filter_args(request.args) + _set_status_filters(filter_args) return ( generate_notifications_csv( notification_api_client.get_notifications_for_service( service_id, job_id, + status=filter_args.get('status'), page_size=job['notification_count'] )['notifications'] ), @@ -132,7 +144,11 @@ def view_job_csv(service_id, job_id): @user_has_permissions('view_activity', admin_override=True) def view_job_updates(service_id, job_id): job = job_api_client.get_job(service_id, job_id)['data'] - notifications = notification_api_client.get_notifications_for_service(service_id, job_id) + filter_args = _parse_filter_args(request.args) + _set_status_filters(filter_args) + notifications = notification_api_client.get_notifications_for_service( + service_id, job_id, status=filter_args.get('status') + ) finished = ( job.get('notifications_sent', 0) - job.get('notifications_delivered', 0) - @@ -142,13 +158,16 @@ def view_job_updates(service_id, job_id): 'counts': render_template( 'partials/jobs/count.html', job=job, - finished=finished + finished=finished, + counts=_get_job_counts(job, request.args.get('help', 0)), + status=request.args.get('status', '') ), 'notifications': render_template( 'partials/jobs/notifications.html', job=job, notifications=notifications['notifications'], - finished=finished + finished=finished, + status=request.args.get('status', '') ), 'status': render_template( 'partials/jobs/status.html', @@ -179,6 +198,9 @@ def view_notifications(service_id, message_type): template_type=[message_type], status=filter_args.get('status'), limit_days=current_app.config['ACTIVITY_STATS_LIMIT_DAYS']) + service_statistics_by_state = statistics_by_state(sum_of_statistics( + statistics_api_client.get_statistics_for_service(service_id, limit_days=7)['data'] + )) view_dict = dict( message_type=message_type, status=request.args.get('status') @@ -229,16 +251,20 @@ def view_notifications(service_id, message_type): status=request.args.get('status') ), status_filters=[ - [item[0], item[1], url_for( - '.view_notifications', - service_id=current_service['id'], - message_type=message_type, - status=item[1] - )] for item in [ - ['Processed', 'sending,delivered,failed'], - ['Sending', 'sending'], - ['Delivered', 'delivered'], - ['Failed', 'failed'], + [ + item[0], item[1], + url_for( + '.view_notifications', + service_id=current_service['id'], + message_type=message_type, + status=item[1] + ), + service_statistics_by_state[message_type][item[0]] + ] for item in [ + ['processed', 'sending,delivered,failed'], + ['sending', 'sending'], + ['delivered', 'delivered'], + ['failed', 'failed'], ] ] ) @@ -260,3 +286,41 @@ def view_notification(service_id, job_id, notification_id): uploaded_at=now, job_id=job_id ) + + +def _get_job_counts(job, help_argument): + return [ + ( + label, + query_param, + url_for( + ".view_job", + service_id=job['service'], + job_id=job['id'], + status=query_param, + help=help_argument + ), + count + ) for label, query_param, count in [ + [ + 'Processed', '', + job.get('notifications_sent', 0) + ], + [ + 'Sending', 'sending', + ( + job.get('notifications_sent', 0) - + job.get('notifications_delivered', 0) - + job.get('notifications_failed', 0) + ) + ], + [ + 'Delivered', 'delivered', + job.get('notifications_delivered', 0) + ], + [ + 'Failed', 'failed', + job.get('notifications_failed') + ] + ] + ] diff --git a/app/notify_client/job_api_client.py b/app/notify_client/job_api_client.py index 77d0d1b01..c3d45c5f5 100644 --- a/app/notify_client/job_api_client.py +++ b/app/notify_client/job_api_client.py @@ -14,9 +14,12 @@ class JobApiClient(BaseAPIClient): self.client_id = app.config['ADMIN_CLIENT_USER_NAME'] self.secret = app.config['ADMIN_CLIENT_SECRET'] - def get_job(self, service_id, job_id=None, limit_days=None): + def get_job(self, service_id, job_id=None, limit_days=None, status=None): if job_id: - return self.get(url='/service/{}/job/{}'.format(service_id, job_id)) + params = {} + if status is not None: + params['status'] = status + return self.get(url='/service/{}/job/{}'.format(service_id, job_id), params=params) params = {} if limit_days is not None: params['limit_days'] = limit_days diff --git a/app/statistics_utils.py b/app/statistics_utils.py index 604b76f1d..eca91fb83 100644 --- a/app/statistics_utils.py +++ b/app/statistics_utils.py @@ -48,3 +48,24 @@ def add_rates_to(delivery_statistics): ), **delivery_statistics ) + + +def statistics_by_state(statistics): + return { + 'sms': { + 'processed': statistics['sms_requested'], + 'sending': ( + statistics['sms_requested'] - statistics['sms_failed'] - statistics['sms_delivered'] + ), + 'delivered': statistics['sms_delivered'], + 'failed': statistics['sms_failed'] + }, + 'email': { + 'processed': statistics['emails_requested'], + 'sending': ( + statistics['emails_requested'] - statistics['emails_failed'] - statistics['emails_delivered'] + ), + 'delivered': statistics['emails_delivered'], + 'failed': statistics['emails_failed'] + } + } diff --git a/app/templates/components/big-number.html b/app/templates/components/big-number.html index 062297855..33996580d 100644 --- a/app/templates/components/big-number.html +++ b/app/templates/components/big-number.html @@ -1,4 +1,7 @@ -{% macro big_number(number, label, label_link=None, currency='', smaller=False, smallest=False) %} +{% macro big_number(number, label, link=None, currency='', smaller=False, smallest=False) %} + {% if link %} + + {% endif %}
{% if number is number %} @@ -11,13 +14,13 @@ {{ number }} {% endif %}
- {% if label_link %} -
{{ label }} - - {% elif label %} + {% if label %} {{ label }} {% endif %}
+ {% if link %} + + {% endif %} {% endmacro %} @@ -28,12 +31,12 @@ failure_percentage, danger_zone=False, failure_link=None, - label_link=None, + link=None, show_more_link=None, show_more_text='' ) %}
- {{ big_number(number, label, label_link) }} + {{ big_number(number, label, link=link) }}
{% if failures %} {% if failure_link %} diff --git a/app/templates/components/pill.html b/app/templates/components/pill.html index 2ca56fa25..7f283d204 100644 --- a/app/templates/components/pill.html +++ b/app/templates/components/pill.html @@ -1,3 +1,5 @@ +{% from 'components/big-number.html' import big_number %} + {% macro pill( title, items=[], @@ -5,12 +7,19 @@ ) %} -{% endmacro %} \ No newline at end of file +{% endmacro %} diff --git a/app/templates/partials/jobs/count.html b/app/templates/partials/jobs/count.html index 38ea4e5e7..a1ad9a0af 100644 --- a/app/templates/partials/jobs/count.html +++ b/app/templates/partials/jobs/count.html @@ -1,30 +1,16 @@ -{% from "components/big-number.html" import big_number %} +{% from "components/pill.html" import pill %}
-
    -
  • - {{ big_number( - job.get('notifications_sent', 0) - job.get('notifications_delivered', 0) - job.get('notifications_failed', 0), 'sending' - )}} -
  • -
  • - {{ big_number( - job.get('notifications_delivered', 0), 'delivered' - )}} -
  • -
  • - {{ big_number( - job.notifications_failed, 'failed' - )}} -
  • -
+
+ {{ pill('Status', counts, status) }} +
diff --git a/app/templates/partials/jobs/notifications.html b/app/templates/partials/jobs/notifications.html index cb87d6280..c045d7bae 100644 --- a/app/templates/partials/jobs/notifications.html +++ b/app/templates/partials/jobs/notifications.html @@ -1,19 +1,30 @@ {% from "components/table.html" import list_table, field, right_aligned_field_heading, date_field, row_heading %}
+ {% if notifications %} +

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

+ {% endif %} + {% call(item, row_number) list_table( notifications, caption=uploaded_file_name, caption_visible=False, - empty_message="No messages to show yet", + empty_message="No messages to show", field_headings=[ 'Recipient', 'Time', diff --git a/app/templates/partials/jobs/status.html b/app/templates/partials/jobs/status.html index 2ef0016c7..ec4f0a308 100644 --- a/app/templates/partials/jobs/status.html +++ b/app/templates/partials/jobs/status.html @@ -1,12 +1,12 @@
-

+

Uploaded by {{ job.created_by.name }} on {{ job.created_at|format_datetime_short }}

diff --git a/app/templates/views/dashboard/template-statistics.html b/app/templates/views/dashboard/template-statistics.html index 80e12fdd7..b67b5c70a 100644 --- a/app/templates/views/dashboard/template-statistics.html +++ b/app/templates/views/dashboard/template-statistics.html @@ -1,26 +1,41 @@ +{% from "components/big-number.html" import big_number %} {% from "components/message-count-label.html" import message_count_label %} +{% from "components/big-number.html" import big_number %}
{% for item in template_statistics %}
- +
{{ item.template.name }} + + {{ message_count_label(1, item.template.template_type, suffix='template')|capitalize }} +
{% if template_statistics|length > 1 %} - - - {{ item.usage_count }} {{ message_count_label(item.usage_count, item.template.template_type) }} - + + + {{ big_number( + item.usage_count, + smallest=True + ) }} + + {{ message_count_label(item.usage_count, item.template.template_type) }} + + + {% else %} - {{ item.usage_count }} {{ message_count_label(item.usage_count, item.template.template_type) }} + + {{ item.usage_count }} + {{ message_count_label(item.usage_count, item.template.template_type) }} + {% endif %}
{% endfor %} -
\ No newline at end of file + diff --git a/app/templates/views/dashboard/today.html b/app/templates/views/dashboard/today.html index d13cfe2bc..6a17e85f4 100644 --- a/app/templates/views/dashboard/today.html +++ b/app/templates/views/dashboard/today.html @@ -14,23 +14,23 @@
{{ big_number_with_status( statistics.emails_requested, - message_count_label(statistics.emails_requested, 'email'), + message_count_label(statistics.emails_requested, 'email', suffix=''), 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, message_type='email', status='failed'), - label_link=url_for(".view_notifications", service_id=current_service.id, message_type='email', status='sending,delivered,failed') + link=url_for(".view_notifications", service_id=current_service.id, message_type='email', status='sending,delivered,failed') ) }}
{{ big_number_with_status( statistics.sms_requested, - message_count_label(statistics.sms_requested, 'sms'), + message_count_label(statistics.sms_requested, 'sms', suffix=''), 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, message_type='sms', status='failed'), - label_link=url_for(".view_notifications", service_id=current_service.id, message_type='sms', status='sending,delivered,failed') + link=url_for(".view_notifications", service_id=current_service.id, message_type='sms', status='sending,delivered,failed') ) }}
diff --git a/app/templates/views/jobs/job.html b/app/templates/views/jobs/job.html index b65b8dda0..befbd0aa4 100644 --- a/app/templates/views/jobs/job.html +++ b/app/templates/views/jobs/job.html @@ -36,5 +36,4 @@ {% include 'partials/jobs/notifications.html' %} - {% endblock %} diff --git a/app/templates/views/notifications.html b/app/templates/views/notifications.html index 4e9c14622..faca04682 100644 --- a/app/templates/views/notifications.html +++ b/app/templates/views/notifications.html @@ -3,7 +3,7 @@ {% 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 %} +{% from "components/message-count-label.html" import message_count_label, recipient_count_label %} {% block page_title %} {{ message_count_label(99, message_type, suffix='') | capitalize }} – GOV.UK Notify @@ -15,7 +15,7 @@ {%- if request_args.get('status') != 'delivered,failed' -%} - {%- for label, option, _ in status_filters -%} + {%- for label, option, _, _ in status_filters -%} {%- if request_args.get('status', 'delivered,failed') == option -%}{{label}} {% endif -%} {%- endfor -%} {%- endif -%} diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index d143166d2..ef5ec8942 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -91,11 +91,15 @@ def test_should_show_recent_templates_on_dashboard(app_, assert len(table_rows) == 2 - assert page.find_all('dt')[0].text.strip() == 'Pickle feet' - assert page.find_all('dd')[0].text.strip() == '206 text messages sent' + assert 'Pickle feet' in page.find_all('dt')[0].text + assert 'Text message template' in page.find_all('dt')[0].text + assert '206' in page.find_all('dd')[0].text + assert 'text messages sent' in page.find_all('dd')[0].text - assert page.find_all('dt')[1].text.strip() == 'Brine Shrimp' - assert page.find_all('dd')[1].text.strip() == '13 text messages sent' + assert 'Brine Shrimp' in page.find_all('dt')[1].text + assert 'Text message template' in page.find_all('dt')[1].text + assert '13' in page.find_all('dd')[1].text + assert 'text messages sent' in page.find_all('dd')[1].text def test_should_show_all_templates_on_template_statistics_page( @@ -129,11 +133,15 @@ def test_should_show_all_templates_on_template_statistics_page( assert len(table_rows) == 2 - assert page.find_all('dt')[0].text.strip() == 'Pickle feet' - assert page.find_all('dd')[0].text.strip() == '206 text messages sent' + assert 'Pickle feet' in page.find_all('dt')[0].text + assert 'Text message template' in page.find_all('dt')[0].text + assert '206' in page.find_all('dd')[0].text + assert 'text messages sent' in page.find_all('dd')[0].text - assert page.find_all('dt')[1].text.strip() == 'Brine Shrimp' - assert page.find_all('dd')[1].text.strip() == '13 text messages sent' + assert 'Brine Shrimp' in page.find_all('dt')[1].text + assert 'Text message template' in page.find_all('dt')[1].text + assert '13' in page.find_all('dd')[1].text + assert 'text messages sent' in page.find_all('dd')[1].text def _test_dashboard_menu(mocker, app_, usr, service, permissions): diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 791183dd7..d6922f0a3 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -26,22 +26,54 @@ def test_should_return_list_of_all_jobs(app_, assert len(jobs) == 5 +@pytest.mark.parametrize( + "status_argument, expected_api_call", [ + ( + '', + ['sending', 'delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] + ), + ( + 'processed', + ['sending', 'delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] + ), + ( + 'sending', + ['sending'] + ), + ( + 'delivered', + ['delivered'] + ), + ( + 'failed', + ['failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] + ) + ] +) @freeze_time("2016-01-01 11:09:00.061258") def test_should_show_page_for_one_job( app_, service_one, active_user_with_permissions, mock_get_service_template, + mock_get_service_statistics, mock_get_job, mocker, mock_get_notifications, - fake_uuid + fake_uuid, + status_argument, + expected_api_call ): file_name = mock_get_job(service_one['id'], fake_uuid)['data']['original_file_name'] 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_job', service_id=service_one['id'], job_id=fake_uuid)) + response = client.get(url_for( + 'main.view_job', + service_id=service_one['id'], + job_id=fake_uuid, + status=status_argument + )) assert response.status_code == 200 content = response.get_data(as_text=True) @@ -49,6 +81,22 @@ def test_should_show_page_for_one_job( assert file_name in content assert 'Delivered' in content assert '11:10' in content + assert url_for( + 'main.view_job_updates', + service_id=service_one['id'], + job_id=fake_uuid, + status=status_argument, + ) in content + assert url_for( + 'main.view_job_csv', + service_id=service_one['id'], + job_id=fake_uuid + ) in content + mock_get_notifications.assert_called_with( + service_one['id'], + fake_uuid, + status=expected_api_call + ) @freeze_time("2016-01-01 11:09:00.061258") @@ -57,6 +105,7 @@ def test_should_show_updates_for_one_job_as_json( service_one, active_user_with_permissions, mock_get_notifications, + mock_get_service_statistics, mock_get_job, mocker, fake_uuid @@ -112,6 +161,7 @@ def test_can_show_notifications( service_one, active_user_with_permissions, mock_get_notifications, + mock_get_service_statistics, mocker, message_type, page_title, @@ -166,11 +216,14 @@ def test_can_show_notifications( assert 'text/csv' in csv_response.headers['Content-Type'] -def test_should_show_notifications_for_a_service_with_next_previous(app_, - service_one, - active_user_with_permissions, - mock_get_notifications_with_previous_next, - mocker): +def test_should_show_notifications_for_a_service_with_next_previous( + app_, + service_one, + active_user_with_permissions, + mock_get_notifications_with_previous_next, + mock_get_service_statistics, + mocker +): with app_.test_request_context(): with app_.test_client() as client: client.login(active_user_with_permissions, mocker, service_one) diff --git a/tests/app/test_statistics_utils.py b/tests/app/test_statistics_utils.py index 5b6c385bc..839fbd8b1 100644 --- a/tests/app/test_statistics_utils.py +++ b/tests/app/test_statistics_utils.py @@ -1,6 +1,6 @@ import pytest -from app.statistics_utils import sum_of_statistics, add_rates_to +from app.statistics_utils import sum_of_statistics, add_rates_to, statistics_by_state @pytest.mark.parametrize('delivery_statistics', [ @@ -96,3 +96,20 @@ def test_add_rates_keeps_original_raw_data(): assert resp['emails_requested'] == 2 assert resp['sms_failed'] == 3 assert resp['sms_requested'] == 4 + + +def test_service_statistics_by_state(): + resp = statistics_by_state({ + 'emails_requested': 3, + 'emails_failed': 1, + 'emails_delivered': 1, + 'sms_requested': 3, + 'sms_failed': 1, + 'sms_delivered': 1 + }) + + for message_type in ['email', 'sms']: + assert resp[message_type]['processed'] == 3 + assert resp[message_type]['sending'] == 1 + assert resp[message_type]['delivered'] == 1 + assert resp[message_type]['failed'] == 1