From 208abfc722996a9c99df427f51437982c6f52406 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 30 May 2019 16:31:11 +0100 Subject: [PATCH 1/3] remove unused function --- app/dao/annual_billing_dao.py | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/app/dao/annual_billing_dao.py b/app/dao/annual_billing_dao.py index 744fc6106..0366e3826 100644 --- a/app/dao/annual_billing_dao.py +++ b/app/dao/annual_billing_dao.py @@ -51,16 +51,3 @@ def dao_get_all_free_sms_fragment_limit(service_id): return AnnualBilling.query.filter_by( service_id=service_id, ).order_by(AnnualBilling.financial_year_start).all() - - -def dao_insert_annual_billing_for_this_year(service, free_sms_fragment_limit): - """ - This method is called from create_service which is wrapped in a transaction. - """ - annual_billing = AnnualBilling( - free_sms_fragment_limit=free_sms_fragment_limit, - financial_year_start=get_current_financial_year_start_year(), - service=service, - ) - - db.session.add(annual_billing) From 968d94d350423f05c8b41852da29fd8da93c221c Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 30 May 2019 17:41:25 +0100 Subject: [PATCH 2/3] clean up dao_fetch_live_services a bit of DRY - use the column definitions to determine what goes into the dict, and use a `next` iterator rather than a while loop to find the existing service row. Take advantage of dict mutability to avoid needing to refer to the list by index. Also change the tests so if there's an error, the diff is slightly more readable. But not much --- app/dao/services_dao.py | 64 ++++++++++-------------------- tests/app/dao/test_services_dao.py | 5 ++- tests/app/service/test_rest.py | 59 ++++++++++++++++++++------- 3 files changed, 68 insertions(+), 60 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 6e1b6ceed..c5a0492c5 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -2,7 +2,7 @@ import uuid from datetime import date, datetime, timedelta from notifications_utils.statsd_decorators import statsd -from sqlalchemy import asc, func, case +from sqlalchemy.sql.expression import asc, case, func from sqlalchemy.orm import joinedload from flask import current_app @@ -77,25 +77,25 @@ def dao_count_live_services(): def dao_fetch_live_services_data(): year_start_date, year_end_date = get_current_financial_year() + this_year_ft_billing = FactBilling.query.filter( FactBilling.bst_date >= year_start_date, FactBilling.bst_date <= year_end_date, ).subquery() + data = db.session.query( - Service.id, - Organisation.name.label("organisation_name"), - Organisation.organisation_type, + Service.id.label('service_id'), Service.name.label("service_name"), - Service.consent_to_research, - Service.go_live_user_id, - Service.count_as_live, - User.name.label('user_name'), - User.email_address, - User.mobile_number, + Organisation.name.label("organisation_name"), + Organisation.organisation_type.label('organisation_type'), + Service.consent_to_research.label('consent_to_research'), + User.name.label('contact_name'), + User.email_address.label('contact_email'), + User.mobile_number.label('contact_mobile'), Service.go_live_at.label("live_date"), - Service.volume_sms, - Service.volume_email, - Service.volume_letter, + Service.volume_sms.label('sms_volume_intent'), + Service.volume_email.label('email_volume_intent'), + Service.volume_letter.label('letter_volume_intent'), case([ (this_year_ft_billing.c.notification_type == 'email', func.sum(this_year_ft_billing.c.notifications_sent)) ], else_=0).label("email_totals"), @@ -130,42 +130,20 @@ def dao_fetch_live_services_data(): Service.volume_sms, Service.volume_email, Service.volume_letter, - this_year_ft_billing.c.notification_type + this_year_ft_billing.c.notification_type, ).order_by( asc(Service.go_live_at) ).all() results = [] for row in data: - is_service_in_list = None - i = 0 - while i < len(results): - if results[i]["service_id"] == row.id: - is_service_in_list = i - break - else: - i += 1 - if is_service_in_list is not None: - results[is_service_in_list]["email_totals"] += row.email_totals - results[is_service_in_list]["sms_totals"] += row.sms_totals - results[is_service_in_list]["letter_totals"] += row.letter_totals + existing_service = next((x for x in results if x['service_id'] == row.service_id), None) + + if existing_service is not None: + existing_service["email_totals"] += row.email_totals + existing_service["sms_totals"] += row.sms_totals + existing_service["letter_totals"] += row.letter_totals else: - results.append({ - "service_id": row.id, - "service_name": row.service_name, - "organisation_name": row.organisation_name, - "organisation_type": row.organisation_type, - "consent_to_research": row.consent_to_research, - "contact_name": row.user_name, - "contact_email": row.email_address, - "contact_mobile": row.mobile_number, - "live_date": row.live_date, - "sms_volume_intent": row.volume_sms, - "email_volume_intent": row.volume_email, - "letter_volume_intent": row.volume_letter, - "sms_totals": row.sms_totals, - "email_totals": row.email_totals, - "letter_totals": row.letter_totals, - }) + results.append(row._asdict()) return results diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index eab886f3f..8e199ae0e 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -1,5 +1,6 @@ import uuid from datetime import datetime +from unittest import mock import pytest from freezegun import freeze_time @@ -386,11 +387,11 @@ def test_get_all_user_services_should_return_empty_list_if_no_services_for_user( @freeze_time('2019-04-23T10:00:00') -def test_dao_fetch_live_services_data(sample_user, mock): +def test_dao_fetch_live_services_data(sample_user): org = create_organisation(organisation_type='crown') service = create_service(go_live_user=sample_user, go_live_at='2014-04-20T10:00:00') template = create_template(service=service) - service_2 = create_service(service_name='second', go_live_user=sample_user, go_live_at='2017-04-20T10:00:00') + service_2 = create_service(service_name='second', go_live_at='2017-04-20T10:00:00', go_live_user=sample_user) create_service(service_name='third', go_live_at='2016-04-20T10:00:00') # below services should be filtered out: create_service(service_name='restricted', restricted=True) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 7afb7786f..9d450dfc7 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -137,28 +137,57 @@ def test_get_service_list_should_return_empty_list_if_no_services(admin_request) assert len(json_resp['data']) == 0 -def test_get_live_services_data(sample_user, admin_request, mock): +def test_get_live_services_data(sample_user, admin_request): org = create_organisation() - service = create_service(go_live_user=sample_user) + + service = create_service(go_live_user=sample_user, go_live_at=datetime(2018, 1, 1)) + create_service(service_name='second', go_live_at=datetime(2019, 1, 1), go_live_user=sample_user) + template = create_template(service=service) - create_service(service_name='second', go_live_user=sample_user) template2 = create_template(service=service, template_type='email') dao_add_service_to_organisation(service=service, organisation_id=org.id) create_ft_billing(bst_date='2019-04-20', notification_type='sms', template=template, service=service) - create_ft_billing(bst_date='2019-04-20', notification_type='email', template=template2, - service=service) + create_ft_billing(bst_date='2019-04-20', notification_type='email', template=template2, service=service) + response = admin_request.get('service.get_live_services_data')["data"] + assert len(response) == 2 - assert {'consent_to_research': None, 'contact_email': 'notify@digital.cabinet-office.gov.uk', - 'contact_mobile': '+447700900986', 'contact_name': 'Test User', 'email_totals': 0, - 'email_volume_intent': None, 'letter_totals': 0, 'letter_volume_intent': None, 'live_date': None, - 'organisation_name': None, 'service_id': mock.ANY, 'service_name': 'second', 'sms_totals': 0, - 'sms_volume_intent': None, 'organisation_type': None} in response - assert {'consent_to_research': None, 'contact_email': 'notify@digital.cabinet-office.gov.uk', - 'contact_mobile': '+447700900986', 'contact_name': 'Test User', 'email_totals': 1, - 'email_volume_intent': None, 'letter_totals': 0, 'letter_volume_intent': None, 'live_date': None, - 'organisation_name': 'test_org_1', 'service_id': mock.ANY, 'service_name': 'Sample service', - 'sms_totals': 1, 'sms_volume_intent': None, 'organisation_type': None} in response + assert response == [ + { + 'consent_to_research': None, + 'contact_email': 'notify@digital.cabinet-office.gov.uk', + 'contact_mobile': '+447700900986', + 'contact_name': 'Test User', + 'email_totals': 1, + 'email_volume_intent': None, + 'letter_totals': 0, + 'letter_volume_intent': None, + 'live_date': 'Mon, 01 Jan 2018 00:00:00 GMT', + 'organisation_name': 'test_org_1', + 'service_id': ANY, + 'service_name': 'Sample service', + 'sms_totals': 1, + 'sms_volume_intent': None, + 'organisation_type': None, + }, + { + 'consent_to_research': None, + 'contact_email': 'notify@digital.cabinet-office.gov.uk', + 'contact_mobile': '+447700900986', + 'contact_name': 'Test User', + 'email_totals': 0, + 'email_volume_intent': None, + 'letter_totals': 0, + 'letter_volume_intent': None, + 'live_date': 'Tue, 01 Jan 2019 00:00:00 GMT', + 'organisation_name': None, + 'service_id': ANY, + 'service_name': 'second', + 'sms_totals': 0, + 'sms_volume_intent': None, + 'organisation_type': None, + }, + ] def test_get_service_by_id(admin_request, sample_service): From 7db2b031a23006916dfab5fada33df61ef3a9e4c Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 30 May 2019 17:47:00 +0100 Subject: [PATCH 3/3] add free_sms_fragment_limit to platform admin billing report a little complicated because the free_sms_fragment_limit comes from the annual_billing table. This relies on there always being at least one row for every service on annual billing - I checked on prod and that is true. Join to the annual billing table, then join to a subquery getting the latest year for that service to extract only the most recent year. --- app/dao/services_dao.py | 19 ++++++++++++++++++- tests/app/dao/test_services_dao.py | 22 +++++++++++++++++----- tests/app/service/test_rest.py | 8 +++++++- 3 files changed, 42 insertions(+), 7 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index c5a0492c5..2523a4d2a 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -2,7 +2,7 @@ import uuid from datetime import date, datetime, timedelta from notifications_utils.statsd_decorators import statsd -from sqlalchemy.sql.expression import asc, case, func +from sqlalchemy.sql.expression import asc, case, and_, func from sqlalchemy.orm import joinedload from flask import current_app @@ -78,6 +78,13 @@ def dao_count_live_services(): def dao_fetch_live_services_data(): year_start_date, year_end_date = get_current_financial_year() + most_recent_annual_billing = db.session.query( + AnnualBilling.service_id, + func.max(AnnualBilling.financial_year_start).label('year') + ).group_by( + AnnualBilling.service_id + ).subquery() + this_year_ft_billing = FactBilling.query.filter( FactBilling.bst_date >= year_start_date, FactBilling.bst_date <= year_end_date, @@ -105,6 +112,15 @@ def dao_fetch_live_services_data(): case([ (this_year_ft_billing.c.notification_type == 'letter', func.sum(this_year_ft_billing.c.notifications_sent)) ], else_=0).label("letter_totals"), + AnnualBilling.free_sms_fragment_limit, + ).join( + Service.annual_billing + ).join( + most_recent_annual_billing, + and_( + Service.id == most_recent_annual_billing.c.service_id, + AnnualBilling.financial_year_start == most_recent_annual_billing.c.year + ) ).outerjoin( Service.organisation ).outerjoin( @@ -131,6 +147,7 @@ def dao_fetch_live_services_data(): Service.volume_email, Service.volume_letter, this_year_ft_billing.c.notification_type, + AnnualBilling.free_sms_fragment_limit, ).order_by( asc(Service.go_live_at) ).all() diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 8e199ae0e..4b9b6b774 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -74,7 +74,8 @@ from tests.app.db import ( create_invited_user, create_email_branding, create_letter_branding, - create_notification_history + create_notification_history, + create_annual_billing, ) @@ -392,7 +393,7 @@ def test_dao_fetch_live_services_data(sample_user): service = create_service(go_live_user=sample_user, go_live_at='2014-04-20T10:00:00') template = create_template(service=service) service_2 = create_service(service_name='second', go_live_at='2017-04-20T10:00:00', go_live_user=sample_user) - create_service(service_name='third', go_live_at='2016-04-20T10:00:00') + service_3 = create_service(service_name='third', go_live_at='2016-04-20T10:00:00') # below services should be filtered out: create_service(service_name='restricted', restricted=True) create_service(service_name='not_active', active=False) @@ -413,6 +414,14 @@ def test_dao_fetch_live_services_data(sample_user): # one letter billing record for 2nd service within current financial year: create_ft_billing(bst_date='2019-04-16', notification_type='letter', template=template_letter_2, service=service_2) + # 1st service: billing from 2018 and 2019 + create_annual_billing(service.id, 500, 2018) + create_annual_billing(service.id, 100, 2019) + # 2nd service: billing from 2018 + create_annual_billing(service_2.id, 300, 2018) + # 3rd service: billing from 2019 + create_annual_billing(service_3.id, 200, 2019) + results = dao_fetch_live_services_data() assert len(results) == 3 # checks the results and that they are ordered by date: @@ -421,17 +430,20 @@ def test_dao_fetch_live_services_data(sample_user): 'organisation_type': 'crown', 'consent_to_research': None, 'contact_name': 'Test User', 'contact_email': 'notify@digital.cabinet-office.gov.uk', 'contact_mobile': '+447700900986', 'live_date': datetime(2014, 4, 20, 10, 0), 'sms_volume_intent': None, 'email_volume_intent': None, - 'letter_volume_intent': None, 'sms_totals': 2, 'email_totals': 1, 'letter_totals': 1}, + 'letter_volume_intent': None, 'sms_totals': 2, 'email_totals': 1, 'letter_totals': 1, + 'free_sms_fragment_limit': 100}, {'service_id': mock.ANY, 'service_name': 'third', 'organisation_name': None, 'consent_to_research': None, 'organisation_type': None, 'contact_name': None, 'contact_email': None, 'contact_mobile': None, 'live_date': datetime(2016, 4, 20, 10, 0), 'sms_volume_intent': None, 'email_volume_intent': None, 'letter_volume_intent': None, - 'sms_totals': 0, 'email_totals': 0, 'letter_totals': 0}, + 'sms_totals': 0, 'email_totals': 0, 'letter_totals': 0, + 'free_sms_fragment_limit': 200}, {'service_id': mock.ANY, 'service_name': 'second', 'organisation_name': None, 'consent_to_research': None, 'contact_name': 'Test User', 'contact_email': 'notify@digital.cabinet-office.gov.uk', 'contact_mobile': '+447700900986', 'live_date': datetime(2017, 4, 20, 10, 0), 'sms_volume_intent': None, 'organisation_type': None, 'email_volume_intent': None, 'letter_volume_intent': None, - 'sms_totals': 0, 'email_totals': 0, 'letter_totals': 1} + 'sms_totals': 0, 'email_totals': 0, 'letter_totals': 1, + 'free_sms_fragment_limit': 300} ] diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 9d450dfc7..0ba6c15a8 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -51,6 +51,7 @@ from tests.app.db import ( create_organisation, create_domain, create_email_branding, + create_annual_billing, ) from tests.app.db import create_user @@ -141,7 +142,7 @@ def test_get_live_services_data(sample_user, admin_request): org = create_organisation() service = create_service(go_live_user=sample_user, go_live_at=datetime(2018, 1, 1)) - create_service(service_name='second', go_live_at=datetime(2019, 1, 1), go_live_user=sample_user) + service_2 = create_service(service_name='second', go_live_at=datetime(2019, 1, 1), go_live_user=sample_user) template = create_template(service=service) template2 = create_template(service=service, template_type='email') @@ -149,6 +150,9 @@ def test_get_live_services_data(sample_user, admin_request): create_ft_billing(bst_date='2019-04-20', notification_type='sms', template=template, service=service) create_ft_billing(bst_date='2019-04-20', notification_type='email', template=template2, service=service) + create_annual_billing(service.id, 1, 2019) + create_annual_billing(service_2.id, 2, 2018) + response = admin_request.get('service.get_live_services_data')["data"] assert len(response) == 2 @@ -169,6 +173,7 @@ def test_get_live_services_data(sample_user, admin_request): 'sms_totals': 1, 'sms_volume_intent': None, 'organisation_type': None, + 'free_sms_fragment_limit': 1 }, { 'consent_to_research': None, @@ -186,6 +191,7 @@ def test_get_live_services_data(sample_user, admin_request): 'sms_totals': 0, 'sms_volume_intent': None, 'organisation_type': None, + 'free_sms_fragment_limit': 2 }, ]