Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix duplicate comments posting related to Log calls from inside processNetworkRequestQueue() #6043

Merged
merged 1 commit into from
Oct 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/libs/Log.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ let timeout = null;
*/
function serverLoggingCallback(logger, params) {
const requestParams = params;
requestParams.shouldProcessImmediately = false;
requestParams.expensifyCashAppVersion = `expensifyCash[${getPlatform()}]${version}`;
if (requestParams.parameters) {
requestParams.parameters = JSON.stringify(params.parameters);
Expand Down
20 changes: 17 additions & 3 deletions src/libs/Network.js
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,14 @@ function processNetworkRequestQueue() {
// Process our write queue very often
setInterval(processNetworkRequestQueue, 1000);

/**
* @param {Object} request
* @returns {Boolean}
*/
function canProcessRequestImmediately(request) {
return lodashGet(request, 'data.shouldProcessImmediately', true);
}

/**
* Perform a queued post request
*
Expand All @@ -243,15 +251,21 @@ setInterval(processNetworkRequestQueue, 1000);
*/
function post(command, data = {}, type = CONST.NETWORK.METHOD.POST, shouldUseSecure = false) {
return new Promise((resolve, reject) => {
// Add the write request to a queue of actions to perform
networkRequestQueue.push({
const request = {
command,
data,
type,
resolve,
reject,
shouldUseSecure,
});
};

// Add the request to a queue of actions to perform
networkRequestQueue.push(request);

if (!canProcessRequestImmediately(request)) {
return;
}

// Try to fire off the request as soon as it's queued so we don't add a delay to every queued command
processNetworkRequestQueue();
Expand Down
42 changes: 42 additions & 0 deletions tests/actions/ReportTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {addAction, togglePinnedState, subscribeToUserEvents} from '../../src/lib
import waitForPromisesToResolve from '../utils/waitForPromisesToResolve';
import PushNotification from '../../src/libs/Notification/PushNotification';
import {signInWithTestUser, fetchPersonalDetailsForTestUser} from '../utils/TestHelper';
import Log from '../../src/libs/Log';

PushNotification.register = () => {};
PushNotification.deregister = () => {};
Expand Down Expand Up @@ -165,4 +166,45 @@ describe('actions/Report', () => {
expect(reportIsPinned).toEqual(true);
});
});

it('Should not leave duplicate comments when logger sends packet because of calling process queue while processing the queue', () => {
const TEST_USER_ACCOUNT_ID = 1;
const TEST_USER_LOGIN = 'test@test.com';
const REPORT_ID = 1;
const LOGGER_MAX_LOG_LINES = 50;

// GIVEN a test user with initial data
return signInWithTestUser(TEST_USER_ACCOUNT_ID, TEST_USER_LOGIN)
.then(() => fetchPersonalDetailsForTestUser(TEST_USER_ACCOUNT_ID, TEST_USER_LOGIN, {
[TEST_USER_LOGIN]: {
accountID: TEST_USER_ACCOUNT_ID,
email: TEST_USER_LOGIN,
firstName: 'Test',
lastName: 'User',
},
}))
.then(() => {
global.fetch = jest.fn()
.mockImplementation(() => Promise.resolve({
json: () => Promise.resolve({
jsonCode: 200,
}),
}));

// WHEN we add 1 less than the max before a log packet is sent
for (let i = 0; i < LOGGER_MAX_LOG_LINES; i++) {
Log.info('Test log info');
}

// And leave a comment on a report which will trigger the log packet to be sent in the same call
addAction(REPORT_ID, 'Testing a comment');
return waitForPromisesToResolve();
})
.then(() => {
// THEN only ONE call to Report_AddComment will happen
const URL_ARGUMENT_INDEX = 0;
const reportAddCommentCalls = _.filter(global.fetch.mock.calls, callArguments => callArguments[URL_ARGUMENT_INDEX].includes('Report_AddComment'));
expect(reportAddCommentCalls.length).toBe(1);
});
});
});
10 changes: 5 additions & 5 deletions tests/utils/TestHelper.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,21 +60,21 @@ function signInWithTestUser(accountID, login, password = 'Password1', authToken
* @returns {Promise}
*/
function fetchPersonalDetailsForTestUser(accountID, email, personalDetailsList) {
// Mock xhr()
const originalXHR = HttpUtils.xhr;
HttpUtils.xhr = jest.fn();

// Get the personalDetails
HttpUtils.xhr

// fetchPersonalDetails
.mockImplementationOnce(() => Promise.resolve({
accountID,
email,
personalDetailsList,
}));

fetchPersonalDetails();
return waitForPromisesToResolve();
return waitForPromisesToResolve()
.then(() => {
HttpUtils.xhr = originalXHR;
});
}

export {
Expand Down