From d14be2067c2db042a3027b703100fa63e93406a4 Mon Sep 17 00:00:00 2001
From: Adam Shimali <adam.shimali@gmail.com>
Date: Thu, 21 Apr 2016 18:10:57 +0100
Subject: [PATCH] When using the versioned decorator I noticed that when adding
 or revoking an api key the service it associated with was of course added to
 db.session.dirty.

That resulted in an updated version of service being added to the
service history table that showed no visible difference from that
record immediately precending it as the change was to another table,
namely the api_key table. A new api key or revoked api key was correctly
added to api_key and api_key_history tables. However I think an
'unchanged' service history record may be a bit confusing as you'd need
to correlate with api_keys to work out what the change was.

I think it's best to just record the new/revoked api_key and not create
another version of the service.

This pr wraps the exisiting versioned decorator with one that take a
class which you are interested in versioning.

Using the new decorator you only get a new version and history record
for the class you pass to outer decorator.

If the exising behaviour is acceptable to the powers that be then by all
means ignore/close this pr.
---
 app/dao/api_key_dao.py            |  4 ++--
 app/dao/dao_utils.py              | 23 +++++++++++++----------
 app/dao/services_dao.py           |  6 +++---
 tests/app/dao/test_api_key_dao.py | 15 +++++++++++++++
 4 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/app/dao/api_key_dao.py b/app/dao/api_key_dao.py
index ee64c0def..d44d16d00 100644
--- a/app/dao/api_key_dao.py
+++ b/app/dao/api_key_dao.py
@@ -7,12 +7,12 @@ from app.models import ApiKey
 
 from app.dao.dao_utils import (
     transactional,
-    versioned
+    version_class
 )
 
 
 @transactional
-@versioned
+@version_class(ApiKey)
 def save_model_api_key(api_key, update_dict={}):
     if update_dict:
         update_dict.pop('id', None)
diff --git a/app/dao/dao_utils.py b/app/dao/dao_utils.py
index d8f2791e9..e4753a1d1 100644
--- a/app/dao/dao_utils.py
+++ b/app/dao/dao_utils.py
@@ -18,13 +18,16 @@ def transactional(func):
     return commit_or_rollback
 
 
-def versioned(func):
-    @wraps(func)
-    def record_version(*args, **kwargs):
-        from app import db
-        func(*args, **kwargs)
-        history_objects = [create_history(obj) for obj in
-                           versioned_objects(itertools.chain(db.session.new, db.session.dirty))]
-        for h_obj in history_objects:
-            db.session.add(h_obj)
-    return record_version
+def version_class(model_class):
+    def versioned(func):
+        @wraps(func)
+        def record_version(*args, **kwargs):
+            from app import db
+            func(*args, **kwargs)
+            history_objects = [create_history(obj) for obj in
+                               versioned_objects(itertools.chain(db.session.new, db.session.dirty))
+                               if isinstance(obj, model_class)]
+            for h_obj in history_objects:
+                db.session.add(h_obj)
+        return record_version
+    return versioned
diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py
index b7f922a6a..4f89b1b16 100644
--- a/app/dao/services_dao.py
+++ b/app/dao/services_dao.py
@@ -6,7 +6,7 @@ from sqlalchemy import asc
 
 from app.dao.dao_utils import (
     transactional,
-    versioned
+    version_class
 )
 
 
@@ -27,7 +27,7 @@ def dao_fetch_service_by_id_and_user(service_id, user_id):
 
 
 @transactional
-@versioned
+@version_class(Service)
 def dao_create_service(service, user):
     from app.dao.permissions_dao import permission_dao
     service.users.append(user)
@@ -37,7 +37,7 @@ def dao_create_service(service, user):
 
 
 @transactional
-@versioned
+@version_class(Service)
 def dao_update_service(service):
     db.session.add(service)
 
diff --git a/tests/app/dao/test_api_key_dao.py b/tests/app/dao/test_api_key_dao.py
index a78f1babf..df81370e2 100644
--- a/tests/app/dao/test_api_key_dao.py
+++ b/tests/app/dao/test_api_key_dao.py
@@ -111,3 +111,18 @@ def test_should_not_allow_duplicate_key_names_per_service(notify_api,
         fail("should throw IntegrityError")
     except:
         pass
+
+
+def test_save_api_key_should_not_create_new_service_history(notify_api, notify_db, notify_db_session, sample_service):
+
+    from app.models import Service
+
+    assert Service.query.count() == 1
+    assert Service.get_history_model().query.count() == 1
+
+    api_key = ApiKey(**{'service': sample_service,
+                        'name': sample_service.name,
+                        'created_by': sample_service.created_by})
+    save_model_api_key(api_key)
+
+    assert Service.get_history_model().query.count() == 1