From b3f5bb6435f5d0ec31fa6d17a27987d8f31d7786 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 8 Apr 2022 16:05:43 +0100 Subject: [PATCH 1/4] add new daily sms provider volumes report nearly identical to the daily-volumes-report but sms only, and split up by provider --- app/main/views/platform_admin.py | 40 ++++++++++++++++ app/navigation.py | 1 + app/notify_client/billing_api_client.py | 9 ++++ .../daily-sms-provider-volumes-report.html | 47 +++++++++++++++++++ .../views/platform-admin/reports.html | 3 ++ tests/app/main/views/test_platform_admin.py | 43 +++++++++++++++++ tests/app/test_navigation.py | 1 + 7 files changed, 144 insertions(+) create mode 100644 app/templates/views/platform-admin/daily-sms-provider-volumes-report.html diff --git a/app/main/views/platform_admin.py b/app/main/views/platform_admin.py index 6c8a0923c..260a2e486 100644 --- a/app/main/views/platform_admin.py +++ b/app/main/views/platform_admin.py @@ -381,6 +381,46 @@ def get_daily_volumes(): return render_template('views/platform-admin/daily-volumes-report.html', form=form) +@main.route("/platform-admin/reports/daily-sms-provider-volumes-report", methods=['GET', 'POST']) +@user_is_platform_admin +def get_daily_sms_provider_volumes(): + form = BillingReportDateFilterForm() + + if form.validate_on_submit(): + start_date = form.start_date.data + end_date = form.end_date.data + headers = [ + "day", + "provider", + "sms totals", + "sms fragment totals", + "sms chargeable units", + "sms cost", + ] + result = billing_api_client.get_data_for_daily_sms_provider_volumes_report(start_date, end_date) + + rows = [ + [ + r["day"], + r["provider"], + r["sms_totals"], + r["sms_fragment_totals"], + r["sms_chargeable_units"], + r["sms_cost"] + ] + for r in result + ] + if rows: + return Spreadsheet.from_rows([headers] + rows).as_csv_data, 200, { + 'Content-Type': 'text/csv; charset=utf-8', + 'Content-Disposition': + f'attachment; filename="Daily SMS provider volumes report from {start_date} to {end_date}.csv"' + } + else: + flash('No results for dates') + return render_template('views/platform-admin/daily-sms-provider-volumes-report.html', form=form) + + @main.route("/platform-admin/complaints") @user_is_platform_admin def platform_admin_list_complaints(): diff --git a/app/navigation.py b/app/navigation.py index a364fd4d4..6053b960d 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -95,6 +95,7 @@ class HeaderNavigation(Navigation): 'notifications_sent_by_service', 'get_billing_report', 'get_daily_volumes', + 'get_daily_sms_provider_volumes', 'get_volumes_by_service', 'organisations', 'platform_admin', diff --git a/app/notify_client/billing_api_client.py b/app/notify_client/billing_api_client.py index 08b5ce52c..44634ff4e 100644 --- a/app/notify_client/billing_api_client.py +++ b/app/notify_client/billing_api_client.py @@ -55,5 +55,14 @@ class BillingAPIClient(NotifyAdminAPIClient): 'end_date': str(end_date), }) + def get_data_for_daily_sms_provider_volumes_report(self, start_date, end_date): + return self.get( + url='/platform-stats/daily-sms-provider-volumes-report', + params={ + 'start_date': str(start_date), + 'end_date': str(end_date), + } + ) + billing_api_client = BillingAPIClient() diff --git a/app/templates/views/platform-admin/daily-sms-provider-volumes-report.html b/app/templates/views/platform-admin/daily-sms-provider-volumes-report.html new file mode 100644 index 000000000..73b3ca4ae --- /dev/null +++ b/app/templates/views/platform-admin/daily-sms-provider-volumes-report.html @@ -0,0 +1,47 @@ +{% extends "views/platform-admin/_base_template.html" %} +{% from "components/form.html" import form_wrapper %} +{% from "components/table.html" import mapping_table, row, text_field %} + +{% block per_page_title %} + Daily SMS provider volumes Report +{% endblock %} + +{% block platform_admin_content %} + +

+ Daily SMS provider volumes Report +

+ + {% call form_wrapper() %} + {{ form.start_date(param_extensions={"hint": {"text": "Use the format YYYY-MM-DD"}}) }} + {{ form.end_date(param_extensions={"hint": {"text": "Use the format YYYY-MM-DD"}}) }} + {{ page_footer('Download report') }} + {% endcall %} + +

+ Data included in the report +

+
+ {% call mapping_table( + caption='Descriptions of daily SMS provider volumes data', + field_headings=['Name', 'Description'], + field_headings_visible=True, + caption_visible=False + ) %} + {% for message_length, description in [ + ('day', 'The whole business day in BST'), + ('provider', 'The SMS provider'), + ('sms totals', 'The number of text messages sent'), + ('sms fragments', 'The number of text message fragments sent'), + ('sms chargeable units', 'The number of text message fragments sent times the rate multiplier'), + ('sms cost', 'The cost of text messages sent'), + ] %} + {% call row() %} + {{ text_field(message_length) }} + {{ text_field(description | safe) }} + {% endcall %} + {% endfor %} + {% endcall %} +
+ +{% endblock %} diff --git a/app/templates/views/platform-admin/reports.html b/app/templates/views/platform-admin/reports.html index dda3f18ff..8dd36ffef 100644 --- a/app/templates/views/platform-admin/reports.html +++ b/app/templates/views/platform-admin/reports.html @@ -25,4 +25,7 @@

Daily volumes Report

+

+ Daily SMS provider volumes Report +

{% endblock %} diff --git a/tests/app/main/views/test_platform_admin.py b/tests/app/main/views/test_platform_admin.py index b106d4255..e6b0e8f2b 100644 --- a/tests/app/main/views/test_platform_admin.py +++ b/tests/app/main/views/test_platform_admin.py @@ -1113,3 +1113,46 @@ def test_get_daily_volumes_report_when_calls_api_and_download_data( '\r\n' ) + + +def test_get_daily_sms_provider_volumes_report_when_calls_api_and_download_data( + client_request, + platform_admin_user, + mocker +): + mocker.patch( + "app.main.views.platform_admin.billing_api_client.get_data_for_daily_sms_provider_volumes_report", + return_value=[{ + "day": '2019-01-01', + "provider": 'foo', + "sms_totals": 20, + "sms_fragment_totals": 40, + "sms_chargeable_units": 60, + "sms_cost": 80, + }] + ) + + client_request.login(platform_admin_user) + response = client_request.post_response( + 'main.get_daily_sms_provider_volumes', + _data={'start_date': '2019-01-01', 'end_date': '2019-03-31'}, + _expected_status=200, + ) + + assert response.content_type == 'text/csv; charset=utf-8' + assert response.headers['Content-Disposition'] == ( + 'attachment; filename="Daily SMS provider volumes report from {} to {}.csv"'.format('2019-01-01', '2019-03-31') + ) + + assert response.get_data(as_text=True) == ( + "day,provider,sms totals,sms fragment totals,sms chargeable units,sms cost\r\n" + + + '2019-01-01,' + + 'foo,' + + '20,' + + '40,' + + '60,' + + '80' + + '\r\n' + ) diff --git a/tests/app/test_navigation.py b/tests/app/test_navigation.py index 06eccdd28..5b6d1eda5 100644 --- a/tests/app/test_navigation.py +++ b/tests/app/test_navigation.py @@ -132,6 +132,7 @@ EXCLUDED_ENDPOINTS = tuple(map(Navigation.get_endpoint_with_blueprint, { 'forgot_password', 'get_billing_report', 'get_daily_volumes', + 'get_daily_sms_provider_volumes', 'get_volumes_by_service', 'get_example_csv', 'get_notifications_as_json', From ebc005005b735f6e928440a24ab47e0d9c85b9b0 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 11 Apr 2022 14:07:37 +0100 Subject: [PATCH 2/4] refactor: DRY up billing api client test at the same time add the new daily_sms_provider_volumes_report func to the tests. --- .../app/notify_client/test_billing_client.py | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/tests/app/notify_client/test_billing_client.py b/tests/app/notify_client/test_billing_client.py index b3c29509b..d657ab258 100644 --- a/tests/app/notify_client/test_billing_client.py +++ b/tests/app/notify_client/test_billing_client.py @@ -1,5 +1,7 @@ import uuid +import pytest + from app.notify_client.billing_api_client import BillingAPIClient @@ -43,19 +45,18 @@ def test_post_free_sms_fragment_limit_for_year_endpoint(mocker, api_user_active) ) -def test_get_data_for_volumes_by_service_report(mocker, api_user_active): +@pytest.mark.parametrize('func, expected_url', [ + (BillingAPIClient.get_data_for_volumes_by_service_report, '/platform-stats/volumes-by-service'), + (BillingAPIClient.get_data_for_daily_volumes_report, '/platform-stats/daily-volumes-report'), + ( + BillingAPIClient.get_data_for_daily_sms_provider_volumes_report, + '/platform-stats/daily-sms-provider-volumes-report' + ), +]) +def test_get_data_for_volume_reports(mocker, api_user_active, func, expected_url): mock_get = mocker.patch('app.notify_client.billing_api_client.BillingAPIClient.get') client = BillingAPIClient() - client.get_data_for_volumes_by_service_report('2022-03-01', '2022-03-31') - mock_get.assert_called_once_with(url='/platform-stats/volumes-by-service', - params={'start_date': '2022-03-01', 'end_date': '2022-03-31'}) + func(client, '2022-03-01', '2022-03-31') - -def test_get_data_for_daily_volumes_report(mocker, api_user_active): - mock_get = mocker.patch('app.notify_client.billing_api_client.BillingAPIClient.get') - client = BillingAPIClient() - - client.get_data_for_daily_volumes_report('2022-03-01', '2022-03-31') - mock_get.assert_called_once_with(url='/platform-stats/daily-volumes-report', - params={'start_date': '2022-03-01', 'end_date': '2022-03-31'}) + mock_get.assert_called_once_with(url=expected_url, params={'start_date': '2022-03-01', 'end_date': '2022-03-31'}) From c2472e55aac5c8c741aec4955c61c13a2505f8a8 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 11 Apr 2022 14:38:20 +0100 Subject: [PATCH 3/4] clean up info tables to not over-use safe markdown filter the safe filter is quite dangerous - it allows HTML to be rendered as passed through (the default action is to escape all html), so should only be used with trusted input. Move it so we only apply it to fields we specifically expect to have HTML in - in this case, some tables contain links to other pages. Also, clean up the variable names for some of these info tables, as they didn't really make sense. --- app/templates/views/message-status.html | 20 +++++++++---------- .../daily-sms-provider-volumes-report.html | 6 +++--- .../platform-admin/daily-volumes-report.html | 6 +++--- .../platform-admin/get-billing-report.html | 8 ++++---- .../volumes-by-service-report.html | 6 +++--- 5 files changed, 23 insertions(+), 23 deletions(-) diff --git a/app/templates/views/message-status.html b/app/templates/views/message-status.html index 4ce23379e..30c551f1e 100644 --- a/app/templates/views/message-status.html +++ b/app/templates/views/message-status.html @@ -22,16 +22,16 @@ field_headings_visible=True, caption_visible=False ) %} - {% for message_length, charge in [ + {% for message_status, description in [ ('Sending', 'Notify has sent the message to the provider. The provider will try to deliver the message to the recipient for up to 72 hours. Notify is waiting for delivery information.'), ('Delivered', 'The message was successfully delivered. Notify will not tell you if a user has opened or read a message.'), ('Email address does not exist', 'The provider could not deliver the message because the email address was wrong. You should remove these email addresses from your database.'), - ('Inbox not accepting messages right now', 'The provider could not deliver the message. This can happen when the recipient’s inbox is full or their anti-spam filter rejects your email. Check your content does not look like spam before you try to send the message again.'), + ('Inbox not accepting messages right now', 'The provider could not deliver the message. This can happen when the recipient’s inbox is full or their anti-spam filter rejects your email. Check your content does not look like spam before you try to send the message again.' | safe), ('Technical failure', 'Your message was not sent because there was a problem between Notify and the provider. You’ll have to try sending your messages again.'), ] %} {% call row() %} - {{ text_field(message_length) }} - {{ text_field(charge | safe) }} + {{ text_field(message_status) }} + {{ text_field(description) }} {% endcall %} {% endfor %} {% endcall %} @@ -51,7 +51,7 @@ field_headings_visible=True, caption_visible=False ) %} - {% for message_length, charge in [ + {% for message_status, description in [ ('Sending', 'Notify has sent the message to the provider. The provider will try to deliver the message to the recipient for up to 72 hours. Notify is waiting for delivery information.'), ('Sent to an international number', 'The mobile networks in some countries do not provide any more delivery information.'), ('Delivered', 'The message was successfully delivered. Notify will not tell you if a user has opened or read a message.'), @@ -60,8 +60,8 @@ ('Technical failure', 'Your message was not sent because there was a problem between Notify and the provider. You’ll have to try sending your messages again. You will not be charged for text messages that are affected by a technical failure.'), ] %} {% call row() %} - {{ text_field(message_length) }} - {{ text_field(charge) }} + {{ text_field(message_status) }} + {{ text_field(description) }} {% endcall %} {% endfor %} {% endcall %} @@ -75,7 +75,7 @@ field_headings_visible=True, caption_visible=False ) %} - {% for message_length, charge in [ + {% for message_status, description in [ ('Sent', 'Notify has sent the letter to the provider to be printed.'), ('Printed', 'The provider has printed the letter. Letters are printed at 5:30pm and dispatched the next working day.'), ('Cancelled', 'Sending cancelled. Your letter will not be printed or dispatched.'), @@ -83,8 +83,8 @@ ('Permanent failure', 'The provider cannot print the letter. Your letter will not be dispatched.') ] %} {% call row() %} - {{ text_field(message_length) }} - {{ text_field(charge) }} + {{ text_field(message_status) }} + {{ text_field(description) }} {% endcall %} {% endfor %} {% endcall %} diff --git a/app/templates/views/platform-admin/daily-sms-provider-volumes-report.html b/app/templates/views/platform-admin/daily-sms-provider-volumes-report.html index 73b3ca4ae..c6e4002af 100644 --- a/app/templates/views/platform-admin/daily-sms-provider-volumes-report.html +++ b/app/templates/views/platform-admin/daily-sms-provider-volumes-report.html @@ -28,7 +28,7 @@ field_headings_visible=True, caption_visible=False ) %} - {% for message_length, description in [ + {% for column_heading, description in [ ('day', 'The whole business day in BST'), ('provider', 'The SMS provider'), ('sms totals', 'The number of text messages sent'), @@ -37,8 +37,8 @@ ('sms cost', 'The cost of text messages sent'), ] %} {% call row() %} - {{ text_field(message_length) }} - {{ text_field(description | safe) }} + {{ text_field(column_heading) }} + {{ text_field(description) }} {% endcall %} {% endfor %} {% endcall %} diff --git a/app/templates/views/platform-admin/daily-volumes-report.html b/app/templates/views/platform-admin/daily-volumes-report.html index 827720e26..be59016d3 100644 --- a/app/templates/views/platform-admin/daily-volumes-report.html +++ b/app/templates/views/platform-admin/daily-volumes-report.html @@ -28,7 +28,7 @@ field_headings_visible=True, caption_visible=False ) %} - {% for message_length, charge in [ + {% for column_heading, description in [ ('day', 'The whole business day in BST.'), ('sms totals', 'The number of text messages sent'), ('sms fragments', 'The number of text message fragments sent times the rate multiplier'), @@ -38,8 +38,8 @@ ('letter sheet totals', 'The number of sheets sent') ] %} {% call row() %} - {{ text_field(message_length) }} - {{ text_field(charge | safe) }} + {{ text_field(column_heading) }} + {{ text_field(description) }} {% endcall %} {% endfor %} {% endcall %} diff --git a/app/templates/views/platform-admin/get-billing-report.html b/app/templates/views/platform-admin/get-billing-report.html index 8e77016d1..7c1fed111 100644 --- a/app/templates/views/platform-admin/get-billing-report.html +++ b/app/templates/views/platform-admin/get-billing-report.html @@ -28,16 +28,16 @@ field_headings_visible=True, caption_visible=False ) %} - {% for message_length, charge in [ + {% for column_heading, description in [ ('sms cost', 'The total cost of text messages sent after a service has used its free allowance.'), - ('sms chargeable units', 'The number of fragments sent after a service has used its free allowance. This number takes into account the cost multiplier for sending international text messages.'), + ('sms chargeable units', 'The number of fragments sent after a service has used its free allowance. This number takes into account the cost multiplier for sending international text messages.' | safe), ('letter cost', 'The total cost of letters sent by a service.'), ('letter breakdown', 'The number of letters sent by a service, grouped by postage and unit cost.'), ('purchase order number, contact names, contact email addresses and billing reference', 'We add this data manually based on the information we get from services. You can help by adding it to the service settings page.'), ] %} {% call row() %} - {{ text_field(message_length) }} - {{ text_field(charge | safe) }} + {{ text_field(column_heading) }} + {{ text_field(description) }} {% endcall %} {% endfor %} {% endcall %} diff --git a/app/templates/views/platform-admin/volumes-by-service-report.html b/app/templates/views/platform-admin/volumes-by-service-report.html index 91c70fca2..4b35a0d4b 100644 --- a/app/templates/views/platform-admin/volumes-by-service-report.html +++ b/app/templates/views/platform-admin/volumes-by-service-report.html @@ -28,7 +28,7 @@ field_headings_visible=True, caption_visible=False ) %} - {% for message_length, charge in [ + {% for column_heading, description in [ ('free allowance', 'Free allowance set for the service. This is the latest free allowance for the date range given'), ('sms notifications', 'The number of text messages sent by the service.'), ('sms chargeable units', 'The number of text message fragments times the rate multiplier sent by the service.'), @@ -38,8 +38,8 @@ ('letter sheet totals', 'The number of sheet sent by a service') ] %} {% call row() %} - {{ text_field(message_length) }} - {{ text_field(charge | safe) }} + {{ text_field(column_heading) }} + {{ text_field(description) }} {% endcall %} {% endfor %} {% endcall %} From 971101d07c142cd770d01e75fb59500f3cdb2a31 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 11 Apr 2022 14:55:58 +0100 Subject: [PATCH 4/4] clean up test_platform_admin string concatenation make it consistent - always use a + to concatenate strings, always have that + at the end of the line. (also remove a rogue "when" from some test names) --- tests/app/main/views/test_platform_admin.py | 44 ++++++++++----------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/tests/app/main/views/test_platform_admin.py b/tests/app/main/views/test_platform_admin.py index e6b0e8f2b..eee0250d8 100644 --- a/tests/app/main/views/test_platform_admin.py +++ b/tests/app/main/views/test_platform_admin.py @@ -854,14 +854,14 @@ def test_get_live_services_report( ) report = response.get_data(as_text=True) assert report.strip() == ( - 'Service ID,Organisation,Organisation type,Service name,Consent to research,Main contact,Contact email,' - + 'Contact mobile,Live date,SMS volume intent,Email volume intent,Letter volume intent,SMS sent this year,' - + 'Emails sent this year,Letters sent this year,Free sms allowance\r\n' + 'Service ID,Organisation,Organisation type,Service name,Consent to research,Main contact,Contact email,' + + 'Contact mobile,Live date,SMS volume intent,Email volume intent,Letter volume intent,SMS sent this year,' + + 'Emails sent this year,Letters sent this year,Free sms allowance\r\n' + - + '1,Forest,Ecosystem,jessie the oak tree,True,Forest fairy,forest.fairy@digital.cabinet-office.gov.uk,' - + '+447700900986,29-03-2014,100,50,20,300,1200,0,100\r\n' + '1,Forest,Ecosystem,jessie the oak tree,True,Forest fairy,forest.fairy@digital.cabinet-office.gov.uk,' + + '+447700900986,29-03-2014,100,50,20,300,1200,0,100\r\n' + - + '2,Forest,Ecosystem,james the pine tree,,,,,,,60,0,0,0,0,200' + '2,Forest,Ecosystem,james the pine tree,,,,,,,60,0,0,0,0,200' ) @@ -920,7 +920,7 @@ def test_get_billing_report_when_no_results_for_date(client_request, platform_ad assert normalize_spaces(error.text) == 'No results for dates' -def test_get_billing_report_when_calls_api_and_download_data( +def test_get_billing_report_calls_api_and_download_data( client_request, platform_admin_user, mocker @@ -957,10 +957,9 @@ def test_get_billing_report_when_calls_api_and_download_data( ) assert response.get_data(as_text=True) == ( - 'organisation_id,organisation_name,service_id,service_name,sms_cost,sms_chargeable_units' + - ',total_letters,letter_cost,letter_breakdown,purchase_order_number,contact_names,contact_email_addresses' + - - ',billing_reference\r\n' + + 'organisation_id,organisation_name,service_id,service_name,sms_cost,sms_chargeable_units,' + + 'total_letters,letter_cost,letter_breakdown,purchase_order_number,contact_names,contact_email_addresses,' + + 'billing_reference\r\n' + '7832a1be-a1f0-4f2a-982f-05adfd3d6354,' + 'Org for a - with sms and letter,' + @@ -973,7 +972,7 @@ def test_get_billing_report_when_calls_api_and_download_data( '"6 second class letters at 45p' + '\n' + '2 first class letters at 35p",' + - 'PO1234,"Anne, Marie, Josh","billing@example.com, accounts@example.com",Notify2020' + 'PO1234,"Anne, Marie, Josh","billing@example.com, accounts@example.com",Notify2020' + '\r\n' ) @@ -1008,15 +1007,16 @@ def test_get_notifications_sent_by_service_calls_api_and_downloads_data( 'attachment; filename="{} to {} notification status per service report.csv"'.format(start_date, end_date) ) assert response.get_data(as_text=True) == ( - 'date_created,service_id,service_name,notification_type,count_sending,count_delivered,count_technical_failure,' - 'count_temporary_failure,count_permanent_failure,count_sent\r\n' - '2019-01-01,596364a0-858e-42c8-9062-a8fe822260eb,service one,email,191,0,0,14,0,0\r\n' - '2019-01-01,596364a0-858e-42c8-9062-a8fe822260eb,service one,sms,42,0,0,8,0,0\r\n' + 'date_created,service_id,service_name,notification_type,count_sending,count_delivered,' + + 'count_technical_failure,count_temporary_failure,count_permanent_failure,count_sent\r\n' + + + '2019-01-01,596364a0-858e-42c8-9062-a8fe822260eb,service one,email,191,0,0,14,0,0\r\n' + + '2019-01-01,596364a0-858e-42c8-9062-a8fe822260eb,service one,sms,42,0,0,8,0,0\r\n' + '2019-01-01,147ad62a-2951-4fa1-9ca0-093cd1a52c52,service two,email,3,1,0,2,0,0\r\n' ) -def test_get_volumes_by_service_report_when_calls_api_and_download_data( +def test_get_volumes_by_service_report_calls_api_and_download_data( client_request, platform_admin_user, mocker @@ -1064,13 +1064,13 @@ def test_get_volumes_by_service_report_when_calls_api_and_download_data( '8,' + '10,' + '4.5,' + - '10' + '10' + '\r\n' ) -def test_get_daily_volumes_report_when_calls_api_and_download_data( +def test_get_daily_volumes_report_calls_api_and_download_data( client_request, platform_admin_user, mocker @@ -1109,13 +1109,13 @@ def test_get_daily_volumes_report_when_calls_api_and_download_data( '60,' + '100,' + '10,' + - '20' + '20' + '\r\n' ) -def test_get_daily_sms_provider_volumes_report_when_calls_api_and_download_data( +def test_get_daily_sms_provider_volumes_report_calls_api_and_download_data( client_request, platform_admin_user, mocker @@ -1152,7 +1152,7 @@ def test_get_daily_sms_provider_volumes_report_when_calls_api_and_download_data( '20,' + '40,' + '60,' + - '80' + '80' + '\r\n' )