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

Weekly Roundup Improvements (Core Data + Localization) #17766

Merged
merged 3 commits into from
Jan 18, 2022

Conversation

frosty
Copy link
Contributor

@frosty frosty commented Jan 14, 2022

This PR makes some changes to the Core Data usage in the Weekly Roundup background task, to hopefully fix #17765.

The original issue was hard to reproduce, but my guess was that it was due to sometimes accessing the site managed objects on a different queue to the one their context belonged to. I was able to recreate it more consistently (but not every time) by wrapping the scheduleNotification and requestData BlockOperations in WeeklyRoundupBackgroundTask in separate DispatchQueue.async calls for different queues.

I also noticed that even without these changes the Weekly Roundup tester in Me > App Settings > Debug > Weekly Roundup didn't seem to work consistently. Sometimes it would fail entirely, other times I would only see the 'static' notification, and other times I would only see a notification for a single site.

With the changes in this PR, it appears to be working for me every time.

I also took the opportunity to localize the notification strings, as I noticed they weren't localized.

To test

  • Build and run on a device
  • Navigate to Me > App Settings > Debug > Weekly Roundup, and tap Schedule immediately. I recommend using an a8c account and having the 'use a8c sites' option checked to ensure you have a lot of views.
  • After a few moments you should see multiple notifications come in containing the view count etc for 5 sites.
  • Repeat the test multiple times. Background tasks do seem a little flaky as we can't guarantee exactly when the OS will run things, but this did seem to be working for me most of the time.

Regression Notes

  1. Potential unintended areas of impact

This should only affect the weekly roundup background task.

  1. What I did to test those areas of impact (or what existing automated tests I relied on)
  2. What automated tests I added (or what prevented me from doing so)

N/A

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • 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.

@frosty frosty added this to the 19.1 milestone Jan 14, 2022
@frosty frosty requested a review from Gio2018 January 14, 2022 17:54
@peril-wordpress-mobile
Copy link

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

Copy link
Contributor

@Gio2018 Gio2018 left a comment

Choose a reason for hiding this comment

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

Performed the test several times and it worked all the times.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants