diff --git a/android/app/build.gradle b/android/app/build.gradle index 143e64f6a2b4..4ef7f4e25a3c 100644 --- a/android/app/build.gradle +++ b/android/app/build.gradle @@ -98,8 +98,8 @@ android { minSdkVersion rootProject.ext.minSdkVersion targetSdkVersion rootProject.ext.targetSdkVersion multiDexEnabled rootProject.ext.multiDexEnabled - versionCode 1001045703 - versionName "1.4.57-3" + versionCode 1001045704 + versionName "1.4.57-4" } flavorDimensions "default" diff --git a/ios/NewExpensify/Info.plist b/ios/NewExpensify/Info.plist index 99be3c26fe5e..a6e2bce73b3b 100644 --- a/ios/NewExpensify/Info.plist +++ b/ios/NewExpensify/Info.plist @@ -40,7 +40,7 @@ CFBundleVersion - 1.4.57.3 + 1.4.57.4 ITSAppUsesNonExemptEncryption LSApplicationQueriesSchemes diff --git a/ios/NewExpensifyTests/Info.plist b/ios/NewExpensifyTests/Info.plist index fa9307884b76..471d8d834886 100644 --- a/ios/NewExpensifyTests/Info.plist +++ b/ios/NewExpensifyTests/Info.plist @@ -19,6 +19,6 @@ CFBundleSignature ???? CFBundleVersion - 1.4.57.3 + 1.4.57.4 diff --git a/ios/NotificationServiceExtension/Info.plist b/ios/NotificationServiceExtension/Info.plist index 953c4eaff93a..ee635424cb87 100644 --- a/ios/NotificationServiceExtension/Info.plist +++ b/ios/NotificationServiceExtension/Info.plist @@ -13,7 +13,7 @@ CFBundleShortVersionString 1.4.57 CFBundleVersion - 1.4.57.3 + 1.4.57.4 NSExtension NSExtensionPointIdentifier diff --git a/package-lock.json b/package-lock.json index ffc43cd8abf9..41432870c33b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "new.expensify", - "version": "1.4.57-3", + "version": "1.4.57-4", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "new.expensify", - "version": "1.4.57-3", + "version": "1.4.57-4", "hasInstallScript": true, "license": "MIT", "dependencies": { diff --git a/package.json b/package.json index 72702a3c3bcc..e497cf40860a 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "new.expensify", - "version": "1.4.57-3", + "version": "1.4.57-4", "author": "Expensify, Inc.", "homepage": "https://new.expensify.com", "description": "New Expensify is the next generation of Expensify: a reimagination of payments based atop a foundation of chat.", diff --git a/src/libs/API/index.ts b/src/libs/API/index.ts index dbbcf790edf0..7d8cb348afbe 100644 --- a/src/libs/API/index.ts +++ b/src/libs/API/index.ts @@ -7,6 +7,7 @@ import * as Pusher from '@libs/Pusher/pusher'; import * as Request from '@libs/Request'; import CONST from '@src/CONST'; import type OnyxRequest from '@src/types/onyx/Request'; +import type {RequestConflictResolver} from '@src/types/onyx/Request'; import type Response from '@src/types/onyx/Response'; import pkg from '../../../package.json'; import type {ApiRequest, ApiRequestCommandParameters, ReadCommand, SideEffectRequestCommand, WriteCommand} from './types'; @@ -51,8 +52,14 @@ type OnyxData = { * @param [onyxData.successData] - Onyx instructions that will be passed to Onyx.update() when the response has jsonCode === 200. * @param [onyxData.failureData] - Onyx instructions that will be passed to Onyx.update() when the response has jsonCode !== 200. * @param [onyxData.finallyData] - Onyx instructions that will be passed to Onyx.update() when the response has jsonCode === 200 or jsonCode !== 200. + * @param [conflictResolver] - callbacks used in special cases to detect and handle conflicting requests in the sequential queue */ -function write(command: TCommand, apiCommandParameters: ApiRequestCommandParameters[TCommand], onyxData: OnyxData = {}) { +function write( + command: TCommand, + apiCommandParameters: ApiRequestCommandParameters[TCommand], + onyxData: OnyxData = {}, + conflictResolver: RequestConflictResolver = {}, +) { Log.info('Called API write', false, {command, ...apiCommandParameters}); const {optimisticData, ...onyxDataWithoutOptimisticData} = onyxData; @@ -83,6 +90,7 @@ function write(command: TCommand, apiCommandParam canCancel: true, }, ...onyxDataWithoutOptimisticData, + ...conflictResolver, }; // Write commands can be saved and retried, so push it to the SequentialQueue diff --git a/src/libs/API/parameters/OpenReportParams.ts b/src/libs/API/parameters/OpenReportParams.ts index 8eaed6bc0fde..330b25bf54e3 100644 --- a/src/libs/API/parameters/OpenReportParams.ts +++ b/src/libs/API/parameters/OpenReportParams.ts @@ -7,7 +7,14 @@ type OpenReportParams = { shouldRetry?: boolean; createdReportActionID?: string; clientLastReadTime?: string; +<<<<<<< HEAD idempotencyKey?: string; +======= + groupChatAdminLogins?: string; + reportName?: string; + chatType?: string; + optimisticAccountIDList?: string; +>>>>>>> 3f3869a (Merge pull request #39024 from Expensify/Rory-GenericConflictingRequests) }; export default OpenReportParams; diff --git a/src/libs/API/parameters/ReconnectAppParams.ts b/src/libs/API/parameters/ReconnectAppParams.ts index d8c1da4f0887..8c5b7d6c0da9 100644 --- a/src/libs/API/parameters/ReconnectAppParams.ts +++ b/src/libs/API/parameters/ReconnectAppParams.ts @@ -2,7 +2,6 @@ type ReconnectAppParams = { mostRecentReportActionLastModified?: string; updateIDFrom?: number; policyIDList: string[]; - idempotencyKey?: string; }; export default ReconnectAppParams; diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index fee8a0b437c7..38b0549b28bc 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -1,6 +1,5 @@ import Onyx from 'react-native-onyx'; import * as ActiveClientManager from '@libs/ActiveClientManager'; -import {WRITE_COMMANDS} from '@libs/API/types'; import * as Request from '@libs/Request'; import * as RequestThrottle from '@libs/RequestThrottle'; import * as PersistedRequests from '@userActions/PersistedRequests'; @@ -46,47 +45,6 @@ function flushOnyxUpdatesQueue() { QueuedOnyxUpdates.flushQueue(); } -/** - * Identifies and removes conflicting requests from the queue - */ -function reconcileRequests(persistedRequests: OnyxRequest[], commands: string[]) { - const requestsByActionID: Record = {}; - - // Group requests by reportActionID - persistedRequests.forEach((request) => { - const {data} = request; - const reportActionID = data?.reportActionID as string; - if (reportActionID) { - if (!requestsByActionID[reportActionID]) { - requestsByActionID[reportActionID] = []; - } - requestsByActionID[reportActionID].push(request); - } - }); - - // Process requests with conflicting actions - Object.values(requestsByActionID).forEach((requests) => { - const conflictingRequests: OnyxRequest[] = []; - commands.forEach((command) => { - const conflictingRequest = requests.find((request) => request.command === command); - if (conflictingRequest) { - conflictingRequests.push(conflictingRequest); - } - }); - - if (conflictingRequests.length > 1) { - // Remove all requests as they cancel each other out - conflictingRequests.forEach((request) => { - // Perform action: Remove request from persisted requests - const index = persistedRequests.findIndex((req) => req === request); - if (index !== -1) { - persistedRequests.splice(index, 1); - } - }); - } - }); -} - /** * Process any persisted requests, when online, one at a time until the queue is empty. * @@ -106,10 +64,6 @@ function process(): Promise { return Promise.resolve(); } - // Remove conflicting requests from the queue to avoid processing them - const commands = [WRITE_COMMANDS.ADD_COMMENT, WRITE_COMMANDS.DELETE_COMMENT]; - reconcileRequests(persistedRequests, commands); - const requestToProcess = persistedRequests[0]; // 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. diff --git a/src/libs/Network/enhanceParameters.ts b/src/libs/Network/enhanceParameters.ts index b8b8993a52b0..5c4590baded3 100644 --- a/src/libs/Network/enhanceParameters.ts +++ b/src/libs/Network/enhanceParameters.ts @@ -36,8 +36,5 @@ export default function enhanceParameters(command: string, parameters: Record = 0) { console.debug(`[OnyxUpdates] App reconnecting with updateIDFrom: ${updateIDFrom}`); getPolicyParamsForOpenOrReconnect().then((policyParams) => { - const params: ReconnectAppParams = { - ...policyParams, - idempotencyKey: `${WRITE_COMMANDS.RECONNECT_APP}`, - }; + const params: ReconnectAppParams = policyParams; // When the app reconnects we do a fast "sync" of the LHN and only return chats that have new messages. We achieve this by sending the most recent reportActionID. // we have locally. And then only update the user about chats with messages that have occurred after that reportActionID. @@ -242,7 +239,9 @@ function reconnectApp(updateIDFrom: OnyxEntry = 0) { params.updateIDFrom = updateIDFrom; } - API.write(WRITE_COMMANDS.RECONNECT_APP, params, getOnyxDataForOpenOrReconnect()); + API.write(WRITE_COMMANDS.RECONNECT_APP, params, getOnyxDataForOpenOrReconnect(), { + getConflictingRequests: (persistedRequests) => persistedRequests.filter((request) => request?.command === WRITE_COMMANDS.RECONNECT_APP), + }); }); } diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts index fcc6450c6072..8adb59824693 100644 --- a/src/libs/actions/PersistedRequests.ts +++ b/src/libs/actions/PersistedRequests.ts @@ -18,17 +18,38 @@ function clear() { } function save(requestToPersist: Request) { - const requests = [...persistedRequests]; - const existingRequestIndex = requests.findIndex((request) => request.data?.idempotencyKey && request.data?.idempotencyKey === requestToPersist.data?.idempotencyKey); - if (existingRequestIndex > -1) { - // Merge the new request into the existing one, keeping its place in the queue - requests.splice(existingRequestIndex, 1, requestToPersist); - } else { - // If not, push the new request to the end of the queue - requests.push(requestToPersist); + const requests = [...persistedRequests, requestToPersist]; + + // identify and handle any existing requests that conflict with the new one + const {getConflictingRequests, handleConflictingRequest, shouldIncludeCurrentRequest} = requestToPersist; + if (getConflictingRequests) { + // Get all the requests, potentially including the one we're adding, which will always be at the end of the array + const potentiallyConflictingRequests = shouldIncludeCurrentRequest ? requests : requests.slice(0, requests.length - 1); + + // Identify conflicting requests according to logic bound to the new request + const conflictingRequests = getConflictingRequests(potentiallyConflictingRequests); + + conflictingRequests.forEach((conflictingRequest) => { + // delete the conflicting request + const index = requests.findIndex((req) => req === conflictingRequest); + if (index !== -1) { + requests.splice(index, 1); + } + + // Allow the new request to perform any additional cleanup for a cancelled request + handleConflictingRequest?.(conflictingRequest); + }); } - persistedRequests = requests; + // Don't try to serialize conflict resolution functions + persistedRequests = requests.map((request) => { + delete request.getConflictingRequests; + delete request.handleConflictingRequest; + delete request.shouldIncludeCurrentRequest; + return request; + }); + + // Save the updated set of requests Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, requests); } diff --git a/src/libs/actions/Report.ts b/src/libs/actions/Report.ts index 93154bfff16b..55bbbda50cf1 100644 --- a/src/libs/actions/Report.ts +++ b/src/libs/actions/Report.ts @@ -612,7 +612,6 @@ function openReport( emailList: participantLoginList ? participantLoginList.join(',') : '', accountIDList: participantAccountIDList ? participantAccountIDList.join(',') : '', parentReportActionID, - idempotencyKey: `${SIDE_EFFECT_REQUEST_COMMANDS.OPEN_REPORT}_${reportID}`, }; if (isFromDeepLink) { @@ -627,7 +626,8 @@ function openReport( } // If we are creating a new report, we need to add the optimistic report data and a report action - if (!isEmptyObject(newReportObject)) { + const isCreatingNewReport = !isEmptyObject(newReportObject); + if (isCreatingNewReport) { // Change the method to set for new reports because it doesn't exist yet, is faster, // and we need the data to be available when we navigate to the chat page optimisticData[0].onyxMethod = Onyx.METHOD.SET; @@ -711,7 +711,6 @@ function openReport( // Add the createdReportActionID parameter to the API call parameters.createdReportActionID = optimisticCreatedAction.reportActionID; - parameters.idempotencyKey = `${parameters.idempotencyKey}_NewReport_${optimisticCreatedAction.reportActionID}`; // If we are creating a thread, ensure the report action has childReportID property added if (newReportObject.parentReportID && parentReportActionID) { @@ -737,7 +736,19 @@ function openReport( }); } else { // eslint-disable-next-line rulesdir/no-multiple-api-calls - API.write(WRITE_COMMANDS.OPEN_REPORT, parameters, {optimisticData, successData, failureData}); + API.write( + WRITE_COMMANDS.OPEN_REPORT, + parameters, + {optimisticData, successData, failureData}, + { + getConflictingRequests: (persistedRequests) => + // requests conflict only if: + // 1. they are OpenReport commands + // 2. they have the same reportID + // 3. they are not creating a report - all calls to OpenReport that create a report will be unique and have a unique createdReportActionID + persistedRequests.filter((request) => request.command === WRITE_COMMANDS.OPEN_REPORT && request.data?.reportID === reportID && !request.data?.createdReportActionID), + }, + ); } } @@ -1137,6 +1148,7 @@ function deleteReportComment(reportID: string, reportAction: ReportAction) { return; } + const isDeletedParentAction = ReportActionsUtils.isThreadParentMessage(reportAction, reportID); const deletedMessage: Message[] = [ { translationKey: '', @@ -1144,7 +1156,7 @@ function deleteReportComment(reportID: string, reportAction: ReportAction) { html: '', text: '', isEdited: true, - isDeletedParentAction: ReportActionsUtils.isThreadParentMessage(reportAction, reportID), + isDeletedParentAction, }, ]; const optimisticReportActions: NullishDeep = { @@ -1238,7 +1250,24 @@ function deleteReportComment(reportID: string, reportAction: ReportAction) { }; CachedPDFPaths.clearByKey(reportActionID); - API.write(WRITE_COMMANDS.DELETE_COMMENT, parameters, {optimisticData, successData, failureData}); + + API.write( + WRITE_COMMANDS.DELETE_COMMENT, + parameters, + {optimisticData, successData, failureData}, + { + getConflictingRequests: (persistedRequests) => { + const conflictingCommands = ( + isDeletedParentAction + ? [WRITE_COMMANDS.UPDATE_COMMENT] + : [WRITE_COMMANDS.ADD_COMMENT, WRITE_COMMANDS.ADD_ATTACHMENT, WRITE_COMMANDS.UPDATE_COMMENT, WRITE_COMMANDS.DELETE_COMMENT] + ) as string[]; + return persistedRequests.filter((request) => conflictingCommands.includes(request.command) && request.data?.reportActionID === reportActionID); + }, + handleConflictingRequest: () => Onyx.update(successData), + shouldIncludeCurrentRequest: !isDeletedParentAction, + }, + ); } /** diff --git a/src/types/onyx/Request.ts b/src/types/onyx/Request.ts index 03d44b1ce94c..74904a0f7194 100644 --- a/src/types/onyx/Request.ts +++ b/src/types/onyx/Request.ts @@ -19,13 +19,40 @@ type RequestData = { successData?: OnyxUpdate[]; failureData?: OnyxUpdate[]; finallyData?: OnyxUpdate[]; - idempotencyKey?: string; - + getConflictingRequests?: (persistedRequests: Request[]) => Request[]; + handleConflictingRequest?: (persistedRequest: Request) => unknown; resolve?: (value: Response) => void; reject?: (value?: unknown) => void; }; -type Request = RequestData & OnyxData; +type RequestConflictResolver = { + /** + * A callback that's provided with all the currently serialized functions in the sequential queue. + * Should return a subset of the requests passed in that conflict with the new request. + * Any conflicting requests will be cancelled and removed from the queue. + * + * @example - In ReconnectApp, you'd only want to have one instance of that command serialized to run on reconnect. The callback for that might look like this: + * (persistedRequests) => persistedRequests.filter((request) => request.command === 'ReconnectApp') + * */ + getConflictingRequests?: (persistedRequests: Request[]) => Request[]; + + /** + * Should the requests provided to getConflictingRequests include the new request? + * This is useful if the new request and an existing request cancel eachother out completely. + * + * @example - In DeleteComment, if you're deleting an optimistic comment, you'd want to cancel the optimistic AddComment call AND the DeleteComment call. + * */ + shouldIncludeCurrentRequest?: boolean; + + /** + * Callback to handle a single conflicting request. + * This is useful if you need to clean up some optimistic data for a request that was queue but never sent. + * In these cases the optimisticData will be applied immediately, but the successData, failureData, and/or finallyData will never be applied unless you do it manually in this callback. + */ + handleConflictingRequest?: (persistedRequest: Request) => unknown; +}; + +type Request = RequestData & OnyxData & RequestConflictResolver; export default Request; -export type {OnyxData, RequestType}; +export type {OnyxData, RequestType, RequestConflictResolver}; diff --git a/tests/unit/PersistedRequests.ts b/tests/unit/PersistedRequests.ts index b1d05c96f12a..f7b740d5e875 100644 --- a/tests/unit/PersistedRequests.ts +++ b/tests/unit/PersistedRequests.ts @@ -3,9 +3,6 @@ import type Request from '../../src/types/onyx/Request'; const request: Request = { command: 'OpenReport', - data: { - idempotencyKey: 'OpenReport_1', - }, successData: [{key: 'reportMetadata_1', onyxMethod: 'merge', value: {}}], failureData: [{key: 'reportMetadata_2', onyxMethod: 'merge', value: {}}], }; @@ -20,44 +17,28 @@ afterEach(() => { }); describe('PersistedRequests', () => { - it('save a new request with an idempotency key which currently exists in the PersistedRequests array', () => { + it('save a request without conflicts', () => { PersistedRequests.save(request); - expect(PersistedRequests.getAll().length).toBe(1); + expect(PersistedRequests.getAll().length).toBe(2); }); - it('save a new request with a new idempotency key', () => { + it('save a new request with conflict resolution', () => { + const handleConflictingRequest = jest.fn(); const newRequest = { - command: 'OpenReport', - data: { - idempotencyKey: 'OpenReport_2', - }, + command: 'ThingA', + getConflictingRequests: (requests: Request[]) => requests, + handleConflictingRequest, }; - PersistedRequests.save(newRequest); - expect(PersistedRequests.getAll().length).toBe(2); - }); - - it('replace a request existing in the PersistedRequests array with a new one', () => { - const newRequest: Request = { - command: 'OpenReport', - data: { - idempotencyKey: 'OpenReport_1', - }, - successData: [{key: 'reportMetadata_3', onyxMethod: 'merge', value: {}}], - failureData: [{key: 'reportMetadata_4', onyxMethod: 'merge', value: {}}], + const secondRequest = { + command: 'ThingB', + getConflictingRequests: (requests: Request[]) => requests, + shouldIncludeCurrentRequest: true, }; - PersistedRequests.save(newRequest); - - const persistedRequests = PersistedRequests.getAll(); - - expect(persistedRequests.length).toBe(1); - - const mergedRequest = persistedRequests[0]; - - expect(mergedRequest.successData?.length).toBe(1); - expect(mergedRequest.failureData?.length).toBe(1); - expect(mergedRequest.successData?.[0]?.key).toBe('reportMetadata_3'); - expect(mergedRequest.failureData?.[0]?.key).toBe('reportMetadata_4'); + PersistedRequests.save(secondRequest); + expect(PersistedRequests.getAll().length).toBe(1); + expect(handleConflictingRequest).toHaveBeenCalledWith(request); + expect(handleConflictingRequest).toHaveBeenCalledTimes(1); }); it('remove a request from the PersistedRequests array', () => {