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

Referral Notifications - FCM Push Notifications #2667

Merged
merged 19 commits into from
Jul 31, 2023
Merged

Conversation

avazirna
Copy link
Contributor

@avazirna avazirna commented May 1, 2023

Summary

This is the first PR related with the implementation of the Referral Notification feature. This part is responsible for the integration of CommCare with Firebase Cloud Message (FCM) as the channel to be used when sending Push Notifications to CommCare apps. The Push Notifications are supposed to trigger syncs in CommCare, when certain conditions are met, allowing the user access to newly created referrals.

In order to test the service, follow the steps below:

  1. Once CommCare is running, Firebase SDK will generate a FCM registration token, if there isn’t one yet, that will be shared with HQ via Heartbeat requests;
    1. For testing purposes, the token is currently outputted with the logs. Look for "New registration token was generated"
  2. Once the FCM token is known, it’s possible to start sending Push notifications to the device using any REST Client, such as Postman or even cURL.
    1. The first step is to obtain a valid access token with Google OAuth playground.
      1. In Select & Authorize APIs, set the scope to https://www.googleapis.com/auth/firebase.messaging under Firebase Cloud Messaging API v1 and press Authorize APIs. This will require authentication with a valid email address.
      2. Once the authentication is complete and the authorization code is available, click Exchange authorization code for tokens to get a refresh and access tokens.
    2. Now that the access token is generated, submit a POST request to https://fcm.googleapis.com/v1/projects/<FIREBASE_PROJECT_ID>/messages:send with the request boby in the following format:
      {
        "message":{
           "token":<FCM_REGISTRATION_TOKEN>,
            "notification":{
               "body": <MESSAGE_BODY>,
               "title": <MESSAGE_TITLE>
            },
            "data":{
               "action" : "sync",
               "username" : <MOBILE_USERNAME>,
               "domain" : <FULLY_QUALIFIED_DOMAIN>, 
               "created_at": <SOME_DATE>
            }
         }
      }
      
    3. Monitor logcat and look for "Message received ..." logs;

Safety Assurance

  • I have confidence that this PR will not introduce a regression for the reasons below

cross-request: dimagi/commcare-core#1275

app/AndroidManifest.xml Outdated Show resolved Hide resolved
app/AndroidManifest.xml Outdated Show resolved Hide resolved
app/src/org/commcare/preferences/HiddenPreferences.java Outdated Show resolved Hide resolved

/**
* This method purpose is to show notifications to the user when the app is in the foreground.
* When the app is in the background, FCM is responsible for notifying the user
Copy link
Contributor

Choose a reason for hiding this comment

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

When the app is in the background, FCM is responsible for notifying the user

Is that true for data notifications as well ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed this, but it seems to have been answered here

@avazirna
Copy link
Contributor Author

avazirna commented May 2, 2023

@damagatchi retest this please?

@avazirna
Copy link
Contributor Author

avazirna commented Jun 22, 2023

@damagatchi retest this please

@avazirna avazirna changed the title [WIP] Referral Notifications - FCM Push Notifications Referral Notifications - FCM Push Notifications Jul 7, 2023
app/AndroidManifest.xml Outdated Show resolved Hide resolved
app/src/org/commcare/CommCareApplication.java Outdated Show resolved Hide resolved
app/AndroidManifest.xml Outdated Show resolved Hide resolved
Comment on lines +48 to +49
Map<String, String> payloadData = remoteMessage.getData();
RemoteMessage.Notification payloadNotification = remoteMessage.getNotification();
Copy link
Contributor

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 and notification object ?

Copy link
Contributor Author

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 the sync to happen regardless of whether the app is in the foreground or background, we shouldn't have a notification object in the message

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 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 of notification or data payload. In that case it might make more sense to rearrange the code flow as -

if (payloadNotification != null) {
            showNotification(payloadNotification);
} else if (payloadData.size() != 0){
     /// process payloadData
}

Copy link
Contributor

Choose a reason for hiding this comment

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

bumping

Copy link
Contributor Author

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 ?

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 a data 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 the data object as an extra.

Also when onMessageReceived gets called (app is in foreground), It seems like we can only have one of notification or data payload.

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 object

Copy link
Contributor

Choose a reason for hiding this comment

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

In case the message also contains a data 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 the data object as an extra.

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 ?

Copy link
Contributor Author

@avazirna avazirna Jul 28, 2023

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 and data 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.
image

Copy link
Contributor

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 -

  1. We trigger a notification with Action 'Background Sync' and type data messages
  2. This notification gets delivered to phone when CC is in background. Therefore Firebase adds the notification to the notification drawer ? (Not sure if it's true for only data messages)
  3. On clicking notification, Firebase launches CC with the intent containing data action. But since we are not handling intents with data payload, background sync doesn't get triggered.

Does it sound right to you ?

Copy link
Contributor Author

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 a notification object and the app is in the background, the message is delivered to the FCM Service, so the onMessageDelivered is called. More about this can be found here.

Copy link
Contributor

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.

* This class is to facilitate handling the FCM Message Data object. It should contain all the
* necessary checks and transformations
*/
public class FCMMessageData {
Copy link
Contributor

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

Copy link
Contributor Author

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

sharedPreferences.edit().putLong(FCM_TOKEN_TIME,System.currentTimeMillis()).apply();
}

public static String removeServerUrlFromUserDomain(String userDomain) {
Copy link
Contributor

Choose a reason for hiding this comment

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

looks unused

Copy link
Contributor Author

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 the FirebaseMessagingDataSyncer class in this commit

FCMMessageData fcmMessageData = new FCMMessageData(payloadData);

switch(fcmMessageData.action){
case SYNC -> {} // trigger sync for fcmMessageData
Copy link
Contributor

Choose a reason for hiding this comment

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

should call setPendingSyncRequestFromServer here ?

Copy link
Contributor Author

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

@avazirna
Copy link
Contributor Author

@damagatchi retest this please

shubham1g5
shubham1g5 previously approved these changes Jul 18, 2023
Copy link
Contributor

@shubham1g5 shubham1g5 left a comment

Choose a reason for hiding this comment

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

Looks safe to merge, we can continue any followups from here in other PRs. We should do a pass on lint errors before merging as well.

@@ -46,4 +51,19 @@ public static String removeServerUrlFromUserDomain(String userDomain) {
}
return userDomain;
}

public static void verifyToken() {
if(!BuildConfig.DEBUG) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not init token in debug ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing in particular, just trying to avoid noise but I'm happy to revert that.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah we should revert to be able to have receive push notifications with debug apk as well.

private static String getISO8601FormattedLastSyncTime() {
long lastSyncTime = SyncDetailCalculations.getLastSyncTime();
if (lastSyncTime == 0) {
// TODO: Move this method to DateUtils
Copy link
Contributor

Choose a reason for hiding this comment

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

pending todo

Comment on lines +48 to +49
Map<String, String> payloadData = remoteMessage.getData();
RemoteMessage.Notification payloadNotification = remoteMessage.getNotification();
Copy link
Contributor

Choose a reason for hiding this comment

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

bumping

@avazirna
Copy link
Contributor Author

@damagatchi retest this please

5 similar comments
@avazirna
Copy link
Contributor Author

@damagatchi retest this please

@avazirna
Copy link
Contributor Author

@damagatchi retest this please

@avazirna
Copy link
Contributor Author

@damagatchi retest this please

@shubham1g5
Copy link
Contributor

@damagatchi retest this please

@shubham1g5
Copy link
Contributor

@damagatchi retest this please

@avazirna avazirna merged commit cfac4ca into master Jul 31, 2023
1 check failed
@shubham1g5 shubham1g5 deleted the fcm_push_notifications branch September 7, 2023 15:32
@avazirna avazirna added this to the 2.54 milestone Sep 14, 2023
@avazirna avazirna self-assigned this Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants