From ac9d4f2daf0594c67da69aa6a70dc24eff3cc1a4 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 30 Jan 2017 17:27:09 +0000 Subject: [PATCH 1/2] Break down usage by month, filter by year MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous, weekly activity breakdown was what we reckoned might be useful. But now that we have people using the platform it feels like aggregating a service’s usage by month is: - matches the timeframe users report on within their organisation - is consistent with the usage page And like the usage page this commit also limits the page to only show one financial year’s worth of data at once (rather than data for all time). This commit also makes some changes to the jobs view code so that our aggregation of failure states is consistent between the dashboard pages and the jobs pages. --- app/assets/stylesheets/app.scss | 2 +- app/assets/stylesheets/views/dashboard.scss | 5 ++ app/main/views/dashboard.py | 80 ++++++++++++------ app/main/views/jobs.py | 11 +-- app/notify_client/service_api_client.py | 4 +- app/templates/views/dashboard/dashboard.html | 4 +- app/templates/views/dashboard/monthly.html | 69 ++++++++++++++++ app/templates/views/weekly.html | 50 ----------- app/utils.py | 6 ++ tests/app/main/views/test_dashboard.py | 87 ++++++++++++-------- tests/app/main/views/test_jobs.py | 16 +++- 11 files changed, 210 insertions(+), 124 deletions(-) create mode 100644 app/templates/views/dashboard/monthly.html delete mode 100644 app/templates/views/weekly.html diff --git a/app/assets/stylesheets/app.scss b/app/assets/stylesheets/app.scss index b58d74b3f..570d997e0 100644 --- a/app/assets/stylesheets/app.scss +++ b/app/assets/stylesheets/app.scss @@ -210,7 +210,7 @@ details summary { table th, table td { - font-size: 19px; + @include core-19; } } diff --git a/app/assets/stylesheets/views/dashboard.scss b/app/assets/stylesheets/views/dashboard.scss index 1b65b6a4b..16fd35d07 100644 --- a/app/assets/stylesheets/views/dashboard.scss +++ b/app/assets/stylesheets/views/dashboard.scss @@ -75,3 +75,8 @@ } } + +.failure-highlight { + @include bold-19; + color: $error-colour; +} diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index f171faf26..230ff3c96 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -19,7 +19,14 @@ from app import ( template_statistics_client ) from app.statistics_utils import get_formatted_percentage, add_rate_to_job -from app.utils import user_has_permissions, get_current_financial_year +from app.utils import ( + user_has_permissions, + get_current_financial_year, + FAILURE_STATUSES, + SENDING_STATUSES, + DELIVERED_STATUSES, + REQUESTED_STATUSES, +) # This is a placeholder view method to be replaced @@ -79,11 +86,7 @@ def template_history(service_id): @login_required @user_has_permissions('manage_settings', admin_override=True) def usage(service_id): - current_financial_year = get_current_financial_year() - try: - year = int(request.args.get('year', current_financial_year)) - except ValueError: - abort(404) + year, current_financial_year = requested_and_current_financial_year(request) return render_template( 'views/usage.html', months=list(get_free_paid_breakdown_for_billable_units( @@ -102,15 +105,25 @@ def usage(service_id): ) -@main.route("/services//weekly") +@main.route("/services//monthly") @login_required @user_has_permissions('manage_settings', admin_override=True) -def weekly(service_id): - stats = service_api_client.get_weekly_notification_stats(service_id)['data'] +def monthly(service_id): + year, current_financial_year = requested_and_current_financial_year(request) return render_template( - 'views/weekly.html', - days=format_weekly_stats_to_list(stats), - now=datetime.utcnow() + 'views/dashboard/monthly.html', + months=format_monthly_stats_to_list( + service_api_client.get_monthly_notification_stats(service_id, year)['data'] + ), + years=[ + ( + 'financial year', + year, + url_for('.monthly', service_id=service_id, year=year), + '{} to {}'.format(year, year + 1), + ) for year in range(2015, current_financial_year + 1) + ], + selected_year=year, ) @@ -198,22 +211,27 @@ def calculate_usage(usage): } -def format_weekly_stats_to_list(historical_stats): - out = [] - for week, weekly_stats in historical_stats.items(): - for stats in weekly_stats.values(): - stats['failure_rate'] = get_formatted_percentage(stats['failed'], stats['requested']) +def format_monthly_stats_to_list(historical_stats): + return sorted(( + dict( + date=key, + name=datetime(int(key[0:4]), int(key[5:7]), 1).strftime('%B'), + **aggregate_status_types(value) + ) for key, value in historical_stats.items() + ), key=lambda x: x['date']) - week_start = dateutil.parser.parse(week) - week_end = week_start + timedelta(days=6) - weekly_stats.update({ - 'week_start': week, - 'week_end': week_end.date().isoformat(), - 'week_end_datetime': week_end, - }) - out.append(weekly_stats) - return sorted(out, key=lambda x: x['week_start'], reverse=True) +def aggregate_status_types(counts_dict): + return get_dashboard_totals({ + '{}_counts'.format(message_type): { + 'failed': sum( + stats.get(status, 0) for status in FAILURE_STATUSES + ), + 'requested': sum( + stats.get(status, 0) for status in REQUESTED_STATUSES + ) + } for message_type, stats in counts_dict.items() + }) def get_months_for_financial_year(year): @@ -269,3 +287,13 @@ def get_free_paid_breakdown_for_month( 'paid': monthly_usage, 'free': 0 } + + +def requested_and_current_financial_year(request): + try: + return ( + int(request.args.get('year', get_current_financial_year())), + get_current_financial_year(), + ) + except ValueError: + abort(404) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 2c282a7d3..846b23074 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -33,6 +33,9 @@ from app.utils import ( generate_notifications_csv, get_help_argument, get_template, + REQUESTED_STATUSES, + FAILURE_STATUSES, + SENDING_STATUSES, ) from app.statistics_utils import add_rate_to_job @@ -53,12 +56,10 @@ def _parse_filter_args(filter_dict): def _set_status_filters(filter_args): status_filters = filter_args.get('status', []) - all_failure_statuses = ['failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] - all_sending_statuses = ['created', 'sending'] return list(OrderedSet(chain( - (status_filters or all_sending_statuses + ['delivered'] + all_failure_statuses), - all_sending_statuses if 'sending' in status_filters else [], - all_failure_statuses if 'failed' in status_filters else [] + (status_filters or REQUESTED_STATUSES), + SENDING_STATUSES if 'sending' in status_filters else [], + FAILURE_STATUSES if 'failed' in status_filters else [] ))) diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index 211eb46a1..8ab6049ae 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -219,8 +219,8 @@ class ServiceAPIClient(NotifyAdminAPIClient): params=dict(year=year) ) - def get_weekly_notification_stats(self, service_id): - return self.get(url='/service/{}/notifications/weekly'.format(service_id)) + def get_monthly_notification_stats(self, service_id, year): + return self.get(url='/service/{}/notifications/monthly?year={}'.format(service_id, year)) def get_whitelist(self, service_id): return self.get(url='/service/{}/whitelist'.format(service_id)) diff --git a/app/templates/views/dashboard/dashboard.html b/app/templates/views/dashboard/dashboard.html index 9ea92fbed..153030719 100644 --- a/app/templates/views/dashboard/dashboard.html +++ b/app/templates/views/dashboard/dashboard.html @@ -31,8 +31,8 @@ {{ ajax_block(partials, updates_url, 'totals') }} {{ show_more( - url_for('.weekly', service_id=current_service.id), - 'Compare to previous weeks' + url_for('.monthly', service_id=current_service.id), + 'See activity breakdown' ) }} {% if partials['has_template_statistics'] %} diff --git a/app/templates/views/dashboard/monthly.html b/app/templates/views/dashboard/monthly.html new file mode 100644 index 000000000..6b5d88d25 --- /dev/null +++ b/app/templates/views/dashboard/monthly.html @@ -0,0 +1,69 @@ +{% from "components/big-number.html" import big_number_with_status, big_number %} +{% from "components/pill.html" import pill %} +{% from "components/table.html" import list_table, field, hidden_field_heading, right_aligned_field_heading, row_heading %} +{% from "components/message-count-label.html" import message_count_label %} + +{% extends "withnav_template.html" %} + +{% block page_title %} + Previous weeks – GOV.UK Notify +{% endblock %} + +{% block maincolumn_content %} + +

+ Activity breakdown +

+
+ {{ pill( + 'financial year', + items=years, + current_value=selected_year, + big_number_args={'smallest': True}, + ) }} +
+ {% if months %} +
+ {% call(month, row_index) list_table( + months, + caption="Total spend", + caption_visible=False, + empty_message='', + field_headings=[ + 'Month', + 'Emails', + 'Text messages', + ], + field_headings_visible=False + ) %} + {% call row_heading() %} + {{ month.name }} + {% endcall %} + {% for counts, template_type in [ + (month.email_counts, 'email'), + (month.sms_counts, 'sms') + ] %} + {% call field(align='left') %} + {{ big_number( + counts.requested, + message_count_label(counts.requested, template_type, suffix=''), + smallest=True, + ) }} + {% if counts.requested %} + + {{ counts.failed }} failed + + {% else %} + – + {% endif %} + {% endcall %} + {% endfor %} + {% endcall %} +
+ {% endif %} + +

+ Financial year ends 31 March. +

+ +{% endblock %} diff --git a/app/templates/views/weekly.html b/app/templates/views/weekly.html deleted file mode 100644 index 5a5a87493..000000000 --- a/app/templates/views/weekly.html +++ /dev/null @@ -1,50 +0,0 @@ -{% from "components/table.html" import list_table, field, hidden_field_heading, right_aligned_field_heading %} - -{% extends "withnav_template.html" %} - -{% block page_title %} - Previous weeks – GOV.UK Notify -{% endblock %} - -{% block maincolumn_content %} - -

- Previous weeks -

- {% call(item, row_number) list_table( - days, - caption="Daily", - caption_visible=False, - empty_message='No data found', - field_headings=[ - hidden_field_heading('Day'), - right_aligned_field_heading('Emails'), - right_aligned_field_heading('Failure rate'), - right_aligned_field_heading('Text messages'), - right_aligned_field_heading('Failure rate') - ] - ) %} - {% call field() %} - {{ item.week_start|format_date_short }} to - {% if item.week_end_datetime > now %} - today - {% else %} - {{ item.week_end|format_date_short }} - {% endif %} - {% endcall %} - {% call field(align='right') %} - {{ item.email.requested }} - {% endcall %} - {% call field(align='right') %} - {{ item.email.failure_rate }}% - {% endcall %} - {% call field(align='right') %} - {{ item.sms.requested }} - {% endcall %} - {% call field(align='right') %} - {{ item.sms.failure_rate }}% - {% endcall %} - {% endcall %} - - -{% endblock %} diff --git a/app/utils.py b/app/utils.py index 002a2dd01..0c03a86c5 100644 --- a/app/utils.py +++ b/app/utils.py @@ -32,6 +32,12 @@ import pyexcel.ext.xlsx import pyexcel.ext.ods3 +SENDING_STATUSES = ['created', 'pending', 'sending'] +DELIVERED_STATUSES = ['delivered'] +FAILURE_STATUSES = ['failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] +REQUESTED_STATUSES = SENDING_STATUSES + DELIVERED_STATUSES + FAILURE_STATUSES + + class BrowsableItem(object): """ Maps for the template browse-list. diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index e3bb754c0..594e79d93 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -8,8 +8,9 @@ from freezegun import freeze_time from app.main.views.dashboard import ( get_dashboard_totals, - format_weekly_stats_to_list, - get_free_paid_breakdown_for_billable_units + format_monthly_stats_to_list, + get_free_paid_breakdown_for_billable_units, + aggregate_status_types, ) from tests import validate_route_permission @@ -534,49 +535,67 @@ def test_get_dashboard_totals_adds_warning(failures, expected): assert get_dashboard_totals(stats)['sms']['show_warning'] == expected -def test_format_weekly_stats_to_list_empty_case(): - assert format_weekly_stats_to_list({}) == [] +def test_format_monthly_stats_empty_case(): + assert format_monthly_stats_to_list({}) == [] -def test_format_weekly_stats_to_list_sorts_by_week(): - stats = { - '2016-07-04': {}, - '2016-07-11': {}, - '2016-07-18': {}, - '2016-07-25': {} +def test_format_monthly_stats_labels_month(): + resp = format_monthly_stats_to_list({'2016-07': {}}) + assert resp[0]['name'] == 'July' + + +def test_format_monthly_stats_has_stats_with_failure_rate(): + resp = format_monthly_stats_to_list({ + '2016-07': {'sms': _stats(3, 1, 2)} + }) + assert resp[0]['sms_counts'] == { + 'failed': 2, + 'failed_percentage': '66.7', + 'requested': 3, + 'show_warning': True, } - resp = format_weekly_stats_to_list(stats) - assert resp[0]['week_start'] == '2016-07-25' - assert resp[1]['week_start'] == '2016-07-18' - assert resp[2]['week_start'] == '2016-07-11' - assert resp[3]['week_start'] == '2016-07-04' -def test_format_weekly_stats_to_list_includes_datetime_for_comparison(): - stats = { - '2016-07-25': {} - } - resp = format_weekly_stats_to_list(stats) - assert resp == [{ - 'week_start': '2016-07-25', - 'week_end': '2016-07-31', - 'week_end_datetime': datetime(2016, 7, 31, 0, 0, 0) - }] - - -def test_format_weekly_stats_to_list_has_stats_with_failure_rate(): - stats = { - '2016-07-25': {'sms': _stats(3, 1, 2)} - } - resp = format_weekly_stats_to_list(stats) - assert resp[0]['sms']['failure_rate'] == '66.7' - assert resp[0]['sms']['requested'] == 3 +def test_format_monthly_stats_works_for_email_letter(): + resp = format_monthly_stats_to_list({ + '2016-07': { + 'sms': {}, + 'email': {}, + 'letter': {}, + } + }) + assert isinstance(resp[0]['sms_counts'], dict) + assert isinstance(resp[0]['email_counts'], dict) + assert isinstance(resp[0]['letter_counts'], dict) def _stats(requested, delivered, failed): return {'requested': requested, 'delivered': delivered, 'failed': failed} +@pytest.mark.parametrize('dict_in, expected_failed, expected_requested', [ + ( + {}, + 0, + 0 + ), + ( + {'temporary-failure': 1, 'permanent-failure': 1, 'technical-failure': 1}, + 3, + 3, + ), + ( + {'created': 1, 'pending': 1, 'sending': 1, 'delivered': 1}, + 0, + 4, + ), +]) +def test_aggregate_status_types(dict_in, expected_failed, expected_requested): + sms_counts = aggregate_status_types({'sms': dict_in})['sms_counts'] + assert sms_counts['failed'] == expected_failed + assert sms_counts['requested'] == expected_requested + + @pytest.mark.parametrize( 'now, expected_number_of_months', [ (freeze_time("2017-12-31 11:09:00.061258"), 12), diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index ebe0002b4..1f7d58da0 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -68,11 +68,15 @@ def test_get_jobs_shows_page_links( "status_argument, expected_api_call", [ ( '', - ['created', 'sending', 'delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] + [ + 'created', 'pending', 'sending', + 'delivered', + 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure', + ] ), ( 'sending', - ['sending', 'created'] + ['sending', 'created', 'pending'] ), ( 'delivered', @@ -304,11 +308,15 @@ def test_should_show_updates_for_one_job_as_json( "status_argument, expected_api_call", [ ( '', - ['created', 'sending', 'delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure'] + [ + 'created', 'pending', 'sending', + 'delivered', + 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure', + ] ), ( 'sending', - ['sending', 'created'] + ['sending', 'created', 'pending'] ), ( 'delivered', From 3c7b41aace53bf9d96f88fa64a94bf0b48392317 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 8 Feb 2017 11:16:11 +0000 Subject: [PATCH 2/2] Limit months shown to current and past MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Matches what we do on the usage page. No need to see months in the future because there’s no way you’ll have sent any messages in those months, unless you’re Marty McFly. --- app/main/views/dashboard.py | 7 +++- app/templates/views/dashboard/monthly.html | 44 +++++++++++----------- 2 files changed, 29 insertions(+), 22 deletions(-) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 230ff3c96..8de2ee003 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -215,12 +215,17 @@ def format_monthly_stats_to_list(historical_stats): return sorted(( dict( date=key, - name=datetime(int(key[0:4]), int(key[5:7]), 1).strftime('%B'), + future=YYYY_MM_to_datetime(key) > datetime.utcnow(), + name=YYYY_MM_to_datetime(key).strftime('%B'), **aggregate_status_types(value) ) for key, value in historical_stats.items() ), key=lambda x: x['date']) +def YYYY_MM_to_datetime(string): + return datetime(int(string[0:4]), int(string[5:7]), 1) + + def aggregate_status_types(counts_dict): return get_dashboard_totals({ '{}_counts'.format(message_type): { diff --git a/app/templates/views/dashboard/monthly.html b/app/templates/views/dashboard/monthly.html index 6b5d88d25..dd4e13fbc 100644 --- a/app/templates/views/dashboard/monthly.html +++ b/app/templates/views/dashboard/monthly.html @@ -36,28 +36,30 @@ ], field_headings_visible=False ) %} - {% call row_heading() %} - {{ month.name }} - {% endcall %} - {% for counts, template_type in [ - (month.email_counts, 'email'), - (month.sms_counts, 'sms') - ] %} - {% call field(align='left') %} - {{ big_number( - counts.requested, - message_count_label(counts.requested, template_type, suffix=''), - smallest=True, - ) }} - {% if counts.requested %} - - {{ counts.failed }} failed - - {% else %} - – - {% endif %} + {% if not month.future %} + {% call row_heading() %} + {{ month.name }} {% endcall %} - {% endfor %} + {% for counts, template_type in [ + (month.email_counts, 'email'), + (month.sms_counts, 'sms') + ] %} + {% call field(align='left') %} + {{ big_number( + counts.requested, + message_count_label(counts.requested, template_type, suffix=''), + smallest=True, + ) }} + {% if counts.requested %} + + {{ counts.failed }} failed + + {% else %} + – + {% endif %} + {% endcall %} + {% endfor %} + {% endif %} {% endcall %} {% endif %}