Setup for local push notifications#14691
Conversation
Add WorkManager setup for handling local push notification like blogging reminders
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
|
You can test the changes on this Pull Request by downloading the APK here. |
fix lint checks
WordPress/src/main/java/org/wordpress/android/workers/LocalPushScheduleWorker.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/workers/LocalPushScheduleWorker.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/workers/LocalPushScheduleWorker.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/workers/LocalPushScheduleWorker.kt
Outdated
Show resolved
Hide resolved
fix for detekt issues
renanferrari
left a comment
There was a problem hiding this comment.
Thanks for taking care of this!
Before we merge this, I just want to suggest a minor (and potentially nitpicky) change:
I believe we should probably avoid calling this a "local push". As far I as understand, a push notification can't be local by definition, so I'd say we should probably rename all "local push" references to just "local notification". What do you think?
(cc @planarvoid as I know most of these changes were made by you)
Rename and replace all instances of Push with Notification word for clarity
Makes sense. Renamed all instances of "Push" to "Notification" |
Replaced some more instance of Push with Notification naming convention
| .addAction(R.drawable.ic_story_icon_24dp, context.getString(R.string.cancel), | ||
| pendingIntent) | ||
| .setPriority(NotificationCompat.PRIORITY_DEFAULT) | ||
| .setCategory(NotificationCompat.CATEGORY_REMINDER) |
There was a problem hiding this comment.
In the following PR we should reconsider if we need these flags. I've originally introduced them only because I was trying what I can set for the design purposes. The same goes to the addAction where the icon and the text should probably be part of the inputData. We should also remove the setLargeIcon which should probably be done in this PR as it doesn't make sense here.
There was a problem hiding this comment.
I've moved icon and title to inputData and removed setLargeIcon. Left the priority and category for now, we can remove them if not required when testing the reminders. Thanks
planarvoid
left a comment
There was a problem hiding this comment.
Thanks for pulling out this PR. I think it can potentially affect the UploadWorker because we're touching the code around its initialization. Could you please check where it's used and try to test if it works as expected? Other than that it's looking good. I have few minor comments.
| private val accountStore: AccountStore, | ||
| private val siteStore: SiteStore | ||
| ) : LocalNotificationHandler { | ||
| override fun shouldShowNotification(): Boolean { |
There was a problem hiding this comment.
I'd maybe add a unit test for this method.
| ) { | ||
| fun buildLocalNotificationHandler(type: Type): LocalNotificationHandler { | ||
| return when (type) { | ||
| CREATE_SITE -> createSiteNotificationHandler |
There was a problem hiding this comment.
Maybe also add a unit test for this factory 👍
Removed unnecessary params like large icon and parameterised action icon and title
add unit test
add unit test
planarvoid
left a comment
There was a problem hiding this comment.
Thanks for the changes! it's looking good 👍
This PR teases apart WorkManager related code from this draft PR to cater for various types of reminders.
WorkManager is part of the Android Jetpack. It is a library for scheduling deferrable background tasks (work). WorkManager helps you to schedule some work. This work can be one-time, or it can be repeating work. Scheduled work is then stored in an internally managed SQLite database, and WorkManager takes care of ensuring that work persists and is rescheduled across device reboots.
It will be enhanced progressively, while implementing various types of local notifications to schedule notifications.
To test:
Regression Notes
Potential unintended areas of impact
NA
What I did to test those areas of impact (or what existing automated tests I relied on)
NA
What automated tests I added (or what prevented me from doing so)
NA
PR submission checklist:
RELEASE-NOTES.txtif necessary.