From 4d89d368478d913adfb2800da1feb0e5f788d194 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 27 Feb 2020 15:20:48 +0000 Subject: [PATCH 1/6] Use big numbers for organisation usage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is the pattern we use to display counts of things on the dashboard and usage pages. It does some nice stuff like dealing with comma-separating and formatting monetary amounts. This commit also adds some logic to show the free allowance used if the service hasn’t spent anything on text messages yet. --- .../organisations/organisation/index.html | 35 +++++++++++++++++-- .../views/organisations/test_organisation.py | 4 +-- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/app/templates/views/organisations/organisation/index.html b/app/templates/views/organisations/organisation/index.html index c26046906..45ca65a66 100644 --- a/app/templates/views/organisations/organisation/index.html +++ b/app/templates/views/organisations/organisation/index.html @@ -1,3 +1,4 @@ +{% from "components/big-number.html" import big_number %} {% extends "org_template.html" %} {% block org_page_title %} @@ -16,9 +17,37 @@ {{ service.service_name }}
-
{{ service.emails_sent|format_thousands }} emails sent
-
{{ "£{:,.2f}".format(service.sms_cost) }} spent on text messages
-
{{ "£{:,.2f}".format(service.letter_cost) }} spent on letters
+
+ {{ big_number( + service.emails_sent, + label='emails sent', + smallest=True + ) }} +
+
+ {% if service.sms_cost %} + {{ big_number( + service.sms_cost, + 'spent on text messages', + currency="£", + smallest=True + ) }} + {% else %} + {{ big_number( + service.free_sms_limit - service.sms_remainder, + 'free text messages sent', + smallest=True + ) }} + {% endif %} +
+
+ {{ big_number( + service.letter_cost, + 'spent on letters', + currency="£", + smallest=True + ) }} +
{% endfor %} diff --git a/tests/app/main/views/organisations/test_organisation.py b/tests/app/main/views/organisations/test_organisation.py index e2a48d01a..9557ceb94 100644 --- a/tests/app/main/views/organisations/test_organisation.py +++ b/tests/app/main/views/organisations/test_organisation.py @@ -403,7 +403,7 @@ def test_organisation_services_shows_live_services_and_usage( 'free_sms_limit': 250000, 'letter_cost': 30.50, 'sms_billable_units': 122, 'sms_cost': 1.93, 'sms_remainder': None}, {'service_id': SERVICE_TWO_ID, 'service_name': '5', 'chargeable_billable_sms': 0, 'emails_sent': 20000, - 'free_sms_limit': 250000, 'letter_cost': 0, 'sms_billable_units': 2500, 'sms_cost': 0.0, + 'free_sms_limit': 250000, 'letter_cost': 0, 'sms_billable_units': 2500, 'sms_cost': 42.0, 'sms_remainder': None} ]} ) @@ -424,7 +424,7 @@ def test_organisation_services_shows_live_services_and_usage( assert normalize_spaces(usage_rows[2].text) == "£30.50 spent on letters" assert services[1].find('a')['href'] == url_for('main.usage', service_id=SERVICE_TWO_ID) assert normalize_spaces(usage_rows[3].text) == "20,000 emails sent" - assert normalize_spaces(usage_rows[4].text) == "£0.00 spent on text messages" + assert normalize_spaces(usage_rows[4].text) == "£42.00 spent on text messages" assert normalize_spaces(usage_rows[5].text) == "£0.00 spent on letters" From bd9e127e57a7700bcca5fcbecd072db4fdcb0007 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 27 Feb 2020 15:21:08 +0000 Subject: [PATCH 2/6] Sum up usage for an whole organisation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We invoice on a per organisation basis, so it’s useful to know the per organisation figures without needing to do any spreadsheet-fu. --- app/main/views/organisations.py | 8 ++++-- .../organisations/organisation/index.html | 28 ++++++++++++++++++- .../views/organisations/test_organisation.py | 22 +++++++++------ tests/app/test_navigation.py | 2 +- 4 files changed, 48 insertions(+), 12 deletions(-) diff --git a/app/main/views/organisations.py b/app/main/views/organisations.py index 05b4fec6d..43959dabd 100644 --- a/app/main/views/organisations.py +++ b/app/main/views/organisations.py @@ -125,10 +125,14 @@ def add_organisation_from_nhs_local_service(service_id): @main.route("/organisations/", methods=['GET']) @user_has_permissions() def organisation_dashboard(org_id): - services = current_organisation.services_and_usage() + services = current_organisation.services_and_usage()['services'] return render_template( 'views/organisations/organisation/index.html', - services=services + services=services, + **{ + f'total_{key}': sum(service[key] for service in services) + for key in ('emails_sent', 'sms_cost', 'letter_cost') + } ) diff --git a/app/templates/views/organisations/organisation/index.html b/app/templates/views/organisations/organisation/index.html index 45ca65a66..31c70be72 100644 --- a/app/templates/views/organisations/organisation/index.html +++ b/app/templates/views/organisations/organisation/index.html @@ -11,8 +11,34 @@ Usage +
+
+ {{ big_number( + total_emails_sent, + label='emails sent', + smaller=True + ) }} +
+
+ {{ big_number( + total_sms_cost, + 'spent on text messages', + currency="£", + smaller=True + ) }} +
+
+ {{ big_number( + total_letter_cost, + 'spent on letters', + currency="£", + smaller=True + ) }} +
+
+
    - {% for service in services['services'] %} + {% for service in services %}
  • {{ service.service_name }}
    • diff --git a/tests/app/main/views/organisations/test_organisation.py b/tests/app/main/views/organisations/test_organisation.py index 9557ceb94..0e2234ab2 100644 --- a/tests/app/main/views/organisations/test_organisation.py +++ b/tests/app/main/views/organisations/test_organisation.py @@ -64,7 +64,7 @@ def test_view_organisation_shows_the_correct_organisation( 'app.organisations_client.get_organisation', return_value=org ) mocker.patch( - 'app.organisations_client.get_services_and_usage', return_value=[] + 'app.organisations_client.get_services_and_usage', return_value={'services': {}} ) page = client_request.get( @@ -413,19 +413,25 @@ def test_organisation_services_shows_live_services_and_usage( mock.assert_called_once_with(ORGANISATION_ID, 2019) services = page.select('.browse-list-item') + usage_rows = page.find_all("div", class_="column-one-third") assert len(services) == 2 + # Totals + assert normalize_spaces(usage_rows[0].text) == "33,000 emails sent" + assert normalize_spaces(usage_rows[1].text) == "£43.93 spent on text messages" + assert normalize_spaces(usage_rows[2].text) == "£30.50 spent on letters" + assert normalize_spaces(services[0].text) == '1' assert normalize_spaces(services[1].text) == '5' assert services[0].find('a')['href'] == url_for('main.usage', service_id=SERVICE_ONE_ID) - usage_rows = page.find_all("div", class_="column-one-third") - assert normalize_spaces(usage_rows[0].text) == "13,000 emails sent" - assert normalize_spaces(usage_rows[1].text) == "£1.93 spent on text messages" - assert normalize_spaces(usage_rows[2].text) == "£30.50 spent on letters" + + assert normalize_spaces(usage_rows[3].text) == "13,000 emails sent" + assert normalize_spaces(usage_rows[4].text) == "£1.93 spent on text messages" + assert normalize_spaces(usage_rows[5].text) == "£30.50 spent on letters" assert services[1].find('a')['href'] == url_for('main.usage', service_id=SERVICE_TWO_ID) - assert normalize_spaces(usage_rows[3].text) == "20,000 emails sent" - assert normalize_spaces(usage_rows[4].text) == "£42.00 spent on text messages" - assert normalize_spaces(usage_rows[5].text) == "£0.00 spent on letters" + assert normalize_spaces(usage_rows[6].text) == "20,000 emails sent" + assert normalize_spaces(usage_rows[7].text) == "£42.00 spent on text messages" + assert normalize_spaces(usage_rows[8].text) == "£0.00 spent on letters" def test_organisation_trial_mode_services_shows_all_non_live_services( diff --git a/tests/app/test_navigation.py b/tests/app/test_navigation.py index 1a5582bcd..c32912ccc 100644 --- a/tests/app/test_navigation.py +++ b/tests/app/test_navigation.py @@ -150,7 +150,7 @@ def test_a_page_should_nave_selected_org_navigation_item( mocker ): mocker.patch( - 'app.organisations_client.get_services_and_usage', return_value=[] + 'app.organisations_client.get_services_and_usage', return_value={'services': {}} ) page = client_request.get(endpoint, org_id=ORGANISATION_ID) selected_nav_items = page.select('.navigation a.selected') From df5b2f00efc530b6389dc6c89a401c228543d504 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 27 Feb 2020 15:29:51 +0000 Subject: [PATCH 3/6] Add some spacing and keylines Just to make the page easier to scan. --- .../organisations/organisation/index.html | 65 ++++++++++--------- 1 file changed, 34 insertions(+), 31 deletions(-) diff --git a/app/templates/views/organisations/organisation/index.html b/app/templates/views/organisations/organisation/index.html index 31c70be72..501a80140 100644 --- a/app/templates/views/organisations/organisation/index.html +++ b/app/templates/views/organisations/organisation/index.html @@ -39,43 +39,46 @@
      {% for service in services %} -
    • - {{ service.service_name }} -
      • -
      -
      - {{ big_number( - service.emails_sent, - label='emails sent', - smallest=True - ) }} -
      -
      - {% if service.sms_cost %} +
      +
    • + {{ service.service_name }} +
      • +
      +
      {{ big_number( - service.sms_cost, - 'spent on text messages', + service.emails_sent, + label='emails sent', + smallest=True + ) }} +
      +
      + {% if service.sms_cost %} + {{ big_number( + service.sms_cost, + 'spent on text messages', + currency="£", + smallest=True + ) }} + {% else %} + {{ big_number( + service.free_sms_limit - service.sms_remainder, + 'free text messages sent', + smallest=True + ) }} + {% endif %} +
      +
      + {{ big_number( + service.letter_cost, + 'spent on letters', currency="£", smallest=True ) }} - {% else %} - {{ big_number( - service.free_sms_limit - service.sms_remainder, - 'free text messages sent', - smallest=True - ) }} - {% endif %} -
      -
      - {{ big_number( - service.letter_cost, - 'spent on letters', - currency="£", - smallest=True - ) }} +
      - {% endfor %} + {% endfor %} +
    {% endblock %} From 408fcf05eb6c6ec85db9f243a1aab32e835a6c60 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 28 Feb 2020 12:32:47 +0000 Subject: [PATCH 4/6] Add financial year filter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Otherwise this page will become less useful come April 1st… --- app/main/views/organisations.py | 16 ++++- app/models/organisation.py | 5 +- .../organisations/organisation/index.html | 58 +++++++++++-------- .../views/organisations/test_organisation.py | 37 ++++++++++++ 4 files changed, 88 insertions(+), 28 deletions(-) diff --git a/app/main/views/organisations.py b/app/main/views/organisations.py index 43959dabd..fd480b9e8 100644 --- a/app/main/views/organisations.py +++ b/app/main/views/organisations.py @@ -1,4 +1,5 @@ from collections import OrderedDict +from functools import partial from flask import flash, redirect, render_template, request, session, url_for from flask_login import current_user @@ -33,6 +34,10 @@ from app.main.forms import ( SetEmailBranding, SetLetterBranding, ) +from app.main.views.dashboard import ( + get_tuples_of_financial_years, + requested_and_current_financial_year, +) from app.main.views.service_settings import get_branding_as_value_and_label from app.models.organisation import Organisation, Organisations from app.models.user import InvitedOrgUser, User @@ -125,10 +130,19 @@ def add_organisation_from_nhs_local_service(service_id): @main.route("/organisations/", methods=['GET']) @user_has_permissions() def organisation_dashboard(org_id): - services = current_organisation.services_and_usage()['services'] + year, current_financial_year = requested_and_current_financial_year(request) + services = current_organisation.services_and_usage( + financial_year=year + )['services'] return render_template( 'views/organisations/organisation/index.html', services=services, + years=get_tuples_of_financial_years( + partial(url_for, '.organisation_dashboard', org_id=current_organisation.id), + start=current_financial_year - 1, + end=current_financial_year + 1, + ), + selected_year=year, **{ f'total_{key}': sum(service[key] for service in services) for key in ('emails_sent', 'sms_cost', 'letter_cost') diff --git a/app/models/organisation.py b/app/models/organisation.py index 07ceccb45..8d1390250 100644 --- a/app/models/organisation.py +++ b/app/models/organisation.py @@ -5,7 +5,6 @@ from app.models import JSONModel, ModelList from app.notify_client.email_branding_client import email_branding_client from app.notify_client.letter_branding_client import letter_branding_client from app.notify_client.organisations_api_client import organisations_client -from app.utils import get_current_financial_year class Organisation(JSONModel): @@ -199,8 +198,8 @@ class Organisation(JSONModel): self.id ) - def services_and_usage(self): - return organisations_client.get_services_and_usage(self.id, get_current_financial_year()) + def services_and_usage(self, financial_year): + return organisations_client.get_services_and_usage(self.id, financial_year) class Organisations(ModelList): diff --git a/app/templates/views/organisations/organisation/index.html b/app/templates/views/organisations/organisation/index.html index 501a80140..039ee9270 100644 --- a/app/templates/views/organisations/organisation/index.html +++ b/app/templates/views/organisations/organisation/index.html @@ -1,4 +1,5 @@ {% from "components/big-number.html" import big_number %} +{% from "components/pill.html" import pill %} {% extends "org_template.html" %} {% block org_page_title %} @@ -11,32 +12,41 @@ Usage -
    -
    - {{ big_number( - total_emails_sent, - label='emails sent', - smaller=True - ) }} -
    -
    - {{ big_number( - total_sms_cost, - 'spent on text messages', - currency="£", - smaller=True - ) }} -
    -
    - {{ big_number( - total_letter_cost, - 'spent on letters', - currency="£", - smaller=True - ) }} -
    +
    + {{ pill(years, selected_year, big_number_args={'smallest': True}) }}
    +
    +
    +
    + {{ big_number( + total_emails_sent, + label='emails sent', + smaller=True + ) }} +
    +
    +
    +
    + {{ big_number( + total_sms_cost, + 'spent on text messages', + currency="£", + smaller=True + ) }} +
    +
    +
    +
    + {{ big_number( + total_letter_cost, + 'spent on letters', + currency="£", + smaller=True + ) }} +
    +
    +
      {% for service in services %}
      diff --git a/tests/app/main/views/organisations/test_organisation.py b/tests/app/main/views/organisations/test_organisation.py index 0e2234ab2..3c7c6e2cd 100644 --- a/tests/app/main/views/organisations/test_organisation.py +++ b/tests/app/main/views/organisations/test_organisation.py @@ -3,6 +3,7 @@ from unittest.mock import ANY, Mock import pytest from bs4 import BeautifulSoup from flask import url_for +from freezegun import freeze_time from notifications_python_client.errors import HTTPError from tests import organisation_json, service_json @@ -389,6 +390,7 @@ def test_nhs_local_assigns_to_selected_organisation( mock_update_service_organisation.assert_called_once_with(SERVICE_ONE_ID, ORGANISATION_ID) +@freeze_time("2020-02-20 20:20") def test_organisation_services_shows_live_services_and_usage( client_request, mock_get_organisation, @@ -434,6 +436,41 @@ def test_organisation_services_shows_live_services_and_usage( assert normalize_spaces(usage_rows[8].text) == "£0.00 spent on letters" +@freeze_time("2020-02-20 20:20") +@pytest.mark.parametrize('financial_year, expected_selected', ( + (2018, '2018 to 2019 financial year'), + (2019, '2019 to 2020 financial year'), + (2020, '2020 to 2021 financial year'), +)) +def test_organisation_services_filters_by_financial_year( + client_request, + mock_get_organisation, + mocker, + active_user_with_permissions, + fake_uuid, + financial_year, + expected_selected, +): + mock = mocker.patch( + 'app.organisations_client.get_services_and_usage', + return_value={"services": []} + ) + page = client_request.get( + '.organisation_dashboard', + org_id=ORGANISATION_ID, + year=financial_year, + ) + mock.assert_called_once_with(ORGANISATION_ID, financial_year) + assert normalize_spaces(page.select_one('.pill').text) == ( + '2018 to 2019 financial year ' + '2019 to 2020 financial year ' + '2020 to 2021 financial year' + ) + assert normalize_spaces(page.select_one('.pill-selected-item').text) == ( + expected_selected + ) + + def test_organisation_trial_mode_services_shows_all_non_live_services( client_request, platform_admin_user, From 519176f903e7ef07487594e571b7b2c84ffd20e1 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 28 Feb 2020 13:40:07 +0000 Subject: [PATCH 5/6] Use new grid classes from GOV.UK frontend --- .../views/organisations/organisation/index.html | 16 ++++++++-------- .../views/organisations/test_organisation.py | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/templates/views/organisations/organisation/index.html b/app/templates/views/organisations/organisation/index.html index 039ee9270..6e07eb456 100644 --- a/app/templates/views/organisations/organisation/index.html +++ b/app/templates/views/organisations/organisation/index.html @@ -16,8 +16,8 @@ {{ pill(years, selected_year, big_number_args={'smallest': True}) }}
      -
      -
      +
      +
      {{ big_number( total_emails_sent, @@ -26,7 +26,7 @@ ) }}
      -
      +
      {{ big_number( total_sms_cost, @@ -36,7 +36,7 @@ ) }}
      -
      +
      {{ big_number( total_letter_cost, @@ -53,15 +53,15 @@
    • {{ service.service_name }}
      • -
      -
      +
      +
      {{ big_number( service.emails_sent, label='emails sent', smallest=True ) }}
      -
      +
      {% if service.sms_cost %} {{ big_number( service.sms_cost, @@ -77,7 +77,7 @@ ) }} {% endif %}
      -
      +
      {{ big_number( service.letter_cost, 'spent on letters', diff --git a/tests/app/main/views/organisations/test_organisation.py b/tests/app/main/views/organisations/test_organisation.py index 3c7c6e2cd..9a9458f93 100644 --- a/tests/app/main/views/organisations/test_organisation.py +++ b/tests/app/main/views/organisations/test_organisation.py @@ -415,7 +415,7 @@ def test_organisation_services_shows_live_services_and_usage( mock.assert_called_once_with(ORGANISATION_ID, 2019) services = page.select('.browse-list-item') - usage_rows = page.find_all("div", class_="column-one-third") + usage_rows = page.select("main .govuk-grid-column-one-third") assert len(services) == 2 # Totals From fa6a59915cbca010cede1843f6496afc5aa8a442 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 28 Feb 2020 13:43:17 +0000 Subject: [PATCH 6/6] Make each service name a `

      ` --- .../views/organisations/organisation/index.html | 10 ++++------ .../app/main/views/organisations/test_organisation.py | 4 ++-- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/app/templates/views/organisations/organisation/index.html b/app/templates/views/organisations/organisation/index.html index 6e07eb456..186a431d7 100644 --- a/app/templates/views/organisations/organisation/index.html +++ b/app/templates/views/organisations/organisation/index.html @@ -47,12 +47,11 @@

      - + {% endfor %} +
      {% endblock %} diff --git a/tests/app/main/views/organisations/test_organisation.py b/tests/app/main/views/organisations/test_organisation.py index 9a9458f93..afb1bb526 100644 --- a/tests/app/main/views/organisations/test_organisation.py +++ b/tests/app/main/views/organisations/test_organisation.py @@ -414,8 +414,8 @@ def test_organisation_services_shows_live_services_and_usage( page = client_request.get('.organisation_dashboard', org_id=ORGANISATION_ID) mock.assert_called_once_with(ORGANISATION_ID, 2019) - services = page.select('.browse-list-item') - usage_rows = page.select("main .govuk-grid-column-one-third") + services = page.select('main h2') + usage_rows = page.select('main .govuk-grid-column-one-third') assert len(services) == 2 # Totals