From 6f41f6c7d74474d286c291923011d4f7843485ff Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 3 Apr 2019 13:07:49 +0100 Subject: [PATCH 1/4] use db models instead of tuples when referring to rate objects makes it less confusing --- app/dao/fact_billing_dao.py | 23 ++++++++++----- tests/app/celery/test_reporting_tasks.py | 36 ++++++++---------------- tests/app/db.py | 19 +++++++++++++ 3 files changed, 46 insertions(+), 32 deletions(-) diff --git a/app/dao/fact_billing_dao.py b/app/dao/fact_billing_dao.py index 44b1786db..9ab56f861 100644 --- a/app/dao/fact_billing_dao.py +++ b/app/dao/fact_billing_dao.py @@ -209,10 +209,8 @@ def fetch_billing_data_for_day(process_day, service_id=None): def get_rates_for_billing(): - non_letter_rates = [(r.notification_type, r.valid_from, r.rate) for r in - Rate.query.order_by(desc(Rate.valid_from)).all()] - letter_rates = [(r.start_date, r.crown, r.sheet_count, r.rate, r.post_class) for r in - LetterRate.query.order_by(desc(LetterRate.start_date)).all()] + non_letter_rates = Rate.query.order_by(desc(Rate.valid_from)).all() + letter_rates = LetterRate.query.order_by(desc(LetterRate.start_date)).all() return non_letter_rates, letter_rates @@ -234,11 +232,22 @@ def get_rate( if letter_page_count == 0: return 0 return next( - r[3] for r in letter_rates if date >= r[0] and crown == r[1] - and letter_page_count == r[2] and post_class == r[4] + r.rate + for r in letter_rates if ( + date >= r.start_date and + crown == r.crown and + letter_page_count == r.sheet_count and + post_class == r.post_class + ) ) elif notification_type == SMS_TYPE: - return next(r[2] for r in non_letter_rates if notification_type == r[0] and date >= r[1]) + return next( + r.rate + for r in non_letter_rates if ( + notification_type == r.notification_type and + date >= r.valid_from + ) + ) else: return 0 diff --git a/tests/app/celery/test_reporting_tasks.py b/tests/app/celery/test_reporting_tasks.py index 7784dcf67..9d3b0bedd 100644 --- a/tests/app/celery/test_reporting_tasks.py +++ b/tests/app/celery/test_reporting_tasks.py @@ -14,10 +14,8 @@ from app.models import ( EMAIL_TYPE, SMS_TYPE, FactNotificationStatus ) -from app.models import LetterRate, Rate -from app import db -from tests.app.db import create_service, create_template, create_notification +from tests.app.db import create_service, create_template, create_notification, create_rate, create_letter_rate def mocker_get_rate( @@ -322,38 +320,26 @@ def test_create_nightly_billing_consolidate_from_3_days_delta( def test_get_rate_for_letter_latest(notify_db_session): - non_letter_rates = [(r.notification_type, r.valid_from, r.rate) for r in - Rate.query.order_by(desc(Rate.valid_from)).all()] - # letter rates should be passed into the get_rate function as a tuple of start_date, crown, sheet_count, # rate and post_class - new_letter_rate = (datetime(2017, 12, 1), True, 1, Decimal(0.33), 'second') - old_letter_rate = (datetime(2016, 12, 1), True, 1, Decimal(0.30), 'second') - letter_rates = [new_letter_rate, old_letter_rate] + new = create_letter_rate(datetime(2017, 12, 1), crown=True, sheet_count=1, rate=0.33, post_class='second') + old = create_letter_rate(datetime(2016, 12, 1), crown=True, sheet_count=1, rate=0.30, post_class='second') + letter_rates = [new, old] - rate = get_rate(non_letter_rates, letter_rates, LETTER_TYPE, datetime(2018, 1, 1), True, 1) + rate = get_rate([], letter_rates, LETTER_TYPE, datetime(2018, 1, 1), True, 1) assert rate == Decimal(0.33) def test_get_rate_for_sms_and_email(notify_db_session): - sms_rate = Rate(valid_from=datetime(2017, 12, 1), - rate=Decimal(0.15), - notification_type=SMS_TYPE) - db.session.add(sms_rate) - email_rate = Rate(valid_from=datetime(2017, 12, 1), - rate=Decimal(0), - notification_type=EMAIL_TYPE) - db.session.add(email_rate) + non_letter_rates = [ + create_rate(datetime(2017, 12, 1), 0.15, SMS_TYPE) + create_rate(datetime(2017, 12, 1), 0, EMAIL_TYPE) + ] - non_letter_rates = [(r.notification_type, r.valid_from, r.rate) for r in - Rate.query.order_by(desc(Rate.valid_from)).all()] - letter_rates = [(r.start_date, r.crown, r.sheet_count, r.rate) for r in - LetterRate.query.order_by(desc(LetterRate.start_date)).all()] - - rate = get_rate(non_letter_rates, letter_rates, SMS_TYPE, datetime(2018, 1, 1)) + rate = get_rate(non_letter_rates, [], SMS_TYPE, datetime(2018, 1, 1)) assert rate == Decimal(0.15) - rate = get_rate(non_letter_rates, letter_rates, EMAIL_TYPE, datetime(2018, 1, 1)) + rate = get_rate(non_letter_rates, [], EMAIL_TYPE, datetime(2018, 1, 1)) assert rate == Decimal(0) diff --git a/tests/app/db.py b/tests/app/db.py index 8d34e8b80..c7493dc1f 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -1,6 +1,7 @@ import random import uuid from datetime import datetime, date +from decimal import Decimal from app import db from app.dao.email_branding_dao import dao_create_email_branding @@ -29,6 +30,7 @@ from app.models import ( Job, Notification, EmailBranding, + LetterRate, Organisation, Rate, Service, @@ -403,6 +405,23 @@ def create_rate(start_date, value, notification_type): return rate +def create_letter_rate(start_date=None, end_date=None, crown=True, sheet_count=1, rate=0.33, post_class='second'): + if start_date is None: + start_date = datetime(2016, 1, 1) + rate = LetterRate( + id=uuid.uuid4(), + start_date=start_date, + end_date=end_date, + crown=crown, + sheet_count=sheet_count, + rate=Decimal(rate), + post_class=post_class + ) + db.session.add(rate) + db.session.commit() + return rate + + def create_api_key(service, key_type=KEY_TYPE_NORMAL): id_ = uuid.uuid4() api_key = ApiKey( From 9f1f8589976b0ef62c2e837985f8f2b0ef015414 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 3 Apr 2019 14:52:41 +0100 Subject: [PATCH 2/4] update fact_billing_dao::get_rate to use dates not datetimes update unit tests too --- app/dao/fact_billing_dao.py | 7 +++++-- tests/app/celery/test_reporting_tasks.py | 11 +++++------ tests/app/dao/test_ft_billing_dao.py | 14 ++++++++------ 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/app/dao/fact_billing_dao.py b/app/dao/fact_billing_dao.py index 9ab56f861..8997626fb 100644 --- a/app/dao/fact_billing_dao.py +++ b/app/dao/fact_billing_dao.py @@ -21,6 +21,7 @@ from app.models import ( EMAIL_TYPE, NOTIFICATION_STATUS_TYPES_BILLABLE_FOR_LETTERS ) +from app.utils import get_london_midnight_in_utc def fetch_billing_totals_for_year(service_id, year): @@ -228,13 +229,15 @@ def get_service_ids_that_need_billing_populated(start_date, end_date): def get_rate( non_letter_rates, letter_rates, notification_type, date, crown=None, letter_page_count=None, post_class='second' ): + start_of_day = get_london_midnight_in_utc(date) + if notification_type == LETTER_TYPE: if letter_page_count == 0: return 0 return next( r.rate for r in letter_rates if ( - date >= r.start_date and + start_of_day >= r.start_date and crown == r.crown and letter_page_count == r.sheet_count and post_class == r.post_class @@ -245,7 +248,7 @@ def get_rate( r.rate for r in non_letter_rates if ( notification_type == r.notification_type and - date >= r.valid_from + start_of_day >= r.valid_from ) ) else: diff --git a/tests/app/celery/test_reporting_tasks.py b/tests/app/celery/test_reporting_tasks.py index 9d3b0bedd..ed87fb285 100644 --- a/tests/app/celery/test_reporting_tasks.py +++ b/tests/app/celery/test_reporting_tasks.py @@ -3,7 +3,6 @@ from decimal import Decimal import pytest from freezegun import freeze_time -from sqlalchemy import desc from app.celery.reporting_tasks import create_nightly_billing, create_nightly_notification_status from app.dao.fact_billing_dao import get_rate @@ -19,7 +18,7 @@ from tests.app.db import create_service, create_template, create_notification, c def mocker_get_rate( - non_letter_rates, letter_rates, notification_type, date, crown=None, rate_multiplier=None, post_class="second" + non_letter_rates, letter_rates, notification_type, bst_date, crown=None, rate_multiplier=None, post_class="second" ): if notification_type == LETTER_TYPE: return Decimal(2.1) @@ -326,20 +325,20 @@ def test_get_rate_for_letter_latest(notify_db_session): old = create_letter_rate(datetime(2016, 12, 1), crown=True, sheet_count=1, rate=0.30, post_class='second') letter_rates = [new, old] - rate = get_rate([], letter_rates, LETTER_TYPE, datetime(2018, 1, 1), True, 1) + rate = get_rate([], letter_rates, LETTER_TYPE, date(2018, 1, 1), True, 1) assert rate == Decimal(0.33) def test_get_rate_for_sms_and_email(notify_db_session): non_letter_rates = [ - create_rate(datetime(2017, 12, 1), 0.15, SMS_TYPE) + create_rate(datetime(2017, 12, 1), 0.15, SMS_TYPE), create_rate(datetime(2017, 12, 1), 0, EMAIL_TYPE) ] - rate = get_rate(non_letter_rates, [], SMS_TYPE, datetime(2018, 1, 1)) + rate = get_rate(non_letter_rates, [], SMS_TYPE, date(2018, 1, 1)) assert rate == Decimal(0.15) - rate = get_rate(non_letter_rates, [], EMAIL_TYPE, datetime(2018, 1, 1)) + rate = get_rate(non_letter_rates, [], EMAIL_TYPE, date(2018, 1, 1)) assert rate == Decimal(0) diff --git a/tests/app/dao/test_ft_billing_dao.py b/tests/app/dao/test_ft_billing_dao.py index f36e02180..7b8760904 100644 --- a/tests/app/dao/test_ft_billing_dao.py +++ b/tests/app/dao/test_ft_billing_dao.py @@ -1,7 +1,7 @@ from calendar import monthrange from decimal import Decimal -from datetime import datetime, timedelta +from datetime import datetime, timedelta, date from freezegun import freeze_time import pytest @@ -288,18 +288,20 @@ def test_get_rates_for_billing(notify_db_session): assert len(letter_rates) == 29 +@freeze_time('2017-06-01 12:00') def test_get_rate(notify_db_session): - create_rate(start_date=datetime.utcnow(), value=1.2, notification_type='email') - create_rate(start_date=datetime.utcnow(), value=2.2, notification_type='sms') - create_rate(start_date=datetime.utcnow(), value=3.3, notification_type='email') + create_rate(start_date=datetime(2017, 5, 30, 23, 00), value=1.2, notification_type='email') + create_rate(start_date=datetime(2017, 5, 30, 23, 00), value=2.2, notification_type='sms') + create_rate(start_date=datetime(2017, 5, 30, 23, 00), value=3.3, notification_type='email') + non_letter_rates, letter_rates = get_rates_for_billing() rate = get_rate(non_letter_rates=non_letter_rates, letter_rates=letter_rates, notification_type='sms', - date=datetime.utcnow()) + date=date(2017, 6, 1)) letter_rate = get_rate(non_letter_rates=non_letter_rates, letter_rates=letter_rates, notification_type='letter', crown=True, letter_page_count=1, - date=datetime.utcnow()) + date=date(2017, 6, 1)) assert rate == 2.2 assert letter_rate == Decimal('0.3') From 28bff287868566d60e9df327875e6f798a61882a Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 3 Apr 2019 15:34:02 +0100 Subject: [PATCH 3/4] remove letter rates between tests we only use them in ft_billing so no reason not to delete them. makes the tests read better as it's obvious how they work too. --- tests/app/celery/test_reporting_tasks.py | 2 +- tests/app/dao/test_ft_billing_dao.py | 21 ++++++++++++++++----- tests/app/db.py | 3 +-- tests/conftest.py | 1 - 4 files changed, 18 insertions(+), 9 deletions(-) diff --git a/tests/app/celery/test_reporting_tasks.py b/tests/app/celery/test_reporting_tasks.py index ed87fb285..3e4c4ec4f 100644 --- a/tests/app/celery/test_reporting_tasks.py +++ b/tests/app/celery/test_reporting_tasks.py @@ -326,7 +326,7 @@ def test_get_rate_for_letter_latest(notify_db_session): letter_rates = [new, old] rate = get_rate([], letter_rates, LETTER_TYPE, date(2018, 1, 1), True, 1) - assert rate == Decimal(0.33) + assert rate == Decimal('0.33') def test_get_rate_for_sms_and_email(notify_db_session): diff --git a/tests/app/dao/test_ft_billing_dao.py b/tests/app/dao/test_ft_billing_dao.py index 7b8760904..43a23dc42 100644 --- a/tests/app/dao/test_ft_billing_dao.py +++ b/tests/app/dao/test_ft_billing_dao.py @@ -28,6 +28,7 @@ from tests.app.db import ( create_template, create_notification, create_rate, + create_letter_rate ) @@ -95,7 +96,7 @@ def test_fetch_billing_data_for_today_includes_data_with_the_right_key_type(noti def test_fetch_billing_data_for_today_includes_data_with_the_right_date(notify_db_session): - process_day = datetime(2018, 4, 1, 13, 30, 00) + process_day = datetime(2018, 4, 1, 13, 30, 0) service = create_service() template = create_template(service=service, template_type="email") create_notification(template=template, status='delivered', created_at=process_day) @@ -282,17 +283,21 @@ def test_get_rates_for_billing(notify_db_session): create_rate(start_date=datetime.utcnow(), value=12, notification_type='email') create_rate(start_date=datetime.utcnow(), value=22, notification_type='sms') create_rate(start_date=datetime.utcnow(), value=33, notification_type='email') + create_letter_rate(start_date=datetime.utcnow(), rate=0.66, post_class='first') + create_letter_rate(start_date=datetime.utcnow(), rate=0.33, post_class='second') non_letter_rates, letter_rates = get_rates_for_billing() assert len(non_letter_rates) == 3 - assert len(letter_rates) == 29 + assert len(letter_rates) == 2 @freeze_time('2017-06-01 12:00') def test_get_rate(notify_db_session): - create_rate(start_date=datetime(2017, 5, 30, 23, 00), value=1.2, notification_type='email') - create_rate(start_date=datetime(2017, 5, 30, 23, 00), value=2.2, notification_type='sms') - create_rate(start_date=datetime(2017, 5, 30, 23, 00), value=3.3, notification_type='email') + create_rate(start_date=datetime(2017, 5, 30, 23, 0), value=1.2, notification_type='email') + create_rate(start_date=datetime(2017, 5, 30, 23, 0), value=2.2, notification_type='sms') + create_rate(start_date=datetime(2017, 5, 30, 23, 0), value=3.3, notification_type='email') + create_letter_rate(start_date=datetime(2017, 5, 30, 23, 0), rate=0.66, post_class='first') + create_letter_rate(start_date=datetime(2017, 5, 30, 23, 0), rate=0.3, post_class='second') non_letter_rates, letter_rates = get_rates_for_billing() rate = get_rate(non_letter_rates=non_letter_rates, letter_rates=letter_rates, notification_type='sms', @@ -309,6 +314,9 @@ def test_get_rate(notify_db_session): @pytest.mark.parametrize("letter_post_class,expected_rate", [("first", "0.61"), ("second", "0.35")]) def test_get_rate_filters_letters_by_post_class(notify_db_session, letter_post_class, expected_rate): + create_letter_rate(start_date=datetime(2017, 5, 30, 23, 0), sheet_count=2, rate=0.61, post_class='first') + create_letter_rate(start_date=datetime(2017, 5, 30, 23, 0), sheet_count=2, rate=0.35, post_class='second') + non_letter_rates, letter_rates = get_rates_for_billing() rate = get_rate(non_letter_rates, letter_rates, "letter", datetime(2018, 10, 1), True, 2, letter_post_class) assert rate == Decimal(expected_rate) @@ -316,6 +324,9 @@ def test_get_rate_filters_letters_by_post_class(notify_db_session, letter_post_c @pytest.mark.parametrize("date,expected_rate", [(datetime(2018, 9, 30), '0.33'), (datetime(2018, 10, 1), '0.35')]) def test_get_rate_chooses_right_rate_depending_on_date(notify_db_session, date, expected_rate): + create_letter_rate(start_date=datetime(2016, 1, 1, 0, 0), sheet_count=2, rate=0.33, post_class='second') + create_letter_rate(start_date=datetime(2018, 9, 30, 23, 0), sheet_count=2, rate=0.35, post_class='second') + non_letter_rates, letter_rates = get_rates_for_billing() rate = get_rate(non_letter_rates, letter_rates, "letter", date, True, 2, "second") assert rate == Decimal(expected_rate) diff --git a/tests/app/db.py b/tests/app/db.py index c7493dc1f..94ca9325b 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -1,7 +1,6 @@ import random import uuid from datetime import datetime, date -from decimal import Decimal from app import db from app.dao.email_branding_dao import dao_create_email_branding @@ -414,7 +413,7 @@ def create_letter_rate(start_date=None, end_date=None, crown=True, sheet_count=1 end_date=end_date, crown=crown, sheet_count=sheet_count, - rate=Decimal(rate), + rate=rate, post_class=post_class ) db.session.add(rate) diff --git a/tests/conftest.py b/tests/conftest.py index 73dea5864..f8c6f798a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -105,7 +105,6 @@ def notify_db_session(notify_db): "service_permission_types", "auth_type", "invite_status_type", - "letter_rates", "service_callback_type"]: notify_db.engine.execute(tbl.delete()) notify_db.session.commit() From 1c6b291a42529c8a1c3d187816aca31c5f044005 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 3 Apr 2019 15:47:53 +0100 Subject: [PATCH 4/4] we cant reason about the order of sets when iterated split out assert into three parts --- tests/app/celery/test_ftp_update_tasks.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/app/celery/test_ftp_update_tasks.py b/tests/app/celery/test_ftp_update_tasks.py index 22367974e..39c25725a 100644 --- a/tests/app/celery/test_ftp_update_tasks.py +++ b/tests/app/celery/test_ftp_update_tasks.py @@ -276,9 +276,10 @@ def test_record_daily_sorted_counts_raises_dvla_exception_with_unknown_sorted_st filename = "failed.txt" with pytest.raises(DVLAException) as e: record_daily_sorted_counts(filename=filename) - unknown_values = set({'invalid', 'mm'}) - assert "DVLA response file: {} contains unknown Sorted status {}".format( - filename, unknown_values.__repr__()) == e.value.message + + assert "DVLA response file: {} contains unknown Sorted status".format(filename) in e.value.message + assert "'mm'" in e.value.message + assert "'invalid'" in e.value.message def test_record_daily_sorted_counts_persists_daily_sorted_letter_count_with_no_sorted_values(