Paginate topics #4616

rtibbles · 2024-07-24T00:07:40Z

Summary

Description of the change(s) you made

Adds simple lft__gt based pagination to contentnode endpoint, only really usable when filtering by parent or tree_id
Deletes unused pagination from the resource layer, and adds support for simple single value incrementing (e.g. lft) pagination to it
Updates the UI to handle this

Manual verification steps performed

Make a topic with more than 50 children.
Check basic pagination works.
Clear cache and all indexedDB.
Load one additional page (so there should still be one more).
Reload the page.
Turn off the network connection in the network tab of the browser.
Press show more.
See that the additional page loads from indexedDB, and that even though there are no more nodes in IndexedDB, the cached pagination result allows 'show more' to still be shown.
Turn the network back on and press show more to see the remainder.

Screenshot

References

Fixes #1280

Comments

Contributor's Checklist

PR process:

If this is an important user-facing change, PR or related issue the CHANGELOG label been added to this PR. Note: items with this label will be added to the CHANGELOG at a later time
If this includes an internal dependency change, a link to the diff is provided
The docs label has been added if this introduces a change that needs to be updated in the user docs?
If any Python requirements have changed, the updated requirements.txt files also included in this PR
Opportunities for using Google Analytics here are noted
Migrations are safe for a large db

Studio-specifc:

All user-facing strings are translated properly
The notranslate class been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)
All UI components are LTR and RTL compliant
Views are organized into pages, components, and layouts directories as described in the docs
Users' storage used is recalculated properly on any changes to main tree files
If there new ways this uses user data that needs to be factored into our Privacy Policy, it has been noted.

Testing:

Code is clean and well-commented
Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
Critical user journeys are covered by Gherkin stories
Any new interactions have been added to the QA Sheet
Critical and brittle code paths are covered by unit tests

Reviewer's Checklist

This section is for reviewers to fill out.

Automated test coverage is satisfactory
PR is fully functional
PR has been tested for accessibility regressions
External dependency files were updated if necessary (yarn and pip)
Documentation is updated
Contributor is in AUTHORS.md

marcellamaki

Did first code read through and had a couple of comprehension questions, mostly because I'm lacking some context and familiarity with the code. I'll do a manual QA review and then a re-read of the code after that.

marcellamaki · 2024-07-31T18:53:19Z

contentcuration/contentcuration/frontend/shared/data/index.js

    // A special table for logging unsynced changes
    // Dexie.js appears to have a table for this,
    // but it seems to squash and remove changes in ways
    // that I do not currently understand, so we engage
    // in somewhat duplicative behaviour instead.
    [CHANGES_TABLE]: 'rev++,[table+key],server_rev',
+    [PAGINATION_TABLE]: 'queryString',


how do i know this is supposed to be 'queryString'? not rhetorical, just wondering what I should look into to understand the syntax/intention here. I did a quick repo search and didn't see anything similar. I can infer what's happening but just unfamiliar with the specifics of this file

The string for each table is a comma separated list of indexed properties, with the first listed one being the primary key. As we are caching the results of paginated queries, the queryString should uniquely identify the request in question. Although perhaps, I should have called it 'queryURL' as I think that's more what I am using, I think (if I'm not, that's a good thing to pull me up on).

Hrm, it does rather look like I forgot to add the table or API endpoint into this, so I'll need to update that (even though we are only currently paginating ContentNode).

Have updated this now so that the primary key is a compound key of the table and the querystring, and updated the querying and saving accordingly too.

marcellamaki · 2024-07-31T19:00:48Z

contentcuration/contentcuration/frontend/shared/data/resources.js

+    return collection.toArray();
+  }
+
+  whereLiveQuery(params = {}) {


why is liveQuery needed here?

It's not - I started trying to implement the pagination stuff using liveQuery to make sure it would be compatible, then realized I was doing more than was necessary for the immediate fix - then left these helper functions in here as generally useful, when I had originally intended to separate them out into a separate PR.

I left this in, because it does no harm, and it's useful tiny flyby refactor.

marcellamaki · 2024-08-12T16:02:27Z

contentcuration/contentcuration/frontend/channelEdit/views/TreeView/index.vue

+        <template #pagination>
+          <div class="pagination-container">
+            <KButton v-if="more" :disabled="moreLoading" @click="loadMore">
+              {{ 'Show more' }}


String on the lose

I had a dream that this was the case - it seems my dreams came true!

…nating parent filtered queries.

…rating over a non-opaque cursor. Add some simple livequery methods to the resource layer.

radinamatic · 2024-08-27T14:49:51Z

Not having luck trying to load more pages after clearing cache and all indexedDB, and I'm seeing some Sentry errors in the Network tab 😕

rtibbles · 2024-08-27T14:55:09Z

Ah yes, there's a bug here, that I've fixed in a follow up PR.

rtibbles added this to the Studio: Q2-24 Release milestone Jul 24, 2024

rtibbles assigned marcellamaki Jul 25, 2024

marcellamaki reviewed Jul 31, 2024

View reviewed changes

rtibbles force-pushed the paginate_topics branch from 3ce1312 to 3813885 Compare August 6, 2024 23:20

marcellamaki reviewed Aug 12, 2024

View reviewed changes

rtibbles force-pushed the paginate_topics branch from 3813885 to 0cba194 Compare August 20, 2024 15:07

rtibbles added 4 commits August 20, 2024 09:11

Add simplistic cursor pagination to contentnode viewset to allow pagi…

287b3e3

…nating parent filtered queries.

Simplify pagination support in the resource layer to only work by ite…

1fc3f1a

…rating over a non-opaque cursor. Add some simple livequery methods to the resource layer.

Add simple pagination to the contents of topics.

c6e4213

Fix tests.

cf8d7b2

rtibbles force-pushed the paginate_topics branch from 0cba194 to cf8d7b2 Compare August 20, 2024 16:11

marcellamaki approved these changes Aug 20, 2024

View reviewed changes

rtibbles merged commit 892ab07 into learningequality:unstable Aug 20, 2024
13 checks passed

rtibbles deleted the paginate_topics branch August 20, 2024 19:13

akolson mentioned this pull request Aug 24, 2024

Bulk Edit Release #4635

Merged

akolson mentioned this pull request Oct 1, 2024

Release v2024.10.02 #4776

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Paginate topics #4616

Paginate topics #4616

rtibbles commented Jul 24, 2024

marcellamaki left a comment

marcellamaki Jul 31, 2024

rtibbles Jul 31, 2024

rtibbles Jul 31, 2024

rtibbles Aug 6, 2024

marcellamaki Jul 31, 2024

rtibbles Jul 31, 2024

rtibbles Aug 6, 2024

marcellamaki Aug 12, 2024

rtibbles Aug 19, 2024

radinamatic commented Aug 27, 2024

rtibbles commented Aug 27, 2024

Paginate topics #4616

Paginate topics #4616

Conversation

rtibbles commented Jul 24, 2024

Summary

Description of the change(s) you made

Manual verification steps performed

Screenshot

References

Comments

Contributor's Checklist

Reviewer's Checklist

This section is for reviewers to fill out.

marcellamaki 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

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

radinamatic commented Aug 27, 2024

rtibbles commented Aug 27, 2024