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

Blogging Prompts: Add wrapper component to interface between schedulers #18772

Merged
merged 4 commits into from
May 30, 2022

Conversation

dvdchr
Copy link
Contributor

@dvdchr dvdchr commented May 30, 2022

Refs #18761

This PR adds ReminderScheduleCoordinator as a wrapper component that can decide whether to use BloggingRemindersScheduler or PromptRemindersScheduler.

The component will decide to use the Blogging Prompts scheduler when all of these are true:

  • The feature flag for Blogging Prompts is enabled,
  • If there exists a settings object for Blogging Prompts in local storage (BloggingPromptsSettings),
  • and the promptRemindersEnabled property is true. This property tracks and gets updated whenever the Include Prompts switch is toggled in the reminder sheet.

In other cases, the coordinator will fall back on the Blogging Reminders scheduler.

To Test

Ensure that the unit tests are passing.

Regression Notes

  1. Potential unintended areas of impact
    N/A. Feature is unreleased and component is not yet used.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    N/A. Feature is unreleased and component is not yet used.

  3. What automated tests I added (or what prevented me from doing so)
    Added unit tests.

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.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented May 30, 2022

You can test the changes in Jetpack from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr18772-bf4c0c8 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented May 30, 2022

You can test the changes in WordPress from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr18772-bf4c0c8 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

Copy link
Contributor

@wargcm wargcm left a comment

Choose a reason for hiding this comment

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

The tests pass and the code looks good to me! :shipit:

@dvdchr
Copy link
Contributor Author

dvdchr commented May 30, 2022

The unit test failure is pretty weird. After spending a bit digging into this, it looks like there are other "live objects" subscribing to the account change notification after calling setDefaultWordPressComAccount.

Screen Shot 2022-05-31 at 00 54 03

The core problem, as I understand, is that different objects hold different context managers but they share the same user defaults; which can lead to inconsistency that caused some stored values to be wiped. In this case, [AccountService defaultWordPressComAccount] will delete the account UUID stored in user defaults if the WPAccount matching the UUID doesn't exist. (Well, it exists on the context manager in my test suite, but it definitely does not in other contexts 😅 )

I think UserDefaults needs to be mocked somehow in unit testing, so we can control what goes in and out.

(cc: @mokagio & team)

@dvdchr dvdchr merged commit d5cb45a into trunk May 30, 2022
@dvdchr dvdchr deleted the feature/18761-reminder-schedule-coordinator branch May 30, 2022 18:07
@dvdchr dvdchr linked an issue May 31, 2022 that may be closed by this pull request
5 tasks
@mokagio
Copy link
Contributor

mokagio commented May 31, 2022

Thanks for the ping @dvdchr! I opened #18778 to track the need for a deeper UserDefaults-usage cleanup.

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.

Prompts reminder scheduling integration
4 participants