Hide activity by site user from activity stream #1330
Conversation
ghost
assigned 
|
I took at a look at fixing this and I think the way to fix it would be to modify the queries in the activity model. @seanh, since you know this feature best, what do you think? |
|
I think a config option with a list of users to exclude sounds good, the default value should be to exclude the site user, but people could add other users (e.g. harvest users). For the implementation - would it be better to exclude the activities at activity_create time, so they don't even get into the db? Pros: pointless stuff not in DB. Cons: if you add an excluded user to the config who wasn't there before, their old activities will still be in the db, so will still show. If implementing this way the excluding can be done in one central place, the activity_create action function. Only complication is that some activities are created in other places, I think in model/package.py and model/resource.py, excludes would have to be adder for those too. Alternative approach is as you suggested: filter the activities at query time. The activities would still be in the db even though not used. If you change the excluded users config, this will affect past activities as well. The various query methods are in model/activity.py, I suggest adding a function there that takes any activity stream and removes excluded users' activities from it, then making all the existing query functions call it. You could also do this at the logic level rather than model. At first glance I'd say logic level seems the right place for this, but currently all the logic for deciding what activities go into what activity stream is in the model, so it seems to make sense there? Not sure. |
|
Ah, I remember now why that activity logic is in the model. It's because it's done using various sqlalchemy queries for efficiency and to remove code duplication, and we didn't want any sqlalchemy code to appear in the logic. So if going the route of excluding activities at query time, I think the question is how do you implement the excluding? If using sqlalchemy, then put it in the model. If just doing it in Python (e.g. loop over a list of activity dicts, and remove the ones that contain an excluded user*) then put it in the logic. I'm not sure which is best, excluding things based on a config option seems "logicy" to me, on the other hand doing it at sql query time is probably more efficient.
|
|
I think doing at query time is best. Most times a user needs to be excluded after you notice how much of that user's activity is in an activity stream. I think doing them in the model would be more efficient, though I feel like this should be done in the logic. |
| @@ -136,7 +136,8 @@ ckan.feeds.author_link = | |||
| #ckan.activity_streams_enabled = true | |||
| #ckan.activity_list_limit = 31 | |||
| #ckan.activity_streams_email_notifications = true | |||
| # ckan.email_notifications_since = 2 days | |||
| #ckan.email_notifications_since = 2 days | |||
| #ckan.hide_activity_from_users = %(ckan.site_id)s | |||
There was a problem hiding this comment.
Wouldn't hurt to add a comment here saying that %(ckan.site_id)s does in this context
|
I'm generally happy with this implementation, I just made some minor comments. The one big comment I have is: needs tests! Which I know is a pain, because there are no new-style tests for activity streams yet, so you'd have to start by adding them. I'm not sure what our policy on this is right now - do we let changes like this in, without tests? Or do we make you write a whole set of new-style tests for these activity action functions? @nigelbabu Assigning back to you for now anyway |
|
|
||
| ckan.hide_activity_from_users = default sysadmin | ||
|
|
||
| Hides activity from the specified users from activity stream. If unspecified, it'll use :ref:`ckan.site_id`. |
There was a problem hiding this comment.
I'd try to make this a bit clearer for users who might not know what site_id or the site user is. Something like "By default only activities from the site user (...) are hidden" where (...) is a brief explanation of who the site user is
Also default in the example setting seems a bit confusing?
|
@seanh I have a quick question for you - Should I bother getting the site user or just enable the setting and use the site_id in the config directly? That would simplify this code quite a bit. |
* Split the helper function into two functions. * Add better docstrings. * Improve the documentation and explain about site user.
|
Actually, nevermind. That was a bad idea. Users who upgrade would not have new settings in their config automatically. |
|
@seanh Can I land this change and file a bug to write tests? |
|
@nigelbabu That's fine, shall I merge this now then? |
|
Yeah, this is good to go as is. |
…-stream Conflicts: ckan/logic/action/get.py ckanext/textpreview/tests/test_preview.py
|
Merge conflict, waiting to see if the tests pass again |
The activity stream should hide activity by the default user (perhaps with a config option). It would also be a good idea to give a config option for other users whose activity should be hidden. The activity would still be saved and shown for sysadmins. Filing this bug for discussion/opposition before I start working on this.