From ca9c886c3e6e00cd4d56cbbc8f0a6b670604a94f Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Thu, 31 Mar 2016 15:57:50 +0100 Subject: [PATCH 1/4] [WIP] On create of notification. Upsert record for template stats recording usages of template by day. --- app/celery/tasks.py | 50 ++++-- app/dao/notifications_dao.py | 33 +++- app/models.py | 20 ++- app/notifications/rest.py | 2 +- .../versions/0044_add_template_stats.py | 42 +++++ tests/app/dao/test_notification_dao.py | 170 +++++++++++++++++- 6 files changed, 289 insertions(+), 28 deletions(-) create mode 100644 migrations/versions/0044_add_template_stats.py diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 84b2a8f8d..fd8ccb3a9 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -1,9 +1,34 @@ -from app import create_uuid, DATETIME_FORMAT, DATE_FORMAT -from app import notify_celery, encryption, firetext_client, aws_ses_client +from datetime import datetime + +from flask import current_app +from sqlalchemy.exc import SQLAlchemyError + from app.clients.email.aws_ses import AwsSesClientException from app.clients.sms.firetext import FiretextClientException from app.dao.services_dao import dao_fetch_service_by_id from app.dao.templates_dao import dao_get_template_by_id + +from utils.template import Template + +from utils.recipients import ( + RecipientCSV, + validate_and_format_phone_number +) + +from app import ( + create_uuid, + DATETIME_FORMAT, + DATE_FORMAT, + notify_celery, + encryption, + firetext_client, + aws_ses_client +) + +from app.aws import s3 +from app.dao.users_dao import delete_codes_older_created_more_than_a_day_ago +from app.dao.invited_user_dao import delete_invitations_created_more_than_two_days_ago + from app.dao.notifications_dao import ( dao_create_notification, dao_update_notification, @@ -12,21 +37,22 @@ from app.dao.notifications_dao import ( dao_get_notification_statistics_for_service_and_day, update_notification_reference_by_id ) -from app.dao.jobs_dao import dao_update_job, dao_get_job_by_id -from app.dao.users_dao import delete_codes_older_created_more_than_a_day_ago -from app.dao.invited_user_dao import delete_invitations_created_more_than_two_days_ago + +from app.dao.jobs_dao import ( + dao_update_job, + dao_get_job_by_id +) + from app.models import ( Notification, TEMPLATE_TYPE_EMAIL, TEMPLATE_TYPE_SMS ) -from flask import current_app -from sqlalchemy.exc import SQLAlchemyError -from app.aws import s3 -from datetime import datetime -from utils.template import Template -from utils.recipients import RecipientCSV, validate_and_format_phone_number -from app.validation import (allowed_send_to_email, allowed_send_to_number) + +from app.validation import ( + allowed_send_to_email, + allowed_send_to_number +) @notify_celery.task(name="delete-verify-codes") diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 93162729d..e5787720f 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -1,15 +1,28 @@ +from sqlalchemy import desc +from datetime import ( + datetime, + timedelta, + date +) + from flask import current_app + from app import db from app.models import ( Notification, Job, NotificationStatistics, + TemplateStatistics, TEMPLATE_TYPE_SMS, TEMPLATE_TYPE_EMAIL, - Template) -from sqlalchemy import desc -from datetime import datetime, timedelta -from app.clients import (STATISTICS_FAILURE, STATISTICS_DELIVERED, STATISTICS_REQUESTED) + Template +) + +from app.clients import ( + STATISTICS_FAILURE, + STATISTICS_DELIVERED, + STATISTICS_REQUESTED +) def dao_get_notification_statistics_for_service(service_id): @@ -48,6 +61,18 @@ def dao_create_notification(notification, notification_type): emails_requested=1 if notification_type == TEMPLATE_TYPE_EMAIL else 0 ) db.session.add(stats) + + update_count = db.session.query(TemplateStatistics).filter_by( + day=date.today(), + service_id=notification.service_id, + template_id=notification.template_id + ).update({'usage_count': TemplateStatistics.usage_count + 1}) + + if update_count == 0: + template_stats = TemplateStatistics(template_id=notification.template_id, + service_id=notification.service_id) + db.session.add(template_stats) + db.session.add(notification) db.session.commit() except: diff --git a/app/models.py b/app/models.py index 0603fb1c3..333d62a20 100644 --- a/app/models.py +++ b/app/models.py @@ -1,9 +1,14 @@ import uuid +import datetime from sqlalchemy import UniqueConstraint, Sequence + from . import db -import datetime -from sqlalchemy.dialects.postgresql import (UUID, ARRAY) +from sqlalchemy.dialects.postgresql import ( + UUID, + ARRAY +) + from app.encryption import ( hashpw, check_hash @@ -346,3 +351,14 @@ class Permission(db.Model): __table_args__ = ( UniqueConstraint('service_id', 'user_id', 'permission', name='uix_service_user_permission'), ) + + +class TemplateStatistics(db.Model): + + id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) + service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, unique=False, nullable=False) + service = db.relationship('Service', backref=db.backref('template_statics', lazy='dynamic')) + template_id = db.Column(db.BigInteger, db.ForeignKey('templates.id'), index=True, nullable=False, unique=False) + template = db.relationship('Template') + usage_count = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=1) + day = db.Column(db.Date, index=True, nullable=False, unique=False, default=datetime.date.today) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 763d63ff6..2c1598410 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -20,11 +20,11 @@ from app.dao import ( services_dao, notifications_dao ) + from app.schemas import ( email_notification_schema, sms_template_notification_schema, notification_status_schema, - template_schema, notifications_filter_schema ) from app.celery.tasks import send_sms, send_email diff --git a/migrations/versions/0044_add_template_stats.py b/migrations/versions/0044_add_template_stats.py new file mode 100644 index 000000000..d85d8fd72 --- /dev/null +++ b/migrations/versions/0044_add_template_stats.py @@ -0,0 +1,42 @@ +"""empty message + +Revision ID: 0044_add_template_stats +Revises: 0043_add_view_activity +Create Date: 2016-03-31 12:05:19.630792 + +""" + +# revision identifiers, used by Alembic. +revision = '0044_add_template_stats' +down_revision = '0043_add_view_activity' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + + +def upgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.create_table('template_statistics', + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('service_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('template_id', sa.BigInteger(), nullable=False), + sa.Column('usage_count', sa.BigInteger(), nullable=False), + sa.Column('day', sa.Date(), nullable=False), + sa.ForeignKeyConstraint(['service_id'], ['services.id'], ), + sa.ForeignKeyConstraint(['template_id'], ['templates.id'], ), + sa.PrimaryKeyConstraint('id') + ) + op.create_index(op.f('ix_template_statistics_day'), 'template_statistics', ['day'], unique=False) + op.create_index(op.f('ix_template_statistics_service_id'), 'template_statistics', ['service_id'], unique=False) + op.create_index(op.f('ix_template_statistics_template_id'), 'template_statistics', ['template_id'], unique=False) + ### end Alembic commands ### + + +def downgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.drop_index(op.f('ix_template_statistics_template_id'), table_name='template_statistics') + op.drop_index(op.f('ix_template_statistics_service_id'), table_name='template_statistics') + op.drop_index(op.f('ix_template_statistics_day'), table_name='template_statistics') + op.drop_table('template_statistics') + ### end Alembic commands ### diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 3fe16c3b9..b7e657fd7 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -1,12 +1,19 @@ -import pytest +from datetime import datetime, timedelta, date import uuid -from app import ( - DATE_FORMAT -) + +import pytest + +from app import DATE_FORMAT from freezegun import freeze_time from sqlalchemy.exc import SQLAlchemyError, IntegrityError -from app.models import Notification, Job, NotificationStatistics -from datetime import datetime, timedelta + +from app.models import ( + Notification, + Job, + NotificationStatistics, + TemplateStatistics +) + from app.dao.notifications_dao import ( dao_create_notification, dao_update_notification, @@ -21,6 +28,7 @@ from app.dao.notifications_dao import ( update_notification_reference_by_id, update_notification_status_by_reference ) + from tests.app.conftest import sample_job from tests.app.conftest import sample_notification @@ -38,6 +46,7 @@ def test_should_by_able_to_update_status_by_reference(sample_email_template): 'service': sample_email_template.service, 'service_id': sample_email_template.service.id, 'template': sample_email_template, + 'template_id': sample_email_template.id, 'created_at': datetime.utcnow() } @@ -97,6 +106,7 @@ def test_should_be_able_to_record_statistics_failure_for_email(sample_email_temp 'service': sample_email_template.service, 'service_id': sample_email_template.service.id, 'template': sample_email_template, + 'template_id': sample_email_template.id, 'created_at': datetime.utcnow() } @@ -132,6 +142,7 @@ def test_should_be_able_to_get_statistics_for_a_service(sample_template): 'service': sample_template.service, 'service_id': sample_template.service.id, 'template': sample_template, + 'template_id': sample_template.id, 'created_at': datetime.utcnow() } @@ -158,6 +169,7 @@ def test_should_be_able_to_get_statistics_for_a_service_for_a_day(sample_templat 'service': sample_template.service, 'service_id': sample_template.service.id, 'template': sample_template, + 'template_id': sample_template.id, 'created_at': now } @@ -183,6 +195,7 @@ def test_should_return_none_if_no_statistics_for_a_service_for_a_day(sample_temp 'service': sample_template.service, 'service_id': sample_template.service.id, 'template': sample_template, + 'template_id': sample_template.id, 'created_at': now } @@ -199,6 +212,7 @@ def test_should_be_able_to_get_all_statistics_for_a_service(sample_template): 'service': sample_template.service, 'service_id': sample_template.service.id, 'template': sample_template, + 'template_id': sample_template.id, 'created_at': datetime.utcnow() } @@ -220,7 +234,8 @@ def test_should_be_able_to_get_all_statistics_for_a_service_for_several_days(sam 'to': '+44709123456', 'service': sample_template.service, 'service_id': sample_template.service.id, - 'template': sample_template + 'template': sample_template, + 'template_id': sample_template.id, } today = datetime.utcnow() @@ -259,14 +274,18 @@ def test_should_be_empty_list_if_no_statistics_for_a_service(sample_service): assert len(dao_get_notification_statistics_for_service(sample_service.id)) == 0 -def test_save_notification_and_create_sms_stats(sample_template, sample_job): +def test_save_notification_creates_sms_and_template_stats(sample_template, sample_job): assert Notification.query.count() == 0 + assert NotificationStatistics.query.count() == 0 + assert TemplateStatistics.query.count() == 0 + data = { 'to': '+44709123456', 'job_id': sample_job.id, 'service': sample_template.service, 'service_id': sample_template.service.id, 'template': sample_template, + 'template_id': sample_template.id, 'created_at': datetime.utcnow() } @@ -291,15 +310,27 @@ def test_save_notification_and_create_sms_stats(sample_template, sample_job): assert stats.emails_requested == 0 assert stats.sms_requested == 1 + template_stats = TemplateStatistics.query.filter(TemplateStatistics.service_id == sample_template.service.id, + TemplateStatistics.template_id == sample_template.id).first() + + assert template_stats.service_id == sample_template.service.id + assert template_stats.template_id == sample_template.id + assert template_stats.usage_count == 1 + + +def test_save_notification_and_create_email_and_template_stats(sample_email_template, sample_job): -def test_save_notification_and_create_email_stats(sample_email_template, sample_job): assert Notification.query.count() == 0 + assert NotificationStatistics.query.count() == 0 + assert TemplateStatistics.query.count() == 0 + data = { 'to': '+44709123456', 'job_id': sample_job.id, 'service': sample_email_template.service, 'service_id': sample_email_template.service.id, 'template': sample_email_template, + 'template_id': sample_email_template.id, 'created_at': datetime.utcnow() } @@ -324,6 +355,13 @@ def test_save_notification_and_create_email_stats(sample_email_template, sample_ assert stats.emails_requested == 1 assert stats.sms_requested == 0 + template_stats = TemplateStatistics.query.filter(TemplateStatistics.service_id == sample_email_template.service.id, + TemplateStatistics.template_id == sample_email_template.id).first() # noqa + + assert template_stats.service_id == sample_email_template.service.id + assert template_stats.template_id == sample_email_template.id + assert template_stats.usage_count == 1 + @freeze_time("2016-01-01 00:00:00.000000") def test_save_notification_handles_midnight_properly(sample_template, sample_job): @@ -334,6 +372,7 @@ def test_save_notification_handles_midnight_properly(sample_template, sample_job 'service': sample_template.service, 'service_id': sample_template.service.id, 'template': sample_template, + 'template_id': sample_template.id, 'created_at': datetime.utcnow() } @@ -358,6 +397,7 @@ def test_save_notification_handles_just_before_midnight_properly(sample_template 'service': sample_template.service, 'service_id': sample_template.service.id, 'template': sample_template, + 'template_id': sample_template.id, 'created_at': datetime.utcnow() } @@ -381,6 +421,7 @@ def test_save_notification_and_increment_email_stats(sample_email_template, samp 'service': sample_email_template.service, 'service_id': sample_email_template.service.id, 'template': sample_email_template, + 'template_id': sample_email_template.id, 'created_at': datetime.utcnow() } @@ -417,6 +458,7 @@ def test_save_notification_and_increment_sms_stats(sample_template, sample_job): 'service': sample_template.service, 'service_id': sample_template.service.id, 'template': sample_template, + 'template_id': sample_template.id, 'created_at': datetime.utcnow() } @@ -455,6 +497,7 @@ def test_not_save_notification_and_not_create_stats_on_commit_error(sample_templ 'service': sample_template.service, 'service_id': sample_template.service.id, 'template': sample_template, + 'template_id': sample_template.id, 'created_at': datetime.utcnow() } @@ -465,6 +508,7 @@ def test_not_save_notification_and_not_create_stats_on_commit_error(sample_templ assert Notification.query.count() == 0 assert Job.query.get(sample_job.id).notifications_sent == 0 assert NotificationStatistics.query.count() == 0 + assert TemplateStatistics.query.count() == 0 def test_save_notification_and_increment_job(sample_template, sample_job): @@ -475,6 +519,7 @@ def test_save_notification_and_increment_job(sample_template, sample_job): 'service': sample_template.service, 'service_id': sample_template.service.id, 'template': sample_template, + 'template_id': sample_template.id, 'created_at': datetime.utcnow() } @@ -509,6 +554,7 @@ def test_should_not_increment_job_if_notification_fails_to_persist(sample_templa 'service_id': sample_template.service.id, 'service': sample_template.service, 'template': sample_template, + 'template_id': sample_template.id, 'created_at': datetime.utcnow() } @@ -539,6 +585,7 @@ def test_save_notification_and_increment_correct_job(notify_db, notify_db_sessio 'service_id': sample_template.service.id, 'service': sample_template.service, 'template': sample_template, + 'template_id': sample_template.id, 'created_at': datetime.utcnow() } @@ -565,6 +612,7 @@ def test_save_notification_with_no_job(sample_template): 'service_id': sample_template.service.id, 'service': sample_template.service, 'template': sample_template, + 'template_id': sample_template.id, 'created_at': datetime.utcnow() } @@ -596,6 +644,7 @@ def test_save_notification_no_job_id(sample_template): 'service_id': sample_template.service.id, 'service': sample_template.service, 'template': sample_template, + 'template_id': sample_template.id, 'created_at': datetime.utcnow() } @@ -688,3 +737,106 @@ def test_should_not_delete_failed_notifications_before_seven_days(notify_db, not delete_failed_notifications_created_more_than_a_week_ago() assert len(Notification.query.all()) == 1 assert Notification.query.first().to == 'valid' + + +@freeze_time("2016-03-30") +def test_save_new_notification_creates_template_stats(sample_template, sample_job): + assert Notification.query.count() == 0 + assert TemplateStatistics.query.count() == 0 + data = { + 'to': '+44709123456', + 'job_id': sample_job.id, + 'service': sample_template.service, + 'service_id': sample_template.service.id, + 'template': sample_template, + 'template_id': sample_template.id, + 'created_at': datetime.utcnow() + } + + notification = Notification(**data) + dao_create_notification(notification, sample_template.template_type) + + assert TemplateStatistics.query.count() == 1 + template_stats = TemplateStatistics.query.filter(TemplateStatistics.service_id == sample_template.service.id, + TemplateStatistics.template_id == sample_template.id).first() + assert template_stats.service_id == sample_template.service.id + assert template_stats.template_id == sample_template.id + assert template_stats.usage_count == 1 + assert template_stats.day == date(2016, 3, 30) + + +@freeze_time("2016-03-30") +def test_save_new_notification_creates_template_stats_per_day(sample_template, sample_job): + assert Notification.query.count() == 0 + assert TemplateStatistics.query.count() == 0 + data = { + 'to': '+44709123456', + 'job_id': sample_job.id, + 'service': sample_template.service, + 'service_id': sample_template.service.id, + 'template': sample_template, + 'template_id': sample_template.id, + 'created_at': datetime.utcnow() + } + + notification = Notification(**data) + dao_create_notification(notification, sample_template.template_type) + + assert TemplateStatistics.query.count() == 1 + template_stats = TemplateStatistics.query.filter(TemplateStatistics.service_id == sample_template.service.id, + TemplateStatistics.template_id == sample_template.id).first() + assert template_stats.service_id == sample_template.service.id + assert template_stats.template_id == sample_template.id + assert template_stats.usage_count == 1 + assert template_stats.day == date(2016, 3, 30) + + # move on one day + with freeze_time('2016-03-31'): + assert TemplateStatistics.query.count() == 1 + new_notification = Notification(**data) + dao_create_notification(new_notification, sample_template.template_type) + + assert TemplateStatistics.query.count() == 2 + first_stats = TemplateStatistics.query.filter(TemplateStatistics.day == datetime(2016, 3, 30)).first() + second_stats = TemplateStatistics.query.filter(TemplateStatistics.day == datetime(2016, 3, 31)).first() + + assert first_stats.template_id == second_stats.template_id + assert first_stats.service_id == second_stats.service_id + + assert first_stats.day == date(2016, 3, 30) + assert first_stats.usage_count == 1 + + assert second_stats.day == date(2016, 3, 31) + assert second_stats.usage_count == 1 + + +def test_save_another_notification_increments_template_stats(sample_template, sample_job): + assert Notification.query.count() == 0 + assert TemplateStatistics.query.count() == 0 + data = { + 'to': '+44709123456', + 'job_id': sample_job.id, + 'service': sample_template.service, + 'service_id': sample_template.service.id, + 'template': sample_template, + 'template_id': sample_template.id, + 'created_at': datetime.utcnow() + } + + notification_1 = Notification(**data) + notification_2 = Notification(**data) + dao_create_notification(notification_1, sample_template.template_type) + + assert TemplateStatistics.query.count() == 1 + template_stats = TemplateStatistics.query.filter(TemplateStatistics.service_id == sample_template.service.id, + TemplateStatistics.template_id == sample_template.id).first() + assert template_stats.service_id == sample_template.service.id + assert template_stats.template_id == sample_template.id + assert template_stats.usage_count == 1 + + dao_create_notification(notification_2, sample_template.template_type) + + assert TemplateStatistics.query.count() == 1 + template_stats = TemplateStatistics.query.filter(TemplateStatistics.service_id == sample_template.service.id, + TemplateStatistics.template_id == sample_template.id).first() + assert template_stats.usage_count == 2 From efc382f18b133f6cd3868c0cad88a635ed18a543 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Thu, 31 Mar 2016 16:53:47 +0100 Subject: [PATCH 2/4] Added test to verify a number of notications followed by a failure does not increment stats counts. --- tests/app/dao/test_notification_dao.py | 64 ++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index b7e657fd7..0f4c5c251 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -840,3 +840,67 @@ def test_save_another_notification_increments_template_stats(sample_template, sa template_stats = TemplateStatistics.query.filter(TemplateStatistics.service_id == sample_template.service.id, TemplateStatistics.template_id == sample_template.id).first() assert template_stats.usage_count == 2 + + +def test_successful_notification_inserts_followed_by_failure_does_not_increment_template_stats(sample_template, + sample_job): + assert Notification.query.count() == 0 + assert NotificationStatistics.query.count() == 0 + assert TemplateStatistics.query.count() == 0 + + data = { + 'to': '+44709123456', + 'job_id': sample_job.id, + 'service': sample_template.service, + 'service_id': sample_template.service.id, + 'template': sample_template, + 'template_id': sample_template.id, + 'created_at': datetime.utcnow() + } + + notification_1 = Notification(**data) + notification_2 = Notification(**data) + notification_3 = Notification(**data) + dao_create_notification(notification_1, sample_template.template_type) + dao_create_notification(notification_2, sample_template.template_type) + dao_create_notification(notification_3, sample_template.template_type) + + assert NotificationStatistics.query.count() == 1 + notication_stats = NotificationStatistics.query.filter( + NotificationStatistics.service_id == sample_template.service.id + ).first() + assert notication_stats.sms_requested == 3 + + assert TemplateStatistics.query.count() == 1 + template_stats = TemplateStatistics.query.filter(TemplateStatistics.service_id == sample_template.service.id, + TemplateStatistics.template_id == sample_template.id).first() + assert template_stats.service_id == sample_template.service.id + assert template_stats.template_id == sample_template.id + assert template_stats.usage_count == 3 + + broken_data = { + 'to': '+44709123456', + 'job_id': sample_job.id, + 'service': None, + 'service_id': None, + 'template': sample_template, + 'template_id': sample_template.id, + 'created_at': datetime.utcnow() + } + + broken_notification = Notification(**broken_data) + try: + dao_create_notification(broken_notification, sample_template.template_type) + except: + assert TemplateStatistics.query.count() == 1 + template_stats = TemplateStatistics.query.filter(TemplateStatistics.service_id == sample_template.service.id, + TemplateStatistics.template_id == sample_template.id).first() # noqa + assert template_stats.service_id == sample_template.service.id + assert template_stats.template_id == sample_template.id + assert template_stats.usage_count == 3 + + assert NotificationStatistics.query.count() == 1 + notication_stats = NotificationStatistics.query.filter( + NotificationStatistics.service_id == sample_template.service.id + ).first() + assert notication_stats.sms_requested == 3 From 0d0cfbb6ac3a1c0beb2f12d67c6a200b18c6db83 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Thu, 31 Mar 2016 17:20:57 +0100 Subject: [PATCH 3/4] Better test for failed update of stats --- tests/app/dao/test_notification_dao.py | 27 ++++++++------------------ 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 0f4c5c251..83d79bd33 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -7,6 +7,8 @@ from app import DATE_FORMAT from freezegun import freeze_time from sqlalchemy.exc import SQLAlchemyError, IntegrityError +from app import db + from app.models import ( Notification, Job, @@ -878,27 +880,14 @@ def test_successful_notification_inserts_followed_by_failure_does_not_increment_ assert template_stats.template_id == sample_template.id assert template_stats.usage_count == 3 - broken_data = { - 'to': '+44709123456', - 'job_id': sample_job.id, - 'service': None, - 'service_id': None, - 'template': sample_template, - 'template_id': sample_template.id, - 'created_at': datetime.utcnow() - } - - broken_notification = Notification(**broken_data) + failing_notification = Notification(**data) try: - dao_create_notification(broken_notification, sample_template.template_type) - except: - assert TemplateStatistics.query.count() == 1 - template_stats = TemplateStatistics.query.filter(TemplateStatistics.service_id == sample_template.service.id, - TemplateStatistics.template_id == sample_template.id).first() # noqa - assert template_stats.service_id == sample_template.service.id - assert template_stats.template_id == sample_template.id - assert template_stats.usage_count == 3 + # Mess up db in really bad way + db.session.execute('DROP TABLE TEMPLATE_STATISTICS') + dao_create_notification(failing_notification, sample_template.template_type) + except Exception as e: + # There should be no additional notification stats or counts assert NotificationStatistics.query.count() == 1 notication_stats = NotificationStatistics.query.filter( NotificationStatistics.service_id == sample_template.service.id From 514d490d2f6146ff82c8b5aefc4b081e8c03c396 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Fri, 1 Apr 2016 11:12:44 +0100 Subject: [PATCH 4/4] No limit for live services. --- app/notifications/rest.py | 2 +- tests/app/conftest.py | 5 ++++ tests/app/notifications/test_rest.py | 38 ++++++++++++++++++++++++++-- tests/conftest.py | 1 + 4 files changed, 43 insertions(+), 3 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 2c1598410..3e536b4ed 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -321,7 +321,7 @@ def send_notification(notification_type): total_sms_count = service_stats.sms_requested total_email_count = service_stats.emails_requested - if total_email_count + total_sms_count >= service.limit: + if (total_email_count + total_sms_count >= service.limit) and service.restricted: return jsonify(result="error", message='Exceeded send limits ({}) for today'.format(service.limit)), 429 notification, errors = ( diff --git a/tests/app/conftest.py b/tests/app/conftest.py index e79b5fe76..d99314af0 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -334,6 +334,11 @@ def mock_celery_email_registration_verification(mocker): return mocker.patch('app.celery.tasks.email_registration_verification.apply_async') +@pytest.fixture(scope='function') +def mock_celery_send_email(mocker): + return mocker.patch('app.celery.tasks.send_email.apply_async') + + @pytest.fixture(scope='function') def mock_encryption(mocker): return mocker.patch('app.encryption.encrypt', return_value="something_encrypted") diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index ceec614d2..d255180a5 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -877,7 +877,7 @@ def test_should_block_api_call_if_over_day_limit(notify_db, notify_db_session, n mocker.patch('app.celery.tasks.send_email.apply_async') mocker.patch('app.encryption.encrypt', return_value="something_encrypted") - service = create_sample_service(notify_db, notify_db_session, limit=1) + service = create_sample_service(notify_db, notify_db_session, limit=1, restricted=True) email_template = create_sample_email_template(notify_db, notify_db_session, service=service) create_sample_notification( notify_db, notify_db_session, template=email_template, service=service, created_at=datetime.utcnow() @@ -905,6 +905,40 @@ def test_should_block_api_call_if_over_day_limit(notify_db, notify_db_session, n assert 'Exceeded send limits (1) for today' in json_resp['message'] +def test_no_limit_for_live_service(notify_api, + notify_db, + notify_db_session, + mock_celery_send_email, + sample_service, + sample_email_template, + sample_notification): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + + sample_service.limit = 1 + notify_db.session.add(sample_service) + notify_db.session.commit() + + data = { + 'to': 'ok@ok.com', + 'template': sample_email_template.id + } + + auth_header = create_authorization_header( + request_body=json.dumps(data), + path='/notifications/email', + method='POST', + service_id=sample_service.id + ) + + response = client.post( + path='/notifications/email', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 201 + + @freeze_time("2016-01-01 12:00:00.061258") def test_should_block_api_call_if_over_day_limit_regardless_of_type(notify_db, notify_db_session, notify_api, mocker): with notify_api.test_request_context(): @@ -912,7 +946,7 @@ def test_should_block_api_call_if_over_day_limit_regardless_of_type(notify_db, n mocker.patch('app.celery.tasks.send_sms.apply_async') mocker.patch('app.encryption.encrypt', return_value="something_encrypted") - service = create_sample_service(notify_db, notify_db_session, limit=1) + service = create_sample_service(notify_db, notify_db_session, limit=1, restricted=True) email_template = create_sample_email_template(notify_db, notify_db_session, service=service) sms_template = create_sample_template(notify_db, notify_db_session, service=service) create_sample_notification( diff --git a/tests/conftest.py b/tests/conftest.py index 55d72a2f2..1908447f0 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -45,6 +45,7 @@ def notify_db(notify_api, request): db.get_engine(notify_api).dispose() request.addfinalizer(teardown) + return db @pytest.fixture(scope='function')