diff --git a/contentcuration/contentcuration/frontend/channelEdit/views/CurrentTopicView.vue b/contentcuration/contentcuration/frontend/channelEdit/views/CurrentTopicView.vue index 7cfec94ce6..afcd143a3a 100644 --- a/contentcuration/contentcuration/frontend/channelEdit/views/CurrentTopicView.vue +++ b/contentcuration/contentcuration/frontend/channelEdit/views/CurrentTopicView.vue @@ -172,7 +172,11 @@ @deselect="selected = selected.filter(id => id !== $event)" @scroll="scroll" @editTitleDescription="showTitleDescriptionModal" - /> + > + + + diff --git a/contentcuration/contentcuration/frontend/channelEdit/views/TreeView/index.spec.js b/contentcuration/contentcuration/frontend/channelEdit/views/TreeView/index.spec.js index 470e67a89d..4ec3d01d61 100644 --- a/contentcuration/contentcuration/frontend/channelEdit/views/TreeView/index.spec.js +++ b/contentcuration/contentcuration/frontend/channelEdit/views/TreeView/index.spec.js @@ -22,7 +22,7 @@ const GETTERS = { canManage: jest.fn(() => true), }, contentNode: { - getContentNodeChildren: () => jest.fn(() => []), + getContentNodeChildren: () => jest.fn(() => ({ results: [], more: null })), getContentNodeAncestors: () => jest.fn(() => []), getContentNode: () => jest.fn(() => ({})), getTopicAndResourceCounts: () => jest.fn(() => ({ topicCount: 0, resourceCount: 0 })), @@ -35,7 +35,7 @@ const ACTIONS = { loadContentNode: jest.fn(), headContentNode: () => jest.fn(), loadContentNodes: jest.fn(), - loadChildren: jest.fn(), + loadChildren: jest.fn(() => ({ results: [], more: null })), }, }; diff --git a/contentcuration/contentcuration/frontend/channelEdit/views/TreeView/index.vue b/contentcuration/contentcuration/frontend/channelEdit/views/TreeView/index.vue index 4ca0ca5f1f..b1cd0a67f7 100644 --- a/contentcuration/contentcuration/frontend/channelEdit/views/TreeView/index.vue +++ b/contentcuration/contentcuration/frontend/channelEdit/views/TreeView/index.vue @@ -128,6 +128,13 @@ /> + @@ -192,6 +199,8 @@ }, loading: true, listElevated: false, + more: null, + moreLoading: false, }; }, computed: { @@ -285,25 +294,30 @@ }, }, created() { - let childrenPromise; - // If viewing the root-level node, don't request anything, since the NodePanel.created - // hook will make a redundant request - if (this.nodeId === this.rootId) { - childrenPromise = Promise.resolve(); - } else { - childrenPromise = this.loadContentNodes({ parent: this.rootId }); - } - Promise.all([childrenPromise, this.loadAncestors({ id: this.nodeId })]).then(() => { - this.loading = false; - this.jumpToLocation(); - }); + const childrenPromise = this.loadChildren({ parent: this.rootId }); + Promise.all([childrenPromise, this.loadAncestors({ id: this.nodeId })]).then( + ([childrenResponse]) => { + this.loading = false; + this.more = childrenResponse.more || null; + this.jumpToLocation(); + } + ); }, methods: { ...mapMutations('contentNode', { collapseAll: 'COLLAPSE_ALL_EXPANDED', setExpanded: 'SET_EXPANSION', }), - ...mapActions('contentNode', ['loadAncestors', 'loadContentNodes']), + ...mapActions('contentNode', ['loadAncestors', 'loadChildren', 'loadContentNodes']), + loadMore() { + if (this.more && !this.moreLoading) { + this.moreLoading = true; + this.loadContentNodes(this.more).then(response => { + this.more = response.more || null; + this.moreLoading = false; + }); + } + }, verifyContentNodeId(id) { this.nodeNotFound = false; return this.$store.dispatch('contentNode/headContentNode', id).catch(() => { @@ -400,6 +414,7 @@ openCurrentLocationButton: 'Expand to current folder location', updatedResourcesReadyForReview: 'Updated resources are ready for review', closeDrawer: 'Close', + showMore: 'Show more', }, }; @@ -447,4 +462,10 @@ } } + .pagination-container { + display: flex; + justify-content: flex-start; + margin: 4px; + } + diff --git a/contentcuration/contentcuration/frontend/channelEdit/vuex/contentNode/actions.js b/contentcuration/contentcuration/frontend/channelEdit/vuex/contentNode/actions.js index c9ec87bc1f..6c42af8974 100644 --- a/contentcuration/contentcuration/frontend/channelEdit/vuex/contentNode/actions.js +++ b/contentcuration/contentcuration/frontend/channelEdit/vuex/contentNode/actions.js @@ -19,9 +19,10 @@ import * as publicApi from 'shared/data/public'; import db from 'shared/data/db'; export function loadContentNodes(context, params = {}) { - return ContentNode.where(params).then(contentNodes => { + return ContentNode.where(params).then(response => { + const contentNodes = response.results ? response.results : response; context.commit('ADD_CONTENTNODES', contentNodes); - return contentNodes; + return response; }); } @@ -70,7 +71,7 @@ export function loadContentNodeByNodeId(context, nodeId) { } export function loadChildren(context, { parent, published = null, complete = null }) { - const params = { parent }; + const params = { parent, max_results: 25 }; if (published !== null) { params.published = published; } diff --git a/contentcuration/contentcuration/frontend/shared/data/constants.js b/contentcuration/contentcuration/frontend/shared/data/constants.js index 1785192a60..b6dc4a176b 100644 --- a/contentcuration/contentcuration/frontend/shared/data/constants.js +++ b/contentcuration/contentcuration/frontend/shared/data/constants.js @@ -29,6 +29,7 @@ export const CHANGE_TYPES_LOOKUP = invert(CHANGE_TYPES); // Tables export const CHANGES_TABLE = 'changesForSyncing'; +export const PAGINATION_TABLE = 'pagination'; export const TABLE_NAMES = { SESSION: 'session', diff --git a/contentcuration/contentcuration/frontend/shared/data/index.js b/contentcuration/contentcuration/frontend/shared/data/index.js index 705f84b619..99bde2bec4 100644 --- a/contentcuration/contentcuration/frontend/shared/data/index.js +++ b/contentcuration/contentcuration/frontend/shared/data/index.js @@ -1,6 +1,6 @@ import * as Sentry from '@sentry/vue'; import mapValues from 'lodash/mapValues'; -import { CHANGES_TABLE, TABLE_NAMES } from './constants'; +import { CHANGES_TABLE, PAGINATION_TABLE, TABLE_NAMES } from './constants'; import db from './db'; import { INDEXEDDB_RESOURCES } from './registry'; import { startSyncing, stopSyncing, syncOnChanges } from './serverSync'; @@ -14,7 +14,10 @@ export function setupSchema() { if (!Object.keys(resources).length) { console.warn('No resources defined!'); // eslint-disable-line no-console } - // Version incremented to 3 to add new index on CHANGES_TABLE. + // Version incremented to 2 to add Bookmark table and new index on CHANGES_TABLE. + // Version incremented to 3 to add: + // new index on CHANGES_TABLE. + // PAGINATION_TABLE. db.version(3).stores({ // A special table for logging unsynced changes // Dexie.js appears to have a table for this, @@ -22,6 +25,7 @@ export function setupSchema() { // that I do not currently understand, so we engage // in somewhat duplicative behaviour instead. [CHANGES_TABLE]: 'rev++,[table+key],server_rev,type', + [PAGINATION_TABLE]: '[table+queryString]', ...mapValues(INDEXEDDB_RESOURCES, value => value.schema), }); } diff --git a/contentcuration/contentcuration/frontend/shared/data/resources.js b/contentcuration/contentcuration/frontend/shared/data/resources.js index c20f18b950..1c684ee07a 100644 --- a/contentcuration/contentcuration/frontend/shared/data/resources.js +++ b/contentcuration/contentcuration/frontend/shared/data/resources.js @@ -3,11 +3,9 @@ import Mutex from 'mutex-js'; import findIndex from 'lodash/findIndex'; import flatMap from 'lodash/flatMap'; import isArray from 'lodash/isArray'; -import isNumber from 'lodash/isNumber'; import isString from 'lodash/isString'; import matches from 'lodash/matches'; import overEvery from 'lodash/overEvery'; -import pick from 'lodash/pick'; import sortBy from 'lodash/sortBy'; import uniq from 'lodash/uniq'; import uniqBy from 'lodash/uniqBy'; @@ -16,6 +14,7 @@ import { v4 as uuidv4 } from 'uuid'; import { CHANGE_TYPES, CHANGES_TABLE, + PAGINATION_TABLE, RELATIVE_TREE_POSITIONS, TABLE_NAMES, COPYING_STATUS, @@ -60,6 +59,7 @@ const QUERY_SUFFIXES = { }; const ORDER_FIELD = 'ordering'; +const PAGINATION_FIELD = 'max_results'; const VALID_SUFFIXES = new Set(Object.values(QUERY_SUFFIXES)); @@ -68,60 +68,6 @@ const validPositions = new Set(Object.values(RELATIVE_TREE_POSITIONS)); const EMPTY_ARRAY = Symbol('EMPTY_ARRAY'); -class Paginator { - constructor(params) { - // Get parameters for page number based pagination - Object.assign(this, pick(params, 'page', 'page_size')); - // At a minimum, this pagination style requires a page_size - // parameter, so we check to see if that exists. - if (this.page_size) { - this.pageNumberType = true; - this.page = this.page || 1; - } - // Get parameters for limit offset pagination - Object.assign(this, pick(params, 'limit', 'offset')); - // At a minimum, this pagination style requires a limit - // parameter, so we check to see if that exists. - if (this.limit) { - this.limitOffsetType = true; - this.offset = this.offset || 0; - } - if (this.pageNumberType && this.limitOffsetType) { - console.warn( - 'Specified both page number type pagination and limit offset may get unexpected results' - ); - } - } - paginate(collection) { - let offset; - let limit; - if (this.pageNumberType) { - offset = (this.page - 1) * this.page_size; - limit = this.page_size; - } - if (this.limitOffsetType) { - offset = this.offset; - limit = this.limit; - } - if (isNumber(offset) && isNumber(limit)) { - const countPromise = collection.count(); - const resultPromise = collection - .offset(offset) - .limit(limit) - .toArray(); - return Promise.all([countPromise, resultPromise]).then(([count, results]) => { - const out = { count, results }; - if (this.pageNumberType) { - out.total_pages = Math.ceil(count / this.page_size); - out.page = this.page; - } - return out; - }); - } - return collection.toArray(); - } -} - let vuexStore; export function injectVuexStore(store) { @@ -422,7 +368,7 @@ class IndexedDBResource { }); } - where(params = {}) { + async where(params = {}) { const table = db[this.tableName]; // Indexed parameters const whereParams = {}; @@ -436,9 +382,17 @@ class IndexedDBResource { let sortBy; let reverse; - // Setup paginator. - const paginator = new Paginator(params); + // Check for pagination + const maxResults = Number(params[PAGINATION_FIELD]); + const paginationActive = !isNaN(maxResults); + if (paginationActive && !params[ORDER_FIELD]) { + params[ORDER_FIELD] = this.defaultOrdering; + } for (const key of Object.keys(params)) { + if (key === PAGINATION_FIELD) { + // Don't filter by parameters that are used for pagination + continue; + } // Partition our parameters const [rootParam, suffix] = key.split(SUFFIX_SEPERATOR); if (suffix && VALID_SUFFIXES.has(suffix) && suffix !== QUERY_SUFFIXES.IN) { @@ -460,8 +414,7 @@ class IndexedDBResource { } else { sortBy = ordering; } - } else if (!paginator[key]) { - // Don't filter by parameters that are used for pagination + } else { filterParams[rootParam] = params[key]; } } @@ -515,8 +468,11 @@ class IndexedDBResource { sortBy = null; } } else { - if (sortBy && this.indexFields.has(sortBy) && !reverse) { + if (sortBy && this.indexFields.has(sortBy)) { collection = table.orderBy(sortBy); + if (reverse) { + collection = collection.reverse(); + } sortBy = null; } else { collection = table.toCollection(); @@ -568,13 +524,40 @@ class IndexedDBResource { if (filterFn) { collection = collection.filter(filterFn); } + if (paginationActive) { + let results; + if (sortBy) { + // If we still have a sortBy value here, then we have not sorted using orderBy + // so we need to sort here. + if (reverse) { + collection = collection.reverse(); + } + results = (await collection.sortBy(sortBy)).slice(0, maxResults + 1); + } else { + results = await collection.limit(maxResults + 1).toArray(); + } + const hasMore = results.length > maxResults; + return { + results: results.slice(0, maxResults), + more: hasMore + ? { + ...params, + lft__gt: results[maxResults - 1].lft, + } + : null, + }; + } if (sortBy) { if (reverse) { collection = collection.reverse(); } collection = collection.sortBy(sortBy); } - return paginator.paginate(collection); + return collection.toArray(); + } + + whereLiveQuery(params = {}) { + return Dexie.liveQuery(() => this.where(params)); } get(id) { @@ -769,6 +752,21 @@ class Resource extends mix(APIResource, IndexedDBResource) { this._requests = {}; } + savePagination(queryString, more) { + if (more) { + return this.transaction({ mode: 'rw' }, PAGINATION_TABLE, () => { + return db[PAGINATION_TABLE].put({ table: this.tableName, queryString, more }); + }); + } + return Promise.resolve(); + } + + loadPagination(queryString) { + return db[PAGINATION_TABLE].get([this.tableName, queryString]).then(pagination => { + return pagination ? pagination.more : null; + }); + } + fetchCollection(params) { const now = Date.now(); const queryString = paramsSerializer(params); @@ -784,16 +782,21 @@ class Resource extends mix(APIResource, IndexedDBResource) { const promise = client.get(this.collectionUrl(), { params }).then(response => { let itemData; let pageData; + let more; if (Array.isArray(response.data)) { itemData = response.data; } else if (response.data && response.data.results) { pageData = response.data; itemData = pageData.results; + more = pageData.more; } else { console.error(`Unexpected response from ${this.urlName}`, response); itemData = []; } - return this.setData(itemData).then(data => { + const paginationPromise = pageData + ? this.savePagination(queryString, more) + : Promise.resolve(); + return Promise.all([this.setData(itemData), paginationPromise]).then(([data]) => { // setData also applies any outstanding local change events to the data // so we return the data returned from setData to make sure the most up to date // representation is returned from the fetch. @@ -813,6 +816,50 @@ class Resource extends mix(APIResource, IndexedDBResource) { return promise; } + conditionalFetch(objs, params, doRefresh) { + if (objs === EMPTY_ARRAY) { + return []; + } + // if there are no objects, and it's also not an empty paginated response (objs.results), + // or we mean to refresh + if ((!objs.length && !objs.results?.length) || doRefresh) { + let refresh = Promise.resolve(true); + // ContentNode tree operations are the troublemakers causing the logic below + if (this.tableName === TABLE_NAMES.CONTENTNODE) { + // Only fetch new updates if we don't have pending changes to ContentNode that + // affect local tree structure + refresh = db[CHANGES_TABLE].where('table') + .equals(TABLE_NAMES.CONTENTNODE) + .filter(c => { + if (!TREE_CHANGE_TYPES.includes(c.type)) { + return false; + } + let parent = c.parent; + if (c.type === CHANGE_TYPES.CREATED) { + parent = c.obj.parent; + } + return ( + params.parent === parent || + params.parent === c.key || + (params.id__in || []).includes(c.key) + ); + }) + .count() + .then(pendingCount => pendingCount === 0); + } + + const fetch = refresh.then(shouldFetch => { + const emptyResults = isArray(objs) ? [] : { results: [] }; + return shouldFetch ? this.fetchCollection(params) : emptyResults; + }); + // Be sure to return the fetch promise to relay fetched objects in this condition + if (!objs.length && !objs.results?.length) { + return fetch; + } + } + return objs; + } + /** * @param {Object} params * @param {Boolean} [doRefresh=true] -- Whether or not to refresh async from server @@ -826,48 +873,37 @@ class Resource extends mix(APIResource, IndexedDBResource) { console.groupEnd(); /* eslint-enable */ } - return super.where(params).then(objs => { - if (objs === EMPTY_ARRAY) { - return []; + const paginationLoadPromise = params[PAGINATION_FIELD] + ? this.loadPagination(paramsSerializer(params)) + : Promise.resolve(null); + return Promise.all([super.where(params), paginationLoadPromise]).then(([objs, more]) => { + objs = this.conditionalFetch(objs, params, doRefresh); + if (!isArray(objs) && !objs.more && more) { + objs.more = more; } - // if there are no objects, and it's also not an empty paginated response (objs.count), - // or we mean to refresh - if ((!objs.length && !objs.count) || doRefresh) { - let refresh = Promise.resolve(true); - // ContentNode tree operations are the troublemakers causing the logic below - if (this.tableName === TABLE_NAMES.CONTENTNODE) { - // Only fetch new updates if we don't have pending changes to ContentNode that - // affect local tree structure - refresh = db[CHANGES_TABLE].where('table') - .equals(TABLE_NAMES.CONTENTNODE) - .filter(c => { - if (!TREE_CHANGE_TYPES.includes(c.type)) { - return false; - } - let parent = c.parent; - if (c.type === CHANGE_TYPES.CREATED) { - parent = c.obj.parent; - } - return ( - params.parent === parent || - params.parent === c.key || - (params.id__in || []).includes(c.key) - ); - }) - .count() - .then(pendingCount => pendingCount === 0); - } + return objs; + }); + } - const fetch = refresh.then(shouldFetch => { - return shouldFetch ? this.fetchCollection(params) : []; - }); - // Be sure to return the fetch promise to relay fetched objects in this condition - if (!objs.length && !objs.count) { - return fetch; + whereLiveQuery(params = {}, doRefresh = true) { + if (process.env.NODE_ENV !== 'production' && process.env.NODE_ENV !== 'test') { + /* eslint-disable no-console */ + console.groupCollapsed(`Getting liveQuery for ${this.tableName} table with params: `, params); + console.trace(); + console.groupEnd(); + /* eslint-enable */ + } + const observable = Dexie.liveQuery(() => super.where(params)); + let fetched = false; + observable.subscribe({ + next: objs => { + if (!fetched) { + fetched = true; + this.conditionalFetch(objs, params, doRefresh); } - } - return objs; + }, }); + return observable; } headModel(id) { @@ -1270,6 +1306,7 @@ export const ContentNode = new TreeResource({ '[root_id+parent]', '[node_id+channel_id]', ], + defaultOrdering: 'lft', addPrerequisite(target_node, prerequisite) { if (target_node === prerequisite) { diff --git a/contentcuration/contentcuration/utils/pagination.py b/contentcuration/contentcuration/utils/pagination.py index d8da8699cc..2a6fb8c946 100644 --- a/contentcuration/contentcuration/utils/pagination.py +++ b/contentcuration/contentcuration/utils/pagination.py @@ -153,13 +153,13 @@ def paginate_queryset(self, queryset, request, view=None): *ordering ) - def get_more(self): # noqa C901 + def _get_more_position_offset(self): """ Vendored and modified from https://github.com/encode/django-rest-framework/blob/6ea95b6ad1bc0d4a4234a267b1ba32701878c6bb/rest_framework/pagination.py#L694 """ if not self.has_next: - return None + return None, None if ( self.page @@ -212,6 +212,13 @@ def get_more(self): # noqa C901 if not self.page: position = self.next_position + return position, offset + + def get_more(self): + position, offset = self._get_more_position_offset() + if position is None and offset is None: + return None + tokens = {} if offset != 0: tokens["o"] = str(offset) diff --git a/contentcuration/contentcuration/viewsets/contentnode.py b/contentcuration/contentcuration/viewsets/contentnode.py index 7e4e7b3edb..b15b717860 100644 --- a/contentcuration/contentcuration/viewsets/contentnode.py +++ b/contentcuration/contentcuration/viewsets/contentnode.py @@ -29,6 +29,8 @@ from le_utils.constants.labels import resource_type from le_utils.constants.labels import subjects from rest_framework.decorators import action +from rest_framework.pagination import Cursor +from rest_framework.pagination import replace_query_param from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response from rest_framework.serializers import BooleanField @@ -55,6 +57,7 @@ from contentcuration.tasks import calculate_resource_size_task from contentcuration.utils.nodes import calculate_resource_size from contentcuration.utils.nodes import migrate_extra_fields +from contentcuration.utils.pagination import ValuesViewsetCursorPagination from contentcuration.viewsets.base import BulkListSerializer from contentcuration.viewsets.base import BulkModelSerializer from contentcuration.viewsets.base import BulkUpdateMixin @@ -651,12 +654,56 @@ def dict_if_none(obj, field_name=None): return obj[field_name] if field_name in obj and obj[field_name] else {} +class ContentNodePagination(ValuesViewsetCursorPagination): + """ + A simplified cursor pagination class for ContentNodeViewSet. + Instead of using an opaque cursor, it uses the lft value for filtering. + As such, if this pagination scheme is used without applying a filter + that will guarantee membership to a specific MPTT tree, such as parent + or tree_id, the pagination scheme will not be predictable. + """ + cursor_query_param = "lft__gt" + ordering = "lft" + page_size_query_param = "max_results" + max_page_size = 100 + + def decode_cursor(self, request): + """ + Given a request with a cursor, return a `Cursor` instance. + """ + # Determine if we have a cursor, and if so then decode it. + value = request.query_params.get(self.cursor_query_param) + if value is None: + return None + + return Cursor(offset=0, reverse=False, position=value) + + def encode_cursor(self, cursor): + """ + Given a Cursor instance, return an url with query parameter. + """ + return replace_query_param(self.base_url, self.cursor_query_param, str(cursor.position)) + + def get_more(self): + position, offset = self._get_more_position_offset() + if position is None and offset is None: + return None + params = self.request.query_params.copy() + params.update({ + self.cursor_query_param: position, + }) + return params + + # Apply mixin first to override ValuesViewset class ContentNodeViewSet(BulkUpdateMixin, ValuesViewset): queryset = ContentNode.objects.all() serializer_class = ContentNodeSerializer permission_classes = [IsAuthenticated] filterset_class = ContentNodeFilter + pagination_class = ContentNodePagination + # This must exactly match the ordering on the pagination class defined above. + ordering = ["lft"] values = ( "id", "content_id",