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

Fix disabling create site notification issue #17534

Merged
merged 1 commit into from
Nov 25, 2022

Conversation

irfano
Copy link
Member

@irfano irfano commented Nov 24, 2022

#17496 added the ability to disable create site notifications via the Notification Settings switch. But it has an issue mentioned in the PR:

Create site notifications won't be rescheduled if you turn on the switch again. Two Create site notifications are scheduled when the app is installed the first time. One is for the next day after the install, another one is for 8 days after the first install. If the switch is turned off and turned on in 8 days, create site notifications won't display. I'll discuss this later and change it in a separate PR if necessary.

This PR fixes that issue.

I removed canceling create site notifications on UpdateNotificationSettingsUseCase. It's already being handled in CreateSiteNotificationHandler when the notification is about to show. Notice the isNotificationSettingsEnabled parameter in the check.

override fun shouldShowNotification(): Boolean {
val isNotificationSettingsEnabled = sharedPrefs.getBoolean(
resourceProvider.getString(R.string.wp_pref_notifications_main),
true
)
return isNotificationSettingsEnabled && accountStore.hasAccessToken() && !siteStore.hasSite()
}

To test:
If you want to manipulate the code to wait a minute instead of a day, change


with delayUnits = TimeUnit.MINUTES

When the switch is off

  1. Uninstall the app if installed.
  2. Install and launch the WordPress app.
  3. Log in with an account without any site.
  4. Navigate to Notifications.
  5. Tap ⚙️ button at the top of the screen.
  6. Turn off the switch at the top of the page.
  7. Wait for the next day. Ensure the create site notification won't be delivered.

When the switch is turned off and then on again

  1. Uninstall the app if installed.
  2. Install and launch the WordPress app.
  3. Log in with an account without any site.
  4. Navigate to Notifications.
  5. Tap ⚙️ button at the top of the screen.
  6. Turn off the switch at the top of the page. Then turn it on again.
  7. Wait for the next day. Ensure the create site notification will be delivered.

Regression Notes

  1. Potential unintended areas of impact
    None.

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

@irfano irfano added this to the 21.3 milestone Nov 24, 2022
@irfano irfano requested a review from ravishanker November 24, 2022 16:37
@peril-wordpress-mobile
Copy link

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

@wpmobilebot
Copy link
Contributor

WordPress📲 You can test these changes on WordPress by downloading wordpress-installable-build-pr17534-d608468.apk
💡 Scan this QR code with your Android phone to download and install the APK directly on it.
AppWordPress
Build FlavorJalapeno
Build TypeDebug
Commitd608468
Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

@wpmobilebot
Copy link
Contributor

Jetpack📲 You can test these changes on Jetpack by downloading jetpack-installable-build-pr17534-d608468.apk
💡 Scan this QR code with your Android phone to download and install the APK directly on it.
AppJetpack
Build FlavorJalapeno
Build TypeDebug
Commitd608468
Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

Copy link
Contributor

@ravishanker ravishanker left a comment

Choose a reason for hiding this comment

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

Nice work 👍

@irfano irfano merged commit 96e07bd into trunk Nov 25, 2022
@irfano irfano deleted the fix/disabling-create-site-notification-issue branch November 25, 2022 13:36
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