Skip to content
This repository has been archived by the owner on Sep 4, 2020. It is now read-only.

Android notification data pass #2369

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Moghul
Copy link

@Moghul Moghul commented May 16, 2018

When using the notification key in combination with the data key in your FCM Android payload, it should be possible to get the values in the data payload to your JS code.

Description

In plugin.xml, a value is defined which, when present in the notification part of the payload, will trigger the a BackgroundHandlerActivity. This triggering happens when the user taps the notification.

The BackgroundHandlerActivity is more or less a copy of the PushHandlerActivity with the changes that

  1. The BackgroundHandlerActivity should only be triggered by tapping the notification when the correct click_action value is given (if this is wrong, tell me, but i have not been able to trigger it otherwise, see tests below)
  2. The BackgroundHandlerActivity always triggers the main activity, background or not. This is because otherwise, the main activity would never be focused and the app wouldn't open.

The payload may look something like this

{
    "priority" : "high",
    "notification" : {
        "title": "Title",
        "body": "Body",
        "sound": "default",
        "click_action": "com.adobe.phonegap.push.background.MESSAGING_EVENT"
    },
    "data" : {
      "more": "data goes here"
    },
    "to" : "id"
}

Related Issue

#2364

Motivation and Context

When using both the notification and the data keys in a push payload on android, it is impossible to get the information in the data payload.

How Has This Been Tested?

Code was tested with version 2.1.2 of this plugin due to cordova-android 7.1.0 requirement in latest version which I can not meet at this time.
App was built from Ubuntu 17.10.
Tests were run on Android 6.0.1 OnePlus One
Notifications were sent with Postman.

Sent notifications with and without the new key, with and without a 'notification' object in the payload, in the foreground, background, and closed.

Using data object only, no notification object
Default behaviour; the click_action is handled by the OS and will only work in the notification payload so this is just does what the existing plugin can do

Using a combination of notification and data objects in the payload
In all cases, the notification is handled by the OS and therefore there isn't any of the custom notification building that this plugin does. Note that the click_action key in the data object is the same as not having it at all.

With the key
Background - Tapping the notification passes the data to the JS and the main activity gets focused so the app opens.
Foreground - Default behaviour, data gets passed to js, no notification is created.
Closed - Tapping the notification opens the app and passes the data to the js

Without the key
Tapping the notification opens the app but no data is passed to the JS

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project. (at least I think it does)
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

docs/PAYLOAD.md Outdated
@@ -258,6 +258,24 @@ My recommended format for your push payload when using this plugin (while it dif
}
```

However, if you want to use the mixed payload, you can make it so the values in your `data` payload is passed to the app when tapping the notification by adding `click_action: com.adobe.phonegap.push.background.MESSAGING_EVENT` to your `notification` payload.
Copy link
Collaborator

Choose a reason for hiding this comment

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

click_action: com.adobe.phonegap.push.background.MESSAGING_EVENT
should read
"click_action": "com.adobe.phonegap.push.background.MESSAGING_EVENT"
for json purity and validity.

@ghost
Copy link

ghost commented May 28, 2018

How long will it take that this pull request get merged?

@Moghul
Copy link
Author

Moghul commented May 28, 2018

Probably when someone has time to do a final review and likely double-check my changes.

@ghost
Copy link

ghost commented May 28, 2018

Should I be able to use your branch with a Cordova app, without forking and making my own builds?

@ghost
Copy link

ghost commented May 30, 2018

I used the 2.2.3 version, and merged in your changes of this pull request. The notification data arrived in the JavaScript callback with the application being in background (not closed). I also tried it with a closed application, but the callback has not been called. Most probably I had something wrong in my configuration or a bug in the application.

@Moghul
Copy link
Author

Moghul commented May 30, 2018

The payload would only arrive when the notification is tapped, and if you have the correct payload in your notification. What did you try?

For me, it has worked fine.

@ghost
Copy link

ghost commented May 30, 2018

My payload was:

const payload = {
        notification: {
            title: notification.title,
            body: notification.message,
            sound: 'default',
            click_action: 'com.adobe.phonegap.push.background.MESSAGING_EVENT',
        },
        data: { ... }
    };

It was not easy to build the app with the plugin changes in my local project, seams the plugin is stored in various locations (/plugins/phonegap-plugin-push, /node_modules/phonegap-plugin-push/, ...?). So I made a local fork, and used this instead. Somehow a mess ...

@Moghul
Copy link
Author

Moghul commented May 30, 2018

Very strange. I'm not sure, but can you clone my repo? If so, try cloning it, switching branches, and installing from that instead.

When it worked from the background (not force-closed), did your in-app object passed to the notification event look the same as usual, or did it look different?

@ghost
Copy link

ghost commented May 31, 2018

So I tried it again. I removed the original plugin with cordova plugin remove phonegap-plugin-push and added the plugin with the merged changes with cordova plugin add @scope/phonegap-plugin-push. config.xml contains the plugin's configuration:

    <plugin name="phonegap-plugin-push" spec="@scope/phonegap-plugin-push@~2.2.4">
        <variable name="ANDROID_SUPPORT_V13_VERSION" value="27.+" />
        <variable name="FCM_VERSION" value="11.6.2" />
    </plugin>

After cordova build android the file platforms/android/app/src/main/AndroidManifest.xml has the additional activity:

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

So I run cordova run android --device, but I noticed that the activity is not more included in the AndroidManifest.xml file. The file BackgroundHandlerActivity.java is contained in platforms/android/app/src/main/java/..., so I assume I'm using the phonegap plugin with the merged changes.

Any ideas why the AndroidManifest.xml is is missing the activity configuration?

@Moghul
Copy link
Author

Moghul commented May 31, 2018

Did you try removing and re-adding the platform? Like remove the platform, reinstall the plugin, add the platform.

@ghost
Copy link

ghost commented May 31, 2018

Ok, found it. I had the following <edit-config /> section in my config.xml:

        <edit-config file="AndroidManifest.xml" mode="merge" target="/manifest/application">
            <application allowBackup="false" />
        </edit-config>

which resulted in the warning during the build Conflict found, edit-config changes from config.xml will overwrite plugin.xml changes. I removed it, and now the activity is included in the AndroidManifest.xml after a cordova build android.

@ghost
Copy link

ghost commented May 31, 2018

Finally tested now with the merged changes, and all 3 tests worked as expected: open (foreground) app, background app, and closed (coldstart) app!

@Moghul
Copy link
Author

Moghul commented Jun 1, 2018

I wasn't worried, who says I was worried?

@ghost
Copy link

ghost commented Jul 27, 2018

When will this be merged? Waiting since 2 months now ...

@bascoder
Copy link

@fredgalvao

When will this PR be merged?
Currently, we are using @Moghul's fork for an app that is being prepared for production.
It works very well, but it would be nice to switch back to the main plugin before going live.

@ghost
Copy link

ghost commented Jul 27, 2018

What version, and is it available in the npm registry?

@bascoder
Copy link

@maitscha

It's not available on a public npm registry, but you can use it by adding the following lines to your config.xml file:

<plugin name="phonegap-plugin-push" spec="git+https://github.com/Moghul/phonegap-plugin-push.git#android-notification-data-pass">
        <variable name="ANDROID_SUPPORT_V13_VERSION" value="27.+" />
        <variable name="FCM_VERSION" value="11.6.2" />
</plugin>

@fredgalvao
Copy link
Collaborator

@bascoder I don't have that answer, unfortunately. cc: @macdonst or @jcesarmobile would be way better at taking a look.

@rosses
Copy link

rosses commented Oct 1, 2018

This PR is excellent. We have already tested it in 2 app and it works as expected

@mzealey
Copy link

mzealey commented Nov 21, 2018

This is exactly what we need, and is similar in concept to https://github.com/fechanique/cordova-plugin-fcm which I have successfully used before in previous apps. I have now rebased this patchset against both

@mzealey
Copy link

mzealey commented Nov 21, 2018

Confirm that this is working correctly with click_action.

@Moghul
Copy link
Author

Moghul commented Nov 27, 2018

@macdonst @jcesarmobile

Hello! This pull request has been up for some time, and I'm wondering why it's not being merged. As you can see, people are eager to get it merged so they can return to a proper version of the plugin, and so am I.

Could one of you guys take a look and maybe let me know exactly what more I need to do to get this merged?

@Moghul
Copy link
Author

Moghul commented Jan 7, 2019

I'll keep merging master into my branch until someone takes care of this pull request, so you guys can keep using the fork.

I'm not sure what more I can do, since no one will take notice.

@fuyucn
Copy link

fuyucn commented Jan 7, 2019

@Moghul Thanks

@Moghul
Copy link
Author

Moghul commented Feb 22, 2019

@SimplyLab as @bascoder said, your plugin definition in your config.xml will look something like this:

<plugin name="phonegap-plugin-push" spec="git+https://github.com/Moghul/phonegap-plugin-push.git#android-notification-data-pass">
        <variable name="ANDROID_SUPPORT_V13_VERSION" value="27.+" />
        <variable name="FCM_VERSION" value="11.6.2" />
</plugin>

It's the first line that points to the branch.

@CSobol
Copy link

CSobol commented Feb 25, 2019

This really needs merged in. It's core FCM functionality.

My project has cherry picked and is now maintaining a custom fork of this project, which is really silly.

@madakopihub
Copy link

+1 for this merge,,
stuck about 2 days because the background notification issue and resolved with this tackle,, many thanks @Moghul 🙏

@niconaso
Copy link

Thanks @Moghul, excellent job!!
I tested on Android 8 and 9 and it works perfectly but on Android 7.1.2 it seems to not be working very well. If you send the "click_action" param the push arrives but when you click on it the app doesn't open at all.

@Moghul
Copy link
Author

Moghul commented Mar 15, 2019

@niconaso I don't have a phone with that version of Android. If you can figure out what the problem is and fix it, you can send your own pull request. Have you tried with an older version of Android too?

@niconaso
Copy link

Thanks @Moghul, excellent job!!
I tested on Android 8 and 9 and it works perfectly but on Android 7.1.2 it seems to not be working very well. If you send the "click_action" param the push arrives but when you click on it the app doesn't open at all.

It works well with current configuration 😄 :

<plugin name="phonegap-plugin-push" spec="git+https://github.com/Moghul/phonegap-plugin-push.git#android-notification-data-pass">
  <variable name="ANDROID_SUPPORT_V13_VERSION" value="27.1.0" />
  <variable name="FCM_VERSION" value="17.4.0" />
</plugin>

@Moghul
Copy link
Author

Moghul commented Mar 15, 2019

So it wasn't the pull request, it was the configuration in your config.xml file?

@niconaso
Copy link

So it wasn't the pull request, it was the configuration in your config.xml file?

Exactly! To work with Android 7.1.2 is mandatory to specify FCM_VERSION = 17.4.0

Thank you very much!

@niconaso
Copy link

Last question. Have you tried this patch with APK in release mode?

@Moghul
Copy link
Author

Moghul commented Mar 17, 2019

Yes, we are actively using this branch in production.

@WhistlerChoi
Copy link

Best ... Thank you for killing my troubles.

@pabloleone
Copy link

I can confirm this solution works for Android 8!

I've applied the changes manually as I couldn't find a way to install this branch specifically. After applying the changes manually I've removed and added the platform again.

I've deployed the app to my phone and used the following code to test the functionality:

curl https://fcm.googleapis.com/fcm/send -X POST \ --header "Authorization: key=server key" \ --Header "Content-Type: application/json" \ -d '{ "to": "device fcm token" "notification": { "title": "Test Notification", "body": "This offer expires at 11:30 or whatever", "notId": 10, "click_action": "com.adobe.phonegap.push.background.MESSAGING_EVENT" }, "data": { "surveyID": "ewtawgreg-gragrag-rgarhthgbad" } }'
I've tested with the app killed, in the background and foreground. It worked perfectly after a day of wasting time looking for a solution.

Please accept this PR!

@Moghul
Copy link
Author

Moghul commented May 29, 2019

@pabloleone as @niconaso has said, it's enough to add an entry like this to your config.xml file in your cordova project

<plugin name="phonegap-plugin-push" spec="git+https://github.com/Moghul/phonegap-plugin-push.git#android-notification-data-pass">
  <variable name="ANDROID_SUPPORT_V13_VERSION" value="27.1.0" />
  <variable name="FCM_VERSION" value="17.4.0" />
</plugin>

Or running

cordova plugin add phonegap-plugin-push@git+https://github.com/Moghul/phonegap-plugin-push.git#android-notification-data-pass

and modifying the values in config.xml later.

@iurynogueira
Copy link

Aprove this SHIT PLS!

@Moghul
Copy link
Author

Moghul commented Jun 10, 2019

For everyone eager to get this pull request merged, please refer to #2753 . When and if there is time, or when and if this repo is taken over, someone will take care of it. In the meantime, refer to the comments above for how to implement the feature, or fork and manage your own fork.

@digaus
Copy link

digaus commented Jul 19, 2019

@Moghul

This works great in Android but on iOS when I recieve more than one notification only the first one which is clicked will be triggered in the app. All others are not recognzied. Do you experience the same issue?

@Moghul
Copy link
Author

Moghul commented Jul 19, 2019

@digaus this pull request is for android notification data pass. Your problem is unrelated to this.

@digaus
Copy link

digaus commented Jul 19, 2019

@digaus this pull request is for android notification data pass. Your problem is unrelated to this.

I know but I thought you might also be using iOS aswell. Ty anyways.

@Moghul
Copy link
Author

Moghul commented Jul 19, 2019

I am using iOS, but that problem is unrelated to this pull request. You want to make an issue on the main repo.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet