From 88a557f6c875cf174d54d05d473e913870b93f48 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 25 Oct 2019 15:52:22 +0100 Subject: [PATCH 1/3] =?UTF-8?q?Don=E2=80=99t=20show=20template=20stats=20i?= =?UTF-8?q?f=20one=20template=20used?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If you’ve only used one template then this section of the page isn’t doing its job, which is to show a comparison of the different kinds of message you’re showing. I think our initial assumption was that everyone would be using multiple templates, so it was good to show this part of the page during the onboarding, to show users where the information was going to appear. But we have lots of services who only send one template now, typically where they’re populating the contents of the template themselves. In which case this part of the page doesn’t offer them any value. --- app/templates/views/dashboard/dashboard.html | 8 +- .../views/dashboard/template-statistics.html | 87 ++++++++++--------- tests/app/main/views/test_dashboard.py | 41 ++++++++- 3 files changed, 89 insertions(+), 47 deletions(-) diff --git a/app/templates/views/dashboard/dashboard.html b/app/templates/views/dashboard/dashboard.html index e480efe958..718187a7b8 100644 --- a/app/templates/views/dashboard/dashboard.html +++ b/app/templates/views/dashboard/dashboard.html @@ -33,13 +33,7 @@

'See messages sent per month' ) }} - {% if partials['has_template_statistics'] %} - {{ ajax_block(partials, updates_url, 'template-statistics', interval=5) }} - {{ show_more( - url_for('.template_usage', service_id=current_service.id), - 'See templates used by month' - ) }} - {% endif %} + {{ ajax_block(partials, updates_url, 'template-statistics', interval=5) }} {% if partials['has_jobs'] %} {{ ajax_block(partials, updates_url, 'jobs', interval=5) }} diff --git a/app/templates/views/dashboard/template-statistics.html b/app/templates/views/dashboard/template-statistics.html index 1c2564035b..1b6fa70d86 100644 --- a/app/templates/views/dashboard/template-statistics.html +++ b/app/templates/views/dashboard/template-statistics.html @@ -2,46 +2,55 @@ {% from "components/message-count-label.html" import message_count_label %} {% from "components/big-number.html" import big_number %} {% from "components/table.html" import list_table, field, right_aligned_field_heading, row_heading, spark_bar_field %} +{% from "components/show-more.html" import show_more %} -
- {% call(item, row_number) list_table( - template_statistics, - caption="Templates used", - caption_visible=False, - empty_message='', - field_headings=[ - 'Template', - 'Messages sent' - ], - field_headings_visible=True - ) %} +
+ {% if template_statistics|length > 1 %} +
+ {% call(item, row_number) list_table( + template_statistics, + caption="Templates used", + caption_visible=False, + empty_message='', + field_headings=[ + 'Template', + 'Messages sent' + ], + field_headings_visible=True + ) %} - {% call row_heading() %} - {% if item.is_precompiled_letter %} - - Provided as PDF - - - Letter - - {% else %} - {{ item.template_name }} - - {{ message_count_label(1, item.template_type, suffix='template')|capitalize }} - - {% endif %} - {% endcall %} - {% if template_statistics|length > 1 %} - {{ spark_bar_field(item.count, most_used_template_count, id=item.template_id) }} - {% else %} - {% call field() %} - - {{ big_number( - item.count, - smallest=True - ) }} - + {% call row_heading() %} + {% if item.is_precompiled_letter %} + + Provided as PDF + + + Letter + + {% else %} + {{ item.template_name }} + + {{ message_count_label(1, item.template_type, suffix='template')|capitalize }} + + {% endif %} + {% endcall %} + {% if template_statistics|length > 1 %} + {{ spark_bar_field(item.count, most_used_template_count, id=item.template_id) }} + {% else %} + {% call field() %} + + {{ big_number( + item.count, + smallest=True + ) }} + + {% endcall %} + {% endif %} {% endcall %} - {% endif %} - {% endcall %} + {{ show_more( + url_for('.template_usage', service_id=current_service.id), + 'See templates used by month' + ) }} +
+ {% endif %}
diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index c06057af1f..409c98362d 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -496,6 +496,45 @@ def test_should_show_recent_templates_on_dashboard( assert '100' in table_rows[3].find_all('td')[0].text +@pytest.mark.parametrize('stats', ( + pytest.param( + [stub_template_stats[0]], + ), + pytest.param( + [stub_template_stats[0], stub_template_stats[1]], + marks=pytest.mark.xfail(raises=AssertionError), + ) +)) +def test_should_not_show_recent_templates_on_dashboard_if_only_one_template_used( + client_request, + mocker, + mock_get_service_templates, + mock_get_jobs, + mock_get_service_statistics, + mock_get_usage, + mock_get_inbound_sms_summary, + stats, +): + mock_template_stats = mocker.patch( + 'app.template_statistics_client.get_template_statistics_for_service', + return_value=stats, + ) + + page = client_request.get('main.service_dashboard', service_id=SERVICE_ONE_ID) + main = page.select_one('main').text + + mock_template_stats.assert_called_once_with(SERVICE_ONE_ID, limit_days=7) + + assert stats[0]['template_name'] == 'one' + assert stats[0]['template_name'] not in main + + # count appears as total, but not per template + expected_count = stats[0]['count'] + assert expected_count == 50 + assert main.count(str(expected_count)) == 1 + assert '50 text messages sent' in normalize_spaces(main) + + @freeze_time("2016-07-01 12:00") # 4 months into 2016 financial year @pytest.mark.parametrize('extra_args', [ {}, @@ -763,7 +802,7 @@ def test_should_show_recent_jobs_on_dashboard( assert third_call[1]['limit_days'] == 7 assert 'scheduled' not in third_call[1]['statuses'] - table_rows = page.find_all('tbody')[2].find_all('tr') + table_rows = page.find_all('tbody')[1].find_all('tr') assert len(table_rows) == 4 From c919c25bf8d71fd2e5c1a897580bc5dd681035dd Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 30 Oct 2019 10:46:48 +0000 Subject: [PATCH 2/3] Remove unreachable code An `if` statement higher up the page means this code will never get run now. We can simplify the template by removing it. --- .../views/dashboard/template-statistics.html | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/app/templates/views/dashboard/template-statistics.html b/app/templates/views/dashboard/template-statistics.html index 1b6fa70d86..cb3554afaf 100644 --- a/app/templates/views/dashboard/template-statistics.html +++ b/app/templates/views/dashboard/template-statistics.html @@ -34,18 +34,7 @@ {% endif %} {% endcall %} - {% if template_statistics|length > 1 %} - {{ spark_bar_field(item.count, most_used_template_count, id=item.template_id) }} - {% else %} - {% call field() %} - - {{ big_number( - item.count, - smallest=True - ) }} - - {% endcall %} - {% endif %} + {{ spark_bar_field(item.count, most_used_template_count, id=item.template_id) }} {% endcall %} {{ show_more( url_for('.template_usage', service_id=current_service.id), From 0ac6a2c1aba790ae1028d212629a965d39319259 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 30 Oct 2019 11:08:03 +0000 Subject: [PATCH 3/3] Remove unused variable We use different logic to decide whether to show/hide the template statistics part of the dashboard now. --- app/main/views/dashboard.py | 1 - 1 file changed, 1 deletion(-) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index b7e64997c1..437ff0644c 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -334,7 +334,6 @@ def get_dashboard_partials(service_id): [row['count'] for row in template_statistics] or [0] ), ), - 'has_template_statistics': bool(template_statistics), 'jobs': render_template( 'views/dashboard/_jobs.html', jobs=immediate_jobs