From 472455daf5d0b9018891d4485ae6f6a120283ecc Mon Sep 17 00:00:00 2001 From: Spencer Spenst Date: Tue, 19 Mar 2024 17:00:48 -0700 Subject: [PATCH] fix slice targetLevelIndex bug --- .../collection/collectionScrollList.tsx | 2 +- lib/initializeLocalDb.ts | 3 +- pages/api/collection-by-id/[id].ts | 68 +++++++------- .../api/collection/collection.byid.test.ts | 88 +++++++++++++++++++ 4 files changed, 126 insertions(+), 35 deletions(-) diff --git a/components/collection/collectionScrollList.tsx b/components/collection/collectionScrollList.tsx index e7f0616b1..8a2562c59 100644 --- a/components/collection/collectionScrollList.tsx +++ b/components/collection/collectionScrollList.tsx @@ -50,7 +50,7 @@ export default function CollectionScrollList({ collection, id, isHidden, onLoadi const fetchLevels = useCallback(async (cursor: string, direction: 'before' | 'after') => { const params = new URLSearchParams({ - populateLevelCursor: cursor, + populateAroundId: cursor, populateLevelDirection: direction, }); diff --git a/lib/initializeLocalDb.ts b/lib/initializeLocalDb.ts index 92aa22bf5..946b0ef9b 100644 --- a/lib/initializeLocalDb.ts +++ b/lib/initializeLocalDb.ts @@ -410,8 +410,7 @@ export default async function initializeLocalDb() { name: 'test collection 3', slug: 'bbb/test-collection', userId: new Types.ObjectId(TestId.USER_B), - } - + }, ], { ordered: false } )); diff --git a/pages/api/collection-by-id/[id].ts b/pages/api/collection-by-id/[id].ts index 315454bef..c79594997 100644 --- a/pages/api/collection-by-id/[id].ts +++ b/pages/api/collection-by-id/[id].ts @@ -15,17 +15,17 @@ export default apiWrapper({ GET: { query: { id: ValidObjectId(true), - populateLevelCursor: ValidObjectId(false), + populateAroundId: ValidObjectId(false), populateLevelDirection: ValidEnum(['before', 'after', 'around'], false), } } }, async (req: NextApiRequest, res: NextApiResponse) => { - const { id, populateLevelCursor, populateLevelDirection } = req.query; + const { id, populateAroundId, populateLevelDirection } = req.query; const token = req.cookies?.token; const gameId = getGameIdFromReq(req); const reqUser = token ? await getUserFromToken(token, req) : null; const collection = await getCollection({ matchQuery: { _id: new Types.ObjectId(id as string), gameId: gameId }, - ...(populateLevelCursor ? { populateLevelCursor: new Types.ObjectId(populateLevelCursor as string) } : {}), + ...(populateAroundId ? { populateAroundId: new Types.ObjectId(populateAroundId as string) } : {}), ...(populateLevelDirection ? { populateLevelDirection: populateLevelDirection as 'before' | 'after' | 'around' } : {}), reqUser, }); @@ -43,8 +43,8 @@ interface GetCollectionProps { includeDraft?: boolean; // eslint-disable-next-line @typescript-eslint/no-explicit-any matchQuery: FilterQuery; + populateAroundId?: Types.ObjectId; // target level populateAroundSlug?: string; // target level - populateLevelCursor?: Types.ObjectId; // target level populateLevelData?: boolean; populateLevelDirection?: 'before' | 'after' | 'around'; reqUser: User | null; @@ -72,8 +72,8 @@ export async function getCollections({ includeDraft, matchQuery, reqUser, + populateAroundId, populateAroundSlug, - populateLevelCursor, populateLevelData = true, populateLevelDirection, }: GetCollectionProps): Promise { @@ -211,17 +211,13 @@ export async function getCollections({ } } }, - ...(populateLevelCursor || populateAroundSlug ? [ - // TODO: we should probably pass in an index rather than a level id/slug - // no guarantee the level will be in the collection, and if it isn't we have no good way to continue the infinite scroll - // BUG: targetLevelIndex can be -1 if the level is not found in the collection, - // which will result in an error if we are populating 'before' ($min must be a positive number) + ...(populateAroundId || populateAroundSlug ? [ { $addFields: { targetLevelIndex: { $cond: { - if: populateLevelCursor, - then: { $indexOfArray: ['$levels._id', populateLevelCursor] }, + if: populateAroundId, + then: { $indexOfArray: ['$levels._id', populateAroundId] }, else: { $indexOfArray: ['$levels.slug', populateAroundSlug] }, } }, @@ -230,34 +226,42 @@ export async function getCollections({ { $addFields: { levels: { + // if the target level is not found, return empty array + // this can happen if an invalid cid is passed in the URL, or if the target level is removed from the collection $cond: { - if: { $eq: [populateLevelDirection, 'before'] }, - // populate the target level + at most 5 levels before - then: { - $slice: [ - '$levels', - { $max: [0, { $subtract: ['$targetLevelIndex', populateLevelCount] }] }, - { $min: [populateLevelCount + 1, { $add: ['$targetLevelIndex', 1] }] } - ] - }, + if: { $lt: ['$targetLevelIndex', 0] }, + then: [], else: { $cond: { - if: { $eq: [populateLevelDirection, 'after'] }, - // populate the target level + at most 5 levels after + if: { $eq: [populateLevelDirection, 'before'] }, + // populate the target level + at most 5 levels before then: { - $slice: [ - '$levels', - '$targetLevelIndex', - { $min: [populateLevelCount + 1, { $subtract: [{ $size: '$levels' }, '$targetLevelIndex'] }] } - ] - }, - else: { // 'around' - // populate the target level + at most 5 levels before and 5 levels after $slice: [ '$levels', { $max: [0, { $subtract: ['$targetLevelIndex', populateLevelCount] }] }, - { $min: [populateLevelCount * 2 + 1, { $size: '$levels' }] }, + { $min: [populateLevelCount + 1, { $add: ['$targetLevelIndex', 1] }] } ] + }, + else: { + $cond: { + if: { $eq: [populateLevelDirection, 'after'] }, + // populate the target level + at most 5 levels after + then: { + $slice: [ + '$levels', + '$targetLevelIndex', + { $min: [populateLevelCount + 1, { $subtract: [{ $size: '$levels' }, '$targetLevelIndex'] }] } + ] + }, + else: { // 'around' + // populate the target level + at most 5 levels before and 5 levels after + $slice: [ + '$levels', + { $max: [0, { $subtract: ['$targetLevelIndex', populateLevelCount] }] }, + { $min: [populateLevelCount * 2 + 1, { $size: '$levels' }] }, + ] + } + } } } } diff --git a/tests/pages/api/collection/collection.byid.test.ts b/tests/pages/api/collection/collection.byid.test.ts index f9d82e8d4..b9987c5de 100644 --- a/tests/pages/api/collection/collection.byid.test.ts +++ b/tests/pages/api/collection/collection.byid.test.ts @@ -1,4 +1,5 @@ import { logger } from '@root/helpers/logger'; +import { EnrichedCollection } from '@root/models/db/collection'; import { enableFetchMocks } from 'jest-fetch-mock'; import { testApiHandler } from 'next-test-api-route-handler'; import { Logger } from 'winston'; @@ -64,6 +65,93 @@ describe('pages/api/collection/[id].ts', () => { }, }); }); + test('GET collection populate before', async () => { + await testApiHandler({ + pagesHandler: async (_, res) => { + const req: NextApiRequestWithAuth = { + method: 'GET', + userId: TestId.USER_B, + cookies: { + token: getTokenCookieValue(TestId.USER_B), + }, + query: { + id: TestId.COLLECTION_B, + populateAroundId: TestId.LEVEL, + populateLevelDirection: 'before', + }, + } as unknown as NextApiRequestWithAuth; + + await getCollectionHandler(req, res); + }, + test: async ({ fetch }) => { + const res = await fetch(); + + expect(res.status).toBe(200); + const response = await res.json() as EnrichedCollection; + + expect(response.levels.length).toBe(1); + expect(response.levels[0]._id).toBe(TestId.LEVEL); + }, + }); + }); + test('GET collection populate after', async () => { + await testApiHandler({ + pagesHandler: async (_, res) => { + const req: NextApiRequestWithAuth = { + method: 'GET', + userId: TestId.USER_B, + cookies: { + token: getTokenCookieValue(TestId.USER_B), + }, + query: { + id: TestId.COLLECTION_B, + populateAroundId: TestId.LEVEL, + populateLevelDirection: 'after', + }, + } as unknown as NextApiRequestWithAuth; + + await getCollectionHandler(req, res); + }, + test: async ({ fetch }) => { + const res = await fetch(); + + expect(res.status).toBe(200); + const response = await res.json() as EnrichedCollection; + + expect(response.levels.length).toBe(2); + expect(response.levels[0]._id).toBe(TestId.LEVEL); + expect(response.levels[1]._id).toBe(TestId.LEVEL_3); + }, + }); + }); + test('GET collection populate not found', async () => { + await testApiHandler({ + pagesHandler: async (_, res) => { + const req: NextApiRequestWithAuth = { + method: 'GET', + userId: TestId.USER_B, + cookies: { + token: getTokenCookieValue(TestId.USER_B), + }, + query: { + id: TestId.COLLECTION_B, + populateAroundId: TestId.LEVEL_4, + populateLevelDirection: 'around', + }, + } as unknown as NextApiRequestWithAuth; + + await getCollectionHandler(req, res); + }, + test: async ({ fetch }) => { + const res = await fetch(); + + expect(res.status).toBe(200); + const response = await res.json() as EnrichedCollection; + + expect(response.levels.length).toBe(0); + }, + }); + }); test('PUT other user\'s collection should 401', async () => { jest.spyOn(logger, 'error').mockImplementation(() => ({} as Logger)); await testApiHandler({