From 4ab7e3ceec364e10155bcfd8d85b3ec23d3c279e Mon Sep 17 00:00:00 2001
From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk>
Date: Fri, 29 Apr 2022 16:25:58 +0100
Subject: [PATCH 01/16] Rename billing API methods to be recognisable

I struggled to distinguish which was which as neither mentioned if
the data they returned was monthly or annual.
---
 app/main/views/dashboard.py                |   6 +-
 app/notify_client/billing_api_client.py    |   4 +-
 tests/app/main/test_permissions.py         |  12 +--
 tests/app/main/views/test_accept_invite.py |   6 +-
 tests/app/main/views/test_dashboard.py     | 110 ++++++++++-----------
 tests/app/main/views/test_sign_out.py      |   2 +-
 tests/conftest.py                          |  16 +--
 7 files changed, 78 insertions(+), 78 deletions(-)

diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py
index fc6210fac..3fff2b36f 100644
--- a/app/main/views/dashboard.py
+++ b/app/main/views/dashboard.py
@@ -134,8 +134,8 @@ def usage(service_id):
     year, current_financial_year = requested_and_current_financial_year(request)
 
     free_sms_allowance = billing_api_client.get_free_sms_fragment_limit_for_year(service_id, year)
-    units = billing_api_client.get_billable_units(service_id, year)
-    yearly_usage = billing_api_client.get_service_usage(service_id, year)
+    units = billing_api_client.get_monthly_usage_for_service(service_id, year)
+    yearly_usage = billing_api_client.get_annual_usage_for_service(service_id, year)
 
     return render_template(
         'views/usage.html',
@@ -289,7 +289,7 @@ def get_dashboard_partials(service_id):
         current_service.id,
         get_current_financial_year(),
     )
-    yearly_usage = billing_api_client.get_service_usage(
+    yearly_usage = billing_api_client.get_annual_usage_for_service(
         service_id,
         get_current_financial_year(),
     )
diff --git a/app/notify_client/billing_api_client.py b/app/notify_client/billing_api_client.py
index 44634ff4e..3c70b573f 100644
--- a/app/notify_client/billing_api_client.py
+++ b/app/notify_client/billing_api_client.py
@@ -3,13 +3,13 @@ from app.notify_client import NotifyAdminAPIClient
 
 class BillingAPIClient(NotifyAdminAPIClient):
 
-    def get_billable_units(self, service_id, year):
+    def get_monthly_usage_for_service(self, service_id, year):
         return self.get(
             '/service/{0}/billing/monthly-usage'.format(service_id),
             params=dict(year=year)
         )
 
-    def get_service_usage(self, service_id, year=None):
+    def get_annual_usage_for_service(self, service_id, year=None):
         return self.get(
             '/service/{0}/billing/yearly-usage-summary'.format(service_id),
             params=dict(year=year)
diff --git a/tests/app/main/test_permissions.py b/tests/app/main/test_permissions.py
index 9c64f2204..d94b53c77 100644
--- a/tests/app/main/test_permissions.py
+++ b/tests/app/main/test_permissions.py
@@ -33,8 +33,8 @@ def test_services_pages_that_org_users_are_allowed_to_see(
     client_request,
     mocker,
     api_user_active,
-    mock_get_usage,
-    mock_get_billable_units,
+    mock_get_annual_usage_for_service,
+    mock_get_monthly_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_service,
     mock_get_invites_for_service,
@@ -88,8 +88,8 @@ def test_service_navigation_for_org_user(
     client_request,
     mocker,
     api_user_active,
-    mock_get_usage,
-    mock_get_billable_units,
+    mock_get_annual_usage_for_service,
+    mock_get_monthly_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_service,
     mock_get_invites_for_service,
@@ -148,8 +148,8 @@ def test_service_user_without_manage_service_permission_can_see_usage_page_when_
     mocker,
     active_caseworking_user,
     mock_has_no_jobs,
-    mock_get_usage,
-    mock_get_billable_units,
+    mock_get_annual_usage_for_service,
+    mock_get_monthly_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_service,
     mock_get_invites_for_service,
diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py
index 783133a9a..a0710e828 100644
--- a/tests/app/main/views/test_accept_invite.py
+++ b/tests/app/main/views/test_accept_invite.py
@@ -221,8 +221,8 @@ def test_accepting_invite_removes_invite_from_session(
     mock_has_no_jobs,
     mock_get_service_statistics,
     mock_get_template_folders,
-    mock_get_usage,
-    mock_get_billable_units,
+    mock_get_annual_usage_for_service,
+    mock_get_monthly_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_inbound_sms_summary,
     mock_get_returned_letter_statistics_with_no_returned_letters,
@@ -606,7 +606,7 @@ def test_new_invited_user_verifies_and_added_to_service(
     mock_has_permissions,
     mock_no_users_for_service,
     mock_get_service_statistics,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_returned_letter_statistics_with_no_returned_letters,
     mock_create_event,
diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py
index 5e6a6d2a7..d5d7f85a4 100644
--- a/tests/app/main/views/test_dashboard.py
+++ b/tests/app/main/views/test_dashboard.py
@@ -141,7 +141,7 @@ def test_get_started(
     mock_get_service_templates_when_no_templates_exist,
     mock_has_no_jobs,
     mock_get_service_statistics,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_inbound_sms_summary,
     mock_get_returned_letter_statistics_with_no_returned_letters,
@@ -166,7 +166,7 @@ def test_get_started_is_hidden_once_templates_exist(
     mock_get_service_templates,
     mock_has_no_jobs,
     mock_get_service_statistics,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_inbound_sms_summary,
     mock_get_returned_letter_statistics_with_no_returned_letters,
@@ -192,7 +192,7 @@ def test_inbound_messages_not_visible_to_service_without_permissions(
     mock_has_no_jobs,
     mock_get_service_statistics,
     mock_get_template_statistics,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_inbound_sms_summary,
     mock_get_returned_letter_statistics_with_no_returned_letters,
@@ -218,7 +218,7 @@ def test_inbound_messages_shows_count_of_messages_when_there_are_messages(
     mock_get_scheduled_job_stats,
     mock_get_service_statistics,
     mock_get_template_statistics,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_inbound_sms_summary,
     mock_get_returned_letter_statistics_with_no_returned_letters,
@@ -246,7 +246,7 @@ def test_inbound_messages_shows_count_of_messages_when_there_are_no_messages(
     mock_get_scheduled_job_stats,
     mock_get_service_statistics,
     mock_get_template_statistics,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_inbound_sms_summary_with_no_messages,
     mock_get_returned_letter_statistics_with_no_returned_letters,
@@ -280,7 +280,7 @@ def test_inbox_showing_inbound_messages(
     mock_get_jobs,
     mock_get_service_statistics,
     mock_get_template_statistics,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_most_recent_inbound_sms,
     index,
     expected_row,
@@ -309,7 +309,7 @@ def test_get_inbound_sms_shows_page_links(
     mock_get_jobs,
     mock_get_service_statistics,
     mock_get_template_statistics,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_most_recent_inbound_sms,
     mock_get_inbound_number_for_service,
 ):
@@ -332,7 +332,7 @@ def test_empty_inbox(
     mock_get_jobs,
     mock_get_service_statistics,
     mock_get_template_statistics,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_most_recent_inbound_sms_with_no_messages,
     mock_get_inbound_number_for_service,
 ):
@@ -489,7 +489,7 @@ def test_returned_letters_not_visible_if_service_has_no_returned_letters(
     mock_has_no_jobs,
     mock_get_service_statistics,
     mock_get_template_statistics,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_inbound_sms_summary,
     mock_get_returned_letter_statistics_with_no_returned_letters,
@@ -525,7 +525,7 @@ def test_returned_letters_shows_count_of_recently_returned_letters(
     mock_get_scheduled_job_stats,
     mock_get_service_statistics,
     mock_get_template_statistics,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_inbound_sms_summary,
     reporting_date,
@@ -585,7 +585,7 @@ def test_returned_letters_only_counts_recently_returned_letters(
     mock_get_scheduled_job_stats,
     mock_get_service_statistics,
     mock_get_template_statistics,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_inbound_sms_summary_with_no_messages,
     reporting_date,
@@ -616,7 +616,7 @@ def test_should_show_recent_templates_on_dashboard(
     mock_get_service_templates,
     mock_has_no_jobs,
     mock_get_service_statistics,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_inbound_sms_summary,
     mock_get_returned_letter_statistics_with_no_returned_letters,
@@ -670,7 +670,7 @@ def test_should_not_show_recent_templates_on_dashboard_if_only_one_template_used
     mock_get_service_templates,
     mock_has_no_jobs,
     mock_get_service_statistics,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_inbound_sms_summary,
     mock_get_returned_letter_statistics_with_no_returned_letters,
@@ -826,7 +826,7 @@ def test_should_show_upcoming_jobs_on_dashboard(
     mock_get_service_statistics,
     mock_get_jobs,
     mock_get_scheduled_job_stats,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_inbound_sms_summary,
     mock_get_returned_letter_statistics_with_no_returned_letters,
@@ -863,7 +863,7 @@ def test_should_not_show_upcoming_jobs_on_dashboard_if_count_is_0(
     mock_get_template_statistics,
     mock_get_service_statistics,
     mock_has_jobs,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_inbound_sms_summary,
     mock_get_returned_letter_statistics_with_no_returned_letters,
@@ -889,7 +889,7 @@ def test_should_not_show_upcoming_jobs_on_dashboard_if_service_has_no_jobs(
     mock_get_service_statistics,
     mock_has_no_jobs,
     mock_get_scheduled_job_stats,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_inbound_sms_summary,
     mock_get_returned_letter_statistics_with_no_returned_letters,
@@ -931,7 +931,7 @@ def test_correct_font_size_for_big_numbers(
     mock_get_template_statistics,
     mock_get_service_statistics,
     mock_has_no_jobs,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_returned_letter_statistics_with_no_returned_letters,
     service_one,
@@ -968,7 +968,7 @@ def test_should_not_show_jobs_on_dashboard_for_users_with_uploads_page(
     mock_get_service_statistics,
     mock_get_jobs,
     mock_get_scheduled_job_stats,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_inbound_sms_summary,
     mock_get_returned_letter_statistics_with_no_returned_letters,
@@ -989,8 +989,8 @@ def test_should_not_show_jobs_on_dashboard_for_users_with_uploads_page(
 @freeze_time("2012-03-31 12:12:12")
 def test_usage_page(
     client_request,
-    mock_get_usage,
-    mock_get_billable_units,
+    mock_get_annual_usage_for_service,
+    mock_get_monthly_usage_for_service,
     mock_get_free_sms_fragment_limit
 ):
     page = client_request.get(
@@ -998,8 +998,8 @@ def test_usage_page(
         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_monthly_usage_for_service.assert_called_once_with(SERVICE_ONE_ID, 2011)
+    mock_get_annual_usage_for_service.assert_called_once_with(SERVICE_ONE_ID, 2011)
     mock_get_free_sms_fragment_limit.assert_called_with(SERVICE_ONE_ID, 2011)
 
     nav = page.find('ul', {'class': 'pill'})
@@ -1034,10 +1034,10 @@ def test_usage_page(
 def test_usage_page_no_sms_spend(
     mocker,
     client_request,
-    mock_get_billable_units,
+    mock_get_monthly_usage_for_service,
     mock_get_free_sms_fragment_limit
 ):
-    mocker.patch('app.billing_api_client.get_service_usage', return_value=[
+    mocker.patch('app.billing_api_client.get_annual_usage_for_service', return_value=[
         {
             "notification_type": "sms",
             "chargeable_units": 1000,
@@ -1065,8 +1065,8 @@ def test_usage_page_no_sms_spend(
 def test_usage_page_monthly_breakdown(
     client_request,
     service_one,
-    mock_get_usage,
-    mock_get_billable_units,
+    mock_get_annual_usage_for_service,
+    mock_get_monthly_usage_for_service,
     mock_get_free_sms_fragment_limit
 ):
     service_one['permissions'].append('letter')
@@ -1097,10 +1097,10 @@ def test_usage_page_displays_letters_ordered_by_postage(
     mocker,
     client_request,
     service_one,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit
 ):
-    billable_units_resp = [
+    monthly_usage = [
         {'month': 'April', 'notification_type': 'letter', 'rate': 0.5, 'billing_units': 1, 'postage': 'second'},
         {'month': 'April', 'notification_type': 'letter', 'rate': 1, 'billing_units': 1, 'postage': 'europe'},
         {'month': 'April', 'notification_type': 'letter', 'rate': 1, 'billing_units': 2, 'postage': 'rest-of-word'},
@@ -1108,7 +1108,7 @@ def test_usage_page_displays_letters_ordered_by_postage(
         {'month': 'April', 'notification_type': 'letter', 'rate': 0.3, 'billing_units': 3, 'postage': 'second'},
         {'month': 'April', 'notification_type': 'letter', 'rate': 0.5, 'billing_units': 1, 'postage': 'first'},
     ]
-    mocker.patch('app.billing_api_client.get_billable_units', return_value=billable_units_resp)
+    mocker.patch('app.billing_api_client.get_monthly_usage_for_service', return_value=monthly_usage)
     service_one['permissions'].append('letter')
     page = client_request.get(
         'main.usage',
@@ -1131,7 +1131,7 @@ def test_usage_page_displays_letters_split_by_month_and_postage(
     mocker,
     client_request,
     service_one,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit
 ):
     billable_units_resp = [
@@ -1141,7 +1141,7 @@ def test_usage_page_displays_letters_split_by_month_and_postage(
         {'month': 'May', 'notification_type': 'letter', 'rate': 0.5, 'billing_units': 3, 'postage': 'second'},
         {'month': 'May', 'notification_type': 'letter', 'rate': 0.7, 'billing_units': 1, 'postage': 'first'},
     ]
-    mocker.patch('app.billing_api_client.get_billable_units', return_value=billable_units_resp)
+    mocker.patch('app.billing_api_client.get_monthly_usage_for_service', return_value=billable_units_resp)
     service_one['permissions'].append('letter')
     page = client_request.get(
         'main.usage',
@@ -1161,8 +1161,8 @@ def test_usage_page_displays_letters_split_by_month_and_postage(
 def test_usage_page_with_0_free_allowance(
     mocker,
     client_request,
-    mock_get_usage,
-    mock_get_billable_units,
+    mock_get_annual_usage_for_service,
+    mock_get_monthly_usage_for_service,
 ):
     mocker.patch(
         'app.billing_api_client.get_free_sms_fragment_limit_for_year',
@@ -1183,8 +1183,8 @@ def test_usage_page_with_0_free_allowance(
 
 def test_usage_page_with_year_argument(
     client_request,
-    mock_get_usage,
-    mock_get_billable_units,
+    mock_get_annual_usage_for_service,
+    mock_get_monthly_usage_for_service,
     mock_get_free_sms_fragment_limit,
 ):
     client_request.get(
@@ -1192,8 +1192,8 @@ def test_usage_page_with_year_argument(
         service_id=SERVICE_ONE_ID,
         year=2000,
     )
-    mock_get_billable_units.assert_called_once_with(SERVICE_ONE_ID, 2000)
-    mock_get_usage.assert_called_once_with(SERVICE_ONE_ID, 2000)
+    mock_get_monthly_usage_for_service.assert_called_once_with(SERVICE_ONE_ID, 2000)
+    mock_get_annual_usage_for_service.assert_called_once_with(SERVICE_ONE_ID, 2000)
     mock_get_free_sms_fragment_limit.assert_called_with(SERVICE_ONE_ID, 2000)
 
 
@@ -1211,8 +1211,8 @@ def test_usage_page_for_invalid_year(
 @freeze_time("2012-03-31 12:12:12")
 def test_future_usage_page(
     client_request,
-    mock_get_future_usage,
-    mock_get_future_billable_units,
+    mock_get_annual_usage_for_service_in_future,
+    mock_get_monthly_usage_for_service_in_future,
     mock_get_free_sms_fragment_limit
 ):
     client_request.get(
@@ -1221,8 +1221,8 @@ def test_future_usage_page(
         year=2014,
     )
 
-    mock_get_future_billable_units.assert_called_once_with(SERVICE_ONE_ID, 2014)
-    mock_get_future_usage.assert_called_once_with(SERVICE_ONE_ID, 2014)
+    mock_get_monthly_usage_for_service_in_future.assert_called_once_with(SERVICE_ONE_ID, 2014)
+    mock_get_annual_usage_for_service_in_future.assert_called_once_with(SERVICE_ONE_ID, 2014)
     mock_get_free_sms_fragment_limit.assert_called_with(SERVICE_ONE_ID, 2014)
 
 
@@ -1248,7 +1248,7 @@ def test_menu_send_messages(
     mock_has_no_jobs,
     mock_get_template_statistics,
     mock_get_service_statistics,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_inbound_sms_summary,
     mock_get_free_sms_fragment_limit,
     mock_get_returned_letter_statistics_with_no_returned_letters,
@@ -1284,7 +1284,7 @@ def test_menu_send_messages_when_service_does_not_have_upload_letters_permission
     mock_has_no_jobs,
     mock_get_template_statistics,
     mock_get_service_statistics,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_inbound_sms_summary,
     mock_get_free_sms_fragment_limit,
     mock_get_returned_letter_statistics_with_no_returned_letters,
@@ -1309,7 +1309,7 @@ def test_menu_manage_service(
     mock_has_no_jobs,
     mock_get_template_statistics,
     mock_get_service_statistics,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_inbound_sms_summary,
     mock_get_returned_letter_statistics_with_no_returned_letters,
     mock_get_free_sms_fragment_limit,
@@ -1340,7 +1340,7 @@ def test_menu_manage_api_keys(
     mock_has_no_jobs,
     mock_get_template_statistics,
     mock_get_service_statistics,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_inbound_sms_summary,
     mock_get_returned_letter_statistics_with_no_returned_letters,
     mock_get_free_sms_fragment_limit,
@@ -1369,7 +1369,7 @@ def test_menu_all_services_for_platform_admin_user(
     mock_has_no_jobs,
     mock_get_template_statistics,
     mock_get_service_statistics,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_inbound_sms_summary,
     mock_get_returned_letter_statistics_with_no_returned_letters,
     mock_get_free_sms_fragment_limit,
@@ -1400,7 +1400,7 @@ def test_route_for_service_permissions(
     mock_has_no_jobs,
     mock_get_template_statistics,
     mock_get_service_statistics,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_inbound_sms_summary,
     mock_get_returned_letter_statistics_with_no_returned_letters,
@@ -1454,7 +1454,7 @@ def test_service_dashboard_updates_gets_dashboard_totals(
     mock_get_template_statistics,
     mock_get_service_statistics,
     mock_has_no_jobs,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_inbound_sms_summary,
     mock_get_returned_letter_statistics_with_no_returned_letters,
@@ -1722,7 +1722,7 @@ def test_org_breadcrumbs_do_not_show_if_service_has_no_org(
     mock_get_template_statistics,
     mock_get_service_templates_when_no_templates_exist,
     mock_has_no_jobs,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_returned_letter_statistics_with_no_returned_letters,
 ):
@@ -1760,7 +1760,7 @@ def test_org_breadcrumbs_show_if_user_is_a_member_of_the_services_org(
     mock_get_template_statistics,
     mock_get_service_templates_when_no_templates_exist,
     mock_has_no_jobs,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_returned_letter_statistics_with_no_returned_letters,
     active_user_with_permissions,
@@ -1790,7 +1790,7 @@ def test_org_breadcrumbs_do_not_show_if_user_is_a_member_of_the_services_org_but
     mock_get_template_statistics,
     mock_get_service_templates_when_no_templates_exist,
     mock_has_no_jobs,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_returned_letter_statistics_with_no_returned_letters,
     active_user_with_permissions,
@@ -1815,7 +1815,7 @@ def test_org_breadcrumbs_show_if_user_is_platform_admin(
     mock_get_template_statistics,
     mock_get_service_templates_when_no_templates_exist,
     mock_has_no_jobs,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_returned_letter_statistics_with_no_returned_letters,
     platform_admin_user,
@@ -1844,7 +1844,7 @@ def test_breadcrumb_shows_if_service_is_suspended(
     mock_get_template_statistics,
     mock_get_service_templates_when_no_templates_exist,
     mock_has_no_jobs,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_returned_letter_statistics_with_no_returned_letters,
     active_user_with_permissions,
@@ -1872,7 +1872,7 @@ def test_service_dashboard_shows_usage(
     mock_get_service_templates,
     mock_get_template_statistics,
     mock_has_no_jobs,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_returned_letter_statistics_with_no_returned_letters,
     permissions,
@@ -1902,7 +1902,7 @@ def test_service_dashboard_shows_free_allowance(
     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=[
+    mocker.patch('app.billing_api_client.get_annual_usage_for_service', return_value=[
         {
             "notification_type": "sms",
             "chargeable_units": 1000,
diff --git a/tests/app/main/views/test_sign_out.py b/tests/app/main/views/test_sign_out.py
index e9b795241..31d4a787e 100644
--- a/tests/app/main/views/test_sign_out.py
+++ b/tests/app/main/views/test_sign_out.py
@@ -31,7 +31,7 @@ def test_sign_out_user(
     mock_has_permissions,
     mock_get_template_statistics,
     mock_get_service_statistics,
-    mock_get_usage,
+    mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit,
     mock_get_inbound_sms_summary,
     mock_get_returned_letter_statistics_with_no_returned_letters
diff --git a/tests/conftest.py b/tests/conftest.py
index fbd2470b8..2daef0e7b 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -2313,7 +2313,7 @@ def mock_get_monthly_notification_stats(mocker, service_one, fake_uuid):
 
 
 @pytest.fixture(scope='function')
-def mock_get_usage(mocker, service_one, fake_uuid):
+def mock_get_annual_usage_for_service(mocker, service_one, fake_uuid):
     def _get_usage(service_id, year=None):
         return [
             {
@@ -2351,11 +2351,11 @@ def mock_get_usage(mocker, service_one, fake_uuid):
         ]
 
     return mocker.patch(
-        'app.billing_api_client.get_service_usage', side_effect=_get_usage)
+        'app.billing_api_client.get_annual_usage_for_service', side_effect=_get_usage)
 
 
 @pytest.fixture(scope='function')
-def mock_get_billable_units(mocker):
+def mock_get_monthly_usage_for_service(mocker):
     def _get_usage(service_id, year):
         return [
             {
@@ -2459,11 +2459,11 @@ def mock_get_billable_units(mocker):
         ]
 
     return mocker.patch(
-        'app.billing_api_client.get_billable_units', side_effect=_get_usage)
+        'app.billing_api_client.get_monthly_usage_for_service', side_effect=_get_usage)
 
 
 @pytest.fixture(scope='function')
-def mock_get_future_usage(mocker, service_one, fake_uuid):
+def mock_get_annual_usage_for_service_in_future(mocker, service_one, fake_uuid):
     def _get_usage(service_id, year=None):
         return [
             {
@@ -2485,16 +2485,16 @@ def mock_get_future_usage(mocker, service_one, fake_uuid):
         ]
 
     return mocker.patch(
-        'app.billing_api_client.get_service_usage', side_effect=_get_usage)
+        'app.billing_api_client.get_annual_usage_for_service', side_effect=_get_usage)
 
 
 @pytest.fixture(scope='function')
-def mock_get_future_billable_units(mocker):
+def mock_get_monthly_usage_for_service_in_future(mocker):
     def _get_usage(service_id, year):
         return []
 
     return mocker.patch(
-        'app.billing_api_client.get_billable_units', side_effect=_get_usage)
+        'app.billing_api_client.get_monthly_usage_for_service', side_effect=_get_usage)
 
 
 @pytest.fixture(scope='function')

From ca2ff00931202dfe7ed3c2979f4d314be7c551a2 Mon Sep 17 00:00:00 2001
From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk>
Date: Fri, 29 Apr 2022 16:29:09 +0100
Subject: [PATCH 02/16] Rename monthly helper function to match annual one

---
 app/main/views/dashboard.py            | 8 ++++----
 tests/app/main/views/test_dashboard.py | 9 +++++----
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py
index 3fff2b36f..4d4605771 100644
--- a/app/main/views/dashboard.py
+++ b/app/main/views/dashboard.py
@@ -139,7 +139,7 @@ def usage(service_id):
 
     return render_template(
         'views/usage.html',
-        months=list(get_free_paid_breakdown_for_billable_units(
+        months=list(get_monthly_usage_breakdown(
             year,
             free_sms_allowance,
             units
@@ -404,10 +404,10 @@ 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):
+def get_monthly_usage_breakdown(year, free_sms_fragment_limit, monthly_usage):
     cumulative = 0
-    sms_units = [x for x in billing_units if x['notification_type'] == 'sms']
-    letter_units = [x for x in billing_units if x['notification_type'] == 'letter']
+    sms_units = [x for x in monthly_usage if x['notification_type'] == 'sms']
+    letter_units = [x for x in monthly_usage if x['notification_type'] == 'letter']
     for month in get_months_for_financial_year(year):
         previous_cumulative = cumulative
         monthly_usage = get_sum_billing_units(sms_units, month)
diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py
index d5d7f85a4..8271b2397 100644
--- a/tests/app/main/views/test_dashboard.py
+++ b/tests/app/main/views/test_dashboard.py
@@ -12,7 +12,7 @@ from app.main.views.dashboard import (
     aggregate_template_usage,
     format_monthly_stats_to_list,
     get_dashboard_totals,
-    get_free_paid_breakdown_for_billable_units,
+    get_monthly_usage_breakdown,
     get_tuples_of_financial_years,
 )
 from tests import (
@@ -1576,10 +1576,10 @@ def test_aggregate_status_types(dict_in, expected_failed, expected_requested):
         (freeze_time("2017-01-01 11:09:00.061258"), 10)
     ]
 )
-def test_get_free_paid_breakdown_for_billable_units(now, expected_number_of_months):
+def test_get_monthly_usage_breakdown(now, expected_number_of_months):
     sms_allowance = 250000
     with now:
-        billing_units = get_free_paid_breakdown_for_billable_units(
+        breakdown = get_monthly_usage_breakdown(
             2016, sms_allowance, [
                 {
                     'month': 'April', 'international': False, 'rate_multiplier': 1,
@@ -1599,7 +1599,8 @@ def test_get_free_paid_breakdown_for_billable_units(now, expected_number_of_mont
                 },
             ]
         )
-        assert list(billing_units) == [
+
+        assert list(breakdown) == [
             {
                 'sms_free_count': 100000,
                 'name': 'April',

From 215a688250104ab5afcc42a4aba0aa731ed03d53 Mon Sep 17 00:00:00 2001
From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk>
Date: Fri, 29 Apr 2022 16:30:53 +0100
Subject: [PATCH 03/16] Reuse helper function to filter usage rows

I've also dispensed with the "units" terminology here, which didn't
represent the "rows" returned by the API.
---
 app/main/views/dashboard.py | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py
index 4d4605771..39fe0eb5f 100644
--- a/app/main/views/dashboard.py
+++ b/app/main/views/dashboard.py
@@ -406,18 +406,19 @@ def get_usage_breakdown_by_type(usage, notification_type):
 
 def get_monthly_usage_breakdown(year, free_sms_fragment_limit, monthly_usage):
     cumulative = 0
-    sms_units = [x for x in monthly_usage if x['notification_type'] == 'sms']
-    letter_units = [x for x in monthly_usage if x['notification_type'] == 'letter']
+    sms = get_usage_breakdown_by_type(monthly_usage, 'sms')
+    letters = get_usage_breakdown_by_type(monthly_usage, 'letter')
+
     for month in get_months_for_financial_year(year):
         previous_cumulative = cumulative
-        monthly_usage = get_sum_billing_units(sms_units, month)
+        monthly_usage = get_sum_billing_units(sms, month)
         cumulative += monthly_usage
         breakdown = get_free_paid_breakdown_for_month(
             free_sms_fragment_limit, cumulative, previous_cumulative,
-            [billing_month for billing_month in sms_units if billing_month['month'] == month]
+            [billing_month for billing_month in sms if billing_month['month'] == month]
         )
 
-        letter_units_for_month = [x for x in letter_units if x['month'] == month]
+        letter_units_for_month = [x for x in letters if x['month'] == month]
         letter_billing = format_letter_details_for_month(letter_units_for_month)
 
         letter_total = 0

From 246356649f16daa289d4ac1e3689c89259db3e5a Mon Sep 17 00:00:00 2001
From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk>
Date: Fri, 29 Apr 2022 16:42:22 +0100
Subject: [PATCH 04/16] Rename monthly usage attributes to match API

This should make the subsequent changes to use the new API fields
a bit clearer, and also matches the annual usage attributes [^1].

[^1]: https://github.com/alphagov/notifications-admin/blob/3a1ac189ff6800d4789b22e4763e5550719e8802/app/main/views/dashboard.py#L343-L350
---
 app/main/views/dashboard.py            |  26 +++---
 app/templates/views/usage.html         |  24 ++---
 tests/app/main/views/test_dashboard.py | 120 ++++++++++++-------------
 3 files changed, 85 insertions(+), 85 deletions(-)

diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py
index 39fe0eb5f..77472d6f8 100644
--- a/app/main/views/dashboard.py
+++ b/app/main/views/dashboard.py
@@ -413,24 +413,24 @@ def get_monthly_usage_breakdown(year, free_sms_fragment_limit, monthly_usage):
         previous_cumulative = cumulative
         monthly_usage = get_sum_billing_units(sms, month)
         cumulative += monthly_usage
-        breakdown = get_free_paid_breakdown_for_month(
+        sms_breakdown = get_free_paid_breakdown_for_month(
             free_sms_fragment_limit, cumulative, previous_cumulative,
             [billing_month for billing_month in sms if billing_month['month'] == month]
         )
 
         letter_units_for_month = [x for x in letters if x['month'] == month]
-        letter_billing = format_letter_details_for_month(letter_units_for_month)
+        letter_breakdown = format_letter_details_for_month(letter_units_for_month)
 
-        letter_total = 0
-        for x in letter_billing:
-            letter_total += x.cost
+        letter_cost = 0
+        for x in letter_breakdown:
+            letter_cost += x.cost
         yield {
-            'name': month,
-            'letter_total': letter_total,
-            'letters': letter_billing,
-            'sms_paid_count': breakdown['paid'],
-            'sms_free_count': breakdown['free'],
-            'sms_rate': breakdown['sms_rate'],
+            'month': month,
+            'letter_cost': letter_cost,
+            'letter_breakdown': letter_breakdown,
+            'sms_charged': sms_breakdown['paid'],
+            'sms_free_allowance_used': sms_breakdown['free'],
+            'sms_rate': sms_breakdown['sms_rate'],
         }
 
 
@@ -445,7 +445,7 @@ def format_letter_details_for_month(letter_units_for_month):
     postage_order = {'first class': 0, 'second class': 1, 'international': 2}
     letter_units_for_month.sort(key=lambda x: (postage_order[x['postage']], x['rate']))
 
-    LetterDetails = namedtuple('LetterDetails', ['billing_units', 'rate', 'cost', 'postage_description'])
+    LetterDetails = namedtuple('LetterDetails', ['sent', 'rate', 'cost', 'postage_description'])
 
     # Aggregate the rows for international letters which have the same rate
     result = []
@@ -453,7 +453,7 @@ def format_letter_details_for_month(letter_units_for_month):
         rate_group = list(rate_group)
 
         letter_details = LetterDetails(
-            billing_units=sum(x['billing_units'] for x in rate_group),
+            sent=sum(x['billing_units'] for x in rate_group),
             rate=rate_group[0]['rate'],
             cost=(sum(x['billing_units'] for x in rate_group) * rate_group[0]['rate']),
             postage_description=rate_group[0]['postage']
diff --git a/app/templates/views/usage.html b/app/templates/views/usage.html
index a9bd086b0..59fd31979 100644
--- a/app/templates/views/usage.html
+++ b/app/templates/views/usage.html
@@ -83,7 +83,7 @@
 
       {% if months %}
         <div class="dashboard-table usage-table body-copy-table">
-          {% call(month, row_index) list_table(
+          {% call(item, row_index) list_table(
             months,
             caption="Total spend",
             caption_visible=False,
@@ -95,29 +95,29 @@
             field_headings_visible=True
           ) %}
             {% call row_heading() %}
-              {{ month.name }}
+              {{ item.month }}
             {% endcall %}
             {% call field(align='left') %}
               {{ big_number(
-                (month.sms_rate * month.sms_paid_count) + month.letter_total,
+                (item.sms_rate * item.sms_charged) + item.letter_cost,
                 currency="£",
                 smallest=True
               ) }}
               <ul>
-              {% if month.sms_free_count %}
-                <li class="tabular-numbers">{{ month.sms_free_count|format_thousands }} free {{ month.sms_free_count|message_count_label('sms', suffix='') }}</li>
+              {% if item.sms_free_allowance_used %}
+                <li class="tabular-numbers">{{ item.sms_free_allowance_used|format_thousands }} free {{ item.sms_free_count|message_count_label('sms', suffix='') }}</li>
               {% endif %}
-              {% if month.sms_paid_count %}
-                <li class="tabular-numbers">{{ month.sms_paid_count|message_count('sms') }} at
-                {{- ' {:.2f}p'.format(month.sms_rate * 100) }}</li>
+              {% if item.sms_charged %}
+                <li class="tabular-numbers">{{ item.sms_charged|message_count('sms') }} at
+                {{- ' {:.2f}p'.format(item.sms_rate * 100) }}</li>
               {% endif %}
-              {% for letter in month.letters%}
-                {% if letter.billing_units %}
-                  <li class="tabular-numbers">{{ "{:,} {}".format(letter.billing_units, letter.postage_description) }} {{ letter.billing_units|message_count_label('letter', suffix='') }} at
+              {% for letter in item.letter_breakdown %}
+                {% if letter.sent %}
+                  <li class="tabular-numbers">{{ "{:,} {}".format(letter.sent, letter.postage_description) }} {{ letter.sent|message_count_label('letter', suffix='') }} at
                   {{ letter.rate | format_number_in_pounds_as_currency }}</li>
                 {% endif %}
               {% endfor %}
-              {% if not (month.sms_free_count or month.sms_paid_count or month.letters) %}
+              {% if not (item.sms_free_allowance_used or item.sms_charged or item.letter_breakdown) %}
                 <li aria-hidden="true">–</li>
               {% endif %}
               </ul>
diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py
index 8271b2397..7e486feda 100644
--- a/tests/app/main/views/test_dashboard.py
+++ b/tests/app/main/views/test_dashboard.py
@@ -1602,100 +1602,100 @@ def test_get_monthly_usage_breakdown(now, expected_number_of_months):
 
         assert list(breakdown) == [
             {
-                'sms_free_count': 100000,
-                'name': 'April',
-                'sms_paid_count': 0,
+                'sms_free_allowance_used': 100000,
+                'month': 'April',
+                'sms_charged': 0,
                 'sms_rate': 1.65,
-                'letter_total': 0,
-                'letters': []
+                'letter_cost': 0,
+                'letter_breakdown': []
             },
             {
-                'sms_free_count': 100000,
-                'name': 'May',
-                'sms_paid_count': 0,
+                'sms_free_allowance_used': 100000,
+                'month': 'May',
+                'sms_charged': 0,
                 'sms_rate': 1.65,
-                'letter_total': 0,
-                'letters': []
+                'letter_cost': 0,
+                'letter_breakdown': []
             },
             {
-                'sms_free_count': 50000,
-                'name': 'June',
-                'sms_paid_count': 50000,
+                'sms_free_allowance_used': 50000,
+                'month': 'June',
+                'sms_charged': 50000,
                 'sms_rate': 1.71,
-                'letter_total': 0,
-                'letters': []
+                'letter_cost': 0,
+                'letter_breakdown': []
             },
             {
-                'sms_free_count': 0,
-                'name': 'July',
-                'sms_paid_count': 0,
+                'sms_free_allowance_used': 0,
+                'month': 'July',
+                'sms_charged': 0,
                 'sms_rate': 0,
-                'letter_total': 0,
-                'letters': []
+                'letter_cost': 0,
+                'letter_breakdown': []
             },
             {
-                'sms_free_count': 0,
-                'name': 'August',
-                'sms_paid_count': 0,
+                'sms_free_allowance_used': 0,
+                'month': 'August',
+                'sms_charged': 0,
                 'sms_rate': 0,
-                'letter_total': 0,
-                'letters': []
+                'letter_cost': 0,
+                'letter_breakdown': []
             },
             {
-                'sms_free_count': 0,
-                'name': 'September',
-                'sms_paid_count': 0,
+                'sms_free_allowance_used': 0,
+                'month': 'September',
+                'sms_charged': 0,
                 'sms_rate': 0,
-                'letter_total': 0,
-                'letters': []
+                'letter_cost': 0,
+                'letter_breakdown': []
             },
             {
-                'sms_free_count': 0,
-                'name': 'October',
-                'sms_paid_count': 0,
+                'sms_free_allowance_used': 0,
+                'month': 'October',
+                'sms_charged': 0,
                 'sms_rate': 0,
-                'letter_total': 0,
-                'letters': []
+                'letter_cost': 0,
+                'letter_breakdown': []
             },
             {
-                'sms_free_count': 0,
-                'name': 'November',
-                'sms_paid_count': 0,
+                'sms_free_allowance_used': 0,
+                'month': 'November',
+                'sms_charged': 0,
                 'sms_rate': 0,
-                'letter_total': 0,
-                'letters': []
+                'letter_cost': 0,
+                'letter_breakdown': []
             },
             {
-                'sms_free_count': 0,
-                'name': 'December',
-                'sms_paid_count': 0,
+                'sms_free_allowance_used': 0,
+                'month': 'December',
+                'sms_charged': 0,
                 'sms_rate': 0,
-                'letter_total': 0,
-                'letters': []
+                'letter_cost': 0,
+                'letter_breakdown': []
             },
             {
-                'sms_free_count': 0,
-                'name': 'January',
-                'sms_paid_count': 0,
+                'sms_free_allowance_used': 0,
+                'month': 'January',
+                'sms_charged': 0,
                 'sms_rate': 0,
-                'letter_total': 0,
-                'letters': []
+                'letter_cost': 0,
+                'letter_breakdown': []
             },
             {
-                'sms_free_count': 0,
-                'name': 'February',
-                'sms_paid_count': 2000,
+                'sms_free_allowance_used': 0,
+                'month': 'February',
+                'sms_charged': 2000,
                 'sms_rate': 1.71,
-                'letter_total': 0,
-                'letters': []
+                'letter_cost': 0,
+                'letter_breakdown': []
             },
             {
-                'sms_free_count': 0,
-                'name': 'March',
-                'sms_paid_count': 0,
+                'sms_free_allowance_used': 0,
+                'month': 'March',
+                'sms_charged': 0,
                 'sms_rate': 0,
-                'letter_total': 0,
-                'letters': []
+                'letter_cost': 0,
+                'letter_breakdown': []
             },
         ][:expected_number_of_months]
 

From 4ab795ad687cf4e10ca5d7748451903f22bfc51e Mon Sep 17 00:00:00 2001
From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk>
Date: Fri, 29 Apr 2022 16:57:55 +0100
Subject: [PATCH 05/16] Replace "sum_billing_units" with inline code

This is also an opportunity to DRY-up the filtering of usage by
month, which we will reuse in the following commits. Doing a sum
is simple enough that it can be done inline, avoiding indirection.
---
 app/main/views/dashboard.py | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py
index 77472d6f8..69a33e5e2 100644
--- a/app/main/views/dashboard.py
+++ b/app/main/views/dashboard.py
@@ -394,12 +394,6 @@ def get_months_for_year(start, end, year):
     return [datetime(year, month, 1) for month in range(start, end)]
 
 
-def get_sum_billing_units(billing_units, month=None):
-    if month:
-        return sum(b['billing_units'] for b in billing_units if b['month'] == month)
-    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]
 
@@ -410,16 +404,16 @@ def get_monthly_usage_breakdown(year, free_sms_fragment_limit, monthly_usage):
     letters = get_usage_breakdown_by_type(monthly_usage, 'letter')
 
     for month in get_months_for_financial_year(year):
+        monthly_sms = [row for row in sms if row['month'] == month]
         previous_cumulative = cumulative
-        monthly_usage = get_sum_billing_units(sms, month)
-        cumulative += monthly_usage
+        monthly_chargeable_units = sum(row['billing_units'] for row in monthly_sms)
+        cumulative += monthly_chargeable_units
         sms_breakdown = get_free_paid_breakdown_for_month(
-            free_sms_fragment_limit, cumulative, previous_cumulative,
-            [billing_month for billing_month in sms if billing_month['month'] == month]
+            free_sms_fragment_limit, cumulative, previous_cumulative, monthly_sms
         )
 
-        letter_units_for_month = [x for x in letters if x['month'] == month]
-        letter_breakdown = format_letter_details_for_month(letter_units_for_month)
+        monthly_letters = [row for row in letters if row['month'] == month]
+        letter_breakdown = format_letter_details_for_month(monthly_letters)
 
         letter_cost = 0
         for x in letter_breakdown:
@@ -483,7 +477,7 @@ def get_free_paid_breakdown_for_month(
     # been sent at a single rate during the month
     sms_rate = monthly_usage[0]['rate'] if len(monthly_usage) else 0
 
-    total_monthly_billing_units = get_sum_billing_units(monthly_usage)
+    total_monthly_billing_units = sum(row['billing_units'] for row in monthly_usage)
 
     if cumulative < allowance:
         return {

From ce8bdea9be06287b3192e05f3933b413ee76beca Mon Sep 17 00:00:00 2001
From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk>
Date: Fri, 29 Apr 2022 17:27:37 +0100
Subject: [PATCH 06/16] Fix inaccurate test data for monthly usage API

It has never been possible to get multiple rows for the same month
and rate. This was making it hard to switch to the new API fields,
which will require some manual calculations. I've added the billing
units together in the remaining data so the tests still pass.

I've also moved the "April" row to the end as it was out-of-order
with all the others: it's the _start_ of the financial year.
---
 tests/conftest.py | 67 +++++++----------------------------------------
 1 file changed, 9 insertions(+), 58 deletions(-)

diff --git a/tests/conftest.py b/tests/conftest.py
index 2daef0e7b..a47b311a8 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -2358,74 +2358,18 @@ def mock_get_annual_usage_for_service(mocker, service_one, fake_uuid):
 def mock_get_monthly_usage_for_service(mocker):
     def _get_usage(service_id, year):
         return [
-            {
-                'month': 'April',
-                'notification_type': 'sms',
-                'rate': 0.017,
-                'billing_units': 249500,
-                'postage': 'none',
-            },
-            {
-                'month': 'April',
-                'notification_type': 'sms',
-                'rate': 0.017,
-                'billing_units': 100,
-                'postage': 'none',
-            },
-            {
-                'month': 'April',
-                'notification_type': 'sms',
-                'rate': 0.017,
-                'billing_units': 200,
-                'postage': 'none',
-            },
-            {
-                'month': 'April',
-                'notification_type': 'sms',
-                'rate': 0.017,
-                'billing_units': 60,
-                'postage': 'none',
-            },
             {
                 'month': 'March',
                 'notification_type': 'sms',
                 'rate': 0.017,
-                'billing_units': 1000,
-                'postage': 'none',
-            },
-            {
-                'month': 'March',
-                'notification_type': 'sms',
-                'rate': 0.017,
-                'billing_units': 100,
-                'postage': 'none',
-            },
-            {
-                'month': 'March',
-                'notification_type': 'sms',
-                'rate': 0.017,
-                'billing_units': 100,
-                'postage': 'none',
-            },
-            {
-                'month': 'March',
-                'notification_type': 'sms',
-                'rate': 0.017,
-                'billing_units': 30,
+                'billing_units': 1230,
                 'postage': 'none',
             },
             {
                 'month': 'February',
                 'notification_type': 'sms',
                 'rate': 0.0165,
-                'billing_units': 1000,
-                'postage': 'none',
-            },
-            {
-                'month': 'February',
-                'notification_type': 'sms',
-                'rate': 0.0165,
-                'billing_units': 100,
+                'billing_units': 1100,
                 'postage': 'none',
             },
             {
@@ -2456,6 +2400,13 @@ def mock_get_monthly_usage_for_service(mocker):
                 'billing_units': 7,
                 'postage': 'rest-of-world',
             },
+            {
+                'month': 'April',
+                'notification_type': 'sms',
+                'rate': 0.017,
+                'billing_units': 249860,
+                'postage': 'none',
+            },
         ]
 
     return mocker.patch(

From f6dc30665f78f85158040e2f537acd12222718ef Mon Sep 17 00:00:00 2001
From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk>
Date: Fri, 29 Apr 2022 17:48:01 +0100
Subject: [PATCH 07/16] Reorder monthly usage assertions (group by month)

This also removes an extremely confusing assertion of "40" free
messages, which was passing because we have "140" free messages.
---
 tests/app/main/views/test_dashboard.py | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py
index 7e486feda..befc8c08c 100644
--- a/tests/app/main/views/test_dashboard.py
+++ b/tests/app/main/views/test_dashboard.py
@@ -1077,20 +1077,21 @@ def test_usage_page_monthly_breakdown(
 
     monthly_breakdown = normalize_spaces(page.find('table').text)
 
-    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 '249,860 free text messages' 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 '960 text messages at 1.65p' 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
 
+    assert 'March' in monthly_breakdown
+    assert '£20.91' in monthly_breakdown
+    assert '1,230 text messages at 1.70p' in monthly_breakdown
+
 
 @freeze_time("2012-04-30 12:12:12")
 def test_usage_page_displays_letters_ordered_by_postage(

From d798a0d60f48aad742d84fbf82055a83cfe4c4ac Mon Sep 17 00:00:00 2001
From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk>
Date: Fri, 29 Apr 2022 17:44:27 +0100
Subject: [PATCH 08/16] Replace manual SMS monthly calculations with API

This starts using the sms_{cost, charged, free_allowance_used}
fields in the new API to replace the "get_free_paid_breakdown"
function we had before, which could not support multiple rates.

In order to use "get_free_paid_breakdown" the calling method had
to store a "cumulative" variable to calculate the free allowance
used so far, which is now done by the API.

To calculate the data for conftest.py, I had to start from the
bottom ("April") and manually calculate the free allowance used
to emulate the API - this is what "cumulative" used to do.
---
 app/main/views/dashboard.py            |  65 ++++------------
 app/templates/views/usage.html         |   2 +-
 tests/app/main/views/test_dashboard.py | 103 +++++++++++++++++--------
 tests/conftest.py                      |  21 +++++
 4 files changed, 105 insertions(+), 86 deletions(-)

diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py
index 69a33e5e2..2f4117bbe 100644
--- a/app/main/views/dashboard.py
+++ b/app/main/views/dashboard.py
@@ -139,11 +139,7 @@ def usage(service_id):
 
     return render_template(
         'views/usage.html',
-        months=list(get_monthly_usage_breakdown(
-            year,
-            free_sms_allowance,
-            units
-        )),
+        months=list(get_monthly_usage_breakdown(year, units)),
         selected_year=year,
         years=get_tuples_of_financial_years(
             partial(url_for, '.usage', service_id=service_id),
@@ -398,19 +394,19 @@ def get_usage_breakdown_by_type(usage, notification_type):
     return [row for row in usage if row['notification_type'] == notification_type]
 
 
-def get_monthly_usage_breakdown(year, free_sms_fragment_limit, monthly_usage):
-    cumulative = 0
+def get_monthly_usage_breakdown(year, monthly_usage):
     sms = get_usage_breakdown_by_type(monthly_usage, 'sms')
     letters = get_usage_breakdown_by_type(monthly_usage, 'letter')
 
     for month in get_months_for_financial_year(year):
         monthly_sms = [row for row in sms if row['month'] == month]
-        previous_cumulative = cumulative
-        monthly_chargeable_units = sum(row['billing_units'] for row in monthly_sms)
-        cumulative += monthly_chargeable_units
-        sms_breakdown = get_free_paid_breakdown_for_month(
-            free_sms_fragment_limit, cumulative, previous_cumulative, monthly_sms
-        )
+        sms_charged = sum(row['sms_charged'] for row in monthly_sms)
+        sms_free_allowance_used = sum(row['sms_free_allowance_used'] for row in monthly_sms)
+        sms_cost = sum(row['sms_cost'] for row in monthly_sms)
+        # makes the assumption that there is either no item in `monthly_sms` because they
+        # have not sent any SMS or that they have sent SMS and that there is only a single
+        # item in `monthly_sms` because they have only been sent at a single rate
+        sms_rate = monthly_sms[0]['rate'] if len(monthly_sms) else 0
 
         monthly_letters = [row for row in letters if row['month'] == month]
         letter_breakdown = format_letter_details_for_month(monthly_letters)
@@ -422,9 +418,10 @@ def get_monthly_usage_breakdown(year, free_sms_fragment_limit, monthly_usage):
             'month': month,
             'letter_cost': letter_cost,
             'letter_breakdown': letter_breakdown,
-            'sms_charged': sms_breakdown['paid'],
-            'sms_free_allowance_used': sms_breakdown['free'],
-            'sms_rate': sms_breakdown['sms_rate'],
+            'sms_charged': sms_charged,
+            'sms_free_allowance_used': sms_free_allowance_used,
+            'sms_rate': sms_rate,
+            'sms_cost': sms_cost,
         }
 
 
@@ -464,42 +461,6 @@ def get_postage_description(postage):
     return 'international'
 
 
-def get_free_paid_breakdown_for_month(
-    free_sms_fragment_limit,
-    cumulative,
-    previous_cumulative,
-    monthly_usage
-):
-    allowance = free_sms_fragment_limit
-
-    # makes the assumption that there is either no item in `monthly_usage` because they have not sent any SMS
-    # or that they have sent SMS and that there is only a single item in `monthly_usage` because they have only
-    # been sent at a single rate during the month
-    sms_rate = monthly_usage[0]['rate'] if len(monthly_usage) else 0
-
-    total_monthly_billing_units = sum(row['billing_units'] for row in monthly_usage)
-
-    if cumulative < allowance:
-        return {
-            'paid': 0,
-            'free': total_monthly_billing_units,
-            'sms_rate': sms_rate,
-        }
-    elif previous_cumulative < allowance:
-        remaining_allowance = allowance - previous_cumulative
-        return {
-            'paid': total_monthly_billing_units - remaining_allowance,
-            'free': remaining_allowance,
-            'sms_rate': sms_rate,
-        }
-    else:
-        return {
-            'paid': total_monthly_billing_units,
-            'free': 0,
-            'sms_rate': sms_rate,
-        }
-
-
 def requested_and_current_financial_year(request):
     try:
         return (
diff --git a/app/templates/views/usage.html b/app/templates/views/usage.html
index 59fd31979..16e04a3ac 100644
--- a/app/templates/views/usage.html
+++ b/app/templates/views/usage.html
@@ -99,7 +99,7 @@
             {% endcall %}
             {% call field(align='left') %}
               {{ big_number(
-                (item.sms_rate * item.sms_charged) + item.letter_cost,
+                item.sms_cost + item.letter_cost,
                 currency="£",
                 smallest=True
               ) }}
diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py
index befc8c08c..dde099c8f 100644
--- a/tests/app/main/views/test_dashboard.py
+++ b/tests/app/main/views/test_dashboard.py
@@ -1578,28 +1578,53 @@ def test_aggregate_status_types(dict_in, expected_failed, expected_requested):
     ]
 )
 def test_get_monthly_usage_breakdown(now, expected_number_of_months):
-    sms_allowance = 250000
     with now:
-        breakdown = get_monthly_usage_breakdown(
-            2016, sms_allowance, [
-                {
-                    'month': 'April', 'international': False, 'rate_multiplier': 1,
-                    'notification_type': 'sms', 'rate': 1.65, 'billing_units': 100000
-                },
-                {
-                    'month': 'May', 'international': False, 'rate_multiplier': 1,
-                    'notification_type': 'sms', 'rate': 1.65, 'billing_units': 100000
-                },
-                {
-                    'month': 'June', 'international': False, 'rate_multiplier': 1,
-                    'notification_type': 'sms', 'rate': 1.71, 'billing_units': 100000
-                },
-                {
-                    'month': 'February', 'international': False, 'rate_multiplier': 1,
-                    'notification_type': 'sms', 'rate': 1.71, 'billing_units': 2000
-                },
-            ]
-        )
+        breakdown = get_monthly_usage_breakdown(2016, [
+            {
+                'month': 'April',
+                'international': False,
+                'rate_multiplier': 1,
+                'notification_type': 'sms',
+                'rate': 1.65,
+                'billing_units': 100000,
+                'sms_charged': 0,
+                'sms_free_allowance_used': 100000,
+                'sms_cost': 0,
+            },
+            {
+                'month': 'May',
+                'international': False,
+                'rate_multiplier': 1,
+                'notification_type': 'sms',
+                'rate': 1.65,
+                'billing_units': 100000,
+                'sms_charged': 0,
+                'sms_free_allowance_used': 100000,
+                'sms_cost': 0,
+            },
+            {
+                'month': 'June',
+                'international': False,
+                'rate_multiplier': 1,
+                'notification_type': 'sms',
+                'rate': 1.71,
+                'billing_units': 100000,
+                'sms_charged': 50000,
+                'sms_free_allowance_used': 50000,
+                'sms_cost': 85500,
+            },
+            {
+                'month': 'February',
+                'international': False,
+                'rate_multiplier': 1,
+                'notification_type': 'sms',
+                'rate': 1.71,
+                'billing_units': 2000,
+                'sms_charged': 2000,
+                'sms_free_allowance_used': 0,
+                'sms_cost': 3420,
+            },
+        ])
 
         assert list(breakdown) == [
             {
@@ -1608,7 +1633,8 @@ def test_get_monthly_usage_breakdown(now, expected_number_of_months):
                 'sms_charged': 0,
                 'sms_rate': 1.65,
                 'letter_cost': 0,
-                'letter_breakdown': []
+                'letter_breakdown': [],
+                'sms_cost': 0,
             },
             {
                 'sms_free_allowance_used': 100000,
@@ -1616,7 +1642,8 @@ def test_get_monthly_usage_breakdown(now, expected_number_of_months):
                 'sms_charged': 0,
                 'sms_rate': 1.65,
                 'letter_cost': 0,
-                'letter_breakdown': []
+                'letter_breakdown': [],
+                'sms_cost': 0,
             },
             {
                 'sms_free_allowance_used': 50000,
@@ -1624,7 +1651,8 @@ def test_get_monthly_usage_breakdown(now, expected_number_of_months):
                 'sms_charged': 50000,
                 'sms_rate': 1.71,
                 'letter_cost': 0,
-                'letter_breakdown': []
+                'letter_breakdown': [],
+                'sms_cost': 85500,
             },
             {
                 'sms_free_allowance_used': 0,
@@ -1632,7 +1660,8 @@ def test_get_monthly_usage_breakdown(now, expected_number_of_months):
                 'sms_charged': 0,
                 'sms_rate': 0,
                 'letter_cost': 0,
-                'letter_breakdown': []
+                'letter_breakdown': [],
+                'sms_cost': 0,
             },
             {
                 'sms_free_allowance_used': 0,
@@ -1640,7 +1669,8 @@ def test_get_monthly_usage_breakdown(now, expected_number_of_months):
                 'sms_charged': 0,
                 'sms_rate': 0,
                 'letter_cost': 0,
-                'letter_breakdown': []
+                'letter_breakdown': [],
+                'sms_cost': 0,
             },
             {
                 'sms_free_allowance_used': 0,
@@ -1648,7 +1678,8 @@ def test_get_monthly_usage_breakdown(now, expected_number_of_months):
                 'sms_charged': 0,
                 'sms_rate': 0,
                 'letter_cost': 0,
-                'letter_breakdown': []
+                'letter_breakdown': [],
+                'sms_cost': 0,
             },
             {
                 'sms_free_allowance_used': 0,
@@ -1656,7 +1687,8 @@ def test_get_monthly_usage_breakdown(now, expected_number_of_months):
                 'sms_charged': 0,
                 'sms_rate': 0,
                 'letter_cost': 0,
-                'letter_breakdown': []
+                'letter_breakdown': [],
+                'sms_cost': 0,
             },
             {
                 'sms_free_allowance_used': 0,
@@ -1664,7 +1696,8 @@ def test_get_monthly_usage_breakdown(now, expected_number_of_months):
                 'sms_charged': 0,
                 'sms_rate': 0,
                 'letter_cost': 0,
-                'letter_breakdown': []
+                'letter_breakdown': [],
+                'sms_cost': 0,
             },
             {
                 'sms_free_allowance_used': 0,
@@ -1672,7 +1705,8 @@ def test_get_monthly_usage_breakdown(now, expected_number_of_months):
                 'sms_charged': 0,
                 'sms_rate': 0,
                 'letter_cost': 0,
-                'letter_breakdown': []
+                'letter_breakdown': [],
+                'sms_cost': 0,
             },
             {
                 'sms_free_allowance_used': 0,
@@ -1680,7 +1714,8 @@ def test_get_monthly_usage_breakdown(now, expected_number_of_months):
                 'sms_charged': 0,
                 'sms_rate': 0,
                 'letter_cost': 0,
-                'letter_breakdown': []
+                'letter_breakdown': [],
+                'sms_cost': 0,
             },
             {
                 'sms_free_allowance_used': 0,
@@ -1688,7 +1723,8 @@ def test_get_monthly_usage_breakdown(now, expected_number_of_months):
                 'sms_charged': 2000,
                 'sms_rate': 1.71,
                 'letter_cost': 0,
-                'letter_breakdown': []
+                'letter_breakdown': [],
+                'sms_cost': 3420,
             },
             {
                 'sms_free_allowance_used': 0,
@@ -1696,7 +1732,8 @@ def test_get_monthly_usage_breakdown(now, expected_number_of_months):
                 'sms_charged': 0,
                 'sms_rate': 0,
                 'letter_cost': 0,
-                'letter_breakdown': []
+                'letter_breakdown': [],
+                'sms_cost': 0,
             },
         ][:expected_number_of_months]
 
diff --git a/tests/conftest.py b/tests/conftest.py
index a47b311a8..1d6940f6d 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -2364,6 +2364,9 @@ def mock_get_monthly_usage_for_service(mocker):
                 'rate': 0.017,
                 'billing_units': 1230,
                 'postage': 'none',
+                'sms_charged': 1230,
+                'sms_free_allowance_used': 0,
+                'sms_cost': 20.91,
             },
             {
                 'month': 'February',
@@ -2371,6 +2374,9 @@ def mock_get_monthly_usage_for_service(mocker):
                 'rate': 0.0165,
                 'billing_units': 1100,
                 'postage': 'none',
+                'sms_charged': 960,
+                'sms_free_allowance_used': 140,
+                'sms_cost': 15.84,
             },
             {
                 'month': 'February',
@@ -2378,6 +2384,9 @@ def mock_get_monthly_usage_for_service(mocker):
                 'rate': 0.31,
                 'billing_units': 10,
                 'postage': 'second',
+                'sms_charged': 0,
+                'sms_free_allowance_used': 0,
+                'sms_cost': 0,
             },
             {
                 'month': 'February',
@@ -2385,6 +2394,9 @@ def mock_get_monthly_usage_for_service(mocker):
                 'rate': 0.33,
                 'billing_units': 5,
                 'postage': 'first',
+                'sms_charged': 0,
+                'sms_free_allowance_used': 0,
+                'sms_cost': 0,
             },
             {
                 'month': 'February',
@@ -2392,6 +2404,9 @@ def mock_get_monthly_usage_for_service(mocker):
                 'rate': 0.84,
                 'billing_units': 3,
                 'postage': 'europe',
+                'sms_charged': 0,
+                'sms_free_allowance_used': 0,
+                'sms_cost': 0,
             },
             {
                 'month': 'February',
@@ -2399,6 +2414,9 @@ def mock_get_monthly_usage_for_service(mocker):
                 'rate': 0.84,
                 'billing_units': 7,
                 'postage': 'rest-of-world',
+                'sms_charged': 0,
+                'sms_free_allowance_used': 0,
+                'sms_cost': 0,
             },
             {
                 'month': 'April',
@@ -2406,6 +2424,9 @@ def mock_get_monthly_usage_for_service(mocker):
                 'rate': 0.017,
                 'billing_units': 249860,
                 'postage': 'none',
+                'sms_charged': 0,
+                'sms_free_allowance_used': 249860,
+                'sms_cost': 0,
             },
         ]
 

From c0ee24ff19d6e37323131e2eaf9866cd1896ee71 Mon Sep 17 00:00:00 2001
From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk>
Date: Tue, 3 May 2022 10:25:58 +0100
Subject: [PATCH 09/16] Migrate letter cost calculation to use API field

The values in the tests are calculated manually from other fields
in the mocked API response.
---
 app/main/views/dashboard.py            |   6 +-
 tests/app/main/views/test_dashboard.py | 101 ++++++++++++++++++++++---
 tests/conftest.py                      |   7 ++
 3 files changed, 99 insertions(+), 15 deletions(-)

diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py
index 2f4117bbe..e29727536 100644
--- a/app/main/views/dashboard.py
+++ b/app/main/views/dashboard.py
@@ -409,11 +409,9 @@ def get_monthly_usage_breakdown(year, monthly_usage):
         sms_rate = monthly_sms[0]['rate'] if len(monthly_sms) else 0
 
         monthly_letters = [row for row in letters if row['month'] == month]
+        letter_cost = sum(row['letter_cost'] for row in monthly_letters)
         letter_breakdown = format_letter_details_for_month(monthly_letters)
 
-        letter_cost = 0
-        for x in letter_breakdown:
-            letter_cost += x.cost
         yield {
             'month': month,
             'letter_cost': letter_cost,
@@ -446,7 +444,7 @@ def format_letter_details_for_month(letter_units_for_month):
         letter_details = LetterDetails(
             sent=sum(x['billing_units'] for x in rate_group),
             rate=rate_group[0]['rate'],
-            cost=(sum(x['billing_units'] for x in rate_group) * rate_group[0]['rate']),
+            cost=sum(x['letter_cost'] for x in rate_group),
             postage_description=rate_group[0]['postage']
         )
 
diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py
index dde099c8f..1a47d4ced 100644
--- a/tests/app/main/views/test_dashboard.py
+++ b/tests/app/main/views/test_dashboard.py
@@ -1102,13 +1102,56 @@ def test_usage_page_displays_letters_ordered_by_postage(
     mock_get_free_sms_fragment_limit
 ):
     monthly_usage = [
-        {'month': 'April', 'notification_type': 'letter', 'rate': 0.5, 'billing_units': 1, 'postage': 'second'},
-        {'month': 'April', 'notification_type': 'letter', 'rate': 1, 'billing_units': 1, 'postage': 'europe'},
-        {'month': 'April', 'notification_type': 'letter', 'rate': 1, 'billing_units': 2, 'postage': 'rest-of-word'},
-        {'month': 'April', 'notification_type': 'letter', 'rate': 1.5, 'billing_units': 7, 'postage': 'europe'},
-        {'month': 'April', 'notification_type': 'letter', 'rate': 0.3, 'billing_units': 3, 'postage': 'second'},
-        {'month': 'April', 'notification_type': 'letter', 'rate': 0.5, 'billing_units': 1, 'postage': 'first'},
+        {
+            'month': 'April',
+            'notification_type': 'letter',
+            'rate': 0.5,
+            'billing_units': 1,
+            'postage': 'second',
+            'letter_cost': 0.5,
+        },
+        {
+            'month': 'April',
+            'notification_type': 'letter',
+            'rate': 1,
+            'billing_units': 1,
+            'postage': 'europe',
+            'letter_cost': 1,
+        },
+        {
+            'month': 'April',
+            'notification_type': 'letter',
+            'rate': 1,
+            'billing_units': 2,
+            'postage': 'rest-of-word',
+            'letter_cost': 2,
+        },
+        {
+            'month': 'April',
+            'notification_type': 'letter',
+            'rate': 1.5,
+            'billing_units': 7,
+            'postage': 'europe',
+            'letter_cost': 11.5,
+        },
+        {
+            'month': 'April',
+            'notification_type': 'letter',
+            'rate': 0.3,
+            'billing_units': 3,
+            'postage': 'second',
+            'letter_cost': 0.9,
+        },
+        {
+            'month': 'April',
+            'notification_type': 'letter',
+            'rate': 0.5,
+            'billing_units': 1,
+            'postage': 'first',
+            'letter_cost': 0.5,
+        },
     ]
+
     mocker.patch('app.billing_api_client.get_monthly_usage_for_service', return_value=monthly_usage)
     service_one['permissions'].append('letter')
     page = client_request.get(
@@ -1136,12 +1179,48 @@ def test_usage_page_displays_letters_split_by_month_and_postage(
     mock_get_free_sms_fragment_limit
 ):
     billable_units_resp = [
-        {'month': 'April', 'notification_type': 'letter', 'rate': 0.5, 'billing_units': 1, 'postage': 'second'},
-        {'month': 'April', 'notification_type': 'letter', 'rate': 1, 'billing_units': 1, 'postage': 'europe'},
-        {'month': 'May', 'notification_type': 'letter', 'rate': 1, 'billing_units': 7, 'postage': 'europe'},
-        {'month': 'May', 'notification_type': 'letter', 'rate': 0.5, 'billing_units': 3, 'postage': 'second'},
-        {'month': 'May', 'notification_type': 'letter', 'rate': 0.7, 'billing_units': 1, 'postage': 'first'},
+        {
+            'month': 'April',
+            'notification_type': 'letter',
+            'rate': 0.5,
+            'billing_units': 1,
+            'postage': 'second',
+            'letter_cost': 0.5,
+        },
+        {
+            'month': 'April',
+            'notification_type': 'letter',
+            'rate': 1,
+            'billing_units': 1,
+            'postage': 'europe',
+            'letter_cost': 1,
+        },
+        {
+            'month': 'May',
+            'notification_type': 'letter',
+            'rate': 1,
+            'billing_units': 7,
+            'postage': 'europe',
+            'letter_cost': 7,
+        },
+        {
+            'month': 'May',
+            'notification_type': 'letter',
+            'rate': 0.5,
+            'billing_units': 3,
+            'postage': 'second',
+            'letter_cost': 1.5,
+        },
+        {
+            'month': 'May',
+            'notification_type': 'letter',
+            'rate': 0.7,
+            'billing_units': 1,
+            'postage': 'first',
+            'letter_cost': 0.7,
+        },
     ]
+
     mocker.patch('app.billing_api_client.get_monthly_usage_for_service', return_value=billable_units_resp)
     service_one['permissions'].append('letter')
     page = client_request.get(
diff --git a/tests/conftest.py b/tests/conftest.py
index 1d6940f6d..388805cc5 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -2367,6 +2367,7 @@ def mock_get_monthly_usage_for_service(mocker):
                 'sms_charged': 1230,
                 'sms_free_allowance_used': 0,
                 'sms_cost': 20.91,
+                'letter_cost': 0,
             },
             {
                 'month': 'February',
@@ -2377,6 +2378,7 @@ def mock_get_monthly_usage_for_service(mocker):
                 'sms_charged': 960,
                 'sms_free_allowance_used': 140,
                 'sms_cost': 15.84,
+                'letter_cost': 0,
             },
             {
                 'month': 'February',
@@ -2387,6 +2389,7 @@ def mock_get_monthly_usage_for_service(mocker):
                 'sms_charged': 0,
                 'sms_free_allowance_used': 0,
                 'sms_cost': 0,
+                'letter_cost': 3.1,
             },
             {
                 'month': 'February',
@@ -2397,6 +2400,7 @@ def mock_get_monthly_usage_for_service(mocker):
                 'sms_charged': 0,
                 'sms_free_allowance_used': 0,
                 'sms_cost': 0,
+                'letter_cost': 1.65,
             },
             {
                 'month': 'February',
@@ -2407,6 +2411,7 @@ def mock_get_monthly_usage_for_service(mocker):
                 'sms_charged': 0,
                 'sms_free_allowance_used': 0,
                 'sms_cost': 0,
+                'letter_cost': 2.52,
             },
             {
                 'month': 'February',
@@ -2417,6 +2422,7 @@ def mock_get_monthly_usage_for_service(mocker):
                 'sms_charged': 0,
                 'sms_free_allowance_used': 0,
                 'sms_cost': 0,
+                'letter_cost': 5.88,
             },
             {
                 'month': 'April',
@@ -2427,6 +2433,7 @@ def mock_get_monthly_usage_for_service(mocker):
                 'sms_charged': 0,
                 'sms_free_allowance_used': 249860,
                 'sms_cost': 0,
+                'letter_cost': 0,
             },
         ]
 

From fb7c1160469cad49438582acf1d7fce41676a0b8 Mon Sep 17 00:00:00 2001
From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk>
Date: Tue, 3 May 2022 10:33:04 +0100
Subject: [PATCH 10/16] Finish migration from billing_units to API fields

This is now only used for letters and represents the number sent
[^1].  We could use the chargeable_units field, but using "_sent"
is more consistent with the annual attributes [^2].

In fact, chargeable_units isn't actually used anywhere, but I've
kept it in the test data as it is part of the real API and helps
clarify the other values for SMS - free vs. charged.

Note: for SMS I've used an arbitrary "1234" for "chargeable_units"
to indicate it's not used and may be different to the number sent -
for SMS it's related to the number of fragments.

[^1]: https://github.com/alphagov/notifications-api/blob/bb62d22f2555db192ab9de66440409158524d790/app/dao/fact_billing_dao.py#L339
[^2]: https://github.com/alphagov/notifications-admin/blob/3a1ac189ff6800d4789b22e4763e5550719e8802/app/main/views/dashboard.py#L339
---
 app/main/views/dashboard.py            |  2 +-
 tests/app/main/views/test_dashboard.py | 45 +++++++++++++++++---------
 tests/conftest.py                      | 21 ++++++++----
 3 files changed, 45 insertions(+), 23 deletions(-)

diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py
index e29727536..9cbeba214 100644
--- a/app/main/views/dashboard.py
+++ b/app/main/views/dashboard.py
@@ -442,7 +442,7 @@ def format_letter_details_for_month(letter_units_for_month):
         rate_group = list(rate_group)
 
         letter_details = LetterDetails(
-            sent=sum(x['billing_units'] for x in rate_group),
+            sent=sum(x['notifications_sent'] for x in rate_group),
             rate=rate_group[0]['rate'],
             cost=sum(x['letter_cost'] for x in rate_group),
             postage_description=rate_group[0]['postage']
diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py
index 1a47d4ced..2e48c1291 100644
--- a/tests/app/main/views/test_dashboard.py
+++ b/tests/app/main/views/test_dashboard.py
@@ -1106,7 +1106,8 @@ def test_usage_page_displays_letters_ordered_by_postage(
             'month': 'April',
             'notification_type': 'letter',
             'rate': 0.5,
-            'billing_units': 1,
+            'chargeable_units': 1,
+            'notifications_sent': 1,
             'postage': 'second',
             'letter_cost': 0.5,
         },
@@ -1114,7 +1115,8 @@ def test_usage_page_displays_letters_ordered_by_postage(
             'month': 'April',
             'notification_type': 'letter',
             'rate': 1,
-            'billing_units': 1,
+            'chargeable_units': 1,
+            'notifications_sent': 1,
             'postage': 'europe',
             'letter_cost': 1,
         },
@@ -1122,7 +1124,8 @@ def test_usage_page_displays_letters_ordered_by_postage(
             'month': 'April',
             'notification_type': 'letter',
             'rate': 1,
-            'billing_units': 2,
+            'chargeable_units': 2,
+            'notifications_sent': 2,
             'postage': 'rest-of-word',
             'letter_cost': 2,
         },
@@ -1130,7 +1133,8 @@ def test_usage_page_displays_letters_ordered_by_postage(
             'month': 'April',
             'notification_type': 'letter',
             'rate': 1.5,
-            'billing_units': 7,
+            'chargeable_units': 7,
+            'notifications_sent': 7,
             'postage': 'europe',
             'letter_cost': 11.5,
         },
@@ -1138,7 +1142,8 @@ def test_usage_page_displays_letters_ordered_by_postage(
             'month': 'April',
             'notification_type': 'letter',
             'rate': 0.3,
-            'billing_units': 3,
+            'chargeable_units': 3,
+            'notifications_sent': 3,
             'postage': 'second',
             'letter_cost': 0.9,
         },
@@ -1146,7 +1151,8 @@ def test_usage_page_displays_letters_ordered_by_postage(
             'month': 'April',
             'notification_type': 'letter',
             'rate': 0.5,
-            'billing_units': 1,
+            'chargeable_units': 1,
+            'notifications_sent': 1,
             'postage': 'first',
             'letter_cost': 0.5,
         },
@@ -1183,7 +1189,8 @@ def test_usage_page_displays_letters_split_by_month_and_postage(
             'month': 'April',
             'notification_type': 'letter',
             'rate': 0.5,
-            'billing_units': 1,
+            'chargeable_units': 1,
+            'notifications_sent': 1,
             'postage': 'second',
             'letter_cost': 0.5,
         },
@@ -1191,7 +1198,8 @@ def test_usage_page_displays_letters_split_by_month_and_postage(
             'month': 'April',
             'notification_type': 'letter',
             'rate': 1,
-            'billing_units': 1,
+            'chargeable_units': 1,
+            'notifications_sent': 1,
             'postage': 'europe',
             'letter_cost': 1,
         },
@@ -1199,7 +1207,8 @@ def test_usage_page_displays_letters_split_by_month_and_postage(
             'month': 'May',
             'notification_type': 'letter',
             'rate': 1,
-            'billing_units': 7,
+            'chargeable_units': 7,
+            'notifications_sent': 7,
             'postage': 'europe',
             'letter_cost': 7,
         },
@@ -1207,7 +1216,8 @@ def test_usage_page_displays_letters_split_by_month_and_postage(
             'month': 'May',
             'notification_type': 'letter',
             'rate': 0.5,
-            'billing_units': 3,
+            'chargeable_units': 3,
+            'notifications_sent': 3,
             'postage': 'second',
             'letter_cost': 1.5,
         },
@@ -1215,7 +1225,8 @@ def test_usage_page_displays_letters_split_by_month_and_postage(
             'month': 'May',
             'notification_type': 'letter',
             'rate': 0.7,
-            'billing_units': 1,
+            'chargeable_units': 1,
+            'notifications_sent': 1,
             'postage': 'first',
             'letter_cost': 0.7,
         },
@@ -1665,7 +1676,8 @@ def test_get_monthly_usage_breakdown(now, expected_number_of_months):
                 'rate_multiplier': 1,
                 'notification_type': 'sms',
                 'rate': 1.65,
-                'billing_units': 100000,
+                'chargeable_units': 100000,
+                'notifications_sent': 1234,
                 'sms_charged': 0,
                 'sms_free_allowance_used': 100000,
                 'sms_cost': 0,
@@ -1676,7 +1688,8 @@ def test_get_monthly_usage_breakdown(now, expected_number_of_months):
                 'rate_multiplier': 1,
                 'notification_type': 'sms',
                 'rate': 1.65,
-                'billing_units': 100000,
+                'chargeable_units': 100000,
+                'notifications_sent': 1234,
                 'sms_charged': 0,
                 'sms_free_allowance_used': 100000,
                 'sms_cost': 0,
@@ -1687,7 +1700,8 @@ def test_get_monthly_usage_breakdown(now, expected_number_of_months):
                 'rate_multiplier': 1,
                 'notification_type': 'sms',
                 'rate': 1.71,
-                'billing_units': 100000,
+                'chargeable_units': 100000,
+                'notifications_sent': 1234,
                 'sms_charged': 50000,
                 'sms_free_allowance_used': 50000,
                 'sms_cost': 85500,
@@ -1698,7 +1712,8 @@ def test_get_monthly_usage_breakdown(now, expected_number_of_months):
                 'rate_multiplier': 1,
                 'notification_type': 'sms',
                 'rate': 1.71,
-                'billing_units': 2000,
+                'chargeable_units': 2000,
+                'notifications_sent': 1234,
                 'sms_charged': 2000,
                 'sms_free_allowance_used': 0,
                 'sms_cost': 3420,
diff --git a/tests/conftest.py b/tests/conftest.py
index 388805cc5..1ae650b5a 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -2362,7 +2362,8 @@ def mock_get_monthly_usage_for_service(mocker):
                 'month': 'March',
                 'notification_type': 'sms',
                 'rate': 0.017,
-                'billing_units': 1230,
+                'chargeable_units': 1230,
+                'notifications_sent': 1234,
                 'postage': 'none',
                 'sms_charged': 1230,
                 'sms_free_allowance_used': 0,
@@ -2373,7 +2374,8 @@ def mock_get_monthly_usage_for_service(mocker):
                 'month': 'February',
                 'notification_type': 'sms',
                 'rate': 0.0165,
-                'billing_units': 1100,
+                'chargeable_units': 1100,
+                'notifications_sent': 1234,
                 'postage': 'none',
                 'sms_charged': 960,
                 'sms_free_allowance_used': 140,
@@ -2384,7 +2386,8 @@ def mock_get_monthly_usage_for_service(mocker):
                 'month': 'February',
                 'notification_type': 'letter',
                 'rate': 0.31,
-                'billing_units': 10,
+                'chargeable_units': 10,
+                'notifications_sent': 10,
                 'postage': 'second',
                 'sms_charged': 0,
                 'sms_free_allowance_used': 0,
@@ -2395,7 +2398,8 @@ def mock_get_monthly_usage_for_service(mocker):
                 'month': 'February',
                 'notification_type': 'letter',
                 'rate': 0.33,
-                'billing_units': 5,
+                'chargeable_units': 5,
+                'notifications_sent': 5,
                 'postage': 'first',
                 'sms_charged': 0,
                 'sms_free_allowance_used': 0,
@@ -2406,7 +2410,8 @@ def mock_get_monthly_usage_for_service(mocker):
                 'month': 'February',
                 'notification_type': 'letter',
                 'rate': 0.84,
-                'billing_units': 3,
+                'chargeable_units': 3,
+                'notifications_sent': 3,
                 'postage': 'europe',
                 'sms_charged': 0,
                 'sms_free_allowance_used': 0,
@@ -2417,7 +2422,8 @@ def mock_get_monthly_usage_for_service(mocker):
                 'month': 'February',
                 'notification_type': 'letter',
                 'rate': 0.84,
-                'billing_units': 7,
+                'chargeable_units': 7,
+                'notifications_sent': 7,
                 'postage': 'rest-of-world',
                 'sms_charged': 0,
                 'sms_free_allowance_used': 0,
@@ -2428,7 +2434,8 @@ def mock_get_monthly_usage_for_service(mocker):
                 'month': 'April',
                 'notification_type': 'sms',
                 'rate': 0.017,
-                'billing_units': 249860,
+                'chargeable_units': 249860,
+                'notifications_sent': 1234,
                 'postage': 'none',
                 'sms_charged': 0,
                 'sms_free_allowance_used': 249860,

From 91e902bc2cf58315a5df6471e1c133ce0d6d08f6 Mon Sep 17 00:00:00 2001
From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk>
Date: Tue, 3 May 2022 12:36:09 +0100
Subject: [PATCH 11/16] Tidy up "format" method for monthly letter usage

This did more than it said and had some unconventional behaviour:

- It modified the input data. We can avoid this by computing the
postage group on-the-fly and using "sorted" instead of "sort".

- It defined a custom, named tuple. This isn't necessary as Jinja
allows us to access elements by qualification (".") already.

We can also use the same lambda function to group and sort items,
since the sort predicate is the same one we use to group them.
---
 app/main/views/dashboard.py | 49 ++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 30 deletions(-)

diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py
index 9cbeba214..9d0653183 100644
--- a/app/main/views/dashboard.py
+++ b/app/main/views/dashboard.py
@@ -1,9 +1,7 @@
 import calendar
-from collections import namedtuple
 from datetime import datetime
 from functools import partial
 from itertools import groupby
-from operator import itemgetter
 
 from flask import (
     Response,
@@ -410,12 +408,12 @@ def get_monthly_usage_breakdown(year, monthly_usage):
 
         monthly_letters = [row for row in letters if row['month'] == month]
         letter_cost = sum(row['letter_cost'] for row in monthly_letters)
-        letter_breakdown = format_letter_details_for_month(monthly_letters)
+        letter_breakdown = get_monthly_usage_breakdown_for_letters(monthly_letters)
 
         yield {
             'month': month,
             'letter_cost': letter_cost,
-            'letter_breakdown': letter_breakdown,
+            'letter_breakdown': list(letter_breakdown),
             'sms_charged': sms_charged,
             'sms_free_allowance_used': sms_free_allowance_used,
             'sms_rate': sms_rate,
@@ -423,39 +421,30 @@ def get_monthly_usage_breakdown(year, monthly_usage):
         }
 
 
-def format_letter_details_for_month(letter_units_for_month):
-    # Format postage descriptions in letter units e.g. to 'international' not 'europe'
-    for month in letter_units_for_month:
-        for k, v in month.items():
-            if k == 'postage':
-                month[k] = get_postage_description(v)
-
-    # letter_units_for_month must be sorted before international postage values can be aggregated
+def get_monthly_usage_breakdown_for_letters(monthly_letters):
     postage_order = {'first class': 0, 'second class': 1, 'international': 2}
-    letter_units_for_month.sort(key=lambda x: (postage_order[x['postage']], x['rate']))
 
-    LetterDetails = namedtuple('LetterDetails', ['sent', 'rate', 'cost', 'postage_description'])
+    group_key = lambda row: (  # noqa: E731
+        postage_order[get_monthly_usage_postage_description(row)], row['rate']
+    )
 
-    # Aggregate the rows for international letters which have the same rate
-    result = []
-    for _key, rate_group in groupby(letter_units_for_month, key=itemgetter('postage', 'rate')):
+    rate_groups = groupby(sorted(monthly_letters, key=group_key), key=group_key)
+
+    for _key, rate_group in rate_groups:
+        # rate_group is a one-time generator so must be converted to a list for reuse
         rate_group = list(rate_group)
 
-        letter_details = LetterDetails(
-            sent=sum(x['notifications_sent'] for x in rate_group),
-            rate=rate_group[0]['rate'],
-            cost=sum(x['letter_cost'] for x in rate_group),
-            postage_description=rate_group[0]['postage']
-        )
-
-        result.append(letter_details)
-
-    return result
+        yield {
+            "sent": sum(x['notifications_sent'] for x in rate_group),
+            "rate": rate_group[0]['rate'],
+            "cost": sum(x['letter_cost'] for x in rate_group),
+            "postage_description": get_monthly_usage_postage_description(rate_group[0])
+        }
 
 
-def get_postage_description(postage):
-    if postage in ('first', 'second'):
-        return f'{postage} class'
+def get_monthly_usage_postage_description(row):
+    if row['postage'] in ('first', 'second'):
+        return f'{row["postage"]} class'
     return 'international'
 
 

From 63c35ec3d9a4b528ab86b7a7348f947754ec632a Mon Sep 17 00:00:00 2001
From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk>
Date: Tue, 3 May 2022 12:45:19 +0100
Subject: [PATCH 12/16] Remove redundant test for letter usage breakdown

This is covered sufficiently by the main "test_usage" assertions,
which prove the usage is broken down by postage.

I don't think we need to explicitly test the usage is broken down
by month as we already prove this for SMS and we also check the
usage is associated with the correct month in the "ordering" test.
---
 tests/app/main/views/test_dashboard.py | 73 --------------------------
 1 file changed, 73 deletions(-)

diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py
index 2e48c1291..a6b5a0e5e 100644
--- a/tests/app/main/views/test_dashboard.py
+++ b/tests/app/main/views/test_dashboard.py
@@ -1176,79 +1176,6 @@ def test_usage_page_displays_letters_ordered_by_postage(
     assert normalize_spaces(postage_details[4].text) == '7 international letters at £1.50'
 
 
-@freeze_time("2012-07-30 12:12:12")
-def test_usage_page_displays_letters_split_by_month_and_postage(
-    mocker,
-    client_request,
-    service_one,
-    mock_get_annual_usage_for_service,
-    mock_get_free_sms_fragment_limit
-):
-    billable_units_resp = [
-        {
-            'month': 'April',
-            'notification_type': 'letter',
-            'rate': 0.5,
-            'chargeable_units': 1,
-            'notifications_sent': 1,
-            'postage': 'second',
-            'letter_cost': 0.5,
-        },
-        {
-            'month': 'April',
-            'notification_type': 'letter',
-            'rate': 1,
-            'chargeable_units': 1,
-            'notifications_sent': 1,
-            'postage': 'europe',
-            'letter_cost': 1,
-        },
-        {
-            'month': 'May',
-            'notification_type': 'letter',
-            'rate': 1,
-            'chargeable_units': 7,
-            'notifications_sent': 7,
-            'postage': 'europe',
-            'letter_cost': 7,
-        },
-        {
-            'month': 'May',
-            'notification_type': 'letter',
-            'rate': 0.5,
-            'chargeable_units': 3,
-            'notifications_sent': 3,
-            'postage': 'second',
-            'letter_cost': 1.5,
-        },
-        {
-            'month': 'May',
-            'notification_type': 'letter',
-            'rate': 0.7,
-            'chargeable_units': 1,
-            'notifications_sent': 1,
-            'postage': 'first',
-            'letter_cost': 0.7,
-        },
-    ]
-
-    mocker.patch('app.billing_api_client.get_monthly_usage_for_service', return_value=billable_units_resp)
-    service_one['permissions'].append('letter')
-    page = client_request.get(
-        'main.usage',
-        service_id=SERVICE_ONE_ID,
-    )
-
-    april_row = normalize_spaces(page.find('table').find_all('tr')[1].text)
-    may_row = normalize_spaces(page.find('table').find_all('tr')[2].text)
-
-    assert '1 second class letter at 50p' in april_row
-    assert '1 international letter at £1.00' in april_row
-    assert '1 first class letter at 70p' in may_row
-    assert '3 second class letters at 50p' in may_row
-    assert '7 international letters at £1.00' in may_row
-
-
 def test_usage_page_with_0_free_allowance(
     mocker,
     client_request,

From c05502835b0201f1856b61871dab5e1f10fdd17b Mon Sep 17 00:00:00 2001
From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk>
Date: Tue, 3 May 2022 13:01:06 +0100
Subject: [PATCH 13/16] Simplify monthly letter breakdown ordering test

This doesn't need to test variable rates for every postage class,
which is more an aspect of grouping. It only needs to check that
some out-of-order usage gets reordered appropriately.
---
 tests/app/main/views/test_dashboard.py | 84 +++-----------------------
 1 file changed, 9 insertions(+), 75 deletions(-)

diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py
index a6b5a0e5e..9d28284ae 100644
--- a/tests/app/main/views/test_dashboard.py
+++ b/tests/app/main/views/test_dashboard.py
@@ -1093,87 +1093,21 @@ def test_usage_page_monthly_breakdown(
     assert '1,230 text messages at 1.70p' in monthly_breakdown
 
 
-@freeze_time("2012-04-30 12:12:12")
-def test_usage_page_displays_letters_ordered_by_postage(
-    mocker,
+@freeze_time("2012-03-31 12:12:12")
+def test_usage_page_letter_breakdown_ordering(
     client_request,
     service_one,
+    mock_get_monthly_usage_for_service,
     mock_get_annual_usage_for_service,
     mock_get_free_sms_fragment_limit
 ):
-    monthly_usage = [
-        {
-            'month': 'April',
-            'notification_type': 'letter',
-            'rate': 0.5,
-            'chargeable_units': 1,
-            'notifications_sent': 1,
-            'postage': 'second',
-            'letter_cost': 0.5,
-        },
-        {
-            'month': 'April',
-            'notification_type': 'letter',
-            'rate': 1,
-            'chargeable_units': 1,
-            'notifications_sent': 1,
-            'postage': 'europe',
-            'letter_cost': 1,
-        },
-        {
-            'month': 'April',
-            'notification_type': 'letter',
-            'rate': 1,
-            'chargeable_units': 2,
-            'notifications_sent': 2,
-            'postage': 'rest-of-word',
-            'letter_cost': 2,
-        },
-        {
-            'month': 'April',
-            'notification_type': 'letter',
-            'rate': 1.5,
-            'chargeable_units': 7,
-            'notifications_sent': 7,
-            'postage': 'europe',
-            'letter_cost': 11.5,
-        },
-        {
-            'month': 'April',
-            'notification_type': 'letter',
-            'rate': 0.3,
-            'chargeable_units': 3,
-            'notifications_sent': 3,
-            'postage': 'second',
-            'letter_cost': 0.9,
-        },
-        {
-            'month': 'April',
-            'notification_type': 'letter',
-            'rate': 0.5,
-            'chargeable_units': 1,
-            'notifications_sent': 1,
-            'postage': 'first',
-            'letter_cost': 0.5,
-        },
-    ]
+    page = client_request.get('main.usage', service_id=SERVICE_ONE_ID)
+    row_for_feb = page.find('table').find_all('tr', class_='table-row')[10]
+    postage_details = row_for_feb.find_all('li', class_='tabular-numbers')
 
-    mocker.patch('app.billing_api_client.get_monthly_usage_for_service', return_value=monthly_usage)
-    service_one['permissions'].append('letter')
-    page = client_request.get(
-        'main.usage',
-        service_id=SERVICE_ONE_ID,
-    )
-
-    row_for_april = page.find('table').find('tr', class_='table-row')
-    postage_details = row_for_april.find_all('li', class_='tabular-numbers')
-
-    assert len(postage_details) == 5
-    assert normalize_spaces(postage_details[0].text) == '1 first class letter at 50p'
-    assert normalize_spaces(postage_details[1].text) == '3 second class letters at 30p'
-    assert normalize_spaces(postage_details[2].text) == '1 second class letter at 50p'
-    assert normalize_spaces(postage_details[3].text) == '3 international letters at £1.00'
-    assert normalize_spaces(postage_details[4].text) == '7 international letters at £1.50'
+    assert normalize_spaces(postage_details[2].text) == '5 first class letters at 33p'
+    assert normalize_spaces(postage_details[3].text) == '10 second class letters at 31p'
+    assert normalize_spaces(postage_details[4].text) == '10 international letters at 84p'
 
 
 def test_usage_page_with_0_free_allowance(

From 75e0d53e766df2a1c28665fb295a956a4947c56e Mon Sep 17 00:00:00 2001
From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk>
Date: Tue, 3 May 2022 15:24:13 +0100
Subject: [PATCH 14/16] Simplify test for only showing historic months

This goal wasn't clear from the original test, which was checking
the entire return value, even though this is covered implicitly by
tests of the usage page itself.
---
 tests/app/main/views/test_dashboard.py | 193 +++----------------------
 1 file changed, 21 insertions(+), 172 deletions(-)

diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py
index 9d28284ae..c2910157c 100644
--- a/tests/app/main/views/test_dashboard.py
+++ b/tests/app/main/views/test_dashboard.py
@@ -12,7 +12,6 @@ from app.main.views.dashboard import (
     aggregate_template_usage,
     format_monthly_stats_to_list,
     get_dashboard_totals,
-    get_monthly_usage_breakdown,
     get_tuples_of_financial_years,
 )
 from tests import (
@@ -1093,6 +1092,27 @@ def test_usage_page_monthly_breakdown(
     assert '1,230 text messages at 1.70p' in monthly_breakdown
 
 
+@pytest.mark.parametrize(
+    'now, expected_number_of_months', [
+        (freeze_time("2017-03-31 11:09:00.061258"), 12),
+        (freeze_time("2017-01-01 11:09:00.061258"), 10)
+    ]
+)
+def test_usage_page_monthly_breakdown_shows_months_so_far(
+    client_request,
+    service_one,
+    mock_get_annual_usage_for_service,
+    mock_get_monthly_usage_for_service,
+    mock_get_free_sms_fragment_limit,
+    now,
+    expected_number_of_months
+):
+    with now:
+        page = client_request.get('main.usage', service_id=SERVICE_ONE_ID)
+        rows = page.find('table').find_all('tr', class_='table-row')
+        assert len(rows) == expected_number_of_months
+
+
 @freeze_time("2012-03-31 12:12:12")
 def test_usage_page_letter_breakdown_ordering(
     client_request,
@@ -1522,177 +1542,6 @@ def test_aggregate_status_types(dict_in, expected_failed, expected_requested):
     assert sms_counts['requested'] == expected_requested
 
 
-@pytest.mark.parametrize(
-    'now, expected_number_of_months', [
-        (freeze_time("2017-12-31 11:09:00.061258"), 12),
-        (freeze_time("2017-01-01 11:09:00.061258"), 10)
-    ]
-)
-def test_get_monthly_usage_breakdown(now, expected_number_of_months):
-    with now:
-        breakdown = get_monthly_usage_breakdown(2016, [
-            {
-                'month': 'April',
-                'international': False,
-                'rate_multiplier': 1,
-                'notification_type': 'sms',
-                'rate': 1.65,
-                'chargeable_units': 100000,
-                'notifications_sent': 1234,
-                'sms_charged': 0,
-                'sms_free_allowance_used': 100000,
-                'sms_cost': 0,
-            },
-            {
-                'month': 'May',
-                'international': False,
-                'rate_multiplier': 1,
-                'notification_type': 'sms',
-                'rate': 1.65,
-                'chargeable_units': 100000,
-                'notifications_sent': 1234,
-                'sms_charged': 0,
-                'sms_free_allowance_used': 100000,
-                'sms_cost': 0,
-            },
-            {
-                'month': 'June',
-                'international': False,
-                'rate_multiplier': 1,
-                'notification_type': 'sms',
-                'rate': 1.71,
-                'chargeable_units': 100000,
-                'notifications_sent': 1234,
-                'sms_charged': 50000,
-                'sms_free_allowance_used': 50000,
-                'sms_cost': 85500,
-            },
-            {
-                'month': 'February',
-                'international': False,
-                'rate_multiplier': 1,
-                'notification_type': 'sms',
-                'rate': 1.71,
-                'chargeable_units': 2000,
-                'notifications_sent': 1234,
-                'sms_charged': 2000,
-                'sms_free_allowance_used': 0,
-                'sms_cost': 3420,
-            },
-        ])
-
-        assert list(breakdown) == [
-            {
-                'sms_free_allowance_used': 100000,
-                'month': 'April',
-                'sms_charged': 0,
-                'sms_rate': 1.65,
-                'letter_cost': 0,
-                'letter_breakdown': [],
-                'sms_cost': 0,
-            },
-            {
-                'sms_free_allowance_used': 100000,
-                'month': 'May',
-                'sms_charged': 0,
-                'sms_rate': 1.65,
-                'letter_cost': 0,
-                'letter_breakdown': [],
-                'sms_cost': 0,
-            },
-            {
-                'sms_free_allowance_used': 50000,
-                'month': 'June',
-                'sms_charged': 50000,
-                'sms_rate': 1.71,
-                'letter_cost': 0,
-                'letter_breakdown': [],
-                'sms_cost': 85500,
-            },
-            {
-                'sms_free_allowance_used': 0,
-                'month': 'July',
-                'sms_charged': 0,
-                'sms_rate': 0,
-                'letter_cost': 0,
-                'letter_breakdown': [],
-                'sms_cost': 0,
-            },
-            {
-                'sms_free_allowance_used': 0,
-                'month': 'August',
-                'sms_charged': 0,
-                'sms_rate': 0,
-                'letter_cost': 0,
-                'letter_breakdown': [],
-                'sms_cost': 0,
-            },
-            {
-                'sms_free_allowance_used': 0,
-                'month': 'September',
-                'sms_charged': 0,
-                'sms_rate': 0,
-                'letter_cost': 0,
-                'letter_breakdown': [],
-                'sms_cost': 0,
-            },
-            {
-                'sms_free_allowance_used': 0,
-                'month': 'October',
-                'sms_charged': 0,
-                'sms_rate': 0,
-                'letter_cost': 0,
-                'letter_breakdown': [],
-                'sms_cost': 0,
-            },
-            {
-                'sms_free_allowance_used': 0,
-                'month': 'November',
-                'sms_charged': 0,
-                'sms_rate': 0,
-                'letter_cost': 0,
-                'letter_breakdown': [],
-                'sms_cost': 0,
-            },
-            {
-                'sms_free_allowance_used': 0,
-                'month': 'December',
-                'sms_charged': 0,
-                'sms_rate': 0,
-                'letter_cost': 0,
-                'letter_breakdown': [],
-                'sms_cost': 0,
-            },
-            {
-                'sms_free_allowance_used': 0,
-                'month': 'January',
-                'sms_charged': 0,
-                'sms_rate': 0,
-                'letter_cost': 0,
-                'letter_breakdown': [],
-                'sms_cost': 0,
-            },
-            {
-                'sms_free_allowance_used': 0,
-                'month': 'February',
-                'sms_charged': 2000,
-                'sms_rate': 1.71,
-                'letter_cost': 0,
-                'letter_breakdown': [],
-                'sms_cost': 3420,
-            },
-            {
-                'sms_free_allowance_used': 0,
-                'month': 'March',
-                'sms_charged': 0,
-                'sms_rate': 0,
-                'letter_cost': 0,
-                'letter_breakdown': [],
-                'sms_cost': 0,
-            },
-        ][:expected_number_of_months]
-
-
 def test_get_tuples_of_financial_years():
     assert list(get_tuples_of_financial_years(
         lambda year: 'http://example.com?year={}'.format(year),

From 3449ccd923e8c1929a340517a8aa68e81503be20 Mon Sep 17 00:00:00 2001
From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk>
Date: Tue, 3 May 2022 13:13:29 +0100
Subject: [PATCH 15/16] Support multiple SMS rates per month on usage page

---
 app/main/views/dashboard.py            |  9 ++-------
 app/templates/views/usage.html         | 10 +++++-----
 tests/app/main/views/test_dashboard.py | 16 ++++++----------
 tests/conftest.py                      | 12 ++++++++++++
 4 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py
index 9d0653183..46520f8b5 100644
--- a/app/main/views/dashboard.py
+++ b/app/main/views/dashboard.py
@@ -398,13 +398,9 @@ def get_monthly_usage_breakdown(year, monthly_usage):
 
     for month in get_months_for_financial_year(year):
         monthly_sms = [row for row in sms if row['month'] == month]
-        sms_charged = sum(row['sms_charged'] for row in monthly_sms)
         sms_free_allowance_used = sum(row['sms_free_allowance_used'] for row in monthly_sms)
         sms_cost = sum(row['sms_cost'] for row in monthly_sms)
-        # makes the assumption that there is either no item in `monthly_sms` because they
-        # have not sent any SMS or that they have sent SMS and that there is only a single
-        # item in `monthly_sms` because they have only been sent at a single rate
-        sms_rate = monthly_sms[0]['rate'] if len(monthly_sms) else 0
+        sms_breakdown = [row for row in monthly_sms if row['sms_charged']]
 
         monthly_letters = [row for row in letters if row['month'] == month]
         letter_cost = sum(row['letter_cost'] for row in monthly_letters)
@@ -414,9 +410,8 @@ def get_monthly_usage_breakdown(year, monthly_usage):
             'month': month,
             'letter_cost': letter_cost,
             'letter_breakdown': list(letter_breakdown),
-            'sms_charged': sms_charged,
             'sms_free_allowance_used': sms_free_allowance_used,
-            'sms_rate': sms_rate,
+            'sms_breakdown': sms_breakdown,
             'sms_cost': sms_cost,
         }
 
diff --git a/app/templates/views/usage.html b/app/templates/views/usage.html
index 16e04a3ac..c0def3336 100644
--- a/app/templates/views/usage.html
+++ b/app/templates/views/usage.html
@@ -107,17 +107,17 @@
               {% if item.sms_free_allowance_used %}
                 <li class="tabular-numbers">{{ item.sms_free_allowance_used|format_thousands }} free {{ item.sms_free_count|message_count_label('sms', suffix='') }}</li>
               {% endif %}
-              {% if item.sms_charged %}
-                <li class="tabular-numbers">{{ item.sms_charged|message_count('sms') }} at
-                {{- ' {:.2f}p'.format(item.sms_rate * 100) }}</li>
-              {% endif %}
+              {% for sms in item.sms_breakdown %}
+                  <li class="tabular-numbers">{{ sms.sms_charged|message_count('sms') }} at
+                  {{- ' {:.2f}p'.format(sms.rate * 100) }}</li>
+              {% endfor %}
               {% for letter in item.letter_breakdown %}
                 {% if letter.sent %}
                   <li class="tabular-numbers">{{ "{:,} {}".format(letter.sent, letter.postage_description) }} {{ letter.sent|message_count_label('letter', suffix='') }} at
                   {{ letter.rate | format_number_in_pounds_as_currency }}</li>
                 {% endif %}
               {% endfor %}
-              {% if not (item.sms_free_allowance_used or item.sms_charged or item.letter_breakdown) %}
+              {% if not (item.sms_free_allowance_used or item.sms_cost or item.letter_breakdown) %}
                 <li aria-hidden="true">–</li>
               {% endif %}
               </ul>
diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py
index c2910157c..a0968c673 100644
--- a/tests/app/main/views/test_dashboard.py
+++ b/tests/app/main/views/test_dashboard.py
@@ -1068,21 +1068,17 @@ def test_usage_page_monthly_breakdown(
     mock_get_monthly_usage_for_service,
     mock_get_free_sms_fragment_limit
 ):
-    service_one['permissions'].append('letter')
-    page = client_request.get(
-        'main.usage',
-        service_id=SERVICE_ONE_ID,
-    )
-
+    page = client_request.get('main.usage', service_id=SERVICE_ONE_ID)
     monthly_breakdown = normalize_spaces(page.find('table').text)
 
     assert 'April' in monthly_breakdown
     assert '249,860 free text messages' in monthly_breakdown
 
     assert 'February' in monthly_breakdown
-    assert '£28.99' in monthly_breakdown
+    assert '£29.55' in monthly_breakdown
     assert '140 free text messages' in monthly_breakdown
     assert '960 text messages at 1.65p' in monthly_breakdown
+    assert '33 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
@@ -1125,9 +1121,9 @@ def test_usage_page_letter_breakdown_ordering(
     row_for_feb = page.find('table').find_all('tr', class_='table-row')[10]
     postage_details = row_for_feb.find_all('li', class_='tabular-numbers')
 
-    assert normalize_spaces(postage_details[2].text) == '5 first class letters at 33p'
-    assert normalize_spaces(postage_details[3].text) == '10 second class letters at 31p'
-    assert normalize_spaces(postage_details[4].text) == '10 international letters at 84p'
+    assert normalize_spaces(postage_details[3].text) == '5 first class letters at 33p'
+    assert normalize_spaces(postage_details[4].text) == '10 second class letters at 31p'
+    assert normalize_spaces(postage_details[5].text) == '10 international letters at 84p'
 
 
 def test_usage_page_with_0_free_allowance(
diff --git a/tests/conftest.py b/tests/conftest.py
index 1ae650b5a..569fcc7cc 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -2370,6 +2370,18 @@ def mock_get_monthly_usage_for_service(mocker):
                 'sms_cost': 20.91,
                 'letter_cost': 0,
             },
+            {
+                'month': 'February',
+                'notification_type': 'sms',
+                'rate': 0.017,
+                'chargeable_units': 33,
+                'notifications_sent': 1234,
+                'postage': 'none',
+                'sms_charged': 33,
+                'sms_free_allowance_used': 0,
+                'sms_cost': 0.561,
+                'letter_cost': 0,
+            },
             {
                 'month': 'February',
                 'notification_type': 'sms',

From bb0fb73bc82a4e83712185fe7eb5ae8ffb7205b8 Mon Sep 17 00:00:00 2001
From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk>
Date: Tue, 10 May 2022 16:48:03 +0100
Subject: [PATCH 16/16] Minor tweaks in response to PR comments

In response to [^1] and [^2].

[^1]: https://github.com/alphagov/notifications-admin/pull/4229#discussion_r869181152
[^2]: https://github.com/alphagov/notifications-admin/pull/4229#discussion_r869186063
---
 app/main/views/dashboard.py            |  4 ++++
 tests/app/main/views/test_dashboard.py | 10 ++++++----
 tests/conftest.py                      |  2 +-
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py
index 46520f8b5..e58eba8a7 100644
--- a/app/main/views/dashboard.py
+++ b/app/main/views/dashboard.py
@@ -423,6 +423,10 @@ def get_monthly_usage_breakdown_for_letters(monthly_letters):
         postage_order[get_monthly_usage_postage_description(row)], row['rate']
     )
 
+    # First sort letter rows by postage and then by rate, clumping "europe" and
+    # "rest-of-world" postage together as "international". Group the sorted rows
+    # together using the same fields - "group_key" is used for both operations.
+    # Note that "groupby" preserves the sort order in the groups it returns.
     rate_groups = groupby(sorted(monthly_letters, key=group_key), key=group_key)
 
     for _key, rate_group in rate_groups:
diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py
index a0968c673..bb3738ff6 100644
--- a/tests/app/main/views/test_dashboard.py
+++ b/tests/app/main/views/test_dashboard.py
@@ -1079,9 +1079,10 @@ def test_usage_page_monthly_breakdown(
     assert '140 free text messages' in monthly_breakdown
     assert '960 text messages at 1.65p' in monthly_breakdown
     assert '33 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
+    assert '10 second class letters at 31p' in monthly_breakdown
+    assert '3 international letters at 55p' in monthly_breakdown
+    assert '7 international letters at 84p' in monthly_breakdown
 
     assert 'March' in monthly_breakdown
     assert '£20.91' in monthly_breakdown
@@ -1110,7 +1111,7 @@ def test_usage_page_monthly_breakdown_shows_months_so_far(
 
 
 @freeze_time("2012-03-31 12:12:12")
-def test_usage_page_letter_breakdown_ordering(
+def test_usage_page_letter_breakdown_ordered_by_postage_and_rate(
     client_request,
     service_one,
     mock_get_monthly_usage_for_service,
@@ -1123,7 +1124,8 @@ def test_usage_page_letter_breakdown_ordering(
 
     assert normalize_spaces(postage_details[3].text) == '5 first class letters at 33p'
     assert normalize_spaces(postage_details[4].text) == '10 second class letters at 31p'
-    assert normalize_spaces(postage_details[5].text) == '10 international letters at 84p'
+    assert normalize_spaces(postage_details[5].text) == '3 international letters at 55p'
+    assert normalize_spaces(postage_details[6].text) == '7 international letters at 84p'
 
 
 def test_usage_page_with_0_free_allowance(
diff --git a/tests/conftest.py b/tests/conftest.py
index 569fcc7cc..6e0e4f51d 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -2421,7 +2421,7 @@ def mock_get_monthly_usage_for_service(mocker):
             {
                 'month': 'February',
                 'notification_type': 'letter',
-                'rate': 0.84,
+                'rate': 0.55,
                 'chargeable_units': 3,
                 'notifications_sent': 3,
                 'postage': 'europe',