From 86efcba7a33507c2a38d1740bda424981071085e Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 22 May 2017 11:33:24 +0100 Subject: [PATCH 01/46] Updated service DAO and API end points --- app/schemas.py | 4 ++++ tests/app/dao/test_services_dao.py | 1 + 2 files changed, 5 insertions(+) diff --git a/app/schemas.py b/app/schemas.py index ff4f83a95..cae2135f4 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -25,7 +25,11 @@ from notifications_utils.recipients import ( from app import ma from app import models +<<<<<<< HEAD from app.models import ServicePermission, INTERNATIONAL_SMS_TYPE, SMS_TYPE, LETTER_TYPE, EMAIL_TYPE +======= +from app.models import ServicePermission +>>>>>>> Updated service DAO and API end points from app.dao.permissions_dao import permission_dao from app.dao.service_permissions_dao import dao_fetch_service_permissions from app.utils import get_template_instance diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index b7b3176dd..98d9f2f36 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -11,6 +11,7 @@ from app.dao.services_dao import ( dao_create_service, dao_add_user_to_service, dao_remove_user_from_service, + dao_remove_service_permission as dao_services_remove_service_permission, dao_fetch_all_services, dao_fetch_service_by_id, dao_fetch_all_services_by_user, From 253614cd196413d2d1a34a60cc3e813461e954d2 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 22 May 2017 13:44:42 +0100 Subject: [PATCH 02/46] Update tests for existing flags to set service permissions --- tests/app/service/test_rest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 7b5fda823..6999593f8 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -159,7 +159,7 @@ def test_get_service_list_has_default_permissions(client, service_factory): assert response.status_code == 200 json_resp = json.loads(response.get_data(as_text=True)) assert len(json_resp['data']) == 3 - assert all([set(json['permissions']) == set([EMAIL_TYPE, SMS_TYPE]) for json in json_resp['data']]) + assert all([set(json['permissions']) & set([EMAIL_TYPE, SMS_TYPE]) for json in json_resp['data']]) def test_get_service_by_id_has_default_service_permissions(client, sample_service): From 67246f2da539e0f7d5c946be29e6d66f59aa6867 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 23 May 2017 13:42:46 +0100 Subject: [PATCH 03/46] Update service permissions to ensure state in sync --- app/schemas.py | 4 ---- tests/app/service/test_rest.py | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index cae2135f4..ff4f83a95 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -25,11 +25,7 @@ from notifications_utils.recipients import ( from app import ma from app import models -<<<<<<< HEAD from app.models import ServicePermission, INTERNATIONAL_SMS_TYPE, SMS_TYPE, LETTER_TYPE, EMAIL_TYPE -======= -from app.models import ServicePermission ->>>>>>> Updated service DAO and API end points from app.dao.permissions_dao import permission_dao from app.dao.service_permissions_dao import dao_fetch_service_permissions from app.utils import get_template_instance diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 6999593f8..7b5fda823 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -159,7 +159,7 @@ def test_get_service_list_has_default_permissions(client, service_factory): assert response.status_code == 200 json_resp = json.loads(response.get_data(as_text=True)) assert len(json_resp['data']) == 3 - assert all([set(json['permissions']) & set([EMAIL_TYPE, SMS_TYPE]) for json in json_resp['data']]) + assert all([set(json['permissions']) == set([EMAIL_TYPE, SMS_TYPE]) for json in json_resp['data']]) def test_get_service_by_id_has_default_service_permissions(client, sample_service): From eda3e412f7529bd752ae102051b2dd77c5c21f31 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 24 May 2017 11:40:57 +0100 Subject: [PATCH 04/46] Add svc perms migration script and update readme --- migrations/README | 4 ++- .../0086_migrate_default_svc_perms.py | 28 +++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) create mode 100644 migrations/versions/0086_migrate_default_svc_perms.py diff --git a/migrations/README b/migrations/README index 6c36a3e0e..7c44eae44 100644 --- a/migrations/README +++ b/migrations/README @@ -1,7 +1,9 @@ Generic single-database configuration. -python application.py db migration to generate migration script. +python application.py db migrate to generate migration script. python application.py db upgrade to upgrade db with script. python application.py db downgrade to rollback db changes. + +python application.py db current to show current script. diff --git a/migrations/versions/0086_migrate_default_svc_perms.py b/migrations/versions/0086_migrate_default_svc_perms.py new file mode 100644 index 000000000..6b740a969 --- /dev/null +++ b/migrations/versions/0086_migrate_default_svc_perms.py @@ -0,0 +1,28 @@ +"""empty message + +Revision ID: 0086_migrate_default_svc_perms +Revises: 0085_update_incoming_to_inbound +Create Date: 2017-05-23 18:13:03.532095 + +""" + +# revision identifiers, used by Alembic. +revision = '0086_migrate_default_svc_perms' +down_revision = '0085_update_incoming_to_inbound' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + def get_values(permission): + return "SELECT id, '{0}' FROM services WHERE "\ + "id NOT IN (SELECT service_id FROM service_permissions "\ + "WHERE service_id=id AND permission='{0}')".format(permission) + + op.execute("INSERT INTO service_permissions (service_id, permission) {}".format(get_values('sms'))) + op.execute("INSERT INTO service_permissions (service_id, permission) {}".format(get_values('email'))) + + +def downgrade(): + op.execute("DELETE FROM service_permissions WHERE created_at IS NULL") From 640ab665d04355d28e7c4c1a87c5d48ea5e364e6 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 24 May 2017 13:22:36 +0100 Subject: [PATCH 05/46] Update migration script for service permissions --- ..._perms.py => 0086_migrate_existing_svc_perms.py} | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) rename migrations/versions/{0086_migrate_default_svc_perms.py => 0086_migrate_existing_svc_perms.py} (54%) diff --git a/migrations/versions/0086_migrate_default_svc_perms.py b/migrations/versions/0086_migrate_existing_svc_perms.py similarity index 54% rename from migrations/versions/0086_migrate_default_svc_perms.py rename to migrations/versions/0086_migrate_existing_svc_perms.py index 6b740a969..14701ce6e 100644 --- a/migrations/versions/0086_migrate_default_svc_perms.py +++ b/migrations/versions/0086_migrate_existing_svc_perms.py @@ -1,13 +1,13 @@ """empty message -Revision ID: 0086_migrate_default_svc_perms +Revision ID: 0086_migrate_existing_svc_perms Revises: 0085_update_incoming_to_inbound Create Date: 2017-05-23 18:13:03.532095 """ # revision identifiers, used by Alembic. -revision = '0086_migrate_default_svc_perms' +revision = '0086_migrate_existing_svc_perms' down_revision = '0085_update_incoming_to_inbound' from alembic import op @@ -20,8 +20,17 @@ def upgrade(): "id NOT IN (SELECT service_id FROM service_permissions "\ "WHERE service_id=id AND permission='{0}')".format(permission) + def get_values_from_flag(permission, flag): + return "SELECT id, '{0}' FROM services WHERE "\ + "{1} AND id NOT IN (SELECT service_id FROM service_permissions "\ + "WHERE service_id=id AND permission='{0}')".format(permission, flag) + op.execute("INSERT INTO service_permissions (service_id, permission) {}".format(get_values('sms'))) op.execute("INSERT INTO service_permissions (service_id, permission) {}".format(get_values('email'))) + op.execute("INSERT INTO service_permissions (service_id, permission) {}".format( + get_values_from_flag('letter', 'can_send_letters'))) + op.execute("INSERT INTO service_permissions (service_id, permission) {}".format( + get_values_from_flag('international_sms', 'can_send_international_sms'))) def downgrade(): From a5ab427577293eed9f17d3f3d42810f6b58876d6 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 26 May 2017 14:56:22 +0100 Subject: [PATCH 06/46] Reword migration script --- ...perms.py => 0088_migrate_existing_svc_perms.py} | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) rename migrations/versions/{0086_migrate_existing_svc_perms.py => 0088_migrate_existing_svc_perms.py} (74%) diff --git a/migrations/versions/0086_migrate_existing_svc_perms.py b/migrations/versions/0088_migrate_existing_svc_perms.py similarity index 74% rename from migrations/versions/0086_migrate_existing_svc_perms.py rename to migrations/versions/0088_migrate_existing_svc_perms.py index 14701ce6e..2556d5bf0 100644 --- a/migrations/versions/0086_migrate_existing_svc_perms.py +++ b/migrations/versions/0088_migrate_existing_svc_perms.py @@ -1,14 +1,14 @@ """empty message -Revision ID: 0086_migrate_existing_svc_perms -Revises: 0085_update_incoming_to_inbound +Revision ID: 0088_migrate_existing_svc_perms +Revises: 0087_scheduled_notifications Create Date: 2017-05-23 18:13:03.532095 """ # revision identifiers, used by Alembic. -revision = '0086_migrate_existing_svc_perms' -down_revision = '0085_update_incoming_to_inbound' +revision = '0088_migrate_existing_svc_perms' +down_revision = '0087_scheduled_notifications' from alembic import op import sqlalchemy as sa @@ -20,7 +20,7 @@ def upgrade(): "id NOT IN (SELECT service_id FROM service_permissions "\ "WHERE service_id=id AND permission='{0}')".format(permission) - def get_values_from_flag(permission, flag): + def get_values_if_flag(permission, flag): return "SELECT id, '{0}' FROM services WHERE "\ "{1} AND id NOT IN (SELECT service_id FROM service_permissions "\ "WHERE service_id=id AND permission='{0}')".format(permission, flag) @@ -28,9 +28,9 @@ def upgrade(): op.execute("INSERT INTO service_permissions (service_id, permission) {}".format(get_values('sms'))) op.execute("INSERT INTO service_permissions (service_id, permission) {}".format(get_values('email'))) op.execute("INSERT INTO service_permissions (service_id, permission) {}".format( - get_values_from_flag('letter', 'can_send_letters'))) + get_values_if_flag('letter', 'can_send_letters'))) op.execute("INSERT INTO service_permissions (service_id, permission) {}".format( - get_values_from_flag('international_sms', 'can_send_international_sms'))) + get_values_if_flag('international_sms', 'can_send_international_sms'))) def downgrade(): From 4228e67486088b228203ae992f164394429397d3 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 26 May 2017 15:21:53 +0100 Subject: [PATCH 07/46] Removed dao_remove_service_permission from import --- tests/app/dao/test_services_dao.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 98d9f2f36..b7b3176dd 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -11,7 +11,6 @@ from app.dao.services_dao import ( dao_create_service, dao_add_user_to_service, dao_remove_user_from_service, - dao_remove_service_permission as dao_services_remove_service_permission, dao_fetch_all_services, dao_fetch_service_by_id, dao_fetch_all_services_by_user, From d2ce7518b43a74fa0734ed0cb483f71a2f0cafbe Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 26 May 2017 17:45:33 +0100 Subject: [PATCH 08/46] Added value for created_at --- .../0088_migrate_existing_svc_perms.py | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/migrations/versions/0088_migrate_existing_svc_perms.py b/migrations/versions/0088_migrate_existing_svc_perms.py index 2556d5bf0..44d830cf4 100644 --- a/migrations/versions/0088_migrate_existing_svc_perms.py +++ b/migrations/versions/0088_migrate_existing_svc_perms.py @@ -12,26 +12,29 @@ down_revision = '0087_scheduled_notifications' from alembic import op import sqlalchemy as sa +import time + +migration_date = time.strftime('2017-05-26 17:30:00.000000') def upgrade(): def get_values(permission): - return "SELECT id, '{0}' FROM services WHERE "\ + return "SELECT id, '{0}', '{1}' FROM services WHERE "\ "id NOT IN (SELECT service_id FROM service_permissions "\ - "WHERE service_id=id AND permission='{0}')".format(permission) + "WHERE service_id=id AND permission='{0}')".format(permission, migration_date) def get_values_if_flag(permission, flag): - return "SELECT id, '{0}' FROM services WHERE "\ - "{1} AND id NOT IN (SELECT service_id FROM service_permissions "\ - "WHERE service_id=id AND permission='{0}')".format(permission, flag) + return "SELECT id, '{0}', '{1}' FROM services WHERE "\ + "{2} AND id NOT IN (SELECT service_id FROM service_permissions "\ + "WHERE service_id=id AND permission='{0}')".format(permission, migration_date, flag) - op.execute("INSERT INTO service_permissions (service_id, permission) {}".format(get_values('sms'))) - op.execute("INSERT INTO service_permissions (service_id, permission) {}".format(get_values('email'))) - op.execute("INSERT INTO service_permissions (service_id, permission) {}".format( + op.execute("INSERT INTO service_permissions (service_id, permission, created_at) {}".format(get_values('sms'))) + op.execute("INSERT INTO service_permissions (service_id, permission, created_at) {}".format(get_values('email'))) + op.execute("INSERT INTO service_permissions (service_id, permission, created_at) {}".format( get_values_if_flag('letter', 'can_send_letters'))) - op.execute("INSERT INTO service_permissions (service_id, permission) {}".format( + op.execute("INSERT INTO service_permissions (service_id, permission, created_at) {}".format( get_values_if_flag('international_sms', 'can_send_international_sms'))) def downgrade(): - op.execute("DELETE FROM service_permissions WHERE created_at IS NULL") + op.execute("DELETE FROM service_permissions WHERE created_at = '{}'::timestamp".format(migration_date)) From c2d308c3b20df52869099159462a21de1a71506c Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 26 May 2017 17:51:06 +0100 Subject: [PATCH 09/46] Removed time import --- migrations/versions/0088_migrate_existing_svc_perms.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/migrations/versions/0088_migrate_existing_svc_perms.py b/migrations/versions/0088_migrate_existing_svc_perms.py index 44d830cf4..9e54ee4b4 100644 --- a/migrations/versions/0088_migrate_existing_svc_perms.py +++ b/migrations/versions/0088_migrate_existing_svc_perms.py @@ -12,9 +12,8 @@ down_revision = '0087_scheduled_notifications' from alembic import op import sqlalchemy as sa -import time -migration_date = time.strftime('2017-05-26 17:30:00.000000') +migration_date = '2017-05-26 17:30:00.000000' def upgrade(): From 400096520d582a51855939c48da357152e2e1bec Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 2 Jun 2017 16:51:27 +0100 Subject: [PATCH 10/46] Not null the provider column on the inbound SMS table. --- app/models.py | 2 +- .../versions/0093_notnull_inbound_provider.py | 26 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 migrations/versions/0093_notnull_inbound_provider.py diff --git a/app/models.py b/app/models.py index 6d5bf19fb..3303f397c 100644 --- a/app/models.py +++ b/app/models.py @@ -1166,7 +1166,7 @@ class InboundSms(db.Model): user_number = db.Column(db.String, nullable=False) # the end user's number, that the msg was sent from provider_date = db.Column(db.DateTime) provider_reference = db.Column(db.String) - provider = db.Column(db.String, nullable=True) + provider = db.Column(db.String, nullable=False) _content = db.Column('content', db.String, nullable=False) @property diff --git a/migrations/versions/0093_notnull_inbound_provider.py b/migrations/versions/0093_notnull_inbound_provider.py new file mode 100644 index 000000000..35d6a9a65 --- /dev/null +++ b/migrations/versions/0093_notnull_inbound_provider.py @@ -0,0 +1,26 @@ +"""empty message + +Revision ID: 0093_notnull_inbound_provider +Revises: 0092_populate_inbound_provider +Create Date: 2017-06-02 16:50:11.698423 + +""" + +# revision identifiers, used by Alembic. +revision = '0093_notnull_inbound_provider' +down_revision = '0092_populate_inbound_provider' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + op.alter_column('inbound_sms', 'provider', + existing_type=sa.VARCHAR(), + nullable=False) + + +def downgrade(): + op.alter_column('inbound_sms', 'provider', + existing_type=sa.VARCHAR(), + nullable=True) From 21a5f018048482eccc5d054a1160b795b56a61a7 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Mon, 5 Jun 2017 13:10:54 +0100 Subject: [PATCH 11/46] Not null in own PR --- ...bound_provider.py => 0094_notnull_inbound_provider.py} | 8 ++++---- tests/app/db.py | 4 +++- 2 files changed, 7 insertions(+), 5 deletions(-) rename migrations/versions/{0093_notnull_inbound_provider.py => 0094_notnull_inbound_provider.py} (71%) diff --git a/migrations/versions/0093_notnull_inbound_provider.py b/migrations/versions/0094_notnull_inbound_provider.py similarity index 71% rename from migrations/versions/0093_notnull_inbound_provider.py rename to migrations/versions/0094_notnull_inbound_provider.py index 35d6a9a65..ebf98e3a7 100644 --- a/migrations/versions/0093_notnull_inbound_provider.py +++ b/migrations/versions/0094_notnull_inbound_provider.py @@ -1,14 +1,14 @@ """empty message -Revision ID: 0093_notnull_inbound_provider -Revises: 0092_populate_inbound_provider +Revision ID: 0094_notnull_inbound_provider +Revises: 0093_populate_inbound_provider Create Date: 2017-06-02 16:50:11.698423 """ # revision identifiers, used by Alembic. -revision = '0093_notnull_inbound_provider' -down_revision = '0092_populate_inbound_provider' +revision = '0094_notnull_inbound_provider' +down_revision = '0093_populate_inbound_provider' from alembic import op import sqlalchemy as sa diff --git a/tests/app/db.py b/tests/app/db.py index 8fcced1b1..bfe292a97 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -193,7 +193,8 @@ def create_inbound_sms( user_number='447700900111', provider_date=None, provider_reference=None, - content='Hello' + content='Hello', + provider="mmg" ): inbound = InboundSms( service=service, @@ -203,6 +204,7 @@ def create_inbound_sms( provider_date=provider_date or datetime.utcnow(), provider_reference=provider_reference or 'foo', content=content, + provider=provider ) dao_create_inbound_sms(inbound) return inbound From bad0ce3d86ef8688444d7b8e93635be142fad9c2 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 5 Jun 2017 15:53:57 +0100 Subject: [PATCH 12/46] Return personalisation in notification dump in search by to --- app/dao/notifications_dao.py | 2 +- tests/app/dao/test_notification_dao.py | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 22da11966..356b74ed2 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -494,7 +494,7 @@ def dao_get_notifications_by_to_field(service_id, search_term, statuses=None): if statuses: filters.append(Notification.status.in_(statuses)) - results = db.session.query(Notification).filter(*filters).all() + results = db.session.query(Notification).filter(*filters).order_by(desc(Notification.created_at)).all() return results diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index dc0f3fc34..086a24c57 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -1903,3 +1903,24 @@ def test_dao_get_notifications_by_to_field_returns_all_if_no_status_filter(sampl assert len(notifications) == 2 assert notification1.id in notification_ids assert notification2.id in notification_ids + + +@freeze_time('2016-01-01 11:10:00') +def test_dao_get_notifications_by_to_field_orders_by_created_at_desc(sample_template): + notification = partial( + create_notification, + template=sample_template, + to_field='+447700900855', + normalised_to='447700900855' + ) + + notification_a_minute_ago = notification(created_at=datetime.utcnow() - timedelta(minutes=1)) + notification = notification(created_at=datetime.utcnow()) + + notifications = dao_get_notifications_by_to_field( + sample_template.service_id, '+447700900855' + ) + + assert len(notifications) == 2 + assert notifications[0].id == notification.id + assert notifications[1].id == notification_a_minute_ago.id From 3177a6ddc46253d518c4b458c6af5860093800a8 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 5 Jun 2017 15:54:40 +0100 Subject: [PATCH 13/46] Return personalisation dump of notifications + small refactor --- app/notifications/rest.py | 10 +++--- app/service/rest.py | 3 +- tests/app/service/test_rest.py | 56 ++++++++++++++++++++++++++-------- 3 files changed, 49 insertions(+), 20 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index bb136f8c8..e831efa9a 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -11,6 +11,10 @@ from app.dao import ( templates_dao, notifications_dao ) +from app.errors import ( + register_errors, + InvalidRequest +) from app.models import KEY_TYPE_TEAM, PRIORITY from app.models import SMS_TYPE from app.notifications.process_notifications import ( @@ -38,12 +42,6 @@ from notifications_utils.recipients import get_international_phone_info notifications = Blueprint('notifications', __name__) -from app.errors import ( - register_errors, - InvalidRequest -) - - register_errors(notifications) diff --git a/app/service/rest.py b/app/service/rest.py index 24fdcd513..4826c8814 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -59,6 +59,7 @@ from app.schemas import ( user_schema, permission_schema, notification_with_template_schema, + notification_with_personalisation_schema, notifications_filter_schema, detailed_service_schema ) @@ -303,7 +304,7 @@ def get_all_notifications_for_service(service_id): def search_for_notification_by_to_field(service_id, search_term, statuses): results = notifications_dao.dao_get_notifications_by_to_field(service_id, search_term, statuses) return jsonify( - notifications=notification_with_template_schema.dump(results, many=True).data + notifications=notification_with_personalisation_schema.dump(results, many=True).data ), 200 diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index f2b0a8247..7a6c71627 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -10,7 +10,12 @@ 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, Rate, ServicePermission +from app.models import ( + Organisation, Rate, Service, ServicePermission, User, + KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, + EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INBOUND_SMS_TYPE, + DVLA_ORG_LAND_REGISTRY +) from tests import create_authorization_header from tests.app.db import create_template from tests.app.conftest import ( @@ -20,11 +25,6 @@ from tests.app.conftest import ( sample_notification_history as create_notification_history, sample_notification_with_job ) -from app.models import ( - Service, ServicePermission, - KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, - EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INBOUND_SMS_TYPE -) from tests.app.db import create_user @@ -1438,13 +1438,12 @@ def test_get_services_with_detailed_flag(client, notify_db, notify_db_session): def test_get_services_with_detailed_flag_excluding_from_test_key(notify_api, notify_db, notify_db_session): - notifications = [ - create_sample_notification(notify_db, notify_db_session, key_type=KEY_TYPE_NORMAL), - create_sample_notification(notify_db, notify_db_session, key_type=KEY_TYPE_TEAM), - create_sample_notification(notify_db, notify_db_session, key_type=KEY_TYPE_TEST), - create_sample_notification(notify_db, notify_db_session, key_type=KEY_TYPE_TEST), - create_sample_notification(notify_db, notify_db_session, key_type=KEY_TYPE_TEST) - ] + create_sample_notification(notify_db, notify_db_session, key_type=KEY_TYPE_NORMAL), + create_sample_notification(notify_db, notify_db_session, key_type=KEY_TYPE_TEAM), + create_sample_notification(notify_db, notify_db_session, key_type=KEY_TYPE_TEST), + create_sample_notification(notify_db, notify_db_session, key_type=KEY_TYPE_TEST), + create_sample_notification(notify_db, notify_db_session, key_type=KEY_TYPE_TEST) + with notify_api.test_request_context(), notify_api.test_client() as client: resp = client.get( '/service?detailed=True&include_from_test_key=False', @@ -2051,3 +2050,34 @@ def test_search_for_notification_by_to_field_filters_by_statuses(client, notify_ assert len(notifications) == 2 assert str(notification1.id) in notification_ids assert str(notification2.id) in notification_ids + + +def test_search_for_notification_by_to_field_returns_content( + client, + notify_db, + notify_db_session, + sample_template_with_placeholders +): + notification = create_sample_notification( + notify_db, + notify_db_session, + to_field='+447700900855', + normalised_to='447700900855', + template=sample_template_with_placeholders, + personalisation={"name": "Foo"} + ) + + response = client.get( + '/service/{}/notifications?to={}'.format( + sample_template_with_placeholders.service_id, '+447700900855' + ), + headers=[create_authorization_header()] + ) + notifications = json.loads(response.get_data(as_text=True))['notifications'] + + assert response.status_code == 200 + assert len(notifications) == 1 + + assert notifications[0]['id'] == str(notification.id) + assert notifications[0]['to'] == '+447700900855' + assert notifications[0]['body'] == 'Hello Foo\nYour thing is due soon' From 7a03ef3de457011f63202dcb93f5fa955cd89e3f Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Mon, 5 Jun 2017 17:25:40 +0100 Subject: [PATCH 14/46] Pseudo Code --- app/dao/notification_usage_dao.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/dao/notification_usage_dao.py b/app/dao/notification_usage_dao.py index b3b582ed3..97fc4925e 100644 --- a/app/dao/notification_usage_dao.py +++ b/app/dao/notification_usage_dao.py @@ -176,8 +176,14 @@ def get_total_billable_units_for_sent_sms_notifications_in_date_range(start_date ) billable_units_by_rate_boundry = result.scalar() if billable_units_by_rate_boundry: + if billable_units >= 250000: + total_cost += int(billable_units_by_rate_boundry) * rate_boundary['rate'] + elif billable_units + billable_units_by_rate_boundry > 250000: + remaining_free_allowance = abs(250000 - billable_units) + total_cost += ((billable_units_by_rate_boundry - remaining_free_allowance) * rate_boundary) + else + total_cost += 0 billable_units += int(billable_units_by_rate_boundry) - total_cost += int(billable_units_by_rate_boundry) * rate_boundary['rate'] return billable_units, total_cost From 18dcc10a0695458df397386af292f5ddfbc9c491 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 6 Jun 2017 14:04:11 +0100 Subject: [PATCH 15/46] Fixed typo --- app/dao/notification_usage_dao.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/dao/notification_usage_dao.py b/app/dao/notification_usage_dao.py index 97fc4925e..d51e96153 100644 --- a/app/dao/notification_usage_dao.py +++ b/app/dao/notification_usage_dao.py @@ -181,7 +181,7 @@ def get_total_billable_units_for_sent_sms_notifications_in_date_range(start_date elif billable_units + billable_units_by_rate_boundry > 250000: remaining_free_allowance = abs(250000 - billable_units) total_cost += ((billable_units_by_rate_boundry - remaining_free_allowance) * rate_boundary) - else + else: total_cost += 0 billable_units += int(billable_units_by_rate_boundry) From 75bf693f444cc7a5fe903a19a4c30a75eccda169 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 6 Jun 2017 14:49:05 +0100 Subject: [PATCH 16/46] Add the yearly free limit to the service model. This allows us to reference it across the API code base and return it in the API. But not currently attached to the service DB model - a static method on the class. --- app/config.py | 2 ++ app/models.py | 4 ++++ app/schemas.py | 7 +++++-- tests/app/service/test_rest.py | 11 +++++++++++ 4 files changed, 22 insertions(+), 2 deletions(-) diff --git a/app/config.py b/app/config.py index 01aca1964..07d451549 100644 --- a/app/config.py +++ b/app/config.py @@ -239,6 +239,8 @@ class Config(object): } } + FREE_SMS_TIER_FRAGMENT_COUNT = 250000 + ###################### # Config overrides ### diff --git a/app/models.py b/app/models.py index 1bcd66a19..a37959156 100644 --- a/app/models.py +++ b/app/models.py @@ -217,6 +217,10 @@ class Service(db.Model, Versioned): self.can_send_letters = LETTER_TYPE in [p.permission for p in self.permissions] self.can_send_international_sms = INTERNATIONAL_SMS_TYPE in [p.permission for p in self.permissions] + @staticmethod + def free_sms_fragment_limit(): + return current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] + @classmethod def from_json(cls, data): """ diff --git a/app/schemas.py b/app/schemas.py index b8d3ee7af..3ca8fda1a 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -25,9 +25,8 @@ from notifications_utils.recipients import ( from app import ma from app import models -from app.models import ServicePermission, INTERNATIONAL_SMS_TYPE, SMS_TYPE, LETTER_TYPE, EMAIL_TYPE +from app.models import ServicePermission, INTERNATIONAL_SMS_TYPE, LETTER_TYPE from app.dao.permissions_dao import permission_dao -from app.dao.service_permissions_dao import dao_fetch_service_permissions from app.utils import get_template_instance @@ -176,6 +175,7 @@ class ProviderDetailsHistorySchema(BaseSchema): class ServiceSchema(BaseSchema): + free_sms_fragment_limit = fields.Method('get_free_sms_fragment_limit') created_by = field_for(models.Service, 'created_by', required=True) organisation = field_for(models.Service, 'organisation') branding = field_for(models.Service, 'branding') @@ -183,6 +183,9 @@ class ServiceSchema(BaseSchema): permissions = fields.Method("service_permissions") override_flag = False + def get_free_sms_fragment_limit(selfs, service): + return service.free_sms_fragment_limit() + def service_permissions(self, service): return [p.permission for p in service.permissions] diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index f2b0a8247..2c2842177 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -147,6 +147,17 @@ def test_get_service_by_id(client, sample_service): assert json_resp['data']['sms_sender'] == current_app.config['FROM_NUMBER'] +def test_get_service_by_id_returns_free_sms_limit(client, sample_service): + auth_header = create_authorization_header() + resp = client.get( + '/service/{}'.format(sample_service.id), + headers=[auth_header] + ) + assert resp.status_code == 200 + json_resp = json.loads(resp.get_data(as_text=True)) + assert json_resp['data']['free_sms_fragment_limit'] == 250000 + + def test_get_service_list_has_default_permissions(client, service_factory): service_factory.get('one') service_factory.get('two') From 96d30d31b1adb671e6b58266f264946fa9904812 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 6 Jun 2017 14:55:37 +0100 Subject: [PATCH 17/46] Get existing tests to pass. Done by ensuring that the rate limit is 0, so that all messages are billable. --- app/dao/notification_usage_dao.py | 12 +++++----- tests/app/dao/test_notification_usage_dao.py | 23 ++++++++++++++++++-- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/app/dao/notification_usage_dao.py b/app/dao/notification_usage_dao.py index d51e96153..1ee3134bd 100644 --- a/app/dao/notification_usage_dao.py +++ b/app/dao/notification_usage_dao.py @@ -1,5 +1,6 @@ from datetime import datetime, timedelta +from flask import current_app from sqlalchemy import Float, Integer from sqlalchemy import func, case, cast from sqlalchemy import literal_column @@ -11,7 +12,7 @@ from app.models import (NotificationHistory, NOTIFICATION_STATUS_TYPES_BILLABLE, KEY_TYPE_TEST, SMS_TYPE, - EMAIL_TYPE) + EMAIL_TYPE, Service) from app.statsd_decorators import statsd from app.utils import get_london_month_from_utc_column @@ -158,6 +159,8 @@ def rate_multiplier(): @statsd(namespace="dao") def get_total_billable_units_for_sent_sms_notifications_in_date_range(start_date, end_date, service_id): + free_sms_limit = Service.free_sms_fragment_limit() + billable_units = 0 total_cost = 0.0 @@ -176,15 +179,14 @@ def get_total_billable_units_for_sent_sms_notifications_in_date_range(start_date ) billable_units_by_rate_boundry = result.scalar() if billable_units_by_rate_boundry: - if billable_units >= 250000: + if billable_units >= free_sms_limit: total_cost += int(billable_units_by_rate_boundry) * rate_boundary['rate'] - elif billable_units + billable_units_by_rate_boundry > 250000: - remaining_free_allowance = abs(250000 - billable_units) + elif billable_units + billable_units_by_rate_boundry > free_sms_limit: + remaining_free_allowance = abs(free_sms_limit - billable_units) total_cost += ((billable_units_by_rate_boundry - remaining_free_allowance) * rate_boundary) else: total_cost += 0 billable_units += int(billable_units_by_rate_boundry) - return billable_units, total_cost diff --git a/tests/app/dao/test_notification_usage_dao.py b/tests/app/dao/test_notification_usage_dao.py index d4aec8531..c20b76a4c 100644 --- a/tests/app/dao/test_notification_usage_dao.py +++ b/tests/app/dao/test_notification_usage_dao.py @@ -2,6 +2,7 @@ import uuid from datetime import datetime, timedelta import pytest +from flask import current_app from app.dao.date_util import get_financial_year from app.dao.notification_usage_dao import ( @@ -15,8 +16,7 @@ from app.models import ( Rate, NOTIFICATION_DELIVERED, NOTIFICATION_STATUS_TYPES_BILLABLE, - NOTIFICATION_STATUS_TYPES_NON_BILLABLE, - Notification) + NOTIFICATION_STATUS_TYPES_NON_BILLABLE) from tests.app.conftest import sample_notification, sample_email_template, sample_letter_template, sample_service from tests.app.db import create_notification from freezegun import freeze_time @@ -266,6 +266,8 @@ def set_up_rate(notify_db, start_date, value): @freeze_time("2016-01-10 12:00:00.000000") def test_returns_total_billable_units_for_sms_notifications(notify_db, notify_db_session, sample_service): + current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = 0 + set_up_rate(notify_db, datetime(2016, 1, 1), 0.016) sample_notification( @@ -288,6 +290,8 @@ def test_returns_total_billable_units_for_sms_notifications(notify_db, notify_db def test_returns_total_billable_units_multiplied_by_multipler_for_sms_notifications( notify_db, notify_db_session, sample_service ): + current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = 0 + set_up_rate(notify_db, datetime(2016, 1, 1), 2.5) sample_notification( @@ -309,6 +313,8 @@ def test_returns_total_billable_units_multiplied_by_multipler_for_sms_notificati def test_returns_total_billable_units_multiplied_by_multipler_for_sms_notifications_for_several_rates( notify_db, notify_db_session, sample_service ): + current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = 0 + set_up_rate(notify_db, datetime(2016, 1, 1), 2) set_up_rate(notify_db, datetime(2016, 10, 1), 4) set_up_rate(notify_db, datetime(2017, 1, 1), 6) @@ -350,6 +356,8 @@ def test_returns_total_billable_units_multiplied_by_multipler_for_sms_notificati def test_returns_total_billable_units_for_sms_notifications_for_several_rates_where_dates_match_rate_boundary( notify_db, notify_db_session, sample_service ): + current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = 0 + set_up_rate(notify_db, datetime(2016, 1, 1), 2) set_up_rate(notify_db, datetime(2016, 10, 1), 4) set_up_rate(notify_db, datetime(2017, 1, 1), 6) @@ -388,6 +396,8 @@ def test_returns_total_billable_units_for_sms_notifications_for_several_rates_wh def test_returns_total_billable_units_for_sms_notifications_ignoring_letters_and_emails( notify_db, notify_db_session, sample_service ): + current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = 0 + set_up_rate(notify_db, datetime(2016, 1, 1), 2.5) email_template = sample_email_template(notify_db, notify_db_session, service=sample_service) @@ -426,6 +436,8 @@ def test_returns_total_billable_units_for_sms_notifications_ignoring_letters_and def test_returns_total_billable_units_for_sms_notifications_for_only_requested_service( notify_db, notify_db_session ): + current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = 0 + set_up_rate(notify_db, datetime(2016, 1, 1), 2.5) service_1 = sample_service(notify_db, notify_db_session, service_name=str(uuid.uuid4())) @@ -463,6 +475,8 @@ def test_returns_total_billable_units_for_sms_notifications_for_only_requested_s def test_returns_total_billable_units_for_sms_notifications_handling_null_values( notify_db, notify_db_session, sample_service ): + current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = 0 + set_up_rate(notify_db, datetime(2016, 1, 1), 2.5) sample_notification( @@ -488,6 +502,9 @@ def test_returns_total_billable_units_for_sms_notifications_handling_null_values def test_ignores_non_billable_states_when_returning_billable_units_for_sms_notifications( notify_db, notify_db_session, sample_service, billable_units, states ): + + current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = 0 + set_up_rate(notify_db, datetime(2016, 1, 1), 2.5) for state in states: @@ -514,6 +531,8 @@ def test_ignores_non_billable_states_when_returning_billable_units_for_sms_notif def test_restricts_to_time_period_when_returning_billable_units_for_sms_notifications( notify_db, notify_db_session, sample_service ): + current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = 0 + set_up_rate(notify_db, datetime(2016, 1, 1), 2.5) sample_notification( From 6b4597149f42256d51da5b9f92c3cda5ff017da3 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 6 Jun 2017 16:01:27 +0100 Subject: [PATCH 18/46] Add filter to get jobs to delete (sms, email, letter) --- app/dao/jobs_dao.py | 26 +++++++------- tests/app/conftest.py | 22 ++++++------ tests/app/dao/test_jobs_dao.py | 66 ++++++++++++++++++++++------------ 3 files changed, 69 insertions(+), 45 deletions(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index d2ec8c367..86e565619 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -1,17 +1,15 @@ -from datetime import datetime +from datetime import datetime, timedelta from flask import current_app from sqlalchemy import func, desc, asc, cast, Date as sql_date from app import db from app.dao import days_ago -from app.models import (Job, - Notification, - NotificationHistory, - Template, - JOB_STATUS_SCHEDULED, - JOB_STATUS_PENDING, - LETTER_TYPE, JobStatistics) +from app.models import ( + Job, JobStatistics, Notification, NotificationHistory, Template, + JOB_STATUS_SCHEDULED, JOB_STATUS_PENDING, + EMAIL_TYPE, SMS_TYPE, LETTER_TYPE +) from app.statsd_decorators import statsd @@ -129,10 +127,14 @@ def dao_update_job_status(job_id, status): db.session.commit() -def dao_get_jobs_older_than_limited_by(older_than=7, limit_days=2): - return Job.query.filter( - cast(Job.created_at, sql_date) < days_ago(older_than), - cast(Job.created_at, sql_date) >= days_ago(older_than + limit_days) +def dao_get_jobs_older_than_limited_by(job_types, older_than=7, limit_days=2): + end_date = datetime.utcnow() - timedelta(days=older_than) + start_date = end_date - timedelta(days=limit_days) + + return Job.query.join(Template).filter( + Job.created_at < end_date, + Job.created_at >= start_date, + Template.template_type.in_(job_types) ).order_by(desc(Job.created_at)).all() diff --git a/tests/app/conftest.py b/tests/app/conftest.py index d49c527aa..91eff00fc 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -280,16 +280,18 @@ def sample_team_api_key(notify_db, notify_db_session, service=None): @pytest.fixture(scope='function') -def sample_job(notify_db, - notify_db_session, - service=None, - template=None, - notification_count=1, - created_at=None, - job_status='pending', - scheduled_for=None, - processing_started=None, - original_file_name='some.csv'): +def sample_job( + notify_db, + notify_db_session, + service=None, + template=None, + notification_count=1, + created_at=None, + job_status='pending', + scheduled_for=None, + processing_started=None, + original_file_name='some.csv' +): if service is None: service = sample_service(notify_db, notify_db_session) if template is None: diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index a9af1afda..42f93d230 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -17,7 +17,10 @@ from app.dao.jobs_dao import ( dao_update_job_status, dao_get_all_notifications_for_job, dao_get_jobs_older_than_limited_by) -from app.models import Job, JobStatistics +from app.models import ( + Job, JobStatistics, + EMAIL_TYPE, SMS_TYPE, LETTER_TYPE +) from tests.app.conftest import sample_notification as create_notification from tests.app.conftest import sample_job as create_job @@ -285,33 +288,30 @@ def test_get_future_scheduled_job_gets_a_job_yet_to_send(sample_scheduled_job): assert result.id == sample_scheduled_job.id -def test_should_get_jobs_seven_days_old(notify_db, notify_db_session): - # job runs at some point on each day - # shouldn't matter when, we are deleting things 7 days ago - job_run_time = '2016-10-31T10:00:00' +@freeze_time('2016-10-31 10:00:00') +def test_should_get_jobs_seven_days_old(notify_db, notify_db_session, sample_template): + """ + Jobs older than seven days are deleted, but only two day's worth (two-day window) + """ + seven_days_ago = datetime.utcnow() - timedelta(days=7) + within_seven_days = seven_days_ago + timedelta(seconds=1) - # running on the 31st means the previous 7 days are ignored + eight_days_ago = seven_days_ago - timedelta(days=1) - # 2 day window for delete jobs - # 7 days of files to skip includes the 30,29,28,27,26,25,24th, so the.... - last_possible_time_for_eligible_job = '2016-10-23T23:59:59' - first_possible_time_for_eligible_job = '2016-10-22T00:00:00' + nine_days_ago = eight_days_ago - timedelta(days=2) + nine_days_one_second_ago = nine_days_ago - timedelta(seconds=1) - job_1 = create_job(notify_db, notify_db_session, created_at=last_possible_time_for_eligible_job) - job_2 = create_job(notify_db, notify_db_session, created_at=first_possible_time_for_eligible_job) + job = partial(create_job, notify_db, notify_db_session) + job(created_at=seven_days_ago) + job(created_at=within_seven_days) + job_to_delete = job(created_at=eight_days_ago) + job(created_at=nine_days_ago) + job(created_at=nine_days_one_second_ago) - # bookmarks for jobs that should be ignored - last_possible_time_for_ineligible_job = '2016-10-24T00:00:00' - create_job(notify_db, notify_db_session, created_at=last_possible_time_for_ineligible_job) + jobs = dao_get_jobs_older_than_limited_by(job_types=[sample_template.template_type]) - first_possible_time_for_ineligible_job = '2016-10-21T23:59:59' - create_job(notify_db, notify_db_session, created_at=first_possible_time_for_ineligible_job) - - with freeze_time(job_run_time): - jobs = dao_get_jobs_older_than_limited_by() - assert len(jobs) == 2 - assert jobs[0].id == job_1.id - assert jobs[1].id == job_2.id + assert len(jobs) == 1 + assert jobs[0].id == job_to_delete.id def test_get_jobs_for_service_is_paginated(notify_db, notify_db_session, sample_service, sample_template): @@ -391,3 +391,23 @@ def test_dao_update_job_status(sample_job): updated_job = Job.query.get(sample_job.id) assert updated_job.job_status == 'sent to dvla' assert updated_job.updated_at + + +@freeze_time('2016-10-31 10:00:00') +def test_should_get_jobs_seven_days_old_filters_type(notify_db, notify_db_session): + eight_days_ago = datetime.utcnow() - timedelta(days=8) + letter_template = create_template(notify_db, notify_db_session, template_type=LETTER_TYPE) + sms_template = create_template(notify_db, notify_db_session, template_type=SMS_TYPE) + email_template = create_template(notify_db, notify_db_session, template_type=EMAIL_TYPE) + + job = partial(create_job, notify_db, notify_db_session, created_at=eight_days_ago) + job_to_remain = job(template=letter_template) + job(template=sms_template) + job(template=email_template) + + jobs = dao_get_jobs_older_than_limited_by( + job_types=[EMAIL_TYPE, SMS_TYPE] + ) + + assert len(jobs) == 2 + assert job_to_remain.id not in [job.id for job in jobs] From 74a8905be91fbb25784ce3a1bd793ce8da7a9130 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 6 Jun 2017 16:02:01 +0100 Subject: [PATCH 19/46] Seperate deletion of jobs: * Two separate jobs, one for sms&email and another for letter * Change celery task for delete to accept template type filter * General refacor of tests to make more readable --- app/celery/scheduled_tasks.py | 4 +- app/config.py | 16 ++++-- tests/app/celery/test_scheduled_tasks.py | 63 ++++++++++++++++++------ 3 files changed, 64 insertions(+), 19 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 6a9cfd055..636522f01 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -32,8 +32,8 @@ from app.config import QueueNames @notify_celery.task(name="remove_csv_files") @statsd(namespace="tasks") -def remove_csv_files(): - jobs = dao_get_jobs_older_than_limited_by() +def remove_csv_files(job_types): + jobs = dao_get_jobs_older_than_limited_by(job_types=job_types) for job in jobs: s3.remove_job_from_s3(job.service_id, job.id) current_app.logger.info("Job ID {} has been removed from s3.".format(job.id)) diff --git a/app/config.py b/app/config.py index 01aca1964..cbaa15d67 100644 --- a/app/config.py +++ b/app/config.py @@ -3,7 +3,10 @@ from celery.schedules import crontab from kombu import Exchange, Queue import os -from app.models import KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST +from app.models import ( + EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, + KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST +) if os.environ.get('VCAP_SERVICES'): # on cloudfoundry, config is a json blob in VCAP_SERVICES - unpack it, and populate @@ -189,10 +192,17 @@ class Config(object): 'schedule': crontab(minute=0, hour=3), 'options': {'queue': QueueNames.PERIODIC} }, - 'remove_csv_files': { + 'remove_sms_email_jobs': { 'task': 'remove_csv_files', 'schedule': crontab(minute=0, hour=4), - 'options': {'queue': QueueNames.PERIODIC} + 'options': {'queue': QueueNames.PERIODIC}, + 'kwargs': {'job_types': [EMAIL_TYPE, SMS_TYPE]} + }, + 'remove_letter_jobs': { + 'task': 'remove_csv_files', + 'schedule': crontab(minute=20, hour=4), + 'options': {'queue': QueueNames.PERIODIC}, + 'kwargs': {'job_types': [LETTER_TYPE]} }, 'timeout-job-statistics': { 'task': 'timeout-job-statistics', diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index f706db28c..b3ff5d478 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -30,9 +30,12 @@ from app.dao.provider_details_dao import ( dao_update_provider_details, get_current_provider ) -from app.models import Service, Template +from app.models import ( + Service, Template, + SMS_TYPE, LETTER_TYPE +) from app.utils import get_london_midnight_in_utc -from tests.app.db import create_notification, create_service +from tests.app.db import create_notification, create_service, create_template, create_job from tests.app.conftest import ( sample_job as create_sample_job, sample_notification_history as create_notification_history, @@ -214,22 +217,33 @@ def test_should_update_all_scheduled_jobs_and_put_on_queue(notify_db, notify_db_ ]) -def test_will_remove_csv_files_for_jobs_older_than_seven_days(notify_db, notify_db_session, mocker): +@freeze_time('2016-10-18T10:00:00') +def test_will_remove_csv_files_for_jobs_older_than_seven_days( + notify_db, notify_db_session, mocker, sample_template +): mocker.patch('app.celery.scheduled_tasks.s3.remove_job_from_s3') + """ + Jobs older than seven days are deleted, but only two day's worth (two-day window) + """ + seven_days_ago = datetime.utcnow() - timedelta(days=7) + just_under_seven_days = seven_days_ago + timedelta(seconds=1) + eight_days_ago = seven_days_ago - timedelta(days=1) + nine_days_ago = eight_days_ago - timedelta(days=1) + just_under_nine_days = nine_days_ago + timedelta(seconds=1) + nine_days_one_second_ago = nine_days_ago - timedelta(seconds=1) - eligible_job_1 = datetime(2016, 10, 10, 23, 59, 59, 000) - eligible_job_2 = datetime(2016, 10, 9, 00, 00, 00, 000) - in_eligible_job_too_new = datetime(2016, 10, 11, 00, 00, 00, 000) - in_eligible_job_too_old = datetime(2016, 10, 8, 23, 59, 59, 999) + create_sample_job(notify_db, notify_db_session, created_at=nine_days_one_second_ago) + job1_to_delete = create_sample_job(notify_db, notify_db_session, created_at=eight_days_ago) + job2_to_delete = create_sample_job(notify_db, notify_db_session, created_at=just_under_nine_days) + create_sample_job(notify_db, notify_db_session, created_at=seven_days_ago) + create_sample_job(notify_db, notify_db_session, created_at=just_under_seven_days) - job_1 = create_sample_job(notify_db, notify_db_session, created_at=eligible_job_1) - job_2 = create_sample_job(notify_db, notify_db_session, created_at=eligible_job_2) - create_sample_job(notify_db, notify_db_session, created_at=in_eligible_job_too_new) - create_sample_job(notify_db, notify_db_session, created_at=in_eligible_job_too_old) + remove_csv_files(job_types=[sample_template.template_type]) - with freeze_time('2016-10-18T10:00:00'): - remove_csv_files() - assert s3.remove_job_from_s3.call_args_list == [call(job_1.service_id, job_1.id), call(job_2.service_id, job_2.id)] + assert s3.remove_job_from_s3.call_args_list == [ + call(job1_to_delete.service_id, job1_to_delete.id), + call(job2_to_delete.service_id, job2_to_delete.id) + ] def test_send_daily_performance_stats_calls_does_not_send_if_inactive( @@ -453,3 +467,24 @@ def test_should_call_delete_inbound_sms_older_than_seven_days(notify_api, mocker mocker.patch('app.celery.scheduled_tasks.delete_inbound_sms_created_more_than_a_week_ago') delete_inbound_sms_older_than_seven_days() assert scheduled_tasks.delete_inbound_sms_created_more_than_a_week_ago.call_count == 1 + + +@freeze_time('2017-01-01 10:00:00') +def test_remove_csv_files_filters_by_type(mocker, sample_service): + mocker.patch('app.celery.scheduled_tasks.s3.remove_job_from_s3') + """ + Jobs older than seven days are deleted, but only two day's worth (two-day window) + """ + letter_template = create_template(service=sample_service, template_type=LETTER_TYPE) + sms_template = create_template(service=sample_service, template_type=SMS_TYPE) + + eight_days_ago = datetime.utcnow() - timedelta(days=8) + + job_to_delete = create_job(template=letter_template, created_at=eight_days_ago) + create_job(template=sms_template, created_at=eight_days_ago) + + remove_csv_files(job_types=[LETTER_TYPE]) + + assert s3.remove_job_from_s3.call_args_list == [ + call(job_to_delete.service_id, job_to_delete.id), + ] From cad195949ac4259d0a4e996380c6d248a335abf7 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 6 Jun 2017 16:21:05 +0100 Subject: [PATCH 20/46] Ensure that the bill includes whatever free allowance is applicable. --- app/dao/notification_usage_dao.py | 9 +-- tests/app/dao/test_notification_usage_dao.py | 76 ++++++++++++++++++++ 2 files changed, 81 insertions(+), 4 deletions(-) diff --git a/app/dao/notification_usage_dao.py b/app/dao/notification_usage_dao.py index 1ee3134bd..995462509 100644 --- a/app/dao/notification_usage_dao.py +++ b/app/dao/notification_usage_dao.py @@ -179,14 +179,15 @@ def get_total_billable_units_for_sent_sms_notifications_in_date_range(start_date ) billable_units_by_rate_boundry = result.scalar() if billable_units_by_rate_boundry: + int_billable_units_by_rate_boundry = int(billable_units_by_rate_boundry) if billable_units >= free_sms_limit: - total_cost += int(billable_units_by_rate_boundry) * rate_boundary['rate'] - elif billable_units + billable_units_by_rate_boundry > free_sms_limit: + total_cost += int_billable_units_by_rate_boundry * rate_boundary['rate'] + elif billable_units + int_billable_units_by_rate_boundry > free_sms_limit: remaining_free_allowance = abs(free_sms_limit - billable_units) - total_cost += ((billable_units_by_rate_boundry - remaining_free_allowance) * rate_boundary) + total_cost += ((int_billable_units_by_rate_boundry - remaining_free_allowance) * rate_boundary['rate']) else: total_cost += 0 - billable_units += int(billable_units_by_rate_boundry) + billable_units += int_billable_units_by_rate_boundry return billable_units, total_cost diff --git a/tests/app/dao/test_notification_usage_dao.py b/tests/app/dao/test_notification_usage_dao.py index c20b76a4c..ef3bdfd13 100644 --- a/tests/app/dao/test_notification_usage_dao.py +++ b/tests/app/dao/test_notification_usage_dao.py @@ -623,3 +623,79 @@ def test_should_calculate_rate_boundaries_for_billing_query_for_three_relevant_r assert rate_boundaries[2]['start_date'] == rate_3_valid_from assert rate_boundaries[2]['end_date'] == end_date assert rate_boundaries[2]['rate'] == 0.06 + + +@freeze_time("2016-01-10 12:00:00.000000") +def test_deducts_free_tier_from_bill( + notify_db, notify_db_session +): + current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = 1 + + set_up_rate(notify_db, datetime(2016, 1, 1), 2.5) + + service_1 = sample_service(notify_db, notify_db_session, service_name=str(uuid.uuid4())) + + sample_notification( + notify_db, + notify_db_session, + service=service_1, + billable_units=1, + status=NOTIFICATION_DELIVERED) + sample_notification( + notify_db, + notify_db_session, + service=service_1, + billable_units=1, + status=NOTIFICATION_DELIVERED) + + start = datetime.utcnow() - timedelta(minutes=10) + end = datetime.utcnow() + timedelta(minutes=10) + + assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, service_1.id)[0] == 2 + assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, service_1.id)[1] == 2.5 + + +@freeze_time("2016-01-10 12:00:00.000000") +@pytest.mark.parametrize( + 'free_tier, expected_cost', + [(0, 24.0), (1, 22.0), (2, 20.0), (3, 16.0), (4, 12.0), (5, 6.0), (6, 0.0)] +) +def test_deducts_free_tier_from_bill_across_rate_boundaries( + notify_db, notify_db_session, sample_service, free_tier, expected_cost +): + current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = free_tier + + set_up_rate(notify_db, datetime(2016, 1, 1), 2) + set_up_rate(notify_db, datetime(2016, 10, 1), 4) + set_up_rate(notify_db, datetime(2017, 1, 1), 6) + + eligble_rate_1_start = datetime(2016, 1, 1, 0, 0, 0, 0) + eligble_rate_1_end = datetime(2016, 9, 30, 23, 59, 59, 999) + eligble_rate_2_start = datetime(2016, 10, 1, 0, 0, 0, 0) + eligble_rate_2_end = datetime(2016, 12, 31, 23, 59, 59, 999) + eligble_rate_3_start = datetime(2017, 1, 1, 0, 0, 0, 0) + eligble_rate_3_whenever = datetime(2017, 12, 12, 0, 0, 0, 0) + + def make_notification(created_at): + sample_notification( + notify_db, + notify_db_session, + service=sample_service, + rate_multiplier=1.0, + status=NOTIFICATION_DELIVERED, + created_at=created_at) + + make_notification(eligble_rate_1_start) + make_notification(eligble_rate_1_end) + make_notification(eligble_rate_2_start) + make_notification(eligble_rate_2_end) + make_notification(eligble_rate_3_start) + make_notification(eligble_rate_3_whenever) + + start = datetime(2016, 1, 1) + end = datetime(2018, 1, 1) + + assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, sample_service.id)[0] == 6 + assert get_total_billable_units_for_sent_sms_notifications_in_date_range( + start, end, sample_service.id + )[1] == expected_cost From 635fb8fe44ca611d95c96a3e769015185d7d2331 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 6 Jun 2017 16:21:57 +0100 Subject: [PATCH 21/46] Add private endpoint to get notification by ID MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We need this for the two way stuff in the admin app. We already have this as a public endpoint, but the admin app can’t use it, because the admin app auths with its own key, not that of the service it’s acting on behalf of. This endpoint makes sure that a request originating from one service can’t be used to see notifications belonging to another service. --- app/service/rest.py | 13 +++++++++++++ tests/app/service/test_rest.py | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/app/service/rest.py b/app/service/rest.py index 24fdcd513..1e301d0a7 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -300,6 +300,19 @@ def get_all_notifications_for_service(service_id): ), 200 +@service_blueprint.route('//notifications/', methods=['GET']) +def get_notification_for_service(service_id, notification_id): + + notification = notifications_dao.get_notification_with_personalisation( + service_id, + notification_id, + key_type=None, + ) + return jsonify( + notification_with_template_schema.dump(notification).data, + ), 200 + + def search_for_notification_by_to_field(service_id, search_term, statuses): results = notifications_dao.dao_get_notifications_by_to_field(service_id, search_term, statuses) return jsonify( diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index f2b0a8247..ac8537a4f 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1248,6 +1248,39 @@ def test_get_all_notifications_for_service_in_order(notify_api, notify_db, notif assert response.status_code == 200 +def test_get_notification_for_service(client, notify_db, notify_db_session): + + service_1 = create_service(notify_db, notify_db_session, service_name="1", email_from='1') + service_2 = create_service(notify_db, notify_db_session, service_name="2", email_from='2') + + service_1_notifications = [ + create_sample_notification(notify_db, notify_db_session, service=service_1), + create_sample_notification(notify_db, notify_db_session, service=service_1), + create_sample_notification(notify_db, notify_db_session, service=service_1), + ] + + service_2_notifications = [ + create_sample_notification(notify_db, notify_db_session, service=service_2) + ] + + for notification in service_1_notifications: + response = client.get( + path='/service/{}/notifications/{}'.format(service_1.id, notification.id), + headers=[create_authorization_header()] + ) + resp = json.loads(response.get_data(as_text=True)) + assert str(resp['id']) == str(notification.id) + assert response.status_code == 200 + + service_2_response = client.get( + path='/service/{}/notifications/{}'.format(service_2.id, notification.id), + headers=[create_authorization_header()] + ) + assert service_2_response.status_code == 404 + service_2_response = json.loads(service_2_response.get_data(as_text=True)) + assert service_2_response == {'message': 'No result found', 'result': 'error'} + + @pytest.mark.parametrize( 'include_from_test_key, expected_count_of_notifications', [ From 23a501af1691227a9fdd30e22115bfc69468f336 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 6 Jun 2017 17:11:59 +0100 Subject: [PATCH 22/46] Add dao to get inbound sms by id --- app/dao/inbound_sms_dao.py | 4 ++++ tests/app/dao/test_inbound_sms_dao.py | 11 ++++++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index 3411aeb22..d87c1b1ed 100644 --- a/app/dao/inbound_sms_dao.py +++ b/app/dao/inbound_sms_dao.py @@ -47,3 +47,7 @@ def delete_inbound_sms_created_more_than_a_week_ago(): ).delete(synchronize_session='fetch') return deleted + + +def dao_get_inbound_sms_by_id(inbound_id): + return InboundSms.query.filter_by(id=inbound_id).one() diff --git a/tests/app/dao/test_inbound_sms_dao.py b/tests/app/dao/test_inbound_sms_dao.py index 2731562c4..6162b6d82 100644 --- a/tests/app/dao/test_inbound_sms_dao.py +++ b/tests/app/dao/test_inbound_sms_dao.py @@ -5,7 +5,8 @@ from freezegun import freeze_time from app.dao.inbound_sms_dao import ( dao_get_inbound_sms_for_service, dao_count_inbound_sms_for_service, - delete_inbound_sms_created_more_than_a_week_ago + delete_inbound_sms_created_more_than_a_week_ago, + dao_get_inbound_sms_by_id ) from tests.app.db import create_inbound_sms, create_service @@ -86,3 +87,11 @@ def test_should_not_delete_inbound_sms_before_seven_days(sample_service): delete_inbound_sms_created_more_than_a_week_ago() assert len(InboundSms.query.all()) == 2 + + +def test_get_inbound_sms_by_id_returns(sample_service): + inbound = create_inbound_sms(sample_service) + + inbound_from_db = dao_get_inbound_sms_by_id(inbound.id) + + assert inbound == inbound_from_db From ee488d416a0186ddb28e47764e9860d969498489 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 6 Jun 2017 17:12:21 +0100 Subject: [PATCH 23/46] Add endpoint to get inbound by id --- app/inbound_sms/rest.py | 23 ++++++++++++++++++++++- tests/app/inbound_sms/test_rest.py | 26 ++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/app/inbound_sms/rest.py b/app/inbound_sms/rest.py index d4072dea3..755f830d3 100644 --- a/app/inbound_sms/rest.py +++ b/app/inbound_sms/rest.py @@ -1,11 +1,19 @@ +import uuid + from flask import ( Blueprint, jsonify, request ) +from werkzeug.exceptions import abort + from notifications_utils.recipients import validate_and_format_phone_number -from app.dao.inbound_sms_dao import dao_get_inbound_sms_for_service, dao_count_inbound_sms_for_service +from app.dao.inbound_sms_dao import ( + dao_get_inbound_sms_for_service, + dao_count_inbound_sms_for_service, + dao_get_inbound_sms_by_id +) from app.errors import register_errors inbound_sms = Blueprint( @@ -40,3 +48,16 @@ def get_inbound_sms_summary_for_service(service_id): count=count, most_recent=most_recent[0].created_at.isoformat() if most_recent else None ) + + +@inbound_sms.route('/', methods=['GET']) +def get_inbound_by_id(service_id, inbound_sms_id): + # TODO: Add JSON Schema here + try: + validated_uuid = uuid.UUID(inbound_sms_id) + except (ValueError, AttributeError): + abort(400) + + inbound_sms = dao_get_inbound_sms_by_id(validated_uuid) + + return jsonify(inbound_sms.serialize()), 200 diff --git a/tests/app/inbound_sms/test_rest.py b/tests/app/inbound_sms/test_rest.py index da10ecb6b..4f021d03a 100644 --- a/tests/app/inbound_sms/test_rest.py +++ b/tests/app/inbound_sms/test_rest.py @@ -112,3 +112,29 @@ def test_get_inbound_sms_summary_with_no_inbound(admin_request, sample_service): 'count': 0, 'most_recent': None } + + +def test_get_inbound_sms_by_id_returns_200(admin_request, sample_service): + inbound = create_inbound_sms(sample_service, user_number='447700900001') + + response = admin_request.get( + 'inbound_sms.get_inbound_by_id', + endpoint_kwargs={ + 'service_id': sample_service.id, + 'inbound_sms_id': inbound.id + } + ) + + assert response['user_number'] == '447700900001' + assert response['service_id'] == str(sample_service.id) + + +def test_get_inbound_sms_by_id_invalid_id_returns_400(admin_request, sample_service): + assert admin_request.get( + 'inbound_sms.get_inbound_by_id', + endpoint_kwargs={ + 'service_id': sample_service.id, + 'inbound_sms_id': 'dsadsda' + }, + expected_status=400 + ) From d97c7c8e56e2a47a8cfe0bfb4d368482d747fc3d Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 7 Jun 2017 09:58:57 +0100 Subject: [PATCH 24/46] - Fix up free tier on the service object, use it only on dump not create/update in marshmallow - Ensure tests leave config as was after a test run that alters free tier quantity --- app/schemas.py | 3 +- tests/app/dao/test_notification_usage_dao.py | 525 ++++++++++--------- tests/app/service/test_rest.py | 4 +- tests/conftest.py | 1 + 4 files changed, 274 insertions(+), 259 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index 3ca8fda1a..8f8395f3e 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -175,7 +175,7 @@ class ProviderDetailsHistorySchema(BaseSchema): class ServiceSchema(BaseSchema): - free_sms_fragment_limit = fields.Method('get_free_sms_fragment_limit') + free_sms_fragment_limit = fields.Method(method_name='get_free_sms_fragment_limit') created_by = field_for(models.Service, 'created_by', required=True) organisation = field_for(models.Service, 'organisation') branding = field_for(models.Service, 'branding') @@ -191,6 +191,7 @@ class ServiceSchema(BaseSchema): class Meta: model = models.Service + dump_only = ['free_sms_fragment_limit'] exclude = ( 'updated_at', 'created_at', diff --git a/tests/app/dao/test_notification_usage_dao.py b/tests/app/dao/test_notification_usage_dao.py index ef3bdfd13..83bfa264c 100644 --- a/tests/app/dao/test_notification_usage_dao.py +++ b/tests/app/dao/test_notification_usage_dao.py @@ -21,6 +21,8 @@ from tests.app.conftest import sample_notification, sample_email_template, sampl from tests.app.db import create_notification from freezegun import freeze_time +from tests.conftest import set_config + def test_get_rates_for_year(notify_db, notify_db_session): set_up_rate(notify_db, datetime(2016, 5, 18), 0.016) @@ -266,232 +268,235 @@ def set_up_rate(notify_db, start_date, value): @freeze_time("2016-01-10 12:00:00.000000") def test_returns_total_billable_units_for_sms_notifications(notify_db, notify_db_session, sample_service): - current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = 0 + with set_config(current_app, 'FREE_SMS_TIER_FRAGMENT_COUNT', 0): - set_up_rate(notify_db, datetime(2016, 1, 1), 0.016) + set_up_rate(notify_db, datetime(2016, 1, 1), 0.016) - sample_notification( - notify_db, notify_db_session, service=sample_service, billable_units=1, status=NOTIFICATION_DELIVERED) - sample_notification( - notify_db, notify_db_session, service=sample_service, billable_units=2, status=NOTIFICATION_DELIVERED) - sample_notification( - notify_db, notify_db_session, service=sample_service, billable_units=3, status=NOTIFICATION_DELIVERED) - sample_notification( - notify_db, notify_db_session, service=sample_service, billable_units=4, status=NOTIFICATION_DELIVERED) + sample_notification( + notify_db, notify_db_session, service=sample_service, billable_units=1, status=NOTIFICATION_DELIVERED) + sample_notification( + notify_db, notify_db_session, service=sample_service, billable_units=2, status=NOTIFICATION_DELIVERED) + sample_notification( + notify_db, notify_db_session, service=sample_service, billable_units=3, status=NOTIFICATION_DELIVERED) + sample_notification( + notify_db, notify_db_session, service=sample_service, billable_units=4, status=NOTIFICATION_DELIVERED) - start = datetime.utcnow() - timedelta(minutes=10) - end = datetime.utcnow() + timedelta(minutes=10) + start = datetime.utcnow() - timedelta(minutes=10) + end = datetime.utcnow() + timedelta(minutes=10) - assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, sample_service.id)[0] == 10 - assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, sample_service.id)[1] == 0.16 + assert get_total_billable_units_for_sent_sms_notifications_in_date_range( + start, end, sample_service.id)[0] == 10 + assert get_total_billable_units_for_sent_sms_notifications_in_date_range( + start, end, sample_service.id)[1] == 0.16 @freeze_time("2016-01-10 12:00:00.000000") def test_returns_total_billable_units_multiplied_by_multipler_for_sms_notifications( notify_db, notify_db_session, sample_service ): - current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = 0 + with set_config(current_app, 'FREE_SMS_TIER_FRAGMENT_COUNT', 0): + set_up_rate(notify_db, datetime(2016, 1, 1), 2.5) - set_up_rate(notify_db, datetime(2016, 1, 1), 2.5) + sample_notification( + notify_db, notify_db_session, service=sample_service, rate_multiplier=1.0, status=NOTIFICATION_DELIVERED) + sample_notification( + notify_db, notify_db_session, service=sample_service, rate_multiplier=2.0, status=NOTIFICATION_DELIVERED) + sample_notification( + notify_db, notify_db_session, service=sample_service, rate_multiplier=5.0, status=NOTIFICATION_DELIVERED) + sample_notification( + notify_db, notify_db_session, service=sample_service, rate_multiplier=10.0, status=NOTIFICATION_DELIVERED) - sample_notification( - notify_db, notify_db_session, service=sample_service, rate_multiplier=1.0, status=NOTIFICATION_DELIVERED) - sample_notification( - notify_db, notify_db_session, service=sample_service, rate_multiplier=2.0, status=NOTIFICATION_DELIVERED) - sample_notification( - notify_db, notify_db_session, service=sample_service, rate_multiplier=5.0, status=NOTIFICATION_DELIVERED) - sample_notification( - notify_db, notify_db_session, service=sample_service, rate_multiplier=10.0, status=NOTIFICATION_DELIVERED) + start = datetime.utcnow() - timedelta(minutes=10) + end = datetime.utcnow() + timedelta(minutes=10) - start = datetime.utcnow() - timedelta(minutes=10) - end = datetime.utcnow() + timedelta(minutes=10) - - assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, sample_service.id)[0] == 18 - assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, sample_service.id)[1] == 45 + assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, sample_service.id)[0] == 18 + assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, sample_service.id)[1] == 45 def test_returns_total_billable_units_multiplied_by_multipler_for_sms_notifications_for_several_rates( notify_db, notify_db_session, sample_service ): - current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = 0 + with set_config(current_app, 'FREE_SMS_TIER_FRAGMENT_COUNT', 0): - set_up_rate(notify_db, datetime(2016, 1, 1), 2) - set_up_rate(notify_db, datetime(2016, 10, 1), 4) - set_up_rate(notify_db, datetime(2017, 1, 1), 6) + set_up_rate(notify_db, datetime(2016, 1, 1), 2) + set_up_rate(notify_db, datetime(2016, 10, 1), 4) + set_up_rate(notify_db, datetime(2017, 1, 1), 6) - eligble_rate_1 = datetime(2016, 2, 1) - eligble_rate_2 = datetime(2016, 11, 1) - eligble_rate_3 = datetime(2017, 2, 1) + eligble_rate_1 = datetime(2016, 2, 1) + eligble_rate_2 = datetime(2016, 11, 1) + eligble_rate_3 = datetime(2017, 2, 1) - sample_notification( - notify_db, - notify_db_session, - service=sample_service, - rate_multiplier=1.0, - status=NOTIFICATION_DELIVERED, - created_at=eligble_rate_1) - - sample_notification( - notify_db, - notify_db_session, - service=sample_service, - rate_multiplier=2.0, - status=NOTIFICATION_DELIVERED, - created_at=eligble_rate_2) - - sample_notification( - notify_db, - notify_db_session, - service=sample_service, - rate_multiplier=5.0, - status=NOTIFICATION_DELIVERED, - created_at=eligble_rate_3) - - start = datetime(2016, 1, 1) - end = datetime(2018, 1, 1) - assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, sample_service.id)[0] == 8 - assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, sample_service.id)[1] == 40 - - -def test_returns_total_billable_units_for_sms_notifications_for_several_rates_where_dates_match_rate_boundary( - notify_db, notify_db_session, sample_service -): - current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = 0 - - set_up_rate(notify_db, datetime(2016, 1, 1), 2) - set_up_rate(notify_db, datetime(2016, 10, 1), 4) - set_up_rate(notify_db, datetime(2017, 1, 1), 6) - - eligble_rate_1_start = datetime(2016, 1, 1, 0, 0, 0, 0) - eligble_rate_1_end = datetime(2016, 9, 30, 23, 59, 59, 999) - eligble_rate_2_start = datetime(2016, 10, 1, 0, 0, 0, 0) - eligble_rate_2_end = datetime(2016, 12, 31, 23, 59, 59, 999) - eligble_rate_3_start = datetime(2017, 1, 1, 0, 0, 0, 0) - eligble_rate_3_whenever = datetime(2017, 12, 12, 0, 0, 0, 0) - - def make_notification(created_at): sample_notification( notify_db, notify_db_session, service=sample_service, rate_multiplier=1.0, status=NOTIFICATION_DELIVERED, - created_at=created_at) + created_at=eligble_rate_1) - make_notification(eligble_rate_1_start) - make_notification(eligble_rate_1_end) - make_notification(eligble_rate_2_start) - make_notification(eligble_rate_2_end) - make_notification(eligble_rate_3_start) - make_notification(eligble_rate_3_whenever) + sample_notification( + notify_db, + notify_db_session, + service=sample_service, + rate_multiplier=2.0, + status=NOTIFICATION_DELIVERED, + created_at=eligble_rate_2) - start = datetime(2016, 1, 1) - end = datetime(2018, 1, 1) + sample_notification( + notify_db, + notify_db_session, + service=sample_service, + rate_multiplier=5.0, + status=NOTIFICATION_DELIVERED, + created_at=eligble_rate_3) - assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, sample_service.id)[0] == 6 - assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, sample_service.id)[1] == 24.0 + start = datetime(2016, 1, 1) + end = datetime(2018, 1, 1) + assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, sample_service.id)[0] == 8 + assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, sample_service.id)[1] == 40 + + +def test_returns_total_billable_units_for_sms_notifications_for_several_rates_where_dates_match_rate_boundary( + notify_db, notify_db_session, sample_service +): + with set_config(current_app, 'FREE_SMS_TIER_FRAGMENT_COUNT', 0): + + set_up_rate(notify_db, datetime(2016, 1, 1), 2) + set_up_rate(notify_db, datetime(2016, 10, 1), 4) + set_up_rate(notify_db, datetime(2017, 1, 1), 6) + + eligble_rate_1_start = datetime(2016, 1, 1, 0, 0, 0, 0) + eligble_rate_1_end = datetime(2016, 9, 30, 23, 59, 59, 999) + eligble_rate_2_start = datetime(2016, 10, 1, 0, 0, 0, 0) + eligble_rate_2_end = datetime(2016, 12, 31, 23, 59, 59, 999) + eligble_rate_3_start = datetime(2017, 1, 1, 0, 0, 0, 0) + eligble_rate_3_whenever = datetime(2017, 12, 12, 0, 0, 0, 0) + + def make_notification(created_at): + sample_notification( + notify_db, + notify_db_session, + service=sample_service, + rate_multiplier=1.0, + status=NOTIFICATION_DELIVERED, + created_at=created_at) + + make_notification(eligble_rate_1_start) + make_notification(eligble_rate_1_end) + make_notification(eligble_rate_2_start) + make_notification(eligble_rate_2_end) + make_notification(eligble_rate_3_start) + make_notification(eligble_rate_3_whenever) + + start = datetime(2016, 1, 1) + end = datetime(2018, 1, 1) + + assert get_total_billable_units_for_sent_sms_notifications_in_date_range( + start, end, sample_service.id)[0] == 6 + assert get_total_billable_units_for_sent_sms_notifications_in_date_range( + start, end, sample_service.id)[1] == 24.0 @freeze_time("2016-01-10 12:00:00.000000") def test_returns_total_billable_units_for_sms_notifications_ignoring_letters_and_emails( notify_db, notify_db_session, sample_service ): - current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = 0 + with set_config(current_app, 'FREE_SMS_TIER_FRAGMENT_COUNT', 0): - set_up_rate(notify_db, datetime(2016, 1, 1), 2.5) + set_up_rate(notify_db, datetime(2016, 1, 1), 2.5) - email_template = sample_email_template(notify_db, notify_db_session, service=sample_service) - letter_template = sample_letter_template(sample_service) + email_template = sample_email_template(notify_db, notify_db_session, service=sample_service) + letter_template = sample_letter_template(sample_service) - sample_notification( - notify_db, - notify_db_session, - service=sample_service, - billable_units=2, - status=NOTIFICATION_DELIVERED) - sample_notification( - notify_db, - notify_db_session, - template=email_template, - service=sample_service, - billable_units=2, - status=NOTIFICATION_DELIVERED) - sample_notification( - notify_db, - notify_db_session, - template=letter_template, - service=sample_service, - billable_units=2, - status=NOTIFICATION_DELIVERED - ) + sample_notification( + notify_db, + notify_db_session, + service=sample_service, + billable_units=2, + status=NOTIFICATION_DELIVERED) + sample_notification( + notify_db, + notify_db_session, + template=email_template, + service=sample_service, + billable_units=2, + status=NOTIFICATION_DELIVERED) + sample_notification( + notify_db, + notify_db_session, + template=letter_template, + service=sample_service, + billable_units=2, + status=NOTIFICATION_DELIVERED + ) - start = datetime.utcnow() - timedelta(minutes=10) - end = datetime.utcnow() + timedelta(minutes=10) + start = datetime.utcnow() - timedelta(minutes=10) + end = datetime.utcnow() + timedelta(minutes=10) - assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, sample_service.id)[0] == 2 - assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, sample_service.id)[1] == 5 + assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, sample_service.id)[0] == 2 + assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, sample_service.id)[1] == 5 @freeze_time("2016-01-10 12:00:00.000000") def test_returns_total_billable_units_for_sms_notifications_for_only_requested_service( notify_db, notify_db_session ): - current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = 0 + with set_config(current_app, 'FREE_SMS_TIER_FRAGMENT_COUNT', 0): - set_up_rate(notify_db, datetime(2016, 1, 1), 2.5) + set_up_rate(notify_db, datetime(2016, 1, 1), 2.5) - service_1 = sample_service(notify_db, notify_db_session, service_name=str(uuid.uuid4())) - service_2 = sample_service(notify_db, notify_db_session, service_name=str(uuid.uuid4())) - service_3 = sample_service(notify_db, notify_db_session, service_name=str(uuid.uuid4())) + service_1 = sample_service(notify_db, notify_db_session, service_name=str(uuid.uuid4())) + service_2 = sample_service(notify_db, notify_db_session, service_name=str(uuid.uuid4())) + service_3 = sample_service(notify_db, notify_db_session, service_name=str(uuid.uuid4())) - sample_notification( - notify_db, - notify_db_session, - service=service_1, - billable_units=2, - status=NOTIFICATION_DELIVERED) - sample_notification( - notify_db, - notify_db_session, - service=service_2, - billable_units=2, - status=NOTIFICATION_DELIVERED) - sample_notification( - notify_db, - notify_db_session, - service=service_3, - billable_units=2, - status=NOTIFICATION_DELIVERED - ) + sample_notification( + notify_db, + notify_db_session, + service=service_1, + billable_units=2, + status=NOTIFICATION_DELIVERED) + sample_notification( + notify_db, + notify_db_session, + service=service_2, + billable_units=2, + status=NOTIFICATION_DELIVERED) + sample_notification( + notify_db, + notify_db_session, + service=service_3, + billable_units=2, + status=NOTIFICATION_DELIVERED + ) - start = datetime.utcnow() - timedelta(minutes=10) - end = datetime.utcnow() + timedelta(minutes=10) + start = datetime.utcnow() - timedelta(minutes=10) + end = datetime.utcnow() + timedelta(minutes=10) - assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, service_1.id)[0] == 2 - assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, service_1.id)[1] == 5 + assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, service_1.id)[0] == 2 + assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, service_1.id)[1] == 5 @freeze_time("2016-01-10 12:00:00.000000") def test_returns_total_billable_units_for_sms_notifications_handling_null_values( notify_db, notify_db_session, sample_service ): - current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = 0 + with set_config(current_app, 'FREE_SMS_TIER_FRAGMENT_COUNT', 0): - set_up_rate(notify_db, datetime(2016, 1, 1), 2.5) + set_up_rate(notify_db, datetime(2016, 1, 1), 2.5) - sample_notification( - notify_db, - notify_db_session, - service=sample_service, - billable_units=2, - rate_multiplier=None, - status=NOTIFICATION_DELIVERED) + sample_notification( + notify_db, + notify_db_session, + service=sample_service, + billable_units=2, + rate_multiplier=None, + status=NOTIFICATION_DELIVERED) - start = datetime.utcnow() - timedelta(minutes=10) - end = datetime.utcnow() + timedelta(minutes=10) + start = datetime.utcnow() - timedelta(minutes=10) + end = datetime.utcnow() + timedelta(minutes=10) - assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, sample_service.id)[0] == 2 - assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, sample_service.id)[1] == 5 + assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, sample_service.id)[0] == 2 + assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, sample_service.id)[1] == 5 @pytest.mark.parametrize('billable_units, states', ([ @@ -502,62 +507,61 @@ def test_returns_total_billable_units_for_sms_notifications_handling_null_values def test_ignores_non_billable_states_when_returning_billable_units_for_sms_notifications( notify_db, notify_db_session, sample_service, billable_units, states ): + with set_config(current_app, 'FREE_SMS_TIER_FRAGMENT_COUNT', 0): + set_up_rate(notify_db, datetime(2016, 1, 1), 2.5) - current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = 0 + for state in states: + sample_notification( + notify_db, + notify_db_session, + service=sample_service, + billable_units=1, + rate_multiplier=None, + status=state) - set_up_rate(notify_db, datetime(2016, 1, 1), 2.5) + start = datetime.utcnow() - timedelta(minutes=10) + end = datetime.utcnow() + timedelta(minutes=10) - for state in states: - sample_notification( - notify_db, - notify_db_session, - service=sample_service, - billable_units=1, - rate_multiplier=None, - status=state) - - start = datetime.utcnow() - timedelta(minutes=10) - end = datetime.utcnow() + timedelta(minutes=10) - - assert get_total_billable_units_for_sent_sms_notifications_in_date_range( - start, end, sample_service.id - )[0] == billable_units - assert get_total_billable_units_for_sent_sms_notifications_in_date_range( - start, end, sample_service.id - )[1] == billable_units * 2.5 + assert get_total_billable_units_for_sent_sms_notifications_in_date_range( + start, end, sample_service.id + )[0] == billable_units + assert get_total_billable_units_for_sent_sms_notifications_in_date_range( + start, end, sample_service.id + )[1] == billable_units * 2.5 @freeze_time("2016-01-10 12:00:00.000000") def test_restricts_to_time_period_when_returning_billable_units_for_sms_notifications( notify_db, notify_db_session, sample_service ): - current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = 0 + with set_config(current_app, 'FREE_SMS_TIER_FRAGMENT_COUNT', 0): + set_up_rate(notify_db, datetime(2016, 1, 1), 2.5) - set_up_rate(notify_db, datetime(2016, 1, 1), 2.5) + sample_notification( + notify_db, + notify_db_session, + service=sample_service, + billable_units=1, + rate_multiplier=1.0, + created_at=datetime.utcnow() - timedelta(minutes=100), + status=NOTIFICATION_DELIVERED) - sample_notification( - notify_db, - notify_db_session, - service=sample_service, - billable_units=1, - rate_multiplier=1.0, - created_at=datetime.utcnow() - timedelta(minutes=100), - status=NOTIFICATION_DELIVERED) + sample_notification( + notify_db, + notify_db_session, + service=sample_service, + billable_units=1, + rate_multiplier=1.0, + created_at=datetime.utcnow() - timedelta(minutes=5), + status=NOTIFICATION_DELIVERED) - sample_notification( - notify_db, - notify_db_session, - service=sample_service, - billable_units=1, - rate_multiplier=1.0, - created_at=datetime.utcnow() - timedelta(minutes=5), - status=NOTIFICATION_DELIVERED) + start = datetime.utcnow() - timedelta(minutes=10) + end = datetime.utcnow() + timedelta(minutes=10) - start = datetime.utcnow() - timedelta(minutes=10) - end = datetime.utcnow() + timedelta(minutes=10) - - assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, sample_service.id)[0] == 1 - assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, sample_service.id)[1] == 2.5 + assert get_total_billable_units_for_sent_sms_notifications_in_date_range( + start, end, sample_service.id)[0] == 1 + assert get_total_billable_units_for_sent_sms_notifications_in_date_range( + start, end, sample_service.id)[1] == 2.5 def test_returns_zero_if_no_matching_rows_when_returning_billable_units_for_sms_notifications( @@ -629,30 +633,34 @@ def test_should_calculate_rate_boundaries_for_billing_query_for_three_relevant_r def test_deducts_free_tier_from_bill( notify_db, notify_db_session ): - current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = 1 + start_value = current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] + try: + current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = 1 - set_up_rate(notify_db, datetime(2016, 1, 1), 2.5) + set_up_rate(notify_db, datetime(2016, 1, 1), 2.5) - service_1 = sample_service(notify_db, notify_db_session, service_name=str(uuid.uuid4())) + service_1 = sample_service(notify_db, notify_db_session, service_name=str(uuid.uuid4())) - sample_notification( - notify_db, - notify_db_session, - service=service_1, - billable_units=1, - status=NOTIFICATION_DELIVERED) - sample_notification( - notify_db, - notify_db_session, - service=service_1, - billable_units=1, - status=NOTIFICATION_DELIVERED) + sample_notification( + notify_db, + notify_db_session, + service=service_1, + billable_units=1, + status=NOTIFICATION_DELIVERED) + sample_notification( + notify_db, + notify_db_session, + service=service_1, + billable_units=1, + status=NOTIFICATION_DELIVERED) - start = datetime.utcnow() - timedelta(minutes=10) - end = datetime.utcnow() + timedelta(minutes=10) + start = datetime.utcnow() - timedelta(minutes=10) + end = datetime.utcnow() + timedelta(minutes=10) - assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, service_1.id)[0] == 2 - assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, service_1.id)[1] == 2.5 + assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, service_1.id)[0] == 2 + assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, service_1.id)[1] == 2.5 + finally: + current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = start_value @freeze_time("2016-01-10 12:00:00.000000") @@ -663,39 +671,42 @@ def test_deducts_free_tier_from_bill( def test_deducts_free_tier_from_bill_across_rate_boundaries( notify_db, notify_db_session, sample_service, free_tier, expected_cost ): - current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = free_tier + start_value = current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] + try: + current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = free_tier + set_up_rate(notify_db, datetime(2016, 1, 1), 2) + set_up_rate(notify_db, datetime(2016, 10, 1), 4) + set_up_rate(notify_db, datetime(2017, 1, 1), 6) - set_up_rate(notify_db, datetime(2016, 1, 1), 2) - set_up_rate(notify_db, datetime(2016, 10, 1), 4) - set_up_rate(notify_db, datetime(2017, 1, 1), 6) + eligble_rate_1_start = datetime(2016, 1, 1, 0, 0, 0, 0) + eligble_rate_1_end = datetime(2016, 9, 30, 23, 59, 59, 999) + eligble_rate_2_start = datetime(2016, 10, 1, 0, 0, 0, 0) + eligble_rate_2_end = datetime(2016, 12, 31, 23, 59, 59, 999) + eligble_rate_3_start = datetime(2017, 1, 1, 0, 0, 0, 0) + eligble_rate_3_whenever = datetime(2017, 12, 12, 0, 0, 0, 0) - eligble_rate_1_start = datetime(2016, 1, 1, 0, 0, 0, 0) - eligble_rate_1_end = datetime(2016, 9, 30, 23, 59, 59, 999) - eligble_rate_2_start = datetime(2016, 10, 1, 0, 0, 0, 0) - eligble_rate_2_end = datetime(2016, 12, 31, 23, 59, 59, 999) - eligble_rate_3_start = datetime(2017, 1, 1, 0, 0, 0, 0) - eligble_rate_3_whenever = datetime(2017, 12, 12, 0, 0, 0, 0) + def make_notification(created_at): + sample_notification( + notify_db, + notify_db_session, + service=sample_service, + rate_multiplier=1.0, + status=NOTIFICATION_DELIVERED, + created_at=created_at) - def make_notification(created_at): - sample_notification( - notify_db, - notify_db_session, - service=sample_service, - rate_multiplier=1.0, - status=NOTIFICATION_DELIVERED, - created_at=created_at) + make_notification(eligble_rate_1_start) + make_notification(eligble_rate_1_end) + make_notification(eligble_rate_2_start) + make_notification(eligble_rate_2_end) + make_notification(eligble_rate_3_start) + make_notification(eligble_rate_3_whenever) - make_notification(eligble_rate_1_start) - make_notification(eligble_rate_1_end) - make_notification(eligble_rate_2_start) - make_notification(eligble_rate_2_end) - make_notification(eligble_rate_3_start) - make_notification(eligble_rate_3_whenever) + start = datetime(2016, 1, 1) + end = datetime(2018, 1, 1) - start = datetime(2016, 1, 1) - end = datetime(2018, 1, 1) - - assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, sample_service.id)[0] == 6 - assert get_total_billable_units_for_sent_sms_notifications_in_date_range( - start, end, sample_service.id - )[1] == expected_cost + assert get_total_billable_units_for_sent_sms_notifications_in_date_range(start, end, sample_service.id)[0] == 6 + assert get_total_billable_units_for_sent_sms_notifications_in_date_range( + start, end, sample_service.id + )[1] == expected_cost + finally: + current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = start_value diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 2c2842177..bc6010fa2 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -27,6 +27,7 @@ from app.models import ( ) from tests.app.db import create_user +from tests.conftest import set_config_values def test_get_service_list(client, service_factory): @@ -148,6 +149,7 @@ def test_get_service_by_id(client, sample_service): def test_get_service_by_id_returns_free_sms_limit(client, sample_service): + auth_header = create_authorization_header() resp = client.get( '/service/{}'.format(sample_service.id), @@ -2001,7 +2003,7 @@ def test_get_yearly_billing_usage_count_returns_from_cache_if_present(client, sa '/service/{}/yearly-sms-billable-units?year=2016'.format(sample_service.id), headers=[create_authorization_header()] ) - print(response.get_data(as_text=True)) + response.get_data(as_text=True) assert response.status_code == 200 assert json.loads(response.get_data(as_text=True)) == { 'billable_sms_units': 50, diff --git a/tests/conftest.py b/tests/conftest.py index bf9823331..a59de1958 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -115,6 +115,7 @@ def set_config(app, name, value): old_val = app.config.get(name) app.config[name] = value yield + print(app.config) app.config[name] = old_val From 1b4097cb16ccfbe9485632d998a9f7854f1016fc Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 7 Jun 2017 11:15:05 +0100 Subject: [PATCH 25/46] Add three new columns to job_statistics for sent, delivered and failed. A job only ever has one notification type. This is the first deploy, where the columns are added and populated. Next a data migration will happen to populate these new columns for the older jobs that do not have the values set. Then we stop populating the old columns and remove them. This refactoring of the table structure will make the queries to the table much easier to handle. --- app/dao/statistics_dao.py | 17 +- app/models.py | 3 + migrations/versions/0094_job_stats_update.py | 25 +++ tests/app/dao/test_statistics_dao.py | 215 ++++++++++++++----- 4 files changed, 203 insertions(+), 57 deletions(-) create mode 100644 migrations/versions/0094_job_stats_update.py diff --git a/app/dao/statistics_dao.py b/app/dao/statistics_dao.py index baff82ba4..48d62e71c 100644 --- a/app/dao/statistics_dao.py +++ b/app/dao/statistics_dao.py @@ -60,7 +60,10 @@ def timeout_job_counts(notifications_type, timeout_start): ).update({ sent: sent_count, failed: failed_count, - delivered: delivered_count + delivered: delivered_count, + 'sent': sent_count, + 'delivered': delivered_count, + 'failed': failed_count }, synchronize_session=False) return total_updated @@ -87,11 +90,13 @@ def create_or_update_job_sending_statistics(notification): @transactional def __update_job_stats_sent_count(notification): column = columns(notification.notification_type, 'sent') + new_column = 'sent' return db.session.query(JobStatistics).filter_by( job_id=notification.job_id, ).update({ - column: column + 1 + column: column + 1, + new_column: column + 1 }) @@ -102,7 +107,8 @@ def __insert_job_stats(notification): emails_sent=1 if notification.notification_type == EMAIL_TYPE else 0, sms_sent=1 if notification.notification_type == SMS_TYPE else 0, letters_sent=1 if notification.notification_type == LETTER_TYPE else 0, - updated_at=datetime.utcnow() + updated_at=datetime.utcnow(), + sent=1 ) db.session.add(stats) @@ -131,10 +137,12 @@ def columns(notification_type, status): def update_job_stats_outcome_count(notification): if notification.status in NOTIFICATION_STATUS_TYPES_FAILED: column = columns(notification.notification_type, 'failed') + new_column = 'failed' elif notification.status in [NOTIFICATION_DELIVERED, NOTIFICATION_SENT] and notification.notification_type != LETTER_TYPE: column = columns(notification.notification_type, 'delivered') + new_column = 'delivered' else: column = None @@ -143,7 +151,8 @@ def update_job_stats_outcome_count(notification): return db.session.query(JobStatistics).filter_by( job_id=notification.job_id, ).update({ - column: column + 1 + column: column + 1, + new_column: column + 1 }) else: return 0 diff --git a/app/models.py b/app/models.py index 1bcd66a19..22d819389 100644 --- a/app/models.py +++ b/app/models.py @@ -1122,6 +1122,9 @@ class JobStatistics(db.Model): sms_failed = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) letters_sent = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) letters_failed = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) + sent = db.Column(db.BigInteger, index=False, unique=False, nullable=True, default=0) + delivered = db.Column(db.BigInteger, index=False, unique=False, nullable=True, default=0) + failed = db.Column(db.BigInteger, index=False, unique=False, nullable=True, default=0) created_at = db.Column( db.DateTime, index=False, diff --git a/migrations/versions/0094_job_stats_update.py b/migrations/versions/0094_job_stats_update.py new file mode 100644 index 000000000..6a7f7db2a --- /dev/null +++ b/migrations/versions/0094_job_stats_update.py @@ -0,0 +1,25 @@ +"""empty message + +Revision ID: 0094_job_stats_update +Revises: 0093_data_gov_uk +Create Date: 2017-06-06 14:37:30.051647 + +""" +from alembic import op +import sqlalchemy as sa + +# revision identifiers, used by Alembic. +revision = '0094_job_stats_update' +down_revision = '0093_data_gov_uk' + + +def upgrade(): + op.add_column('job_statistics', sa.Column('sent', sa.BigInteger(), nullable=True)) + op.add_column('job_statistics', sa.Column('delivered', sa.BigInteger(), nullable=True)) + op.add_column('job_statistics', sa.Column('failed', sa.BigInteger(), nullable=True)) + + +def downgrade(): + op.drop_column('job_statistics', 'sent') + op.drop_column('job_statistics', 'failed') + op.drop_column('job_statistics', 'delivered') diff --git a/tests/app/dao/test_statistics_dao.py b/tests/app/dao/test_statistics_dao.py index 4425a3409..5a73ea672 100644 --- a/tests/app/dao/test_statistics_dao.py +++ b/tests/app/dao/test_statistics_dao.py @@ -199,6 +199,10 @@ def test_should_update_a_stats_entry_with_its_success_outcome_for_a_job( assert stat.sms_failed == 0 assert stat.letters_failed == 0 + assert stat.sent == email_count + sms_count + letter_count + assert stat.delivered == email_count + sms_count + assert stat.failed == 0 + @pytest.mark.parametrize('notification_type, sms_count, email_count, letter_count, status', [ (SMS_TYPE, 1, 0, 0, NOTIFICATION_TECHNICAL_FAILURE), @@ -264,6 +268,10 @@ def test_should_update_a_stats_entry_with_its_error_outcomes_for_a_job( assert stat.emails_delivered == 0 assert stat.sms_delivered == 0 + assert stat.sent == email_count + sms_count + letter_count + assert stat.delivered == 0 + assert stat.failed == email_count + sms_count + letter_count + @pytest.mark.parametrize('notification_type, sms_count, email_count, letter_count, status', [ (SMS_TYPE, 1, 0, 0, NOTIFICATION_DELIVERED), @@ -326,6 +334,10 @@ def test_should_update_a_stats_entry_with_its_success_outcomes_for_a_job( assert stat.emails_delivered == email_count assert stat.sms_delivered == sms_count + assert stat.sent == email_count + sms_count + letter_count + assert stat.delivered == 0 if notification_type == LETTER_TYPE else 1 + assert stat.failed == 0 + @pytest.mark.parametrize('notification_type, sms_count, email_count, letter_count, status', [ (SMS_TYPE, 1, 0, 0, NOTIFICATION_PENDING), @@ -394,6 +406,10 @@ def test_should_not_update_job_stats_if_irrelevant_status( assert stat.emails_delivered == 0 assert stat.sms_delivered == 0 + assert stat.sent == email_count + sms_count + letter_count + assert stat.delivered == 0 + assert stat.failed == 0 + @pytest.mark.parametrize('notification_type, sms_count, email_count, letter_count', [ (SMS_TYPE, 2, 1, 1), @@ -480,41 +496,52 @@ def test_inserting_one_type_of_notification_maintains_other_counts( assert updated_stats[0].sms_sent == sms_count assert updated_stats[0].letters_sent == letter_count + if notification_type == EMAIL_TYPE: + assert updated_stats[0].sent == email_count + elif notification_type == SMS_TYPE: + assert updated_stats[0].sent == sms_count + elif notification_type == LETTER_TYPE: + assert updated_stats[0].sent == letter_count + def test_updating_one_type_of_notification_to_success_maintains_other_counts( notify_db, notify_db_session, - sample_job, + sample_service, sample_letter_template ): - sms_template = sample_template(notify_db, notify_db_session, service=sample_job.service) - email_template = sample_email_template(notify_db, notify_db_session, service=sample_job.service) + job_1 = sample_job(notify_db, notify_db_session, service=sample_service) + job_2 = sample_job(notify_db, notify_db_session, service=sample_service) + job_3 = sample_job(notify_db, notify_db_session, service=sample_service) + + sms_template = sample_template(notify_db, notify_db_session, service=sample_service) + email_template = sample_email_template(notify_db, notify_db_session, service=sample_service) letter_template = sample_letter_template letter = sample_notification( notify_db, notify_db_session, - service=sample_job.service, + service=sample_service, template=letter_template, - job=sample_job, + job=job_1, status=NOTIFICATION_CREATED ) email = sample_notification( notify_db, notify_db_session, - service=sample_job.service, + service=sample_service, template=email_template, - job=sample_job, + job=job_2, status=NOTIFICATION_CREATED ) sms = sample_notification( notify_db, notify_db_session, - service=sample_job.service, + service=sample_service, template=sms_template, - job=sample_job, + job=job_3, status=NOTIFICATION_CREATED ) @@ -530,49 +557,76 @@ def test_updating_one_type_of_notification_to_success_maintains_other_counts( update_job_stats_outcome_count(email) update_job_stats_outcome_count(sms) - stats = JobStatistics.query.all() - assert len(stats) == 1 - assert stats[0].emails_sent == 1 - assert stats[0].sms_sent == 1 + stats = JobStatistics.query.order_by(JobStatistics.created_at).all() + assert len(stats) == 3 assert stats[0].letters_sent == 1 - assert stats[0].emails_delivered == 1 - assert stats[0].sms_delivered == 1 + assert stats[0].emails_sent == 0 + assert stats[0].sms_sent == 0 + assert stats[0].emails_delivered == 0 + assert stats[0].sms_delivered == 0 + + assert stats[1].letters_sent == 0 + assert stats[1].emails_sent == 1 + assert stats[1].sms_sent == 0 + assert stats[1].emails_delivered == 1 + assert stats[1].sms_delivered == 0 + + assert stats[2].letters_sent == 0 + assert stats[2].emails_sent == 0 + assert stats[2].sms_sent == 1 + assert stats[2].emails_delivered == 0 + assert stats[2].sms_delivered == 1 + + assert stats[0].sent == 1 + assert stats[0].delivered == 0 + assert stats[0].failed == 0 + + assert stats[1].sent == 1 + assert stats[1].delivered == 1 + assert stats[1].failed == 0 + + assert stats[2].sent == 1 + assert stats[2].delivered == 1 + assert stats[2].failed == 0 def test_updating_one_type_of_notification_to_error_maintains_other_counts( notify_db, notify_db_session, - sample_job, + sample_service, sample_letter_template ): - sms_template = sample_template(notify_db, notify_db_session, service=sample_job.service) - email_template = sample_email_template(notify_db, notify_db_session, service=sample_job.service) + job_1 = sample_job(notify_db, notify_db_session, service=sample_service) + job_2 = sample_job(notify_db, notify_db_session, service=sample_service) + job_3 = sample_job(notify_db, notify_db_session, service=sample_service) + sms_template = sample_template(notify_db, notify_db_session, service=sample_service) + email_template = sample_email_template(notify_db, notify_db_session, service=sample_service) letter_template = sample_letter_template letter = sample_notification( notify_db, notify_db_session, - service=sample_job.service, + service=sample_service, template=letter_template, - job=sample_job, + job=job_1, status=NOTIFICATION_CREATED ) email = sample_notification( notify_db, notify_db_session, - service=sample_job.service, + service=sample_service, template=email_template, - job=sample_job, + job=job_2, status=NOTIFICATION_CREATED ) sms = sample_notification( notify_db, notify_db_session, - service=sample_job.service, + service=sample_service, template=sms_template, - job=sample_job, + job=job_3, status=NOTIFICATION_CREATED ) @@ -588,20 +642,50 @@ def test_updating_one_type_of_notification_to_error_maintains_other_counts( update_job_stats_outcome_count(email) update_job_stats_outcome_count(sms) - stats = JobStatistics.query.all() - assert len(stats) == 1 - assert stats[0].emails_sent == 1 - assert stats[0].sms_sent == 1 + stats = JobStatistics.query.order_by(JobStatistics.created_at).all() + assert len(stats) == 3 + assert stats[0].emails_sent == 0 + assert stats[0].sms_sent == 0 assert stats[0].letters_sent == 1 assert stats[0].emails_delivered == 0 assert stats[0].sms_delivered == 0 - assert stats[0].sms_failed == 1 - assert stats[0].emails_failed == 1 + assert stats[0].sms_failed == 0 + assert stats[0].emails_failed == 0 + assert stats[0].letters_failed == 1 + + assert stats[1].emails_sent == 1 + assert stats[1].sms_sent == 0 + assert stats[1].letters_sent == 0 + assert stats[1].emails_delivered == 0 + assert stats[1].sms_delivered == 0 + assert stats[1].sms_failed == 0 + assert stats[1].emails_failed == 1 + assert stats[1].letters_failed == 0 + + assert stats[2].emails_sent == 0 + assert stats[2].sms_sent == 1 + assert stats[2].letters_sent == 0 + assert stats[2].emails_delivered == 0 + assert stats[2].sms_delivered == 0 + assert stats[2].sms_failed == 1 + assert stats[2].emails_failed == 0 + assert stats[1].letters_failed == 0 + + assert stats[0].sent == 1 + assert stats[0].delivered == 0 + assert stats[0].failed == 1 + + assert stats[1].sent == 1 + assert stats[1].delivered == 0 + assert stats[1].failed == 1 + + assert stats[2].sent == 1 + assert stats[2].delivered == 0 + assert stats[2].failed == 1 -def test_will_not_timeout_job_counts_before_notification_timeouts(notify_db, notify_db_session, sample_job): - sms_template = sample_template(notify_db, notify_db_session, service=sample_job.service) - email_template = sample_email_template(notify_db, notify_db_session, service=sample_job.service) +def test_will_not_timeout_job_counts_before_notification_timeouts(notify_db, notify_db_session, + sample_job, sample_template): one_minute_ago = datetime.utcnow() - timedelta(minutes=1) @@ -609,43 +693,51 @@ def test_will_not_timeout_job_counts_before_notification_timeouts(notify_db, not notify_db, notify_db_session, service=sample_job.service, - template=sms_template, + template=sample_template, job=sample_job, status=NOTIFICATION_CREATED ) - email = sample_notification( + sms_2 = sample_notification( notify_db, notify_db_session, service=sample_job.service, - template=email_template, + template=sample_template, job=sample_job, status=NOTIFICATION_CREATED ) - create_or_update_job_sending_statistics(email) create_or_update_job_sending_statistics(sms) + create_or_update_job_sending_statistics(sms_2) JobStatistics.query.update({JobStatistics.created_at: one_minute_ago}) - intial_stats = JobStatistics.query.all() + initial_stats = JobStatistics.query.all() - assert intial_stats[0].emails_sent == 1 - assert intial_stats[0].sms_sent == 1 - assert intial_stats[0].emails_delivered == 0 - assert intial_stats[0].sms_delivered == 0 - assert intial_stats[0].sms_failed == 0 - assert intial_stats[0].emails_failed == 0 + assert initial_stats[0].emails_sent == 0 + assert initial_stats[0].sms_sent == 2 + assert initial_stats[0].emails_delivered == 0 + assert initial_stats[0].sms_delivered == 0 + assert initial_stats[0].sms_failed == 0 + assert initial_stats[0].emails_failed == 0 + + assert initial_stats[0].sent == 2 + assert initial_stats[0].delivered == 0 + assert initial_stats[0].failed == 0 dao_timeout_job_statistics(61) updated_stats = JobStatistics.query.all() - assert updated_stats[0].emails_sent == 1 - assert updated_stats[0].sms_sent == 1 + assert updated_stats[0].emails_sent == 0 + assert updated_stats[0].sms_sent == 2 assert updated_stats[0].emails_delivered == 0 assert updated_stats[0].sms_delivered == 0 assert updated_stats[0].sms_failed == 0 assert updated_stats[0].emails_failed == 0 + assert initial_stats[0].sent == 2 + assert initial_stats[0].delivered == 0 + assert initial_stats[0].failed == 0 + @pytest.mark.parametrize('notification_type, sms_count, email_count', [ (SMS_TYPE, 3, 0), @@ -688,6 +780,9 @@ def test_timeout_job_counts_timesout_multiple_jobs( assert stats.sms_delivered == 0 assert stats.sms_failed == 0 assert stats.emails_failed == 0 + assert stats.sent == email_count + sms_count + assert stats.delivered == 0 + assert stats.failed == 0 dao_timeout_job_statistics(1) updated_stats = JobStatistics.query.all() @@ -698,6 +793,9 @@ def test_timeout_job_counts_timesout_multiple_jobs( assert stats.sms_delivered == 0 assert stats.sms_failed == sms_count assert stats.emails_failed == email_count + assert stats.sent == email_count + sms_count + assert stats.delivered == 0 + assert stats.failed == email_count + sms_count count_notifications = len(NOTIFICATION_STATUS_TYPES) @@ -754,17 +852,13 @@ def test_timeout_job_sets_all_non_delivered_emails_to_error_and_doesnt_affect_sm assert initial_stats[0].sms_failed == 0 assert initial_stats[0].emails_failed == 0 - all = JobStatistics.query.all() - for a in all: - print(a) + assert initial_stats[0].sent == count_notifications + assert initial_stats[0].delivered == 0 + assert initial_stats[0].failed == 0 # timeout the notifications dao_timeout_job_statistics(1) - all = JobStatistics.query.all() - for a in all: - print(a) - # after timeout all delivered states are success and ALL other states are failed updated_stats = JobStatistics.query.filter_by(job_id=email_job.id).all() assert updated_stats[0].emails_sent == count_notifications @@ -774,6 +868,10 @@ def test_timeout_job_sets_all_non_delivered_emails_to_error_and_doesnt_affect_sm assert updated_stats[0].sms_failed == 0 assert updated_stats[0].emails_failed == count_error_notifications + assert initial_stats[0].sent == count_notifications + assert initial_stats[0].delivered == count_success_notifications + assert initial_stats[0].failed == count_error_notifications + sms_stats = JobStatistics.query.filter_by(job_id=sms_job.id).all() assert sms_stats[0].emails_sent == 0 assert sms_stats[0].sms_sent == 1 @@ -781,6 +879,9 @@ def test_timeout_job_sets_all_non_delivered_emails_to_error_and_doesnt_affect_sm assert sms_stats[0].sms_delivered == 0 assert sms_stats[0].sms_failed == 1 assert sms_stats[0].emails_failed == 0 + assert sms_stats[0].sent == 1 + assert sms_stats[0].delivered == 0 + assert sms_stats[0].failed == 1 # this test is as above, but for SMS not email @@ -810,6 +911,10 @@ def test_timeout_job_sets_all_non_delivered_states_to_error( assert stats.sms_failed == 0 assert stats.emails_failed == 0 + assert stats.sent == count_notifications + assert stats.delivered == 0 + assert stats.failed == 0 + dao_timeout_job_statistics(1) updated_stats = JobStatistics.query.all() @@ -820,3 +925,7 @@ def test_timeout_job_sets_all_non_delivered_states_to_error( assert stats.sms_delivered == count_success_notifications assert stats.sms_failed == count_error_notifications assert stats.emails_failed == 0 + + assert stats.sent == count_notifications + assert stats.delivered == count_success_notifications + assert stats.failed == count_error_notifications From f1399ca7f1cffe6ec2bbbbf3e0018866538cffea Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 7 Jun 2017 11:58:10 +0100 Subject: [PATCH 26/46] Fix support URLs in Notify emails --- app/user/rest.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/user/rest.py b/app/user/rest.py index cc0c6e41b..9a9cf3832 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -206,7 +206,7 @@ def send_user_confirm_new_email(user_id): personalisation={ 'name': user_to_send_to.name, 'url': _create_confirmation_url(user=user_to_send_to, email_address=email['email']), - 'feedback_url': current_app.config['ADMIN_BASE_URL'] + '/feedback' + 'feedback_url': current_app.config['ADMIN_BASE_URL'] + '/support' }, notification_type=EMAIL_TYPE, api_key_id=None, @@ -259,7 +259,7 @@ def send_already_registered_email(user_id): personalisation={ 'signin_url': current_app.config['ADMIN_BASE_URL'] + '/sign-in', 'forgot_password_url': current_app.config['ADMIN_BASE_URL'] + '/forgot-password', - 'feedback_url': current_app.config['ADMIN_BASE_URL'] + '/feedback' + 'feedback_url': current_app.config['ADMIN_BASE_URL'] + '/support' }, notification_type=EMAIL_TYPE, api_key_id=None, From 6b5451ea879c143538ffb9dc67397293e5bec679 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 7 Jun 2017 13:18:51 +0100 Subject: [PATCH 27/46] Add test for invalid UUID --- tests/app/service/test_rest.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index ac8537a4f..f65825227 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1248,6 +1248,15 @@ def test_get_all_notifications_for_service_in_order(notify_api, notify_db, notif assert response.status_code == 200 +def test_get_notification_for_service_without_uuid(client, notify_db, notify_db_session): + service_1 = create_service(notify_db, notify_db_session, service_name="1", email_from='1') + response = client.get( + path='/service/{}/notifications/{}'.format(service_1.id, 'foo'), + headers=[create_authorization_header()] + ) + assert response.status_code == 404 + + def test_get_notification_for_service(client, notify_db, notify_db_session): service_1 = create_service(notify_db, notify_db_session, service_name="1", email_from='1') From fa0d51b66c5ec6e177eed4dd2678ce5655ed8e14 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 7 Jun 2017 14:19:25 +0100 Subject: [PATCH 28/46] Added the free limit to the detailed service representation. --- app/schemas.py | 5 +++++ tests/app/service/test_rest.py | 13 +++++++++++++ 2 files changed, 18 insertions(+) diff --git a/app/schemas.py b/app/schemas.py index 8f8395f3e..7971300b7 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -260,6 +260,11 @@ class ServiceSchema(BaseSchema): class DetailedServiceSchema(BaseSchema): statistics = fields.Dict() + free_sms_fragment_limit = fields.Method(method_name='get_free_sms_fragment_limit') + + def get_free_sms_fragment_limit(selfs, service): + return service.free_sms_fragment_limit() + class Meta: model = models.Service exclude = ( diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index bc6010fa2..2f16543df 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -160,7 +160,20 @@ def test_get_service_by_id_returns_free_sms_limit(client, sample_service): assert json_resp['data']['free_sms_fragment_limit'] == 250000 +def test_get_detailed_service_by_id_returns_free_sms_limit(client, sample_service): + + auth_header = create_authorization_header() + resp = client.get( + '/service/{}?detailed=True'.format(sample_service.id), + headers=[auth_header] + ) + assert resp.status_code == 200 + json_resp = json.loads(resp.get_data(as_text=True)) + assert json_resp['data']['free_sms_fragment_limit'] == 250000 + + def test_get_service_list_has_default_permissions(client, service_factory): + service_factory.get('one') service_factory.get('one') service_factory.get('two') service_factory.get('three') From 5b4ceda1c6c49fc57b3c4fc978428354ac3cdfba Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Wed, 7 Jun 2017 14:23:31 +0100 Subject: [PATCH 29/46] Refactor: * Filter inbound by service_id * Refactor to return 404 instead of 400 for consistency --- app/dao/inbound_sms_dao.py | 7 +++++-- app/inbound_sms/rest.py | 15 +++------------ tests/app/dao/test_inbound_sms_dao.py | 2 +- tests/app/inbound_sms/test_rest.py | 17 ++++++++++++++--- 4 files changed, 23 insertions(+), 18 deletions(-) diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index d87c1b1ed..18060cced 100644 --- a/app/dao/inbound_sms_dao.py +++ b/app/dao/inbound_sms_dao.py @@ -49,5 +49,8 @@ def delete_inbound_sms_created_more_than_a_week_ago(): return deleted -def dao_get_inbound_sms_by_id(inbound_id): - return InboundSms.query.filter_by(id=inbound_id).one() +def dao_get_inbound_sms_by_id(service_id, inbound_id): + return InboundSms.query.filter_by( + id=inbound_id, + service_id=service_id + ).one() diff --git a/app/inbound_sms/rest.py b/app/inbound_sms/rest.py index 755f830d3..1eca4b089 100644 --- a/app/inbound_sms/rest.py +++ b/app/inbound_sms/rest.py @@ -1,11 +1,8 @@ -import uuid - from flask import ( Blueprint, jsonify, request ) -from werkzeug.exceptions import abort from notifications_utils.recipients import validate_and_format_phone_number @@ -19,7 +16,7 @@ from app.errors import register_errors inbound_sms = Blueprint( 'inbound_sms', __name__, - url_prefix='/service//inbound-sms' + url_prefix='/service//inbound-sms' ) register_errors(inbound_sms) @@ -50,14 +47,8 @@ def get_inbound_sms_summary_for_service(service_id): ) -@inbound_sms.route('/', methods=['GET']) +@inbound_sms.route('/', methods=['GET']) def get_inbound_by_id(service_id, inbound_sms_id): - # TODO: Add JSON Schema here - try: - validated_uuid = uuid.UUID(inbound_sms_id) - except (ValueError, AttributeError): - abort(400) - - inbound_sms = dao_get_inbound_sms_by_id(validated_uuid) + inbound_sms = dao_get_inbound_sms_by_id(service_id, inbound_sms_id) return jsonify(inbound_sms.serialize()), 200 diff --git a/tests/app/dao/test_inbound_sms_dao.py b/tests/app/dao/test_inbound_sms_dao.py index 6162b6d82..b26dc913e 100644 --- a/tests/app/dao/test_inbound_sms_dao.py +++ b/tests/app/dao/test_inbound_sms_dao.py @@ -92,6 +92,6 @@ def test_should_not_delete_inbound_sms_before_seven_days(sample_service): def test_get_inbound_sms_by_id_returns(sample_service): inbound = create_inbound_sms(sample_service) - inbound_from_db = dao_get_inbound_sms_by_id(inbound.id) + inbound_from_db = dao_get_inbound_sms_by_id(sample_service.id, inbound.id) assert inbound == inbound_from_db diff --git a/tests/app/inbound_sms/test_rest.py b/tests/app/inbound_sms/test_rest.py index 4f021d03a..b4c55cd83 100644 --- a/tests/app/inbound_sms/test_rest.py +++ b/tests/app/inbound_sms/test_rest.py @@ -129,12 +129,23 @@ def test_get_inbound_sms_by_id_returns_200(admin_request, sample_service): assert response['service_id'] == str(sample_service.id) -def test_get_inbound_sms_by_id_invalid_id_returns_400(admin_request, sample_service): +def test_get_inbound_sms_by_id_invalid_id_returns_404(admin_request, sample_service): assert admin_request.get( 'inbound_sms.get_inbound_by_id', endpoint_kwargs={ 'service_id': sample_service.id, - 'inbound_sms_id': 'dsadsda' + 'inbound_sms_id': 'bar' }, - expected_status=400 + expected_status=404 + ) + + +def test_get_inbound_sms_by_id_with_invalid_service_id_returns_404(admin_request, sample_service): + assert admin_request.get( + 'inbound_sms.get_inbound_by_id', + endpoint_kwargs={ + 'service_id': 'foo', + 'inbound_sms_id': '2cfbd6a1-1575-4664-8969-f27be0ea40d9' + }, + expected_status=404 ) From d5fc02b14b5cd9aca9e7dd00350b1de799a01cd5 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 7 Jun 2017 15:13:48 +0100 Subject: [PATCH 30/46] removed print statement :-( --- tests/conftest.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/conftest.py b/tests/conftest.py index a59de1958..bf9823331 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -115,7 +115,6 @@ def set_config(app, name, value): old_val = app.config.get(name) app.config[name] = value yield - print(app.config) app.config[name] = old_val From 1d30d93c6f1182103a4d9f4e6dc2c6a89a52e968 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Wed, 7 Jun 2017 16:31:14 +0100 Subject: [PATCH 31/46] Add s3 method to remove transformed dvla files --- app/aws/s3.py | 7 +++++++ app/v2/notifications/get_notifications.py | 2 +- tests/app/aws/test_s3.py | 20 ++++++++++++++++++-- 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index ed206b3a4..f5798314b 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -26,3 +26,10 @@ def remove_job_from_s3(service_id, job_id): file_location = FILE_LOCATION_STRUCTURE.format(service_id, job_id) obj = get_s3_object(bucket_name, file_location) return obj.delete() + + +def remove_transformed_dvla_file(job_id): + bucket_name = current_app.config['DVLA_UPLOAD_BUCKET_NAME'] + file_location = '{}-dvla-job.text'.format(job_id) + obj = get_s3_object(bucket_name, file_location) + return obj.delete() diff --git a/app/v2/notifications/get_notifications.py b/app/v2/notifications/get_notifications.py index 08a0d3c5e..cd721f596 100644 --- a/app/v2/notifications/get_notifications.py +++ b/app/v2/notifications/get_notifications.py @@ -14,7 +14,7 @@ from app.v2.notifications.notification_schemas import get_notifications_request def get_notification_by_id(id): try: casted_id = uuid.UUID(id) - except ValueError or AttributeError: + except (ValueError, AttributeError): abort(404) notification = notifications_dao.get_notification_with_personalisation( authenticated_service.id, casted_id, key_type=None diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py index 38c9966dd..fd30c16df 100644 --- a/tests/app/aws/test_s3.py +++ b/tests/app/aws/test_s3.py @@ -1,7 +1,11 @@ -from app.aws.s3 import get_s3_file +from unittest.mock import call + +from flask import current_app + +from app.aws.s3 import get_s3_file, remove_transformed_dvla_file -def test_get_s3_file_makes_correct_call(sample_service, sample_job, mocker): +def test_get_s3_file_makes_correct_call(notify_api, mocker): get_s3_mock = mocker.patch('app.aws.s3.get_s3_object') get_s3_file('foo-bucket', 'bar-file.txt') @@ -9,3 +13,15 @@ def test_get_s3_file_makes_correct_call(sample_service, sample_job, mocker): 'foo-bucket', 'bar-file.txt' ) + + +def test_remove_transformed_dvla_file_makes_correct_call(notify_api, mocker): + s3_mock = mocker.patch('app.aws.s3.get_s3_object') + fake_uuid = '5fbf9799-6b9b-4dbb-9a4e-74a939f3bb49' + + remove_transformed_dvla_file(fake_uuid) + + s3_mock.assert_has_calls([ + call(current_app.config['DVLA_UPLOAD_BUCKET_NAME'], '{}-dvla-job.text'.format(fake_uuid)), + call().delete() + ]) From c4a89b77e38b79e345601187e644a3ad3a83623c Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Wed, 7 Jun 2017 16:31:51 +0100 Subject: [PATCH 32/46] Add task to remove transformed dvla files: * Runs at 4:40am * Remove files older than 7 days and only two day's worth --- app/celery/scheduled_tasks.py | 10 ++++++ app/config.py | 5 +++ tests/app/celery/test_scheduled_tasks.py | 44 ++++++++++++++++++++++-- 3 files changed, 56 insertions(+), 3 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 636522f01..11e4de49d 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -24,6 +24,7 @@ from app.dao.provider_details_dao import ( dao_toggle_sms_provider ) from app.dao.users_dao import delete_codes_older_created_more_than_a_day_ago +from app.models import LETTER_TYPE from app.notifications.process_notifications import send_notification_to_queue from app.statsd_decorators import statsd from app.celery.tasks import process_job @@ -245,3 +246,12 @@ def delete_inbound_sms_older_than_seven_days(): except SQLAlchemyError as e: current_app.logger.exception("Failed to delete inbound sms notifications") raise + + +@notify_celery.task(name="remove_transformed_dvla_files") +@statsd(namespace="tasks") +def remove_transformed_dvla_files(): + jobs = dao_get_jobs_older_than_limited_by(job_types=[LETTER_TYPE]) + for job in jobs: + s3.remove_transformed_dvla_file(job.id) + current_app.logger.info("Transformed dvla file for job {} has been removed from s3.".format(job.id)) diff --git a/app/config.py b/app/config.py index cbaa15d67..08f39e656 100644 --- a/app/config.py +++ b/app/config.py @@ -204,6 +204,11 @@ class Config(object): 'options': {'queue': QueueNames.PERIODIC}, 'kwargs': {'job_types': [LETTER_TYPE]} }, + 'remove_transformed_dvla_files': { + 'task': 'remove_transformed_dvla_files', + 'schedule': crontab(minute=40, hour=4), + 'options': {'queue': QueueNames.PERIODIC} + }, 'timeout-job-statistics': { 'task': 'timeout-job-statistics', 'schedule': crontab(minute=0, hour=5), diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index b3ff5d478..36b9ad668 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -1,10 +1,12 @@ -import pytest - from datetime import datetime, timedelta from functools import partial +from unittest.mock import call, patch, PropertyMock from flask import current_app + +import pytest from freezegun import freeze_time + from app.celery import scheduled_tasks from app.celery.scheduled_tasks import ( delete_email_notifications_older_than_seven_days, @@ -15,6 +17,7 @@ from app.celery.scheduled_tasks import ( delete_sms_notifications_older_than_seven_days, delete_verify_codes, remove_csv_files, + remove_transformed_dvla_files, run_scheduled_jobs, s3, send_daily_performance_platform_stats, @@ -41,7 +44,6 @@ from tests.app.conftest import ( sample_notification_history as create_notification_history, create_custom_template) from tests.conftest import set_config_values -from unittest.mock import call, patch, PropertyMock def _create_slow_delivery_notification(provider='mmg'): @@ -89,6 +91,8 @@ def test_should_have_decorated_tasks_functions(): 'switch_current_sms_provider_on_slow_delivery' assert delete_inbound_sms_older_than_seven_days.__wrapped__.__name__ == \ 'delete_inbound_sms_older_than_seven_days' + assert remove_transformed_dvla_files.__wrapped__.__name__ == \ + 'remove_transformed_dvla_files' def test_should_call_delete_sms_notifications_more_than_week_in_task(notify_api, mocker): @@ -488,3 +492,37 @@ def test_remove_csv_files_filters_by_type(mocker, sample_service): assert s3.remove_job_from_s3.call_args_list == [ call(job_to_delete.service_id, job_to_delete.id), ] + + +@freeze_time('2017-01-01 10:00:00') +def test_remove_dvla_transformed_files_removes_expected_files(mocker, sample_service): + mocker.patch('app.celery.scheduled_tasks.s3.remove_transformed_dvla_file') + + letter_template = create_template(service=sample_service, template_type=LETTER_TYPE) + + job = partial(create_job, template=letter_template) + + seven_days_ago = datetime.utcnow() - timedelta(days=7) + just_under_seven_days = seven_days_ago + timedelta(seconds=1) + just_over_seven_days = seven_days_ago - timedelta(seconds=1) + eight_days_ago = seven_days_ago - timedelta(days=1) + nine_days_ago = eight_days_ago - timedelta(days=1) + just_under_nine_days = nine_days_ago + timedelta(seconds=1) + just_over_nine_days = nine_days_ago - timedelta(seconds=1) + + job(created_at=seven_days_ago) + job(created_at=just_under_seven_days) + job_to_delete_1 = job(created_at=just_over_seven_days) + job_to_delete_2 = job(created_at=eight_days_ago) + job_to_delete_3 = job(created_at=nine_days_ago) + job_to_delete_4 = job(created_at=just_under_nine_days) + job(created_at=just_over_nine_days) + + remove_transformed_dvla_files() + + s3.remove_transformed_dvla_file.assert_has_calls([ + call(job_to_delete_1.id), + call(job_to_delete_2.id), + call(job_to_delete_3.id), + call(job_to_delete_4.id), + ], any_order=True) From 2fe427ac5acbbe25e62e6e4195983946a2b00ad5 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 8 Jun 2017 11:04:07 +0100 Subject: [PATCH 33/46] Bumped version of celery --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 46b02daa3..deef81325 100644 --- a/requirements.txt +++ b/requirements.txt @@ -13,7 +13,7 @@ flask-marshmallow==0.6.2 Flask-Bcrypt==0.6.2 credstash==1.8.0 boto3==1.4.4 -celery==3.1.23 +celery==3.1.25 monotonic==1.2 statsd==3.2.1 jsonschema==2.5.1 From c6db12731a0114c50988ce75a1737bcbdc1b299f Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 8 Jun 2017 13:06:24 +0100 Subject: [PATCH 34/46] Add test to check dvla files (unexpected) are not removed --- tests/app/celery/test_scheduled_tasks.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 36b9ad668..fcdd8ea93 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -526,3 +526,25 @@ def test_remove_dvla_transformed_files_removes_expected_files(mocker, sample_ser call(job_to_delete_3.id), call(job_to_delete_4.id), ], any_order=True) + + +def test_remove_dvla_transformed_files_does_not_remove_files(mocker, sample_service): + mocker.patch('app.celery.scheduled_tasks.s3.remove_transformed_dvla_file') + + letter_template = create_template(service=sample_service, template_type=LETTER_TYPE) + + job = partial(create_job, template=letter_template) + + yesterday = datetime.utcnow() - timedelta(days=1) + six_days_ago = datetime.utcnow() - timedelta(days=6) + seven_days_ago = six_days_ago - timedelta(days=1) + just_over_nine_days = seven_days_ago - timedelta(days=2, seconds=1) + + job(created_at=yesterday) + job(created_at=six_days_ago) + job(created_at=seven_days_ago) + job(created_at=just_over_nine_days) + + remove_transformed_dvla_files() + + s3.remove_transformed_dvla_file.assert_has_calls([]) From 0cee176d8b06d13d0406d0ce015c8fcffa9b3ecb Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 8 Jun 2017 15:24:18 +0100 Subject: [PATCH 35/46] Fix merge conflicts --- ...ng_svc_perms.py => 0095_migrate_existing_svc_perms.py} | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) rename migrations/versions/{0088_migrate_existing_svc_perms.py => 0095_migrate_existing_svc_perms.py} (89%) diff --git a/migrations/versions/0088_migrate_existing_svc_perms.py b/migrations/versions/0095_migrate_existing_svc_perms.py similarity index 89% rename from migrations/versions/0088_migrate_existing_svc_perms.py rename to migrations/versions/0095_migrate_existing_svc_perms.py index 9e54ee4b4..0211450f8 100644 --- a/migrations/versions/0088_migrate_existing_svc_perms.py +++ b/migrations/versions/0095_migrate_existing_svc_perms.py @@ -1,14 +1,14 @@ """empty message -Revision ID: 0088_migrate_existing_svc_perms -Revises: 0087_scheduled_notifications +Revision ID: 0095_migrate_existing_svc_perms +Revises: 0094_job_stats_update Create Date: 2017-05-23 18:13:03.532095 """ # revision identifiers, used by Alembic. -revision = '0088_migrate_existing_svc_perms' -down_revision = '0087_scheduled_notifications' +revision = '0095_migrate_existing_svc_perms' +down_revision = '0094_job_stats_update' from alembic import op import sqlalchemy as sa From 7b58886d04db8a49ce1551e23079f8ec2b8c0cd5 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 8 Jun 2017 16:24:16 +0100 Subject: [PATCH 36/46] Update the new job_statistics columns with the right values. --- migrations/versions/0096_update_job_stats.py | 35 ++++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 migrations/versions/0096_update_job_stats.py diff --git a/migrations/versions/0096_update_job_stats.py b/migrations/versions/0096_update_job_stats.py new file mode 100644 index 000000000..75b345ead --- /dev/null +++ b/migrations/versions/0096_update_job_stats.py @@ -0,0 +1,35 @@ +"""empty message + +Revision ID: 0096_update_job_stats +Revises: 0095_migrate_existing_svc_perms +Create Date: 2017-06-08 15:46:49.637642 + +""" + +# revision identifiers, used by Alembic. +revision = '0096_update_job_stats' +down_revision = '0095_migrate_existing_svc_perms' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + + +def upgrade(): + query = "UPDATE job_statistics " \ + "set sent = sms_sent + emails_sent + letters_sent, " \ + " delivered = sms_delivered + emails_delivered, " \ + " failed = sms_failed + emails_failed + letters_failed " + + conn = op.get_bind() + conn.execute(query) + + +def downgrade(): + query = "UPDATE job_statistics " \ + "set sent = 0, " \ + " delivered = 0, " \ + " failed = 0 " + + conn = op.get_bind() + conn.execute(query) From 7dccf796dd4f1af66db6a3c547a34c87d6b86339 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 9 Jun 2017 10:34:02 +0100 Subject: [PATCH 37/46] Adds a POST version of get inbound messages by phone number. Allows us to POST the search so the phone number is hidden --- app/errors.py | 10 +- app/inbound_sms/inbound_sms_schemas.py | 9 ++ app/inbound_sms/rest.py | 35 ++++-- tests/app/inbound_sms/test_rest.py | 156 +++++++++++++++++++++++++ 4 files changed, 196 insertions(+), 14 deletions(-) create mode 100644 app/inbound_sms/inbound_sms_schemas.py diff --git a/app/errors.py b/app/errors.py index 7e7790bc8..5a0d4710a 100644 --- a/app/errors.py +++ b/app/errors.py @@ -1,10 +1,11 @@ from flask import ( jsonify, - current_app -) + current_app, + json) from sqlalchemy.exc import SQLAlchemyError, DataError from sqlalchemy.orm.exc import NoResultFound from marshmallow import ValidationError +from jsonschema import ValidationError as JsonSchemaValidationError from app.authentication.auth import AuthError from app.notifications import SendNotificationToQueueError @@ -50,6 +51,11 @@ def register_errors(blueprint): current_app.logger.error(error) return jsonify(result='error', message=error.messages), 400 + @blueprint.errorhandler(JsonSchemaValidationError) + def validation_error(error): + current_app.logger.exception(error) + return jsonify(json.loads(error.message)), 400 + @blueprint.errorhandler(InvalidRequest) def invalid_data(error): response = jsonify(error.to_dict()) diff --git a/app/inbound_sms/inbound_sms_schemas.py b/app/inbound_sms/inbound_sms_schemas.py new file mode 100644 index 000000000..448a4030e --- /dev/null +++ b/app/inbound_sms/inbound_sms_schemas.py @@ -0,0 +1,9 @@ +get_inbound_sms_for_service_schema = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "schema for parameters allowed when searching for to field=", + "type": "object", + "properties": { + "phone_number": {"type": "string", "format": "phone_number"}, + "limit": {"type": ["integer", "null"], "minimum": 1} + } +} \ No newline at end of file diff --git a/app/inbound_sms/rest.py b/app/inbound_sms/rest.py index 1eca4b089..a78e3c237 100644 --- a/app/inbound_sms/rest.py +++ b/app/inbound_sms/rest.py @@ -1,8 +1,9 @@ from flask import ( Blueprint, jsonify, - request -) + request, + current_app, json) +from jsonschema import ValidationError from notifications_utils.recipients import validate_and_format_phone_number @@ -12,6 +13,9 @@ from app.dao.inbound_sms_dao import ( dao_get_inbound_sms_by_id ) from app.errors import register_errors +from app.schema_validation import validate + +from app.inbound_sms.inbound_sms_schemas import get_inbound_sms_for_service_schema inbound_sms = Blueprint( 'inbound_sms', @@ -22,18 +26,25 @@ inbound_sms = Blueprint( register_errors(inbound_sms) -@inbound_sms.route('') +@inbound_sms.route('', methods=['POST', 'GET']) def get_inbound_sms_for_service(service_id): - limit = request.args.get('limit') - user_number = request.args.get('user_number') - if user_number: - # we use this to normalise to an international phone number - user_number = validate_and_format_phone_number(user_number, international=True) + if request.method == 'GET': + limit = request.args.get('limit') + user_number = request.args.get('user_number') - results = dao_get_inbound_sms_for_service(service_id, limit, user_number) + if user_number: + # we use this to normalise to an international phone number + user_number = validate_and_format_phone_number(user_number, international=True) - return jsonify(data=[row.serialize() for row in results]) + results = dao_get_inbound_sms_for_service(service_id, limit, user_number) + + return jsonify(data=[row.serialize() for row in results]) + else: + form = validate(request.get_json(), get_inbound_sms_for_service_schema) + results = dao_get_inbound_sms_for_service(service_id, form.get('limit'), form.get('phone_number')) + + return jsonify(data=[row.serialize() for row in results]) @inbound_sms.route('/summary') @@ -49,6 +60,6 @@ def get_inbound_sms_summary_for_service(service_id): @inbound_sms.route('/', methods=['GET']) def get_inbound_by_id(service_id, inbound_sms_id): - inbound_sms = dao_get_inbound_sms_by_id(service_id, inbound_sms_id) + message = dao_get_inbound_sms_by_id(service_id, inbound_sms_id) - return jsonify(inbound_sms.serialize()), 200 + return jsonify(message.serialize()), 200 diff --git a/tests/app/inbound_sms/test_rest.py b/tests/app/inbound_sms/test_rest.py index b4c55cd83..5edc3bbcb 100644 --- a/tests/app/inbound_sms/test_rest.py +++ b/tests/app/inbound_sms/test_rest.py @@ -1,11 +1,163 @@ from datetime import datetime import pytest +from flask import json from freezegun import freeze_time +from tests import create_authorization_header from tests.app.db import create_inbound_sms, create_service +def test_get_inbound_sms_with_no_params(client, sample_service): + one = create_inbound_sms(sample_service) + two = create_inbound_sms(sample_service) + + auth_header = create_authorization_header() + + data = {} + + response = client.post( + path='/service/{}/inbound-sms'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + json_resp = json.loads(response.get_data(as_text=True)) + sms = json_resp['data'] + + assert len(sms) == 2 + assert {inbound['id'] for inbound in sms} == {str(one.id), str(two.id)} + assert sms[0]['content'] == 'Hello' + assert set(sms[0].keys()) == { + 'id', + 'created_at', + 'service_id', + 'notify_number', + 'user_number', + 'content', + 'provider_date', + 'provider_reference' + } + + +def test_get_inbound_sms_with_limit(client, sample_service): + with freeze_time('2017-01-01'): + one = create_inbound_sms(sample_service) + with freeze_time('2017-01-02'): + two = create_inbound_sms(sample_service) + + auth_header = create_authorization_header() + + data = {'limit': 1} + + response = client.post( + path='/service/{}/inbound-sms'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + json_resp = json.loads(response.get_data(as_text=True)) + sms = json_resp['data'] + + assert len(sms) == 1 + assert sms[0]['id'] == str(two.id) + + +def test_get_inbound_sms_should_error_with_invalid_limit(client, sample_service): + + auth_header = create_authorization_header() + + data = {'limit': 'limit'} + + response = client.post( + path='/service/{}/inbound-sms'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + error_resp = json.loads(response.get_data(as_text=True)) + assert error_resp['status_code'] == 400 + assert error_resp['errors'] == [{ + 'error': 'ValidationError', + 'message': "limit limit is not of type integer, null" + }] + + +def test_get_inbound_sms_should_error_with_invalid_phone_number(client, sample_service): + + auth_header = create_authorization_header() + + data = {'phone_number': 'invalid phone number'} + + response = client.post( + path='/service/{}/inbound-sms'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + error_resp = json.loads(response.get_data(as_text=True)) + assert error_resp['status_code'] == 400 + assert error_resp['errors'] == [{ + 'error': 'ValidationError', + 'message': "phone_number Must not contain letters or symbols" + }] + + +@pytest.mark.parametrize('user_number', [ + '(07700) 900-001', + '+4407700900001', + '447700900001', +]) +def test_get_inbound_sms_filters_user_number(client, sample_service, user_number): + # user_number in the db is international and normalised + one = create_inbound_sms(sample_service, user_number='447700900001') + two = create_inbound_sms(sample_service, user_number='447700900002') + + auth_header = create_authorization_header() + + data = { + 'limit': 1, + 'phone_number': user_number + } + + response = client.post( + path='/service/{}/inbound-sms'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + json_resp = json.loads(response.get_data(as_text=True)) + sms = json_resp['data'] + assert len(sms) == 1 + assert sms[0]['id'] == str(one.id) + assert sms[0]['user_number'] == str(one.user_number) + + +def test_get_inbound_sms_filters_international_user_number(admin_request, sample_service): + # user_number in the db is international and normalised + one = create_inbound_sms(sample_service, user_number='12025550104') + two = create_inbound_sms(sample_service) + + auth_header = create_authorization_header() + + data = { + 'limit': 1, + 'phone_number': '+1 (202) 555-0104' + } + + response = client.post( + path='/service/{}/inbound-sms'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + json_resp = json.loads(response.get_data(as_text=True)) + sms = json_resp['data'] + + assert len(sms) == 1 + assert sms[0]['id'] == str(one.id) + assert sms[0]['user_number'] == str(one.user_number) + + +############################################################## +# REMOVE ONCE ADMIN MIGRATED AND GET ENDPOINT REMOVED +############################################################## + + def test_get_inbound_sms(admin_request, sample_service): one = create_inbound_sms(sample_service) two = create_inbound_sms(sample_service) @@ -82,6 +234,10 @@ def test_get_inbound_sms_filters_international_user_number(admin_request, sample assert sms['data'][0]['user_number'] == str(one.user_number) +############################## +# End delete section +############################## + def test_get_inbound_sms_summary(admin_request, sample_service): other_service = create_service(service_name='other_service') with freeze_time('2017-01-01'): From ccfcb5678f3af1fcd22238d04a7516d3f6f3d25f Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 9 Jun 2017 10:44:48 +0100 Subject: [PATCH 38/46] Missing white space --- app/inbound_sms/inbound_sms_schemas.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/inbound_sms/inbound_sms_schemas.py b/app/inbound_sms/inbound_sms_schemas.py index 448a4030e..c0e644d68 100644 --- a/app/inbound_sms/inbound_sms_schemas.py +++ b/app/inbound_sms/inbound_sms_schemas.py @@ -6,4 +6,4 @@ get_inbound_sms_for_service_schema = { "phone_number": {"type": "string", "format": "phone_number"}, "limit": {"type": ["integer", "null"], "minimum": 1} } -} \ No newline at end of file +} From c7707873e4877093a10964338aa5a9a4b35bcdf1 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 12 Jun 2017 14:25:17 +0100 Subject: [PATCH 39/46] Queries to get job statistics by job_id or by service_id. --- app/dao/jobs_dao.py | 35 +++++++++- tests/app/dao/test_jobs_dao.py | 117 ++++++++++++++++++++++++++++++++- 2 files changed, 150 insertions(+), 2 deletions(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index 86e565619..d6770943f 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -8,7 +8,7 @@ from app.dao import days_ago from app.models import ( Job, JobStatistics, Notification, NotificationHistory, Template, JOB_STATUS_SCHEDULED, JOB_STATUS_PENDING, - EMAIL_TYPE, SMS_TYPE, LETTER_TYPE + LETTER_TYPE ) from app.statsd_decorators import statsd @@ -142,3 +142,36 @@ def dao_get_all_letter_jobs(): return db.session.query(Job).join(Job.template).filter( Template.template_type == LETTER_TYPE ).order_by(desc(Job.created_at)).all() + + +@statsd(namespace="dao") +def dao_get_job_statistics_for_job(job_id): + query = db.session.query( + JobStatistics.job_id, + Job.original_file_name, + Job.created_at, + JobStatistics.sent, + JobStatistics.delivered, + JobStatistics.failed + ).filter(JobStatistics.job_id == job_id).filter(Job.id == JobStatistics.job_id) + return query.one() + + +@statsd(namespace="dao") +def dao_get_job_stats_for_service(service_id, page=1, page_size=50, limit_days=None): + query = Job.query.join( + JobStatistics, Job.id == JobStatistics.job_id + ).filter( + Job.service_id == service_id + ).add_columns( + JobStatistics.job_id, + Job.original_file_name, + Job.created_at, + JobStatistics.sent, + JobStatistics.delivered, + JobStatistics.failed + ) + if limit_days: + query = query.filter(Job.created_at >= days_ago(limit_days)) + query = query.order_by(Job.created_at.desc()) + return query.paginate(page=page, per_page=page_size).items diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index 42f93d230..476e6b1de 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -16,7 +16,10 @@ from app.dao.jobs_dao import ( all_notifications_are_created_for_job, dao_update_job_status, dao_get_all_notifications_for_job, - dao_get_jobs_older_than_limited_by) + dao_get_jobs_older_than_limited_by, + dao_get_job_statistics_for_job, + dao_get_job_stats_for_service) +from app.dao.statistics_dao import create_or_update_job_sending_statistics, update_job_stats_outcome_count from app.models import ( Job, JobStatistics, EMAIL_TYPE, SMS_TYPE, LETTER_TYPE @@ -411,3 +414,115 @@ def test_should_get_jobs_seven_days_old_filters_type(notify_db, notify_db_sessio assert len(jobs) == 2 assert job_to_remain.id not in [job.id for job in jobs] + + +def test_dao_get_job_statistics_for_job(notify_db, notify_db_session, sample_job): + notification = create_notification(notify_db=notify_db, notify_db_session=notify_db_session, job=sample_job) + notification_delivered = create_notification(notify_db=notify_db, notify_db_session=notify_db_session, + job=sample_job, status='delivered') + notification_failed = create_notification(notify_db=notify_db, notify_db_session=notify_db_session, job=sample_job, + status='permanent-failure') + + create_or_update_job_sending_statistics(notification) + create_or_update_job_sending_statistics(notification_delivered) + create_or_update_job_sending_statistics(notification_failed) + update_job_stats_outcome_count(notification_delivered) + update_job_stats_outcome_count(notification_failed) + results = dao_get_job_statistics_for_job(sample_job.id) + assert results.job_id == sample_job.id + assert results.sent == 3 + assert results.delivered == 1 + assert results.failed == 1 + + +def test_dao_get_job_statistics_for_job(notify_db, notify_db_session, sample_service): + job_1, job_2 = stats_set_up(notify_db, notify_db_session, sample_service) + results = dao_get_job_statistics_for_job(job_1.id) + assert_job_stat(job_1, results, 2, 1, 0) + + results_2 = dao_get_job_statistics_for_job(job_2.id) + assert_job_stat(job_2, results_2, 1, 0, 1) + + +def test_dao_get_job_stats_for_service(notify_db, notify_db_session, sample_service): + job_1, job_2 = stats_set_up(notify_db, notify_db_session, sample_service) + + results = dao_get_job_stats_for_service(sample_service.id) + assert len(results) == 2 + assert_job_stat(job_2, results[0], 1, 0, 1) + assert_job_stat(job_1, results[1], 2, 1, 0) + + +def test_dao_get_job_stats_for_service_only_returns_stats_for_service(notify_db, notify_db_session, sample_service): + job_1, job_2 = stats_set_up(notify_db, notify_db_session, sample_service) + another_service = create_service(notify_db=notify_db, notify_db_session=notify_db_session, + service_name='Another Service') + job_3, job_4 = stats_set_up(notify_db, notify_db_session, service=another_service) + + results = dao_get_job_stats_for_service(sample_service.id) + assert len(results) == 2 + assert_job_stat(job_2, results[0], 1, 0, 1) + assert_job_stat(job_1, results[1], 2, 1, 0) + + results = dao_get_job_stats_for_service(another_service.id) + assert len(results) == 2 + assert_job_stat(job_4, results[0], 1, 0, 1) + assert_job_stat(job_3, results[1], 2, 1, 0) + + +def test_dao_get_job_stats_for_service_only_returns_jobs_created_within_limited_days( + notify_db, notify_db_session, sample_service): + job_1, job_2 = stats_set_up(notify_db, notify_db_session, sample_service) + + results = dao_get_job_stats_for_service(sample_service.id, limit_days=1) + assert len(results) == 1 + assert_job_stat(job_2, results[0], 1, 0, 1) + + +def test_dao_get_job_stats_for_service_only_returns_jobs_created_within_limited_days_inclusive( + notify_db, notify_db_session, sample_service): + job_1, job_2 = stats_set_up(notify_db, notify_db_session, sample_service) + + results = dao_get_job_stats_for_service(sample_service.id, limit_days=2) + assert len(results) == 2 + assert_job_stat(job_2, results[0], 1, 0, 1) + assert_job_stat(job_1, results[1], 2, 1, 0) + + +def test_dao_get_job_stats_paginates_results( + notify_db, notify_db_session, sample_service): + job_1, job_2 = stats_set_up(notify_db, notify_db_session, sample_service) + + results = dao_get_job_stats_for_service(sample_service.id, page=1, page_size=1) + assert len(results) == 1 + assert_job_stat(job_2, results[0], 1, 0, 1) + results_2 = dao_get_job_stats_for_service(sample_service.id, page=2, page_size=1) + assert len(results_2) == 1 + assert_job_stat(job_1, results_2[0], 2, 1, 0) + + +def assert_job_stat(job, result, sent, delivered, failed): + assert result.job_id == job.id + assert result.original_file_name == job.original_file_name + assert result.created_at == job.created_at + assert result.sent == sent + assert result.delivered == delivered + assert result.failed == failed + + +def stats_set_up(notify_db, notify_db_session, service): + job_1 = create_job(notify_db=notify_db, notify_db_session=notify_db_session, + service=service, created_at=datetime.utcnow() - timedelta(days=2)) + job_2 = create_job(notify_db=notify_db, notify_db_session=notify_db_session, + service=service, original_file_name='Another job') + notification = create_notification(notify_db=notify_db, notify_db_session=notify_db_session, job=job_1) + notification_delivered = create_notification(notify_db=notify_db, notify_db_session=notify_db_session, + job=job_1, status='delivered') + notification_failed = create_notification(notify_db=notify_db, notify_db_session=notify_db_session, job=job_2, + status='permanent-failure') + create_or_update_job_sending_statistics(notification) + create_or_update_job_sending_statistics(notification_delivered) + create_or_update_job_sending_statistics(notification_failed) + update_job_stats_outcome_count(notification_delivered) + update_job_stats_outcome_count(notification_failed) + return job_1, job_2 From cc04b5eb1ddb3d568183ee5f7e50f67384ea25c1 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 12 Jun 2017 17:15:32 +0100 Subject: [PATCH 40/46] Added new endpoint for job statistics. The structure has been flattened, so I need to create a new endpoint, start using that endpoint, then change the name back. Added template_id and version to the get job stats by id. --- app/dao/jobs_dao.py | 25 +++++++++++++---- app/job/rest.py | 50 ++++++++++++++++++++++++++++++++-- tests/app/dao/test_jobs_dao.py | 49 ++++++++++++++++++++------------- tests/app/job/test_rest.py | 43 +++++++++++++++++++++++++++++ 4 files changed, 141 insertions(+), 26 deletions(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index d6770943f..9d791b8ad 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -145,20 +145,31 @@ def dao_get_all_letter_jobs(): @statsd(namespace="dao") -def dao_get_job_statistics_for_job(job_id): - query = db.session.query( +def dao_get_job_statistics_for_job(service_id, job_id): + query = Job.query.join( + JobStatistics, Job.id == JobStatistics.job_id + ).filter( + Job.id == job_id + ).join( + Template, Template.id == Job.template_id and Template.version == Job.template_version + ).add_columns( JobStatistics.job_id, Job.original_file_name, Job.created_at, + Job.scheduled_for, + Job.template_id, + Job.template_version, JobStatistics.sent, JobStatistics.delivered, JobStatistics.failed - ).filter(JobStatistics.job_id == job_id).filter(Job.id == JobStatistics.job_id) + ).filter( + Job.service_id == service_id + ) return query.one() @statsd(namespace="dao") -def dao_get_job_stats_for_service(service_id, page=1, page_size=50, limit_days=None): +def dao_get_job_stats_for_service(service_id, page=1, page_size=50, limit_days=None, statuses=None): query = Job.query.join( JobStatistics, Job.id == JobStatistics.job_id ).filter( @@ -167,11 +178,15 @@ def dao_get_job_stats_for_service(service_id, page=1, page_size=50, limit_days=N JobStatistics.job_id, Job.original_file_name, Job.created_at, + Job.scheduled_for, JobStatistics.sent, JobStatistics.delivered, JobStatistics.failed ) if limit_days: query = query.filter(Job.created_at >= days_ago(limit_days)) + if statuses is not None and statuses != ['']: + query = query.filter(Job.job_status.in_(statuses)) + query = query.order_by(Job.created_at.desc()) - return query.paginate(page=page, per_page=page_size).items + return query.paginate(page=page, per_page=page_size) diff --git a/app/job/rest.py b/app/job/rest.py index b4882945f..5e5bc33c1 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -11,8 +11,8 @@ from app.dao.jobs_dao import ( dao_get_job_by_service_id_and_job_id, dao_get_jobs_by_service_id, dao_get_future_scheduled_job_by_id_and_service_id, - dao_get_notification_outcomes_for_job -) + dao_get_notification_outcomes_for_job, + dao_get_job_stats_for_service) from app.dao.services_dao import ( dao_fetch_service_by_id @@ -117,6 +117,52 @@ def get_jobs_by_service(service_id): return jsonify(**get_paginated_jobs(service_id, limit_days, statuses, page)) +@job_blueprint.route('/job-stats', methods=['GET']) +def get_jobs_for_service(service_id): + if request.args.get('limit_days'): + try: + limit_days = int(request.args['limit_days']) + except ValueError: + errors = {'limit_days': ['{} is not an integer'.format(request.args['limit_days'])]} + raise InvalidRequest(errors, status_code=400) + else: + limit_days = None + statuses = _parse_statuses(request.args.get('statuses', '')) + page = int(request.args.get('page', 1)) + + pagination = dao_get_job_stats_for_service(service_id=service_id, + page=page, + page_size=current_app.config['PAGE_SIZE'], + limit_days=limit_days, + statuses=statuses) + return jsonify({ + 'data': [_serialize_job_stats(x) for x in pagination.items], + 'page_size': pagination.per_page, + 'total': pagination.total, + 'links': pagination_links( + pagination, + '.get_jobs_by_service', + service_id=service_id + ) + }) + + +def _parse_statuses(statuses): + return [x.strip() for x in statuses.split(',')] + + +def _serialize_job_stats(stat): + return { + "job_id": stat.job_id, + "original_file_name": stat.original_file_name, + "created_at": stat.created_at, + "scheduled_for": stat.scheduled_for, + "sent": stat.sent, + "delivered": stat.delivered, + "failed": stat.failed + } + + @job_blueprint.route('', methods=['POST']) def create_job(service_id): service = dao_fetch_service_by_id(service_id) diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index 476e6b1de..32508c628 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -428,26 +428,23 @@ def test_dao_get_job_statistics_for_job(notify_db, notify_db_session, sample_job create_or_update_job_sending_statistics(notification_failed) update_job_stats_outcome_count(notification_delivered) update_job_stats_outcome_count(notification_failed) - results = dao_get_job_statistics_for_job(sample_job.id) - assert results.job_id == sample_job.id - assert results.sent == 3 - assert results.delivered == 1 - assert results.failed == 1 + result = dao_get_job_statistics_for_job(sample_job.service_id, sample_job.id) + assert_job_stat(job=sample_job, result=result, sent=3, delivered=1, failed=1, with_template=True) def test_dao_get_job_statistics_for_job(notify_db, notify_db_session, sample_service): job_1, job_2 = stats_set_up(notify_db, notify_db_session, sample_service) - results = dao_get_job_statistics_for_job(job_1.id) - assert_job_stat(job_1, results, 2, 1, 0) + result = dao_get_job_statistics_for_job(sample_service.id, job_1.id) + assert_job_stat(job=job_1, result=result, sent=2, delivered=1, failed=0, with_template=True) - results_2 = dao_get_job_statistics_for_job(job_2.id) - assert_job_stat(job_2, results_2, 1, 0, 1) + result_2 = dao_get_job_statistics_for_job(sample_service.id, job_2.id) + assert_job_stat(job=job_2, result=result_2, sent=1, delivered=0, failed=1, with_template=True) def test_dao_get_job_stats_for_service(notify_db, notify_db_session, sample_service): job_1, job_2 = stats_set_up(notify_db, notify_db_session, sample_service) - results = dao_get_job_stats_for_service(sample_service.id) + results = dao_get_job_stats_for_service(sample_service.id).items assert len(results) == 2 assert_job_stat(job_2, results[0], 1, 0, 1) assert_job_stat(job_1, results[1], 2, 1, 0) @@ -459,12 +456,12 @@ def test_dao_get_job_stats_for_service_only_returns_stats_for_service(notify_db, service_name='Another Service') job_3, job_4 = stats_set_up(notify_db, notify_db_session, service=another_service) - results = dao_get_job_stats_for_service(sample_service.id) + results = dao_get_job_stats_for_service(sample_service.id).items assert len(results) == 2 assert_job_stat(job_2, results[0], 1, 0, 1) assert_job_stat(job_1, results[1], 2, 1, 0) - results = dao_get_job_stats_for_service(another_service.id) + results = dao_get_job_stats_for_service(another_service.id).items assert len(results) == 2 assert_job_stat(job_4, results[0], 1, 0, 1) assert_job_stat(job_3, results[1], 2, 1, 0) @@ -475,15 +472,15 @@ def test_dao_get_job_stats_for_service_only_returns_jobs_created_within_limited_ job_1, job_2 = stats_set_up(notify_db, notify_db_session, sample_service) results = dao_get_job_stats_for_service(sample_service.id, limit_days=1) - assert len(results) == 1 - assert_job_stat(job_2, results[0], 1, 0, 1) + assert results.total == 1 + assert_job_stat(job_2, results.items[0], 1, 0, 1) def test_dao_get_job_stats_for_service_only_returns_jobs_created_within_limited_days_inclusive( notify_db, notify_db_session, sample_service): job_1, job_2 = stats_set_up(notify_db, notify_db_session, sample_service) - results = dao_get_job_stats_for_service(sample_service.id, limit_days=2) + results = dao_get_job_stats_for_service(sample_service.id, limit_days=2).items assert len(results) == 2 assert_job_stat(job_2, results[0], 1, 0, 1) assert_job_stat(job_1, results[1], 2, 1, 0) @@ -493,28 +490,42 @@ def test_dao_get_job_stats_paginates_results( notify_db, notify_db_session, sample_service): job_1, job_2 = stats_set_up(notify_db, notify_db_session, sample_service) - results = dao_get_job_stats_for_service(sample_service.id, page=1, page_size=1) + results = dao_get_job_stats_for_service(sample_service.id, page=1, page_size=1).items assert len(results) == 1 assert_job_stat(job_2, results[0], 1, 0, 1) - results_2 = dao_get_job_stats_for_service(sample_service.id, page=2, page_size=1) + results_2 = dao_get_job_stats_for_service(sample_service.id, page=2, page_size=1).items assert len(results_2) == 1 assert_job_stat(job_1, results_2[0], 2, 1, 0) -def assert_job_stat(job, result, sent, delivered, failed): +def test_dao_get_job_returns_jobs_for_status( + notify_db, notify_db_session, sample_service): + stats_set_up(notify_db, notify_db_session, sample_service) + + results = dao_get_job_stats_for_service(sample_service.id, statuses=['pending']) + assert results.total == 1 + results_2 = dao_get_job_stats_for_service(sample_service.id, statuses=['pending', 'finished']) + assert results_2.total == 2 + + +def assert_job_stat(job, result, sent, delivered, failed, with_template=False): assert result.job_id == job.id assert result.original_file_name == job.original_file_name assert result.created_at == job.created_at + assert result.scheduled_for == job.scheduled_for assert result.sent == sent assert result.delivered == delivered assert result.failed == failed + if with_template: + assert result.template_id == job.template_id + assert result.template_version == job.template_version def stats_set_up(notify_db, notify_db_session, service): job_1 = create_job(notify_db=notify_db, notify_db_session=notify_db_session, service=service, created_at=datetime.utcnow() - timedelta(days=2)) job_2 = create_job(notify_db=notify_db, notify_db_session=notify_db_session, - service=service, original_file_name='Another job') + service=service, original_file_name='Another job', job_status='finished') notification = create_notification(notify_db=notify_db, notify_db_session=notify_db_session, job=job_1) notification_delivered = create_notification(notify_db=notify_db, notify_db_session=notify_db_session, job=job_1, status='delivered') diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index 6ebcb2e89..37fbe02dd 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -762,3 +762,46 @@ def test_get_all_notifications_for_job_returns_csv_format( notification = resp['notifications'][0] assert set(notification.keys()) == \ set(['created_at', 'template_type', 'template_name', 'job_name', 'status', 'row_number', 'recipient']) + + +# New endpoint to get job statistics the old tests will be refactored away. +def test_get_jobs_for_service_new_endpoint(client, notify_db, notify_db_session, sample_template): + _setup_jobs(notify_db, notify_db_session, sample_template) + + service_id = sample_template.service.id + + path = '/service/{}/job/job-stats'.format(service_id) + auth_header = create_authorization_header() + response = client.get(path, headers=[auth_header]) + assert response.status_code == 200 + resp_json = json.loads(response.get_data(as_text=True)) + assert len(resp_json['data']) == 5 + assert resp_json['data'][0]["job_id"] + assert resp_json['data'][0]["created_at"] + assert not resp_json['data'][0]["scheduled_for"] + assert resp_json['data'][0]["sent"] == 0 + assert resp_json['data'][0]["delivered"] == 0 + assert resp_json['data'][0]["failed"] == 0 + + +def test_get_jobs_raises_for_bad_limit_days(client, sample_service): + path = '/service/{}/job/job-stats'.format(sample_service.id) + auth_header = create_authorization_header() + response = client.get(path, + query_string={'limit_days': 'bad_number'}, + headers=[auth_header]) + assert response.status_code == 400 + assert response.get_data(as_text=True) == '{\n "message": {\n "limit_days": [\n ' \ + '"bad_number is not an integer"\n ]\n },\n "result": "error"\n}' + + +def test_parse_status_turns_comma_sep_strings_into_list(): + statuses = "started, finished, pending" + from app.job.rest import _parse_statuses + assert _parse_statuses(statuses) == ["started", "finished", "pending"] + + +def test_parse_status_turns_empty_string_into_empty_list(): + statuses = "" + from app.job.rest import _parse_statuses + assert _parse_statuses(statuses) == [''] From 4fefec6aa34bb8528e78006f907b39fcfeb00fb7 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 13 Jun 2017 10:56:03 +0100 Subject: [PATCH 41/46] New endpoints to return job stats. Next step is to use the new endpoints in admin. --- app/dao/jobs_dao.py | 8 +++++++ app/job/rest.py | 18 ++++++++++++-- tests/app/dao/test_jobs_dao.py | 16 +++++++------ tests/app/job/test_rest.py | 44 ++++++++++++++++++++++++++++++++++ 4 files changed, 77 insertions(+), 9 deletions(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index 9d791b8ad..236ddcab8 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -159,6 +159,9 @@ def dao_get_job_statistics_for_job(service_id, job_id): Job.scheduled_for, Job.template_id, Job.template_version, + Job.job_status, + Job.service_id, + Job.notification_count, JobStatistics.sent, JobStatistics.delivered, JobStatistics.failed @@ -179,6 +182,11 @@ def dao_get_job_stats_for_service(service_id, page=1, page_size=50, limit_days=N Job.original_file_name, Job.created_at, Job.scheduled_for, + Job.template_id, + Job.template_version, + Job.job_status, + Job.service_id, + Job.notification_count, JobStatistics.sent, JobStatistics.delivered, JobStatistics.failed diff --git a/app/job/rest.py b/app/job/rest.py index 5e5bc33c1..6a8a86ee4 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -5,6 +5,7 @@ from flask import ( current_app ) +from app import DATETIME_FORMAT from app.dao.jobs_dao import ( dao_create_job, dao_update_job, @@ -12,7 +13,8 @@ from app.dao.jobs_dao import ( dao_get_jobs_by_service_id, dao_get_future_scheduled_job_by_id_and_service_id, dao_get_notification_outcomes_for_job, - dao_get_job_stats_for_service) + dao_get_job_stats_for_service, + dao_get_job_statistics_for_job) from app.dao.services_dao import ( dao_fetch_service_by_id @@ -57,6 +59,13 @@ def get_job_by_service_and_job_id(service_id, job_id): return jsonify(data=data) +@job_blueprint.route('/job-stats/', methods=['GET']) +def get_job_stats_by_service_and_job_id(service_id, job_id): + statistic = dao_get_job_statistics_for_job(service_id=service_id, job_id=job_id) + + return jsonify(_serialize_job_stats(statistic)) + + @job_blueprint.route('//cancel', methods=['POST']) def cancel_job(service_id, job_id): job = dao_get_future_scheduled_job_by_id_and_service_id(job_id, service_id) @@ -155,8 +164,13 @@ def _serialize_job_stats(stat): return { "job_id": stat.job_id, "original_file_name": stat.original_file_name, - "created_at": stat.created_at, + "created_at": stat.created_at.strftime(DATETIME_FORMAT), "scheduled_for": stat.scheduled_for, + "template_id": stat.template_id, + "template_version": stat.template_version, + "job_status": stat.job_status, + "service_id": stat.service_id, + "requested": stat.notification_count, "sent": stat.sent, "delivered": stat.delivered, "failed": stat.failed diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index 32508c628..a4517aee5 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -429,16 +429,16 @@ def test_dao_get_job_statistics_for_job(notify_db, notify_db_session, sample_job update_job_stats_outcome_count(notification_delivered) update_job_stats_outcome_count(notification_failed) result = dao_get_job_statistics_for_job(sample_job.service_id, sample_job.id) - assert_job_stat(job=sample_job, result=result, sent=3, delivered=1, failed=1, with_template=True) + assert_job_stat(job=sample_job, result=result, sent=3, delivered=1, failed=1) def test_dao_get_job_statistics_for_job(notify_db, notify_db_session, sample_service): job_1, job_2 = stats_set_up(notify_db, notify_db_session, sample_service) result = dao_get_job_statistics_for_job(sample_service.id, job_1.id) - assert_job_stat(job=job_1, result=result, sent=2, delivered=1, failed=0, with_template=True) + assert_job_stat(job=job_1, result=result, sent=2, delivered=1, failed=0) result_2 = dao_get_job_statistics_for_job(sample_service.id, job_2.id) - assert_job_stat(job=job_2, result=result_2, sent=1, delivered=0, failed=1, with_template=True) + assert_job_stat(job=job_2, result=result_2, sent=1, delivered=0, failed=1) def test_dao_get_job_stats_for_service(notify_db, notify_db_session, sample_service): @@ -508,17 +508,19 @@ def test_dao_get_job_returns_jobs_for_status( assert results_2.total == 2 -def assert_job_stat(job, result, sent, delivered, failed, with_template=False): +def assert_job_stat(job, result, sent, delivered, failed): assert result.job_id == job.id assert result.original_file_name == job.original_file_name assert result.created_at == job.created_at assert result.scheduled_for == job.scheduled_for + assert result.template_id == job.template_id + assert result.template_version == job.template_version + assert result.job_status == job.job_status + assert result.service_id == job.service_id + assert result.notification_count == job.notification_count assert result.sent == sent assert result.delivered == delivered assert result.failed == failed - if with_template: - assert result.template_id == job.template_id - assert result.template_version == job.template_version def stats_set_up(notify_db, notify_db_session, service): diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index 37fbe02dd..b7ba7689d 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -6,6 +6,7 @@ from freezegun import freeze_time import pytest import pytz import app.celery.tasks +from app import DATETIME_FORMAT from tests import create_authorization_header from tests.conftest import set_config @@ -779,6 +780,10 @@ def test_get_jobs_for_service_new_endpoint(client, notify_db, notify_db_session, assert resp_json['data'][0]["job_id"] assert resp_json['data'][0]["created_at"] assert not resp_json['data'][0]["scheduled_for"] + assert resp_json['data'][0]["template_id"] + assert resp_json['data'][0]["template_version"] + assert resp_json['data'][0]["service_id"] + assert resp_json['data'][0]["requested"] assert resp_json['data'][0]["sent"] == 0 assert resp_json['data'][0]["delivered"] == 0 assert resp_json['data'][0]["failed"] == 0 @@ -805,3 +810,42 @@ def test_parse_status_turns_empty_string_into_empty_list(): statuses = "" from app.job.rest import _parse_statuses assert _parse_statuses(statuses) == [''] + + +def test_get_job_stats_by_service_id_and_job_id(client, sample_job): + auth_header = create_authorization_header() + response = client.get("/service/{}/job/job-stats/{}".format(sample_job.service_id, sample_job.id), + headers=[auth_header]) + assert response.status_code == 200 + resp_json = json.loads(response.get_data(as_text=True)) + assert resp_json["job_id"] == str(sample_job.id) + assert resp_json["created_at"] == sample_job.created_at.strftime(DATETIME_FORMAT) + assert not resp_json["scheduled_for"] + assert resp_json["template_id"] == str(sample_job.template_id) + assert resp_json["template_version"] == sample_job.template_version + assert resp_json["service_id"] == str(sample_job.service_id) + assert resp_json["requested"] == sample_job.notification_count + assert resp_json["sent"] == 0 + assert resp_json["delivered"] == 0 + assert resp_json["failed"] == 0 + + +def test_get_job_stats_with_invalid_job_id_returns404(client, sample_template): + service_id = sample_template.service.id + path = '/service/{}/job/job-=stats{}'.format(service_id, "bad-id") + auth_header = create_authorization_header() + response = client.get(path, headers=[auth_header]) + assert response.status_code == 404 + resp_json = json.loads(response.get_data(as_text=True)) + assert resp_json['result'] == 'error' + assert resp_json['message'] == 'No result found' + + +def test_get_job_stats_with_invalid_service_id_returns404(client, sample_job): + path = '/service/{}/job/job-=stats{}'.format(uuid.uuid4(), sample_job.id) + auth_header = create_authorization_header() + response = client.get(path, headers=[auth_header]) + assert response.status_code == 404 + resp_json = json.loads(response.get_data(as_text=True)) + assert resp_json['result'] == 'error' + assert resp_json['message'] == 'No result found' From a83e744c684ef91d69f7e59e1b63b535821de8df Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 13 Jun 2017 11:55:14 +0100 Subject: [PATCH 42/46] Remove join to Template table. Fix typo in unit test. --- app/dao/jobs_dao.py | 7 ++----- tests/app/job/test_rest.py | 10 +++++----- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index 236ddcab8..748b7cbaa 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -149,9 +149,8 @@ def dao_get_job_statistics_for_job(service_id, job_id): query = Job.query.join( JobStatistics, Job.id == JobStatistics.job_id ).filter( - Job.id == job_id - ).join( - Template, Template.id == Job.template_id and Template.version == Job.template_version + Job.id == job_id, + Job.service_id == service_id ).add_columns( JobStatistics.job_id, Job.original_file_name, @@ -165,8 +164,6 @@ def dao_get_job_statistics_for_job(service_id, job_id): JobStatistics.sent, JobStatistics.delivered, JobStatistics.failed - ).filter( - Job.service_id == service_id ) return query.one() diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index b7ba7689d..0ed3a533b 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -796,8 +796,9 @@ def test_get_jobs_raises_for_bad_limit_days(client, sample_service): query_string={'limit_days': 'bad_number'}, headers=[auth_header]) assert response.status_code == 400 - assert response.get_data(as_text=True) == '{\n "message": {\n "limit_days": [\n ' \ - '"bad_number is not an integer"\n ]\n },\n "result": "error"\n}' + resp_json = json.loads(response.get_data(as_text=True)) + assert resp_json["result"] == "error" + assert resp_json["message"] == {'limit_days': ['bad_number is not an integer']} def test_parse_status_turns_comma_sep_strings_into_list(): @@ -831,8 +832,7 @@ def test_get_job_stats_by_service_id_and_job_id(client, sample_job): def test_get_job_stats_with_invalid_job_id_returns404(client, sample_template): - service_id = sample_template.service.id - path = '/service/{}/job/job-=stats{}'.format(service_id, "bad-id") + path = '/service/{}/job/job-stats{}'.format(sample_template.service.id, uuid.uuid4()) auth_header = create_authorization_header() response = client.get(path, headers=[auth_header]) assert response.status_code == 404 @@ -842,7 +842,7 @@ def test_get_job_stats_with_invalid_job_id_returns404(client, sample_template): def test_get_job_stats_with_invalid_service_id_returns404(client, sample_job): - path = '/service/{}/job/job-=stats{}'.format(uuid.uuid4(), sample_job.id) + path = '/service/{}/job/job-stats{}'.format(uuid.uuid4(), sample_job.id) auth_header = create_authorization_header() response = client.get(path, headers=[auth_header]) assert response.status_code == 404 From b65ee1d99a9bfead1046be44259400af390e1de9 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 13 Jun 2017 12:08:09 +0100 Subject: [PATCH 43/46] Fix missing slash --- tests/app/job/test_rest.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index 0ed3a533b..b41dfe065 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -832,7 +832,7 @@ def test_get_job_stats_by_service_id_and_job_id(client, sample_job): def test_get_job_stats_with_invalid_job_id_returns404(client, sample_template): - path = '/service/{}/job/job-stats{}'.format(sample_template.service.id, uuid.uuid4()) + path = '/service/{}/job/job-stats/{}'.format(sample_template.service.id, uuid.uuid4()) auth_header = create_authorization_header() response = client.get(path, headers=[auth_header]) assert response.status_code == 404 @@ -842,7 +842,7 @@ def test_get_job_stats_with_invalid_job_id_returns404(client, sample_template): def test_get_job_stats_with_invalid_service_id_returns404(client, sample_job): - path = '/service/{}/job/job-stats{}'.format(uuid.uuid4(), sample_job.id) + path = '/service/{}/job/job-stats/{}'.format(uuid.uuid4(), sample_job.id) auth_header = create_authorization_header() response = client.get(path, headers=[auth_header]) assert response.status_code == 404 From a1a5377f9c1e88b38481d8d4c3c581039bfb2bf8 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 12 Jun 2017 15:55:05 +0100 Subject: [PATCH 44/46] Add methods to get and remove s3 bucket objects --- app/aws/s3.py | 47 +++++++++++++++++- tests/app/aws/test_s3.py | 103 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 147 insertions(+), 3 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index f5798314b..48a54e709 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -1,6 +1,10 @@ -from boto3 import resource +from datetime import datetime, timedelta + from flask import current_app +import pytz +from boto3 import client, resource + FILE_LOCATION_STRUCTURE = 'service-{}-notify/{}.csv' @@ -24,7 +28,46 @@ def get_job_from_s3(service_id, job_id): def remove_job_from_s3(service_id, job_id): bucket_name = current_app.config['CSV_UPLOAD_BUCKET_NAME'] file_location = FILE_LOCATION_STRUCTURE.format(service_id, job_id) - obj = get_s3_object(bucket_name, file_location) + return remove_s3_object(bucket_name, file_location) + + +def get_s3_bucket_objects(bucket_name, subfolder='', older_than=7, limit_days=2): + boto_client = client('s3', current_app.config['AWS_REGION']) + paginator = boto_client.get_paginator('list_objects_v2') + page_iterator = paginator.paginate( + Bucket=bucket_name, + Prefix=subfolder + ) + + all_objects_in_bucket = [] + for page in page_iterator: + all_objects_in_bucket.extend(page['Contents']) + + return all_objects_in_bucket + + +def filter_s3_bucket_objects_within_date_range(bucket_objects, older_than=7, limit_days=2): + """ + S3 returns the Object['LastModified'] as an 'offset-aware' timestamp so the + date range filter must take this into account. + + Additionally an additional Object is returned by S3 corresponding to the + container directory. This is redundant and should be removed. + + """ + end_date = datetime.now(tz=pytz.utc) - timedelta(days=older_than) + start_date = end_date - timedelta(days=limit_days) + filtered_items = [item for item in bucket_objects if all([ + not item['Key'].endswith('/'), + item['LastModified'] > start_date, + item['LastModified'] < end_date + ])] + + return filtered_items + + +def remove_s3_object(bucket_name, object_key): + obj = get_s3_object(bucket_name, object_key) return obj.delete() diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py index fd30c16df..7312ad549 100644 --- a/tests/app/aws/test_s3.py +++ b/tests/app/aws/test_s3.py @@ -1,8 +1,29 @@ from unittest.mock import call +from datetime import datetime, timedelta from flask import current_app -from app.aws.s3 import get_s3_file, remove_transformed_dvla_file +from freezegun import freeze_time +import pytz + +from app.aws.s3 import ( + get_s3_bucket_objects, + get_s3_file, + filter_s3_bucket_objects_within_date_range, + remove_transformed_dvla_file +) + + +def datetime_in_past(days=0, seconds=0): + return datetime.now(tz=pytz.utc) - timedelta(days=days, seconds=seconds) + + +def single_s3_object_stub(key='foo', last_modified=datetime.utcnow()): + return { + 'ETag': '"d41d8cd98f00b204e9800998ecf8427e"', + 'Key': key, + 'LastModified': last_modified + } def test_get_s3_file_makes_correct_call(notify_api, mocker): @@ -25,3 +46,83 @@ def test_remove_transformed_dvla_file_makes_correct_call(notify_api, mocker): call(current_app.config['DVLA_UPLOAD_BUCKET_NAME'], '{}-dvla-job.text'.format(fake_uuid)), call().delete() ]) + + +def test_get_s3_bucket_objects_make_correct_pagination_call(notify_api, mocker): + paginator_mock = mocker.patch('app.aws.s3.client') + + get_s3_bucket_objects('foo-bucket', subfolder='bar') + + paginator_mock.assert_has_calls([ + call().get_paginator().paginate(Bucket='foo-bucket', Prefix='bar') + ]) + + +def test_get_s3_bucket_objects_builds_objects_list_from_paginator(notify_api, mocker): + paginator_mock = mocker.patch('app.aws.s3.client') + multiple_pages_s3_object = [ + { + "Contents": [ + single_s3_object_stub('bar/foo.txt', datetime_in_past(days=8)), + ] + }, + { + "Contents": [ + single_s3_object_stub('bar/foo1.txt', datetime_in_past(days=8)), + ] + } + ] + paginator_mock.return_value.get_paginator.return_value.paginate.return_value = multiple_pages_s3_object + + bucket_objects = get_s3_bucket_objects('foo-bucket', subfolder='bar') + + assert len(bucket_objects) == 2 + assert set(bucket_objects[0].keys()) == set(['ETag', 'Key', 'LastModified']) + + +@freeze_time("2016-01-01 11:00:00") +def test_get_s3_bucket_objects_removes_redundant_root_object(notify_api, mocker): + s3_objects_stub = [ + single_s3_object_stub('bar/', datetime_in_past(days=8)), + single_s3_object_stub('bar/foo.txt', datetime_in_past(days=8)), + ] + + filtered_items = filter_s3_bucket_objects_within_date_range(s3_objects_stub) + + assert len(filtered_items) == 1 + + assert filtered_items[0]["Key"] == 'bar/foo.txt' + assert filtered_items[0]["LastModified"] == datetime_in_past(days=8) + + +@freeze_time("2016-01-01 11:00:00") +def test_filter_s3_bucket_objects_within_date_range_filters_by_date_range(notify_api, mocker): + s3_objects_stub = [ + single_s3_object_stub('bar/', datetime_in_past(days=8)), + single_s3_object_stub('bar/foo.txt', datetime_in_past(days=8)), + single_s3_object_stub('bar/foo1.txt', datetime_in_past(days=8)), + ] + + filtered_items = filter_s3_bucket_objects_within_date_range(s3_objects_stub) + + assert len(filtered_items) == 2 + + assert filtered_items[0]["Key"] == 'bar/foo.txt' + assert filtered_items[0]["LastModified"] == datetime_in_past(days=8) + + assert filtered_items[1]["Key"] == 'bar/foo1.txt' + assert filtered_items[1]["LastModified"] == datetime_in_past(days=8) + + +@freeze_time("2016-01-01 11:00:00") +def test_get_s3_bucket_objects_does_not_return_outside_of_date_range(notify_api, mocker): + s3_objects_stub = [ + single_s3_object_stub('bar/', datetime_in_past(days=7)), + single_s3_object_stub('bar/foo.txt', datetime_in_past(days=7)), + single_s3_object_stub('bar/foo2.txt', datetime_in_past(days=9)), + single_s3_object_stub('bar/foo2.txt', datetime_in_past(days=9, seconds=1)), + ] + + filtered_items = filter_s3_bucket_objects_within_date_range(s3_objects_stub) + + assert len(filtered_items) == 0 From f29ebb34f6f70c05ca60350dfeb7c1156d811711 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 12 Jun 2017 15:55:42 +0100 Subject: [PATCH 45/46] Add scheduled task to delete response files older than 7 days --- app/celery/scheduled_tasks.py | 26 ++++++++++++++ app/config.py | 14 +++++++- tests/app/celery/test_scheduled_tasks.py | 46 ++++++++++++++++++++++-- 3 files changed, 83 insertions(+), 3 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 11e4de49d..e86a7c113 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -255,3 +255,29 @@ def remove_transformed_dvla_files(): for job in jobs: s3.remove_transformed_dvla_file(job.id) current_app.logger.info("Transformed dvla file for job {} has been removed from s3.".format(job.id)) + + +@notify_celery.task(name="delete_dvla_response_files") +@statsd(namespace="tasks") +def delete_dvla_response_files_older_than_seven_days(): + try: + start = datetime.utcnow() + bucket_objects = s3.get_s3_bucket_objects( + current_app.config['DVLA_RESPONSE_BUCKET_NAME'], + 'root/dispatch' + ) + older_than_seven_days = s3.filter_s3_bucket_objects_within_date_range(bucket_objects) + + for f in older_than_seven_days: + s3.remove_s3_object(current_app.config['DVLA_RESPONSE_BUCKET_NAME'], f['Key']) + + current_app.logger.info( + "Delete dvla response files started {} finished {} deleted {} files".format( + start, + datetime.utcnow(), + len(older_than_seven_days) + ) + ) + except SQLAlchemyError as e: + current_app.logger.exception("Failed to delete dvla response files") + raise diff --git a/app/config.py b/app/config.py index f470666a7..b3d76dc93 100644 --- a/app/config.py +++ b/app/config.py @@ -1,7 +1,8 @@ from datetime import timedelta +import os + from celery.schedules import crontab from kombu import Exchange, Queue -import os from app.models import ( EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, @@ -209,6 +210,11 @@ class Config(object): 'schedule': crontab(minute=40, hour=4), 'options': {'queue': QueueNames.PERIODIC} }, + 'delete_dvla_response_files': { + 'task': 'delete_dvla_response_files', + 'schedule': crontab(minute=10, hour=5), + 'options': {'queue': QueueNames.PERIODIC} + }, 'timeout-job-statistics': { 'task': 'timeout-job-statistics', 'schedule': crontab(minute=0, hour=5), @@ -265,6 +271,7 @@ class Development(Config): SQLALCHEMY_ECHO = False NOTIFY_EMAIL_DOMAIN = 'notify.tools' CSV_UPLOAD_BUCKET_NAME = 'development-notifications-csv-upload' + DVLA_RESPONSE_BUCKET_NAME = 'notify.tools-ftp' NOTIFY_ENVIRONMENT = 'development' NOTIFICATION_QUEUE_PREFIX = 'development' DEBUG = True @@ -284,6 +291,7 @@ class Test(Config): NOTIFY_ENVIRONMENT = 'test' DEBUG = True CSV_UPLOAD_BUCKET_NAME = 'test-notifications-csv-upload' + DVLA_RESPONSE_BUCKET_NAME = 'test.notify.com-ftp' STATSD_ENABLED = True STATSD_HOST = "localhost" STATSD_PORT = 1000 @@ -316,6 +324,7 @@ class Preview(Config): NOTIFY_EMAIL_DOMAIN = 'notify.works' NOTIFY_ENVIRONMENT = 'preview' CSV_UPLOAD_BUCKET_NAME = 'preview-notifications-csv-upload' + DVLA_RESPONSE_BUCKET_NAME = 'notify.works-ftp' FROM_NUMBER = 'preview' API_RATE_LIMIT_ENABLED = True @@ -324,6 +333,7 @@ class Staging(Config): NOTIFY_EMAIL_DOMAIN = 'staging-notify.works' NOTIFY_ENVIRONMENT = 'staging' CSV_UPLOAD_BUCKET_NAME = 'staging-notify-csv-upload' + DVLA_RESPONSE_BUCKET_NAME = 'staging-notify.works-ftp' STATSD_ENABLED = True FROM_NUMBER = 'stage' API_RATE_LIMIT_ENABLED = True @@ -333,6 +343,7 @@ class Live(Config): NOTIFY_EMAIL_DOMAIN = 'notifications.service.gov.uk' NOTIFY_ENVIRONMENT = 'live' CSV_UPLOAD_BUCKET_NAME = 'live-notifications-csv-upload' + DVLA_RESPONSE_BUCKET_NAME = 'notifications.service.gov.uk-ftp' STATSD_ENABLED = True FROM_NUMBER = 'GOVUK' FUNCTIONAL_TEST_PROVIDER_SERVICE_ID = '6c1d81bb-dae2-4ee9-80b0-89a4aae9f649' @@ -350,6 +361,7 @@ class Sandbox(CloudFoundryConfig): NOTIFY_EMAIL_DOMAIN = 'notify.works' NOTIFY_ENVIRONMENT = 'sandbox' CSV_UPLOAD_BUCKET_NAME = 'cf-sandbox-notifications-csv-upload' + DVLA_RESPONSE_BUCKET_NAME = 'notify.works-ftp' FROM_NUMBER = 'sandbox' REDIS_ENABLED = False diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index fcdd8ea93..fb1a3981e 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -9,6 +9,7 @@ from freezegun import freeze_time from app.celery import scheduled_tasks from app.celery.scheduled_tasks import ( + delete_dvla_response_files_older_than_seven_days, delete_email_notifications_older_than_seven_days, delete_inbound_sms_older_than_seven_days, delete_invitations, @@ -42,8 +43,10 @@ from tests.app.db import create_notification, create_service, create_template, c from tests.app.conftest import ( sample_job as create_sample_job, sample_notification_history as create_notification_history, - create_custom_template) -from tests.conftest import set_config_values + create_custom_template, + set_config_values +) +from tests.app.aws.test_s3 import single_s3_object_stub, datetime_in_past def _create_slow_delivery_notification(provider='mmg'): @@ -93,6 +96,8 @@ def test_should_have_decorated_tasks_functions(): 'delete_inbound_sms_older_than_seven_days' assert remove_transformed_dvla_files.__wrapped__.__name__ == \ 'remove_transformed_dvla_files' + assert delete_dvla_response_files_older_than_seven_days.__wrapped__.__name__ == \ + 'delete_dvla_response_files_older_than_seven_days' def test_should_call_delete_sms_notifications_more_than_week_in_task(notify_api, mocker): @@ -548,3 +553,40 @@ def test_remove_dvla_transformed_files_does_not_remove_files(mocker, sample_serv remove_transformed_dvla_files() s3.remove_transformed_dvla_file.assert_has_calls([]) + + +def test_delete_dvla_response_files_older_than_seven_days_removes_old_files(notify_api, mocker): + single_page_s3_objects = [{ + "Contents": [ + single_s3_object_stub('bar/foo1.txt', datetime_in_past(days=8)), + single_s3_object_stub('bar/foo2.txt', datetime_in_past(days=8)), + ] + }] + mocker.patch( + 'app.celery.scheduled_tasks.s3.get_s3_bucket_objects', return_value=single_page_s3_objects[0]["Contents"] + ) + remove_s3_mock = mocker.patch('app.celery.scheduled_tasks.s3.remove_s3_object') + + delete_dvla_response_files_older_than_seven_days() + + remove_s3_mock.assert_has_calls([ + call(current_app.config['DVLA_RESPONSE_BUCKET_NAME'], single_page_s3_objects[0]["Contents"][0]["Key"]), + call(current_app.config['DVLA_RESPONSE_BUCKET_NAME'], single_page_s3_objects[0]["Contents"][1]["Key"]) + ]) + + +def test_delete_dvla_response_files_older_than_seven_days_does_not_remove_files(notify_api, mocker): + single_page_s3_objects = [{ + "Contents": [ + single_s3_object_stub('bar/foo1.txt', datetime_in_past(days=6)), + single_s3_object_stub('bar/foo2.txt', datetime_in_past(days=9)), + ] + }] + mocker.patch( + 'app.celery.scheduled_tasks.s3.get_s3_bucket_objects', return_value=single_page_s3_objects[0]["Contents"] + ) + remove_s3_mock = mocker.patch('app.celery.scheduled_tasks.s3.remove_s3_object') + + delete_dvla_response_files_older_than_seven_days() + + remove_s3_mock.assert_not_called() From 1dacc6c3a378da5e210e443cf04ce2c60a83b984 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 13 Jun 2017 15:22:31 +0100 Subject: [PATCH 46/46] Updates: * Skip the decorated tests as that doesn't work as we expect * Remove magic numbers in tests --- tests/app/aws/test_s3.py | 53 +++++++++++++++--------- tests/app/celery/test_scheduled_tasks.py | 44 +++++++++++++------- tests/app/conftest.py | 13 ++++-- 3 files changed, 72 insertions(+), 38 deletions(-) diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py index 7312ad549..b44d66fc8 100644 --- a/tests/app/aws/test_s3.py +++ b/tests/app/aws/test_s3.py @@ -4,7 +4,6 @@ from datetime import datetime, timedelta from flask import current_app from freezegun import freeze_time -import pytz from app.aws.s3 import ( get_s3_bucket_objects, @@ -12,10 +11,7 @@ from app.aws.s3 import ( filter_s3_bucket_objects_within_date_range, remove_transformed_dvla_file ) - - -def datetime_in_past(days=0, seconds=0): - return datetime.now(tz=pytz.utc) - timedelta(days=days, seconds=seconds) +from tests.app.conftest import datetime_in_past def single_s3_object_stub(key='foo', last_modified=datetime.utcnow()): @@ -59,16 +55,17 @@ def test_get_s3_bucket_objects_make_correct_pagination_call(notify_api, mocker): def test_get_s3_bucket_objects_builds_objects_list_from_paginator(notify_api, mocker): + AFTER_SEVEN_DAYS = datetime_in_past(days=8) paginator_mock = mocker.patch('app.aws.s3.client') multiple_pages_s3_object = [ { "Contents": [ - single_s3_object_stub('bar/foo.txt', datetime_in_past(days=8)), + single_s3_object_stub('bar/foo.txt', AFTER_SEVEN_DAYS), ] }, { "Contents": [ - single_s3_object_stub('bar/foo1.txt', datetime_in_past(days=8)), + single_s3_object_stub('bar/foo1.txt', AFTER_SEVEN_DAYS), ] } ] @@ -82,9 +79,10 @@ def test_get_s3_bucket_objects_builds_objects_list_from_paginator(notify_api, mo @freeze_time("2016-01-01 11:00:00") def test_get_s3_bucket_objects_removes_redundant_root_object(notify_api, mocker): + AFTER_SEVEN_DAYS = datetime_in_past(days=8) s3_objects_stub = [ - single_s3_object_stub('bar/', datetime_in_past(days=8)), - single_s3_object_stub('bar/foo.txt', datetime_in_past(days=8)), + single_s3_object_stub('bar/', AFTER_SEVEN_DAYS), + single_s3_object_stub('bar/foo.txt', AFTER_SEVEN_DAYS), ] filtered_items = filter_s3_bucket_objects_within_date_range(s3_objects_stub) @@ -97,30 +95,45 @@ def test_get_s3_bucket_objects_removes_redundant_root_object(notify_api, mocker) @freeze_time("2016-01-01 11:00:00") def test_filter_s3_bucket_objects_within_date_range_filters_by_date_range(notify_api, mocker): + START_DATE = datetime_in_past(days=9) + JUST_BEFORE_START_DATE = START_DATE - timedelta(seconds=1) + JUST_AFTER_START_DATE = START_DATE + timedelta(seconds=1) + END_DATE = datetime_in_past(days=7) + JUST_BEFORE_END_DATE = END_DATE - timedelta(seconds=1) + JUST_AFTER_END_DATE = END_DATE + timedelta(seconds=1) + s3_objects_stub = [ - single_s3_object_stub('bar/', datetime_in_past(days=8)), - single_s3_object_stub('bar/foo.txt', datetime_in_past(days=8)), - single_s3_object_stub('bar/foo1.txt', datetime_in_past(days=8)), + single_s3_object_stub('bar/', JUST_BEFORE_START_DATE), + single_s3_object_stub('bar/foo.txt', START_DATE), + single_s3_object_stub('bar/foo2.txt', JUST_AFTER_START_DATE), + single_s3_object_stub('bar/foo3.txt', JUST_BEFORE_END_DATE), + single_s3_object_stub('bar/foo4.txt', END_DATE), + single_s3_object_stub('bar/foo5.txt', JUST_AFTER_END_DATE), ] filtered_items = filter_s3_bucket_objects_within_date_range(s3_objects_stub) assert len(filtered_items) == 2 - assert filtered_items[0]["Key"] == 'bar/foo.txt' - assert filtered_items[0]["LastModified"] == datetime_in_past(days=8) + assert filtered_items[0]["Key"] == 'bar/foo2.txt' + assert filtered_items[0]["LastModified"] == JUST_AFTER_START_DATE - assert filtered_items[1]["Key"] == 'bar/foo1.txt' - assert filtered_items[1]["LastModified"] == datetime_in_past(days=8) + assert filtered_items[1]["Key"] == 'bar/foo3.txt' + assert filtered_items[1]["LastModified"] == JUST_BEFORE_END_DATE @freeze_time("2016-01-01 11:00:00") def test_get_s3_bucket_objects_does_not_return_outside_of_date_range(notify_api, mocker): + START_DATE = datetime_in_past(days=9) + JUST_BEFORE_START_DATE = START_DATE - timedelta(seconds=1) + END_DATE = datetime_in_past(days=7) + JUST_AFTER_END_DATE = END_DATE + timedelta(seconds=1) + s3_objects_stub = [ - single_s3_object_stub('bar/', datetime_in_past(days=7)), - single_s3_object_stub('bar/foo.txt', datetime_in_past(days=7)), - single_s3_object_stub('bar/foo2.txt', datetime_in_past(days=9)), - single_s3_object_stub('bar/foo2.txt', datetime_in_past(days=9, seconds=1)), + single_s3_object_stub('bar/', JUST_BEFORE_START_DATE), + single_s3_object_stub('bar/foo1.txt', START_DATE), + single_s3_object_stub('bar/foo2.txt', END_DATE), + single_s3_object_stub('bar/foo3.txt', JUST_AFTER_END_DATE) ] filtered_items = filter_s3_bucket_objects_within_date_range(s3_objects_stub) diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index fb1a3981e..47a4ed9ff 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -44,9 +44,10 @@ from tests.app.conftest import ( sample_job as create_sample_job, sample_notification_history as create_notification_history, create_custom_template, - set_config_values + datetime_in_past ) -from tests.app.aws.test_s3 import single_s3_object_stub, datetime_in_past +from tests.app.aws.test_s3 import single_s3_object_stub +from tests.conftest import set_config_values def _create_slow_delivery_notification(provider='mmg'): @@ -74,14 +75,13 @@ def _create_slow_delivery_notification(provider='mmg'): ) -@pytest.fixture(scope='function') -def prepare_current_provider(restore_provider_details): - initial_provider = get_current_provider('sms') - initial_provider.updated_at = datetime.utcnow() - timedelta(minutes=30) - dao_update_provider_details(initial_provider) - - +@pytest.mark.skip(reason="This doesn't actually test the celery task wraps the function") def test_should_have_decorated_tasks_functions(): + """ + TODO: This test needs to be reviewed as this doesn't actually + test that the celery task is wrapping the function. We're also + running similar tests elsewhere which also need review. + """ assert delete_verify_codes.__wrapped__.__name__ == 'delete_verify_codes' assert delete_notifications_created_more_than_a_week_ago_by_type.__wrapped__.__name__ == \ 'delete_notifications_created_more_than_a_week_ago_by_type' @@ -100,6 +100,13 @@ def test_should_have_decorated_tasks_functions(): 'delete_dvla_response_files_older_than_seven_days' +@pytest.fixture(scope='function') +def prepare_current_provider(restore_provider_details): + initial_provider = get_current_provider('sms') + initial_provider.updated_at = datetime.utcnow() - timedelta(minutes=30) + dao_update_provider_details(initial_provider) + + def test_should_call_delete_sms_notifications_more_than_week_in_task(notify_api, mocker): mocked = mocker.patch('app.celery.scheduled_tasks.delete_notifications_created_more_than_a_week_ago_by_type') delete_sms_notifications_older_than_seven_days() @@ -555,11 +562,13 @@ def test_remove_dvla_transformed_files_does_not_remove_files(mocker, sample_serv s3.remove_transformed_dvla_file.assert_has_calls([]) +@freeze_time("2016-01-01 11:00:00") def test_delete_dvla_response_files_older_than_seven_days_removes_old_files(notify_api, mocker): + AFTER_SEVEN_DAYS = datetime_in_past(days=8) single_page_s3_objects = [{ "Contents": [ - single_s3_object_stub('bar/foo1.txt', datetime_in_past(days=8)), - single_s3_object_stub('bar/foo2.txt', datetime_in_past(days=8)), + single_s3_object_stub('bar/foo1.txt', AFTER_SEVEN_DAYS), + single_s3_object_stub('bar/foo2.txt', AFTER_SEVEN_DAYS), ] }] mocker.patch( @@ -575,18 +584,25 @@ def test_delete_dvla_response_files_older_than_seven_days_removes_old_files(noti ]) +@freeze_time("2016-01-01 11:00:00") def test_delete_dvla_response_files_older_than_seven_days_does_not_remove_files(notify_api, mocker): + START_DATE = datetime_in_past(days=9) + JUST_BEFORE_START_DATE = datetime_in_past(days=9, seconds=1) + END_DATE = datetime_in_past(days=7) + JUST_AFTER_END_DATE = END_DATE + timedelta(seconds=1) + single_page_s3_objects = [{ "Contents": [ - single_s3_object_stub('bar/foo1.txt', datetime_in_past(days=6)), - single_s3_object_stub('bar/foo2.txt', datetime_in_past(days=9)), + single_s3_object_stub('bar/foo1.txt', JUST_BEFORE_START_DATE), + single_s3_object_stub('bar/foo2.txt', START_DATE), + single_s3_object_stub('bar/foo3.txt', END_DATE), + single_s3_object_stub('bar/foo4.txt', JUST_AFTER_END_DATE), ] }] mocker.patch( 'app.celery.scheduled_tasks.s3.get_s3_bucket_objects', return_value=single_page_s3_objects[0]["Contents"] ) remove_s3_mock = mocker.patch('app.celery.scheduled_tasks.s3.remove_s3_object') - delete_dvla_response_files_older_than_seven_days() remove_s3_mock.assert_not_called() diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 91eff00fc..f01fcae15 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -1,12 +1,14 @@ +from datetime import (datetime, date, timedelta) import json import uuid -from datetime import (datetime, date, timedelta) -import requests_mock +from flask import current_app, url_for + import pytest +import pytz +import requests_mock from sqlalchemy import asc from sqlalchemy.orm.session import make_transient -from flask import current_app, url_for from app import db from app.models import ( @@ -35,7 +37,6 @@ from app.dao.notifications_dao import dao_create_notification from app.dao.invited_user_dao import save_invited_user from app.dao.provider_rates_dao import create_provider_rates from app.clients.sms.firetext import FiretextClient - from tests import create_authorization_header from tests.app.db import create_user, create_template, create_notification @@ -1018,3 +1019,7 @@ def admin_request(client): return json_resp return AdminRequest + + +def datetime_in_past(days=0, seconds=0): + return datetime.now(tz=pytz.utc) - timedelta(days=days, seconds=seconds)