From bac54462db7bf10e694440b639353163bc33c104 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 5 Mar 2021 15:03:23 +0000 Subject: [PATCH 1/5] Change config structure so allowances are dated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We’re going to have different allowances next financial year. This means that when someone adds a service, we’ll need to check which year it is, so we can give them the right allowance. This commit changes the config structure so that the current allowances are explicitly assigned to the 2020/21 financial year. It freezes the tests to the 2020/21 financial year, so they won’t start failing automatically when next financial year comes around. --- app/config.py | 32 ++++++++++++++++++------ app/main/views/add_service.py | 14 +++++++++-- tests/app/main/views/test_add_service.py | 25 ++++++++++++------ 3 files changed, 53 insertions(+), 18 deletions(-) diff --git a/app/config.py b/app/config.py index aa3ecb4e7..fbef50457 100644 --- a/app/config.py +++ b/app/config.py @@ -33,14 +33,30 @@ class Config(object): AWS_REGION = 'eu-west-1' DEFAULT_SERVICE_LIMIT = 50 DEFAULT_FREE_SMS_FRAGMENT_LIMITS = { - 'central': 250000, - 'local': 25000, - 'nhs_central': 250000, - 'nhs_local': 25000, - 'nhs_gp': 25000, - 'emergency_service': 25000, - 'school_or_college': 25000, - 'other': 25000, + 'central': { + 2020: 250000, + }, + 'local': { + 2020: 25000, + }, + 'nhs_central': { + 2020: 250000, + }, + 'nhs_local': { + 2020: 25000, + }, + 'nhs_gp': { + 2020: 25000, + }, + 'emergency_service': { + 2020: 25000, + }, + 'school_or_college': { + 2020: 25000, + }, + 'other': { + 2020: 25000, + }, } EMAIL_EXPIRY_SECONDS = 3600 # 1 hour INVITATION_EXPIRY_SECONDS = 3600 * 24 * 2 # 2 days - also set on api diff --git a/app/main/views/add_service.py b/app/main/views/add_service.py index 62337f2a1..975279667 100644 --- a/app/main/views/add_service.py +++ b/app/main/views/add_service.py @@ -6,11 +6,21 @@ from app import billing_api_client, service_api_client from app.formatters import email_safe from app.main import main from app.main.forms import CreateNhsServiceForm, CreateServiceForm -from app.utils import user_is_gov_user, user_is_logged_in +from app.utils import ( + get_current_financial_year, + user_is_gov_user, + user_is_logged_in, +) def _create_service(service_name, organisation_type, email_from, form): - free_sms_fragment_limit = current_app.config['DEFAULT_FREE_SMS_FRAGMENT_LIMITS'].get(organisation_type) + free_sms_fragment_limit = current_app.config[ + 'DEFAULT_FREE_SMS_FRAGMENT_LIMITS' + ][ + organisation_type + ][ + get_current_financial_year() + ] try: service_id = service_api_client.create_service( diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py index 11ef3dc0a..bed1c687b 100644 --- a/tests/app/main/views/test_add_service.py +++ b/tests/app/main/views/test_add_service.py @@ -1,5 +1,6 @@ import pytest from flask import session, url_for +from freezegun import freeze_time from notifications_python_client.errors import HTTPError from app.utils import is_gov_user @@ -116,6 +117,7 @@ def test_show_different_page_if_user_org_type_is_local( ('other', None, 'other', 25000), ('central', 'local', 'central', 250000), )) +@freeze_time("2021-01-01") def test_should_add_service_and_redirect_to_tour_when_no_services( mocker, client_request, @@ -239,14 +241,14 @@ def test_get_should_only_show_nhs_org_types_radios_if_user_has_nhs_email( ] -@pytest.mark.parametrize('organisation_type, free_allowance', [ - ('central', 250 * 1000), - ('local', 25 * 1000), - ('nhs_central', 250 * 1000), - ('nhs_local', 25 * 1000), - ('school_or_college', 25 * 1000), - ('emergency_service', 25 * 1000), - ('other', 25 * 1000), +@pytest.mark.parametrize('financial_year, organisation_type, free_allowance', [ + (2020, 'central', 250 * 1000), + (2020, 'local', 25 * 1000), + (2020, 'nhs_central', 250 * 1000), + (2020, 'nhs_local', 25 * 1000), + (2020, 'school_or_college', 25 * 1000), + (2020, 'emergency_service', 25 * 1000), + (2020, 'other', 25 * 1000), ]) def test_should_add_service_and_redirect_to_dashboard_when_existing_service( app_, @@ -261,7 +263,13 @@ def test_should_add_service_and_redirect_to_dashboard_when_existing_service( free_allowance, mock_create_or_update_free_sms_fragment_limit, mock_get_all_email_branding, + financial_year, ): + mocker.patch( + 'app.main.views.add_service.get_current_financial_year', + return_value=financial_year, + ) + client_request.post( 'main.add_service', _data={ @@ -308,6 +316,7 @@ def test_add_service_fails_if_service_name_fails_validation( assert error_message in page.find("span", {"class": "govuk-error-message"}).text +@freeze_time("2021-01-01") def test_should_return_form_errors_with_duplicate_service_name_regardless_of_case( client_request, mock_get_organisation_by_domain, From 2138ce02d962841d6daf7696555a46ce54a2b111 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 5 Mar 2021 15:06:40 +0000 Subject: [PATCH 2/5] Add NHS GP organisation type to test cases This was missing. --- tests/app/main/views/test_add_service.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py index bed1c687b..55db79a14 100644 --- a/tests/app/main/views/test_add_service.py +++ b/tests/app/main/views/test_add_service.py @@ -246,6 +246,7 @@ def test_get_should_only_show_nhs_org_types_radios_if_user_has_nhs_email( (2020, 'local', 25 * 1000), (2020, 'nhs_central', 250 * 1000), (2020, 'nhs_local', 25 * 1000), + (2020, 'nhs_gp', 25 * 1000), (2020, 'school_or_college', 25 * 1000), (2020, 'emergency_service', 25 * 1000), (2020, 'other', 25 * 1000), From 9a3f2c30ef5bbb5e84e36504816efb1e0dab90e3 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 5 Mar 2021 15:09:21 +0000 Subject: [PATCH 3/5] Use underscores to notate 1000s This is easier to read than using multiplication, or nothing. --- app/config.py | 16 ++++++++-------- tests/app/main/views/test_add_service.py | 16 ++++++++-------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/app/config.py b/app/config.py index fbef50457..0045088b7 100644 --- a/app/config.py +++ b/app/config.py @@ -34,28 +34,28 @@ class Config(object): DEFAULT_SERVICE_LIMIT = 50 DEFAULT_FREE_SMS_FRAGMENT_LIMITS = { 'central': { - 2020: 250000, + 2020: 250_000, }, 'local': { - 2020: 25000, + 2020: 25_000, }, 'nhs_central': { - 2020: 250000, + 2020: 250_000, }, 'nhs_local': { - 2020: 25000, + 2020: 25_000, }, 'nhs_gp': { - 2020: 25000, + 2020: 25_000, }, 'emergency_service': { - 2020: 25000, + 2020: 25_000, }, 'school_or_college': { - 2020: 25000, + 2020: 25_000, }, 'other': { - 2020: 25000, + 2020: 25_000, }, } EMAIL_EXPIRY_SECONDS = 3600 # 1 hour diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py index 55db79a14..6dd40b1da 100644 --- a/tests/app/main/views/test_add_service.py +++ b/tests/app/main/views/test_add_service.py @@ -242,14 +242,14 @@ def test_get_should_only_show_nhs_org_types_radios_if_user_has_nhs_email( @pytest.mark.parametrize('financial_year, organisation_type, free_allowance', [ - (2020, 'central', 250 * 1000), - (2020, 'local', 25 * 1000), - (2020, 'nhs_central', 250 * 1000), - (2020, 'nhs_local', 25 * 1000), - (2020, 'nhs_gp', 25 * 1000), - (2020, 'school_or_college', 25 * 1000), - (2020, 'emergency_service', 25 * 1000), - (2020, 'other', 25 * 1000), + (2020, 'central', 250_000), + (2020, 'local', 25_000), + (2020, 'nhs_central', 250_000), + (2020, 'nhs_local', 25_000), + (2020, 'nhs_gp', 25_000), + (2020, 'school_or_college', 25_000), + (2020, 'emergency_service', 25_000), + (2020, 'other', 25_000), ]) def test_should_add_service_and_redirect_to_dashboard_when_existing_service( app_, From 61f605ff67ad18c2023dc97680de30c1a0cecc74 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 5 Mar 2021 15:55:59 +0000 Subject: [PATCH 4/5] Add new rates for 2021 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These are the new rates from https://docs.google.com/document/d/1aObNQNBw3ayPMl3b_Qc5kZBdaXIAMDnBGtV-xPtFBsc/edit?ts=603f68ea# We’re changing the free allowance so we can continue to support all the teams that use Notify. The new allowance means over 90% of teams can still send all the text messages they need to without paying. --- app/config.py | 8 ++++++++ tests/app/main/views/test_add_service.py | 9 +++++++++ 2 files changed, 17 insertions(+) diff --git a/app/config.py b/app/config.py index 0045088b7..b881a06c5 100644 --- a/app/config.py +++ b/app/config.py @@ -35,27 +35,35 @@ class Config(object): DEFAULT_FREE_SMS_FRAGMENT_LIMITS = { 'central': { 2020: 250_000, + 2021: 150_000, }, 'local': { 2020: 25_000, + 2021: 25_000, }, 'nhs_central': { 2020: 250_000, + 2021: 150_000, }, 'nhs_local': { 2020: 25_000, + 2021: 25_000, }, 'nhs_gp': { 2020: 25_000, + 2021: 10_000, }, 'emergency_service': { 2020: 25_000, + 2021: 25_000, }, 'school_or_college': { 2020: 25_000, + 2021: 10_000, }, 'other': { 2020: 25_000, + 2021: 10_000, }, } EMAIL_EXPIRY_SECONDS = 3600 # 1 hour diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py index 6dd40b1da..dcc54a5ec 100644 --- a/tests/app/main/views/test_add_service.py +++ b/tests/app/main/views/test_add_service.py @@ -250,6 +250,15 @@ def test_get_should_only_show_nhs_org_types_radios_if_user_has_nhs_email( (2020, 'school_or_college', 25_000), (2020, 'emergency_service', 25_000), (2020, 'other', 25_000), + + (2021, 'central', 150_000), + (2021, 'local', 25_000), + (2021, 'nhs_central', 150_000), + (2021, 'nhs_local', 25_000), + (2021, 'nhs_gp', 10_000), + (2021, 'school_or_college', 10_000), + (2021, 'emergency_service', 25_000), + (2021, 'other', 10_000), ]) def test_should_add_service_and_redirect_to_dashboard_when_existing_service( app_, From 7013eb9696de6385de655cc89a510137f03c212b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 5 Mar 2021 16:26:26 +0000 Subject: [PATCH 5/5] Make get_current_financial_year timezone aware MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It would have given the wrong answer for the first hour of the 2021 financial year. This was OK before, because we didn’t need this kind of precision. But now it could mean someone signing up in the middle of the night getting the wrong free text message allowance, so we should fix it. --- app/utils.py | 4 +++- tests/app/test_utils.py | 12 ++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/app/utils.py b/app/utils.py index b89c88262..31b89267e 100644 --- a/app/utils.py +++ b/app/utils.py @@ -334,7 +334,9 @@ def get_template( def get_current_financial_year(): - now = datetime.utcnow() + now = utc_string_to_aware_gmt_datetime( + datetime.utcnow() + ) current_month = int(now.strftime('%-m')) current_year = int(now.strftime('%Y')) return current_year if current_month > 3 else current_year - 1 diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index 47bdf1a2e..3ebc2bda6 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -16,6 +16,7 @@ from app.utils import ( generate_next_dict, generate_notifications_csv, generate_previous_dict, + get_current_financial_year, get_letter_printing_statement, get_letter_validation_error, get_logo_cdn_domain, @@ -657,3 +658,14 @@ def test_merge_jsonlike_merges_jsonlike_objects_correctly(source_object, destina )) def test_round_to_significant_figures(value, significant_figures, expected_result): assert round_to_significant_figures(value, significant_figures) == expected_result + + +@pytest.mark.parametrize('datetime_string, financial_year', ( + ('2021-01-01T00:00:00+00:00', 2020), # Start of 2021 + ('2021-03-31T22:59:59+00:00', 2020), # One minute before midnight (BST) + ('2021-03-31T23:00:00+00:00', 2021), # Midnight (BST) + ('2021-12-12T12:12:12+01:00', 2021), # Later in the year +)) +def test_get_financial_year(datetime_string, financial_year): + with freeze_time(datetime_string): + assert get_current_financial_year() == financial_year