Get user ID for activity streams from revision

When creating Activity objects for activity streams, get the user_id
from the revision, not from the pylons context. It's not good to access
the global pylons context from the model module, e.g. because when unit
tests do things through the logic module directly (instead of through
API calls) then the pylons context will not be present.

Also change activity_test.py, can now test for correct user IDs.

ckan/lib/activity.py

@@ -2,9 +2,9 @@
 import logging
 logger = logging.getLogger(__name__)
 
-def activity_stream_item(obj, activity_type, revision_id):
+def activity_stream_item(obj, activity_type, revision):
     try:
-        return obj.activity_stream_item(activity_type, revision_id)
+        return obj.activity_stream_item(activity_type, revision)
     except (AttributeError, TypeError):
         logger.debug("Object did not have a suitable "
             "activity_stream_item() method, it must not be a package.")
@@ -46,7 +46,7 @@ def before_commit(self, session):
         logger.debug("Looking for new packages...")
         for obj in obj_cache['new']:
             logger.debug("Looking at object %s" % obj)
-            activity = activity_stream_item(obj, 'new', revision.id)
+            activity = activity_stream_item(obj, 'new', revision)
             if activity is None:
                 continue
             # If the object returns an activity stream item we know that the
@@ -86,7 +86,7 @@ def before_commit(self, session):
                         activity = activities[package.id]
                     else:
                         activity = activity_stream_item(package, "changed",
-                                revision.id)
+                                revision)
                         activities[package.id] = activity
                     assert activity is not None
                     logger.debug("activity: %s" % activity)

ckan/model/package.py

@@ -537,30 +537,18 @@ def get_fields(core_only=False, fields_to_ignore=None):
 
         return fields
 
-    def activity_stream_item(self, activity_type, revision_id):
-        try:
-            user_id = c.user_obj.id
-        except TypeError:
-            # Cannot access user ID through Pylons context, try to get their IP
-            # address instead.
-            try:
-                user_id = request.environ.get('REMOTE_ADDR', 'Unknown IP Address')
-            except TypeError:
-                # Cannot access user IP address through Pylons request,
-                # fallback on a default value.
-                user_id = 'Unknown IP Address'
-        logger.debug("user_id: %s" % user_id)
+    def activity_stream_item(self, activity_type, revision):
         assert activity_type in ("new", "changed", "deleted"), \
             str(activity_type)
+        user_id = revision.user.id
         if activity_type == "new":
-            return Activity(user_id, self.id, revision_id, "new package",
-                None)
+            return Activity(user_id, self.id, revision.id, "new package", None)
         elif activity_type == "changed":
-            return Activity(user_id, self.id, revision_id, "changed package",
-                None)
+            return Activity(user_id, self.id, revision.id, "changed package",
+                    None)
         elif activity_type == "deleted":
-            return Activity(user_id, self.id, revision_id, "deleted package",
-                None)
+            return Activity(user_id, self.id, revision.id, "deleted package",
+                    None)
 
     def activity_stream_detail(self, activity_id, activity_type):
         return ActivityDetail(activity_id, self.id, u"Package", activity_type, 

ckan/tests/models/test_activity.py

@@ -96,7 +96,8 @@ def test_create_package(self):
         activity = activities[-1]
         assert activity.object_id == package_created['id'], \
             str(activity.object_id)
-        assert activity.user_id == "Unknown IP Address", str(activity.user_id)
+        assert activity.user_id == TestActivity.normal_user.id, \
+            str(activity.user_id)
         assert activity.activity_type == 'new package', \
             str(activity.activity_type)
         if not activity.id:
@@ -170,7 +171,8 @@ def _add_resource(self, package):
         activity = activities[-1]
         assert activity.object_id == updated_package['id'], \
             str(activity.object_id)
-        assert activity.user_id == "Unknown IP Address", str(activity.user_id)
+        assert activity.user_id == TestActivity.normal_user.id, \
+            str(activity.user_id)
         assert activity.activity_type == 'changed package', \
             str(activity.activity_type)
         if not activity.id:
@@ -240,7 +242,8 @@ def _update_package(self, package):
                 len(activities)))
         activity = activities[-1]
         assert activity.object_id == package.id, str(activity.object_id)
-        assert activity.user_id == "Unknown IP Address", str(activity.user_id)
+        assert activity.user_id == TestActivity.normal_user.id, \
+            str(activity.user_id)
         assert activity.activity_type == 'changed package', \
             str(activity.activity_type)
         if not activity.id:
@@ -305,7 +308,8 @@ def _update_resource(self, package, resource):
         assert len(activities) == length_before + 1, str(activities)
         activity = activities[-1]
         assert activity.object_id == package.id, str(activity.object_id)
-        assert activity.user_id == "Unknown IP Address", str(activity.user_id)
+        assert activity.user_id == TestActivity.normal_user.id, \
+            str(activity.user_id)
         assert activity.activity_type == 'changed package', \
             str(activity.activity_type)
         if not activity.id:
@@ -371,7 +375,8 @@ def _delete_package(self, package):
                 len(activities)))
         activity = activities[-1]
         assert activity.object_id == package.id, str(activity.object_id)
-        assert activity.user_id == "Unknown IP Address", str(activity.user_id)
+        assert activity.user_id == TestActivity.normal_user.id, \
+            str(activity.user_id)
         # "Deleted" packages actually show up as changed (the package's status
         # changes to "deleted" but the package is not expunged).
         assert activity.activity_type == 'changed package', \
@@ -441,7 +446,8 @@ def _delete_resources(self, package):
                 len(activities)))
         activity = activities[-1]
         assert activity.object_id == package.id, str(activity.object_id)
-        assert activity.user_id == "Unknown IP Address", str(activity.user_id)
+        assert activity.user_id == TestActivity.normal_user.id, \
+            str(activity.user_id)
         assert activity.activity_type == 'changed package', \
             str(activity.activity_type)
         if not activity.id: