From 6cfb8b4eed1500369fd2a388f1a8c0131cba824f Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Mon, 12 Dec 2011 17:55:32 +0100 Subject: [PATCH] Get user ID for activity streams from revision When creating Activity objects for activity streams, get the user_id from the revision, not from the pylons context. It's not good to access the global pylons context from the model module, e.g. because when unit tests do things through the logic module directly (instead of through API calls) then the pylons context will not be present. Also change activity_test.py, can now test for correct user IDs. --- ckan/lib/activity.py | 8 ++++---- ckan/model/package.py | 26 +++++++------------------- ckan/tests/models/test_activity.py | 18 ++++++++++++------ 3 files changed, 23 insertions(+), 29 deletions(-) diff --git a/ckan/lib/activity.py b/ckan/lib/activity.py index 2f7622e1175..30c0beca892 100644 --- a/ckan/lib/activity.py +++ b/ckan/lib/activity.py @@ -2,9 +2,9 @@ import logging logger = logging.getLogger(__name__) -def activity_stream_item(obj, activity_type, revision_id): +def activity_stream_item(obj, activity_type, revision): try: - return obj.activity_stream_item(activity_type, revision_id) + return obj.activity_stream_item(activity_type, revision) except (AttributeError, TypeError): logger.debug("Object did not have a suitable " "activity_stream_item() method, it must not be a package.") @@ -46,7 +46,7 @@ def before_commit(self, session): logger.debug("Looking for new packages...") for obj in obj_cache['new']: logger.debug("Looking at object %s" % obj) - activity = activity_stream_item(obj, 'new', revision.id) + activity = activity_stream_item(obj, 'new', revision) if activity is None: continue # If the object returns an activity stream item we know that the @@ -86,7 +86,7 @@ def before_commit(self, session): activity = activities[package.id] else: activity = activity_stream_item(package, "changed", - revision.id) + revision) activities[package.id] = activity assert activity is not None logger.debug("activity: %s" % activity) diff --git a/ckan/model/package.py b/ckan/model/package.py index ed559ca2362..cce0f953c27 100644 --- a/ckan/model/package.py +++ b/ckan/model/package.py @@ -537,30 +537,18 @@ def get_fields(core_only=False, fields_to_ignore=None): return fields - def activity_stream_item(self, activity_type, revision_id): - try: - user_id = c.user_obj.id - except TypeError: - # Cannot access user ID through Pylons context, try to get their IP - # address instead. - try: - user_id = request.environ.get('REMOTE_ADDR', 'Unknown IP Address') - except TypeError: - # Cannot access user IP address through Pylons request, - # fallback on a default value. - user_id = 'Unknown IP Address' - logger.debug("user_id: %s" % user_id) + def activity_stream_item(self, activity_type, revision): assert activity_type in ("new", "changed", "deleted"), \ str(activity_type) + user_id = revision.user.id if activity_type == "new": - return Activity(user_id, self.id, revision_id, "new package", - None) + return Activity(user_id, self.id, revision.id, "new package", None) elif activity_type == "changed": - return Activity(user_id, self.id, revision_id, "changed package", - None) + return Activity(user_id, self.id, revision.id, "changed package", + None) elif activity_type == "deleted": - return Activity(user_id, self.id, revision_id, "deleted package", - None) + return Activity(user_id, self.id, revision.id, "deleted package", + None) def activity_stream_detail(self, activity_id, activity_type): return ActivityDetail(activity_id, self.id, u"Package", activity_type, diff --git a/ckan/tests/models/test_activity.py b/ckan/tests/models/test_activity.py index 36af160c2ed..45159c7e9f6 100644 --- a/ckan/tests/models/test_activity.py +++ b/ckan/tests/models/test_activity.py @@ -96,7 +96,8 @@ def test_create_package(self): activity = activities[-1] assert activity.object_id == package_created['id'], \ str(activity.object_id) - assert activity.user_id == "Unknown IP Address", str(activity.user_id) + assert activity.user_id == TestActivity.normal_user.id, \ + str(activity.user_id) assert activity.activity_type == 'new package', \ str(activity.activity_type) if not activity.id: @@ -170,7 +171,8 @@ def _add_resource(self, package): activity = activities[-1] assert activity.object_id == updated_package['id'], \ str(activity.object_id) - assert activity.user_id == "Unknown IP Address", str(activity.user_id) + assert activity.user_id == TestActivity.normal_user.id, \ + str(activity.user_id) assert activity.activity_type == 'changed package', \ str(activity.activity_type) if not activity.id: @@ -240,7 +242,8 @@ def _update_package(self, package): len(activities))) activity = activities[-1] assert activity.object_id == package.id, str(activity.object_id) - assert activity.user_id == "Unknown IP Address", str(activity.user_id) + assert activity.user_id == TestActivity.normal_user.id, \ + str(activity.user_id) assert activity.activity_type == 'changed package', \ str(activity.activity_type) if not activity.id: @@ -305,7 +308,8 @@ def _update_resource(self, package, resource): assert len(activities) == length_before + 1, str(activities) activity = activities[-1] assert activity.object_id == package.id, str(activity.object_id) - assert activity.user_id == "Unknown IP Address", str(activity.user_id) + assert activity.user_id == TestActivity.normal_user.id, \ + str(activity.user_id) assert activity.activity_type == 'changed package', \ str(activity.activity_type) if not activity.id: @@ -371,7 +375,8 @@ def _delete_package(self, package): len(activities))) activity = activities[-1] assert activity.object_id == package.id, str(activity.object_id) - assert activity.user_id == "Unknown IP Address", str(activity.user_id) + assert activity.user_id == TestActivity.normal_user.id, \ + str(activity.user_id) # "Deleted" packages actually show up as changed (the package's status # changes to "deleted" but the package is not expunged). assert activity.activity_type == 'changed package', \ @@ -441,7 +446,8 @@ def _delete_resources(self, package): len(activities))) activity = activities[-1] assert activity.object_id == package.id, str(activity.object_id) - assert activity.user_id == "Unknown IP Address", str(activity.user_id) + assert activity.user_id == TestActivity.normal_user.id, \ + str(activity.user_id) assert activity.activity_type == 'changed package', \ str(activity.activity_type) if not activity.id: