From eb55a659db75a21803a0a9bf66d50e6918152c87 Mon Sep 17 00:00:00 2001
From: Sean Hammond <seanhammond@lavabit.com>
Date: Wed, 21 Nov 2012 18:32:57 +0100
Subject: [PATCH 1/9] Refactor dashboard activity stream tests

- Move tests for contents of dashboard activity stream out of test_activity.py,
this test module was way too long and confusing, leave it for testing the
public activity streams only. Add a docstring to the module saying so.

- Add a docstring to test_follow.py explaining that it tests the follower
functions only (follow, unfollow, etc.) and not the contents of the dashboard
activity stream that is generated from what you're folllowing.

- Add new tests for the contents of the dashboard activity stream in
test_dashboard.py along with other dashboard tests.

Currently some of these tests are failing because activities from followed
groups are not appearing in the dashboard.
---
 ckan/tests/functional/api/test_activity.py  | 164 ++------------------
 ckan/tests/functional/api/test_dashboard.py | 142 +++++++++++++++--
 ckan/tests/functional/api/test_follow.py    |  12 ++
 3 files changed, 148 insertions(+), 170 deletions(-)

diff --git a/ckan/tests/functional/api/test_activity.py b/ckan/tests/functional/api/test_activity.py
index 4f7eff2a380..dfc14f9f6c4 100644
--- a/ckan/tests/functional/api/test_activity.py
+++ b/ckan/tests/functional/api/test_activity.py
@@ -1,3 +1,13 @@
+'''Functional tests for the public activity streams API.
+
+This module tests the contents of the various public activity streams:
+use activity streams, dataset activity streams, group activity streams, etc.
+
+This module _does not_ test the private user dashboard activity stream (which
+is different because the contents depend on what the user is following), that
+is tested in test_dashboard.py.
+
+'''
 import datetime
 import logging
 logger = logging.getLogger(__name__)
@@ -163,12 +173,6 @@ def setup_class(self):
                 'apikey': normal_user.apikey,
                 'name': normal_user.name,
                 }
-        follower = model.User.get('tester')
-        self.follower = {
-                'id': follower.id,
-                'apikey': follower.apikey,
-                'name': follower.name,
-                }
         warandpeace = model.Package.get('warandpeace')
         self.warandpeace = {
                 'id': warandpeace.id,
@@ -177,54 +181,14 @@ def setup_class(self):
         self.annakarenina = {
                 'id': annakarenina.id,
                 }
-        self.users = [self.sysadmin_user, self.normal_user, self.follower]
+        self.users = [self.sysadmin_user, self.normal_user]
         self.app = paste.fixture.TestApp(pylons.test.pylonsapp)
 
-        # Make follower follow everything else.
-        params = {'id': 'testsysadmin'}
-        extra_environ = {'Authorization': str(self.follower['apikey'])}
-        response = self.app.post('/api/action/follow_user',
-            params=json.dumps(params), extra_environ=extra_environ).json
-        assert response['success'] is True
-        params = {'id': 'annafan'}
-        extra_environ = {'Authorization': str(self.follower['apikey'])}
-        response = self.app.post('/api/action/follow_user',
-            params=json.dumps(params), extra_environ=extra_environ).json
-        assert response['success'] is True
-        params = {'id': 'warandpeace'}
-        extra_environ = {'Authorization': str(self.follower['apikey'])}
-        response = self.app.post('/api/action/follow_dataset',
-            params=json.dumps(params), extra_environ=extra_environ).json
-        assert response['success'] is True
-        params = {'id': 'annakarenina'}
-        extra_environ = {'Authorization': str(self.follower['apikey'])}
-        response = self.app.post('/api/action/follow_dataset',
-            params=json.dumps(params), extra_environ=extra_environ).json
-        assert response['success'] is True
-
-        self.followees = \
-            [
-                self.sysadmin_user['id'],
-                self.normal_user['id'],
-                self.follower['id'],
-                self.warandpeace['id'],
-                self.annakarenina['id']
-            ]
-
     @classmethod
     def teardown_class(self):
         import ckan.model as model
         model.repo.rebuild_db()
 
-    def dashboard_activity_stream(self, apikey):
-
-        response = self.app.get("/api/action/dashboard_activity_list",
-                json.dumps({}),
-                extra_environ={'Authorization': str(apikey)})
-        response_dict = json.loads(response.body)
-        assert response_dict['success'] is True
-        return response_dict['result']
-
     def user_activity_stream(self, user_id, apikey=None):
         if apikey:
             extra_environ = {'Authorization': str(apikey)}
@@ -290,28 +254,9 @@ def record_details(self, user_id, package_id=None, group_ids=None,
         details['recently changed datasets stream'] = \
                 self.recently_changed_datasets_stream(apikey)
 
-        details['user dashboard activity stream'] = (
-                self.dashboard_activity_stream(apikey))
-
-        details['follower dashboard activity stream'] = (
-                self.dashboard_activity_stream(self.follower['apikey']))
-
         details['time'] = datetime.datetime.now()
         return details
 
-    def check_dashboards(self, before, after, activity):
-        new_activities = [activity_ for activity_ in
-            after['user dashboard activity stream']
-            if activity_ not in before['user dashboard activity stream']]
-        assert [activity['id'] for activity in new_activities] == [
-                activity['id']]
-
-        new_activities = [activity_ for activity_ in
-            after['follower dashboard activity stream']
-            if activity_ not in before['follower dashboard activity stream']]
-        assert [activity['id'] for activity in new_activities] == [
-                activity['id']]
-
     def _create_package(self, user, name=None):
         if user:
             user_id = user['id']
@@ -358,31 +303,6 @@ def _create_package(self, user, name=None):
                 after['recently changed datasets stream'])
         assert new_rcd_activities == [activity]
 
-        # The new activity should appear in the user's dashboard activity
-        # stream.
-        new_activities = [activity_ for activity_ in
-            after['user dashboard activity stream']
-            if activity_ not in before['user dashboard activity stream']]
-        # There will be other new activities besides the 'follow dataset' one
-        # because all the dataset's old activities appear in the user's
-        # dashboard when she starts to follow the dataset.
-        assert activity['id'] in [
-                activity['id'] for activity in new_activities]
-
-        # The new activity should appear in the user "follower"'s dashboard
-        # activity stream because she follows all the other users and datasets.
-        new_activities = [activity_ for activity_ in
-            after['follower dashboard activity stream']
-            if activity_ not in before['follower dashboard activity stream']]
-        # There will be other new activities besides the 'follow dataset' one
-        # because all the dataset's old activities appear in the user's
-        # dashboard when she starts to follow the dataset.
-        assert [activity['id'] for activity in new_activities] == [
-                activity['id']]
-
-        # The same new activity should appear on the dashboard's of the user's
-        # followers.
-
         # The same new activity should appear in the activity streams of the
         # package's groups.
         for group_dict in package_created['groups']:
@@ -497,8 +417,6 @@ def _add_resource(self, package, user):
                 after['group activity streams'][group_dict['name']])
             assert grp_new_activities == [activity]
 
-        self.check_dashboards(before, after, activity)
-
         # Check that the new activity has the right attributes.
         assert activity['object_id'] == updated_package['id'], \
             str(activity['object_id'])
@@ -589,8 +507,6 @@ def _delete_extra(self, package_dict, user):
                 after['group activity streams'][group_dict['name']])
             assert grp_new_activities == [activity]
 
-        self.check_dashboards(before, after, activity)
-
         # Check that the new activity has the right attributes.
         assert activity['object_id'] == updated_package['id'], \
             str(activity['object_id'])
@@ -679,8 +595,6 @@ def _update_extra(self, package_dict, user):
                 after['recently changed datasets stream']) \
                         == user_new_activities
 
-        self.check_dashboards(before, after, activity)
-
         # If the package has any groups, the same new activity should appear
         # in the activity stream of each group.
         for group_dict in package_dict['groups']:
@@ -783,8 +697,6 @@ def _add_extra(self, package_dict, user, key=None):
                 after['group activity streams'][group_dict['name']])
             assert grp_new_activities == [activity]
 
-        self.check_dashboards(before, after, activity)
-
         # Check that the new activity has the right attributes.
         assert activity['object_id'] == updated_package['id'], \
             str(activity['object_id'])
@@ -845,8 +757,6 @@ def _create_activity(self, user, package, params):
             after['package activity stream']))
         assert pkg_new_activities == user_new_activities
 
-        self.check_dashboards(before, after, activity)
-
         # Check that the new activity has the right attributes.
         assert activity['object_id'] == params['object_id'], (
             str(activity['object_id']))
@@ -892,8 +802,6 @@ def _delete_group(self, group, user):
                         new_activities, ("The same activity should also "
                         "appear in the group's activity stream.")
 
-        self.check_dashboards(before, after, activity)
-
         # Check that the new activity has the right attributes.
         assert activity['object_id'] == group['id'], str(activity['object_id'])
         assert activity['user_id'] == user['id'], str(activity['user_id'])
@@ -937,8 +845,6 @@ def _update_group(self, group, user):
                         new_activities, ("The same activity should also "
                         "appear in the group's activity stream.")
 
-        self.check_dashboards(before, after, activity)
-
         # Check that the new activity has the right attributes.
         assert activity['object_id'] == group['id'], str(activity['object_id'])
         assert activity['user_id'] == user['id'], str(activity['user_id'])
@@ -986,8 +892,6 @@ def _update_user(self, user):
             "the user's activity stream, but found %i" % len(new_activities))
         activity = new_activities[0]
 
-        self.check_dashboards(before, after, activity)
-
         # Check that the new activity has the right attributes.
         assert activity['object_id'] == user_dict['id'], (
                 str(activity['object_id']))
@@ -1054,8 +958,6 @@ def _delete_resources(self, package):
                 after['group activity streams'][group_dict['name']])
             assert grp_new_activities == [activity]
 
-        self.check_dashboards(before, after, activity)
-
         # Check that the new activity has the right attributes.
         assert activity['object_id'] == package['id'], (
             str(activity['object_id']))
@@ -1132,8 +1034,6 @@ def _update_package(self, package, user):
                 after['recently changed datasets stream']) \
                         == user_new_activities
 
-        self.check_dashboards(before, after, activity)
-
         # If the package has any groups, the same new activity should appear
         # in the activity stream of each group.
         for group_dict in package['groups']:
@@ -1211,8 +1111,6 @@ def _update_resource(self, package, resource, user):
                 after['recently changed datasets stream']) \
                         == user_new_activities
 
-        self.check_dashboards(before, after, activity)
-
         # If the package has any groups, the same new activity should appear
         # in the activity stream of each group.
         for group_dict in package['groups']:
@@ -1289,8 +1187,6 @@ def _delete_package(self, package):
                 after['recently changed datasets stream']) \
                         == user_new_activities
 
-        self.check_dashboards(before, after, activity)
-
         # If the package has any groups, the same new activity should appear
         # in the activity stream of each group.
         for group_dict in package['groups']:
@@ -1406,8 +1302,6 @@ def test_01_remove_tag(self):
                 after['recently changed datasets stream']) \
                         == user_new_activities
 
-        self.check_dashboards(before, after, activity)
-
         # If the package has any groups, the same new activity should appear
         # in the activity stream of each group.
         for group_dict in pkg_dict['groups']:
@@ -1618,8 +1512,6 @@ def test_create_group(self):
                 new_activities, ("The same activity should also appear in "
                 "the group's activity stream.")
 
-        self.check_dashboards(before, after, activity)
-
         # Check that the new activity has the right attributes.
         assert activity['object_id'] == group_created['id'], \
             str(activity['object_id'])
@@ -1692,8 +1584,6 @@ def test_add_tag(self):
                 after['recently changed datasets stream']) \
                         == user_new_activities
 
-        self.check_dashboards(before, after, activity)
-
         # If the package has any groups, the same new activity should appear
         # in the activity stream of each group.
         for group_dict in pkg_dict['groups']:
@@ -2119,28 +2009,6 @@ def test_follow_dataset(self):
         for activity in user_new_activities:
             assert activity in pkg_new_activities
 
-        # The new activity should appear in the user's dashboard activity
-        # stream.
-        new_activities = [activity_ for activity_ in
-            after['user dashboard activity stream']
-            if activity_ not in before['user dashboard activity stream']]
-        # There will be other new activities besides the 'follow dataset' one
-        # because all the dataset's old activities appear in the user's
-        # dashboard when she starts to follow the dataset.
-        assert activity['id'] in [
-                activity['id'] for activity in new_activities]
-
-        # The new activity should appear in the user "follower"'s dashboard
-        # activity stream because she follows all the other users and datasets.
-        new_activities = [activity_ for activity_ in
-            after['follower dashboard activity stream']
-            if activity_ not in before['follower dashboard activity stream']]
-        # There will be other new activities besides the 'follow dataset' one
-        # because all the dataset's old activities appear in the user's
-        # dashboard when she starts to follow the dataset.
-        assert [activity['id'] for activity in new_activities] == [
-                activity['id']]
-
         # Check that the new activity has the right attributes.
         assert activity['object_id'] == self.warandpeace['id'], \
             str(activity['object_id'])
@@ -2182,16 +2050,6 @@ def test_follow_user(self):
             len(user_new_activities))
         activity = user_new_activities[0]
 
-        # Check that the new activity appears in the user's private activity
-        # stream.
-        user_new_activities = (find_new_activities(
-            before['follower dashboard activity stream'],
-            after['follower dashboard activity stream']))
-        assert len(user_new_activities) == 1, ("There should be 1 new "
-            " activity in the user's activity stream, but found %i" %
-            len(user_new_activities))
-        assert user_new_activities[0]['id'] == activity['id']
-
         # Check that the new activity has the right attributes.
         assert activity['object_id'] == self.sysadmin_user['id'], \
             str(activity['object_id'])
diff --git a/ckan/tests/functional/api/test_dashboard.py b/ckan/tests/functional/api/test_dashboard.py
index 1d7c291c200..6cef63beb21 100644
--- a/ckan/tests/functional/api/test_dashboard.py
+++ b/ckan/tests/functional/api/test_dashboard.py
@@ -1,3 +1,10 @@
+'''Test for the dashboard API.
+
+This module tests the various functions of the user dashboard, such as the
+contents of the dashboard activity stream and reporting the number of new
+activities.
+
+'''
 import ckan
 from ckan.lib.helpers import json
 import paste
@@ -92,6 +99,16 @@ def test_00_dashboard_new_activities_count_not_logged_in(self):
     def test_00_dashboard_mark_new_activities_not_logged_in(self):
         self.post('dashboard_mark_all_new_activities_as_old', status=403)
 
+    def test_01_dashboard_activity_list_for_new_user(self):
+        '''Test the contents of a new user's dashboard activity stream.'''
+        activities = self.dashboard_activity_list(self.new_user)
+        # We expect to find a single 'new user' activity.
+        assert len(activities) == 1
+        activity = activities[0]
+        assert activity['activity_type'] == 'new user'
+        assert activity['user_id'] == activity['object_id']
+        assert activity['user_id'] == self.new_user['id']
+
     def test_01_new_activities_count_for_new_user(self):
         '''Test that a newly registered user's new activities count is 0.'''
         assert self.dashboard_new_activities_count(self.new_user) == 0
@@ -151,14 +168,38 @@ def test_02_own_activities_do_not_count_as_new(self):
         # User's own actions should not increase her activity count.
         assert self.dashboard_new_activities_count(self.new_user) == 0
 
+    def test_03_dashboard_activity_list_own_activities(self):
+        '''Test that a user's own activities appear in her dashboard.'''
+        activities = self.dashboard_activity_list(self.new_user)
+
+        # FIXME: There should actually be 6 activities here, but when you
+        # follow something it's old activities (from before you followed it)
+        # appear in your activity stream. So here we get one more activity
+        # than expected.
+        assert len(activities) == 7
+
+        assert activities[0]['activity_type'] == 'changed package'
+        assert activities[1]['activity_type'] == 'follow group'
+        assert activities[2]['activity_type'] == 'follow user'
+        assert activities[3]['activity_type'] == 'follow dataset'
+        assert activities[4]['activity_type'] == 'new package'
+        assert activities[5]['activity_type'] == 'new user'
+
+        # FIXME: Shouldn't need the [:6] here, it's because when you follow
+        # something its old activities (from before you started following it)
+        # appear in your dashboard.
+        for activity in activities[:6]:
+            assert activity['user_id'] == self.new_user['id']
+
     def test_03_own_activities_not_marked_as_new(self):
         '''Make a user do some activities and check that her own activities
         aren't marked as new in her dashboard activity stream.'''
         assert len(self.dashboard_new_activities(self.new_user)) == 0
 
-    def test_04_new_activities_count(self):
-        '''Test that new activities from objects that a user follows increase
-        her new activities count.'''
+    def test_04_activities_from_followed_datasets(self):
+        '''Activities from followed datasets should show in dashboard.'''
+
+        activities_before = self.dashboard_activity_list(self.new_user)
 
         # Make someone else who new_user is not following update a dataset that
         # new_user is following.
@@ -167,34 +208,101 @@ def test_04_new_activities_count(self):
                 extra_environ={'Authorization': str(self.joeadmin['apikey'])})
         assert response.json['success'] is True
 
+        # Check the new activity in new_user's dashboard.
+        activities = self.dashboard_activity_list(self.new_user)
+        new_activities = [activity for activity in activities
+                if activity not in activities_before]
+        assert len(new_activities) == 1
+        activity = new_activities[0]
+        assert activity['activity_type'] == 'changed package'
+        assert activity['user_id'] == self.joeadmin['id']
+        assert activity['data']['package']['name'] == 'warandpeace'
+
+    def test_04_activities_from_followed_users(self):
+        '''Activities from followed users should show in the dashboard.'''
+
+        activities_before = self.dashboard_activity_list(self.new_user)
+
         # Make someone that the user is following create a new dataset.
         params = json.dumps({'name': 'annas_new_dataset'})
         response = self.app.post('/api/action/package_create', params=params,
                 extra_environ={'Authorization': str(self.annafan['apikey'])})
         assert response.json['success'] is True
 
-        # Make someone that the user is not following update a dataset that
-        # the user is not following, but that belongs to a group that the user
-        # is following.
+        # Check the new activity in new_user's dashboard.
+        activities = self.dashboard_activity_list(self.new_user)
+        new_activities = [activity for activity in activities
+                if activity not in activities_before]
+        assert len(new_activities) == 1
+        activity = new_activities[0]
+        assert activity['activity_type'] == 'new package'
+        assert activity['user_id'] == self.annafan['id']
+        assert activity['data']['package']['name'] == 'annas_new_dataset'
+
+    def test_04_activities_from_followed_groups(self):
+        '''Activities from followed groups should show in the dashboard.'''
+
+        activities_before = self.dashboard_activity_list(self.new_user)
+
+        # Make someone that the user is not following update a group that the
+        # user is following.
+        params = json.dumps({'id': 'roger', 'description': 'updated'})
+        response = self.app.post('/api/action/group_update', params=params,
+            extra_environ={'Authorization': str(self.testsysadmin['apikey'])})
+        assert response.json['success'] is True
+
+        # Check the new activity in new_user's dashboard.
+        activities = self.dashboard_activity_list(self.new_user)
+        new_activities = [activity for activity in activities
+                if activity not in activities_before]
+        assert len(new_activities) == 1
+        activity = new_activities[0]
+        assert activity['activity_type'] == 'changed group'
+        assert activity['user_id'] == self.testsysadmin['id']
+        assert activity['data']['group']['name'] == 'roger'
+
+    def test_04_activities_from_datasets_of_followed_groups(self):
+        '''Activities from datasets of followed groups should show in the
+        dashboard.
+
+        '''
+        activities_before = self.dashboard_activity_list(self.new_user)
+
+        # Make someone that the user is not following update a dataset that the
+        # user is not following either, but that belongs to a group that the
+        # user is following.
         params = json.dumps({'name': 'annakarenina', 'notes': 'updated'})
         response = self.app.post('/api/action/package_update', params=params,
             extra_environ={'Authorization': str(self.testsysadmin['apikey'])})
         assert response.json['success'] is True
 
-        # FIXME: The number here should be 3 but activities from followed
-        # groups are not appearing in dashboard. When that is fixed, fix this
-        # number.
-        assert self.dashboard_new_activities_count(self.new_user) == 2
+        # Check the new activity in new_user's dashboard.
+        activities = self.dashboard_activity_list(self.new_user)
+        new_activities = [activity for activity in activities
+                if activity not in activities_before]
+        assert len(new_activities) == 1
+        activity = new_activities[0]
+        assert activity['activity_type'] == 'changed package'
+        assert activity['user_id'] == self.testsysadmin['id']
+        assert activity['data']['package']['name'] == 'annakarenina'
+
+    def test_05_new_activities_count(self):
+        '''Test that new activities from objects that a user follows increase
+        her new activities count.'''
+        # FIXME: The number here should be 4 but activities from datasets of
+        # followed groups are not appearing in dashboard. When that is fixed,
+        # fix this number.
+        assert self.dashboard_new_activities_count(self.new_user) == 3
 
-    def test_05_activities_marked_as_new(self):
+    def test_06_activities_marked_as_new(self):
         '''Test that new activities from objects that a user follows are
         marked as new in her dashboard activity stream.'''
-        # FIXME: The number here should be 3 but activities from followed
-        # groups are not appearing in dashboard. When that is fixed, fix this
-        # number.
-        assert len(self.dashboard_new_activities(self.new_user)) == 2
+        # FIXME: The number here should be 4 but activities from datasets of
+        # followed groups are not appearing in dashboard. When that is fixed,
+        # fix this number.
+        assert len(self.dashboard_new_activities(self.new_user)) == 3
 
-    def test_06_mark_new_activities_as_read(self):
+    def test_07_mark_new_activities_as_read(self):
         '''Test that a user's new activities are marked as old when she views
         her dashboard activity stream.'''
         assert self.dashboard_new_activities_count(self.new_user) > 0
@@ -203,7 +311,7 @@ def test_06_mark_new_activities_as_read(self):
         assert self.dashboard_new_activities_count(self.new_user) == 0
         assert len(self.dashboard_new_activities(self.new_user)) == 0
 
-    def test_07_maximum_number_of_new_activities(self):
+    def test_08_maximum_number_of_new_activities(self):
         '''Test that the new activities count does not go higher than 15, even
         if there are more than 15 new activities from the user's followers.'''
         for n in range(0, 20):
diff --git a/ckan/tests/functional/api/test_follow.py b/ckan/tests/functional/api/test_follow.py
index ab0df832f48..a084c930fd9 100644
--- a/ckan/tests/functional/api/test_follow.py
+++ b/ckan/tests/functional/api/test_follow.py
@@ -1,3 +1,15 @@
+'''Test for the follower API.
+
+This module tests following, unfollowing, getting a list of what you're
+following or the number of things you're following, getting a list of who's
+following you or the number of followers you have, testing whether or not
+you're following something, etc.
+
+This module _does not_ test the user dashboard activity stream (which shows
+activities from everything you're following), that is tested in
+test_dashboard.py.
+
+'''
 import datetime
 import paste
 import pylons.test

From 0235f1a812cd879c3f540807afb6a462f3ccecf3 Mon Sep 17 00:00:00 2001
From: Sean Hammond <seanhammond@lavabit.com>
Date: Wed, 21 Nov 2012 18:34:39 +0100
Subject: [PATCH 2/9] Add activities from followed groups to dashboard activity
 stream

Add activities from groups that a user is following (e.g. when someone
updates a group) to the user's dashboard activity stream.

There are still some test_dashboard.py tests failing because activities
from datasets belonging to followed groups dob't appear in the dashboard
yet.
---
 ckan/model/activity.py | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/ckan/model/activity.py b/ckan/model/activity.py
index 9181acaa20d..f0f706a8df3 100644
--- a/ckan/model/activity.py
+++ b/ckan/model/activity.py
@@ -151,9 +151,19 @@ def _activities_from_datasets_followed_by_user_query(user_id):
     return q
 
 
+def _activities_from_groups_followed_by_user_query(user_id):
+    import ckan.model as model
+    q = model.Session.query(model.Activity)
+    q = q.join(model.UserFollowingGroup,
+            model.UserFollowingGroup.object_id == model.Activity.object_id)
+    q = q.filter(model.UserFollowingGroup.follower_id == user_id)
+    return q
+
+
 def _activities_from_everything_followed_by_user_query(user_id):
     q = _activites_from_users_followed_by_user_query(user_id)
     q = q.union(_activities_from_datasets_followed_by_user_query(user_id))
+    q = q.union(_activities_from_groups_followed_by_user_query(user_id))
     return q
 
 

From 5847bd00065b4fc92121432d57a002fc1fd688dd Mon Sep 17 00:00:00 2001
From: Sean Hammond <seanhammond@lavabit.com>
Date: Wed, 21 Nov 2012 20:24:29 +0100
Subject: [PATCH 3/9] Move group_package_show SQLAlchemy into model

Move the SQLAlchemy query that the group_package_show() action function
uses into the model. I need this for architectural reasons for upcoming
commits, and we're supposed to encapsulate SQLAlchemy in the model
anyway.

I made the new model function support the 'return_query' option but note
that there are no tests covering this, all the tests pass even without
this option.
---
 ckan/logic/action/get.py | 28 ++++++++--------------------
 ckan/model/group.py      | 27 +++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/ckan/logic/action/get.py b/ckan/logic/action/get.py
index 146cd899b86..8bfb4f5095d 100644
--- a/ckan/logic/action/get.py
+++ b/ckan/logic/action/get.py
@@ -750,34 +750,22 @@ def group_package_show(context, data_dict):
     :rtype: list of dictionaries
 
     '''
-    model = context["model"]
-    user = context["user"]
-    id = _get_or_bust(data_dict, 'id')
-    limit = data_dict.get("limit")
+    model = context['model']
+    group_id = _get_or_bust(data_dict, 'id')
 
-    group = model.Group.get(id)
+    # FIXME: What if limit is not an int? Schema and validation needed.
+    limit = data_dict.get('limit')
+
+    group = model.Group.get(group_id)
     context['group'] = group
     if group is None:
         raise NotFound
 
     _check_access('group_show', context, data_dict)
 
-    query = model.Session.query(model.PackageRevision)\
-        .filter(model.PackageRevision.state=='active')\
-        .filter(model.PackageRevision.current==True)\
-        .join(model.Member, model.Member.table_id==model.PackageRevision.id)\
-        .join(model.Group, model.Group.id==model.Member.group_id)\
-        .filter_by(id=group.id)\
-        .order_by(model.PackageRevision.name)
-
-    if limit:
-        query = query.limit(limit)
-
-    if context.get('return_query'):
-        return query
-
     result = []
-    for pkg_rev in query.all():
+    for pkg_rev in group.get_package_revisions(limit=limit,
+            return_query=context.get('return_query')):
         result.append(model_dictize.package_dictize(pkg_rev, context))
 
     return result
diff --git a/ckan/model/group.py b/ckan/model/group.py
index 62767671538..b0227c05203 100644
--- a/ckan/model/group.py
+++ b/ckan/model/group.py
@@ -199,6 +199,33 @@ def active_packages(self, load_eager=True, with_private=False):
 
         return query
 
+    def get_package_revisions(self, limit=None, return_query=False):
+        '''Return a group's packages.
+
+        :param limit: the maximum number of packages to return
+        :type limit: int
+
+        :returns: a list of the group's packages, sorted by name
+        :rtype: list of PackageRevision objects
+
+        '''
+        import ckan.model as model
+        q = model.Session.query(model.PackageRevision)
+        q = q.filter(model.PackageRevision.state == 'active')
+        q = q.filter(model.PackageRevision.current == True)
+        q = q.join(model.Member,
+                model.Member.table_id == model.PackageRevision.id)
+        q = q.join(model.Group, model.Group.id == model.Member.group_id)
+        q = q.filter_by(id=self.id)
+        q = q.order_by(model.PackageRevision.name)
+        if limit is not None:
+            q = q.limit(limit)
+        if return_query:
+            return q
+        else:
+            return q.all()
+
+
     @classmethod
     def search_by_name_or_title(cls, text_query, group_type=None):
         text_query = text_query.strip().lower()

From 8f5c897c06633b1f2e7cb7d7cd78573fac5fb57d Mon Sep 17 00:00:00 2001
From: Sean Hammond <seanhammond@lavabit.com>
Date: Wed, 21 Nov 2012 21:04:02 +0100
Subject: [PATCH 4/9] Remove group.members_of_type()

Remove group.members_of_type() and related code. It isn't used anywhere
except in the old publisher and organizations extensions which are to be
replaced by a new organizations implementation in CKAN core. Also it
duplicates functionality provided by group.active_packages().
---
 ckan/model/group.py | 42 +-----------------------------------------
 1 file changed, 1 insertion(+), 41 deletions(-)

diff --git a/ckan/model/group.py b/ckan/model/group.py
index b0227c05203..5692e48e0cc 100644
--- a/ckan/model/group.py
+++ b/ckan/model/group.py
@@ -143,35 +143,6 @@ def set_approval_status(self, status):
         if status == "denied":
             pass
 
-    def members_of_type(self, object_type, capacity=None):
-        from ckan import model
-        object_type_string = object_type.__name__.lower()
-        query = meta.Session.query(object_type).\
-            filter(model.Group.id == self.id).\
-            filter(model.Member.state == 'active').\
-            filter(model.Member.table_name == object_type_string)
-
-        if hasattr(object_type, 'state'):
-            query = query.filter(object_type.state == 'active')
-
-        if capacity:
-            query = query.filter(model.Member.capacity == capacity)
-
-        query = query.join(model.Member, member_table.c.table_id ==
-                           getattr(object_type, 'id')).\
-            join(model.Group, group_table.c.id == member_table.c.group_id)
-
-        return query
-
-    def add_child(self, object_instance):
-        object_type_string = object_instance.__class__.__name__.lower()
-        if not object_instance in self.members_of_type(
-                object_instance.__class__).all():
-            member = Member(group=self,
-                            table_id=getattr(object_instance, 'id'),
-                            table_name=object_type_string)
-            meta.Session.add(member)
-
     def get_children_groups(self, type='group'):
         # Returns a list of dicts where each dict contains "id", "name",
         # and "title" When querying with a CTE specifying a model in the
@@ -236,23 +207,12 @@ def search_by_name_or_title(cls, text_query, group_type=None):
             q = q.filter(cls.type == group_type)
         return q.order_by(cls.title)
 
-    def as_dict(self, ref_package_by='name'):
-        _dict = domain_object.DomainObject.as_dict(self)
-        _dict['packages'] = [getattr(package, ref_package_by)
-                             for package in self.packages]
-        _dict['extras'] = dict([(key, value) for key, value
-                                in self.extras.items()])
-        if (self.type == 'organization'):
-            _dict['users'] = [getattr(user, "name")
-                              for user in self.members_of_type(_user.User)]
-        return _dict
-
     def add_package_by_name(self, package_name):
         if not package_name:
             return
         package = _package.Package.by_name(package_name)
         assert package
-        if not package in self.members_of_type(package.__class__).all():
+        if not package in self.active_packages().all():
             member = Member(group=self, table_id=package.id,
                             table_name='package')
             meta.Session.add(member)

From bdc72d7abd41fbdad5a5a30c8468ce6ea370ebc1 Mon Sep 17 00:00:00 2001
From: Sean Hammond <seanhammond@lavabit.com>
Date: Wed, 21 Nov 2012 23:36:17 +0100
Subject: [PATCH 5/9] Remove duplicate methods to get a group's packages

Refactor active_packages() and get_package_revisions(), both of which
return a group's packages (but in slightly different ways), replace with
just one method packages().

We now have just one way to get a group's packages, the packages()
method of the group model. The group_package_show() action function
calls it.
---
 ckan/controllers/group.py                 |  2 +-
 ckan/lib/dictization/model_dictize.py     |  2 +-
 ckan/logic/action/get.py                  |  2 +-
 ckan/model/group.py                       | 63 ++++++++++++-----------
 ckan/tests/functional/test_group.py       | 18 +++----
 ckan/tests/lib/test_dictization_schema.py |  2 +-
 ckan/tests/models/test_group.py           |  4 +-
 7 files changed, 47 insertions(+), 46 deletions(-)

diff --git a/ckan/controllers/group.py b/ckan/controllers/group.py
index d6bffcc8be0..1867079529b 100644
--- a/ckan/controllers/group.py
+++ b/ckan/controllers/group.py
@@ -342,7 +342,7 @@ def _force_reindex(self, grp):
         appearing on the read page for the group (as they're connected via
         the group name)'''
         group = model.Group.get(grp['name'])
-        for dataset in group.active_packages().all():
+        for dataset in group.packages():
             search.rebuild(dataset.name)
 
     def _save_edit(self, id, context):
diff --git a/ckan/lib/dictization/model_dictize.py b/ckan/lib/dictization/model_dictize.py
index 8a08ff50b03..6926e4a3395 100644
--- a/ckan/lib/dictization/model_dictize.py
+++ b/ckan/lib/dictization/model_dictize.py
@@ -32,7 +32,7 @@ def group_list_dictize(obj_list, context,
         group_dict['display_name'] = obj.display_name
 
         group_dict['packages'] = \
-                len(obj.active_packages(with_private=with_private).all())
+                len(obj.packages(with_private=with_private))
 
         if context.get('for_view'):
             for item in plugins.PluginImplementations(
diff --git a/ckan/logic/action/get.py b/ckan/logic/action/get.py
index 8bfb4f5095d..f1fc99b05a7 100644
--- a/ckan/logic/action/get.py
+++ b/ckan/logic/action/get.py
@@ -764,7 +764,7 @@ def group_package_show(context, data_dict):
     _check_access('group_show', context, data_dict)
 
     result = []
-    for pkg_rev in group.get_package_revisions(limit=limit,
+    for pkg_rev in group.packages(limit=limit,
             return_query=context.get('return_query')):
         result.append(model_dictize.package_dictize(pkg_rev, context))
 
diff --git a/ckan/model/group.py b/ckan/model/group.py
index 5692e48e0cc..35dfb73b461 100644
--- a/ckan/model/group.py
+++ b/ckan/model/group.py
@@ -155,47 +155,48 @@ def get_children_groups(self, type='group'):
         return [{"id":idf, "name": name, "title": title}
                 for idf, name, title in results]
 
-    def active_packages(self, load_eager=True, with_private=False):
-        query = meta.Session.query(_package.Package).\
-            filter_by(state=vdm.sqlalchemy.State.ACTIVE).\
-            filter(group_table.c.id == self.id).\
-            filter(member_table.c.state == 'active')
+    def packages(self, with_private=False, limit=None,
+            return_query=False):
+        '''Return this group's active and pending packages.
 
-        if not with_private:
-            query = query.filter(member_table.c.capacity == 'public')
-
-        query = query.join(member_table, member_table.c.table_id ==
-                           _package.Package.id).\
-            join(group_table, group_table.c.id == member_table.c.group_id)
-
-        return query
+        Returns all packages in this group with VDM revision state ACTIVE or
+        PENDING.
 
-    def get_package_revisions(self, limit=None, return_query=False):
-        '''Return a group's packages.
+        :param with_private: if True, include the group's private packages
+        :type with_private: boolean
 
         :param limit: the maximum number of packages to return
         :type limit: int
 
-        :returns: a list of the group's packages, sorted by name
-        :rtype: list of PackageRevision objects
+        :param return_query: if True, return the SQLAlchemy query object
+            instead of the list of Packages resulting from the query
+        :type return_query: boolean
+
+        :returns: a list of this group's packages
+        :rtype: list of ckan.model.package.Package objects
 
         '''
-        import ckan.model as model
-        q = model.Session.query(model.PackageRevision)
-        q = q.filter(model.PackageRevision.state == 'active')
-        q = q.filter(model.PackageRevision.current == True)
-        q = q.join(model.Member,
-                model.Member.table_id == model.PackageRevision.id)
-        q = q.join(model.Group, model.Group.id == model.Member.group_id)
-        q = q.filter_by(id=self.id)
-        q = q.order_by(model.PackageRevision.name)
+        query = meta.Session.query(_package.Package)
+        query = query.filter(
+                or_(_package.Package.state == vdm.sqlalchemy.State.ACTIVE,
+                    _package.Package.state == vdm.sqlalchemy.State.PENDING))
+        query = query.filter(group_table.c.id == self.id)
+
+        if not with_private:
+            query = query.filter(member_table.c.capacity == 'public')
+
+        query = query.join(member_table,
+                member_table.c.table_id == _package.Package.id)
+        query = query.join(group_table,
+                group_table.c.id == member_table.c.group_id)
+
         if limit is not None:
-            q = q.limit(limit)
+            query = query.limit(limit)
+
         if return_query:
-            return q
+            return query
         else:
-            return q.all()
-
+            return query.all()
 
     @classmethod
     def search_by_name_or_title(cls, text_query, group_type=None):
@@ -212,7 +213,7 @@ def add_package_by_name(self, package_name):
             return
         package = _package.Package.by_name(package_name)
         assert package
-        if not package in self.active_packages().all():
+        if not package in self.packages():
             member = Member(group=self, table_id=package.id,
                             table_name='package')
             meta.Session.add(member)
diff --git a/ckan/tests/functional/test_group.py b/ckan/tests/functional/test_group.py
index 7bfc04aaadd..57e2c62574c 100644
--- a/ckan/tests/functional/test_group.py
+++ b/ckan/tests/functional/test_group.py
@@ -187,7 +187,7 @@ def test_index(self):
         groupname = 'david'
         group = model.Group.by_name(unicode(groupname))
         group_title = group.title
-        group_packages_count = len(group.active_packages().all())
+        group_packages_count = len(group.packages())
         group_description = group.description
         self.check_named_element(res, 'tr', group_title,
                                  group_packages_count,
@@ -321,7 +321,7 @@ def test_2_edit(self):
         assert group.description == newdesc, group
 
         # now look at datasets
-        assert len(group.active_packages().all()) == 3
+        assert len(group.packages()) == 3
 
     def test_3_edit_form_has_new_package(self):
         # check for dataset in autocomplete
@@ -369,7 +369,7 @@ def test_4_new_duplicate_package(self):
 
         # check package only added to the group once
         group = model.Group.by_name(group_name)
-        pkg_names = [pkg.name for pkg in group.active_packages().all()]
+        pkg_names = [pkg.name for pkg in group.packages()]
         assert_equal(pkg_names, [self.packagename])
 
     def test_edit_plugin_hook(self):
@@ -435,7 +435,7 @@ def update_group(res, name, with_pkg=True):
 
         # We have the datasets in the DB, but we should also see that many
         # on the group read page.
-        assert len(group.active_packages().all()) == 3
+        assert len(group.packages()) == 3
 
         offset = url_for(controller='group', action='read', id='newname')
         res = self.app.get(offset, status=200,
@@ -543,9 +543,9 @@ def test_2_new(self):
         group = model.Group.by_name(group_name)
         assert group.title == group_title, group
         assert group.description == group_description, group
-        assert len(group.active_packages().all()) == 1
+        assert len(group.packages()) == 1
         pkg = model.Package.by_name(self.packagename)
-        assert group.active_packages().all() == [pkg]
+        assert group.packages() == [pkg]
 
     def test_3_new_duplicate_group(self):
         prefix = ''
@@ -702,7 +702,7 @@ def test_index(self):
         groupname = 'david'
         group = model.Group.by_name(unicode(groupname))
         group_title = group.title
-        group_packages_count = len(group.active_packages().all())
+        group_packages_count = len(group.packages())
         group_description = group.description
         self.check_named_element(res, 'tr', group_title,
                                  group_packages_count,
@@ -820,7 +820,7 @@ def test_2_edit(self):
         assert group.description == newdesc, group
 
         # now look at datasets
-        assert len(group.active_packages().all()) == 3
+        assert len(group.packages()) == 3
 
     def test_3_edit_form_has_new_package(self):
         # check for dataset in autocomplete
@@ -868,7 +868,7 @@ def test_4_new_duplicate_package(self):
 
         # check package only added to the group once
         group = model.Group.by_name(group_name)
-        pkg_names = [pkg.name for pkg in group.active_packages().all()]
+        pkg_names = [pkg.name for pkg in group.packages()]
         assert_equal(pkg_names, [self.packagename])
 
     def test_edit_plugin_hook(self):
diff --git a/ckan/tests/lib/test_dictization_schema.py b/ckan/tests/lib/test_dictization_schema.py
index ad9fa290cf7..ef1391b98b8 100644
--- a/ckan/tests/lib/test_dictization_schema.py
+++ b/ckan/tests/lib/test_dictization_schema.py
@@ -148,7 +148,7 @@ def test_2_group_schema(self):
         del data['extras']
 
         converted_data, errors = validate(data, default_group_schema(), self.context)
-        group_pack = sorted(group.active_packages().all(), key=lambda x:x.id)
+        group_pack = sorted(group.packages(), key=lambda x:x.id)
 
         converted_data["packages"] = sorted(converted_data["packages"], key=lambda x:x["id"])
 
diff --git a/ckan/tests/models/test_group.py b/ckan/tests/models/test_group.py
index 703b63778f8..424e8f3c198 100644
--- a/ckan/tests/models/test_group.py
+++ b/ckan/tests/models/test_group.py
@@ -25,7 +25,7 @@ def test_1_basic(self):
         grp = model.Group.by_name(u'group1')
         assert grp.title == u'Test Group'
         assert grp.description == u'This is a test group'
-        assert grp.active_packages().all() == []
+        assert grp.packages() == []
 
     def test_2_add_packages(self):
         model.repo.new_revision()
@@ -50,7 +50,7 @@ def test_2_add_packages(self):
         assert grp.title == u'Russian Group'
         anna = model.Package.by_name(u'annakarenina')
         war = model.Package.by_name(u'warandpeace')
-        assert set(grp.active_packages().all()) == set((anna, war)), grp.active_packages().all()
+        assert set(grp.packages()) == set((anna, war)), grp.packages()
         assert grp in anna.get_groups()
 
     def test_3_search(self):

From e9a55a11c26e037da62b4230d54ee11611a431e4 Mon Sep 17 00:00:00 2001
From: Sean Hammond <seanhammond@lavabit.com>
Date: Thu, 22 Nov 2012 16:48:34 +0100
Subject: [PATCH 6/9] Fix following of groups

Add activities from the datasets of followed groups into the user's
dashboard activity stream. All the test_dashboard.py tests now pass.

Move SQLAlchemy from group_activity_list() into model, alongside the SQLAlchemy
queries for the other types of activity stream.

Change the SQLAlchemy in activities_from_groups_followed_by_user() to be the
union of group_activity_list() for each of the followed groups.

Update some tests.
---
 ckan/logic/action/get.py                    | 25 +--------------
 ckan/model/activity.py                      | 34 ++++++++++++++++++---
 ckan/tests/functional/api/test_dashboard.py | 16 +++-------
 3 files changed, 36 insertions(+), 39 deletions(-)

diff --git a/ckan/logic/action/get.py b/ckan/logic/action/get.py
index f1fc99b05a7..c85e3a7ece9 100644
--- a/ckan/logic/action/get.py
+++ b/ckan/logic/action/get.py
@@ -1767,30 +1767,7 @@ def group_activity_list(context, data_dict):
     group_show = logic.get_action('group_show')
     group_id = group_show(context, {'id': group_id})['id']
 
-    # FIXME: The SQLAlchemy below should be moved into ckan/model/activity.py
-    # (to encapsulate SQLALchemy in the model and avoid using it from the
-    # logic) but it can't be because it requires the list of dataset_ids which
-    # comes from logic.group_package_show() (and I don't want to access the
-    # logic from the model). Need to change it to get the dataset_ids from the
-    # model instead. There seems to be multiple methods for getting a group's
-    # datasets, some in the logic and some in the model.
-
-    # Get a list of the IDs of the group's datasets.
-    group_package_show = logic.get_action('group_package_show')
-    datasets = group_package_show(context, {'id': group_id})
-    dataset_ids = [dataset['id'] for dataset in datasets]
-
-    # Get the group's activities.
-    query = model.Session.query(model.Activity)
-    if dataset_ids:
-        query = query.filter(_or_(model.Activity.object_id == group_id,
-            model.Activity.object_id.in_(dataset_ids)))
-    else:
-        query = query.filter(model.Activity.object_id == group_id)
-    query = query.order_by(_desc(model.Activity.timestamp))
-    query = query.limit(15)
-    activity_objects = query.all()
-
+    activity_objects = model.activity.group_activity_list(group_id)
     return model_dictize.activity_list_dictize(activity_objects, context)
 
 def recently_changed_packages_activity_list(context, data_dict):
diff --git a/ckan/model/activity.py b/ckan/model/activity.py
index f0f706a8df3..23abbce1cd6 100644
--- a/ckan/model/activity.py
+++ b/ckan/model/activity.py
@@ -1,6 +1,6 @@
 import datetime
 
-from sqlalchemy import orm, types, Column, Table, ForeignKey, desc
+from sqlalchemy import orm, types, Column, Table, ForeignKey, desc, or_
 
 import meta
 import types as _types
@@ -133,6 +133,26 @@ def package_activity_list(package_id, limit=15):
     return _most_recent_activities(q, limit)
 
 
+def _group_activity_query(group_id, limit=15):
+    import ckan.model as model
+
+    group = model.Group.get(group_id)
+    dataset_ids = [dataset.id for dataset in group.packages()]
+
+    q = model.Session.query(model.Activity)
+    if dataset_ids:
+        q = q.filter(or_(model.Activity.object_id == group_id,
+            model.Activity.object_id.in_(dataset_ids)))
+    else:
+        q = q.filter(model.Activity.object_id == group_id)
+    return q
+
+
+def group_activity_list(group_id, limit=15):
+    q = _group_activity_query(group_id)
+    return _most_recent_activities(q, limit)
+
+
 def _activites_from_users_followed_by_user_query(user_id):
     import ckan.model as model
     q = model.Session.query(model.Activity)
@@ -153,10 +173,16 @@ def _activities_from_datasets_followed_by_user_query(user_id):
 
 def _activities_from_groups_followed_by_user_query(user_id):
     import ckan.model as model
+
+    # Get a list of the group's that the user is following.
+    follower_objects = model.UserFollowingGroup.followee_list(user_id)
+    if not follower_objects:
+        # Return a query with no results.
+        return model.Session.query(model.Activity).filter("0=1")
+
     q = model.Session.query(model.Activity)
-    q = q.join(model.UserFollowingGroup,
-            model.UserFollowingGroup.object_id == model.Activity.object_id)
-    q = q.filter(model.UserFollowingGroup.follower_id == user_id)
+    q = q.union_all(*[_group_activity_query(follower.object_id)
+            for follower in follower_objects])
     return q
 
 
diff --git a/ckan/tests/functional/api/test_dashboard.py b/ckan/tests/functional/api/test_dashboard.py
index 6cef63beb21..0a5803e7cff 100644
--- a/ckan/tests/functional/api/test_dashboard.py
+++ b/ckan/tests/functional/api/test_dashboard.py
@@ -174,9 +174,9 @@ def test_03_dashboard_activity_list_own_activities(self):
 
         # FIXME: There should actually be 6 activities here, but when you
         # follow something it's old activities (from before you followed it)
-        # appear in your activity stream. So here we get one more activity
-        # than expected.
-        assert len(activities) == 7
+        # appear in your activity stream. So here we get more activities than
+        # expected.
+        assert len(activities) == 8
 
         assert activities[0]['activity_type'] == 'changed package'
         assert activities[1]['activity_type'] == 'follow group'
@@ -289,18 +289,12 @@ def test_04_activities_from_datasets_of_followed_groups(self):
     def test_05_new_activities_count(self):
         '''Test that new activities from objects that a user follows increase
         her new activities count.'''
-        # FIXME: The number here should be 4 but activities from datasets of
-        # followed groups are not appearing in dashboard. When that is fixed,
-        # fix this number.
-        assert self.dashboard_new_activities_count(self.new_user) == 3
+        assert self.dashboard_new_activities_count(self.new_user) == 4
 
     def test_06_activities_marked_as_new(self):
         '''Test that new activities from objects that a user follows are
         marked as new in her dashboard activity stream.'''
-        # FIXME: The number here should be 4 but activities from datasets of
-        # followed groups are not appearing in dashboard. When that is fixed,
-        # fix this number.
-        assert len(self.dashboard_new_activities(self.new_user)) == 3
+        assert len(self.dashboard_new_activities(self.new_user)) == 4
 
     def test_07_mark_new_activities_as_read(self):
         '''Test that a user's new activities are marked as old when she views

From 8e2f2a0baa703998337e3f035ab97064341cca4d Mon Sep 17 00:00:00 2001
From: Sean Hammond <seanhammond@lavabit.com>
Date: Thu, 22 Nov 2012 17:41:20 +0100
Subject: [PATCH 7/9] Add docstring to group_activity_list()

---
 ckan/model/activity.py | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/ckan/model/activity.py b/ckan/model/activity.py
index 23abbce1cd6..47ef8b6e7bf 100644
--- a/ckan/model/activity.py
+++ b/ckan/model/activity.py
@@ -149,6 +149,16 @@ def _group_activity_query(group_id, limit=15):
 
 
 def group_activity_list(group_id, limit=15):
+    '''Return the given group's public activity stream.
+
+    Returns all activities where the given group or one of its datasets is the
+    object of the activity, e.g.:
+
+    "{USER} updated the group {GROUP}"
+    "{USER} updated the dataset {DATASET}"
+    etc.
+
+    '''
     q = _group_activity_query(group_id)
     return _most_recent_activities(q, limit)
 

From 3a8c37c41efa548a0f59f475e01c748a3a0669d6 Mon Sep 17 00:00:00 2001
From: Sean Hammond <seanhammond@lavabit.com>
Date: Mon, 26 Nov 2012 14:10:31 +0100
Subject: [PATCH 8/9] Add some docstrings to model/activity.py

---
 ckan/model/activity.py | 46 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 41 insertions(+), 5 deletions(-)

diff --git a/ckan/model/activity.py b/ckan/model/activity.py
index 47ef8b6e7bf..92017619cdc 100644
--- a/ckan/model/activity.py
+++ b/ckan/model/activity.py
@@ -69,6 +69,7 @@ def __init__(self, activity_id, object_id, object_type, activity_type,
 
 
 def _most_recent_activities(q, limit):
+    '''Return the 'limit' most recent activites from activity query 'q'.'''
     import ckan.model as model
     q = q.order_by(desc(model.Activity.timestamp))
     if limit:
@@ -77,6 +78,7 @@ def _most_recent_activities(q, limit):
 
 
 def _activities_from_user_query(user_id):
+    '''Return an SQLAlchemy query for all activities from user_id.'''
     import ckan.model as model
     q = model.Session.query(model.Activity)
     q = q.filter(model.Activity.user_id == user_id)
@@ -84,6 +86,7 @@ def _activities_from_user_query(user_id):
 
 
 def _activities_about_user_query(user_id):
+    '''Return an SQLAlchemy query for all activities about user_id.'''
     import ckan.model as model
     q = model.Session.query(model.Activity)
     q = q.filter(model.Activity.object_id == user_id)
@@ -91,16 +94,17 @@ def _activities_about_user_query(user_id):
 
 
 def _user_activity_query(user_id):
+    '''Return an SQLAlchemy query for all activities from or about user_id.'''
     q = _activities_from_user_query(user_id)
     q = q.union(_activities_about_user_query(user_id))
     return q
 
 
 def user_activity_list(user_id, limit=15):
-    '''Return the given user's public activity stream.
+    '''Return user_id's public activity stream.
 
-    Returns all activities from or about the given user, i.e. where the given
-    user is the subject or object of the activity, e.g.:
+    Return a list of all activities from or about the given user, i.e. where
+    the given user is the subject or object of the activity, e.g.:
 
     "{USER} created the dataset {DATASET}"
     "{OTHER_USER} started following {USER}"
@@ -112,6 +116,9 @@ def user_activity_list(user_id, limit=15):
 
 
 def _package_activity_query(package_id):
+    '''Return an SQLAlchemy query for all activities about package_id.
+
+    '''
     import ckan.model as model
     q = model.Session.query(model.Activity)
     q = q.filter_by(object_id=package_id)
@@ -134,6 +141,12 @@ def package_activity_list(package_id, limit=15):
 
 
 def _group_activity_query(group_id, limit=15):
+    '''Return an SQLAlchemy query for all activities about group_id.
+
+    Returns a query for all activities whose object is either the group itself
+    or one of the group's datasets.
+
+    '''
     import ckan.model as model
 
     group = model.Group.get(group_id)
@@ -164,6 +177,7 @@ def group_activity_list(group_id, limit=15):
 
 
 def _activites_from_users_followed_by_user_query(user_id):
+    '''Return a query for all activities from users that user_id follows.'''
     import ckan.model as model
     q = model.Session.query(model.Activity)
     q = q.join(model.UserFollowingUser,
@@ -173,6 +187,7 @@ def _activites_from_users_followed_by_user_query(user_id):
 
 
 def _activities_from_datasets_followed_by_user_query(user_id):
+    '''Return a query for all activities from datasets that user_id follows.'''
     import ckan.model as model
     q = model.Session.query(model.Activity)
     q = q.join(model.UserFollowingDataset,
@@ -182,6 +197,13 @@ def _activities_from_datasets_followed_by_user_query(user_id):
 
 
 def _activities_from_groups_followed_by_user_query(user_id):
+    '''Return a query for all activities about groups the given user follows.
+
+    Return a query for all activities about the groups the given user follows,
+    or about any of the group's datasets. This is the union of
+    _group_activity_query(group_id) for each of the groups the user follows.
+
+    '''
     import ckan.model as model
 
     # Get a list of the group's that the user is following.
@@ -197,6 +219,7 @@ def _activities_from_groups_followed_by_user_query(user_id):
 
 
 def _activities_from_everything_followed_by_user_query(user_id):
+    '''Return a query for all activities from everything user_id follows.'''
     q = _activites_from_users_followed_by_user_query(user_id)
     q = q.union(_activities_from_datasets_followed_by_user_query(user_id))
     q = q.union(_activities_from_groups_followed_by_user_query(user_id))
@@ -215,6 +238,7 @@ def activities_from_everything_followed_by_user(user_id, limit=15):
 
 
 def _dashboard_activity_query(user_id):
+    '''Return an SQLAlchemy query for user_id's dashboard activity stream.'''
     q = _user_activity_query(user_id)
     q = q.union(_activities_from_everything_followed_by_user_query(user_id))
     return q
@@ -234,7 +258,13 @@ def dashboard_activity_list(user_id, limit=15):
     return _most_recent_activities(q, limit)
 
 
-def _recently_changed_packages_activity_query():
+def _changed_packages_activity_query():
+    '''Return an SQLAlchemyu query for all changed package activities.
+
+    Return a query for all activities with activity_type '*package', e.g.
+    'new_package', 'changed_package', 'deleted_package'.
+
+    '''
     import ckan.model as model
     q = model.Session.query(model.Activity)
     q = q.filter(model.Activity.activity_type.endswith('package'))
@@ -242,5 +272,11 @@ def _recently_changed_packages_activity_query():
 
 
 def recently_changed_packages_activity_list(limit=15):
-    q = _recently_changed_packages_activity_query()
+    '''Return the site-wide stream of recently changed package activities.
+
+    This activity stream includes recent 'new package', 'changed package' and
+    'deleted package' activities for the whole site.
+
+    '''
+    q = _changed_packages_activity_query()
     return _most_recent_activities(q, limit)

From a96757e1cd082f6c449ec5cb9eb4a60c4b610d84 Mon Sep 17 00:00:00 2001
From: Sean Hammond <seanhammond@lavabit.com>
Date: Mon, 26 Nov 2012 14:38:08 +0100
Subject: [PATCH 9/9] Fix dashboard activity stream from followed groups

It was returning all activities from the entire site, fix to return
activities from followed groups only.

The tests didn't catch this error because they didn't test doing random
activities that should _not_ appear in the user's dashboard and
asserting that they don't.

Add a quick test that catches this.

This is probably a problem for the other activity streams tests in
test_activity.py as well.
---
 ckan/model/activity.py                      |  4 ++--
 ckan/tests/functional/api/test_dashboard.py | 15 +++++++++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/ckan/model/activity.py b/ckan/model/activity.py
index 92017619cdc..6d9970e09ce 100644
--- a/ckan/model/activity.py
+++ b/ckan/model/activity.py
@@ -212,9 +212,9 @@ def _activities_from_groups_followed_by_user_query(user_id):
         # Return a query with no results.
         return model.Session.query(model.Activity).filter("0=1")
 
-    q = model.Session.query(model.Activity)
+    q = _group_activity_query(follower_objects[0].object_id)
     q = q.union_all(*[_group_activity_query(follower.object_id)
-            for follower in follower_objects])
+        for follower in follower_objects[1:]])
     return q
 
 
diff --git a/ckan/tests/functional/api/test_dashboard.py b/ckan/tests/functional/api/test_dashboard.py
index 0a5803e7cff..1b285a222c3 100644
--- a/ckan/tests/functional/api/test_dashboard.py
+++ b/ckan/tests/functional/api/test_dashboard.py
@@ -316,3 +316,18 @@ def test_08_maximum_number_of_new_activities(self):
                 extra_environ={'Authorization': str(self.joeadmin['apikey'])})
             assert response.json['success'] is True
         assert self.dashboard_new_activities_count(self.new_user) == 15
+
+    def test_09_activities_that_should_not_show(self):
+        '''Test that other activities do not appear on the user's dashboard.'''
+
+        before = self.dashboard_activity_list(self.new_user)
+
+        # Make someone else who new_user is not following create a new dataset.
+        params = json.dumps({'name': 'irrelevant_dataset'})
+        response = self.app.post('/api/action/package_create', params=params,
+            extra_environ={'Authorization': str(self.testsysadmin['apikey'])})
+        assert response.json['success'] is True
+
+        after = self.dashboard_activity_list(self.new_user)
+
+        assert before == after