MelvinBot
commented
1 year ago Triggered auto assignment to @abekkala (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Closed kavimuru closed 11 months ago
MelvinBot
commented
1 year ago Triggered auto assignment to @abekkala (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
MelvinBot
commented
1 year ago Platforms in OP are ✅)MelvinBot
commented
1 year ago Job added to Upwork: https://www.upwork.com/jobs/~012b41f48d555b7fe0
MelvinBot
commented
1 year ago Current assignee @abekkala is eligible for the External assigner, not assigning anyone new.
MelvinBot
commented
1 year ago Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)
MelvinBot
commented
1 year ago Triggered auto assignment to @chiragsalian (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.
PankajAS
commented
1 year ago Delete a message doesn't delete notification of message
Root cause of this issue is we are not deleting notification when message delete
To clear deleted message notification for all platform we have to use different approach
UrbanAirship.clearNotification(notificationID)BrowserNotifications.js notification reference with notifiction.close() function.
Backend Changes: Onyx data should receive notification id for each commentNone
Prince-Mendiratta
commented
1 year ago In this issue, when we send a series of messages, all these messages are shown in the notification. If we were to delete any unread message, the message is not removed from the notification.
This is not an issue, more like a feature request.
Implementing this feature will require changes on both E/App and on the backend. Having this feature just for Android would be relatively simpler but to enable it for both native platforms, we will have to store the notification ID for each comment in Onyx. This can be done by adding the notification ID to the onyxData received here: https://github.com/Expensify/App/blob/9b190e8ddf1650e366057819fe8e633d8f2d6989/src/libs/Notification/PushNotification/index.native.js#L26
The notification ID can be extracted from the notification parameter.
The onyxData received from Urban Airship (UA) when a new notification is sent to the user looks like this:
[
{
"onyxMethod": "merge",
"key": "report_1417044219319203",
"value": {
"reportID": "1417044219319203",
"maxSequenceNumber": 28,
"lastVisibleActionCreated": "2023-03-09 17:38:42.826",
"lastReadTime": "2023-03-09 17:27:52.887",
"lastMessageText": "hi",
"lastActorEmail": "prince.mendi+3@gmail.com"
}
},
{
"onyxMethod": "merge",
"key": "reportActions_1417044219319203",
"value": {
"1718706535649794612": {
"person": [
{
"type": "TEXT",
"style": "strong",
"text": "Prince Mendiratta"
}
],
"actorEmail": "prince.mendi+3@gmail.com",
"actorAccountID": 13072436,
"message": [
{
"type": "COMMENT",
"html": "hi",
"text": "hi",
"isEdited": false,
"reactions": []
}
],
"originalMessage": {
"html": "hi"
},
"avatar": "https://d1wpcgnaa73g0y.cloudfront.net/f05e9850d1cf553f5389c5c305210ddb4ed59aee_128.jpeg",
"created": "2023-03-09 17:38:42.826",
"timestamp": 1678383522,
"reportActionTimestamp": 1678383522826,
"automatic": false,
"sequenceNumber": 28,
"actionName": "ADDCOMMENT",
"shouldShow": true,
"reportActionID": "1718706535649794612",
"isAttachment": false
}
},
"shouldNotify": true
}
]Here, in the report action key, we can add the notification ID as well.
Then, when a comment is deleted, we will receive the pusher event for it. From there, we can extract the corresponding onyx value for the report action and then call the UrbanAirship.clearNotification(reportActionData.notificationID) to get rid of that notification.
We could've directly appended the notification ID in the payload in android/app/src/main/java/com/expensify/chat/customairshipextender/CustomNotificationProvider.java but I'm unsure as to why we do not use any custom configuration for iOS. Asked for the same here - https://expensify.slack.com/archives/C01GTK53T8Q/p1678386405083379
MelvinBot
commented
1 year ago @eVoloshchak, @abekkala, @chiragsalian Huh... This is 4 days overdue. Who can take care of this?
eVoloshchak
commented
1 year ago @PankajAS's proposal is correct, we just need to use UrbanAirship.clearNotification(notificationID)
However, @Prince-Mendiratta's proposal is much more detailed, explaining where should we get notificationID and, most importantly, handles the iOS mobile app.
If this feature needs to be implemented on iOS and Android only - @Prince-Mendiratta's proposal looks good to me
If not - we need to investigate how could this be implemented on other platforms
@chiragsalian, should this feature be implemented on all platforms or just on native platforms?
chiragsalian
commented
1 year ago should this feature be implemented on all platforms or just on native platforms?
Is it possible for this issue to occur on other platforms like web / desktop app/ mweb? I haven't tested but is there a way we can clear a web / desktop / mweb notification too?
If the issue is reproducible on web / desktop / mweb too i would love a solution that covers all platforms. If its not reproducible on other platforms I'm okay with just a native platform fix.
eVoloshchak
commented
1 year ago Is it possible for this issue to occur on other platforms like web / desktop app/ mweb?
@chiragsalian, yes, I've tested other platforms and here are my findings: mWeb Chrome - just can't receive any notifications mWeb Safari - can't receive any notifications, but I believe this isn't supported for Safari mobile macOS Desktop - can reproduce the issue, deleting the message doesn't delete it from macOS's notifications feed macOS Chrome/Safari - can reproduce the issue, deleting the message doesn't delete it from macOS's notifications feed
I haven't tested but is there a way we can clear a web / desktop / mweb notification too?
Judging by this file we're only using UrbanShip notifications on mobile.
// Push notifications are only supported on mobile, so we'll just noop here
So Desktop and web are using native notifications? @PankajAS, @Prince-Mendiratta, do you have ideas on how to resolve this on all platforms?
PankajAS
commented
1 year ago Is it possible for this issue to occur on other platforms like web / desktop app/ mweb?
@chiragsalian, yes, I've tested other platforms and here are my findings: mWeb Chrome - just can't receive any notifications mWeb Safari - can't receive any notifications, but I believe this isn't supported for Safari mobile macOS Desktop - can reproduce the issue, deleting the message doesn't delete it from macOS's notifications feed macOS Chrome/Safari - can reproduce the issue, deleting the message doesn't delete it from macOS's notifications feed
I haven't tested but is there a way we can clear a web / desktop / mweb notification too?
Judging by this file we're only using UrbanShip notifications on mobile.
// Push notifications are only supported on mobile, so we'll just noop here
So Desktop and web are using native notifications? @PankajAS, @Prince-Mendiratta, do you have ideas on how to resolve this on all platforms?
yes @eVoloshchak for web and dekstop we have to use notification reference from BrowserNotifications.js and we can delete notifiction by call notifiction.close(), currently its closing notification onclick.
eVoloshchak
commented
1 year ago @PankajAS, awesome! Could you post an updated proposal that would include all platforms?
eVoloshchak
commented
1 year ago proposal Updated @eVoloshchak
Could you elaborate on backend changes that are required?
PankajAS
commented
1 year ago @eVoloshchak we need notifiction id for each comment from backend in Onyx Data
eVoloshchak
commented
1 year ago @eVoloshchak we need notifiction id for each comment from backend in Onyx Data
This is better explained in @Prince-Mendiratta's proposal here
We can proceed with @PankajAS's proposal, that way we'll fix this on all platforms. We'll also need changes to the back-end, which are best described by @Prince-Mendiratta here
🎀👀🎀 C+ reviewed!
Since we're implementing this feature, we should also add a feature-request to update notification when you edit a message.
Prince-Mendiratta
commented
1 year ago @eVoloshchak I missed out the comment, I believe @PankajAS's is not incomplete and incorrect. The notification IDs for each platform are different and should not be saved in the backend, only in local onyx storage. I'll send in a proposal later tonight once I get the time.
eVoloshchak
commented
1 year ago The notification IDs for each platform are different and should not be saved in the backend, only in local onyx storage. I'll send in a proposal later tonight once I get the time.
Awesome, thank you @chiragsalian, should we start a wider discussion about the backend changes necessary in slack?
PankajAS
commented
1 year ago @eVoloshchak only for Android and IOS platform we need notification id(separate for both platform), we just need to add id for both platform in notification payload from backend:
'notification': {
'android': {
'notification_id': '1234',
},
'ios': {
'thread-id': 'com.app.notification-group',
},
},for web and desktop app we are using local notification and we can clear desktop/web app notification from reference on frontend side
MelvinBot
commented
1 year ago @eVoloshchak @abekkala @chiragsalian this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
MelvinBot
commented
1 year ago Triggered auto assignment to @muttmuure (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
MelvinBot
commented
1 year ago Platforms in OP are ✅)
abekkala
commented
1 year ago I'm reassigning as I'm ooo until Tues April 04 and this one will see some movement during that time. @muttmuure Here is where this GH is at!:
Issue reported by: @esh-g Fix: TBD - proposal not chosen, fixer not assigned PR Review: will be done by @eVoloshchak
Prince-Mendiratta
commented
1 year ago Posting a new proposal to have a clearer presentation
In this issue, when we send a series of messages, all these messages are shown in the notifications. If we were to delete any unread message, the message is not removed from the notification.
This is not an issue, more like a feature request.
A very crude and hacky (but working) implementation of this feature can be found at https://github.com/Prince-Mendiratta/expensify-app/tree/notifications.
The problem can be further decomposed in two parts.
Let's tackle this problem platform wise. Image source.
Right now, we use Urban Airship for delivering notifications on native devices. It uses PushNotifications. For Web, mWeb and Desktop, a Pusher event is used. It uses LocalNotification.
For native devices, we will have to store the notification ID in local Onyx storage. Doing so directly inside the reportAction would be unwise since it is platform dependent. So I propose we create a new Onyx key, notificationIDs of the format:
{
reportActionID: notificationID,
}This will be a single object that contains all notification IDs with the reportActionID (comment ID) being the key and the notification ID being the value. This can be done in PushNotification.
https://github.com/Expensify/App/blob/7b8f085e2e58793d8168d8fc08bf95c14db9cb01/src/libs/Notification/PushNotification/index.native.js#L24
We can extract the notification ID using:
const notificationID = lodashGet(notification, 'notificationId');Then, we can add a new method in src/libs/actions/PushNotification.js,
function setPushNotificationID(reportActionID, notificationID) {
Onyx.merge(ONYXKEYS.NOTIFICATION_IDS, {[reportActionID]: notificationID});
}Then, we can call this method in the pushNotificationEventCallback function like this:
const reportActionID = lodashGet(payload, 'reportAction.reportActionID');
PushNotification.setPushNotificationID(reportActionID, notificationID);With this, we tackle the issue of saving notification IDs.
Next, to delete this notification, we need backend changes in UrbanAirship configuration to send UA updates when any comment is deleted. Then, once the update is received when the comment is deleted, we can obtain the notification ID from Onyx and then clear it using UrbanAirship.clearNotification(id);, which can be defined as a new method in PushNotification.clearNotification(id).
Note that this will only work for iOS out of the box. On android, since we use a custom notification cache, we need to remove the notification from the cache itself. This has to be done in the android/app/src/main/java/com/expensify/chat/customairshipextender/CustomNotificationProvider.java. I'm unable to test on iOS right now because of lack of documentation, asked for help here
Now comes Web and desktop platforms. They use local notifications and the notification.close() method can only be used on object instances of the Notification class. This means that we can't remotely close the notification using the event when the comment is deleted without somehow storing each notification object. JavaScript Notification API does not create a notification ID as is done by Urban Airship. A better way to do this is by using notification tag. This tag can be set here:
https://github.com/Expensify/App/blob/7b8f085e2e58793d8168d8fc08bf95c14db9cb01/src/libs/Notification/LocalNotification/BrowserNotifications.js#L115-L121
The value of the tag can be the reportActionID itself.
This tackles the problem 1 of storing/referencing notification ID. Now, to delete the notification, two things need to be done, one involving backend changes.
We will need to add the shouldNotify property to the deleted comment action, as is done in newly created comment action. This has to be done in backend as the event is sent by the Pusher. This is done to get to the notification handler in this condition:
https://github.com/Expensify/App/blob/7b8f085e2e58793d8168d8fc08bf95c14db9cb01/src/libs/actions/User.js#L263-L274
We need to extract the reportActionID from the deleted comment action. A helpful backend change would be to include the reportActionID in the newly created report comment. The objects sent the pusher are:
Once we have the reportActionID, we need to pass it to the Report.showReportActionNotification function.
Here, we need to move the below mentioned condition a bit below, just above sending a new notification. In this condition, we need to clear the notification.
https://github.com/Expensify/App/blob/7b8f085e2e58793d8168d8fc08bf95c14db9cb01/src/libs/actions/Report.js#L1140-L1143
To delete the notification, we need to add a new method clearNotification inside src/libs/Notification/LocalNotification.
In this function, we need to create an empty notification with the same tag and run the notification.close() method immediately. This will replace the old notification and clear the new notification, resulting in that notification disappearing.
clearNotification(reportActionID) {
const notification = new Notification('', {
tag: reportActionID,
silent: true,
});
setTimeout(notification.close.bind(notification), 1); // need to use timeout as this needs to be delayed by 1ms to delete
},For deleting the native notification, we can actually call PushNotification.clearNotification() from showReportActionNotification as well since the local notification flow is triggered for native apps as well, even though they do not actually have any method and nor do they use local notifications.
Secondly, since we will be further expanding to edit notification comment on comment edit, I think we can look into the concept using Urban Airship for Web as well and keep using the Desktop + local notification flow while the support for desktop is added to UA. Source
cc @eVoloshchak @muttmuure @chiragsalian
Julesssss
commented
1 year ago Hey @Prince-Mendiratta. Thanks for the detailed proposal, but I think this should be held for now on our internal notification changes. Sorry for not noticing sooner, but I only just found this issue.
This solution works for the local client only, and we're planning to instead clear notifications from all user clients once read, which obviously requires some backend changes too. I'm also not sure we should create a new key for notificationID, as this complicates our backend logic.
Having said that, the code you present has some value for the client notification dismissal.
PankajAS
commented
1 year ago @Julesssss we don't need change too much on backend side its just you have to send notification ID which i mention here, and it will close notification for all users.
Julesssss
commented
1 year ago Right, I don't think it'll take much work. But in order to clear notifications for ALL clients will need to start sending a new silent notification.
The only way to clear android/iOS notifications when I view the message on web is to inform the backend, which will then send a new silent notification to all clients to wipe the current notification -- this will need to be implemented first, and this is what I want to hold the changes on.
Prince-Mendiratta
commented
1 year ago Hi @Julesssss!
we're planning to instead clear notifications from all user clients once read, which obviously requires some backend changes too
Wouldn't this still involve making changes to the Urban Airship and frontend code since we'll want to clear notification from user device as well? Does there exist a backend method on UA that is able to clear out specific notifications?
I'm also not sure we should create a new key for notificationID, as this complicates our backend logic.
I don't think this will complicate backend logic, this doesn't require backend changes and is stored only on the local storage. The notificationID is used just for iOS right now, it can be bypassed for android with some manipulation in the notification cache.
chiragsalian
commented
1 year ago Discussed 1:1 with jules and he's taking over this issue.
Julesssss
commented
1 year ago @Prince-Mendiratta Yeah, we'll need backend and frontend changes, and I think some parts of your proposal can be used for this still.
Does there exist a backend method on UA that is able to clear out specific notifications?
Perhaps, but if there is we don't want to be calling this directly from the front-end client. We have a lot more control (and a less confusing flow) by delegating this to the backend which has access to a lot more information.
Prince-Mendiratta
commented
1 year ago Got it, thanks @Julesssss. Do let me know if I can help out in any way. I believe I've become quite familiar with the notification flow so I'd be able to contribute 😄
Julesssss
commented
1 year ago Thanks, yeah I appreciate that you've spent time here already so I'll try to pass off the front-end changes to you. Maybe we'll need to reduce the bonus by a % though.
Prince-Mendiratta
commented
1 year ago Sounds good to me, thank you @Julesssss!
PankajAS
commented
1 year ago @Julesssss my Proposal gave complete solution for all platform and i mentioned backend changes, @eVoloshchak agree on that so i hope this will be consider first when you assign frontend changes and i will able to contribute on this.
Julesssss
commented
1 year ago we need notifiction id for each comment from backend in Onyx Data
I don't agree this is the desired solution.
MelvinBot
commented
1 year ago @eVoloshchak, @Julesssss, @muttmuure Whoops! This issue is 2 days overdue. Let's get this updated quick!
eVoloshchak
commented
1 year ago Not overdue, on hold for Expensify#255265
muttmuure
commented
1 year ago on hold
eVoloshchak
commented
1 year ago Not overdue, on hold
MelvinBot
commented
1 year ago @eVoloshchak, @Julesssss, @muttmuure Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
MelvinBot
commented
1 year ago @eVoloshchak, @Julesssss, @muttmuure Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
eVoloshchak
commented
1 year ago Not overdue, on hold
Julesssss
commented
1 year ago Yep, still held. Reducing the priority here as it's less important than other tasks.
dylanexpensify
commented
1 year ago @Julesssss I have this issue here that deals with notifications on chrome/desktop that don't update once deleted. Do you feel this could be related?
Julesssss
commented
1 year ago That seems like a different existing issue to me
dylanexpensify
commented
1 year ago Ok cool cool, thanks for the input!
eVoloshchak
commented
1 year ago Not overdue, still on HOLD
muttmuure
commented
1 year ago Still hold
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
The notification that B gets from A should be deleted like in Whatsapp
Actual Result:
The notification stays there and doesn't get deleted
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.2.80-1 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:
https://user-images.githubusercontent.com/43996225/223887763-acf06762-8059-47f2-ae2f-f9af197ecfc9.mp4
https://user-images.githubusercontent.com/43996225/223887914-52d5fbfe-6fe0-4b9d-b681-a957e2dcd800.mp4
Expensify/Expensify Issue URL: Issue reported by: @esh-g Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1678213390607719
View all open jobs on GitHub
Upwork Automation - Do Not Edit