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
Referral Notifications - FCM Push Notifications #2667
Referral Notifications - FCM Push Notifications #2667
Changes from 14 commits
244f34b
5fbcdc7
c833a92
c7ece13
934379d
c818d08
03840ac
eab4931
43dd926
3ddf08d
9656cdb
2c5472a
095650a
6f2eec2
6531282
57bee40
9558b83
065c2c9
a9a7d18
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
pending todo
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.
Can a notification contains both
data
andnotification
object ?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.
Yes. My understanding is that the main difference is when the app is in the background, the SDK doesn't call
onMessageReceived
only triggers the notification, i.e. if we want thesync
to happen regardless of whether the app is in the foreground or background, we shouldn't have anotification
object in the messageThere 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 completely understand. How do we handle the
sync
notifcation when the app is in background ? Do we just discard the notification ? Or it's routed thorough a different pathway ?Also when
onMessageReceived
gets called (app is in foreground), It seems like we can only have one ofnotification
ordata
payload. In that case it might make more sense to rearrange the code flow as -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.
bumping
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.
Yeah, this is a bit tricky. So, when the app is in the background and the message contains a
notification
object, Firebase SDK takes care of delivering the notification to the Notification Drawer. In case the message also contains adata
object, Firebase SDK adds it to the extras of the intent of the launcher activity of the app. This is the default behaviour, so when the user taps the notification it opens the app launcher with thedata
object as an extra.We can have both, but in this case because we want to trigger the sync when the app is in the background too, we are having messages with only the
data
objectThere 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.
That means we should be handling these intents on app startup right ? Or they by default gets delivered to this messaging service when user clicks on notification and pull the app in foreground ?
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.
@shubham1g5 This is supposed to be handled on app startup but it's not yet implemented because our current implementation on HQ doesn't trigger FCM messages with both
notification
anddata
objects (see image below). So, the intention is to incorporate that on a later stage depending on the feedback we get as well, however, not in the scope of this work.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.
Should not we still need to handle only
data
messages that gets send when app is in background ? if not, How does the app handle sync data messages that gets delivered to the phone when the CC is in background ? For ex, my current understanding is -Does it sound right to you ?
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.
When the
message
doesn't contain anotification
object and the app is in the background, the message is delivered to the FCM Service, so theonMessageDelivered
is called. More about this can be found 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.
got it, thanks for confirming.
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.
should call
setPendingSyncRequestFromServer
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.
This was done on #2680 in this commit
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.
nit: best to put this model in a new file
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.
@shubham1g5 This was done on #2680 in this commit
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.
looks unused
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.
The method
checkUserAndDomain
was moved to theFirebaseMessagingDataSyncer
class in this commit