From 42aa69108479383e43b6e83b430d86ae47c515c0 Mon Sep 17 00:00:00 2001 From: Liana Harris <46411498+LianaHarris360@users.noreply.github.com> Date: Mon, 24 Aug 2026 15:59:19 -0500 Subject: [PATCH] Add normalizeParams to return copy with default ordering already applied and call it before lookup. Adds tests covering a cached full page and the final page --- .../shared/data/__tests__/resources.spec.js | 109 +++++++++++++++++- .../frontend/shared/data/resources.js | 34 +++++- 2 files changed, 138 insertions(+), 5 deletions(-) diff --git a/contentcuration/contentcuration/frontend/shared/data/__tests__/resources.spec.js b/contentcuration/contentcuration/frontend/shared/data/__tests__/resources.spec.js index e4333cf52b..63f7c7e5af 100644 --- a/contentcuration/contentcuration/frontend/shared/data/__tests__/resources.spec.js +++ b/contentcuration/contentcuration/frontend/shared/data/__tests__/resources.spec.js @@ -1,7 +1,7 @@ import { UpdatedDescendantsChange } from '../changes'; -import { ViewerM2M, ChannelUser, Channel, ContentNode } from '../resources'; +import { ViewerM2M, ChannelUser, Channel, ContentNode, TreeResource } from '../resources'; import db from 'shared/data/db'; -import { CHANGE_TYPES, TABLE_NAMES } from 'shared/data/constants'; +import { CHANGE_TYPES, PAGINATION_TABLE, TABLE_NAMES } from 'shared/data/constants'; import { ContentKindsNames } from 'shared/leUtils/ContentKinds'; import { mockChannelScope, resetMockChannelScope } from 'shared/utils/testing'; import client from 'shared/client'; @@ -172,6 +172,111 @@ describe('Resources', () => { expect(change.mods).toEqual(changes); }); }); + describe('Paginated where', () => { + const maxResults = 3; + + // A full page of children for parent, plus the more obj the server would send with it + const makePage = parent => { + const results = [1, 2, 3].map(lft => ({ + id: `${parent}-child-${lft}`, + parent, + lft, + title: `test-child-${lft}`, + kind: ContentKindsNames.TOPIC, + })); + return { + results, + more: { parent, max_results: maxResults, ordering: 'lft', lft__gt: maxResults }, + }; + }; + + const mockPage = (parent, { hasMore = true } = {}) => { + const page = makePage(parent); + const more = hasMore ? page.more : null; + jest + .spyOn(client, 'get') + .mockResolvedValue({ data: { results: page.results, more, count: 10 } }); + return { ...page, more }; + }; + + // Loads a page the way loadChildren does, without an explicit ordering + const loadPage = parent => + ContentNode.where({ parent, max_results: maxResults, ordering: null }); + + beforeEach(async () => { + await db[PAGINATION_TABLE].clear(); + // Test setup stubs out fetching for every resource, but saving pagination only happens + // inside the real fetchCollection + jest + .spyOn(ContentNode, 'fetchCollection') + .mockImplementation(params => + TreeResource.prototype.fetchCollection.call(ContentNode, params), + ); + ContentNode._requests = {}; + }); + + afterEach(() => { + // Restore by registry, since a failure in the hook above could leave a spy uninstalled + jest.restoreAllMocks(); + ContentNode._requests = {}; + }); + + it('should return the server "more" object when nothing is cached locally', async () => { + const parent = 'test-uncached-parent-id'; + const { more } = mockPage(parent); + + const response = await ContentNode.where({ parent, max_results: maxResults }); + + expect(response.more).toEqual(more); + }); + + it('should return the saved more obj when the cache holds exactly one full page', async () => { + const parent = 'test-cached-parent-id'; + const { more } = mockPage(parent); + + await loadPage(parent); + + // One cached page is indistinguishable from a complete list locally, so the saved + // pagination has to supply the more obj + const response = await loadPage(parent); + + expect(response.results.map(node => node.id)).toEqual([ + `${parent}-child-1`, + `${parent}-child-2`, + `${parent}-child-3`, + ]); + expect(response.more).toEqual(more); + }); + + it('should not invent a more object when the server said there was nothing more', async () => { + const parent = 'test-final-page-parent-id'; + mockPage(parent, { hasMore: false }); + + await loadPage(parent); + const response = await loadPage(parent); + + expect(response.results).toHaveLength(maxResults); + expect(response.more).toBeNull(); + }); + + it('should return the saved more obj even once children have been removed since it was saved', async () => { + const parent = 'test-shrunken-parent-id'; + const { more } = mockPage(parent); + + await loadPage(parent); + // Nothing invalidates saved pagination when children leave the parent. A more obj that + // fetches nothing beats hiding children that are still there, so it stands until the + // next fetch replaces it. + await db[TABLE_NAMES.CONTENTNODE].delete(`${parent}-child-2`); + await db[TABLE_NAMES.CONTENTNODE].delete(`${parent}-child-3`); + + const response = await loadPage(parent); + + expect(response.results).toHaveLength(1); + expect(response.more).toEqual(more); + }); + }); + describe('ChannelUser resource', () => { const testChannelId = 'test-channel-id'; const testUserId = 'test-user-id'; diff --git a/contentcuration/contentcuration/frontend/shared/data/resources.js b/contentcuration/contentcuration/frontend/shared/data/resources.js index 2a458cd2a3..18b965785c 100644 --- a/contentcuration/contentcuration/frontend/shared/data/resources.js +++ b/contentcuration/contentcuration/frontend/shared/data/resources.js @@ -400,6 +400,23 @@ class IndexedDBResource { }); } + isPaginated(params = {}) { + return !isNaN(Number(params[PAGINATION_FIELD])); + } + + /** + * Returns a copy of the params with implicit values made explicit, so that equivalent queries + * serialize identically. Saved pagination is keyed off that serialization, which is also + * sensitive to key order, so equivalent queries must build their params in the same order. + */ + normalizeParams(params = {}) { + const normalized = { ...params }; + if (this.isPaginated(normalized) && !normalized[ORDER_FIELD] && this.defaultOrdering) { + normalized[ORDER_FIELD] = this.defaultOrdering; + } + return normalized; + } + async where(params = {}) { const table = db[this.tableName]; // Indexed parameters @@ -414,11 +431,17 @@ class IndexedDBResource { let sortBy; let reverse; + params = this.normalizeParams(params); + // Check for pagination const maxResults = Number(params[PAGINATION_FIELD]); - const paginationActive = !isNaN(maxResults); - if (paginationActive && !params[ORDER_FIELD]) { - params[ORDER_FIELD] = this.defaultOrdering; + const paginationActive = this.isPaginated(params); + if (paginationActive && !params[ORDER_FIELD] && process.env.NODE_ENV !== 'production') { + // `normalizeParams` fills in the default ordering, so reaching here means the resource + // has none, and both this page and its cursor will be arbitrary + /* eslint-disable no-console */ + console.warn(`Tried to paginate ${this.tableName} which has no defaultOrdering`); + /* eslint-enable */ } for (const key of Object.keys(params)) { if (key === PAGINATION_FIELD) { @@ -906,6 +929,8 @@ class Resource extends mix(APIResource, IndexedDBResource) { * @return {Promise} */ where(params = {}, doRefresh = true) { + // Normalize before serializing, so this key matches the one `fetchCollection` saves under + params = this.normalizeParams(params); if (process.env.NODE_ENV !== 'production' && process.env.NODE_ENV !== 'test') { /* eslint-disable no-console */ console.groupCollapsed(`Getting data for ${this.tableName} table with params: `, params); @@ -926,6 +951,9 @@ class Resource extends mix(APIResource, IndexedDBResource) { } whereLiveQuery(params = {}, doRefresh = true) { + // `super.where` normalizes its own copy, so without this `conditionalFetch` below would + // fetch under a different key than the query it is refreshing + params = this.normalizeParams(params); 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);