phonegap / phonegap-plugin-push

Register and receive push notifications
MIT License
1.94k stars 1.91k forks source link

Feature request: Same payload functionality for Android/iOS on FCM #2364

Open Moghul opened 6 years ago

Moghul commented 6 years ago

Expected Behaviour

You should be able to send the same payload to both iOS and Android on FCM and achieve the same results.

Actual Behaviour

Sending this payload

{
  "notification": {
    "title": "Test Notification",
    "body": "This offer expires at 11:30 or whatever",
    "notId": 10
  },
  "data": {
    "surveyID": "ewtawgreg-gragrag-rgarhthgbad"
  }
}

Will result in notifications on both platforms. However, only on iOS is the onNotification event triggered.

Reproduce Scenario (including but not limited to)

Send this payload to fully closed apps on Android/iOS and check for onNotification to trigger.

Steps to Reproduce

Platform and Version (eg. Android 5.0 or iOS 9.2.1)

Android 4.3 and up. iOS 8 and up (I'm sure it's like this for other versions but that's what I'm using)

(Android) What device vendor (e.g. Samsung, HTC, Sony...)

Any Android and iPhone.

Cordova CLI version and cordova platform version

Cordova cli 8.0.0 Cordova-android 6.3.0 (non-updateable ATM) Cordova-iOS 4.5.0

Plugin version

plugin version 2.1.2 (currently not updateable due to cordova-android)

Sample Push Data Payload

{
  "notification": {
    "title": "Test Notification",
    "body": "This offer expires at 11:30 or whatever",
    "notId": 10
  },
  "data": {
    "surveyID": "ewtawgreg-gragrag-rgarhthgbad"
  }
}

Sample Code that illustrates the problem

N/A

Logs taken while reproducing problem

N/A

Now... I'm the guy who discovered the information currently found in the documentation. I had proposed at the time the use of a GCMReceiver class to help fix this and provided the sample that worked for my simple text-only notifications. However, that fell through because it failed too many tests and didn't work for a bunch of other scenarios which I didn't have time to test and fix. See here.

I'm back! I need to update to FCM, and I need my universal notifications too. I can't use my janky GCMReceiver class anymore and I still can not implement different payloads for different platforms.

I'll be working on it for myself, of course, but I'd very much appreciate any help anyone can offer. Ideas, your own solutions, some information, documentation, whatever. Whatever I come up with I'll post here as well so if anyone is interested, let's try to work together on this.

macdonst commented 6 years ago

@Moghul I hope you can figure it out and I don't want to discourage you but my findings indicate that any push message coming into an Android device with a notification section will skip calling the onMessageReceived function of the FCMService class that is implemented in this plugin.

https://github.com/phonegap/phonegap-plugin-push/blob/master/docs/PAYLOAD.md#notification-vs-data-payloads

Moghul commented 6 years ago

Sadly, I know. As far as I can tell, they took people's complaints and threw them out the window. Slapped a new name on the class and called it a new system.

I'm just hoping maybe some code gets triggered (in another class even), that can get that notification data. Then, I don't know. Put it in preferences, if I have to. I need the app to perform an action with the payload when you tap it.

macdonst commented 6 years ago

@Moghul yeah, I checked when FCM was released and there appeared to be no changes.

dais-set commented 6 years ago

Hi, as we've discovered the same problem with the plugin, I'd like to ask for the FCM implementation to be fixed to correspond to the documentation. Per the FCM documentation, a message that includes both notification and data should let the client handle the data: (see https://firebase.google.com/docs/cloud-messaging/concept-options)

FCM can send a notification message including an optional data payload. In such cases, FCM handles displaying the notification payload, and the client app handles the data payload.

Moghul commented 6 years ago

Yeah, was just looking at https://firebase.google.com/docs/cloud-messaging/concept-options and saw that. Really curious how that's supposed to work now.

EDIT:

When in the background, apps receive the notification payload in the notification tray, and only handle the data payload when the user taps on the notification.

This is fine by me, as long as I get that data.

dais-set commented 6 years ago

As other plugins (admittedly, non-cordova plugins) pass the data just fine, I'd say you do get the data payload. We use Plugin.FirebasePushNotification on a Xamarin project.

macdonst commented 6 years ago

@dais-set I'd love if that were the case but in my testing if notification is in the payload even if data is there as well onMessageReceived does not get called. My recommendation for this plugin is to put everything in data.

I really need onMessageReceived to be called so I can setup things like background notifications, action buttons, etc.

Moghul commented 6 years ago

Yeah, just tested with logcat, onMessageReceived doesn't get called when tapping the notification. Something's up.

EDIT: https://stackoverflow.com/questions/37358462/firebase-onmessagereceived-not-called-when-app-in-background

Looks like the data isn't passed to onMessageReceived, but to an intent. Maybe there's something we can do with that. If not to make proper notifications and stuff, then at least to pass the notification data forwards.

dais-set commented 6 years ago

Thanks, @macdonst! I'll see if I can share anything useful. Unfortunately, it's not enough to put "everything" in data, as you're actually requesting for us to stop passing "notification".

ghost commented 6 years ago

What does it mean, the data is passed to an intend? I have no Android skills, but I'am interested in this issue because we are facing the same problem.

Moghul commented 6 years ago

https://stackoverflow.com/questions/40718155/open-specific-activity-when-notification-clicked-in-fcm

Maybe, just maybe, we can also do something else with it. The click_action thing might be what saves us.

macdonst commented 6 years ago

The problem with notification in the payload is the OS will read it and try to display it's own notification. So if you use some method to have onMessageReceived or handleIntent called you may end up with two notifications in the shade.

Moghul commented 6 years ago

I was thinking about just making PushPlugin.sendExtras(originalExtras); work from a separate activity or something.

The push notification works as far as a push notification goes, fine. It's when you tap it that none of the code actually triggers (apart from the PushPlugin.java class code).

I have currently made it so that I can trigger a separate activity for now (just so i don't mess with the OG code) when I tap the notification. Now I need to figure out this stuff:

05-15 13:43:00.867 28849 28849 E AndroidRuntime: java.lang.RuntimeException: Unable to start activity ComponentInfo{opeepl.respondent.app/com.adobe.phonegap.push.BackgroundNotificationActivity}: java.lang.NullPointerException: Attempt to invoke virtual method 'void android.os.Bundle.putBoolean(java.lang.String, boolean)' on a null object reference
05-15 13:43:00.867 28849 28849 E AndroidRuntime:    at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2450)
05-15 13:43:00.867 28849 28849 E AndroidRuntime:    at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2510)
05-15 13:43:00.867 28849 28849 E AndroidRuntime:    at android.app.ActivityThread.-wrap11(ActivityThread.java)
05-15 13:43:00.867 28849 28849 E AndroidRuntime:    at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1363)
05-15 13:43:00.867 28849 28849 E AndroidRuntime:    at android.os.Handler.dispatchMessage(Handler.java:102)
05-15 13:43:00.867 28849 28849 E AndroidRuntime:    at android.os.Looper.loop(Looper.java:148)
05-15 13:43:00.867 28849 28849 E AndroidRuntime:    at android.app.ActivityThread.main(ActivityThread.java:5461)
05-15 13:43:00.867 28849 28849 E AndroidRuntime:    at java.lang.reflect.Method.invoke(Native Method)
05-15 13:43:00.867 28849 28849 E AndroidRuntime:    at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726)
05-15 13:43:00.867 28849 28849 E AndroidRuntime:    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)
05-15 13:43:00.867 28849 28849 E AndroidRuntime: Caused by: java.lang.NullPointerException: Attempt to invoke virtual method 'void android.os.Bundle.putBoolean(java.lang.String, boolean)' on a null object reference
05-15 13:43:00.867 28849 28849 E AndroidRuntime:    at com.adobe.phonegap.push.BackgroundNotificationActivity.processPushBundle(BackgroundNotificationActivity.java:81)
05-15 13:43:00.867 28849 28849 E AndroidRuntime:    at com.adobe.phonegap.push.BackgroundNotificationActivity.onCreate(BackgroundNotificationActivity.java:46)
05-15 13:43:00.867 28849 28849 E AndroidRuntime:    at android.app.Activity.performCreate(Activity.java:6251)
05-15 13:43:00.867 28849 28849 E AndroidRuntime:    at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1108)
05-15 13:43:00.867 28849 28849 E AndroidRuntime:    at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2403)
05-15 13:43:00.867 28849 28849 E AndroidRuntime:    ... 9 more

Then I will figure out how to use sendExtras, then how to switch back to the main activity. Baby steps.

EDIT: The crash happens somewhere in here

        if(!startOnBackground){
            NotificationManager notificationManager = (NotificationManager) getSystemService(Context.NOTIFICATION_SERVICE);
            notificationManager.cancel(FCMService.getAppName(this), notId);
        }

        boolean isPushPluginActive = PushPlugin.isActive();
        boolean inline = processPushBundle(isPushPluginActive, intent);

        if(inline && android.os.Build.VERSION.SDK_INT < android.os.Build.VERSION_CODES.N && !startOnBackground){
            foreground = true;
        }

Edit 2: forgot to mention my new activity is a carbon copy of the PushHandlerActivity right now.

Edit 3:
boolean inline = processPushBundle(isPushPluginActive, intent);

This specific line

Edit 4: looks like this Bundle originalExtras = extras.getBundle(PUSH_BUNDLE); is null in that function.

Moghul commented 6 years ago

Check it out, the data is there.

    private boolean processPushBundle(boolean isPushPluginActive, Intent intent) {
        Bundle extras = getIntent().getExtras();
        Bundle remoteInput = null;

        for (String key: extras.keySet()) {
          Log.d("LOG_TAG", key + " is a key in the bundle ----");
        }
05-15 14:09:51.569  8915  8915 D LOG_TAG : google.sent_time is a key in the bundle ----
05-15 14:09:51.569  8915  8915 D LOG_TAG : surveyId is a key in the bundle ----
05-15 14:09:51.569  8915  8915 D LOG_TAG : google.ttl is a key in the bundle ----
05-15 14:09:51.569  8915  8915 D LOG_TAG : _fbSourceApplicationHasBeenSet is a key in the bundle ----
05-15 14:09:51.569  8915  8915 D LOG_TAG : from is a key in the bundle ----
05-15 14:09:51.569  8915  8915 D LOG_TAG : google.message_id is a key in the bundle ----
05-15 14:09:51.569  8915  8915 D LOG_TAG : collapse_key is a key in the bundle ----

surveyId specifically is mine.

Moghul commented 6 years ago

Yaaaas, it works!

Here's the hacked up code for processPushBundle

    /**
     * Takes the pushBundle extras from the intent,
     * and sends it through to the PushPlugin for processing.
     */
    private boolean processPushBundle(boolean isPushPluginActive, Intent intent) {
        Bundle extras = getIntent().getExtras();
        Bundle remoteInput = null;

        if (extras != null) {
            Bundle originalExtras = extras.getBundle(PUSH_BUNDLE);

            if (originalExtras == null) {
                originalExtras = extras;
            }

            originalExtras.putBoolean(FOREGROUND, false);
            originalExtras.putBoolean(COLDSTART, !isPushPluginActive);
            originalExtras.putBoolean(DISMISSED, extras.getBoolean(DISMISSED));
            originalExtras.putString(ACTION_CALLBACK, extras.getString(CALLBACK));
            originalExtras.remove(NO_CACHE);

            remoteInput = RemoteInput.getResultsFromIntent(intent);
            if (remoteInput != null) {
                String inputString = remoteInput.getCharSequence(INLINE_REPLY).toString();
                Log.d(LOG_TAG, "response: " + inputString);
                originalExtras.putString(INLINE_REPLY, inputString);
            }

            PushPlugin.sendExtras(originalExtras);
        }
        return remoteInput == null;
    }

Here's the rest of the stuff I made:

plugin.xml

      <activity android:name="com.adobe.phonegap.push.BackgroundNotificationActivity">
        <intent-filter>
          <action android:name="com.opeepl.background.MESSAGING_EVENT" android:permission="${applicationId}.permission.BackgroundNotificationActivity" />
          <category android:name="android.intent.category.DEFAULT" />
        </intent-filter>
      </activity>
{
    "priority" : "high",
    "notification" : {
        "title": "Test Notification",
        "body": "Please disregard",
        "notId": 10,
        "sound": "default",
        "click_action": "com.opeepl.background.MESSAGING_EVENT"
    },
    "data" : {
      "surveyId": "id goes here"
    },
    "to" : "id"
}

Here's the data in the onNotificationReceived event in the app

selection_0s24

Now I guess someone just has to test & better merge this with the rest of the plugin. Any takers?

macdonst commented 6 years ago

@Moghul now check to see if the notification show uses the icon and color specified by the init option fo the plugin. Does it play the correct sound? Can you add action buttons. When clicked do the action buttons call their JS handler correctly?

Moghul commented 6 years ago

Before I do all that, do you know that it doesn't? What I mean is, have you tried this and actually found that it doesn't work, or do you actually want me to do the tests?

macdonst commented 6 years ago

@Moghul my suspicion is that in seeing the notification part of the payload the OS will take over to display the notification in the notification shade. It will ignore the icon and sound properties set in the init method. Such are the constraints we are faced with when we want to enable folks to set this from JS code.

Moghul commented 6 years ago

Oh yeah, I see what you mean. I'm pretty sure you're right about it taking over since... well, it says so. It's definitely a limitation of the solution if you have custom notifications.

The code doesn't run until you actually tap the notification so I can confirm this is the case pretty much.

macdonst commented 6 years ago

Hence our balancing act in this project.

Moghul commented 6 years ago

True, but if you're using default notifications, this isn't a bad solution. It needs cleaning up so you don't pass the sender and whatnot to the js, but for example, for me, it does precisely what I want - pass the data to the JS.

Right now, this is a flat improvement IMHO and only needs a quirk section in the documentation. No, it does not completely fulfill the requirements in the title of this thread, but I still think it's worth putting in the plugin.

dais-set commented 6 years ago

Thanks @Moghul!

Moghul commented 6 years ago

No problem @dais-set, but it still needs testing. Maybe you don't even need a separate activity. Hopefully @macdonst will give this minor improvement a pass so I don't have to have custom code in my plugin.

Moghul commented 6 years ago

We have a bug with this right now. If the app is in background (not closed), and you send the payload with the click_action key, tapping the notification will not bring the app to foreground and will remove it from recent screens. Going to the menu and opening the app will make it behave as if opening from recent screens.

05-15 15:50:03.501 31067 31067 D Push_HandlerActivity: bringToForeground = true
05-15 15:50:03.807   795  5754 I WindowManager: Screenshot max retries 4 of Token{ed23228 ActivityRecord{858284b u0 opeepl.respondent.app/com.adobe.phonegap.push.PushHandlerActivity t742 f}} appWin=Window{f5d8f04 u0 Starting opeepl.respondent.app} drawState=3
05-15 15:50:03.808 31067 31067 D Push_HandlerActivity: isPushPluginActive = true
05-15 15:50:03.808 31067 31067 D Push_HandlerActivity: don't want main activity
05-15 15:50:03.810 31067 31067 D SystemWebChromeClient: file:///android_asset/www/js/generalManagers/notificationManager.js: Line 165 : [object Object]
05-15 15:50:03.810 31067 31067 I chromium: [INFO:CONSOLE(165)] "[object Object]", source: file:///android_asset/www/js/generalManagers/notificationManager.js (165)
05-15 15:50:03.824   795 11380 W InputMethodManagerService: Window already focused, ignoring focus gain of: com.android.internal.view.IInputMethodClient$Stub$Proxy@1cc2722 attribute=null, token = android.os.BinderProxy@b1f9f7d

But as you can see, the notification event actually triggers. Any help would be appreciated. Working on a PR right now but ofc I can't submit it like this.

macdonst commented 6 years ago

@Moghul you can send a PR when you feel it's ready.

Moghul commented 6 years ago

I'll do it after this bug is fixed.

Moghul commented 6 years ago

If anyone has any improvements or corrections, please speak up, I'd love to get this one merged. @macdonst do you think we could do another 2.1.x version with this change so that I can keep using cordova-android 6.3.0?

chiemekailo commented 4 years ago

Hi @moghul, I know it has been a while with this thread, but I use the phonegap-plugin-push v2.3.0 and looking at plugin.xml I can see that your fine hack was not merged? The Yaaaas, it works! post.

I would like to apply same to my project, but my problem is that when I build with cordova build android command, cordova first sets the gradle version to gradle-4.4-all.zip, which messes up my build. I use gradle-4.1-all.zip with ./gradlew build from the project folder.

It also returns some app level dependencies to versions that would not build, as well as resets the settings.gradle file which clears some of my manual library includes. So unless i am removing and adding back the specific plugin with your hack, I cannot get it on board my android build.

So, Do You know how I may apply your hack directly to the files in Android Studio?

Actually, I just need to manually configure the below intent properly. I get the Unresolved Class error for BackgroundNotificationActivity when I manually add it to AndroidManifest.xml file. The error further reads Validates resource references inside Android XML files.

<activity android:name="com.adobe.phonegap.push.BackgroundNotificationActivity">
   <intent-filter>
      <action android:name="com.opeepl.background.MESSAGING_EVENT" android:permission="${applicationId}.permission.BackgroundNotificationActivity" />
      <category android:name="android.intent.category.DEFAULT" />
  </intent-filter>
</activity>

Alternatively, I think it is possible to manipulate the cordova pre & post build hooks, but I dont know how to do this.

For my Project I use the following:

Installed platforms:
  android 7.1.0
  browser 5.0.3
  ios 4.5.5

Ionic:
   ionic (Ionic CLI)  : 4.6.0
   Ionic Framework    : ionic-angular 3.9.2
   @ionic/app-scripts : 3.2.1

Cordova:
   cordova (Cordova CLI) : 8.1.2 (cordova-lib@8.1.1)
   Cordova Platforms     : android 7.1.0, browser 5.0.3, ios 4.5.5
   Cordova Plugins       : cordova-plugin-ionic-keyboard 2.1.3, cordova-plugin-ionic-webview 1.2.1, (and 28 other plugins)

System:
   Android SDK Tools : 26.1.1 (/Users/chiemekailo/Library/Android/sdk)
   ios-deploy        : 1.9.4
   NodeJS            : v11.6.0 (/Users/chiemekailo/.nvm/versions/node/v11.6.0/bin/node)
   npm               : 6.12.0
   OS                : macOS Mojave
   Xcode             : Xcode 10.1