Skip to content

Conversation

@malmstein
Copy link
Contributor

@malmstein malmstein commented Apr 29, 2020

Task/Issue URL: https://app.asana.com/0/414730916066338/1173445999500153/f

Description:

  • We use WorkManager to enqueue work that will be done in the background at some point in the future.
  • Some experiments will use this feature to schedule future work. Android: Sticky Search Notification is an example.
  • When the experiment is removed, we need to make sure that those scheduled jobs are also removed from the user's device.

Our current implementation has AndroidNotificationScheduler cleaning up old jobs AND scheduling new work. The idea in this PR is to separate the responsibilities making it more readable.


Internal references:

Software Engineering Expectations
Technical Design Template

Copy link
Member

@CDRussell CDRussell left a comment

Choose a reason for hiding this comment

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

Looks like there's now some duplicate code left over in NotificationScheduler

  1. it still also tries to cancel unnecessary work
  2. it also defines deprecated work tag list

@malmstein
Copy link
Contributor Author

@CDRussell I've changed how the WorkManager is initialized for testing according to the documentation. This has shown that we were not properly testing it, for that I've created a TestWorker that has no dependencies and it's sole purpose is to be tested

@malmstein malmstein requested a review from CDRussell May 13, 2020 07:03
val requestBuilder = OneTimeWorkRequestBuilder<TestWorker>()
val request = requestBuilder
.addTag(it)
.setInitialDelay(10, TimeUnit.SECONDS)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we set this delay because if we don't the Worker will be immediately completed.

Copy link
Member

@CDRussell CDRussell left a comment

Choose a reason for hiding this comment

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

This is great, and i never knew about that worker testing lib so excited to see its inclusion.

Some minor tidy-up suggestions. Feel free to merge after (no re-reviewing needed)

  1. Remove sticky search comment from JobCleaner
  2. Format JobCleaner class
  3. Format JobCleanerTest class

Great stuff! 👍

@CDRussell
Copy link
Member

Looks like there's now some duplicate code left over in NotificationScheduler

  1. it still also tries to cancel unnecessary work
  2. it also defines deprecated work tag list

Heads up that this comment hasn't been addressed

@malmstein malmstein merged commit 6afd992 into develop May 13, 2020
@malmstein malmstein deleted the feature/david/remove_old_jobs branch May 13, 2020 10:57
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.

2 participants