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

Added method getScheduledLocalNotifications() #1466

Merged
merged 16 commits into from
Jun 9, 2020
Merged

Conversation

lukebars
Copy link
Contributor

@lukebars lukebars commented Jun 5, 2020

This method is really useful for organizing notifications, e.g. cancelling some of already scheduled ones. Solves #673

For iOS it uses already existing method

I've written a method for android which does the same.

@lukebars
Copy link
Contributor Author

lukebars commented Jun 5, 2020

If everything is a-okay, I'll write some documentation for this.

Currently notifications are returned this way, not sure if we should trim android part or not:

Android:

[{
    "actions": null,
    "autoCancel": false,
    "bigText": null,
    "color": null,
    "fireDate": 1591359819185,
    "group": null,
    "id": "1890571246",
    "largeIcon": "ic_notif",
    "message": "nope",
    "number": null,
    "ongoing": false,
    "playSound": false,
    "repeatTime": 0,
    "repeatType": null,
    "smallIcon": "ic_notif",
    "sound": null,
    "subText": null,
    "tag": null,
    "ticker": null,
    "title": "hello",
    "userInteraction": false,
    "vibrate": false,
    "vibration": 0
}], 

iOS:

[{
    "alertAction": null,
    "alertBody": "nope",
    "applicationIconBadgeNumber": 0,
    "category": null,
    "fireDate": "2020-06-05T15:34:03.327+03:00",
    "remote": false,
    "soundName": "default",
    "userInfo": {
        "id": "1890571246",
    }
}]

@Dallas62
Copy link
Collaborator

Dallas62 commented Jun 5, 2020

Hi @lukebars
Thanks for this PR,
I think we should change the output of this function to unify between iOS and Android.

Not all parameters, but we can try at least shared params.

Something like

[{
  "id": "12345",
  "date": new Date(),
  "message": "Body Message",
  "number": 0,
  "sound": "default",
  "raw": {
    // full output iOS:
    "alertAction": null,
    "alertBody": "Body Message",
    "applicationIconBadgeNumber": 0,
    "category": null,
    "fireDate": "2020-06-05T15:34:03.327+03:00",
    "remote": false,
    "soundName": "default",
    "userInfo": {
        "id": "12345",
    }

    // full output Android:
    "actions": null,
    "autoCancel": false,
    "bigText": null,
    "color": null,
    "fireDate": 1591359819185,
    "group": null,
    "id": "12345",
    "largeIcon": "ic_notif",
    "message": "Body Message",
    "number": null,
    "ongoing": false,
    "playSound": false,
    "repeatTime": 0,
    "repeatType": null,
    "smallIcon": "ic_notif",
    "sound": null,
    "subText": null,
    "tag": null,
    "ticker": null,
    "title": "Notification",
    "userInteraction": false,
    "vibrate": false,
    "vibration": 0
  }
}]

@lukebars
Copy link
Contributor Author

lukebars commented Jun 8, 2020

@Dallas62 I've decided that the best solution would be to keep it as clean and unified as possible, and staying true to RNCPushNotificationsIOS structure on both platforms.

This would be the structure I've prepared to return on both platforms rn:

"alertAction": null,
"alertBody": "Body text",
"alertTitle": "Notification title",
"applicationIconBadgeNumber": null,
"fireDate": "2020-06-08T13:01:52.542+03:00",
"id": 1793677506,
"remote": false,
"repeatInterval": null,
"soundName": null,

I've raised a PR in react-native-community that will return alertTitle and repeatInterval on iOS.

@Dallas62
Copy link
Collaborator

Dallas62 commented Jun 8, 2020

Nice for the iOS PR ! 😉
The problem of using:

  • alertBody
  • alertTitle
  • applicationIconBadgeNumber
  • fireDate (wrong type and named date)

This is not the name used in the initial method PushNotification.localNotificationSchedule. This might lead to confusion.
alertAction is iOS-only, so no problem with this.

This changes can be made in the JS part.

@lukebars
Copy link
Contributor Author

lukebars commented Jun 8, 2020

Sounds good. I'll reflect the changes

@lukebars
Copy link
Contributor Author

lukebars commented Jun 8, 2020

@Dallas62 iOS changes have been merged and released to @react-native-community/push-notification-ios: v1.2.1, so I've bumped the version in package.json.

I've also updated the code for getScheduledLocalNotifications, currently it returns this on both platforms:

soundName: 'default',
repeatInterval: 0,
id: 5,
date: 2020-06-08T12:07:30.039Z, // Date instance (new Date(fireDate))
number: 0,
message: 'Notif message',
title: 'Notif title',

@lukebars lukebars changed the base branch from dev to master June 8, 2020 12:14
@lukebars lukebars changed the base branch from master to dev June 8, 2020 12:14
@Dallas62
Copy link
Collaborator

Dallas62 commented Jun 8, 2020

Thanks for all these changes, I will find time the week to review and merge it 😉

@lukebars
Copy link
Contributor Author

lukebars commented Jun 9, 2020

@Dallas62 thanks for your collaboration! I've bumped the RNCPushNotificationIOS to 1.2.2 as 1.2.1 had a bug. Also, it looks like dev branch is not in sync with master, some things on example are little bit broken, like getChannels is undefined, and getDeliveredNotifications is not there too.

return ({
soundName: notif.soundName,
repeatInterval: notif.repeatInterval,
id: notif.userInfo?.id,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a bit confused about this line,
I totally agree iOS should have the notification ID, but by default, userInfo is set to {}.
Not sure if we need to set a default value on userInfo (notificationId ?) or let this field empty when the developer doesn't set it in userInfo...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's why it's optional chained here. It would result in undefined if it is not set. Currently we're using userInfo.id for cancelling notifications by id, so it makes sense to me.

Copy link
Contributor Author

@lukebars lukebars Jun 9, 2020

Choose a reason for hiding this comment

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

But setting the id as default when scheduling the notification would be an awesome feature IMHO.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will look into it, thanks for the PR !

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just added the commit here:
6bcc2a9

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good stuff! 🙏

@Dallas62 Dallas62 merged commit 89ea506 into zo0r:dev Jun 9, 2020
@jaikantshikray
Copy link

when getScheduledLocalNotifications() will live for android in the master branch ?

@Dallas62
Copy link
Collaborator

Dallas62 commented Jul 1, 2020

Hi,
This will be available at the end of the week.

@jaikantshikray
Copy link

Great i am waiting for this change just moved to react-native-push-notification for local notification scheduling from react native firebase 6.0

@lukebars
Copy link
Contributor Author

lukebars commented Jul 1, 2020

@Dallas62 what about onNotification listener methods? Are you planning on releasing those? I'd be glad to help if you're facing any problems regarding that functionality

@Dallas62
Copy link
Collaborator

Dallas62 commented Jul 1, 2020

Hi @lukebars,
Which functionality? Actions ?

@lukebars
Copy link
Contributor Author

lukebars commented Jul 1, 2020

I might be wrong, but I think I've read that you were planning to add methods e.g. onNotificationClicked or onNotificationDisplayed or something like that.

@Dallas62
Copy link
Collaborator

Dallas62 commented Jul 1, 2020

I didn't planned this kind of listener, but I was looking to implement actions for iOS.
But was too busy last weeks, I will release the dev at the end of the week with few more changes.
Then try to find time for iOS actions 😄

@jaikantshikray
Copy link

i have used the dev branch code of @lukebars the getScheduleLocalNotifications() function still not working in there for android i might be wrong but can you confirm it will work for android as well after releasing dev branch code at the end of the week ?

@lukebars
Copy link
Contributor Author

lukebars commented Jul 1, 2020

@jaikantshikray Hmm.. It's working for me. I'm currently using it in production in one of our apps. Can you share some code of usage?

@jaikantshikray
Copy link

@lukebars i have copied the java files from your dev branch and replacing the code of react-native-push-notifications java files you have created some new files as well and from your example file i have copied how you are using the getScheduledLocalNotifications() but unfortunately it is not working for me

getScheduledLocalNotifications(callback) {
PushNotification.getScheduledLocalNotifications(callback);
}

MobxStore.getScheduledLocalNotifications((notifs)=>{
console.log("notifs array ",notifs)
});

i have tried this as well

MobxStore.getScheduledLocalNotifications().then(notifs => console.log(notifs));

but no response neither a crash

@Dallas62
Copy link
Collaborator

Dallas62 commented Jul 2, 2020

Hi,
Why didn’t you install directly dev branch ? For testing

yarn add zo0r/react-native-push-notification#dev

Or

nom install zo0r/react-native-push-notification#dev

@jaikantshikray
Copy link

image

Yes it is working from dev branch it will be great if we can get more info about notification in that response

i am waiting for update in the master branch then i can push my update to play store

@lukebars
Copy link
Contributor Author

lukebars commented Jul 2, 2020

@jaikantshikray can you share your thoughts on needed info?

@jaikantshikray
Copy link

i need this data: { type: "schedule", data: item }, value in this response as well so i can utilize it and more if we can have date in unix format

@lukebars
Copy link
Contributor Author

lukebars commented Jul 2, 2020

i need this data: { type: "schedule", data: item }, value in this response as well so i can utilize it and more if we can have date in unix format

If you need unix time format just do this:

unixDate = +date;

@jaikantshikray
Copy link

what about the data value ?

@Dallas62
Copy link
Collaborator

Dallas62 commented Jul 2, 2020

Data value will not be retrieve in iOS,
You should store data in another place I guess

@lukebars
Copy link
Contributor Author

lukebars commented Jul 2, 2020

I think this is possible if you would care to submit a PR to RNCPushNotificationIOS. Until then, you need to figure out an another way.

@jaikantshikray
Copy link

ok thank you waiting for this release i hope i can get that at the end of this week

@lukebars
Copy link
Contributor Author

lukebars commented Jul 2, 2020

@Dallas62 is onNotification method currently limited to remote notifs?

@Dallas62
Copy link
Collaborator

Dallas62 commented Jul 2, 2020

It's a bit tricky, I set the scheduled notification on dev trigger onNotification but I'm not sure to keep it, since there is no away to do it in iOS.
For me this is a huge problem because developper will implement logic in Android but will be forced to use another method on iOS.

There is some way to run tasks in background with iOS/Android, but using notifications for this is not a good thing I think.

@lukebars
Copy link
Contributor Author

lukebars commented Jul 3, 2020

@Dallas62 I remember using RNFirebase v5 local notifications had such method onNotification that fired on both android and iOS when local notification was pressed, that means it's possible to do it.

If I understand correctly, current limitation is on iOS side? I could look into it and create a PR in the RNCPushNotificationIOS.

Let me know what you think.

EDIT: Nvm looks like onNotification works perfectly with local notifications on both platforms.

I'm thinking about digging more into doing some research on creatin onNotificationDisplayed method

@Dallas62
Copy link
Collaborator

Dallas62 commented Jul 5, 2020

In iOS and Android, when notification are pressed every thing works fine.
The change on dev is when the scheduled notification popup, onNotification is triggered without user interaction. On Android we can do it. On iOS the Notification SDK has depreciated this...

@lukebars
Copy link
Contributor Author

lukebars commented Jul 8, 2020

@Dallas62 I think onNotification method triggering on android notification displayed and not on iOS is very inconsistent. Maybe that should be separate method e.g. onNotificationDisplayed? Currently that gives me really hard time to implement onNotification logic, because on android NotificationHandler console.logs two times on click.

@davidmargoses
Copy link

@Dallas62 I tried to use on Android : PushNotification.getScheduledLocalNotifications((notifs) => console.log(notifs)) but it doesn't work.
error: [TypeError: undefined is not an object (evaluating '_reactNativePushNotification.PushNotificationScheduledLocalObject.getScheduledLocalNotifications')]

I'm on 5.1.0.
On IOS PushNotificationIOS.getScheduledLocalNotifications works !

@Dallas62
Copy link
Collaborator

Dallas62 commented Sep 9, 2020

Hi,

Can you check that the version of the library is correctly installed ? Using yarn install instead of yarn upgrade,
And clean your gradle build cache

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

Successfully merging this pull request may close these issues.

None yet

5 participants