From 2f29985cdf3c1b580c691a2a5a9e63ad5b1d7a88 Mon Sep 17 00:00:00 2001 From: rory Date: Fri, 22 Sep 2023 16:21:59 +0800 Subject: [PATCH 01/28] Create deepReplaceKeysAndValues function --- src/libs/deepReplaceKeysAndValues.js | 51 +++++++++ tests/unit/deepReplaceKeysAndValuesTest.js | 127 +++++++++++++++++++++ 2 files changed, 178 insertions(+) create mode 100644 src/libs/deepReplaceKeysAndValues.js create mode 100644 tests/unit/deepReplaceKeysAndValuesTest.js diff --git a/src/libs/deepReplaceKeysAndValues.js b/src/libs/deepReplaceKeysAndValues.js new file mode 100644 index 00000000000..445c36b6705 --- /dev/null +++ b/src/libs/deepReplaceKeysAndValues.js @@ -0,0 +1,51 @@ +import _ from 'underscore'; + +/** + * @param {Object} obj the object to transform + * @param {String} oldVal the value to search for + * @param {String} newVal the replacement value + */ +function deepReplaceKeysAndValues(obj, oldVal, newVal) { + if (!obj) { + return obj; + } + + if (typeof obj === 'string') { + return obj.replace(oldVal, newVal); + } + + if (typeof obj !== 'object') { + return obj; + } + + if (_.isArray(obj)) { + return _.map(obj, (item) => deepReplaceKeysAndValues(item, oldVal, newVal)); + } + + const newObj = {}; + _.each(obj, (value, key) => { + // eslint-disable-next-line eqeqeq + let newKey = key == oldVal ? newVal : key; + if (_.isString(newKey)) { + newKey = newKey.replace(oldVal, newVal); + } + + if (_.isObject(value)) { + newObj[newKey] = deepReplaceKeysAndValues(value, oldVal, newVal); + return; + } + // eslint-disable-next-line eqeqeq + if (value == oldVal) { + newObj[newKey] = newVal; + return; + } + if (_.isString(value)) { + newObj[newKey] = value.replace(oldVal, newVal); + return; + } + newObj[newKey] = value; + }); + return newObj; +} + +export default deepReplaceKeysAndValues; diff --git a/tests/unit/deepReplaceKeysAndValuesTest.js b/tests/unit/deepReplaceKeysAndValuesTest.js new file mode 100644 index 00000000000..ecb91802604 --- /dev/null +++ b/tests/unit/deepReplaceKeysAndValuesTest.js @@ -0,0 +1,127 @@ +import deepReplaceKeysAndValues from '../../src/libs/deepReplaceKeysAndValues'; + +describe('deepReplaceKeysAndValues', () => { + test.each([ + [undefined, undefined], + [null, null], + [3, 3], + [true, true], + ['someString', 'someString'], + ['oldVal', 'newVal'], + ['prefix_oldVal', 'prefix_newVal'], + [ + ['a', 'b', 'oldVal'], + ['a', 'b', 'newVal'], + ], + [ + ['a', 'oldVal', 'c'], + ['a', 'newVal', 'c'], + ], + [ + ['a', 'b', 'prefix_oldVal'], + ['a', 'b', 'prefix_newVal'], + ], + [ + { + a: '1', + b: 2, + c: 'oldVal', + }, + { + a: '1', + b: 2, + c: 'newVal', + }, + ], + [ + { + a: '1', + b: 2, + c: 'prefix_oldVal', + }, + { + a: '1', + b: 2, + c: 'prefix_newVal', + }, + ], + [ + { + a: '1', + b: ['a', 'oldVal'], + }, + { + a: '1', + b: ['a', 'newVal'], + }, + ], + [ + { + a: '1', + b: ['a', 'prefix_oldVal'], + }, + { + a: '1', + b: ['a', 'prefix_newVal'], + }, + ], + [ + { + a: { + a: 1, + b: 'oldVal', + }, + b: 2, + }, + { + a: { + a: 1, + b: 'newVal', + }, + b: 2, + }, + ], + [ + { + a: { + a: 1, + b: 'prefix_oldVal', + c: null, + }, + b: 2, + c: null, + }, + { + a: { + a: 1, + b: 'prefix_newVal', + c: null, + }, + b: 2, + c: null, + }, + ], + [ + { + oldVal: 1, + someOtherKey: 2, + }, + { + newVal: 1, + someOtherKey: 2, + }, + ], + [ + { + prefix_oldVal: 1, + someOtherKey: 2, + }, + { + prefix_newVal: 1, + someOtherKey: 2, + }, + ], + ])('deepReplaceKeysAndValues(%s)', (input, expected) => { + expect(deepReplaceKeysAndValues(input, 'oldVal', 'newVal')).toStrictEqual(expected); + }); +}); From 5a4676d2650a9540aa3094086a46c0f4ad88fc7f Mon Sep 17 00:00:00 2001 From: rory Date: Fri, 22 Sep 2023 16:22:57 +0800 Subject: [PATCH 02/28] Fix JSDoc comment --- src/libs/deepReplaceKeysAndValues.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libs/deepReplaceKeysAndValues.js b/src/libs/deepReplaceKeysAndValues.js index 445c36b6705..8f1cc80bb60 100644 --- a/src/libs/deepReplaceKeysAndValues.js +++ b/src/libs/deepReplaceKeysAndValues.js @@ -1,9 +1,10 @@ import _ from 'underscore'; /** - * @param {Object} obj the object to transform + * @param {Object|String|number|boolean} obj the object to transform * @param {String} oldVal the value to search for * @param {String} newVal the replacement value + * @returns {Object|String|number|boolean} */ function deepReplaceKeysAndValues(obj, oldVal, newVal) { if (!obj) { From 80102cae0708723092e8f06bdb9945355324e193 Mon Sep 17 00:00:00 2001 From: rory Date: Mon, 25 Sep 2023 09:49:50 +0800 Subject: [PATCH 03/28] Convert deepReplaceKeysAndValues to TS --- src/libs/deepReplaceKeysAndValues.js | 52 --------------------------- src/libs/deepReplaceKeysAndValues.ts | 53 ++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 52 deletions(-) delete mode 100644 src/libs/deepReplaceKeysAndValues.js create mode 100644 src/libs/deepReplaceKeysAndValues.ts diff --git a/src/libs/deepReplaceKeysAndValues.js b/src/libs/deepReplaceKeysAndValues.js deleted file mode 100644 index 8f1cc80bb60..00000000000 --- a/src/libs/deepReplaceKeysAndValues.js +++ /dev/null @@ -1,52 +0,0 @@ -import _ from 'underscore'; - -/** - * @param {Object|String|number|boolean} obj the object to transform - * @param {String} oldVal the value to search for - * @param {String} newVal the replacement value - * @returns {Object|String|number|boolean} - */ -function deepReplaceKeysAndValues(obj, oldVal, newVal) { - if (!obj) { - return obj; - } - - if (typeof obj === 'string') { - return obj.replace(oldVal, newVal); - } - - if (typeof obj !== 'object') { - return obj; - } - - if (_.isArray(obj)) { - return _.map(obj, (item) => deepReplaceKeysAndValues(item, oldVal, newVal)); - } - - const newObj = {}; - _.each(obj, (value, key) => { - // eslint-disable-next-line eqeqeq - let newKey = key == oldVal ? newVal : key; - if (_.isString(newKey)) { - newKey = newKey.replace(oldVal, newVal); - } - - if (_.isObject(value)) { - newObj[newKey] = deepReplaceKeysAndValues(value, oldVal, newVal); - return; - } - // eslint-disable-next-line eqeqeq - if (value == oldVal) { - newObj[newKey] = newVal; - return; - } - if (_.isString(value)) { - newObj[newKey] = value.replace(oldVal, newVal); - return; - } - newObj[newKey] = value; - }); - return newObj; -} - -export default deepReplaceKeysAndValues; diff --git a/src/libs/deepReplaceKeysAndValues.ts b/src/libs/deepReplaceKeysAndValues.ts new file mode 100644 index 00000000000..9f33629e97a --- /dev/null +++ b/src/libs/deepReplaceKeysAndValues.ts @@ -0,0 +1,53 @@ +type ReplaceableValue = Record | unknown[] | string | number | boolean | undefined | null; + +/** + * @param obj the object to transform + * @param oldVal the value to search for + * @param newVal the replacement value + */ +function deepReplaceKeysAndValues(obj: T, oldVal: string, newVal: string): T { + if (!obj) { + return obj; + } + + if (typeof obj === 'string') { + return obj.replace(oldVal, newVal) as T; + } + + if (typeof obj !== 'object') { + return obj; + } + + if (Array.isArray(obj)) { + return obj.map((item) => deepReplaceKeysAndValues(item as T, oldVal, newVal)) as T; + } + + const newObj: Record = {}; + Object.entries(obj).forEach(([key, val]) => { + let newKey = key === oldVal ? newVal : key; + if (typeof newKey === 'string') { + newKey = newKey.replace(oldVal, newVal); + } + + if (typeof val === 'object') { + newObj[newKey] = deepReplaceKeysAndValues(val as T, oldVal, newVal); + return; + } + + if (val === oldVal) { + newObj[newKey] = newVal; + return; + } + + if (typeof val === 'string') { + newObj[newKey] = val.replace(oldVal, newVal); + return; + } + + newObj[newKey] = val; + }); + + return newObj as T; +} + +export default deepReplaceKeysAndValues; From 0ebf3dc5f00274a305c318f46eebfd39a0e8ec02 Mon Sep 17 00:00:00 2001 From: rory Date: Mon, 25 Sep 2023 22:16:08 +0800 Subject: [PATCH 04/28] Save draft state --- .../Middleware/HandleUnusedOptimisticID.ts | 10 ++++ src/libs/Middleware/SaveResponseInOnyx.js | 2 + src/libs/Request.ts | 3 +- tests/unit/MiddlewareTest.js | 56 +++++++++++++++++++ 4 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 src/libs/Middleware/HandleUnusedOptimisticID.ts create mode 100644 tests/unit/MiddlewareTest.js diff --git a/src/libs/Middleware/HandleUnusedOptimisticID.ts b/src/libs/Middleware/HandleUnusedOptimisticID.ts new file mode 100644 index 00000000000..849588d95b1 --- /dev/null +++ b/src/libs/Middleware/HandleUnusedOptimisticID.ts @@ -0,0 +1,10 @@ +import {Middleware} from '../Request'; + +const handleUnusedOptimisticID: Middleware = (requestResponse, request) => + requestResponse.then((response) => { + if (!response) { + return response; + } + }); + +export default handleUnusedOptimisticID; diff --git a/src/libs/Middleware/SaveResponseInOnyx.js b/src/libs/Middleware/SaveResponseInOnyx.js index 8cb66c0c10d..9ebc616c122 100644 --- a/src/libs/Middleware/SaveResponseInOnyx.js +++ b/src/libs/Middleware/SaveResponseInOnyx.js @@ -15,6 +15,8 @@ const requestsToIgnoreLastUpdateID = ['OpenApp', 'ReconnectApp', 'GetMissingOnyx */ function SaveResponseInOnyx(requestResponse, request) { return requestResponse.then((response) => { + console.log('RORY_DEBUG', request, response); + // Make sure we have response data (i.e. response isn't a promise being passed down to us by a failed retry request and response undefined) if (!response) { return; diff --git a/src/libs/Request.ts b/src/libs/Request.ts index 459deaf89e1..7f037dd21db 100644 --- a/src/libs/Request.ts +++ b/src/libs/Request.ts @@ -3,7 +3,7 @@ import enhanceParameters from './Network/enhanceParameters'; import * as NetworkStore from './Network/NetworkStore'; import Request from '../types/onyx/Request'; -type Middleware = (response: unknown, request: Request, isFromSequentialQueue: boolean) => Promise; +type Middleware = (response: Promise, request: Request, isFromSequentialQueue: boolean) => Promise; let middlewares: Middleware[] = []; @@ -32,3 +32,4 @@ function clearMiddlewares() { } export {clearMiddlewares, processWithMiddleware, use}; +export type {Middleware}; diff --git a/tests/unit/MiddlewareTest.js b/tests/unit/MiddlewareTest.js new file mode 100644 index 00000000000..c591506af01 --- /dev/null +++ b/tests/unit/MiddlewareTest.js @@ -0,0 +1,56 @@ +import Onyx from 'react-native-onyx'; +import * as NetworkStore from '../../src/libs/Network/NetworkStore'; +import * as Request from '../../src/libs/Request'; +import * as SequentialQueue from '../../src/libs/Network/SequentialQueue'; +import * as TestHelper from '../utils/TestHelper'; +import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; +import waitForNetworkPromises from '../utils/waitForNetworkPromises'; +import ONYXKEYS from '../../src/ONYXKEYS'; +import * as MainQueue from '../../src/libs/Network/MainQueue'; +import HttpUtils from '../../src/libs/HttpUtils'; + +Onyx.init({ + keys: ONYXKEYS, +}); + +beforeAll(() => { + global.fetch = TestHelper.getGlobalFetchMock(); +}); + +beforeEach(async () => { + SequentialQueue.pause(); + MainQueue.clear(); + HttpUtils.cancelPendingRequests(); + NetworkStore.checkRequiredData(); + await waitForNetworkPromises(); + jest.clearAllMocks(); +}); + +describe('Middleware', () => { + describe('HandleUnusedOptimisticID', () => { + test('Normal request', async () => { + const actual = jest.requireActual('../../src/libs/Middleware/HandleUnusedOptimisticID'); + const handleUnusedOptimisticID = jest.spyOn(actual, 'default'); + Request.use(handleUnusedOptimisticID); + const requests = [ + { + command: 'OpenReport', + data: {authToken: 'testToken', reportID: '1234'}, + }, + { + command: 'AddComment', + data: {authToken: 'testToken', reportID: '1234', reportActionID: '5678', reportComment: 'foo'}, + }, + ]; + for (const request of requests) { + SequentialQueue.push(request); + } + SequentialQueue.unpause(); + await waitForBatchedUpdates(); + await waitForNetworkPromises(); + + expect(global.fetch).expect(global.fetch).toHaveBeenCalledTimes(2); + expect(handleUnusedOptimisticID).not.toHaveBeenCalled(); + }); + }); +}); From 0d5f4ac2b9a9a52300c32c019681a7c700072eae Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 27 Sep 2023 12:10:36 +0800 Subject: [PATCH 05/28] Create base test --- src/libs/Middleware/HandleUnusedOptimisticID.ts | 7 +------ src/libs/Middleware/SaveResponseInOnyx.js | 2 -- src/libs/Middleware/index.js | 3 ++- tests/unit/MiddlewareTest.js | 12 +++++++----- tests/utils/TestHelper.js | 6 +++++- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/libs/Middleware/HandleUnusedOptimisticID.ts b/src/libs/Middleware/HandleUnusedOptimisticID.ts index 849588d95b1..32e113584c8 100644 --- a/src/libs/Middleware/HandleUnusedOptimisticID.ts +++ b/src/libs/Middleware/HandleUnusedOptimisticID.ts @@ -1,10 +1,5 @@ import {Middleware} from '../Request'; -const handleUnusedOptimisticID: Middleware = (requestResponse, request) => - requestResponse.then((response) => { - if (!response) { - return response; - } - }); +const handleUnusedOptimisticID: Middleware = (requestResponse, request) => requestResponse.then((response) => response); export default handleUnusedOptimisticID; diff --git a/src/libs/Middleware/SaveResponseInOnyx.js b/src/libs/Middleware/SaveResponseInOnyx.js index 9ebc616c122..8cb66c0c10d 100644 --- a/src/libs/Middleware/SaveResponseInOnyx.js +++ b/src/libs/Middleware/SaveResponseInOnyx.js @@ -15,8 +15,6 @@ const requestsToIgnoreLastUpdateID = ['OpenApp', 'ReconnectApp', 'GetMissingOnyx */ function SaveResponseInOnyx(requestResponse, request) { return requestResponse.then((response) => { - console.log('RORY_DEBUG', request, response); - // Make sure we have response data (i.e. response isn't a promise being passed down to us by a failed retry request and response undefined) if (!response) { return; diff --git a/src/libs/Middleware/index.js b/src/libs/Middleware/index.js index 3859fb9c349..3b1790b3cda 100644 --- a/src/libs/Middleware/index.js +++ b/src/libs/Middleware/index.js @@ -1,6 +1,7 @@ +import HandleUnusedOptimisticID from './HandleUnusedOptimisticID'; import Logging from './Logging'; import Reauthentication from './Reauthentication'; import RecheckConnection from './RecheckConnection'; import SaveResponseInOnyx from './SaveResponseInOnyx'; -export {Logging, Reauthentication, RecheckConnection, SaveResponseInOnyx}; +export {HandleUnusedOptimisticID, Logging, Reauthentication, RecheckConnection, SaveResponseInOnyx}; diff --git a/tests/unit/MiddlewareTest.js b/tests/unit/MiddlewareTest.js index c591506af01..a916029f2bd 100644 --- a/tests/unit/MiddlewareTest.js +++ b/tests/unit/MiddlewareTest.js @@ -3,7 +3,6 @@ import * as NetworkStore from '../../src/libs/Network/NetworkStore'; import * as Request from '../../src/libs/Request'; import * as SequentialQueue from '../../src/libs/Network/SequentialQueue'; import * as TestHelper from '../utils/TestHelper'; -import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; import waitForNetworkPromises from '../utils/waitForNetworkPromises'; import ONYXKEYS from '../../src/ONYXKEYS'; import * as MainQueue from '../../src/libs/Network/MainQueue'; @@ -24,6 +23,7 @@ beforeEach(async () => { NetworkStore.checkRequiredData(); await waitForNetworkPromises(); jest.clearAllMocks(); + Request.clearMiddlewares(); }); describe('Middleware', () => { @@ -31,7 +31,7 @@ describe('Middleware', () => { test('Normal request', async () => { const actual = jest.requireActual('../../src/libs/Middleware/HandleUnusedOptimisticID'); const handleUnusedOptimisticID = jest.spyOn(actual, 'default'); - Request.use(handleUnusedOptimisticID); + // Request.use(handleUnusedOptimisticID); const requests = [ { command: 'OpenReport', @@ -46,11 +46,13 @@ describe('Middleware', () => { SequentialQueue.push(request); } SequentialQueue.unpause(); - await waitForBatchedUpdates(); await waitForNetworkPromises(); - expect(global.fetch).expect(global.fetch).toHaveBeenCalledTimes(2); - expect(handleUnusedOptimisticID).not.toHaveBeenCalled(); + expect(global.fetch).toHaveBeenCalledTimes(2); + expect(global.fetch).toHaveBeenLastCalledWith('https://www.expensify.com.dev/api?command=AddComment', expect.anything()); + TestHelper.assertFormDataMatchesObject(global.fetch.mock.calls[1][1].body, {reportID: '1234', reportActionID: '5678', reportComment: 'foo'}); + expect(global.fetch).toHaveBeenNthCalledWith(1, 'https://www.expensify.com.dev/api?command=OpenReport', expect.anything()); + TestHelper.assertFormDataMatchesObject(global.fetch.mock.calls[0][1].body, {reportID: '1234'}); }); }); }); diff --git a/tests/utils/TestHelper.js b/tests/utils/TestHelper.js index ca3955e9eb9..4c658f00489 100644 --- a/tests/utils/TestHelper.js +++ b/tests/utils/TestHelper.js @@ -212,4 +212,8 @@ function buildTestReportComment(created, actorAccountID, actionID = null) { }; } -export {getGlobalFetchMock, signInWithTestUser, signOutTestUser, setPersonalDetails, buildPersonalDetails, buildTestReportComment}; +function assertFormDataMatchesObject(formData, obj) { + expect(_.reduce(Array.from(formData.entries()), (memo, x) => ({...memo, [x[0]]: x[1]}), {})).toEqual(expect.objectContaining(obj)); +} + +export {getGlobalFetchMock, signInWithTestUser, signOutTestUser, setPersonalDetails, buildPersonalDetails, buildTestReportComment, assertFormDataMatchesObject}; From f1441a8d4092ec9bad944cccb453e4775ffa23b4 Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 27 Sep 2023 13:19:58 +0800 Subject: [PATCH 06/28] Implement middleware to handle unused optimistic IDs --- .../Middleware/HandleUnusedOptimisticID.ts | 15 +++++++- src/libs/Request.ts | 6 +-- src/libs/actions/PersistedRequests.ts | 27 +++++++++---- tests/unit/MiddlewareTest.js | 38 ++++++++++++++++++- 4 files changed, 74 insertions(+), 12 deletions(-) diff --git a/src/libs/Middleware/HandleUnusedOptimisticID.ts b/src/libs/Middleware/HandleUnusedOptimisticID.ts index 32e113584c8..085308b7b2a 100644 --- a/src/libs/Middleware/HandleUnusedOptimisticID.ts +++ b/src/libs/Middleware/HandleUnusedOptimisticID.ts @@ -1,5 +1,18 @@ import {Middleware} from '../Request'; +import * as PersistedRequests from '../actions/PersistedRequests'; +import deepReplaceKeysAndValues from '../deepReplaceKeysAndValues'; -const handleUnusedOptimisticID: Middleware = (requestResponse, request) => requestResponse.then((response) => response); +const handleUnusedOptimisticID: Middleware = (requestResponse, request) => + requestResponse.then((response) => { + if ('preexistingReportID' in response) { + const oldReportID = request.data?.reportID; + PersistedRequests.getAll().forEach((persistedRequest, index) => { + // eslint-disable-next-line no-param-reassign + persistedRequest.data = deepReplaceKeysAndValues(persistedRequest.data, oldReportID as string, response.preexistingReportID as string); + PersistedRequests.update(index, persistedRequest); + }); + } + return response; + }); export default handleUnusedOptimisticID; diff --git a/src/libs/Request.ts b/src/libs/Request.ts index 7f037dd21db..1d03aca2941 100644 --- a/src/libs/Request.ts +++ b/src/libs/Request.ts @@ -3,11 +3,11 @@ import enhanceParameters from './Network/enhanceParameters'; import * as NetworkStore from './Network/NetworkStore'; import Request from '../types/onyx/Request'; -type Middleware = (response: Promise, request: Request, isFromSequentialQueue: boolean) => Promise; +type Middleware = (response: Promise>, request: Request, isFromSequentialQueue: boolean) => Promise>; let middlewares: Middleware[] = []; -function makeXHR(request: Request): Promise { +function makeXHR(request: Request): Promise> { const finalParameters = enhanceParameters(request.command, request?.data ?? {}); return NetworkStore.hasReadRequiredDataFromStorage().then(() => { // If we're using the Supportal token and this is not a Supportal request @@ -19,7 +19,7 @@ function makeXHR(request: Request): Promise { }); } -function processWithMiddleware(request: Request, isFromSequentialQueue = false): Promise { +function processWithMiddleware(request: Request, isFromSequentialQueue = false): Promise> { return middlewares.reduce((last, middleware) => middleware(last, request, isFromSequentialQueue), makeXHR(request)); } diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts index 040c7d3d87a..319bd9efda7 100644 --- a/src/libs/actions/PersistedRequests.ts +++ b/src/libs/actions/PersistedRequests.ts @@ -31,18 +31,31 @@ 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) { - requests.splice(index, 1); + const index = persistedRequests.findIndex((persistedRequest) => isEqual(persistedRequest, requestToRemove)); + if (index === -1) { + return; } + persistedRequests.splice(index, 1); + Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests); +} - persistedRequests = requests; - Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, requests); +function update(oldRequest: number | Request, newRequest: Request) { + let index = -1; + if (typeof oldRequest === 'number') { + index = oldRequest; + persistedRequests.splice(oldRequest, 1, newRequest); + } else { + index = persistedRequests.findIndex((persistedRequest) => isEqual(persistedRequest, oldRequest)); + if (index === -1) { + return; + } + } + persistedRequests.splice(index, 1, newRequest); + Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests); } function getAll(): Request[] { return persistedRequests; } -export {clear, save, getAll, remove}; +export {clear, save, getAll, remove, update}; diff --git a/tests/unit/MiddlewareTest.js b/tests/unit/MiddlewareTest.js index a916029f2bd..beb0276f028 100644 --- a/tests/unit/MiddlewareTest.js +++ b/tests/unit/MiddlewareTest.js @@ -31,7 +31,7 @@ describe('Middleware', () => { test('Normal request', async () => { const actual = jest.requireActual('../../src/libs/Middleware/HandleUnusedOptimisticID'); const handleUnusedOptimisticID = jest.spyOn(actual, 'default'); - // Request.use(handleUnusedOptimisticID); + Request.use(handleUnusedOptimisticID); const requests = [ { command: 'OpenReport', @@ -54,5 +54,41 @@ describe('Middleware', () => { expect(global.fetch).toHaveBeenNthCalledWith(1, 'https://www.expensify.com.dev/api?command=OpenReport', expect.anything()); TestHelper.assertFormDataMatchesObject(global.fetch.mock.calls[0][1].body, {reportID: '1234'}); }); + + test('Request with preexistingReportID', async () => { + const actual = jest.requireActual('../../src/libs/Middleware/HandleUnusedOptimisticID'); + const handleUnusedOptimisticID = jest.spyOn(actual, 'default'); + Request.use(handleUnusedOptimisticID); + const requests = [ + { + command: 'OpenReport', + data: {authToken: 'testToken', reportID: '1234'}, + }, + { + command: 'AddComment', + data: {authToken: 'testToken', reportID: '1234', reportActionID: '5678', reportComment: 'foo'}, + }, + ]; + for (const request of requests) { + SequentialQueue.push(request); + } + + global.fetch.mockImplementationOnce(async () => ({ + ok: true, + json: async () => ({ + jsonCode: 200, + preexistingReportID: '5555', + }), + })); + + SequentialQueue.unpause(); + await waitForNetworkPromises(); + + expect(global.fetch).toHaveBeenCalledTimes(2); + expect(global.fetch).toHaveBeenLastCalledWith('https://www.expensify.com.dev/api?command=AddComment', expect.anything()); + TestHelper.assertFormDataMatchesObject(global.fetch.mock.calls[1][1].body, {reportID: '5555', reportActionID: '5678', reportComment: 'foo'}); + expect(global.fetch).toHaveBeenNthCalledWith(1, 'https://www.expensify.com.dev/api?command=OpenReport', expect.anything()); + TestHelper.assertFormDataMatchesObject(global.fetch.mock.calls[0][1].body, {reportID: '1234'}); + }); }); }); From 09ba949a7788b38cbc979944c4ac736a5444cb8a Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 27 Sep 2023 13:27:43 +0800 Subject: [PATCH 07/28] Fix typescript types --- src/libs/Middleware/HandleUnusedOptimisticID.ts | 2 +- src/libs/Request.ts | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libs/Middleware/HandleUnusedOptimisticID.ts b/src/libs/Middleware/HandleUnusedOptimisticID.ts index 085308b7b2a..1c25a2dc9e3 100644 --- a/src/libs/Middleware/HandleUnusedOptimisticID.ts +++ b/src/libs/Middleware/HandleUnusedOptimisticID.ts @@ -4,7 +4,7 @@ import deepReplaceKeysAndValues from '../deepReplaceKeysAndValues'; const handleUnusedOptimisticID: Middleware = (requestResponse, request) => requestResponse.then((response) => { - if ('preexistingReportID' in response) { + if (response && typeof response === 'object' && 'preexistingReportID' in response) { const oldReportID = request.data?.reportID; PersistedRequests.getAll().forEach((persistedRequest, index) => { // eslint-disable-next-line no-param-reassign diff --git a/src/libs/Request.ts b/src/libs/Request.ts index 1d03aca2941..7f037dd21db 100644 --- a/src/libs/Request.ts +++ b/src/libs/Request.ts @@ -3,11 +3,11 @@ import enhanceParameters from './Network/enhanceParameters'; import * as NetworkStore from './Network/NetworkStore'; import Request from '../types/onyx/Request'; -type Middleware = (response: Promise>, request: Request, isFromSequentialQueue: boolean) => Promise>; +type Middleware = (response: Promise, request: Request, isFromSequentialQueue: boolean) => Promise; let middlewares: Middleware[] = []; -function makeXHR(request: Request): Promise> { +function makeXHR(request: Request): Promise { const finalParameters = enhanceParameters(request.command, request?.data ?? {}); return NetworkStore.hasReadRequiredDataFromStorage().then(() => { // If we're using the Supportal token and this is not a Supportal request @@ -19,7 +19,7 @@ function makeXHR(request: Request): Promise> { }); } -function processWithMiddleware(request: Request, isFromSequentialQueue = false): Promise> { +function processWithMiddleware(request: Request, isFromSequentialQueue = false): Promise { return middlewares.reduce((last, middleware) => middleware(last, request, isFromSequentialQueue), makeXHR(request)); } From cb5888b0724ce486ba5de8b0c4148a8637ce296d Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 28 Sep 2023 09:49:44 +0800 Subject: [PATCH 08/28] Don't suppress lint rule --- src/libs/Middleware/HandleUnusedOptimisticID.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libs/Middleware/HandleUnusedOptimisticID.ts b/src/libs/Middleware/HandleUnusedOptimisticID.ts index 1c25a2dc9e3..15e455c6698 100644 --- a/src/libs/Middleware/HandleUnusedOptimisticID.ts +++ b/src/libs/Middleware/HandleUnusedOptimisticID.ts @@ -7,9 +7,9 @@ const handleUnusedOptimisticID: Middleware = (requestResponse, request) => if (response && typeof response === 'object' && 'preexistingReportID' in response) { const oldReportID = request.data?.reportID; PersistedRequests.getAll().forEach((persistedRequest, index) => { - // eslint-disable-next-line no-param-reassign - persistedRequest.data = deepReplaceKeysAndValues(persistedRequest.data, oldReportID as string, response.preexistingReportID as string); - PersistedRequests.update(index, persistedRequest); + const oldRequest = persistedRequest; + oldRequest.data = deepReplaceKeysAndValues(oldRequest.data, oldReportID as string, response.preexistingReportID as string); + PersistedRequests.update(index, oldRequest); }); } return response; From 5f3110d51d73762521faefe4bb7956df1d68f795 Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 28 Sep 2023 09:51:20 +0800 Subject: [PATCH 09/28] Remove unnecessary type check --- src/libs/deepReplaceKeysAndValues.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/libs/deepReplaceKeysAndValues.ts b/src/libs/deepReplaceKeysAndValues.ts index 9f33629e97a..3703d5957bf 100644 --- a/src/libs/deepReplaceKeysAndValues.ts +++ b/src/libs/deepReplaceKeysAndValues.ts @@ -24,10 +24,7 @@ function deepReplaceKeysAndValues(obj: T, oldVal: st const newObj: Record = {}; Object.entries(obj).forEach(([key, val]) => { - let newKey = key === oldVal ? newVal : key; - if (typeof newKey === 'string') { - newKey = newKey.replace(oldVal, newVal); - } + const newKey = (key === oldVal ? newVal : key).replace(oldVal, newVal); if (typeof val === 'object') { newObj[newKey] = deepReplaceKeysAndValues(val as T, oldVal, newVal); From 7f839363d4a4f67a959fc6cb383b40b894c7f4f7 Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 28 Sep 2023 09:52:39 +0800 Subject: [PATCH 10/28] Simplify key generation --- src/libs/deepReplaceKeysAndValues.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/deepReplaceKeysAndValues.ts b/src/libs/deepReplaceKeysAndValues.ts index 3703d5957bf..5156b3e06a2 100644 --- a/src/libs/deepReplaceKeysAndValues.ts +++ b/src/libs/deepReplaceKeysAndValues.ts @@ -24,7 +24,7 @@ function deepReplaceKeysAndValues(obj: T, oldVal: st const newObj: Record = {}; Object.entries(obj).forEach(([key, val]) => { - const newKey = (key === oldVal ? newVal : key).replace(oldVal, newVal); + const newKey = key.replace(oldVal, newVal); if (typeof val === 'object') { newObj[newKey] = deepReplaceKeysAndValues(val as T, oldVal, newVal); From bf153a5d698d302d582156b2f09f1aefeb347deb Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 28 Sep 2023 09:58:09 +0800 Subject: [PATCH 11/28] Turn on the middleware --- src/libs/API.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/libs/API.js b/src/libs/API.js index 491503f0738..2ad1f32347d 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -21,6 +21,9 @@ Request.use(Middleware.RecheckConnection); // Reauthentication - Handles jsonCode 407 which indicates an expired authToken. We need to reauthenticate and get a new authToken with our stored credentials. Request.use(Middleware.Reauthentication); +// If an optimistic ID is not used by the server, this will update the remaining serialized requests using that optimistic ID to use the correct ID instead. +Request.use(Middleware.HandleUnusedOptimisticID); + // SaveResponseInOnyx - Merges either the successData or failureData into Onyx depending on if the call was successful or not. This needs to be the LAST middleware we use, don't add any // middlewares after this, because the SequentialQueue depends on the result of this middleware to pause the queue (if needed) to bring the app to an up-to-date state. Request.use(Middleware.SaveResponseInOnyx); From 15756baa8466632c2f52bfdce827e170c2728e91 Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 28 Sep 2023 14:27:16 +0800 Subject: [PATCH 12/28] Get middleware working correctly --- .../Middleware/HandleUnusedOptimisticID.ts | 33 +++++++++++++++---- src/types/onyx/Report.ts | 3 +- 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/src/libs/Middleware/HandleUnusedOptimisticID.ts b/src/libs/Middleware/HandleUnusedOptimisticID.ts index 15e455c6698..20f10cc9073 100644 --- a/src/libs/Middleware/HandleUnusedOptimisticID.ts +++ b/src/libs/Middleware/HandleUnusedOptimisticID.ts @@ -1,17 +1,36 @@ +import ONYXKEYS from '../../ONYXKEYS'; +import Report from '../../types/onyx/Report'; import {Middleware} from '../Request'; import * as PersistedRequests from '../actions/PersistedRequests'; import deepReplaceKeysAndValues from '../deepReplaceKeysAndValues'; const handleUnusedOptimisticID: Middleware = (requestResponse, request) => requestResponse.then((response) => { - if (response && typeof response === 'object' && 'preexistingReportID' in response) { + const responseOnyxData = response?.onyxData; + responseOnyxData.forEach((onyxData: {key: string; value: Record}) => { + const key = onyxData.key; + if (!key.startsWith(ONYXKEYS.COLLECTION.REPORT)) { + return; + } + + if (!onyxData.value) { + return; + } + + const report: Report = onyxData.value as Report; + if (!report.preexistingReportID) { + return; + } + const preexistingReportID = report.preexistingReportID; const oldReportID = request.data?.reportID; - PersistedRequests.getAll().forEach((persistedRequest, index) => { - const oldRequest = persistedRequest; - oldRequest.data = deepReplaceKeysAndValues(oldRequest.data, oldReportID as string, response.preexistingReportID as string); - PersistedRequests.update(index, oldRequest); - }); - } + PersistedRequests.getAll() + .slice(1) + .forEach((persistedRequest, index) => { + const oldRequest = persistedRequest; + oldRequest.data = deepReplaceKeysAndValues(oldRequest.data, oldReportID as string, preexistingReportID); + PersistedRequests.update(index, oldRequest); + }); + }); return response; }); diff --git a/src/types/onyx/Report.ts b/src/types/onyx/Report.ts index 88caa683305..7c4eef83d01 100644 --- a/src/types/onyx/Report.ts +++ b/src/types/onyx/Report.ts @@ -55,7 +55,7 @@ type Report = { reportName?: string; /** ID of the report */ - reportID?: string; + reportID: string; /** The state that the report is currently in */ stateNum?: ValueOf; @@ -83,6 +83,7 @@ type Report = { participantAccountIDs?: number[]; total?: number; currency?: string; + preexistingReportID?: string; }; export default Report; From 80af4cc828c4c44a6c53c576e9eb44c4e4c63b52 Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 28 Sep 2023 14:40:31 +0800 Subject: [PATCH 13/28] Fix TypeScript types --- src/libs/Middleware/HandleUnusedOptimisticID.ts | 8 ++++---- src/libs/Request.ts | 7 ++++--- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/libs/Middleware/HandleUnusedOptimisticID.ts b/src/libs/Middleware/HandleUnusedOptimisticID.ts index 20f10cc9073..3160ca9e356 100644 --- a/src/libs/Middleware/HandleUnusedOptimisticID.ts +++ b/src/libs/Middleware/HandleUnusedOptimisticID.ts @@ -6,8 +6,8 @@ import deepReplaceKeysAndValues from '../deepReplaceKeysAndValues'; const handleUnusedOptimisticID: Middleware = (requestResponse, request) => requestResponse.then((response) => { - const responseOnyxData = response?.onyxData; - responseOnyxData.forEach((onyxData: {key: string; value: Record}) => { + const responseOnyxData = response?.onyxData ?? []; + responseOnyxData.forEach((onyxData) => { const key = onyxData.key; if (!key.startsWith(ONYXKEYS.COLLECTION.REPORT)) { return; @@ -18,10 +18,10 @@ const handleUnusedOptimisticID: Middleware = (requestResponse, request) => } const report: Report = onyxData.value as Report; - if (!report.preexistingReportID) { + const preexistingReportID = report.preexistingReportID; + if (!preexistingReportID) { return; } - const preexistingReportID = report.preexistingReportID; const oldReportID = request.data?.reportID; PersistedRequests.getAll() .slice(1) diff --git a/src/libs/Request.ts b/src/libs/Request.ts index 7f037dd21db..f8fb04595bb 100644 --- a/src/libs/Request.ts +++ b/src/libs/Request.ts @@ -2,12 +2,13 @@ import HttpUtils from './HttpUtils'; import enhanceParameters from './Network/enhanceParameters'; import * as NetworkStore from './Network/NetworkStore'; import Request from '../types/onyx/Request'; +import Response from '../types/onyx/Response'; -type Middleware = (response: Promise, request: Request, isFromSequentialQueue: boolean) => Promise; +type Middleware = (response: Promise, request: Request, isFromSequentialQueue: boolean) => Promise; let middlewares: Middleware[] = []; -function makeXHR(request: Request): Promise { +function makeXHR(request: Request): Promise { const finalParameters = enhanceParameters(request.command, request?.data ?? {}); return NetworkStore.hasReadRequiredDataFromStorage().then(() => { // If we're using the Supportal token and this is not a Supportal request @@ -19,7 +20,7 @@ function makeXHR(request: Request): Promise { }); } -function processWithMiddleware(request: Request, isFromSequentialQueue = false): Promise { +function processWithMiddleware(request: Request, isFromSequentialQueue = false): Promise { return middlewares.reduce((last, middleware) => middleware(last, request, isFromSequentialQueue), makeXHR(request)); } From f0fbf0d917641e62ff12fab97e60c3f82d26a08f Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 28 Sep 2023 14:58:45 +0800 Subject: [PATCH 14/28] Update test and fix off-by-one error --- src/libs/Middleware/HandleUnusedOptimisticID.ts | 2 +- tests/unit/MiddlewareTest.js | 10 +++++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/libs/Middleware/HandleUnusedOptimisticID.ts b/src/libs/Middleware/HandleUnusedOptimisticID.ts index 3160ca9e356..da487954547 100644 --- a/src/libs/Middleware/HandleUnusedOptimisticID.ts +++ b/src/libs/Middleware/HandleUnusedOptimisticID.ts @@ -28,7 +28,7 @@ const handleUnusedOptimisticID: Middleware = (requestResponse, request) => .forEach((persistedRequest, index) => { const oldRequest = persistedRequest; oldRequest.data = deepReplaceKeysAndValues(oldRequest.data, oldReportID as string, preexistingReportID); - PersistedRequests.update(index, oldRequest); + PersistedRequests.update(index + 1, oldRequest); }); }); return response; diff --git a/tests/unit/MiddlewareTest.js b/tests/unit/MiddlewareTest.js index beb0276f028..db020ea924e 100644 --- a/tests/unit/MiddlewareTest.js +++ b/tests/unit/MiddlewareTest.js @@ -77,7 +77,15 @@ describe('Middleware', () => { ok: true, json: async () => ({ jsonCode: 200, - preexistingReportID: '5555', + onyxData: [ + { + onyxMethod: Onyx.METHOD.MERGE, + key: `${ONYXKEYS.COLLECTION.REPORT}1234`, + value: { + preexistingReportID: '5555', + }, + }, + ], }), })); From 224a134a273cd2ada32baa04e64cba78e6e5ed29 Mon Sep 17 00:00:00 2001 From: rory Date: Fri, 29 Sep 2023 12:33:24 +0800 Subject: [PATCH 15/28] Replace reportID in navigation stack --- src/libs/Navigation/Navigation.js | 69 +++++++++++++++++++- src/libs/Navigation/getTopmostCentralPane.js | 18 +++++ src/libs/Navigation/getTopmostReportId.js | 7 +- src/libs/actions/Report.js | 7 +- 4 files changed, 88 insertions(+), 13 deletions(-) create mode 100644 src/libs/Navigation/getTopmostCentralPane.js diff --git a/src/libs/Navigation/Navigation.js b/src/libs/Navigation/Navigation.js index dc4f35a59cb..2d40d8af0d7 100644 --- a/src/libs/Navigation/Navigation.js +++ b/src/libs/Navigation/Navigation.js @@ -1,5 +1,4 @@ import _ from 'lodash'; -import lodashGet from 'lodash/get'; import {CommonActions, getPathFromState, StackActions} from '@react-navigation/native'; import {getActionFromState} from '@react-navigation/core'; import Log from '../Log'; @@ -13,6 +12,7 @@ import originalGetTopmostReportId from './getTopmostReportId'; import getStateFromPath from './getStateFromPath'; import SCREENS from '../../SCREENS'; import CONST from '../../CONST'; +import getTopmostCentralPane from './getTopmostCentralPane'; let resolveNavigationIsReadyPromise; const navigationIsReadyPromise = new Promise((resolve) => { @@ -148,6 +148,70 @@ function setParams(params, routeKey) { }); } +/** + * Replace a given reportID in the navigation stack. + * + * @param {String} oldReportID + * @param {String} newReportID + */ +function replaceReportIDInNavigationStack(oldReportID, newReportID) { + const state = navigationRef.getState(); + + const topmostCentralPane = getTopmostCentralPane(state); + if (!topmostCentralPane) { + return; + } + + const directReportIDParam = _.get(topmostCentralPane, 'params.params.reportID'); + if (directReportIDParam === oldReportID) { + setParams( + { + reportID: newReportID, + }, + _.get(topmostCentralPane, 'key', ''), + ); + } + + const topmostReportID = getTopmostReportId(); + if (topmostReportID === oldReportID) { + navigationRef.current.dispatch({ + ...StackActions.replace('Report', { + reportID: newReportID, + }), + }); + } + + if (!topmostCentralPane.state) { + return; + } + + const centralPaneRoutes = _.get(topmostCentralPane, 'state.routes', []); + _.each(centralPaneRoutes, (route) => { + const reportIDParam = _.get(route, 'params.reportID'); + if (!reportIDParam) { + return; + } + + if (reportIDParam !== oldReportID) { + return; + } + + route.setParams({reportID: newReportID}); + + if (route.name !== 'Report') { + return; + } + + navigationRef.current.dispatch({ + ...StackActions.replace('Report', { + reportID: newReportID, + }), + source: route.key, + target: navigationRef.current.getState().key, + }); + }); +} + /** * Dismisses the last modal stack if there is any * @@ -186,7 +250,7 @@ function dismissModal(targetReportID) { */ function getActiveRoute() { const currentRoute = navigationRef.current && navigationRef.current.getCurrentRoute(); - const currentRouteHasName = lodashGet(currentRoute, 'name', false); + const currentRouteHasName = _.get(currentRoute, 'name', false); if (!currentRouteHasName) { return ''; } @@ -268,6 +332,7 @@ export default { setIsNavigationReady, getTopmostReportId, getRouteNameFromStateEvent, + replaceReportIDInNavigationStack, }; export {navigationRef}; diff --git a/src/libs/Navigation/getTopmostCentralPane.js b/src/libs/Navigation/getTopmostCentralPane.js new file mode 100644 index 00000000000..edc55ff8955 --- /dev/null +++ b/src/libs/Navigation/getTopmostCentralPane.js @@ -0,0 +1,18 @@ +import lodashFindLast from 'lodash/findLast'; + +// This function is in a separate file than Navigation.js to avoid cyclic dependency. + +/** + * Find the last visited report screen in the navigation state and get the id of it. + * + * @param {Object} state - The react-navigation state + * @returns {String | undefined} + */ +function getTopmostCentralPane(state) { + if (!state) { + return; + } + return lodashFindLast(state.routes, (route) => route.name === 'CentralPaneNavigator'); +} + +export default getTopmostCentralPane; diff --git a/src/libs/Navigation/getTopmostReportId.js b/src/libs/Navigation/getTopmostReportId.js index 8ca9c39baf6..b697b3ad9a3 100644 --- a/src/libs/Navigation/getTopmostReportId.js +++ b/src/libs/Navigation/getTopmostReportId.js @@ -1,5 +1,6 @@ import lodashFindLast from 'lodash/findLast'; import lodashGet from 'lodash/get'; +import getTopmostCentralPane from './getTopmostCentralPane'; // This function is in a separate file than Navigation.js to avoid cyclic dependency. @@ -10,11 +11,7 @@ import lodashGet from 'lodash/get'; * @returns {String | undefined} - It's possible that there is no report screen */ function getTopmostReportId(state) { - if (!state) { - return; - } - const topmostCentralPane = lodashFindLast(state.routes, (route) => route.name === 'CentralPaneNavigator'); - + const topmostCentralPane = getTopmostCentralPane(state); if (!topmostCentralPane) { return; } diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index 296abc6a9cf..872f83bffe1 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -942,12 +942,7 @@ function handleReportChanged(report) { // We should clear out the optimistically created report and re-route the user to the preexisting report. if (report && report.reportID && report.preexistingReportID) { Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`, null); - - // Only re-route them if they are still looking at the optimistically created report - if (Navigation.getActiveRoute().includes(`/r/${report.reportID}`)) { - // Pass 'FORCED_UP' type to replace new report on second login with proper one in the Navigation - Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report.preexistingReportID), CONST.NAVIGATION.TYPE.FORCED_UP); - } + Navigation.replaceReportIDInNavigationStack(report.reportID, report.preexistingReportID); return; } From 1d05d9b5320a4209ad52a536c1cfee2c9b87e760 Mon Sep 17 00:00:00 2001 From: rory Date: Fri, 29 Sep 2023 13:37:35 +0800 Subject: [PATCH 16/28] Start creating UI tests --- tests/ui/Navigation.js | 210 ++++++++++++++++++++++++++++++++++++++ tests/utils/TestHelper.js | 7 +- 2 files changed, 214 insertions(+), 3 deletions(-) create mode 100644 tests/ui/Navigation.js diff --git a/tests/ui/Navigation.js b/tests/ui/Navigation.js new file mode 100644 index 00000000000..546f9cd87a2 --- /dev/null +++ b/tests/ui/Navigation.js @@ -0,0 +1,210 @@ +import React from 'react'; +import Onyx from 'react-native-onyx'; +import {render, screen, fireEvent} from '@testing-library/react-native'; +import {Linking} from 'react-native'; +import App from '../../src/App'; +import CONST from '../../src/CONST'; +import ROUTES from '../../src/ROUTES'; +import ONYXKEYS from '../../src/ONYXKEYS'; +import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; +import waitForBatchedUpdatesWithAct from '../utils/waitForBatchedUpdatesWithAct'; +import * as TestHelper from '../utils/TestHelper'; +import appSetup from '../../src/setup'; +import PusherConnectionManager from '../../src/libs/PusherConnectionManager'; +import * as Pusher from '../../src/libs/Pusher/pusher'; +import CONFIG from '../../src/CONFIG'; +import * as Localize from '../../src/libs/Localize'; +import * as User from '../../src/libs/actions/User'; +import * as AppActions from '../../src/libs/actions/App'; +import DateUtils from '../../src/libs/DateUtils'; +import * as NumberUtils from '../../src/libs/NumberUtils'; +import Navigation from '../../src/libs/Navigation/Navigation'; + +// We need a large timeout here as we are lazy loading React Navigation screens and this test is running against the entire mounted App +jest.setTimeout(30000); + +jest.mock('../../src/libs/Notification/LocalNotification'); +jest.mock('../../src/components/Icon/Expensicons'); + +beforeAll(() => { + // In this test, we are generically mocking the responses of all API requests by mocking fetch() and having it + // return 200. In other tests, we might mock HttpUtils.xhr() with a more specific mock data response (which means + // fetch() never gets called so it does not need mocking) or we might have fetch throw an error to test error handling + // behavior. But here we just want to treat all API requests as a generic "success" and in the cases where we need to + // simulate data arriving we will just set it into Onyx directly with Onyx.merge() or Onyx.set() etc. + global.fetch = TestHelper.getGlobalFetchMock(); + + Linking.setInitialURL('https://new.expensify.com/'); + appSetup(); + + // Connect to Pusher + PusherConnectionManager.init(); + Pusher.init({ + appKey: CONFIG.PUSHER.APP_KEY, + cluster: CONFIG.PUSHER.CLUSTER, + authEndpoint: `${CONFIG.EXPENSIFY.DEFAULT_API_ROOT}api?command=AuthenticatePusher`, + }); +}); + +const ALICE = { + ACCOUNT_ID: 1, + EMAIL: 'alice@expensifail.com', + NAME: 'Alice', +}; +const BOB = { + ACCOUNT_ID: 2, + EMAIL: 'bob@expensifail.com', + NAME: 'Bob', +}; + +async function signInToApp() { + // Render the App and sign in as a test user. + render(); + await waitForBatchedUpdatesWithAct(); + const hintText = Localize.translateLocal('loginForm.loginForm'); + const loginForm = screen.queryAllByLabelText(hintText); + expect(loginForm).toHaveLength(1); + await TestHelper.signInWithTestUser(ALICE.ACCOUNT_ID, ALICE.EMAIL, undefined, undefined, 'A'); + User.subscribeToUserEvents(); + await waitForBatchedUpdates(); + + // We manually setting the sidebar as loaded since the onLayout event does not fire in tests + AppActions.setSidebarLoaded(true); + await waitForBatchedUpdates(); +} + +/** + * @param {Number} index + * @return {Promise} + */ +function navigateToSidebarOption(index) { + const hintText = Localize.translateLocal('accessibilityHints.navigatesToChat'); + const optionRows = screen.getAllByAccessibilityHint(hintText); + fireEvent(optionRows[index], 'press'); + return waitForBatchedUpdates(); +} + +describe('Navigation', () => { + beforeEach(async () => { + await Onyx.merge(ONYXKEYS.PERSONAL_DETAILS_LIST, { + [ALICE.ACCOUNT_ID]: TestHelper.buildPersonalDetails(ALICE.EMAIL, ALICE.ACCOUNT_ID, ALICE.NAME), + [BOB.ACCOUNT_ID]: TestHelper.buildPersonalDetails(BOB.EMAIL, BOB.ACCOUNT_ID, BOB.NAME), + }); + await waitForBatchedUpdates(); + }); + + afterEach(async () => { + jest.clearAllMocks(); + await Onyx.clear(); + }); + + it('Replaces report screen in stack', async () => { + await signInToApp(); + + // Verify the sidebar links are rendered + const sidebarLinksHintText = Localize.translateLocal('sidebarScreen.listOfChats'); + const sidebarLinks = screen.queryAllByLabelText(sidebarLinksHintText); + expect(sidebarLinks).toHaveLength(1); + + // Verify there is nothing in the sidebar to begin with + const optionRowsHintText = Localize.translateLocal('accessibilityHints.navigatesToChat'); + let optionRows = screen.queryAllByAccessibilityHint(optionRowsHintText); + expect(optionRows).toHaveLength(0); + + // Add a report in Onyx + let now = DateUtils.getDBTime(); + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${1}`, { + reportID: 1, + reportName: CONST.REPORT.DEFAULT_REPORT_NAME, + lastReadTime: now, + lastVisibleActionCreated: now, + + // Note: this is intentionally different from the actual last message to prevent the sidebar from matching when we want to make sure that the central report pane is visible + lastMessageText: `Hi from ${ALICE.NAME}`, + participantAccountIDs: [ALICE.ACCOUNT_ID, BOB.ACCOUNT_ID], + type: CONST.REPORT.TYPE.CHAT, + }); + let createdReportActionID = NumberUtils.rand64(); + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${1}`, { + [createdReportActionID]: { + actionName: CONST.REPORT.ACTIONS.TYPE.CREATED, + automatic: false, + created: now, + reportActionID: createdReportActionID, + message: [ + { + style: 'strong', + text: '__FAKE__', + type: 'TEXT', + }, + { + style: 'normal', + text: 'created this report', + type: 'TEXT', + }, + ], + }, + 1: TestHelper.buildTestReportComment(now, ALICE.ACCOUNT_ID, '1', ALICE.NAME), + }); + await waitForBatchedUpdates(); + + // Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute('1')); + // await new Promise((resolve) => setTimeout(resolve, CONST.ANIMATED_TRANSITION)); + + // Verify there is now one option in the sidebar + optionRows = screen.queryAllByAccessibilityHint(optionRowsHintText); + expect(optionRows).toHaveLength(1); + screen.getByText(`Hi from ${ALICE.NAME}`); + + await navigateToSidebarOption(0); + await navigateToSidebarOption(0); + const welcomeMessageHintText = Localize.translateLocal('accessibilityHints.chatWelcomeMessage'); + const createdAction = screen.queryByLabelText(welcomeMessageHintText); + expect(createdAction).toBeTruthy(); + const reportCommentsHintText = Localize.translateLocal('accessibilityHints.chatMessage'); + const reportComments = screen.queryAllByLabelText(reportCommentsHintText); + expect(reportComments).toHaveLength(1); + screen.getByText(`Comment 1 from ${ALICE.NAME}`); + + // Add another report to Onyx + now = DateUtils.getDBTime(); + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${2}`, { + reportID: 2, + reportName: CONST.REPORT.DEFAULT_REPORT_NAME, + lastReadTime: now, + lastVisibleActionCreated: now, + + // Note: this is intentionally different from the actual last message to prevent the sidebar from matching when we want to make sure that the central report pane is visible + lastMessageText: `Hi from ${BOB.NAME}`, + participantAccountIDs: [ALICE.ACCOUNT_ID, BOB.ACCOUNT_ID], + type: CONST.REPORT.TYPE.CHAT, + }); + createdReportActionID = NumberUtils.rand64(); + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${2}`, { + [createdReportActionID]: { + actionName: CONST.REPORT.ACTIONS.TYPE.CREATED, + automatic: false, + created: now, + reportActionID: createdReportActionID, + message: [ + { + style: 'strong', + text: '__FAKE__', + type: 'TEXT', + }, + { + style: 'normal', + text: 'created this report', + type: 'TEXT', + }, + ], + }, + 2: TestHelper.buildTestReportComment(now, BOB.ACCOUNT_ID, '2', BOB.NAME), + }); + await waitForBatchedUpdates(); + + // Message from alice should still be visible + messageFromAlice = screen.getByText(`1 from ${ALICE.NAME}`); + expect(messageFromAlice).not.toBeNull(); + }); +}); diff --git a/tests/utils/TestHelper.js b/tests/utils/TestHelper.js index 4c658f00489..5d6290dc18c 100644 --- a/tests/utils/TestHelper.js +++ b/tests/utils/TestHelper.js @@ -197,16 +197,17 @@ function setPersonalDetails(login, accountID) { /** * @param {String} created * @param {Number} actorAccountID - * @param {String} actionID + * @param {String} [actionID] + * @param {String} [actorName] * @returns {Object} */ -function buildTestReportComment(created, actorAccountID, actionID = null) { +function buildTestReportComment(created, actorAccountID, actionID = null, actorName = '') { const reportActionID = actionID || NumberUtils.rand64(); return { actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, person: [{type: 'TEXT', style: 'strong', text: 'User B'}], created, - message: [{type: 'COMMENT', html: `Comment ${actionID}`, text: `Comment ${actionID}`}], + message: [{type: 'COMMENT', html: `Comment ${actionID}${actorName ? ` from ${actorName}` : ''}`, text: `Comment ${actionID}${actorName ? ` from ${actorName}` : ''}`}], reportActionID, actorAccountID, }; From 06cc7bd20c37574bc389701b7b3b19ed96b67b76 Mon Sep 17 00:00:00 2001 From: rory Date: Fri, 29 Sep 2023 14:38:12 +0800 Subject: [PATCH 17/28] Finish writing test --- tests/ui/{Navigation.js => NavigationTest.js} | 24 ++++++++++++------- 1 file changed, 15 insertions(+), 9 deletions(-) rename tests/ui/{Navigation.js => NavigationTest.js} (92%) diff --git a/tests/ui/Navigation.js b/tests/ui/NavigationTest.js similarity index 92% rename from tests/ui/Navigation.js rename to tests/ui/NavigationTest.js index 546f9cd87a2..3a72efbaa42 100644 --- a/tests/ui/Navigation.js +++ b/tests/ui/NavigationTest.js @@ -4,7 +4,6 @@ import {render, screen, fireEvent} from '@testing-library/react-native'; import {Linking} from 'react-native'; import App from '../../src/App'; import CONST from '../../src/CONST'; -import ROUTES from '../../src/ROUTES'; import ONYXKEYS from '../../src/ONYXKEYS'; import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; import waitForBatchedUpdatesWithAct from '../utils/waitForBatchedUpdatesWithAct'; @@ -114,7 +113,7 @@ describe('Navigation', () => { // Add a report in Onyx let now = DateUtils.getDBTime(); await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${1}`, { - reportID: 1, + reportID: '1', reportName: CONST.REPORT.DEFAULT_REPORT_NAME, lastReadTime: now, lastVisibleActionCreated: now, @@ -156,20 +155,18 @@ describe('Navigation', () => { expect(optionRows).toHaveLength(1); screen.getByText(`Hi from ${ALICE.NAME}`); - await navigateToSidebarOption(0); await navigateToSidebarOption(0); const welcomeMessageHintText = Localize.translateLocal('accessibilityHints.chatWelcomeMessage'); - const createdAction = screen.queryByLabelText(welcomeMessageHintText); - expect(createdAction).toBeTruthy(); + screen.getByLabelText(welcomeMessageHintText); const reportCommentsHintText = Localize.translateLocal('accessibilityHints.chatMessage'); - const reportComments = screen.queryAllByLabelText(reportCommentsHintText); + let reportComments = screen.getAllByLabelText(reportCommentsHintText); expect(reportComments).toHaveLength(1); screen.getByText(`Comment 1 from ${ALICE.NAME}`); // Add another report to Onyx now = DateUtils.getDBTime(); await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${2}`, { - reportID: 2, + reportID: '2', reportName: CONST.REPORT.DEFAULT_REPORT_NAME, lastReadTime: now, lastVisibleActionCreated: now, @@ -204,7 +201,16 @@ describe('Navigation', () => { await waitForBatchedUpdates(); // Message from alice should still be visible - messageFromAlice = screen.getByText(`1 from ${ALICE.NAME}`); - expect(messageFromAlice).not.toBeNull(); + screen.getByText(`Comment 1 from ${ALICE.NAME}`); + + reportComments = screen.queryByText(`Comment 2 from ${BOB.NAME}`); + expect(reportComments).toBeNull(); + + // Replace report 1 with report 2 in the stack + Navigation.replaceReportIDInNavigationStack('1', '2'); + await waitForBatchedUpdates(); + + // Report 2 should now be visible, and report 1 should not + screen.getByText(`Comment 2 from ${BOB.NAME}`); }); }); From 1aad488ab76b66a7393ffb49979762ebda52db05 Mon Sep 17 00:00:00 2001 From: rory Date: Fri, 29 Sep 2023 14:41:15 +0800 Subject: [PATCH 18/28] Remove unnecessary local reference --- src/libs/Middleware/HandleUnusedOptimisticID.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libs/Middleware/HandleUnusedOptimisticID.ts b/src/libs/Middleware/HandleUnusedOptimisticID.ts index da487954547..30edfe7c5b6 100644 --- a/src/libs/Middleware/HandleUnusedOptimisticID.ts +++ b/src/libs/Middleware/HandleUnusedOptimisticID.ts @@ -26,9 +26,9 @@ const handleUnusedOptimisticID: Middleware = (requestResponse, request) => PersistedRequests.getAll() .slice(1) .forEach((persistedRequest, index) => { - const oldRequest = persistedRequest; - oldRequest.data = deepReplaceKeysAndValues(oldRequest.data, oldReportID as string, preexistingReportID); - PersistedRequests.update(index + 1, oldRequest); + // eslint-disable-next-line no-param-reassign + persistedRequest.data = deepReplaceKeysAndValues(persistedRequest.data, oldReportID as string, preexistingReportID); + PersistedRequests.update(index + 1, persistedRequest); }); }); return response; From e583ca44c05309eb1058bdc5ace7f0de2f91040d Mon Sep 17 00:00:00 2001 From: rory Date: Fri, 29 Sep 2023 14:48:10 +0800 Subject: [PATCH 19/28] Simplify PersistedRequests.update --- src/libs/actions/PersistedRequests.ts | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts index 319bd9efda7..92a3b817959 100644 --- a/src/libs/actions/PersistedRequests.ts +++ b/src/libs/actions/PersistedRequests.ts @@ -39,18 +39,8 @@ function remove(requestToRemove: Request) { Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests); } -function update(oldRequest: number | Request, newRequest: Request) { - let index = -1; - if (typeof oldRequest === 'number') { - index = oldRequest; - persistedRequests.splice(oldRequest, 1, newRequest); - } else { - index = persistedRequests.findIndex((persistedRequest) => isEqual(persistedRequest, oldRequest)); - if (index === -1) { - return; - } - } - persistedRequests.splice(index, 1, newRequest); +function update(oldRequestIndex: number, newRequest: Request) { + persistedRequests.splice(oldRequestIndex, 1, newRequest); Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests); } From 0d69f32c7b18ac02de52ab88fc8467cc00ce1b5e Mon Sep 17 00:00:00 2001 From: rory Date: Fri, 29 Sep 2023 14:50:31 +0800 Subject: [PATCH 20/28] Only skip the first persisted request if from sequential queue --- src/libs/Middleware/HandleUnusedOptimisticID.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libs/Middleware/HandleUnusedOptimisticID.ts b/src/libs/Middleware/HandleUnusedOptimisticID.ts index 30edfe7c5b6..89c5f467428 100644 --- a/src/libs/Middleware/HandleUnusedOptimisticID.ts +++ b/src/libs/Middleware/HandleUnusedOptimisticID.ts @@ -4,7 +4,7 @@ import {Middleware} from '../Request'; import * as PersistedRequests from '../actions/PersistedRequests'; import deepReplaceKeysAndValues from '../deepReplaceKeysAndValues'; -const handleUnusedOptimisticID: Middleware = (requestResponse, request) => +const handleUnusedOptimisticID: Middleware = (requestResponse, request, isFromSequentialQueue) => requestResponse.then((response) => { const responseOnyxData = response?.onyxData ?? []; responseOnyxData.forEach((onyxData) => { @@ -24,7 +24,7 @@ const handleUnusedOptimisticID: Middleware = (requestResponse, request) => } const oldReportID = request.data?.reportID; PersistedRequests.getAll() - .slice(1) + .slice(isFromSequentialQueue ? 1 : 0) .forEach((persistedRequest, index) => { // eslint-disable-next-line no-param-reassign persistedRequest.data = deepReplaceKeysAndValues(persistedRequest.data, oldReportID as string, preexistingReportID); From ed415dc9d853d69771ee77bd4358d4a71abc352b Mon Sep 17 00:00:00 2001 From: rory Date: Fri, 29 Sep 2023 14:51:47 +0800 Subject: [PATCH 21/28] Fix spacing --- src/libs/deepReplaceKeysAndValues.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/deepReplaceKeysAndValues.ts b/src/libs/deepReplaceKeysAndValues.ts index 5156b3e06a2..ccf19038185 100644 --- a/src/libs/deepReplaceKeysAndValues.ts +++ b/src/libs/deepReplaceKeysAndValues.ts @@ -1,7 +1,7 @@ type ReplaceableValue = Record | unknown[] | string | number | boolean | undefined | null; /** - * @param obj the object to transform + * @param obj the object to transform * @param oldVal the value to search for * @param newVal the replacement value */ From e1f470f26ecb184f34dde67749a1441a19decade Mon Sep 17 00:00:00 2001 From: rory Date: Fri, 29 Sep 2023 15:06:41 +0800 Subject: [PATCH 22/28] Renamed obj to target --- src/libs/deepReplaceKeysAndValues.ts | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/libs/deepReplaceKeysAndValues.ts b/src/libs/deepReplaceKeysAndValues.ts index ccf19038185..58a96a58a2c 100644 --- a/src/libs/deepReplaceKeysAndValues.ts +++ b/src/libs/deepReplaceKeysAndValues.ts @@ -1,29 +1,29 @@ type ReplaceableValue = Record | unknown[] | string | number | boolean | undefined | null; /** - * @param obj the object to transform + * @param target the object or value to transform * @param oldVal the value to search for * @param newVal the replacement value */ -function deepReplaceKeysAndValues(obj: T, oldVal: string, newVal: string): T { - if (!obj) { - return obj; +function deepReplaceKeysAndValues(target: T, oldVal: string, newVal: string): T { + if (!target) { + return target; } - if (typeof obj === 'string') { - return obj.replace(oldVal, newVal) as T; + if (typeof target === 'string') { + return target.replace(oldVal, newVal) as T; } - if (typeof obj !== 'object') { - return obj; + if (typeof target !== 'object') { + return target; } - if (Array.isArray(obj)) { - return obj.map((item) => deepReplaceKeysAndValues(item as T, oldVal, newVal)) as T; + if (Array.isArray(target)) { + return target.map((item) => deepReplaceKeysAndValues(item as T, oldVal, newVal)) as T; } const newObj: Record = {}; - Object.entries(obj).forEach(([key, val]) => { + Object.entries(target).forEach(([key, val]) => { const newKey = key.replace(oldVal, newVal); if (typeof val === 'object') { From d9863e113a9a06a0a648d95e1572b54c8a94a246 Mon Sep 17 00:00:00 2001 From: rory Date: Fri, 29 Sep 2023 15:10:46 +0800 Subject: [PATCH 23/28] Fix typescript types --- src/libs/Request.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/Request.ts b/src/libs/Request.ts index f8fb04595bb..903e70358da 100644 --- a/src/libs/Request.ts +++ b/src/libs/Request.ts @@ -17,7 +17,7 @@ function makeXHR(request: Request): Promise { return new Promise((resolve) => resolve()); } return HttpUtils.xhr(request.command, finalParameters, request.type, request.shouldUseSecure); - }); + }) as Promise; } function processWithMiddleware(request: Request, isFromSequentialQueue = false): Promise { From e72dded4f44072163b61d39fe082345259b0a43d Mon Sep 17 00:00:00 2001 From: rory Date: Sun, 1 Oct 2023 16:25:11 +0800 Subject: [PATCH 24/28] Remove commented out code --- tests/ui/NavigationTest.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/ui/NavigationTest.js b/tests/ui/NavigationTest.js index 3a72efbaa42..8115eeaf0b7 100644 --- a/tests/ui/NavigationTest.js +++ b/tests/ui/NavigationTest.js @@ -147,9 +147,6 @@ describe('Navigation', () => { }); await waitForBatchedUpdates(); - // Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute('1')); - // await new Promise((resolve) => setTimeout(resolve, CONST.ANIMATED_TRANSITION)); - // Verify there is now one option in the sidebar optionRows = screen.queryAllByAccessibilityHint(optionRowsHintText); expect(optionRows).toHaveLength(1); From 0d477ac3a36f46ac977d2cb30e84b2ec8486d460 Mon Sep 17 00:00:00 2001 From: rory Date: Sun, 1 Oct 2023 16:28:03 +0800 Subject: [PATCH 25/28] Update outdated comment --- src/libs/Navigation/getTopmostCentralPane.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/Navigation/getTopmostCentralPane.js b/src/libs/Navigation/getTopmostCentralPane.js index edc55ff8955..8289f922d44 100644 --- a/src/libs/Navigation/getTopmostCentralPane.js +++ b/src/libs/Navigation/getTopmostCentralPane.js @@ -3,7 +3,7 @@ import lodashFindLast from 'lodash/findLast'; // This function is in a separate file than Navigation.js to avoid cyclic dependency. /** - * Find the last visited report screen in the navigation state and get the id of it. + * Find the route object from the topmost CentralPaneNavigator * * @param {Object} state - The react-navigation state * @returns {String | undefined} From 8b777eaeffdc03d91cf739b3a8a4ba0be2940261 Mon Sep 17 00:00:00 2001 From: rory Date: Mon, 2 Oct 2023 09:14:14 +0800 Subject: [PATCH 26/28] Revert navigation changes --- src/libs/Navigation/Navigation.js | 69 +----- src/libs/Navigation/getTopmostCentralPane.js | 18 -- src/libs/Navigation/getTopmostReportId.js | 7 +- src/libs/actions/Report.js | 7 +- tests/ui/NavigationTest.js | 213 ------------------- tests/utils/TestHelper.js | 7 +- 6 files changed, 16 insertions(+), 305 deletions(-) delete mode 100644 src/libs/Navigation/getTopmostCentralPane.js delete mode 100644 tests/ui/NavigationTest.js diff --git a/src/libs/Navigation/Navigation.js b/src/libs/Navigation/Navigation.js index 2d40d8af0d7..dc4f35a59cb 100644 --- a/src/libs/Navigation/Navigation.js +++ b/src/libs/Navigation/Navigation.js @@ -1,4 +1,5 @@ import _ from 'lodash'; +import lodashGet from 'lodash/get'; import {CommonActions, getPathFromState, StackActions} from '@react-navigation/native'; import {getActionFromState} from '@react-navigation/core'; import Log from '../Log'; @@ -12,7 +13,6 @@ import originalGetTopmostReportId from './getTopmostReportId'; import getStateFromPath from './getStateFromPath'; import SCREENS from '../../SCREENS'; import CONST from '../../CONST'; -import getTopmostCentralPane from './getTopmostCentralPane'; let resolveNavigationIsReadyPromise; const navigationIsReadyPromise = new Promise((resolve) => { @@ -148,70 +148,6 @@ function setParams(params, routeKey) { }); } -/** - * Replace a given reportID in the navigation stack. - * - * @param {String} oldReportID - * @param {String} newReportID - */ -function replaceReportIDInNavigationStack(oldReportID, newReportID) { - const state = navigationRef.getState(); - - const topmostCentralPane = getTopmostCentralPane(state); - if (!topmostCentralPane) { - return; - } - - const directReportIDParam = _.get(topmostCentralPane, 'params.params.reportID'); - if (directReportIDParam === oldReportID) { - setParams( - { - reportID: newReportID, - }, - _.get(topmostCentralPane, 'key', ''), - ); - } - - const topmostReportID = getTopmostReportId(); - if (topmostReportID === oldReportID) { - navigationRef.current.dispatch({ - ...StackActions.replace('Report', { - reportID: newReportID, - }), - }); - } - - if (!topmostCentralPane.state) { - return; - } - - const centralPaneRoutes = _.get(topmostCentralPane, 'state.routes', []); - _.each(centralPaneRoutes, (route) => { - const reportIDParam = _.get(route, 'params.reportID'); - if (!reportIDParam) { - return; - } - - if (reportIDParam !== oldReportID) { - return; - } - - route.setParams({reportID: newReportID}); - - if (route.name !== 'Report') { - return; - } - - navigationRef.current.dispatch({ - ...StackActions.replace('Report', { - reportID: newReportID, - }), - source: route.key, - target: navigationRef.current.getState().key, - }); - }); -} - /** * Dismisses the last modal stack if there is any * @@ -250,7 +186,7 @@ function dismissModal(targetReportID) { */ function getActiveRoute() { const currentRoute = navigationRef.current && navigationRef.current.getCurrentRoute(); - const currentRouteHasName = _.get(currentRoute, 'name', false); + const currentRouteHasName = lodashGet(currentRoute, 'name', false); if (!currentRouteHasName) { return ''; } @@ -332,7 +268,6 @@ export default { setIsNavigationReady, getTopmostReportId, getRouteNameFromStateEvent, - replaceReportIDInNavigationStack, }; export {navigationRef}; diff --git a/src/libs/Navigation/getTopmostCentralPane.js b/src/libs/Navigation/getTopmostCentralPane.js deleted file mode 100644 index 8289f922d44..00000000000 --- a/src/libs/Navigation/getTopmostCentralPane.js +++ /dev/null @@ -1,18 +0,0 @@ -import lodashFindLast from 'lodash/findLast'; - -// This function is in a separate file than Navigation.js to avoid cyclic dependency. - -/** - * Find the route object from the topmost CentralPaneNavigator - * - * @param {Object} state - The react-navigation state - * @returns {String | undefined} - */ -function getTopmostCentralPane(state) { - if (!state) { - return; - } - return lodashFindLast(state.routes, (route) => route.name === 'CentralPaneNavigator'); -} - -export default getTopmostCentralPane; diff --git a/src/libs/Navigation/getTopmostReportId.js b/src/libs/Navigation/getTopmostReportId.js index b697b3ad9a3..8ca9c39baf6 100644 --- a/src/libs/Navigation/getTopmostReportId.js +++ b/src/libs/Navigation/getTopmostReportId.js @@ -1,6 +1,5 @@ import lodashFindLast from 'lodash/findLast'; import lodashGet from 'lodash/get'; -import getTopmostCentralPane from './getTopmostCentralPane'; // This function is in a separate file than Navigation.js to avoid cyclic dependency. @@ -11,7 +10,11 @@ import getTopmostCentralPane from './getTopmostCentralPane'; * @returns {String | undefined} - It's possible that there is no report screen */ function getTopmostReportId(state) { - const topmostCentralPane = getTopmostCentralPane(state); + if (!state) { + return; + } + const topmostCentralPane = lodashFindLast(state.routes, (route) => route.name === 'CentralPaneNavigator'); + if (!topmostCentralPane) { return; } diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index d7495bb83c5..66008ae5ae2 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -968,7 +968,12 @@ function handleReportChanged(report) { // We should clear out the optimistically created report and re-route the user to the preexisting report. if (report && report.reportID && report.preexistingReportID) { Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`, null); - Navigation.replaceReportIDInNavigationStack(report.reportID, report.preexistingReportID); + + // Only re-route them if they are still looking at the optimistically created report + if (Navigation.getActiveRoute().includes(`/r/${report.reportID}`)) { + // Pass 'FORCED_UP' type to replace new report on second login with proper one in the Navigation + Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report.preexistingReportID), CONST.NAVIGATION.TYPE.FORCED_UP); + } return; } diff --git a/tests/ui/NavigationTest.js b/tests/ui/NavigationTest.js deleted file mode 100644 index 8115eeaf0b7..00000000000 --- a/tests/ui/NavigationTest.js +++ /dev/null @@ -1,213 +0,0 @@ -import React from 'react'; -import Onyx from 'react-native-onyx'; -import {render, screen, fireEvent} from '@testing-library/react-native'; -import {Linking} from 'react-native'; -import App from '../../src/App'; -import CONST from '../../src/CONST'; -import ONYXKEYS from '../../src/ONYXKEYS'; -import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; -import waitForBatchedUpdatesWithAct from '../utils/waitForBatchedUpdatesWithAct'; -import * as TestHelper from '../utils/TestHelper'; -import appSetup from '../../src/setup'; -import PusherConnectionManager from '../../src/libs/PusherConnectionManager'; -import * as Pusher from '../../src/libs/Pusher/pusher'; -import CONFIG from '../../src/CONFIG'; -import * as Localize from '../../src/libs/Localize'; -import * as User from '../../src/libs/actions/User'; -import * as AppActions from '../../src/libs/actions/App'; -import DateUtils from '../../src/libs/DateUtils'; -import * as NumberUtils from '../../src/libs/NumberUtils'; -import Navigation from '../../src/libs/Navigation/Navigation'; - -// We need a large timeout here as we are lazy loading React Navigation screens and this test is running against the entire mounted App -jest.setTimeout(30000); - -jest.mock('../../src/libs/Notification/LocalNotification'); -jest.mock('../../src/components/Icon/Expensicons'); - -beforeAll(() => { - // In this test, we are generically mocking the responses of all API requests by mocking fetch() and having it - // return 200. In other tests, we might mock HttpUtils.xhr() with a more specific mock data response (which means - // fetch() never gets called so it does not need mocking) or we might have fetch throw an error to test error handling - // behavior. But here we just want to treat all API requests as a generic "success" and in the cases where we need to - // simulate data arriving we will just set it into Onyx directly with Onyx.merge() or Onyx.set() etc. - global.fetch = TestHelper.getGlobalFetchMock(); - - Linking.setInitialURL('https://new.expensify.com/'); - appSetup(); - - // Connect to Pusher - PusherConnectionManager.init(); - Pusher.init({ - appKey: CONFIG.PUSHER.APP_KEY, - cluster: CONFIG.PUSHER.CLUSTER, - authEndpoint: `${CONFIG.EXPENSIFY.DEFAULT_API_ROOT}api?command=AuthenticatePusher`, - }); -}); - -const ALICE = { - ACCOUNT_ID: 1, - EMAIL: 'alice@expensifail.com', - NAME: 'Alice', -}; -const BOB = { - ACCOUNT_ID: 2, - EMAIL: 'bob@expensifail.com', - NAME: 'Bob', -}; - -async function signInToApp() { - // Render the App and sign in as a test user. - render(); - await waitForBatchedUpdatesWithAct(); - const hintText = Localize.translateLocal('loginForm.loginForm'); - const loginForm = screen.queryAllByLabelText(hintText); - expect(loginForm).toHaveLength(1); - await TestHelper.signInWithTestUser(ALICE.ACCOUNT_ID, ALICE.EMAIL, undefined, undefined, 'A'); - User.subscribeToUserEvents(); - await waitForBatchedUpdates(); - - // We manually setting the sidebar as loaded since the onLayout event does not fire in tests - AppActions.setSidebarLoaded(true); - await waitForBatchedUpdates(); -} - -/** - * @param {Number} index - * @return {Promise} - */ -function navigateToSidebarOption(index) { - const hintText = Localize.translateLocal('accessibilityHints.navigatesToChat'); - const optionRows = screen.getAllByAccessibilityHint(hintText); - fireEvent(optionRows[index], 'press'); - return waitForBatchedUpdates(); -} - -describe('Navigation', () => { - beforeEach(async () => { - await Onyx.merge(ONYXKEYS.PERSONAL_DETAILS_LIST, { - [ALICE.ACCOUNT_ID]: TestHelper.buildPersonalDetails(ALICE.EMAIL, ALICE.ACCOUNT_ID, ALICE.NAME), - [BOB.ACCOUNT_ID]: TestHelper.buildPersonalDetails(BOB.EMAIL, BOB.ACCOUNT_ID, BOB.NAME), - }); - await waitForBatchedUpdates(); - }); - - afterEach(async () => { - jest.clearAllMocks(); - await Onyx.clear(); - }); - - it('Replaces report screen in stack', async () => { - await signInToApp(); - - // Verify the sidebar links are rendered - const sidebarLinksHintText = Localize.translateLocal('sidebarScreen.listOfChats'); - const sidebarLinks = screen.queryAllByLabelText(sidebarLinksHintText); - expect(sidebarLinks).toHaveLength(1); - - // Verify there is nothing in the sidebar to begin with - const optionRowsHintText = Localize.translateLocal('accessibilityHints.navigatesToChat'); - let optionRows = screen.queryAllByAccessibilityHint(optionRowsHintText); - expect(optionRows).toHaveLength(0); - - // Add a report in Onyx - let now = DateUtils.getDBTime(); - await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${1}`, { - reportID: '1', - reportName: CONST.REPORT.DEFAULT_REPORT_NAME, - lastReadTime: now, - lastVisibleActionCreated: now, - - // Note: this is intentionally different from the actual last message to prevent the sidebar from matching when we want to make sure that the central report pane is visible - lastMessageText: `Hi from ${ALICE.NAME}`, - participantAccountIDs: [ALICE.ACCOUNT_ID, BOB.ACCOUNT_ID], - type: CONST.REPORT.TYPE.CHAT, - }); - let createdReportActionID = NumberUtils.rand64(); - await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${1}`, { - [createdReportActionID]: { - actionName: CONST.REPORT.ACTIONS.TYPE.CREATED, - automatic: false, - created: now, - reportActionID: createdReportActionID, - message: [ - { - style: 'strong', - text: '__FAKE__', - type: 'TEXT', - }, - { - style: 'normal', - text: 'created this report', - type: 'TEXT', - }, - ], - }, - 1: TestHelper.buildTestReportComment(now, ALICE.ACCOUNT_ID, '1', ALICE.NAME), - }); - await waitForBatchedUpdates(); - - // Verify there is now one option in the sidebar - optionRows = screen.queryAllByAccessibilityHint(optionRowsHintText); - expect(optionRows).toHaveLength(1); - screen.getByText(`Hi from ${ALICE.NAME}`); - - await navigateToSidebarOption(0); - const welcomeMessageHintText = Localize.translateLocal('accessibilityHints.chatWelcomeMessage'); - screen.getByLabelText(welcomeMessageHintText); - const reportCommentsHintText = Localize.translateLocal('accessibilityHints.chatMessage'); - let reportComments = screen.getAllByLabelText(reportCommentsHintText); - expect(reportComments).toHaveLength(1); - screen.getByText(`Comment 1 from ${ALICE.NAME}`); - - // Add another report to Onyx - now = DateUtils.getDBTime(); - await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${2}`, { - reportID: '2', - reportName: CONST.REPORT.DEFAULT_REPORT_NAME, - lastReadTime: now, - lastVisibleActionCreated: now, - - // Note: this is intentionally different from the actual last message to prevent the sidebar from matching when we want to make sure that the central report pane is visible - lastMessageText: `Hi from ${BOB.NAME}`, - participantAccountIDs: [ALICE.ACCOUNT_ID, BOB.ACCOUNT_ID], - type: CONST.REPORT.TYPE.CHAT, - }); - createdReportActionID = NumberUtils.rand64(); - await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${2}`, { - [createdReportActionID]: { - actionName: CONST.REPORT.ACTIONS.TYPE.CREATED, - automatic: false, - created: now, - reportActionID: createdReportActionID, - message: [ - { - style: 'strong', - text: '__FAKE__', - type: 'TEXT', - }, - { - style: 'normal', - text: 'created this report', - type: 'TEXT', - }, - ], - }, - 2: TestHelper.buildTestReportComment(now, BOB.ACCOUNT_ID, '2', BOB.NAME), - }); - await waitForBatchedUpdates(); - - // Message from alice should still be visible - screen.getByText(`Comment 1 from ${ALICE.NAME}`); - - reportComments = screen.queryByText(`Comment 2 from ${BOB.NAME}`); - expect(reportComments).toBeNull(); - - // Replace report 1 with report 2 in the stack - Navigation.replaceReportIDInNavigationStack('1', '2'); - await waitForBatchedUpdates(); - - // Report 2 should now be visible, and report 1 should not - screen.getByText(`Comment 2 from ${BOB.NAME}`); - }); -}); diff --git a/tests/utils/TestHelper.js b/tests/utils/TestHelper.js index 5d6290dc18c..4c658f00489 100644 --- a/tests/utils/TestHelper.js +++ b/tests/utils/TestHelper.js @@ -197,17 +197,16 @@ function setPersonalDetails(login, accountID) { /** * @param {String} created * @param {Number} actorAccountID - * @param {String} [actionID] - * @param {String} [actorName] + * @param {String} actionID * @returns {Object} */ -function buildTestReportComment(created, actorAccountID, actionID = null, actorName = '') { +function buildTestReportComment(created, actorAccountID, actionID = null) { const reportActionID = actionID || NumberUtils.rand64(); return { actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, person: [{type: 'TEXT', style: 'strong', text: 'User B'}], created, - message: [{type: 'COMMENT', html: `Comment ${actionID}${actorName ? ` from ${actorName}` : ''}`, text: `Comment ${actionID}${actorName ? ` from ${actorName}` : ''}`}], + message: [{type: 'COMMENT', html: `Comment ${actionID}`, text: `Comment ${actionID}`}], reportActionID, actorAccountID, }; From 0094aab83ed65a64a47cec2829a3b6a4e8e3ea43 Mon Sep 17 00:00:00 2001 From: rory Date: Mon, 2 Oct 2023 09:47:25 +0800 Subject: [PATCH 27/28] Updated to use lodashClone --- src/libs/Middleware/HandleUnusedOptimisticID.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/libs/Middleware/HandleUnusedOptimisticID.ts b/src/libs/Middleware/HandleUnusedOptimisticID.ts index 89c5f467428..b6d2be45ea2 100644 --- a/src/libs/Middleware/HandleUnusedOptimisticID.ts +++ b/src/libs/Middleware/HandleUnusedOptimisticID.ts @@ -1,3 +1,4 @@ +import _ from 'lodash'; import ONYXKEYS from '../../ONYXKEYS'; import Report from '../../types/onyx/Report'; import {Middleware} from '../Request'; @@ -26,9 +27,9 @@ const handleUnusedOptimisticID: Middleware = (requestResponse, request, isFromSe PersistedRequests.getAll() .slice(isFromSequentialQueue ? 1 : 0) .forEach((persistedRequest, index) => { - // eslint-disable-next-line no-param-reassign - persistedRequest.data = deepReplaceKeysAndValues(persistedRequest.data, oldReportID as string, preexistingReportID); - PersistedRequests.update(index + 1, persistedRequest); + const persistedRequestClone = _.clone(persistedRequest); + persistedRequestClone.data = deepReplaceKeysAndValues(persistedRequest.data, oldReportID as string, preexistingReportID); + PersistedRequests.update(index + 1, persistedRequestClone); }); }); return response; From f0b1a4ead05f9c63b71d7ff7e6a42d1ce860d1dd Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 3 Oct 2023 06:53:18 +0800 Subject: [PATCH 28/28] Use offset in PersistedRequests.update --- src/libs/Middleware/HandleUnusedOptimisticID.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libs/Middleware/HandleUnusedOptimisticID.ts b/src/libs/Middleware/HandleUnusedOptimisticID.ts index b6d2be45ea2..a7e90accd3f 100644 --- a/src/libs/Middleware/HandleUnusedOptimisticID.ts +++ b/src/libs/Middleware/HandleUnusedOptimisticID.ts @@ -24,12 +24,13 @@ const handleUnusedOptimisticID: Middleware = (requestResponse, request, isFromSe return; } const oldReportID = request.data?.reportID; + const offset = isFromSequentialQueue ? 1 : 0; PersistedRequests.getAll() - .slice(isFromSequentialQueue ? 1 : 0) + .slice(offset) .forEach((persistedRequest, index) => { const persistedRequestClone = _.clone(persistedRequest); persistedRequestClone.data = deepReplaceKeysAndValues(persistedRequest.data, oldReportID as string, preexistingReportID); - PersistedRequests.update(index + 1, persistedRequestClone); + PersistedRequests.update(index + offset, persistedRequestClone); }); }); return response;