Extract acitivities into a separate plugin #6790

smotornyuk · 2022-04-03T01:19:09Z

Just as the title says. If anybody doesn't like the idea and the text below will not convince them, feel free to close this PR. Its only aim is to simplify activity improvements in the future and it has no value other than that.

Why?

Make core simpler
Activities naturally work as a separate extension. They don't change CKAN, they only add few new pages/actions. And there are no features inside activities that require low-level interactions with CKAN.
In this way people can extend/replace activities with their own implementation or with the newer version even when the CKAN core upgrade is not possible. For example, I would be really happy to have group changes feature from the CKAN master on my projects using CKAN v2.9. If activities are implemented as an extension, I can just download the activity extension from the upstream and register it as my-activity extension, getting all the new features. And it will be much simpler to locate any incompatibilities and bugs, because the extension is not as huge, as the CKAN core itself.
Narrow scope of changes, related to activities. For example, it will be much easier to locate and reuse code for Paginate activity list for datasets #6754
Put everything related to activities into one place. Usually, we improve only package activities, because no one knows all the places where activities are used. And afterward, someone asks to implement the same feature for the group activities(for example, group changes were not added to 2.9). And, do you remember that we have user activities, that are completely neglected by devs?:)
Identify issues that big extensions may have. It may serve us as a source of ideas on what can be improved. For example, while extracting activities, I noticed that we:
- don't have a mechanism for registering models - even though action context has model property, it just contains ckan.model. There is no way to add\override items there. So it has sense either to extend this item or remove it from context(if it is there for simplifying the import, it's better to move it into the toolkit).
- Don't have recommended way of defining new models and attaching them to the sqlalchemy's metadata. I've created ckan.model.base:Base instance of SQLAlchemy's declarative base, which can be a solution for this problem.
- Don't have a recommended way for setting relationships between plugins. For example, stats plugins depend on activities. If the activity plugin is disabled, old stats can be shown, but there will be no updates to them.

Details

ℹ️ I only moved the code and tried to keep all the features\details\bugs in place. All the improvements and refactoring can be done separately("Good for contribution").

I created the activity plugin and moved almost everything related to activities into it:

activity API
activity model
activity templates
extracted changes/history/activity endpoints from package/group/user blueprints and moved them into the activity view
a couple of helpers
few validators
dashboard.index page
activity tests

What I left:

following functionality
config declarations
"Subscribe to activity notifications" flag from the user's profile

If you have activity enabled, everything should work as it was before, but there are a couple of changes in code required:

import ckan.model.Activity -> import ckanext.activity.model.Activity
url_for("dataset/group/user.activity/changes/changes_multiple") -> url_for("activity.package/group/user_activity/changes/changes_multiple")

If the activity plugin is not enabled:

new activities are not created
corresponding API actions are not available
Activity pages from the user's profile, dataset, and group\organization pages are not shown
Dashboard has no activity tab. After login user is redirected to dashboard.datasets instead.

If you are wondering, how activities are created, there is no magic. I've just used signals:)

PS sorry for another big PR

…ty-extension

amercader

Had a first quick look and overall it looks good, see some questions and comments @smotornyuk

My main concerns are compatibility, making the transition transparent for most users, specially regarding URLs.

For those wanting to have a look check the changes in the logic folder, specially where the new action_succeededsignal is fired, and the subscriptions file where the activity plugin registers its listeners

amercader · 2022-04-29T11:09:45Z

ckan/logic/action/create.py

-        'activity_type': 'new user',
-    }
-    logic.get_action('activity_create')(activity_create_context, activity_dict)
+    # session.flush()


Let's remove this and the comment above is no longer required

Yes, has sense.

amercader · 2022-04-29T11:11:46Z

changes/6790.misc

@@ -0,0 +1,5 @@
+Activites extracted into `activity` plugin:


We should make clear that users should add the activity plugin to their ckan.plugins config option if they want to keep seeing the activities!

Also I think this plugin should be added by default when creating a new ini file (can we add a default for ckan.plugins like we use to do adding stuff to deployment.ini_tmpl?

i agree about adding as default to ckan.plugins

Yep, changelog record updated and activity added to the list of plugins inside the generated config file

amercader · 2022-04-29T11:22:10Z

ckan/views/dashboard.py

@@ -159,12 +64,6 @@ def groups() -> str:
    return base.render(u'user/dashboard_groups.html', extra_vars)


-dashboard.add_url_rule(


Will url_for('dashboard.index') still work? Otherwise is a pain in the ass for extensions

Yes, it works but produces the following log record:

2022-05-15 18:40:30,579 INFO [ckan.lib.helpers] Route name "dashboard.index" is deprecated and will be removed. Please update calls to use "activity.dashboard" instead

amercader · 2022-04-29T12:36:17Z

ckanext/activity/subscriptions.py

+    activity = Activity.activity_stream_item(pkg, type_, user_id)
+    context["session"].add(activity)
+    context["session"].commit()


Why aren't the dataset activities created via the activity_create action like the group or user ones?

activity_create action requires an existing user ID. Such code bypasses this restriction. If you check a few lines above, you'll see user_id = 'not logged in' line, which allows changing a package anonymously(via ignore_auth I suppose).

Even though it looks like a really bad idea, we cannot just require an existing user here. I definitely have seen not logged in in the activity streams, which means that such a change can potentially break existing workflows.

So it's one of those "bugs and problems" that I noticed and decided to keep in order to avoid behavioral changes as much as possible.

ckan/lib/helpers.py

amercader · 2022-04-29T12:54:42Z

ckan/views/dataset.py

@@ -452,50 +450,12 @@ def read(package_type: str, id: str) -> Union[Response, str]:

    g.pkg_dict = pkg_dict
    g.pkg = pkg
-    # NB templates should not use g.pkg, because it takes no account of


IIUC We are changing the URL here right? so on CKAN < 2.10 it would be:

/dataset/my-dataset?activity_id=<activity_id>

And going forward it will be:

/dataset/my-dataset/archive/<activity_id>

Can we keep a small compatibility check to not break URLs? Something like:

if p.plugin_loaded("activity"): activity_id = request.args.get("activity_id") if activity_id: return redirect("activity.package_archive", id=pkg_id, activity_id=activity_id)

WDYT?

(also see note about the name "archive" below)

makes perfect sense. I wanted to handle this case, but I just hadn't come to such an obvious solution 😆

amercader · 2022-04-29T12:57:03Z

ckanext/activity/views.py

+    return tk.render("package/resource_archive.html", extra_vars)
+
+
+@bp.route('/dataset/<id>/archive/<activity_id>')


I'm not sure about "archive" to describe this. Maybe "history"?

@ckan/core any thoughts?

as the method names are _archive maybe we should keep with that but also the option of _activities could have more sense as we use that in tests for the creation of the activities

amercader · 2022-04-29T12:59:20Z

ckanext/activity/views.py

+    })
+
+    try:
+        package = tk.get_action(u'package_show')(context, {u'id': id})


Can we get rid of all the u prefixes on all new files? 🙏
(and maybe blackify them :p, or at least use double quotes consistently. but only the new ones!)

Yes, I like this idea. I'll keep the current formatting till activity-pagination PR is merged so that there will be fewer conflicts. And then I'll run all activity-plugin files through black

amercader · 2022-05-17T10:37:33Z

This is good to go!

@smotornyuk #6798 is also good to be merged. If I understood correctly in here we should merge #6798 first, then you pick the changes in this PR right?

smotornyuk · 2022-05-18T17:01:31Z

Updated. I think that all mentioned issues were addressed, so it's ready for the second round of review.

amercader · 2022-05-20T13:32:58Z

@smotornyuk all good. Do you want to first merge this and then port the #6798 changes on a separate PR or add them here and then merge?

smotornyuk · 2022-05-22T15:57:59Z

I've already ported it. The last four commits(starting from 83c2755) contain the upstream with #6798 included 😄

amercader · 2022-05-23T10:16:20Z

Thanks @smotornyuk !

Next up, port the pagination changes to the user, group etc activity streams

smotornyuk · 2022-05-23T10:42:54Z

Thank you for the quick review:)

@avdata99, this PR is merged, so you can use it for replicating your pagination-feature. I've tried to keep every bit of functionality from activity-streams intact and basically just moved the code and templates to the ckanext/activity folder. But if you have some questions or issues, don't hesitate and mention me somewhere in the comment to your PR.

Add plugin

09ece3f

smotornyuk force-pushed the activity-extension branch from de37100 to 09ece3f Compare April 3, 2022 03:18

smotornyuk added 5 commits April 3, 2022 07:45

Update test durations

4867f55

Oooops

dbc0bf0

Add activity assets

977132a

Merge branch 'activity-extension' of github.com:ckan/ckan into activi…

74b4304

…ty-extension

Add cypress config

de38939

amercader added the To Discuss label Apr 5, 2022

amercader assigned tino097 and amercader Apr 7, 2022

amercader added this to the CKAN 2.10 milestone Apr 7, 2022

Merge remote-tracking branch 'origin/master' into activity-extension

4968a53

tino097 mentioned this pull request Apr 25, 2022

Paginate the activity list #6798

Merged

5 tasks

amercader reviewed Apr 29, 2022

View reviewed changes

smotornyuk added 2 commits May 15, 2022 18:14

Merge remote-tracking branch 'origin/master' into activity-extension

b1891c3

Apply review suggestions

9450c7c

amercader removed the To Discuss label May 17, 2022

smotornyuk added 4 commits May 18, 2022 17:53

Merge remote-tracking branch 'origin/master' into activity-extension

83c2755

Fix tests

8b3574f

blacken

b4993a2

flake8

a9056a6

amercader merged commit f079328 into master May 23, 2022

amercader deleted the activity-extension branch May 23, 2022 10:15

amercader mentioned this pull request Jun 9, 2022

Fix activity stream pagination on non-dataset streams #6900

Closed

amercader mentioned this pull request Mar 22, 2023

defer_commit not working properly on most create actions #7486

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract acitivities into a separate plugin #6790

Extract acitivities into a separate plugin #6790

smotornyuk commented Apr 3, 2022 •

edited

Loading

amercader left a comment •

edited

Loading

amercader Apr 29, 2022

smotornyuk May 15, 2022

amercader Apr 29, 2022

amercader Apr 29, 2022

tino097 May 2, 2022

smotornyuk May 15, 2022 •

edited

Loading

amercader Apr 29, 2022

smotornyuk May 15, 2022

amercader Apr 29, 2022

smotornyuk May 15, 2022

amercader Apr 29, 2022

smotornyuk May 15, 2022

amercader Apr 29, 2022

tino097 May 2, 2022

amercader Apr 29, 2022

smotornyuk May 15, 2022

amercader commented May 17, 2022

smotornyuk commented May 18, 2022

amercader commented May 20, 2022

smotornyuk commented May 22, 2022 •

edited

Loading

amercader commented May 23, 2022

smotornyuk commented May 23, 2022

		@@ -159,12 +64,6 @@ def groups() -> str:
		return base.render(u'user/dashboard_groups.html', extra_vars)


		dashboard.add_url_rule(

		return tk.render("package/resource_archive.html", extra_vars)


		@bp.route('/dataset/<id>/archive/<activity_id>')

Extract acitivities into a separate plugin #6790

Extract acitivities into a separate plugin #6790

Conversation

smotornyuk commented Apr 3, 2022 • edited Loading

Why?

Details

amercader left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smotornyuk May 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amercader commented May 17, 2022

smotornyuk commented May 18, 2022

amercader commented May 20, 2022

smotornyuk commented May 22, 2022 • edited Loading

amercader commented May 23, 2022

smotornyuk commented May 23, 2022

smotornyuk commented Apr 3, 2022 •

edited

Loading

amercader left a comment •

edited

Loading

smotornyuk May 15, 2022 •

edited

Loading

smotornyuk commented May 22, 2022 •

edited

Loading