update notification_history table from notification_dao create/update functions

please ensure that any changes to notifications table happen through either dao_create_notification or dao_update_notification.
changed the notification status update triggered by the provider callbacks to ensure that sets updated_by and can update the history table.
also re-added the character_count so we can reconstruct billing data if needed.

app/dao/notifications_dao.py

@@ -1,6 +1,4 @@
-from sqlalchemy import (desc, func, Integer, or_, and_, asc)
+import uuid
-from sqlalchemy.sql.expression import cast
-
 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
 
 

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))

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(**{c.name: getattr(notification, c.name) for c in cls.__table__.columns})
-        return cls(notification_status_schema.dump(notification))
 
     def update_from_notification(self, notification):
-        from app.schemas import notification_status_schema
+        for c in self.__table__.columns:
-        new_notification_schema = cls(notification_status_schema.dump(notification))
+            setattr(self, c.name, getattr(notification, c.name))
-
 
 
 INVITED_USER_STATUS_TYPES = ['pending', 'accepted', 'cancelled']

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()

migrations/versions/0041_notification_history.py

@@ -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 ###

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 ###

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')
+    with freeze_time('2000-01-01 12:00:00'):
-    notification = Notification(**data)
+        data = _notification_json(sample_template, job_id=sample_job.id, status='sending')
-    dao_create_notification(notification, sample_template.template_type)
+        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'