From 6fb4e1606773b4430a9474af5742dc6afed3850b Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 12 Jul 2017 14:19:39 +0100 Subject: [PATCH 01/22] Added logging to show the entire form posted to us by the SMS client providers. This can be useful information when debugging what happened to a notificaiton. Recently there was a discrepancy between the failure type used by each provider for a particular number, this logging would have helped. --- app/notifications/notifications_sms_callback.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/app/notifications/notifications_sms_callback.py b/app/notifications/notifications_sms_callback.py index 3c569e158..430e1c149 100644 --- a/app/notifications/notifications_sms_callback.py +++ b/app/notifications/notifications_sms_callback.py @@ -1,4 +1,5 @@ from flask import Blueprint +from flask import current_app from flask import json from flask import request, jsonify @@ -22,6 +23,10 @@ def process_mmg_response(): success, errors = process_sms_client_response(status=str(data.get('status')), reference=data.get('CID'), client_name=client_name) + + current_app.logger.info( + "Full delivery response from {} for notification: {}\n{}".format(client_name, request.form.get('reference'), + request.form)) if errors: raise InvalidRequest(errors, status_code=400) else: @@ -38,6 +43,9 @@ def process_firetext_response(): raise InvalidRequest(errors, status_code=400) status = request.form.get('status') + current_app.logger.info( + "Full delivery response from {} for notification: {}\n{}".format(client_name, request.form.get('reference'), + request.form)) success, errors = process_sms_client_response(status=status, reference=request.form.get('reference'), client_name=client_name) From d18ce47114af7fa29967878a5c17a3bc88485c6c Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 12 Jul 2017 15:32:59 +0100 Subject: [PATCH 02/22] Make sure we don't log the phone number --- app/notifications/notifications_sms_callback.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/app/notifications/notifications_sms_callback.py b/app/notifications/notifications_sms_callback.py index 430e1c149..5120a48c8 100644 --- a/app/notifications/notifications_sms_callback.py +++ b/app/notifications/notifications_sms_callback.py @@ -24,9 +24,11 @@ def process_mmg_response(): reference=data.get('CID'), client_name=client_name) + safe_to_log = data.copy() + safe_to_log.pop("MSISDN") current_app.logger.info( - "Full delivery response from {} for notification: {}\n{}".format(client_name, request.form.get('reference'), - request.form)) + "Full delivery response from {} for notification: {}\n{}".format(client_name, request.form.get('CID'), + safe_to_log)) if errors: raise InvalidRequest(errors, status_code=400) else: @@ -41,12 +43,12 @@ def process_firetext_response(): client_name=client_name) if errors: raise InvalidRequest(errors, status_code=400) - - status = request.form.get('status') + safe_to_log = dict(request.form).copy() + safe_to_log.pop('mobile') current_app.logger.info( "Full delivery response from {} for notification: {}\n{}".format(client_name, request.form.get('reference'), - request.form)) - success, errors = process_sms_client_response(status=status, + safe_to_log)) + success, errors = process_sms_client_response(status=request.form.get('status'), reference=request.form.get('reference'), client_name=client_name) if errors: From 49d1f52aef73e75b754c4db82af66924af9f07d6 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 12 Jul 2017 15:49:43 +0100 Subject: [PATCH 03/22] Fix code style --- app/notifications/notifications_sms_callback.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/notifications/notifications_sms_callback.py b/app/notifications/notifications_sms_callback.py index 5120a48c8..4cf954840 100644 --- a/app/notifications/notifications_sms_callback.py +++ b/app/notifications/notifications_sms_callback.py @@ -28,7 +28,7 @@ def process_mmg_response(): safe_to_log.pop("MSISDN") current_app.logger.info( "Full delivery response from {} for notification: {}\n{}".format(client_name, request.form.get('CID'), - safe_to_log)) + safe_to_log)) if errors: raise InvalidRequest(errors, status_code=400) else: From 4b05c32b622d36ee8e5f3b1b9ca3d99735bc2c1f Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 13 Jul 2017 17:22:11 +0100 Subject: [PATCH 04/22] Create a new table to warehouse the monthly billing numbers --- app/dao/monthly_billing_dao.py | 6 ++++ app/models.py | 17 +++++++++- migrations/versions/0109_monthly_billing.py | 37 +++++++++++++++++++++ tests/app/dao/test_monthly_billing.py | 33 ++++++++++++++++++ 4 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 app/dao/monthly_billing_dao.py create mode 100644 migrations/versions/0109_monthly_billing.py create mode 100644 tests/app/dao/test_monthly_billing.py diff --git a/app/dao/monthly_billing_dao.py b/app/dao/monthly_billing_dao.py new file mode 100644 index 000000000..c037b5c54 --- /dev/null +++ b/app/dao/monthly_billing_dao.py @@ -0,0 +1,6 @@ +from app import db + + +def update_monthly_billing(monthly_billing): + db.session.add(monthly_billing) + db.session.commit() diff --git a/app/models.py b/app/models.py index 0e3079c80..07ba505d8 100644 --- a/app/models.py +++ b/app/models.py @@ -4,7 +4,6 @@ import datetime from flask import url_for, current_app from sqlalchemy.ext.associationproxy import association_proxy -from sqlalchemy.ext.hybrid import hybrid_property from sqlalchemy.dialects.postgresql import ( UUID, JSON @@ -1246,3 +1245,19 @@ class LetterRateDetail(db.Model): letter_rate = db.relationship('LetterRate', backref='letter_rates') page_total = db.Column(db.Integer, nullable=False) rate = db.Column(db.Numeric(), nullable=False) + + +class MonthlyBilling(db.Model): + __tablename__ = 'monthly_billing' + + id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) + service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, nullable=False) + service = db.relationship('Service', backref='monthly_billing') + month = db.Column(db.String, nullable=False) + year = db.Column(db.Float(asdecimal=False), nullable=False) + notification_type = db.Column(notification_types, nullable=False) + monthly_totals = db.Column(JSON, nullable=False) + + __table_args__ = ( + UniqueConstraint('service_id', 'month', 'year', 'notification_type', name='uix_monthly_billing'), + ) diff --git a/migrations/versions/0109_monthly_billing.py b/migrations/versions/0109_monthly_billing.py new file mode 100644 index 000000000..124a509cf --- /dev/null +++ b/migrations/versions/0109_monthly_billing.py @@ -0,0 +1,37 @@ +"""empty message + +Revision ID: 0109_monthly_billing +Revises: 0108_change_logo_not_nullable +Create Date: 2017-07-13 14:35:03.183659 + +""" + +# revision identifiers, used by Alembic. +revision = '0109_monthly_billing' +down_revision = '0108_change_logo_not_nullable' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + + +def upgrade(): + + op.create_table('monthly_billing', + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('service_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('month', sa.String(), nullable=False), + sa.Column('year', sa.Float(), nullable=False), + sa.Column('notification_type', + postgresql.ENUM('email', 'sms', 'letter', name='notification_type', create_type=False), + nullable=False), + sa.Column('monthly_totals', postgresql.JSON(), nullable=False), + sa.ForeignKeyConstraint(['service_id'], ['services.id'], ), + sa.PrimaryKeyConstraint('id') + ) + op.create_index(op.f('ix_monthly_billing_service_id'), 'monthly_billing', ['service_id'], unique=False) + op.create_index(op.f('uix_monthly_billing'), 'monthly_billing', ['service_id', 'month', 'year', 'notification_type'], unique=True) + + +def downgrade(): + op.drop_table('monthly_billing') diff --git a/tests/app/dao/test_monthly_billing.py b/tests/app/dao/test_monthly_billing.py new file mode 100644 index 000000000..f04fa79ca --- /dev/null +++ b/tests/app/dao/test_monthly_billing.py @@ -0,0 +1,33 @@ +import uuid + +import pytest +from sqlalchemy.exc import IntegrityError + +from app.dao.monthly_billing_dao import update_monthly_billing +from app.models import MonthlyBilling + + +def test_add_monthly_billing_only_allows_one_row_per_service_month_type(sample_service): + first = MonthlyBilling(id=uuid.uuid4(), + service_id=sample_service.id, + notification_type='sms', + month='January', + year='2017', + monthly_totals={'billing_units': 100, + 'rate': 0.0158}) + + second = MonthlyBilling(id=uuid.uuid4(), + service_id=sample_service.id, + notification_type='sms', + month='January', + year='2017', + monthly_totals={'billing_units': 50, + 'rate': 0.0162}) + + update_monthly_billing(first) + with pytest.raises(IntegrityError): + update_monthly_billing(second) + monthly = MonthlyBilling.query.all() + assert len(monthly) == 1 + assert monthly[0].monthly_totals == {'billing_units': 100, + 'rate': 0.0158} From 9400988d72e97f9613f4681293f54018a3650a5b Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 18 Jul 2017 18:21:35 +0100 Subject: [PATCH 05/22] Monthly billing - part 1 This is still a work in progress but it would be good to get some eyes on it. This commit includes creating and updating a row in the monthly billing table and a method to fetch the results. There is a command to populate the monthly billing for a service and month so we can try it out. The total cost at the moment are wrong, they do not take into account the free allowance - see notes below about adding that to the table. Left to do: create a nightly task to run to update the monthly totals. create an endpoint to return the yearly billing, the current day will need to be calculated on the fly and added to the totals. Add the free allowance into the total costs. --- app/commands.py | 18 +++- app/dao/date_util.py | 13 +++ app/dao/monthly_billing_dao.py | 42 +++++++- app/dao/notification_usage_dao.py | 17 ++- app/dao/provider_rates_dao.py | 5 + app/delivery/send_to_providers.py | 2 +- app/models.py | 9 ++ app/service/rest.py | 1 - application.py | 1 + tests/app/dao/test_date_utils.py | 19 +++- tests/app/dao/test_monthly_billing.py | 128 ++++++++++++++++++----- tests/app/dao/test_provider_rates_dao.py | 13 ++- tests/app/db.py | 9 +- 13 files changed, 240 insertions(+), 37 deletions(-) diff --git a/app/commands.py b/app/commands.py index 6e4bc53d1..c7ec0567b 100644 --- a/app/commands.py +++ b/app/commands.py @@ -5,7 +5,8 @@ from flask.ext.script import Command, Manager, Option from app import db -from app.models import (PROVIDERS, Service, User, NotificationHistory) +from app.dao.monthly_billing_dao import create_or_update_monthly_billing_sms, get_monthly_billing_sms +from app.models import (PROVIDERS, User) from app.dao.services_dao import ( delete_service_and_all_associated_db_objects, dao_fetch_all_services_by_user @@ -146,3 +147,18 @@ class CustomDbScript(Command): print('Committed {} updates at {}'.format(len(result), datetime.utcnow())) db.session.commit() result = db.session.execute(subq_hist).fetchall() + + +class PopulateMonthlyBilling(Command): + option_list = ( + Option('-s', '-service-id', dest='service_id', + help="Service id to populate monthly billing for"), + Option('-m', '-month', dest="month", help="Use for integer value for month, e.g. 7 for July"), + Option('-y', '-year', dest="year", help="Use for integer value for year, e.g. 2017") + ) + + def run(self, service_id, month, year): + create_or_update_monthly_billing_sms(service_id, datetime(int(year), int(month), 1)) + results = get_monthly_billing_sms(service_id, datetime(int(year), int(month), 1)) + print("Finished populating data for {} for service id {}".format(month, service_id)) + print(results.monthly_totals) diff --git a/app/dao/date_util.py b/app/dao/date_util.py index 2471865bd..41ad9dc72 100644 --- a/app/dao/date_util.py +++ b/app/dao/date_util.py @@ -16,3 +16,16 @@ def get_april_fools(year): """ return pytz.timezone('Europe/London').localize(datetime(year, 4, 1, 0, 0, 0)).astimezone(pytz.UTC).replace( tzinfo=None) + + +def get_month_start_end_date(month_year): + """ + This function return the start and date of the month_year as UTC, + :param month_year: the datetime to calculate the start and end date for that month + :return: start_date, end_date, month + """ + import calendar + _, num_days = calendar.monthrange(month_year.year, month_year.month) + first_day = datetime(month_year.year, month_year.month, 1, 0, 0, 0) + last_day = datetime(month_year.year, month_year.month, num_days, 23, 59, 59, 99999) + return first_day, last_day diff --git a/app/dao/monthly_billing_dao.py b/app/dao/monthly_billing_dao.py index c037b5c54..8e3204584 100644 --- a/app/dao/monthly_billing_dao.py +++ b/app/dao/monthly_billing_dao.py @@ -1,6 +1,44 @@ +from datetime import datetime + from app import db +from app.dao.notification_usage_dao import get_billing_data_for_month +from app.models import MonthlyBilling, SMS_TYPE -def update_monthly_billing(monthly_billing): - db.session.add(monthly_billing) +def create_or_update_monthly_billing_sms(service_id, billing_month): + monthly = get_billing_data_for_month(service_id=service_id, billing_month=billing_month) + # update monthly + monthly_totals = _monthly_billing_data_to_json(monthly) + row = MonthlyBilling.query.filter_by(year=billing_month.year, + month=datetime.strftime(billing_month, "%B"), + notification_type='sms').first() + if row: + row.monthly_totals = monthly_totals + else: + row = MonthlyBilling(service_id=service_id, + notification_type=SMS_TYPE, + year=billing_month.year, + month=datetime.strftime(billing_month, "%B"), + monthly_totals=monthly_totals) + db.session.add(row) db.session.commit() + + +def get_monthly_billing_sms(service_id, billing_month): + monthly = MonthlyBilling.query.filter_by(service_id=service_id, + year=billing_month.year, + month=datetime.strftime(billing_month, "%B"), + notification_type=SMS_TYPE).first() + return monthly + + +def _monthly_billing_data_to_json(monthly): + # ('April', 6, 1, False, 'sms', 0.014) + # (month, billing_units, rate_multiplier, international, notification_type, rate) + # total cost must take into account the free allowance. + # might be a good idea to capture free allowance in this table + return [{"billing_units": x[1], + "rate_multiplier": x[2], + "international": x[3], + "rate": x[5], + "total_cost": (x[1] * x[2]) * x[5]} for x in monthly] diff --git a/app/dao/notification_usage_dao.py b/app/dao/notification_usage_dao.py index 995462509..289a5a03d 100644 --- a/app/dao/notification_usage_dao.py +++ b/app/dao/notification_usage_dao.py @@ -6,7 +6,7 @@ from sqlalchemy import func, case, cast from sqlalchemy import literal_column from app import db -from app.dao.date_util import get_financial_year +from app.dao.date_util import get_financial_year, get_month_start_end_date from app.models import (NotificationHistory, Rate, NOTIFICATION_STATUS_TYPES_BILLABLE, @@ -35,6 +35,21 @@ def get_yearly_billing_data(service_id, year): return sum(result, []) +@statsd(namespace="dao") +def get_billing_data_for_month(service_id, billing_month): + start_date, end_date = get_month_start_end_date(billing_month) + rates = get_rates_for_year(start_date, end_date, SMS_TYPE) + result = [] + # so the start end date in the query are the valid from the rate, not the month - this is going to take some thought + for r, n in zip(rates, rates[1:]): + result.extend(sms_billing_data_per_month_query(r.rate, service_id, max(r.valid_from, start_date), + min(n.valid_from, end_date))) + result.extend( + sms_billing_data_per_month_query(rates[-1].rate, service_id, max(rates[-1].valid_from, start_date), end_date)) + + return result + + @statsd(namespace="dao") def get_monthly_billing_data(service_id, year): start_date, end_date = get_financial_year(year) diff --git a/app/dao/provider_rates_dao.py b/app/dao/provider_rates_dao.py index 145bd431e..443543f86 100644 --- a/app/dao/provider_rates_dao.py +++ b/app/dao/provider_rates_dao.py @@ -9,3 +9,8 @@ def create_provider_rates(provider_identifier, valid_from, rate): provider_rates = ProviderRates(provider_id=provider.id, valid_from=valid_from, rate=rate) db.session.add(provider_rates) + + +@transactional +def create_sms_rate(rate): + db.session.add(rate) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index df4c664f9..5e33e2113 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -18,7 +18,7 @@ from app.dao.templates_dao import dao_get_template_by_id from app.models import SMS_TYPE, KEY_TYPE_TEST, BRANDING_ORG, EMAIL_TYPE, NOTIFICATION_TECHNICAL_FAILURE, \ NOTIFICATION_SENT, NOTIFICATION_SENDING -from app.celery.statistics_tasks import record_initial_job_statistics, create_initial_notification_statistic_tasks +from app.celery.statistics_tasks import create_initial_notification_statistic_tasks def send_sms_to_provider(notification): diff --git a/app/models.py b/app/models.py index 07ba505d8..e035acccb 100644 --- a/app/models.py +++ b/app/models.py @@ -1261,3 +1261,12 @@ class MonthlyBilling(db.Model): __table_args__ = ( UniqueConstraint('service_id', 'month', 'year', 'notification_type', name='uix_monthly_billing'), ) + + def serialized(self): + return { + "month": self.month, + "year": self.year, + "service_id": str(self.service_id), + "notification_type": self.notification_type, + "monthly_totals": self.monthly_totals + } diff --git a/app/service/rest.py b/app/service/rest.py index ae57f5390..deb37f525 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -68,7 +68,6 @@ from app.schemas import ( user_schema, permission_schema, notification_with_template_schema, - notification_with_personalisation_schema, notifications_filter_schema, detailed_service_schema ) diff --git a/application.py b/application.py index b2a59a6fd..5ff7dd1e4 100644 --- a/application.py +++ b/application.py @@ -16,6 +16,7 @@ manager.add_command('db', MigrateCommand) manager.add_command('create_provider_rate', commands.CreateProviderRateCommand) manager.add_command('purge_functional_test_data', commands.PurgeFunctionalTestDataCommand) manager.add_command('custom_db_script', commands.CustomDbScript) +manager.add_command('populate_monthly_billing', commands.PopulateMonthlyBilling) @manager.command diff --git a/tests/app/dao/test_date_utils.py b/tests/app/dao/test_date_utils.py index 760923c5a..4ede18853 100644 --- a/tests/app/dao/test_date_utils.py +++ b/tests/app/dao/test_date_utils.py @@ -1,4 +1,8 @@ -from app.dao.date_util import get_financial_year, get_april_fools +from datetime import datetime + +import pytest + +from app.dao.date_util import get_financial_year, get_april_fools, get_month_start_end_date def test_get_financial_year(): @@ -11,3 +15,16 @@ def test_get_april_fools(): april_fools = get_april_fools(2016) assert str(april_fools) == '2016-03-31 23:00:00' assert april_fools.tzinfo is None + + +@pytest.mark.parametrize("month, year, expected_end", + [(7, 2017, 31), + (2, 2016, 29), + (2, 2017, 28), + (9, 2018, 30), + (12, 2019, 31)]) +def test_get_month_start_end_date(month, year, expected_end): + month_year = datetime(year, month, 10, 13, 30, 00) + result = get_month_start_end_date(month_year) + assert result[0] == datetime(year, month, 1, 0, 0, 0, 0) + assert result[1] == datetime(year, month, expected_end, 23, 59, 59, 99999) diff --git a/tests/app/dao/test_monthly_billing.py b/tests/app/dao/test_monthly_billing.py index f04fa79ca..6a0399d67 100644 --- a/tests/app/dao/test_monthly_billing.py +++ b/tests/app/dao/test_monthly_billing.py @@ -1,33 +1,107 @@ -import uuid +from datetime import datetime -import pytest -from sqlalchemy.exc import IntegrityError - -from app.dao.monthly_billing_dao import update_monthly_billing +from app.dao.monthly_billing_dao import create_or_update_monthly_billing_sms, get_monthly_billing_sms from app.models import MonthlyBilling +from tests.app.db import create_notification, create_rate -def test_add_monthly_billing_only_allows_one_row_per_service_month_type(sample_service): - first = MonthlyBilling(id=uuid.uuid4(), - service_id=sample_service.id, - notification_type='sms', - month='January', - year='2017', - monthly_totals={'billing_units': 100, - 'rate': 0.0158}) +def test_add_monthly_billing(sample_template): + jan = datetime(2017, 1, 1) + feb = datetime(2017, 2, 15) + create_rate(start_date=jan, value=0.0158, notification_type='sms') + create_notification(template=sample_template, created_at=jan, billable_units=1, status='delivered') + create_notification(template=sample_template, created_at=feb, billable_units=2, status='delivered') - second = MonthlyBilling(id=uuid.uuid4(), - service_id=sample_service.id, - notification_type='sms', - month='January', - year='2017', - monthly_totals={'billing_units': 50, - 'rate': 0.0162}) + create_or_update_monthly_billing_sms(service_id=sample_template.service_id, + billing_month=jan) + create_or_update_monthly_billing_sms(service_id=sample_template.service_id, + billing_month=feb) + monthly_billing = MonthlyBilling.query.all() + assert len(monthly_billing) == 2 + assert monthly_billing[0].month == 'January' + assert monthly_billing[1].month == 'February' - update_monthly_billing(first) - with pytest.raises(IntegrityError): - update_monthly_billing(second) - monthly = MonthlyBilling.query.all() - assert len(monthly) == 1 - assert monthly[0].monthly_totals == {'billing_units': 100, - 'rate': 0.0158} + january = get_monthly_billing_sms(service_id=sample_template.service_id, billing_month=jan) + expected_jan = {"billing_units": 1, + "rate_multiplier": 1, + "international": False, + "rate": 0.0158, + "total_cost": 1 * 0.0158} + assert_monthly_billing(january, 2017, "January", sample_template.service_id, 1, expected_jan) + + february = get_monthly_billing_sms(service_id=sample_template.service_id, billing_month=feb) + expected_feb = {"billing_units": 2, + "rate_multiplier": 1, + "international": False, + "rate": 0.0158, + "total_cost": 2 * 0.0158} + assert_monthly_billing(february, 2017, "February", sample_template.service_id, 1, expected_feb) + + +def test_add_monthly_billing_multiple_rates_in_a_month(sample_template): + rate_1 = datetime(2016, 12, 1) + rate_2 = datetime(2017, 1, 15) + create_rate(start_date=rate_1, value=0.0158, notification_type='sms') + create_rate(start_date=rate_2, value=0.0124, notification_type='sms') + + create_notification(template=sample_template, created_at=datetime(2017, 1, 1), billable_units=1, status='delivered') + create_notification(template=sample_template, created_at=datetime(2017, 1, 14, 23, 59), billable_units=1, + status='delivered') + + create_notification(template=sample_template, created_at=datetime(2017, 1, 15), billable_units=2, + status='delivered') + create_notification(template=sample_template, created_at=datetime(2017, 1, 17, 13, 30, 57), billable_units=4, + status='delivered') + + create_or_update_monthly_billing_sms(service_id=sample_template.service_id, + billing_month=rate_2) + monthly_billing = MonthlyBilling.query.all() + assert len(monthly_billing) == 1 + assert monthly_billing[0].month == 'January' + + january = get_monthly_billing_sms(service_id=sample_template.service_id, billing_month=rate_2) + first_row = {"billing_units": 2, + "rate_multiplier": 1, + "international": False, + "rate": 0.0158, + "total_cost": 3 * 0.0158} + assert_monthly_billing(january, 2017, "January", sample_template.service_id, 2, first_row) + second_row = {"billing_units": 6, + "rate_multiplier": 1, + "international": False, + "rate": 0.0124, + "total_cost": 1 * 0.0124} + assert sorted(january.monthly_totals[1]) == sorted(second_row) + + +def test_update_monthly_billing_overwrites_old_totals(sample_template): + july = datetime(2017, 7, 1) + create_rate(july, 0.123, 'sms') + create_notification(template=sample_template, created_at=datetime(2017, 7, 2), billable_units=1, status='delivered') + + create_or_update_monthly_billing_sms(sample_template.service_id, july) + first_update = get_monthly_billing_sms(sample_template.service_id, july) + expected = {"billing_units": 1, + "rate_multiplier": 1, + "international": False, + "rate": 0.123, + "total_cost": 1 * 0.123} + assert_monthly_billing(first_update, 2017, "July", sample_template.service_id, 1, expected) + + create_notification(template=sample_template, created_at=datetime(2017, 7, 5), billable_units=2, status='delivered') + create_or_update_monthly_billing_sms(sample_template.service_id, july) + second_update = get_monthly_billing_sms(sample_template.service_id, july) + expected_update = {"billing_units": 3, + "rate_multiplier": 1, + "international": False, + "rate": 0.123, + "total_cost": 3 * 0.123} + assert_monthly_billing(second_update, 2017, "July", sample_template.service_id, 1, expected_update) + + +def assert_monthly_billing(monthly_billing, year, month, service_id, expected_len, first_row): + assert monthly_billing.year == year + assert monthly_billing.month == month + assert monthly_billing.service_id == service_id + assert len(monthly_billing.monthly_totals) == expected_len + assert sorted(monthly_billing.monthly_totals[0]) == sorted(first_row) diff --git a/tests/app/dao/test_provider_rates_dao.py b/tests/app/dao/test_provider_rates_dao.py index 417612781..7edb7de43 100644 --- a/tests/app/dao/test_provider_rates_dao.py +++ b/tests/app/dao/test_provider_rates_dao.py @@ -1,7 +1,8 @@ +import uuid from datetime import datetime from decimal import Decimal -from app.dao.provider_rates_dao import create_provider_rates -from app.models import ProviderRates, ProviderDetails +from app.dao.provider_rates_dao import create_provider_rates, create_sms_rate +from app.models import ProviderRates, ProviderDetails, Rate def test_create_provider_rates(notify_db, notify_db_session, mmg_provider): @@ -15,3 +16,11 @@ def test_create_provider_rates(notify_db, notify_db_session, mmg_provider): assert ProviderRates.query.first().rate == rate assert ProviderRates.query.first().valid_from == now assert ProviderRates.query.first().provider_id == provider.id + + +def test_create_sms_rate(): + rate = Rate(id=uuid.uuid4(), valid_from=datetime.now(), rate=0.014, notification_type='sms') + create_sms_rate(rate) + rates = Rate.query.all() + assert len(rates) == 1 + assert rates[0] == rate diff --git a/tests/app/db.py b/tests/app/db.py index ea9e4cbdd..669803d92 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -1,8 +1,8 @@ from datetime import datetime import uuid - from app.dao.jobs_dao import dao_create_job +from app.dao.provider_rates_dao import create_sms_rate from app.dao.service_inbound_api_dao import save_service_inbound_api from app.models import ( Service, @@ -11,6 +11,7 @@ from app.models import ( Notification, ScheduledNotification, ServicePermission, + Rate, Job, InboundSms, Organisation, @@ -239,3 +240,9 @@ def create_organisation(colour='blue', logo='test_x2.png', name='test_org_1'): dao_create_organisation(organisation) return organisation + + +def create_rate(start_date, value, notification_type): + rate = Rate(id=uuid.uuid4(), valid_from=start_date, rate=value, notification_type=notification_type) + create_sms_rate(rate) + return rate From 6c61a3fc2ae33324f9530be5379a826d9991c19b Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 19 Jul 2017 13:50:29 +0100 Subject: [PATCH 06/22] Revert celery4 Revert the following three pull requests: https://github.com/alphagov/notifications-api/pull/1085 https://github.com/alphagov/notifications-api/pull/1086 https://github.com/alphagov/notifications-api/pull/1088 celery 4.0.2 looked promising, however, on staging under mild load (5/sec api calls) the performance was actually worse than 3.1.25 --- app/__init__.py | 1 + app/celery/__init__.py | 27 ---- app/celery/celery.py | 126 +-------------- app/celery/provider_tasks.py | 2 +- app/celery/research_mode_tasks.py | 1 + app/celery/scheduled_tasks.py | 2 +- app/celery/statistics_tasks.py | 2 +- app/celery/tasks.py | 2 +- app/config.py | 144 +++++++++++++++++- app/delivery/rest.py | 2 +- app/invite/rest.py | 2 +- app/job/rest.py | 2 +- app/letters/send_letter_jobs.py | 2 +- .../notifications_letter_callback.py | 2 +- app/notifications/process_notifications.py | 2 +- app/notifications/receive_notifications.py | 2 +- app/notifications/rest.py | 2 +- app/service/send_notification.py | 2 +- app/service/sender.py | 2 +- app/user/rest.py | 2 +- app/v2/notifications/post_notifications.py | 7 +- docker/Dockerfile | 1 - requirements.txt | 11 +- tests/app/celery/test_statistics_tasks.py | 15 +- .../service/test_send_one_off_notification.py | 2 +- 25 files changed, 173 insertions(+), 192 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 72b8f2885..e0ec02723 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -49,6 +49,7 @@ def create_app(app_name=None): from app.config import configs notify_environment = os.environ['NOTIFY_ENVIRONMENT'] + application.config.from_object(configs[notify_environment]) if app_name: diff --git a/app/celery/__init__.py b/app/celery/__init__.py index 270bb54dd..e69de29bb 100644 --- a/app/celery/__init__.py +++ b/app/celery/__init__.py @@ -1,27 +0,0 @@ - -class QueueNames(object): - PERIODIC = 'periodic-tasks' - PRIORITY = 'priority-tasks' - DATABASE = 'database-tasks' - SEND = 'send-tasks' - RESEARCH_MODE = 'research-mode-tasks' - STATISTICS = 'statistics-tasks' - JOBS = 'job-tasks' - RETRY = 'retry-tasks' - NOTIFY = 'notify-internal-tasks' - PROCESS_FTP = 'process-ftp-tasks' - - @staticmethod - def all_queues(): - return [ - QueueNames.PRIORITY, - QueueNames.PERIODIC, - QueueNames.DATABASE, - QueueNames.SEND, - QueueNames.RESEARCH_MODE, - QueueNames.STATISTICS, - QueueNames.JOBS, - QueueNames.RETRY, - QueueNames.NOTIFY, - QueueNames.PROCESS_FTP - ] diff --git a/app/celery/celery.py b/app/celery/celery.py index d723bfdaa..183e50bd6 100644 --- a/app/celery/celery.py +++ b/app/celery/celery.py @@ -1,130 +1,11 @@ -from datetime import timedelta - from celery import Celery -from celery.schedules import crontab -from kombu import Queue, Exchange - -from app.celery import QueueNames - - -class CeleryConfig: - def __init__(self, config): - self.broker_transport_options['queue_name_prefix'] = config['NOTIFICATION_QUEUE_PREFIX'] - self.broker_url = config.get('BROKER_URL', 'sqs://') - - broker_transport_options = { - 'region': 'eu-west-1', - 'polling_interval': 1, # 1 second - 'visibility_timeout': 310, - 'queue_name_prefix': None - } - enable_utc = True, - timezone = 'Europe/London' - accept_content = ['json'] - task_serializer = 'json' - imports = ('app.celery.tasks', 'app.celery.scheduled_tasks') - beat_schedule = { - 'run-scheduled-jobs': { - 'task': 'run-scheduled-jobs', - 'schedule': crontab(minute=1), - 'options': {'queue': QueueNames.PERIODIC} - }, - # 'send-scheduled-notifications': { - # 'task': 'send-scheduled-notifications', - # 'schedule': crontab(minute='*/15'), - # 'options': {'queue': 'periodic'} - # }, - 'delete-verify-codes': { - 'task': 'delete-verify-codes', - 'schedule': timedelta(minutes=63), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'delete-invitations': { - 'task': 'delete-invitations', - 'schedule': timedelta(minutes=66), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'delete-sms-notifications': { - 'task': 'delete-sms-notifications', - 'schedule': crontab(minute=0, hour=0), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'delete-email-notifications': { - 'task': 'delete-email-notifications', - 'schedule': crontab(minute=20, hour=0), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'delete-letter-notifications': { - 'task': 'delete-letter-notifications', - 'schedule': crontab(minute=40, hour=0), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'delete-inbound-sms': { - 'task': 'delete-inbound-sms', - 'schedule': crontab(minute=0, hour=1), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'send-daily-performance-platform-stats': { - 'task': 'send-daily-performance-platform-stats', - 'schedule': crontab(minute=0, hour=2), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'switch-current-sms-provider-on-slow-delivery': { - 'task': 'switch-current-sms-provider-on-slow-delivery', - 'schedule': crontab(), # Every minute - 'options': {'queue': QueueNames.PERIODIC} - }, - 'timeout-sending-notifications': { - 'task': 'timeout-sending-notifications', - 'schedule': crontab(minute=0, hour=3), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'remove_sms_email_jobs': { - 'task': 'remove_csv_files', - 'schedule': crontab(minute=0, hour=4), - 'options': {'queue': QueueNames.PERIODIC}, - # TODO: Avoid duplication of keywords - ideally by moving definitions out of models.py - 'kwargs': {'job_types': ['email', 'sms']} - }, - 'remove_letter_jobs': { - 'task': 'remove_csv_files', - 'schedule': crontab(minute=20, hour=4), - 'options': {'queue': QueueNames.PERIODIC}, - # TODO: Avoid duplication of keywords - ideally by moving definitions out of models.py - 'kwargs': {'job_types': ['letter']} - }, - 'remove_transformed_dvla_files': { - 'task': 'remove_transformed_dvla_files', - 'schedule': crontab(minute=40, hour=4), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'delete_dvla_response_files': { - 'task': 'delete_dvla_response_files', - 'schedule': crontab(minute=10, hour=5), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'timeout-job-statistics': { - 'task': 'timeout-job-statistics', - 'schedule': crontab(minute=0, hour=5), - 'options': {'queue': QueueNames.PERIODIC} - } - } - task_queues = [] class NotifyCelery(Celery): + def init_app(self, app): - celery_config = CeleryConfig(app.config) - - super().__init__(app.import_name, broker=celery_config.broker_url) - - if app.config['INITIALISE_QUEUES']: - for queue in QueueNames.all_queues(): - CeleryConfig.task_queues.append( - Queue(queue, Exchange('default'), routing_key=queue) - ) - - self.config_from_object(celery_config) + super().__init__(app.import_name, broker=app.config['BROKER_URL']) + self.conf.update(app.config) TaskBase = self.Task class ContextTask(TaskBase): @@ -133,5 +14,4 @@ class NotifyCelery(Celery): def __call__(self, *args, **kwargs): with app.app_context(): return TaskBase.__call__(self, *args, **kwargs) - self.Task = ContextTask diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index e748c3bd2..50d5a31b7 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -3,7 +3,7 @@ from notifications_utils.recipients import InvalidEmailError from sqlalchemy.orm.exc import NoResultFound from app import notify_celery -from app.celery import QueueNames +from app.config import QueueNames from app.dao import notifications_dao from app.dao.notifications_dao import update_notification_status_by_id from app.statsd_decorators import statsd diff --git a/app/celery/research_mode_tasks.py b/app/celery/research_mode_tasks.py index 356bc3488..ced35b1d4 100644 --- a/app/celery/research_mode_tasks.py +++ b/app/celery/research_mode_tasks.py @@ -1,6 +1,7 @@ import json from flask import current_app +from app import notify_celery from requests import request, RequestException, HTTPError from app.models import SMS_TYPE diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index b14f7a5be..e86a7c113 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -28,7 +28,7 @@ from app.models import LETTER_TYPE from app.notifications.process_notifications import send_notification_to_queue from app.statsd_decorators import statsd from app.celery.tasks import process_job -from app.celery import QueueNames +from app.config import QueueNames @notify_celery.task(name="remove_csv_files") diff --git a/app/celery/statistics_tasks.py b/app/celery/statistics_tasks.py index b00f88bdd..150fa6aac 100644 --- a/app/celery/statistics_tasks.py +++ b/app/celery/statistics_tasks.py @@ -10,7 +10,7 @@ from app.dao.statistics_dao import ( ) from app.dao.notifications_dao import get_notification_by_id from app.models import NOTIFICATION_STATUS_TYPES_COMPLETED -from app.celery import QueueNames +from app.config import QueueNames def create_initial_notification_statistic_tasks(notification): diff --git a/app/celery/tasks.py b/app/celery/tasks.py index bbd1fa207..da9d1f9ee 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -19,8 +19,8 @@ from app import ( ) from app.aws import s3 from app.celery import provider_tasks +from app.config import QueueNames from app.dao.inbound_sms_dao import dao_get_inbound_sms_by_id -from app.celery import QueueNames from app.dao.jobs_dao import ( dao_update_job, dao_get_job_by_id, diff --git a/app/config.py b/app/config.py index 290cc3211..1bbbb2057 100644 --- a/app/config.py +++ b/app/config.py @@ -1,6 +1,10 @@ +from datetime import timedelta import os import json +from celery.schedules import crontab +from kombu import Exchange, Queue + from app.models import ( EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST @@ -14,6 +18,34 @@ if os.environ.get('VCAP_SERVICES'): extract_cloudfoundry_config() +class QueueNames(object): + PERIODIC = 'periodic-tasks' + PRIORITY = 'priority-tasks' + DATABASE = 'database-tasks' + SEND = 'send-tasks' + RESEARCH_MODE = 'research-mode-tasks' + STATISTICS = 'statistics-tasks' + JOBS = 'job-tasks' + RETRY = 'retry-tasks' + NOTIFY = 'notify-internal-tasks' + PROCESS_FTP = 'process-ftp-tasks' + + @staticmethod + def all_queues(): + return [ + QueueNames.PRIORITY, + QueueNames.PERIODIC, + QueueNames.DATABASE, + QueueNames.SEND, + QueueNames.RESEARCH_MODE, + QueueNames.STATISTICS, + QueueNames.JOBS, + QueueNames.RETRY, + QueueNames.NOTIFY, + QueueNames.PROCESS_FTP + ] + + class Config(object): # URL of admin app ADMIN_BASE_URL = os.environ['ADMIN_BASE_URL'] @@ -94,6 +126,104 @@ class Config(object): CHANGE_EMAIL_CONFIRMATION_TEMPLATE_ID = 'eb4d9930-87ab-4aef-9bce-786762687884' SERVICE_NOW_LIVE_TEMPLATE_ID = '618185c6-3636-49cd-b7d2-6f6f5eb3bdde' + BROKER_URL = 'sqs://' + BROKER_TRANSPORT_OPTIONS = { + 'region': AWS_REGION, + 'polling_interval': 1, # 1 second + 'visibility_timeout': 310, + 'queue_name_prefix': NOTIFICATION_QUEUE_PREFIX + } + CELERY_ENABLE_UTC = True, + CELERY_TIMEZONE = 'Europe/London' + CELERY_ACCEPT_CONTENT = ['json'] + CELERY_TASK_SERIALIZER = 'json' + CELERY_IMPORTS = ('app.celery.tasks', 'app.celery.scheduled_tasks') + CELERYBEAT_SCHEDULE = { + 'run-scheduled-jobs': { + 'task': 'run-scheduled-jobs', + 'schedule': crontab(minute=1), + 'options': {'queue': QueueNames.PERIODIC} + }, + # 'send-scheduled-notifications': { + # 'task': 'send-scheduled-notifications', + # 'schedule': crontab(minute='*/15'), + # 'options': {'queue': 'periodic'} + # }, + 'delete-verify-codes': { + 'task': 'delete-verify-codes', + 'schedule': timedelta(minutes=63), + 'options': {'queue': QueueNames.PERIODIC} + }, + 'delete-invitations': { + 'task': 'delete-invitations', + 'schedule': timedelta(minutes=66), + 'options': {'queue': QueueNames.PERIODIC} + }, + 'delete-sms-notifications': { + 'task': 'delete-sms-notifications', + 'schedule': crontab(minute=0, hour=0), + 'options': {'queue': QueueNames.PERIODIC} + }, + 'delete-email-notifications': { + 'task': 'delete-email-notifications', + 'schedule': crontab(minute=20, hour=0), + 'options': {'queue': QueueNames.PERIODIC} + }, + 'delete-letter-notifications': { + 'task': 'delete-letter-notifications', + 'schedule': crontab(minute=40, hour=0), + 'options': {'queue': QueueNames.PERIODIC} + }, + 'delete-inbound-sms': { + 'task': 'delete-inbound-sms', + 'schedule': crontab(minute=0, hour=1), + 'options': {'queue': QueueNames.PERIODIC} + }, + 'send-daily-performance-platform-stats': { + 'task': 'send-daily-performance-platform-stats', + 'schedule': crontab(minute=0, hour=2), + 'options': {'queue': QueueNames.PERIODIC} + }, + 'switch-current-sms-provider-on-slow-delivery': { + 'task': 'switch-current-sms-provider-on-slow-delivery', + 'schedule': crontab(), # Every minute + 'options': {'queue': QueueNames.PERIODIC} + }, + 'timeout-sending-notifications': { + 'task': 'timeout-sending-notifications', + 'schedule': crontab(minute=0, hour=3), + 'options': {'queue': QueueNames.PERIODIC} + }, + 'remove_sms_email_jobs': { + 'task': 'remove_csv_files', + 'schedule': crontab(minute=0, hour=4), + 'options': {'queue': QueueNames.PERIODIC}, + 'kwargs': {'job_types': [EMAIL_TYPE, SMS_TYPE]} + }, + 'remove_letter_jobs': { + 'task': 'remove_csv_files', + 'schedule': crontab(minute=20, hour=4), + 'options': {'queue': QueueNames.PERIODIC}, + 'kwargs': {'job_types': [LETTER_TYPE]} + }, + 'remove_transformed_dvla_files': { + 'task': 'remove_transformed_dvla_files', + 'schedule': crontab(minute=40, hour=4), + 'options': {'queue': QueueNames.PERIODIC} + }, + 'delete_dvla_response_files': { + 'task': 'delete_dvla_response_files', + 'schedule': crontab(minute=10, hour=5), + 'options': {'queue': QueueNames.PERIODIC} + }, + 'timeout-job-statistics': { + 'task': 'timeout-job-statistics', + 'schedule': crontab(minute=0, hour=5), + 'options': {'queue': QueueNames.PERIODIC} + } + } + CELERY_QUEUES = [] + NOTIFICATIONS_ALERT = 5 # five mins FROM_NUMBER = 'development' @@ -132,7 +262,6 @@ class Config(object): } FREE_SMS_TIER_FRAGMENT_COUNT = 250000 - INITIALISE_QUEUES = False SMS_INBOUND_WHITELIST = json.loads(os.environ.get('SMS_INBOUND_WHITELIST', '[]')) @@ -142,20 +271,24 @@ class Config(object): ###################### class Development(Config): - INITIALISE_QUEUES = True SQLALCHEMY_ECHO = False NOTIFY_EMAIL_DOMAIN = 'notify.tools' CSV_UPLOAD_BUCKET_NAME = 'development-notifications-csv-upload' DVLA_RESPONSE_BUCKET_NAME = 'notify.tools-ftp' NOTIFY_ENVIRONMENT = 'development' + NOTIFICATION_QUEUE_PREFIX = 'development' DEBUG = True + for queue in QueueNames.all_queues(): + Config.CELERY_QUEUES.append( + Queue(queue, Exchange('default'), routing_key=queue) + ) + API_HOST_NAME = "http://localhost:6011" API_RATE_LIMIT_ENABLED = True class Test(Config): - INITIALISE_QUEUES = True NOTIFY_EMAIL_DOMAIN = 'test.notify.com' FROM_NUMBER = 'testing' NOTIFY_ENVIRONMENT = 'test' @@ -169,6 +302,11 @@ class Test(Config): BROKER_URL = 'you-forgot-to-mock-celery-in-your-tests://' + for queue in QueueNames.all_queues(): + Config.CELERY_QUEUES.append( + Queue(queue, Exchange('default'), routing_key=queue) + ) + API_RATE_LIMIT_ENABLED = True API_HOST_NAME = "http://localhost:6011" diff --git a/app/delivery/rest.py b/app/delivery/rest.py index 5f4abb70b..489a5fcda 100644 --- a/app/delivery/rest.py +++ b/app/delivery/rest.py @@ -1,6 +1,6 @@ from flask import Blueprint, jsonify -from app.celery import QueueNames +from app.config import QueueNames from app.delivery import send_to_providers from app.models import EMAIL_TYPE from app.celery import provider_tasks diff --git a/app/invite/rest.py b/app/invite/rest.py index e1e401190..8105b171f 100644 --- a/app/invite/rest.py +++ b/app/invite/rest.py @@ -4,7 +4,7 @@ from flask import ( jsonify, current_app) -from app.celery import QueueNames +from app.config import QueueNames from app.dao.invited_user_dao import ( save_invited_user, get_invited_user, diff --git a/app/job/rest.py b/app/job/rest.py index e25aa5fcb..6a8a86ee4 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -36,7 +36,7 @@ from app.models import JOB_STATUS_SCHEDULED, JOB_STATUS_PENDING, JOB_STATUS_CANC from app.utils import pagination_links -from app.celery import QueueNames +from app.config import QueueNames job_blueprint = Blueprint('job', __name__, url_prefix='/service//job') diff --git a/app/letters/send_letter_jobs.py b/app/letters/send_letter_jobs.py index f90d7f280..91c39615a 100644 --- a/app/letters/send_letter_jobs.py +++ b/app/letters/send_letter_jobs.py @@ -2,7 +2,7 @@ from flask import Blueprint, jsonify from flask import request from app import notify_celery -from app.celery import QueueNames +from app.config import QueueNames from app.dao.jobs_dao import dao_get_all_letter_jobs from app.schemas import job_schema from app.v2.errors import register_errors diff --git a/app/notifications/notifications_letter_callback.py b/app/notifications/notifications_letter_callback.py index b8b587cf0..ac2de9e5c 100644 --- a/app/notifications/notifications_letter_callback.py +++ b/app/notifications/notifications_letter_callback.py @@ -13,7 +13,7 @@ from app.celery.tasks import update_letter_notifications_statuses from app.v2.errors import register_errors from app.notifications.utils import autoconfirm_subscription from app.schema_validation import validate -from app.celery import QueueNames +from app.config import QueueNames letter_callback_blueprint = Blueprint('notifications_letter_callback', __name__) register_errors(letter_callback_blueprint) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index da757db4b..ca3ae0b53 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -12,7 +12,7 @@ from app import redis_store from app.celery import provider_tasks from notifications_utils.clients import redis -from app.celery import QueueNames +from app.config import QueueNames from app.models import SMS_TYPE, Notification, KEY_TYPE_TEST, EMAIL_TYPE, ScheduledNotification from app.dao.notifications_dao import (dao_create_notification, dao_delete_notifications_and_history_by_id, diff --git a/app/notifications/receive_notifications.py b/app/notifications/receive_notifications.py index 07847a45c..34e726cf1 100644 --- a/app/notifications/receive_notifications.py +++ b/app/notifications/receive_notifications.py @@ -6,7 +6,7 @@ from notifications_utils.recipients import validate_and_format_phone_number from app import statsd_client, firetext_client, mmg_client from app.celery import tasks -from app.celery import QueueNames +from app.config import QueueNames from app.dao.services_dao import dao_fetch_services_by_sms_sender from app.dao.inbound_sms_dao import dao_create_inbound_sms from app.models import InboundSms, INBOUND_SMS_TYPE, SMS_TYPE diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 16ba13fee..5c7f8fdea 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -6,7 +6,7 @@ from flask import ( ) from app import api_user, authenticated_service -from app.celery import QueueNames +from app.config import QueueNames from app.dao import ( templates_dao, notifications_dao diff --git a/app/service/send_notification.py b/app/service/send_notification.py index 6119ac714..1ca19661c 100644 --- a/app/service/send_notification.py +++ b/app/service/send_notification.py @@ -1,4 +1,4 @@ -from app.celery import QueueNames +from app.config import QueueNames from app.notifications.validators import ( check_service_over_daily_message_limit, validate_and_format_recipient, diff --git a/app/service/sender.py b/app/service/sender.py index 3c4ef17d9..4919a93bf 100644 --- a/app/service/sender.py +++ b/app/service/sender.py @@ -1,6 +1,6 @@ from flask import current_app -from app.celery import QueueNames +from app.config import QueueNames from app.dao.services_dao import dao_fetch_service_by_id, dao_fetch_active_users_for_service from app.dao.templates_dao import dao_get_template_by_id from app.models import EMAIL_TYPE, KEY_TYPE_NORMAL diff --git a/app/user/rest.py b/app/user/rest.py index 45c649940..9a9cf3832 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -4,7 +4,7 @@ from datetime import datetime from flask import (jsonify, request, Blueprint, current_app) -from app.celery import QueueNames +from app.config import QueueNames from app.dao.users_dao import ( get_user_by_id, save_model_user, diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index f9da36f88..274005386 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -1,8 +1,8 @@ from flask import request, jsonify, current_app from app import api_user, authenticated_service -from app.models import SMS_TYPE, EMAIL_TYPE, PRIORITY -from app.celery import QueueNames +from app.config import QueueNames +from app.models import SMS_TYPE, EMAIL_TYPE, PRIORITY, SCHEDULE_NOTIFICATIONS from app.notifications.process_notifications import ( persist_notification, send_notification_to_queue, @@ -11,17 +11,20 @@ from app.notifications.process_notifications import ( from app.notifications.validators import ( validate_and_format_recipient, check_rate_limiting, + service_has_permission, check_service_can_schedule_notification, check_service_has_permission, validate_template ) from app.schema_validation import validate +from app.utils import get_public_notify_type_text from app.v2.notifications import v2_notification_blueprint from app.v2.notifications.notification_schemas import ( post_sms_request, create_post_sms_response_from_notification, post_email_request, create_post_email_response_from_notification) +from app.v2.errors import BadRequestError @v2_notification_blueprint.route('/', methods=['POST']) diff --git a/docker/Dockerfile b/docker/Dockerfile index 0a96a16f6..2ccfe0e1e 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -20,7 +20,6 @@ RUN \ zip \ libpq-dev \ jq \ - libcurl4-openssl-dev \ && echo "Clean up" \ && rm -rf /var/lib/apt/lists/* /tmp/* diff --git a/requirements.txt b/requirements.txt index de027e13c..b1d1cd307 100644 --- a/requirements.txt +++ b/requirements.txt @@ -13,7 +13,7 @@ flask-marshmallow==0.6.2 Flask-Bcrypt==0.6.2 credstash==1.8.0 boto3==1.4.4 -celery==4.0.2 +celery==3.1.25 monotonic==1.2 statsd==3.2.1 jsonschema==2.5.1 @@ -21,7 +21,6 @@ gunicorn==19.6.0 docopt==0.6.2 six==1.10.0 iso8601==0.1.11 -pycurl==7.43.0 # pin to minor version 3.1.x notifications-python-client>=3.1,<3.2 @@ -32,10 +31,4 @@ awscli-cwlogs>=1.4,<1.5 git+https://github.com/alphagov/notifications-utils.git@17.5.3#egg=notifications-utils==17.5.3 -# Kombu is a library that celery uses under the hood. -# Kombu v4.0.2 (which ships with celery v4.0.2) doesn't work with SQS due to problems with their use of boto2, so -# Kombu migrated to boto3 - We're waiting for that to get a release version, and then to get a new version of celery -# that pulls that in. Until that point, we should override the kombu version to get these SQS fixes. -# Additionally, kombu master also includes a fix for the main process taking 100% CPU and not distributing tasks (!) -# See https://github.com/celery/kombu/pull/693 and https://github.com/celery/kombu/pull/760 -https://github.com/celery/kombu/zipball/b2f21289284496efd89acea003ff9c24105b970e +git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/tests/app/celery/test_statistics_tasks.py b/tests/app/celery/test_statistics_tasks.py index c97007f55..40d20117d 100644 --- a/tests/app/celery/test_statistics_tasks.py +++ b/tests/app/celery/test_statistics_tasks.py @@ -1,21 +1,14 @@ import pytest -from sqlalchemy.exc import SQLAlchemyError - -from app import create_uuid from app.celery.statistics_tasks import ( record_initial_job_statistics, record_outcome_job_statistics, create_initial_notification_statistic_tasks, create_outcome_notification_statistic_tasks) -from app.models import ( - NOTIFICATION_STATUS_TYPES_COMPLETED, - NOTIFICATION_SENDING, - NOTIFICATION_PENDING, - NOTIFICATION_CREATED, - NOTIFICATION_DELIVERED -) - +from sqlalchemy.exc import SQLAlchemyError +from app import create_uuid from tests.app.conftest import sample_notification +from app.models import NOTIFICATION_STATUS_TYPES_COMPLETED, NOTIFICATION_SENT, NOTIFICATION_SENDING, \ + NOTIFICATION_PENDING, NOTIFICATION_CREATED, NOTIFICATION_DELIVERED def test_should_create_initial_job_task_if_notification_is_related_to_a_job( diff --git a/tests/app/service/test_send_one_off_notification.py b/tests/app/service/test_send_one_off_notification.py index 3baf886e7..89ddcd91a 100644 --- a/tests/app/service/test_send_one_off_notification.py +++ b/tests/app/service/test_send_one_off_notification.py @@ -5,7 +5,7 @@ import pytest from notifications_utils.recipients import InvalidPhoneError from app.v2.errors import BadRequestError, TooManyRequestsError -from app.celery import QueueNames +from app.config import QueueNames from app.service.send_notification import send_one_off_notification from app.models import KEY_TYPE_NORMAL, PRIORITY, SMS_TYPE From 793248a74f0da249bbe9e23582b00dff953919aa Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 19 Jul 2017 15:47:12 +0100 Subject: [PATCH 07/22] Fix data migration merge conflict --- .../{0109_monthly_billing.py => 0110_monthly_billing.py} | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) rename migrations/versions/{0109_monthly_billing.py => 0110_monthly_billing.py} (89%) diff --git a/migrations/versions/0109_monthly_billing.py b/migrations/versions/0110_monthly_billing.py similarity index 89% rename from migrations/versions/0109_monthly_billing.py rename to migrations/versions/0110_monthly_billing.py index 124a509cf..19b1ce4fc 100644 --- a/migrations/versions/0109_monthly_billing.py +++ b/migrations/versions/0110_monthly_billing.py @@ -1,14 +1,14 @@ """empty message -Revision ID: 0109_monthly_billing -Revises: 0108_change_logo_not_nullable +Revision ID: 0110_monthly_billing +Revises: 0109_rem_old_noti_status Create Date: 2017-07-13 14:35:03.183659 """ # revision identifiers, used by Alembic. -revision = '0109_monthly_billing' -down_revision = '0108_change_logo_not_nullable' +revision = '0110_monthly_billing' +down_revision = '0109_rem_old_noti_status' from alembic import op import sqlalchemy as sa From 7f4eec79e421bc7de97ac60e28d3461094b5495c Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 7 Jul 2017 10:46:26 +0100 Subject: [PATCH 08/22] add POST letter schema similar to sms/email, however, for consistency with response values and internal storage, rather than supplying an "email_address" field or a "phone_number" field, supply an "address_line_1" and "postcode" field within the personalisation object. --- app/schema_validation/definitions.py | 3 + app/v2/notifications/notification_schemas.py | 109 ++++++++---- app/v2/notifications/post_notifications.py | 7 +- .../test_post_letter_notifications.py | 158 ++++++++++++++++++ .../notifications/test_post_notifications.py | 4 +- 5 files changed, 243 insertions(+), 38 deletions(-) create mode 100644 tests/app/v2/notifications/test_post_letter_notifications.py diff --git a/app/schema_validation/definitions.py b/app/schema_validation/definitions.py index f0fdc9356..e779c5794 100644 --- a/app/schema_validation/definitions.py +++ b/app/schema_validation/definitions.py @@ -20,6 +20,9 @@ personalisation = { } +letter_personalisation = dict(personalisation, required=["address_line_1", "postcode"]) + + https_url = { "type": "string", "format": "uri", diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py index 69372c4e1..0f7081e00 100644 --- a/app/v2/notifications/notification_schemas.py +++ b/app/v2/notifications/notification_schemas.py @@ -1,5 +1,5 @@ from app.models import NOTIFICATION_STATUS_TYPES, TEMPLATE_TYPES -from app.schema_validation.definitions import (uuid, personalisation) +from app.schema_validation.definitions import (uuid, personalisation, letter_personalisation) template = { @@ -192,40 +192,89 @@ post_email_response = { } -def create_post_sms_response_from_notification(notification, body, from_number, url_root, service_id, scheduled_for): - return {"id": notification.id, - "reference": notification.client_reference, - "content": {'body': body, - 'from_number': from_number}, - "uri": "{}v2/notifications/{}".format(url_root, str(notification.id)), - "template": __create_template_from_notification(notification=notification, - url_root=url_root, - service_id=service_id), - "scheduled_for": scheduled_for if scheduled_for else None - } +post_letter_request = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "POST letter notification schema", + "type": "object", + "title": "POST v2/notifications/letter", + "properties": { + "reference": {"type": "string"}, + "template_id": uuid, + "personalisation": letter_personalisation + }, + "required": ["template_id", "personalisation"] +} + +letter_content = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "Letter content for POST letter notification", + "type": "object", + "title": "notification letter content", + "properties": { + "body": {"type": "string"}, + "subject": {"type": "string"} + }, + "required": ["body", "subject"] +} + +post_letter_response = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "POST sms notification response schema", + "type": "object", + "title": "response v2/notifications/letter", + "properties": { + "id": uuid, + "reference": {"type": ["string", "null"]}, + "content": letter_content, + "uri": {"type": "string", "format": "uri"}, + "template": template, + "scheduled_for": {"type": ["string", "null"]} + }, + "required": ["id", "content", "uri", "template"] +} -def create_post_email_response_from_notification(notification, content, subject, email_from, url_root, service_id, - scheduled_for): +def create_post_sms_response_from_notification(notification, body, from_number, url_root, scheduled_for): + noti = __create_notification_response(notification, url_root, scheduled_for) + noti['content'] = { + 'from_number': from_number, + 'body': body + } + return noti + + +def create_post_email_response_from_notification(notification, content, subject, email_from, url_root, scheduled_for): + noti = __create_notification_response(notification, url_root, scheduled_for) + noti['content'] = { + "from_email": email_from, + "body": content, + "subject": subject + } + return noti + + +def create_post_letter_response_from_notification(notification, content, subject, url_root, scheduled_for): + noti = __create_notification_response(notification, url_root, scheduled_for) + noti['content'] = { + "body": content, + "subject": subject + } + return noti + + +def __create_notification_response(notification, url_root, scheduled_for): return { "id": notification.id, "reference": notification.client_reference, - "content": { - "from_email": email_from, - "body": content, - "subject": subject - }, "uri": "{}v2/notifications/{}".format(url_root, str(notification.id)), - "template": __create_template_from_notification(notification=notification, - url_root=url_root, - service_id=service_id), + 'template': { + "id": notification.template_id, + "version": notification.template_version, + "uri": "{}services/{}/templates/{}".format( + url_root, + str(notification.service_id), + str(notification.template_id) + ) + }, "scheduled_for": scheduled_for if scheduled_for else None } - - -def __create_template_from_notification(notification, url_root, service_id): - return { - "id": notification.template_id, - "version": notification.template_version, - "uri": "{}services/{}/templates/{}".format(url_root, str(service_id), str(notification.template_id)) - } diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 274005386..9ccbdb70a 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -2,7 +2,7 @@ from flask import request, jsonify, current_app from app import api_user, authenticated_service from app.config import QueueNames -from app.models import SMS_TYPE, EMAIL_TYPE, PRIORITY, SCHEDULE_NOTIFICATIONS +from app.models import SMS_TYPE, EMAIL_TYPE, PRIORITY from app.notifications.process_notifications import ( persist_notification, send_notification_to_queue, @@ -11,20 +11,17 @@ from app.notifications.process_notifications import ( from app.notifications.validators import ( validate_and_format_recipient, check_rate_limiting, - service_has_permission, check_service_can_schedule_notification, check_service_has_permission, validate_template ) from app.schema_validation import validate -from app.utils import get_public_notify_type_text from app.v2.notifications import v2_notification_blueprint from app.v2.notifications.notification_schemas import ( post_sms_request, create_post_sms_response_from_notification, post_email_request, create_post_email_response_from_notification) -from app.v2.errors import BadRequestError @v2_notification_blueprint.route('/', methods=['POST']) @@ -87,7 +84,6 @@ def post_notification(notification_type): body=str(template_with_content), from_number=sms_sender, url_root=request.url_root, - service_id=authenticated_service.id, scheduled_for=scheduled_for) else: resp = create_post_email_response_from_notification(notification=notification, @@ -95,6 +91,5 @@ def post_notification(notification_type): subject=template_with_content.subject, email_from=authenticated_service.email_from, url_root=request.url_root, - service_id=authenticated_service.id, scheduled_for=scheduled_for) return jsonify(resp), 201 diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py new file mode 100644 index 000000000..394c6bc92 --- /dev/null +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -0,0 +1,158 @@ +import uuid + +from flask import url_for, json +import pytest + +from app.models import Job, Notification, SMS_TYPE, EMAIL_TYPE, LETTER_TYPE +from app.v2.errors import RateLimitError + +from tests import create_authorization_header +from tests.app.db import create_service, create_template + + +def letter_request(client, data, service_id, _expected_status=201): + resp = client.post( + url_for('v2_notifications.post_notification', notification_type='letter'), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header(service_id=service_id)] + ) + assert resp.status_code == _expected_status + json_resp = json.loads(resp.get_data(as_text=True)) + return json_resp + + +@pytest.mark.parametrize('reference', [None, 'reference_from_client']) +def test_post_letter_notification_returns_201(client, sample_letter_template, mocker, reference): + mocked = mocker.patch('app.celery.tasks.build_dvla_file.apply_async') + data = { + 'template_id': str(sample_letter_template.id), + 'personalisation': { + 'address_line_1': 'Her Royal Highness Queen Elizabeth II', + 'address_line_2': 'Buckingham Palace', + 'address_line_3': 'London', + 'postcode': 'SW1 1AA', + 'name': 'Lizzie' + } + } + + if reference: + data.update({'reference': reference}) + + resp_json = letter_request(client, data, service_id=sample_letter_template.service_id) + + job = Job.query.one() + notification = Notification.query.all() + notification_id = notification.id + assert resp_json['id'] == str(notification_id) + assert resp_json['reference'] == reference + assert resp_json['content']['subject'] == sample_letter_template.subject + assert resp_json['content']['body'] == sample_letter_template.content + assert 'v2/notifications/{}'.format(notification_id) in resp_json['uri'] + assert resp_json['template']['id'] == str(sample_letter_template.id) + assert resp_json['template']['version'] == sample_letter_template.version + assert ( + 'services/{}/templates/{}'.format( + sample_letter_template.service_id, + sample_letter_template.id + ) in resp_json['template']['uri'] + ) + assert not resp_json['scheduled_for'] + + mocked.assert_called_once_with((str(job.id), ), queue='job-tasks') + + +def test_post_letter_notification_returns_400_and_missing_template( + client, + sample_service +): + data = { + 'template_id': str(uuid.uuid4()), + 'personalisation': {'address_line_1': '', 'postcode': ''} + } + + error_json = letter_request(client, data, service_id=sample_service.id, _expected_status=400) + + assert error_json['status_code'] == 400 + assert error_json['errors'] == [{'error': 'BadRequestError', 'message': 'Template not found'}] + + + +def test_post_notification_returns_403_and_well_formed_auth_error( + client, + sample_letter_template +): + data = { + 'template_id': str(sample_letter_template.id), + 'personalisation': {'address_line_1': '', 'postcode': ''} + } + + error_json = letter_request(client, data, service_id=sample_letter_template.service_id, _expected_status=401) + + assert error_json['status_code'] == 401 + assert error_json['errors'] == [{ + 'error': 'AuthError', + 'message': 'Unauthorized, authentication token must be provided' + }] + + +def test_notification_returns_400_for_schema_problems( + client, + sample_service +): + data = { + 'personalisation': {'address_line_1': '', 'postcode': ''} + } + + error_json = letter_request(client, data, service_id=sample_service.id, _expected_status=400) + + assert error_json['status_code'] == 400 + assert error_json['errors'] == [{ + 'error': 'ValidationError', + 'message': 'template_id is a required property' + }] + + +def test_returns_a_429_limit_exceeded_if_rate_limit_exceeded( + client, + sample_letter_template, + mocker +): + persist_mock = mocker.patch('app.v2.notifications.post_notifications.persist_notification') + mocker.patch( + 'app.v2.notifications.post_notifications.check_rate_limiting', + side_effect=RateLimitError('LIMIT', 'INTERVAL', 'TYPE') + ) + + data = { + 'template_id': str(sample_letter_template.id), + 'personalisation': {'address_line_1': '', 'postcode': ''} + } + + error_json = letter_request(client, data, service_id=sample_letter_template.service_id, _expected_status=429) + + assert error_json['status_code'] == 429 + assert error_json['errors'] == [{ + 'error': 'RateLimitError', + 'message': 'Exceeded rate limit for key type TYPE of LIMIT requests per INTERVAL seconds' + }] + + assert not persist_mock.called + + +def test_post_letter_notification_returns_400_if_not_allowed_to_send_notification( + client, + notify_db_session +): + service = create_service(service_permissions=[EMAIL_TYPE, SMS_TYPE]) + template = create_template(service, template_type=LETTER_TYPE) + + data = { + 'template_id': str(template.id), + 'personalisation': {'address_line_1': '', 'postcode': ''} + } + + error_json = letter_request(client, data, service_id=service.id, _expected_status=400) + assert error_json['status_code'] == 400 + assert error_json['errors'] == [ + {'error': 'BadRequestError', 'message': 'Cannot send text letters'} + ] diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index aea003b7e..8a180adff 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -58,8 +58,8 @@ def test_post_sms_notification_returns_201(client, sample_template_with_placehol @pytest.mark.parametrize("notification_type, key_send_to, send_to", [("sms", "phone_number", "+447700900855"), ("email", "email_address", "sample@email.com")]) -def test_post_sms_notification_returns_400_and_missing_template(client, sample_service, - notification_type, key_send_to, send_to): +def test_post_notification_returns_400_and_missing_template(client, sample_service, + notification_type, key_send_to, send_to): data = { key_send_to: send_to, 'template_id': str(uuid.uuid4()) From 2be194d9cee1061b9604d8141f2611e8facb8fc2 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 7 Jul 2017 17:10:16 +0100 Subject: [PATCH 09/22] refactor post_notification to separate sms/email and letter flows --- app/v2/notifications/notification_schemas.py | 4 +- app/v2/notifications/post_notifications.py | 125 +++++++++++++------ 2 files changed, 91 insertions(+), 38 deletions(-) diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py index 0f7081e00..fbd8fba53 100644 --- a/app/v2/notifications/notification_schemas.py +++ b/app/v2/notifications/notification_schemas.py @@ -234,11 +234,11 @@ post_letter_response = { } -def create_post_sms_response_from_notification(notification, body, from_number, url_root, scheduled_for): +def create_post_sms_response_from_notification(notification, content, from_number, url_root, scheduled_for): noti = __create_notification_response(notification, url_root, scheduled_for) noti['content'] = { 'from_number': from_number, - 'body': body + 'body': content } return noti diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 9ccbdb70a..ea02f3c7f 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -1,8 +1,10 @@ +import functools + from flask import request, jsonify, current_app from app import api_user, authenticated_service from app.config import QueueNames -from app.models import SMS_TYPE, EMAIL_TYPE, PRIORITY +from app.models import SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, PRIORITY from app.notifications.process_notifications import ( persist_notification, send_notification_to_queue, @@ -16,20 +18,28 @@ from app.notifications.validators import ( validate_template ) from app.schema_validation import validate +from app.v2.errors import BadRequestError from app.v2.notifications import v2_notification_blueprint from app.v2.notifications.notification_schemas import ( post_sms_request, - create_post_sms_response_from_notification, post_email_request, - create_post_email_response_from_notification) + post_letter_request, + create_post_sms_response_from_notification, + create_post_email_response_from_notification, + create_post_letter_response_from_notification +) @v2_notification_blueprint.route('/', methods=['POST']) def post_notification(notification_type): if notification_type == EMAIL_TYPE: form = validate(request.get_json(), post_email_request) - else: + elif notification_type == SMS_TYPE: form = validate(request.get_json(), post_sms_request) + elif notification_type == LETTER_TYPE: + form = validate(request.get_json(), post_letter_request) + else: + raise BadRequestError(message='Unknown notification type {}'.format(notification_type)) check_service_has_permission(notification_type, authenticated_service.permissions) @@ -38,12 +48,6 @@ def post_notification(notification_type): check_rate_limiting(authenticated_service, api_user) - form_send_to = form['phone_number'] if notification_type == SMS_TYPE else form['email_address'] - send_to = validate_and_format_recipient(send_to=form_send_to, - key_type=api_user.key_type, - service=authenticated_service, - notification_type=notification_type) - template, template_with_content = validate_template( form['template_id'], form.get('personalisation', {}), @@ -51,20 +55,74 @@ def post_notification(notification_type): notification_type, ) + if notification_type == LETTER_TYPE: + notification = process_letter_notification( + form=form, + api_key=api_user, + template=template, + service=authenticated_service, + ) + else: + notification = process_sms_or_email_notification( + form=form, + notification_type=notification_type, + api_key=api_user, + template=template, + service=authenticated_service + ) + + if notification_type == SMS_TYPE: + sms_sender = authenticated_service.sms_sender or current_app.config.get('FROM_NUMBER') + create_resp_partial = functools.partial( + create_post_sms_response_from_notification, + from_number=sms_sender + ) + elif notification_type == EMAIL_TYPE: + create_resp_partial = functools.partial( + create_post_email_response_from_notification, + subject=template_with_content.subject, + email_from=authenticated_service.email_from + ) + elif notification_type == LETTER_TYPE: + create_resp_partial = functools.partial( + create_post_letter_response_from_notification, + subject=template_with_content.subject, + ) + + resp = create_resp_partial( + notification=notification, + content=str(template_with_content), + url_root=request.url_root, + scheduled_for=scheduled_for + ) + return jsonify(resp), 201 + + +def process_sms_or_email_notification(*, form, notification_type, api_key, template, service): + form_send_to = form['email_address'] if notification_type == EMAIL_TYPE else form['phone_number'] + + send_to = validate_and_format_recipient(send_to=form_send_to, + key_type=api_key.key_type, + service=service, + notification_type=notification_type) + # Do not persist or send notification to the queue if it is a simulated recipient simulated = simulated_recipient(send_to, notification_type) - notification = persist_notification(template_id=template.id, - template_version=template.version, - recipient=form_send_to, - service=authenticated_service, - personalisation=form.get('personalisation', None), - notification_type=notification_type, - api_key_id=api_user.id, - key_type=api_user.key_type, - client_reference=form.get('reference', None), - simulated=simulated) + notification = persist_notification( + template_id=template.id, + template_version=template.version, + recipient=form_send_to, + service=service, + personalisation=form.get('personalisation', None), + notification_type=notification_type, + api_key_id=api_key.id, + key_type=api_key.key_type, + client_reference=form.get('reference', None), + simulated=simulated + ) + scheduled_for = form.get("scheduled_for", None) if scheduled_for: persist_scheduled_notification(notification.id, form["scheduled_for"]) else: @@ -72,24 +130,19 @@ def post_notification(notification_type): queue_name = QueueNames.PRIORITY if template.process_type == PRIORITY else None send_notification_to_queue( notification=notification, - research_mode=authenticated_service.research_mode, + research_mode=service.research_mode, queue=queue_name ) else: current_app.logger.info("POST simulated notification for id: {}".format(notification.id)) - if notification_type == SMS_TYPE: - sms_sender = authenticated_service.sms_sender or current_app.config.get('FROM_NUMBER') - resp = create_post_sms_response_from_notification(notification=notification, - body=str(template_with_content), - from_number=sms_sender, - url_root=request.url_root, - scheduled_for=scheduled_for) - else: - resp = create_post_email_response_from_notification(notification=notification, - content=str(template_with_content), - subject=template_with_content.subject, - email_from=authenticated_service.email_from, - url_root=request.url_root, - scheduled_for=scheduled_for) - return jsonify(resp), 201 + return notification + + +def process_letter_notification(*, form, api_key, template, service): + # create job + + # create notification + + # trigger build_dvla_file task + raise NotImplementedError From 9caf45451e388e254b74777044676d9d3c4b24b6 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 7 Jul 2017 17:10:25 +0100 Subject: [PATCH 10/22] make persist_notification require kwargs when functions get as big as that, it's confusing to try and work out what things are what. By including a * as the first arg, we require that anyone calling the function has to use kwargs to reference the parameters --- app/notifications/process_notifications.py | 1 + app/notifications/validators.py | 4 +- .../test_process_notification.py | 59 +++++++++++++------ .../test_post_letter_notifications.py | 17 ++++-- .../notifications/test_post_notifications.py | 14 +++++ 5 files changed, 70 insertions(+), 25 deletions(-) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index ca3ae0b53..ee03b537e 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -35,6 +35,7 @@ def check_placeholders(template_object): def persist_notification( + *, template_id, template_version, recipient, diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 3aa86c963..83b620506 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -9,7 +9,7 @@ from notifications_utils.clients.redis import rate_limit_cache_key, daily_limit_ from app.dao import services_dao, templates_dao from app.models import ( - INTERNATIONAL_SMS_TYPE, SMS_TYPE, + INTERNATIONAL_SMS_TYPE, SMS_TYPE, EMAIL_TYPE, KEY_TYPE_TEST, KEY_TYPE_TEAM, SCHEDULE_NOTIFICATIONS ) from app.service.utils import service_allowed_to_send_to @@ -104,7 +104,7 @@ def validate_and_format_recipient(send_to, key_type, service, notification_type) number=send_to, international=international_phone_info.international ) - else: + elif notification_type == EMAIL_TYPE: return validate_and_format_email_address(email_address=send_to) diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index ac73b36da..407ad2093 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -51,10 +51,18 @@ def test_persist_notification_creates_and_save_to_db(sample_template, sample_api assert Notification.query.count() == 0 assert NotificationHistory.query.count() == 0 - notification = persist_notification(sample_template.id, sample_template.version, '+447111111111', - sample_template.service, {}, 'sms', sample_api_key.id, - sample_api_key.key_type, job_id=sample_job.id, - job_row_number=100, reference="ref") + notification = persist_notification( + template_id=sample_template.id, + template_version=sample_template.version, + recipient='+447111111111', + service=sample_template.service, + personalisation={}, + notification_type='sms', + api_key_id=sample_api_key.id, + key_type=sample_api_key.key_type, + job_id=sample_job.id, + job_row_number=100, + reference="ref") assert Notification.query.get(notification.id) is not None assert NotificationHistory.query.get(notification.id) is not None @@ -127,14 +135,14 @@ def test_persist_notification_does_not_increment_cache_if_test_key( assert Notification.query.count() == 0 assert NotificationHistory.query.count() == 0 persist_notification( - sample_template.id, - sample_template.version, - '+447111111111', - sample_template.service, - {}, - 'sms', - api_key.id, - api_key.key_type, + template_id=sample_template.id, + template_version=sample_template.version, + recipient='+447111111111', + service=sample_template.service, + personalisation={}, + notification_type='sms', + api_key_id=api_key.id, + key_type=api_key.key_type, job_id=sample_job.id, job_row_number=100, reference="ref", @@ -193,18 +201,33 @@ def test_persist_notification_increments_cache_if_key_exists(sample_template, sa mock_incr = mocker.patch('app.notifications.process_notifications.redis_store.incr') mock_incr_hash_value = mocker.patch('app.notifications.process_notifications.redis_store.increment_hash_value') - persist_notification(sample_template.id, sample_template.version, '+447111111111', - sample_template.service, {}, 'sms', sample_api_key.id, - sample_api_key.key_type, reference="ref") + persist_notification( + template_id=sample_template.id, + template_version=sample_template.version, + recipient='+447111111111', + service=sample_template.service, + personalisation={}, + notification_type='sms', + api_key_id=sample_api_key.id, + key_type=sample_api_key.key_type, + reference="ref" + ) mock_incr.assert_not_called() mock_incr_hash_value.assert_not_called() mocker.patch('app.notifications.process_notifications.redis_store.get', return_value=1) mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash', return_value={sample_template.id, 1}) - persist_notification(sample_template.id, sample_template.version, '+447111111122', - sample_template.service, {}, 'sms', sample_api_key.id, - sample_api_key.key_type, reference="ref2") + persist_notification( + template_id=sample_template.id, + template_version=sample_template.version, + recipient='+447111111122', + service=sample_template.service, + personalisation={}, + notification_type='sms', + api_key_id=sample_api_key.id, + key_type=sample_api_key.key_type, + reference="ref2") mock_incr.assert_called_once_with(str(sample_template.service_id) + "-2016-01-01-count", ) mock_incr_hash_value.assert_called_once_with(cache_key_for_service_template_counter(sample_template.service_id), sample_template.id) diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index 394c6bc92..7c8f2b34d 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -10,14 +10,17 @@ from tests import create_authorization_header from tests.app.db import create_service, create_template +pytestmark = pytest.mark.skip('Leters not currently implemented') + + def letter_request(client, data, service_id, _expected_status=201): resp = client.post( url_for('v2_notifications.post_notification', notification_type='letter'), data=json.dumps(data), headers=[('Content-Type', 'application/json'), create_authorization_header(service_id=service_id)] ) - assert resp.status_code == _expected_status json_resp = json.loads(resp.get_data(as_text=True)) + assert resp.status_code == _expected_status, json_resp return json_resp @@ -76,7 +79,6 @@ def test_post_letter_notification_returns_400_and_missing_template( assert error_json['errors'] == [{'error': 'BadRequestError', 'message': 'Template not found'}] - def test_post_notification_returns_403_and_well_formed_auth_error( client, sample_letter_template @@ -139,11 +141,16 @@ def test_returns_a_429_limit_exceeded_if_rate_limit_exceeded( assert not persist_mock.called +@pytest.mark.parametrize('service_args', [ + {'service_permissions': [EMAIL_TYPE, SMS_TYPE]}, + {'restricted': True} +]) def test_post_letter_notification_returns_400_if_not_allowed_to_send_notification( client, - notify_db_session + notify_db_session, + service_args ): - service = create_service(service_permissions=[EMAIL_TYPE, SMS_TYPE]) + service = create_service(**service_args) template = create_template(service, template_type=LETTER_TYPE) data = { @@ -154,5 +161,5 @@ def test_post_letter_notification_returns_400_if_not_allowed_to_send_notificatio error_json = letter_request(client, data, service_id=service.id, _expected_status=400) assert error_json['status_code'] == 400 assert error_json['errors'] == [ - {'error': 'BadRequestError', 'message': 'Cannot send text letters'} + {'error': 'BadRequestError', 'message': 'Cannot send letters'} ] diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 8a180adff..597cdf1b7 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -433,3 +433,17 @@ def test_post_notification_raises_bad_request_if_service_not_invited_to_schedule error_json = json.loads(response.get_data(as_text=True)) assert error_json['errors'] == [ {"error": "BadRequestError", "message": 'Cannot schedule notifications (this feature is invite-only)'}] + + +def test_post_notification_raises_bad_request_if_not_valid_notification_type(client, sample_service): + auth_header = create_authorization_header(service_id=sample_service.id) + response = client.post( + '/v2/notifications/foo', + data='{}', + headers=[('Content-Type', 'application/json'), auth_header] + ) + assert response.status_code == 400 + error_json = json.loads(response.get_data(as_text=True)) + assert error_json['errors'] == [ + {'error': 'BadRequestError', 'message': 'Unknown notification type foo'} + ] From 79e33073c9bf90bf2dbd89ade8a037673bf5920a Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 20 Jul 2017 15:23:46 +0100 Subject: [PATCH 11/22] raise 404 when unknown url consistent with other endpoints. also refactor of notification_schema to separate some fns to a different file --- app/v2/notifications/create_response.py | 45 ++++++++++++++++++ app/v2/notifications/notification_schemas.py | 46 ------------------- app/v2/notifications/post_notifications.py | 9 ++-- .../test_post_letter_notifications.py | 5 +- .../notifications/test_post_notifications.py | 6 +-- 5 files changed, 55 insertions(+), 56 deletions(-) create mode 100644 app/v2/notifications/create_response.py diff --git a/app/v2/notifications/create_response.py b/app/v2/notifications/create_response.py new file mode 100644 index 000000000..7eecfb1b9 --- /dev/null +++ b/app/v2/notifications/create_response.py @@ -0,0 +1,45 @@ + +def create_post_sms_response_from_notification(notification, content, from_number, url_root, scheduled_for): + noti = __create_notification_response(notification, url_root, scheduled_for) + noti['content'] = { + 'from_number': from_number, + 'body': content + } + return noti + + +def create_post_email_response_from_notification(notification, content, subject, email_from, url_root, scheduled_for): + noti = __create_notification_response(notification, url_root, scheduled_for) + noti['content'] = { + "from_email": email_from, + "body": content, + "subject": subject + } + return noti + + +def create_post_letter_response_from_notification(notification, content, subject, url_root, scheduled_for): + noti = __create_notification_response(notification, url_root, scheduled_for) + noti['content'] = { + "body": content, + "subject": subject + } + return noti + + +def __create_notification_response(notification, url_root, scheduled_for): + return { + "id": notification.id, + "reference": notification.client_reference, + "uri": "{}v2/notifications/{}".format(url_root, str(notification.id)), + 'template': { + "id": notification.template_id, + "version": notification.template_version, + "uri": "{}services/{}/templates/{}".format( + url_root, + str(notification.service_id), + str(notification.template_id) + ) + }, + "scheduled_for": scheduled_for if scheduled_for else None + } diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py index fbd8fba53..7f61f0ee0 100644 --- a/app/v2/notifications/notification_schemas.py +++ b/app/v2/notifications/notification_schemas.py @@ -232,49 +232,3 @@ post_letter_response = { }, "required": ["id", "content", "uri", "template"] } - - -def create_post_sms_response_from_notification(notification, content, from_number, url_root, scheduled_for): - noti = __create_notification_response(notification, url_root, scheduled_for) - noti['content'] = { - 'from_number': from_number, - 'body': content - } - return noti - - -def create_post_email_response_from_notification(notification, content, subject, email_from, url_root, scheduled_for): - noti = __create_notification_response(notification, url_root, scheduled_for) - noti['content'] = { - "from_email": email_from, - "body": content, - "subject": subject - } - return noti - - -def create_post_letter_response_from_notification(notification, content, subject, url_root, scheduled_for): - noti = __create_notification_response(notification, url_root, scheduled_for) - noti['content'] = { - "body": content, - "subject": subject - } - return noti - - -def __create_notification_response(notification, url_root, scheduled_for): - return { - "id": notification.id, - "reference": notification.client_reference, - "uri": "{}v2/notifications/{}".format(url_root, str(notification.id)), - 'template': { - "id": notification.template_id, - "version": notification.template_version, - "uri": "{}services/{}/templates/{}".format( - url_root, - str(notification.service_id), - str(notification.template_id) - ) - }, - "scheduled_for": scheduled_for if scheduled_for else None - } diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index ea02f3c7f..f39f54345 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -1,6 +1,6 @@ import functools -from flask import request, jsonify, current_app +from flask import request, jsonify, current_app, abort from app import api_user, authenticated_service from app.config import QueueNames @@ -18,12 +18,13 @@ from app.notifications.validators import ( validate_template ) from app.schema_validation import validate -from app.v2.errors import BadRequestError from app.v2.notifications import v2_notification_blueprint from app.v2.notifications.notification_schemas import ( post_sms_request, post_email_request, - post_letter_request, + post_letter_request +) +from app.v2.notifications.create_response import ( create_post_sms_response_from_notification, create_post_email_response_from_notification, create_post_letter_response_from_notification @@ -39,7 +40,7 @@ def post_notification(notification_type): elif notification_type == LETTER_TYPE: form = validate(request.get_json(), post_letter_request) else: - raise BadRequestError(message='Unknown notification type {}'.format(notification_type)) + abort(404) check_service_has_permission(notification_type, authenticated_service.permissions) diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index 7c8f2b34d..d65c6c5d8 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -1,3 +1,4 @@ + import uuid from flask import url_for, json @@ -145,7 +146,7 @@ def test_returns_a_429_limit_exceeded_if_rate_limit_exceeded( {'service_permissions': [EMAIL_TYPE, SMS_TYPE]}, {'restricted': True} ]) -def test_post_letter_notification_returns_400_if_not_allowed_to_send_notification( +def test_post_letter_notification_returns_403_if_not_allowed_to_send_notification( client, notify_db_session, service_args @@ -159,7 +160,7 @@ def test_post_letter_notification_returns_400_if_not_allowed_to_send_notificatio } error_json = letter_request(client, data, service_id=service.id, _expected_status=400) - assert error_json['status_code'] == 400 + assert error_json['status_code'] == 403 assert error_json['errors'] == [ {'error': 'BadRequestError', 'message': 'Cannot send letters'} ] diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 597cdf1b7..40475e030 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -442,8 +442,6 @@ def test_post_notification_raises_bad_request_if_not_valid_notification_type(cli data='{}', headers=[('Content-Type', 'application/json'), auth_header] ) - assert response.status_code == 400 + assert response.status_code == 404 error_json = json.loads(response.get_data(as_text=True)) - assert error_json['errors'] == [ - {'error': 'BadRequestError', 'message': 'Unknown notification type foo'} - ] + assert 'The requested URL was not found on the server.' in error_json['message'] From 4d330406534ba10b0fcdc757daa4237adf16cfe1 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 20 Jul 2017 15:48:21 +0100 Subject: [PATCH 12/22] add separate send-sms and send-email queues we're reading from those two queues as well as teh existing send queue, however for now we don't send anything to them --- app/celery/tasks.py | 4 ++-- app/config.py | 8 ++++++-- app/delivery/rest.py | 2 +- app/notifications/process_notifications.py | 2 +- manifest-delivery-base.yml | 2 +- 5 files changed, 11 insertions(+), 7 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index da9d1f9ee..c3f533a6d 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -182,7 +182,7 @@ def send_sms(self, provider_tasks.deliver_sms.apply_async( [str(saved_notification.id)], - queue=QueueNames.SEND if not service.research_mode else QueueNames.RESEARCH_MODE + queue=QueueNames.SEND_COMBINED if not service.research_mode else QueueNames.RESEARCH_MODE ) current_app.logger.info( @@ -227,7 +227,7 @@ def send_email(self, provider_tasks.deliver_email.apply_async( [str(saved_notification.id)], - queue=QueueNames.SEND if not service.research_mode else QueueNames.RESEARCH_MODE + queue=QueueNames.SEND_COMBINED if not service.research_mode else QueueNames.RESEARCH_MODE ) current_app.logger.info("Email {} created at {}".format(saved_notification.id, created_at)) diff --git a/app/config.py b/app/config.py index 1bbbb2057..99b77cc8d 100644 --- a/app/config.py +++ b/app/config.py @@ -22,7 +22,9 @@ class QueueNames(object): PERIODIC = 'periodic-tasks' PRIORITY = 'priority-tasks' DATABASE = 'database-tasks' - SEND = 'send-tasks' + SEND_COMBINED = 'send-tasks' + SEND_SMS = 'send-sms-tasks' + SEND_EMAIL = 'send-email-tasks' RESEARCH_MODE = 'research-mode-tasks' STATISTICS = 'statistics-tasks' JOBS = 'job-tasks' @@ -36,7 +38,9 @@ class QueueNames(object): QueueNames.PRIORITY, QueueNames.PERIODIC, QueueNames.DATABASE, - QueueNames.SEND, + QueueNames.SEND_COMBINED, + QueueNames.SEND_SMS, + QueueNames.SEND_EMAIL, QueueNames.RESEARCH_MODE, QueueNames.STATISTICS, QueueNames.JOBS, diff --git a/app/delivery/rest.py b/app/delivery/rest.py index 489a5fcda..4dba91208 100644 --- a/app/delivery/rest.py +++ b/app/delivery/rest.py @@ -42,4 +42,4 @@ def send_response(send_call, task_call, notification): notification.id, notification.notification_type), e) - task_call.apply_async((str(notification.id)), queue=QueueNames.SEND) + task_call.apply_async((str(notification.id)), queue=QueueNames.SEND_COMBINED) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index ca3ae0b53..78881d2b6 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -101,7 +101,7 @@ def send_notification_to_queue(notification, research_mode, queue=None): if research_mode or notification.key_type == KEY_TYPE_TEST: queue = QueueNames.RESEARCH_MODE elif not queue: - queue = QueueNames.SEND + queue = QueueNames.SEND_COMBINED if notification.notification_type == SMS_TYPE: deliver_task = provider_tasks.deliver_sms diff --git a/manifest-delivery-base.yml b/manifest-delivery-base.yml index 7f73a4b54..597df9f3f 100644 --- a/manifest-delivery-base.yml +++ b/manifest-delivery-base.yml @@ -33,7 +33,7 @@ applications: NOTIFY_APP_NAME: delivery-worker-research - name: notify-delivery-worker-sender - command: scripts/run_app_paas.sh celery -A aws_run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q send-tasks + command: scripts/run_app_paas.sh celery -A aws_run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q send-tasks,send-sms-tasks,send-email-tasks env: NOTIFY_APP_NAME: delivery-worker-sender From e65619cb9020bed01ee00b49da22795c9ab8f00f Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 17 Jul 2017 09:40:05 +0100 Subject: [PATCH 13/22] Bump utils Brings in: - [ ] https://github.com/alphagov/notifications-utils/pull/182 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index b1d1cd307..875e27a94 100644 --- a/requirements.txt +++ b/requirements.txt @@ -29,6 +29,6 @@ notifications-python-client>=3.1,<3.2 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@17.5.3#egg=notifications-utils==17.5.3 +git+https://github.com/alphagov/notifications-utils.git@17.5.7#egg=notifications-utils==17.5.7 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 From d4bbca259232a0d1ab14532802778073cc026aa7 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 20 Jul 2017 17:56:51 +0100 Subject: [PATCH 14/22] Fix for simulated notifications. When a post is made for a simulated number the id is empty in the notificaiton object that we return. This fixes that. --- app/notifications/process_notifications.py | 4 +++- tests/app/v2/notifications/test_post_notifications.py | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index ca3ae0b53..950df8304 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -1,3 +1,4 @@ +import uuid from datetime import datetime from flask import current_app @@ -53,7 +54,8 @@ def persist_notification( created_by_id=None ): notification_created_at = created_at or datetime.utcnow() - + if not notification_id and simulated: + notification_id = uuid.uuid4() notification = Notification( id=notification_id, template_id=template_id, diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index aea003b7e..7433b9bcf 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -201,6 +201,7 @@ def test_should_not_persist_or_send_notification_if_simulated_recipient( assert response.status_code == 201 apply_async.assert_not_called() + assert json.loads(response.get_data(as_text=True))["id"] assert Notification.query.count() == 0 From 614880f6d9f0da758bd9e52dc484da8b77212124 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 20 Jul 2017 16:17:04 +0100 Subject: [PATCH 15/22] send to send-sms-tasks and send-email-tasks instead of send-tasks --- app/celery/tasks.py | 4 +- app/delivery/rest.py | 12 +- app/notifications/process_notifications.py | 6 +- tests/app/celery/test_scheduled_tasks.py | 2 +- tests/app/celery/test_tasks.py | 18 +-- tests/app/delivery/test_rest.py | 4 +- .../rest/test_send_notification.py | 109 ++++++++++-------- .../test_process_notification.py | 6 +- 8 files changed, 90 insertions(+), 71 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index c3f533a6d..008b0fd5e 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -182,7 +182,7 @@ def send_sms(self, provider_tasks.deliver_sms.apply_async( [str(saved_notification.id)], - queue=QueueNames.SEND_COMBINED if not service.research_mode else QueueNames.RESEARCH_MODE + queue=QueueNames.SEND_SMS if not service.research_mode else QueueNames.RESEARCH_MODE ) current_app.logger.info( @@ -227,7 +227,7 @@ def send_email(self, provider_tasks.deliver_email.apply_async( [str(saved_notification.id)], - queue=QueueNames.SEND_COMBINED if not service.research_mode else QueueNames.RESEARCH_MODE + queue=QueueNames.SEND_EMAIL if not service.research_mode else QueueNames.RESEARCH_MODE ) current_app.logger.info("Email {} created at {}".format(saved_notification.id, created_at)) diff --git a/app/delivery/rest.py b/app/delivery/rest.py index 4dba91208..83236d521 100644 --- a/app/delivery/rest.py +++ b/app/delivery/rest.py @@ -24,16 +24,20 @@ def send_notification_to_provider(notification_id): send_response( send_to_providers.send_email_to_provider, provider_tasks.deliver_email, - notification) + notification, + QueueNames.SEND_EMAIL + ) else: send_response( send_to_providers.send_sms_to_provider, provider_tasks.deliver_sms, - notification) + notification, + QueueNames.SEND_SMS + ) return jsonify({}), 204 -def send_response(send_call, task_call, notification): +def send_response(send_call, task_call, notification, queue): try: send_call(notification) except Exception as e: @@ -42,4 +46,4 @@ def send_response(send_call, task_call, notification): notification.id, notification.notification_type), e) - task_call.apply_async((str(notification.id)), queue=QueueNames.SEND_COMBINED) + task_call.apply_async((str(notification.id)), queue=queue) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 78881d2b6..76da026c4 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -100,12 +100,14 @@ def persist_notification( def send_notification_to_queue(notification, research_mode, queue=None): if research_mode or notification.key_type == KEY_TYPE_TEST: queue = QueueNames.RESEARCH_MODE - elif not queue: - queue = QueueNames.SEND_COMBINED if notification.notification_type == SMS_TYPE: + if not queue: + queue = QueueNames.SEND_SMS deliver_task = provider_tasks.deliver_sms if notification.notification_type == EMAIL_TYPE: + if not queue: + queue = QueueNames.SEND_EMAIL deliver_task = provider_tasks.deliver_email try: diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index b4ad7e255..0282f8074 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -468,7 +468,7 @@ def test_should_send_all_scheduled_notifications_to_deliver_queue(sample_templat send_scheduled_notifications() - mocked.apply_async.assert_called_once_with([str(message_to_deliver.id)], queue='send-tasks') + mocked.apply_async.assert_called_once_with([str(message_to_deliver.id)], queue='send-sms-tasks') scheduled_notifications = dao_get_scheduled_notifications() assert not scheduled_notifications diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 55b154aa9..c6b09e333 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -422,7 +422,7 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi assert persisted_notification.notification_type == 'sms' mocked_deliver_sms.assert_called_once_with( [str(persisted_notification.id)], - queue="send-tasks" + queue="send-sms-tasks" ) @@ -483,7 +483,7 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif assert persisted_notification.notification_type == 'sms' provider_tasks.deliver_sms.apply_async.assert_called_once_with( [str(persisted_notification.id)], - queue="send-tasks" + queue="send-sms-tasks" ) @@ -509,7 +509,7 @@ def test_should_send_sms_if_restricted_service_and_non_team_number_with_test_key persisted_notification = Notification.query.one() mocked_deliver_sms.assert_called_once_with( [str(persisted_notification.id)], - queue="send-tasks" + queue="send-sms-tasks" ) @@ -537,7 +537,7 @@ def test_should_send_email_if_restricted_service_and_non_team_email_address_with persisted_notification = Notification.query.one() mocked_deliver_email.assert_called_once_with( [str(persisted_notification.id)], - queue="send-tasks" + queue="send-email-tasks" ) @@ -641,7 +641,7 @@ def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_ provider_tasks.deliver_sms.apply_async.assert_called_once_with( [str(persisted_notification.id)], - queue="send-tasks" + queue="send-sms-tasks" ) @@ -738,7 +738,7 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh assert persisted_notification.notification_type == 'email' provider_tasks.deliver_email.apply_async.assert_called_once_with( - [str(persisted_notification.id)], queue='send-tasks') + [str(persisted_notification.id)], queue='send-email-tasks') def test_send_email_should_use_template_version_from_job_not_latest(sample_email_template, mocker): @@ -769,7 +769,7 @@ def test_send_email_should_use_template_version_from_job_not_latest(sample_email assert not persisted_notification.sent_by assert persisted_notification.notification_type == 'email' provider_tasks.deliver_email.apply_async.assert_called_once_with([str(persisted_notification.id)], - queue='send-tasks') + queue='send-email-tasks') def test_should_use_email_template_subject_placeholders(sample_email_template_with_placeholders, mocker): @@ -795,7 +795,7 @@ def test_should_use_email_template_subject_placeholders(sample_email_template_wi assert not persisted_notification.reference assert persisted_notification.notification_type == 'email' provider_tasks.deliver_email.apply_async.assert_called_once_with( - [str(persisted_notification.id)], queue='send-tasks' + [str(persisted_notification.id)], queue='send-email-tasks' ) @@ -823,7 +823,7 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em assert not persisted_notification.reference assert persisted_notification.notification_type == 'email' provider_tasks.deliver_email.apply_async.assert_called_once_with([str(persisted_notification.id)], - queue='send-tasks') + queue='send-email-tasks') def test_send_sms_should_go_to_retry_queue_if_database_errors(sample_template, mocker): diff --git a/tests/app/delivery/test_rest.py b/tests/app/delivery/test_rest.py index 984bf90e1..08a5d0a05 100644 --- a/tests/app/delivery/test_rest.py +++ b/tests/app/delivery/test_rest.py @@ -78,7 +78,7 @@ def test_should_call_deliver_sms_task_if_send_sms_to_provider_fails(notify_api, ) app.delivery.send_to_providers.send_sms_to_provider.assert_called_with(sample_notification) app.celery.provider_tasks.deliver_sms.apply_async.assert_called_with( - (str(sample_notification.id)), queue='send-tasks' + (str(sample_notification.id)), queue='send-sms-tasks' ) assert response.status_code == 204 @@ -100,6 +100,6 @@ def test_should_call_deliver_email_task_if_send_email_to_provider_fails( ) app.delivery.send_to_providers.send_email_to_provider.assert_called_with(sample_email_notification) app.celery.provider_tasks.deliver_email.apply_async.assert_called_with( - (str(sample_email_notification.id)), queue='send-tasks' + (str(sample_email_notification.id)), queue='send-email-tasks' ) assert response.status_code == 204 diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index aa7ab2d86..3822395cd 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -132,7 +132,7 @@ def test_send_notification_with_placeholders_replaced(notify_api, sample_email_t mocked.assert_called_once_with( [notification_id], - queue="send-tasks" + queue="send-email-tasks" ) assert response.status_code == 201 assert response_data['body'] == u'Hello Jo\nThis is an email from GOV.\u200BUK' @@ -342,7 +342,7 @@ def test_should_allow_valid_sms_notification(notify_api, sample_template, mocker response_data = json.loads(response.data)['data'] notification_id = response_data['notification']['id'] - mocked.assert_called_once_with([notification_id], queue='send-tasks') + mocked.assert_called_once_with([notification_id], queue='send-sms-tasks') assert response.status_code == 201 assert notification_id assert 'subject' not in response_data @@ -395,7 +395,7 @@ def test_should_allow_valid_email_notification(notify_api, sample_email_template notification_id = response_data['notification']['id'] app.celery.provider_tasks.deliver_email.apply_async.assert_called_once_with( [notification_id], - queue="send-tasks" + queue="send-email-tasks" ) assert response.status_code == 201 @@ -593,7 +593,10 @@ def test_should_send_email_if_team_api_key_and_a_service_user(notify_api, sample data=json.dumps(data), headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) - app.celery.provider_tasks.deliver_email.apply_async.assert_called_once_with([fake_uuid], queue='send-tasks') + app.celery.provider_tasks.deliver_email.apply_async.assert_called_once_with( + [fake_uuid], + queue='send-email-tasks' + ) assert response.status_code == 201 @@ -689,57 +692,67 @@ def test_should_send_sms_if_team_api_key_and_a_service_user(notify_api, sample_t data=json.dumps(data), headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) - app.celery.provider_tasks.deliver_sms.apply_async.assert_called_once_with([fake_uuid], queue='send-tasks') + app.celery.provider_tasks.deliver_sms.apply_async.assert_called_once_with([fake_uuid], queue='send-sms-tasks') assert response.status_code == 201 -@pytest.mark.parametrize('template_type', - [SMS_TYPE, EMAIL_TYPE]) -def test_should_persist_notification(notify_api, sample_template, - sample_email_template, - template_type, - fake_uuid, mocker): - with notify_api.test_request_context(), notify_api.test_client() as client: - mocked = mocker.patch('app.celery.provider_tasks.deliver_{}.apply_async'.format(template_type)) - mocker.patch('app.dao.notifications_dao.create_uuid', return_value=fake_uuid) - template = sample_template if template_type == SMS_TYPE else sample_email_template - to = sample_template.service.created_by.mobile_number if template_type == SMS_TYPE \ - else sample_email_template.service.created_by.email_address - data = { - 'to': to, - 'template': template.id - } - api_key = ApiKey( - service=template.service, - name='team_key', - created_by=template.created_by, - key_type=KEY_TYPE_TEAM) - save_model_api_key(api_key) - auth_header = create_jwt_token(secret=api_key.secret, client_id=str(api_key.service_id)) +@pytest.mark.parametrize('template_type,queue_name', [ + (SMS_TYPE, 'send-sms-tasks'), + (EMAIL_TYPE, 'send-email-tasks') +]) +def test_should_persist_notification( + client, + sample_template, + sample_email_template, + fake_uuid, + mocker, + template_type, + queue_name +): + mocked = mocker.patch('app.celery.provider_tasks.deliver_{}.apply_async'.format(template_type)) + mocker.patch('app.dao.notifications_dao.create_uuid', return_value=fake_uuid) + template = sample_template if template_type == SMS_TYPE else sample_email_template + to = sample_template.service.created_by.mobile_number if template_type == SMS_TYPE \ + else sample_email_template.service.created_by.email_address + data = { + 'to': to, + 'template': template.id + } + api_key = ApiKey( + service=template.service, + name='team_key', + created_by=template.created_by, + key_type=KEY_TYPE_TEAM) + save_model_api_key(api_key) + auth_header = create_jwt_token(secret=api_key.secret, client_id=str(api_key.service_id)) - response = client.post( - path='/notifications/{}'.format(template_type), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) + response = client.post( + path='/notifications/{}'.format(template_type), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) - mocked.assert_called_once_with([fake_uuid], queue='send-tasks') - assert response.status_code == 201 + mocked.assert_called_once_with([fake_uuid], queue=queue_name) + assert response.status_code == 201 - notification = notifications_dao.get_notification_by_id(fake_uuid) - assert notification.to == to - assert notification.template_id == template.id - assert notification.notification_type == template_type + notification = notifications_dao.get_notification_by_id(fake_uuid) + assert notification.to == to + assert notification.template_id == template.id + assert notification.notification_type == template_type -@pytest.mark.parametrize('template_type', - [SMS_TYPE, EMAIL_TYPE]) +@pytest.mark.parametrize('template_type,queue_name', [ + (SMS_TYPE, 'send-sms-tasks'), + (EMAIL_TYPE, 'send-email-tasks') +]) def test_should_delete_notification_and_return_error_if_sqs_fails( - client, - sample_email_template, - sample_template, - fake_uuid, - mocker, - template_type): + client, + sample_email_template, + sample_template, + fake_uuid, + mocker, + template_type, + queue_name +): mocked = mocker.patch( 'app.celery.provider_tasks.deliver_{}.apply_async'.format(template_type), side_effect=Exception("failed to talk to SQS") @@ -768,7 +781,7 @@ def test_should_delete_notification_and_return_error_if_sqs_fails( ) assert str(e.value) == 'failed to talk to SQS' - mocked.assert_called_once_with([fake_uuid], queue='send-tasks') + mocked.assert_called_once_with([fake_uuid], queue=queue_name) assert not notifications_dao.get_notification_by_id(fake_uuid) assert not NotificationHistory.query.get(fake_uuid) @@ -1119,7 +1132,7 @@ def test_should_allow_store_original_number_on_sms_notification(client, sample_t response_data = json.loads(response.data)['data'] notification_id = response_data['notification']['id'] - mocked.assert_called_once_with([notification_id], queue='send-tasks') + mocked.assert_called_once_with([notification_id], queue='send-sms-tasks') assert response.status_code == 201 assert notification_id notifications = Notification.query.all() diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index ac73b36da..3e4d1d9c2 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -214,9 +214,9 @@ def test_persist_notification_increments_cache_if_key_exists(sample_template, sa [(True, None, 'research-mode-tasks', 'sms', 'normal'), (True, None, 'research-mode-tasks', 'email', 'normal'), (True, None, 'research-mode-tasks', 'email', 'team'), - (False, None, 'send-tasks', 'sms', 'normal'), - (False, None, 'send-tasks', 'email', 'normal'), - (False, None, 'send-tasks', 'sms', 'team'), + (False, None, 'send-sms-tasks', 'sms', 'normal'), + (False, None, 'send-email-tasks', 'email', 'normal'), + (False, None, 'send-sms-tasks', 'sms', 'team'), (False, None, 'research-mode-tasks', 'sms', 'test'), (True, 'notify-internal-tasks', 'research-mode-tasks', 'email', 'normal'), (False, 'notify-internal-tasks', 'notify-internal-tasks', 'sms', 'normal'), From 6da3d3ed0b0447285e3aa8d3256ed80d0d0b6da1 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Fri, 21 Jul 2017 14:26:59 +0100 Subject: [PATCH 16/22] Remove wheels-ing on deployment --- .gitignore | 2 -- Makefile | 5 ++--- scripts/aws_install_dependencies.sh | 2 +- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/.gitignore b/.gitignore index 12f77b15c..ffd54699f 100644 --- a/.gitignore +++ b/.gitignore @@ -69,7 +69,5 @@ environment.sh celerybeat-schedule -wheelhouse/ - # CloudFoundry .cf diff --git a/Makefile b/Makefile index 4142722ff..d8c650d5f 100644 --- a/Makefile +++ b/Makefile @@ -80,8 +80,7 @@ generate-version-file: ## Generates the app version file .PHONY: build build: dependencies generate-version-file ## Build project - rm -rf wheelhouse - . venv/bin/activate && PIP_ACCEL_CACHE=${PIP_ACCEL_CACHE} pip-accel wheel --wheel-dir=wheelhouse -r requirements.txt + . venv/bin/activate && PIP_ACCEL_CACHE=${PIP_ACCEL_CACHE} pip-accel install -r requirements.txt .PHONY: cf-build cf-build: dependencies generate-version-file ## Build project for PAAS @@ -260,7 +259,7 @@ clean-docker-containers: ## Clean up any remaining docker containers .PHONY: clean clean: - rm -rf node_modules cache target venv .coverage build tests/.cache wheelhouse + rm -rf node_modules cache target venv .coverage build tests/.cache .PHONY: cf-login cf-login: ## Log in to Cloud Foundry diff --git a/scripts/aws_install_dependencies.sh b/scripts/aws_install_dependencies.sh index 03daadb21..b82881d45 100755 --- a/scripts/aws_install_dependencies.sh +++ b/scripts/aws_install_dependencies.sh @@ -5,4 +5,4 @@ set -eo pipefail echo "Install dependencies" cd /home/notify-app/notifications-api; -pip3 install --find-links=wheelhouse -r /home/notify-app/notifications-api/requirements.txt +pip3 install -r /home/notify-app/notifications-api/requirements.txt From 3e2b8190b9262734f0849e33ff778f69be034e6e Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 24 Jul 2017 15:13:18 +0100 Subject: [PATCH 17/22] - Added a scheduled task to create or update billing for the month, yesterday is used to calculate the start and end date for the month. - The new task has not been added to the beat application yet. - Added an updated_at column to the monthly billing table, we may want to only calculate from the last updated date rather than the entire month. --- app/celery/scheduled_tasks.py | 21 ++++++++++- app/dao/monthly_billing_dao.py | 17 +++++++-- app/dao/notification_usage_dao.py | 3 +- app/models.py | 1 + migrations/versions/0110_monthly_billing.py | 1 + tests/app/celery/test_scheduled_tasks.py | 39 ++++++++++++++++++--- tests/app/dao/test_monthly_billing.py | 23 ++++++++++-- 7 files changed, 93 insertions(+), 12 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index b14f7a5be..ceb813e25 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -9,9 +9,17 @@ from sqlalchemy.exc import SQLAlchemyError from app.aws import s3 from app import notify_celery from app import performance_platform_client +from app.dao.date_util import get_month_start_end_date from app.dao.inbound_sms_dao import delete_inbound_sms_created_more_than_a_week_ago from app.dao.invited_user_dao import delete_invitations_created_more_than_two_days_ago -from app.dao.jobs_dao import dao_set_scheduled_jobs_to_pending, dao_get_jobs_older_than_limited_by +from app.dao.jobs_dao import ( + dao_set_scheduled_jobs_to_pending, + dao_get_jobs_older_than_limited_by +) +from app.dao.monthly_billing_dao import ( + get_service_ids_that_need_sms_billing_populated, + create_or_update_monthly_billing_sms +) from app.dao.notifications_dao import ( dao_timeout_notifications, is_delivery_slow_for_provider, @@ -281,3 +289,14 @@ def delete_dvla_response_files_older_than_seven_days(): except SQLAlchemyError as e: current_app.logger.exception("Failed to delete dvla response files") raise + + +@notify_celery.task(name="populate_monthly_billing") +@statsd(namespace="tasks") +def populate_monthly_billing(): + # for every service with billable units this month update billing totals for yesterday + # this will overwrite the existing amount. + yesterday = datetime.utcnow() - timedelta(days=1) + start_date, end_date = get_month_start_end_date(yesterday) + services = get_service_ids_that_need_sms_billing_populated(start_date, end_date=end_date) + [create_or_update_monthly_billing_sms(service_id=s.service_id, billing_month=start_date) for s in services] diff --git a/app/dao/monthly_billing_dao.py b/app/dao/monthly_billing_dao.py index 8e3204584..4dede3f2d 100644 --- a/app/dao/monthly_billing_dao.py +++ b/app/dao/monthly_billing_dao.py @@ -1,12 +1,25 @@ from datetime import datetime from app import db +from app.dao.date_util import get_month_start_end_date from app.dao.notification_usage_dao import get_billing_data_for_month -from app.models import MonthlyBilling, SMS_TYPE +from app.models import MonthlyBilling, SMS_TYPE, NotificationHistory + + +def get_service_ids_that_need_sms_billing_populated(start_date, end_date): + return db.session.query( + NotificationHistory.service_id + ).filter( + NotificationHistory.created_at >= start_date, + NotificationHistory.created_at <= end_date, + NotificationHistory.notification_type == SMS_TYPE, + NotificationHistory.billable_units != 0 + ).distinct().all() def create_or_update_monthly_billing_sms(service_id, billing_month): - monthly = get_billing_data_for_month(service_id=service_id, billing_month=billing_month) + start_date, end_date = get_month_start_end_date(billing_month) + monthly = get_billing_data_for_month(service_id=service_id, start_date=start_date, end_date=end_date) # update monthly monthly_totals = _monthly_billing_data_to_json(monthly) row = MonthlyBilling.query.filter_by(year=billing_month.year, diff --git a/app/dao/notification_usage_dao.py b/app/dao/notification_usage_dao.py index 289a5a03d..13b8944c6 100644 --- a/app/dao/notification_usage_dao.py +++ b/app/dao/notification_usage_dao.py @@ -36,8 +36,7 @@ def get_yearly_billing_data(service_id, year): @statsd(namespace="dao") -def get_billing_data_for_month(service_id, billing_month): - start_date, end_date = get_month_start_end_date(billing_month) +def get_billing_data_for_month(service_id, start_date, end_date): rates = get_rates_for_year(start_date, end_date, SMS_TYPE) result = [] # so the start end date in the query are the valid from the rate, not the month - this is going to take some thought diff --git a/app/models.py b/app/models.py index e035acccb..b58eaf348 100644 --- a/app/models.py +++ b/app/models.py @@ -1257,6 +1257,7 @@ class MonthlyBilling(db.Model): year = db.Column(db.Float(asdecimal=False), nullable=False) notification_type = db.Column(notification_types, nullable=False) monthly_totals = db.Column(JSON, nullable=False) + updated_at = db.Column(db.DateTime, nullable=False, default=datetime.datetime.utcnow) __table_args__ = ( UniqueConstraint('service_id', 'month', 'year', 'notification_type', name='uix_monthly_billing'), diff --git a/migrations/versions/0110_monthly_billing.py b/migrations/versions/0110_monthly_billing.py index 19b1ce4fc..19fa1dbdd 100644 --- a/migrations/versions/0110_monthly_billing.py +++ b/migrations/versions/0110_monthly_billing.py @@ -26,6 +26,7 @@ def upgrade(): postgresql.ENUM('email', 'sms', 'letter', name='notification_type', create_type=False), nullable=False), sa.Column('monthly_totals', postgresql.JSON(), nullable=False), + sa.Column('updated_at', sa.DateTime, nullable=False), sa.ForeignKeyConstraint(['service_id'], ['services.id'], ), sa.PrimaryKeyConstraint('id') ) diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index b4ad7e255..0f54f9dde 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -25,8 +25,8 @@ from app.celery.scheduled_tasks import ( send_scheduled_notifications, switch_current_sms_provider_on_slow_delivery, timeout_job_statistics, - timeout_notifications -) + timeout_notifications, + populate_monthly_billing) from app.clients.performance_platform.performance_platform_client import PerformancePlatformClient from app.dao.jobs_dao import dao_get_job_by_id from app.dao.notifications_dao import dao_get_scheduled_notifications @@ -36,10 +36,10 @@ from app.dao.provider_details_dao import ( ) from app.models import ( Service, Template, - SMS_TYPE, LETTER_TYPE -) + SMS_TYPE, LETTER_TYPE, + MonthlyBilling) from app.utils import get_london_midnight_in_utc -from tests.app.db import create_notification, create_service, create_template, create_job +from tests.app.db import create_notification, create_service, create_template, create_job, create_rate from tests.app.conftest import ( sample_job as create_sample_job, sample_notification_history as create_notification_history, @@ -98,6 +98,8 @@ def test_should_have_decorated_tasks_functions(): 'remove_transformed_dvla_files' assert delete_dvla_response_files_older_than_seven_days.__wrapped__.__name__ == \ 'delete_dvla_response_files_older_than_seven_days' + assert populate_monthly_billing.__wrapped__.__name__ == \ + 'populate_monthly_billing' @pytest.fixture(scope='function') @@ -607,3 +609,30 @@ def test_delete_dvla_response_files_older_than_seven_days_does_not_remove_files( delete_dvla_response_files_older_than_seven_days() remove_s3_mock.assert_not_called() + + +@freeze_time("2017-07-12 02:00:00") +def test_populate_monthly_billing(sample_template): + yesterday = datetime(2017, 7, 11, 13, 30) + create_rate(datetime(2016, 1, 1), 0.0123, 'sms') + create_notification(template=sample_template, status='delivered', created_at=yesterday) + create_notification(template=sample_template, status='delivered', created_at=yesterday - timedelta(days=1)) + create_notification(template=sample_template, status='delivered', created_at=yesterday + timedelta(days=1)) + # not included in billing + create_notification(template=sample_template, status='delivered', created_at=yesterday - timedelta(days=30)) + + assert len(MonthlyBilling.query.all()) == 0 + populate_monthly_billing() + + monthly_billing = MonthlyBilling.query.all() + assert len(monthly_billing) == 1 + assert monthly_billing[0].service_id == sample_template.service_id + assert monthly_billing[0].year == 2017 + assert monthly_billing[0].month == 'July' + assert monthly_billing[0].notification_type == 'sms' + assert len(monthly_billing[0].monthly_totals) == 1 + assert sorted(monthly_billing[0].monthly_totals[0]) == sorted({'international': False, + 'rate_multiplier': 1, + 'billing_units': 3, + 'rate': 0.0123, + 'total_cost': 0.0369}) diff --git a/tests/app/dao/test_monthly_billing.py b/tests/app/dao/test_monthly_billing.py index 6a0399d67..2ed13aaa2 100644 --- a/tests/app/dao/test_monthly_billing.py +++ b/tests/app/dao/test_monthly_billing.py @@ -1,8 +1,12 @@ from datetime import datetime -from app.dao.monthly_billing_dao import create_or_update_monthly_billing_sms, get_monthly_billing_sms +from app.dao.monthly_billing_dao import ( + create_or_update_monthly_billing_sms, + get_monthly_billing_sms, + get_service_ids_that_need_sms_billing_populated +) from app.models import MonthlyBilling -from tests.app.db import create_notification, create_rate +from tests.app.db import create_notification, create_rate, create_service, create_template def test_add_monthly_billing(sample_template): @@ -105,3 +109,18 @@ def assert_monthly_billing(monthly_billing, year, month, service_id, expected_le assert monthly_billing.service_id == service_id assert len(monthly_billing.monthly_totals) == expected_len assert sorted(monthly_billing.monthly_totals[0]) == sorted(first_row) + + +def test_get_service_id(): + service_1 = create_service(service_name="Service One") + template_1 = create_template(service=service_1) + service_2 = create_service(service_name="Service Two") + template_2 = create_template(service=service_2) + create_notification(template=template_1, created_at=datetime(2017, 6, 30, 13, 30), status='delivered') + create_notification(template=template_1, created_at=datetime(2017, 7, 1, 14, 30), status='delivered') + create_notification(template=template_2, created_at=datetime(2017, 7, 15, 13, 30)) + create_notification(template=template_2, created_at=datetime(2017, 7, 31, 13, 30)) + services = get_service_ids_that_need_sms_billing_populated(start_date=datetime(2017, 7, 1), + end_date=datetime(2017, 7, 16)) + expected_services = [service_1.id, service_2.id] + assert sorted([x.service_id for x in services]) == sorted(expected_services) From cc32cff32a8e2da10cf6e2d9338d7133165cb637 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 25 Jul 2017 11:40:12 +0100 Subject: [PATCH 18/22] bump test requirements also ignore celery improvements --- requirements.txt | 2 +- requirements_for_test.txt | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/requirements.txt b/requirements.txt index 875e27a94..0c9b3f1e0 100644 --- a/requirements.txt +++ b/requirements.txt @@ -13,7 +13,7 @@ flask-marshmallow==0.6.2 Flask-Bcrypt==0.6.2 credstash==1.8.0 boto3==1.4.4 -celery==3.1.25 +celery==3.1.25 # pyup: <4 monotonic==1.2 statsd==3.2.1 jsonschema==2.5.1 diff --git a/requirements_for_test.txt b/requirements_for_test.txt index 62e3e4049..78d2d58ae 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -1,11 +1,11 @@ -r requirements.txt pycodestyle==2.3.1 -pytest==3.0.1 -pytest-mock==1.2 -pytest-cov==2.3.1 +pytest==3.1.3 +pytest-mock==1.6.2 +pytest-cov==2.5.1 coveralls==1.1 -moto==0.4.25 -flex==5.8.0 -freezegun==0.3.7 -requests-mock==1.0.0 +moto==1.0.1 +flex==6.11.0 +freezegun==0.3.9 +requests-mock==1.3.0 strict-rfc3339==0.7 From eaf5cbb86876c417e6dec4d6b76e24f94cbe71bd Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 25 Jul 2017 11:43:41 +0100 Subject: [PATCH 19/22] Add labels to query so that the named tuples can be referenced later. Remove unnecessary function --- app/dao/monthly_billing_dao.py | 10 +++++----- app/dao/notification_usage_dao.py | 8 ++++---- app/dao/provider_rates_dao.py | 5 ----- tests/app/dao/test_monthly_billing.py | 2 +- tests/app/dao/test_provider_rates_dao.py | 12 ++---------- tests/app/db.py | 5 +++-- 6 files changed, 15 insertions(+), 27 deletions(-) diff --git a/app/dao/monthly_billing_dao.py b/app/dao/monthly_billing_dao.py index 4dede3f2d..6cc499001 100644 --- a/app/dao/monthly_billing_dao.py +++ b/app/dao/monthly_billing_dao.py @@ -50,8 +50,8 @@ def _monthly_billing_data_to_json(monthly): # (month, billing_units, rate_multiplier, international, notification_type, rate) # total cost must take into account the free allowance. # might be a good idea to capture free allowance in this table - return [{"billing_units": x[1], - "rate_multiplier": x[2], - "international": x[3], - "rate": x[5], - "total_cost": (x[1] * x[2]) * x[5]} for x in monthly] + return [{"billing_units": x.billing_units, + "rate_multiplier": x.rate_multiplier, + "international": x.international, + "rate": x.rate, + "total_cost": (x.billing_units * x.rate_multiplier) * x.rate} for x in monthly] diff --git a/app/dao/notification_usage_dao.py b/app/dao/notification_usage_dao.py index 13b8944c6..43fb6a6cd 100644 --- a/app/dao/notification_usage_dao.py +++ b/app/dao/notification_usage_dao.py @@ -142,12 +142,12 @@ def is_between(date, start_date, end_date): def sms_billing_data_per_month_query(rate, service_id, start_date, end_date): month = get_london_month_from_utc_column(NotificationHistory.created_at) result = db.session.query( - month, - func.sum(NotificationHistory.billable_units), - rate_multiplier(), + month.label('month'), + func.sum(NotificationHistory.billable_units).label('billing_units'), + rate_multiplier().label('rate_multiplier'), NotificationHistory.international, NotificationHistory.notification_type, - cast(rate, Float()) + cast(rate, Float()).label('rate') ).filter( *billing_data_filter(SMS_TYPE, start_date, end_date, service_id) ).group_by( diff --git a/app/dao/provider_rates_dao.py b/app/dao/provider_rates_dao.py index 443543f86..145bd431e 100644 --- a/app/dao/provider_rates_dao.py +++ b/app/dao/provider_rates_dao.py @@ -9,8 +9,3 @@ def create_provider_rates(provider_identifier, valid_from, rate): provider_rates = ProviderRates(provider_id=provider.id, valid_from=valid_from, rate=rate) db.session.add(provider_rates) - - -@transactional -def create_sms_rate(rate): - db.session.add(rate) diff --git a/tests/app/dao/test_monthly_billing.py b/tests/app/dao/test_monthly_billing.py index 2ed13aaa2..9027ff870 100644 --- a/tests/app/dao/test_monthly_billing.py +++ b/tests/app/dao/test_monthly_billing.py @@ -111,7 +111,7 @@ def assert_monthly_billing(monthly_billing, year, month, service_id, expected_le assert sorted(monthly_billing.monthly_totals[0]) == sorted(first_row) -def test_get_service_id(): +def test_get_service_id(notify_db_session): service_1 = create_service(service_name="Service One") template_1 = create_template(service=service_1) service_2 = create_service(service_name="Service Two") diff --git a/tests/app/dao/test_provider_rates_dao.py b/tests/app/dao/test_provider_rates_dao.py index 7edb7de43..c78290a90 100644 --- a/tests/app/dao/test_provider_rates_dao.py +++ b/tests/app/dao/test_provider_rates_dao.py @@ -1,8 +1,8 @@ import uuid from datetime import datetime from decimal import Decimal -from app.dao.provider_rates_dao import create_provider_rates, create_sms_rate -from app.models import ProviderRates, ProviderDetails, Rate +from app.dao.provider_rates_dao import create_provider_rates +from app.models import ProviderRates, ProviderDetails def test_create_provider_rates(notify_db, notify_db_session, mmg_provider): @@ -16,11 +16,3 @@ def test_create_provider_rates(notify_db, notify_db_session, mmg_provider): assert ProviderRates.query.first().rate == rate assert ProviderRates.query.first().valid_from == now assert ProviderRates.query.first().provider_id == provider.id - - -def test_create_sms_rate(): - rate = Rate(id=uuid.uuid4(), valid_from=datetime.now(), rate=0.014, notification_type='sms') - create_sms_rate(rate) - rates = Rate.query.all() - assert len(rates) == 1 - assert rates[0] == rate diff --git a/tests/app/db.py b/tests/app/db.py index 669803d92..ace7b7c1c 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -1,8 +1,8 @@ from datetime import datetime import uuid +from app import db from app.dao.jobs_dao import dao_create_job -from app.dao.provider_rates_dao import create_sms_rate from app.dao.service_inbound_api_dao import save_service_inbound_api from app.models import ( Service, @@ -244,5 +244,6 @@ def create_organisation(colour='blue', logo='test_x2.png', name='test_org_1'): def create_rate(start_date, value, notification_type): rate = Rate(id=uuid.uuid4(), valid_from=start_date, rate=value, notification_type=notification_type) - create_sms_rate(rate) + db.session.add(rate) + db.session.commit() return rate From d2a1da9ea6cf134f51ef73280a0e09af9033e8f2 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 25 Jul 2017 11:44:39 +0100 Subject: [PATCH 20/22] Removed comment --- app/dao/monthly_billing_dao.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/dao/monthly_billing_dao.py b/app/dao/monthly_billing_dao.py index 6cc499001..cd51c19d3 100644 --- a/app/dao/monthly_billing_dao.py +++ b/app/dao/monthly_billing_dao.py @@ -46,8 +46,6 @@ def get_monthly_billing_sms(service_id, billing_month): def _monthly_billing_data_to_json(monthly): - # ('April', 6, 1, False, 'sms', 0.014) - # (month, billing_units, rate_multiplier, international, notification_type, rate) # total cost must take into account the free allowance. # might be a good idea to capture free allowance in this table return [{"billing_units": x.billing_units, From 5612ca023e094c8eecb9e42761d3c289ca5d85e7 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 25 Jul 2017 14:26:42 +0100 Subject: [PATCH 21/22] - Add transactional - Rename function for clarity --- app/dao/monthly_billing_dao.py | 5 ++++- app/dao/notification_usage_dao.py | 10 +++++----- tests/app/dao/test_notification_usage_dao.py | 16 ++++++++-------- 3 files changed, 17 insertions(+), 14 deletions(-) diff --git a/app/dao/monthly_billing_dao.py b/app/dao/monthly_billing_dao.py index cd51c19d3..49a571e5c 100644 --- a/app/dao/monthly_billing_dao.py +++ b/app/dao/monthly_billing_dao.py @@ -1,9 +1,11 @@ from datetime import datetime from app import db +from app.dao.dao_utils import transactional from app.dao.date_util import get_month_start_end_date from app.dao.notification_usage_dao import get_billing_data_for_month from app.models import MonthlyBilling, SMS_TYPE, NotificationHistory +from app.statsd_decorators import statsd def get_service_ids_that_need_sms_billing_populated(start_date, end_date): @@ -17,6 +19,7 @@ def get_service_ids_that_need_sms_billing_populated(start_date, end_date): ).distinct().all() +@transactional def create_or_update_monthly_billing_sms(service_id, billing_month): start_date, end_date = get_month_start_end_date(billing_month) monthly = get_billing_data_for_month(service_id=service_id, start_date=start_date, end_date=end_date) @@ -34,9 +37,9 @@ def create_or_update_monthly_billing_sms(service_id, billing_month): month=datetime.strftime(billing_month, "%B"), monthly_totals=monthly_totals) db.session.add(row) - db.session.commit() +@statsd(namespace="dao") def get_monthly_billing_sms(service_id, billing_month): monthly = MonthlyBilling.query.filter_by(service_id=service_id, year=billing_month.year, diff --git a/app/dao/notification_usage_dao.py b/app/dao/notification_usage_dao.py index 43fb6a6cd..7cd6517e9 100644 --- a/app/dao/notification_usage_dao.py +++ b/app/dao/notification_usage_dao.py @@ -20,7 +20,7 @@ from app.utils import get_london_month_from_utc_column @statsd(namespace="dao") def get_yearly_billing_data(service_id, year): start_date, end_date = get_financial_year(year) - rates = get_rates_for_year(start_date, end_date, SMS_TYPE) + rates = get_rates_for_daterange(start_date, end_date, SMS_TYPE) def get_valid_from(valid_from): return start_date if valid_from < start_date else valid_from @@ -37,7 +37,7 @@ def get_yearly_billing_data(service_id, year): @statsd(namespace="dao") def get_billing_data_for_month(service_id, start_date, end_date): - rates = get_rates_for_year(start_date, end_date, SMS_TYPE) + rates = get_rates_for_daterange(start_date, end_date, SMS_TYPE) result = [] # so the start end date in the query are the valid from the rate, not the month - this is going to take some thought for r, n in zip(rates, rates[1:]): @@ -52,7 +52,7 @@ def get_billing_data_for_month(service_id, start_date, end_date): @statsd(namespace="dao") def get_monthly_billing_data(service_id, year): start_date, end_date = get_financial_year(year) - rates = get_rates_for_year(start_date, end_date, SMS_TYPE) + rates = get_rates_for_daterange(start_date, end_date, SMS_TYPE) result = [] for r, n in zip(rates, rates[1:]): @@ -117,7 +117,7 @@ def sms_yearly_billing_data_query(rate, service_id, start_date, end_date): return result -def get_rates_for_year(start_date, end_date, notification_type): +def get_rates_for_daterange(start_date, end_date, notification_type): rates = Rate.query.filter(Rate.notification_type == notification_type).order_by(Rate.valid_from).all() results = [] for current_rate, current_rate_expiry_date in zip(rates, rates[1:]): @@ -207,7 +207,7 @@ def get_total_billable_units_for_sent_sms_notifications_in_date_range(start_date def discover_rate_bounds_for_billing_query(start_date, end_date): bounds = [] - rates = get_rates_for_year(start_date, end_date, SMS_TYPE) + rates = get_rates_for_daterange(start_date, end_date, SMS_TYPE) def current_valid_from(index): return rates[index].valid_from diff --git a/tests/app/dao/test_notification_usage_dao.py b/tests/app/dao/test_notification_usage_dao.py index 83bfa264c..1555bad77 100644 --- a/tests/app/dao/test_notification_usage_dao.py +++ b/tests/app/dao/test_notification_usage_dao.py @@ -6,7 +6,7 @@ from flask import current_app from app.dao.date_util import get_financial_year from app.dao.notification_usage_dao import ( - get_rates_for_year, + get_rates_for_daterange, get_yearly_billing_data, get_monthly_billing_data, get_total_billable_units_for_sent_sms_notifications_in_date_range, @@ -28,7 +28,7 @@ def test_get_rates_for_year(notify_db, notify_db_session): set_up_rate(notify_db, datetime(2016, 5, 18), 0.016) set_up_rate(notify_db, datetime(2017, 3, 31, 23), 0.0158) start_date, end_date = get_financial_year(2017) - rates = get_rates_for_year(start_date, end_date, 'sms') + rates = get_rates_for_daterange(start_date, end_date, 'sms') assert len(rates) == 1 assert datetime.strftime(rates[0].valid_from, '%Y-%m-%d %H:%M:%S') == "2017-03-31 23:00:00" assert rates[0].rate == 0.0158 @@ -39,7 +39,7 @@ def test_get_rates_for_year_multiple_result_per_year(notify_db, notify_db_sessio set_up_rate(notify_db, datetime(2016, 5, 18), 0.016) set_up_rate(notify_db, datetime(2017, 4, 1), 0.0158) start_date, end_date = get_financial_year(2016) - rates = get_rates_for_year(start_date, end_date, 'sms') + rates = get_rates_for_daterange(start_date, end_date, 'sms') assert len(rates) == 2 assert datetime.strftime(rates[0].valid_from, '%Y-%m-%d %H:%M:%S') == "2016-04-01 00:00:00" assert rates[0].rate == 0.015 @@ -52,7 +52,7 @@ def test_get_rates_for_year_returns_correct_rates(notify_db, notify_db_session): set_up_rate(notify_db, datetime(2016, 9, 1), 0.016) set_up_rate(notify_db, datetime(2017, 6, 1), 0.0175) start_date, end_date = get_financial_year(2017) - rates_2017 = get_rates_for_year(start_date, end_date, 'sms') + rates_2017 = get_rates_for_daterange(start_date, end_date, 'sms') assert len(rates_2017) == 2 assert datetime.strftime(rates_2017[0].valid_from, '%Y-%m-%d %H:%M:%S') == "2016-09-01 00:00:00" assert rates_2017[0].rate == 0.016 @@ -64,7 +64,7 @@ def test_get_rates_for_year_in_the_future(notify_db, notify_db_session): set_up_rate(notify_db, datetime(2016, 4, 1), 0.015) set_up_rate(notify_db, datetime(2017, 6, 1), 0.0175) start_date, end_date = get_financial_year(2018) - rates = get_rates_for_year(start_date, end_date, 'sms') + rates = get_rates_for_daterange(start_date, end_date, 'sms') assert datetime.strftime(rates[0].valid_from, '%Y-%m-%d %H:%M:%S') == "2017-06-01 00:00:00" assert rates[0].rate == 0.0175 @@ -73,7 +73,7 @@ def test_get_rates_for_year_returns_empty_list_if_year_is_before_earliest_rate(n set_up_rate(notify_db, datetime(2016, 4, 1), 0.015) set_up_rate(notify_db, datetime(2017, 6, 1), 0.0175) start_date, end_date = get_financial_year(2015) - rates = get_rates_for_year(start_date, end_date, 'sms') + rates = get_rates_for_daterange(start_date, end_date, 'sms') assert rates == [] @@ -83,7 +83,7 @@ def test_get_rates_for_year_early_rate(notify_db, notify_db_session): set_up_rate(notify_db, datetime(2016, 9, 1), 0.016) set_up_rate(notify_db, datetime(2017, 6, 1), 0.0175) start_date, end_date = get_financial_year(2016) - rates = get_rates_for_year(start_date, end_date, 'sms') + rates = get_rates_for_daterange(start_date, end_date, 'sms') assert len(rates) == 3 @@ -91,7 +91,7 @@ def test_get_rates_for_year_edge_case(notify_db, notify_db_session): set_up_rate(notify_db, datetime(2016, 3, 31, 23, 00), 0.015) set_up_rate(notify_db, datetime(2017, 3, 31, 23, 00), 0.0175) start_date, end_date = get_financial_year(2016) - rates = get_rates_for_year(start_date, end_date, 'sms') + rates = get_rates_for_daterange(start_date, end_date, 'sms') assert len(rates) == 1 assert datetime.strftime(rates[0].valid_from, '%Y-%m-%d %H:%M:%S') == "2016-03-31 23:00:00" assert rates[0].rate == 0.015 From e23d38de26132240e36516f7b5d66fe78679b608 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 25 Jul 2017 15:50:14 +0100 Subject: [PATCH 22/22] Fix bug in get rates function. --- app/dao/notification_usage_dao.py | 6 ++-- tests/app/dao/test_monthly_billing.py | 1 + tests/app/dao/test_notification_usage_dao.py | 30 ++++++++++++++------ 3 files changed, 26 insertions(+), 11 deletions(-) diff --git a/app/dao/notification_usage_dao.py b/app/dao/notification_usage_dao.py index 7cd6517e9..79f786dd0 100644 --- a/app/dao/notification_usage_dao.py +++ b/app/dao/notification_usage_dao.py @@ -129,8 +129,10 @@ def get_rates_for_daterange(start_date, end_date, notification_type): results.append(rates[-1]) if not results: - if start_date >= rates[-1].valid_from: - results.append(rates[-1]) + for x in reversed(rates): + if start_date >= x.valid_from: + results.append(x) + break return results diff --git a/tests/app/dao/test_monthly_billing.py b/tests/app/dao/test_monthly_billing.py index 9027ff870..538f19c51 100644 --- a/tests/app/dao/test_monthly_billing.py +++ b/tests/app/dao/test_monthly_billing.py @@ -13,6 +13,7 @@ def test_add_monthly_billing(sample_template): jan = datetime(2017, 1, 1) feb = datetime(2017, 2, 15) create_rate(start_date=jan, value=0.0158, notification_type='sms') + create_rate(start_date=datetime(2017, 3, 31, 23, 00, 00), value=0.123, notification_type='sms') create_notification(template=sample_template, created_at=jan, billable_units=1, status='delivered') create_notification(template=sample_template, created_at=feb, billable_units=2, status='delivered') diff --git a/tests/app/dao/test_notification_usage_dao.py b/tests/app/dao/test_notification_usage_dao.py index 1555bad77..9eab4f089 100644 --- a/tests/app/dao/test_notification_usage_dao.py +++ b/tests/app/dao/test_notification_usage_dao.py @@ -24,7 +24,7 @@ from freezegun import freeze_time from tests.conftest import set_config -def test_get_rates_for_year(notify_db, notify_db_session): +def test_get_rates_for_daterange(notify_db, notify_db_session): set_up_rate(notify_db, datetime(2016, 5, 18), 0.016) set_up_rate(notify_db, datetime(2017, 3, 31, 23), 0.0158) start_date, end_date = get_financial_year(2017) @@ -34,7 +34,7 @@ def test_get_rates_for_year(notify_db, notify_db_session): assert rates[0].rate == 0.0158 -def test_get_rates_for_year_multiple_result_per_year(notify_db, notify_db_session): +def test_get_rates_for_daterange_multiple_result_per_year(notify_db, notify_db_session): set_up_rate(notify_db, datetime(2016, 4, 1), 0.015) set_up_rate(notify_db, datetime(2016, 5, 18), 0.016) set_up_rate(notify_db, datetime(2017, 4, 1), 0.0158) @@ -47,7 +47,7 @@ def test_get_rates_for_year_multiple_result_per_year(notify_db, notify_db_sessio assert rates[1].rate == 0.016 -def test_get_rates_for_year_returns_correct_rates(notify_db, notify_db_session): +def test_get_rates_for_daterange_returns_correct_rates(notify_db, notify_db_session): set_up_rate(notify_db, datetime(2016, 4, 1), 0.015) set_up_rate(notify_db, datetime(2016, 9, 1), 0.016) set_up_rate(notify_db, datetime(2017, 6, 1), 0.0175) @@ -60,7 +60,7 @@ def test_get_rates_for_year_returns_correct_rates(notify_db, notify_db_session): assert rates_2017[1].rate == 0.0175 -def test_get_rates_for_year_in_the_future(notify_db, notify_db_session): +def test_get_rates_for_daterange_in_the_future(notify_db, notify_db_session): set_up_rate(notify_db, datetime(2016, 4, 1), 0.015) set_up_rate(notify_db, datetime(2017, 6, 1), 0.0175) start_date, end_date = get_financial_year(2018) @@ -69,7 +69,7 @@ def test_get_rates_for_year_in_the_future(notify_db, notify_db_session): assert rates[0].rate == 0.0175 -def test_get_rates_for_year_returns_empty_list_if_year_is_before_earliest_rate(notify_db, notify_db_session): +def test_get_rates_for_daterange_returns_empty_list_if_year_is_before_earliest_rate(notify_db, notify_db_session): set_up_rate(notify_db, datetime(2016, 4, 1), 0.015) set_up_rate(notify_db, datetime(2017, 6, 1), 0.0175) start_date, end_date = get_financial_year(2015) @@ -77,7 +77,7 @@ def test_get_rates_for_year_returns_empty_list_if_year_is_before_earliest_rate(n assert rates == [] -def test_get_rates_for_year_early_rate(notify_db, notify_db_session): +def test_get_rates_for_daterange_early_rate(notify_db, notify_db_session): set_up_rate(notify_db, datetime(2015, 6, 1), 0.014) set_up_rate(notify_db, datetime(2016, 6, 1), 0.015) set_up_rate(notify_db, datetime(2016, 9, 1), 0.016) @@ -87,7 +87,7 @@ def test_get_rates_for_year_early_rate(notify_db, notify_db_session): assert len(rates) == 3 -def test_get_rates_for_year_edge_case(notify_db, notify_db_session): +def test_get_rates_for_daterange_edge_case(notify_db, notify_db_session): set_up_rate(notify_db, datetime(2016, 3, 31, 23, 00), 0.015) set_up_rate(notify_db, datetime(2017, 3, 31, 23, 00), 0.0175) start_date, end_date = get_financial_year(2016) @@ -97,6 +97,19 @@ def test_get_rates_for_year_edge_case(notify_db, notify_db_session): assert rates[0].rate == 0.015 +def test_get_rates_for_daterange_where_daterange_is_one_month_that_falls_between_rate_valid_from( + notify_db, notify_db_session +): + set_up_rate(notify_db, datetime(2017, 1, 1), 0.175) + set_up_rate(notify_db, datetime(2017, 3, 31), 0.123) + start_date = datetime(2017, 2, 1, 00, 00, 00) + end_date = datetime(2017, 2, 28, 23, 59, 59, 99999) + rates = get_rates_for_daterange(start_date, end_date, 'sms') + assert len(rates) == 1 + assert datetime.strftime(rates[0].valid_from, '%Y-%m-%d %H:%M:%S') == "2017-01-01 00:00:00" + assert rates[0].rate == 0.175 + + def test_get_yearly_billing_data(notify_db, notify_db_session, sample_template, sample_email_template): set_up_rate(notify_db, datetime(2016, 4, 1), 0.014) set_up_rate(notify_db, datetime(2016, 6, 1), 0.0158) @@ -254,8 +267,7 @@ def test_get_monthly_billing_data_with_multiple_rates(notify_db, notify_db_sessi assert results[3] == ('June', 4, 1, False, 'sms', 0.0175) -def test_get_monthly_billing_data_with_no_notifications_for_year(notify_db, notify_db_session, sample_template, - sample_email_template): +def test_get_monthly_billing_data_with_no_notifications_for_daterange(notify_db, notify_db_session, sample_template): set_up_rate(notify_db, datetime(2016, 4, 1), 0.014) results = get_monthly_billing_data(sample_template.service_id, 2016) assert len(results) == 0