Merge pull request #30425 from software-mansion-labs/refactor-persist…

…ed-requests

Refactor save method in PersistedRequests

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()) {

src/libs/Network/enhanceParameters.ts

@@ -37,5 +37,8 @@ export default function enhanceParameters(command: string, parameters: Record<st
     // Include current user's email in every request and the server logs
     finalParameters.email = parameters.email ?? NetworkStore.getCurrentUserEmail();
 
+    // idempotencyKey declared in JS is front-end-only. We delete it here so it doesn't interfere with idempotency in other layers.
+    delete finalParameters.idempotencyKey;
+
     return finalParameters;
 }

src/libs/actions/PersistedRequests.ts

@@ -17,15 +17,17 @@ function clear() {
     return Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, []);
 }
 
-function save(requestsToPersist: Request[]) {
-    let requests: Request[] = [];
-    if (persistedRequests.length) {
-        requests = persistedRequests.concat(requestsToPersist);
+function save(requestToPersist: Request) {
+    // Check for a request w/ matching idempotencyKey in the queue
+    const existingRequestIndex = persistedRequests.findIndex((request) => 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
+        persistedRequests.splice(existingRequestIndex, 1, requestToPersist);
     } else {
-        requests = requestsToPersist;
+        // If not, push the new request to the end of the queue
+        persistedRequests.push(requestToPersist);
     }
-    persistedRequests = requests;
-    Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, requests);
+    Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests);
 }
 
 function remove(requestToRemove: Request) {

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);
     }
 }
 

src/types/onyx/Request.ts

@@ -15,6 +15,7 @@ type RequestData = {
     shouldUseSecure?: boolean;
     successData?: OnyxUpdate[];
     failureData?: OnyxUpdate[];
+    idempotencyKey?: string;
 
     resolve?: (value: Response) => void;
     reject?: (value?: unknown) => void;

tests/unit/PersistedRequestsTest.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);
+    });
+});