From 1617f058e2dd623925c9325f1d7732a07de76e1e Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 29 Jul 2016 16:39:51 +0100 Subject: [PATCH 1/7] rework get_fragment_count to not use ProviderStatistics use NotficationHistory instead. Unfortunately this means the SQL gets a bit gnarly, as we have to repeat notifications_utils' `get_sms_fragment_count` functionality inside a SELECT :scream: --- app/dao/provider_statistics_dao.py | 77 +++++++------ app/models.py | 29 ++++- app/service/rest.py | 8 +- ...ics.py => test_provider_statistics_dao.py} | 104 ++++++++---------- 4 files changed, 118 insertions(+), 100 deletions(-) rename tests/app/dao/{test_notifications_dao_provider_statistics.py => test_provider_statistics_dao.py} (64%) diff --git a/app/dao/provider_statistics_dao.py b/app/dao/provider_statistics_dao.py index f84625429..da942854f 100644 --- a/app/dao/provider_statistics_dao.py +++ b/app/dao/provider_statistics_dao.py @@ -1,42 +1,51 @@ -from sqlalchemy import func -from app.models import (ProviderStatistics, SMS_PROVIDERS, EMAIL_PROVIDERS, ProviderDetails) +from sqlalchemy import func, cast, Float, case + +from app import db +from app.models import ( + ProviderStatistics, + ProviderDetails, + NotificationHistory, + SMS_TYPE, + EMAIL_TYPE, + NOTIFICATION_STATUS_TYPES_BILLABLE + ) def get_provider_statistics(service, **kwargs): - return filter_query(ProviderStatistics.query, service, **kwargs) - - -def get_fragment_count(service, date_from, date_to): - sms_query = filter_query( - ProviderStatistics.query, - service, - providers=SMS_PROVIDERS, - date_from=date_from, - date_to=date_to - ) - email_query = filter_query( - ProviderStatistics.query, - service, - providers=EMAIL_PROVIDERS, - date_from=date_from, - date_to=date_to - ) - return { - 'sms_count': int(sms_query.with_entities( - func.sum(ProviderStatistics.unit_count)).scalar()) if sms_query.count() > 0 else 0, - 'email_count': int(email_query.with_entities( - func.sum(ProviderStatistics.unit_count)).scalar()) if email_query.count() > 0 else 0 - } - - -def filter_query(query, service, **kwargs): - query = query.filter_by(service=service) + query = ProviderStatistics.query.filter_by(service=service) if 'providers' in kwargs: providers = ProviderDetails.query.filter(ProviderDetails.identifier.in_(kwargs['providers'])).all() provider_ids = [provider.id for provider in providers] query = query.filter(ProviderStatistics.provider_id.in_(provider_ids)) - if 'date_from' in kwargs: - query.filter(ProviderStatistics.day >= kwargs['date_from']) - if 'date_to' in kwargs: - query.filter(ProviderStatistics.day <= kwargs['date_to']) return query + + +def get_fragment_count(service_id): + sms_count = db.session.query( + func.sum( + case( + [ + ( + NotificationHistory.content_char_count <= 160, + func.ceil(cast(NotificationHistory.content_char_count, Float) / 153) + ) + ], + else_=1 + ) + ) + ).filter( + NotificationHistory.service_id == service_id, + NotificationHistory.notification_type == SMS_TYPE, + NotificationHistory.status.in_(NOTIFICATION_STATUS_TYPES_BILLABLE) + ) + email_count = db.session.query( + func.count(NotificationHistory.id) + ).filter( + NotificationHistory.service_id == service_id, + NotificationHistory.notification_type == EMAIL_TYPE, + NotificationHistory.status.in_(NOTIFICATION_STATUS_TYPES_BILLABLE) + ) + return { + 'sms_count': int(sms_count.scalar() or 0), + 'email_count': email_count.scalar() or 0 + } diff --git a/app/models.py b/app/models.py index 139a29823..9fe39b0f3 100644 --- a/app/models.py +++ b/app/models.py @@ -329,9 +329,34 @@ class VerifyCode(db.Model): def check_code(self, cde): return check_hash(cde, self._code) +NOTIFICATION_CREATED = 'created' +NOTIFICATION_SENDING = 'sending' +NOTIFICATION_DELIVERED = 'delivered' +NOTIFICATION_PENDING = 'pending' +NOTIFICATION_FAILED = 'failed' +NOTIFICATION_TECHNICAL_FAILURE = 'technical-failure' +NOTIFICATION_TEMPORARY_FAILURE = 'temporary-failure' +NOTIFICATION_PERMANENT_FAILURE = 'permanent-failure' -NOTIFICATION_STATUS_TYPES = ['created', 'sending', 'delivered', 'pending', 'failed', - 'technical-failure', 'temporary-failure', 'permanent-failure'] +NOTIFICATION_STATUS_TYPES_BILLABLE = [ + NOTIFICATION_SENDING, + NOTIFICATION_DELIVERED, + NOTIFICATION_FAILED, + NOTIFICATION_TECHNICAL_FAILURE, + NOTIFICATION_TEMPORARY_FAILURE, + NOTIFICATION_PERMANENT_FAILURE +] + +NOTIFICATION_STATUS_TYPES = [ + NOTIFICATION_CREATED, + NOTIFICATION_SENDING, + NOTIFICATION_DELIVERED, + NOTIFICATION_PENDING, + NOTIFICATION_FAILED, + NOTIFICATION_TECHNICAL_FAILURE, + NOTIFICATION_TEMPORARY_FAILURE, + NOTIFICATION_PERMANENT_FAILURE +] NOTIFICATION_STATUS_TYPES_ENUM = db.Enum(*NOTIFICATION_STATUS_TYPES, name='notify_status_type') diff --git a/app/service/rest.py b/app/service/rest.py index f81a27f50..bf84fb859 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -168,13 +168,7 @@ def remove_user_from_service(service_id, user_id): @service.route('//fragment/aggregate_statistics') def get_service_provider_aggregate_statistics(service_id): - service = dao_fetch_service_by_id(service_id) - data = from_to_date_schema.load(request.args).data - return jsonify(data=get_fragment_count( - service, - date_from=(data.pop('date_from') if 'date_from' in data else date.today()), - date_to=(data.pop('date_to') if 'date_to' in data else date.today()) - )) + return jsonify(data=get_fragment_count(service_id)) # This is placeholder get method until more thought diff --git a/tests/app/dao/test_notifications_dao_provider_statistics.py b/tests/app/dao/test_provider_statistics_dao.py similarity index 64% rename from tests/app/dao/test_notifications_dao_provider_statistics.py rename to tests/app/dao/test_provider_statistics_dao.py index 262a008f4..b0c5b4a12 100644 --- a/tests/app/dao/test_notifications_dao_provider_statistics.py +++ b/tests/app/dao/test_provider_statistics_dao.py @@ -1,5 +1,7 @@ -from datetime import (date, timedelta) -from app.models import ProviderStatistics +from datetime import datetime +import uuid + +from app.models import NotificationHistory, KEY_TYPE_NORMAL, NOTIFICATION_STATUS_TYPES from app.dao.notifications_dao import update_provider_stats from app.dao.provider_statistics_dao import ( get_provider_statistics, get_fragment_count) @@ -89,63 +91,51 @@ def test_should_update_provider_statistics_email_multi(notify_db, assert provider_stats.unit_count == 3 -def test_should_aggregate_fragment_count(notify_db, - notify_db_session, - sample_service, - mmg_provider, - firetext_provider, - ses_provider): - day = date.today() - stats_mmg = ProviderStatistics( - service=sample_service, - day=day, - provider_id=mmg_provider.id, - unit_count=2 - ) +def test_get_fragment_count_with_no_data(sample_template): + assert get_fragment_count(sample_template.service_id)['sms_count'] == 0 + assert get_fragment_count(sample_template.service_id)['email_count'] == 0 - stats_firetext = ProviderStatistics( - service=sample_service, - day=day, - provider_id=firetext_provider.id, - unit_count=3 - ) - stats_ses = ProviderStatistics( - service=sample_service, - day=day, - provider_id=ses_provider.id, - unit_count=1 +def test_get_fragment_count_separates_sms_and_email(notify_db, sample_template, sample_email_template): + noti_hist(notify_db, sample_template) + noti_hist(notify_db, sample_template) + noti_hist(notify_db, sample_email_template) + assert get_fragment_count(sample_template.service_id) == { + 'sms_count': 2, + 'email_count': 1 + } + + +def test_get_fragment_count_filters_on_status(notify_db, sample_template): + for status in NOTIFICATION_STATUS_TYPES: + noti_hist(notify_db, sample_template, status=status) + # sending, delivered, failed, technical-failure, temporary-failure, permanent-failure + assert get_fragment_count(sample_template.service_id)['sms_count'] == 6 + + +def test_get_fragment_count_sums_char_count_for_sms(notify_db, sample_template): + noti_hist(notify_db, sample_template, content_char_count=1) # 1 + noti_hist(notify_db, sample_template, content_char_count=159) # 1 + noti_hist(notify_db, sample_template, content_char_count=310) # 2 + assert get_fragment_count(sample_template.service_id)['sms_count'] == 4 + + +def noti_hist(notify_db, template, status='delivered', content_char_count=None): + if not content_char_count and template.template_type == 'sms': + content_char_count = 1 + + notification_history = NotificationHistory( + id=uuid.uuid4(), + service=template.service, + template=template, + template_version=template.version, + status=status, + created_at=datetime.utcnow(), + content_char_count=content_char_count, + notification_type=template.template_type, + key_type=KEY_TYPE_NORMAL ) - notify_db.session.add(stats_mmg) - notify_db.session.add(stats_firetext) - notify_db.session.add(stats_ses) + notify_db.session.add(notification_history) notify_db.session.commit() - results = get_fragment_count(sample_service, day, day) - assert results['sms_count'] == 5 - assert results['email_count'] == 1 - -def test_should_aggregate_fragment_count_over_days(notify_db, - notify_db_session, - sample_service, - mmg_provider): - today = date.today() - yesterday = today - timedelta(days=1) - stats_today = ProviderStatistics( - service=sample_service, - day=today, - provider_id=mmg_provider.id, - unit_count=2 - ) - stats_yesterday = ProviderStatistics( - service=sample_service, - day=yesterday, - provider_id=mmg_provider.id, - unit_count=3 - ) - notify_db.session.add(stats_today) - notify_db.session.add(stats_yesterday) - notify_db.session.commit() - results = get_fragment_count(sample_service, yesterday, today) - assert results['sms_count'] == 5 - assert results['email_count'] == 0 + return notification_history From 9a9ebf088600622d33d78f2ca4fe805127c64891 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 1 Aug 2016 10:22:22 +0100 Subject: [PATCH 2/7] filter on key types to avoid research mode (that dont actually send) --- app/dao/provider_statistics_dao.py | 17 ++++++++++++----- tests/app/dao/test_provider_statistics_dao.py | 18 +++++++++++++++++- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/app/dao/provider_statistics_dao.py b/app/dao/provider_statistics_dao.py index da942854f..4fffb7dd6 100644 --- a/app/dao/provider_statistics_dao.py +++ b/app/dao/provider_statistics_dao.py @@ -7,7 +7,8 @@ from app.models import ( NotificationHistory, SMS_TYPE, EMAIL_TYPE, - NOTIFICATION_STATUS_TYPES_BILLABLE + NOTIFICATION_STATUS_TYPES_BILLABLE, + KEY_TYPE_TEST ) @@ -21,6 +22,13 @@ def get_provider_statistics(service, **kwargs): def get_fragment_count(service_id): + live_dates = get_service_live_dates(service_id) + shared_filters = [ + NotificationHistory.service_id == service_id, + NotificationHistory.status.in_(NOTIFICATION_STATUS_TYPES_BILLABLE), + NotificationHistory.key_type != KEY_TYPE_TEST + ] + sms_count = db.session.query( func.sum( case( @@ -34,16 +42,15 @@ def get_fragment_count(service_id): ) ) ).filter( - NotificationHistory.service_id == service_id, NotificationHistory.notification_type == SMS_TYPE, - NotificationHistory.status.in_(NOTIFICATION_STATUS_TYPES_BILLABLE) + *shared_filters ) + email_count = db.session.query( func.count(NotificationHistory.id) ).filter( - NotificationHistory.service_id == service_id, NotificationHistory.notification_type == EMAIL_TYPE, - NotificationHistory.status.in_(NOTIFICATION_STATUS_TYPES_BILLABLE) + *shared_filters ) return { 'sms_count': int(sms_count.scalar() or 0), diff --git a/tests/app/dao/test_provider_statistics_dao.py b/tests/app/dao/test_provider_statistics_dao.py index b0c5b4a12..f5f349b7c 100644 --- a/tests/app/dao/test_provider_statistics_dao.py +++ b/tests/app/dao/test_provider_statistics_dao.py @@ -113,6 +113,12 @@ def test_get_fragment_count_filters_on_status(notify_db, sample_template): assert get_fragment_count(sample_template.service_id)['sms_count'] == 6 +def test_get_fragment_count_filters_on_service_id(notify_db, sample_template, service_factory): + service_2 = service_factory.get('service 2', email_from='service.2') + noti_hist(notify_db, sample_template) + assert get_fragment_count(service_2.id)['sms_count'] == 0 + + def test_get_fragment_count_sums_char_count_for_sms(notify_db, sample_template): noti_hist(notify_db, sample_template, content_char_count=1) # 1 noti_hist(notify_db, sample_template, content_char_count=159) # 1 @@ -120,7 +126,17 @@ def test_get_fragment_count_sums_char_count_for_sms(notify_db, sample_template): assert get_fragment_count(sample_template.service_id)['sms_count'] == 4 -def noti_hist(notify_db, template, status='delivered', content_char_count=None): +@pytest.mark.parametrize('key_type,sms_count', [ + (KEY_TYPE_NORMAL, 1), + (KEY_TYPE_TEAM, 1), + (KEY_TYPE_TEST, 0), +]) +def test_get_fragment_count_ignores_test_api_keys(notify_db, sample_template, key_type, sms_count): + noti_hist(notify_db, sample_template, key_type=key_type) + assert get_fragment_count(sample_template.service_id)['sms_count'] == sms_count + + +def noti_hist(notify_db, template, status='delivered', content_char_count=None, key_type=KEY_TYPE_NORMAL): if not content_char_count and template.template_type == 'sms': content_char_count = 1 From 4ca23b22828f32ba024470c103e481bf0a555c49 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 2 Aug 2016 12:14:42 +0100 Subject: [PATCH 3/7] bring models in line with alembic prevents new alembic scripts being pre-populated with index downgrades --- app/models.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/models.py b/app/models.py index 9fe39b0f3..9bcfdf74a 100644 --- a/app/models.py +++ b/app/models.py @@ -378,10 +378,10 @@ class Notification(db.Model): api_key = db.relationship('ApiKey') key_type = db.Column(db.String, db.ForeignKey('key_types.name'), index=True, unique=False, nullable=False) content_char_count = db.Column(db.Integer, nullable=True) - notification_type = db.Column(notification_types, nullable=False) + notification_type = db.Column(notification_types, index=True, nullable=False) created_at = db.Column( db.DateTime, - index=False, + index=True, unique=False, nullable=False) sent_at = db.Column( @@ -396,7 +396,7 @@ class Notification(db.Model): unique=False, nullable=True, onupdate=datetime.datetime.utcnow) - status = db.Column(NOTIFICATION_STATUS_TYPES_ENUM, nullable=False, default='created') + status = db.Column(NOTIFICATION_STATUS_TYPES_ENUM, index=True, nullable=False, default='created') reference = db.Column(db.String, nullable=True, index=True) _personalisation = db.Column(db.String, nullable=True) @@ -428,12 +428,12 @@ class NotificationHistory(db.Model): api_key = db.relationship('ApiKey') key_type = db.Column(db.String, db.ForeignKey('key_types.name'), index=True, unique=False, nullable=False) content_char_count = db.Column(db.Integer, nullable=True) - notification_type = db.Column(notification_types, nullable=False) - created_at = db.Column(db.DateTime, index=False, unique=False, nullable=False) + notification_type = db.Column(notification_types, index=True, nullable=False) + created_at = db.Column(db.DateTime, index=True, unique=False, nullable=False) sent_at = db.Column(db.DateTime, index=False, unique=False, nullable=True) sent_by = db.Column(db.String, nullable=True) updated_at = db.Column(db.DateTime, index=False, unique=False, nullable=True) - status = db.Column(NOTIFICATION_STATUS_TYPES_ENUM, nullable=False, default='created') + status = db.Column(NOTIFICATION_STATUS_TYPES_ENUM, index=True, nullable=False, default='created') reference = db.Column(db.String, nullable=True, index=True) @classmethod From 2793541b9c5d10f1c2cfb91ce37862dd15621ddd Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 3 Aug 2016 14:27:58 +0100 Subject: [PATCH 4/7] add billable_units column to notifications table this replaces content_char_count, by performing the additional steps to calculated billable units at insert time, rather than read time. This means we can take into account whether the service was in research mode or using a test api key when the notification was sent :tada --- app/dao/provider_statistics_dao.py | 1 - app/models.py | 2 + migrations/versions/0045_billable_units.py | 113 +++++++++++++++++++++ 3 files changed, 115 insertions(+), 1 deletion(-) create mode 100644 migrations/versions/0045_billable_units.py diff --git a/app/dao/provider_statistics_dao.py b/app/dao/provider_statistics_dao.py index 4fffb7dd6..36636a481 100644 --- a/app/dao/provider_statistics_dao.py +++ b/app/dao/provider_statistics_dao.py @@ -22,7 +22,6 @@ def get_provider_statistics(service, **kwargs): def get_fragment_count(service_id): - live_dates = get_service_live_dates(service_id) shared_filters = [ NotificationHistory.service_id == service_id, NotificationHistory.status.in_(NOTIFICATION_STATUS_TYPES_BILLABLE), diff --git a/app/models.py b/app/models.py index 9bcfdf74a..3b712e0e5 100644 --- a/app/models.py +++ b/app/models.py @@ -378,6 +378,7 @@ class Notification(db.Model): api_key = db.relationship('ApiKey') key_type = db.Column(db.String, db.ForeignKey('key_types.name'), index=True, unique=False, nullable=False) content_char_count = db.Column(db.Integer, nullable=True) + billable_units = db.Column(db.Integer, nullable=False, default=0) notification_type = db.Column(notification_types, index=True, nullable=False) created_at = db.Column( db.DateTime, @@ -428,6 +429,7 @@ class NotificationHistory(db.Model): api_key = db.relationship('ApiKey') key_type = db.Column(db.String, db.ForeignKey('key_types.name'), index=True, unique=False, nullable=False) content_char_count = db.Column(db.Integer, nullable=True) + billable_units = db.Column(db.Integer, nullable=False, default=0) notification_type = db.Column(notification_types, index=True, nullable=False) created_at = db.Column(db.DateTime, index=True, unique=False, nullable=False) sent_at = db.Column(db.DateTime, index=False, unique=False, nullable=True) diff --git a/migrations/versions/0045_billable_units.py b/migrations/versions/0045_billable_units.py new file mode 100644 index 000000000..b75662a15 --- /dev/null +++ b/migrations/versions/0045_billable_units.py @@ -0,0 +1,113 @@ +"""empty message + +Revision ID: 0045_billable_units +Revises: 0044_jobs_to_notification_hist +Create Date: 2016-08-02 16:36:42.455838 + +""" + +# revision identifiers, used by Alembic. +revision = '0045_billable_units' +down_revision = '0044_jobs_to_notification_hist' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.orm.session import Session + +from app.models import Service +import logging + +logging.basicConfig() +logging.getLogger('sqlalchemy.engine').setLevel(logging.INFO) + +def upgrade(): + op.add_column('notifications', sa.Column('billable_units', sa.Integer())) + op.add_column('notification_history', sa.Column('billable_units', sa.Integer())) + + op.execute('update notifications set billable_units = 0') + op.execute('update notification_history set billable_units = 0') + + op.alter_column('notifications', 'billable_units', nullable=False) + op.alter_column('notification_history', 'billable_units', nullable=False) + + + conn = op.get_bind() + + # caveats + # only adjusts notifications for services that have never been in research mode. On live, research mode was + # limited to only services that we have set up ourselves so deemed this acceptable. + billable_services = conn.execute(''' + SELECT id FROM services_history WHERE id not in (select id from services_history where research_mode) + ''') + # set to 'null' if there are no billable services so we don't get a syntax error in the update statement + service_ids = ','.join("'{}'".format(service.id) for service in billable_services) or 'null' + + + update_statement = ''' + UPDATE {} + SET billable_units = ( + CASE + WHEN content_char_count <= 160 THEN 1 + ELSE ceil(content_char_count::float / 153::float) + END + ) + WHERE content_char_count is not null + AND service_id in ({}) + AND notification_type = 'sms' + ''' + + conn = op.get_bind() + conn.execute(update_statement.format('notifications', service_ids)) + conn.execute(update_statement.format('notification_history', service_ids)) + op.drop_column('notifications', 'content_char_count') + op.drop_column('notification_history', 'content_char_count') + + +def downgrade(): + op.add_column('notifications', sa.Column( + 'content_char_count', + sa.INTEGER(), + autoincrement=False, + nullable=True) + ) + op.add_column('notification_history', sa.Column( + 'content_char_count', + sa.INTEGER(), + autoincrement=False, + nullable=True) + ) + + conn = op.get_bind() + + # caveats + # only adjusts notifications for services that have never been in research mode. On live, research mode was + # limited to only services that we have set up ourselves + billable_services = conn.execute(''' + SELECT id FROM services_history WHERE id not in (select id from services_history where research_mode) + ''') + # set to 'null' if there are no billable services so we don't get a syntax error in the update statement + service_ids = ','.join("'{}'".format(service.id) for service in billable_services) or 'null' + billable_services = session.query(Service).filter(Service.research_mode == False).all() + + # caveats: + # only approximates character counts - billable * 153 to get at least a decent ballpark + # research mode messages assumed to be one message length + update_statement = ''' + UPDATE {table} + SET content_char_count = GREATEST(billable_units, 1) * 150 + ) + WHERE service_id in ({}) + AND notification_type = 'sms' + ''' + + conn = op.get_bind() + conn.execute(update_statement.format( + 'notifications', + services=','.join(repr(str(service.id)) for service in billable_services) + )) + conn.execute(update_statement.format( + 'notification_history', + services=','.join(repr(str(service.id)) for service in billable_services) + )) + op.drop_column('notifications', 'billable_units') + op.drop_column('notification_history', 'billable_units') From 527a5c4eaadcbb9acb17d061b1ecdddd99755967 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 3 Aug 2016 16:26:11 +0100 Subject: [PATCH 5/7] calculate billable units when sending an sms don't calculate it if we're in research mode * added tests to prove this * removed last code referring to content_char_count --- app/celery/provider_tasks.py | 7 +++-- app/dao/provider_statistics_dao.py | 12 +------- app/models.py | 2 -- app/spec/rest.py | 4 +-- migrations/versions/0045_billable_units.py | 4 --- tests/app/celery/test_provider_tasks.py | 30 +++++++++++++++++-- tests/app/conftest.py | 4 +-- tests/app/dao/test_notification_dao.py | 2 +- tests/app/dao/test_provider_statistics_dao.py | 19 ++++++------ 9 files changed, 47 insertions(+), 37 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index d485e3560..32889b663 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -20,7 +20,7 @@ from notifications_utils.recipients import ( ) from app.dao.templates_dao import dao_get_template_by_id -from notifications_utils.template import Template +from notifications_utils.template import Template, get_sms_fragment_count from notifications_utils.renderers import HTMLEmail, PlainTextEmail, SMSMessage from app.models import SMS_TYPE, EMAIL_TYPE, KEY_TYPE_TEST @@ -68,6 +68,7 @@ def send_sms_to_provider(self, service_id, notification_id): send_sms_response.apply_async( (provider.get_name(), str(notification_id), notification.to), queue='research-mode' ) + notification.billable_units = 0 else: provider.send_sms( to=validate_and_format_phone_number(notification.to), @@ -75,6 +76,7 @@ def send_sms_to_provider(self, service_id, notification_id): reference=str(notification_id), sender=service.sms_sender ) + notification.billable_units = get_sms_fragment_count(template.replaced_content_count) update_provider_stats( notification_id, @@ -84,8 +86,7 @@ def send_sms_to_provider(self, service_id, notification_id): ) notification.sent_at = datetime.utcnow() - notification.sent_by = provider.get_name(), - notification.content_char_count = template.replaced_content_count + notification.sent_by = provider.get_name() notification.status = 'sending' dao_update_notification(notification) except SmsClientException as e: diff --git a/app/dao/provider_statistics_dao.py b/app/dao/provider_statistics_dao.py index 36636a481..a7f13fbf4 100644 --- a/app/dao/provider_statistics_dao.py +++ b/app/dao/provider_statistics_dao.py @@ -29,17 +29,7 @@ def get_fragment_count(service_id): ] sms_count = db.session.query( - func.sum( - case( - [ - ( - NotificationHistory.content_char_count <= 160, - func.ceil(cast(NotificationHistory.content_char_count, Float) / 153) - ) - ], - else_=1 - ) - ) + func.sum(NotificationHistory.billable_units) ).filter( NotificationHistory.notification_type == SMS_TYPE, *shared_filters diff --git a/app/models.py b/app/models.py index 3b712e0e5..f236841ac 100644 --- a/app/models.py +++ b/app/models.py @@ -377,7 +377,6 @@ class Notification(db.Model): api_key_id = db.Column(UUID(as_uuid=True), db.ForeignKey('api_keys.id'), index=True, unique=False) api_key = db.relationship('ApiKey') key_type = db.Column(db.String, db.ForeignKey('key_types.name'), index=True, unique=False, nullable=False) - content_char_count = db.Column(db.Integer, nullable=True) billable_units = db.Column(db.Integer, nullable=False, default=0) notification_type = db.Column(notification_types, index=True, nullable=False) created_at = db.Column( @@ -428,7 +427,6 @@ class NotificationHistory(db.Model): api_key_id = db.Column(UUID(as_uuid=True), db.ForeignKey('api_keys.id'), index=True, unique=False) api_key = db.relationship('ApiKey') key_type = db.Column(db.String, db.ForeignKey('key_types.name'), index=True, unique=False, nullable=False) - content_char_count = db.Column(db.Integer, nullable=True) billable_units = db.Column(db.Integer, nullable=False, default=0) notification_type = db.Column(notification_types, index=True, nullable=False) created_at = db.Column(db.DateTime, index=True, unique=False, nullable=False) diff --git a/app/spec/rest.py b/app/spec/rest.py index 9cd36c381..e4045f20b 100644 --- a/app/spec/rest.py +++ b/app/spec/rest.py @@ -1,4 +1,4 @@ -from flask import jsonify, current_app, Blueprint +from flask import jsonify, Blueprint from apispec import APISpec @@ -11,7 +11,7 @@ api_spec = APISpec( ) api_spec.definition('NotificationWithTemplateSchema', properties={ - "content_char_count": { + "billable_units": { "format": "int32", "type": "integer" }, diff --git a/migrations/versions/0045_billable_units.py b/migrations/versions/0045_billable_units.py index b75662a15..ed1d9d262 100644 --- a/migrations/versions/0045_billable_units.py +++ b/migrations/versions/0045_billable_units.py @@ -15,10 +15,6 @@ import sqlalchemy as sa from sqlalchemy.orm.session import Session from app.models import Service -import logging - -logging.basicConfig() -logging.getLogger('sqlalchemy.engine').setLevel(logging.INFO) def upgrade(): op.add_column('notifications', sa.Column('billable_units', sa.Integer())) diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 64d1d296d..3f8f293ff 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -111,7 +111,7 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist( assert notification.status == 'sending' assert notification.sent_at <= datetime.utcnow() assert notification.sent_by == 'mmg' - assert notification.content_char_count == len("Sample service: Hello Jo\nYour thing is due soon") + assert notification.billable_units == 1 assert notification.personalisation == {"name": "Jo"} @@ -194,7 +194,6 @@ def test_send_sms_should_use_template_version_from_notification_not_latest( assert persisted_notification.template_id == sample_template.id assert persisted_notification.template_version == version_on_notification assert persisted_notification.template_version != sample_template.version - assert persisted_notification.content_char_count == len("Sample service: This is a template:\nwith a newline") assert persisted_notification.status == 'sending' assert not persisted_notification.personalisation @@ -546,3 +545,30 @@ def test_send_email_should_use_service_reply_to_email( html_body=ANY, reply_to_address=sample_service.reply_to_email_address ) + + +def test_should_not_set_billable_units_if_research_mode(notify_db, sample_service, sample_notification, mocker): + mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.mmg_client.get_name', return_value="mmg") + mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async') + + sample_service.research_mode = True + notify_db.session.add(sample_service) + notify_db.session.commit() + + send_sms_to_provider( + sample_notification.service_id, + sample_notification.id + ) + + persisted_notification = notifications_dao.get_notification(sample_service.id, sample_notification.id) + assert persisted_notification.billable_units == 0 + + +def _get_provider_statistics(service, **kwargs): + query = ProviderStatistics.query.filter_by(service=service) + if 'providers' in kwargs: + providers = ProviderDetails.query.filter(ProviderDetails.identifier.in_(kwargs['providers'])).all() + provider_ids = [provider.id for provider in providers] + query = query.filter(ProviderStatistics.provider_id.in_(provider_ids)) + return query diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 4a95f6e67..38a5ca956 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -323,7 +323,7 @@ def sample_notification(notify_db, status='created', reference=None, created_at=None, - content_char_count=160, + billable_units=1, create=True, personalisation=None, api_key_id=None, @@ -356,7 +356,7 @@ def sample_notification(notify_db, 'status': status, 'reference': reference, 'created_at': created_at, - 'content_char_count': content_char_count, + 'billable_units': billable_units, 'personalisation': personalisation, 'notification_type': template.template_type, 'api_key_id': api_key_id, diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index a963dd648..972f65944 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -1028,7 +1028,7 @@ def _notification_json(sample_template, job_id=None, id=None, status=None): 'template_id': sample_template.id, 'template_version': sample_template.version, 'created_at': datetime.utcnow(), - 'content_char_count': 160, + 'billable_units': 1, 'notification_type': sample_template.template_type, 'key_type': KEY_TYPE_NORMAL } diff --git a/tests/app/dao/test_provider_statistics_dao.py b/tests/app/dao/test_provider_statistics_dao.py index f5f349b7c..c3b268de4 100644 --- a/tests/app/dao/test_provider_statistics_dao.py +++ b/tests/app/dao/test_provider_statistics_dao.py @@ -119,11 +119,10 @@ def test_get_fragment_count_filters_on_service_id(notify_db, sample_template, se assert get_fragment_count(service_2.id)['sms_count'] == 0 -def test_get_fragment_count_sums_char_count_for_sms(notify_db, sample_template): - noti_hist(notify_db, sample_template, content_char_count=1) # 1 - noti_hist(notify_db, sample_template, content_char_count=159) # 1 - noti_hist(notify_db, sample_template, content_char_count=310) # 2 - assert get_fragment_count(sample_template.service_id)['sms_count'] == 4 +def test_get_fragment_count_sums_billable_units_for_sms(notify_db, sample_template): + noti_hist(notify_db, sample_template, billable_units=1) + noti_hist(notify_db, sample_template, billable_units=2) + assert get_fragment_count(sample_template.service_id)['sms_count'] == 3 @pytest.mark.parametrize('key_type,sms_count', [ @@ -136,9 +135,9 @@ def test_get_fragment_count_ignores_test_api_keys(notify_db, sample_template, ke assert get_fragment_count(sample_template.service_id)['sms_count'] == sms_count -def noti_hist(notify_db, template, status='delivered', content_char_count=None, key_type=KEY_TYPE_NORMAL): - if not content_char_count and template.template_type == 'sms': - content_char_count = 1 +def noti_hist(notify_db, template, status='delivered', billable_units=None, key_type=KEY_TYPE_NORMAL): + if not billable_units and template.template_type == 'sms': + billable_units = 1 notification_history = NotificationHistory( id=uuid.uuid4(), @@ -147,9 +146,9 @@ def noti_hist(notify_db, template, status='delivered', content_char_count=None, template_version=template.version, status=status, created_at=datetime.utcnow(), - content_char_count=content_char_count, + billable_units=billable_units, notification_type=template.template_type, - key_type=KEY_TYPE_NORMAL + key_type=key_type ) notify_db.session.add(notification_history) notify_db.session.commit() From 143cfb526c769d7acba5af9a101168e1669ef274 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 3 Aug 2016 17:22:20 +0100 Subject: [PATCH 6/7] change update_provider_stats to use billable_units updated tests etc, and removed some old tests that are no longer relevant --- app/celery/provider_tasks.py | 5 +- app/dao/notifications_dao.py | 14 +--- tests/app/celery/test_provider_tasks.py | 4 +- tests/app/dao/test_provider_statistics_dao.py | 21 ++--- .../service/test_service_fragment_count.py | 77 ------------------- 5 files changed, 19 insertions(+), 102 deletions(-) delete mode 100644 tests/app/service/test_service_fragment_count.py diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 32889b663..8b7af54fd 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -82,7 +82,7 @@ def send_sms_to_provider(self, service_id, notification_id): notification_id, SMS_TYPE, provider.get_name(), - content_char_count=template.replaced_content_count + billable_units=notification.billable_units ) notification.sent_at = datetime.utcnow() @@ -164,7 +164,8 @@ def send_email_to_provider(self, service_id, notification_id): update_provider_stats( notification_id, EMAIL_TYPE, - provider.get_name() + provider.get_name(), + billable_units=1 ) notification.reference = reference notification.sent_at = datetime.utcnow() diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 14a3821b3..1e7f2d75e 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -299,30 +299,22 @@ def update_provider_stats( id_, notification_type, provider_name, - content_char_count=None): + billable_units=1): notification = Notification.query.filter(Notification.id == id_).one() provider = ProviderDetails.query.filter_by(identifier=provider_name).one() - def unit_count(): - if notification_type == EMAIL_TYPE: - return 1 - else: - if (content_char_count): - return get_sms_fragment_count(content_char_count) - return get_sms_fragment_count(notification.content_char_count) - update_count = db.session.query(ProviderStatistics).filter_by( day=date.today(), service_id=notification.service_id, provider_id=provider.id - ).update({'unit_count': ProviderStatistics.unit_count + unit_count()}) + ).update({'unit_count': ProviderStatistics.unit_count + billable_units}) if update_count == 0: provider_stats = ProviderStatistics( day=notification.created_at.date(), service_id=notification.service_id, provider_id=provider.id, - unit_count=unit_count() + unit_count=billable_units ) db.session.add(provider_stats) diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 3f8f293ff..eb788129a 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -557,8 +557,8 @@ def test_should_not_set_billable_units_if_research_mode(notify_db, sample_servic notify_db.session.commit() send_sms_to_provider( - sample_notification.service_id, - sample_notification.id + sample_notification.service_id, + sample_notification.id ) persisted_notification = notifications_dao.get_notification(sample_service.id, sample_notification.id) diff --git a/tests/app/dao/test_provider_statistics_dao.py b/tests/app/dao/test_provider_statistics_dao.py index c3b268de4..955aa3c23 100644 --- a/tests/app/dao/test_provider_statistics_dao.py +++ b/tests/app/dao/test_provider_statistics_dao.py @@ -1,10 +1,11 @@ from datetime import datetime import uuid -from app.models import NotificationHistory, KEY_TYPE_NORMAL, NOTIFICATION_STATUS_TYPES +import pytest + +from app.models import NotificationHistory, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, NOTIFICATION_STATUS_TYPES from app.dao.notifications_dao import update_provider_stats -from app.dao.provider_statistics_dao import ( - get_provider_statistics, get_fragment_count) +from app.dao.provider_statistics_dao import get_provider_statistics, get_fragment_count from tests.app.conftest import sample_notification as create_sample_notification @@ -46,24 +47,24 @@ def test_should_update_provider_statistics_sms_multi(notify_db, notify_db, notify_db_session, template=sample_template, - content_char_count=160) - update_provider_stats(n1.id, 'sms', mmg_provider.identifier) + billable_units=1) + update_provider_stats(n1.id, 'sms', mmg_provider.identifier, n1.billable_units) n2 = create_sample_notification( notify_db, notify_db_session, template=sample_template, - content_char_count=161) - update_provider_stats(n2.id, 'sms', mmg_provider.identifier) + billable_units=2) + update_provider_stats(n2.id, 'sms', mmg_provider.identifier, n2.billable_units) n3 = create_sample_notification( notify_db, notify_db_session, template=sample_template, - content_char_count=307) - update_provider_stats(n3.id, 'sms', mmg_provider.identifier) + billable_units=4) + update_provider_stats(n3.id, 'sms', mmg_provider.identifier, n3.billable_units) provider_stats = get_provider_statistics( sample_template.service, providers=[mmg_provider.identifier]).one() - assert provider_stats.unit_count == 6 + assert provider_stats.unit_count == 7 def test_should_update_provider_statistics_email_multi(notify_db, diff --git a/tests/app/service/test_service_fragment_count.py b/tests/app/service/test_service_fragment_count.py deleted file mode 100644 index 090e34d7b..000000000 --- a/tests/app/service/test_service_fragment_count.py +++ /dev/null @@ -1,77 +0,0 @@ -import json -from datetime import (date, timedelta) -from flask import url_for -from tests import create_authorization_header - - -def test_fragment_count(notify_api, sample_provider_statistics): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - endpoint = url_for( - 'service.get_service_provider_aggregate_statistics', - service_id=str(sample_provider_statistics.service.id)) - auth_header = create_authorization_header() - resp = client.get( - endpoint, - headers=[auth_header] - ) - assert resp.status_code == 200 - json_resp = json.loads(resp.get_data(as_text=True)) - assert json_resp['data']['sms_count'] == 1 - - -def test_fragment_count_from_to(notify_api, sample_provider_statistics): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - today_str = date.today().strftime('%Y-%m-%d') - endpoint = url_for( - 'service.get_service_provider_aggregate_statistics', - service_id=str(sample_provider_statistics.service.id), - date_from=today_str, - date_to=today_str) - auth_header = create_authorization_header() - resp = client.get( - endpoint, - headers=[auth_header] - ) - assert resp.status_code == 200 - json_resp = json.loads(resp.get_data(as_text=True)) - assert json_resp['data']['sms_count'] == 1 - - -def test_fragment_count_from_greater_than_to(notify_api, sample_provider_statistics): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - today_str = date.today().strftime('%Y-%m-%d') - yesterday_str = date.today() - timedelta(days=1) - endpoint = url_for( - 'service.get_service_provider_aggregate_statistics', - service_id=str(sample_provider_statistics.service.id), - date_from=today_str, - date_to=yesterday_str) - auth_header = create_authorization_header() - resp = client.get( - endpoint, - headers=[auth_header] - ) - assert resp.status_code == 400 - json_resp = json.loads(resp.get_data(as_text=True)) - assert 'date_from needs to be greater than date_to' in json_resp['message']['_schema'] - - -def test_fragment_count_in_future(notify_api, sample_provider_statistics): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - tomorrow_str = (date.today() + timedelta(days=1)).strftime('%Y-%m-%d') - endpoint = url_for( - 'service.get_service_provider_aggregate_statistics', - service_id=str(sample_provider_statistics.service.id), - date_from=tomorrow_str) - auth_header = create_authorization_header() - resp = client.get( - endpoint, - headers=[auth_header] - ) - assert resp.status_code == 400 - json_resp = json.loads(resp.get_data(as_text=True)) - assert 'Date cannot be in the future' in json_resp['message']['date_from'] From d86af3ce83d78676d043655b9364300d3052c590 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 4 Aug 2016 12:00:26 +0100 Subject: [PATCH 7/7] fix syntax errors in downgrade script --- migrations/versions/0045_billable_units.py | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/migrations/versions/0045_billable_units.py b/migrations/versions/0045_billable_units.py index ed1d9d262..ab24ea7d6 100644 --- a/migrations/versions/0045_billable_units.py +++ b/migrations/versions/0045_billable_units.py @@ -83,27 +83,19 @@ def downgrade(): ''') # set to 'null' if there are no billable services so we don't get a syntax error in the update statement service_ids = ','.join("'{}'".format(service.id) for service in billable_services) or 'null' - billable_services = session.query(Service).filter(Service.research_mode == False).all() # caveats: # only approximates character counts - billable * 153 to get at least a decent ballpark # research mode messages assumed to be one message length update_statement = ''' - UPDATE {table} + UPDATE {} SET content_char_count = GREATEST(billable_units, 1) * 150 - ) WHERE service_id in ({}) AND notification_type = 'sms' ''' conn = op.get_bind() - conn.execute(update_statement.format( - 'notifications', - services=','.join(repr(str(service.id)) for service in billable_services) - )) - conn.execute(update_statement.format( - 'notification_history', - services=','.join(repr(str(service.id)) for service in billable_services) - )) + conn.execute(update_statement.format('notifications', service_ids)) + conn.execute(update_statement.format('notification_history', service_ids)) op.drop_column('notifications', 'billable_units') op.drop_column('notification_history', 'billable_units')