From 0b0616d6102ada4ed2b2f35fe3c404a7fefdbb9f Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Mon, 6 Jun 2022 10:01:37 -1000 Subject: [PATCH 1/4] Move the notification logic into its own method Use timestamp based solution to determine if we need to send a notification set notificationPreference in report object Use notification preference from Pusher for now improve comment Get rid of sendLocalNotification and just perform directly in Onyx.connect change name to viewNewReportAction Make a better diff Fix tests --- src/libs/actions/Report.js | 201 ++++++++++++++++++++---------------- tests/actions/ReportTest.js | 2 + 2 files changed, 116 insertions(+), 87 deletions(-) diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index 1dd7a21b4685..a4aec02b5e5c 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -591,42 +591,20 @@ function updateReportWithNewAction( reportAction, notificationPreference = CONST.REPORT.NOTIFICATION_PREFERENCE.ALWAYS, ) { - const newMaxSequenceNumber = reportAction.sequenceNumber; - const isFromCurrentUser = reportAction.actorAccountID === currentUserAccountID; - const initialLastReadSequenceNumber = lastReadSequenceNumbers[reportID] || 0; + const messageText = reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.RENAMED + ? lodashGet(reportAction, 'originalMessage.html', '') + : lodashGet(reportAction, ['message', 0, 'text'], ''); - // When handling an action from the current users we can assume that their - // last read actionID has been updated in the server but not necessarily reflected - // locally so we must first update it and then calculate the unread (which should be 0) - if (isFromCurrentUser) { - setLocalLastRead(reportID, newMaxSequenceNumber); - } - - let messageText = lodashGet(reportAction, ['message', 0, 'text'], ''); - if (reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.RENAMED) { - messageText = lodashGet(reportAction, 'originalMessage.html', ''); - } - - // Always merge the reportID into Onyx - // If the report doesn't exist in Onyx yet, then all the rest of the data will be filled out - // by handleReportChanged const updatedReportObject = { + // Always merge the reportID into Onyx. If the report doesn't exist in Onyx yet, then all the rest of the data will be filled out by handleReportChanged reportID, - - // Use updated lastReadSequenceNumber, value may have been modified by setLocalLastRead - // Here deletedComments count does not include the new action being added. We can safely assume that newly received action is not deleted. - unreadActionCount: newMaxSequenceNumber - (lastReadSequenceNumbers[reportID] || 0) - ReportActions.getDeletedCommentsCount(reportID, lastReadSequenceNumbers[reportID] || 0), maxSequenceNumber: reportAction.sequenceNumber, + notificationPreference, + lastMessageTimestamp: reportAction.timestamp, + lastMessageText: ReportUtils.formatReportLastMessageText(messageText), + lastActorEmail: reportAction.actorEmail, }; - // If the report action from pusher is a higher sequence number than we know about (meaning it has come from - // a chat participant in another application), then the last message text and author needs to be updated as well - if (newMaxSequenceNumber > initialLastReadSequenceNumber) { - updatedReportObject.lastMessageTimestamp = reportAction.timestamp; - updatedReportObject.lastMessageText = ReportUtils.formatReportLastMessageText(messageText); - updatedReportObject.lastActorEmail = reportAction.actorEmail; - } - Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, updatedReportObject); const reportActionsToMerge = {}; @@ -644,63 +622,6 @@ function updateReportWithNewAction( }; Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, reportActionsToMerge); - - // If chat report receives an action with IOU and we have an IOUReportID, update IOU object - if (reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.IOU && reportAction.originalMessage.IOUReportID) { - const iouReportID = reportAction.originalMessage.IOUReportID; - - // If the IOUDetails object exists we are in the Send Money flow, and we should not fetch and update the chatReport - // as this would overwrite any existing IOUs. For all other cases we must update the chatReport with the iouReportID as - // if we don't, new IOUs would not be displayed and paid IOUs would still show as unpaid. - if (reportAction.originalMessage.IOUDetails === undefined) { - fetchIOUReportByIDAndUpdateChatReport(iouReportID, reportID); - } - } - - if (!ActiveClientManager.isClientTheLeader()) { - Log.info('[LOCAL_NOTIFICATION] Skipping notification because this client is not the leader'); - return; - } - - // We don't want to send a local notification if the user preference is daily or mute - if (notificationPreference === 'mute' || notificationPreference === 'daily') { - // eslint-disable-next-line max-len - Log.info(`[LOCAL_NOTIFICATION] No notification because user preference is to be notified: ${notificationPreference}`); - return; - } - - // If this comment is from the current user we don't want to parrot whatever they wrote back to them. - if (isFromCurrentUser) { - Log.info('[LOCAL_NOTIFICATION] No notification because comment is from the currently logged in user'); - return; - } - - // If we are currently viewing this report do not show a notification. - if (reportID === lastViewedReportID && Visibility.isVisible()) { - Log.info('[LOCAL_NOTIFICATION] No notification because it was a comment for the current report'); - return; - } - - // If the comment came from Concierge let's not show a notification since we already show one for expensify.com - if (lodashGet(reportAction, 'actorEmail') === CONST.EMAIL.CONCIERGE) { - return; - } - - // When a new message comes in, if the New marker is not already set (newMarkerSequenceNumber === 0), set the - // marker above the incoming message. - if (lodashGet(allReports, [reportID, 'newMarkerSequenceNumber'], 0) === 0 - && updatedReportObject.unreadActionCount > 0) { - const oldestUnreadSeq = (updatedReportObject.maxSequenceNumber - updatedReportObject.unreadActionCount) + 1; - setNewMarkerPosition(reportID, oldestUnreadSeq); - } - Log.info('[LOCAL_NOTIFICATION] Creating notification'); - LocalNotification.showCommentNotification({ - reportAction, - onClick: () => { - // Navigate to this report onClick - Navigation.navigate(ROUTES.getReportRoute(reportID)); - }, - }); } /** @@ -1543,6 +1464,112 @@ function renameReport(reportID, reportName) { .finally(() => Onyx.set(ONYXKEYS.IS_LOADING_RENAME_POLICY_ROOM, false)); } +/** + * @param {Number} reportID + * @param {Object} action + */ +function viewNewReportAction(reportID, action) { + const newMaxSequenceNumber = action.sequenceNumber; + const isFromCurrentUser = action.actorAccountID === currentUserAccountID; + + // When handling an action from the current users we can assume that their + // last read actionID has been updated in the server but not necessarily reflected + // locally so we must first update it and then calculate the unread (which should be 0) + if (isFromCurrentUser) { + setLocalLastRead(reportID, newMaxSequenceNumber); + } + + const updatedReportObject = { + // Use updated lastReadSequenceNumber, value may have been modified by setLocalLastRead + // Here deletedComments count does not include the new action being added. We can safely assume that newly received action is not deleted. + unreadActionCount: newMaxSequenceNumber - (lastReadSequenceNumbers[reportID] || 0) - ReportActions.getDeletedCommentsCount(reportID, lastReadSequenceNumbers[reportID] || 0), + }; + + Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, updatedReportObject); + + // If chat report receives an action with IOU and we have an IOUReportID, update IOU object + if (action.actionName === CONST.REPORT.ACTIONS.TYPE.IOU && action.originalMessage.IOUReportID) { + const iouReportID = action.originalMessage.IOUReportID; + + // If the IOUDetails object exists we are in the Send Money flow, and we should not fetch and update the chatReport + // as this would overwrite any existing IOUs. For all other cases we must update the chatReport with the iouReportID as + // if we don't, new IOUs would not be displayed and paid IOUs would still show as unpaid. + if (action.originalMessage.IOUDetails === undefined) { + fetchIOUReportByIDAndUpdateChatReport(iouReportID, reportID); + } + } + + const notificationPreference = lodashGet(allReports, [reportID, 'notificationPreference'], CONST.REPORT.NOTIFICATION_PREFERENCE.ALWAYS); + if (!ActiveClientManager.isClientTheLeader()) { + Log.info('[LOCAL_NOTIFICATION] Skipping notification because this client is not the leader'); + return; + } + + // We don't want to send a local notification if the user preference is daily or mute + if (notificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.ALWAYS || notificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.DAILY) { + Log.info(`[LOCAL_NOTIFICATION] No notification because user preference is to be notified: ${notificationPreference}`); + return; + } + + // If this comment is from the current user we don't want to parrot whatever they wrote back to them. + if (isFromCurrentUser) { + Log.info('[LOCAL_NOTIFICATION] No notification because comment is from the currently logged in user'); + return; + } + + // If we are currently viewing this report do not show a notification. + if (reportID === lastViewedReportID && Visibility.isVisible()) { + Log.info('[LOCAL_NOTIFICATION] No notification because it was a comment for the current report'); + return; + } + + // If the comment came from Concierge let's not show a notification since we already show one for expensify.com + if (lodashGet(action, 'actorEmail') === CONST.EMAIL.CONCIERGE) { + return; + } + + // When a new message comes in, if the New marker is not already set (newMarkerSequenceNumber === 0), set the marker above the incoming message. + const report = lodashGet(allReports, 'reportID', {}); + if (lodashGet(report, 'newMarkerSequenceNumber', 0) === 0 && report.unreadActionCount > 0) { + const oldestUnreadSeq = (report.maxSequenceNumber - report.unreadActionCount) + 1; + setNewMarkerPosition(reportID, oldestUnreadSeq); + } + + Log.info('[LOCAL_NOTIFICATION] Creating notification'); + LocalNotification.showCommentNotification({ + reportAction: action, + onClick: () => { + // Navigate to this report onClick + Navigation.navigate(ROUTES.getReportRoute(reportID)); + }, + }); +} + +Onyx.connect({ + key: ONYXKEYS.COLLECTION.REPORT_ACTIONS, + initWithStoredValues: false, + callback: (actions, key) => { + // reportID can be derived from the Onyx key + const reportID = parseInt(key.split('_')[1], 10); + if (!reportID) { + return; + } + + _.each(actions, (action) => { + if (!action.timestamp) { + return; + } + + // If we are past the deadline to notify for this comment don't do it + if (moment.utc(action.timestamp * 1000).isBefore(moment.utc().subtract(10, 'seconds'))) { + return; + } + + viewNewReportAction(reportID, action); + }); + }, +}); + export { fetchAllReports, fetchActions, diff --git a/tests/actions/ReportTest.js b/tests/actions/ReportTest.js index 7d1c569603ed..f0a4fda3c6a0 100644 --- a/tests/actions/ReportTest.js +++ b/tests/actions/ReportTest.js @@ -47,6 +47,8 @@ describe('actions/Report', () => { }); it('should store a new report action in Onyx when reportComment event is handled via Pusher', () => { + global.fetch = TestHelper.getGlobalFetchMock(); + const TEST_USER_ACCOUNT_ID = 1; const TEST_USER_LOGIN = 'test@test.com'; const REPORT_ID = 1; From 71152fe719ba9047756da4042c321109702fe985 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Mon, 6 Jun 2022 15:58:11 -1000 Subject: [PATCH 2/4] woopsies --- src/libs/actions/Report.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index a4aec02b5e5c..e73b32004bde 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -1506,7 +1506,7 @@ function viewNewReportAction(reportID, action) { } // We don't want to send a local notification if the user preference is daily or mute - if (notificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.ALWAYS || notificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.DAILY) { + if (notificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.MUTE || notificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.DAILY) { Log.info(`[LOCAL_NOTIFICATION] No notification because user preference is to be notified: ${notificationPreference}`); return; } From 856015abb275dc2b4e9324fb27ed0fe3caa15944 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Tue, 7 Jun 2022 05:38:28 -1000 Subject: [PATCH 3/4] Use hasAttemptedToNotify --- src/libs/actions/Report.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index e73b32004bde..921fb7c28bea 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -1469,6 +1469,10 @@ function renameReport(reportID, reportName) { * @param {Object} action */ function viewNewReportAction(reportID, action) { + if (action.hasAttemptedToNotify) { + return; + } + const newMaxSequenceNumber = action.sequenceNumber; const isFromCurrentUser = action.actorAccountID === currentUserAccountID; @@ -1486,6 +1490,7 @@ function viewNewReportAction(reportID, action) { }; Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, updatedReportObject); + Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, {hasAttemptedToNotify: true}); // If chat report receives an action with IOU and we have an IOUReportID, update IOU object if (action.actionName === CONST.REPORT.ACTIONS.TYPE.IOU && action.originalMessage.IOUReportID) { From a6910e7de872e8752e59a80df61ebd88f374fe57 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Tue, 7 Jun 2022 06:17:33 -1000 Subject: [PATCH 4/4] remove hasAttemptedToNotify because it doesnt seem to work --- src/libs/actions/Report.js | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index 921fb7c28bea..e73b32004bde 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -1469,10 +1469,6 @@ function renameReport(reportID, reportName) { * @param {Object} action */ function viewNewReportAction(reportID, action) { - if (action.hasAttemptedToNotify) { - return; - } - const newMaxSequenceNumber = action.sequenceNumber; const isFromCurrentUser = action.actorAccountID === currentUserAccountID; @@ -1490,7 +1486,6 @@ function viewNewReportAction(reportID, action) { }; Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, updatedReportObject); - Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, {hasAttemptedToNotify: true}); // If chat report receives an action with IOU and we have an IOUReportID, update IOU object if (action.actionName === CONST.REPORT.ACTIONS.TYPE.IOU && action.originalMessage.IOUReportID) {