From aed9bc0fe73fa2044c6957a49cf14527de6b7fb9 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 24 Apr 2017 13:01:13 +0100 Subject: [PATCH 01/46] Added tests for international sms flag --- tests/app/service/test_rest.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 43edbc040..2cf666be4 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -224,6 +224,7 @@ def test_create_service(client, sample_user): assert json_resp['data']['name'] == 'created service' assert not json_resp['data']['research_mode'] assert not json_resp['data']['can_send_letters'] + assert not json_resp['data']['can_send_international_sms'] def test_should_not_create_service_with_missing_user_id_field(notify_api, fake_uuid): @@ -419,10 +420,12 @@ def test_update_service_flags(notify_api, sample_service): assert json_resp['data']['name'] == sample_service.name assert json_resp['data']['research_mode'] is False assert json_resp['data']['can_send_letters'] is False + assert json_resp['data']['can_send_international_sms'] is False data = { 'research_mode': True, - 'can_send_letters': True + 'can_send_letters': True, + 'can_send_international_sms': True, } auth_header = create_authorization_header() @@ -436,6 +439,7 @@ def test_update_service_flags(notify_api, sample_service): assert resp.status_code == 200 assert result['data']['research_mode'] is True assert result['data']['can_send_letters'] is True + assert result['data']['can_send_international_sms'] is True def test_update_service_research_mode_throws_validation_error(notify_api, sample_service): From e977e7cee487bdfc7597f46d303f350c9ebd74d2 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 25 Apr 2017 14:56:16 +0100 Subject: [PATCH 02/46] Wired in a simple callback to handle SNS notifications from S3 S3 will send a message when a file lands - which will trigger processing of DVLA responses. --- app/__init__.py | 4 ++ .../notifications_letter_callback.py | 39 +++++++++++++++++++ .../app/notifications/rest/test_callbacks.py | 10 +++++ 3 files changed, 53 insertions(+) create mode 100644 app/notifications/notifications_letter_callback.py diff --git a/app/__init__.py b/app/__init__.py index 0c2734f77..8d8e18270 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -95,6 +95,7 @@ def register_blueprint(application): from app.notifications.receive_notifications import receive_notifications_blueprint from app.notifications.notifications_ses_callback import ses_callback_blueprint from app.notifications.notifications_sms_callback import sms_callback_blueprint + from app.notifications.notifications_letter_callback import letter_callback_blueprint from app.authentication.auth import requires_admin_auth, requires_auth, requires_no_auth from app.letters.send_letter_jobs import letter_job @@ -155,6 +156,9 @@ def register_blueprint(application): letter_job.before_request(requires_admin_auth) application.register_blueprint(letter_job) + letter_callback_blueprint.before_request(requires_no_auth) + application.register_blueprint(letter_callback_blueprint) + def register_v2_blueprints(application): from app.v2.notifications.post_notifications import v2_notification_blueprint as post_notifications diff --git a/app/notifications/notifications_letter_callback.py b/app/notifications/notifications_letter_callback.py new file mode 100644 index 000000000..f7c357055 --- /dev/null +++ b/app/notifications/notifications_letter_callback.py @@ -0,0 +1,39 @@ +from datetime import datetime + +from flask import ( + Blueprint, + jsonify, + request, + current_app, + json +) + +from app import statsd_client +from app.clients.email.aws_ses import get_aws_responses +from app.dao import ( + notifications_dao +) + +from app.notifications.process_client_response import validate_callback_data + +letter_callback_blueprint = Blueprint('notifications_letter_callback', __name__) + +from app.errors import ( + register_errors, + InvalidRequest +) + +register_errors(letter_callback_blueprint) + + +@letter_callback_blueprint.route('/notifications/letter/dvla', methods=['POST']) +def process_letter_response(): + try: + dvla_request = json.loads(request.data) + current_app.logger.info(dvla_request) + return jsonify( + result="success", message="DVLA callback succeeded" + ), 200 + except ValueError: + error = "DVLA callback failed: invalid json" + raise InvalidRequest(error, status_code=400) diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py index c24bb76d3..780076b34 100644 --- a/tests/app/notifications/rest/test_callbacks.py +++ b/tests/app/notifications/rest/test_callbacks.py @@ -13,6 +13,16 @@ from app.models import NotificationStatistics from tests.app.conftest import sample_notification as create_sample_notification +def test_dvla_callback_should_not_need_auth(client): + data = json.dumps({"somekey": "somevalue"}) + response = client.post( + path='/notifications/letter/dvla', + data=data, + headers=[('Content-Type', 'application/json')]) + + assert response.status_code == 200 + + def test_firetext_callback_should_not_need_auth(client, mocker): mocker.patch('app.statsd_client.incr') response = client.post( From c87be0f98295d64addc01529999996b566c80f2c Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 25 Apr 2017 16:29:07 +0100 Subject: [PATCH 03/46] add sent notification status --- .../00xx_add_sent_notification_status.py | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 migrations/versions/00xx_add_sent_notification_status.py diff --git a/migrations/versions/00xx_add_sent_notification_status.py b/migrations/versions/00xx_add_sent_notification_status.py new file mode 100644 index 000000000..60a4d3a2e --- /dev/null +++ b/migrations/versions/00xx_add_sent_notification_status.py @@ -0,0 +1,60 @@ +"""empty message + +Revision ID: 00xx_add_sent_notification_status +Revises: 0075_create_rates_table +Create Date: 2017-04-24 16:55:20.731069 + +""" + +# revision identifiers, used by Alembic. +revision = '00xx_sent_notification_status' +down_revision = '0075_create_rates_table' + +from alembic import op +import sqlalchemy as sa + +enum_name = 'notify_status_type' +tmp_name = 'tmp_' + enum_name + +old_options = ( + 'created', + 'sending', + 'delivered', + 'pending', + 'failed', + 'technical-failure', + 'temporary-failure', + 'permanent-failure' +) +new_options = old_options + ('sent',) + +old_type = sa.Enum(*old_options, name=enum_name) +new_type = sa.Enum(*new_options, name=enum_name) + +alter_str = 'ALTER TABLE {table} ALTER COLUMN status TYPE {enum} USING status::text::notify_status_type ' + +def upgrade(): + op.execute('ALTER TYPE {enum} RENAME TO {tmp_name}'.format(enum=enum_name, tmp_name=tmp_name)) + + new_type.create(op.get_bind()) + op.execute(alter_str.format(table='notifications', enum=enum_name)) + op.execute(alter_str.format(table='notification_history', enum=enum_name)) + + op.execute('DROP TYPE ' + tmp_name) + + +def downgrade(): + op.execute('ALTER TYPE {enum} RENAME TO {tmp_name}'.format(enum=enum_name, tmp_name=tmp_name)) + + # Convert 'sent' template into 'sending' + update_str = "UPDATE TABLE {table} SET status='sending' where status='sent'" + + op.execute(update_str.format(table='notifications')) + op.execute(update_str.format(table='notification_history')) + + old_type.create(op.get_bind()) + + op.execute(alter_str.format(table='notifications', enum=enum_name)) + op.execute(alter_str.format(table='notification_history', enum=enum_name)) + + op.execute('DROP TYPE ' + tmp_name) From 02e8f454e12e3db0d001747d5ff1067cd985ed8c Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 25 Apr 2017 17:08:29 +0100 Subject: [PATCH 04/46] add new notification status 'sent' for international text messages, when we may not ever receive callback info, so we need a separate status that doesn't get timed out etc --- app/config.py | 1 - app/models.py | 4 ++++ tests/app/dao/test_provider_statistics_dao.py | 4 ++-- tests/app/v2/notifications/test_get_notifications.py | 4 ++-- tests/app/v2/notifications/test_notification_schemas.py | 4 ++-- 5 files changed, 10 insertions(+), 7 deletions(-) diff --git a/app/config.py b/app/config.py index 5dd33557e..4e4a77aec 100644 --- a/app/config.py +++ b/app/config.py @@ -187,7 +187,6 @@ class Development(Config): NOTIFY_ENVIRONMENT = 'development' NOTIFICATION_QUEUE_PREFIX = 'development' DEBUG = True - SQLALCHEMY_ECHO = False CELERY_QUEUES = Config.CELERY_QUEUES + [ Queue('db-sms', Exchange('default'), routing_key='db-sms'), Queue('priority', Exchange('default'), routing_key='priority'), diff --git a/app/models.py b/app/models.py index d54f413e4..a4bb1d5cf 100644 --- a/app/models.py +++ b/app/models.py @@ -572,6 +572,7 @@ class VerifyCode(db.Model): NOTIFICATION_CREATED = 'created' NOTIFICATION_SENDING = 'sending' +NOTIFICATION_SENT = 'sent' NOTIFICATION_DELIVERED = 'delivered' NOTIFICATION_PENDING = 'pending' NOTIFICATION_FAILED = 'failed' @@ -586,6 +587,7 @@ NOTIFICATION_STATUS_TYPES_FAILED = [ ] NOTIFICATION_STATUS_TYPES_COMPLETED = [ + NOTIFICATION_SENT, NOTIFICATION_DELIVERED, NOTIFICATION_FAILED, NOTIFICATION_TECHNICAL_FAILURE, @@ -595,6 +597,7 @@ NOTIFICATION_STATUS_TYPES_COMPLETED = [ NOTIFICATION_STATUS_TYPES_BILLABLE = [ NOTIFICATION_SENDING, + NOTIFICATION_SENT, NOTIFICATION_DELIVERED, NOTIFICATION_FAILED, NOTIFICATION_TECHNICAL_FAILURE, @@ -605,6 +608,7 @@ NOTIFICATION_STATUS_TYPES_BILLABLE = [ NOTIFICATION_STATUS_TYPES = [ NOTIFICATION_CREATED, NOTIFICATION_SENDING, + NOTIFICATION_SENT, NOTIFICATION_DELIVERED, NOTIFICATION_PENDING, NOTIFICATION_FAILED, diff --git a/tests/app/dao/test_provider_statistics_dao.py b/tests/app/dao/test_provider_statistics_dao.py index e3249ec64..54e429b0e 100644 --- a/tests/app/dao/test_provider_statistics_dao.py +++ b/tests/app/dao/test_provider_statistics_dao.py @@ -26,8 +26,8 @@ def test_get_fragment_count_separates_sms_and_email(notify_db, sample_template, 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 + # sending, sent, delivered, failed, technical-failure, temporary-failure, permanent-failure + assert get_fragment_count(sample_template.service_id)['sms_count'] == 7 def test_get_fragment_count_filters_on_service_id(notify_db, sample_template, service_factory): diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index 5c2c31af3..6e3b72ed2 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -307,8 +307,8 @@ def test_get_all_notifications_filter_by_status_invalid_status(client, sample_no assert json_response['status_code'] == 400 assert len(json_response['errors']) == 1 - assert json_response['errors'][0]['message'] == "status elephant is not one of [created, sending, delivered, " \ - "pending, failed, technical-failure, temporary-failure, permanent-failure]" + assert json_response['errors'][0]['message'] == "status elephant is not one of [created, sending, sent, " \ + "delivered, pending, failed, technical-failure, temporary-failure, permanent-failure]" def test_get_all_notifications_filter_by_multiple_statuses(client, notify_db, notify_db_session): diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py index 44a743a1a..04c32e1c7 100644 --- a/tests/app/v2/notifications/test_notification_schemas.py +++ b/tests/app/v2/notifications/test_notification_schemas.py @@ -27,7 +27,7 @@ def test_get_notifications_request_invalid_statuses( invalid_statuses, valid_statuses ): partial_error_status = "is not one of " \ - "[created, sending, delivered, pending, failed, " \ + "[created, sending, sent, delivered, pending, failed, " \ "technical-failure, temporary-failure, permanent-failure]" with pytest.raises(ValidationError) as e: @@ -74,7 +74,7 @@ def test_get_notifications_request_invalid_statuses_and_template_types(): error_messages = [error['message'] for error in errors] for invalid_status in ["elephant", "giraffe"]: - assert "status {} is not one of [created, sending, delivered, " \ + assert "status {} is not one of [created, sending, sent, delivered, " \ "pending, failed, technical-failure, temporary-failure, permanent-failure]".format( invalid_status ) in error_messages From 25ce3851eb0807bee1943196245c93ace7f5aff8 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 25 Apr 2017 10:36:37 +0100 Subject: [PATCH 05/46] Add international support flag for providers --- app/models.py | 3 +++ .../0074_add_intl_flag_to_provider.py | 27 +++++++++++++++++++ tests/app/provider_details/test_rest.py | 4 +-- 3 files changed, 32 insertions(+), 2 deletions(-) create mode 100644 migrations/versions/0074_add_intl_flag_to_provider.py diff --git a/app/models.py b/app/models.py index d54f413e4..11cb1803c 100644 --- a/app/models.py +++ b/app/models.py @@ -443,6 +443,8 @@ class ProviderDetails(db.Model): updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=True) created_by = db.relationship('User') + supports_international = db.Column(db.Boolean, nullable=False, default=False) + class ProviderDetailsHistory(db.Model, HistoryModel): @@ -458,6 +460,7 @@ class ProviderDetailsHistory(db.Model, HistoryModel): updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=True) created_by = db.relationship('User') + supports_international = db.Column(db.Boolean, nullable=False, default=False) JOB_STATUS_PENDING = 'pending' diff --git a/migrations/versions/0074_add_intl_flag_to_provider.py b/migrations/versions/0074_add_intl_flag_to_provider.py new file mode 100644 index 000000000..687eb5ea3 --- /dev/null +++ b/migrations/versions/0074_add_intl_flag_to_provider.py @@ -0,0 +1,27 @@ +"""empty message + +Revision ID: 0074_add_intl_flag_to_provider +Revises: 0073_add_international_sms_flag +Create Date: 2017-04-25 09:44:13.194164 + +""" + +# revision identifiers, used by Alembic. +revision = '0074_add_intl_flag_to_provider' +down_revision = '0073_add_international_sms_flag' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + op.add_column('provider_details', sa.Column('supports_international', sa.Boolean(), nullable=False, server_default=sa.false())) + op.add_column('provider_details_history', sa.Column('supports_international', sa.Boolean(), nullable=False, server_default=sa.false())) + + op.execute("UPDATE provider_details SET supports_international=True WHERE identifier='mmg'") + op.execute("UPDATE provider_details_history SET supports_international=True WHERE identifier='mmg'") + + +def downgrade(): + op.drop_column('provider_details_history', 'supports_international') + op.drop_column('provider_details', 'supports_international') diff --git a/tests/app/provider_details/test_rest.py b/tests/app/provider_details/test_rest.py index 15e2381be..36a40d2a0 100644 --- a/tests/app/provider_details/test_rest.py +++ b/tests/app/provider_details/test_rest.py @@ -47,7 +47,7 @@ def test_get_provider_details_contains_correct_fields(client, notify_db): allowed_keys = { "id", "created_by", "display_name", "identifier", "priority", 'notification_type', - "active", "version", "updated_at" + "active", "version", "updated_at", "supports_international" } assert allowed_keys == set(json_resp[0].keys()) @@ -116,7 +116,7 @@ def test_get_provider_versions_contains_correct_fields(client, notify_db): allowed_keys = { "id", "created_by", "display_name", "identifier", "priority", 'notification_type', - "active", "version", "updated_at" + "active", "version", "updated_at", "supports_international" } assert allowed_keys == set(json_resp[0].keys()) From 20d6d9d333bb32a4891db271076c06cae20c7bca Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 25 Apr 2017 17:35:22 +0100 Subject: [PATCH 06/46] Update migration off master --- ...lag_to_provider.py => 0076_add_intl_flag_to_provider.py} | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) rename migrations/versions/{0074_add_intl_flag_to_provider.py => 0076_add_intl_flag_to_provider.py} (85%) diff --git a/migrations/versions/0074_add_intl_flag_to_provider.py b/migrations/versions/0076_add_intl_flag_to_provider.py similarity index 85% rename from migrations/versions/0074_add_intl_flag_to_provider.py rename to migrations/versions/0076_add_intl_flag_to_provider.py index 687eb5ea3..240acf038 100644 --- a/migrations/versions/0074_add_intl_flag_to_provider.py +++ b/migrations/versions/0076_add_intl_flag_to_provider.py @@ -1,14 +1,14 @@ """empty message -Revision ID: 0074_add_intl_flag_to_provider +Revision ID: 0076_add_intl_flag_to_provider Revises: 0073_add_international_sms_flag Create Date: 2017-04-25 09:44:13.194164 """ # revision identifiers, used by Alembic. -revision = '0074_add_intl_flag_to_provider' -down_revision = '0073_add_international_sms_flag' +revision = '0076_add_intl_flag_to_provider' +down_revision = '0075_create_rates_table' from alembic import op import sqlalchemy as sa From 51715479907c1b7216951802a0e4be36737465c0 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Wed, 26 Apr 2017 09:52:02 +0100 Subject: [PATCH 07/46] Remove blank lines --- app/models.py | 1 - 1 file changed, 1 deletion(-) diff --git a/app/models.py b/app/models.py index 11cb1803c..71957d99c 100644 --- a/app/models.py +++ b/app/models.py @@ -446,7 +446,6 @@ class ProviderDetails(db.Model): supports_international = db.Column(db.Boolean, nullable=False, default=False) - class ProviderDetailsHistory(db.Model, HistoryModel): __tablename__ = 'provider_details_history' From c69ee2405a78e2137543ed23fdde16ed58ef2c86 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Wed, 26 Apr 2017 10:37:12 +0100 Subject: [PATCH 08/46] Fix comment --- migrations/versions/0076_add_intl_flag_to_provider.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/migrations/versions/0076_add_intl_flag_to_provider.py b/migrations/versions/0076_add_intl_flag_to_provider.py index 240acf038..921bb85dd 100644 --- a/migrations/versions/0076_add_intl_flag_to_provider.py +++ b/migrations/versions/0076_add_intl_flag_to_provider.py @@ -1,7 +1,7 @@ """empty message Revision ID: 0076_add_intl_flag_to_provider -Revises: 0073_add_international_sms_flag +Revises: 0075_create_rates_table Create Date: 2017-04-25 09:44:13.194164 """ From 26b35c5d15635bd9a9ea6a82eb54b26a35bebff0 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Wed, 26 Apr 2017 10:22:20 +0100 Subject: [PATCH 09/46] Add intl notificiation fields --- app/models.py | 10 ++++++ .../versions/0076_add_intl_notification.py | 32 +++++++++++++++++++ .../test_process_notification.py | 30 ++++++++++------- 3 files changed, 60 insertions(+), 12 deletions(-) create mode 100644 migrations/versions/0076_add_intl_notification.py diff --git a/app/models.py b/app/models.py index 71957d99c..d21c53ff0 100644 --- a/app/models.py +++ b/app/models.py @@ -662,6 +662,12 @@ class Notification(db.Model): foreign(template_version) == remote(TemplateHistory.version) )) + client_reference = db.Column(db.String, index=True, nullable=True) + + international = db.Column(db.Boolean, nullable=False, default=False) + phone_prefix = db.Column(db.String, nullable=True) + rate_multiplier = db.Column(db.Numeric(), nullable=True) + @property def personalisation(self): if self._personalisation: @@ -835,6 +841,10 @@ class NotificationHistory(db.Model, HistoryModel): reference = db.Column(db.String, nullable=True, index=True) client_reference = db.Column(db.String, nullable=True) + international = db.Column(db.Boolean, nullable=False, default=False) + phone_prefix = db.Column(db.String, nullable=True) + rate_multiplier = db.Column(db.Numeric(), nullable=True) + @classmethod def from_original(cls, notification): history = super().from_original(notification) diff --git a/migrations/versions/0076_add_intl_notification.py b/migrations/versions/0076_add_intl_notification.py new file mode 100644 index 000000000..13a74aba6 --- /dev/null +++ b/migrations/versions/0076_add_intl_notification.py @@ -0,0 +1,32 @@ +"""empty message + +Revision ID: 0076_add_intl_notification +Revises: 0075_create_rates_table +Create Date: 2017-04-25 11:34:43.229494 + +""" + +# revision identifiers, used by Alembic. +revision = '0076_add_intl_notification' +down_revision = '0075_create_rates_table' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + op.add_column('notification_history', sa.Column('international', sa.Boolean(), nullable=True)) + op.add_column('notification_history', sa.Column('phone_prefix', sa.String(), nullable=True)) + op.add_column('notification_history', sa.Column('rate_multiplier', sa.Numeric(), nullable=True)) + op.add_column('notifications', sa.Column('international', sa.Boolean(), nullable=True)) + op.add_column('notifications', sa.Column('phone_prefix', sa.String(), nullable=True)) + op.add_column('notifications', sa.Column('rate_multiplier', sa.Numeric(), nullable=True)) + + +def downgrade(): + op.drop_column('notifications', 'rate_multiplier') + op.drop_column('notifications', 'phone_prefix') + op.drop_column('notifications', 'international') + op.drop_column('notification_history', 'rate_multiplier') + op.drop_column('notification_history', 'phone_prefix') + op.drop_column('notification_history', 'international') diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 9e428a0ab..533dd1a84 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -169,18 +169,20 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker) 'app.notifications.process_notifications.redis_store.get_all_from_hash') n_id = uuid.uuid4() created_at = datetime.datetime(2016, 11, 11, 16, 8, 18) - persist_notification(template_id=sample_job.template.id, - template_version=sample_job.template.version, - recipient='+447111111111', - service=sample_job.service, - personalisation=None, notification_type='sms', - api_key_id=sample_api_key.id, - key_type=sample_api_key.key_type, - created_at=created_at, - job_id=sample_job.id, - job_row_number=10, - client_reference="ref from client", - notification_id=n_id) + persist_notification( + template_id=sample_job.template.id, + template_version=sample_job.template.version, + recipient='+447111111111', + service=sample_job.service, + personalisation=None, notification_type='sms', + api_key_id=sample_api_key.id, + key_type=sample_api_key.key_type, + created_at=created_at, + job_id=sample_job.id, + job_row_number=10, + client_reference="ref from client", + notification_id=n_id + ) assert Notification.query.count() == 1 assert NotificationHistory.query.count() == 1 persisted_notification = Notification.query.all()[0] @@ -192,6 +194,10 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker) mock_service_template_cache.assert_called_once_with(cache_key_for_service_template_counter(sample_job.service_id)) assert persisted_notification.client_reference == "ref from client" assert persisted_notification.reference is None + assert persisted_notification.international is False + assert persisted_notification.phone_prefix is None + assert persisted_notification.rate_multiplier is None + @freeze_time("2016-01-01 11:09:00.061258") From 014a29170caea10621759f3b1752baed36318d52 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Wed, 26 Apr 2017 11:31:58 +0100 Subject: [PATCH 10/46] Rebase off master --- ...intl_notification.py => 0077_add_intl_notification.py} | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) rename migrations/versions/{0076_add_intl_notification.py => 0077_add_intl_notification.py} (86%) diff --git a/migrations/versions/0076_add_intl_notification.py b/migrations/versions/0077_add_intl_notification.py similarity index 86% rename from migrations/versions/0076_add_intl_notification.py rename to migrations/versions/0077_add_intl_notification.py index 13a74aba6..0e1d513c5 100644 --- a/migrations/versions/0076_add_intl_notification.py +++ b/migrations/versions/0077_add_intl_notification.py @@ -1,14 +1,14 @@ """empty message -Revision ID: 0076_add_intl_notification -Revises: 0075_create_rates_table +Revision ID: 0077_add_intl_notification +Revises: 0076_add_intl_flag_to_provider Create Date: 2017-04-25 11:34:43.229494 """ # revision identifiers, used by Alembic. -revision = '0076_add_intl_notification' -down_revision = '0075_create_rates_table' +revision = '0077_add_intl_notification' +down_revision = '0076_add_intl_flag_to_provider' from alembic import op import sqlalchemy as sa From 68edd7a33ff7896092652b2d6d65335e0657e8b4 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Wed, 26 Apr 2017 11:52:03 +0100 Subject: [PATCH 11/46] Fix PEP issues :-| --- .../test_process_notification.py | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 533dd1a84..33ebdc727 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -170,19 +170,20 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker) n_id = uuid.uuid4() created_at = datetime.datetime(2016, 11, 11, 16, 8, 18) persist_notification( - template_id=sample_job.template.id, - template_version=sample_job.template.version, - recipient='+447111111111', - service=sample_job.service, - personalisation=None, notification_type='sms', - api_key_id=sample_api_key.id, - key_type=sample_api_key.key_type, - created_at=created_at, - job_id=sample_job.id, - job_row_number=10, - client_reference="ref from client", - notification_id=n_id - ) + template_id=sample_job.template.id, + template_version=sample_job.template.version, + recipient='+447111111111', + service=sample_job.service, + personalisation=None, + notification_type='sms', + api_key_id=sample_api_key.id, + key_type=sample_api_key.key_type, + created_at=created_at, + job_id=sample_job.id, + job_row_number=10, + client_reference="ref from client", + notification_id=n_id + ) assert Notification.query.count() == 1 assert NotificationHistory.query.count() == 1 persisted_notification = Notification.query.all()[0] @@ -199,7 +200,6 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker) assert persisted_notification.rate_multiplier is None - @freeze_time("2016-01-01 11:09:00.061258") def test_persist_notification_increments_cache_if_key_exists(sample_template, sample_api_key, mocker): mock_incr = mocker.patch('app.notifications.process_notifications.redis_store.incr') From 2a0f8c88084fb6d6fa087c89de9a8f187590483d Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 26 Apr 2017 15:56:45 +0100 Subject: [PATCH 12/46] Validate International phone numbers - uses new utils methods to validate phone numbers - defaults to International=True on validation. This ensures the validator works on all numbers - Then check if the user can send this message to the number internationally if needed. --- app/notifications/process_notifications.py | 11 ++- app/notifications/rest.py | 29 +++++-- app/notifications/validators.py | 17 +++- app/schema_validation/__init__.py | 2 +- app/schemas.py | 4 +- app/v2/notifications/post_notifications.py | 3 +- requirements.txt | 2 +- tests/app/conftest.py | 4 +- tests/app/delivery/test_send_to_providers.py | 8 +- .../rest/test_send_notification.py | 82 ++++++++++++++++++- tests/app/notifications/test_validators.py | 20 ++++- .../notifications/test_post_notifications.py | 70 +++++++++++++++- 12 files changed, 224 insertions(+), 28 deletions(-) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 31d8b8c1b..033943511 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -4,6 +4,7 @@ from flask import current_app from app import redis_store from app.celery import provider_tasks +from notifications_utils.recipients import validate_and_format_phone_number from notifications_utils.clients import redis from app.dao.notifications_dao import dao_create_notification, dao_delete_notifications_and_history_by_id from app.models import SMS_TYPE, Notification, KEY_TYPE_TEST, EMAIL_TYPE @@ -98,6 +99,10 @@ def send_notification_to_queue(notification, research_mode, queue=None): def simulated_recipient(to_address, notification_type): - return (to_address in current_app.config['SIMULATED_SMS_NUMBERS'] - if notification_type == SMS_TYPE - else to_address in current_app.config['SIMULATED_EMAIL_ADDRESSES']) + if notification_type == SMS_TYPE: + formatted_simulated_numbers = [ + validate_and_format_phone_number(number) for number in current_app.config['SIMULATED_SMS_NUMBERS'] + ] + return to_address in formatted_simulated_numbers + else: + return to_address in current_app.config['SIMULATED_EMAIL_ADDRESSES'] diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 57e1efeed..2c225361b 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -2,8 +2,7 @@ from flask import ( Blueprint, jsonify, request, - current_app, - json + current_app ) from app import api_user @@ -14,10 +13,6 @@ from app.dao import ( ) from app.models import KEY_TYPE_TEAM, PRIORITY from app.models import SMS_TYPE -from app.notifications.process_client_response import ( - validate_callback_data, - process_sms_client_response -) from app.notifications.process_notifications import (persist_notification, send_notification_to_queue, simulated_recipient) @@ -35,6 +30,8 @@ from app.schemas import ( from app.service.utils import service_allowed_to_send_to from app.utils import pagination_links, get_template_instance +from notifications_utils.recipients import get_international_phone_info + notifications = Blueprint('notifications', __name__) from app.errors import ( @@ -104,13 +101,15 @@ def send_notification(notification_type): notification_form, errors = ( sms_template_notification_schema if notification_type == SMS_TYPE else email_notification_schema ).load(request.get_json()) + if errors: raise InvalidRequest(errors, status_code=400) check_service_message_limit(api_user.key_type, service) - template = templates_dao.dao_get_template_by_id_and_service_id(template_id=notification_form['template'], - service_id=service.id) + template = templates_dao.dao_get_template_by_id_and_service_id( + template_id=notification_form['template'], + service_id=service.id) check_template_is_for_notification_type(notification_type, template.template_type) check_template_is_active(template) @@ -118,12 +117,14 @@ def send_notification(notification_type): template_object = create_template_object_for_notification(template, notification_form.get('personalisation', {})) _service_allowed_to_send_to(notification_form, service) + if notification_type == SMS_TYPE: + _service_can_send_internationally(service, notification_form['to']) # Do not persist or send notification to the queue if it is a simulated recipient simulated = simulated_recipient(notification_form['to'], notification_type) notification_model = persist_notification(template_id=template.id, template_version=template.version, - recipient=notification_form['to'], + recipient=request.get_json()['to'], service=service, personalisation=notification_form.get('personalisation', None), notification_type=notification_type, @@ -160,6 +161,16 @@ def get_notification_return_data(notification_id, notification, template): return output +def _service_can_send_internationally(service, number): + international_phone_info = get_international_phone_info(number) + + if international_phone_info.international and not service.can_send_international_sms: + raise InvalidRequest( + {'to': ["Cannot send to international mobile numbers"]}, + status_code=400 + ) + + def _service_allowed_to_send_to(notification, service): if not service_allowed_to_send_to(notification['to'], service, api_user.key_type): if api_user.key_type == KEY_TYPE_TEAM: diff --git a/app/notifications/validators.py b/app/notifications/validators.py index aefee34e2..92f15b909 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -1,5 +1,9 @@ from flask import current_app -from notifications_utils.recipients import validate_and_format_phone_number, validate_and_format_email_address +from notifications_utils.recipients import ( + validate_and_format_phone_number, + validate_and_format_email_address, + get_international_phone_info +) from app.dao import services_dao from app.models import KEY_TYPE_TEST, KEY_TYPE_TEAM, SMS_TYPE @@ -47,8 +51,17 @@ def service_can_send_to_recipient(send_to, key_type, service): def validate_and_format_recipient(send_to, key_type, service, notification_type): service_can_send_to_recipient(send_to, key_type, service) + if notification_type == SMS_TYPE: - return validate_and_format_phone_number(number=send_to) + international_phone_info = get_international_phone_info(send_to) + + if international_phone_info.international and not service.can_send_international_sms: + raise BadRequestError(message="Cannot send to international mobile numbers") + + return validate_and_format_phone_number( + number=send_to, + international=international_phone_info.international + ) else: return validate_and_format_email_address(email_address=send_to) diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index 674d83ff8..72767c881 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -11,7 +11,7 @@ def validate(json_to_validate, schema): @format_checker.checks('phone_number', raises=InvalidPhoneError) def validate_schema_phone_number(instance): if instance is not None: - validate_phone_number(instance) + validate_phone_number(instance, international=True) return True @format_checker.checks('email_address', raises=InvalidEmailError) diff --git a/app/schemas.py b/app/schemas.py index 510d7e59e..2a42c7d15 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -324,13 +324,13 @@ class SmsNotificationSchema(NotificationSchema): @validates('to') def validate_to(self, value): try: - validate_phone_number(value) + validate_phone_number(value, international=True) except InvalidPhoneError as error: raise ValidationError('Invalid phone number: {}'.format(error)) @post_load def format_phone_number(self, item): - item['to'] = validate_and_format_phone_number(item['to']) + item['to'] = validate_and_format_phone_number(item['to'], international=True) return item diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 5f046edb0..1822e8734 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -27,6 +27,7 @@ def post_notification(notification_type): form = validate(request.get_json(), post_email_request) else: form = validate(request.get_json(), post_sms_request) + service = services_dao.dao_fetch_service_by_id(api_user.service_id) check_service_message_limit(api_user.key_type, service) form_send_to = form['phone_number'] if notification_type == SMS_TYPE else form['email_address'] @@ -41,7 +42,7 @@ def post_notification(notification_type): simulated = simulated_recipient(send_to, notification_type) notification = persist_notification(template_id=template.id, template_version=template.version, - recipient=send_to, + recipient=form_send_to, service=service, personalisation=form.get('personalisation', None), notification_type=notification_type, diff --git a/requirements.txt b/requirements.txt index a3dee4741..c90f21848 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@15.2.1#egg=notifications-utils==15.2.1 +git+https://github.com/alphagov/notifications-utils.git@16.1.0#egg=notifications-utils==16.1.0 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/tests/app/conftest.py b/tests/app/conftest.py index fab72ae7e..de69e9240 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -123,7 +123,8 @@ def sample_service( user=None, restricted=False, limit=1000, - email_from=None + email_from=None, + can_send_international_sms=False ): if user is None: user = create_user() @@ -136,6 +137,7 @@ def sample_service( 'email_from': email_from, 'created_by': user, 'letter_contact_block': 'London,\nSW1A 1AA', + 'can_send_international_sms': can_send_international_sms } service = Service.query.filter_by(name=service_name).first() if not service: diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 2894d75e9..80b08de4a 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -4,7 +4,7 @@ from collections import namedtuple from unittest.mock import ANY import pytest -from notifications_utils.recipients import validate_phone_number, format_phone_number +from notifications_utils.recipients import validate_and_format_phone_number import app from app import mmg_client @@ -69,7 +69,7 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist( ) mmg_client.send_sms.assert_called_once_with( - to=format_phone_number(validate_phone_number("+447234123123")), + to=validate_and_format_phone_number("+447234123123"), content="Sample service: Hello Jo\nHere is some HTML & entities", reference=str(db_notification.id), sender=None @@ -151,7 +151,7 @@ def test_send_sms_should_use_template_version_from_notification_not_latest( ) mmg_client.send_sms.assert_called_once_with( - to=format_phone_number(validate_phone_number("+447234123123")), + to=validate_and_format_phone_number("+447234123123"), content="Sample service: This is a template:\nwith a newline", reference=str(db_notification.id), sender=None @@ -254,7 +254,7 @@ def test_should_send_sms_sender_from_service_if_present( ) mmg_client.send_sms.assert_called_once_with( - to=format_phone_number(validate_phone_number("+447234123123")), + to=validate_and_format_phone_number("+447234123123"), content="This is a template:\nwith a newline", reference=str(db_notification.id), sender=sample_service.sms_sender diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index d6a4e9c31..8fe9f09b3 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -20,8 +20,8 @@ from tests.app.conftest import ( sample_email_template as create_sample_email_template, sample_template as create_sample_template, sample_service_whitelist as create_sample_service_whitelist, - sample_api_key as create_sample_api_key -) + sample_api_key as create_sample_api_key, + sample_service) from app.models import Template from app.errors import InvalidRequest @@ -1046,3 +1046,81 @@ def test_send_notification_uses_priority_queue_when_template_is_marked_as_priori assert response.status_code == 201 mocked.assert_called_once_with([notification_id], queue='priority') + + +def test_should_allow_store_original_number_on_sms_notification(notify_api, sample_template, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocker.patch('app.encryption.encrypt', return_value="something_encrypted") + + data = { + 'to': '+(44) 7700-900 855', + 'template': str(sample_template.id) + } + + auth_header = create_authorization_header(service_id=sample_template.service_id) + + response = client.post( + path='/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + response_data = json.loads(response.data)['data'] + notification_id = response_data['notification']['id'] + + mocked.assert_called_once_with([notification_id], queue='send-sms') + assert response.status_code == 201 + assert notification_id + notifications = Notification.query.all() + assert len(notifications) == 1 + assert '+(44) 7700-900 855' == notifications[0].to + + +def test_should_not_allow_international_number_on_sms_notification(notify_api, sample_template, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocker.patch('app.encryption.encrypt', return_value="something_encrypted") + + data = { + 'to': '20-12-1234-1234', + 'template': str(sample_template.id) + } + + auth_header = create_authorization_header(service_id=sample_template.service_id) + + response = client.post( + path='/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert not mocked.called + assert response.status_code == 400 + error_json = json.loads(response.get_data(as_text=True)) + assert error_json['result'] == 'error' + assert error_json['message']['to'][0] == 'Cannot send to international mobile numbers' + + +def test_should_allow_international_number_on_sms_notification(notify_api, notify_db, notify_db_session, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocker.patch('app.encryption.encrypt', return_value="something_encrypted") + + service = sample_service(notify_db, notify_db_session, can_send_international_sms=True) + template = create_sample_template(notify_db, notify_db_session, service=service) + + data = { + 'to': '20-12-1234-1234', + 'template': str(template.id) + } + + auth_header = create_authorization_header(service_id=service.id) + + response = client.post( + path='/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 201 diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index 951a43280..c663860b6 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -2,12 +2,14 @@ import pytest from freezegun import freeze_time import app +from app.models import KEY_TYPE_NORMAL from app.notifications.validators import ( check_service_message_limit, check_template_is_for_notification_type, check_template_is_active, service_can_send_to_recipient, - check_sms_content_char_count + check_sms_content_char_count, + validate_and_format_recipient ) from app.v2.errors import ( BadRequestError, @@ -262,3 +264,19 @@ def test_check_sms_content_char_count_fails(char_count, notify_api): assert e.value.message == 'Content for template has a character count greater than the limit of {}'.format( notify_api.config['SMS_CHAR_COUNT_LIMIT']) assert e.value.fields == [] + + +@pytest.mark.parametrize('key_type', ['test', 'normal']) +def test_rejects_api_calls_with_international_numbers_if_service_does_not_allow_int_sms(sample_service, key_type): + with pytest.raises(BadRequestError) as e: + validate_and_format_recipient('20-12-1234-1234', key_type, sample_service, 'sms') + assert e.value.status_code == 400 + assert e.value.message == 'Cannot send to international mobile numbers' + assert e.value.fields == [] + + +@pytest.mark.parametrize('key_type', ['test', 'normal']) +def test_allows_api_calls_with_international_numbers_if_service_does_allow_int_sms(sample_service, key_type): + sample_service.can_send_international_sms = True + result = validate_and_format_recipient('20-12-1234-1234', key_type, sample_service, 'sms') + assert result == '201212341234' diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index bf15dd8e3..23bc9ecb1 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -3,7 +3,7 @@ import pytest from flask import json from app.models import Notification from tests import create_authorization_header -from tests.app.conftest import sample_template as create_sample_template +from tests.app.conftest import sample_template as create_sample_template, sample_service @pytest.mark.parametrize("reference", [None, "reference_from_client"]) @@ -224,3 +224,71 @@ def test_send_notification_uses_priority_queue_when_template_is_marked_as_priori assert response.status_code == 201 mocked.assert_called_once_with([notification_id], queue='priority') + + +def test_post_sms_notification_returns_400_if_not_allowed_to_send_int_sms(client, sample_service, sample_template): + data = { + 'phone_number': '20-12-1234-1234', + 'template_id': sample_template.id + } + auth_header = create_authorization_header(service_id=sample_service.id) + + response = client.post( + path='/v2/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 400 + assert response.headers['Content-type'] == 'application/json' + + error_json = json.loads(response.get_data(as_text=True)) + assert error_json['status_code'] == 400 + assert error_json['errors'] == [ + {"error": "BadRequestError", "message": 'Cannot send to international mobile numbers'} + ] + + +def test_post_sms_notification_returns_201_if_allowed_to_send_int_sms(notify_db, notify_db_session, client): + + service = sample_service(notify_db, notify_db_session, can_send_international_sms=True) + template = create_sample_template(notify_db, notify_db_session, service=service) + + data = { + 'phone_number': '20-12-1234-1234', + 'template_id': template.id + } + auth_header = create_authorization_header(service_id=service.id) + + response = client.post( + path='/v2/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 201 + assert response.headers['Content-type'] == 'application/json' + + +def test_post_sms_should_persist_supplied_sms_number(notify_api, sample_template_with_placeholders, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + data = { + 'phone_number': '+(44) 77009-00855', + 'template_id': str(sample_template_with_placeholders.id), + 'personalisation': {' Name': 'Jo'} + } + + auth_header = create_authorization_header(service_id=sample_template_with_placeholders.service_id) + + response = client.post( + path='/v2/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + assert response.status_code == 201 + resp_json = json.loads(response.get_data(as_text=True)) + notifications = Notification.query.all() + assert len(notifications) == 1 + notification_id = notifications[0].id + assert '+(44) 77009-00855' == notifications[0].to + assert resp_json['id'] == str(notification_id) + assert mocked.called From 9afc07175ac9894601b76b4882d1cf5ffdb85ca2 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 26 Apr 2017 16:51:40 +0100 Subject: [PATCH 13/46] fix typo in downgrade --- migrations/versions/00xx_add_sent_notification_status.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/migrations/versions/00xx_add_sent_notification_status.py b/migrations/versions/00xx_add_sent_notification_status.py index 60a4d3a2e..2451cb64d 100644 --- a/migrations/versions/00xx_add_sent_notification_status.py +++ b/migrations/versions/00xx_add_sent_notification_status.py @@ -47,7 +47,7 @@ def downgrade(): op.execute('ALTER TYPE {enum} RENAME TO {tmp_name}'.format(enum=enum_name, tmp_name=tmp_name)) # Convert 'sent' template into 'sending' - update_str = "UPDATE TABLE {table} SET status='sending' where status='sent'" + update_str = "UPDATE {table} SET status='sending' where status='sent'" op.execute(update_str.format(table='notifications')) op.execute(update_str.format(table='notification_history')) From c8c6675ec6855b913e95c31c3c51bda7611b56f2 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 26 Apr 2017 16:54:24 +0100 Subject: [PATCH 14/46] set migration number --- ...ion_status.py => 0078_add_sent_notification_status.py} | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) rename migrations/versions/{00xx_add_sent_notification_status.py => 0078_add_sent_notification_status.py} (90%) diff --git a/migrations/versions/00xx_add_sent_notification_status.py b/migrations/versions/0078_add_sent_notification_status.py similarity index 90% rename from migrations/versions/00xx_add_sent_notification_status.py rename to migrations/versions/0078_add_sent_notification_status.py index 2451cb64d..a52526c60 100644 --- a/migrations/versions/00xx_add_sent_notification_status.py +++ b/migrations/versions/0078_add_sent_notification_status.py @@ -1,14 +1,14 @@ """empty message -Revision ID: 00xx_add_sent_notification_status -Revises: 0075_create_rates_table +Revision ID: 0078_sent_notification_status +Revises: 0077_add_intl_notification Create Date: 2017-04-24 16:55:20.731069 """ # revision identifiers, used by Alembic. -revision = '00xx_sent_notification_status' -down_revision = '0075_create_rates_table' +revision = '0078_sent_notification_status' +down_revision = '0077_add_intl_notification' from alembic import op import sqlalchemy as sa From 05145afcecb7ac7a455e9bae19ffbbbc2100c0a0 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Wed, 26 Apr 2017 17:26:06 +0100 Subject: [PATCH 15/46] Fix tests for checking simulated recipients --- .../test_process_notification.py | 59 ++++++++++++------- 1 file changed, 38 insertions(+), 21 deletions(-) diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 9e428a0ab..b581f79a5 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -9,10 +9,13 @@ from collections import namedtuple from app.models import Template, Notification, NotificationHistory from app.notifications import SendNotificationToQueueError -from app.notifications.process_notifications import (create_content_for_notification, - persist_notification, - send_notification_to_queue, - simulated_recipient) +from app.notifications.process_notifications import ( + create_content_for_notification, + persist_notification, + send_notification_to_queue, + simulated_recipient +) +from notifications_utils.recipients import validate_and_format_phone_number, validate_and_format_email_address from app.utils import cache_key_for_service_template_counter from app.v2.errors import BadRequestError from tests.app.conftest import sample_api_key as create_api_key @@ -255,22 +258,36 @@ def test_send_notification_to_queue_throws_exception_deletes_notification(sample assert NotificationHistory.query.count() == 0 -@pytest.mark.parametrize("to_address, notification_type, expected", - [("+447700900000", "sms", True), - ("+447700900111", "sms", True), - ("+447700900222", "sms", True), - ("simulate-delivered@notifications.service.gov.uk", "email", True), - ("simulate-delivered-2@notifications.service.gov.uk", "email", True), - ("simulate-delivered-3@notifications.service.gov.uk", "email", True), - ("07515896969", "sms", False), - ("valid_email@test.com", "email", False)]) +@pytest.mark.parametrize("to_address, notification_type, expected", [ + ("+447700900000", "sms", True), + ("+447700900111", "sms", True), + ("+447700900222", "sms", True), + ("07700900000", "sms", True), + ("7700900111", "sms", True), + ("simulate-delivered@notifications.service.gov.uk", "email", True), + ("simulate-delivered-2@notifications.service.gov.uk", "email", True), + ("simulate-delivered-3@notifications.service.gov.uk", "email", True), + ("07515896969", "sms", False), + ("valid_email@test.com", "email", False) +]) def test_simulated_recipient(notify_api, to_address, notification_type, expected): - # The values where the expected = 'research-mode' are listed in the config['SIMULATED_EMAIL_ADDRESSES'] - # and config['SIMULATED_SMS_NUMBERS']. These values should result in using the research mode queue. - # SIMULATED_EMAIL_ADDRESSES = ('simulate-delivered@notifications.service.gov.uk', - # 'simulate-delivered-2@notifications.service.gov.uk', - # 'simulate-delivered-2@notifications.service.gov.uk') - # SIMULATED_SMS_NUMBERS = ('+447700900000', '+447700900111', '+447700900222') + """ + The values where the expected = 'research-mode' are listed in the config['SIMULATED_EMAIL_ADDRESSES'] + and config['SIMULATED_SMS_NUMBERS']. These values should result in using the research mode queue. + SIMULATED_EMAIL_ADDRESSES = ( + 'simulate-delivered@notifications.service.gov.uk', + 'simulate-delivered-2@notifications.service.gov.uk', + 'simulate-delivered-2@notifications.service.gov.uk' + ) + SIMULATED_SMS_NUMBERS = ('+447700900000', '+447700900111', '+447700900222') + """ + formatted_address = None - actual = simulated_recipient(to_address, notification_type) - assert actual == expected + if notification_type == 'email': + formatted_address = validate_and_format_email_address(to_address) + else: + formatted_address = validate_and_format_phone_number(to_address) + + is_simulated_address = simulated_recipient(formatted_address, notification_type) + + assert is_simulated_address == expected From 2a1c3c248fab20f69e96b5454512b6601e9683ff Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 26 Apr 2017 21:21:00 +0100 Subject: [PATCH 16/46] Bumped utils --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index c90f21848..7994f02b8 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@16.1.0#egg=notifications-utils==16.1.0 +git+https://github.com/alphagov/notifications-utils.git@16.1.1#egg=notifications-utils==16.1.1 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 From 86a5445fb60f161ba32c044ecff9cbb0e72cb074 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 27 Apr 2017 09:26:12 +0100 Subject: [PATCH 17/46] Bumped utils version --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 7994f02b8..a757b27b6 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@16.1.1#egg=notifications-utils==16.1.1 +git+https://github.com/alphagov/notifications-utils.git@16.1.2#egg=notifications-utils==16.1.2 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 From 56a82bb593a925a92b8f6b879a3fcd7ddc73d515 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 27 Apr 2017 09:45:36 +0100 Subject: [PATCH 18/46] Bumped utils version --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index a757b27b6..3bc64fbc2 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@16.1.2#egg=notifications-utils==16.1.2 +git+https://github.com/alphagov/notifications-utils.git@16.1.3#egg=notifications-utils==16.1.3 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 From 83dc7c2bb7af8832a21ec397ae7d984d7c833870 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 27 Apr 2017 09:58:37 +0100 Subject: [PATCH 19/46] Little test updates --- .../rest/test_send_notification.py | 110 +++++++++--------- 1 file changed, 52 insertions(+), 58 deletions(-) diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index 8fe9f09b3..dbdea24b9 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -1048,79 +1048,73 @@ def test_send_notification_uses_priority_queue_when_template_is_marked_as_priori mocked.assert_called_once_with([notification_id], queue='priority') -def test_should_allow_store_original_number_on_sms_notification(notify_api, sample_template, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') - mocker.patch('app.encryption.encrypt', return_value="something_encrypted") +def test_should_allow_store_original_number_on_sms_notification(client, sample_template, mocker): + mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocker.patch('app.encryption.encrypt', return_value="something_encrypted") - data = { - 'to': '+(44) 7700-900 855', - 'template': str(sample_template.id) - } + data = { + 'to': '+(44) 7700-900 855', + 'template': str(sample_template.id) + } - auth_header = create_authorization_header(service_id=sample_template.service_id) + auth_header = create_authorization_header(service_id=sample_template.service_id) - response = client.post( - path='/notifications/sms', - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) + response = client.post( + path='/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) - response_data = json.loads(response.data)['data'] - notification_id = response_data['notification']['id'] + response_data = json.loads(response.data)['data'] + notification_id = response_data['notification']['id'] - mocked.assert_called_once_with([notification_id], queue='send-sms') - assert response.status_code == 201 - assert notification_id - notifications = Notification.query.all() - assert len(notifications) == 1 - assert '+(44) 7700-900 855' == notifications[0].to + mocked.assert_called_once_with([notification_id], queue='send-sms') + assert response.status_code == 201 + assert notification_id + notifications = Notification.query.all() + assert len(notifications) == 1 + assert '+(44) 7700-900 855' == notifications[0].to -def test_should_not_allow_international_number_on_sms_notification(notify_api, sample_template, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') - mocker.patch('app.encryption.encrypt', return_value="something_encrypted") +def test_should_not_allow_international_number_on_sms_notification(client, sample_template, mocker): + mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocker.patch('app.encryption.encrypt', return_value="something_encrypted") - data = { - 'to': '20-12-1234-1234', - 'template': str(sample_template.id) - } + data = { + 'to': '20-12-1234-1234', + 'template': str(sample_template.id) + } - auth_header = create_authorization_header(service_id=sample_template.service_id) + auth_header = create_authorization_header(service_id=sample_template.service_id) - response = client.post( - path='/notifications/sms', - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) + response = client.post( + path='/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) - assert not mocked.called - assert response.status_code == 400 - error_json = json.loads(response.get_data(as_text=True)) - assert error_json['result'] == 'error' - assert error_json['message']['to'][0] == 'Cannot send to international mobile numbers' + assert not mocked.called + assert response.status_code == 400 + error_json = json.loads(response.get_data(as_text=True)) + assert error_json['result'] == 'error' + assert error_json['message']['to'][0] == 'Cannot send to international mobile numbers' -def test_should_allow_international_number_on_sms_notification(notify_api, notify_db, notify_db_session, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') - mocker.patch('app.encryption.encrypt', return_value="something_encrypted") +def test_should_allow_international_number_on_sms_notification(client, notify_db, notify_db_session, mocker): + mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocker.patch('app.encryption.encrypt', return_value="something_encrypted") - service = sample_service(notify_db, notify_db_session, can_send_international_sms=True) - template = create_sample_template(notify_db, notify_db_session, service=service) + service = sample_service(notify_db, notify_db_session, can_send_international_sms=True) + template = create_sample_template(notify_db, notify_db_session, service=service) - data = { - 'to': '20-12-1234-1234', - 'template': str(template.id) - } + data = { + 'to': '20-12-1234-1234', + 'template': str(template.id) + } - auth_header = create_authorization_header(service_id=service.id) + auth_header = create_authorization_header(service_id=service.id) - response = client.post( - path='/notifications/sms', - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) + response = client.post( + path='/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) - assert response.status_code == 201 + assert response.status_code == 201 From d0978e52fb6e0d6aa399aed5cd43b358c56f01b9 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 27 Apr 2017 10:18:28 +0100 Subject: [PATCH 20/46] Use intl provider for int sms notifications (needs tests) --- app/delivery/send_to_providers.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index f6991c0b0..7ff67b50e 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -25,7 +25,7 @@ def send_sms_to_provider(notification): return if notification.status == 'created': - provider = provider_to_use(SMS_TYPE, notification.id) + provider = provider_to_use(SMS_TYPE, notification.id, notification.international) current_app.logger.info( "Starting sending SMS {} to provider at {}".format(notification.id, datetime.utcnow()) ) @@ -120,18 +120,22 @@ def update_notification(notification, provider): dao_update_notification(notification) -def provider_to_use(notification_type, notification_id): - active_providers_in_order = [ - provider for provider in get_provider_details_by_notification_type(notification_type) if provider.active - ] +def provider_to_use(notification_type, notification_id, international=False): + provider = None + # Default to MMG for international SMS + if notification_type == SMS_TYPE: + provider_id = 'mmg' if international else get_current_provider('sms') + provider = get_provider_details_by_identifier('mmg') + elif notification_type == EMAIL_TYPE: + provider = get_current_provider(notification_type) - if not active_providers_in_order: + if not provider: current_app.logger.error( "{} {} failed as no active providers".format(notification_type, notification_id) ) raise Exception("No active {} providers".format(notification_type)) - return clients.get_client_by_name_and_type(active_providers_in_order[0].identifier, notification_type) + return clients.get_client_by_name_and_type(provider.identifier, notification_type) def get_logo_url(base_url, branding_path, logo_file): From 84860b2a1de02febac162bb8820215fe0f559161 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 27 Apr 2017 10:41:31 +0100 Subject: [PATCH 21/46] Adds a debug line to try and debug jenkins --- tests/app/v2/notifications/test_post_notifications.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 23bc9ecb1..42a53192e 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -264,6 +264,7 @@ def test_post_sms_notification_returns_201_if_allowed_to_send_int_sms(notify_db, data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header]) + print(json.loads(response.get_data(as_text=True))) assert response.status_code == 201 assert response.headers['Content-type'] == 'application/json' From ba58c55c3b756e449c44265bee9a8d22808fb1c7 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 27 Apr 2017 12:13:58 +0100 Subject: [PATCH 22/46] Filter provider details by international flags --- app/dao/provider_details_dao.py | 15 ++++++++++----- tests/app/dao/test_provider_details_dao.py | 14 +++++++++++--- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/app/dao/provider_details_dao.py b/app/dao/provider_details_dao.py index ff4db1a93..1e2c11727 100644 --- a/app/dao/provider_details_dao.py +++ b/app/dao/provider_details_dao.py @@ -60,6 +60,7 @@ def dao_toggle_sms_provider(identifier): @transactional def dao_switch_sms_provider_to_provider_with_identifier(identifier): new_provider = get_provider_details_by_identifier(identifier) + if provider_is_inactive(new_provider): return @@ -69,7 +70,7 @@ def dao_switch_sms_provider_to_provider_with_identifier(identifier): providers_to_update = [] if conflicting_provider: - providers_to_update = switch_providers(conflicting_provider, new_provider) + switch_providers(conflicting_provider, new_provider) else: current_provider = get_current_provider('sms') if not provider_is_primary(current_provider, new_provider, identifier): @@ -79,10 +80,14 @@ def dao_switch_sms_provider_to_provider_with_identifier(identifier): dao_update_provider_details(provider) -def get_provider_details_by_notification_type(notification_type): - return ProviderDetails.query.filter_by( - notification_type=notification_type - ).order_by(asc(ProviderDetails.priority)).all() +def get_provider_details_by_notification_type(notification_type, supports_international=False): + + filters = [ProviderDetails.notification_type == notification_type] + + if supports_international: + filters.append(ProviderDetails.supports_international == supports_international) + + return ProviderDetails.query.filter(*filters).order_by(asc(ProviderDetails.priority)).all() @transactional diff --git a/tests/app/dao/test_provider_details_dao.py b/tests/app/dao/test_provider_details_dao.py index 5092bd1b9..21f74e3d9 100644 --- a/tests/app/dao/test_provider_details_dao.py +++ b/tests/app/dao/test_provider_details_dao.py @@ -35,16 +35,24 @@ def test_can_get_all_providers(restore_provider_details): assert len(get_provider_details()) == 5 -def test_can_get_sms_providers(restore_provider_details): +def test_can_get_sms_non_international_providers(restore_provider_details): sms_providers = get_provider_details_by_notification_type('sms') assert len(sms_providers) == 3 assert all('sms' == prov.notification_type for prov in sms_providers) + assert all(not prov.supports_international for prov in sms_providers) + + +def test_can_get_sms_international_providers(restore_provider_details): + sms_providers = get_provider_details_by_notification_type('sms', True) + assert len(sms_providers) == 1 + assert all('sms' == prov.notification_type for prov in sms_providers) + assert all(prov.supports_international for prov in sms_providers) def test_can_get_sms_providers_in_order_of_priority(restore_provider_details): - providers = get_provider_details_by_notification_type('sms') + providers = get_provider_details_by_notification_type('sms', False) - assert providers[0].priority < providers[1].priority < providers[2].priority + assert providers[0].priority < providers[1].priority def test_can_get_email_providers_in_order_of_priority(restore_provider_details): From 109b1727f2276d4fcfe406fe2ea7af451a4b6236 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 27 Apr 2017 12:14:22 +0100 Subject: [PATCH 23/46] Fish out the international provider if needed. --- app/delivery/send_to_providers.py | 21 +++---- tests/app/delivery/test_send_to_providers.py | 63 +++++++++++++++++++- 2 files changed, 68 insertions(+), 16 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 7ff67b50e..6634daa32 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -121,21 +121,17 @@ def update_notification(notification, provider): def provider_to_use(notification_type, notification_id, international=False): - provider = None - # Default to MMG for international SMS - if notification_type == SMS_TYPE: - provider_id = 'mmg' if international else get_current_provider('sms') - provider = get_provider_details_by_identifier('mmg') - elif notification_type == EMAIL_TYPE: - provider = get_current_provider(notification_type) + active_providers_in_order = [ + p for p in get_provider_details_by_notification_type(notification_type, international) if p.active + ] - if not provider: + if not active_providers_in_order: current_app.logger.error( "{} {} failed as no active providers".format(notification_type, notification_id) ) raise Exception("No active {} providers".format(notification_type)) - return clients.get_client_by_name_and_type(provider.identifier, notification_type) + return clients.get_client_by_name_and_type(active_providers_in_order[0].identifier, notification_type) def get_logo_url(base_url, branding_path, logo_file): @@ -151,11 +147,8 @@ def get_logo_url(base_url, branding_path, logo_file): base_url = parse.urlparse(base_url) netloc = base_url.netloc - if ( - base_url.netloc.startswith('localhost') or - # covers both preview and staging - 'notify.works' in base_url.netloc - ): + # covers both preview and staging + if base_url.netloc.startswith('localhost') or 'notify.works' in base_url.netloc: path = '/static' + branding_path + logo_file else: if base_url.netloc.startswith('www'): diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 2894d75e9..cbd384844 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -7,8 +7,9 @@ import pytest from notifications_utils.recipients import validate_phone_number, format_phone_number import app -from app import mmg_client +from app import mmg_client, firetext_client from app.dao import (provider_details_dao, notifications_dao) +from app.dao.provider_details_dao import dao_switch_sms_provider_to_provider_with_identifier from app.delivery import send_to_providers from app.models import ( Notification, @@ -18,7 +19,7 @@ from app.models import ( KEY_TYPE_TEAM, BRANDING_ORG, BRANDING_BOTH, -) + ProviderDetails) from tests.app.db import create_service, create_template, create_notification @@ -471,3 +472,61 @@ def test_should_update_billable_units_according_to_research_mode_and_key_type(no ) assert sample_notification.billable_units == billable_units + + +def test_should_send_sms_to_international_providers( + restore_provider_details, + sample_sms_template_with_html, + sample_user, + mocker +): + mocker.patch('app.provider_details.switch_providers.get_user_by_id', return_value=sample_user) + + dao_switch_sms_provider_to_provider_with_identifier('firetext') + + db_notification_uk = create_notification( + template=sample_sms_template_with_html, + to_field="+447234123999", + personalisation={"name": "Jo"}, + status='created', + international=False) + + db_notification_international = create_notification( + template=sample_sms_template_with_html, + to_field="+447234123111", + personalisation={"name": "Jo"}, + status='created', + international=True) + + mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.firetext_client.send_sms') + + send_to_providers.send_sms_to_provider( + db_notification_uk + ) + + firetext_client.send_sms.assert_called_once_with( + to=format_phone_number(validate_phone_number("+447234123999")), + content=ANY, + reference=str(db_notification_uk.id), + sender=None + ) + + send_to_providers.send_sms_to_provider( + db_notification_international + ) + + mmg_client.send_sms.assert_called_once_with( + to=format_phone_number(validate_phone_number("+447234123111")), + content=ANY, + reference=str(db_notification_international.id), + sender=None + ) + + notification_uk = Notification.query.filter_by(id=db_notification_uk.id).one() + notification_int = Notification.query.filter_by(id=db_notification_international.id).one() + + assert notification_uk.status == 'sending' + assert notification_uk.sent_by == 'firetext' + assert notification_int.status == 'sending' + assert notification_int.sent_by == 'mmg' From cdd3ad687c07a2cb3b2d5385466107273f418b84 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 27 Apr 2017 12:14:31 +0100 Subject: [PATCH 24/46] updated the fixture --- tests/app/db.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/app/db.py b/tests/app/db.py index 195a77ea7..38d5eca9d 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -72,7 +72,8 @@ def create_notification( api_key_id=None, key_type=KEY_TYPE_NORMAL, sent_by=None, - client_reference=None + client_reference=None, + international=False ): if created_at is None: created_at = datetime.utcnow() @@ -103,7 +104,8 @@ def create_notification( 'sent_by': sent_by, 'updated_at': updated_at, 'client_reference': client_reference, - 'job_row_number': job_row_number + 'job_row_number': job_row_number, + 'international': international } notification = Notification(**data) dao_create_notification(notification) From bedbd8e21fff019dec3f72c9dc4cdd43132faeab Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 27 Apr 2017 12:15:47 +0100 Subject: [PATCH 25/46] Removed unneeded test case --- tests/app/dao/test_provider_details_dao.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/app/dao/test_provider_details_dao.py b/tests/app/dao/test_provider_details_dao.py index 21f74e3d9..ed3f0e1f0 100644 --- a/tests/app/dao/test_provider_details_dao.py +++ b/tests/app/dao/test_provider_details_dao.py @@ -39,7 +39,6 @@ def test_can_get_sms_non_international_providers(restore_provider_details): sms_providers = get_provider_details_by_notification_type('sms') assert len(sms_providers) == 3 assert all('sms' == prov.notification_type for prov in sms_providers) - assert all(not prov.supports_international for prov in sms_providers) def test_can_get_sms_international_providers(restore_provider_details): From 99081488f14b3f78cef3bfd89394b295a8651362 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 27 Apr 2017 12:47:08 +0100 Subject: [PATCH 26/46] Mock out some SQS calls --- tests/app/v2/notifications/test_post_notifications.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 42a53192e..7a0b026c5 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -200,6 +200,9 @@ def test_send_notification_uses_priority_queue_when_template_is_marked_as_priori notification_type, key_send_to, send_to): + + mocker.patch('app.celery.provider_tasks.deliver_{}.apply_async'.format(notification_type)) + sample = create_sample_template( notify_db, notify_db_session, @@ -248,11 +251,13 @@ def test_post_sms_notification_returns_400_if_not_allowed_to_send_int_sms(client ] -def test_post_sms_notification_returns_201_if_allowed_to_send_int_sms(notify_db, notify_db_session, client): +def test_post_sms_notification_returns_201_if_allowed_to_send_int_sms(notify_db, notify_db_session, client, mocker): service = sample_service(notify_db, notify_db_session, can_send_international_sms=True) template = create_sample_template(notify_db, notify_db_session, service=service) + mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + data = { 'phone_number': '20-12-1234-1234', 'template_id': template.id From 252c3235d71a1c9e48c4a75af9b480c4ddfb3844 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 27 Apr 2017 12:41:10 +0100 Subject: [PATCH 27/46] Persist intl fields on notification --- app/notifications/process_notifications.py | 48 ++++++++----- .../test_process_notification.py | 67 ++++++++++++++++++- 2 files changed, 95 insertions(+), 20 deletions(-) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 033943511..e8510ba63 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -2,9 +2,13 @@ from datetime import datetime from flask import current_app +from notifications_utils.recipients import ( + get_international_phone_info, + validate_and_format_phone_number +) + from app import redis_store from app.celery import provider_tasks -from notifications_utils.recipients import validate_and_format_phone_number from notifications_utils.clients import redis from app.dao.notifications_dao import dao_create_notification, dao_delete_notifications_and_history_by_id from app.models import SMS_TYPE, Notification, KEY_TYPE_TEST, EMAIL_TYPE @@ -25,22 +29,23 @@ def check_placeholders(template_object): raise BadRequestError(fields=[{'template': message}], message=message) -def persist_notification(template_id, - template_version, - recipient, - service, - personalisation, - notification_type, - api_key_id, - key_type, - created_at=None, - job_id=None, - job_row_number=None, - reference=None, - client_reference=None, - notification_id=None, - simulated=False): - # if simulated create a Notification model to return but do not persist the Notification to the dB +def persist_notification( + template_id, + template_version, + recipient, + service, + personalisation, + notification_type, + api_key_id, + key_type, + created_at=None, + job_id=None, + job_row_number=None, + reference=None, + client_reference=None, + notification_id=None, + simulated=False +): notification = Notification( id=notification_id, template_id=template_id, @@ -58,6 +63,15 @@ def persist_notification(template_id, client_reference=client_reference, reference=reference ) + + if notification_type == SMS_TYPE: + formatted_recipient = validate_and_format_phone_number(recipient, international=True) + recipient_info = get_international_phone_info(formatted_recipient) + notification.international = recipient_info.international + notification.phone_prefix = recipient_info.country_prefix + notification.rate_multiplier = recipient_info.billable_units + + # if simulated create a Notification model to return but do not persist the Notification to the dB if not simulated: dao_create_notification(notification) if key_type != KEY_TYPE_TEST: diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 766e3f250..f697047e7 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -1,7 +1,7 @@ import datetime import uuid - import pytest + from boto3.exceptions import Boto3Error from sqlalchemy.exc import SQLAlchemyError from freezegun import freeze_time @@ -199,8 +199,8 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker) assert persisted_notification.client_reference == "ref from client" assert persisted_notification.reference is None assert persisted_notification.international is False - assert persisted_notification.phone_prefix is None - assert persisted_notification.rate_multiplier is None + assert persisted_notification.phone_prefix == '44' + assert persisted_notification.rate_multiplier == 1 @freeze_time("2016-01-01 11:09:00.061258") @@ -297,3 +297,64 @@ def test_simulated_recipient(notify_api, to_address, notification_type, expected is_simulated_address = simulated_recipient(formatted_address, notification_type) assert is_simulated_address == expected + + +@pytest.mark.parametrize('recipient, expected_international, expected_prefix, expected_units', [ + ('7900900123', False, '44', 1), # UK + ('+447900900123', False, '44', 1), # UK + ('07700900222', False, '44', 1), # UK + ('73122345678', True, '7', 1), # Russia + ('360623400400', True, '36', 3)] # Hungary +) +def test_persist_notification_with_international_info_stores_correct_info( + sample_job, + sample_api_key, + mocker, + recipient, + expected_international, + expected_prefix, + expected_units +): + persist_notification( + template_id=sample_job.template.id, + template_version=sample_job.template.version, + recipient=recipient, + service=sample_job.service, + personalisation=None, + 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=10, + client_reference="ref from client" + ) + persisted_notification = Notification.query.all()[0] + + assert persisted_notification.international is expected_international + assert persisted_notification.phone_prefix == expected_prefix + assert persisted_notification.rate_multiplier == expected_units + + +def test_persist_notification_with_international_info_does_not_store_for_email( + sample_job, + sample_api_key, + mocker +): + persist_notification( + template_id=sample_job.template.id, + template_version=sample_job.template.version, + recipient='foo@bar.com', + service=sample_job.service, + personalisation=None, + notification_type='email', + api_key_id=sample_api_key.id, + key_type=sample_api_key.key_type, + job_id=sample_job.id, + job_row_number=10, + client_reference="ref from client" + ) + persisted_notification = Notification.query.all()[0] + + assert persisted_notification.international is False + assert persisted_notification.phone_prefix is None + assert persisted_notification.rate_multiplier is None From 868e102f8a59047730cf56a4d4cc7ca1e8a26d80 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 27 Apr 2017 12:42:00 +0100 Subject: [PATCH 28/46] Make rate_multiplier on notification return non-decimal --- app/models.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models.py b/app/models.py index 44487c334..56979985f 100644 --- a/app/models.py +++ b/app/models.py @@ -670,7 +670,7 @@ class Notification(db.Model): international = db.Column(db.Boolean, nullable=False, default=False) phone_prefix = db.Column(db.String, nullable=True) - rate_multiplier = db.Column(db.Numeric(), nullable=True) + rate_multiplier = db.Column(db.Float(as_decimal=False), nullable=True) @property def personalisation(self): @@ -847,7 +847,7 @@ class NotificationHistory(db.Model, HistoryModel): international = db.Column(db.Boolean, nullable=False, default=False) phone_prefix = db.Column(db.String, nullable=True) - rate_multiplier = db.Column(db.Numeric(), nullable=True) + rate_multiplier = db.Column(db.Float(as_decimal=False), nullable=True) @classmethod def from_original(cls, notification): From 3de93cbcd0eabe309156954691786231ccd7aaa2 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 27 Apr 2017 12:42:45 +0100 Subject: [PATCH 29/46] Assume that we will only receive a UK number: * We currently don't validate the number so this test * will fail assuming an invalid number was passed. * Since we do validation on the front end, for now * we'll assume a valid number. This does need to be * looked at in future. --- tests/app/user/test_rest_verify.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/user/test_rest_verify.py b/tests/app/user/test_rest_verify.py index 1e08115a9..08f510bbd 100644 --- a/tests/app/user/test_rest_verify.py +++ b/tests/app/user/test_rest_verify.py @@ -230,7 +230,7 @@ def test_send_user_code_for_sms_with_optional_to_field(client, """ Tests POST endpoint /user//sms-code with optional to field """ - to_number = '+441119876757' + to_number = '+447119876757' mocked = mocker.patch('app.user.rest.create_secret_code', return_value='11111') mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') auth_header = create_authorization_header() From 164ba0c6b31d2829907dc553af575b5199c4e798 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 27 Apr 2017 13:50:51 +0100 Subject: [PATCH 30/46] as_decimal defaults to False --- app/models.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models.py b/app/models.py index 56979985f..82d9efb43 100644 --- a/app/models.py +++ b/app/models.py @@ -670,7 +670,7 @@ class Notification(db.Model): international = db.Column(db.Boolean, nullable=False, default=False) phone_prefix = db.Column(db.String, nullable=True) - rate_multiplier = db.Column(db.Float(as_decimal=False), nullable=True) + rate_multiplier = db.Column(db.Float(), nullable=True) @property def personalisation(self): @@ -847,7 +847,7 @@ class NotificationHistory(db.Model, HistoryModel): international = db.Column(db.Boolean, nullable=False, default=False) phone_prefix = db.Column(db.String, nullable=True) - rate_multiplier = db.Column(db.Float(as_decimal=False), nullable=True) + rate_multiplier = db.Column(db.Float(), nullable=True) @classmethod def from_original(cls, notification): From 698b20596ff31895fd434a89c5f0b610f850e848 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 27 Apr 2017 16:03:27 +0100 Subject: [PATCH 31/46] Remove references to old utils code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These methods don’t exist any more. Not doing this reformatting in the tests probably makes the tests more robust too 😬 --- tests/app/delivery/test_send_to_providers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 9852721c9..063d86e71 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -506,7 +506,7 @@ def test_should_send_sms_to_international_providers( ) firetext_client.send_sms.assert_called_once_with( - to=format_phone_number(validate_phone_number("+447234123999")), + to="447234123999", content=ANY, reference=str(db_notification_uk.id), sender=None @@ -517,7 +517,7 @@ def test_should_send_sms_to_international_providers( ) mmg_client.send_sms.assert_called_once_with( - to=format_phone_number(validate_phone_number("+447234123111")), + to="447234123111", content=ANY, reference=str(db_notification_international.id), sender=None From 349fb3529e6d0d8b7d251c81a8f4f993843e3d83 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 27 Apr 2017 13:22:17 +0100 Subject: [PATCH 32/46] Handle case for international SMS - use correct phone validator, and also set status correctly. This relies on some other code so this commit has placeholder failing tests to be populated when other PRs are merged. --- app/dao/notifications_dao.py | 4 ++-- app/delivery/send_to_providers.py | 14 +++++++++----- tests/app/dao/test_notification_dao.py | 8 ++++++++ tests/app/delivery/test_send_to_providers.py | 9 +++++++++ 4 files changed, 28 insertions(+), 7 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 29632e69d..6a759f261 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -26,8 +26,8 @@ from app.models import ( NOTIFICATION_TEMPORARY_FAILURE, NOTIFICATION_PERMANENT_FAILURE, KEY_TYPE_NORMAL, KEY_TYPE_TEST, - LETTER_TYPE -) + LETTER_TYPE, + NOTIFICATION_SENT) from app.dao.dao_utils import transactional from app.statsd_decorators import statsd diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 6634daa32..4eb2f61e9 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -15,7 +15,8 @@ from app.dao.provider_details_dao import ( ) from app.celery.research_mode_tasks import send_sms_response, send_email_response 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 +from app.models import SMS_TYPE, KEY_TYPE_TEST, BRANDING_ORG, EMAIL_TYPE, NOTIFICATION_TECHNICAL_FAILURE, \ + NOTIFICATION_SENT, NOTIFICATION_SENDING def send_sms_to_provider(notification): @@ -44,7 +45,7 @@ def send_sms_to_provider(notification): else: try: provider.send_sms( - to=validate_and_format_phone_number(notification.to), + to=validate_and_format_phone_number(notification.to, internationl=notification.international), content=str(template), reference=str(notification.id), sender=service.sms_sender @@ -54,7 +55,7 @@ def send_sms_to_provider(notification): raise e else: notification.billable_units = template.fragment_count - update_notification(notification, provider) + update_notification(notification, provider, notification.international) current_app.logger.info( "SMS {} sent to provider {} at {}".format(notification.id, provider.get_name(), notification.sent_at) @@ -113,10 +114,13 @@ def send_email_to_provider(notification): statsd_client.timing("email.total-time", delta_milliseconds) -def update_notification(notification, provider): +def update_notification(notification, provider, international=False): notification.sent_at = datetime.utcnow() notification.sent_by = provider.get_name() - notification.status = 'sending' + if international: + notification.status = NOTIFICATION_SENT + else: + notification.status = NOTIFICATION_SENDING dao_update_notification(notification) diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 99d1200ec..e43f5f5de 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -352,6 +352,14 @@ def test_should_update_status_by_id_if_created(notify_db, notify_db_session): assert updated.status == 'failed' +def test_should_not_update_status_by_reference_if_in_sent_status(notify_db, notify_db_session): + assert 1 == 2 + + +def test_should_not_update_status_by_id_if_in_sent_status(notify_db, notify_db_session): + assert 1 == 2 + + def test_should_not_update_status_by_reference_if_not_sending(notify_db, notify_db_session): notification = sample_notification(notify_db, notify_db_session, status='created', reference='reference') assert Notification.query.get(notification.id).status == 'created' diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 063d86e71..3965c2af3 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -474,6 +474,7 @@ def test_should_update_billable_units_according_to_research_mode_and_key_type(no assert sample_notification.billable_units == billable_units +<<<<<<< HEAD def test_should_send_sms_to_international_providers( restore_provider_details, sample_sms_template_with_html, @@ -530,3 +531,11 @@ def test_should_send_sms_to_international_providers( assert notification_uk.sent_by == 'firetext' assert notification_int.status == 'sending' assert notification_int.sent_by == 'mmg' + + +def test_should_send_international_sms_with_formatted_phone_number(): + assert 1 == 2 + + +def test_should_set_international_phone_number_to_sent_status(): + assert 1 == 2 From c5bd685cef12d7c296b865c23ee249ab47e01bb5 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 27 Apr 2017 16:55:39 +0100 Subject: [PATCH 33/46] Don't update sent notifications (dao) --- app/dao/notifications_dao.py | 18 ++++++++++++------ tests/app/dao/test_notification_dao.py | 21 +++++++++++++++++++-- 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 6a759f261..377b47e3c 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -175,11 +175,14 @@ def _update_notification_status(notification, status): def update_notification_status_by_id(notification_id, status): notification = Notification.query.with_lockmode("update").filter( Notification.id == notification_id, - or_(Notification.status == NOTIFICATION_CREATED, + or_( + Notification.status == NOTIFICATION_CREATED, Notification.status == NOTIFICATION_SENDING, - Notification.status == NOTIFICATION_PENDING)).first() + Notification.status == NOTIFICATION_PENDING, + Notification.status == NOTIFICATION_SENT + )).first() - if not notification: + if not notification or notification.status == NOTIFICATION_SENT: return None return _update_notification_status( @@ -193,10 +196,13 @@ def update_notification_status_by_id(notification_id, status): def update_notification_status_by_reference(reference, status): notification = Notification.query.filter( Notification.reference == reference, - or_(Notification.status == NOTIFICATION_SENDING, - Notification.status == NOTIFICATION_PENDING)).first() + or_( + Notification.status == NOTIFICATION_SENDING, + Notification.status == NOTIFICATION_PENDING, + Notification.status == NOTIFICATION_SENT + )).first() - if not notification: + if not notification or notification.status == NOTIFICATION_SENT: return None return _update_notification_status( diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index e43f5f5de..8f67d2335 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -15,6 +15,7 @@ from app.models import ( TemplateStatistics, NOTIFICATION_STATUS_TYPES, NOTIFICATION_STATUS_TYPES_FAILED, + NOTIFICATION_SENT, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST @@ -353,11 +354,27 @@ def test_should_update_status_by_id_if_created(notify_db, notify_db_session): def test_should_not_update_status_by_reference_if_in_sent_status(notify_db, notify_db_session): - assert 1 == 2 + notification = sample_notification( + notify_db, + notify_db_session, + status=NOTIFICATION_SENT, + reference='foo' + ) + + update_notification_status_by_reference('foo', 'failed') + assert Notification.query.get(notification.id).status == NOTIFICATION_SENT def test_should_not_update_status_by_id_if_in_sent_status(notify_db, notify_db_session): - assert 1 == 2 + notification = sample_notification( + notify_db, + notify_db_session, + status=NOTIFICATION_SENT + ) + + update_notification_status_by_id(notification.id, 'failed') + + assert Notification.query.get(notification.id).status == NOTIFICATION_SENT def test_should_not_update_status_by_reference_if_not_sending(notify_db, notify_db_session): From e450f15b2b7b1cbe3d739920e7c5b329e7bc03ed Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 27 Apr 2017 16:57:04 +0100 Subject: [PATCH 34/46] Fix typo and update test now that we expect sent for international --- app/delivery/send_to_providers.py | 2 +- tests/app/delivery/test_send_to_providers.py | 45 +++++++++++++++++--- 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 4eb2f61e9..f9ca7861f 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -45,7 +45,7 @@ def send_sms_to_provider(notification): else: try: provider.send_sms( - to=validate_and_format_phone_number(notification.to, internationl=notification.international), + to=validate_and_format_phone_number(notification.to, international=notification.international), content=str(template), reference=str(notification.id), sender=service.sms_sender diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 3965c2af3..0e35ad413 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -474,7 +474,6 @@ def test_should_update_billable_units_according_to_research_mode_and_key_type(no assert sample_notification.billable_units == billable_units -<<<<<<< HEAD def test_should_send_sms_to_international_providers( restore_provider_details, sample_sms_template_with_html, @@ -529,13 +528,47 @@ def test_should_send_sms_to_international_providers( assert notification_uk.status == 'sending' assert notification_uk.sent_by == 'firetext' - assert notification_int.status == 'sending' + assert notification_int.status == 'sent' assert notification_int.sent_by == 'mmg' -def test_should_send_international_sms_with_formatted_phone_number(): - assert 1 == 2 +def test_should_send_international_sms_with_formatted_phone_number( + notify_db, + sample_template, + mocker +): + notification = create_notification( + template=sample_template, + to_field="+6011-17224412", + international=True + ) + + send_notification_mock = mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.delivery.send_to_providers.send_sms_response') + + send_to_providers.send_sms_to_provider( + notification + ) + + assert send_notification_mock.called is True -def test_should_set_international_phone_number_to_sent_status(): - assert 1 == 2 +def test_should_set_international_phone_number_to_sent_status( + notify_db, + sample_template, + mocker +): + notification = create_notification( + template=sample_template, + to_field="+6011-17224412", + international=True + ) + + mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.delivery.send_to_providers.send_sms_response') + + send_to_providers.send_sms_to_provider( + notification + ) + + assert notification.status == 'sent' From b5d4acb75853a680d2d1052088b9ad28b8a80e62 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 27 Apr 2017 16:58:00 +0100 Subject: [PATCH 35/46] Make message more accurate --- app/celery/provider_tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index b0df268b3..e00591f09 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -44,7 +44,7 @@ def deliver_sms(self, notification_id): except Exception as e: try: current_app.logger.exception( - "RETRY: SMS notification {} failed".format(notification_id) + "SMS notification delivery for id: {} failed".format(notification_id) ) self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries)) except self.MaxRetriesExceededError: From 6dc336ad6c3834729bc78474a980bd3b12627726 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 26 Apr 2017 14:16:47 +0100 Subject: [PATCH 36/46] Created new queries to return the rate with the sum of billable units for the year totals. Once we have the new columns in notifications table, the query will need to include the rate multiplier and if the number is international. The monthly billing query will be built next. --- app/dao/date_util.py | 18 ++++ app/dao/notification_usage_dao.py | 69 ++++++++++++++ app/dao/notifications_dao.py | 21 +---- app/dao/rates_dao.py | 11 --- app/dao/services_dao.py | 9 +- tests/app/dao/test_date_utils.py | 13 +++ tests/app/dao/test_notification_dao.py | 18 +--- tests/app/dao/test_notification_usage_dao.py | 97 ++++++++++++++++++++ tests/app/dao/test_rates_dao.py | 18 ---- tests/conftest.py | 3 +- 10 files changed, 210 insertions(+), 67 deletions(-) create mode 100644 app/dao/date_util.py create mode 100644 app/dao/notification_usage_dao.py delete mode 100644 app/dao/rates_dao.py create mode 100644 tests/app/dao/test_date_utils.py create mode 100644 tests/app/dao/test_notification_usage_dao.py delete mode 100644 tests/app/dao/test_rates_dao.py diff --git a/app/dao/date_util.py b/app/dao/date_util.py new file mode 100644 index 000000000..92932bb4c --- /dev/null +++ b/app/dao/date_util.py @@ -0,0 +1,18 @@ +from datetime import datetime + +import pytz + + +def get_financial_year(year): + return get_april_fools(year), get_april_fools(year + 1) + + +def get_april_fools(year): + """ + This function converts the start of the financial year April 1, 00:00 as BST (British Standard Time) to UTC, + the tzinfo is lastly removed from the datetime becasue the database stores the timestamps without timezone. + :param year: the year to calculate the April 1, 00:00 BST for + :return: the datetime of April 1 for the given year, for example 2016 = 2016-03-31 23:00:00 + """ + return pytz.timezone('Europe/London').localize(datetime(year, 4, 1, 0, 0, 0)).astimezone(pytz.UTC).replace( + tzinfo=None) diff --git a/app/dao/notification_usage_dao.py b/app/dao/notification_usage_dao.py new file mode 100644 index 000000000..fbd53681c --- /dev/null +++ b/app/dao/notification_usage_dao.py @@ -0,0 +1,69 @@ +from decimal import Decimal + +from sqlalchemy import func + +from app import db +from app.dao.date_util import get_financial_year +from app.models import (NotificationHistory, + Rate, + NOTIFICATION_STATUS_TYPES_BILLABLE, + KEY_TYPE_TEST, + SMS_TYPE, + EMAIL_TYPE) +from app.statsd_decorators import statsd + + +@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) + + result = [] + for r, n in zip(rates, rates[1:]): + result.append( + sms_billing_data_query(str(r.rate), service_id, r.valid_from, n.valid_from)) + + result.append(sms_billing_data_query(str(rates[-1].rate), service_id, rates[-1].valid_from, end_date)) + + result.append(email_billing_data_query(service_id, start_date, end_date)) + + return result + + +def billing_data_filter(notification_type, start_date, end_date, service_id): + return [NotificationHistory.notification_type == notification_type, + NotificationHistory.created_at >= start_date, + NotificationHistory.created_at < end_date, + NotificationHistory.service_id == service_id, + NotificationHistory.status.in_(NOTIFICATION_STATUS_TYPES_BILLABLE), + NotificationHistory.key_type != KEY_TYPE_TEST + ] + + +def email_billing_data_query(service_id, start_date, end_date): + result = db.session.query(func.count(NotificationHistory.id), + NotificationHistory.notification_type, + "0" + ).filter(*billing_data_filter(EMAIL_TYPE, start_date, end_date, service_id) + ).group_by(NotificationHistory.notification_type).first() + if not result: + return 0, EMAIL_TYPE, Decimal("0") + else: + return result + + +def sms_billing_data_query(rate, service_id, start_date, end_date): + result = db.session.query(func.sum(NotificationHistory.billable_units), + NotificationHistory.notification_type, + rate + ).filter(*billing_data_filter(SMS_TYPE, start_date, end_date, service_id) + ).group_by(NotificationHistory.notification_type).first() + if not result: + return 0, SMS_TYPE, Decimal("0") + else: + return result + + +def get_rates_for_year(start_date, end_date, notification_type): + return Rate.query.filter(Rate.valid_from >= start_date, Rate.valid_from < end_date, + Rate.notification_type == notification_type).order_by(Rate.valid_from).all() diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 377b47e3c..6bb5337f8 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -1,5 +1,4 @@ import functools -import pytz from datetime import ( datetime, timedelta, @@ -12,6 +11,7 @@ from sqlalchemy.orm import joinedload from app import db, create_uuid from app.dao import days_ago +from app.dao.date_util import get_financial_year from app.models import ( Service, Notification, @@ -243,13 +243,15 @@ def get_notifications_for_job(service_id, job_id, filter_dict=None, page=1, page def get_notification_billable_unit_count_per_month(service_id, year): month = get_london_month_from_utc_column(NotificationHistory.created_at) + start_date, end_date = get_financial_year(year) notifications = db.session.query( month, func.sum(NotificationHistory.billable_units) ).filter( NotificationHistory.billable_units != 0, NotificationHistory.service_id == service_id, - NotificationHistory.created_at.between(*get_financial_year(year)), + NotificationHistory.created_at >= start_date, + NotificationHistory.created_at < end_date ).group_by( month ).order_by( @@ -410,21 +412,6 @@ def dao_timeout_notifications(timeout_period_in_seconds): return updated -def get_financial_year(year): - return get_april_fools(year), get_april_fools(year + 1) - - -def get_april_fools(year): - """ - This function converts the start of the financial year April 1, 00:00 as BST (British Standard Time) to UTC, - the tzinfo is lastly removed from the datetime becasue the database stores the timestamps without timezone. - :param year: the year to calculate the April 1, 00:00 BST for - :return: the datetime of April 1 for the given year, for example 2016 = 2016-03-31 23:00:00 - """ - return pytz.timezone('Europe/London').localize(datetime(year, 4, 1, 0, 0, 0)).astimezone(pytz.UTC).replace( - tzinfo=None) - - def get_total_sent_notifications_in_date_range(start_date, end_date, notification_type): result = db.session.query( func.count(NotificationHistory.id).label('count') diff --git a/app/dao/rates_dao.py b/app/dao/rates_dao.py deleted file mode 100644 index 47ed83a4e..000000000 --- a/app/dao/rates_dao.py +++ /dev/null @@ -1,11 +0,0 @@ -from sqlalchemy import desc - -from app import db -from app.models import Rate - - -def get_rate_for_type_and_date(notification_type, date_sent): - return db.session.query(Rate).filter(Rate.notification_type == notification_type, - Rate.valid_from <= date_sent - ).order_by(Rate.valid_from.desc() - ).limit(1).first() diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 928b3188b..fe25469da 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -229,6 +229,7 @@ def _stats_for_service_query(service_id): def dao_fetch_monthly_historical_stats_by_template_for_service(service_id, year): month = get_london_month_from_utc_column(NotificationHistory.created_at) + start_date, end_date = get_financial_year(year) sq = db.session.query( NotificationHistory.template_id, NotificationHistory.status, @@ -236,7 +237,9 @@ def dao_fetch_monthly_historical_stats_by_template_for_service(service_id, year) func.count().label('count') ).filter( NotificationHistory.service_id == service_id, - NotificationHistory.created_at.between(*get_financial_year(year)) + NotificationHistory.created_at >= start_date, + NotificationHistory.created_at < end_date + ).group_by( month, NotificationHistory.template_id, @@ -262,6 +265,7 @@ def dao_fetch_monthly_historical_stats_by_template_for_service(service_id, year) def dao_fetch_monthly_historical_stats_for_service(service_id, year): month = get_london_month_from_utc_column(NotificationHistory.created_at) + start_date, end_date = get_financial_year(year) rows = db.session.query( NotificationHistory.notification_type, NotificationHistory.status, @@ -269,7 +273,8 @@ def dao_fetch_monthly_historical_stats_for_service(service_id, year): func.count(NotificationHistory.id).label('count') ).filter( NotificationHistory.service_id == service_id, - NotificationHistory.created_at.between(*get_financial_year(year)), + NotificationHistory.created_at >= start_date, + NotificationHistory.created_at < end_date ).group_by( NotificationHistory.notification_type, NotificationHistory.status, diff --git a/tests/app/dao/test_date_utils.py b/tests/app/dao/test_date_utils.py new file mode 100644 index 000000000..d6be85da2 --- /dev/null +++ b/tests/app/dao/test_date_utils.py @@ -0,0 +1,13 @@ +from app.dao.date_util import get_financial_year, get_april_fools + + +def test_get_financial_year(): + start, end = get_financial_year(2000) + assert str(start) == '2000-03-31 23:00:00' + assert str(end) == '2001-03-31 23:00:00' + + +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 diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 8f67d2335..b5461e036 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -3,7 +3,6 @@ import uuid from functools import partial import pytest - from freezegun import freeze_time from sqlalchemy.exc import SQLAlchemyError, IntegrityError @@ -18,8 +17,7 @@ from app.models import ( NOTIFICATION_SENT, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, - KEY_TYPE_TEST -) + KEY_TYPE_TEST) from app.dao.notifications_dao import ( dao_create_notification, @@ -40,8 +38,6 @@ from app.dao.notifications_dao import ( update_notification_status_by_reference, dao_delete_notifications_and_history_by_id, dao_timeout_notifications, - get_financial_year, - get_april_fools, is_delivery_slow_for_provider, dao_update_notifications_sent_to_dvla) @@ -1359,18 +1355,6 @@ def test_should_exclude_test_key_notifications_by_default( assert len(all_notifications) == 1 -def test_get_financial_year(): - start, end = get_financial_year(2000) - assert str(start) == '2000-03-31 23:00:00' - assert str(end) == '2001-03-31 23:00:00' - - -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('notification_type', ['sms', 'email']) def test_get_total_sent_notifications_in_date_range_returns_only_in_date_range( notify_db, diff --git a/tests/app/dao/test_notification_usage_dao.py b/tests/app/dao/test_notification_usage_dao.py new file mode 100644 index 000000000..39525338b --- /dev/null +++ b/tests/app/dao/test_notification_usage_dao.py @@ -0,0 +1,97 @@ +import uuid +from datetime import datetime + +from decimal import Decimal + +from app.dao.notification_usage_dao import (get_rates_for_year, get_yearly_billing_data) +from app.models import Rate +from tests.app.db import create_notification + + +def test_get_rates_for_year(notify_db, notify_db_session): + set_up_rate(notify_db, datetime(2016, 4, 1), 1.50) + set_up_rate(notify_db, datetime(2017, 6, 1), 1.75) + rates = get_rates_for_year(datetime(2016, 4, 1), datetime(2017, 3, 31), 'sms') + assert len(rates) == 1 + assert datetime.strftime(rates[0].valid_from, '%Y-%m-%d %H:%M:%S') == "2016-04-01 00:00:00" + assert rates[0].rate == Decimal("1.50") + rates = get_rates_for_year(datetime(2017, 4, 1), datetime(2018, 3, 31), 'sms') + assert len(rates) == 1 + assert datetime.strftime(rates[0].valid_from, '%Y-%m-%d %H:%M:%S') == "2017-06-01 00:00:00" + assert rates[0].rate == Decimal("1.75") + + +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), 1.40) + set_up_rate(notify_db, datetime(2016, 6, 1), 1.58) + # previous year + create_notification(template=sample_template, created_at=datetime(2016, 3, 31), sent_at=datetime(2016, 3, 31), + status='sending', billable_units=1) + # current year + create_notification(template=sample_template, created_at=datetime(2016, 4, 2), sent_at=datetime(2016, 4, 2), + status='sending', billable_units=1) + create_notification(template=sample_template, created_at=datetime(2016, 5, 18), sent_at=datetime(2016, 5, 18), + status='sending', billable_units=2) + create_notification(template=sample_template, created_at=datetime(2016, 7, 22), sent_at=datetime(2016, 7, 22), + status='sending', billable_units=3) + create_notification(template=sample_template, created_at=datetime(2016, 9, 15), sent_at=datetime(2016, 9, 15), + status='sending', billable_units=4) + create_notification(template=sample_template, created_at=datetime(2017, 3, 31), sent_at=datetime(2017, 3, 31), + status='sending', billable_units=5) + create_notification(template=sample_email_template, created_at=datetime(2016, 9, 15), sent_at=datetime(2016, 9, 15), + status='sending', billable_units=0) + create_notification(template=sample_email_template, created_at=datetime(2017, 3, 31), sent_at=datetime(2017, 3, 31), + status='sending', billable_units=0) + # next year + create_notification(template=sample_template, created_at=datetime(2017, 4, 1), sent_at=datetime(2017, 4, 1), + status='sending', billable_units=6) + results = get_yearly_billing_data(sample_template.service_id, 2016) + assert len(results) == 3 + assert results[0] == (3, 'sms', Decimal('1.4')) + assert results[1] == (12, 'sms', Decimal('1.58')) + assert results[2] == (2, 'email', Decimal("0")) + + +def test_get_yearly_billing_data_with_one_rate(notify_db, notify_db_session, sample_template): + set_up_rate(notify_db, datetime(2016, 4, 1), 1.40) + # previous year + create_notification(template=sample_template, created_at=datetime(2016, 3, 31), sent_at=datetime(2016, 3, 31), + status='sending', billable_units=1) + # current year + create_notification(template=sample_template, created_at=datetime(2016, 4, 2), sent_at=datetime(2016, 4, 2), + status='sending', billable_units=1) + create_notification(template=sample_template, created_at=datetime(2016, 5, 18), sent_at=datetime(2016, 5, 18), + status='sending', billable_units=2) + create_notification(template=sample_template, created_at=datetime(2016, 7, 22), sent_at=datetime(2016, 7, 22), + status='sending', billable_units=3) + create_notification(template=sample_template, created_at=datetime(2016, 9, 15), sent_at=datetime(2016, 9, 15), + status='sending', billable_units=4) + create_notification(template=sample_template, created_at=datetime(2017, 3, 31, 22, 59, 59), + sent_at=datetime(2017, 3, 31), status='sending', billable_units=5) + # next year + create_notification(template=sample_template, created_at=datetime(2017, 3, 31, 23, 00, 00), + sent_at=datetime(2017, 3, 31), status='sending', billable_units=6) + create_notification(template=sample_template, created_at=datetime(2017, 4, 1), sent_at=datetime(2017, 4, 1), + status='sending', billable_units=7) + results = get_yearly_billing_data(sample_template.service_id, 2016) + assert len(results) == 2 + assert results[0] == (15, 'sms', Decimal('1.4')) + assert results[1] == (0, 'email', Decimal('0')) + + +def test_get_yearly_billing_data_with_no_sms_notifications(notify_db, notify_db_session, sample_email_template): + set_up_rate(notify_db, datetime(2016, 4, 1), 1.40) + create_notification(template=sample_email_template, created_at=datetime(2016, 7, 31), sent_at=datetime(2016, 3, 31), + status='sending', billable_units=0) + create_notification(template=sample_email_template, created_at=datetime(2016, 10, 2), sent_at=datetime(2016, 4, 2), + status='sending', billable_units=0) + + results = get_yearly_billing_data(sample_email_template.service_id, 2016) + assert len(results) == 2 + assert results[0] == (0, 'sms', Decimal('0')) + assert results[1] == (2, 'email', Decimal('0')) + + +def set_up_rate(notify_db, start_date, value): + rate = Rate(id=uuid.uuid4(), valid_from=start_date, rate=value, notification_type='sms') + notify_db.session.add(rate) diff --git a/tests/app/dao/test_rates_dao.py b/tests/app/dao/test_rates_dao.py deleted file mode 100644 index 33f68a671..000000000 --- a/tests/app/dao/test_rates_dao.py +++ /dev/null @@ -1,18 +0,0 @@ -from datetime import datetime - -from decimal import Decimal - -from app.dao.rates_dao import get_rate_for_type_and_date - - -def test_get_rate_for_type_and_date(notify_db): - rate = get_rate_for_type_and_date('sms', datetime.utcnow()) - assert rate.rate == Decimal("1.58") - - rate = get_rate_for_type_and_date('sms', datetime(2016, 6, 1)) - assert rate.rate == Decimal("1.65") - - -def test_get_rate_for_type_and_date_early_date(notify_db): - rate = get_rate_for_type_and_date('sms', datetime(2014, 6, 1)) - assert not rate diff --git a/tests/conftest.py b/tests/conftest.py index c7ec7ee3b..b08059725 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -76,8 +76,7 @@ def notify_db_session(notify_db): "job_status", "provider_details_history", "template_process_type", - "dvla_organisation", - "rates"]: + "dvla_organisation"]: notify_db.engine.execute(tbl.delete()) notify_db.session.commit() From 4c37c8bdbb2ae74f0ae9b5b44d5b39930e785dd9 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 26 Apr 2017 15:31:25 +0100 Subject: [PATCH 37/46] New query to get billing data per month. --- app/dao/notification_usage_dao.py | 92 ++++++++++++++++---- tests/app/dao/test_notification_usage_dao.py | 66 +++++++++++++- 2 files changed, 140 insertions(+), 18 deletions(-) diff --git a/app/dao/notification_usage_dao.py b/app/dao/notification_usage_dao.py index fbd53681c..b1db43329 100644 --- a/app/dao/notification_usage_dao.py +++ b/app/dao/notification_usage_dao.py @@ -1,3 +1,4 @@ +from datetime import datetime from decimal import Decimal from sqlalchemy import func @@ -11,6 +12,7 @@ from app.models import (NotificationHistory, SMS_TYPE, EMAIL_TYPE) from app.statsd_decorators import statsd +from app.utils import get_london_month_from_utc_column @statsd(namespace="dao") @@ -31,21 +33,26 @@ def get_yearly_billing_data(service_id, year): def billing_data_filter(notification_type, start_date, end_date, service_id): - return [NotificationHistory.notification_type == notification_type, - NotificationHistory.created_at >= start_date, - NotificationHistory.created_at < end_date, - NotificationHistory.service_id == service_id, - NotificationHistory.status.in_(NOTIFICATION_STATUS_TYPES_BILLABLE), - NotificationHistory.key_type != KEY_TYPE_TEST - ] + return [ + NotificationHistory.notification_type == notification_type, + NotificationHistory.created_at >= start_date, + NotificationHistory.created_at < end_date, + NotificationHistory.service_id == service_id, + NotificationHistory.status.in_(NOTIFICATION_STATUS_TYPES_BILLABLE), + NotificationHistory.key_type != KEY_TYPE_TEST + ] def email_billing_data_query(service_id, start_date, end_date): - result = db.session.query(func.count(NotificationHistory.id), - NotificationHistory.notification_type, - "0" - ).filter(*billing_data_filter(EMAIL_TYPE, start_date, end_date, service_id) - ).group_by(NotificationHistory.notification_type).first() + result = db.session.query( + func.count(NotificationHistory.id), + NotificationHistory.notification_type, + "0" + ).filter( + *billing_data_filter(EMAIL_TYPE, start_date, end_date, service_id) + ).group_by( + NotificationHistory.notification_type + ).first() if not result: return 0, EMAIL_TYPE, Decimal("0") else: @@ -53,11 +60,15 @@ def email_billing_data_query(service_id, start_date, end_date): def sms_billing_data_query(rate, service_id, start_date, end_date): - result = db.session.query(func.sum(NotificationHistory.billable_units), - NotificationHistory.notification_type, - rate - ).filter(*billing_data_filter(SMS_TYPE, start_date, end_date, service_id) - ).group_by(NotificationHistory.notification_type).first() + result = db.session.query( + func.sum(NotificationHistory.billable_units), + NotificationHistory.notification_type, + rate + ).filter( + *billing_data_filter(SMS_TYPE, start_date, end_date, service_id) + ).group_by( + NotificationHistory.notification_type + ).first() if not result: return 0, SMS_TYPE, Decimal("0") else: @@ -67,3 +78,50 @@ def sms_billing_data_query(rate, service_id, start_date, end_date): def get_rates_for_year(start_date, end_date, notification_type): return Rate.query.filter(Rate.valid_from >= start_date, Rate.valid_from < end_date, Rate.notification_type == notification_type).order_by(Rate.valid_from).all() + + +def sms_billing_data_per_month_query(rate, service_id, start_date, end_date): + month = get_london_month_from_utc_column(NotificationHistory.created_at) + return db.session.query( + month, + func.sum(NotificationHistory.billable_units), + NotificationHistory.notification_type, + rate + ).filter( + *billing_data_filter(SMS_TYPE, start_date, end_date, service_id) + ).group_by( + NotificationHistory.notification_type, month + ).order_by( + month + ).all() + + +def email_billing_data_per_month_query(rate, service_id, start_date, end_date): + month = get_london_month_from_utc_column(NotificationHistory.created_at) + return db.session.query( + month, + func.count(NotificationHistory.id), + NotificationHistory.notification_type, + rate + ).filter( + *billing_data_filter(EMAIL_TYPE, start_date, end_date, service_id) + ).group_by( + NotificationHistory.notification_type, month + ).order_by( + month + ).all() + + +@statsd(namespace="dao") +def get_notification_billing_data_per_month(service_id, year): + start_date, end_date = get_financial_year(year) + rates = get_rates_for_year(start_date, end_date, SMS_TYPE) + + result = [] + for r, n in zip(rates, rates[1:]): + result.extend(sms_billing_data_per_month_query(str(r.rate), service_id, r.valid_from, n.valid_from)) + result.extend(sms_billing_data_per_month_query(str(rates[-1].rate), service_id, rates[-1].valid_from, end_date)) + + result.extend(email_billing_data_per_month_query("0", service_id, start_date, end_date)) + + return [(datetime.strftime(x[0], "%B"), x[1:]) for x in result] diff --git a/tests/app/dao/test_notification_usage_dao.py b/tests/app/dao/test_notification_usage_dao.py index 39525338b..0386d5f85 100644 --- a/tests/app/dao/test_notification_usage_dao.py +++ b/tests/app/dao/test_notification_usage_dao.py @@ -3,7 +3,8 @@ from datetime import datetime from decimal import Decimal -from app.dao.notification_usage_dao import (get_rates_for_year, get_yearly_billing_data) +from app.dao.notification_usage_dao import (get_rates_for_year, get_yearly_billing_data, + get_notification_billing_data_per_month) from app.models import Rate from tests.app.db import create_notification @@ -92,6 +93,69 @@ def test_get_yearly_billing_data_with_no_sms_notifications(notify_db, notify_db_ assert results[1] == (2, 'email', Decimal('0')) +def test_get_notification_billing_data_per_month(notify_db, notify_db_session, sample_template, sample_email_template): + set_up_rate(notify_db, datetime(2016, 4, 1), 1.40) + # previous year + create_notification(template=sample_template, created_at=datetime(2016, 3, 31), sent_at=datetime(2016, 3, 31), + status='sending', billable_units=1) + # current year + create_notification(template=sample_template, created_at=datetime(2016, 4, 2), sent_at=datetime(2016, 4, 2), + status='sending', billable_units=1) + create_notification(template=sample_template, created_at=datetime(2016, 5, 18), sent_at=datetime(2016, 5, 18), + status='sending', billable_units=2) + create_notification(template=sample_template, created_at=datetime(2016, 7, 22), sent_at=datetime(2016, 7, 22), + status='sending', billable_units=3) + create_notification(template=sample_template, created_at=datetime(2016, 7, 30), sent_at=datetime(2016, 7, 22), + status='sending', billable_units=4) + create_notification(template=sample_email_template, created_at=datetime(2016, 8, 22), sent_at=datetime(2016, 7, 22), + status='sending', billable_units=0) + create_notification(template=sample_email_template, created_at=datetime(2016, 8, 30), sent_at=datetime(2016, 7, 22), + status='sending', billable_units=0) + # next year + create_notification(template=sample_template, created_at=datetime(2017, 3, 31, 23, 00, 00), + sent_at=datetime(2017, 3, 31), status='sending', billable_units=6) + results = get_notification_billing_data_per_month(sample_template.service_id, 2016) + assert len(results) == 4 + assert results[0] == ('April', (1, 'sms', Decimal('1.4'))) + assert results[1] == ('May', (2, 'sms', Decimal('1.4'))) + assert results[2] == ('July', (7, 'sms', Decimal('1.4'))) + assert results[3] == ('August', (2, 'email', Decimal('0'))) + + +def test_get_notification_billing_data_per_month_with_multiple_rates(notify_db, notify_db_session, sample_template, + sample_email_template): + set_up_rate(notify_db, datetime(2016, 4, 1), 1.40) + set_up_rate(notify_db, datetime(2016, 6, 5), 1.75) + # previous year + create_notification(template=sample_template, created_at=datetime(2016, 3, 31), sent_at=datetime(2016, 3, 31), + status='sending', billable_units=1) + # current year + create_notification(template=sample_template, created_at=datetime(2016, 4, 2), sent_at=datetime(2016, 4, 2), + status='sending', billable_units=1) + create_notification(template=sample_template, created_at=datetime(2016, 5, 18), sent_at=datetime(2016, 5, 18), + status='sending', billable_units=2) + create_notification(template=sample_template, created_at=datetime(2016, 6, 1), sent_at=datetime(2016, 6, 1), + status='sending', billable_units=3) + create_notification(template=sample_template, created_at=datetime(2016, 6, 15), sent_at=datetime(2016, 6, 15), + status='sending', billable_units=4) + create_notification(template=sample_email_template, created_at=datetime(2016, 8, 22), + sent_at=datetime(2016, 7, 22), + status='sending', billable_units=0) + create_notification(template=sample_email_template, created_at=datetime(2016, 8, 30), + sent_at=datetime(2016, 7, 22), + status='sending', billable_units=0) + # next year + create_notification(template=sample_template, created_at=datetime(2017, 3, 31, 23, 00, 00), + sent_at=datetime(2017, 3, 31), status='sending', billable_units=6) + results = get_notification_billing_data_per_month(sample_template.service_id, 2016) + assert len(results) == 5 + assert results[0] == ('April', (1, 'sms', Decimal('1.4'))) + assert results[1] == ('May', (2, 'sms', Decimal('1.4'))) + assert results[2] == ('June', (3, 'sms', Decimal('1.4'))) + assert results[3] == ('June', (4, 'sms', Decimal('1.75'))) + assert results[4] == ('August', (2, 'email', Decimal('0'))) + + def set_up_rate(notify_db, start_date, value): rate = Rate(id=uuid.uuid4(), valid_from=start_date, rate=value, notification_type='sms') notify_db.session.add(rate) From e1e55edd9cae767b2dfa7c3eaedc075263240d37 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 26 Apr 2017 15:57:11 +0100 Subject: [PATCH 38/46] Add new fields to the usage queries: rate_multiplier, international, phone_prefix. --- app/dao/notification_usage_dao.py | 46 +++++++++++++++----- tests/app/dao/test_notification_usage_dao.py | 32 +++++++------- 2 files changed, 51 insertions(+), 27 deletions(-) diff --git a/app/dao/notification_usage_dao.py b/app/dao/notification_usage_dao.py index b1db43329..7dffaca7a 100644 --- a/app/dao/notification_usage_dao.py +++ b/app/dao/notification_usage_dao.py @@ -47,14 +47,20 @@ def email_billing_data_query(service_id, start_date, end_date): result = db.session.query( func.count(NotificationHistory.id), NotificationHistory.notification_type, - "0" + "0", + NotificationHistory.rate_multiplier, + NotificationHistory.international, + NotificationHistory.phone_prefix ).filter( *billing_data_filter(EMAIL_TYPE, start_date, end_date, service_id) ).group_by( - NotificationHistory.notification_type + NotificationHistory.notification_type, + NotificationHistory.rate_multiplier, + NotificationHistory.international, + NotificationHistory.phone_prefix ).first() if not result: - return 0, EMAIL_TYPE, Decimal("0") + return 0, EMAIL_TYPE, Decimal("0"), None, False, None else: return result @@ -63,14 +69,20 @@ def sms_billing_data_query(rate, service_id, start_date, end_date): result = db.session.query( func.sum(NotificationHistory.billable_units), NotificationHistory.notification_type, - rate + rate, + NotificationHistory.rate_multiplier, + NotificationHistory.international, + NotificationHistory.phone_prefix ).filter( *billing_data_filter(SMS_TYPE, start_date, end_date, service_id) ).group_by( - NotificationHistory.notification_type + NotificationHistory.notification_type, + NotificationHistory.rate_multiplier, + NotificationHistory.international, + NotificationHistory.phone_prefix ).first() if not result: - return 0, SMS_TYPE, Decimal("0") + return 0, SMS_TYPE, Decimal("0"), None, False, None else: return result @@ -86,11 +98,17 @@ def sms_billing_data_per_month_query(rate, service_id, start_date, end_date): month, func.sum(NotificationHistory.billable_units), NotificationHistory.notification_type, - rate + rate, + NotificationHistory.rate_multiplier, + NotificationHistory.international, + NotificationHistory.phone_prefix ).filter( *billing_data_filter(SMS_TYPE, start_date, end_date, service_id) ).group_by( - NotificationHistory.notification_type, month + NotificationHistory.notification_type, month, + NotificationHistory.rate_multiplier, + NotificationHistory.international, + NotificationHistory.phone_prefix ).order_by( month ).all() @@ -102,11 +120,17 @@ def email_billing_data_per_month_query(rate, service_id, start_date, end_date): month, func.count(NotificationHistory.id), NotificationHistory.notification_type, - rate + rate, + NotificationHistory.rate_multiplier, + NotificationHistory.international, + NotificationHistory.phone_prefix ).filter( *billing_data_filter(EMAIL_TYPE, start_date, end_date, service_id) ).group_by( - NotificationHistory.notification_type, month + NotificationHistory.notification_type, month, + NotificationHistory.rate_multiplier, + NotificationHistory.international, + NotificationHistory.phone_prefix ).order_by( month ).all() @@ -124,4 +148,4 @@ def get_notification_billing_data_per_month(service_id, year): result.extend(email_billing_data_per_month_query("0", service_id, start_date, end_date)) - return [(datetime.strftime(x[0], "%B"), x[1:]) for x in result] + return [(datetime.strftime(x[0], "%B"), x[1], x[2], x[3], x[4], x[5], x[6]) for x in result] diff --git a/tests/app/dao/test_notification_usage_dao.py b/tests/app/dao/test_notification_usage_dao.py index 0386d5f85..7b53ab6f1 100644 --- a/tests/app/dao/test_notification_usage_dao.py +++ b/tests/app/dao/test_notification_usage_dao.py @@ -48,9 +48,9 @@ def test_get_yearly_billing_data(notify_db, notify_db_session, sample_template, status='sending', billable_units=6) results = get_yearly_billing_data(sample_template.service_id, 2016) assert len(results) == 3 - assert results[0] == (3, 'sms', Decimal('1.4')) - assert results[1] == (12, 'sms', Decimal('1.58')) - assert results[2] == (2, 'email', Decimal("0")) + assert results[0] == (3, 'sms', Decimal('1.4'), None, False, None) + assert results[1] == (12, 'sms', Decimal('1.58'), None, False, None) + assert results[2] == (2, 'email', Decimal("0"), None, False, None) def test_get_yearly_billing_data_with_one_rate(notify_db, notify_db_session, sample_template): @@ -76,8 +76,8 @@ def test_get_yearly_billing_data_with_one_rate(notify_db, notify_db_session, sam status='sending', billable_units=7) results = get_yearly_billing_data(sample_template.service_id, 2016) assert len(results) == 2 - assert results[0] == (15, 'sms', Decimal('1.4')) - assert results[1] == (0, 'email', Decimal('0')) + assert results[0] == (15, 'sms', Decimal('1.4'), None, False, None) + assert results[1] == (0, 'email', Decimal('0'), None, False, None) def test_get_yearly_billing_data_with_no_sms_notifications(notify_db, notify_db_session, sample_email_template): @@ -89,8 +89,8 @@ def test_get_yearly_billing_data_with_no_sms_notifications(notify_db, notify_db_ results = get_yearly_billing_data(sample_email_template.service_id, 2016) assert len(results) == 2 - assert results[0] == (0, 'sms', Decimal('0')) - assert results[1] == (2, 'email', Decimal('0')) + assert results[0] == (0, 'sms', Decimal('0'), None, False, None) + assert results[1] == (2, 'email', Decimal('0'), None, False, None) def test_get_notification_billing_data_per_month(notify_db, notify_db_session, sample_template, sample_email_template): @@ -116,10 +116,10 @@ def test_get_notification_billing_data_per_month(notify_db, notify_db_session, s sent_at=datetime(2017, 3, 31), status='sending', billable_units=6) results = get_notification_billing_data_per_month(sample_template.service_id, 2016) assert len(results) == 4 - assert results[0] == ('April', (1, 'sms', Decimal('1.4'))) - assert results[1] == ('May', (2, 'sms', Decimal('1.4'))) - assert results[2] == ('July', (7, 'sms', Decimal('1.4'))) - assert results[3] == ('August', (2, 'email', Decimal('0'))) + assert results[0] == ('April', 1, 'sms', Decimal('1.4'), None, False, None) + assert results[1] == ('May', 2, 'sms', Decimal('1.4'), None, False, None) + assert results[2] == ('July', 7, 'sms', Decimal('1.4'), None, False, None) + assert results[3] == ('August', 2, 'email', Decimal('0'), None, False, None) def test_get_notification_billing_data_per_month_with_multiple_rates(notify_db, notify_db_session, sample_template, @@ -149,11 +149,11 @@ def test_get_notification_billing_data_per_month_with_multiple_rates(notify_db, sent_at=datetime(2017, 3, 31), status='sending', billable_units=6) results = get_notification_billing_data_per_month(sample_template.service_id, 2016) assert len(results) == 5 - assert results[0] == ('April', (1, 'sms', Decimal('1.4'))) - assert results[1] == ('May', (2, 'sms', Decimal('1.4'))) - assert results[2] == ('June', (3, 'sms', Decimal('1.4'))) - assert results[3] == ('June', (4, 'sms', Decimal('1.75'))) - assert results[4] == ('August', (2, 'email', Decimal('0'))) + assert results[0] == ('April', 1, 'sms', Decimal('1.4'), None, False, None) + assert results[1] == ('May', 2, 'sms', Decimal('1.4'), None, False, None) + assert results[2] == ('June', 3, 'sms', Decimal('1.4'), None, False, None) + assert results[3] == ('June', 4, 'sms', Decimal('1.75'), None, False, None) + assert results[4] == ('August', 2, 'email', Decimal('0'), None, False, None) def set_up_rate(notify_db, start_date, value): From a186fc95be918e891344b04d517a37eff413baff Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 27 Apr 2017 10:00:09 +0100 Subject: [PATCH 39/46] Added new endpoints to return the yearly-usage and monthly-usage for a given financial year and service id. Since the response has changed I have created new endpoints so that the deployments for Admin are more managable. Removed print statements from some tests. --- app/dao/notification_usage_dao.py | 34 ++++------ app/models.py | 2 +- app/service/rest.py | 38 +++++++++++ tests/app/dao/test_notification_usage_dao.py | 50 +++++++------- tests/app/job/test_rest.py | 2 - tests/app/service/test_rest.py | 68 +++++++++++++++++++- tests/app/v2/test_errors.py | 1 - 7 files changed, 144 insertions(+), 51 deletions(-) diff --git a/app/dao/notification_usage_dao.py b/app/dao/notification_usage_dao.py index 7dffaca7a..7d9d5e1d0 100644 --- a/app/dao/notification_usage_dao.py +++ b/app/dao/notification_usage_dao.py @@ -1,6 +1,4 @@ from datetime import datetime -from decimal import Decimal - from sqlalchemy import func from app import db @@ -23,9 +21,9 @@ def get_yearly_billing_data(service_id, year): result = [] for r, n in zip(rates, rates[1:]): result.append( - sms_billing_data_query(str(r.rate), service_id, r.valid_from, n.valid_from)) + sms_billing_data_query(r.rate, service_id, r.valid_from, n.valid_from)) - result.append(sms_billing_data_query(str(rates[-1].rate), service_id, rates[-1].valid_from, end_date)) + result.append(sms_billing_data_query(rates[-1].rate, service_id, rates[-1].valid_from, end_date)) result.append(email_billing_data_query(service_id, start_date, end_date)) @@ -47,7 +45,6 @@ def email_billing_data_query(service_id, start_date, end_date): result = db.session.query( func.count(NotificationHistory.id), NotificationHistory.notification_type, - "0", NotificationHistory.rate_multiplier, NotificationHistory.international, NotificationHistory.phone_prefix @@ -59,17 +56,14 @@ def email_billing_data_query(service_id, start_date, end_date): NotificationHistory.international, NotificationHistory.phone_prefix ).first() - if not result: - return 0, EMAIL_TYPE, Decimal("0"), None, False, None - else: - return result + + return tuple(result) + (0,) def sms_billing_data_query(rate, service_id, start_date, end_date): result = db.session.query( func.sum(NotificationHistory.billable_units), NotificationHistory.notification_type, - rate, NotificationHistory.rate_multiplier, NotificationHistory.international, NotificationHistory.phone_prefix @@ -82,9 +76,9 @@ def sms_billing_data_query(rate, service_id, start_date, end_date): NotificationHistory.phone_prefix ).first() if not result: - return 0, SMS_TYPE, Decimal("0"), None, False, None + return 0, SMS_TYPE, None, False, None, 0 else: - return result + return tuple(result) + (rate,) def get_rates_for_year(start_date, end_date, notification_type): @@ -94,11 +88,10 @@ def get_rates_for_year(start_date, end_date, notification_type): def sms_billing_data_per_month_query(rate, service_id, start_date, end_date): month = get_london_month_from_utc_column(NotificationHistory.created_at) - return db.session.query( + return [result + (rate,) for result in db.session.query( month, func.sum(NotificationHistory.billable_units), NotificationHistory.notification_type, - rate, NotificationHistory.rate_multiplier, NotificationHistory.international, NotificationHistory.phone_prefix @@ -112,15 +105,15 @@ def sms_billing_data_per_month_query(rate, service_id, start_date, end_date): ).order_by( month ).all() + ] def email_billing_data_per_month_query(rate, service_id, start_date, end_date): month = get_london_month_from_utc_column(NotificationHistory.created_at) - return db.session.query( + return [result + (rate,) for result in db.session.query( month, func.count(NotificationHistory.id), NotificationHistory.notification_type, - rate, NotificationHistory.rate_multiplier, NotificationHistory.international, NotificationHistory.phone_prefix @@ -134,18 +127,19 @@ def email_billing_data_per_month_query(rate, service_id, start_date, end_date): ).order_by( month ).all() + ] @statsd(namespace="dao") -def get_notification_billing_data_per_month(service_id, year): +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) result = [] for r, n in zip(rates, rates[1:]): - result.extend(sms_billing_data_per_month_query(str(r.rate), service_id, r.valid_from, n.valid_from)) - result.extend(sms_billing_data_per_month_query(str(rates[-1].rate), service_id, rates[-1].valid_from, end_date)) + result.extend(sms_billing_data_per_month_query(r.rate, service_id, r.valid_from, n.valid_from)) + result.extend(sms_billing_data_per_month_query(rates[-1].rate, service_id, rates[-1].valid_from, end_date)) - result.extend(email_billing_data_per_month_query("0", service_id, start_date, end_date)) + result.extend(email_billing_data_per_month_query(0, service_id, start_date, end_date)) return [(datetime.strftime(x[0], "%B"), x[1], x[2], x[3], x[4], x[5], x[6]) for x in result] diff --git a/app/models.py b/app/models.py index 82d9efb43..b6af23aac 100644 --- a/app/models.py +++ b/app/models.py @@ -968,5 +968,5 @@ class Rate(db.Model): id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) valid_from = db.Column(db.DateTime, nullable=False) - rate = db.Column(db.Numeric(), nullable=False) + rate = db.Column(db.Numeric(asdecimal=False), nullable=False) notification_type = db.Column(notification_types, index=True, nullable=False) diff --git a/app/service/rest.py b/app/service/rest.py index 26722373c..8027125bd 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -1,4 +1,5 @@ import itertools +import json from datetime import datetime from flask import ( @@ -8,6 +9,7 @@ from flask import ( ) from sqlalchemy.orm.exc import NoResultFound +from app.dao import notification_usage_dao from app.dao.dao_utils import dao_rollback from app.dao.api_key_dao import ( save_model_api_key, @@ -411,3 +413,39 @@ def get_monthly_template_stats(service_id): )) except ValueError: raise InvalidRequest('Year must be a number', status_code=400) + + +@service_blueprint.route('//yearly-usage') +def get_yearly_billing_usage(service_id): + try: + year = int(request.args.get('year')) + results = notification_usage_dao.get_yearly_billing_data(service_id, year) + json_result = [{"billing_units": x[0], + "notification_type": x[1], + "rate_multiplier": x[2], + "international": x[3], + "phone_prefix": x[4], + "rate": x[5] + } for x in results] + return json.dumps(json_result) + + except TypeError: + return jsonify(result='error', message='No valid year provided'), 400 + + +@service_blueprint.route('//monthly-usage') +def get_yearly_monthly_usage(service_id): + try: + year = int(request.args.get('year')) + results = notification_usage_dao.get_monthly_billing_data(service_id, year) + json_results = [{"month": x[0], + "billing_units": x[1], + "notification_type": x[2], + "rate_multiplier": x[3], + "international": x[4], + "phone_prefix": x[5], + "rate": x[6] + } for x in results] + return json.dumps(json_results) + except TypeError: + return jsonify(result='error', message='No valid year provided'), 400 diff --git a/tests/app/dao/test_notification_usage_dao.py b/tests/app/dao/test_notification_usage_dao.py index 7b53ab6f1..75942f15a 100644 --- a/tests/app/dao/test_notification_usage_dao.py +++ b/tests/app/dao/test_notification_usage_dao.py @@ -1,10 +1,8 @@ import uuid from datetime import datetime -from decimal import Decimal - from app.dao.notification_usage_dao import (get_rates_for_year, get_yearly_billing_data, - get_notification_billing_data_per_month) + get_monthly_billing_data) from app.models import Rate from tests.app.db import create_notification @@ -15,11 +13,11 @@ def test_get_rates_for_year(notify_db, notify_db_session): rates = get_rates_for_year(datetime(2016, 4, 1), datetime(2017, 3, 31), 'sms') assert len(rates) == 1 assert datetime.strftime(rates[0].valid_from, '%Y-%m-%d %H:%M:%S') == "2016-04-01 00:00:00" - assert rates[0].rate == Decimal("1.50") + assert rates[0].rate == 1.50 rates = get_rates_for_year(datetime(2017, 4, 1), datetime(2018, 3, 31), 'sms') assert len(rates) == 1 assert datetime.strftime(rates[0].valid_from, '%Y-%m-%d %H:%M:%S') == "2017-06-01 00:00:00" - assert rates[0].rate == Decimal("1.75") + assert rates[0].rate == 1.75 def test_get_yearly_billing_data(notify_db, notify_db_session, sample_template, sample_email_template): @@ -48,9 +46,9 @@ def test_get_yearly_billing_data(notify_db, notify_db_session, sample_template, status='sending', billable_units=6) results = get_yearly_billing_data(sample_template.service_id, 2016) assert len(results) == 3 - assert results[0] == (3, 'sms', Decimal('1.4'), None, False, None) - assert results[1] == (12, 'sms', Decimal('1.58'), None, False, None) - assert results[2] == (2, 'email', Decimal("0"), None, False, None) + assert results[0] == (3, 'sms', None, False, None, 1.40) + assert results[1] == (12, 'sms', None, False, None, 1.58) + assert results[2] == (2, 'email', None, False, None, 0) def test_get_yearly_billing_data_with_one_rate(notify_db, notify_db_session, sample_template): @@ -76,8 +74,8 @@ def test_get_yearly_billing_data_with_one_rate(notify_db, notify_db_session, sam status='sending', billable_units=7) results = get_yearly_billing_data(sample_template.service_id, 2016) assert len(results) == 2 - assert results[0] == (15, 'sms', Decimal('1.4'), None, False, None) - assert results[1] == (0, 'email', Decimal('0'), None, False, None) + assert results[0] == (15, 'sms', None, False, None, 1.4) + assert results[1] == (0, 'email', None, False, None, 0) def test_get_yearly_billing_data_with_no_sms_notifications(notify_db, notify_db_session, sample_email_template): @@ -89,11 +87,11 @@ def test_get_yearly_billing_data_with_no_sms_notifications(notify_db, notify_db_ results = get_yearly_billing_data(sample_email_template.service_id, 2016) assert len(results) == 2 - assert results[0] == (0, 'sms', Decimal('0'), None, False, None) - assert results[1] == (2, 'email', Decimal('0'), None, False, None) + assert results[0] == (0, 'sms', None, False, None, 0) + assert results[1] == (2, 'email', None, False, None, 0) -def test_get_notification_billing_data_per_month(notify_db, notify_db_session, sample_template, sample_email_template): +def test_get_monthly_billing_data(notify_db, notify_db_session, sample_template, sample_email_template): set_up_rate(notify_db, datetime(2016, 4, 1), 1.40) # previous year create_notification(template=sample_template, created_at=datetime(2016, 3, 31), sent_at=datetime(2016, 3, 31), @@ -114,16 +112,16 @@ def test_get_notification_billing_data_per_month(notify_db, notify_db_session, s # next year create_notification(template=sample_template, created_at=datetime(2017, 3, 31, 23, 00, 00), sent_at=datetime(2017, 3, 31), status='sending', billable_units=6) - results = get_notification_billing_data_per_month(sample_template.service_id, 2016) + results = get_monthly_billing_data(sample_template.service_id, 2016) assert len(results) == 4 - assert results[0] == ('April', 1, 'sms', Decimal('1.4'), None, False, None) - assert results[1] == ('May', 2, 'sms', Decimal('1.4'), None, False, None) - assert results[2] == ('July', 7, 'sms', Decimal('1.4'), None, False, None) - assert results[3] == ('August', 2, 'email', Decimal('0'), None, False, None) + assert results[0] == ('April', 1, 'sms', None, False, None, 1.4) + assert results[1] == ('May', 2, 'sms', None, False, None, 1.4) + assert results[2] == ('July', 7, 'sms', None, False, None, 1.4) + assert results[3] == ('August', 2, 'email', None, False, None, 0) -def test_get_notification_billing_data_per_month_with_multiple_rates(notify_db, notify_db_session, sample_template, - sample_email_template): +def test_get_monthly_billing_data_with_multiple_rates(notify_db, notify_db_session, sample_template, + sample_email_template): set_up_rate(notify_db, datetime(2016, 4, 1), 1.40) set_up_rate(notify_db, datetime(2016, 6, 5), 1.75) # previous year @@ -147,13 +145,13 @@ def test_get_notification_billing_data_per_month_with_multiple_rates(notify_db, # next year create_notification(template=sample_template, created_at=datetime(2017, 3, 31, 23, 00, 00), sent_at=datetime(2017, 3, 31), status='sending', billable_units=6) - results = get_notification_billing_data_per_month(sample_template.service_id, 2016) + results = get_monthly_billing_data(sample_template.service_id, 2016) assert len(results) == 5 - assert results[0] == ('April', 1, 'sms', Decimal('1.4'), None, False, None) - assert results[1] == ('May', 2, 'sms', Decimal('1.4'), None, False, None) - assert results[2] == ('June', 3, 'sms', Decimal('1.4'), None, False, None) - assert results[3] == ('June', 4, 'sms', Decimal('1.75'), None, False, None) - assert results[4] == ('August', 2, 'email', Decimal('0'), None, False, None) + assert results[0] == ('April', 1, 'sms', None, False, None, 1.4) + assert results[1] == ('May', 2, 'sms', None, False, None, 1.4) + assert results[2] == ('June', 3, 'sms', None, False, None, 1.4) + assert results[3] == ('June', 4, 'sms', None, False, None, 1.75) + assert results[4] == ('August', 2, 'email', None, False, None, 0) def set_up_rate(notify_db, start_date, value): diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index e5a3cc453..e05d52b3a 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -205,7 +205,6 @@ def test_should_not_create_scheduled_job_more_then_24_hours_hence(notify_api, sa auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] - print(json.dumps(data)) response = client.post( path, data=json.dumps(data), @@ -240,7 +239,6 @@ def test_should_not_create_scheduled_job_in_the_past(notify_api, sample_template auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] - print(json.dumps(data)) response = client.post( path, data=json.dumps(data), diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 2cf666be4..37331b182 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -10,7 +10,7 @@ from freezegun import freeze_time from app.dao.users_dao import save_model_user from app.dao.services_dao import dao_remove_user_from_service -from app.models import User, Organisation, DVLA_ORG_LAND_REGISTRY +from app.models import User, Organisation, DVLA_ORG_LAND_REGISTRY, Rate from tests import create_authorization_header from tests.app.conftest import ( sample_service as create_service, @@ -1509,3 +1509,69 @@ def test_get_template_stats_by_month_returns_error_for_incorrect_year( ) assert response.status_code == expected_status assert json.loads(response.get_data(as_text=True)) == expected_json + + +def test_get_yearly_billing_usage(client, notify_db, notify_db_session): + rate = Rate(id=uuid.uuid4(), valid_from=datetime(2016, 3, 31, 23, 00), rate=1.58, notification_type='sms') + notify_db.session.add(rate) + notification = create_sample_notification(notify_db, notify_db_session, created_at=datetime(2016, 6, 5), + sent_at=datetime(2016, 6, 5), + status='sending') + response = client.get( + '/service/{}/yearly-usage?year=2016'.format(notification.service_id), + headers=[create_authorization_header()] + ) + assert response.status_code == 200 + + assert json.loads(response.get_data(as_text=True)) == [{'billing_units': 1, 'notification_type': 'sms', + 'phone_prefix': None, 'international': False, + 'rate_multiplier': None, 'rate': 1.58}, + {'billing_units': 0, 'notification_type': 'email', + 'phone_prefix': None, 'international': False, + 'rate_multiplier': None, 'rate': 0}] + + +def test_get_yearly_billing_usage_returns_400_if_missing_year(client, sample_service): + response = client.get( + '/service/{}/yearly-usage'.format(sample_service.id), + headers=[create_authorization_header()] + ) + assert response.status_code == 400 + assert json.loads(response.get_data(as_text=True)) == { + 'message': 'No valid year provided', 'result': 'error' + } + + +def test_get_monthly_billing_usage(client, notify_db, notify_db_session): + rate = Rate(id=uuid.uuid4(), valid_from=datetime(2016, 3, 31, 23, 00), rate=1.58, notification_type='sms') + notify_db.session.add(rate) + notification = create_sample_notification(notify_db, notify_db_session, created_at=datetime(2016, 6, 5), + sent_at=datetime(2016, 6, 5), + status='sending') + create_sample_notification(notify_db, notify_db_session, created_at=datetime(2016, 7, 5), + sent_at=datetime(2016, 7, 5), + status='sending') + response = client.get( + '/service/{}/monthly-usage?year=2016'.format(notification.service_id), + headers=[create_authorization_header()] + ) + assert response.status_code == 200 + actual = json.loads(response.get_data(as_text=True)) + assert len(actual) == 2 + assert actual == [{'month': 'June', 'billing_units': 1, 'notification_type': 'sms', + 'phone_prefix': None, 'international': False, + 'rate_multiplier': None, 'rate': 1.58}, + {'month': 'July', 'billing_units': 1, 'notification_type': 'sms', + 'phone_prefix': None, 'international': False, + 'rate_multiplier': None, 'rate': 1.58}] + + +def test_get_monthly_billing_usage_returns_400_if_missing_year(client, sample_service): + response = client.get( + '/service/{}/monthly-usage'.format(sample_service.id), + headers=[create_authorization_header()] + ) + assert response.status_code == 400 + assert json.loads(response.get_data(as_text=True)) == { + 'message': 'No valid year provided', 'result': 'error' + } diff --git a/tests/app/v2/test_errors.py b/tests/app/v2/test_errors.py index 5f0ed0f81..8345faacc 100644 --- a/tests/app/v2/test_errors.py +++ b/tests/app/v2/test_errors.py @@ -88,7 +88,6 @@ def test_validation_error(app_for_test): response = client.get(url_for('v2_under_test.raising_validation_error')) assert response.status_code == 400 error = json.loads(response.get_data(as_text=True)) - print(error) assert len(error.keys()) == 2 assert error['status_code'] == 400 assert len(error['errors']) == 2 From fdbadf967e4541a8933aeb3fc264b354ea89212b Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 27 Apr 2017 10:16:29 +0100 Subject: [PATCH 40/46] Fix the email billing data when there is no results. --- app/dao/notification_usage_dao.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/dao/notification_usage_dao.py b/app/dao/notification_usage_dao.py index 7d9d5e1d0..e07f575e0 100644 --- a/app/dao/notification_usage_dao.py +++ b/app/dao/notification_usage_dao.py @@ -56,8 +56,10 @@ def email_billing_data_query(service_id, start_date, end_date): NotificationHistory.international, NotificationHistory.phone_prefix ).first() - - return tuple(result) + (0,) + if not result: + return 0, EMAIL_TYPE, None, False, None, 0 + else: + return tuple(result) + (0,) def sms_billing_data_query(rate, service_id, start_date, end_date): From 1a64509186410c40f6da9daaea3fa1e56b948761 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 27 Apr 2017 15:43:57 +0100 Subject: [PATCH 41/46] Change the resultset from the yearly and monthly billing data queries. Fix some formatting of the return objects. --- app/dao/notification_usage_dao.py | 119 +++++++++---------- app/models.py | 4 +- app/service/rest.py | 17 ++- tests/app/dao/test_notification_usage_dao.py | 44 ++++--- tests/app/db.py | 8 +- tests/app/service/test_rest.py | 37 ++++-- 6 files changed, 121 insertions(+), 108 deletions(-) diff --git a/app/dao/notification_usage_dao.py b/app/dao/notification_usage_dao.py index e07f575e0..fb412d849 100644 --- a/app/dao/notification_usage_dao.py +++ b/app/dao/notification_usage_dao.py @@ -1,5 +1,8 @@ from datetime import datetime -from sqlalchemy import func + +from sqlalchemy import Float, Integer +from sqlalchemy import func, case, cast +from sqlalchemy import literal_column from app import db from app.dao.date_util import get_financial_year @@ -21,13 +24,26 @@ def get_yearly_billing_data(service_id, year): result = [] for r, n in zip(rates, rates[1:]): result.append( - sms_billing_data_query(r.rate, service_id, r.valid_from, n.valid_from)) + sms_yearly_billing_data_query(r.rate, service_id, r.valid_from, n.valid_from)) - result.append(sms_billing_data_query(rates[-1].rate, service_id, rates[-1].valid_from, end_date)) + result.append(sms_yearly_billing_data_query(rates[-1].rate, service_id, rates[-1].valid_from, end_date)) - result.append(email_billing_data_query(service_id, start_date, end_date)) + result.append(email_yearly_billing_data_query(service_id, start_date, end_date)) - return result + return sum(result, []) + + +@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) + + result = [] + for r, n in zip(rates, rates[1:]): + result.extend(sms_billing_data_per_month_query(r.rate, service_id, r.valid_from, n.valid_from)) + result.extend(sms_billing_data_per_month_query(rates[-1].rate, service_id, rates[-1].valid_from, end_date)) + + return [(datetime.strftime(x[0], "%B"), x[1], x[2], x[3], x[4], x[5]) for x in result] def billing_data_filter(notification_type, start_date, end_date, service_id): @@ -41,46 +57,49 @@ def billing_data_filter(notification_type, start_date, end_date, service_id): ] -def email_billing_data_query(service_id, start_date, end_date): +def email_yearly_billing_data_query(service_id, start_date, end_date, rate=0): result = db.session.query( func.count(NotificationHistory.id), + func.count(NotificationHistory.id), + rate_multiplier(), NotificationHistory.notification_type, - NotificationHistory.rate_multiplier, NotificationHistory.international, - NotificationHistory.phone_prefix + cast(rate, Integer()) ).filter( *billing_data_filter(EMAIL_TYPE, start_date, end_date, service_id) ).group_by( NotificationHistory.notification_type, - NotificationHistory.rate_multiplier, - NotificationHistory.international, - NotificationHistory.phone_prefix + rate_multiplier(), + NotificationHistory.international ).first() if not result: - return 0, EMAIL_TYPE, None, False, None, 0 + return [(0, 0, 1, EMAIL_TYPE, False, 0)] else: - return tuple(result) + (0,) + return [result] -def sms_billing_data_query(rate, service_id, start_date, end_date): +def sms_yearly_billing_data_query(rate, service_id, start_date, end_date): result = db.session.query( + cast(func.sum(NotificationHistory.billable_units * rate_multiplier()), Integer()), func.sum(NotificationHistory.billable_units), + rate_multiplier(), NotificationHistory.notification_type, - NotificationHistory.rate_multiplier, NotificationHistory.international, - NotificationHistory.phone_prefix + cast(rate, Float()) ).filter( *billing_data_filter(SMS_TYPE, start_date, end_date, service_id) ).group_by( NotificationHistory.notification_type, - NotificationHistory.rate_multiplier, NotificationHistory.international, - NotificationHistory.phone_prefix - ).first() + rate_multiplier() + ).order_by( + rate_multiplier() + ).all() + if not result: - return 0, SMS_TYPE, None, False, None, 0 + return [(0, 0, 1, SMS_TYPE, False, rate)] else: - return tuple(result) + (rate,) + return result def get_rates_for_year(start_date, end_date, notification_type): @@ -90,58 +109,30 @@ def get_rates_for_year(start_date, end_date, notification_type): def sms_billing_data_per_month_query(rate, service_id, start_date, end_date): month = get_london_month_from_utc_column(NotificationHistory.created_at) - return [result + (rate,) for result in db.session.query( + result = db.session.query( month, func.sum(NotificationHistory.billable_units), - NotificationHistory.notification_type, - NotificationHistory.rate_multiplier, + rate_multiplier(), NotificationHistory.international, - NotificationHistory.phone_prefix + NotificationHistory.notification_type, + cast(rate, Float()) ).filter( *billing_data_filter(SMS_TYPE, start_date, end_date, service_id) ).group_by( - NotificationHistory.notification_type, month, - NotificationHistory.rate_multiplier, - NotificationHistory.international, - NotificationHistory.phone_prefix - ).order_by( - month - ).all() - ] - - -def email_billing_data_per_month_query(rate, service_id, start_date, end_date): - month = get_london_month_from_utc_column(NotificationHistory.created_at) - return [result + (rate,) for result in db.session.query( - month, - func.count(NotificationHistory.id), NotificationHistory.notification_type, + month, NotificationHistory.rate_multiplier, - NotificationHistory.international, - NotificationHistory.phone_prefix - ).filter( - *billing_data_filter(EMAIL_TYPE, start_date, end_date, service_id) - ).group_by( - NotificationHistory.notification_type, month, - NotificationHistory.rate_multiplier, - NotificationHistory.international, - NotificationHistory.phone_prefix + NotificationHistory.international ).order_by( - month + month, + rate_multiplier() ).all() - ] + + return result -@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) - - result = [] - for r, n in zip(rates, rates[1:]): - result.extend(sms_billing_data_per_month_query(r.rate, service_id, r.valid_from, n.valid_from)) - result.extend(sms_billing_data_per_month_query(rates[-1].rate, service_id, rates[-1].valid_from, end_date)) - - result.extend(email_billing_data_per_month_query(0, service_id, start_date, end_date)) - - return [(datetime.strftime(x[0], "%B"), x[1], x[2], x[3], x[4], x[5], x[6]) for x in result] +def rate_multiplier(): + return cast(case([ + (NotificationHistory.rate_multiplier == None, literal_column("'1'")), # noqa + (NotificationHistory.rate_multiplier != None, NotificationHistory.rate_multiplier), # noqa + ]), Integer()) diff --git a/app/models.py b/app/models.py index b6af23aac..59e6f7a91 100644 --- a/app/models.py +++ b/app/models.py @@ -847,7 +847,7 @@ class NotificationHistory(db.Model, HistoryModel): international = db.Column(db.Boolean, nullable=False, default=False) phone_prefix = db.Column(db.String, nullable=True) - rate_multiplier = db.Column(db.Float(), nullable=True) + rate_multiplier = db.Column(db.Float(asdecimal=False), nullable=True) @classmethod def from_original(cls, notification): @@ -968,5 +968,5 @@ class Rate(db.Model): id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) valid_from = db.Column(db.DateTime, nullable=False) - rate = db.Column(db.Numeric(asdecimal=False), nullable=False) + rate = db.Column(db.Float(asdecimal=False), nullable=False) notification_type = db.Column(notification_types, index=True, nullable=False) diff --git a/app/service/rest.py b/app/service/rest.py index 8027125bd..25991f30d 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -420,11 +420,11 @@ def get_yearly_billing_usage(service_id): try: year = int(request.args.get('year')) results = notification_usage_dao.get_yearly_billing_data(service_id, year) - json_result = [{"billing_units": x[0], - "notification_type": x[1], + json_result = [{"credits": x[0], + "billing_units": x[1], "rate_multiplier": x[2], - "international": x[3], - "phone_prefix": x[4], + "notification_type": x[3], + "international": x[4], "rate": x[5] } for x in results] return json.dumps(json_result) @@ -440,11 +440,10 @@ def get_yearly_monthly_usage(service_id): results = notification_usage_dao.get_monthly_billing_data(service_id, year) json_results = [{"month": x[0], "billing_units": x[1], - "notification_type": x[2], - "rate_multiplier": x[3], - "international": x[4], - "phone_prefix": x[5], - "rate": x[6] + "rate_multiplier": x[2], + "international": x[3], + "notification_type": x[4], + "rate": x[5] } for x in results] return json.dumps(json_results) except TypeError: diff --git a/tests/app/dao/test_notification_usage_dao.py b/tests/app/dao/test_notification_usage_dao.py index 75942f15a..72891d46e 100644 --- a/tests/app/dao/test_notification_usage_dao.py +++ b/tests/app/dao/test_notification_usage_dao.py @@ -32,7 +32,7 @@ def test_get_yearly_billing_data(notify_db, notify_db_session, sample_template, create_notification(template=sample_template, created_at=datetime(2016, 5, 18), sent_at=datetime(2016, 5, 18), status='sending', billable_units=2) create_notification(template=sample_template, created_at=datetime(2016, 7, 22), sent_at=datetime(2016, 7, 22), - status='sending', billable_units=3) + status='sending', billable_units=3, rate_multiplier=2, international=True, phone_prefix="1") create_notification(template=sample_template, created_at=datetime(2016, 9, 15), sent_at=datetime(2016, 9, 15), status='sending', billable_units=4) create_notification(template=sample_template, created_at=datetime(2017, 3, 31), sent_at=datetime(2017, 3, 31), @@ -45,10 +45,11 @@ def test_get_yearly_billing_data(notify_db, notify_db_session, sample_template, create_notification(template=sample_template, created_at=datetime(2017, 4, 1), sent_at=datetime(2017, 4, 1), status='sending', billable_units=6) results = get_yearly_billing_data(sample_template.service_id, 2016) - assert len(results) == 3 - assert results[0] == (3, 'sms', None, False, None, 1.40) - assert results[1] == (12, 'sms', None, False, None, 1.58) - assert results[2] == (2, 'email', None, False, None, 0) + assert len(results) == 4 + assert results[0] == (3, 3, 1, 'sms', False, 1.4) + assert results[1] == (9, 9, 1, 'sms', False, 1.58) + assert results[2] == (6, 3, 2, 'sms', True, 1.58) + assert results[3] == (2, 2, 1, 'email', False, 0) def test_get_yearly_billing_data_with_one_rate(notify_db, notify_db_session, sample_template): @@ -74,8 +75,8 @@ def test_get_yearly_billing_data_with_one_rate(notify_db, notify_db_session, sam status='sending', billable_units=7) results = get_yearly_billing_data(sample_template.service_id, 2016) assert len(results) == 2 - assert results[0] == (15, 'sms', None, False, None, 1.4) - assert results[1] == (0, 'email', None, False, None, 0) + assert results[0] == (15, 15, 1, 'sms', False, 1.4) + assert results[1] == (0, 0, 1, 'email', False, 0) def test_get_yearly_billing_data_with_no_sms_notifications(notify_db, notify_db_session, sample_email_template): @@ -87,8 +88,8 @@ def test_get_yearly_billing_data_with_no_sms_notifications(notify_db, notify_db_ results = get_yearly_billing_data(sample_email_template.service_id, 2016) assert len(results) == 2 - assert results[0] == (0, 'sms', None, False, None, 0) - assert results[1] == (2, 'email', None, False, None, 0) + assert results[0] == (0, 0, 1, 'sms', False, 1.4) + assert results[1] == (2, 2, 1, 'email', False, 0) def test_get_monthly_billing_data(notify_db, notify_db_session, sample_template, sample_email_template): @@ -103,8 +104,13 @@ def test_get_monthly_billing_data(notify_db, notify_db_session, sample_template, status='sending', billable_units=2) create_notification(template=sample_template, created_at=datetime(2016, 7, 22), sent_at=datetime(2016, 7, 22), status='sending', billable_units=3) + create_notification(template=sample_template, created_at=datetime(2016, 7, 22), sent_at=datetime(2016, 7, 22), + status='sending', billable_units=3, rate_multiplier=2) + create_notification(template=sample_template, created_at=datetime(2016, 7, 22), sent_at=datetime(2016, 7, 22), + status='sending', billable_units=3, rate_multiplier=2) create_notification(template=sample_template, created_at=datetime(2016, 7, 30), sent_at=datetime(2016, 7, 22), status='sending', billable_units=4) + create_notification(template=sample_email_template, created_at=datetime(2016, 8, 22), sent_at=datetime(2016, 7, 22), status='sending', billable_units=0) create_notification(template=sample_email_template, created_at=datetime(2016, 8, 30), sent_at=datetime(2016, 7, 22), @@ -114,10 +120,11 @@ def test_get_monthly_billing_data(notify_db, notify_db_session, sample_template, sent_at=datetime(2017, 3, 31), status='sending', billable_units=6) results = get_monthly_billing_data(sample_template.service_id, 2016) assert len(results) == 4 - assert results[0] == ('April', 1, 'sms', None, False, None, 1.4) - assert results[1] == ('May', 2, 'sms', None, False, None, 1.4) - assert results[2] == ('July', 7, 'sms', None, False, None, 1.4) - assert results[3] == ('August', 2, 'email', None, False, None, 0) + # (billable_units, rate_multiplier, international, type, rate) + assert results[0] == ('April', 1, 1, False, 'sms', 1.4) + assert results[1] == ('May', 2, 1, False, 'sms', 1.4) + assert results[2] == ('July', 7, 1, False, 'sms', 1.4) + assert results[3] == ('July', 6, 2, False, 'sms', 1.4) def test_get_monthly_billing_data_with_multiple_rates(notify_db, notify_db_session, sample_template, @@ -146,12 +153,11 @@ def test_get_monthly_billing_data_with_multiple_rates(notify_db, notify_db_sessi create_notification(template=sample_template, created_at=datetime(2017, 3, 31, 23, 00, 00), sent_at=datetime(2017, 3, 31), status='sending', billable_units=6) results = get_monthly_billing_data(sample_template.service_id, 2016) - assert len(results) == 5 - assert results[0] == ('April', 1, 'sms', None, False, None, 1.4) - assert results[1] == ('May', 2, 'sms', None, False, None, 1.4) - assert results[2] == ('June', 3, 'sms', None, False, None, 1.4) - assert results[3] == ('June', 4, 'sms', None, False, None, 1.75) - assert results[4] == ('August', 2, 'email', None, False, None, 0) + assert len(results) == 4 + assert results[0] == ('April', 1, 1, False, 'sms', 1.4) + assert results[1] == ('May', 2, 1, False, 'sms', 1.4) + assert results[2] == ('June', 3, 1, False, 'sms', 1.4) + assert results[3] == ('June', 4, 1, False, 'sms', 1.75) def set_up_rate(notify_db, start_date, value): diff --git a/tests/app/db.py b/tests/app/db.py index 38d5eca9d..b5838d693 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -73,7 +73,9 @@ def create_notification( key_type=KEY_TYPE_NORMAL, sent_by=None, client_reference=None, - international=False + rate_multiplier=None, + international=False, + phone_prefix=None ): if created_at is None: created_at = datetime.utcnow() @@ -105,7 +107,9 @@ def create_notification( 'updated_at': updated_at, 'client_reference': client_reference, 'job_row_number': job_row_number, - 'international': international + 'rate_multiplier': rate_multiplier, + 'international': international, + 'phone_prefix': phone_prefix } notification = Notification(**data) dao_create_notification(notification) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 37331b182..89cfc27f1 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1523,12 +1523,18 @@ def test_get_yearly_billing_usage(client, notify_db, notify_db_session): ) assert response.status_code == 200 - assert json.loads(response.get_data(as_text=True)) == [{'billing_units': 1, 'notification_type': 'sms', - 'phone_prefix': None, 'international': False, - 'rate_multiplier': None, 'rate': 1.58}, - {'billing_units': 0, 'notification_type': 'email', - 'phone_prefix': None, 'international': False, - 'rate_multiplier': None, 'rate': 0}] + assert json.loads(response.get_data(as_text=True)) == [{'credits': 1, + 'billing_units': 1, + 'rate_multiplier': 1, + 'notification_type': 'sms', + 'international': False, + 'rate': 1.58}, + {'credits': 0, + 'billing_units': 0, + 'rate_multiplier': 1, + 'notification_type': 'email', + 'international': False, + 'rate': 0}] def test_get_yearly_billing_usage_returns_400_if_missing_year(client, sample_service): @@ -1558,12 +1564,19 @@ def test_get_monthly_billing_usage(client, notify_db, notify_db_session): assert response.status_code == 200 actual = json.loads(response.get_data(as_text=True)) assert len(actual) == 2 - assert actual == [{'month': 'June', 'billing_units': 1, 'notification_type': 'sms', - 'phone_prefix': None, 'international': False, - 'rate_multiplier': None, 'rate': 1.58}, - {'month': 'July', 'billing_units': 1, 'notification_type': 'sms', - 'phone_prefix': None, 'international': False, - 'rate_multiplier': None, 'rate': 1.58}] + print(actual) + assert actual == [{'month': 'June', + 'international': False, + 'rate_multiplier': 1, + 'notification_type': 'sms', + 'rate': 1.58, + 'billing_units': 1}, + {'month': 'July', + 'international': False, + 'rate_multiplier': 1, + 'notification_type': 'sms', + 'rate': 1.58, + 'billing_units': 1}] def test_get_monthly_billing_usage_returns_400_if_missing_year(client, sample_service): From 3b41478a0aa6471be5413b7d175c54aedbf6e733 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 27 Apr 2017 16:40:00 +0100 Subject: [PATCH 42/46] Updated Notification model to use Float(asdecimal=False) for rate_mutliplier. Added test with multiple rows for a month. --- app/models.py | 2 +- tests/app/conftest.py | 6 +++-- tests/app/dao/test_notification_usage_dao.py | 7 ++++++ tests/app/service/test_rest.py | 23 ++++++++++++++++++-- 4 files changed, 33 insertions(+), 5 deletions(-) diff --git a/app/models.py b/app/models.py index 59e6f7a91..0d7f7236d 100644 --- a/app/models.py +++ b/app/models.py @@ -670,7 +670,7 @@ class Notification(db.Model): international = db.Column(db.Boolean, nullable=False, default=False) phone_prefix = db.Column(db.String, nullable=True) - rate_multiplier = db.Column(db.Float(), nullable=True) + rate_multiplier = db.Column(db.Float(asdecimal=False), nullable=True) @property def personalisation(self): diff --git a/tests/app/conftest.py b/tests/app/conftest.py index de69e9240..fc9748870 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -444,7 +444,8 @@ def sample_notification( api_key_id=None, key_type=KEY_TYPE_NORMAL, sent_by=None, - client_reference=None + client_reference=None, + rate_multiplier=1.0 ): if created_at is None: created_at = datetime.utcnow() @@ -481,7 +482,8 @@ def sample_notification( 'key_type': key_type, 'sent_by': sent_by, 'updated_at': created_at if status in NOTIFICATION_STATUS_TYPES_COMPLETED else None, - 'client_reference': client_reference + 'client_reference': client_reference, + 'rate_multiplier': rate_multiplier } if job_row_number is not None: data['job_row_number'] = job_row_number diff --git a/tests/app/dao/test_notification_usage_dao.py b/tests/app/dao/test_notification_usage_dao.py index 72891d46e..0c3ddc952 100644 --- a/tests/app/dao/test_notification_usage_dao.py +++ b/tests/app/dao/test_notification_usage_dao.py @@ -160,6 +160,13 @@ def test_get_monthly_billing_data_with_multiple_rates(notify_db, notify_db_sessi assert results[3] == ('June', 4, 1, False, 'sms', 1.75) +def test_get_monthly_billing_data_with_no_notifications_for_year(notify_db, notify_db_session, sample_template, + sample_email_template): + set_up_rate(notify_db, datetime(2016, 4, 1), 1.40) + results = get_monthly_billing_data(sample_template.service_id, 2016) + assert len(results) == 0 + + def set_up_rate(notify_db, start_date, value): rate = Rate(id=uuid.uuid4(), valid_from=start_date, rate=value, notification_type='sms') notify_db.session.add(rate) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 89cfc27f1..f289960a9 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1554,6 +1554,9 @@ def test_get_monthly_billing_usage(client, notify_db, notify_db_session): notification = create_sample_notification(notify_db, notify_db_session, created_at=datetime(2016, 6, 5), sent_at=datetime(2016, 6, 5), status='sending') + create_sample_notification(notify_db, notify_db_session, created_at=datetime(2016, 6, 5), + sent_at=datetime(2016, 6, 5), + status='sending', rate_multiplier=2) create_sample_notification(notify_db, notify_db_session, created_at=datetime(2016, 7, 5), sent_at=datetime(2016, 7, 5), status='sending') @@ -1563,14 +1566,19 @@ def test_get_monthly_billing_usage(client, notify_db, notify_db_session): ) assert response.status_code == 200 actual = json.loads(response.get_data(as_text=True)) - assert len(actual) == 2 - print(actual) + assert len(actual) == 3 assert actual == [{'month': 'June', 'international': False, 'rate_multiplier': 1, 'notification_type': 'sms', 'rate': 1.58, 'billing_units': 1}, + {'month': 'June', + 'international': False, + 'rate_multiplier': 2, + 'notification_type': 'sms', + 'rate': 1.58, + 'billing_units': 1}, {'month': 'July', 'international': False, 'rate_multiplier': 1, @@ -1588,3 +1596,14 @@ def test_get_monthly_billing_usage_returns_400_if_missing_year(client, sample_se assert json.loads(response.get_data(as_text=True)) == { 'message': 'No valid year provided', 'result': 'error' } + + +def test_get_monthly_billing_usage_returns_empty_list_if_no_notifications(client, notify_db, sample_service): + rate = Rate(id=uuid.uuid4(), valid_from=datetime(2016, 3, 31, 23, 00), rate=1.58, notification_type='sms') + notify_db.session.add(rate) + response = client.get( + '/service/{}/monthly-usage?year=2016'.format(sample_service.id), + headers=[create_authorization_header()] + ) + assert response.status_code == 200 + assert json.loads(response.get_data(as_text=True)) == [] From baf84b53d312503e4a400d76440aeb68e5831bd3 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 27 Apr 2017 18:06:23 +0100 Subject: [PATCH 43/46] Update test to check it doesn't get email usage --- tests/app/service/test_rest.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index f289960a9..f2150df1a 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -12,6 +12,7 @@ from app.dao.users_dao import save_model_user from app.dao.services_dao import dao_remove_user_from_service from app.models import User, Organisation, DVLA_ORG_LAND_REGISTRY, Rate from tests import create_authorization_header +from tests.app.db import create_template from tests.app.conftest import ( sample_service as create_service, sample_service_permission as create_service_permission, @@ -1548,7 +1549,7 @@ def test_get_yearly_billing_usage_returns_400_if_missing_year(client, sample_ser } -def test_get_monthly_billing_usage(client, notify_db, notify_db_session): +def test_get_monthly_billing_usage(client, notify_db, notify_db_session, sample_service): rate = Rate(id=uuid.uuid4(), valid_from=datetime(2016, 3, 31, 23, 00), rate=1.58, notification_type='sms') notify_db.session.add(rate) notification = create_sample_notification(notify_db, notify_db_session, created_at=datetime(2016, 6, 5), @@ -1560,6 +1561,12 @@ def test_get_monthly_billing_usage(client, notify_db, notify_db_session): create_sample_notification(notify_db, notify_db_session, created_at=datetime(2016, 7, 5), sent_at=datetime(2016, 7, 5), status='sending') + + template = create_template(sample_service, template_type='email') + create_sample_notification(notify_db, notify_db_session, created_at=datetime(2016, 6, 5), + sent_at=datetime(2016, 6, 5), + status='sending', + template=template) response = client.get( '/service/{}/monthly-usage?year=2016'.format(notification.service_id), headers=[create_authorization_header()] From fd4e27bfd32c576bd52d82852c359be9e1dc89e8 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 28 Apr 2017 11:00:55 +0100 Subject: [PATCH 44/46] add type conversion to report downloads "sent" is fine as an internal marker but not very obvious to the end user that it specifically refers to international messages. We now say "Sent internationally" in the CSV --- app/models.py | 9 ++++++--- tests/app/test_model.py | 1 + 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/app/models.py b/app/models.py index 82d9efb43..064e62ac5 100644 --- a/app/models.py +++ b/app/models.py @@ -752,7 +752,8 @@ class Notification(db.Model): 'permanent-failure': 'Email address doesn’t exist', 'delivered': 'Delivered', 'sending': 'Sending', - 'created': 'Sending' + 'created': 'Sending', + 'sent': 'Delivered' }, 'sms': { 'failed': 'Failed', @@ -761,7 +762,8 @@ class Notification(db.Model): 'permanent-failure': 'Phone number doesn’t exist', 'delivered': 'Delivered', 'sending': 'Sending', - 'created': 'Sending' + 'created': 'Sending', + 'sent': 'Sent internationally' }, 'letter': { 'failed': 'Failed', @@ -770,7 +772,8 @@ class Notification(db.Model): 'permanent-failure': 'Permanent failure', 'delivered': 'Delivered', 'sending': 'Sending', - 'created': 'Sending' + 'created': 'Sending', + 'sent': 'Delivered' } }[self.template.template_type].get(self.status, self.status) diff --git a/tests/app/test_model.py b/tests/app/test_model.py index ba0920745..14fa5609f 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -111,6 +111,7 @@ def test_notification_for_csv_returns_correct_job_row_number(notify_db, notify_d ('email', 'permanent-failure', 'Email address doesn’t exist'), ('sms', 'temporary-failure', 'Phone not accepting messages right now'), ('sms', 'permanent-failure', 'Phone number doesn’t exist'), + ('sms', 'sent', 'Sent internationally'), ('letter', 'permanent-failure', 'Permanent failure'), ('letter', 'delivered', 'Delivered') ]) From f577e91134568b5dc5160078c225b952d8d0c625 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 28 Apr 2017 11:56:12 +0100 Subject: [PATCH 45/46] treat sent as delivered in detailed service api this is for when we fetch the large blue numbers for viewing notifications for an entire service --- app/service/statistics.py | 2 +- tests/app/service/test_statistics.py | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/app/service/statistics.py b/app/service/statistics.py index b9cd985f2..8ebf8434b 100644 --- a/app/service/statistics.py +++ b/app/service/statistics.py @@ -48,7 +48,7 @@ def create_zeroed_stats_dicts(): def _update_statuses_from_row(update_dict, row): update_dict['requested'] += row.count - if row.status == 'delivered': + if row.status in ('delivered', 'sent'): update_dict['delivered'] += row.count elif row.status in ('failed', 'technical-failure', 'temporary-failure', 'permanent-failure'): update_dict['failed'] += row.count diff --git a/tests/app/service/test_statistics.py b/tests/app/service/test_statistics.py index fd686be18..9c35ad9c7 100644 --- a/tests/app/service/test_statistics.py +++ b/tests/app/service/test_statistics.py @@ -30,6 +30,11 @@ StatsRow = collections.namedtuple('row', ('notification_type', 'status', 'count' StatsRow('email', 'temporary-failure', 1), StatsRow('email', 'permanent-failure', 1), ], [4, 0, 4], [0, 0, 0], [0, 0, 0]), + 'convert_sent_to_delivered': ([ + StatsRow('sms', 'sending', 1), + StatsRow('sms', 'delivered', 1), + StatsRow('sms', 'sent', 1), + ], [0, 0, 0], [3, 2, 0], [0, 0, 0]), }) def test_format_statistics(stats, email_counts, sms_counts, letter_counts): From 34de722e73e507604d30ce67f7b359c4a200c4a8 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Fri, 28 Apr 2017 13:32:23 +0100 Subject: [PATCH 46/46] Add test to check for sent notification --- tests/app/dao/test_jobs_dao.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index b3598fd7f..f01b631ba 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -43,6 +43,7 @@ def test_should_get_all_statuses_for_notifications_associated_with_job( notification(status='technical-failure') notification(status='temporary-failure') notification(status='permanent-failure') + notification(status='sent') results = dao_get_notification_outcomes_for_job(sample_service.id, sample_job.id) assert [(row.count, row.status) for row in results] == [ @@ -53,7 +54,8 @@ def test_should_get_all_statuses_for_notifications_associated_with_job( (1, 'failed'), (1, 'technical-failure'), (1, 'temporary-failure'), - (1, 'permanent-failure') + (1, 'permanent-failure'), + (1, 'sent') ]