diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 59d765be2..14a3821b3 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -1,6 +1,4 @@ -from sqlalchemy import (desc, func, Integer, or_, and_, asc) -from sqlalchemy.sql.expression import cast - +import uuid from datetime import ( datetime, timedelta, @@ -9,6 +7,9 @@ from datetime import ( from flask import current_app from werkzeug.datastructures import MultiDict +from sqlalchemy import (desc, func, Integer, or_, and_, asc) +from sqlalchemy.sql.expression import cast +from notifications_utils.template import get_sms_fragment_count from app import db from app.dao import days_ago @@ -24,15 +25,11 @@ from app.models import ( Template, ProviderStatistics, ProviderDetails) - -from notifications_utils.template import get_sms_fragment_count - from app.clients import ( STATISTICS_FAILURE, STATISTICS_DELIVERED, STATISTICS_REQUESTED ) - from app.dao.dao_utils import transactional @@ -183,6 +180,12 @@ def dao_create_notification(notification, notification_type): service_id=notification.service_id) db.session.add(template_stats) + if not notification.id: + # need to populate defaulted fields before we create the notification history object + notification.id = uuid.uuid4() + if not notification.status: + notification.status = 'created' + notification_history = NotificationHistory.from_notification(notification) db.session.add(notification) @@ -244,7 +247,8 @@ def _update_notification_status(notification, status, notification_statistics_st if notification_statistics_status: _update_statistics(notification, notification_statistics_status) - db.session.query(Notification).filter(Notification.id == notification.id).update({Notification.status: status}) + notification.status = status + dao_update_notification(notification) return True diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index fd73d03e4..e86bf74f6 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -17,6 +17,7 @@ from app.models import ( ApiKey, Template, Job, + NotificationHistory, Notification, Permission, User, @@ -97,6 +98,7 @@ def delete_service_and_all_associated_db_objects(service): _delete_commit(Permission.query.filter_by(service=service)) _delete_commit(ApiKey.query.filter_by(service=service)) _delete_commit(ApiKey.get_history_model().query.filter_by(service_id=service.id)) + _delete_commit(NotificationHistory.query.filter_by(service=service)) _delete_commit(Notification.query.filter_by(service=service)) _delete_commit(Job.query.filter_by(service=service)) _delete_commit(Template.query.filter_by(service=service)) diff --git a/app/models.py b/app/models.py index c95fc9718..139a29823 100644 --- a/app/models.py +++ b/app/models.py @@ -334,6 +334,7 @@ NOTIFICATION_STATUS_TYPES = ['created', 'sending', 'delivered', 'pending', 'fail 'technical-failure', 'temporary-failure', 'permanent-failure'] NOTIFICATION_STATUS_TYPES_ENUM = db.Enum(*NOTIFICATION_STATUS_TYPES, name='notify_status_type') + class Notification(db.Model): __tablename__ = 'notifications' @@ -401,6 +402,7 @@ class NotificationHistory(db.Model): api_key_id = db.Column(UUID(as_uuid=True), db.ForeignKey('api_keys.id'), index=True, unique=False) api_key = db.relationship('ApiKey') key_type = db.Column(db.String, db.ForeignKey('key_types.name'), index=True, unique=False, nullable=False) + content_char_count = db.Column(db.Integer, nullable=True) notification_type = db.Column(notification_types, nullable=False) created_at = db.Column(db.DateTime, index=False, unique=False, nullable=False) sent_at = db.Column(db.DateTime, index=False, unique=False, nullable=True) @@ -411,13 +413,11 @@ class NotificationHistory(db.Model): @classmethod def from_notification(cls, notification): - from app.schemas import notification_status_schema - return cls(notification_status_schema.dump(notification)) + return cls(**{c.name: getattr(notification, c.name) for c in cls.__table__.columns}) def update_from_notification(self, notification): - from app.schemas import notification_status_schema - new_notification_schema = cls(notification_status_schema.dump(notification)) - + for c in self.__table__.columns: + setattr(self, c.name, getattr(notification, c.name)) INVITED_USER_STATUS_TYPES = ['pending', 'accepted', 'cancelled'] diff --git a/app/schemas.py b/app/schemas.py index a04dadbc0..b4ac775d9 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -288,12 +288,6 @@ class NotificationStatusSchema(BaseSchema): return in_data -class NotificationHistorySchema(BaseSchema): - class Meta: - model = models.NotificationHistory - strict = True - - class InvitedUserSchema(BaseSchema): class Meta: @@ -498,7 +492,6 @@ email_notification_schema = EmailNotificationSchema() job_email_template_notification_schema = JobEmailTemplateNotificationSchema() notification_status_schema = NotificationStatusSchema() notification_status_schema_load_json = NotificationStatusSchema(load_json=True) -notification_history_schema = NotificationHistorySchema() invited_user_schema = InvitedUserSchema() permission_schema = PermissionSchema() email_data_request_schema = EmailDataSchema() diff --git a/migrations/versions/0041_notification_history.py b/migrations/versions/0041_notification_history.py deleted file mode 100644 index d0609c5d1..000000000 --- a/migrations/versions/0041_notification_history.py +++ /dev/null @@ -1,60 +0,0 @@ -"""empty message - -Revision ID: 807f0b497157 -Revises: 0041_notification_history -Create Date: 2016-07-07 13:15:35.503107 - -""" - -# revision identifiers, used by Alembic. -revision = '0040_adjust_mmg_provider_rate' -down_revision = '0039_fix_notifications' - -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('notification_history', - sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), - sa.Column('job_id', postgresql.UUID(as_uuid=True), nullable=True), - sa.Column('job_row_number', sa.Integer(), nullable=True), - sa.Column('service_id', postgresql.UUID(as_uuid=True), nullable=True), - sa.Column('template_id', postgresql.UUID(as_uuid=True), nullable=True), - sa.Column('template_version', sa.Integer(), nullable=False), - sa.Column('api_key_id', postgresql.UUID(as_uuid=True), nullable=True), - sa.Column('key_type', sa.String(), nullable=False), - sa.Column('notification_type', sa.Enum('email', 'sms', 'letter', name='notification_type'), nullable=False), - sa.Column('created_at', sa.DateTime(), nullable=False), - sa.Column('sent_at', sa.DateTime(), nullable=True), - sa.Column('sent_by', sa.String(), nullable=True), - sa.Column('updated_at', sa.DateTime(), nullable=True), - sa.Column('status', sa.Enum('created', 'sending', 'delivered', 'pending', 'failed', 'technical-failure', 'temporary-failure', 'permanent-failure', name='notify_status_type'), nullable=False), - sa.Column('reference', sa.String(), nullable=True), - sa.ForeignKeyConstraint(['api_key_id'], ['api_keys.id'], ), - sa.ForeignKeyConstraint(['job_id'], ['jobs.id'], ), - sa.ForeignKeyConstraint(['key_type'], ['key_types.name'], ), - sa.ForeignKeyConstraint(['service_id'], ['services.id'], ), - sa.ForeignKeyConstraint(['template_id'], ['templates.id'], ), - sa.PrimaryKeyConstraint('id') - ) - op.create_index(op.f('ix_notification_history_api_key_id'), 'notification_history', ['api_key_id'], unique=False) - op.create_index(op.f('ix_notification_history_job_id'), 'notification_history', ['job_id'], unique=False) - op.create_index(op.f('ix_notification_history_key_type'), 'notification_history', ['key_type'], unique=False) - op.create_index(op.f('ix_notification_history_reference'), 'notification_history', ['reference'], unique=False) - op.create_index(op.f('ix_notification_history_service_id'), 'notification_history', ['service_id'], unique=False) - op.create_index(op.f('ix_notification_history_template_id'), 'notification_history', ['template_id'], unique=False) - ### end Alembic commands ### - - -def downgrade(): - ### commands auto generated by Alembic - please adjust! ### - op.drop_index(op.f('ix_notification_history_template_id'), table_name='notification_history') - op.drop_index(op.f('ix_notification_history_service_id'), table_name='notification_history') - op.drop_index(op.f('ix_notification_history_reference'), table_name='notification_history') - op.drop_index(op.f('ix_notification_history_key_type'), table_name='notification_history') - op.drop_index(op.f('ix_notification_history_job_id'), table_name='notification_history') - op.drop_index(op.f('ix_notification_history_api_key_id'), table_name='notification_history') - op.drop_table('notification_history') - ### end Alembic commands ### diff --git a/migrations/versions/0042_notification_history.py b/migrations/versions/0042_notification_history.py new file mode 100644 index 000000000..4ae8be593 --- /dev/null +++ b/migrations/versions/0042_notification_history.py @@ -0,0 +1,61 @@ +"""empty message + +Revision ID: 0042_notification_history +Revises: 0041_email_template +Create Date: 2016-07-07 13:15:35.503107 + +""" + +# revision identifiers, used by Alembic. +revision = '0042_notification_history' +down_revision = '0041_email_template' + +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('notification_history', + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('job_id', postgresql.UUID(as_uuid=True), nullable=True), + sa.Column('job_row_number', sa.Integer(), nullable=True), + sa.Column('service_id', postgresql.UUID(as_uuid=True), nullable=True), + sa.Column('template_id', postgresql.UUID(as_uuid=True), nullable=True), + sa.Column('template_version', sa.Integer(), nullable=False), + sa.Column('api_key_id', postgresql.UUID(as_uuid=True), nullable=True), + sa.Column('key_type', sa.String(), nullable=False), + sa.Column('content_char_count', sa.Integer(), nullable=True), + sa.Column('notification_type', postgresql.ENUM('email', 'sms', 'letter', name='notification_type', create_type=False), nullable=False), + sa.Column('created_at', sa.DateTime(), nullable=False), + sa.Column('sent_at', sa.DateTime(), nullable=True), + sa.Column('sent_by', sa.String(), nullable=True), + sa.Column('updated_at', sa.DateTime(), nullable=True), + sa.Column('status', postgresql.ENUM('created', 'sending', 'delivered', 'pending', 'failed', 'technical-failure', 'temporary-failure', 'permanent-failure', name='notify_status_type', create_type=False), nullable=False), + sa.Column('reference', sa.String(), nullable=True), + sa.ForeignKeyConstraint(['api_key_id'], ['api_keys.id'], ), + sa.ForeignKeyConstraint(['job_id'], ['jobs.id'], ), + sa.ForeignKeyConstraint(['key_type'], ['key_types.name'], ), + sa.ForeignKeyConstraint(['service_id'], ['services.id'], ), + sa.ForeignKeyConstraint(['template_id'], ['templates.id'], ), + sa.PrimaryKeyConstraint('id') + ) + op.create_index(op.f('ix_notification_history_api_key_id'), 'notification_history', ['api_key_id'], unique=False) + op.create_index(op.f('ix_notification_history_job_id'), 'notification_history', ['job_id'], unique=False) + op.create_index(op.f('ix_notification_history_key_type'), 'notification_history', ['key_type'], unique=False) + op.create_index(op.f('ix_notification_history_reference'), 'notification_history', ['reference'], unique=False) + op.create_index(op.f('ix_notification_history_service_id'), 'notification_history', ['service_id'], unique=False) + op.create_index(op.f('ix_notification_history_template_id'), 'notification_history', ['template_id'], unique=False) + ### end Alembic commands ### + + +def downgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.drop_index(op.f('ix_notification_history_template_id'), table_name='notification_history') + op.drop_index(op.f('ix_notification_history_service_id'), table_name='notification_history') + op.drop_index(op.f('ix_notification_history_reference'), table_name='notification_history') + op.drop_index(op.f('ix_notification_history_key_type'), table_name='notification_history') + op.drop_index(op.f('ix_notification_history_job_id'), table_name='notification_history') + op.drop_index(op.f('ix_notification_history_api_key_id'), table_name='notification_history') + op.drop_table('notification_history') + ### end Alembic commands ### diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index cae2cfc17..a963dd648 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -58,14 +58,20 @@ def test_should_by_able_to_update_status_by_reference(sample_email_template, ses def test_should_by_able_to_update_status_by_id(sample_template, sample_job, mmg_provider): - data = _notification_json(sample_template, job_id=sample_job.id, status='sending') - notification = Notification(**data) - dao_create_notification(notification, sample_template.template_type) + with freeze_time('2000-01-01 12:00:00'): + data = _notification_json(sample_template, job_id=sample_job.id, status='sending') + notification = Notification(**data) + dao_create_notification(notification, sample_template.template_type) + assert Notification.query.get(notification.id).status == 'sending' - assert update_notification_status_by_id(notification.id, 'delivered', 'delivered') + + with freeze_time('2000-01-02 12:00:00'): + assert update_notification_status_by_id(notification.id, 'delivered', 'delivered') + assert Notification.query.get(notification.id).status == 'delivered' _assert_notification_stats(notification.service_id, sms_delivered=1, sms_requested=1, sms_failed=0) _assert_job_stats(notification.job_id, sent=1, count=1, delivered=1, failed=0) + assert notification.updated_at == datetime(2000, 1, 2, 12, 0, 0) def test_should_not_update_status_by_id_if_not_sending_and_does_not_update_job(notify_db, notify_db_session): @@ -609,10 +615,10 @@ def test_save_notification_with_no_job(sample_template, mmg_provider): def test_get_notification(sample_notification): - notifcation_from_db = get_notification( + notification_from_db = get_notification( sample_notification.service.id, sample_notification.id) - assert sample_notification == notifcation_from_db + assert sample_notification == notification_from_db def test_save_notification_no_job_id(sample_template, mmg_provider): @@ -634,11 +640,11 @@ def test_save_notification_no_job_id(sample_template, mmg_provider): def test_get_notification_for_job(sample_notification): - notifcation_from_db = get_notification_for_job( + notification_from_db = get_notification_for_job( sample_notification.service.id, sample_notification.job_id, sample_notification.id) - assert sample_notification == notifcation_from_db + assert sample_notification == notification_from_db def test_get_all_notifications_for_job(notify_db, notify_db_session, sample_job): @@ -693,7 +699,7 @@ def test_should_delete_notifications_after_seven_days(notify_db, notify_db_sessi # create one notification a day between 1st and 10th from 11:00 to 19:00 for i in range(1, 11): - past_date = '2016-01-{0:02d} {0:02d}:00:00.000000'.format(i, i) + past_date = '2016-01-{0:02d} {0:02d}:00:00.000000'.format(i) with freeze_time(past_date): sample_notification(notify_db, notify_db_session, created_at=datetime.utcnow(), status="failed") @@ -708,6 +714,22 @@ def test_should_delete_notifications_after_seven_days(notify_db, notify_db_sessi assert notification.created_at.date() >= date(2016, 1, 3) +@freeze_time("2016-01-10 12:00:00.000000") +def test_should_not_delete_notification_history(notify_db, notify_db_session): + with freeze_time('2016-01-01 12:00'): + notification = sample_notification(notify_db, notify_db_session, created_at=datetime.utcnow(), status="failed") + notification_id = notification.id + + assert Notification.query.count() == 1 + assert NotificationHistory.query.count() == 1 + + delete_notifications_created_more_than_a_week_ago('failed') + + assert Notification.query.count() == 0 + assert NotificationHistory.query.count() == 1 + assert NotificationHistory.query.one().id == notification_id + + def test_should_not_delete_failed_notifications_before_seven_days(notify_db, notify_db_session): should_delete = datetime.utcnow() - timedelta(days=8) do_not_delete = datetime.utcnow() - timedelta(days=7) @@ -968,6 +990,35 @@ def test_sms_fragment_count(char_count, expected_sms_fragment_count): assert get_sms_fragment_count(char_count) == expected_sms_fragment_count +def test_creating_notification_adds_to_notification_history(sample_template): + data = _notification_json(sample_template) + notification = Notification(**data) + + dao_create_notification(notification, sample_template.template_type) + + assert Notification.query.count() == 1 + + hist = NotificationHistory.query.one() + assert hist.id == notification.id + assert hist.created_at == notification.created_at + assert hist.status == notification.status + assert not hasattr(hist, 'to') + assert not hasattr(hist, '_personalisation') + + +def test_updating_notification_updates_notification_history(sample_notification): + hist = NotificationHistory.query.one() + assert hist.id == sample_notification.id + assert hist.status == 'created' + + sample_notification.status = 'sending' + dao_update_notification(sample_notification) + + hist = NotificationHistory.query.one() + assert hist.id == sample_notification.id + assert hist.status == 'sending' + + def _notification_json(sample_template, job_id=None, id=None, status=None): data = { 'to': '+44709123456', @@ -1011,31 +1062,3 @@ def _assert_job_stats(job_id, sent=0, count=0, delivered=0, failed=0): assert job.notification_count == count assert job.notifications_delivered == delivered assert job.notifications_failed == failed - - -def test_creating_notification_adds_to_notification_history(sample_template): - data = _notification_json(sample_template) - notification = Notification(**data) - - dao_create_notification(notification, sample_template.template_type) - - assert Notification.query.count() == 1 - - hist = NotificationHistory.query.one() - assert hist.id == notifcation.id - assert not hasattr(hist.to) - assert not hasattr(hist._personalisation) - assert not hasattr(hist.content_char_count) - - -def test_updating_notification_updates_notification_history(sample_notification): - hist = NotificationHistory.query.one() - assert hist.id == notifcation.id - assert hist.status == 'sending' - - sample_notification.status = 'delivered' - dao_update_notification(sample_notification) - - hist = NotificationHistory.query.one() - assert hist.id == notification.id - assert hist.status == 'delivered'