Fix activity streams when no user logged in

Still get the user's ID from the revision, but fall back on 'not logged
in' if there is no user object in the revision. Put this logic in
lib.activity.

Add user_id param to lib.activity.activity_stream_item() and
model.package.Package.activity_stream_item().

Add a debug message when we skip an SQLAlchemy commit because it has no
_object_cache or no revision.

Add test cases for adding and updating packages and resources when not logged in.

ckan/lib/activity.py

@@ -2,9 +2,9 @@
 import logging
 logger = logging.getLogger(__name__)
 
-def activity_stream_item(obj, activity_type, revision):
+def activity_stream_item(obj, activity_type, revision, user_id):
     try:
-        return obj.activity_stream_item(activity_type, revision)
+        return obj.activity_stream_item(activity_type, revision, user_id)
     except (AttributeError, TypeError):
         logger.debug("Object did not have a suitable "
             "activity_stream_item() method, it must not be a package.")
@@ -25,11 +25,22 @@ def before_commit(self, session):
         session.flush()
 
         try:
-            obj_cache = session._object_cache
+            object_cache = session._object_cache
             revision = session.revision
         except AttributeError:
+            logger.debug('session had no _object_cache or no revision,'
+                    ' skipping this commit', exc_info=True)
             return
 
+        if revision.user:
+            user_id = revision.user.id
+        else:
+            # If the user is not logged in then revision.user is None and
+            # revision.author is their IP address. Just log them as 'not logged
+            # in' rather than logging their IP address.
+            user_id = 'not logged in'
+        logger.debug('user_id: %s' % user_id)
+
         # The top-level objects that we will append to the activity table. The
         # keys here are package IDs, and the values are model.activity:Activity
         # objects.
@@ -44,9 +55,9 @@ def before_commit(self, session):
         # Log new packages first to prevent them from getting incorrectly
         # logged as changed packages.
         logger.debug("Looking for new packages...")
-        for obj in obj_cache['new']:
+        for obj in object_cache['new']:
             logger.debug("Looking at object %s" % obj)
-            activity = activity_stream_item(obj, 'new', revision)
+            activity = activity_stream_item(obj, 'new', revision, user_id)
             if activity is None:
                 continue
             # If the object returns an activity stream item we know that the
@@ -63,7 +74,7 @@ def before_commit(self, session):
         # Now process other objects.
         logger.debug("Looking for other objects...")
         for activity_type in ('new', 'changed', 'deleted'):
-            objects = obj_cache[activity_type]
+            objects = object_cache[activity_type]
             for obj in objects:
                 logger.debug("Looking at %s object %s" % (activity_type, obj))
                 if activity_type == "new" and obj.id in activities:
@@ -86,7 +97,7 @@ def before_commit(self, session):
                         activity = activities[package.id]
                     else:
                         activity = activity_stream_item(package, "changed",
-                                revision)
+                                revision, user_id)
                         activities[package.id] = activity
                     assert activity is not None
                     logger.debug("activity: %s" % activity)

ckan/model/package.py

@@ -537,10 +537,9 @@ def get_fields(core_only=False, fields_to_ignore=None):
 
         return fields
 
-    def activity_stream_item(self, activity_type, revision):
+    def activity_stream_item(self, activity_type, revision, user_id):
         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)
         elif activity_type == "changed":

ckan/tests/models/test_activity.py

@@ -20,21 +20,19 @@ def _make_resource():
 
 class TestActivity:
 
-    @classmethod
-    def setup_class(cls):
+    def setUp(self):
         ckan.tests.CreateTestData.create()
-        cls.sysadmin_user = model.User.get('testsysadmin')
-        cls.normal_user = model.User.get('annafan')
+        self.sysadmin_user = model.User.get('testsysadmin')
+        self.normal_user = model.User.get('annafan')
 
-    @classmethod
-    def tearDownClass(cls):
+    def tearDown(self):
+        ckan.tests.CreateTestData.delete()
         model.repo.rebuild_db()
-        model.Session.remove()
 
     def _make_test_package(self):
         """Return a test package in dictionary form."""
         # A package with no resources, tags, extras or groups.
-        pkg1 = {
+        pkg = {
             'name' : 'test_package',
             'title' : 'My Test Package',
             'author' : 'test author',
@@ -57,30 +55,27 @@ def _make_test_package(self):
                 'format': 'PDF',
                 'name': 'another example resource',
             }
-        pkg1['resources'] = [res1, res2]
+        pkg['resources'] = [res1, res2]
         # Add some tags to the package.
         tag1 = { 'name': 'a_test_tag' }
         tag2 = { 'name': 'another_test_tag' }
-        pkg1['tags'] = [tag1, tag2]
-        return pkg1
+        pkg['tags'] = [tag1, tag2]
+        return pkg
 
-    def test_create_package(self):
-        """
-        Test new package activity stream.
-
-        Test that correct activity stream item and detail items are emitted
-        when a new package is created.
-
-        """
+    def _create_package(self, user):
         # Record some details before creating a new package.
         length_before = len(model.Session.query(model.activity.Activity).all())
         details_length_before = len(model.Session.query(
             model.activity.ActivityDetail).all())
         before = datetime.datetime.now()
 
         # Create a new package.
-        context = {'model': model, 'session': model.Session,
-                'user':TestActivity.normal_user.name}
+        if user is None:
+            context = {'model': model, 'session': model.Session,
+                    'user':'127.0.0.1'}
+        else:
+            context = {'model': model, 'session': model.Session,
+                    'user':user.name}
         request_data = self._make_test_package()
         package_created = package_create(context, request_data)
 
@@ -96,8 +91,11 @@ def test_create_package(self):
         activity = activities[-1]
         assert activity.object_id == package_created['id'], \
             str(activity.object_id)
-        assert activity.user_id == TestActivity.normal_user.id, \
-            str(activity.user_id)
+        if user:
+            user_id = user.id
+        else:
+            user_id = 'not logged in'
+        assert activity.user_id == user_id, str(activity.user_id)
         assert activity.activity_type == 'new package', \
             str(activity.activity_type)
         if not activity.id:
@@ -132,7 +130,27 @@ def test_create_package(self):
                     "package or any of its resources: %s" \
                     % str(detail.object_id))
 
-    def _add_resource(self, package):
+    def test_create_package(self):
+        """
+        Test new package activity stream.
+
+        Test that correct activity stream item and detail items are emitted
+        when a new package is created.
+
+        """
+        self._create_package(user=self.normal_user)
+
+    def test_create_package_not_logged_in(self):
+        """
+        Test new package activity stream when not logged in.
+
+        Test that correct activity stream item and detail items are emitted
+        when a new package is created by a user who is not logged in.
+
+        """
+        self._create_package(user=None)
+
+    def _add_resource(self, package, user):
         # Record some details before creating a new resource.
         length_before = len(model.Session.query(model.activity.Activity).all())
         details_length_before = len(model.Session.query(
@@ -146,8 +164,12 @@ def _add_resource(self, package):
         resource_ids_before = [resource.id for resource in package.resources]
 
         # Create a new resource.
-        context = {'model': model, 'session': model.Session,
-                'user':TestActivity.normal_user.name}
+        if user is None:
+            context = {'model': model, 'session': model.Session,
+                    'user':'127.0.0.1'}
+        else:
+            context = {'model': model, 'session': model.Session,
+                    'user':user.name}
         resources = resource_list_dictize(package.resources, context)
         resources.append(_make_resource())
         request_data = {
@@ -171,8 +193,11 @@ def _add_resource(self, package):
         activity = activities[-1]
         assert activity.object_id == updated_package['id'], \
             str(activity.object_id)
-        assert activity.user_id == TestActivity.normal_user.id, \
-            str(activity.user_id)
+        if user:
+            user_id = user.id
+        else:
+            user_id = 'not logged in'
+        assert activity.user_id == user_id, str(activity.user_id)
         assert activity.activity_type == 'changed package', \
             str(activity.activity_type)
         if not activity.id:
@@ -207,9 +232,20 @@ def test_add_resources(self):
 
         """
         for package in model.Session.query(model.Package).all():
-            self._add_resource(package)
+            self._add_resource(package, user=self.normal_user)
 
-    def _update_package(self, package):
+    def test_add_resources_not_logged_in(self):
+        """
+        Test new resource activity stream when no user logged in.
+
+        Test that correct activity stream item and detail items are emitted
+        when a resource is added to a package by a user who is not logged in.
+
+        """
+        for package in model.Session.query(model.Package).all():
+            self._add_resource(package, user=None)
+
+    def _update_package(self, package, user):
         """
         Update the given package and test that the correct activity stream
         item and detail are emitted.
@@ -227,7 +263,7 @@ def _update_package(self, package):
 
         # Update the package.
         context = {'model': model, 'session': model.Session,
-                'user':TestActivity.normal_user.name,
+                'user':self.normal_user.name,
                 'allow_partial_update':True}
         package_dict = {'id':package.id, 'title':'edited'}
         result = package_update(context, package_dict)
@@ -242,7 +278,7 @@ def _update_package(self, package):
                 len(activities)))
         activity = activities[-1]
         assert activity.object_id == package.id, str(activity.object_id)
-        assert activity.user_id == TestActivity.normal_user.id, \
+        assert activity.user_id == self.normal_user.id, \
             str(activity.user_id)
         assert activity.activity_type == 'changed package', \
             str(activity.activity_type)
@@ -274,9 +310,20 @@ def test_update_package(self):
 
         """
         for package in model.Session.query(model.Package).all():
-            self._update_package(package)
+            self._update_package(package, user=self.normal_user)
 
-    def _update_resource(self, package, resource):
+    def test_update_package_not_logged_in(self):
+        """
+        Test updated package activity stream when not logged in.
+
+        Test that correct activity stream item and detail items are created
+        when packages are updated by a user who is not logged in.
+
+        """
+        for package in model.Session.query(model.Package).all():
+            self._update_package(package, user=None)
+
+    def _update_resource(self, package, resource, user):
         """
         Update the given resource and test that the correct activity stream
         item and detail are emitted.
@@ -294,9 +341,12 @@ def _update_resource(self, package, resource):
         resource = model.Session.query(model.Resource).get(resource.id)
 
         # Update the resource.
-        context = {'model': model, 'session': model.Session,
-                'user':TestActivity.normal_user.name,
-                'allow_partial_update':True}
+        if user is None:
+            context = {'model': model, 'session': model.Session,
+                    'user':'127.0.0.1', 'allow_partial_update':True}
+        else:
+            context = {'model': model, 'session': model.Session,
+                    'user':user.name, 'allow_partial_update':True}
         resource_dict = {'id':resource.id, 'name':'edited'}
         result = resource_update(context, resource_dict)
 
@@ -308,8 +358,11 @@ 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 == TestActivity.normal_user.id, \
-            str(activity.user_id)
+        if user:
+            user_id = user.id
+        else:
+            user_id = 'not logged in'
+        assert activity.user_id == user_id, str(activity.user_id)
         assert activity.activity_type == 'changed package', \
             str(activity.activity_type)
         if not activity.id:
@@ -341,7 +394,21 @@ def test_update_resource(self):
             # it belongs to may have been closed.
             pkg = model.Session.query(model.Package).get(package.id)
             for resource in pkg.resources:
-                self._update_resource(pkg, resource)
+                self._update_resource(pkg, resource, user=self.normal_user)
+
+    def test_update_resource_not_logged_in(self):
+        """
+        Test that a correct activity stream item and detail item are emitted
+        when a resource is updated by a user who is not logged in.
+
+        """
+        packages = model.Session.query(model.Package).all()
+        for package in packages:
+            # Query the model for the Package object again, as the session that
+            # it belongs to may have been closed.
+            pkg = model.Session.query(model.Package).get(package.id)
+            for resource in pkg.resources:
+                self._update_resource(pkg, resource, user=None)
 
     def _delete_package(self, package):
         """
@@ -361,7 +428,7 @@ def _delete_package(self, package):
 
         # Delete the package.
         context = {'model': model, 'session': model.Session,
-                'user':TestActivity.normal_user.name}
+                'user':self.normal_user.name}
         package_dict = {'id':package.id}
         result = package_delete(context, package_dict)
 
@@ -375,7 +442,7 @@ def _delete_package(self, package):
                 len(activities)))
         activity = activities[-1]
         assert activity.object_id == package.id, str(activity.object_id)
-        assert activity.user_id == TestActivity.normal_user.id, \
+        assert activity.user_id == self.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).
@@ -432,7 +499,7 @@ def _delete_resources(self, package):
 
         # Delete the resources.
         context = {'model': model, 'session': model.Session,
-                'user':TestActivity.normal_user.name}
+                'user':self.normal_user.name}
         data_dict = { 'id':package.id, 'resources':[] }
         result = package_update(context, data_dict)
 
@@ -446,7 +513,7 @@ def _delete_resources(self, package):
                 len(activities)))
         activity = activities[-1]
         assert activity.object_id == package.id, str(activity.object_id)
-        assert activity.user_id == TestActivity.normal_user.id, \
+        assert activity.user_id == self.normal_user.id, \
             str(activity.user_id)
         assert activity.activity_type == 'changed package', \
             str(activity.activity_type)