From 968d94d350423f05c8b41852da29fd8da93c221c Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 30 May 2019 17:41:25 +0100 Subject: [PATCH] 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):