Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support money request push notifications #41861
Support money request push notifications #41861
Changes from 5 commits
e0dc8df
3516441
4c8b147
8669849
f6823bd
b6e9949
feae90f
5642411
1dbd34c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check failure on line 1 in src/libs/Notification/PushNotification/NotificationType.ts
Check warning on line 17 in src/libs/Notification/PushNotification/subscribePushNotification/index.ts
Check failure on line 17 in src/libs/Notification/PushNotification/subscribePushNotification/index.ts
Check failure on line 33 in src/libs/Notification/PushNotification/subscribePushNotification/index.ts
Check failure on line 34 in src/libs/Notification/PushNotification/subscribePushNotification/index.ts
Check failure on line 36 in src/libs/Notification/PushNotification/subscribePushNotification/index.ts
Check failure on line 37 in src/libs/Notification/PushNotification/subscribePushNotification/index.ts
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.
Additional function comments would be useful here
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 thought about it, but I'm not sure there's anything to comment on. "Apply the onyx data" seems like a useless comment and we didn't have any comments for it before either
Check failure on line 87 in src/libs/Notification/PushNotification/subscribePushNotification/index.ts
This file was deleted.
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.
This subscription was a duplicate and has been unnecessary for a while
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 didn't understand. The comment says that it is necessary to be executed in Headless process. Are you saying that it is not needed anymore? We are already executing it somewhere else? If so, do you have more insights on where it is called?
App currently calls this twice. One in Setup and other one in Expensify.tsx file. But this file is also called in headLess env where Expensify.tsx is not loaded so it is necessary here.
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.
We've been handling the subscription here so it's a dupe:
App/src/libs/Notification/PushNotification/subscribePushNotification/index.ts
Lines 111 to 135 in 5642411
The callbacks should work there, but I haven't been able to build Android to test it. Can you build Android to test?
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.
Nvm I fixed the build here. Background updates are still working well on Android: