From 313f669690bd0be16872ade7817f5f4e9eb6f97b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 6 Jun 2017 12:57:55 +0100 Subject: [PATCH 1/4] Roll up messages in inbox MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The inbox should work a bit like the one on your phone. You shouldn’t see all the messages, but the latest one from each of your ‘contacts’ only. --- app/__init__.py | 7 ++++++- app/main/views/dashboard.py | 16 +++++++++++++++- app/templates/views/dashboard/inbox.html | 7 ++++--- requirements.txt | 2 +- tests/app/main/views/test_dashboard.py | 13 ++++++++----- tests/conftest.py | 4 ++-- 6 files changed, 36 insertions(+), 13 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 795987bd5..ef10702ba 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -29,7 +29,11 @@ from functools import partial from notifications_python_client.errors import HTTPError from notifications_utils import logging, request_id, formatters from notifications_utils.clients.statsd.statsd_client import StatsdClient -from notifications_utils.recipients import validate_phone_number, InvalidPhoneError +from notifications_utils.recipients import ( + validate_phone_number, + InvalidPhoneError, + format_phone_number_human_readable, +) from notifications_utils.formatters import formatted_list from pygments import highlight from pygments.formatters.html import HtmlFormatter @@ -139,6 +143,7 @@ def create_app(): application.add_template_filter(format_notification_status_as_url) application.add_template_filter(formatted_list) application.add_template_filter(nl2br) + application.add_template_filter(format_phone_number_human_readable) application.after_request(useful_headers_after_request) application.after_request(save_service_after_request) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index b78fadc13..02292b400 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -10,6 +10,8 @@ from flask import ( ) from flask_login import login_required +from notifications_utils.recipients import format_phone_number_human_readable + from app.main import main from app import ( current_service, @@ -144,9 +146,21 @@ def inbox(service_id): if 'inbound_sms' not in current_service['permissions']: abort(403) + messages_to_show = list() + inbound_messages = service_api_client.get_inbound_sms(service_id) + + for message in inbound_messages: + if format_phone_number_human_readable(message['user_number']) not in { + format_phone_number_human_readable(message['user_number']) + for message in messages_to_show + }: + messages_to_show.append(message) + return render_template( 'views/dashboard/inbox.html', - messages=service_api_client.get_inbound_sms(service_id), + messages=messages_to_show, + count_of_messages=len(inbound_messages), + count_of_users=len(messages_to_show), ) diff --git a/app/templates/views/dashboard/inbox.html b/app/templates/views/dashboard/inbox.html index ace1cafbd..6d5612bb4 100644 --- a/app/templates/views/dashboard/inbox.html +++ b/app/templates/views/dashboard/inbox.html @@ -4,7 +4,7 @@ {% extends "withnav_template.html" %} {% block service_page_title %} - Inbox + Received text messages {% endblock %} {% block maincolumn_content %} @@ -25,7 +25,7 @@ field_headings_visible=False ) %} {% call field() %} - {{ item.user_number }} + {{ item.user_number | format_phone_number_human_readable }} {{ item.content }} {% endcall %} {% call field(align='right') %} @@ -36,7 +36,8 @@ {% endcall %} {% if messages %}

- Showing all {{ messages | length }} messages + {{ count_of_messages }} message{{ '' if 1 == count_of_messages else 's' }} + from {{ count_of_users }} user{{ '' if 1 == count_of_users else 's' }}

{% endif %} diff --git a/requirements.txt b/requirements.txt index 98ed8f5bf..03158b9fd 100644 --- a/requirements.txt +++ b/requirements.txt @@ -28,4 +28,4 @@ notifications-python-client>=3.1,<3.2 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@17.1.2#egg=notifications-utils==17.1.2 +git+https://github.com/alphagov/notifications-utils.git@17.2.0#egg=notifications-utils==17.2.0 diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index 152d0a9d0..b05299f01 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -131,11 +131,11 @@ def test_inbound_messages_shows_count_of_messages( @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', + '07900 900000 message-1 1 hour ago', + '07900 900002 message-4 3 hours ago', + '07900 900004 message-5 5 hours ago', + '07900 900006 message-6 7 hours ago', + '07900 900008 message-7 9 hours ago', ])) def test_inbox_showing_inbound_messages( logged_in_client, @@ -159,6 +159,9 @@ def test_inbox_showing_inbound_messages( rows = page.select('tbody tr') assert len(rows) == 5 assert normalize_spaces(rows[index].text) == expected_row + assert normalize_spaces(page.select('.table-show-more-link')) == ( + '8 messages from 5 users' + ) def test_empty_inbox( diff --git a/tests/conftest.py b/tests/conftest.py index 958aac94d..71d6e8588 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1139,9 +1139,9 @@ def mock_get_inbound_sms(mocker): ): return [{ 'user_number': '0790090000' + str(i), - 'content': 'foo', + 'content': 'message-{}'.format(index + 1), 'created_at': (datetime.utcnow() - timedelta(minutes=60 * (i + 1))).isoformat() - } for i in range(5)] + } for index, i in enumerate([0, 0, 0, 2, 4, 6, 8, 8])] return mocker.patch( 'app.service_api_client.get_inbound_sms', From 4259fb02bae9a4c35dff5a9d49b7ad2527ffefcc Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 7 Jun 2017 12:03:47 +0100 Subject: [PATCH 2/4] Add handler for old feedback form MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cool URIs don’t change 😎 https://www.w3.org/Provider/Style/URI We still have links to `/feedback` in our emails. These will live in people’s inboxes forever. --- app/main/views/feedback.py | 5 +++++ tests/app/main/views/test_feedback.py | 15 ++++++++++++--- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/app/main/views/feedback.py b/app/main/views/feedback.py index 775d79878..6f1631a2f 100644 --- a/app/main/views/feedback.py +++ b/app/main/views/feedback.py @@ -8,6 +8,11 @@ from app.main.forms import SupportType, Feedback, Problem, Triage from datetime import datetime +@main.route('/feedback', methods=['GET']) +def old_feedback(): + return redirect(url_for('.support')) + + @main.route('/support', methods=['GET', 'POST']) def support(): form = SupportType() diff --git a/tests/app/main/views/test_feedback.py b/tests/app/main/views/test_feedback.py index 6b4bfa0f2..97e1ae0d7 100644 --- a/tests/app/main/views/test_feedback.py +++ b/tests/app/main/views/test_feedback.py @@ -17,9 +17,18 @@ def no_redirect(): return lambda _external=True: None -def test_get_support_index_page(client): - resp = client.get(url_for('main.support')) - assert resp.status_code == 200 +@pytest.mark.parametrize('endpoint', [ + 'main.old_feedback', + 'main.support', +]) +def test_get_support_index_page( + client, + endpoint, +): + response = client.get(url_for('main.support'), follow_redirects=True) + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.h1.string.strip() == 'Support' @freeze_time('2016-12-12 12:00:00.000000') From d2ecd2121e6bfd0f861839ad3308de2768299dce Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 7 Jun 2017 14:26:00 +0100 Subject: [PATCH 3/4] Wired in the free limit from the API --- app/main/views/dashboard.py | 7 +++---- tests/conftest.py | 2 ++ 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 9091a2036..45736ac1e 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -190,7 +190,7 @@ def get_dashboard_partials(service_id): **calculate_free_tier_usage(service_api_client.get_yearly_sms_unit_count_and_cost( service_id, get_current_financial_year(), - )) + ), service) ), } @@ -202,9 +202,8 @@ def get_dashboard_totals(statistics): return statistics -def calculate_free_tier_usage(usage): - sms_free_allowance = current_app.config['SMS_FREE_TIER_AMOUNT'] - +def calculate_free_tier_usage(usage, service): + sms_free_allowance = service['data']['free_sms_fragment_limit'] return({ 'sms_chargeable': max(0, usage['billable_sms_units'] - sms_free_allowance), 'total_sms_bill': usage['billable_sms_units'], diff --git a/tests/conftest.py b/tests/conftest.py index 29a1969f8..2aa63a34b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -92,6 +92,7 @@ def mock_get_detailed_service(mocker, api_user_active): return { 'data': { 'id': service_id, + 'free_sms_fragment_limit': 250000, 'statistics': { 'email': {'requested': 0, 'delivered': 0, 'failed': 0}, 'sms': {'requested': 0, 'delivered': 0, 'failed': 0} @@ -109,6 +110,7 @@ def mock_get_detailed_service_for_today(mocker, api_user_active): return { 'data': { 'id': service_id, + 'free_sms_fragment_limit': 250000, 'statistics': { 'email': {'requested': 0, 'delivered': 0, 'failed': 0}, 'sms': {'requested': 0, 'delivered': 0, 'failed': 0} From 834c255195865468bf7c916d18c0b38aa47564a7 Mon Sep 17 00:00:00 2001 From: minglis Date: Thu, 8 Jun 2017 13:55:13 +0100 Subject: [PATCH 4/4] Fixed typos --- tests/app/main/views/test_dashboard.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index 587a1fe46..be6aaead8 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -832,7 +832,7 @@ def test_should_show_all_jobs_with_valid_statuses( }) -def test_should_show_remaining_fee_tier_count( +def test_should_show_remaining_free_tier_count( logged_in_client, mock_get_service_templates, mock_get_template_statistics, @@ -853,7 +853,7 @@ def test_should_show_remaining_fee_tier_count( assert 'free text messages left' in response.get_data(as_text=True) -def test_should_show_cost_if_exceeded_fee_tier_count( +def test_should_show_cost_if_exceeded_free_tier_count( logged_in_client, mock_get_service_templates, mock_get_template_statistics,