-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Notifications for editing money requests #27299
Changes from all commits
d0b45ec
93a5891
cabe7a5
ada2432
d74bc4e
1cc8736
b6be992
6db3f48
0223dc5
127c0a0
5f80689
9ed14a9
eceb13f
f6c2b2a
63ecfa2
682be4a
b173012
b8791c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -128,6 +128,16 @@ export default { | |
}); | ||
}, | ||
|
||
pushModifiedExpenseNotification({reportAction, onClick}, usesIcon = false) { | ||
push({ | ||
title: _.map(reportAction.person, (f) => f.text).join(', '), | ||
body: ReportUtils.getModifiedExpenseMessage(reportAction), | ||
delay: 0, | ||
onClick, | ||
icon: usesIcon ? EXPENSIFY_ICON_URL : '', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what kinds of notifications don't use the icon? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Web does here for comment notifications but desktop does not. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NAB, just sort of noticing that this module is formatted a bit differently than others in that it's missing JSDocs and the methods come after the export keyword. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeahh... I'd change it but I'm trying not to make this PR bigger than it already is 😅. Maybe just wait for the TS migration on this lib to clean it all up? |
||
}); | ||
}, | ||
|
||
/** | ||
* Create a notification to indicate that an update is available. | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1542,6 +1542,9 @@ function getProperSchemaForModifiedExpenseMessage(newValue, oldValue, valueName, | |
/** | ||
* Get the report action message when expense has been modified. | ||
* | ||
* ModifiedExpense::getNewDotComment in Web-Expensify should match this. | ||
* If we change this function be sure to update the backend as well. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NAB, this comment is well intentioned, but I wonder if we should take any additional steps besides this reminder. Another thought, this kind of applies to basically any API call we have. Anytime the schema of the data passed to the frontend changes there is a risk of things breaking or being inconsistent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking the best next step is to actually get rid of this function and only format the message in Web-E. I can make a follow-up issue to address it. |
||
* | ||
arosiclair marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* @param {Object} reportAction | ||
* @returns {String} | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1627,9 +1627,9 @@ function shouldShowReportActionNotification(reportID, action = null, isRemote = | |
return false; | ||
} | ||
|
||
// Don't show a notification if no comment exists | ||
if (action && !_.some(action.message, (f) => f.type === 'COMMENT')) { | ||
Log.info(`${tag} No notification because no comments exist for the current action`); | ||
// Only show notifications for supported types of report actions | ||
if (!ReportActionsUtils.isNotifiableReportAction(action)) { | ||
Log.info(`${tag} No notification because this action type is not supported`, false, {actionName: action.actionName}); | ||
Comment on lines
+1631
to
+1632
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is causing some crashes:
If the action is not defined, we still proceed to the the block and we try to access the action.actionName which will cause crash There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. created a quick PR for this here #28880 |
||
return false; | ||
} | ||
|
||
|
@@ -1638,23 +1638,28 @@ function shouldShowReportActionNotification(reportID, action = null, isRemote = | |
|
||
/** | ||
* @param {String} reportID | ||
* @param {Object} action | ||
* @param {Object} reportAction | ||
*/ | ||
function showReportActionNotification(reportID, action) { | ||
if (!shouldShowReportActionNotification(reportID, action)) { | ||
function showReportActionNotification(reportID, reportAction) { | ||
if (!shouldShowReportActionNotification(reportID, reportAction)) { | ||
return; | ||
} | ||
|
||
Log.info('[LocalNotification] Creating notification'); | ||
LocalNotification.showCommentNotification({ | ||
report: allReports[reportID], | ||
reportAction: action, | ||
onClick: () => { | ||
// Navigate to this report onClick | ||
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(reportID)); | ||
}, | ||
}); | ||
notifyNewAction(reportID, action.actorAccountID, action.reportActionID); | ||
const report = allReports[reportID]; | ||
|
||
const notificationParams = { | ||
report, | ||
reportAction, | ||
onClick: () => Navigation.navigate(ROUTES.getReportRoute(reportID)), | ||
}; | ||
Comment on lines
-1652
to
+1655
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This caused a regression #28328. We should have kept using (Seems like a merge conflict) |
||
if (reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.MODIFIEDEXPENSE) { | ||
LocalNotification.showModifiedExpenseNotification(notificationParams); | ||
} else { | ||
LocalNotification.showCommentNotification(notificationParams); | ||
} | ||
|
||
notifyNewAction(reportID, reportAction.actorAccountID, reportAction.reportActionID); | ||
} | ||
|
||
/** | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know a ton about notifications, can you explain why we use the backend message over the Onyx data? Are they different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup for the same reason as here. The text in the report action isn't quite formatted the same as it is in
getModifiedExpenseMessage