Skip to content

Blogging Prompts: Add syncing reminder UI with remote prompts schedule - #18753

Merged
dvdchr merged 4 commits into
trunkfrom
feature/18429-reminders-settings-ui-sync
May 30, 2022
Merged

Blogging Prompts: Add syncing reminder UI with remote prompts schedule#18753
dvdchr merged 4 commits into
trunkfrom
feature/18429-reminders-settings-ui-sync

Conversation

@wargcm

@wargcm wargcm commented May 26, 2022

Copy link
Copy Markdown
Contributor

See: #18429

Description

Syncs the reminders UI with the remote settings if it exists. The order of priority for which schedule to display is as follows:
Local schedule > Remote prompt settings > None

So if a user already has reminders scheduled but it wasn't synced with the remote backend, their schedule would still be preserved.

Testing

To test:

  • Enable bloggingPrompts feature flag
  • Open a site and go to My Site > Menu > Site Settings > Blogging Reminders
  • Set a schedule and hit 'Notify me'
  • Verify the network call to update the schedule is successful
  • Delete the app and relaunch
  • Pick the same site and navigate back to the reminders screen
  • Verify the schedule matches what was saved

Regression Notes

  1. Potential unintended areas of impact
    N/A

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    N/A

  3. 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.

@wargcm wargcm added this to the 20.1 milestone May 26, 2022
@wargcm
wargcm requested a review from dvdchr May 26, 2022 20:40
@wargcm wargcm self-assigned this May 26, 2022
@wpmobilebot

wpmobilebot commented May 26, 2022

Copy link
Copy Markdown
Contributor
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 pr18753-62a5afd on your iPhone

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

@wpmobilebot

wpmobilebot commented May 26, 2022

Copy link
Copy Markdown
Contributor
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 pr18753-62a5afd on your iPhone

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

@dvdchr dvdchr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👋🏼 ! Thanks for wrangling this @wargcm. I've got some notes:

  • We probably should only prepopulate the reminder settings when promptRemindersEnabled is true, so we don't disrupt the current Blogging Reminders flow (without prompts). What do you think?
  • Additionally, since Blogging Prompts is still feature-flagged, we should check if the flag is enabled first before pre-populating the reminders.

thursday,
friday,
saturday
].enumerated().flatMap { (index: Int, isReminderActive: Bool) in

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's make Xcode happy 😄 (flatMap is deprecated)

Suggested change
].enumerated().flatMap { (index: Int, isReminderActive: Bool) in
].enumerated().compactMap { (index: Int, isReminderActive: Bool) in

@wargcm

wargcm commented May 27, 2022

Copy link
Copy Markdown
Contributor Author

@dvdchr thanks for the flatMap catch!

We probably should only prepopulate the reminder settings when promptRemindersEnabled is true, so we don't disrupt the current Blogging Reminders flow (without prompts). What do you think?

The local settings are favored over the remote settings already. So if they had prompts off and had local reminders already, it wouldn't overwrite their schedule. I don't feel strongly one way or the other about wrapping the syncing behind that bool so I went ahead and made the changes.

Additionally, since Blogging Prompts is still feature-flagged, we should check if the flag is enabled first before pre-populating the reminders.

I was thinking it wouldn't be a huge deal since the default remote settings already matched the default reminder settings locally. But it's a good call in case the backend ever changes, so I made that change.

@wargcm
wargcm requested a review from dvdchr May 27, 2022 22:30

@dvdchr dvdchr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@dvdchr
dvdchr merged commit 6590dcc into trunk May 30, 2022
@dvdchr
dvdchr deleted the feature/18429-reminders-settings-ui-sync branch May 30, 2022 04:31
@dvdchr dvdchr modified the milestones: 20.1, 20.0 ❄️ May 30, 2022
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.

3 participants