From e38859753d3628edb3a47f4bd6f19a86891bb6bb Mon Sep 17 00:00:00 2001 From: Eduardo Date: Thu, 22 Aug 2024 17:16:55 +0200 Subject: [PATCH 01/22] Updating reconnectApp from the persisted requests queue --- src/libs/actions/PersistedRequests.ts | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts index 851e5387650..310ae2ea3e0 100644 --- a/src/libs/actions/PersistedRequests.ts +++ b/src/libs/actions/PersistedRequests.ts @@ -1,10 +1,12 @@ import isEqual from 'lodash/isEqual'; import Onyx from 'react-native-onyx'; +import {WRITE_COMMANDS} from '@libs/API/types'; import Log from '@libs/Log'; import ONYXKEYS from '@src/ONYXKEYS'; import type {Request} from '@src/types/onyx'; let persistedRequests: Request[] = []; +const keepLastInstance: string[] = [WRITE_COMMANDS.RECONNECT_APP]; Onyx.connect({ key: ONYXKEYS.PERSISTED_REQUESTS, @@ -23,9 +25,22 @@ function getLength(): number { } function save(requestToPersist: Request) { - const requests = [...persistedRequests, requestToPersist]; - persistedRequests = requests; - Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, requests).then(() => { + if (keepLastInstance.includes(requestToPersist.command)) { + // Find the index of an existing request with the same command + const index = persistedRequests.findIndex((request) => request.command === requestToPersist.command); + + if (index !== -1) { + // If found, update the existing request with the new one + persistedRequests[index] = requestToPersist; + } else { + // If not found, add the new request + persistedRequests.push(requestToPersist); + } + } else { + // If the command is not in the keepLastInstance array, add the new request as usual + persistedRequests = [...persistedRequests, requestToPersist]; + } + Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests).then(() => { Log.info(`[SequentialQueue] '${requestToPersist.command}' command queued. Queue length is ${getLength()}`); }); } From 615a8babb3b6f8eb42620aa7a81baded8c55a72f Mon Sep 17 00:00:00 2001 From: Eduardo Date: Thu, 29 Aug 2024 19:02:57 +0200 Subject: [PATCH 02/22] Applying comments + unit tests --- src/libs/Network/SequentialQueue.ts | 6 +++--- src/libs/actions/App.ts | 8 +++----- src/libs/actions/PersistedRequests.ts | 2 -- src/types/onyx/Request.ts | 29 ++++++++++++++++----------- 4 files changed, 23 insertions(+), 22 deletions(-) diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index 9fd65602eca..57bfdc176fe 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -199,11 +199,11 @@ function push(newRequest: OnyxRequest) { const {checkAndFixConflictingRequest} = newRequest; if (checkAndFixConflictingRequest) { - const {conflictAction} = checkAndFixConflictingRequest(requests, newRequest); + const {conflictAction} = checkAndFixConflictingRequest(requests); - if (conflictAction.type === 'save') { + if (conflictAction.type === 'push') { PersistedRequests.save(newRequest); - } else { + } else if (conflictAction.type === 'replace') { PersistedRequests.update(conflictAction.index, newRequest); } } else { diff --git a/src/libs/actions/App.ts b/src/libs/actions/App.ts index 7f7fc95ae5d..331c39c3513 100644 --- a/src/libs/actions/App.ts +++ b/src/libs/actions/App.ts @@ -275,21 +275,19 @@ function reconnectApp(updateIDFrom: OnyxEntry = 0) { } API.write(WRITE_COMMANDS.RECONNECT_APP, params, getOnyxDataForOpenOrReconnect(), { - checkAndFixConflictingRequest: (persistedRequests, newRequest) => { + checkAndFixConflictingRequest: (persistedRequests) => { const index = persistedRequests.findIndex((request) => request.command === WRITE_COMMANDS.RECONNECT_APP); if (index === -1) { return { - request: newRequest, conflictAction: { - type: 'save', + type: 'push', }, }; } return { - request: newRequest, conflictAction: { - type: 'update', + type: 'replace', index, }, }; diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts index 18d66ee9ccb..50a273e07a9 100644 --- a/src/libs/actions/PersistedRequests.ts +++ b/src/libs/actions/PersistedRequests.ts @@ -1,12 +1,10 @@ import isEqual from 'lodash/isEqual'; import Onyx from 'react-native-onyx'; -import {WRITE_COMMANDS} from '@libs/API/types'; import Log from '@libs/Log'; import ONYXKEYS from '@src/ONYXKEYS'; import type {Request} from '@src/types/onyx'; let persistedRequests: Request[] = []; -const keepLastInstance: string[] = [WRITE_COMMANDS.RECONNECT_APP]; Onyx.connect({ key: ONYXKEYS.PERSISTED_REQUESTS, diff --git a/src/types/onyx/Request.ts b/src/types/onyx/Request.ts index 0b583422f73..7f17ae6b9d7 100644 --- a/src/types/onyx/Request.ts +++ b/src/types/onyx/Request.ts @@ -55,9 +55,6 @@ type RequestData = { shouldSkipWebProxy?: boolean; }; -/** Model of requests sent to the API */ -type Request = RequestData & OnyxData & RequestConflictResolver; - /** * Model of a conflict request that has to be updated, in the request queue. */ @@ -65,7 +62,7 @@ type ConflictRequestUpdate = { /** * The action to take in case of a conflict. */ - type: 'update'; + type: 'replace'; /** * The index of the request in the queue to update. @@ -74,28 +71,33 @@ type ConflictRequestUpdate = { }; /** - * Model of a conflict request that has to be saved, in the request queue. + * Model of a conflict request that has to be saved at the end the request queue. */ type ConflictRequestSave = { /** * The action to take in case of a conflict. */ - type: 'save'; + type: 'push'; }; /** - * An object that has the request and the action to take in case of a conflict. + * Model of a conflict request that no need to be updated or saved, in the request queue. */ -type ConflictActionData = { +type ConflictRequestNoAction = { /** - * The request that is conflicting with the new request. + * The action to take in case of a conflict. */ - request: Request; + type: 'noAction'; +}; +/** + * An object that has the request and the action to take in case of a conflict. + */ +type ConflictActionData = { /** * The action to take in case of a conflict. */ - conflictAction: ConflictRequestUpdate | ConflictRequestSave; + conflictAction: ConflictRequestUpdate | ConflictRequestSave | ConflictRequestNoAction; }; /** @@ -106,9 +108,12 @@ type RequestConflictResolver = { /** * A function that checks if a new request conflicts with any existing requests in the queue. */ - checkAndFixConflictingRequest?: (persistedRequest: Request[], request: Request) => ConflictActionData; + checkAndFixConflictingRequest?: (persistedRequest: Request[]) => ConflictActionData; }; +/** Model of requests sent to the API */ +type Request = RequestData & OnyxData & RequestConflictResolver; + /** * An object used to describe how a request can be paginated. */ From 8b505ab4985fb0e555e329048171f107b731936a Mon Sep 17 00:00:00 2001 From: Eduardo Date: Thu, 29 Aug 2024 19:33:27 +0200 Subject: [PATCH 03/22] Adding comment --- src/libs/Network/SequentialQueue.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index 57bfdc176fe..c0173df024a 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -195,6 +195,7 @@ function isPaused(): boolean { NetworkStore.onReconnection(flush); function push(newRequest: OnyxRequest) { + // If a request is already being processed, ignore it when looking for potentially conflicting requests const requests = PersistedRequests.getAll().filter((persistedRequest) => persistedRequest !== currentRequest); const {checkAndFixConflictingRequest} = newRequest; From be72fec72f0797d8c390e861a4f0b229c46795df Mon Sep 17 00:00:00 2001 From: Eduardo Date: Fri, 30 Aug 2024 15:30:31 +0200 Subject: [PATCH 04/22] Adding test to Sequential Queue --- tests/unit/SequentialQueueTest.ts | 147 ++++++++++++++++++++++++++++++ 1 file changed, 147 insertions(+) create mode 100644 tests/unit/SequentialQueueTest.ts diff --git a/tests/unit/SequentialQueueTest.ts b/tests/unit/SequentialQueueTest.ts new file mode 100644 index 00000000000..4a0c4a540e5 --- /dev/null +++ b/tests/unit/SequentialQueueTest.ts @@ -0,0 +1,147 @@ +import Onyx from 'react-native-onyx'; +import * as PersistedRequests from '@userActions/PersistedRequests'; +import ONYXKEYS from '@src/ONYXKEYS'; +import * as SequentialQueue from '../../src/libs/Network/SequentialQueue'; +import type Request from '../../src/types/onyx/Request'; +import * as TestHelper from '../utils/TestHelper'; +import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; + +const request: Request = { + command: 'ReconnectApp', + successData: [{key: 'userMetadata', onyxMethod: 'set', value: {accountID: 1234}}], + failureData: [{key: 'userMetadata', onyxMethod: 'set', value: {}}], +}; + +describe('SequentialQueue', () => { + beforeAll(() => { + Onyx.init({ + keys: ONYXKEYS, + }); + }); + beforeEach(() => { + global.fetch = TestHelper.getGlobalFetchMock(); + return Onyx.clear().then(waitForBatchedUpdates); + }); + + it('should push one request and persist one', () => { + SequentialQueue.push(request); + expect(PersistedRequests.getLength()).toBe(1); + }); + + it('should push two requests and persist two', () => { + SequentialQueue.push(request); + SequentialQueue.push(request); + expect(PersistedRequests.getLength()).toBe(2); + }); + + it('should push two requests with conflict resolution and replace', () => { + SequentialQueue.push(request); + const requestWithConflictResolution: Request = { + command: 'ReconnectApp', + data: {accountID: 56789}, + checkAndFixConflictingRequest: (persistedRequests) => { + // should be one instance of ReconnectApp, get the index to replace it later + const index = persistedRequests.findIndex((r) => r.command === 'ReconnectApp'); + if (index === -1) { + return {conflictAction: {type: 'push'}}; + } + + return { + conflictAction: {type: 'replace', index}, + }; + }, + }; + SequentialQueue.push(requestWithConflictResolution); + expect(PersistedRequests.getLength()).toBe(1); + }); + + it('should push two requests with conflict resolution and push', () => { + SequentialQueue.push(request); + const requestWithConflictResolution: Request = { + command: 'ReconnectApp', + data: {accountID: 56789}, + checkAndFixConflictingRequest: () => { + return {conflictAction: {type: 'push'}}; + }, + }; + SequentialQueue.push(requestWithConflictResolution); + expect(PersistedRequests.getLength()).toBe(2); + }); + + it('should push two requests with conflict resolution and noAction', () => { + SequentialQueue.push(request); + const requestWithConflictResolution: Request = { + command: 'ReconnectApp', + data: {accountID: 56789}, + checkAndFixConflictingRequest: () => { + return {conflictAction: {type: 'noAction'}}; + }, + }; + SequentialQueue.push(requestWithConflictResolution); + expect(PersistedRequests.getLength()).toBe(1); + }); + + it('should add a new request even if a similar one is ongoing', async () => { + // .push at the end flush the queue + SequentialQueue.push(request); + + // wait for Onyx.connect execute the callback and start processing the queue + await Promise.resolve(); + + const requestWithConflictResolution: Request = { + command: 'ReconnectApp', + data: {accountID: 56789}, + checkAndFixConflictingRequest: (persistedRequests) => { + // should be one instance of ReconnectApp, get the index to replace it later + const index = persistedRequests.findIndex((r) => r.command === 'ReconnectApp'); + if (index === -1) { + return {conflictAction: {type: 'push'}}; + } + + return { + conflictAction: {type: 'replace', index}, + }; + }, + }; + + SequentialQueue.push(requestWithConflictResolution); + expect(PersistedRequests.getLength()).toBe(2); + }); + + it.only('should replace request request in queue while a similar one is ongoing', async () => { + // .push at the end flush the queue + SequentialQueue.push(request); + + // wait for Onyx.connect execute the callback and start processing the queue + await Promise.resolve(); + + const conflicyResolver = (persistedRequests: Request[]) => { + // should be one instance of ReconnectApp, get the index to replace it later + const index = persistedRequests.findIndex((r) => r.command === 'ReconnectApp'); + if (index === -1) { + return {conflictAction: {type: 'push'}}; + } + + return { + conflictAction: {type: 'replace', index}, + }; + }; + + const requestWithConflictResolution: Request = { + command: 'ReconnectApp', + data: {accountID: 56789}, + checkAndFixConflictingRequest: conflicyResolver, + }; + + const requestWithConflictResolution2: Request = { + command: 'ReconnectApp', + data: {accountID: 56789}, + checkAndFixConflictingRequest: conflicyResolver, + }; + + SequentialQueue.push(requestWithConflictResolution); + SequentialQueue.push(requestWithConflictResolution2); + + expect(PersistedRequests.getLength()).toBe(2); + }); +}); From 6e40d666187c1237e56c2241348c8af9d498637d Mon Sep 17 00:00:00 2001 From: Eduardo Date: Fri, 30 Aug 2024 15:42:49 +0200 Subject: [PATCH 05/22] Fixed TS error --- src/types/onyx/Request.ts | 2 +- tests/unit/SequentialQueueTest.ts | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/types/onyx/Request.ts b/src/types/onyx/Request.ts index 7f17ae6b9d7..5d37cc559a4 100644 --- a/src/types/onyx/Request.ts +++ b/src/types/onyx/Request.ts @@ -141,4 +141,4 @@ type PaginatedRequest = Request & }; export default Request; -export type {OnyxData, RequestType, PaginationConfig, PaginatedRequest, RequestConflictResolver}; +export type {OnyxData, RequestType, PaginationConfig, PaginatedRequest, RequestConflictResolver, ConflictActionData}; diff --git a/tests/unit/SequentialQueueTest.ts b/tests/unit/SequentialQueueTest.ts index 4a0c4a540e5..1a314903930 100644 --- a/tests/unit/SequentialQueueTest.ts +++ b/tests/unit/SequentialQueueTest.ts @@ -3,6 +3,7 @@ import * as PersistedRequests from '@userActions/PersistedRequests'; import ONYXKEYS from '@src/ONYXKEYS'; import * as SequentialQueue from '../../src/libs/Network/SequentialQueue'; import type Request from '../../src/types/onyx/Request'; +import type {ConflictActionData} from '../../src/types/onyx/Request'; import * as TestHelper from '../utils/TestHelper'; import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; @@ -115,7 +116,7 @@ describe('SequentialQueue', () => { // wait for Onyx.connect execute the callback and start processing the queue await Promise.resolve(); - const conflicyResolver = (persistedRequests: Request[]) => { + const conflicyResolver = (persistedRequests: Request[]): ConflictActionData => { // should be one instance of ReconnectApp, get the index to replace it later const index = persistedRequests.findIndex((r) => r.command === 'ReconnectApp'); if (index === -1) { From 31bf2867b7a9182a593be9d2f15d1ccab83a828b Mon Sep 17 00:00:00 2001 From: Eduardo Date: Fri, 30 Aug 2024 16:41:47 +0200 Subject: [PATCH 06/22] removed the .only from test --- tests/unit/SequentialQueueTest.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/SequentialQueueTest.ts b/tests/unit/SequentialQueueTest.ts index 1a314903930..9fb78082811 100644 --- a/tests/unit/SequentialQueueTest.ts +++ b/tests/unit/SequentialQueueTest.ts @@ -109,7 +109,7 @@ describe('SequentialQueue', () => { expect(PersistedRequests.getLength()).toBe(2); }); - it.only('should replace request request in queue while a similar one is ongoing', async () => { + it('should replace request request in queue while a similar one is ongoing', async () => { // .push at the end flush the queue SequentialQueue.push(request); From 45bf5a61c160656f114c19bdbdd8c0e08be40af4 Mon Sep 17 00:00:00 2001 From: Eduardo Date: Tue, 10 Sep 2024 09:28:34 +0200 Subject: [PATCH 07/22] Fixes and moving the ongoing request to PersistedRequests --- .../LHNOptionsList/LHNOptionsList.tsx | 2 +- src/hooks/useReportIDs.tsx | 6 +- src/libs/Network/SequentialQueue.ts | 14 +-- src/libs/OptionsListUtils.ts | 2 +- src/libs/ReportActionsUtils.ts | 37 ++++++-- src/libs/SidebarUtils.ts | 2 +- src/libs/actions/PersistedRequests.ts | 88 +++++++++++++++---- src/types/onyx/Request.ts | 12 +-- tests/unit/APITest.ts | 16 ++-- tests/unit/SequentialQueueTest.ts | 41 ++++++++- 10 files changed, 169 insertions(+), 51 deletions(-) diff --git a/src/components/LHNOptionsList/LHNOptionsList.tsx b/src/components/LHNOptionsList/LHNOptionsList.tsx index a734890a1f3..321cf4d6676 100644 --- a/src/components/LHNOptionsList/LHNOptionsList.tsx +++ b/src/components/LHNOptionsList/LHNOptionsList.tsx @@ -137,7 +137,7 @@ function LHNOptionsList({style, contentContainerStyles, data, onSelectRow, optio : '-1'; const itemTransaction = transactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`]; const hasDraftComment = DraftCommentUtils.isValidDraftComment(draftComments?.[`${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`]); - const sortedReportActions = ReportActionsUtils.getSortedReportActionsForDisplay(itemReportActions); + const sortedReportActions = ReportActionsUtils.getSortedReportActionsForDisplay(itemReportActions, false, reportID); const lastReportAction = sortedReportActions[0]; // Get the transaction for the last report action diff --git a/src/hooks/useReportIDs.tsx b/src/hooks/useReportIDs.tsx index b7d84cb2519..0ac82a61171 100644 --- a/src/hooks/useReportIDs.tsx +++ b/src/hooks/useReportIDs.tsx @@ -37,8 +37,9 @@ const ReportIDsContext = createContext({ * This function (and the few below it), narrow down the data from Onyx to just the properties that we want to trigger a re-render of the component. This helps minimize re-rendering * and makes the entire component more performant because it's not re-rendering when a bunch of properties change which aren't ever used in the UI. */ -const reportActionsSelector = (reportActions: OnyxEntry): ReportActionsSelector => - (reportActions && +const reportActionsSelector = (reportActions: OnyxEntry): ReportActionsSelector => { + console.log('reportActions', reportActions); + return (reportActions && Object.values(reportActions) .filter(Boolean) .map((reportAction) => { @@ -58,6 +59,7 @@ const reportActionsSelector = (reportActions: OnyxEntry originalMessage, }; })) as ReportActionsSelector; +}; const policySelector = (policy: OnyxEntry): PolicySelector => (policy && { diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index 26051657eb8..c1b79b015e1 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -24,7 +24,7 @@ let isReadyPromise = new Promise((resolve) => { resolveIsReadyPromise?.(); let isSequentialQueueRunning = false; -let currentRequest: OnyxRequest | null = null; +// let currentRequest: OnyxRequest | null = null; let currentRequestPromise: Promise | null = null; let isQueuePaused = false; @@ -80,9 +80,9 @@ function process(): Promise { return Promise.resolve(); } - const requestToProcess = persistedRequests[0]; - - currentRequest = requestToProcess; + const requestToProcess = PersistedRequests.processNextRequest(); // persistedRequests[0]; + console.log('next process requestToProcess', {...requestToProcess}); + // currentRequest = requestToProcess; // Set the current request to a promise awaiting its processing so that getCurrentRequest can be used to take some action after the current request has processed. currentRequestPromise = Request.processWithMiddleware(requestToProcess, true) .then((response) => { @@ -162,7 +162,7 @@ function flush() { if (NetworkStore.isOffline() || PersistedRequests.getAll().length === 0) { resolveIsReadyPromise?.(); } - currentRequest = null; + // currentRequest = null; currentRequestPromise = null; // The queue can be paused when we sync the data with backend so we should only update the Onyx data when the queue is empty @@ -202,10 +202,11 @@ NetworkStore.onReconnection(flush); function push(newRequest: OnyxRequest) { // If a request is already being processed, ignore it when looking for potentially conflicting requests - const requests = PersistedRequests.getAll().filter((persistedRequest) => persistedRequest !== currentRequest); + const requests = PersistedRequests.getAll(); //.filter((persistedRequest) => persistedRequest !== currentRequest); const {checkAndFixConflictingRequest} = newRequest; if (checkAndFixConflictingRequest) { + console.log('ReconnectApp checkAndFixConflictingRequest', {...requests}); const {conflictAction} = checkAndFixConflictingRequest(requests); if (conflictAction.type === 'push') { @@ -218,6 +219,7 @@ function push(newRequest: OnyxRequest) { PersistedRequests.save(newRequest); } + const final = PersistedRequests.getAll(); // 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()) { return; diff --git a/src/libs/OptionsListUtils.ts b/src/libs/OptionsListUtils.ts index acc9d4bdefc..a1be5bcdfaf 100644 --- a/src/libs/OptionsListUtils.ts +++ b/src/libs/OptionsListUtils.ts @@ -297,7 +297,7 @@ Onyx.connect({ Object.entries(allReportActions).forEach((reportActions) => { const reportID = reportActions[0].split('_')[1]; const reportActionsArray = Object.values(reportActions[1] ?? {}); - let sortedReportActions = ReportActionUtils.getSortedReportActions(reportActionsArray, true); + let sortedReportActions = ReportActionUtils.getSortedReportActions(reportActionsArray, true, reportID); allSortedReportActions[reportID] = sortedReportActions; // If the report is a one-transaction report and has , we need to return the combined reportActions so that the LHN can display modifications diff --git a/src/libs/ReportActionsUtils.ts b/src/libs/ReportActionsUtils.ts index 4d126cf9cbf..414b4a7e1a2 100644 --- a/src/libs/ReportActionsUtils.ts +++ b/src/libs/ReportActionsUtils.ts @@ -49,6 +49,7 @@ type MemberChangeMessageRoomReferenceElement = { type MemberChangeMessageElement = MessageTextElement | MemberChangeMessageUserMentionElement | MemberChangeMessageRoomReferenceElement; let allReportActions: OnyxCollection; +const cachedSortedReportActions = new Map(); Onyx.connect({ key: ONYXKEYS.COLLECTION.REPORT_ACTIONS, waitForCollectionCallback: true, @@ -56,11 +57,25 @@ Onyx.connect({ if (!actions) { return; } - + // console.log('allReportActions', actions); allReportActions = actions; }, }); +Onyx.connect({ + key: ONYXKEYS.COLLECTION.REPORT, + callback: (actions) => { + if (!actions) { + return; + } + // console.log('LOOKING FOR REPORT changed>', actions); + if (cachedSortedReportActions.has(actions.reportID)) { + // console.log('LOOKING FOR REPORT changed> DELETING', actions.reportID); + cachedSortedReportActions.delete(actions.reportID); + } + }, +}); + let isNetworkOffline = false; Onyx.connect({ key: ONYXKEYS.NETWORK, @@ -364,11 +379,16 @@ function isTransactionThread(parentReportAction: OnyxInputOrEntry) * This gives us a stable order even in the case of multiple reportActions created on the same millisecond * */ -function getSortedReportActions(reportActions: ReportAction[] | null, shouldSortInDescendingOrder = false): ReportAction[] { +function getSortedReportActions(reportActions: ReportAction[] | null, shouldSortInDescendingOrder = false, reportID: string | undefined = undefined): ReportAction[] { if (!Array.isArray(reportActions)) { throw new Error(`ReportActionsUtils.getSortedReportActions requires an array, received ${typeof reportActions}`); } + // console.log('getSortedReportActions - INITIAL reportActions', reportActions); + if (reportID && cachedSortedReportActions.has(reportID)) { + // console.log('getSortedReportActions - CACHED sortedActions', cachedSortedReportActions.get(reportID)); + return cachedSortedReportActions.get(reportID) ?? []; + } const invertedMultiplier = shouldSortInDescendingOrder ? -1 : 1; const sortedActions = reportActions?.filter(Boolean).sort((first, second) => { @@ -391,7 +411,10 @@ function getSortedReportActions(reportActions: ReportAction[] | null, shouldSort // will be consistent across all users and devices return (first.reportActionID < second.reportActionID ? -1 : 1) * invertedMultiplier; }); - + // console.log('getSortedReportActions - FINAL sortedActions', sortedActions); + if (reportID) { + cachedSortedReportActions.set(reportID, sortedActions); + } return sortedActions; } @@ -441,7 +464,7 @@ function getCombinedReportActions( return actionType !== CONST.IOU.REPORT_ACTION_TYPE.CREATE && actionType !== CONST.IOU.REPORT_ACTION_TYPE.TRACK; }); - return getSortedReportActions(filteredReportActions, true); + return getSortedReportActions(filteredReportActions, true, report?.reportID); } /** @@ -731,7 +754,7 @@ function getLastVisibleAction(reportID: string, actionsToMerge: Record shouldReportActionBeVisibleAsLastAction(action)); - const sortedReportActions = getSortedReportActions(visibleReportActions, true); + const sortedReportActions = getSortedReportActions(visibleReportActions, true, reportID); if (sortedReportActions.length === 0) { return undefined; } @@ -784,7 +807,7 @@ function filterOutDeprecatedReportActions(reportActions: OnyxEntry | ReportAction[], shouldIncludeInvisibleActions = false): ReportAction[] { +function getSortedReportActionsForDisplay(reportActions: OnyxEntry | ReportAction[], shouldIncludeInvisibleActions = false, reportID: string | undefined): ReportAction[] { let filteredReportActions: ReportAction[] = []; if (!reportActions) { return []; @@ -799,7 +822,7 @@ function getSortedReportActionsForDisplay(reportActions: OnyxEntry replaceBaseURLInPolicyChangeLogAction(reportAction)); - return getSortedReportActions(baseURLAdjustedReportActions, true); + return getSortedReportActions(baseURLAdjustedReportActions, true, reportID); } /** diff --git a/src/libs/SidebarUtils.ts b/src/libs/SidebarUtils.ts index 0e844763509..52adafc0839 100644 --- a/src/libs/SidebarUtils.ts +++ b/src/libs/SidebarUtils.ts @@ -41,7 +41,7 @@ Onyx.connect({ } const reportID = CollectionUtils.extractCollectionItemID(key); - const actionsArray: ReportAction[] = ReportActionsUtils.getSortedReportActions(Object.values(actions)); + const actionsArray: ReportAction[] = ReportActionsUtils.getSortedReportActions(Object.values(actions), false, reportID); // The report is only visible if it is the last action not deleted that // does not match a closed or created state. diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts index 50a273e07a9..43675ecf8fe 100644 --- a/src/libs/actions/PersistedRequests.ts +++ b/src/libs/actions/PersistedRequests.ts @@ -5,58 +5,112 @@ import ONYXKEYS from '@src/ONYXKEYS'; import type {Request} from '@src/types/onyx'; let persistedRequests: Request[] = []; +let ongoingRequest: Request | null = null; Onyx.connect({ key: ONYXKEYS.PERSISTED_REQUESTS, - callback: (val) => (persistedRequests = val ?? []), + callback: (val) => { + console.log('ReconnectApp PERSISTED_REQUESTS val', {...val}, ongoingRequest); + // it has the ongoingRequest in here? + persistedRequests = val ?? []; + + if (ongoingRequest && persistedRequests.length > 0) { + const elem = {...persistedRequests}[0]; + console.log('First persistedRequests', elem, ' are equals: ', isEqual(elem, ongoingRequest)); + // here we try to remove the first element from the persistedRequests if it is the same as ongoingRequest + if (isEqual(elem, ongoingRequest)) { + console.log('First persistedRequests is equal to ongoingRequest'); + persistedRequests = persistedRequests.slice(1); + } + } + }, }); /** * This promise is only used by tests. DO NOT USE THIS PROMISE IN THE APPLICATION CODE */ function clear() { + ongoingRequest = null; return Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, []); } function getLength(): number { - return persistedRequests.length; + // Making it backwards compatible with the old implementation + return persistedRequests.length + (ongoingRequest ? 1 : 0); } function save(requestToPersist: Request) { // If the command is not in the keepLastInstance array, add the new request as usual - persistedRequests = [...persistedRequests, requestToPersist]; - - Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests).then(() => { + const requests = [...persistedRequests, requestToPersist]; + persistedRequests = requests; + Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, requests).then(() => { Log.info(`[SequentialQueue] '${requestToPersist.command}' command queued. Queue length is ${getLength()}`); }); } function remove(requestToRemove: Request) { - /** - * We only remove the first matching request because the order of requests matters. - * If we were to remove all matching requests, we can end up with a final state that is different than what the user intended. - */ - const requests = [...persistedRequests]; - const index = requests.findIndex((persistedRequest) => isEqual(persistedRequest, requestToRemove)); - if (index === -1) { - return; + console.log('remove requestToRemove - init>', {...requestToRemove}); + if (isEqual(ongoingRequest, requestToRemove)) { + console.log('remove ongoingRequest', {...ongoingRequest}); + ongoingRequest = null; + } else { + /** + * We only remove the first matching request because the order of requests matters. + * If we were to remove all matching requests, we can end up with a final state that is different than what the user intended. + */ + const requests = [...persistedRequests]; + const index = requests.findIndex((persistedRequest) => isEqual(persistedRequest, requestToRemove)); + console.log('current queue: ', requests, 'remove index', index); + if (index === -1) { + return; + } + requests.splice(index, 1); + persistedRequests = requests; } - requests.splice(index, 1); - persistedRequests = requests; - Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, requests).then(() => { + Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests).then(() => { Log.info(`[SequentialQueue] '${requestToRemove.command}' removed from the queue. Queue length is ${getLength()}`); }); } function update(oldRequestIndex: number, newRequest: Request) { + console.log(`${newRequest.command} oldRequestIndex`, oldRequestIndex); const requests = [...persistedRequests]; + console.log(`${newRequest.command} before requests`, {...requests}); requests.splice(oldRequestIndex, 1, newRequest); + console.log(`${newRequest.command} after requests`, {...requests}); persistedRequests = requests; + console.log(`${newRequest.command} persistedRequests`, {...persistedRequests}); Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, requests); } +function updateOngoingRequest(newRequest: Request) { + ongoingRequest = newRequest; +} + +function processNextRequest(): Request { + // You must handle the case where there are no requests to process + if (persistedRequests.length === 0) { + throw new Error('No requests to process'); + } + + // At least for now, you must handle the case where there is an ongoing request + if (ongoingRequest) { + throw new Error('There is already an ongoing request'); + } + ongoingRequest = persistedRequests[0]; + persistedRequests = persistedRequests.slice(1); + // We don't need to update Onyx persistedRequests just in case the ongoingRequest fails + // we want to keep trying if the user closes the app + return ongoingRequest; +} + function getAll(): Request[] { + console.log('getAll persistedRequests', {...persistedRequests}); return persistedRequests; } -export {clear, save, getAll, remove, update, getLength}; +function getOngoingRequest(): Request | null { + return ongoingRequest; +} + +export {clear, save, getAll, remove, update, getLength, getOngoingRequest, processNextRequest, updateOngoingRequest}; diff --git a/src/types/onyx/Request.ts b/src/types/onyx/Request.ts index 5d37cc559a4..739e69669c4 100644 --- a/src/types/onyx/Request.ts +++ b/src/types/onyx/Request.ts @@ -56,9 +56,9 @@ type RequestData = { }; /** - * Model of a conflict request that has to be updated, in the request queue. + * Model of a conflict request that has to be replaced in the request queue. */ -type ConflictRequestUpdate = { +type ConflictRequestReplace = { /** * The action to take in case of a conflict. */ @@ -71,9 +71,9 @@ type ConflictRequestUpdate = { }; /** - * Model of a conflict request that has to be saved at the end the request queue. + * Model of a conflict request that has to be enqueued at the end of request queue. */ -type ConflictRequestSave = { +type ConflictRequestPush = { /** * The action to take in case of a conflict. */ @@ -81,7 +81,7 @@ type ConflictRequestSave = { }; /** - * Model of a conflict request that no need to be updated or saved, in the request queue. + * Model of a conflict request that does not need to be updated or saved in the request queue. */ type ConflictRequestNoAction = { /** @@ -97,7 +97,7 @@ type ConflictActionData = { /** * The action to take in case of a conflict. */ - conflictAction: ConflictRequestUpdate | ConflictRequestSave | ConflictRequestNoAction; + conflictAction: ConflictRequestReplace | ConflictRequestPush | ConflictRequestNoAction; }; /** diff --git a/tests/unit/APITest.ts b/tests/unit/APITest.ts index 74be6c742f5..400d460f0bd 100644 --- a/tests/unit/APITest.ts +++ b/tests/unit/APITest.ts @@ -43,6 +43,7 @@ const originalXHR = HttpUtils.xhr; beforeEach(() => { global.fetch = TestHelper.getGlobalFetchMock(); HttpUtils.xhr = originalXHR; + MainQueue.clear(); HttpUtils.cancelPendingRequests(); PersistedRequests.clear(); @@ -166,23 +167,23 @@ describe('APITests', () => { .then(waitForBatchedUpdates) .then(() => { // Then requests should remain persisted until the xhr call is resolved - expect(PersistedRequests.getAll().length).toEqual(2); + expect(PersistedRequests.getAll().length).toEqual(1); xhrCalls[0].resolve({jsonCode: CONST.JSON_CODE.SUCCESS}); return waitForBatchedUpdates(); }) .then(waitForBatchedUpdates) .then(() => { - expect(PersistedRequests.getAll().length).toEqual(1); - expect(PersistedRequests.getAll()).toEqual([expect.objectContaining({command: 'mock command', data: expect.objectContaining({param2: 'value2'})})]); + expect(PersistedRequests.getAll().length).toEqual(0); + expect(PersistedRequests.getOngoingRequest()).toEqual(expect.objectContaining({command: 'mock command', data: expect.objectContaining({param2: 'value2'})})); // When a request fails it should be retried xhrCalls[1].reject(new Error(CONST.ERROR.FAILED_TO_FETCH)); return waitForBatchedUpdates(); }) .then(() => { - expect(PersistedRequests.getAll().length).toEqual(1); - expect(PersistedRequests.getAll()).toEqual([expect.objectContaining({command: 'mock command', data: expect.objectContaining({param2: 'value2'})})]); + expect(PersistedRequests.getAll().length).toEqual(0); + expect(PersistedRequests.getOngoingRequest()).toEqual(expect.objectContaining({command: 'mock command', data: expect.objectContaining({param2: 'value2'})})); // We need to advance past the request throttle back off timer because the request won't be retried until then return new Promise((resolve) => { @@ -191,7 +192,7 @@ describe('APITests', () => { }) .then(() => { // Finally, after it succeeds the queue should be empty - xhrCalls[2].resolve({jsonCode: CONST.JSON_CODE.SUCCESS}); + xhrCalls[1].resolve({jsonCode: CONST.JSON_CODE.SUCCESS}); return waitForBatchedUpdates(); }) .then(() => { @@ -366,6 +367,7 @@ describe('APITests', () => { test('several actions made while offline will get added in the order they are created when we need to reauthenticate', () => { // Given offline state where all requests will eventualy succeed without issue and assumed to be valid credentials const xhr = jest.spyOn(HttpUtils, 'xhr').mockResolvedValueOnce({jsonCode: CONST.JSON_CODE.NOT_AUTHENTICATED}).mockResolvedValue({jsonCode: CONST.JSON_CODE.SUCCESS}); + xhr.mockClear(); return Onyx.multiSet({ [ONYXKEYS.NETWORK]: {isOffline: true}, @@ -551,7 +553,7 @@ describe('APITests', () => { // THEN the queue should be stopped and there should be no more requests to run expect(SequentialQueue.isRunning()).toBe(false); expect(PersistedRequests.getAll().length).toBe(0); - + console.log('CALLS: ', xhr.mock.calls); // And our Write request should run before our non persistable one in a blocking way const firstRequest = xhr.mock.calls[0]; const [firstRequestCommandName] = firstRequest; diff --git a/tests/unit/SequentialQueueTest.ts b/tests/unit/SequentialQueueTest.ts index 9fb78082811..edae1df5062 100644 --- a/tests/unit/SequentialQueueTest.ts +++ b/tests/unit/SequentialQueueTest.ts @@ -54,6 +54,10 @@ describe('SequentialQueue', () => { }; SequentialQueue.push(requestWithConflictResolution); expect(PersistedRequests.getLength()).toBe(1); + // We know there is only one request in the queue, so we can get the first one and verify + // that the persisted request is the second one. + const persistedRequest = PersistedRequests.getAll()[0]; + expect(persistedRequest?.data?.accountID).toBe(56789); }); it('should push two requests with conflict resolution and push', () => { @@ -116,7 +120,7 @@ describe('SequentialQueue', () => { // wait for Onyx.connect execute the callback and start processing the queue await Promise.resolve(); - const conflicyResolver = (persistedRequests: Request[]): ConflictActionData => { + const conflictResolver = (persistedRequests: Request[]): ConflictActionData => { // should be one instance of ReconnectApp, get the index to replace it later const index = persistedRequests.findIndex((r) => r.command === 'ReconnectApp'); if (index === -1) { @@ -131,13 +135,13 @@ describe('SequentialQueue', () => { const requestWithConflictResolution: Request = { command: 'ReconnectApp', data: {accountID: 56789}, - checkAndFixConflictingRequest: conflicyResolver, + checkAndFixConflictingRequest: conflictResolver, }; const requestWithConflictResolution2: Request = { command: 'ReconnectApp', data: {accountID: 56789}, - checkAndFixConflictingRequest: conflicyResolver, + checkAndFixConflictingRequest: conflictResolver, }; SequentialQueue.push(requestWithConflictResolution); @@ -145,4 +149,35 @@ describe('SequentialQueue', () => { expect(PersistedRequests.getLength()).toBe(2); }); + + it('should replace request request in queue while a similar one is ongoing and keep the same index', async () => { + SequentialQueue.push({command: 'OpenReport'}); + SequentialQueue.push(request); + + const requestWithConflictResolution: Request = { + command: 'ReconnectApp', + data: {accountID: 56789}, + checkAndFixConflictingRequest: (persistedRequests) => { + // should be one instance of ReconnectApp, get the index to replace it later + const index = persistedRequests.findIndex((r) => r.command === 'ReconnectApp'); + if (index === -1) { + return {conflictAction: {type: 'push'}}; + } + + return { + conflictAction: {type: 'replace', index}, + }; + }, + }; + + SequentialQueue.push(requestWithConflictResolution); + SequentialQueue.push({command: 'AddComment'}); + SequentialQueue.push({command: 'OpenReport'}); + + expect(PersistedRequests.getLength()).toBe(4); + const persistedRequests = PersistedRequests.getAll(); + // We know ReconnectApp is at index 1 in the queue, so we can get it to verify + // that was replaced by the new request. + expect(persistedRequests[1]?.data?.accountID).toBe(56789); + }); }); From 3620da6b3a014890cfd1e92f2e553ab8f4ed512a Mon Sep 17 00:00:00 2001 From: Eduardo Date: Tue, 10 Sep 2024 21:14:09 +0200 Subject: [PATCH 08/22] Fixed APITest --- src/libs/Network/SequentialQueue.ts | 10 +++++++--- src/libs/actions/PersistedRequests.ts | 14 +++++++++++++- tests/unit/APITest.ts | 12 ++++++------ 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index c1b79b015e1..a7ad79c4c28 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -63,6 +63,7 @@ function flushOnyxUpdatesQueue() { * requests to our backend is evenly distributed and it gradually decreases with time, which helps the servers catch up. */ function process(): Promise { + console.log('PROCESS'); // When the queue is paused, return early. This prevents any new requests from happening. The queue will be flushed again when the queue is unpaused. if (isQueuePaused) { Log.info('[SequentialQueue] Unable to process. Queue is paused.'); @@ -73,7 +74,7 @@ function process(): Promise { Log.info('[SequentialQueue] Unable to process. We are offline.'); return Promise.resolve(); } - + console.log('process -> checkking getAll'); const persistedRequests = PersistedRequests.getAll(); if (persistedRequests.length === 0) { Log.info('[SequentialQueue] Unable to process. No requests to process.'); @@ -99,14 +100,18 @@ function process(): Promise { .catch((error: RequestError) => { // On sign out we cancel any in flight requests from the user. Since that user is no longer signed in their requests should not be retried. // Duplicate records don't need to be retried as they just mean the record already exists on the server + console.log('CATCHING 1st error', error); if (error.name === CONST.ERROR.REQUEST_CANCELLED || error.message === CONST.ERROR.DUPLICATE_RECORD) { + console.log('CATCHING 1st error inside 1st IF -> remove clear RETURN'); PersistedRequests.remove(requestToProcess); RequestThrottle.clear(); return process(); } + PersistedRequests.rollbackOngoingRequest(); return RequestThrottle.sleep() .then(process) .catch(() => { + console.log('CATCHING 2nd error', error); Onyx.update(requestToProcess.failureData ?? []); PersistedRequests.remove(requestToProcess); RequestThrottle.clear(); @@ -140,7 +145,7 @@ function flush() { Log.info('[SequentialQueue] Unable to flush. Client is not the leader.'); return; } - + console.log('flushing -> isSequentialQueueRunning true'); isSequentialQueueRunning = true; // Reset the isReadyPromise so that the queue will be flushed as soon as the request is finished @@ -219,7 +224,6 @@ function push(newRequest: OnyxRequest) { PersistedRequests.save(newRequest); } - const final = PersistedRequests.getAll(); // 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()) { return; diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts index 43675ecf8fe..6e2fac33955 100644 --- a/src/libs/actions/PersistedRequests.ts +++ b/src/libs/actions/PersistedRequests.ts @@ -104,6 +104,18 @@ function processNextRequest(): Request { return ongoingRequest; } +function rollbackOngoingRequest() { + if (!ongoingRequest) { + return; + } + + // Prepend ongoingRequest to persistedRequests + persistedRequests = [ongoingRequest, ...persistedRequests]; + + // Clear the ongoingRequest + ongoingRequest = null; +} + function getAll(): Request[] { console.log('getAll persistedRequests', {...persistedRequests}); return persistedRequests; @@ -113,4 +125,4 @@ function getOngoingRequest(): Request | null { return ongoingRequest; } -export {clear, save, getAll, remove, update, getLength, getOngoingRequest, processNextRequest, updateOngoingRequest}; +export {clear, save, getAll, remove, update, getLength, getOngoingRequest, processNextRequest, updateOngoingRequest, rollbackOngoingRequest}; diff --git a/tests/unit/APITest.ts b/tests/unit/APITest.ts index 400d460f0bd..63d38fd7079 100644 --- a/tests/unit/APITest.ts +++ b/tests/unit/APITest.ts @@ -182,21 +182,23 @@ describe('APITests', () => { return waitForBatchedUpdates(); }) .then(() => { - expect(PersistedRequests.getAll().length).toEqual(0); - expect(PersistedRequests.getOngoingRequest()).toEqual(expect.objectContaining({command: 'mock command', data: expect.objectContaining({param2: 'value2'})})); - + // The ongoingRequest it is moving back to the persistedRequests queue + expect(PersistedRequests.getAll().length).toEqual(1); + expect(PersistedRequests.getAll()).toEqual([expect.objectContaining({command: 'mock command', data: expect.objectContaining({param2: 'value2'})})]); // We need to advance past the request throttle back off timer because the request won't be retried until then return new Promise((resolve) => { setTimeout(resolve, CONST.NETWORK.MAX_RANDOM_RETRY_WAIT_TIME_MS); }).then(waitForBatchedUpdates); }) .then(() => { + // A new promise is created after the back off timer // Finally, after it succeeds the queue should be empty - xhrCalls[1].resolve({jsonCode: CONST.JSON_CODE.SUCCESS}); + xhrCalls[2].resolve({jsonCode: CONST.JSON_CODE.SUCCESS}); return waitForBatchedUpdates(); }) .then(() => { expect(PersistedRequests.getAll().length).toEqual(0); + expect(PersistedRequests.getOngoingRequest()).toBeNull(); }) ); }); @@ -367,7 +369,6 @@ describe('APITests', () => { test('several actions made while offline will get added in the order they are created when we need to reauthenticate', () => { // Given offline state where all requests will eventualy succeed without issue and assumed to be valid credentials const xhr = jest.spyOn(HttpUtils, 'xhr').mockResolvedValueOnce({jsonCode: CONST.JSON_CODE.NOT_AUTHENTICATED}).mockResolvedValue({jsonCode: CONST.JSON_CODE.SUCCESS}); - xhr.mockClear(); return Onyx.multiSet({ [ONYXKEYS.NETWORK]: {isOffline: true}, @@ -553,7 +554,6 @@ describe('APITests', () => { // THEN the queue should be stopped and there should be no more requests to run expect(SequentialQueue.isRunning()).toBe(false); expect(PersistedRequests.getAll().length).toBe(0); - console.log('CALLS: ', xhr.mock.calls); // And our Write request should run before our non persistable one in a blocking way const firstRequest = xhr.mock.calls[0]; const [firstRequestCommandName] = firstRequest; From d07d9126f2df2c10e1cf883f671cfea84e32f641 Mon Sep 17 00:00:00 2001 From: Eduardo Date: Wed, 11 Sep 2024 16:07:48 +0200 Subject: [PATCH 09/22] Fixed tests and code --- .../LHNOptionsList/LHNOptionsList.tsx | 2 +- .../Middleware/HandleUnusedOptimisticID.ts | 25 +++++--- src/libs/Network/SequentialQueue.ts | 17 ++---- src/libs/OptionsListUtils.ts | 2 +- src/libs/ReportActionsUtils.ts | 35 ++--------- src/libs/SidebarUtils.ts | 2 +- src/libs/actions/PersistedRequests.ts | 45 ++++++-------- tests/unit/PersistedRequests.ts | 61 +++++++++++++++++++ 8 files changed, 110 insertions(+), 79 deletions(-) diff --git a/src/components/LHNOptionsList/LHNOptionsList.tsx b/src/components/LHNOptionsList/LHNOptionsList.tsx index 04cba45a0be..624e8f18e69 100644 --- a/src/components/LHNOptionsList/LHNOptionsList.tsx +++ b/src/components/LHNOptionsList/LHNOptionsList.tsx @@ -139,7 +139,7 @@ function LHNOptionsList({style, contentContainerStyles, data, onSelectRow, optio : '-1'; const itemTransaction = transactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`]; const hasDraftComment = DraftCommentUtils.isValidDraftComment(draftComments?.[`${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`]); - const sortedReportActions = ReportActionsUtils.getSortedReportActionsForDisplay(itemReportActions, false, reportID); + const sortedReportActions = ReportActionsUtils.getSortedReportActionsForDisplay(itemReportActions); const lastReportAction = sortedReportActions[0]; // Get the transaction for the last report action diff --git a/src/libs/Middleware/HandleUnusedOptimisticID.ts b/src/libs/Middleware/HandleUnusedOptimisticID.ts index db726855d07..30707769a05 100644 --- a/src/libs/Middleware/HandleUnusedOptimisticID.ts +++ b/src/libs/Middleware/HandleUnusedOptimisticID.ts @@ -34,14 +34,23 @@ const handleUnusedOptimisticID: Middleware = (requestResponse, request, isFromSe return; } const oldReportID = request.data?.reportID; - const offset = isFromSequentialQueue ? 1 : 0; - PersistedRequests.getAll() - .slice(offset) - .forEach((persistedRequest, index) => { - const persistedRequestClone = _.clone(persistedRequest); - persistedRequestClone.data = deepReplaceKeysAndValues(persistedRequest.data, oldReportID as string, preexistingReportID); - PersistedRequests.update(index + offset, persistedRequestClone); - }); + + if (isFromSequentialQueue) { + const ongoingRequest = PersistedRequests.getOngoingRequest(); + console.log('ongoingRequest', ongoingRequest); + console.log('oldReportID', oldReportID, 'preexistingReportID', preexistingReportID); + if (ongoingRequest && ongoingRequest.data?.reportID === oldReportID) { + const ongoingRequestClone = _.clone(ongoingRequest); + ongoingRequestClone.data = deepReplaceKeysAndValues(ongoingRequest.data, oldReportID as string, preexistingReportID); + PersistedRequests.updateOngoingRequest(ongoingRequestClone); + } + } + + PersistedRequests.getAll().forEach((persistedRequest, index) => { + const persistedRequestClone = _.clone(persistedRequest); + persistedRequestClone.data = deepReplaceKeysAndValues(persistedRequest.data, oldReportID as string, preexistingReportID); + PersistedRequests.update(index, persistedRequestClone); + }); }); return response; }); diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index a7ad79c4c28..4d0f8c6c7b5 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -63,7 +63,6 @@ function flushOnyxUpdatesQueue() { * requests to our backend is evenly distributed and it gradually decreases with time, which helps the servers catch up. */ function process(): Promise { - console.log('PROCESS'); // When the queue is paused, return early. This prevents any new requests from happening. The queue will be flushed again when the queue is unpaused. if (isQueuePaused) { Log.info('[SequentialQueue] Unable to process. Queue is paused.'); @@ -74,16 +73,15 @@ function process(): Promise { Log.info('[SequentialQueue] Unable to process. We are offline.'); return Promise.resolve(); } - console.log('process -> checkking getAll'); + const persistedRequests = PersistedRequests.getAll(); if (persistedRequests.length === 0) { Log.info('[SequentialQueue] Unable to process. No requests to process.'); return Promise.resolve(); } - const requestToProcess = PersistedRequests.processNextRequest(); // persistedRequests[0]; - console.log('next process requestToProcess', {...requestToProcess}); - // currentRequest = requestToProcess; + const requestToProcess = PersistedRequests.processNextRequest(); + // Set the current request to a promise awaiting its processing so that getCurrentRequest can be used to take some action after the current request has processed. currentRequestPromise = Request.processWithMiddleware(requestToProcess, true) .then((response) => { @@ -93,6 +91,7 @@ function process(): Promise { Log.info("[SequentialQueue] Handled 'shouldPauseQueue' in response. Pausing the queue."); pause(); } + PersistedRequests.remove(requestToProcess); RequestThrottle.clear(); return process(); @@ -100,9 +99,7 @@ function process(): Promise { .catch((error: RequestError) => { // On sign out we cancel any in flight requests from the user. Since that user is no longer signed in their requests should not be retried. // Duplicate records don't need to be retried as they just mean the record already exists on the server - console.log('CATCHING 1st error', error); if (error.name === CONST.ERROR.REQUEST_CANCELLED || error.message === CONST.ERROR.DUPLICATE_RECORD) { - console.log('CATCHING 1st error inside 1st IF -> remove clear RETURN'); PersistedRequests.remove(requestToProcess); RequestThrottle.clear(); return process(); @@ -111,7 +108,6 @@ function process(): Promise { return RequestThrottle.sleep() .then(process) .catch(() => { - console.log('CATCHING 2nd error', error); Onyx.update(requestToProcess.failureData ?? []); PersistedRequests.remove(requestToProcess); RequestThrottle.clear(); @@ -145,7 +141,7 @@ function flush() { Log.info('[SequentialQueue] Unable to flush. Client is not the leader.'); return; } - console.log('flushing -> isSequentialQueueRunning true'); + isSequentialQueueRunning = true; // Reset the isReadyPromise so that the queue will be flushed as soon as the request is finished @@ -207,11 +203,10 @@ NetworkStore.onReconnection(flush); function push(newRequest: OnyxRequest) { // If a request is already being processed, ignore it when looking for potentially conflicting requests - const requests = PersistedRequests.getAll(); //.filter((persistedRequest) => persistedRequest !== currentRequest); + const requests = PersistedRequests.getAll(); const {checkAndFixConflictingRequest} = newRequest; if (checkAndFixConflictingRequest) { - console.log('ReconnectApp checkAndFixConflictingRequest', {...requests}); const {conflictAction} = checkAndFixConflictingRequest(requests); if (conflictAction.type === 'push') { diff --git a/src/libs/OptionsListUtils.ts b/src/libs/OptionsListUtils.ts index 048f40257f0..f191c1d0653 100644 --- a/src/libs/OptionsListUtils.ts +++ b/src/libs/OptionsListUtils.ts @@ -299,7 +299,7 @@ Onyx.connect({ Object.entries(allReportActions).forEach((reportActions) => { const reportID = reportActions[0].split('_')[1]; const reportActionsArray = Object.values(reportActions[1] ?? {}); - let sortedReportActions = ReportActionUtils.getSortedReportActions(reportActionsArray, true, reportID); + let sortedReportActions = ReportActionUtils.getSortedReportActions(reportActionsArray, true); allSortedReportActions[reportID] = sortedReportActions; // If the report is a one-transaction report and has , we need to return the combined reportActions so that the LHN can display modifications diff --git a/src/libs/ReportActionsUtils.ts b/src/libs/ReportActionsUtils.ts index e653bcccbeb..673fafe90fd 100644 --- a/src/libs/ReportActionsUtils.ts +++ b/src/libs/ReportActionsUtils.ts @@ -49,7 +49,6 @@ type MemberChangeMessageRoomReferenceElement = { type MemberChangeMessageElement = MessageTextElement | MemberChangeMessageUserMentionElement | MemberChangeMessageRoomReferenceElement; let allReportActions: OnyxCollection; -const cachedSortedReportActions = new Map(); Onyx.connect({ key: ONYXKEYS.COLLECTION.REPORT_ACTIONS, waitForCollectionCallback: true, @@ -62,20 +61,6 @@ Onyx.connect({ }, }); -Onyx.connect({ - key: ONYXKEYS.COLLECTION.REPORT, - callback: (actions) => { - if (!actions) { - return; - } - // console.log('LOOKING FOR REPORT changed>', actions); - if (cachedSortedReportActions.has(actions.reportID)) { - // console.log('LOOKING FOR REPORT changed> DELETING', actions.reportID); - cachedSortedReportActions.delete(actions.reportID); - } - }, -}); - let isNetworkOffline = false; Onyx.connect({ key: ONYXKEYS.NETWORK, @@ -379,16 +364,11 @@ function isTransactionThread(parentReportAction: OnyxInputOrEntry) * This gives us a stable order even in the case of multiple reportActions created on the same millisecond * */ -function getSortedReportActions(reportActions: ReportAction[] | null, shouldSortInDescendingOrder = false, reportID: string | undefined = undefined): ReportAction[] { +function getSortedReportActions(reportActions: ReportAction[] | null, shouldSortInDescendingOrder = false): ReportAction[] { if (!Array.isArray(reportActions)) { throw new Error(`ReportActionsUtils.getSortedReportActions requires an array, received ${typeof reportActions}`); } - // console.log('getSortedReportActions - INITIAL reportActions', reportActions); - if (reportID && cachedSortedReportActions.has(reportID)) { - // console.log('getSortedReportActions - CACHED sortedActions', cachedSortedReportActions.get(reportID)); - return cachedSortedReportActions.get(reportID) ?? []; - } const invertedMultiplier = shouldSortInDescendingOrder ? -1 : 1; const sortedActions = reportActions?.filter(Boolean).sort((first, second) => { @@ -411,10 +391,7 @@ function getSortedReportActions(reportActions: ReportAction[] | null, shouldSort // will be consistent across all users and devices return (first.reportActionID < second.reportActionID ? -1 : 1) * invertedMultiplier; }); - // console.log('getSortedReportActions - FINAL sortedActions', sortedActions); - if (reportID) { - cachedSortedReportActions.set(reportID, sortedActions); - } + return sortedActions; } @@ -465,7 +442,7 @@ function getCombinedReportActions( return actionType !== CONST.IOU.REPORT_ACTION_TYPE.CREATE && actionType !== CONST.IOU.REPORT_ACTION_TYPE.TRACK; }); - return getSortedReportActions(filteredReportActions, true, report?.reportID); + return getSortedReportActions(filteredReportActions, true); } /** @@ -755,7 +732,7 @@ function getLastVisibleAction(reportID: string, actionsToMerge: Record shouldReportActionBeVisibleAsLastAction(action)); - const sortedReportActions = getSortedReportActions(visibleReportActions, true, reportID); + const sortedReportActions = getSortedReportActions(visibleReportActions, true); if (sortedReportActions.length === 0) { return undefined; } @@ -808,7 +785,7 @@ function filterOutDeprecatedReportActions(reportActions: OnyxEntry | ReportAction[], shouldIncludeInvisibleActions = false, reportID: string | undefined): ReportAction[] { +function getSortedReportActionsForDisplay(reportActions: OnyxEntry | ReportAction[], shouldIncludeInvisibleActions = false): ReportAction[] { let filteredReportActions: ReportAction[] = []; if (!reportActions) { return []; @@ -823,7 +800,7 @@ function getSortedReportActionsForDisplay(reportActions: OnyxEntry replaceBaseURLInPolicyChangeLogAction(reportAction)); - return getSortedReportActions(baseURLAdjustedReportActions, true, reportID); + return getSortedReportActions(baseURLAdjustedReportActions, true); } /** diff --git a/src/libs/SidebarUtils.ts b/src/libs/SidebarUtils.ts index 404f665e4aa..4bd7e2714e2 100644 --- a/src/libs/SidebarUtils.ts +++ b/src/libs/SidebarUtils.ts @@ -41,7 +41,7 @@ Onyx.connect({ } const reportID = CollectionUtils.extractCollectionItemID(key); - const actionsArray: ReportAction[] = ReportActionsUtils.getSortedReportActions(Object.values(actions), false, reportID); + const actionsArray: ReportAction[] = ReportActionsUtils.getSortedReportActions(Object.values(actions)); // The report is only visible if it is the last action not deleted that // does not match a closed or created state. diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts index 6e2fac33955..49ed6bc6473 100644 --- a/src/libs/actions/PersistedRequests.ts +++ b/src/libs/actions/PersistedRequests.ts @@ -10,16 +10,14 @@ let ongoingRequest: Request | null = null; Onyx.connect({ key: ONYXKEYS.PERSISTED_REQUESTS, callback: (val) => { - console.log('ReconnectApp PERSISTED_REQUESTS val', {...val}, ongoingRequest); // it has the ongoingRequest in here? persistedRequests = val ?? []; if (ongoingRequest && persistedRequests.length > 0) { - const elem = {...persistedRequests}[0]; - console.log('First persistedRequests', elem, ' are equals: ', isEqual(elem, ongoingRequest)); - // here we try to remove the first element from the persistedRequests if it is the same as ongoingRequest - if (isEqual(elem, ongoingRequest)) { - console.log('First persistedRequests is equal to ongoingRequest'); + const nextRequestToProcess = persistedRequests[0]; + // We try to remove the next request from the persistedRequests if it is the same as ongoingRequest + // so we don't process it twice. + if (isEqual(nextRequestToProcess, ongoingRequest)) { persistedRequests = persistedRequests.slice(1); } } @@ -49,37 +47,29 @@ function save(requestToPersist: Request) { } function remove(requestToRemove: Request) { - console.log('remove requestToRemove - init>', {...requestToRemove}); - if (isEqual(ongoingRequest, requestToRemove)) { - console.log('remove ongoingRequest', {...ongoingRequest}); - ongoingRequest = null; - } else { - /** - * We only remove the first matching request because the order of requests matters. - * If we were to remove all matching requests, we can end up with a final state that is different than what the user intended. - */ - const requests = [...persistedRequests]; - const index = requests.findIndex((persistedRequest) => isEqual(persistedRequest, requestToRemove)); - console.log('current queue: ', requests, 'remove index', index); - if (index === -1) { - return; - } - requests.splice(index, 1); - persistedRequests = requests; + ongoingRequest = null; + /** + * We only remove the first matching request because the order of requests matters. + * If we were to remove all matching requests, we can end up with a final state that is different than what the user intended. + */ + const requests = [...persistedRequests]; + const index = requests.findIndex((persistedRequest) => isEqual(persistedRequest, requestToRemove)); + + if (index === -1) { + return; } + requests.splice(index, 1); + persistedRequests = requests; + Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests).then(() => { Log.info(`[SequentialQueue] '${requestToRemove.command}' removed from the queue. Queue length is ${getLength()}`); }); } function update(oldRequestIndex: number, newRequest: Request) { - console.log(`${newRequest.command} oldRequestIndex`, oldRequestIndex); const requests = [...persistedRequests]; - console.log(`${newRequest.command} before requests`, {...requests}); requests.splice(oldRequestIndex, 1, newRequest); - console.log(`${newRequest.command} after requests`, {...requests}); persistedRequests = requests; - console.log(`${newRequest.command} persistedRequests`, {...persistedRequests}); Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, requests); } @@ -117,7 +107,6 @@ function rollbackOngoingRequest() { } function getAll(): Request[] { - console.log('getAll persistedRequests', {...persistedRequests}); return persistedRequests; } diff --git a/tests/unit/PersistedRequests.ts b/tests/unit/PersistedRequests.ts index 670625f65f9..3a814ca28c2 100644 --- a/tests/unit/PersistedRequests.ts +++ b/tests/unit/PersistedRequests.ts @@ -1,5 +1,9 @@ +import Onyx from 'react-native-onyx'; import * as PersistedRequests from '../../src/libs/actions/PersistedRequests'; +import ONYXKEYS from '../../src/ONYXKEYS'; import type Request from '../../src/types/onyx/Request'; +import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; +import wrapOnyxWithWaitForBatchedUpdates from '../utils/wrapOnyxWithWaitForBatchedUpdates'; const request: Request = { command: 'OpenReport', @@ -7,13 +11,22 @@ const request: Request = { failureData: [{key: 'reportMetadata_2', onyxMethod: 'merge', value: {}}], }; +beforeAll(() => + Onyx.init({ + keys: ONYXKEYS, + safeEvictionKeys: [ONYXKEYS.COLLECTION.REPORT_ACTIONS], + }), +); + beforeEach(() => { + wrapOnyxWithWaitForBatchedUpdates(Onyx); PersistedRequests.clear(); PersistedRequests.save(request); }); afterEach(() => { PersistedRequests.clear(); + Onyx.clear(); }); describe('PersistedRequests', () => { @@ -26,4 +39,52 @@ describe('PersistedRequests', () => { PersistedRequests.remove(request); expect(PersistedRequests.getAll().length).toBe(0); }); + + it('when process the next request, queue should be empty', () => { + const nextRequest = PersistedRequests.processNextRequest(); + expect(PersistedRequests.getAll().length).toBe(0); + expect(nextRequest).toEqual(request); + }); + + it('when onyx persist the request, it should remove from the list the ongoing request', () => { + const request2: Request = { + command: 'AddComment', + successData: [{key: 'reportMetadata_3', onyxMethod: 'merge', value: {}}], + failureData: [{key: 'reportMetadata_4', onyxMethod: 'merge', value: {}}], + }; + PersistedRequests.save(request2); + PersistedRequests.processNextRequest(); + return waitForBatchedUpdates().then(() => { + expect(PersistedRequests.getAll().length).toBe(1); + expect(PersistedRequests.getAll()[0]).toEqual(request2); + }); + }); + + it('update the request at the given index with new data', () => { + const newRequest: Request = { + command: 'OpenReport', + successData: [{key: 'reportMetadata_1', onyxMethod: 'set', value: {}}], + failureData: [{key: 'reportMetadata_2', onyxMethod: 'set', value: {}}], + }; + PersistedRequests.update(0, newRequest); + expect(PersistedRequests.getAll()[0]).toEqual(newRequest); + }); + + it('update the ongoing request with new data', () => { + const newRequest: Request = { + command: 'OpenReport', + successData: [{key: 'reportMetadata_1', onyxMethod: 'set', value: {}}], + failureData: [{key: 'reportMetadata_2', onyxMethod: 'set', value: {}}], + }; + PersistedRequests.updateOngoingRequest(newRequest); + expect(PersistedRequests.getOngoingRequest()).toEqual(newRequest); + }); + + it('when removing a request should update the persistedRequests queue and clear the ongoing request', () => { + PersistedRequests.processNextRequest(); + expect(PersistedRequests.getOngoingRequest()).toEqual(request); + PersistedRequests.remove(request); + expect(PersistedRequests.getOngoingRequest()).toBeNull(); + expect(PersistedRequests.getAll().length).toBe(0); + }); }); From 7170e14a130af8b1d0ef15acb818c7e340331cd9 Mon Sep 17 00:00:00 2001 From: Eduardo Date: Wed, 11 Sep 2024 19:26:46 +0200 Subject: [PATCH 10/22] Fixed lint issues --- src/hooks/useReportIDs.tsx | 6 ++---- src/libs/Middleware/HandleUnusedOptimisticID.ts | 2 -- tests/unit/SequentialQueueTest.ts | 2 +- 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/hooks/useReportIDs.tsx b/src/hooks/useReportIDs.tsx index 0ac82a61171..b7d84cb2519 100644 --- a/src/hooks/useReportIDs.tsx +++ b/src/hooks/useReportIDs.tsx @@ -37,9 +37,8 @@ const ReportIDsContext = createContext({ * This function (and the few below it), narrow down the data from Onyx to just the properties that we want to trigger a re-render of the component. This helps minimize re-rendering * and makes the entire component more performant because it's not re-rendering when a bunch of properties change which aren't ever used in the UI. */ -const reportActionsSelector = (reportActions: OnyxEntry): ReportActionsSelector => { - console.log('reportActions', reportActions); - return (reportActions && +const reportActionsSelector = (reportActions: OnyxEntry): ReportActionsSelector => + (reportActions && Object.values(reportActions) .filter(Boolean) .map((reportAction) => { @@ -59,7 +58,6 @@ const reportActionsSelector = (reportActions: OnyxEntry originalMessage, }; })) as ReportActionsSelector; -}; const policySelector = (policy: OnyxEntry): PolicySelector => (policy && { diff --git a/src/libs/Middleware/HandleUnusedOptimisticID.ts b/src/libs/Middleware/HandleUnusedOptimisticID.ts index 30707769a05..eaf827af885 100644 --- a/src/libs/Middleware/HandleUnusedOptimisticID.ts +++ b/src/libs/Middleware/HandleUnusedOptimisticID.ts @@ -37,8 +37,6 @@ const handleUnusedOptimisticID: Middleware = (requestResponse, request, isFromSe if (isFromSequentialQueue) { const ongoingRequest = PersistedRequests.getOngoingRequest(); - console.log('ongoingRequest', ongoingRequest); - console.log('oldReportID', oldReportID, 'preexistingReportID', preexistingReportID); if (ongoingRequest && ongoingRequest.data?.reportID === oldReportID) { const ongoingRequestClone = _.clone(ongoingRequest); ongoingRequestClone.data = deepReplaceKeysAndValues(ongoingRequest.data, oldReportID as string, preexistingReportID); diff --git a/tests/unit/SequentialQueueTest.ts b/tests/unit/SequentialQueueTest.ts index edae1df5062..2338e8f7c65 100644 --- a/tests/unit/SequentialQueueTest.ts +++ b/tests/unit/SequentialQueueTest.ts @@ -150,7 +150,7 @@ describe('SequentialQueue', () => { expect(PersistedRequests.getLength()).toBe(2); }); - it('should replace request request in queue while a similar one is ongoing and keep the same index', async () => { + it('should replace request request in queue while a similar one is ongoing and keep the same index', () => { SequentialQueue.push({command: 'OpenReport'}); SequentialQueue.push(request); From b9d19f0edbae89173d3b6922ea79c4d212e31547 Mon Sep 17 00:00:00 2001 From: Eduardo Date: Mon, 16 Sep 2024 15:46:47 +0200 Subject: [PATCH 11/22] Tests to App.reconnectApp + some minors --- src/libs/Network/SequentialQueue.ts | 2 ++ tests/actions/SessionTest.ts | 38 +++++++++++++++++++++++++++++ tests/unit/PersistedRequests.ts | 1 + 3 files changed, 41 insertions(+) diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index 08f55bab5ae..1502b822463 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -214,6 +214,8 @@ function push(newRequest: OnyxRequest) { PersistedRequests.save(newRequest); } else if (conflictAction.type === 'replace') { PersistedRequests.update(conflictAction.index, newRequest); + } else { + Log.info(`[SequentialQueue] No action performed to command ${newRequest.command} and it will be ignored.`); } } else { // Add request to Persisted Requests so that it can be retried if it fails diff --git a/tests/actions/SessionTest.ts b/tests/actions/SessionTest.ts index 62d6a54b20b..f36efa8e4cd 100644 --- a/tests/actions/SessionTest.ts +++ b/tests/actions/SessionTest.ts @@ -7,6 +7,7 @@ import HttpUtils from '@libs/HttpUtils'; import PushNotification from '@libs/Notification/PushNotification'; // This lib needs to be imported, but it has nothing to export since all it contains is an Onyx connection import '@libs/Notification/PushNotification/subscribePushNotification'; +import * as PersistedRequests from '@userActions/PersistedRequests'; import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; import type {Credentials, Session} from '@src/types/onyx'; @@ -105,4 +106,41 @@ describe('Session', () => { TestHelper.signInWithTestUser() .then(TestHelper.signOutTestUser) .then(() => expect(PushNotification.deregister).toBeCalled())); + + test('ReconnectApp should push request to the queue', () => { + return TestHelper.signInWithTestUser() + .then(() => Onyx.set(ONYXKEYS.NETWORK, {isOffline: true})) + .then(() => { + App.confirmReadyToOpenApp(); + App.reconnectApp(); + }) + .then(waitForBatchedUpdates) + .then(() => { + expect(PersistedRequests.getAll().length).toBe(1); + Onyx.set(ONYXKEYS.NETWORK, {isOffline: false}); + }) + .then(waitForBatchedUpdates) + .then(() => { + expect(PersistedRequests.getAll().length).toBe(0); + }); + }); + test('ReconnectApp should replace same requests from the queue', () => { + return TestHelper.signInWithTestUser() + .then(() => Onyx.set(ONYXKEYS.NETWORK, {isOffline: true})) + .then(() => { + App.confirmReadyToOpenApp(); + App.reconnectApp(); + App.reconnectApp(); + App.reconnectApp(); + App.reconnectApp(); + }) + .then(waitForBatchedUpdates) + .then(() => { + expect(PersistedRequests.getAll().length).toBe(1); + }) + .then(() => Onyx.set(ONYXKEYS.NETWORK, {isOffline: false})) + .then(() => { + expect(PersistedRequests.getAll().length).toBe(0); + }); + }); }); diff --git a/tests/unit/PersistedRequests.ts b/tests/unit/PersistedRequests.ts index 3a814ca28c2..476b3f96395 100644 --- a/tests/unit/PersistedRequests.ts +++ b/tests/unit/PersistedRequests.ts @@ -47,6 +47,7 @@ describe('PersistedRequests', () => { }); it('when onyx persist the request, it should remove from the list the ongoing request', () => { + expect(PersistedRequests.getAll().length).toBe(1); const request2: Request = { command: 'AddComment', successData: [{key: 'reportMetadata_3', onyxMethod: 'merge', value: {}}], From dc6aa40a0aa77b870366a793b74ad69166543c4b Mon Sep 17 00:00:00 2001 From: Eduardo Date: Tue, 17 Sep 2024 13:04:20 +0200 Subject: [PATCH 12/22] Cleanup --- src/libs/Network/SequentialQueue.ts | 2 - src/libs/ReportActionsUtils.ts | 1 - tests/actions/SessionTest.ts | 185 ++++++++++++++-------------- 3 files changed, 94 insertions(+), 94 deletions(-) diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index 1502b822463..82093e3eb45 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -25,7 +25,6 @@ let isReadyPromise = new Promise((resolve) => { resolveIsReadyPromise?.(); let isSequentialQueueRunning = false; -// let currentRequest: OnyxRequest | null = null; let currentRequestPromise: Promise | null = null; let isQueuePaused = false; @@ -164,7 +163,6 @@ function flush() { if (NetworkStore.isOffline() || PersistedRequests.getAll().length === 0) { resolveIsReadyPromise?.(); } - // currentRequest = null; currentRequestPromise = null; // The queue can be paused when we sync the data with backend so we should only update the Onyx data when the queue is empty diff --git a/src/libs/ReportActionsUtils.ts b/src/libs/ReportActionsUtils.ts index 422904e8b5a..c78406df139 100644 --- a/src/libs/ReportActionsUtils.ts +++ b/src/libs/ReportActionsUtils.ts @@ -56,7 +56,6 @@ Onyx.connect({ if (!actions) { return; } - // console.log('allReportActions', actions); allReportActions = actions; }, }); diff --git a/tests/actions/SessionTest.ts b/tests/actions/SessionTest.ts index f36efa8e4cd..e46806bef99 100644 --- a/tests/actions/SessionTest.ts +++ b/tests/actions/SessionTest.ts @@ -3,6 +3,7 @@ import Onyx from 'react-native-onyx'; import type {OnyxEntry} from 'react-native-onyx'; import * as App from '@libs/actions/App'; import OnyxUpdateManager from '@libs/actions/OnyxUpdateManager'; +import {WRITE_COMMANDS} from '@libs/API/types'; import HttpUtils from '@libs/HttpUtils'; import PushNotification from '@libs/Notification/PushNotification'; // This lib needs to be imported, but it has nothing to export since all it contains is an Onyx connection @@ -29,7 +30,7 @@ OnyxUpdateManager(); beforeEach(() => Onyx.clear().then(waitForBatchedUpdates)); describe('Session', () => { - test('Authenticate is called with saved credentials when a session expires', () => { + test('Authenticate is called with saved credentials when a session expires', async () => { // Given a test user and set of authToken with subscriptions to session and credentials const TEST_USER_LOGIN = 'test@testguy.com'; const TEST_USER_ACCOUNT_ID = 1; @@ -49,98 +50,100 @@ describe('Session', () => { }); // When we sign in with the test user - return TestHelper.signInWithTestUser(TEST_USER_ACCOUNT_ID, TEST_USER_LOGIN, 'Password1', TEST_INITIAL_AUTH_TOKEN) - .then(waitForBatchedUpdates) - .then(() => { - // Then our re-authentication credentials should be generated and our session data - // have the correct information + initial authToken. - expect(credentials?.login).toBe(TEST_USER_LOGIN); - expect(credentials?.autoGeneratedLogin).not.toBeUndefined(); - expect(credentials?.autoGeneratedPassword).not.toBeUndefined(); - expect(session?.authToken).toBe(TEST_INITIAL_AUTH_TOKEN); - expect(session?.accountID).toBe(TEST_USER_ACCOUNT_ID); - expect(session?.email).toBe(TEST_USER_LOGIN); - - // At this point we have an authToken. To simulate it expiring we'll just make another - // request and mock the response so it returns 407. Once this happens we should attempt - // to Re-Authenticate with the stored credentials. Our next call will be to Authenticate - // so we will mock that response with a new authToken and then verify that Onyx has our - // data. - (HttpUtils.xhr as jest.MockedFunction) - - // This will make the call to OpenApp below return with an expired session code - .mockImplementationOnce(() => - Promise.resolve({ - jsonCode: CONST.JSON_CODE.NOT_AUTHENTICATED, - }), - ) - - // The next call should be Authenticate since we are reauthenticating - .mockImplementationOnce(() => - Promise.resolve({ - jsonCode: CONST.JSON_CODE.SUCCESS, - accountID: TEST_USER_ACCOUNT_ID, - authToken: TEST_REFRESHED_AUTH_TOKEN, - email: TEST_USER_LOGIN, - }), - ); - - // When we attempt to fetch the initial app data via the API - App.confirmReadyToOpenApp(); - App.openApp(); - return waitForBatchedUpdates(); - }) - .then(() => { - // Then it should fail and reauthenticate the user adding the new authToken to the session - // data in Onyx - expect(session?.authToken).toBe(TEST_REFRESHED_AUTH_TOKEN); - }); + await TestHelper.signInWithTestUser(TEST_USER_ACCOUNT_ID, TEST_USER_LOGIN, 'Password1', TEST_INITIAL_AUTH_TOKEN); + await waitForBatchedUpdates(); + + // Then our re-authentication credentials should be generated and our session data + // have the correct information + initial authToken. + expect(credentials?.login).toBe(TEST_USER_LOGIN); + expect(credentials?.autoGeneratedLogin).not.toBeUndefined(); + expect(credentials?.autoGeneratedPassword).not.toBeUndefined(); + expect(session?.authToken).toBe(TEST_INITIAL_AUTH_TOKEN); + expect(session?.accountID).toBe(TEST_USER_ACCOUNT_ID); + expect(session?.email).toBe(TEST_USER_LOGIN); + + // At this point we have an authToken. To simulate it expiring we'll just make another + // request and mock the response so it returns 407. Once this happens we should attempt + // to Re-Authenticate with the stored credentials. Our next call will be to Authenticate + // so we will mock that response with a new authToken and then verify that Onyx has our + // data. + (HttpUtils.xhr as jest.MockedFunction) + + // This will make the call to OpenApp below return with an expired session code + .mockImplementationOnce(() => + Promise.resolve({ + jsonCode: CONST.JSON_CODE.NOT_AUTHENTICATED, + }), + ) + + // The next call should be Authenticate since we are reauthenticating + .mockImplementationOnce(() => + Promise.resolve({ + jsonCode: CONST.JSON_CODE.SUCCESS, + accountID: TEST_USER_ACCOUNT_ID, + authToken: TEST_REFRESHED_AUTH_TOKEN, + email: TEST_USER_LOGIN, + }), + ); + + // When we attempt to fetch the initial app data via the API + App.confirmReadyToOpenApp(); + App.openApp(); + await waitForBatchedUpdates(); + + // Then it should fail and reauthenticate the user adding the new authToken to the session + // data in Onyx + expect(session?.authToken).toBe(TEST_REFRESHED_AUTH_TOKEN); }); - test('Push notifications are subscribed after signing in', () => - TestHelper.signInWithTestUser() - .then(waitForBatchedUpdates) - .then(() => expect(PushNotification.register).toBeCalled())); - - test('Push notifications are unsubscribed after signing out', () => - TestHelper.signInWithTestUser() - .then(TestHelper.signOutTestUser) - .then(() => expect(PushNotification.deregister).toBeCalled())); - - test('ReconnectApp should push request to the queue', () => { - return TestHelper.signInWithTestUser() - .then(() => Onyx.set(ONYXKEYS.NETWORK, {isOffline: true})) - .then(() => { - App.confirmReadyToOpenApp(); - App.reconnectApp(); - }) - .then(waitForBatchedUpdates) - .then(() => { - expect(PersistedRequests.getAll().length).toBe(1); - Onyx.set(ONYXKEYS.NETWORK, {isOffline: false}); - }) - .then(waitForBatchedUpdates) - .then(() => { - expect(PersistedRequests.getAll().length).toBe(0); - }); + test('Push notifications are subscribed after signing in', async () => { + await TestHelper.signInWithTestUser(); + await waitForBatchedUpdates(); + expect(PushNotification.register).toBeCalled(); + }); + + test('Push notifications are unsubscribed after signing out', async () => { + await TestHelper.signInWithTestUser(); + await TestHelper.signOutTestUser(); + expect(PushNotification.deregister).toBeCalled(); + }); + + test('ReconnectApp should push request to the queue', async () => { + await TestHelper.signInWithTestUser(); + await Onyx.set(ONYXKEYS.NETWORK, {isOffline: true}); + + App.confirmReadyToOpenApp(); + App.reconnectApp(); + + await waitForBatchedUpdates(); + + expect(PersistedRequests.getAll().length).toBe(1); + expect(PersistedRequests.getAll()[0].command).toBe(WRITE_COMMANDS.RECONNECT_APP); + + await Onyx.set(ONYXKEYS.NETWORK, {isOffline: false}); + + await waitForBatchedUpdates(); + + expect(PersistedRequests.getAll().length).toBe(0); }); - test('ReconnectApp should replace same requests from the queue', () => { - return TestHelper.signInWithTestUser() - .then(() => Onyx.set(ONYXKEYS.NETWORK, {isOffline: true})) - .then(() => { - App.confirmReadyToOpenApp(); - App.reconnectApp(); - App.reconnectApp(); - App.reconnectApp(); - App.reconnectApp(); - }) - .then(waitForBatchedUpdates) - .then(() => { - expect(PersistedRequests.getAll().length).toBe(1); - }) - .then(() => Onyx.set(ONYXKEYS.NETWORK, {isOffline: false})) - .then(() => { - expect(PersistedRequests.getAll().length).toBe(0); - }); + + test('ReconnectApp should replace same requests from the queue', async () => { + await TestHelper.signInWithTestUser(); + await Onyx.set(ONYXKEYS.NETWORK, {isOffline: true}); + + App.confirmReadyToOpenApp(); + App.reconnectApp(); + App.reconnectApp(); + App.reconnectApp(); + App.reconnectApp(); + + await waitForBatchedUpdates(); + + expect(PersistedRequests.getAll().length).toBe(1); + expect(PersistedRequests.getAll()[0].command).toBe(WRITE_COMMANDS.RECONNECT_APP); + + await Onyx.set(ONYXKEYS.NETWORK, {isOffline: false}); + + expect(PersistedRequests.getAll().length).toBe(0); }); }); From b6b893be8635b1b8ec27770f3022eaa5a215179c Mon Sep 17 00:00:00 2001 From: Eduardo Date: Tue, 17 Sep 2024 15:30:38 +0200 Subject: [PATCH 13/22] Fixed some deprecated eslints --- src/libs/ReportActionsUtils.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libs/ReportActionsUtils.ts b/src/libs/ReportActionsUtils.ts index 3f9436d1db2..3dd6925614f 100644 --- a/src/libs/ReportActionsUtils.ts +++ b/src/libs/ReportActionsUtils.ts @@ -213,9 +213,9 @@ function isActionOfType( function getOriginalMessage(reportAction: OnyxInputOrEntry>): OriginalMessage | undefined { if (!Array.isArray(reportAction?.message)) { - return reportAction?.message ?? reportAction?.originalMessage; + return reportAction?.message ?? getOriginalMessage(reportAction); } - return reportAction.originalMessage; + return getOriginalMessage(reportAction); } function isExportIntegrationAction(reportAction: OnyxInputOrEntry): boolean { @@ -591,7 +591,7 @@ function isReportActionDeprecated(reportAction: OnyxEntry, key: st // HACK ALERT: We're temporarily filtering out any reportActions keyed by sequenceNumber // to prevent bugs during the migration from sequenceNumber -> reportActionID - if (String(reportAction.sequenceNumber) === key) { + if (String(reportAction.reportActionID) === key) { Log.info('Front-end filtered out reportAction keyed by sequenceNumber!', false, reportAction); return true; } @@ -1696,7 +1696,7 @@ function isCardIssuedAction(reportAction: OnyxEntry) { } function getCardIssuedMessage(reportAction: OnyxEntry, shouldRenderHTML = false) { - const assigneeAccountID = (reportAction?.originalMessage as IssueNewCardOriginalMessage)?.assigneeAccountID; + const assigneeAccountID = (getOriginalMessage(reportAction) as IssueNewCardOriginalMessage)?.assigneeAccountID ?? -1; const assigneeDetails = PersonalDetailsUtils.getPersonalDetailsByIDs([assigneeAccountID], currentUserAccountID ?? -1)[0]; const assignee = shouldRenderHTML ? `` : assigneeDetails?.firstName ?? assigneeDetails.login ?? ''; From 1fcd87d327be21c085ab1194d5d24164e5fcedef Mon Sep 17 00:00:00 2001 From: Eduardo Date: Tue, 17 Sep 2024 17:15:15 +0200 Subject: [PATCH 14/22] Rolling back change --- src/libs/ReportActionsUtils.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libs/ReportActionsUtils.ts b/src/libs/ReportActionsUtils.ts index 3dd6925614f..3d0f3e1989a 100644 --- a/src/libs/ReportActionsUtils.ts +++ b/src/libs/ReportActionsUtils.ts @@ -213,9 +213,9 @@ function isActionOfType( function getOriginalMessage(reportAction: OnyxInputOrEntry>): OriginalMessage | undefined { if (!Array.isArray(reportAction?.message)) { - return reportAction?.message ?? getOriginalMessage(reportAction); + return reportAction?.message ?? reportAction?.originalMessage; } - return getOriginalMessage(reportAction); + return reportAction?.originalMessage; } function isExportIntegrationAction(reportAction: OnyxInputOrEntry): boolean { From 3b21e417a06e8aa863f75fb2c29d13191077fa13 Mon Sep 17 00:00:00 2001 From: Eduardo Date: Tue, 17 Sep 2024 21:45:17 +0200 Subject: [PATCH 15/22] disable deprecation fields --- .eslintrc.js | 3 ++- src/libs/ReportActionsUtils.ts | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/.eslintrc.js b/.eslintrc.js index cb121953327..c9d11b97d7d 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -108,7 +108,7 @@ module.exports = { 'plugin:you-dont-need-lodash-underscore/all', 'plugin:prettier/recommended', ], - plugins: ['@typescript-eslint', 'jsdoc', 'you-dont-need-lodash-underscore', 'react-native-a11y', 'react', 'testing-library', 'eslint-plugin-react-compiler'], + plugins: ['@typescript-eslint', 'jsdoc', 'you-dont-need-lodash-underscore', 'react-native-a11y', 'react', 'testing-library', 'eslint-plugin-react-compiler', 'deprecation'], ignorePatterns: ['lib/**'], parser: '@typescript-eslint/parser', parserOptions: { @@ -254,6 +254,7 @@ module.exports = { }, }, ], + 'deprecation/deprecation': 'off', }, overrides: [ diff --git a/src/libs/ReportActionsUtils.ts b/src/libs/ReportActionsUtils.ts index 3d0f3e1989a..1bed7fcac48 100644 --- a/src/libs/ReportActionsUtils.ts +++ b/src/libs/ReportActionsUtils.ts @@ -213,8 +213,10 @@ function isActionOfType( function getOriginalMessage(reportAction: OnyxInputOrEntry>): OriginalMessage | undefined { if (!Array.isArray(reportAction?.message)) { + // eslint-disable-next-line deprecation/deprecation return reportAction?.message ?? reportAction?.originalMessage; } + // eslint-disable-next-line deprecation/deprecation return reportAction?.originalMessage; } @@ -1752,6 +1754,7 @@ export { getNumberOfMoneyRequests, getOneTransactionThreadReportID, getOriginalMessage, + // eslint-disable-next-line deprecation/deprecation getParentReportAction, getRemovedFromApprovalChainMessage, getReportAction, From 919e2548038e348a825e2adc97ac0a6706786806 Mon Sep 17 00:00:00 2001 From: Eduardo Date: Wed, 18 Sep 2024 12:06:35 +0200 Subject: [PATCH 16/22] Removed hack, seems we don't need it anymore --- src/libs/ReportActionsUtils.ts | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/libs/ReportActionsUtils.ts b/src/libs/ReportActionsUtils.ts index 1bed7fcac48..ab9ccafa057 100644 --- a/src/libs/ReportActionsUtils.ts +++ b/src/libs/ReportActionsUtils.ts @@ -591,13 +591,6 @@ function isReportActionDeprecated(reportAction: OnyxEntry, key: st return true; } - // HACK ALERT: We're temporarily filtering out any reportActions keyed by sequenceNumber - // to prevent bugs during the migration from sequenceNumber -> reportActionID - if (String(reportAction.reportActionID) === key) { - Log.info('Front-end filtered out reportAction keyed by sequenceNumber!', false, reportAction); - return true; - } - const deprecatedOldDotReportActions: ReportActionName[] = [ CONST.REPORT.ACTIONS.TYPE.DELETED_ACCOUNT, CONST.REPORT.ACTIONS.TYPE.REIMBURSEMENT_REQUESTED, From 1c5c49b7e17d908932f20b8273c1a83380e021ac Mon Sep 17 00:00:00 2001 From: Eduardo Date: Wed, 18 Sep 2024 12:20:16 +0200 Subject: [PATCH 17/22] Removed unused key --- .../AttachmentCarousel/extractAttachments.ts | 4 ++-- src/components/ParentNavigationSubtitle.tsx | 2 +- src/libs/OptionsListUtils.ts | 8 ++++---- src/libs/ReportActionsUtils.ts | 12 ++++++------ src/pages/home/ReportScreen.tsx | 2 +- .../home/report/ReportActionItemParentAction.tsx | 2 +- src/pages/home/report/ReportActionsList.tsx | 2 +- src/pages/home/report/ThreadDivider.tsx | 2 +- 8 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/components/Attachments/AttachmentCarousel/extractAttachments.ts b/src/components/Attachments/AttachmentCarousel/extractAttachments.ts index 81ee6d08934..7183dd1c59c 100644 --- a/src/components/Attachments/AttachmentCarousel/extractAttachments.ts +++ b/src/components/Attachments/AttachmentCarousel/extractAttachments.ts @@ -94,8 +94,8 @@ function extractAttachments( } const actions = [...(parentReportAction ? [parentReportAction] : []), ...ReportActionsUtils.getSortedReportActions(Object.values(reportActions ?? {}))]; - actions.forEach((action, key) => { - if (!ReportActionsUtils.shouldReportActionBeVisible(action, key) || ReportActionsUtils.isMoneyRequestAction(action)) { + actions.forEach((action) => { + if (!ReportActionsUtils.shouldReportActionBeVisible(action) || ReportActionsUtils.isMoneyRequestAction(action)) { return; } diff --git a/src/components/ParentNavigationSubtitle.tsx b/src/components/ParentNavigationSubtitle.tsx index f60b877a5d2..433ff6f0f29 100644 --- a/src/components/ParentNavigationSubtitle.tsx +++ b/src/components/ParentNavigationSubtitle.tsx @@ -39,7 +39,7 @@ function ParentNavigationSubtitle({parentNavigationSubtitleData, parentReportAct { const parentAction = ReportActionsUtils.getReportAction(parentReportID, parentReportActionID ?? '-1'); - const isVisibleAction = ReportActionsUtils.shouldReportActionBeVisible(parentAction, parentAction?.reportActionID ?? '-1'); + const isVisibleAction = ReportActionsUtils.shouldReportActionBeVisible(parentAction); // Pop the thread report screen before navigating to the chat report. Navigation.goBack(ROUTES.REPORT_WITH_ID.getRoute(parentReportID)); if (isVisibleAction && !isOffline) { diff --git a/src/libs/OptionsListUtils.ts b/src/libs/OptionsListUtils.ts index afedd308371..edaeba37e86 100644 --- a/src/libs/OptionsListUtils.ts +++ b/src/libs/OptionsListUtils.ts @@ -315,8 +315,8 @@ Onyx.connect({ // The report is only visible if it is the last action not deleted that // does not match a closed or created state. const reportActionsForDisplay = sortedReportActions.filter( - (reportAction, actionKey) => - ReportActionUtils.shouldReportActionBeVisible(reportAction, actionKey) && + (reportAction) => + ReportActionUtils.shouldReportActionBeVisible(reportAction) && !ReportActionUtils.isWhisperAction(reportAction) && reportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.CREATED && reportAction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE && @@ -624,8 +624,8 @@ function getLastMessageTextForReport(report: OnyxEntry, lastActorDetails } else if (ReportActionUtils.isReportPreviewAction(lastReportAction)) { const iouReport = ReportUtils.getReportOrDraftReport(ReportActionUtils.getIOUReportIDFromReportActionPreview(lastReportAction)); const lastIOUMoneyReportAction = allSortedReportActions[iouReport?.reportID ?? '-1']?.find( - (reportAction, key): reportAction is ReportAction => - ReportActionUtils.shouldReportActionBeVisible(reportAction, key) && + (reportAction): reportAction is ReportAction => + ReportActionUtils.shouldReportActionBeVisible(reportAction) && reportAction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE && ReportActionUtils.isMoneyRequestAction(reportAction), ); diff --git a/src/libs/ReportActionsUtils.ts b/src/libs/ReportActionsUtils.ts index ab9ccafa057..cefa569048f 100644 --- a/src/libs/ReportActionsUtils.ts +++ b/src/libs/ReportActionsUtils.ts @@ -586,7 +586,7 @@ function isConsecutiveActionMadeByPreviousActor(reportActions: ReportAction[] | /** * Checks if a reportAction is deprecated. */ -function isReportActionDeprecated(reportAction: OnyxEntry, key: string | number): boolean { +function isReportActionDeprecated(reportAction: OnyxEntry): boolean { if (!reportAction) { return true; } @@ -612,12 +612,12 @@ const supportedActionTypes: ReportActionName[] = [...Object.values(otherActionTy * Checks if a reportAction is fit for display, meaning that it's not deprecated, is of a valid * and supported type, it's not deleted and also not closed. */ -function shouldReportActionBeVisible(reportAction: OnyxEntry, key: string | number): boolean { +function shouldReportActionBeVisible(reportAction: OnyxEntry): boolean { if (!reportAction) { return false; } - if (isReportActionDeprecated(reportAction, key)) { + if (isReportActionDeprecated(reportAction)) { return false; } @@ -700,7 +700,7 @@ function shouldReportActionBeVisibleAsLastAction(reportAction: OnyxInputOrEntry< // If a whisper action is the REPORT_PREVIEW action, we are displaying it. // If the action's message text is empty and it is not a deleted parent with visible child actions, hide it. Else, consider the action to be displayable. return ( - shouldReportActionBeVisible(reportAction, reportAction.reportActionID) && + shouldReportActionBeVisible(reportAction) && !(isWhisperAction(reportAction) && !isReportPreviewAction(reportAction) && !isMoneyRequestAction(reportAction)) && !(isDeletedAction(reportAction) && !isDeletedParentAction(reportAction)) && !isResolvedActionTrackExpense(reportAction) @@ -782,7 +782,7 @@ function getLastVisibleMessage( */ function filterOutDeprecatedReportActions(reportActions: OnyxEntry): ReportAction[] { return Object.entries(reportActions ?? {}) - .filter(([key, reportAction]) => !isReportActionDeprecated(reportAction, key)) + .filter(([, reportAction]) => !isReportActionDeprecated(reportAction)) .map((entry) => entry[1]); } @@ -802,7 +802,7 @@ function getSortedReportActionsForDisplay(reportActions: OnyxEntry shouldReportActionBeVisible(reportAction, key)) + .filter(([, reportAction]) => shouldReportActionBeVisible(reportAction)) .map(([, reportAction]) => reportAction); } diff --git a/src/pages/home/ReportScreen.tsx b/src/pages/home/ReportScreen.tsx index 9da19f192c1..38fd9e5729a 100644 --- a/src/pages/home/ReportScreen.tsx +++ b/src/pages/home/ReportScreen.tsx @@ -377,7 +377,7 @@ function ReportScreen({route, currentReportID = '', navigation}: ReportScreenPro ? reportActions.length > 0 : reportActions.length >= CONST.REPORT.MIN_INITIAL_REPORT_ACTION_COUNT || isPendingActionExist || (doesCreatedActionExists() && reportActions.length > 0); - const isLinkedActionDeleted = useMemo(() => !!linkedAction && !ReportActionsUtils.shouldReportActionBeVisible(linkedAction, linkedAction.reportActionID), [linkedAction]); + const isLinkedActionDeleted = useMemo(() => !!linkedAction && !ReportActionsUtils.shouldReportActionBeVisible(linkedAction), [linkedAction]); const prevIsLinkedActionDeleted = usePrevious(linkedAction ? isLinkedActionDeleted : undefined); const isLinkedActionInaccessibleWhisper = useMemo( () => !!linkedAction && ReportActionsUtils.isWhisperAction(linkedAction) && !(linkedAction?.whisperedToAccountIDs ?? []).includes(currentUserAccountID), diff --git a/src/pages/home/report/ReportActionItemParentAction.tsx b/src/pages/home/report/ReportActionItemParentAction.tsx index d3842786b16..e8ee03f0234 100644 --- a/src/pages/home/report/ReportActionItemParentAction.tsx +++ b/src/pages/home/report/ReportActionItemParentAction.tsx @@ -130,7 +130,7 @@ function ReportActionItemParentAction({ onPress={ ReportUtils.canCurrentUserOpenReport(ancestorReports.current?.[ancestor?.report?.reportID ?? '-1']) ? () => { - const isVisibleAction = ReportActionsUtils.shouldReportActionBeVisible(ancestor.reportAction, ancestor.reportAction.reportActionID ?? '-1'); + const isVisibleAction = ReportActionsUtils.shouldReportActionBeVisible(ancestor.reportAction); // Pop the thread report screen before navigating to the chat report. Navigation.goBack(ROUTES.REPORT_WITH_ID.getRoute(ancestor.report.reportID ?? '-1')); if (isVisibleAction && !isOffline) { diff --git a/src/pages/home/report/ReportActionsList.tsx b/src/pages/home/report/ReportActionsList.tsx index 6828e10e7e3..667e94944e3 100644 --- a/src/pages/home/report/ReportActionsList.tsx +++ b/src/pages/home/report/ReportActionsList.tsx @@ -195,7 +195,7 @@ function ReportActionsList({ ReportActionsUtils.isDeletedParentAction(reportAction) || reportAction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE || reportAction.errors) && - ReportActionsUtils.shouldReportActionBeVisible(reportAction, reportAction.reportActionID), + ReportActionsUtils.shouldReportActionBeVisible(reportAction), ), [sortedReportActions, isOffline], ); diff --git a/src/pages/home/report/ThreadDivider.tsx b/src/pages/home/report/ThreadDivider.tsx index d2ffa97f58b..f1ec14b332b 100644 --- a/src/pages/home/report/ThreadDivider.tsx +++ b/src/pages/home/report/ThreadDivider.tsx @@ -47,7 +47,7 @@ function ThreadDivider({ancestor, isLinkDisabled = false}: ThreadDividerProps) { ) : ( { - const isVisibleAction = ReportActionsUtils.shouldReportActionBeVisible(ancestor.reportAction, ancestor.reportAction.reportActionID ?? '-1'); + const isVisibleAction = ReportActionsUtils.shouldReportActionBeVisible(ancestor.reportAction); // Pop the thread report screen before navigating to the chat report. Navigation.goBack(ROUTES.REPORT_WITH_ID.getRoute(ancestor.report.reportID ?? '-1')); if (isVisibleAction && !isOffline) { From 9ff5edd510141344fa09b8eb1cd8c01929c86626 Mon Sep 17 00:00:00 2001 From: Eduardo Date: Tue, 24 Sep 2024 19:25:27 +0200 Subject: [PATCH 18/22] Added the chance to save the ongoing request into Onyx for later processing --- package-lock.json | 16 ------ src/ONYXKEYS.ts | 2 + src/libs/Network/SequentialQueue.ts | 13 ++++- src/libs/actions/PersistedRequests.ts | 42 ++++++++++----- src/types/onyx/Request.ts | 6 +++ tests/unit/SequentialQueueTest.ts | 77 +++++++++++++++++++++++++++ 6 files changed, 125 insertions(+), 31 deletions(-) diff --git a/package-lock.json b/package-lock.json index 8d484d2a7e2..897de399cd0 100644 --- a/package-lock.json +++ b/package-lock.json @@ -27904,22 +27904,6 @@ "jest": "bin/jest.js" } }, - "node_modules/jest-expo/node_modules/@babel/code-frame": { - "version": "7.10.4", - "license": "MIT", - "dependencies": { - "@babel/highlight": "^7.10.4" - } - }, - "node_modules/jest-expo/node_modules/@expo/json-file": { - "version": "8.3.1", - "license": "MIT", - "dependencies": { - "@babel/code-frame": "~7.10.4", - "json5": "^2.2.2", - "write-file-atomic": "^2.3.0" - } - }, "node_modules/jest-expo/node_modules/json5": { "version": "2.2.2", "resolved": "https://registry.npmjs.org/json5/-/json5-2.2.2.tgz", diff --git a/src/ONYXKEYS.ts b/src/ONYXKEYS.ts index 38affd97c63..14a4243700f 100755 --- a/src/ONYXKEYS.ts +++ b/src/ONYXKEYS.ts @@ -32,6 +32,7 @@ const ONYXKEYS = { /** Note: These are Persisted Requests - not all requests in the main queue as the key name might lead one to believe */ PERSISTED_REQUESTS: 'networkRequestQueue', + PERSISTED_ONGOING_REQUESTS: 'networkOngoingRequestQueue', /** Stores current date */ CURRENT_DATE: 'currentDate', @@ -857,6 +858,7 @@ type OnyxValuesMapping = { [ONYXKEYS.DEVICE_ID]: string; [ONYXKEYS.IS_SIDEBAR_LOADED]: boolean; [ONYXKEYS.PERSISTED_REQUESTS]: OnyxTypes.Request[]; + [ONYXKEYS.PERSISTED_ONGOING_REQUESTS]: OnyxTypes.Request; [ONYXKEYS.CURRENT_DATE]: string; [ONYXKEYS.CREDENTIALS]: OnyxTypes.Credentials; [ONYXKEYS.STASHED_CREDENTIALS]: OnyxTypes.Credentials; diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index 82093e3eb45..14157d2533e 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -81,6 +81,10 @@ function process(): Promise { } const requestToProcess = PersistedRequests.processNextRequest(); + if (!requestToProcess) { + Log.info('[SequentialQueue] Unable to process. No next request to handle.'); + return Promise.resolve(); + } // Set the current request to a promise awaiting its processing so that getCurrentRequest can be used to take some action after the current request has processed. currentRequestPromise = Request.processWithMiddleware(requestToProcess, true) @@ -184,7 +188,7 @@ function unpause() { } const numberOfPersistedRequests = PersistedRequests.getAll().length || 0; - console.debug(`[SequentialQueue] Unpausing the queue and flushing ${numberOfPersistedRequests} requests`); + Log.info(`[SequentialQueue] Unpausing the queue and flushing ${numberOfPersistedRequests} requests`); isQueuePaused = false; flush(); } @@ -207,6 +211,11 @@ function push(newRequest: OnyxRequest) { const {checkAndFixConflictingRequest} = newRequest; if (checkAndFixConflictingRequest) { const {conflictAction} = checkAndFixConflictingRequest(requests); + Log.info(`[SequentialQueue] Conflict action for command ${newRequest.command} - ${conflictAction.type}:`); + + // No need to serialize it. + // eslint-disable-next-line no-param-reassign + delete newRequest.checkAndFixConflictingRequest; if (conflictAction.type === 'push') { PersistedRequests.save(newRequest); @@ -248,5 +257,5 @@ function waitForIdle(): Promise { return isReadyPromise; } -export {flush, getCurrentRequest, isRunning, isPaused, push, waitForIdle, pause, unpause}; +export {flush, getCurrentRequest, isRunning, isPaused, push, waitForIdle, pause, unpause, process}; export type {RequestError}; diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts index 49ed6bc6473..cee89f1fe23 100644 --- a/src/libs/actions/PersistedRequests.ts +++ b/src/libs/actions/PersistedRequests.ts @@ -10,11 +10,11 @@ let ongoingRequest: Request | null = null; Onyx.connect({ key: ONYXKEYS.PERSISTED_REQUESTS, callback: (val) => { - // it has the ongoingRequest in here? persistedRequests = val ?? []; if (ongoingRequest && persistedRequests.length > 0) { const nextRequestToProcess = persistedRequests[0]; + // We try to remove the next request from the persistedRequests if it is the same as ongoingRequest // so we don't process it twice. if (isEqual(nextRequestToProcess, ongoingRequest)) { @@ -23,6 +23,12 @@ Onyx.connect({ } }, }); +Onyx.connect({ + key: ONYXKEYS.PERSISTED_ONGOING_REQUESTS, + callback: (val) => { + ongoingRequest = val ?? null; + }, +}); /** * This promise is only used by tests. DO NOT USE THIS PROMISE IN THE APPLICATION CODE @@ -55,13 +61,16 @@ function remove(requestToRemove: Request) { const requests = [...persistedRequests]; const index = requests.findIndex((persistedRequest) => isEqual(persistedRequest, requestToRemove)); - if (index === -1) { - return; + if (index !== -1) { + requests.splice(index, 1); } - requests.splice(index, 1); + persistedRequests = requests; - Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests).then(() => { + Onyx.multiSet({ + [ONYXKEYS.PERSISTED_REQUESTS]: persistedRequests, + [ONYXKEYS.PERSISTED_ONGOING_REQUESTS]: null, + }).then(() => { Log.info(`[SequentialQueue] '${requestToRemove.command}' removed from the queue. Queue length is ${getLength()}`); }); } @@ -75,22 +84,29 @@ function update(oldRequestIndex: number, newRequest: Request) { function updateOngoingRequest(newRequest: Request) { ongoingRequest = newRequest; + + if (newRequest.persistWhenOngoing) { + Onyx.set(ONYXKEYS.PERSISTED_ONGOING_REQUESTS, newRequest); + } } -function processNextRequest(): Request { +function processNextRequest(): Request | null { + if (ongoingRequest) { + Log.info(`Ongoing Request already set returning same one ${ongoingRequest.commandName}`); + return ongoingRequest; + } + // You must handle the case where there are no requests to process if (persistedRequests.length === 0) { throw new Error('No requests to process'); } - // At least for now, you must handle the case where there is an ongoing request - if (ongoingRequest) { - throw new Error('There is already an ongoing request'); + ongoingRequest = persistedRequests.shift() ?? null; + + if (ongoingRequest && ongoingRequest.persistWhenOngoing) { + Onyx.set(ONYXKEYS.PERSISTED_ONGOING_REQUESTS, ongoingRequest); } - ongoingRequest = persistedRequests[0]; - persistedRequests = persistedRequests.slice(1); - // We don't need to update Onyx persistedRequests just in case the ongoingRequest fails - // we want to keep trying if the user closes the app + return ongoingRequest; } diff --git a/src/types/onyx/Request.ts b/src/types/onyx/Request.ts index 739e69669c4..238e3a8c6a8 100644 --- a/src/types/onyx/Request.ts +++ b/src/types/onyx/Request.ts @@ -109,6 +109,12 @@ type RequestConflictResolver = { * A function that checks if a new request conflicts with any existing requests in the queue. */ checkAndFixConflictingRequest?: (persistedRequest: Request[]) => ConflictActionData; + + /** + * A boolean flag to mark a request as persisting into Onyx, if set to true it means when Onyx loads + * the ongoing request, it will be removed from the persisted request queue. + */ + persistWhenOngoing?: boolean; }; /** Model of requests sent to the API */ diff --git a/tests/unit/SequentialQueueTest.ts b/tests/unit/SequentialQueueTest.ts index 2338e8f7c65..b795683995a 100644 --- a/tests/unit/SequentialQueueTest.ts +++ b/tests/unit/SequentialQueueTest.ts @@ -1,4 +1,5 @@ import Onyx from 'react-native-onyx'; +import {waitForActiveRequestsToBeEmpty} from '@libs/E2E/utils/NetworkInterceptor'; import * as PersistedRequests from '@userActions/PersistedRequests'; import ONYXKEYS from '@src/ONYXKEYS'; import * as SequentialQueue from '../../src/libs/Network/SequentialQueue'; @@ -180,4 +181,80 @@ describe('SequentialQueue', () => { // that was replaced by the new request. expect(persistedRequests[1]?.data?.accountID).toBe(56789); }); + + // need to test a rance condition between processing the next request and then pushing a new request with conflict resolver + it('should resolve the conflict and replace the correct request in the queue while a new request is picked up after unpausing', async () => { + SequentialQueue.pause(); + for (let i = 0; i < 5; i++) { + SequentialQueue.push({command: `OpenReport${i}`}); + SequentialQueue.push({command: `AddComment${i}`}); + } + SequentialQueue.push(request); + SequentialQueue.push({command: 'AddComment6'}); + SequentialQueue.push({command: 'OpenReport6'}); + // wait for Onyx.connect execute the callback and start processing the queue + await Promise.resolve(); + const requestWithConflictResolution: Request = { + command: 'ReconnectApp-replaced', + data: {accountID: 56789}, + checkAndFixConflictingRequest: (persistedRequests) => { + // should be one instance of ReconnectApp, get the index to replace it later + const index = persistedRequests.findIndex((r) => r.command === 'ReconnectApp'); + if (index === -1) { + return {conflictAction: {type: 'push'}}; + } + + return { + conflictAction: {type: 'replace', index}, + }; + }, + }; + + Promise.resolve().then(() => { + SequentialQueue.unpause(); + }); + Promise.resolve().then(() => { + SequentialQueue.push(requestWithConflictResolution); + }); + + await Promise.resolve(); + await waitForActiveRequestsToBeEmpty(); + const persistedRequests = PersistedRequests.getAll(); + + // We know ReconnectApp is at index 9 in the queue, so we can get it to verify + // that was replaced by the new request. + expect(persistedRequests[9]?.command).toBe('ReconnectApp-replaced'); + expect(persistedRequests[9]?.data?.accountID).toBe(56789); + }); + + // I need to test now when moving the request from the queue to the ongoing request the PERSISTED_REQUESTS is decreased and PERSISTED_ONGOING_REQUESTS has the new request + it('should move the request from the queue to the ongoing request and save it into Onyx', async () => { + const persistedRequest = {...request, persistWhenOngoing: true}; + SequentialQueue.push(persistedRequest); + + const connectionId = Onyx.connect({ + key: ONYXKEYS.PERSISTED_ONGOING_REQUESTS, + callback: (ongoingRequest) => { + if (!ongoingRequest) { + return; + } + + Onyx.disconnect(connectionId); + expect(ongoingRequest).toEqual(persistedRequest); + expect(ongoingRequest).toEqual(PersistedRequests.getOngoingRequest()); + expect(PersistedRequests.getAll().length).toBe(0); + }, + }); + }); + + it('should get the ongoing request from onyx and start processing it', async () => { + const persistedRequest = {...request, persistWhenOngoing: true}; + Onyx.set(ONYXKEYS.PERSISTED_ONGOING_REQUESTS, persistedRequest); + SequentialQueue.push({command: 'OpenReport'}); + + await Promise.resolve(); + + expect(persistedRequest).toEqual(PersistedRequests.getOngoingRequest()); + expect(PersistedRequests.getAll().length).toBe(1); + }); }); From 2bc9f32740ddff7f4d64e89c940c2d858eb802de Mon Sep 17 00:00:00 2001 From: Eduardo Date: Tue, 24 Sep 2024 19:51:53 +0200 Subject: [PATCH 19/22] fixed eslint error --- tests/unit/SequentialQueueTest.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/SequentialQueueTest.ts b/tests/unit/SequentialQueueTest.ts index b795683995a..8651d7e95e3 100644 --- a/tests/unit/SequentialQueueTest.ts +++ b/tests/unit/SequentialQueueTest.ts @@ -228,7 +228,7 @@ describe('SequentialQueue', () => { }); // I need to test now when moving the request from the queue to the ongoing request the PERSISTED_REQUESTS is decreased and PERSISTED_ONGOING_REQUESTS has the new request - it('should move the request from the queue to the ongoing request and save it into Onyx', async () => { + it('should move the request from the queue to the ongoing request and save it into Onyx', () => { const persistedRequest = {...request, persistWhenOngoing: true}; SequentialQueue.push(persistedRequest); From 703957d7c007db384a84458da6a702f60c8223df Mon Sep 17 00:00:00 2001 From: Eduardo Date: Fri, 27 Sep 2024 21:25:37 +0200 Subject: [PATCH 20/22] Fixed comments --- src/libs/Network/SequentialQueue.ts | 7 +++---- src/libs/ReportActionsUtils.ts | 8 ++++---- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index 14157d2533e..35c7b2bf779 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -205,15 +205,14 @@ function isPaused(): boolean { NetworkStore.onReconnection(flush); function push(newRequest: OnyxRequest) { - // If a request is already being processed, ignore it when looking for potentially conflicting requests - const requests = PersistedRequests.getAll(); - const {checkAndFixConflictingRequest} = newRequest; + if (checkAndFixConflictingRequest) { + const requests = PersistedRequests.getAll(); const {conflictAction} = checkAndFixConflictingRequest(requests); Log.info(`[SequentialQueue] Conflict action for command ${newRequest.command} - ${conflictAction.type}:`); - // No need to serialize it. + // don't try to serialize a function. // eslint-disable-next-line no-param-reassign delete newRequest.checkAndFixConflictingRequest; diff --git a/src/libs/ReportActionsUtils.ts b/src/libs/ReportActionsUtils.ts index ee2da7f1f84..6b176edb691 100644 --- a/src/libs/ReportActionsUtils.ts +++ b/src/libs/ReportActionsUtils.ts @@ -215,7 +215,7 @@ function isActionOfType( function getOriginalMessage(reportAction: OnyxInputOrEntry>): OriginalMessage | undefined { if (!Array.isArray(reportAction?.message)) { // eslint-disable-next-line deprecation/deprecation - return reportAction?.message ?? reportAction.originalMessage; + return reportAction?.message ?? reportAction?.originalMessage; } // eslint-disable-next-line deprecation/deprecation return reportAction.originalMessage; @@ -697,7 +697,7 @@ function isResolvedActionTrackExpense(reportAction: OnyxEntry): bo * Checks if a reportAction is fit for display as report last action, meaning that * it satisfies shouldReportActionBeVisible, it's not whisper action and not deleted. */ -function shouldReportActionBeVisibleAsLastAction(reportAction: OnyxInputOrEntry, key: string | number): boolean { +function shouldReportActionBeVisibleAsLastAction(reportAction: OnyxInputOrEntry): boolean { if (!reportAction) { return false; } @@ -709,7 +709,7 @@ function shouldReportActionBeVisibleAsLastAction(reportAction: OnyxInputOrEntry< // If a whisper action is the REPORT_PREVIEW action, we are displaying it. // If the action's message text is empty and it is not a deleted parent with visible child actions, hide it. Else, consider the action to be displayable. return ( - shouldReportActionBeVisible(reportAction, key) && + shouldReportActionBeVisible(reportAction, reportAction.reportActionID) && !(isWhisperAction(reportAction) && !isReportPreviewAction(reportAction) && !isMoneyRequestAction(reportAction)) && !(isDeletedAction(reportAction) && !isDeletedParentAction(reportAction)) && !isResolvedActionTrackExpense(reportAction) @@ -823,7 +823,7 @@ function getSortedReportActionsForDisplay(reportActions: OnyxEntry shouldReportActionBeVisible(reportAction)) + .filter(([key, reportAction]) => shouldReportActionBeVisible(reportAction, key)) .map(([, reportAction]) => reportAction); } From 7d90d667703336556eacce2c8bc51b5e1a6ef5ce Mon Sep 17 00:00:00 2001 From: Eduardo Date: Fri, 27 Sep 2024 21:27:03 +0200 Subject: [PATCH 21/22] replaced with unshift --- src/libs/actions/PersistedRequests.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts index cee89f1fe23..77d6e388438 100644 --- a/src/libs/actions/PersistedRequests.ts +++ b/src/libs/actions/PersistedRequests.ts @@ -116,7 +116,7 @@ function rollbackOngoingRequest() { } // Prepend ongoingRequest to persistedRequests - persistedRequests = [ongoingRequest, ...persistedRequests]; + persistedRequests.unshift(ongoingRequest); // Clear the ongoingRequest ongoingRequest = null; From 1e0d4edf4dfb06fb2bcb74bcd12e2d07b3cbe4ae Mon Sep 17 00:00:00 2001 From: Eduardo Date: Tue, 1 Oct 2024 11:37:06 +0200 Subject: [PATCH 22/22] removed duplicated rule --- .eslintrc.js | 1 - 1 file changed, 1 deletion(-) diff --git a/.eslintrc.js b/.eslintrc.js index e09ed9be479..761a62b8314 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -256,7 +256,6 @@ module.exports = { }, }, ], - 'deprecation/deprecation': 'off', }, overrides: [