From e373296bd9726f821ffe8f4db4486112f82daf06 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 22 May 2017 17:02:03 +0100 Subject: [PATCH] Show inbound messages on the dashboard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit adds two things: a section on the dashboard to show how many inbound messages the service has received in the last 7 days, and how recently an inbound message has been received --- Doesn’t show the contents of any messages, just like how the rest of the dashboard is an aggregation, never individual messages. a page to show all the inbound messages the service has received in the last 7 days --- This shows the first line of the message. Eventually this will link through to a ‘conversation’ page, where a service can see all the messages it’s received from a given phone number. --- .../stylesheets/components/big-number.scss | 23 +++++ app/assets/stylesheets/components/table.scss | 8 ++ app/assets/stylesheets/views/dashboard.scss | 4 + app/main/views/dashboard.py | 14 +++ app/notify_client/service_api_client.py | 11 +++ app/templates/components/big-number.html | 25 ++--- app/templates/views/dashboard/_inbox.html | 19 ++++ app/templates/views/dashboard/dashboard.html | 2 + app/templates/views/dashboard/inbox.html | 43 +++++++++ tests/app/main/views/test_dashboard.py | 96 ++++++++++++++++++- tests/app/main/views/test_sign_out.py | 1 + tests/app/test_utils.py | 6 +- tests/conftest.py | 62 ++++++++++++ 13 files changed, 300 insertions(+), 14 deletions(-) create mode 100644 app/templates/views/dashboard/_inbox.html create mode 100644 app/templates/views/dashboard/inbox.html diff --git a/app/assets/stylesheets/components/big-number.scss b/app/assets/stylesheets/components/big-number.scss index 2375757dd..d2cfba1b0 100644 --- a/app/assets/stylesheets/components/big-number.scss +++ b/app/assets/stylesheets/components/big-number.scss @@ -129,3 +129,26 @@ } } + +.big-number-meta-wrapper { + + position: relative; + margin: $gutter-half 0 $gutter 0; + background: $govuk-blue; + + .big-number-meta { + + padding: ($gutter / 3) $gutter-half; + color: $white; + pointer-events: none; + + @include media(desktop) { + position: absolute; + bottom: 7px; + right: 5px; + text-align: right; + } + + } + +} diff --git a/app/assets/stylesheets/components/table.scss b/app/assets/stylesheets/components/table.scss index 7d1931395..152d209be 100644 --- a/app/assets/stylesheets/components/table.scss +++ b/app/assets/stylesheets/components/table.scss @@ -210,3 +210,11 @@ a.table-show-more-link { border-bottom: 1px solid $border-colour; padding: 0.75em 0 0.5625em 0; } + +.wide-left-hand-column { + display: block; + max-width: 560px; + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis; +} diff --git a/app/assets/stylesheets/views/dashboard.scss b/app/assets/stylesheets/views/dashboard.scss index a99623384..a1fed973a 100644 --- a/app/assets/stylesheets/views/dashboard.scss +++ b/app/assets/stylesheets/views/dashboard.scss @@ -94,3 +94,7 @@ } } + +.align-with-message-body { + margin-top: $gutter * 5 / 6; +} diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 2d59d822f..aef002398 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -135,6 +135,16 @@ def monthly(service_id): ) +@main.route("/services//inbox") +@login_required +@user_has_permissions('manage_settings', admin_override=True) +def inbox(service_id): + return render_template( + 'views/dashboard/inbox.html', + messages=service_api_client.get_inbound_sms(service_id), + ) + + def aggregate_usage(template_statistics, sort_key='count'): return sorted( template_statistics, @@ -166,6 +176,10 @@ def get_dashboard_partials(service_id): 'views/dashboard/_upcoming.html', scheduled_jobs=scheduled_jobs ), + 'inbox': render_template( + 'views/dashboard/_inbox.html', + inbound_sms_summary=service_api_client.get_inbound_sms_summary(service_id), + ), 'totals': render_template( 'views/dashboard/_totals.html', service_id=service_id, diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index e97919257..b5c82656c 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -1,4 +1,5 @@ from __future__ import unicode_literals + from flask import url_for from app.utils import BrowsableItem from app.notify_client import _attach_current_user, NotifyAdminAPIClient @@ -237,6 +238,16 @@ class ServiceAPIClient(NotifyAdminAPIClient): params=dict(year=year) ) + def get_inbound_sms(self, service_id): + return self.get( + '/service/{}/inbound-sms'.format(service_id) + )['data'] + + def get_inbound_sms_summary(self, service_id): + return self.get( + '/service/{}/inbound-sms/summary'.format(service_id) + ) + class ServicesBrowsableItem(BrowsableItem): @property diff --git a/app/templates/components/big-number.html b/app/templates/components/big-number.html index af0e56383..048a0f3df 100644 --- a/app/templates/components/big-number.html +++ b/app/templates/components/big-number.html @@ -31,22 +31,25 @@ failure_percentage, danger_zone=False, failure_link=None, - link=None + link=None, + show_failures=True ) %}
{{ big_number(number, label, link=link) }} -
- {% if failures %} - {% if failure_link %} - + {% if show_failures %} +
+ {% if failures %} + {% if failure_link %} + + {{ "{:,}".format(failures) }} failed – {{ failure_percentage }}% + + {% else %} {{ "{:,}".format(failures) }} failed – {{ failure_percentage }}% - + {% endif %} {% else %} - {{ "{:,}".format(failures) }} failed – {{ failure_percentage }}% + No failures {% endif %} - {% else %} - No failures - {% endif %} -
+
+ {% endif %}
{% endmacro %} diff --git a/app/templates/views/dashboard/_inbox.html b/app/templates/views/dashboard/_inbox.html new file mode 100644 index 000000000..7cba62016 --- /dev/null +++ b/app/templates/views/dashboard/_inbox.html @@ -0,0 +1,19 @@ +{% from "components/big-number.html" import big_number, big_number_with_status %} + +
+
+ {{ + big_number_with_status( + inbound_sms_summary.count, + 'text messages received', + link=url_for('.inbox', service_id=current_service.id), + show_failures=False + ) + }} +
+ {% if inbound_sms_summary.latest_message %} + latest message {{ inbound_sms_summary.latest_message | format_delta }} + {% endif %} +
+
+
diff --git a/app/templates/views/dashboard/dashboard.html b/app/templates/views/dashboard/dashboard.html index c18fbb4e4..8e7e58f40 100644 --- a/app/templates/views/dashboard/dashboard.html +++ b/app/templates/views/dashboard/dashboard.html @@ -29,6 +29,8 @@ In the last 7 days + {{ ajax_block(partials, updates_url, 'inbox') }} + {{ ajax_block(partials, updates_url, 'totals') }} {{ show_more( url_for('.monthly', service_id=current_service.id), diff --git a/app/templates/views/dashboard/inbox.html b/app/templates/views/dashboard/inbox.html new file mode 100644 index 000000000..ace1cafbd --- /dev/null +++ b/app/templates/views/dashboard/inbox.html @@ -0,0 +1,43 @@ +{% 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 service_page_title %} + Inbox +{% endblock %} + +{% block maincolumn_content %} + +

+ Received text messages +

+
+ {% call(item, row_number) list_table( + messages, + caption="Inbox", + caption_visible=False, + empty_message='When users text your service’s phone number ({}) you’ll see the messages here'.format(current_service.sms_sender), + field_headings=[ + 'From', + 'First two lines of message' + ], + field_headings_visible=False + ) %} + {% call field() %} + {{ item.user_number }} + {{ item.content }} + {% endcall %} + {% call field(align='right') %} + + {{ item.created_at | format_delta }} + + {% endcall %} + {% endcall %} + {% if messages %} +

+ Showing all {{ messages | length }} messages +

+ {% endif %} +
+{% endblock %} diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index b6c8b9404..32a2e5319 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -5,6 +5,7 @@ from unittest.mock import call, ANY from flask import url_for import pytest from bs4 import BeautifulSoup +from datetime import datetime, timedelta from freezegun import freeze_time from app.main.views.dashboard import ( @@ -18,7 +19,11 @@ from app.main.views.dashboard import ( ) from tests import validate_route_permission -from tests.conftest import SERVICE_ONE_ID +from tests.conftest import ( + SERVICE_ONE_ID, + mock_get_inbound_sms_summary, + mock_get_inbound_sms_summary_with_no_messages, +) from tests.app.test_utils import normalize_spaces stub_template_stats = [ @@ -44,6 +49,7 @@ def test_get_started( mock_get_jobs, mock_get_detailed_service, mock_get_usage, + mock_get_inbound_sms_summary, ): mock_template_stats = mocker.patch('app.template_statistics_client.get_template_statistics_for_service', return_value=copy.deepcopy(stub_template_stats)) @@ -62,6 +68,7 @@ def test_get_started_is_hidden_once_templates_exist( mock_get_jobs, mock_get_detailed_service, mock_get_usage, + mock_get_inbound_sms_summary, ): mock_template_stats = mocker.patch('app.template_statistics_client.get_template_statistics_for_service', return_value=copy.deepcopy(stub_template_stats)) @@ -72,6 +79,83 @@ def test_get_started_is_hidden_once_templates_exist( assert 'Get started' not in response.get_data(as_text=True) +@pytest.mark.parametrize('inbound_summary_mock, expected_text', [ + (mock_get_inbound_sms_summary_with_no_messages, '0 text messages received'), + (mock_get_inbound_sms_summary, '99 text messages received latest message just now'), +]) +def test_inbound_messages_shows_count_of_messages( + logged_in_client, + mocker, + mock_get_service_templates_when_no_templates_exist, + mock_get_jobs, + mock_get_detailed_service, + mock_get_template_statistics, + mock_get_usage, + inbound_summary_mock, + expected_text, +): + + inbound_summary_mock(mocker) + + response = logged_in_client.get(url_for('main.service_dashboard', service_id=SERVICE_ONE_ID)) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + assert response.status_code == 200 + assert normalize_spaces(page.select('.big-number-meta-wrapper')[0].text) == expected_text + assert page.select('.big-number-meta-wrapper a')[0]['href'] == url_for( + 'main.inbox', service_id=SERVICE_ONE_ID + ) + + +@pytest.mark.parametrize('index, expected_row', enumerate([ + '07900900000 foo 1 hour ago', + '07900900001 foo 2 hours ago', + '07900900002 foo 3 hours ago', + '07900900003 foo 4 hours ago', + '07900900004 foo 5 hours ago', +])) +def test_inbox_showing_inbound_messages( + logged_in_client, + mocker, + mock_get_service_templates_when_no_templates_exist, + mock_get_jobs, + mock_get_detailed_service, + mock_get_template_statistics, + mock_get_usage, + mock_get_inbound_sms, + index, + expected_row, +): + + response = logged_in_client.get(url_for('main.inbox', service_id=SERVICE_ONE_ID)) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + assert response.status_code == 200 + rows = page.select('tbody tr') + assert len(rows) == 5 + assert normalize_spaces(rows[index].text) == expected_row + + +def test_empty_inbox( + logged_in_client, + mocker, + mock_get_service_templates_when_no_templates_exist, + mock_get_jobs, + mock_get_detailed_service, + mock_get_template_statistics, + mock_get_usage, + mock_get_inbound_sms_with_no_messages, +): + + response = logged_in_client.get(url_for('main.inbox', service_id=SERVICE_ONE_ID)) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + assert response.status_code == 200 + assert normalize_spaces(page.select('tbody tr')) == ( + 'When users text your service’s phone number (GOVUK) you’ll see the messages here' + ) + + def test_should_show_recent_templates_on_dashboard( logged_in_client, mocker, @@ -79,6 +163,7 @@ def test_should_show_recent_templates_on_dashboard( mock_get_jobs, mock_get_detailed_service, mock_get_usage, + mock_get_inbound_sms_summary, ): mock_template_stats = mocker.patch('app.template_statistics_client.get_template_statistics_for_service', return_value=copy.deepcopy(stub_template_stats)) @@ -144,6 +229,7 @@ def test_should_show_upcoming_jobs_on_dashboard( mock_get_detailed_service, mock_get_jobs, mock_get_usage, + mock_get_inbound_sms_summary, ): response = logged_in_client.get(url_for('main.service_dashboard', service_id=SERVICE_ONE_ID)) @@ -174,6 +260,7 @@ def test_should_show_recent_jobs_on_dashboard( mock_get_detailed_service, mock_get_jobs, mock_get_usage, + mock_get_inbound_sms_summary, ): response = logged_in_client.get(url_for('main.service_dashboard', service_id=SERVICE_ONE_ID)) @@ -292,6 +379,7 @@ def test_menu_send_messages( mock_get_template_statistics, mock_get_detailed_service, mock_get_usage, + mock_get_inbound_sms_summary, ): with app_.test_request_context(): resp = _test_dashboard_menu( @@ -322,6 +410,7 @@ def test_menu_manage_service( mock_get_template_statistics, mock_get_detailed_service, mock_get_usage, + mock_get_inbound_sms_summary, ): with app_.test_request_context(): resp = _test_dashboard_menu( @@ -351,6 +440,7 @@ def test_menu_manage_api_keys( mock_get_template_statistics, mock_get_detailed_service, mock_get_usage, + mock_get_inbound_sms_summary, ): with app_.test_request_context(): resp = _test_dashboard_menu( @@ -380,6 +470,7 @@ def test_menu_all_services_for_platform_admin_user( mock_get_template_statistics, mock_get_detailed_service, mock_get_usage, + mock_get_inbound_sms_summary, ): with app_.test_request_context(): resp = _test_dashboard_menu( @@ -409,6 +500,7 @@ def test_route_for_service_permissions( mock_get_template_statistics, mock_get_detailed_service, mock_get_usage, + mock_get_inbound_sms_summary, ): with app_.test_request_context(): validate_route_permission( @@ -445,6 +537,7 @@ def test_service_dashboard_updates_gets_dashboard_totals( mock_get_detailed_service, mock_get_jobs, mock_get_usage, + mock_get_inbound_sms_summary, ): mocker.patch('app.main.views.dashboard.get_dashboard_totals', return_value={ 'email': {'requested': 123, 'delivered': 0, 'failed': 0}, @@ -668,6 +761,7 @@ def test_should_show_all_jobs_with_valid_statuses( mock_get_detailed_service, mock_get_jobs, mock_get_usage, + mock_get_inbound_sms_summary, ): get_dashboard_partials(service_id=SERVICE_ONE_ID) diff --git a/tests/app/main/views/test_sign_out.py b/tests/app/main/views/test_sign_out.py index d4dc88b46..3b550f377 100644 --- a/tests/app/main/views/test_sign_out.py +++ b/tests/app/main/views/test_sign_out.py @@ -24,6 +24,7 @@ def test_sign_out_user( mock_get_template_statistics, mock_get_detailed_service, mock_get_usage, + mock_get_inbound_sms_summary, ): with logged_in_client.session_transaction() as session: assert session.get('user_id') is not None diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index 0c0da896e..9bfe8828b 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -156,5 +156,7 @@ def test_generate_notifications_csv_calls_twice_if_next_link(mocker): assert mock_get_notifications.mock_calls[1][2]['page'] == 2 -def normalize_spaces(string): - return ' '.join(string.split()) +def normalize_spaces(input): + if isinstance(input, str): + return ' '.join(input.split()) + return normalize_spaces(' '.join(item.text for item in input)) diff --git a/tests/conftest.py b/tests/conftest.py index a59ce5382..0201f99bc 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1132,6 +1132,68 @@ def mock_get_notifications_with_no_notifications(mocker): ) +@pytest.fixture(scope='function') +def mock_get_inbound_sms(mocker): + def _get_inbound_sms( + service_id, + ): + return [{ + 'user_number': '0790090000' + str(i), + 'content': 'foo', + 'created_at': (datetime.utcnow() - timedelta(minutes=60 * (i + 1))).isoformat() + } for i in range(5)] + + return mocker.patch( + 'app.service_api_client.get_inbound_sms', + side_effect=_get_inbound_sms, + ) + + +@pytest.fixture(scope='function') +def mock_get_inbound_sms_with_no_messages(mocker): + def _get_inbound_sms( + service_id, + ): + return [] + + return mocker.patch( + 'app.service_api_client.get_inbound_sms', + side_effect=_get_inbound_sms, + ) + + +@pytest.fixture(scope='function') +def mock_get_inbound_sms_summary(mocker): + def _get_inbound_sms_summary( + service_id, + ): + return { + 'count': 99, + 'latest_message': datetime.utcnow().isoformat() + } + + return mocker.patch( + 'app.service_api_client.get_inbound_sms_summary', + side_effect=_get_inbound_sms_summary, + ) + + +@pytest.fixture(scope='function') +def mock_get_inbound_sms_summary_with_no_messages(mocker): + def _get_inbound_sms_summary( + service_id, + ): + return { + 'count': 0, + 'latest_message': None + } + + return mocker.patch( + 'app.service_api_client.get_inbound_sms_summary', + side_effect=_get_inbound_sms_summary, + ) + + @pytest.fixture(scope='function') def mock_has_permissions(mocker): def _has_permission(permissions=None, any_=False, admin_override=False):