diff --git a/app/main/views/add_service.py b/app/main/views/add_service.py index ecf2b9723..e2e328977 100644 --- a/app/main/views/add_service.py +++ b/app/main/views/add_service.py @@ -54,7 +54,7 @@ def _create_service(service_name, organisation_type, email_from, form): ) session['service_id'] = service_id - billing_api_client.create_or_update_free_sms_fragment_limit_for_year(service_id, free_sms_fragment_limit) + billing_api_client.create_or_update_free_sms_fragment_limit(service_id, free_sms_fragment_limit) return service_id, None except HTTPError as e: diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 9901c3fda..22a85e486 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -112,11 +112,12 @@ def template_history(service_id): 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) return render_template( 'views/usage.html', months=list(get_free_paid_breakdown_for_billable_units( year, - billing_api_client.get_free_sms_fragment_limit_for_year(service_id, year), + free_sms_allowance, billing_api_client.get_billable_units(service_id, year) )), selected_year=year, @@ -126,7 +127,7 @@ def usage(service_id): end=current_financial_year + 1, ), **calculate_usage(billing_api_client.get_service_usage(service_id, year), - billing_api_client.get_free_sms_fragment_limit_for_year(service_id, year)) + free_sms_allowance) ) @@ -291,8 +292,7 @@ def get_dashboard_totals(statistics): def calculate_usage(usage, free_sms_fragment_limit): - # TODO: Don't hardcode these - get em from the API - sms_free_allowance = free_sms_fragment_limit # VB-Progress + sms_free_allowance = free_sms_fragment_limit sms_rate = 0 if len(usage) == 0 else usage[0].get("rate", 0) sms_sent = get_sum_billing_units(breakdown for breakdown in usage if breakdown['notification_type'] == 'sms') diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 3306cdcbc..f4cb3c69e 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -42,7 +42,6 @@ from app.main.forms import ( ) from app import user_api_client, current_service, organisations_client, inbound_number_client, billing_api_client from notifications_utils.formatters import formatted_list -from app.utils import get_current_financial_year dummy_bearer_token = 'bearer_token_set' @@ -737,22 +736,16 @@ def set_organisation_type(service_id): def set_free_sms_allowance(service_id): form = FreeSMSAllowance(free_sms_allowance=billing_api_client.get_free_sms_fragment_limit_for_year(service_id)) + if form.validate_on_submit(): service_api_client.update_service( service_id, # TODO: Retire this after new end points are added. free_sms_fragment_limit=form.free_sms_allowance.data, ) - # get a list of all the free sms allowance entries for this service - sms_list = billing_api_client.get_free_sms_fragment_limit_for_all_years(service_id) - for item in range(0, len(sms_list)): - if sms_list[item]['financial_year_start'] >= get_current_financial_year(): - form.set_free_sms_allowance = \ - billing_api_client.create_or_update_free_sms_fragment_limit_for_year(service_id, - form.free_sms_allowance.data, - sms_list[item][ - 'financial_year_start']) + billing_api_client.create_or_update_free_sms_fragment_limit(service_id, form.free_sms_allowance.data) + return redirect(url_for('.service_settings', service_id=service_id)) return render_template( diff --git a/app/notify_client/billing_api_client.py b/app/notify_client/billing_api_client.py index 7e303ca7c..ab6e76e13 100644 --- a/app/notify_client/billing_api_client.py +++ b/app/notify_client/billing_api_client.py @@ -1,6 +1,4 @@ from app.notify_client import NotifyAdminAPIClient -from flask import current_app -from notifications_python_client.errors import HTTPError class BillingAPIClient(NotifyAdminAPIClient): @@ -27,44 +25,23 @@ class BillingAPIClient(NotifyAdminAPIClient): ) def get_free_sms_fragment_limit_for_year(self, service_id, year=None): - try: - if year is None: - result = self.get( - '/service/{0}/billing/free-sms-fragment-limit/current-year'.format(service_id) - ) - else: - result = self.get( - '/service/{0}/billing/free-sms-fragment-limit'.format(service_id), - params=dict(financial_year_start=year) - ) - return result['free_sms_fragment_limit'] - except HTTPError: - current_app.logger.info( - 'Requested free_sms_fragment_limit entry for service {0} and year {1} does not exist' - .format(service_id, year)) - return -1 + result = self.get( + '/service/{0}/billing/free-sms-fragment-limit'.format(service_id), + params=dict(financial_year_start=year) + ) + return result['free_sms_fragment_limit'] def get_free_sms_fragment_limit_for_all_years(self, service_id, year=None): - try: - return self.get( - '/service/{0}/billing/free-sms-fragment-limit'.format(service_id), - ) - except HTTPError: - current_app.logger.info( - 'No free_sms_fragment_limit entry exists for service {0} ' - .format(service_id, year)) - return [] + return self.get( + '/service/{0}/billing/free-sms-fragment-limit'.format(service_id)) + + def create_or_update_free_sms_fragment_limit(self, service_id, free_sms_fragment_limit, year=None): + # year = None will update current and future year in the API + data = { + "financial_year_start": year, + "free_sms_fragment_limit": free_sms_fragment_limit + } - def create_or_update_free_sms_fragment_limit_for_year(self, service_id, free_sms_fragment_limit, year=None): - if year is None: - data = { - "free_sms_fragment_limit": free_sms_fragment_limit, - } - else: - data = { - "financial_year_start": year, - "free_sms_fragment_limit": free_sms_fragment_limit, - } return self.post( url='/service/{0}/billing/free-sms-fragment-limit'.format(service_id), data=data diff --git a/tests/app/main/views/service_settings/test_service_setting_permissions.py b/tests/app/main/views/service_settings/test_service_setting_permissions.py index 0002e23bc..8996989a1 100644 --- a/tests/app/main/views/service_settings/test_service_setting_permissions.py +++ b/tests/app/main/views/service_settings/test_service_setting_permissions.py @@ -12,6 +12,7 @@ def get_service_settings_page( service_one, mock_get_inbound_number_for_service, mock_get_letter_organisations, + mock_get_free_sms_fragment_limit, no_reply_to_email_addresses, no_letter_contact_blocks, single_sms_sender, @@ -83,11 +84,12 @@ def test_service_setting_toggles_dont_show(get_service_settings_page, service_on def test_normal_user_doesnt_see_any_toggle_buttons( client_request, service_one, - mock_get_inbound_number_for_service, - mock_get_letter_organisations, no_reply_to_email_addresses, no_letter_contact_blocks, single_sms_sender, + mock_get_letter_organisations, + mock_get_inbound_number_for_service, + mock_get_free_sms_fragment_limit, ): page = client_request.get('main.service_settings', service_id=service_one['id']) toggles = page.find('a', {'class': 'button'}) diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index f78ee5b17..d8eb6c20e 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -595,7 +595,8 @@ def test_menu_send_messages( mock_get_template_statistics, mock_get_detailed_service, mock_get_usage, - mock_get_inbound_sms_summary + mock_get_inbound_sms_summary, + mock_get_free_sms_fragment_limit, ): with app_.test_request_context(): resp = _test_dashboard_menu( @@ -626,7 +627,8 @@ def test_menu_manage_service( mock_get_template_statistics, mock_get_detailed_service, mock_get_usage, - mock_get_inbound_sms_summary + mock_get_inbound_sms_summary, + mock_get_free_sms_fragment_limit, ): with app_.test_request_context(): resp = _test_dashboard_menu( @@ -656,7 +658,8 @@ def test_menu_manage_api_keys( mock_get_template_statistics, mock_get_detailed_service, mock_get_usage, - mock_get_inbound_sms_summary + mock_get_inbound_sms_summary, + mock_get_free_sms_fragment_limit, ): with app_.test_request_context(): resp = _test_dashboard_menu( @@ -686,7 +689,8 @@ def test_menu_all_services_for_platform_admin_user( mock_get_template_statistics, mock_get_detailed_service, mock_get_usage, - mock_get_inbound_sms_summary + mock_get_inbound_sms_summary, + mock_get_free_sms_fragment_limit, ): with app_.test_request_context(): resp = _test_dashboard_menu( diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index fa7ca8424..47e0092b9 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -32,6 +32,15 @@ from tests.conftest import ( from freezegun import freeze_time +@pytest.fixture +def mock_get_service_settings_page_common( + mock_get_letter_organisations, + mock_get_inbound_number_for_service, + mock_get_free_sms_fragment_limit, +): + return + + @pytest.mark.parametrize('user, expected_rows', [ (active_user_with_permissions, [ @@ -85,14 +94,12 @@ def test_should_show_overview( mocker, service_one, fake_uuid, - mock_get_letter_organisations, no_reply_to_email_addresses, no_letter_contact_blocks, single_sms_sender, user, expected_rows, - mock_get_inbound_number_for_service, - mock_get_free_sms_fragment_limit + mock_get_service_settings_page_common, ): service_one['permissions'] = ['sms', 'email'] @@ -161,8 +168,7 @@ def test_should_show_overview_for_service_with_more_things_set( single_letter_contact_block, single_sms_sender, mock_get_organisation, - mock_get_letter_organisations, - mock_get_inbound_number_for_service, + mock_get_service_settings_page_common, permissions, expected_rows ): @@ -184,7 +190,6 @@ def test_should_show_overview_for_service_with_more_things_set( def test_service_settings_show_elided_api_url_if_needed( logged_in_platform_admin_client, service_one, - mock_get_letter_organisations, single_reply_to_email_address, single_sms_sender, single_letter_contact_block, @@ -192,7 +197,7 @@ def test_service_settings_show_elided_api_url_if_needed( fake_uuid, url, elided_url, - mock_get_inbound_number_for_service + mock_get_service_settings_page_common, ): service_one['permissions'] = ['sms', 'email', 'inbound_sms'] service_one['inbound_api'] = [fake_uuid] @@ -221,8 +226,7 @@ def test_service_settings_show_elided_api_url_if_needed( def test_if_cant_send_letters_then_cant_see_letter_contact_block( logged_in_client, service_one, - mock_get_letter_organisations, - mock_get_inbound_number_for_service + mock_get_service_settings_page_common, ): response = logged_in_client.get(url_for( 'main.service_settings', service_id=service_one['id'] @@ -237,8 +241,7 @@ def test_letter_contact_block_shows_none_if_not_set( single_reply_to_email_address, no_letter_contact_blocks, single_sms_sender, - mock_get_letter_organisations, - mock_get_inbound_number_for_service + mock_get_service_settings_page_common, ): service_one['permissions'] = ['letter'] response = logged_in_client.get(url_for( @@ -258,8 +261,7 @@ def test_escapes_letter_contact_block( single_reply_to_email_address, single_sms_sender, injected_letter_contact_block, - mock_get_letter_organisations, - mock_get_inbound_number_for_service + mock_get_service_settings_page_common, ): service_one['permissions'] = ['letter'] response = logged_in_client.get(url_for( @@ -305,11 +307,10 @@ def test_should_redirect_after_change_service_name( def test_show_restricted_service( logged_in_client, service_one, - mock_get_letter_organisations, single_reply_to_email_address, single_letter_contact_block, single_sms_sender, - mock_get_inbound_number_for_service + mock_get_service_settings_page_common, ): response = logged_in_client.get(url_for('main.service_settings', service_id=service_one['id'])) page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') @@ -343,8 +344,7 @@ def test_show_live_service( single_reply_to_email_address, single_letter_contact_block, single_sms_sender, - mock_get_letter_organisations, - mock_get_inbound_number_for_service + mock_get_service_settings_page_common, ): response = logged_in_client.get(url_for('main.service_settings', service_id=service_one['id'])) page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') @@ -357,7 +357,7 @@ def test_switch_service_to_restricted( service_one, mock_get_live_service, mock_update_service, - mock_get_inbound_number_for_service + mock_get_inbound_number_for_service, ): response = logged_in_platform_admin_client.get( url_for('main.service_switch_live', service_id=service_one['id'])) @@ -406,7 +406,7 @@ def test_should_redirect_after_service_name_confirmation( service_one, mock_update_service, mock_verify_password, - mock_get_inbound_number_for_service + mock_get_inbound_number_for_service, ): service_id = service_one['id'] service_new_name = 'New Name' @@ -470,8 +470,7 @@ def test_should_redirect_after_request_to_go_live( single_reply_to_email_address, single_letter_contact_block, single_sms_sender, - mock_get_letter_organisations, - mock_get_inbound_number_for_service, + mock_get_service_settings_page_common ): mock_post = mocker.patch( 'app.main.views.feedback.requests.post', @@ -569,9 +568,8 @@ def test_route_permissions( single_reply_to_email_address, single_letter_contact_block, single_sms_sender, - mock_get_letter_organisations, route, - mock_get_inbound_number_for_service + mock_get_service_settings_page_common, ): validate_route_permission( mocker, @@ -629,9 +627,8 @@ def test_route_for_platform_admin( single_reply_to_email_address, single_letter_contact_block, single_sms_sender, - mock_get_letter_organisations, route, - mock_get_inbound_number_for_service + mock_get_service_settings_page_common, ): validate_route_permission(mocker, app_, @@ -699,11 +696,10 @@ def test_enabling_and_disabling_email_and_sms( def test_and_more_hint_appears_on_settings_with_more_than_just_a_single_sender( client_request, service_one, - mock_get_letter_organisations, - mock_get_inbound_number_for_service, multiple_reply_to_email_addresses, multiple_letter_contact_blocks, - multiple_sms_senders + multiple_sms_senders, + mock_get_service_settings_page_common, ): service_one['permissions'] = ['email', 'sms', 'letter'] @@ -1242,11 +1238,10 @@ def test_shows_research_mode_indicator( logged_in_client, service_one, mocker, - mock_get_letter_organisations, single_reply_to_email_address, single_letter_contact_block, single_sms_sender, - mock_get_inbound_number_for_service + mock_get_service_settings_page_common, ): service_one['research_mode'] = True mocker.patch('app.service_api_client.update_service_with_properties', return_value=service_one) @@ -1262,11 +1257,10 @@ def test_shows_research_mode_indicator( def test_does_not_show_research_mode_indicator( logged_in_client, service_one, - mock_get_letter_organisations, single_reply_to_email_address, single_letter_contact_block, single_sms_sender, - mock_get_inbound_number_for_service + mock_get_service_settings_page_common, ): response = logged_in_client.get(url_for('main.service_settings', service_id=service_one['id'])) assert response.status_code == 200 @@ -1577,6 +1571,7 @@ def test_should_set_organisation_type( def test_should_show_page_to_set_sms_allowance( logged_in_platform_admin_client, + mock_get_free_sms_fragment_limit ): response = logged_in_platform_admin_client.get(url_for( 'main.set_free_sms_allowance', @@ -1586,6 +1581,7 @@ def test_should_show_page_to_set_sms_allowance( page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert normalize_spaces(page.select_one('label').text) == 'Numbers of text message fragments per year' + mock_get_free_sms_fragment_limit.assert_called_once_with(SERVICE_ONE_ID) @freeze_time("2017-04-01 11:09:00.061258") @@ -1599,8 +1595,8 @@ def test_should_set_sms_allowance( mock_update_service, given_allowance, expected_api_argument, + mock_get_free_sms_fragment_limit, mock_create_or_update_free_sms_fragment_limit, - mock_get_free_sms_fragment_limit_for_all_years ): response = logged_in_platform_admin_client.post( @@ -1621,10 +1617,8 @@ def test_should_set_sms_allowance( ) mock_create_or_update_free_sms_fragment_limit.assert_called_with( SERVICE_ONE_ID, - expected_api_argument, - 2017 + expected_api_argument ) - mock_get_free_sms_fragment_limit_for_all_years.assert_called_once_with(SERVICE_ONE_ID) def test_switch_service_enable_letters( @@ -1843,8 +1837,7 @@ def test_archive_service_prompts_user( single_reply_to_email_address, single_letter_contact_block, single_sms_sender, - mock_get_letter_organisations, - mock_get_inbound_number_for_service + mock_get_service_settings_page_common, ): mocked_fn = mocker.patch('app.service_api_client.post') @@ -1862,8 +1855,7 @@ def test_cant_archive_inactive_service( single_reply_to_email_address, single_letter_contact_block, single_sms_sender, - mock_get_letter_organisations, - mock_get_inbound_number_for_service + mock_get_service_settings_page_common ): service_one['active'] = False @@ -1896,8 +1888,7 @@ def test_suspend_service_prompts_user( single_reply_to_email_address, single_letter_contact_block, single_sms_sender, - mock_get_letter_organisations, - mock_get_inbound_number_for_service + mock_get_service_settings_page_common, ): mocked_fn = mocker.patch('app.service_api_client.post') @@ -1916,8 +1907,7 @@ def test_cant_suspend_inactive_service( single_reply_to_email_address, single_letter_contact_block, single_sms_sender, - mock_get_letter_organisations, - mock_get_inbound_number_for_service + mock_get_service_settings_page_common, ): service_one['active'] = False @@ -1953,8 +1943,7 @@ def test_resume_service_prompts_user( single_letter_contact_block, single_sms_sender, mocker, - mock_get_letter_organisations, - mock_get_inbound_number_for_service + mock_get_service_settings_page_common, ): service_one['active'] = False mocked_fn = mocker.patch('app.service_api_client.post') @@ -1974,8 +1963,7 @@ def test_cant_resume_active_service( single_reply_to_email_address, single_letter_contact_block, single_sms_sender, - mock_get_letter_organisations, - mock_get_inbound_number_for_service + mock_get_service_settings_page_common ): response = logged_in_platform_admin_client.get(url_for('main.service_settings', service_id=service_one['id'])) @@ -2039,6 +2027,7 @@ def test_service_settings_when_inbound_number_is_not_set( single_sms_sender, mocker, mock_get_letter_organisations, + mock_get_free_sms_fragment_limit, ): mocker.patch('app.inbound_number_client.get_inbound_sms_number_for_service', return_value={'data': {}}) diff --git a/tests/app/notify_client/test_billing_client.py b/tests/app/notify_client/test_billing_client.py index 8e3d403a3..1ba7a4be0 100644 --- a/tests/app/notify_client/test_billing_client.py +++ b/tests/app/notify_client/test_billing_client.py @@ -27,17 +27,6 @@ def test_get_get_service_usage_calls_correct_endpoint(mocker, api_user_active): mock_get.assert_called_once_with(expected_url, params={'year': 2017}) -def test_get_free_sms_fragment_limit_for_current_year_correct_endpoint(mocker, api_user_active): - service_id = uuid.uuid4() - expected_url = '/service/{}/billing/free-sms-fragment-limit/current-year'.format(service_id) - client = BillingAPIClient() - - mock_get = mocker.patch('app.notify_client.billing_api_client.BillingAPIClient.get') - - client.get_free_sms_fragment_limit_for_year(service_id) - mock_get.assert_called_once_with(expected_url) - - def test_get_free_sms_fragment_limit_for_year_correct_endpoint(mocker, api_user_active): service_id = uuid.uuid4() expected_url = '/service/{}/billing/free-sms-fragment-limit'.format(service_id) @@ -51,11 +40,11 @@ def test_get_free_sms_fragment_limit_for_year_correct_endpoint(mocker, api_user_ def test_post_free_sms_fragment_limit_for_current_year_endpoint(mocker, api_user_active): service_id = uuid.uuid4() - sms_limit_data = {'free_sms_fragment_limit': 1111} + sms_limit_data = {'free_sms_fragment_limit': 1111, 'financial_year_start': None} mock_post = mocker.patch('app.notify_client.billing_api_client.BillingAPIClient.post') client = BillingAPIClient() - client.create_or_update_free_sms_fragment_limit_for_year(service_id=service_id, free_sms_fragment_limit=1111) + client.create_or_update_free_sms_fragment_limit(service_id=service_id, free_sms_fragment_limit=1111) mock_post.assert_called_once_with( url='/service/{}/billing/free-sms-fragment-limit'.format(service_id), @@ -69,9 +58,9 @@ def test_post_free_sms_fragment_limit_for_year_endpoint(mocker, api_user_active) mock_post = mocker.patch('app.notify_client.billing_api_client.BillingAPIClient.post') client = BillingAPIClient() - client.create_or_update_free_sms_fragment_limit_for_year(service_id=service_id, - free_sms_fragment_limit=1111, - year=2017) + client.create_or_update_free_sms_fragment_limit(service_id=service_id, + free_sms_fragment_limit=1111, + year=2017) mock_post.assert_called_once_with( url='/service/{}/billing/free-sms-fragment-limit'.format(service_id), data=sms_limit_data diff --git a/tests/conftest.py b/tests/conftest.py index a2966b7fa..8ea18f652 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2436,7 +2436,7 @@ def mock_get_free_sms_fragment_limit(mocker): @pytest.fixture(scope='function') def mock_create_or_update_free_sms_fragment_limit(mocker): sample_limit = 250000 - return mocker.patch('app.billing_api_client.create_or_update_free_sms_fragment_limit_for_year', + return mocker.patch('app.billing_api_client.create_or_update_free_sms_fragment_limit', return_value=sample_limit)