Merge pull request #236 from alphagov/record-service-history

A slightly manual way of recording history.

app/dao/dao_utils.py

@@ -0,0 +1,48 @@
+import datetime
+
+from functools import wraps
+
+
+def create_history(obj):
+    history_mapper = obj.__history_mapper__
+    history_model = history_mapper.class_
+    history = history_model()
+    if obj.version:
+        obj.version += 1
+    else:
+        obj.version = 1
+        obj.created_at = datetime.datetime.now()
+    for prop in history_mapper.iterate_properties:
+        if obj.__mapper__.get_property(prop.key):
+            setattr(history, prop.key, getattr(obj, prop.key))
+    history.created_by_id = obj.created_by.id
+    return history
+
+
+def transactional(func):
+    @wraps(func)
+    def commit_or_rollback(*args, **kwargs):
+        from flask import current_app
+        from app import db
+        try:
+            func(*args, **kwargs)
+            db.session.commit()
+        except Exception as e:
+            current_app.logger.error(e)
+            db.session.rollback()
+            raise
+    return commit_or_rollback
+
+
+def versioned(func):
+    @wraps(func)
+    def record_version(*args, **kwargs):
+        import itertools
+        from app import db
+        from app.history_meta import versioned_objects
+        from app.dao.dao_utils import create_history
+        func(*args, **kwargs)
+        for obj in versioned_objects(itertools.chain(db.session.new, db.session.dirty)):
+            history = create_history(obj)
+            db.session.add(history)
+    return record_version

app/dao/notifications_dao.py

@@ -30,22 +30,7 @@ from app.clients import (
     STATISTICS_REQUESTED
 )
 
-from functools import wraps
-
-
-def transactional(func):
-    @wraps(func)
-    def commit_or_rollback(*args, **kwargs):
-        from flask import current_app
-        from app import db
-        try:
-            func(*args, **kwargs)
-            db.session.commit()
-        except Exception as e:
-            current_app.logger.error(e)
-            db.session.rollback()
-            raise
-    return commit_or_rollback
+from app.dao.dao_utils import transactional
 
 
 def get_character_count_of_content(content, encoding='utf-8'):

app/dao/services_dao.py

@@ -1,7 +1,14 @@
+import uuid
+
 from app import db
 from app.models import Service
 from sqlalchemy import asc
 
+from app.dao.dao_utils import (
+    transactional,
+    versioned
+)
+
 
 def dao_fetch_all_services():
     return Service.query.order_by(asc(Service.created_at)).all()
@@ -19,22 +26,20 @@ def dao_fetch_service_by_id_and_user(service_id, user_id):
     return Service.query.filter(Service.users.any(id=user_id)).filter_by(id=service_id).one()
 
 
+@transactional
+@versioned
 def dao_create_service(service, user):
-    try:
-        from app.dao.permissions_dao import permission_dao
-        service.users.append(user)
-        permission_dao.add_default_service_permissions_for_user(user, service)
-        db.session.add(service)
-    except Exception as e:
-        db.session.rollback()
-        raise e
-    else:
-        db.session.commit()
+    from app.dao.permissions_dao import permission_dao
+    service.users.append(user)
+    permission_dao.add_default_service_permissions_for_user(user, service)
+    service.id = uuid.uuid4()  # must be set now so version history model can use same id
+    db.session.add(service)
 
 
+@transactional
+@versioned
 def dao_update_service(service):
     db.session.add(service)
-    db.session.commit()
 
 
 def dao_add_user_to_service(service, user, permissions=[]):

app/history_meta.py

@@ -0,0 +1,167 @@
+"""Versioned mixin class and other utilities.
+
+This is an adapted version of:
+
+https://bitbucket.org/zzzeek/sqlalchemy/raw/master/examples/versioned_history/history_meta.py
+
+It does not use the create_version function from the orginal which looks for changes to models
+as we just insert a copy of a model to the history table on create or update.
+
+Also it does not add a created_at timestamp to the history table as we already have created_at
+and updated_at timestamps.
+
+Lastly when to create a version is done manually in dao_utils version decorator and not via
+session events.
+
+"""
+
+
+from sqlalchemy.ext.declarative import declared_attr
+from sqlalchemy.orm import mapper
+from sqlalchemy import Table, Column, ForeignKeyConstraint, Integer
+from sqlalchemy import util
+
+
+def col_references_table(col, table):
+    for fk in col.foreign_keys:
+        if fk.references(table):
+            return True
+    return False
+
+
+def _is_versioning_col(col):
+    return "version_meta" in col.info
+
+
+def _history_mapper(local_mapper):
+    cls = local_mapper.class_
+
+    # set the "active_history" flag
+    # on on column-mapped attributes so that the old version
+    # of the info is always loaded (currently sets it on all attributes)
+    for prop in local_mapper.iterate_properties:
+        getattr(local_mapper.class_, prop.key).impl.active_history = True
+
+    super_mapper = local_mapper.inherits
+    super_history_mapper = getattr(cls, '__history_mapper__', None)
+
+    polymorphic_on = None
+    super_fks = []
+
+    def _col_copy(col):
+        orig = col
+        col = col.copy()
+        orig.info['history_copy'] = col
+        col.unique = False
+        col.default = col.server_default = None
+        return col
+
+    properties = util.OrderedDict()
+    if not super_mapper or \
+            local_mapper.local_table is not super_mapper.local_table:
+        cols = []
+        version_meta = {"version_meta": True}
+        for column in local_mapper.local_table.c:
+            if _is_versioning_col(column):
+                continue
+
+            col = _col_copy(column)
+
+            if super_mapper and \
+                    col_references_table(column, super_mapper.local_table):
+                super_fks.append(
+                    (
+                        col.key,
+                        list(super_history_mapper.local_table.primary_key)[0]
+                    )
+                )
+
+            cols.append(col)
+
+            if column is local_mapper.polymorphic_on:
+                polymorphic_on = col
+
+            orig_prop = local_mapper.get_property_by_column(column)
+            # carry over column re-mappings
+            if len(orig_prop.columns) > 1 or \
+                    orig_prop.columns[0].key != orig_prop.key:
+                properties[orig_prop.key] = tuple(
+                    col.info['history_copy'] for col in orig_prop.columns)
+
+        if super_mapper:
+            super_fks.append(
+                (
+                    'version', super_history_mapper.local_table.c.version
+                )
+            )
+
+        # "version" stores the integer version id.  This column is
+        # required.
+        cols.append(
+            Column(
+                'version', Integer, primary_key=True,
+                autoincrement=False, info=version_meta))
+
+        if super_fks:
+            cols.append(ForeignKeyConstraint(*zip(*super_fks)))
+
+        table = Table(
+            local_mapper.local_table.name + '_history',
+            local_mapper.local_table.metadata,
+            *cols,
+            schema=local_mapper.local_table.schema
+        )
+    else:
+        # single table inheritance.  take any additional columns that may have
+        # been added and add them to the history table.
+        for column in local_mapper.local_table.c:
+            if column.key not in super_history_mapper.local_table.c:
+                col = _col_copy(column)
+                super_history_mapper.local_table.append_column(col)
+        table = None
+
+    if super_history_mapper:
+        bases = (super_history_mapper.class_,)
+
+        if table is not None:
+            properties['changed'] = (
+                (table.c.changed, ) +
+                tuple(super_history_mapper.attrs.changed.columns)
+            )
+
+    else:
+        bases = local_mapper.base_mapper.class_.__bases__
+    versioned_cls = type.__new__(type, "%sHistory" % cls.__name__, bases, {})
+
+    m = mapper(
+        versioned_cls,
+        table,
+        inherits=super_history_mapper,
+        polymorphic_on=polymorphic_on,
+        polymorphic_identity=local_mapper.polymorphic_identity,
+        properties=properties
+    )
+    cls.__history_mapper__ = m
+
+    if not super_history_mapper:
+        local_mapper.local_table.append_column(
+            Column('version', Integer, default=1, nullable=False)
+        )
+        local_mapper.add_property(
+            "version", local_mapper.local_table.c.version)
+
+
+class Versioned(object):
+    @declared_attr
+    def __mapper_cls__(cls):
+        def map(cls, *arg, **kw):
+            mp = mapper(cls, *arg, **kw)
+            _history_mapper(mp)
+            return mp
+        return map
+
+
+def versioned_objects(iter):
+    for obj in iter:
+        if hasattr(obj, '__history_mapper__'):
+            yield obj

app/models.py

@@ -1,19 +1,19 @@
 import uuid
 import datetime
 
-from sqlalchemy import UniqueConstraint, Sequence, CheckConstraint
+from sqlalchemy.dialects.postgresql import UUID
 
-from . import db
-from sqlalchemy.dialects.postgresql import (
-    UUID,
-    ARRAY
-)
+from sqlalchemy import UniqueConstraint
 
 from app.encryption import (
     hashpw,
     check_hash
 )
 
+from app import db
+
+from app.history_meta import Versioned
+
 
 def filter_null_value_fields(obj):
     return dict(
@@ -68,7 +68,7 @@ user_to_service = db.Table(
 )
 
 
-class Service(db.Model):
+class Service(db.Model, Versioned):
     __tablename__ = 'services'
 
     id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4)
@@ -93,6 +93,13 @@ class Service(db.Model):
         backref=db.backref('user_to_service', lazy='dynamic'))
     restricted = db.Column(db.Boolean, index=False, unique=False, nullable=False)
     email_from = db.Column(db.Text, index=False, unique=True, nullable=False)
+    created_by = db.relationship('User')
+    created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False)
+
+    @classmethod
+    def get_history_model(cls):
+        history_mapper = cls.__history_mapper__
+        return history_mapper.class_
 
 
 class ApiKey(db.Model):

app/service/rest.py

@@ -71,7 +71,6 @@ def create_service():
     user = get_model_users(data['user_id'])
 
     data.pop('user_id', None)
-
     valid_service, errors = service_schema.load(request.get_json())
 
     if errors:

migrations/versions/0003_add_service_history.py

@@ -0,0 +1,56 @@
+"""empty message
+
+Revision ID: 0003_add_service_history
+Revises: 0002_add_content_char_count
+Create Date: 2016-04-19 13:01:54.519821
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = '0003_add_service_history'
+down_revision = '0002_add_content_char_count'
+
+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('services_history',
+    sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False),
+    sa.Column('name', sa.String(length=255), nullable=False),
+    sa.Column('created_at', sa.DateTime(), nullable=False),
+    sa.Column('updated_at', sa.DateTime(), nullable=True),
+    sa.Column('active', sa.Boolean(), nullable=False),
+    sa.Column('message_limit', sa.BigInteger(), nullable=False),
+    sa.Column('restricted', sa.Boolean(), nullable=False),
+    sa.Column('email_from', sa.Text(), nullable=False),
+    sa.Column('created_by_id', postgresql.UUID(as_uuid=True), nullable=False),
+    sa.Column('version', sa.Integer(), autoincrement=False, nullable=False),
+    sa.PrimaryKeyConstraint('id', 'version')
+    )
+    op.create_index(op.f('ix_services_history_created_by_id'), 'services_history', ['created_by_id'], unique=False)
+    op.add_column('services', sa.Column('created_by_id', postgresql.UUID(as_uuid=True), nullable=True))
+    op.add_column('services', sa.Column('version', sa.Integer(), nullable=True))
+    op.create_index(op.f('ix_services_created_by_id'), 'services', ['created_by_id'], unique=False)
+    op.create_foreign_key('fk_services_created_by_id', 'services', 'users', ['created_by_id'], ['id'])
+
+    op.get_bind()
+    op.execute('UPDATE services SET created_by_id = (SELECT user_id FROM user_to_service WHERE services.id = user_to_service.service_id LIMIT 1)')
+    op.execute('UPDATE services SET version = 1')
+    op.execute('INSERT INTO services_history SELECT * FROM services')
+
+    op.alter_column('services', 'created_by_id', nullable=False)
+    op.alter_column('services', 'version', nullable=False)
+    ### end Alembic commands ###
+
+
+def downgrade():
+    ### commands auto generated by Alembic - please adjust! ###
+    op.drop_constraint('fk_services_created_by_id', 'services', type_='foreignkey')
+    op.drop_index(op.f('ix_services_created_by_id'), table_name='services')
+    op.drop_column('services', 'version')
+    op.drop_column('services', 'created_by_id')
+    op.drop_index(op.f('ix_services_history_created_by_id'), table_name='services_history')
+    op.drop_table('services_history')
+    ### end Alembic commands ###

tests/app/conftest.py

@@ -115,7 +115,8 @@ def sample_service(notify_db,
         'message_limit': limit,
         'active': False,
         'restricted': restricted,
-        'email_from': email_from
+        'email_from': email_from,
+        'created_by': user
     }
     service = Service.query.filter_by(name=service_name).first()
     if not service:

tests/app/dao/test_services_dao.py

@@ -7,7 +7,8 @@ from app.dao.services_dao import (
     dao_fetch_all_services,
     dao_fetch_service_by_id,
     dao_fetch_all_services_by_user,
-    dao_fetch_service_by_id_and_user
+    dao_fetch_service_by_id_and_user,
+    dao_update_service
 )
 from app.dao.users_dao import save_model_user
 from app.models import Service, User
@@ -17,7 +18,12 @@ from sqlalchemy.exc import IntegrityError
 
 def test_create_service(sample_user):
     assert Service.query.count() == 0
-    service = Service(name="service_name", email_from="email_from", message_limit=1000, active=True, restricted=False)
+    service = Service(name="service_name",
+                      email_from="email_from",
+                      message_limit=1000,
+                      active=True,
+                      restricted=False,
+                      created_by=sample_user)
     dao_create_service(service, sample_user)
     assert Service.query.count() == 1
     assert Service.query.first().name == "service_name"
@@ -27,8 +33,19 @@ def test_create_service(sample_user):
 
 def test_cannot_create_two_services_with_same_name(sample_user):
     assert Service.query.count() == 0
-    service1 = Service(name="service_name", email_from="email_from1", message_limit=1000, active=True, restricted=False)
-    service2 = Service(name="service_name", email_from="email_from2", message_limit=1000, active=True, restricted=False)
+    service1 = Service(name="service_name",
+                       email_from="email_from1",
+                       message_limit=1000,
+                       active=True,
+                       restricted=False,
+                       created_by=sample_user)
+
+    service2 = Service(name="service_name",
+                       email_from="email_from2",
+                       message_limit=1000,
+                       active=True,
+                       restricted=False,
+                       created_by=sample_user)
     with pytest.raises(IntegrityError) as excinfo:
         dao_create_service(service1, sample_user)
         dao_create_service(service2, sample_user)
@@ -37,24 +54,44 @@ def test_cannot_create_two_services_with_same_name(sample_user):
 
 def test_cannot_create_two_services_with_same_email_from(sample_user):
     assert Service.query.count() == 0
-    service1 = Service(name="service_name1", email_from="email_from", message_limit=1000, active=True, restricted=False)
-    service2 = Service(name="service_name2", email_from="email_from", message_limit=1000, active=True, restricted=False)
+    service1 = Service(name="service_name1",
+                       email_from="email_from",
+                       message_limit=1000,
+                       active=True,
+                       restricted=False,
+                       created_by=sample_user)
+    service2 = Service(name="service_name2",
+                       email_from="email_from",
+                       message_limit=1000,
+                       active=True,
+                       restricted=False,
+                       created_by=sample_user)
     with pytest.raises(IntegrityError) as excinfo:
         dao_create_service(service1, sample_user)
         dao_create_service(service2, sample_user)
     assert 'duplicate key value violates unique constraint "services_email_from_key"' in str(excinfo.value)
 
 
-def test_cannot_create_service_with_no_user(notify_db_session):
+def test_cannot_create_service_with_no_user(notify_db_session, sample_user):
     assert Service.query.count() == 0
-    service = Service(name="service_name", email_from="email_from", message_limit=1000, active=True, restricted=False)
+    service = Service(name="service_name",
+                      email_from="email_from",
+                      message_limit=1000,
+                      active=True,
+                      restricted=False,
+                      created_by=sample_user)
     with pytest.raises(FlushError) as excinfo:
         dao_create_service(service, None)
     assert "Can't flush None value found in collection Service.users" in str(excinfo.value)
 
 
 def test_should_add_user_to_service(sample_user):
-    service = Service(name="service_name", email_from="email_from", message_limit=1000, active=True, restricted=False)
+    service = Service(name="service_name",
+                      email_from="email_from",
+                      message_limit=1000,
+                      active=True,
+                      restricted=False,
+                      created_by=sample_user)
     dao_create_service(service, sample_user)
     assert sample_user in Service.query.first().users
     new_user = User(
@@ -69,7 +106,12 @@ def test_should_add_user_to_service(sample_user):
 
 
 def test_should_remove_user_from_service(sample_user):
-    service = Service(name="service_name", email_from="email_from", message_limit=1000, active=True, restricted=False)
+    service = Service(name="service_name",
+                      email_from="email_from",
+                      message_limit=1000,
+                      active=True,
+                      restricted=False,
+                      created_by=sample_user)
     dao_create_service(service, sample_user)
     new_user = User(
         name='Test User',
@@ -174,3 +216,74 @@ def test_cannot_get_service_by_id_and_owned_by_different_user(service_factory, s
     with pytest.raises(NoResultFound) as e:
         dao_fetch_service_by_id_and_user(service2.id, sample_user.id)
     assert 'No row was found for one()' in str(e)
+
+
+def test_create_service_creates_a_history_record_with_current_data(sample_user):
+    assert Service.query.count() == 0
+    assert Service.get_history_model().query.count() == 0
+    service = Service(name="service_name",
+                      email_from="email_from",
+                      message_limit=1000,
+                      active=True,
+                      restricted=False,
+                      created_by=sample_user)
+    dao_create_service(service, sample_user)
+    assert Service.query.count() == 1
+    assert Service.get_history_model().query.count() == 1
+
+    service_from_db = Service.query.first()
+    service_history = Service.get_history_model().query.first()
+
+    assert service_from_db.id == service_history.id
+    assert service_from_db.name == service_history.name
+    assert service_from_db.version == 1
+    assert service_from_db.version == service_history.version
+    assert sample_user.id == service_history.created_by_id
+    assert service_from_db.created_by.id == service_history.created_by_id
+
+
+def test_update_service_creates_a_history_record_with_current_data(sample_user):
+    assert Service.query.count() == 0
+    assert Service.get_history_model().query.count() == 0
+    service = Service(name="service_name",
+                      email_from="email_from",
+                      message_limit=1000,
+                      active=True,
+                      restricted=False,
+                      created_by=sample_user)
+    dao_create_service(service, sample_user)
+
+    assert Service.query.count() == 1
+    assert Service.query.first().version == 1
+    assert Service.get_history_model().query.count() == 1
+
+    service.name = 'updated_service_name'
+    dao_update_service(service)
+
+    assert Service.query.count() == 1
+    assert Service.get_history_model().query.count() == 2
+
+    service_from_db = Service.query.first()
+
+    assert service_from_db.version == 2
+
+    assert Service.get_history_model().query.filter_by(name='service_name').one().version == 1
+    assert Service.get_history_model().query.filter_by(name='updated_service_name').one().version == 2
+
+
+def test_create_service_and_history_is_transactional(sample_user):
+    assert Service.query.count() == 0
+    assert Service.get_history_model().query.count() == 0
+    service = Service(name=None,
+                      email_from="email_from",
+                      message_limit=1000,
+                      active=True,
+                      restricted=False,
+                      created_by=sample_user)
+
+    with pytest.raises(IntegrityError) as excinfo:
+        dao_create_service(service, sample_user)
+
+    assert 'column "name" violates not-null constraint' in str(excinfo.value)
+    assert Service.query.count() == 0
+    assert Service.get_history_model().query.count() == 0

tests/app/service/test_rest.py

@@ -181,7 +181,8 @@ def test_create_service(notify_api, sample_user):
                 'message_limit': 1000,
                 'restricted': False,
                 'active': False,
-                'email_from': 'created.service'}
+                'email_from': 'created.service',
+                'created_by': str(sample_user.id)}
             auth_header = create_authorization_header(
                 path='/service',
                 method='POST',
@@ -212,7 +213,7 @@ def test_create_service(notify_api, sample_user):
             assert json_resp['data']['name'] == 'created service'
 
 
-def test_should_not_create_service_with_missing_user_id_field(notify_api):
+def test_should_not_create_service_with_missing_user_id_field(notify_api, fake_uuid):
     with notify_api.test_request_context():
         with notify_api.test_client() as client:
             data = {
@@ -220,7 +221,8 @@ def test_should_not_create_service_with_missing_user_id_field(notify_api):
                 'name': 'created service',
                 'message_limit': 1000,
                 'restricted': False,
-                'active': False
+                'active': False,
+                'created_by': str(fake_uuid)
             }
             auth_header = create_authorization_header(
                 path='/service',
@@ -250,7 +252,8 @@ def test_should_not_create_service_with_missing_if_user_id_is_not_in_database(no
                 'name': 'created service',
                 'message_limit': 1000,
                 'restricted': False,
-                'active': False
+                'active': False,
+                'created_by': str(fake_uuid)
             }
             auth_header = create_authorization_header(
                 path='/service',
@@ -306,7 +309,8 @@ def test_should_not_create_service_with_duplicate_name(notify_api,
                 'message_limit': 1000,
                 'restricted': False,
                 'active': False,
-                'email_from': 'sample.service2'}
+                'email_from': 'sample.service2',
+                'created_by': str(sample_user.id)}
             auth_header = create_authorization_header(
                 path='/service',
                 method='POST',
@@ -324,7 +328,8 @@ def test_should_not_create_service_with_duplicate_name(notify_api,
 def test_create_service_should_throw_duplicate_key_constraint_for_existing_email_from(notify_api,
                                                                                       notify_db,
                                                                                       notify_db_session,
-                                                                                      service_factory):
+                                                                                      service_factory,
+                                                                                      sample_user):
     first_service = service_factory.get('First service', email_from='first.service')
     with notify_api.test_request_context():
         with notify_api.test_client() as client:
@@ -334,7 +339,8 @@ def test_create_service_should_throw_duplicate_key_constraint_for_existing_email
                 'message_limit': 1000,
                 'restricted': False,
                 'active': False,
-                'email_from': 'first.service'}
+                'email_from': 'first.service',
+                'created_by': str(sample_user.id)}
             auth_header = create_authorization_header(
                 path='/service',
                 method='POST',
@@ -550,7 +556,8 @@ def test_default_permissions_are_added_for_user_service(notify_api,
                 'message_limit': 1000,
                 'restricted': False,
                 'active': False,
-                'email_from': 'created.service'}
+                'email_from': 'created.service',
+                'created_by': str(sample_user.id)}
             auth_header = create_authorization_header(
                 path='/service',
                 method='POST',