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

Notifications for editing money requests #27299

Merged
merged 18 commits into from
Sep 26, 2023
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
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,8 @@ protected NotificationCompat.Builder onExtendBuilder(@NonNull Context context, @
if (payload.containsKey(ONYX_DATA_KEY)) {
Objects.requireNonNull(payload.get(ONYX_DATA_KEY)).isNull();
Log.d(TAG, "payload contains onxyData");
applyMessageStyle(context, builder, payload, arguments.getNotificationId());
String alert = message.getExtra(PushMessage.EXTRA_ALERT);
applyMessageStyle(context, builder, payload, arguments.getNotificationId(), alert);
}
} catch (Exception e) {
Log.e(TAG, "Failed to parse conversation, falling back to default notification style. SendID=" + message.getSendId(), e);
Expand Down Expand Up @@ -163,7 +164,7 @@ public Bitmap getCroppedBitmap(Bitmap bitmap) {
* @param payload Notification payload, which contains all the data we need to build the notifications.
* @param notificationID Current notification ID
*/
private void applyMessageStyle(@NonNull Context context, NotificationCompat.Builder builder, JsonMap payload, int notificationID) {
private void applyMessageStyle(@NonNull Context context, NotificationCompat.Builder builder, JsonMap payload, int notificationID, String alert) {
long reportID = payload.get("reportID").getLong(-1);
if (reportID == -1) {
return;
Expand All @@ -181,7 +182,9 @@ private void applyMessageStyle(@NonNull Context context, NotificationCompat.Buil
String name = messageData.get("person").getList().get(0).getMap().get("text").getString();
String avatar = messageData.get("avatar").getString();
String accountID = Integer.toString(messageData.get("actorAccountID").getInt(-1));
String message = messageData.get("message").getList().get(0).getMap().get("text").getString();

// Use the formatted alert message from the backend. Otherwise fallback on the message in the Onyx data.
String message = alert != null ? alert : messageData.get("message").getList().get(0).getMap().get("text").getString();
Comment on lines +186 to +187
Copy link
Contributor

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?

Copy link
Contributor Author

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

String conversationName = payload.get("roomName") == null ? "" : payload.get("roomName").getString("");

// Retrieve or create the Person object who sent the latest report comment
Expand Down
10 changes: 10 additions & 0 deletions src/libs/Notification/LocalNotification/BrowserNotifications.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 : '',
Copy link
Contributor

Choose a reason for hiding this comment

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

what kinds of notifications don't use the icon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Web does here for comment notifications but desktop does not.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
*/
Expand Down
17 changes: 17 additions & 0 deletions src/libs/Notification/LocalNotification/index.desktop.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
import BrowserNotifications from './BrowserNotifications';

/**
* @param {Object} options
* @param {Object} options.report
* @param {Object} options.reportAction
* @param {Function} options.onClick
*/
function showCommentNotification({report, reportAction, onClick}) {
BrowserNotifications.pushReportCommentNotification({report, reportAction, onClick});
}
Expand All @@ -8,7 +14,18 @@ function showUpdateAvailableNotification() {
BrowserNotifications.pushUpdateAvailableNotification();
}

/**
* @param {Object} options
* @param {Object} options.report
* @param {Object} options.reportAction
* @param {Function} options.onClick
*/
function showModifiedExpenseNotification({report, reportAction, onClick}) {
arosiclair marked this conversation as resolved.
Show resolved Hide resolved
BrowserNotifications.pushModifiedExpenseNotification({report, reportAction, onClick});
}

export default {
showCommentNotification,
showUpdateAvailableNotification,
showModifiedExpenseNotification,
};
1 change: 1 addition & 0 deletions src/libs/Notification/LocalNotification/index.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,5 @@
export default {
showCommentNotification: () => {},
showUpdateAvailableNotification: () => {},
showModifiedExpenseNotification: () => {},
};
17 changes: 17 additions & 0 deletions src/libs/Notification/LocalNotification/index.website.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
import BrowserNotifications from './BrowserNotifications';

/**
* @param {Object} options
* @param {Object} options.report
* @param {Object} options.reportAction
* @param {Function} options.onClick
*/
function showCommentNotification({report, reportAction, onClick}) {
BrowserNotifications.pushReportCommentNotification({report, reportAction, onClick}, true);
}
Expand All @@ -8,7 +14,18 @@ function showUpdateAvailableNotification() {
BrowserNotifications.pushUpdateAvailableNotification();
}

/**
* @param {Object} options
* @param {Object} options.report
* @param {Object} options.reportAction
* @param {Function} options.onClick
*/
function showModifiedExpenseNotification({report, reportAction, onClick}) {
BrowserNotifications.pushModifiedExpenseNotification({report, reportAction, onClick}, true);
}

export default {
showCommentNotification,
showUpdateAvailableNotification,
showModifiedExpenseNotification,
};
6 changes: 6 additions & 0 deletions src/libs/ReportActionsUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -652,6 +652,11 @@ function isReportActionAttachment(reportAction) {
return _.has(reportAction, 'isAttachment') ? reportAction.isAttachment : isReportMessageAttachment(message);
}

// eslint-disable-next-line rulesdir/no-negated-variables
function isNotifiableReportAction(reportAction) {
return reportAction && _.contains([CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, CONST.REPORT.ACTIONS.TYPE.IOU, CONST.REPORT.ACTIONS.TYPE.MODIFIEDEXPENSE], reportAction.actionName);
}

export {
getSortedReportActions,
getLastVisibleAction,
Expand Down Expand Up @@ -690,4 +695,5 @@ export {
isTaskAction,
getAllReportActions,
isReportActionAttachment,
isNotifiableReportAction,
};
3 changes: 3 additions & 0 deletions src/libs/ReportUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

@arosiclair arosiclair Sep 26, 2023

Choose a reason for hiding this comment

The 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}
*/
Expand Down
35 changes: 20 additions & 15 deletions src/libs/actions/Report.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

This is causing some crashes:

Fatal Exception: com.facebook.react.common.JavascriptException: TypeError: Cannot read property 'actionName' of null, js engine: hermes, stack:
shouldShowReportActionNotification@1:3040125
shouldShowPushNotification@1:13695857
anonymous@1:13694964
anonymous@1:13688405
emit@1:162156
__callFunction@1:170848
anonymous@1:169200
__guard@1:170153
callFunctionReturnFlushedQueue@1:169158

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

Copy link
Contributor

Choose a reason for hiding this comment

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

created a quick PR for this here #28880

return false;
}

Expand All @@ -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
Copy link
Contributor

@s77rt s77rt Oct 2, 2023

Choose a reason for hiding this comment

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

This caused a regression #28328. We should have kept using ROUTES.REPORT_WITH_ID.getRoute because ROUTES.getReportRoute no longer exists.

(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);
}

/**
Expand Down
6 changes: 4 additions & 2 deletions src/libs/actions/User.js
Original file line number Diff line number Diff line change
Expand Up @@ -471,8 +471,10 @@ function triggerNotifications(onyxUpdates) {

const reportID = update.key.replace(ONYXKEYS.COLLECTION.REPORT_ACTIONS, '');
const reportActions = _.values(update.value);
const sortedReportActions = ReportActionsUtils.getSortedReportActions(reportActions);
Report.showReportActionNotification(reportID, _.last(sortedReportActions));

// eslint-disable-next-line rulesdir/no-negated-variables
grgia marked this conversation as resolved.
Show resolved Hide resolved
const notifiableActions = _.filter(reportActions, (action) => ReportActionsUtils.isNotifiableReportAction(action));
_.each(notifiableActions, (action) => Report.showReportActionNotification(reportID, action));
});
}

Expand Down
4 changes: 3 additions & 1 deletion tests/actions/ReportTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -503,7 +503,9 @@ describe('actions/Report', () => {
it('should show a notification for report action updates with shouldNotify', () => {
const TEST_USER_ACCOUNT_ID = 1;
const REPORT_ID = '1';
const REPORT_ACTION = {};
const REPORT_ACTION = {
actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT,
};

// Setup user and pusher listeners
return TestHelper.signInWithTestUser(TEST_USER_ACCOUNT_ID)
Expand Down
Loading