From 0648f80b9d3f1f1a6fce7a8cb132f717db367c75 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 26 Apr 2022 14:34:33 +0100 Subject: [PATCH 01/10] Refactor "calculate_usage" function This will make the following changes clearer. In the next commits we'll go into more detail about "billing_units" and how it differs for SMS vs. emails and letters. --- app/main/views/dashboard.py | 39 +++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 1d143ec31..748d54c36 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -150,8 +150,7 @@ def usage(service_id): start=current_financial_year - 2, end=current_financial_year, ), - **calculate_usage(yearly_usage, - free_sms_allowance) + **get_annual_usage_breakdown(yearly_usage, free_sms_allowance) ) @@ -315,7 +314,7 @@ def get_dashboard_partials(service_id): ), 'usage': render_template( 'views/dashboard/_usage.html', - **calculate_usage(yearly_usage, free_sms_allowance), + **get_annual_usage_breakdown(yearly_usage, free_sms_allowance), ), } @@ -327,31 +326,29 @@ def get_dashboard_totals(statistics): return statistics -def calculate_usage(usage, free_sms_fragment_limit): - sms_breakdowns = [breakdown for breakdown in usage if breakdown['notification_type'] == 'sms'] - +def get_annual_usage_breakdown(usage, free_sms_fragment_limit): + sms = get_usage_breakdown_by_type(usage, 'sms') # this relies on the assumption: only one SMS rate per financial year. - sms_rate = 0 if len(sms_breakdowns) == 0 else sms_breakdowns[0].get("rate", 0) - sms_sent = get_sum_billing_units(sms_breakdowns) + sms_rate = 0 if len(sms) == 0 else sms[0].get("rate", 0) + sms_chargeable_units = sum(row['billing_units'] for row in sms) sms_free_allowance = free_sms_fragment_limit - emails = [breakdown["billing_units"] for breakdown in usage if breakdown['notification_type'] == 'email'] - emails_sent = 0 if len(emails) == 0 else emails[0] + emails = get_usage_breakdown_by_type(usage, 'email') + emails_sent = sum(row['billing_units'] for row in emails) - letters = [(breakdown["billing_units"], breakdown['letter_total']) for breakdown in usage if - breakdown['notification_type'] == 'letter'] - letter_sent = sum(row[0] for row in letters) - letter_cost = sum(row[1] for row in letters) + letters = get_usage_breakdown_by_type(usage, 'letter') + letters_sent = sum(row['billing_units'] for row in letters) + letters_cost = sum(row['letter_total'] for row in letters) return { 'emails_sent': emails_sent, 'sms_free_allowance': sms_free_allowance, - 'sms_sent': sms_sent, - 'sms_allowance_remaining': max(0, (sms_free_allowance - sms_sent)), - 'sms_chargeable': max(0, sms_sent - sms_free_allowance), + 'sms_sent': sms_chargeable_units, + 'sms_allowance_remaining': max(0, (sms_free_allowance - sms_chargeable_units)), + 'sms_chargeable': max(0, sms_chargeable_units - sms_free_allowance), 'sms_rate': sms_rate, - 'letter_sent': letter_sent, - 'letter_cost': letter_cost + 'letter_sent': letters_sent, + 'letter_cost': letters_cost } @@ -404,6 +401,10 @@ def get_sum_billing_units(billing_units, month=None): return sum(b['billing_units'] for b in billing_units) +def get_usage_breakdown_by_type(usage, notification_type): + return [row for row in usage if row['notification_type'] == notification_type] + + def get_free_paid_breakdown_for_billable_units(year, free_sms_fragment_limit, billing_units): cumulative = 0 sms_units = [x for x in billing_units if x['notification_type'] == 'sms'] From 55ea5d90c7d86b2f97b2a5962b649d99498140bb Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 26 Apr 2022 14:40:01 +0100 Subject: [PATCH 02/10] Migrate away from ambiguous "billing_units" This uses two new fields in the annual usage API instead of the old one, which was actually a mix of two kinds of data [^1]. [^1]: https://github.com/alphagov/notifications-api/pull/3520/commits/fc378fed96b90d42d2357624c6f937b2c954b58e --- app/main/views/dashboard.py | 6 ++--- tests/conftest.py | 54 ++++++++++++++++++++++++++++++------- 2 files changed, 48 insertions(+), 12 deletions(-) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 748d54c36..06b78355b 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -330,14 +330,14 @@ def get_annual_usage_breakdown(usage, free_sms_fragment_limit): sms = get_usage_breakdown_by_type(usage, 'sms') # this relies on the assumption: only one SMS rate per financial year. sms_rate = 0 if len(sms) == 0 else sms[0].get("rate", 0) - sms_chargeable_units = sum(row['billing_units'] for row in sms) + sms_chargeable_units = sum(row['chargeable_units'] for row in sms) sms_free_allowance = free_sms_fragment_limit emails = get_usage_breakdown_by_type(usage, 'email') - emails_sent = sum(row['billing_units'] for row in emails) + emails_sent = sum(row['notifications_sent'] for row in emails) letters = get_usage_breakdown_by_type(usage, 'letter') - letters_sent = sum(row['billing_units'] for row in letters) + letters_sent = sum(row['notifications_sent'] for row in letters) letters_cost = sum(row['letter_total'] for row in letters) return { diff --git a/tests/conftest.py b/tests/conftest.py index f5e397870..721474770 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2316,11 +2316,41 @@ def mock_get_monthly_notification_stats(mocker, service_one, fake_uuid): def mock_get_usage(mocker, service_one, fake_uuid): def _get_usage(service_id, year=None): return [ - {"notification_type": "email", "billing_units": 1000, "rate": 0.00, "letter_total": 0}, - {"notification_type": "sms", "billing_units": 251500, "rate": 0.0165, "letter_total": 0}, - {"notification_type": "sms", "billing_units": 300, "rate": 0.0165, "letter_total": 0}, - {"notification_type": "sms", "billing_units": 300, "rate": 0.0165, "letter_total": 0}, - {"notification_type": "sms", "billing_units": 90, "rate": 0.0165, "letter_total": 0} + { + "notification_type": "email", + "chargeable_units": 1000, + "notifications_sent": 1000, + "rate": 0.00, + "letter_total": 0 + }, + { + "notification_type": "sms", + "chargeable_units": 251500, + "notifications_sent": 105000, + "rate": 0.0165, + "letter_total": 0 + }, + { + "notification_type": "sms", + "chargeable_units": 300, + "notifications_sent": 300, + "rate": 0.0165, + "letter_total": 0 + }, + { + "notification_type": "sms", + "chargeable_units": 300, + "notifications_sent": 150, + "rate": 0.0165, + "letter_total": 0 + }, + { + "notification_type": "sms", + "chargeable_units": 90, + "notifications_sent": 90, + "rate": 0.0165, + "letter_total": 0 + } ] return mocker.patch( @@ -2440,12 +2470,18 @@ def mock_get_future_usage(mocker, service_one, fake_uuid): def _get_usage(service_id, year=None): return [ { - 'notification_type': 'sms', 'billing_units': 0, - 'rate': 0.0158, 'letter_total': 0 + 'notification_type': 'sms', + 'chargeable_units': 0, + 'notifications_sent': 0, + 'rate': 0.0158, + 'letter_total': 0 }, { - 'notification_type': 'email', 'billing_units': 0, - 'rate': 0.0, 'letter_total': 0 + 'notification_type': 'email', + 'chargeable_units': 0, + 'notifications_sent': 0, + 'rate': 0.0, + 'letter_total': 0 } ] From 2aa3e78db212ecde300e944da5be1538d6080425 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 26 Apr 2022 15:19:24 +0100 Subject: [PATCH 03/10] Tidy up monthly usage test assertions The "with_letters" was mostly a duplicate of the one before - no change in test setup - bar the three assertions at the end. Having the assertions in a separate test will help keep the one above manageable as we add more assertions for the annual usage. --- tests/app/main/views/test_dashboard.py | 57 +++++++------------------- 1 file changed, 15 insertions(+), 42 deletions(-) diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index de47ffde5..4f51a50d0 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -1012,22 +1012,9 @@ def test_usage_page( assert '252,190' in cols[1].text assert 'Text messages' in cols[1].text - table = page.find('table').text.strip() - - assert '249,860 free text messages' in table - assert '40 free text messages' in table - assert '960 text messages at 1.65p' in table - assert 'April' in table - assert 'February' in table - assert 'March' in table - assert '£28.99' in table - assert '140 free text messages' in table - assert '£20.91' in table - assert '1,230 text messages at 1.70p' in table - @freeze_time("2012-03-31 12:12:12") -def test_usage_page_with_letters( +def test_usage_page_monthly_breakdown( client_request, service_one, mock_get_usage, @@ -1040,35 +1027,21 @@ def test_usage_page_with_letters( service_id=SERVICE_ONE_ID, ) - mock_get_billable_units.assert_called_once_with(SERVICE_ONE_ID, 2011) - mock_get_usage.assert_called_once_with(SERVICE_ONE_ID, 2011) - mock_get_free_sms_fragment_limit.assert_called_with(SERVICE_ONE_ID, 2011) + monthly_breakdown = normalize_spaces(page.find('table').text) - cols = page.find_all('div', {'class': 'govuk-grid-column-one-third'}) - nav = page.find('ul', {'class': 'pill'}) - unselected_nav_links = nav.select('a:not(.pill-item--selected)') - - assert normalize_spaces(nav.find('a', {'aria-current': 'page'}).text) == '2011 to 2012 financial year' - assert normalize_spaces(unselected_nav_links[0].text) == '2010 to 2011 financial year' - assert normalize_spaces(unselected_nav_links[1].text) == '2009 to 2010 financial year' - assert '252,190' in cols[1].text - assert 'Text messages' in cols[1].text - - table = page.find('table').text.strip() - - assert '249,860 free text messages' in table - assert '40 free text messages' in table - assert '960 text messages at 1.65p' in table - assert 'April' in table - assert 'February' in table - assert 'March' in table - assert '£28.99' in table - assert '140 free text messages' in table - assert '£20.91' in table - assert '1,230 text messages at 1.70p' in table - assert '10 second class letters at 31p' in normalize_spaces(table) - assert '5 first class letters at 33p' in normalize_spaces(table) - assert '10 international letters at 84p' in normalize_spaces(table) + assert '249,860 free text messages' in monthly_breakdown + assert '40 free text messages' in monthly_breakdown + assert '960 text messages at 1.65p' in monthly_breakdown + assert 'April' in monthly_breakdown + assert 'February' in monthly_breakdown + assert 'March' in monthly_breakdown + assert '£28.99' in monthly_breakdown + assert '140 free text messages' in monthly_breakdown + assert '£20.91' in monthly_breakdown + assert '1,230 text messages at 1.70p' in monthly_breakdown + assert '10 second class letters at 31p' in monthly_breakdown + assert '5 first class letters at 33p' in monthly_breakdown + assert '10 international letters at 84p' in monthly_breakdown @freeze_time("2012-04-30 12:12:12") From c6f5467009d8246fd8e67fd9d4781132a64b2eaf Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 26 Apr 2022 15:26:08 +0100 Subject: [PATCH 04/10] Extend main test for service usage page This adds missing assertions for email and SMS usage, as well as letters with the help of some additional test data. Previously we were only checking monthly usage (in other tests). --- tests/app/main/views/test_dashboard.py | 28 ++++++++++++++++++++------ tests/conftest.py | 9 ++++++++- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index 4f51a50d0..877dc3b3a 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -1002,15 +1002,31 @@ def test_usage_page( mock_get_usage.assert_called_once_with(SERVICE_ONE_ID, 2011) mock_get_free_sms_fragment_limit.assert_called_with(SERVICE_ONE_ID, 2011) - cols = page.find_all('div', {'class': 'govuk-grid-column-one-third'}) nav = page.find('ul', {'class': 'pill'}) unselected_nav_links = nav.select('a:not(.pill-item--selected)') - assert normalize_spaces(nav.find('a', {'aria-current': 'page'}).text) == '2011 to 2012 financial year' assert normalize_spaces(unselected_nav_links[0].text) == '2010 to 2011 financial year' assert normalize_spaces(unselected_nav_links[1].text) == '2009 to 2010 financial year' - assert '252,190' in cols[1].text - assert 'Text messages' in cols[1].text + + annual_usage = page.find_all('div', {'class': 'govuk-grid-column-one-third'}) + + # annual stats are shown in two rows, each with three column; email is col 1 + email_column = normalize_spaces(annual_usage[0].text + annual_usage[3].text) + assert 'Emails' in email_column + assert '1,000 sent' in email_column + + sms_column = normalize_spaces(annual_usage[1].text + annual_usage[4].text) + assert 'Text messages' in sms_column + assert '252,190 sent' in sms_column + assert '250,000 free allowance' in sms_column + assert '0 free allowance remaining' in sms_column + assert '£36.14 spent' in sms_column + assert '2,190 at 1.65 pence' in sms_column + + letter_column = normalize_spaces(annual_usage[2].text + annual_usage[5].text) + assert 'Letters' in letter_column + assert '100 sent' in letter_column + assert '£30.00 spent' in letter_column @freeze_time("2012-03-31 12:12:12") @@ -1834,7 +1850,7 @@ def test_breadcrumb_shows_if_service_is_suspended( ['email', 'sms'], ['email', 'sms', 'letter'], )) -def test_should_show_usage_on_dashboard( +def test_service_dashboard_shows_usage( client_request, service_one, mock_get_service_templates, @@ -1855,6 +1871,6 @@ def test_should_show_usage_on_dashboard( 'free email allowance ' '£36.14 ' 'spent on text messages ' - '£0.00 ' + '£30.00 ' 'spent on letters' ) diff --git a/tests/conftest.py b/tests/conftest.py index 721474770..9d4dc3a61 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2350,7 +2350,14 @@ def mock_get_usage(mocker, service_one, fake_uuid): "notifications_sent": 90, "rate": 0.0165, "letter_total": 0 - } + }, + { + "notification_type": "letter", + "chargeable_units": 300, + "notifications_sent": 100, + "rate": 0.1, + "letter_total": 30 + }, ] return mocker.patch( From 4925264fb71b0284863f7cb8d81097934ca12b0b Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 26 Apr 2022 15:23:01 +0100 Subject: [PATCH 05/10] Remove duplicate test for usage page This is covered by the main test for the page. --- tests/app/main/views/test_dashboard.py | 24 +++++------------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index 877dc3b3a..9760da0ba 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -1126,32 +1126,15 @@ def test_usage_page_displays_letters_split_by_month_and_postage( assert '7 international letters at £1.00' in may_row -@pytest.mark.parametrize('free_allowance, expected_sms_usage_breakdown', ( - (0, ( - 'Text messages ' - '252,190 sent ' - '0 free allowance ' - '252,190 at 1.65 pence per message' - )), - (100_000, ( - 'Text messages ' - '252,190 sent ' - '100,000 free allowance ' - '0 free allowance remaining ' - '152,190 at 1.65 pence per message' - )), -)) def test_usage_page_with_0_free_allowance( mocker, client_request, mock_get_usage, mock_get_billable_units, - free_allowance, - expected_sms_usage_breakdown, ): mocker.patch( 'app.billing_api_client.get_free_sms_fragment_limit_for_year', - return_value=free_allowance, + return_value=0, ) page = client_request.get( 'main.usage', @@ -1161,7 +1144,10 @@ def test_usage_page_with_0_free_allowance( assert normalize_spaces( page.select('main .govuk-grid-column-one-third')[1].text ) == ( - expected_sms_usage_breakdown + 'Text messages ' + '251,800 sent ' + '0 free allowance ' + '251,800 at 1.65 pence per message' ) From 82c3e8093d2a19d4f53b322b936b9875eb9b9c3b Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 27 Apr 2022 16:47:37 +0100 Subject: [PATCH 06/10] Remove redundant SMS test data for annual usage --- tests/app/main/views/test_dashboard.py | 4 ++-- tests/conftest.py | 14 -------------- 2 files changed, 2 insertions(+), 16 deletions(-) diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index 9760da0ba..9dc529a96 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -1017,11 +1017,11 @@ def test_usage_page( sms_column = normalize_spaces(annual_usage[1].text + annual_usage[4].text) assert 'Text messages' in sms_column - assert '252,190 sent' in sms_column + assert '251,800 sent' in sms_column assert '250,000 free allowance' in sms_column assert '0 free allowance remaining' in sms_column assert '£36.14 spent' in sms_column - assert '2,190 at 1.65 pence' in sms_column + assert '1,800 at 1.65 pence' in sms_column letter_column = normalize_spaces(annual_usage[2].text + annual_usage[5].text) assert 'Letters' in letter_column diff --git a/tests/conftest.py b/tests/conftest.py index 9d4dc3a61..01a5a4db6 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2337,20 +2337,6 @@ def mock_get_usage(mocker, service_one, fake_uuid): "rate": 0.0165, "letter_total": 0 }, - { - "notification_type": "sms", - "chargeable_units": 300, - "notifications_sent": 150, - "rate": 0.0165, - "letter_total": 0 - }, - { - "notification_type": "sms", - "chargeable_units": 90, - "notifications_sent": 90, - "rate": 0.0165, - "letter_total": 0 - }, { "notification_type": "letter", "chargeable_units": 300, From e0aa51c306a644bb85f11aabe01e0ebb4405b281 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 26 Apr 2022 15:40:29 +0100 Subject: [PATCH 07/10] Use new "cost" field in usage APIs The previous, manual calculation could be incorrect depending on which SMS rates the free allowance was attributed to. The new field also supersedes the old "letter_total" bolt-on so we can get cost information consistently for both types. --- app/main/views/dashboard.py | 4 +++- app/templates/views/dashboard/_usage.html | 4 ++-- app/templates/views/usage.html | 2 +- tests/app/main/views/test_dashboard.py | 4 ++-- tests/conftest.py | 12 ++++++------ 5 files changed, 14 insertions(+), 12 deletions(-) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 06b78355b..61a5c6daa 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -332,13 +332,14 @@ def get_annual_usage_breakdown(usage, free_sms_fragment_limit): sms_rate = 0 if len(sms) == 0 else sms[0].get("rate", 0) sms_chargeable_units = sum(row['chargeable_units'] for row in sms) sms_free_allowance = free_sms_fragment_limit + sms_cost = sum(row['cost'] for row in sms) emails = get_usage_breakdown_by_type(usage, 'email') emails_sent = sum(row['notifications_sent'] for row in emails) letters = get_usage_breakdown_by_type(usage, 'letter') letters_sent = sum(row['notifications_sent'] for row in letters) - letters_cost = sum(row['letter_total'] for row in letters) + letters_cost = sum(row['cost'] for row in letters) return { 'emails_sent': emails_sent, @@ -346,6 +347,7 @@ def get_annual_usage_breakdown(usage, free_sms_fragment_limit): 'sms_sent': sms_chargeable_units, 'sms_allowance_remaining': max(0, (sms_free_allowance - sms_chargeable_units)), 'sms_chargeable': max(0, sms_chargeable_units - sms_free_allowance), + 'sms_cost': sms_cost, 'sms_rate': sms_rate, 'letter_sent': letters_sent, 'letter_cost': letters_cost diff --git a/app/templates/views/dashboard/_usage.html b/app/templates/views/dashboard/_usage.html index 6cad43324..4046e6c91 100644 --- a/app/templates/views/dashboard/_usage.html +++ b/app/templates/views/dashboard/_usage.html @@ -8,9 +8,9 @@
- {% if sms_chargeable %} + {% if sms_cost %} {{ big_number( - sms_chargeable * sms_rate, + sms_cost, 'spent on text messages', currency="£", smaller=True diff --git a/app/templates/views/usage.html b/app/templates/views/usage.html index 4479c97fa..9f0557a50 100644 --- a/app/templates/views/usage.html +++ b/app/templates/views/usage.html @@ -59,7 +59,7 @@
{{ big_number( - (sms_chargeable * sms_rate), + sms_cost, 'spent', currency="£", smaller=True diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index 9dc529a96..4e2341f22 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -1020,7 +1020,7 @@ def test_usage_page( assert '251,800 sent' in sms_column assert '250,000 free allowance' in sms_column assert '0 free allowance remaining' in sms_column - assert '£36.14 spent' in sms_column + assert '£29.85 spent' in sms_column assert '1,800 at 1.65 pence' in sms_column letter_column = normalize_spaces(annual_usage[2].text + annual_usage[5].text) @@ -1855,7 +1855,7 @@ def test_service_dashboard_shows_usage( ) == ( 'Unlimited ' 'free email allowance ' - '£36.14 ' + '£29.85 ' 'spent on text messages ' '£30.00 ' 'spent on letters' diff --git a/tests/conftest.py b/tests/conftest.py index 01a5a4db6..68f7b7231 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2321,28 +2321,28 @@ def mock_get_usage(mocker, service_one, fake_uuid): "chargeable_units": 1000, "notifications_sent": 1000, "rate": 0.00, - "letter_total": 0 + "cost": 0 }, { "notification_type": "sms", "chargeable_units": 251500, "notifications_sent": 105000, "rate": 0.0165, - "letter_total": 0 + "cost": 24.75 # 250K free allowance }, { "notification_type": "sms", "chargeable_units": 300, "notifications_sent": 300, "rate": 0.0165, - "letter_total": 0 + "cost": 5.1 }, { "notification_type": "letter", "chargeable_units": 300, "notifications_sent": 100, "rate": 0.1, - "letter_total": 30 + "cost": 30 }, ] @@ -2467,14 +2467,14 @@ def mock_get_future_usage(mocker, service_one, fake_uuid): 'chargeable_units': 0, 'notifications_sent': 0, 'rate': 0.0158, - 'letter_total': 0 + 'cost': 0 }, { 'notification_type': 'email', 'chargeable_units': 0, 'notifications_sent': 0, 'rate': 0.0, - 'letter_total': 0 + 'cost': 0 } ] From 715a3c137f4c2f0c8ce8787a3174aceb5afacbe8 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 26 Apr 2022 15:44:15 +0100 Subject: [PATCH 08/10] Rename ambiguous "sms_chargeable" Jinja variable Now that we have the term "charge*able*_units" we should clarify this variable is the number we will actually charge for. --- app/main/views/dashboard.py | 2 +- app/templates/views/usage.html | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 61a5c6daa..913676a1d 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -346,7 +346,7 @@ def get_annual_usage_breakdown(usage, free_sms_fragment_limit): 'sms_free_allowance': sms_free_allowance, 'sms_sent': sms_chargeable_units, 'sms_allowance_remaining': max(0, (sms_free_allowance - sms_chargeable_units)), - 'sms_chargeable': max(0, sms_chargeable_units - sms_free_allowance), + 'sms_charged': max(0, sms_chargeable_units - sms_free_allowance), 'sms_cost': sms_cost, 'sms_rate': sms_rate, 'letter_sent': letters_sent, diff --git a/app/templates/views/usage.html b/app/templates/views/usage.html index 9f0557a50..b062edfee 100644 --- a/app/templates/views/usage.html +++ b/app/templates/views/usage.html @@ -33,9 +33,9 @@ {% if sms_free_allowance > 0 %} {{ big_number(sms_allowance_remaining, 'free allowance remaining', smaller=True) }} {% endif %} - {% if sms_chargeable %} + {% if sms_charged %} {{ big_number( - sms_chargeable, + sms_charged, 'at {:.2f} pence per message'.format(sms_rate * 100), smaller=True ) }} From e6c04ef556feba0e18cfca5090c49c3b84dc46a4 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 26 Apr 2022 17:37:34 +0100 Subject: [PATCH 09/10] Support variable rates for annual usage stats Note: I've removed the pricing assertion in the "0_free_allowance" test as it's covered elsewhere - the value of the test is really to check that we don't show the remainder if there never was any. --- app/main/views/dashboard.py | 5 +-- app/templates/views/usage.html | 16 +++++---- tests/app/main/views/test_dashboard.py | 48 +++++++++++++++++++++----- tests/conftest.py | 8 ++++- 4 files changed, 56 insertions(+), 21 deletions(-) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 913676a1d..fc6210fac 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -328,8 +328,6 @@ def get_dashboard_totals(statistics): def get_annual_usage_breakdown(usage, free_sms_fragment_limit): sms = get_usage_breakdown_by_type(usage, 'sms') - # this relies on the assumption: only one SMS rate per financial year. - sms_rate = 0 if len(sms) == 0 else sms[0].get("rate", 0) sms_chargeable_units = sum(row['chargeable_units'] for row in sms) sms_free_allowance = free_sms_fragment_limit sms_cost = sum(row['cost'] for row in sms) @@ -346,9 +344,8 @@ def get_annual_usage_breakdown(usage, free_sms_fragment_limit): 'sms_free_allowance': sms_free_allowance, 'sms_sent': sms_chargeable_units, 'sms_allowance_remaining': max(0, (sms_free_allowance - sms_chargeable_units)), - 'sms_charged': max(0, sms_chargeable_units - sms_free_allowance), 'sms_cost': sms_cost, - 'sms_rate': sms_rate, + 'sms_breakdown': sms, 'letter_sent': letters_sent, 'letter_cost': letters_cost } diff --git a/app/templates/views/usage.html b/app/templates/views/usage.html index b062edfee..a9bd086b0 100644 --- a/app/templates/views/usage.html +++ b/app/templates/views/usage.html @@ -33,13 +33,15 @@ {% if sms_free_allowance > 0 %} {{ big_number(sms_allowance_remaining, 'free allowance remaining', smaller=True) }} {% endif %} - {% if sms_charged %} - {{ big_number( - sms_charged, - 'at {:.2f} pence per message'.format(sms_rate * 100), - smaller=True - ) }} - {% endif %} + {% for row in sms_breakdown %} + {% if row.charged_units > 0 %} + {{ big_number( + row.charged_units, + 'at {:.2f} pence per message'.format(row.rate * 100), + smaller=True + ) }} + {% endif %} + {% endfor %}
diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index 4e2341f22..3e87f9c7f 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -1021,7 +1021,8 @@ def test_usage_page( assert '250,000 free allowance' in sms_column assert '0 free allowance remaining' in sms_column assert '£29.85 spent' in sms_column - assert '1,800 at 1.65 pence' in sms_column + assert '1,500 at 1.65 pence' in sms_column + assert '300 at 1.70 pence' in sms_column letter_column = normalize_spaces(annual_usage[2].text + annual_usage[5].text) assert 'Letters' in letter_column @@ -1029,6 +1030,37 @@ def test_usage_page( assert '£30.00 spent' in letter_column +@freeze_time("2012-03-31 12:12:12") +def test_usage_page_no_sms_spend( + mocker, + client_request, + mock_get_billable_units, + mock_get_free_sms_fragment_limit +): + mocker.patch('app.billing_api_client.get_service_usage', return_value=[ + { + "notification_type": "sms", + "chargeable_units": 1000, + "charged_units": 0, + "rate": 0.0165, + "cost": 0 + } + ]) + + page = client_request.get( + 'main.usage', + service_id=SERVICE_ONE_ID, + ) + + annual_usage = page.find_all('div', {'class': 'govuk-grid-column-one-third'}) + sms_column = normalize_spaces(annual_usage[1].text + annual_usage[4].text) + assert 'Text messages' in sms_column + assert '250,000 free allowance' in sms_column + assert '249,000 free allowance remaining' in sms_column + assert '£0.00 spent' in sms_column + assert 'pence per message' not in sms_column + + @freeze_time("2012-03-31 12:12:12") def test_usage_page_monthly_breakdown( client_request, @@ -1141,14 +1173,12 @@ def test_usage_page_with_0_free_allowance( service_id=SERVICE_ONE_ID, year=2020, ) - assert normalize_spaces( - page.select('main .govuk-grid-column-one-third')[1].text - ) == ( - 'Text messages ' - '251,800 sent ' - '0 free allowance ' - '251,800 at 1.65 pence per message' - ) + + annual_usage = page.select('main .govuk-grid-column-one-third') + sms_column = normalize_spaces(annual_usage[1].text) + + assert '0 free allowance' in sms_column + assert 'free allowance remaining' not in sms_column def test_usage_page_with_year_argument( diff --git a/tests/conftest.py b/tests/conftest.py index 68f7b7231..fbd2470b8 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2320,6 +2320,7 @@ def mock_get_usage(mocker, service_one, fake_uuid): "notification_type": "email", "chargeable_units": 1000, "notifications_sent": 1000, + "charged_units": 1000, "rate": 0.00, "cost": 0 }, @@ -2327,6 +2328,7 @@ def mock_get_usage(mocker, service_one, fake_uuid): "notification_type": "sms", "chargeable_units": 251500, "notifications_sent": 105000, + "charged_units": 1500, "rate": 0.0165, "cost": 24.75 # 250K free allowance }, @@ -2334,13 +2336,15 @@ def mock_get_usage(mocker, service_one, fake_uuid): "notification_type": "sms", "chargeable_units": 300, "notifications_sent": 300, - "rate": 0.0165, + "charged_units": 300, + "rate": 0.017, "cost": 5.1 }, { "notification_type": "letter", "chargeable_units": 300, "notifications_sent": 100, + "charged_units": 300, "rate": 0.1, "cost": 30 }, @@ -2466,6 +2470,7 @@ def mock_get_future_usage(mocker, service_one, fake_uuid): 'notification_type': 'sms', 'chargeable_units': 0, 'notifications_sent': 0, + 'charged_units': 0, 'rate': 0.0158, 'cost': 0 }, @@ -2473,6 +2478,7 @@ def mock_get_future_usage(mocker, service_one, fake_uuid): 'notification_type': 'email', 'chargeable_units': 0, 'notifications_sent': 0, + 'charged_units': 0, 'rate': 0.0, 'cost': 0 } From 5511f15ae83b1a07540bee7a5aec1ab57320500d Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 27 Apr 2022 17:08:44 +0100 Subject: [PATCH 10/10] Add missing dashboard test for zero SMS cost --- tests/app/main/views/test_dashboard.py | 27 ++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index 3e87f9c7f..5e6a6d2a7 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -1890,3 +1890,30 @@ def test_service_dashboard_shows_usage( '£30.00 ' 'spent on letters' ) + + +def test_service_dashboard_shows_free_allowance( + mocker, + client_request, + service_one, + mock_get_service_templates, + mock_get_template_statistics, + mock_has_no_jobs, + mock_get_free_sms_fragment_limit, + mock_get_returned_letter_statistics_with_no_returned_letters, +): + mocker.patch('app.billing_api_client.get_service_usage', return_value=[ + { + "notification_type": "sms", + "chargeable_units": 1000, + "charged_units": 0, + "rate": 0.0165, + "cost": 0 + } + ]) + + page = client_request.get('main.service_dashboard', service_id=SERVICE_ONE_ID) + + usage_text = normalize_spaces(page.select_one('[data-key=usage]').text) + assert 'spent on text messages' not in usage_text + assert '249,000 free text messages left' in usage_text