diff --git a/app/src/api/services/__tests__/editHistory.test.ts b/app/src/api/services/__tests__/editHistory.test.ts index 45963ea2..9876d5ac 100644 --- a/app/src/api/services/__tests__/editHistory.test.ts +++ b/app/src/api/services/__tests__/editHistory.test.ts @@ -1,5 +1,6 @@ import { EditHistoryEntry } from '../../../frontend/models/edit-history-entry'; import * as editHistoryData from '../../dataAccess/editHistory'; // manually mocked +import { UserInputError } from '../../errors'; import { getGlobalEditHistory } from '../editHistory'; jest.mock('../../dataAccess/editHistory'); @@ -19,10 +20,16 @@ function generateHistory(n: number, firstId: number = 100) { describe('getGlobalEditHistory()', () => { - beforeEach(() => { - mockedEditHistoryData.__setHistory( - generateHistory(20) - ); + beforeEach(() => mockedEditHistoryData.__setHistory(generateHistory(20))); + + afterEach(() => jest.clearAllMocks()); + + it.each([ + [null, null], + ['100', null], + [null, '100'] + ])('Should error when requesting non-positive number of records', (beforeId: string, afterId: string) => { + expect(getGlobalEditHistory(beforeId, afterId, 0)).rejects.toBeInstanceOf(UserInputError); }); describe('getting history before a point', () => { @@ -37,7 +44,9 @@ describe('getGlobalEditHistory()', () => { [ [null, 3, ['119', '118', '117']], [null, 6, ['119', '118', '117', '116', '115', '114']], - ['118', 0, []], + ['118', 1, ['117']], + ['104', 10, ['103', '102','101', '100']], + ['100', 2, []] ] )('should return the N records before the specified ID in descending order [beforeId: %p, count: %p]', async ( beforeId: string, count: number, ids: string[] @@ -48,43 +57,39 @@ describe('getGlobalEditHistory()', () => { }); it.each([ - [null, 4, true, false], - [null, 10, true, false], - [null, 20, false, false], - [null, 30, false, false], - ['50', 10, false, true], - ['100', 10, false, true], - ['105', 2, true, true], - ])('should indicate if there are any newer or older records [beforeId: %p, count: %p]', async ( - beforeId: string, count: number, hasOlder: boolean, hasNewer: boolean + [null, 4, null], + [null, 10, null], + [null, 20, null], + [null, 30, null], + ['50', 10, '99'], + ['100', 10, '99'], + ['130', 10, null], + ['105', 2, '104'], + ['120', 20, null], + ])('should detect if there are any newer records left [beforeId: %p, count: %p]', async ( + beforeId: string, count: number, idForNewerQuery: string ) => { const result = await getGlobalEditHistory(beforeId, null, count); - expect(result.paging.has_older).toBe(hasOlder); - expect(result.paging.has_newer).toBe(hasNewer); + expect(result.paging.id_for_newer_query).toBe(idForNewerQuery); }); + it.each([ + [null, 4, '116'], + [null, 10, '110'], + [null, 20, null], + [null, 30, null], + ['50', 10, null], + ['100', 10, null], + ['130', 10, '110'], + ['105', 2, '103'], + ['120', 20, null], + ])('should detect if there are any older records left [beforeId: %p, count: %p]', async ( + beforeId: string, count: number, idForOlderQuery: string + ) => { + const result = await getGlobalEditHistory(beforeId, null, count); - it('should not return more than 100 entries', async () => { - mockedEditHistoryData.__setHistory( - generateHistory(200) - ); - - const result = await getGlobalEditHistory(null, null, 200); - - expect(result.paging.has_older).toBeTruthy(); - expect(result.history.length).toBe(100); - }); - - it('should default to 100 entries', async () => { - mockedEditHistoryData.__setHistory( - generateHistory(200) - ); - - const result = await getGlobalEditHistory(null, null, 200); - - expect(result.paging.has_older).toBeTruthy(); - expect(result.history.length).toBe(100); + expect(result.paging.id_for_older_query).toBe(idForOlderQuery); }); }); @@ -104,17 +109,66 @@ describe('getGlobalEditHistory()', () => { }); it.each([ - ['99', 10, false, true], - ['110', 5, true, true], - ['119', 20, true, false], - ['99', 20, false, false] - ])('should indicate if there are any newer or older records [afterId: %p, count: %p]', async ( - afterId: string, count: number, hasOlder:boolean, hasNewer: boolean + ['99', 10, '109'], + ['110', 5, '115'], + ['119', 20, null], + ['99', 20, null], + ])('should detect if there are any newer records left [afterId: %p, count: %p]', async ( + afterId: string, count: number, idForNewerQuery: string ) => { const result = await getGlobalEditHistory(null, afterId, count); - expect(result.paging.has_older).toBe(hasOlder); - expect(result.paging.has_newer).toBe(hasNewer); + expect(result.paging.id_for_newer_query).toBe(idForNewerQuery); }); + + it.each([ + ['99', 10, null], + ['110', 5, '111'], + ['119', 20, '120'], + ['99', 20, null], + ])('should detect if there are any older records left [afterId: %p, count: %p]', async ( + afterId: string, count: number, idForOlderQuery: string + ) => { + const result = await getGlobalEditHistory(null, afterId, count); + + expect(result.paging.id_for_older_query).toBe(idForOlderQuery); + }); + + }); + + describe('result count limit', () => { + + it.each([ + [null, null], + [null, '100'], + ['300', null] + ])('should not return more than 100 entries (beforeId: %p, afterId: %p)', async ( + beforeId: string, afterId: string + ) => { + mockedEditHistoryData.__setHistory( + generateHistory(200) + ); + + const result = await getGlobalEditHistory(beforeId, afterId, 200); + + expect(result.history.length).toBe(100); + }); + + it.each([ + [null, null], + [null, '100'], + ['300', null] + ])('should default to 100 entries', async ( + beforeId: string, afterId: string + ) => { + mockedEditHistoryData.__setHistory( + generateHistory(200) + ); + + const result = await getGlobalEditHistory(beforeId, afterId); + + expect(result.history.length).toBe(100); + }); + }); }); diff --git a/app/src/api/services/editHistory.ts b/app/src/api/services/editHistory.ts index 58edae16..40299fe1 100644 --- a/app/src/api/services/editHistory.ts +++ b/app/src/api/services/editHistory.ts @@ -1,7 +1,10 @@ import { EditHistoryEntry } from '../../frontend/models/edit-history-entry'; +import { decBigInt, incBigInt } from '../../helpers'; import { getHistoryAfterId, getHistoryBeforeId, getIdNewerThan, getIdOlderThan } from '../dataAccess/editHistory'; +import { UserInputError } from '../errors'; async function getGlobalEditHistory(beforeId?: string, afterId?: string, count: number = 100) { + if(count <= 0) throw new UserInputError('cannot request less than 1 history record'); if(count > 100) count = 100; // limited set of records. Expected to be already ordered from newest to oldest @@ -13,22 +16,20 @@ async function getGlobalEditHistory(beforeId?: string, afterId?: string, count: editHistoryRecords = await getHistoryBeforeId(beforeId, count); } - const currentBatchNewerBound = editHistoryRecords[0]?.revision_id ?? beforeId; - const newer = currentBatchNewerBound && await getIdNewerThan(currentBatchNewerBound); + const currentBatchMaxId = editHistoryRecords[0]?.revision_id ?? decBigInt(beforeId); + const newer = currentBatchMaxId && await getIdNewerThan(currentBatchMaxId); - const currentBatchOlderBound = editHistoryRecords[editHistoryRecords.length-1]?.revision_id ?? afterId; - const older = currentBatchOlderBound && await getIdOlderThan(currentBatchOlderBound); - - const hasNewer = newer != undefined; - const hasOlder = older != undefined; + const currentBatchMinId = editHistoryRecords[editHistoryRecords.length-1]?.revision_id ?? incBigInt(afterId); + const older = currentBatchMinId && await getIdOlderThan(currentBatchMinId); + + const idForOlderQuery = older != undefined ? incBigInt(older) : null; + const idForNewerQuery = newer != undefined ? decBigInt(newer) : null; return { history: editHistoryRecords, paging: { - has_newer: hasNewer, - id_for_newer_query: null, - has_older: hasOlder, - id_for_older_query: null + id_for_newer_query: idForNewerQuery, + id_for_older_query: idForOlderQuery } }; } diff --git a/app/src/helpers.ts b/app/src/helpers.ts index 6f156561..905b59d9 100644 --- a/app/src/helpers.ts +++ b/app/src/helpers.ts @@ -46,3 +46,15 @@ export function numAsc(accessor: AccessorFunction< export function numDesc(accessor: AccessorFunction): CompareFunction { return (a: T, b: T) => Number(accessor(b) - accessor(a)); } + +/** + * As of Jan 2020, bigint literals e.g. 1n can only be used with TS target esnext + * which then doesn't transpile the null conditional/coalescing operators and breaks on Node 12 + * So BigInt(1) needs to be used here + * */ +export function incBigInt(bigStr: string): string { + return bigStr == undefined ? bigStr : String(BigInt(bigStr) + BigInt(1)); +} +export function decBigInt(bigStr: string): string { + return bigStr == undefined ? bigStr : String(BigInt(bigStr) - BigInt(1)); +}