From 010153fea1b87a8de15dee21afb948315b7be98f Mon Sep 17 00:00:00 2001 From: Wojciech Boman Date: Thu, 23 Nov 2023 10:59:49 +0100 Subject: [PATCH] Refactor PersistedRequests --- src/libs/Network/SequentialQueue.ts | 2 +- src/libs/Network/enhanceParameters.ts | 3 ++ src/libs/actions/PersistedRequests.ts | 14 ++++-- src/libs/actions/Report.js | 9 +++- src/types/onyx/Request.ts | 1 + tests/unit/PersistedRequests.ts | 67 +++++++++++++++++++++++++++ 6 files changed, 88 insertions(+), 8 deletions(-) create mode 100644 tests/unit/PersistedRequests.ts diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index d4aee4a221e5..5da032baaf45 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -160,7 +160,7 @@ NetworkStore.onReconnection(flush); function push(request: OnyxRequest) { // Add request to Persisted Requests so that it can be retried if it fails - PersistedRequests.save([request]); + PersistedRequests.save(request); // If we are offline we don't need to trigger the queue to empty as it will happen when we come back online if (NetworkStore.isOffline()) { diff --git a/src/libs/Network/enhanceParameters.ts b/src/libs/Network/enhanceParameters.ts index 6ff54f94bc88..3fadeea7447c 100644 --- a/src/libs/Network/enhanceParameters.ts +++ b/src/libs/Network/enhanceParameters.ts @@ -37,5 +37,8 @@ export default function enhanceParameters(command: string, parameters: Record request.data?.idempotencyKey && request.data?.idempotencyKey === requestToPersist.data?.idempotencyKey); + if (existingRequestIndex > -1) { + // Merge the new request into the existing one, keeping its place in the queue + requests.splice(existingRequestIndex, 1, requestToPersist); } else { - requests = requestsToPersist; + // If not, push the new request to the end of the queue + requests.push(requestToPersist); } persistedRequests = requests; + Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, requests); } diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index a03488429405..9bc2aa1b3f2f 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -470,6 +470,9 @@ function openReport(reportID, participantLoginList = [], newReportObject = {}, p if (!reportID) { return; } + + const commandName = 'OpenReport'; + const optimisticReportData = [ { onyxMethod: Onyx.METHOD.MERGE, @@ -535,6 +538,7 @@ function openReport(reportID, participantLoginList = [], newReportObject = {}, p emailList: participantLoginList ? participantLoginList.join(',') : '', accountIDList: participantAccountIDList ? participantAccountIDList.join(',') : '', parentReportActionID, + idempotencyKey: `${commandName}_${reportID}`, }; if (isFromDeepLink) { @@ -612,6 +616,7 @@ function openReport(reportID, participantLoginList = [], newReportObject = {}, p // Add the createdReportActionID parameter to the API call params.createdReportActionID = optimisticCreatedAction.reportActionID; + params.idempotencyKey = `${params.idempotencyKey}_NewReport_${optimisticCreatedAction.reportActionID}`; // If we are creating a thread, ensure the report action has childReportID property added if (newReportObject.parentReportID && parentReportActionID) { @@ -632,12 +637,12 @@ function openReport(reportID, participantLoginList = [], newReportObject = {}, p if (isFromDeepLink) { // eslint-disable-next-line rulesdir/no-api-side-effects-method - API.makeRequestWithSideEffects('OpenReport', params, onyxData).finally(() => { + API.makeRequestWithSideEffects(commandName, params, onyxData).finally(() => { Onyx.set(ONYXKEYS.IS_CHECKING_PUBLIC_ROOM, false); }); } else { // eslint-disable-next-line rulesdir/no-multiple-api-calls - API.write('OpenReport', params, onyxData); + API.write(commandName, params, onyxData); } } diff --git a/src/types/onyx/Request.ts b/src/types/onyx/Request.ts index 33105b914b6f..746e7f75b3d5 100644 --- a/src/types/onyx/Request.ts +++ b/src/types/onyx/Request.ts @@ -17,6 +17,7 @@ type RequestData = { shouldUseSecure?: boolean; successData?: OnyxUpdate[]; failureData?: OnyxUpdate[]; + idempotencyKey?: string; resolve?: (value: Response) => void; reject?: (value?: unknown) => void; diff --git a/tests/unit/PersistedRequests.ts b/tests/unit/PersistedRequests.ts new file mode 100644 index 000000000000..79153efdc806 --- /dev/null +++ b/tests/unit/PersistedRequests.ts @@ -0,0 +1,67 @@ +import * as PersistedRequests from '../../src/libs/actions/PersistedRequests'; +import Request from '../../src/types/onyx/Request'; + +const request: Request = { + command: 'OpenReport', + data: { + idempotencyKey: 'OpenReport_1', + }, + successData: [{key: 'reportMetadata_1', onyxMethod: 'merge', value: {}}], + failureData: [{key: 'reportMetadata_2', onyxMethod: 'merge', value: {}}], +}; + +beforeEach(() => { + PersistedRequests.clear(); + PersistedRequests.save(request); +}); + +afterEach(() => { + PersistedRequests.clear(); +}); + +describe('PersistedRequests', () => { + it('save a new request with an idempotency key which currently exists in the PersistedRequests array', () => { + PersistedRequests.save(request); + expect(PersistedRequests.getAll().length).toBe(1); + }); + + it('save a new request with a new idempotency key', () => { + const newRequest = { + command: 'OpenReport', + data: { + idempotencyKey: 'OpenReport_2', + }, + }; + PersistedRequests.save(newRequest); + expect(PersistedRequests.getAll().length).toBe(2); + }); + + it('replace a request existing in the PersistedRequests array with a new one', () => { + const newRequest: Request = { + command: 'OpenReport', + data: { + idempotencyKey: 'OpenReport_1', + }, + successData: [{key: 'reportMetadata_3', onyxMethod: 'merge', value: {}}], + failureData: [{key: 'reportMetadata_4', onyxMethod: 'merge', value: {}}], + }; + + PersistedRequests.save(newRequest); + + const persistedRequests = PersistedRequests.getAll(); + + expect(persistedRequests.length).toBe(1); + + const mergedRequest = persistedRequests[0]; + + expect(mergedRequest.successData?.length).toBe(1); + expect(mergedRequest.failureData?.length).toBe(1); + expect(mergedRequest.successData?.[0]?.key).toBe('reportMetadata_3'); + expect(mergedRequest.failureData?.[0]?.key).toBe('reportMetadata_4'); + }); + + it('remove a request from the PersistedRequests array', () => { + PersistedRequests.remove(request); + expect(PersistedRequests.getAll().length).toBe(0); + }); +});