From 527a5c4eaadcbb9acb17d061b1ecdddd99755967 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 3 Aug 2016 16:26:11 +0100 Subject: [PATCH] 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()