Merge pull request #6183 from Expensify/marco-checkIfOnyxIsReady3

Retry requests if storage (auth + credentials) is not ready

__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 */

src/libs/API.js

@@ -6,25 +6,40 @@ 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
 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);
 

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<Object> | 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,
 };

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

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';

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