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

fix(android): implement getDeliveredNotifications and removeDeliveredNotifications #2266

Conversation

p7g
Copy link
Contributor

@p7g p7g commented Dec 17, 2019

This PR implements the PushNotifications plugin getDeliveredNotifications and removeDeliveredNotifications methods.

The fields available on the objects returned by getDeliveredNotifications are a subset of those available on iOS, but otherwise the behaviour should be the same.

Copy link
Contributor

@macdja38 macdja38 left a comment

Choose a reason for hiding this comment

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

Looks good!

@trytuna
Copy link

trytuna commented Jan 9, 2020

This is 23 days old. Can we please merge that?

@jcesarmobile
Copy link
Member

How I'm supposed to use the removeDeliveredNotifications? If I first use getDeliveredNotifications and pass the result to removeDeliveredNotifications, all the ids are 0, so doesn't remove the notifications.

For local notifications it works since we set the id, but it's confusing since the PR is about Push notifications

@p7g
Copy link
Contributor Author

p7g commented Jan 29, 2020

@jcesarmobile In my use-case I have a subclass of FirebaseMessagingService that sets the IDs of incoming notifications. I understand that not everyone will want to have this, but I think the problem of IDs being 0 is a different issue than the one this PR addresses.

@jcesarmobile
Copy link
Member

yeah, but on a regular Capacitor app this will happen, and I'm concerned about users complaining about it

@p7g
Copy link
Contributor Author

p7g commented Jan 29, 2020

I think that having these methods implemented is better than not.

I agree that users may encounter the issue you mentioned, but we can make a new issue to find out why the IDs of the notifications are 0.

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

ok, looks good

@jcesarmobile jcesarmobile changed the title Android PushNotifications get/remove delivered notifications fix(android): implement getDeliveredNotifications and removeDeliveredNotifications Jan 30, 2020
@jcesarmobile jcesarmobile merged commit d942523 into ionic-team:master Jan 30, 2020
@p7g p7g deleted the android-push-notifications-delivered-notifications branch January 30, 2020 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants