diff --git a/__mocks__/react-native-onyx.js b/__mocks__/react-native-onyx.js new file mode 100644 index 00000000000..dc9593e2750 --- /dev/null +++ b/__mocks__/react-native-onyx.js @@ -0,0 +1,26 @@ +/* eslint-disable rulesdir/prefer-onyx-connect-in-libs */ +import Onyx, {withOnyx} from 'react-native-onyx'; + +let connectCallbackDelay = 0; +function addDelayToConnectCallback(delay) { + connectCallbackDelay = delay; +} + +export default { + ...Onyx, + connect: mapping => Onyx.connect({ + ...mapping, + callback: (...params) => { + if (connectCallbackDelay > 0) { + setTimeout(() => { + mapping.callback(...params); + }, connectCallbackDelay); + } else { + mapping.callback(...params); + } + }, + }), + addDelayToConnectCallback, +}; +export {withOnyx}; +/* eslint-enable rulesdir/prefer-onyx-connect-in-libs */ diff --git a/src/libs/API.js b/src/libs/API.js index f5635fa7fc5..f7d5678d304 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -6,9 +6,9 @@ import CONFIG from '../CONFIG'; import ONYXKEYS from '../ONYXKEYS'; // eslint-disable-next-line import/no-cycle import redirectToSignIn from './actions/SignInRedirect'; -import * as Network from './Network'; import isViaExpensifyCashNative from './isViaExpensifyCashNative'; - +// eslint-disable-next-line import/no-cycle +import * as Network from './Network'; // eslint-disable-next-line import/no-cycle import LogUtil from './Log'; // eslint-disable-next-line import/no-cycle @@ -16,15 +16,30 @@ import * as Session from './actions/Session'; let isAuthenticating; let credentials; +let authToken; + +function checkRequiredDataAndSetNetworkReady() { + if (_.isUndefined(authToken) || _.isUndefined(credentials)) { + return; + } + + Network.setIsReady(true); +} + Onyx.connect({ key: ONYXKEYS.CREDENTIALS, - callback: val => credentials = val, + callback: (val) => { + credentials = val || null; + checkRequiredDataAndSetNetworkReady(); + }, }); -let authToken; Onyx.connect({ key: ONYXKEYS.SESSION, - callback: val => authToken = val ? val.authToken : null, + callback: (val) => { + authToken = lodashGet(val, 'authToken', null); + checkRequiredDataAndSetNetworkReady(); + }, }); /** @@ -59,18 +74,6 @@ function addDefaultValuesToParameters(command, parameters) { const finalParameters = {...parameters}; if (isAuthTokenRequired(command) && !parameters.authToken) { - // If we end up here with no authToken it means we are trying to make an API request before we are signed in. - // In this case, we should cancel the current request by pausing the queue and clearing the remaining requests. - if (!authToken) { - redirectToSignIn(); - - LogUtil.info('A request was made without an authToken', false, {command, parameters}); - Network.pauseRequestQueue(); - Network.clearRequestQueue(); - Network.unpauseRequestQueue(); - return; - } - finalParameters.authToken = authToken; } @@ -197,7 +200,7 @@ Network.registerErrorHandler((queuedRequest, error) => { /** * @param {Object} parameters - * @param {String} [parameters.useExpensifyLogin] + * @param {Boolean} [parameters.useExpensifyLogin] * @param {String} parameters.partnerName * @param {String} parameters.partnerPassword * @param {String} parameters.partnerUserID @@ -448,6 +451,7 @@ function Get(parameters, shouldUseSecure = false) { /** * @param {Object} parameters * @param {String} parameters.email + * @param {Boolean} parameters.forceNetworkRequest * @returns {Promise} */ function GetAccountStatus(parameters) { @@ -1015,14 +1019,13 @@ function DeleteBankAccount(parameters) { /** * @param {Object} parameters - * @param {String[]} data * @returns {Promise} */ function Mobile_GetConstants(parameters) { const commandName = 'Mobile_GetConstants'; requireParameters(['data'], parameters, commandName); - // Stringinfy the parameters object as we cannot send an object via FormData + // Stringify the parameters object as we cannot send an object via FormData const finalParameters = parameters; finalParameters.data = JSON.stringify(parameters.data); diff --git a/src/libs/Network.js b/src/libs/Network.js index 2b5e602869d..cc9e2b8e878 100644 --- a/src/libs/Network.js +++ b/src/libs/Network.js @@ -5,8 +5,11 @@ import HttpUtils from './HttpUtils'; import ONYXKEYS from '../ONYXKEYS'; import * as ActiveClientManager from './ActiveClientManager'; import CONST from '../CONST'; +// eslint-disable-next-line import/no-cycle +import LogUtil from './Log'; import * as NetworkRequestQueue from './actions/NetworkRequestQueue'; +let isReady = false; let isQueuePaused = false; // Queue for network requests so we don't lose actions done by the user while offline @@ -25,6 +28,8 @@ let onError = () => {}; let didLoadPersistedRequests; let isOffline; +const PROCESS_REQUEST_DELAY_MS = 1000; + /** * Process the offline NETWORK_REQUEST_QUEUE * @param {Array | null} persistedRequests - Requests @@ -96,15 +101,29 @@ Onyx.connect({ callback: val => email = val ? val.email : null, }); +/** + * @param {Boolean} val + */ +function setIsReady(val) { + isReady = val; +} + /** * Checks to see if a request can be made. * * @param {Object} request + * @param {String} request.type + * @param {String} request.command * @param {Object} request.data * @param {Boolean} request.data.forceNetworkRequest * @return {Boolean} */ function canMakeRequest(request) { + if (!isReady) { + LogUtil.hmmm('Trying to make a request when Network is not ready', {command: request.command, type: request.type}); + return false; + } + // These requests are always made even when the queue is paused if (request.data.forceNetworkRequest === true) { return true; @@ -202,13 +221,6 @@ function processNetworkRequestQueue() { ? enhanceParameters(queuedRequest.command, requestData) : requestData; - // Check to see if the queue has paused again. It's possible that a call to enhanceParameters() - // has paused the queue and if this is the case we must return. We don't retry these requests - // since if a request is made without an authToken we sign out the user. - if (!canMakeRequest(queuedRequest)) { - return; - } - HttpUtils.xhr(queuedRequest.command, finalParameters, queuedRequest.type, queuedRequest.shouldUseSecure) .then(response => onResponse(queuedRequest, response)) .catch(error => onError(queuedRequest, error)); @@ -232,7 +244,7 @@ function processNetworkRequestQueue() { } // Process our write queue very often -setInterval(processNetworkRequestQueue, 1000); +setInterval(processNetworkRequestQueue, PROCESS_REQUEST_DELAY_MS); /** * @param {Object} request @@ -331,9 +343,11 @@ function registerErrorHandler(callback) { export { post, pauseRequestQueue, + PROCESS_REQUEST_DELAY_MS, unpauseRequestQueue, registerParameterEnhancer, clearRequestQueue, registerResponseHandler, registerErrorHandler, + setIsReady, }; diff --git a/tests/actions/ReportTest.js b/tests/actions/ReportTest.js index 718e5f9ef00..1696f93271e 100644 --- a/tests/actions/ReportTest.js +++ b/tests/actions/ReportTest.js @@ -1,5 +1,9 @@ import _ from 'underscore'; import Onyx from 'react-native-onyx'; +import lodashGet from 'lodash/get'; +import { + beforeEach, beforeAll, afterEach, jest, describe, it, expect, +} from '@jest/globals'; import ONYXKEYS from '../../src/ONYXKEYS'; import * as Pusher from '../../src/libs/Pusher/pusher'; import PusherConnectionManager from '../../src/libs/PusherConnectionManager'; @@ -30,8 +34,15 @@ describe('actions/Report', () => { cluster: CONFIG.PUSHER.CLUSTER, authEndpoint: `${CONFIG.EXPENSIFY.URL_API_ROOT}api?command=Push_Authenticate`, }); + + Onyx.init({ + keys: ONYXKEYS, + registerStorageEventListener: () => {}, + }); }); + beforeEach(() => Onyx.clear().then(waitForPromisesToResolve)); + afterEach(() => { // Unsubscribe from account channel after each test since we subscribe in the function // subscribeToUserEvents and we don't want duplicate event subscriptions. @@ -130,7 +141,7 @@ describe('actions/Report', () => { let reportIsPinned; Onyx.connect({ key: `${ONYXKEYS.COLLECTION.REPORT}${REPORT_ID}`, - callback: val => reportIsPinned = val.isPinned, + callback: val => reportIsPinned = lodashGet(val, 'isPinned'), }); // Set up Onyx with some test user data diff --git a/tests/actions/SessionTest.js b/tests/actions/SessionTest.js index 9f6d6ee5ad3..4e6fe3d71ed 100644 --- a/tests/actions/SessionTest.js +++ b/tests/actions/SessionTest.js @@ -1,4 +1,5 @@ import Onyx from 'react-native-onyx'; +import {beforeEach, jest, test} from '@jest/globals'; import * as API from '../../src/libs/API'; import HttpUtils from '../../src/libs/HttpUtils'; import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; @@ -16,6 +17,13 @@ jest.mock('../../src/libs/Notification/PushNotification', () => ({ // We test HttpUtils.xhr() since this means that our API command turned into a network request and isn't only queued. HttpUtils.xhr = jest.fn(); +Onyx.init({ + keys: ONYXKEYS, + registerStorageEventListener: () => {}, +}); + +beforeEach(() => Onyx.clear().then(waitForPromisesToResolve)); + test('Authenticate is called with saved credentials when a session expires', () => { // Given a test user and set of authToken with subscriptions to session and credentials const TEST_USER_LOGIN = 'test@testguy.com'; diff --git a/tests/unit/NetworkTest.js b/tests/unit/NetworkTest.js index ac0e411bd8b..af2d90fcb6e 100644 --- a/tests/unit/NetworkTest.js +++ b/tests/unit/NetworkTest.js @@ -1,10 +1,16 @@ import _ from 'underscore'; import Onyx from 'react-native-onyx'; + +import { + beforeEach, jest, test, expect, afterEach, +} from '@jest/globals'; import * as API from '../../src/libs/API'; import {signInWithTestUser} from '../utils/TestHelper'; import HttpUtils from '../../src/libs/HttpUtils'; import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; import ONYXKEYS from '../../src/ONYXKEYS'; +import * as Network from '../../src/libs/Network'; +import {fetchAccountDetails} from '../../src/libs/actions/Session'; // Set up manual mocks for methods used in the actions so our test does not fail. jest.mock('../../src/libs/Notification/PushNotification', () => ({ @@ -22,6 +28,12 @@ Onyx.init({ beforeEach(() => Onyx.clear().then(waitForPromisesToResolve)); +afterEach(() => { + Network.setIsReady(false); + Onyx.addDelayToConnectCallback(0); + jest.clearAllMocks(); +}); + test('failing to reauthenticate while offline should not log out user', () => { // Given a test user login and account ID const TEST_USER_LOGIN = 'test@testguy.com'; @@ -209,3 +221,53 @@ test('consecutive API calls eventually succeed when authToken is expired', () => expect(chatList).toEqual(TEST_CHAT_LIST); }); }); + +test('retry network request if auth and credentials are not read from Onyx yet', () => { + // For this test we're having difficulty creating a situation where Onyx.connect() has not yet run + // because some Onyx.connect callbacks are already registered in API.js (which happens before this + // unit test is setup), so in order to test an scenario where the auth token and credentials hasn't + // been read from storage we set Network.setIsReady(false) and trigger an update in the Onyx.connect + // callbacks registered in API.js merging an empty object. + + // Given a test user login and account ID + const TEST_USER_LOGIN = 'test@testguy.com'; + + // Given a delay to the Onyx.connect callbacks + const ONYX_DELAY_MS = 3000; + Onyx.addDelayToConnectCallback(ONYX_DELAY_MS); + + // Given initial state to Network + Network.setIsReady(false); + + // Given any initial value to trigger an update + Onyx.merge(ONYXKEYS.CREDENTIALS, {}); + Onyx.merge(ONYXKEYS.SESSION, {}); + + // Given some mock functions to track the isReady + // flag in Network and the http requests made + const spyNetworkSetIsReady = jest.spyOn(Network, 'setIsReady'); + const spyHttpUtilsXhr = jest.spyOn(HttpUtils, 'xhr').mockImplementation(() => Promise.resolve({})); + + // When we make an arbitrary request that can be retried + // And we wait for the Onyx.callbacks to be set + fetchAccountDetails(TEST_USER_LOGIN); + return waitForPromisesToResolve().then(() => { + // Then we expect not having the Network ready and not making an http request + expect(spyNetworkSetIsReady).not.toHaveBeenCalled(); + expect(spyHttpUtilsXhr).not.toHaveBeenCalled(); + + // When we resolve Onyx.connect callbacks + jest.advanceTimersByTime(ONYX_DELAY_MS); + + // Then we should expect call Network.setIsReady(true) + // And We should expect not making an http request yet + expect(spyNetworkSetIsReady).toHaveBeenLastCalledWith(true); + expect(spyHttpUtilsXhr).not.toHaveBeenCalled(); + + // When we run processNetworkRequestQueue in the setInterval of Network.js + jest.advanceTimersByTime(Network.PROCESS_REQUEST_DELAY_MS); + + // Then we should expect a retry of the network request + expect(spyHttpUtilsXhr).toHaveBeenCalled(); + }); +});