OneSignal / OneSignal-Android-SDK

OneSignal is a free push notification service for mobile apps. This plugin makes it easy to integrate your native Android or Amazon app with OneSignal. https://onesignal.com
Other
604 stars 368 forks source link

OneSignal.NotificationOpenedHandler null notification.payload and empty/null groupedNotifications #146

Closed kistitan closed 7 years ago

kistitan commented 7 years ago

In the notificationOpened method of OneSignal.NotificationOpenedHandler we receive OSNotificationOpenResult.OSNotification with null payload and empty/null groupedNotifications.

In theory we do not send empty push messages. In what circumstances can this happen?

Thank you

jkasten2 commented 7 years ago

@kistitan These properties should always be set. Are you able to reproduce the issue when sending an notification through OneSignal? Via the REST API or Dashboard? If so please attach the full list of options here.

Or are you able to reproduce the issue by sending a push to your app directly through FCM/GCM?

In either case our SDK should either omit the call or correctly set properties.

Thanks.

kistitan commented 7 years ago

Hi @jkasten2

Thank you for your reply. We user Crashlyticis library to monitor crashes and we saw there were some crashes that effected about 1% of our user base. Crashes could occur based on our code if the mentioned condition is there, but as it happens only for 1% of the users we cannot intentionally reproduce it.

We will add some additional logging to our next version to have more insight. I will come back with those soon.

kistitan commented 7 years ago

hi there,

Sorry for the late reply. So we get notifications like this when the user opens them:

json: {"action":{"type":0},"notification":{"androidNotificationId":-36511111@,"shown":true,"isAppInFocus":true,"displayType":0}}

How this is possible?

kistitan commented 7 years ago

Can I have an update on this? It seems it cases even more issue, as we keep getting complaint from users about phantom notification count on Samsung devices.

jkasten2 commented 7 years ago

@kistitan Sorry for the delay. For the payload issue can you share the code you used to print the JSON payload from above? Also could you share the full crash log from Crashlyticis?

The badge icon count should sync up once the user closes the app(or Android closes it to free RAM) and opens it again. Are you able to reproduce this issue on your devices?

Please also include the version of the SDK you are using.

Thanks.

kistitan commented 7 years ago
public class OnNotificationOpenedHandler implements OneSignal.NotificationOpenedHandler {

    @Override
    public void notificationOpened(OSNotificationOpenResult osNotificationOpenResult) {
        if (osNotificationOpenResult == null || osNotificationOpenResult.notification == null) {
            Crashlytics.logException(new Exception("Null notification from one signal"));
            return;
        }
        if (!isValidNotification(osNotificationOpenResult)) {
            //Investigating further some strange exceptions at this point
            Crashlytics.logException(new Exception("Invalid notification from one signal; json: " + osNotificationOpenResult.toJSONObject().toString()));
            return;
        }
//...
    }

    private boolean isValidNotification(OSNotificationOpenResult osNotificationOpenResult) {
        List<OSNotificationPayload> groupedNotifications = osNotificationOpenResult.notification.groupedNotifications;
        if (groupedNotifications == null || groupedNotifications.isEmpty()) {
            return osNotificationOpenResult.notification.payload != null;
        }
        return true;
    }
}
kistitan commented 7 years ago
Non-fatal Exception: java.lang.Exception: Invalid notification from one signal; json: {"action":{"type":0},"notification":{"isAppInFocus":false,"shown":true,"androidNotificationId":-658134862,"displayType":0}}
       at com.bitlove.fetlife.inbound.OnNotificationOpenedHandler.notificationOpened(OnNotificationOpenedHandler.java:26)
       at com.onesignal.OneSignal.fireNotificationOpenedHandler(OneSignal.java:1009)
       at com.onesignal.OneSignal.runNotificationOpenedCallback(OneSignal.java:954)
       at com.onesignal.OneSignal.handleNotificationOpen(OneSignal.java:1041)
       at com.onesignal.NotificationOpenedProcessor.processIntent(NotificationOpenedProcessor.java:101)
       at com.onesignal.NotificationOpenedProcessor.processFromActivity(NotificationOpenedProcessor.java:57)
       at com.onesignal.NotificationOpenedReceiver.onReceive(NotificationOpenedReceiver.java:11)
       at android.app.ActivityThread.handleReceiver(ActivityThread.java:2609)
       at android.app.ActivityThread.access$1700(ActivityThread.java:151)
       at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1380)
       at android.os.Handler.dispatchMessage(Handler.java:102)
       at android.os.Looper.loop(Looper.java:135)
       at android.app.ActivityThread.main(ActivityThread.java:5254)
       at java.lang.reflect.Method.invoke(Method.java)
       at java.lang.reflect.Method.invoke(Method.java:372)
       at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:903)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:698)
kistitan commented 7 years ago

Based on Crashlitics it happens for users with various Android version from 5.0.1 to 7.1.1; 63% Samsung, 6-6-6% Dell, ZTE, LGE, the rest is others.

kistitan commented 7 years ago

Note: the json object says the notification is displayed, however based on the feedback it is not, but the badge count is still displays: one notification.

I cannot reproduce it on my Nexus 5S, but it can be also because my test accounts does not receive the problematic type of notifications.

I hope this helps.

jkasten2 commented 7 years ago

@kistitan I have fixed the null notification.payload issue in commit 777b7b561abdafca237b3c25dcf423017fe57b3d. The issue would occur after opening a summary notification (AKA stacked / grouped notification) that was received 4 or more weeks ago.

The Samsung badge issue isn't related to the notification.payload issue. You can follow the issue below for an updates on this. https://github.com/OneSignal/OneSignal-Android-SDK/issues/186

kistitan commented 7 years ago

Hi @jkasten2,

Thanks for the fix. About the Samsung badge issue, I still think it might be related as if I added the following line for each invalid OneSignal notification it fixed the Samsung badge issue completely.

OneSignal.cancelNotification(osNotificationOpenResult.notification.androidNotificationId);

All version without this fix has sometimes the stucked Samsung Badge, versions with this workaround does not have it at all.

jkasten2 commented 7 years ago

@kistitan The fix won't have an effect on badges. The same badge update call is made based on the number of notifications in the sql lite database the SDK uses.

Could you open a new issue for the Badge issue? Since I don't believe it is related to issue #186 either since the cancel is working for you. Please include Android version, device model, SDK version, and exact steps to reproduce the issue.