From 76b4af8d13e74511512b7e8d79b4994a112651df Mon Sep 17 00:00:00 2001 From: Andrew Rosiclair Date: Tue, 17 Jan 2023 13:41:10 -0500 Subject: [PATCH 1/6] better local notification logging --- src/libs/actions/Report.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index 3cbb5a2b0150..c7743d39090e 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -1092,6 +1092,7 @@ function subscribeToNewActionEvent(reportID, callback) { */ function showReportActionNotification(reportID, action) { if (ReportActionsUtils.isDeletedAction(action)) { + Log.info('[LOCAL_NOTIFICATION] Skipping notification because the action was deleted', false, {reportID, action}); return; } @@ -1115,7 +1116,7 @@ function showReportActionNotification(reportID, action) { // If we are currently viewing this report do not show a notification. if (reportID === Navigation.getReportIDFromRoute() && Visibility.isVisible()) { - Log.info('[LOCAL_NOTIFICATION] No notification because it was a comment for the current report'); + Log.info('[LOCAL_NOTIFICATION] No notification because it was a comment for the current report', false, {currentReport: Navigation.getReportIDFromRoute(), reportID, action}); return; } @@ -1126,7 +1127,7 @@ function showReportActionNotification(reportID, action) { // Don't show a notification if no comment exists if (!_.some(action.message, f => f.type === 'COMMENT')) { - Log.info('[LOCAL_NOTIFICATION] No notification because no comments exist for the current report'); + Log.info('[LOCAL_NOTIFICATION] No notification because no comments exist for the current action'); return; } From e9aa9095adaf8990c61aac6f20f01c38bcea4ef6 Mon Sep 17 00:00:00 2001 From: Andrew Rosiclair Date: Tue, 17 Jan 2023 13:42:00 -0500 Subject: [PATCH 2/6] notify ReportActionsView for locally added comments --- src/libs/actions/Report.js | 48 +++++++++++++++++++++----------------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index c7743d39090e..78faeaa3e3a9 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -163,6 +163,27 @@ function unsubscribeFromReportChannel(reportID) { Pusher.unsubscribe(pusherChannelName); } +const defaultNewActionSubscriber = { + reportID: '', + callback: () => {}, +}; + +let newActionSubscriber = defaultNewActionSubscriber; + +/** + * Enables the Report actions file to let the ReportActionsView know that a new comment has arrived in realtime for the current report + * + * @param {String} reportID + * @param {Function} callback + * @returns {Function} + */ +function subscribeToNewActionEvent(reportID, callback) { + newActionSubscriber = {callback, reportID}; + return () => { + newActionSubscriber = defaultNewActionSubscriber; + }; +} + /** * Add up to two report actions to a report. This method can be called for the following situations: * @@ -265,6 +286,12 @@ function addActions(reportID, text = '', file) { API.write(commandName, parameters, { optimisticData, }); + + // Notify the ReportActionsView that a new comment has arrived + if (reportID === newActionSubscriber.reportID) { + const isFromCurrentUser = lastAction.actorAccountID === currentUserAccountID; + newActionSubscriber.callback(isFromCurrentUser, lastAction.reportActionID); + } } /** @@ -1065,27 +1092,6 @@ function setIsComposerFullSize(reportID, isComposerFullSize) { Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_IS_COMPOSER_FULL_SIZE}${reportID}`, isComposerFullSize); } -const defaultNewActionSubscriber = { - reportID: '', - callback: () => {}, -}; - -let newActionSubscriber = defaultNewActionSubscriber; - -/** - * Enables the Report actions file to let the ReportActionsView know that a new comment has arrived in realtime for the current report - * - * @param {String} reportID - * @param {Function} callback - * @returns {Function} - */ -function subscribeToNewActionEvent(reportID, callback) { - newActionSubscriber = {callback, reportID}; - return () => { - newActionSubscriber = defaultNewActionSubscriber; - }; -} - /** * @param {String} reportID * @param {Object} action From 9ba65fd2cd25f56bde87aed4e16778f9c9605294 Mon Sep 17 00:00:00 2001 From: Andrew Rosiclair Date: Tue, 17 Jan 2023 13:43:06 -0500 Subject: [PATCH 3/6] simulate pusher message instead of direct onyx action --- tests/ui/UnreadIndicatorsTest.js | 100 +++++++++++++++++++++---------- 1 file changed, 70 insertions(+), 30 deletions(-) diff --git a/tests/ui/UnreadIndicatorsTest.js b/tests/ui/UnreadIndicatorsTest.js index 8475f8cfc999..82c05ac863ee 100644 --- a/tests/ui/UnreadIndicatorsTest.js +++ b/tests/ui/UnreadIndicatorsTest.js @@ -18,6 +18,10 @@ import LocalNotification from '../../src/libs/Notification/LocalNotification'; import * as Report from '../../src/libs/actions/Report'; import * as CollectionUtils from '../../src/libs/CollectionUtils'; import DateUtils from '../../src/libs/DateUtils'; +import * as User from '../../src/libs/actions/User'; +import * as Pusher from '../../src/libs/Pusher/pusher'; +import PusherConnectionManager from '../../src/libs/PusherConnectionManager'; +import CONFIG from '../../src/CONFIG'; jest.mock('../../src/libs/Notification/LocalNotification'); @@ -33,6 +37,21 @@ beforeAll(() => { jest.setTimeout(30000); Linking.setInitialURL('https://new.expensify.com/r/1'); appSetup(); + + // When using the Pusher mock the act of calling Pusher.isSubscribed will create a + // channel already in a subscribed state. These methods are normally used to prevent + // duplicated subscriptions, but we don't need them for this test so forcing them to + // return false will make the testing less complex. + Pusher.isSubscribed = jest.fn().mockReturnValue(false); + Pusher.isAlreadySubscribing = jest.fn().mockReturnValue(false); + + // Connect to Pusher + PusherConnectionManager.init(); + Pusher.init({ + appKey: CONFIG.PUSHER.APP_KEY, + cluster: CONFIG.PUSHER.CLUSTER, + authEndpoint: `${CONFIG.EXPENSIFY.URL_API_ROOT}api?command=AuthenticatePusher`, + }); }); /** @@ -127,6 +146,10 @@ function signInAndGetAppWithUnreadChat() { return TestHelper.signInWithTestUser(USER_A_ACCOUNT_ID, USER_A_EMAIL, undefined, undefined, 'A'); }) + .then(() => { + User.subscribeToUserEvents(); + return waitForPromisesToResolve(); + }) .then(() => { const MOMENT_TEN_MINUTES_AGO = moment().subtract(10, 'minutes'); reportAction3CreatedDate = MOMENT_TEN_MINUTES_AGO.clone().add(30, 'seconds').format(MOMENT_FORMAT); @@ -283,38 +306,55 @@ describe('Unread Indicators', () => { const NEW_REPORT_ID = '2'; const NEW_REPORT_CREATED_MOMENT = moment().subtract(5, 'seconds'); const NEW_REPORT_FIST_MESSAGE_CREATED_MOMENT = NEW_REPORT_CREATED_MOMENT.add(1, 'seconds'); - Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${NEW_REPORT_ID}`, { - reportID: NEW_REPORT_ID, - reportName: CONST.REPORT.DEFAULT_REPORT_NAME, - maxSequenceNumber: 1, - lastReadSequenceNumber: 0, - lastReadTime: '', - lastActionCreated: DateUtils.getDBTime(NEW_REPORT_FIST_MESSAGE_CREATED_MOMENT.utc().valueOf()), - lastMessageText: 'Comment 1', - participants: [USER_C_EMAIL], - }); - Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${NEW_REPORT_ID}`, { - 0: { - actionName: CONST.REPORT.ACTIONS.TYPE.CREATED, - automatic: false, - sequenceNumber: 0, - created: NEW_REPORT_CREATED_MOMENT.format(MOMENT_FORMAT), - reportActionID: NumberUtils.rand64(), + + const channel = Pusher.getChannel(`${CONST.PUSHER.PRIVATE_USER_CHANNEL_PREFIX}${USER_A_ACCOUNT_ID}${CONFIG.PUSHER.SUFFIX}`); + channel.emit(Pusher.TYPE.ONYX_API_UPDATE, [ + { + onyxMethod: CONST.ONYX.METHOD.MERGE, + key: `${ONYXKEYS.COLLECTION.REPORT}${NEW_REPORT_ID}`, + value: { + reportID: NEW_REPORT_ID, + reportName: CONST.REPORT.DEFAULT_REPORT_NAME, + maxSequenceNumber: 1, + lastReadSequenceNumber: 0, + lastReadTime: '', + lastActionCreated: DateUtils.getDBTime(NEW_REPORT_FIST_MESSAGE_CREATED_MOMENT.utc().valueOf()), + lastMessageText: 'Comment 1', + participants: [USER_C_EMAIL], + }, }, - 1: { - actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, - actorEmail: USER_C_EMAIL, - actorAccountID: USER_C_ACCOUNT_ID, - person: [{type: 'TEXT', style: 'strong', text: 'User C'}], - sequenceNumber: 1, - created: NEW_REPORT_FIST_MESSAGE_CREATED_MOMENT.format(MOMENT_FORMAT), - message: [{type: 'COMMENT', html: 'Comment 1', text: 'Comment 1'}], - reportActionID: NumberUtils.rand64(), + { + onyxMethod: CONST.ONYX.METHOD.MERGE, + key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${NEW_REPORT_ID}`, + value: { + 0: { + actionName: CONST.REPORT.ACTIONS.TYPE.CREATED, + automatic: false, + sequenceNumber: 0, + created: NEW_REPORT_CREATED_MOMENT.format(MOMENT_FORMAT), + reportActionID: NumberUtils.rand64(), + }, + 1: { + actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, + actorEmail: USER_C_EMAIL, + actorAccountID: USER_C_ACCOUNT_ID, + person: [{type: 'TEXT', style: 'strong', text: 'User C'}], + sequenceNumber: 1, + created: NEW_REPORT_FIST_MESSAGE_CREATED_MOMENT.format(MOMENT_FORMAT), + message: [{type: 'COMMENT', html: 'Comment 1', text: 'Comment 1'}], + reportActionID: NumberUtils.rand64(), + }, + }, + shouldNotify: true, }, - }); - Onyx.merge(ONYXKEYS.PERSONAL_DETAILS, { - [USER_C_EMAIL]: TestHelper.buildPersonalDetails(USER_C_EMAIL, USER_C_ACCOUNT_ID, 'C'), - }); + { + onyxMethod: CONST.ONYX.METHOD.MERGE, + key: ONYXKEYS.PERSONAL_DETAILS, + value: { + [USER_C_EMAIL]: TestHelper.buildPersonalDetails(USER_C_EMAIL, USER_C_ACCOUNT_ID, 'C'), + }, + }, + ]); return waitForPromisesToResolve(); }) .then(() => { From 37214c4ea9614105af48ee848e14be5dea22995e Mon Sep 17 00:00:00 2001 From: Andrew Rosiclair Date: Tue, 17 Jan 2023 13:50:22 -0500 Subject: [PATCH 4/6] use most recent action for notification --- src/libs/actions/User.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libs/actions/User.js b/src/libs/actions/User.js index 784a23a51320..ec61089d30ec 100644 --- a/src/libs/actions/User.js +++ b/src/libs/actions/User.js @@ -267,7 +267,8 @@ function triggerNotifications(onyxUpdates) { const reportAction = _.chain(update.value) .values() .compact() - .first() + .sort((actionA, actionB) => moment(actionA).unix() - moment(actionB).unix()) + .last() // We want to notify for the most recent action .value(); Report.showReportActionNotification(reportID, reportAction); }); From b26e0ca677e95e1e8a832f9174e0151c48b74c02 Mon Sep 17 00:00:00 2001 From: Andrew Rosiclair Date: Tue, 17 Jan 2023 15:24:23 -0500 Subject: [PATCH 5/6] be more lenient on the number of notification calls --- tests/ui/UnreadIndicatorsTest.js | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/tests/ui/UnreadIndicatorsTest.js b/tests/ui/UnreadIndicatorsTest.js index 82c05ac863ee..b2c10c890f75 100644 --- a/tests/ui/UnreadIndicatorsTest.js +++ b/tests/ui/UnreadIndicatorsTest.js @@ -38,13 +38,6 @@ beforeAll(() => { Linking.setInitialURL('https://new.expensify.com/r/1'); appSetup(); - // When using the Pusher mock the act of calling Pusher.isSubscribed will create a - // channel already in a subscribed state. These methods are normally used to prevent - // duplicated subscriptions, but we don't need them for this test so forcing them to - // return false will make the testing less complex. - Pusher.isSubscribed = jest.fn().mockReturnValue(false); - Pusher.isAlreadySubscribing = jest.fn().mockReturnValue(false); - // Connect to Pusher PusherConnectionManager.init(); Pusher.init({ @@ -208,7 +201,10 @@ function signInAndGetAppWithUnreadChat() { } describe('Unread Indicators', () => { - afterEach(() => Onyx.clear()); + afterEach(() => { + jest.clearAllMocks(); + Onyx.clear(); + }); it('Display bold in the LHN for unread chat and new line indicator above the chat message when we navigate to it', () => { let renderedApp; @@ -358,8 +354,8 @@ describe('Unread Indicators', () => { return waitForPromisesToResolve(); }) .then(() => { - // Verify notification was created as the new message that has arrived is very recent - expect(LocalNotification.showCommentNotification.mock.calls).toHaveLength(1); + // Verify notification was created + expect(LocalNotification.showCommentNotification).toBeCalled(); // // Navigate back to the sidebar return navigateToSidebar(renderedApp); From 13e5520fd61fe24733213f112fd8dfb13c2d5cde Mon Sep 17 00:00:00 2001 From: Andrew Rosiclair Date: Wed, 18 Jan 2023 17:07:49 -0500 Subject: [PATCH 6/6] fix report action sorting for notifications --- src/libs/ReportActionsUtils.js | 37 ++++++++++++++++++---------------- src/libs/actions/User.js | 11 ++++------ 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/src/libs/ReportActionsUtils.js b/src/libs/ReportActionsUtils.js index cc438b605c35..bb67b2fb39b6 100644 --- a/src/libs/ReportActionsUtils.js +++ b/src/libs/ReportActionsUtils.js @@ -50,24 +50,27 @@ function getSortedReportActions(reportActions, shouldSortInDescendingOrder = fal if (!_.isArray(reportActions)) { throw new Error(`ReportActionsUtils.getSortedReportActions requires an array, received ${typeof reportActions}`); } - const invertedMultiplier = shouldSortInDescendingOrder ? -1 : 1; - reportActions.sort((first, second) => { - // First sort by timestamp - if (first.created !== second.created) { - return (first.created < second.created ? -1 : 1) * invertedMultiplier; - } - - // Then by action type, ensuring that `CREATED` actions always come first if they have the same timestamp as another action type - if ((first.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED || second.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED) && first.actionName !== second.actionName) { - return ((first.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED) ? -1 : 1) * invertedMultiplier; - } - // Then fallback on reportActionID as the final sorting criteria. It is a random number, - // but using this will ensure that the order of reportActions with the same created time and action type - // will be consistent across all users and devices - return (first.reportActionID < second.reportActionID ? -1 : 1) * invertedMultiplier; - }); - return reportActions; + const invertedMultiplier = shouldSortInDescendingOrder ? -1 : 1; + return _.chain(reportActions) + .compact() + .sort((first, second) => { + // First sort by timestamp + if (first.created !== second.created) { + return (first.created < second.created ? -1 : 1) * invertedMultiplier; + } + + // Then by action type, ensuring that `CREATED` actions always come first if they have the same timestamp as another action type + if ((first.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED || second.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED) && first.actionName !== second.actionName) { + return ((first.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED) ? -1 : 1) * invertedMultiplier; + } + + // Then fallback on reportActionID as the final sorting criteria. It is a random number, + // but using this will ensure that the order of reportActions with the same created time and action type + // will be consistent across all users and devices + return (first.reportActionID < second.reportActionID ? -1 : 1) * invertedMultiplier; + }) + .value(); } /** diff --git a/src/libs/actions/User.js b/src/libs/actions/User.js index ec61089d30ec..09e7d6873084 100644 --- a/src/libs/actions/User.js +++ b/src/libs/actions/User.js @@ -18,6 +18,7 @@ import * as Link from './Link'; import * as SequentialQueue from '../Network/SequentialQueue'; import PusherUtils from '../PusherUtils'; import * as Report from './Report'; +import * as ReportActionsUtils from '../ReportActionsUtils'; let currentUserAccountID = ''; Onyx.connect({ @@ -264,13 +265,9 @@ function triggerNotifications(onyxUpdates) { } const reportID = update.key.replace(ONYXKEYS.COLLECTION.REPORT_ACTIONS, ''); - const reportAction = _.chain(update.value) - .values() - .compact() - .sort((actionA, actionB) => moment(actionA).unix() - moment(actionB).unix()) - .last() // We want to notify for the most recent action - .value(); - Report.showReportActionNotification(reportID, reportAction); + const reportActions = _.values(update.value); + const sortedReportActions = ReportActionsUtils.getSortedReportActions(reportActions); + Report.showReportActionNotification(reportID, _.last(sortedReportActions)); }); }