Populate userInfo on android, update RNCPushNotificationIOS #1516

lukebars · 2020-07-08T14:47:33Z

This aligns the object returned by onNotification.

Currently onNotification returns:

{
   "android":{
      "android.intent.extra.ALARM_COUNT":1,
      "autoCancel":false,
      "finish":[],
      "fireDate":1594216704146,
      "foreground":false,
      "id":"2941",
      "largeIcon":"ic_notification",
      "message":"message",
      "notificationId":2941,
      "smallIcon":"ic_notification",
      "title":"hello",
      "userInfo":{
         "screen":"welcome"
      },
      "userInteraction":true
   },
   "ios":{
      "alert":"message",
      "badge":0,
      "data":{
         "id":"6380",
         "screen":"welcome"
      },
      "finish":[
         "Function finish"
      ],
      "fireDate":"2020-07-08T16:59:14.916+03:00",
      "foreground":true,
      "id":"undefined",
      "message":"message",
      "sound":"default",
      "userInteraction":false
   }
}

As you can see iOS returns userInfo as data, and android returns userInfo as userInfo. Also - id is not being correctly populated on iOS.

My proposal would return this object onNotification:

{
   "android":{
      "android.intent.extra.ALARM_COUNT":1,
      "autoCancel":false,
      "data":{
         "id":"9683",
         "screen":"welcome"
      },
      "finish":[
         "Function finish"
      ],
      "fireDate":1594219524047,
      "foreground":false,
      "id":"9683",
      "largeIcon":"ic_notification",
      "message":"message",
      "notificationId":9683,
      "smallIcon":"ic_notification",
      "title":"hello",
      "userInfo":{
         "screen":"welcome"
      },
      "userInteraction":true
   },
   "ios":{
      "alert":"message",
      "badge":0,
      "data":{
         "id":"2593",
         "screen":"welcome"
      },
      "finish":[
         "Function finish"
      ],
      "fireDate":"2020-07-08T17:41:44.910+03:00",
      "foreground":true,
      "id":"2593",
      "message":"message",
      "sound":"default",
      "userInteraction":false
   }
}

This would also allow us to provide data when getting scheduledNotifications, which I would implement little bit later. P.S. currently on iOS alert(title) is wrong, that's because iOS doesn't return notification title. I will raise a PR in RNC repo for that. Naming should be tweaked too (android - title, iOS - alert). Another possible solution would be naming data as userInfo

Patch 2

* dev: (48 commits) Release v4.0.0. Use SecureRandom and remove `onNotification` call for Scheduled notifications when there is no user interation. Update CHANGELOG.md replaced java.util.Random with java.security.SecureRandom in RNPushNotification.java Remove WAKE_LOCK from documentation. zo0r#1494 Add `onRegistrationError`. Allow override the channel name and description in App. Add the detection of blocked channels. Fix notif reference undefined. Fix typo. Add notification id to iOS if userInfo doesn't exist `userInfo` is now populated with id by default to allow operation based on `id` "@react-native-community/push-notification-ios": "^1.2.2" add docs add example button to log scheduled notifications bump @react-native-community/push-notification-ios to v1.2.1 format date for scheduled notifications object map scheduled local notifications to united return object for both platforms add missing getters for notification attributes change naming of notification params returned on android, return date in unix timestamp as before format android getScheduledLocalNotifications return object to match iOS return object ...

Fixed version from 4.4.0 to 4.0.0

Update CHANGELOG.md

…ample

…h-notification into patch-3

lukebars · 2020-07-08T14:50:32Z

cc @Dallas62

Dallas62 · 2020-07-13T11:50:30Z

Hi @lukebars
Missed your PR in tons of notifications 😅
Thanks for this PR, changes looks great!
I will look into it ASAP 😉

lukebars · 2020-07-16T13:20:57Z

@Dallas62 did you manage to look into it?

Dallas62 · 2020-07-16T14:03:56Z

Will check this this afternoon

Dallas62 · 2020-07-16T15:23:22Z

Hi @lukebars
What's strange is that there is no userInfo in the iOS return

lukebars · 2020-07-16T16:48:01Z

Hmmm. I'll check that in few hours. Very strange.

lukebars · 2020-07-17T09:02:42Z

@Dallas62 thing is that iOS returns userInfo as data, so we should choose do we return it as data to keep consistency between this lib and ios one, or should we return it as userInfo. Also we could return both, but that wouldn't make sense..

I could remove userInfo from Android to keep it the same way as in iOS

Dallas62 · 2020-07-18T09:04:57Z

I will take a close look to this, because it's strange to set userInfo in iOS, use it every where but return it as data.
Busy this weekend, will dig into it on monday.

Dallas62 · 2020-07-20T13:08:31Z

@Dallas62 thing is that iOS returns userInfo as data, so we should choose do we return it as data to keep consistency between this lib and iOS one, or should we return it as userInfo. Also we could return both, but that wouldn't make sense..

I could remove userInfo from Android to keep it the same way as in iOS

Hi @lukebars

I'm not totally convinced by this kind of "renaming" as an optimal solution, but it's probably the best way to unify Android and iOS actually. So, no problem to remove userInfo from Android and keep it the same way as in iOS.
We should probably check that Scheduled Notification accept userInfo and that there is no mix of naming when the application start in Android.

In the future, I think we should rewrite the API to unify and clean it. Also make a clear documentation. 😄

lukebars · 2020-07-20T14:24:07Z

@Dallas62 or we could currently use userInfo on both platforms instead of data. Maybe that would be more clear.

Dallas62 · 2020-07-20T14:29:51Z

Not sure it's better, because this is the name of the attribute of remote notifications.

lukebars · 2020-07-20T14:56:28Z

Ok, sure.
I've raised a PR for getting alertTitle in iOS @rnc, as soon as it's merged I'll push some changes here to keep it straightforward.

lukebars · 2020-07-28T07:29:17Z

Looks good

lukebars · 2020-07-29T08:15:38Z

Are we ready for some sort of release?

Dallas62 · 2020-07-29T08:54:29Z

Hi @lukebars
I'm waiting for feedback from @mathiasmoeller on a fix introduced in this PR. (#1501)
I will re-check it this week and merge ASAP.

lukebars · 2020-07-29T10:12:25Z

Hey @Dallas62, I've noticed a strange behavior on android that comes from handlePopInitialNotification.

When app is launched from killed state / fresh launch, and you schedule a first notification, onNotification fires twice for the first one. Native listener fires only one.

I see that if you comment out this._onNotification(firstNotification, true);, this behavior stops.

Was it intentional to do this or is it a bug?

Dallas62 · 2020-07-29T13:25:14Z

@lukebars This is strange, I don't see this behavior

Dallas62 · 2020-07-29T14:28:18Z

Can you test on the example project (without change expect setting the PR), if his is happening this is probably in the library.

In your code, does Notifications.scheduleLocalNotification(...); is trigger by an event such as button pressed ?

Dallas62 · 2020-07-29T14:41:17Z

Can you give the steps to reproduce it on the example project ?

Dallas62 · 2020-07-29T14:58:25Z

I don't see any problem, does the log:

console.log('NotificationHandler:', notification);

inside NotifSetup is right ? it's written NotificationHandler

Dallas62 · 2020-07-29T17:49:29Z

Yes it's logged twice the notification, but the example Application has the same console.log in NotificationHandler and in the NotifSetup you provided, which explain this behavior

lukebars · 2020-07-30T11:52:18Z

@Dallas62 Oh I was wrong about that 🤦

I've actually managed to reproduce it now. We're using branch in our projects for analytics and it requires setting intent.

It's easy to reproduce with only a minor change in repo example too: example/android/app/src/main/java/com/example/MainActivity.java:

package com.example;
// Add this:
import android.content.Intent;

public class MainActivity extends ReactActivity {

  // Add this:
  @Override
  public void onNewIntent(Intent intent) {
      super.onNewIntent(intent);
      setIntent(intent); // this is what causes double onNotif call when scheduling one on a fresh launch and clicking it
  }

// Everything else
...

Dallas62 · 2020-07-31T12:32:15Z

Hi @lukebars
This change is wrong, the first parameter of .bind() is context. By changing this, the onNotification on the listener will never trigger with the right parameter.

A correct change would be:

  AppState.addEventListener('change', (state) => {
    handlePopInitialNotification(state);
  });

But you will see the same result.

There is a protection to block double trigger based on notification.id. Do you see any id in the notification ?

lukebars · 2020-07-31T12:41:14Z

@Dallas62 eh, my bad.

Both onNotification messages fire with "id": "1".

Looks like if setIntent is configured in MainActivity.java, popInitialNotification is only needed for iOS, because onNotification fires even with listener commented out.

Dallas62 · 2020-07-31T12:49:01Z

What is strange is that you are talking about double trigger even when you comment this._onNotification(firstNotification, true);

But it's impossible to get onNotification triggered whitout this line. So it can't be trigger twice and only once after removing this line.

lukebars · 2020-07-31T14:03:46Z

Nope. When that line is commented out, it get triggered only once on Android.

Have you tried adding

  @Override
  public void onNewIntent(Intent intent) {
      super.onNewIntent(intent);
      setIntent(intent);
  }

to MainActivity.java ?

Instant double call without any code modification inside the lib.

Dallas62 · 2020-07-31T14:05:35Z

I'm currently testing it, and I found the reason of that. Making more tests.
popInitialNotification is working fine, this is a problem with onNewIntent inside the library which looks like useless.

lukebars · 2020-07-31T14:08:58Z

That explains everything. Looks like it's doing the same thing as the js initial notif part, only a bit worse. Nice catch 🚀

Dallas62 · 2020-07-31T14:13:24Z

Yes but it's probably not so easy. on iOS, clicking on a notification when the Application is opened doesn't trigger the current part of "initialNotification", only Android do that

Dallas62 · 2020-07-31T14:14:42Z

Also, the value of foreground looks not valid.

Fix foreground value.

Dallas62 · 2020-07-31T16:09:36Z

I think it's good now 😉
Nice investigations !

lukebars · 2020-08-03T07:48:51Z

@Dallas62 awesome work! Tested it out and it looks really good 🚀

Are we getting close to closing this and releasing it out to the daylight?

Dallas62 · 2020-08-03T07:50:30Z

Hi @lukebars
I was waiting for your feedback, will take a look today 😉

Dallas62 · 2020-08-03T19:00:30Z

Done ✅

Found the same issue you reported on iOS while testing 😄

Thanks a lot for this PR !

lukebars and others added 16 commits June 10, 2020 14:26

return a Promise in getScheduledLocalNotifications

c90fc6e

updated example/App.js

a5cdcda

Merge pull request #1 from lukebars/patch-2

b76a4e4

Patch 2

Create FUNDING.yml

9aa88d6

Fix custom link

da34ac1

Bump package.json to 4.0.0

86af98d

Update CHANGELOG.md

2816253

Fixed version from 4.4.0 to 4.0.0

Merge pull request zo0r#1508 from Brianop/patch-1

1b73593

Update CHANGELOG.md

add data to android notification when scheduling

74e9b8b

"@react-native-community/push-notification-ios": "^1.3.0"

454af8c

reflecting userInfo working on both platforms in documentation and ex…

579ffb8

…ample

Merge branch 'master' of https://github.com/lukebars/react-native-pus…

0c828e9

…h-notification into patch-3

revert changes

3e19105

populate userInfo with id on both platforms

85952df

return notification id in onNotification method on iOS

22d8c75

Dallas62 self-assigned this Jul 13, 2020

Dallas62 approved these changes Jul 16, 2020

View reviewed changes

Dallas62 mentioned this pull request Jul 27, 2020

onNotification not being called when app was killed on iOS #1501

Closed

This comment has been minimized.

Sign in to view

Fix behaviour of popInitialNotification and onNotification.

762f4c0

Fix foreground value.

Dallas62 merged commit ab8e29f into zo0r:dev Aug 3, 2020

Populate userInfo on android, update RNCPushNotificationIOS #1516

Populate userInfo on android, update RNCPushNotificationIOS #1516

Conversation

lukebars commented Jul 8, 2020 • edited Loading

lukebars commented Jul 8, 2020

Dallas62 commented Jul 13, 2020

lukebars commented Jul 16, 2020

Dallas62 commented Jul 16, 2020

Dallas62 commented Jul 16, 2020

lukebars commented Jul 16, 2020

lukebars commented Jul 17, 2020 • edited Loading

Dallas62 commented Jul 18, 2020

Dallas62 commented Jul 20, 2020

lukebars commented Jul 20, 2020 • edited Loading

Dallas62 commented Jul 20, 2020

lukebars commented Jul 20, 2020

lukebars commented Jul 28, 2020

lukebars commented Jul 29, 2020

Dallas62 commented Jul 29, 2020

lukebars commented Jul 29, 2020

Dallas62 commented Jul 29, 2020

This comment has been minimized.

Dallas62 commented Jul 29, 2020

This comment has been minimized.

Dallas62 commented Jul 29, 2020

This comment has been minimized.

Dallas62 commented Jul 29, 2020

This comment has been minimized.

Dallas62 commented Jul 29, 2020 • edited Loading

lukebars commented Jul 30, 2020 • edited Loading

This comment has been minimized.

Dallas62 commented Jul 31, 2020 • edited Loading

lukebars commented Jul 31, 2020

Dallas62 commented Jul 31, 2020

lukebars commented Jul 31, 2020

Dallas62 commented Jul 31, 2020

lukebars commented Jul 31, 2020

Dallas62 commented Jul 31, 2020

Dallas62 commented Jul 31, 2020

Dallas62 commented Jul 31, 2020

lukebars commented Aug 3, 2020

Dallas62 commented Aug 3, 2020

Dallas62 commented Aug 3, 2020

lukebars commented Jul 8, 2020 •

edited

Loading

lukebars commented Jul 17, 2020 •

edited

Loading

lukebars commented Jul 20, 2020 •

edited

Loading

Dallas62 commented Jul 29, 2020 •

edited

Loading

lukebars commented Jul 30, 2020 •

edited

Loading

Dallas62 commented Jul 31, 2020 •

edited

Loading