From 78c6bb45d74f0b5cc317433ceeb78f87e2bfac60 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= <sybren@stuvel.eu>
Date: Wed, 12 Oct 2016 18:17:49 +0200
Subject: [PATCH] Better activity logging of task changes

---
 attract/subquery.py        |  6 +++---
 attract/tasks/__init__.py  |  3 +--
 attract/tasks/eve_hooks.py | 43 ++++++++++++++++++++++++++++++++++----
 3 files changed, 43 insertions(+), 9 deletions(-)

diff --git a/attract/subquery.py b/attract/subquery.py
index 5c55f75..c5f2597 100644
--- a/attract/subquery.py
+++ b/attract/subquery.py
@@ -15,15 +15,15 @@ def get_user_info(user_id):
     for authenticated & non-authenticated users, which is why we're
     allowed to cache it globally.
 
-    Returns None when the user cannot be found.
+    Returns an empty dict when the user cannot be found.
     """
 
     if user_id is None:
-        return None
+        return {}
 
     user = pillarsdk.User.find(user_id, api=pillar_api())
     if not user:
-        return user
+        return {}
 
     return {'email': user.email,
             'full_name': user.full_name}
diff --git a/attract/tasks/__init__.py b/attract/tasks/__init__.py
index acc08f3..767e915 100644
--- a/attract/tasks/__init__.py
+++ b/attract/tasks/__init__.py
@@ -78,8 +78,7 @@ class TaskManager(object):
         task.properties.task_type = fields.pop('task_type', '').strip() or None
 
         users = fields.pop('users', None)
-        if users:
-            task.properties.assigned_to = {'users': users}
+        task.properties.assigned_to = {'users': users or []}
 
         self._log.info('Saving task %s', task.to_dict())
 
diff --git a/attract/tasks/eve_hooks.py b/attract/tasks/eve_hooks.py
index 94295ae..30d2a81 100644
--- a/attract/tasks/eve_hooks.py
+++ b/attract/tasks/eve_hooks.py
@@ -1,14 +1,16 @@
 import logging
 
 import itertools
-from flask import current_app
+from flask import current_app, g
 
-from attract.node_types.task import node_type_task
 from pillar.api.nodes import only_for_node_type_decorator
 import pillar.api.activities
 import pillar.api.utils.authentication
 import pillar.web.jinja
 
+from attract.node_types.task import node_type_task
+from attract import subquery
+
 log = logging.getLogger(__name__)
 only_for_task = only_for_node_type_decorator(node_type_task['name'])
 
@@ -90,6 +92,13 @@ def _parent_name(task):
 
 def register_task_activity(task, descr):
     user_id = pillar.api.utils.authentication.current_user_id()
+    if not user_id:
+        import flask
+
+        log.error('Unable to register task activity %r for task %s: user_id=%s',
+                  descr, task['_id'], user_id)
+        return
+
     pillar.api.activities.register_activity(
         user_id,
         descr + _parent_name(task),
@@ -100,6 +109,23 @@ def register_task_activity(task, descr):
     )
 
 
+def get_user_list(user_list):
+    if not user_list:
+        return u'-nobody-'
+
+    user_coll = current_app.db()['users']
+    users = user_coll.find({
+            '_id': {'$in': user_list}
+        },
+        projection={
+            'full_name': 1,
+        }
+    )
+
+    names = [user['full_name'] for user in users]
+    return u', '.join(names)
+
+
 @only_for_task
 def activity_after_replacing_task(task, original):
     # Compare to original, and either mention the things that changed,
@@ -112,10 +138,19 @@ def activity_after_replacing_task(task, original):
 
     if len(changes) == 1:
         (key, val_task, _) = changes[0]
-        human_key = key.rsplit('.', 1)[-1]
+        human_key = pillar.web.jinja.format_undertitle(key.rsplit('.', 1)[-1])
+        descr = None
+
+        # Some key-specific overrides
         if key == 'properties.status':
             val_task = pillar.web.jinja.format_undertitle(val_task)
-        descr = 'changed %s to %s in task "%s"' % (human_key, val_task, task['name'])
+        elif key == 'properties.assigned_to.users':
+            human_key = 'assigned users'
+            val_task = get_user_list(val_task)
+            descr = 'assigned task "%s" to %s' % (task['name'], val_task)
+
+        if descr is None:
+            descr = 'changed %s to %s in task "%s"' % (human_key, val_task, task['name'])
     else:
         descr = 'edited task "%s"' % task['name']