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.

Sign up for GitHub

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

Jump to bottom

[No QA] Add support for Pusher responses #9219

Merged
merged 12 commits into from
Jun 2, 2022
Merged

[No QA] Add support for Pusher responses #9219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
0658ff1
Move subscribeToPrivateUserChannelEvent to Pusher
Gonals May 27, 2022
5445a6a
add type
Gonals May 27, 2022
729eeee
subscribe to the event in User
Gonals May 30, 2022
4689132
quotes
Gonals May 30, 2022
a3b3879
handle chunks
Gonals May 30, 2022
2ac4921
remove extra code
Gonals May 30, 2022
66e9256
remove console line
Gonals May 30, 2022
a25bd56
remove debug lines
Gonals May 30, 2022
ee589cf
merge main
Gonals Jun 2, 2022
29ad302
Move function to PusherUtils
Gonals Jun 2, 2022
089326d
add newline
Gonals Jun 2, 2022
6977583
add const
Gonals Jun 2, 2022
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
4 changes: 4 additions & 0 deletions src/CONST.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,10 @@ const CONST = {
},
},

PUSHER: {
PRIVATE_USER_CHANNEL_PREFIX: 'private-encrypted-user-accountID-',
},

EMOJI_SPACER: 'SPACER',

EMOJI_NUM_PER_ROW: 8,
Expand Down
19 changes: 18 additions & 1 deletion src/libs/Network/SequentialQueue.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ resolveIsReadyPromise();

let isSequentialQueueRunning = false;

let currentRequest = null;

/**
* This method will get any persisted requests and fire them off in sequence to retry them.
*
Expand All @@ -29,7 +31,10 @@ function process() {
return Promise.resolve();
}

const task = _.reduce(persistedRequests, (previousRequest, request) => previousRequest.then(() => Request.processWithMiddleware(request, true)), Promise.resolve());
const task = _.reduce(persistedRequests, (previousRequest, request) => previousRequest.then(() => {
currentRequest = Request.processWithMiddleware(request, true);
return currentRequest;
}), Promise.resolve());

// Do a recursive call in case the queue is not empty after processing the current batch
return task.then(process);
Expand Down Expand Up @@ -62,6 +67,7 @@ function flush() {
.finally(() => {
isSequentialQueueRunning = false;
resolveIsReadyPromise();
currentRequest = null;
});
},
});
Expand Down Expand Up @@ -98,8 +104,19 @@ function push(request) {
flush();
}

/**
* @returns {Promise}
*/
function getCurrentRequest() {
if (currentRequest === null) {
return Promise.resolve();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAB, could also initialize/reset with a Promise.resolve() but this works.

return currentRequest;
}

export {
flush,
getCurrentRequest,
isRunning,
push,
};
2 changes: 2 additions & 0 deletions src/libs/Pusher/EventType.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,6 @@ export default {
PREFERRED_LOCALE: 'preferredLocale',
EXPENSIFY_CARD_UPDATE: 'expensifyCardUpdate',
SCREEN_SHARE_REQUEST: 'screenshareRequest',
ONYX_API_UPDATE: 'onyxApiUpdate',
ONYX_API_UPDATE_CHUNK: 'chunked-onyxApiUpdate',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After these changes we won't need the prepend chunked to channel names.

cc @danieldoglas to confirm

Copy link
Contributor Author

@Gonals Gonals Jun 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice! That one seems fully approved, so it should be merging today, right @alex-mechler? I can update this then :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just self merged it, since all reviewers had approved!

That said, we still need the chunked- channel to be listened to for now, since Web-E will still be sending them until we make the changes to stop sending them. That is HELD on the linked PR getting deployed to production.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can move ahead with this PR as-is and remove that afterwards?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, that sounds fine. I have to remove the other chunked- channels later from this repo anyways.

};
49 changes: 49 additions & 0 deletions src/libs/PusherUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import CONFIG from '../CONFIG';
import Log from './Log';
import NetworkConnection from './NetworkConnection';
import * as Pusher from './Pusher/pusher';
import CONST from '../CONST';

/**
* Abstraction around subscribing to private user channel events. Handles all logs and errors automatically.
*
* @param {String} eventName
* @param {String} accountID
* @param {Function} onEvent
* @param {Boolean} isChunked
*/
function subscribeToPrivateUserChannelEvent(eventName, accountID, onEvent, isChunked = false) {
const pusherChannelName = `${CONST.PUSHER.PRIVATE_USER_CHANNEL_PREFIX}${accountID}${CONFIG.PUSHER.SUFFIX}`;

/**
* @param {Object} pushJSON
*/
function logPusherEvent(pushJSON) {
Log.info(`[Report] Handled ${eventName} event sent by Pusher`, false, pushJSON);
}

function onPusherResubscribeToPrivateUserChannel() {
NetworkConnection.triggerReconnectionCallbacks('Pusher re-subscribed to private user channel');
}

/**
* @param {*} pushJSON
*/
function onEventPush(pushJSON) {
logPusherEvent(pushJSON);
onEvent(pushJSON);
}

/**
* @param {*} error
*/
function onSubscriptionFailed(error) {
Log.hmmm('Failed to subscribe to Pusher channel', false, {error, pusherChannelName, eventName});
}
Pusher.subscribe(pusherChannelName, eventName, onEventPush, isChunked, onPusherResubscribeToPrivateUserChannel)
.catch(onSubscriptionFailed);
}

export default {
subscribeToPrivateUserChannelEvent,
};
69 changes: 24 additions & 45 deletions src/libs/actions/Report.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import Navigation from '../Navigation/Navigation';
import * as ActiveClientManager from '../ActiveClientManager';
import Visibility from '../Visibility';
import ROUTES from '../../ROUTES';
import NetworkConnection from '../NetworkConnection';
import Timing from './Timing';
import * as DeprecatedAPI from '../deprecatedAPI';
import CONFIG from '../../CONFIG';
Expand All @@ -27,6 +26,7 @@ import Timers from '../Timers';
import * as ReportActions from './ReportActions';
import Growl from '../Growl';
import * as Localize from '../Localize';
import PusherUtils from '../PusherUtils';

let currentUserEmail;
let currentUserAccountID;
Expand Down Expand Up @@ -723,45 +723,6 @@ function getReportChannelName(reportID) {
return `private-report-reportID-${reportID}${CONFIG.PUSHER.SUFFIX}`;
}

/**
* Abstraction around subscribing to private user channel events. Handles all logs and errors automatically.
*
* @param {String} eventName
* @param {Function} onEvent
*/
function subscribeToPrivateUserChannelEvent(eventName, onEvent) {
const pusherChannelName = `private-encrypted-user-accountID-${currentUserAccountID}${CONFIG.PUSHER.SUFFIX}`;

/**
* @param {Object} pushJSON
*/
function logPusherEvent(pushJSON) {
Log.info(`[Report] Handled ${eventName} event sent by Pusher`, false, {reportID: pushJSON.reportID, reportActionID: pushJSON.reportActionID});
}

function onPusherResubscribeToPrivateUserChannel() {
NetworkConnection.triggerReconnectionCallbacks('Pusher re-subscribed to private user channel');
}

/**
* @param {*} pushJSON
*/
function onEventPush(pushJSON) {
logPusherEvent(pushJSON);
onEvent(pushJSON);
}

/**
* @param {*} error
*/
function onSubscriptionFailed(error) {
Log.hmmm('[Report] Failed to subscribe to Pusher channel', false, {error, pusherChannelName, eventName});
}

Pusher.subscribe(pusherChannelName, eventName, onEventPush, onPusherResubscribeToPrivateUserChannel)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here there was not the isChunked but in the reportUtils it is back.

.catch(onSubscriptionFailed);
}

/**
* Initialize our pusher subscriptions to listen for new report comments and pin toggles
*/
Expand All @@ -777,19 +738,37 @@ function subscribeToUserEvents() {
}

// Live-update a report's actions when a 'report comment' event is received.
subscribeToPrivateUserChannelEvent(Pusher.TYPE.REPORT_COMMENT, pushJSON => updateReportWithNewAction(pushJSON.reportID, pushJSON.reportAction, pushJSON.notificationPreference));
PusherUtils.subscribeToPrivateUserChannelEvent(
Pusher.TYPE.REPORT_COMMENT,
currentUserAccountID,
pushJSON => updateReportWithNewAction(pushJSON.reportID, pushJSON.reportAction, pushJSON.notificationPreference),
);

// Live-update a report's actions when a 'chunked report comment' event is received.
subscribeToPrivateUserChannelEvent(Pusher.TYPE.REPORT_COMMENT_CHUNK, pushJSON => updateReportWithNewAction(pushJSON.reportID, pushJSON.reportAction, pushJSON.notificationPreference));
PusherUtils.subscribeToPrivateUserChannelEvent(
Pusher.TYPE.REPORT_COMMENT_CHUNK,
currentUserAccountID,
pushJSON => updateReportWithNewAction(pushJSON.reportID, pushJSON.reportAction, pushJSON.notificationPreference),
true,
);

// Live-update a report's actions when an 'edit comment' event is received.
subscribeToPrivateUserChannelEvent(Pusher.TYPE.REPORT_COMMENT_EDIT, pushJSON => updateReportActionMessage(pushJSON.reportID, pushJSON.sequenceNumber, pushJSON.message));
PusherUtils.subscribeToPrivateUserChannelEvent(Pusher.TYPE.REPORT_COMMENT_EDIT,
currentUserAccountID,
pushJSON => updateReportActionMessage(pushJSON.reportID, pushJSON.sequenceNumber, pushJSON.message));

// Live-update a report's actions when an 'edit comment chunk' event is received.
subscribeToPrivateUserChannelEvent(Pusher.TYPE.REPORT_COMMENT_EDIT_CHUNK, pushJSON => updateReportActionMessage(pushJSON.reportID, pushJSON.sequenceNumber, pushJSON.message));
PusherUtils.subscribeToPrivateUserChannelEvent(
Pusher.TYPE.REPORT_COMMENT_EDIT_CHUNK,
currentUserAccountID,
pushJSON => updateReportActionMessage(pushJSON.reportID, pushJSON.sequenceNumber, pushJSON.message),
true,
);

// Live-update a report's pinned state when a 'report toggle pinned' event is received.
subscribeToPrivateUserChannelEvent(Pusher.TYPE.REPORT_TOGGLE_PINNED, pushJSON => updateReportPinnedState(pushJSON.reportID, pushJSON.isPinned));
PusherUtils.subscribeToPrivateUserChannelEvent(Pusher.TYPE.REPORT_TOGGLE_PINNED,
currentUserAccountID,
pushJSON => updateReportPinnedState(pushJSON.reportID, pushJSON.isPinned));
}

/**
Expand Down
14 changes: 14 additions & 0 deletions src/libs/actions/User.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import * as Localize from '../Localize';
import * as CloseAccountActions from './CloseAccount';
import * as Link from './Link';
import getSkinToneEmojiFromIndex from '../../components/EmojiPicker/getSkinToneEmojiFromIndex';
import * as SequentialQueue from '../Network/SequentialQueue';
import PusherUtils from '../PusherUtils';

let sessionAuthToken = '';
let sessionEmail = '';
Expand Down Expand Up @@ -302,6 +304,18 @@ function subscribeToUserEvents() {

const pusherChannelName = `private-encrypted-user-accountID-${currentUserAccountID}${CONFIG.PUSHER.SUFFIX}`;

// Receive any relevant Onyx updates from the server
PusherUtils.subscribeToPrivateUserChannelEvent(Pusher.TYPE.ONYX_API_UPDATE, currentUserAccountID, (pushJSON) => {
SequentialQueue.getCurrentRequest().then(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey everyone 👋🏼 I know it's been a little while, but can someone explain why we needed to add SequentialQueue.getCurrentRequest() here? It's not clear to me what it's doing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first I heard about it you were the one proposing it 😄

https://expensify.slack.com/archives/C02HWMSMZEC/p1652731618392499?thread_ts=1652466984.820589&cid=C02HWMSMZEC

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I understand why this was added now, I tried to summarize it as concisely as possible here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was always a longer goal of mine to add a NETWORK.md and that explanation would be perfect for it (or added as a comment here).

Onyx.update(pushJSON);
});
});
PusherUtils.subscribeToPrivateUserChannelEvent(Pusher.TYPE.ONYX_API_UPDATE_CHUNK, currentUserAccountID, (pushJSON) => {
SequentialQueue.getCurrentRequest().then(() => {
Onyx.update(pushJSON.onyxData);
});
}, true);

// Live-update an user's preferred locale
Pusher.subscribe(pusherChannelName, Pusher.TYPE.PREFERRED_LOCALE, (pushJSON) => {
Onyx.merge(ONYXKEYS.NVP_PREFERRED_LOCALE, pushJSON.preferredLocale);
Expand Down