From ac6d81fb37bae61911ecb839c9c46c42449dde18 Mon Sep 17 00:00:00 2001 From: John Martin Date: Wed, 14 Nov 2012 13:16:09 +0000 Subject: [PATCH 01/16] IE fixes for top account dropdown --- ckan/public/base/less/iehacks.less | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ckan/public/base/less/iehacks.less b/ckan/public/base/less/iehacks.less index 3dd0f3be9df..f64464a0140 100644 --- a/ckan/public/base/less/iehacks.less +++ b/ckan/public/base/less/iehacks.less @@ -68,6 +68,8 @@ .masthead .account a.image { display: block; width: 25px; + padding-right: 10px; + white-space: nowrap; } } From 367aee938d9efdd0841cfb903de0a3fb53525108 Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Wed, 14 Nov 2012 17:01:49 +0100 Subject: [PATCH 02/16] [#3009] Rename migration script Clashed with another branch --- .../{061_add_dashboard_table.py => 062_add_dashboard_table.py} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename ckan/migration/versions/{061_add_dashboard_table.py => 062_add_dashboard_table.py} (100%) diff --git a/ckan/migration/versions/061_add_dashboard_table.py b/ckan/migration/versions/062_add_dashboard_table.py similarity index 100% rename from ckan/migration/versions/061_add_dashboard_table.py rename to ckan/migration/versions/062_add_dashboard_table.py From cb3984e594768ecef113517a168b8b42f631df24 Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Wed, 14 Nov 2012 17:02:45 +0100 Subject: [PATCH 03/16] [#3009] Refactor activity streams SQLAlchemy queries This is necessary so that we can get the number of new activities in the user's dashboard activity stream, not counting activities from the user herself. Rewrite the activity streams queries in a building blocks fashion. For example: _user_activity_query(user_id) Returns a query for all activities from or about a given user, used for the user's public activity stream. _activities_from_everything_followed_by_user_query(user_id) Returns a query from all activities from everything the given user follows. _dashboard_activity_query(user_id) Used for the user's dashboard, returns the union of _user_activity_query(user_id) and _activities_from_everything_followed_by_user_query(user_id). Now we can call _dashboard_activity_query(user_id) to get the activities for the user's dashboard, and we can call _activities_from_everything_followed_by_user_query(user_id) to get the number of new activities on the user's dashboard, not counting the activities from the user herself. Also move all the activity streams SQLAlchemy queries out of logic/action/get.py (except for the group's activity stream which is problematic) and into model/activity.py, because I want to encapsulate SQLAlchemy in the model and avoid using it from the logic. This also groups all the activity streams queries together in the code and makes them easier to understand and build on. --- ckan/logic/action/get.py | 49 +++++--------- ckan/model/activity.py | 136 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 152 insertions(+), 33 deletions(-) diff --git a/ckan/logic/action/get.py b/ckan/logic/action/get.py index 5b8f0607b08..0c85d77fa6f 100644 --- a/ckan/logic/action/get.py +++ b/ckan/logic/action/get.py @@ -1735,11 +1735,7 @@ def user_activity_list(context, data_dict): _check_access('user_show', context, data_dict) model = context['model'] user_id = _get_or_bust(data_dict, 'id') - query = model.Session.query(model.Activity) - query = query.filter_by(user_id=user_id) - query = query.order_by(_desc(model.Activity.timestamp)) - query = query.limit(15) - activity_objects = query.all() + activity_objects = model.activity.user_activity_list(user_id) return model_dictize.activity_list_dictize(activity_objects, context) def package_activity_list(context, data_dict): @@ -1758,11 +1754,7 @@ def package_activity_list(context, data_dict): _check_access('package_show', context, data_dict) model = context['model'] package_id = _get_or_bust(data_dict, 'id') - query = model.Session.query(model.Activity) - query = query.filter_by(object_id=package_id) - query = query.order_by(_desc(model.Activity.timestamp)) - query = query.limit(15) - activity_objects = query.all() + activity_objects = model.activity.package_activity_list(package_id) return model_dictize.activity_list_dictize(activity_objects, context) def group_activity_list(context, data_dict): @@ -1787,6 +1779,14 @@ 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}) @@ -1811,11 +1811,7 @@ def recently_changed_packages_activity_list(context, data_dict): # FIXME: Filter out activities whose subject or object the user is not # authorized to read. model = context['model'] - query = model.Session.query(model.Activity) - query = query.filter(model.Activity.activity_type.endswith('package')) - query = query.order_by(_desc(model.Activity.timestamp)) - query = query.limit(15) - activity_objects = query.all() + activity_objects = model.activity.recently_changed_packages_activity_list() return model_dictize.activity_list_dictize(activity_objects, context) def activity_detail_list(context, data_dict): @@ -2219,21 +2215,7 @@ def dashboard_activity_list(context, data_dict): _("You must be logged in to access your dashboard.")) user_id = userobj.id - activity_query = model.Session.query(model.Activity) - user_followees_query = activity_query.join(model.UserFollowingUser, model.UserFollowingUser.object_id == model.Activity.user_id) - dataset_followees_query = activity_query.join(model.UserFollowingDataset, model.UserFollowingDataset.object_id == model.Activity.object_id) - - from_user_query = activity_query.filter(model.Activity.user_id==user_id) - about_user_query = activity_query.filter(model.Activity.object_id==user_id) - user_followees_query = user_followees_query.filter(model.UserFollowingUser.follower_id==user_id) - dataset_followees_query = dataset_followees_query.filter(model.UserFollowingDataset.follower_id==user_id) - - query = from_user_query.union(about_user_query).union( - user_followees_query).union(dataset_followees_query) - query = query.order_by(_desc(model.Activity.timestamp)) - query = query.limit(15) - activity_objects = query.all() - + activity_objects = model.activity.dashboard_activity_list(user_id) return model_dictize.activity_list_dictize(activity_objects, context) @@ -2247,7 +2229,8 @@ def dashboard_activity_list_html(context, data_dict): ''' activity_stream = dashboard_activity_list(context, data_dict) - return activity_streams.activity_list_to_html(context, activity_stream, is_dashboard=True) + return activity_streams.activity_list_to_html(context, activity_stream, + is_dashboard=True) def dashboard_new_activities_count(context, data_dict): @@ -2273,12 +2256,14 @@ def dashboard_new_activities_count(context, data_dict): strptime(activity['timestamp'], fmt) > last_viewed] return len(new_activities) + def dashboard_get_last_viewed(context, data_dict): model = context['model'] - user = model.User.get(context['user']) # The authorized user. + user = model.User.get(context['user']) # The authorized user. last_viewed = model.Dashboard.get_activity_stream_last_viewed(user.id) return last_viewed.timetuple() + def dashboard_mark_activities_as_read(context, data_dict): '''Mark all the authorized user's new dashboard activities as old. diff --git a/ckan/model/activity.py b/ckan/model/activity.py index ff72337f04d..9181acaa20d 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 +from sqlalchemy import orm, types, Column, Table, ForeignKey, desc import meta import types as _types @@ -48,6 +48,7 @@ def __init__(self, user_id, object_id, revision_id, activity_type, meta.mapper(Activity, activity_table) + class ActivityDetail(domain_object.DomainObject): def __init__(self, activity_id, object_id, object_type, activity_type, @@ -61,6 +62,139 @@ def __init__(self, activity_id, object_id, object_type, activity_type, else: self.data = data + meta.mapper(ActivityDetail, activity_detail_table, properties = { 'activity':orm.relation ( Activity, backref=orm.backref('activity_detail')) }) + + +def _most_recent_activities(q, limit): + import ckan.model as model + q = q.order_by(desc(model.Activity.timestamp)) + if limit: + q = q.limit(limit) + return q.all() + + +def _activities_from_user_query(user_id): + import ckan.model as model + q = model.Session.query(model.Activity) + q = q.filter(model.Activity.user_id == user_id) + return q + + +def _activities_about_user_query(user_id): + import ckan.model as model + q = model.Session.query(model.Activity) + q = q.filter(model.Activity.object_id == user_id) + return q + + +def _user_activity_query(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. + + 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.: + + "{USER} created the dataset {DATASET}" + "{OTHER_USER} started following {USER}" + etc. + + ''' + q = _user_activity_query(user_id) + return _most_recent_activities(q, limit) + + +def _package_activity_query(package_id): + import ckan.model as model + q = model.Session.query(model.Activity) + q = q.filter_by(object_id=package_id) + return q + + +def package_activity_list(package_id, limit=15): + '''Return the given dataset (package)'s public activity stream. + + Returns all activities about the given dataset, i.e. where the given + dataset is the object of the activity, e.g.: + + "{USER} created the dataset {DATASET}" + "{USER} updated the dataset {DATASET}" + etc. + + ''' + q = _package_activity_query(package_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) + q = q.join(model.UserFollowingUser, + model.UserFollowingUser.object_id == model.Activity.user_id) + q = q.filter(model.UserFollowingUser.follower_id == user_id) + return q + + +def _activities_from_datasets_followed_by_user_query(user_id): + import ckan.model as model + q = model.Session.query(model.Activity) + q = q.join(model.UserFollowingDataset, + model.UserFollowingDataset.object_id == model.Activity.object_id) + q = q.filter(model.UserFollowingDataset.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)) + return q + + +def activities_from_everything_followed_by_user(user_id, limit=15): + '''Return activities from everything that the given user is following. + + Returns all activities where the object of the activity is anything + (user, dataset, group...) that the given user is following. + + ''' + q = _activities_from_everything_followed_by_user_query(user_id) + return _most_recent_activities(q, limit) + + +def _dashboard_activity_query(user_id): + q = _user_activity_query(user_id) + q = q.union(_activities_from_everything_followed_by_user_query(user_id)) + return q + + +def dashboard_activity_list(user_id, limit=15): + '''Return the given user's dashboard activity stream. + + Returns activities from the user's public activity stream, plus + activities from everything that the user is following. + + This is the union of user_activity_list(user_id) and + activities_from_everything_followed_by_user(user_id). + + ''' + q = _dashboard_activity_query(user_id) + return _most_recent_activities(q, limit) + + +def _recently_changed_packages_activity_query(): + import ckan.model as model + q = model.Session.query(model.Activity) + q = q.filter(model.Activity.activity_type.endswith('package')) + return q + + +def recently_changed_packages_activity_list(limit=15): + q = _recently_changed_packages_activity_query() + return _most_recent_activities(q, limit) From 73fe090f22e0414e987c229d8b4f5b95118fa60b Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Wed, 14 Nov 2012 18:29:11 +0100 Subject: [PATCH 04/16] [#3009] Refactor dashboard_activity_list() auth Add auth functions for dashboard_activity_list and dashboard_new_activities_count. --- ckan/logic/action/get.py | 20 ++++++-------------- ckan/logic/auth/get.py | 15 +++++++++++++++ 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/ckan/logic/action/get.py b/ckan/logic/action/get.py index 0c85d77fa6f..34d23a20889 100644 --- a/ckan/logic/action/get.py +++ b/ckan/logic/action/get.py @@ -2201,21 +2201,15 @@ def dashboard_activity_list(context, data_dict): :rtype: list of dictionaries ''' - # FIXME: Filter out activities whose subject or object the user is not - # authorized to read. - if 'user' not in context: - raise logic.NotAuthorized( - _("You must be logged in to access your dashboard.")) + _check_access('dashboard_activity_list', context, data_dict) model = context['model'] + user_id = model.User.get(context['user']).id - userobj = model.User.get(context['user']) - if not userobj: - raise logic.NotAuthorized( - _("You must be logged in to access your dashboard.")) - user_id = userobj.id - + # FIXME: Filter out activities whose subject or object the user is not + # authorized to read. activity_objects = model.activity.dashboard_activity_list(user_id) + return model_dictize.activity_list_dictize(activity_objects, context) @@ -2242,9 +2236,7 @@ def dashboard_new_activities_count(context, data_dict): :rtype: int ''' - # We don't bother to do our own auth check in this function, because we - # assume dashboard_activity_list will do it. - activities = dashboard_activity_list(context, data_dict) + _check_access('dashboard_new_activities_count', context, data_dict) model = context['model'] user = model.User.get(context['user']) # The authorized user. diff --git a/ckan/logic/auth/get.py b/ckan/logic/auth/get.py index 395bace3b15..55c5bd6ffe0 100644 --- a/ckan/logic/auth/get.py +++ b/ckan/logic/auth/get.py @@ -189,3 +189,18 @@ def get_site_user(context, data_dict): return {'success': False, 'msg': 'Only internal services allowed to use this action'} else: return {'success': True} + + +def dashboard_activity_list(context, data_dict): + if 'user' in context: + return {'success': True} + else: + return {'success': False, + 'msg': _("You must be logged in to access your dashboard.")} + + +def dashboard_new_activities_count(context, data_dict): + if logic.check_access('dashboard_activity_list', context, data_dict): + return {'success': True} + else: + return {'success': False} From 1cb7b539fcbdfe53c153c97f651deef44b7395e9 Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Wed, 14 Nov 2012 18:39:38 +0100 Subject: [PATCH 05/16] [#3009] Don't notify a user about her own activities --- ckan/logic/action/get.py | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/ckan/logic/action/get.py b/ckan/logic/action/get.py index 34d23a20889..98ab706c835 100644 --- a/ckan/logic/action/get.py +++ b/ckan/logic/action/get.py @@ -2228,25 +2228,38 @@ def dashboard_activity_list_html(context, data_dict): def dashboard_new_activities_count(context, data_dict): - '''Return the number of new activities in the user's activity stream. + '''Return the number of new activities in the user's dashboard. Return the number of new activities in the authorized user's dashboard activity stream. + Activities from the user herself are not counted by this function even + though they appear in the dashboard (users don't want to be notified about + things they did themselves). + :rtype: int ''' _check_access('dashboard_new_activities_count', context, data_dict) + activities = logic.get_action('dashboard_activity_list')( + context, data_dict) + model = context['model'] - user = model.User.get(context['user']) # The authorized user. - last_viewed = model.Dashboard.get_activity_stream_last_viewed(user.id) + user_id = model.User.get(context['user']).id + + # Filter out the user's own activities. + activities = [activity for activity in activities + if activity['user_id'] != user_id] + # Filter out the old (already seen) activities. strptime = datetime.datetime.strptime fmt = '%Y-%m-%dT%H:%M:%S.%f' - new_activities = [activity for activity in activities if - strptime(activity['timestamp'], fmt) > last_viewed] - return len(new_activities) + last_viewed = model.Dashboard.get_activity_stream_last_viewed(user_id) + activities = [activity for activity in activities + if strptime(activity['timestamp'], fmt) > last_viewed] + + return len(activities) def dashboard_get_last_viewed(context, data_dict): From 8469a508a6add6a8bae578e273cdf64cef8b2fdf Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Wed, 14 Nov 2012 19:01:48 +0100 Subject: [PATCH 06/16] [#3009] Use is_authorized not check_access to wrap auth function I think this is the right way to do it --- ckan/logic/auth/get.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/ckan/logic/auth/get.py b/ckan/logic/auth/get.py index 55c5bd6ffe0..bbe4db700a0 100644 --- a/ckan/logic/auth/get.py +++ b/ckan/logic/auth/get.py @@ -200,7 +200,5 @@ def dashboard_activity_list(context, data_dict): def dashboard_new_activities_count(context, data_dict): - if logic.check_access('dashboard_activity_list', context, data_dict): - return {'success': True} - else: - return {'success': False} + return ckan.new_authz.is_authorized('dashboard_activity_list', + context, data_dict) From eb086d5da0ccbe7ea46b6f2bb70aafc0474dd748 Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Wed, 14 Nov 2012 19:31:18 +0100 Subject: [PATCH 07/16] [#3009] Don't mark the user's own activities as new on her dashboard --- ckan/lib/activity_streams.py | 12 ++------- ckan/logic/action/get.py | 47 ++++++++++++++++++++---------------- 2 files changed, 28 insertions(+), 31 deletions(-) diff --git a/ckan/lib/activity_streams.py b/ckan/lib/activity_streams.py index da3cac1f736..49184e29bf1 100644 --- a/ckan/lib/activity_streams.py +++ b/ckan/lib/activity_streams.py @@ -187,13 +187,9 @@ def activity_stream_string_new_related_item(): # A list of activity types that may have details activity_stream_actions_with_detail = ['changed package'] -def activity_list_to_html(context, activity_stream, is_dashboard=False): +def activity_list_to_html(context, activity_stream): '''Return the given activity stream as a snippet of HTML.''' - # get the last time they read the dashboard - if (is_dashboard): - last_viewed = logic.get_action('dashboard_get_last_viewed')(context, {}) - activity_list = [] # These are the activity stream messages. for activity in activity_stream: detail = None @@ -234,16 +230,12 @@ def activity_list_to_html(context, activity_stream, is_dashboard=False): snippet = activity_snippet_functions[match](activity, detail) data[str(match)] = snippet timestamp = datetime.datetime.strptime(activity['timestamp'], '%Y-%m-%dT%H:%M:%S.%f').timetuple() - if (is_dashboard): - is_new = ( timestamp > last_viewed ) - else: - is_new = False; activity_list.append({'msg': activity_msg, 'type': activity_type.replace(' ', '-').lower(), 'icon': activity_icon, 'data': data, 'timestamp': activity['timestamp'], - 'is_new': is_new}) + 'is_new': activity.get('is_new', False)}) return literal(base.render('activity_streams/activity_stream_items.html', extra_vars={'activities': activity_list})) diff --git a/ckan/logic/action/get.py b/ckan/logic/action/get.py index 98ab706c835..6ba9b8bae60 100644 --- a/ckan/logic/action/get.py +++ b/ckan/logic/action/get.py @@ -2198,7 +2198,14 @@ def group_followee_list(context, data_dict): def dashboard_activity_list(context, data_dict): '''Return the authorized user's dashboard activity stream. - :rtype: list of dictionaries + Unlike the activity dictionaries returned by other *_activity_list actions, + these activity dictionaries have an extra boolean value with key 'is_new' + that tells you whether the activity happened since the user last viewed her + dashboard ('is_new': True) or not ('is_new': False). + + The user's own activities are always marked 'is_new': False. + + :rtype: list of activity dictionaries ''' _check_access('dashboard_activity_list', context, data_dict) @@ -2210,7 +2217,22 @@ def dashboard_activity_list(context, data_dict): # authorized to read. activity_objects = model.activity.dashboard_activity_list(user_id) - return model_dictize.activity_list_dictize(activity_objects, context) + activity_dicts = model_dictize.activity_list_dictize( + activity_objects, context) + + # Mark the new (not yet seen by user) activities. + strptime = datetime.datetime.strptime + fmt = '%Y-%m-%dT%H:%M:%S.%f' + last_viewed = model.Dashboard.get_activity_stream_last_viewed(user_id) + for activity in activity_dicts: + if activity['user_id'] == user_id: + # Never mark the user's own activities as new. + activity['is_new'] = False + else: + activity['is_new'] = (strptime(activity['timestamp'], fmt) + > last_viewed) + + return activity_dicts def dashboard_activity_list_html(context, data_dict): @@ -2223,8 +2245,7 @@ def dashboard_activity_list_html(context, data_dict): ''' activity_stream = dashboard_activity_list(context, data_dict) - return activity_streams.activity_list_to_html(context, activity_stream, - is_dashboard=True) + return activity_streams.activity_list_to_html(context, activity_stream) def dashboard_new_activities_count(context, data_dict): @@ -2241,25 +2262,9 @@ def dashboard_new_activities_count(context, data_dict): ''' _check_access('dashboard_new_activities_count', context, data_dict) - activities = logic.get_action('dashboard_activity_list')( context, data_dict) - - model = context['model'] - user_id = model.User.get(context['user']).id - - # Filter out the user's own activities. - activities = [activity for activity in activities - if activity['user_id'] != user_id] - - # Filter out the old (already seen) activities. - strptime = datetime.datetime.strptime - fmt = '%Y-%m-%dT%H:%M:%S.%f' - last_viewed = model.Dashboard.get_activity_stream_last_viewed(user_id) - activities = [activity for activity in activities - if strptime(activity['timestamp'], fmt) > last_viewed] - - return len(activities) + return len([activity for activity in activities if activity['is_new']]) def dashboard_get_last_viewed(context, data_dict): From fc626a5caf1d0ade528aae527f2efef7482aebca Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Wed, 14 Nov 2012 20:31:00 +0100 Subject: [PATCH 08/16] [#3009] Add new auth functions to publisher auth profile This fixes a publisher auth test that was failing. --- ckan/logic/auth/publisher/get.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ckan/logic/auth/publisher/get.py b/ckan/logic/auth/publisher/get.py index 0e936c95105..f7725c5a068 100644 --- a/ckan/logic/auth/publisher/get.py +++ b/ckan/logic/auth/publisher/get.py @@ -5,6 +5,10 @@ from ckan.logic.auth.publisher import _groups_intersect from ckan.authz import Authorizer from ckan.logic.auth import get_package_object, get_group_object, get_resource_object +from ckan.logic.auth.get import ( + dashboard_new_activities_count, + dashboard_activity_list, + ) def site_read(context, data_dict): """\ From 8ea022a01f5578ddbf418e9c10d7d03c5f57fc32 Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Wed, 14 Nov 2012 20:31:49 +0100 Subject: [PATCH 09/16] [#3009] Fix some broken activity streams tests These were all broken by the addition of 'is_new' to activity stream dicts in dashboard_activity_list. I removed one broken test that didn't seem to be testing what it thought it was testing. --- ckan/tests/functional/api/test_activity.py | 33 ++++++++++------------ 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/ckan/tests/functional/api/test_activity.py b/ckan/tests/functional/api/test_activity.py index 6222ac48d02..4f7eff2a380 100644 --- a/ckan/tests/functional/api/test_activity.py +++ b/ckan/tests/functional/api/test_activity.py @@ -303,12 +303,14 @@ 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 new_activities == [activity] + 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 new_activities == [activity] + assert [activity['id'] for activity in new_activities] == [ + activity['id']] def _create_package(self, user, name=None): if user: @@ -364,7 +366,8 @@ def _create_package(self, user, name=None): # 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 in new_activities + 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. @@ -374,7 +377,8 @@ def _create_package(self, user, name=None): # 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 new_activities == [activity] + 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. @@ -385,7 +389,8 @@ def _create_package(self, user, name=None): grp_new_activities = find_new_activities( before['group activity streams'][group_dict['name']], after['group activity streams'][group_dict['name']]) - assert grp_new_activities == [activity] + assert [activity['id'] for activity in grp_new_activities] == [ + activity['id']] # Check that the new activity has the right attributes. assert activity['object_id'] == package_created['id'], \ @@ -2122,7 +2127,8 @@ def test_follow_dataset(self): # 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 in new_activities + 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. @@ -2132,7 +2138,8 @@ def test_follow_dataset(self): # 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 new_activities == [activity] + 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'], \ @@ -2183,17 +2190,7 @@ def test_follow_user(self): 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] == activity - - # Check that the new activity appears in the followee's private - # activity stream. - followee_new_activities = (find_new_activities( - followee_before['follower dashboard activity stream'], - followee_after['follower dashboard activity stream'])) - assert len(followee_new_activities) == 1, ("There should be 1 new " - " activity in the user's activity stream, but found %i" % - len(followee_new_activities)) - assert followee_new_activities[0] == activity + 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'], \ From 2bfde7c1411e609c09e2443b9bd1439473f42900 Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Thu, 15 Nov 2012 14:58:22 +0100 Subject: [PATCH 10/16] [#3009] Better tests for on-site activity stream notifications API Test both the dashboard_new_activities_count and the marking of new activities with 'is_new': True in dashboard_activity_list, and test that the user's own activities are not counted as new but activities from things she follows are. --- ckan/tests/functional/api/test_dashboard.py | 176 ++++++++++++++++---- 1 file changed, 143 insertions(+), 33 deletions(-) diff --git a/ckan/tests/functional/api/test_dashboard.py b/ckan/tests/functional/api/test_dashboard.py index 9cd330dd905..a9ae85f1d54 100644 --- a/ckan/tests/functional/api/test_dashboard.py +++ b/ckan/tests/functional/api/test_dashboard.py @@ -7,6 +7,20 @@ class TestDashboard(object): '''Tests for the logic action functions related to the user's dashboard.''' + @classmethod + def user_create(cls): + '''Create a new user.''' + params = json.dumps({ + 'name': 'mr_new_user', + 'email': 'mr@newuser.com', + 'password': 'iammrnew', + }) + response = cls.app.post('/api/action/user_create', params=params, + extra_environ={'Authorization': str(cls.joeadmin['apikey'])}) + assert response.json['success'] is True + new_user = response.json['result'] + return new_user + @classmethod def setup_class(cls): ckan.tests.CreateTestData.create() @@ -16,14 +30,24 @@ def setup_class(cls): 'id': joeadmin.id, 'apikey': joeadmin.apikey } + annafan = ckan.model.User.get('annafan') + cls.annafan = { + 'id': annafan.id, + 'apikey': annafan.apikey + } + testsysadmin = ckan.model.User.get('testsysadmin') + cls.testsysadmin = { + 'id': testsysadmin.id, + 'apikey': testsysadmin.apikey + } + cls.new_user = cls.user_create() @classmethod def teardown_class(cls): ckan.model.repo.rebuild_db() - def new_activities_count(self, user): + def dashboard_new_activities_count(self, user): '''Return the given user's new activities count from the CKAN API.''' - params = json.dumps({}) response = self.app.post('/api/action/dashboard_new_activities_count', params=params, @@ -32,56 +56,142 @@ def new_activities_count(self, user): new_activities_count = response.json['result'] return new_activities_count - def mark_as_read(self, user): + def dashboard_activity_list(self, user): + '''Return the given user's dashboard activity list from the CKAN API. + + ''' params = json.dumps({}) - response = self.app.post( - '/api/action/dashboard_mark_activities_as_read', + response = self.app.post('/api/action/dashboard_activity_list', params=params, extra_environ={'Authorization': str(user['apikey'])}) assert response.json['success'] is True + activity_list = response.json['result'] + return activity_list - def test_01_num_new_activities_new_user(self): - '''Test retreiving the number of new activities for a new user.''' + def dashboard_new_activities(self, user): + '''Return the activities from the user's dashboard activity stream + that are currently marked as new.''' + activity_list = self.dashboard_activity_list(user) + return [activity for activity in activity_list if activity['is_new']] - # Create a new user. - params = json.dumps({ - 'name': 'mr_new_user', - 'email': 'mr@newuser.com', - 'password': 'iammrnew', - }) - response = self.app.post('/api/action/user_create', params=params, - extra_environ={'Authorization': str(self.joeadmin['apikey'])}) + def dashboard_mark_activities_as_read(self, user): + params = json.dumps({}) + response = self.app.post( + '/api/action/dashboard_mark_activities_as_read', + params=params, + extra_environ={'Authorization': str(user['apikey'])}) assert response.json['success'] is True - new_user = response.json['result'] - # We expect to find only one new activity for a newly registered user - # (A "{USER} signed up" activity). - assert self.new_activities_count(new_user) == 1 + 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 + + def test_01_new_activities_for_new_user(self): + '''Test that a newly registered user has no activities marked as new + in their dashboard activity stream.''' + assert len(self.dashboard_new_activities(self.new_user)) == 0 - self.mark_as_read(new_user) - assert self.new_activities_count(new_user) == 0 + def test_02_own_activities_do_not_count_as_new(self): + '''Make a user do some activities and check that her own activities + don't increase her new activities count.''' + # Include following a dataset and doing some stuff to that dataset. # Create a dataset. + + # The user has to view her dashboard activity stream first to mark any + # existing activities as read. For example when she follows a dataset + # below, past activities from the dataset (e.g. when someone created + # the dataset, etc.) will appear in her dashboard, and if she has never + # viewed her dashboard then those activities will be considered + # "unseen". + # We would have to do this if, when you follow something, you only get + # the activities from that object since you started following it, and + # not all its past activities as well. + self.dashboard_mark_activities_as_read(self.new_user) + + # Create a new dataset. params = json.dumps({ 'name': 'my_new_package', }) response = self.app.post('/api/action/package_create', params=params, - extra_environ={'Authorization': str(new_user['apikey'])}) + extra_environ={'Authorization': str(self.new_user['apikey'])}) assert response.json['success'] is True - # Now there should be a new 'user created dataset' activity. - assert self.new_activities_count(new_user) == 1 + # Follow a dataset. + params = json.dumps({'id': 'warandpeace'}) + response = self.app.post('/api/action/follow_dataset', params=params, + extra_environ={'Authorization': str(self.new_user['apikey'])}) + assert response.json['success'] is True - # Update the dataset. - params = json.dumps({ - 'name': 'my_new_package', - 'title': 'updated description', - }) + # Follow a user. + params = json.dumps({'id': 'annafan'}) + response = self.app.post('/api/action/follow_user', params=params, + extra_environ={'Authorization': str(self.new_user['apikey'])}) + assert response.json['success'] is True + + # Follow a group. + params = json.dumps({'id': 'roger'}) + response = self.app.post('/api/action/follow_group', params=params, + extra_environ={'Authorization': str(self.new_user['apikey'])}) + assert response.json['success'] is True + + # Update the dataset that we're following. + params = json.dumps({'name': 'warandpeace', 'notes': 'updated'}) response = self.app.post('/api/action/package_update', params=params, - extra_environ={'Authorization': str(new_user['apikey'])}) + extra_environ={'Authorization': str(self.new_user['apikey'])}) assert response.json['success'] is True - assert self.new_activities_count(new_user) == 2 + # User's own actions should not increase her activity count. + assert self.dashboard_new_activities_count(self.new_user) == 0 + + 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.''' + + # Make someone else who new_user is not following update a dataset that + # new_user is following. + params = json.dumps({'name': 'warandpeace', 'notes': 'updated again'}) + response = self.app.post('/api/action/package_update', params=params, + extra_environ={'Authorization': str(self.joeadmin['apikey'])}) + assert response.json['success'] is True + + # 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. + 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 - self.mark_as_read(new_user) - assert self.new_activities_count(new_user) == 0 + # 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 + + def test_05_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 + + def test_06_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 + assert len(self.dashboard_new_activities(self.new_user)) > 0 + self.dashboard_mark_activities_as_read(self.new_user) + assert self.dashboard_new_activities_count(self.new_user) == 0 + assert len(self.dashboard_new_activities(self.new_user)) == 0 From 78cbd6228c41d7b7abe333bb9bcb511e310664fe Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Thu, 15 Nov 2012 15:02:19 +0100 Subject: [PATCH 11/16] [#3009] Delete an unnecessary comment --- ckan/tests/functional/api/test_dashboard.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/ckan/tests/functional/api/test_dashboard.py b/ckan/tests/functional/api/test_dashboard.py index a9ae85f1d54..f60b97346b7 100644 --- a/ckan/tests/functional/api/test_dashboard.py +++ b/ckan/tests/functional/api/test_dashboard.py @@ -95,9 +95,6 @@ def test_02_own_activities_do_not_count_as_new(self): '''Make a user do some activities and check that her own activities don't increase her new activities count.''' - # Include following a dataset and doing some stuff to that dataset. - # Create a dataset. - # The user has to view her dashboard activity stream first to mark any # existing activities as read. For example when she follows a dataset # below, past activities from the dataset (e.g. when someone created From 79e28c0485b039831fa3812bea3637b272d6afbc Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Thu, 15 Nov 2012 15:05:39 +0100 Subject: [PATCH 12/16] [#3009] Delete an unnecessary line --- ckan/lib/activity_streams.py | 1 - 1 file changed, 1 deletion(-) diff --git a/ckan/lib/activity_streams.py b/ckan/lib/activity_streams.py index 49184e29bf1..8ca1f193079 100644 --- a/ckan/lib/activity_streams.py +++ b/ckan/lib/activity_streams.py @@ -229,7 +229,6 @@ def activity_list_to_html(context, activity_stream): for match in matches: snippet = activity_snippet_functions[match](activity, detail) data[str(match)] = snippet - timestamp = datetime.datetime.strptime(activity['timestamp'], '%Y-%m-%dT%H:%M:%S.%f').timetuple() activity_list.append({'msg': activity_msg, 'type': activity_type.replace(' ', '-').lower(), From af925e247f8c224662f17202926541938e39031b Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Thu, 15 Nov 2012 15:08:22 +0100 Subject: [PATCH 13/16] [#3009] Delete an unused function --- ckan/logic/action/get.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/ckan/logic/action/get.py b/ckan/logic/action/get.py index 6ba9b8bae60..f6788ef9652 100644 --- a/ckan/logic/action/get.py +++ b/ckan/logic/action/get.py @@ -2267,13 +2267,6 @@ def dashboard_new_activities_count(context, data_dict): return len([activity for activity in activities if activity['is_new']]) -def dashboard_get_last_viewed(context, data_dict): - model = context['model'] - user = model.User.get(context['user']) # The authorized user. - last_viewed = model.Dashboard.get_activity_stream_last_viewed(user.id) - return last_viewed.timetuple() - - def dashboard_mark_activities_as_read(context, data_dict): '''Mark all the authorized user's new dashboard activities as old. From aed806f72134f728af7eee468f695bc6f5fbcfd4 Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Thu, 15 Nov 2012 15:15:45 +0100 Subject: [PATCH 14/16] [#3009] Mark new activities terminology consistent Always use new/old activities, don't mix in other terms like read/unread, seen/unseen. --- ckan/controllers/user.py | 4 ++-- ckan/logic/action/get.py | 2 +- ckan/tests/functional/api/test_dashboard.py | 8 ++++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/ckan/controllers/user.py b/ckan/controllers/user.py index 3976088886a..e32d32587cc 100644 --- a/ckan/controllers/user.py +++ b/ckan/controllers/user.py @@ -503,9 +503,9 @@ def dashboard(self, id=None): c.dashboard_activity_stream = h.dashboard_activity_stream(id) - # Mark the user's new activities as read whenever they view their + # Mark the user's new activities as old whenever they view their # dashboard page. - get_action('dashboard_mark_activities_as_read')(context, {}) + get_action('dashboard_mark_all_new_activities_as_old')(context, {}) return render('user/dashboard.html') diff --git a/ckan/logic/action/get.py b/ckan/logic/action/get.py index f6788ef9652..88ebc87801d 100644 --- a/ckan/logic/action/get.py +++ b/ckan/logic/action/get.py @@ -2267,7 +2267,7 @@ def dashboard_new_activities_count(context, data_dict): return len([activity for activity in activities if activity['is_new']]) -def dashboard_mark_activities_as_read(context, data_dict): +def dashboard_mark_all_new_activities_as_old(context, data_dict): '''Mark all the authorized user's new dashboard activities as old. This will reset dashboard_new_activities_count to 0. diff --git a/ckan/tests/functional/api/test_dashboard.py b/ckan/tests/functional/api/test_dashboard.py index f60b97346b7..fa79eedd54e 100644 --- a/ckan/tests/functional/api/test_dashboard.py +++ b/ckan/tests/functional/api/test_dashboard.py @@ -74,10 +74,10 @@ def dashboard_new_activities(self, user): activity_list = self.dashboard_activity_list(user) return [activity for activity in activity_list if activity['is_new']] - def dashboard_mark_activities_as_read(self, user): + def dashboard_mark_all_new_activities_as_old(self, user): params = json.dumps({}) response = self.app.post( - '/api/action/dashboard_mark_activities_as_read', + '/api/action/dashboard_mark_all_new_activities_as_old', params=params, extra_environ={'Authorization': str(user['apikey'])}) assert response.json['success'] is True @@ -104,7 +104,7 @@ def test_02_own_activities_do_not_count_as_new(self): # We would have to do this if, when you follow something, you only get # the activities from that object since you started following it, and # not all its past activities as well. - self.dashboard_mark_activities_as_read(self.new_user) + self.dashboard_mark_all_new_activities_as_old(self.new_user) # Create a new dataset. params = json.dumps({ @@ -189,6 +189,6 @@ def test_06_mark_new_activities_as_read(self): her dashboard activity stream.''' assert self.dashboard_new_activities_count(self.new_user) > 0 assert len(self.dashboard_new_activities(self.new_user)) > 0 - self.dashboard_mark_activities_as_read(self.new_user) + self.dashboard_mark_all_new_activities_as_old(self.new_user) assert self.dashboard_new_activities_count(self.new_user) == 0 assert len(self.dashboard_new_activities(self.new_user)) == 0 From 3007c853ae90b7317bff955cd5837d58d59cfded Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Thu, 15 Nov 2012 15:20:26 +0100 Subject: [PATCH 15/16] [#3009] Fix auth for dashboard_mark_all_new_activities_as_old I think this is the correct way to do it, so that the auth function can be overridden by IAuth plugins. --- ckan/logic/action/get.py | 13 +++---------- ckan/logic/auth/get.py | 5 +++++ ckan/logic/auth/publisher/get.py | 1 + 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/ckan/logic/action/get.py b/ckan/logic/action/get.py index 88ebc87801d..a6776216aff 100644 --- a/ckan/logic/action/get.py +++ b/ckan/logic/action/get.py @@ -2273,17 +2273,10 @@ def dashboard_mark_all_new_activities_as_old(context, data_dict): This will reset dashboard_new_activities_count to 0. ''' - if 'user' not in context: - raise logic.NotAuthorized( - _("You must be logged in to access your dashboard.")) - + _check_access('dashboard_mark_all_new_activities_as_old', context, + data_dict) model = context['model'] - - userobj = model.User.get(context['user']) - if not userobj: - raise logic.NotAuthorized( - _("You must be logged in to access your dashboard.")) - user_id = userobj.id + user_id = model.User.get(context['user']).id model.Dashboard.update_activity_stream_last_viewed(user_id) diff --git a/ckan/logic/auth/get.py b/ckan/logic/auth/get.py index bbe4db700a0..cc3c20f88fe 100644 --- a/ckan/logic/auth/get.py +++ b/ckan/logic/auth/get.py @@ -202,3 +202,8 @@ def dashboard_activity_list(context, data_dict): def dashboard_new_activities_count(context, data_dict): return ckan.new_authz.is_authorized('dashboard_activity_list', context, data_dict) + + +def dashboard_mark_all_new_activities_as_old(context, data_dict): + return ckan.new_authz.is_authorized('dashboard_activity_list', + context, data_dict) diff --git a/ckan/logic/auth/publisher/get.py b/ckan/logic/auth/publisher/get.py index f7725c5a068..41383de6a41 100644 --- a/ckan/logic/auth/publisher/get.py +++ b/ckan/logic/auth/publisher/get.py @@ -8,6 +8,7 @@ from ckan.logic.auth.get import ( dashboard_new_activities_count, dashboard_activity_list, + dashboard_mark_all_new_activities_as_old, ) def site_read(context, data_dict): From 11be4834e86228987b763249521c7e931dee2d4d Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Thu, 15 Nov 2012 17:00:42 +0100 Subject: [PATCH 16/16] [#3009] Add max. num. new activities test --- ckan/tests/functional/api/test_dashboard.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/ckan/tests/functional/api/test_dashboard.py b/ckan/tests/functional/api/test_dashboard.py index fa79eedd54e..5c5cbd8443f 100644 --- a/ckan/tests/functional/api/test_dashboard.py +++ b/ckan/tests/functional/api/test_dashboard.py @@ -192,3 +192,14 @@ def test_06_mark_new_activities_as_read(self): self.dashboard_mark_all_new_activities_as_old(self.new_user) 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): + '''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): + notes = "Updated {n} times".format(n=n) + params = json.dumps({'name': 'warandpeace', 'notes': notes}) + response = self.app.post('/api/action/package_update', params=params, + extra_environ={'Authorization': str(self.joeadmin['apikey'])}) + assert response.json['success'] is True + assert self.dashboard_new_activities_count(self.new_user) == 15