Skip to content

Blogging Reminders: Add logic to schedule and display notifications#14719

Merged
renanferrari merged 13 commits intofeature/blogging-remindersfrom
feature/14678-blogging-reminders-scheduling-notification
Jun 17, 2021
Merged

Blogging Reminders: Add logic to schedule and display notifications#14719
renanferrari merged 13 commits intofeature/blogging-remindersfrom
feature/14678-blogging-reminders-scheduling-notification

Conversation

@renanferrari
Copy link
Copy Markdown
Contributor

@renanferrari renanferrari commented May 26, 2021

Fixes #14678

This is a WIP and it's not ready to merge yet, as it depends on #14698.

This PR adds all the logic necessary to schedule and show reminder notifications using WorkManager. The main classes added here are:

  • ReminderConfig, which has all the logic to create daily and weekly reminders.
  • ReminderScheduler, which uses a ReminderConfig to create a WorkManager request.
  • ReminderNotifier, which is responsible for building and showing a blogging reminder notification.
  • ReminderWorker, which is the actual worker implementation that will call both the ReminderNotifier and ReminderScheduler (to schedule the next reminder).

A few things to note:

  • Even though the WorkManager framework supports creating periodic works, these are not really suitable for our use case here. The reason is that we can never control the exact time in which a work would be triggered, we can only control its initial delay and the interval between each work. And since WorkManager respects things like Doze and battery management, we don’t know for sure at which time the following works would be triggered. So if our work is delayed a little bit every time, this means that over time the notification would appear much later than before. This video explains this in greater detail and suggests following the approach we're using here, which is to schedule a one time work that schedules another one time work by the end of it.
  • There are many things that were done in Setup for local push notifications #14691 that could probably be put to good use here. I tried to refactor this multiple times in order to achieve a more generic solution for local notifications, but there were several different requirements here, mostly related to things that we need to do inside the worker (checking if we can show a notification, build a notification and schedule the next work) that eventually required larger changes and, in the end, I wasn't happy with the direction it was taking. I'm thinking that maybe we shouldn't share the same worker for different types of local notifications as each of them would probably have different requirements, and the WorkManager framework already provides us tools to handle this. But this is something we can discuss later. For now, I've left this PR as simple as possible, just so we can move on with it.

To test:

Follow the instructions in #14720.

Regression Notes

  1. Potential unintended areas of impact

N/A

  1. What I did to test those areas of impact (or what existing automated tests I relied on)

N/A

  1. What automated tests I added (or what prevented me from doing so)

Unit tests for newly added classes.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented May 26, 2021

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented May 26, 2021

You can test the changes on this Pull Request by downloading the APK here.

@renanferrari renanferrari force-pushed the feature/14678-blogging-reminders-scheduling-notification branch from 6a18e9c to b7a02a7 Compare May 26, 2021 02:10
@renanferrari renanferrari modified the milestones: 17.5, 17.6 May 27, 2021
@renanferrari renanferrari changed the base branch from develop to feature/enable-core-library-desugaring May 27, 2021 19:29
@renanferrari renanferrari changed the base branch from feature/enable-core-library-desugaring to develop May 27, 2021 19:31
@renanferrari renanferrari changed the base branch from develop to feature/enable-core-library-desugaring May 29, 2021 00:30
…678-blogging-reminders-scheduling-notification
@renanferrari renanferrari marked this pull request as ready for review May 29, 2021 00:57
import java.time.temporal.TemporalAdjusters.next

sealed class ReminderConfig(val type: ReminderType) {
object DailyReminder : ReminderConfig(DAILY)
Copy link
Copy Markdown
Contributor

@planarvoid planarvoid Jun 1, 2021

Choose a reason for hiding this comment

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

Do we need the DailyReminder? What I mean is that the current designs only allow weekly reminders and we can use weekly reminders for each day instead of daily reminders, right?

Copy link
Copy Markdown
Contributor Author

@renanferrari renanferrari Jun 1, 2021

Choose a reason for hiding this comment

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

You're right, we don't really need it right now. I just think having explicit models for each type of recurrence should make this much more flexible in the future, if we ever decide to build upon it with more advanced rules (interval, for example).

Also, as a bonus, it should make it a bit easier to translate rules into natural language for the UI.

Copy link
Copy Markdown
Contributor

@planarvoid planarvoid left a comment

Choose a reason for hiding this comment

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

The code looks good 👍 and as far as I could see this works well. We should spend some time reviewing the real functionality once we have everything in place 👍

I have one minor comment, let me know what you think

Copy link
Copy Markdown
Contributor

@planarvoid planarvoid left a comment

Choose a reason for hiding this comment

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

I tried to approve this before but Github kept on failing 👍

@AliSoftware AliSoftware mentioned this pull request Jun 14, 2021
3 tasks
@AliSoftware AliSoftware modified the milestones: 17.6, 17.7 Jun 14, 2021
@AliSoftware
Copy link
Copy Markdown
Contributor

Moving this PR to next milestone, since 17.6 goes into code freeze today.

Base automatically changed from feature/enable-core-library-desugaring to feature/blogging-reminders June 17, 2021 21:03
@renanferrari renanferrari merged commit a11aa5f into feature/blogging-reminders Jun 17, 2021
@renanferrari renanferrari deleted the feature/14678-blogging-reminders-scheduling-notification branch June 17, 2021 21:04
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.

Blogging Reminders: Implement scheduling logic and notification

3 participants