Paginate the activity list #6798

avdata99 · 2022-04-06T20:15:19Z

Fixes #6108

Proposed fixes:

This PR:

Completely change the JS module activity-stream.js. The *_activity_list_html actions were removed at Activity stream html #4627 and this file should be removed
Remove template with the Load more button (no longer in use)
Allow pagination over activity lists adding the next and previous page buttons
Allow filter activities by type

If this is correct we can implement same changes for other activity streams

Features:

includes tests covering changes
includes updated documentation
includes user-visible changes
includes API changes
includes bugfix for possible backport

tino097 · 2022-04-25T18:01:13Z

@avdata99 @amercader
This is good to go, but we need to check if we shoudl merge this first or #6790

My suggestion is to merge this one first

wardi · 2022-04-26T13:26:25Z

for performance it would be much better to use timestamps to paginate activities instead of offsets. The activity table will be very large on busy sites and a web crawler could inadvertently create a DoS when using offset.

amercader · 2022-04-26T13:54:08Z

ckan/views/dataset.py

-    # TODO: remove
-    g.pkg_dict = pkg_dict
-    g.pkg = pkg
+    all_activities = activity_model.package_activity_list(


This looks like a really expensive way of calculating the total number of activities. Maybe we can create a new package_activity_count function that accepts the same params but just returns the count needed, computed in an efficient way

I added the package_activity_count here @amercader

amercader · 2022-04-26T13:57:07Z

@wardi This is how activity lists are paginated now:

ckan/ckan/model/activity.py

Lines 136 to 140 in 636135f

    
           q = q.order_by(desc(model.Activity.timestamp))  # type: ignore 
        
           if offset: 
        
               q = q.offset(offset) 
        
           if limit: 
        
               q = q.limit(limit)

Essentially we pass ?offset=10&limit=20

What you are suggesting is paginating with something like ?since=<timestamp>&limit=10?
Would we need some extra index in the activity table?

wardi · 2022-04-26T14:33:39Z

Exactly. A since or before timestamp parameter is something the db can return instantly based on an index.

We have some indexes that include timestamp:

    "idx_activity_object_id" btree (object_id, "timestamp")
    "idx_activity_user_id" btree (user_id, "timestamp")

so as long as we're only looking at the activities for a specific user or object we should be covered.

wardi · 2022-04-26T16:49:22Z

Offset by timestamp also has the benefit of having persistent urls and not having duplicate items show up while moving through pages when new activities are added at the same time

avdata99 · 2022-04-26T18:45:31Z

@avdata99 @amercader This is good to go, but we need to check if we shoudl merge this first or #6790

My suggestion is to merge this one first

@tino097 @amercader, I agree to merge this one first.
But we need to consider other activity streams (e.g. user, groups and others).
Do we want to apply this to all of them before #6790?

amercader · 2022-04-29T13:20:40Z

So we could approach this in two ways:

Option A

Implement timestamp based pagination on this PR (for datasets)
Replicate the changes on this PR to all activity streams (user, group/org, dashboard) on a separate PR
Port the changes to the new activities plugin PR (Extract acitivities into a separate plugin #6790)

or Option B

Merge the new activities plugin PR (Extract acitivities into a separate plugin #6790)
Port the changes of this PR to the activities plugin
Implement timestamp based pagination on this PR (for datasets) on the activities plugin
Replicate the changes on this PR to all activity streams (user, group/org, dashboard) on the activities plugin

I don't have any strong opinions either way, @smotornyuk you probably have a better idea of the effort involved and are best placed to decide

smotornyuk · 2022-04-29T14:27:01Z

I prefer option A. For me, it will be as simple as picking a few extra changes into a separate plugin(i have plenty of macros, so it will be a trivial task). And I don't want @avdata99 to repeat his work:)

After merging this PR, if we go with #6790 next, it will be even easier for @avdata99 to replicate the changes on this PR to all activity streams.

BTW, starting the next week I'll stop my volunteer activities(as the situation is more or less stabilized and I'm not really useful anymore). This means I'll have more time for CKAN and will get back to the TechTeam meetings:)

PS @avdata99, as for your question: It should be a separate PR. I think that it will be simpler to replicate your changes to other activities after #6790. But if you don't want to wait/don't want to work on the updated codebase, go ahead and I'll just adopt your changes into my PR afterward.

avdata99 · 2022-05-02T05:55:36Z

Thanks @smotornyuk, I agree to go for this PR first, then move this feature to this PR and finally create independent PRs for other activity streams.

avdata99 · 2022-05-03T08:00:31Z

@smotornyuk @amercader
I finished a first version of pagination by timestamp (including some basic tests).
Some notes:

If we expect duplicate timestamps (I think we don't), we'll need to change this approach to one that includes timestamp+id approach
For the previous page I'm still using offset, is there a better way to do it?

amercader

@avdata99 looking good! I added a few more comments to polish the feature a bit

amercader · 2022-05-03T10:01:44Z

ckan/views/dataset.py

 ) -> Union[Response, str]:  # noqa

    """Render this package's public activity stream page.
    """
+    after = h.get_request_param(u'after')


Don't introduce new u prefixes please 🙏

Changed here.

amercader · 2022-05-03T10:07:18Z

ckan/views/dataset.py

+    if after and before:
+        raise ValidationError(
+            {'after': ['Cannot be used together with `before']}
+        )
+


These checks should be done via schema validators, not directly in the blueprints (otherwise they are not applied when using the API)

A new validator was created here.
Now we allow after and before together.

ckan/views/dataset.py

amercader · 2022-05-03T10:10:30Z

ckan/logic/action/get.py

@@ -2533,14 +2533,17 @@ def package_activity_list(

    :param id: the id or name of the package
    :type id: string
-    :param offset: where to start getting activity items from


@wardi was your idea to remove support for offset pagination entirely or keep it at the API level and just not use it from the UI?

Do we know how many users of the activity API we have?

My guess is we would want to keep it for compatibility, at least for a while.

The offset para was restored. Works well in conjunction with after and before (and not used for pagination).
Also, some test were added

ckan/views/dataset.py

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

ckan/logic/schema.py

+    schema['before'] = [ignore_missing]
+    schema['after'] = [ignore_missing]


These should be validated as timestamps, and as mentioned before if you want to check that both are not present do it here.

ckan/tests/controllers/test_package.py

ckan/templates/package/activity.html

avdata99 · 2022-05-05T09:59:28Z

Latest changes:

New validator added
Tests added
after and before works together
offset can be used again (also combined with after and before)

wardi · 2022-05-06T05:06:00Z

ckan/model/activity.py

+        q = q.filter(model.Activity.timestamp > timestamp)
+    else:
+        q = q.filter(model.Activity.timestamp < timestamp)
+    return q


This kind of 1-line function shouldn't exist because it makes calling code more difficult to understand. Instead please use the line of code itself.

updated here

wardi · 2022-05-06T05:09:33Z

ckan/model/activity.py

+
+    # revert sort queries for "only after" queries
+    revese_order = before and not after
+    results = _activities_limit(q, limit, offset, revese_order).all()


instead of calling _activities_limit please use order_by, offset and limit here. We should aim to remove functions like _activities_limit because they make the code harder to understand and optimize.

updated here

wardi · 2022-05-06T05:16:04Z

@avdata99 thank you for your work on this. I suggested a few other small changes above to reduce the number of functions that hide really simple operations and to make the code easier to understand and improve in the future.

avdata99 · 2022-05-06T14:14:01Z

@wardi thanks for your review
I updated the activity queries here.

wardi · 2022-05-06T14:47:45Z

@avdata99 thank you those changes look much better!

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

This is good to go, but let's wait on #6790 (comment)

amercader · 2022-05-17T13:34:03Z

@avdata99 thanks for your work on this. @smotornyuk will port this changes to #6790 and once that is merged you can replicate the changes to the user and group streams

avdata99 added 2 commits April 6, 2022 16:58

Paginate the activity list

c9f671e

Improve URL

fe61b4d

amercader assigned tino097 Apr 7, 2022

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

avdata99 mentioned this pull request Apr 8, 2022

Paginate activity list for datasets #6754

Closed

amercader reviewed Apr 26, 2022

View reviewed changes

Add package_activity_count

cc14698

avdata99 marked this pull request as draft April 26, 2022 18:46

Add missing auth function

199aeda

Paginate activities by timestamp

0080efb

avdata99 force-pushed the 6108_improve_pkg_actvity_list branch from 2242740 to 0080efb Compare May 2, 2022 10:39

avdata99 added 5 commits May 2, 2022 12:52

Add notes

cd03704

Remove unused activity count

85946e1

Back to previous query version

5e23803

Lint errors

3ad4a30

Fix and add new tests for activity

3b2cc2a

avdata99 marked this pull request as ready for review May 3, 2022 08:00

clean

40e5d58

amercader reviewed May 3, 2022

View reviewed changes

Remove "u'" for new changes

da28b9a

avdata99 added 6 commits May 4, 2022 09:45

Change var name

b1a86bf

Humanize activities

7a55c0e

Re enable offset and fix after-bore params

5e08ebc

Add tests

129e562

remove unused import

04945fe

Add more tests

32977dc

avdata99 marked this pull request as draft May 5, 2022 08:50

avdata99 added 3 commits May 5, 2022 10:52

Add more tests

ea6d2a1

Fix and add tests

c1f7b01

Timestamp validator

a317327

avdata99 force-pushed the 6108_improve_pkg_actvity_list branch from 8ababa5 to a317327 Compare May 5, 2022 09:52

Fix tests

c41334c

avdata99 marked this pull request as ready for review May 5, 2022 10:49

Final fixes

e0abe03

avdata99 requested review from wardi and amercader May 5, 2022 11:13

wardi reviewed May 6, 2022

View reviewed changes

Improve activity queries

64443b2

avdata99 requested a review from wardi May 6, 2022 14:25

amercader mentioned this pull request May 17, 2022

Extract acitivities into a separate plugin #6790

Merged

amercader merged commit b3a9f5e into ckan:master May 17, 2022

amercader mentioned this pull request Jun 9, 2022

Fix activity stream pagination on non-dataset streams #6900

Closed

wardi mentioned this pull request Jun 17, 2022

6900: Add activity stream pagination to all entities #6902

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Paginate the activity list #6798

Paginate the activity list #6798

avdata99 commented Apr 6, 2022 •

edited

Loading

tino097 commented Apr 25, 2022

wardi commented Apr 26, 2022

amercader Apr 26, 2022

avdata99 Apr 26, 2022

amercader commented Apr 26, 2022

wardi commented Apr 26, 2022

wardi commented Apr 26, 2022

avdata99 commented Apr 26, 2022

amercader commented Apr 29, 2022

smotornyuk commented Apr 29, 2022 •

edited

Loading

avdata99 commented May 2, 2022

avdata99 commented May 3, 2022

amercader left a comment

amercader May 3, 2022

avdata99 May 3, 2022

amercader May 3, 2022

avdata99 May 5, 2022 •

edited

Loading

amercader May 3, 2022

wardi May 3, 2022

avdata99 May 5, 2022

amercader May 3, 2022

avdata99 May 5, 2022

avdata99 commented May 5, 2022 •

edited

Loading

wardi May 6, 2022

avdata99 May 6, 2022

wardi May 6, 2022

avdata99 May 6, 2022

wardi commented May 6, 2022

avdata99 commented May 6, 2022

wardi commented May 6, 2022 •

edited

Loading

amercader commented May 17, 2022

amercader commented May 17, 2022

		schema['before'] = [ignore_missing]
		schema['after'] = [ignore_missing]

Paginate the activity list #6798

Paginate the activity list #6798

Conversation

avdata99 commented Apr 6, 2022 • edited Loading

Proposed fixes:

Features:

tino097 commented Apr 25, 2022

wardi commented Apr 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amercader commented Apr 26, 2022

wardi commented Apr 26, 2022

wardi commented Apr 26, 2022

avdata99 commented Apr 26, 2022

amercader commented Apr 29, 2022

smotornyuk commented Apr 29, 2022 • edited Loading

avdata99 commented May 2, 2022

avdata99 commented May 3, 2022

amercader left a 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

avdata99 May 5, 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

avdata99 commented May 5, 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

wardi commented May 6, 2022

avdata99 commented May 6, 2022

wardi commented May 6, 2022 • edited Loading

amercader commented May 17, 2022

amercader commented May 17, 2022

avdata99 commented Apr 6, 2022 •

edited

Loading

smotornyuk commented Apr 29, 2022 •

edited

Loading

avdata99 May 5, 2022 •

edited

Loading

avdata99 commented May 5, 2022 •

edited

Loading

wardi commented May 6, 2022 •

edited

Loading