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(local-notifications): Make getPending not return already fired notifications #256

Merged
merged 7 commits into from
Feb 23, 2021

Conversation

theproducer
Copy link
Contributor

This PR adds changes to the local-notification plugin to filter out already fired notifications from the getPending notifications call.

Since Capacitor supports Android API 21+, we can't rely on NotificationManager.getActiveNotifications() to easily get a list of notifications in the status bar. Instead, this PR sets a visible flag on each LocalNotification that gets set when a notification is fired, then cleared when a notification is interacted with.

fixes: #221

@Ionitron Ionitron added this to Needs review 🤔 in Capacitor Engineering ⚡️ Feb 16, 2021
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.

I've tested and it works, but it adds a ton of complexity and I think using storage.deleteNotification when the notification fires is enough to not show it on pending and I don't see any side effects on doing that.
The only thing is have in mind that should only be deleted if not repeatable, so probably should be done inside of the rescheduleNotificationIfNeeded method or make it return a boolean to know if it was rescheduled or not and delete it only if not rescheduled

@theproducer
Copy link
Contributor Author

Thanks - just deleting the notification worked just as well without any issues!

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.

looks good

let's wait for next beta before merging so tests pass again (hopefully)

@jcesarmobile jcesarmobile changed the title fix(local-notifications): Filter fired notifications from getPending fix(local-notifications): Make getPending not return already fired notifications Feb 18, 2021
@theproducer theproducer merged commit fb96f8a into main Feb 23, 2021
Capacitor Engineering ⚡️ automation moved this from Needs review 🤔 to Done 🎉 Feb 23, 2021
@theproducer theproducer deleted the local-notifications-pending-fired branch February 23, 2021 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

local-notifications getPending shouldn't return notifications that already fired
2 participants