Disable local notifications with Notification Settings switch #17496

irfano · 2022-11-20T00:37:54Z

This prevents local notifications when the Notification Settings switch is off. Prevented local notifications are

Weekly roundup notifications,
Blogging reminder notifications,
Create site notifications.

Blogging reminders popups are still active and can be displayed in some cases (eg. from Site Settings) even Notification Settings switch is off. If the user sets a new reminder from the popup, it will be scheduled even if the switch is off.

Merge requirements:

1. Merge FluxC PR
2. Update FluxC reference to trunk
3. Remove [Status] Not Ready for Merge label
4. Merge this

To test:

You can wait for scheduled dates to test notifications. It would be the best method but testing all cases takes an unrealistic long time. Alternatively, you can check App Inspection > Background Task Inspector on Android Studio if these notifications are scheduled.

Weekly roundup notifications

Launch the WordPress app.
Navigate to Notifications.
Tap ⚙️ button at the top of the screen.
Turn off the switch at the top of the page.
Verify WeeklyRoundupWorker is canceled on App Inspection > Background Task Inspector.
Turn on the switch again.
Verify WeeklyRoundupWorker is scheduled on App Inspection > Background Task Inspector.
Repeat 1-7 for Jetpack app.

Blogging reminder notifications

Launch the WordPress app.
Navigate to My Site → MENU → Site Settings.
Tap Blogging Reminders.
Set a reminder.
Verify ReminderWorker is scheduled on App Inspection > Background Task Inspector.
Navigate back and change your selected site from the site picker at the top of the MENU page.
Navigate to Site Settings and set another reminder for the selected site.
Verify a new ReminderWorker is scheduled on App Inspection > Background Task Inspector.
Navigate to Notifications. Tap ⚙️ button at the top of the screen.
Turn off the switch at the top of the page.
Verify both ReminderWorkers are canceled on App Inspection > Background Task Inspector.
Turn on the switch.
Verify both ReminderWorkers are scheduled on App Inspection > Background Task Inspector.
Repeat 1-13 for Jetpack app.

Create site notifications

Uninstall the app if it's installed.
Install and launch the WordPress app.
Log in with an account without any site.
Verify LocalNotificationWorker is scheduled on App Inspection > Background Task Inspector.
Navigate to Notifications.
Tap ⚙️ button at the top of the screen.
Turn off the switch at the top of the page.
Verify LocalNotificationWorker is canceled on App Inspection > Background Task Inspector.
NOTE: 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.

Regression Notes

Potential unintended areas of impact
Rescheduling blogging reminders after turning Notification Settings off and on again.
What I did to test those areas of impact (or what existing automated tests I relied on)
Tested manually.
What automated tests I added (or what prevented me from doing so)
Update CreateSiteNotificationHandlerTest.

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.

# Conflicts: # build.gradle

wpmobilebot · 2022-11-21T19:32:23Z

Found 1 violations:

The PR caused the following dependency changes:

-+--- org.wordpress:fluxc:{strictly trunk-5a4f8132de39ae666f841edc4522f87b1b02a265} -> trunk-5a4f8132de39ae666f841edc4522f87b1b02a265
-|    +--- org.wordpress:wellsql:1.7.0
-|    |    \--- org.wordpress.wellsql:wellsql-annotations:1.7.0
-|    +--- org.wordpress.fluxc:fluxc-annotations:trunk-5a4f8132de39ae666f841edc4522f87b1b02a265
-|    +--- org.greenrobot:eventbus:3.3.1 (*)
-|    +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.9.2 (*)
-|    +--- com.android.volley:volley:1.1.1 -> 1.2.0
-|    +--- androidx.paging:paging-runtime:2.1.2
-|    |    +--- androidx.paging:paging-common:2.1.2
-|    |    |    +--- androidx.annotation:annotation:1.0.0 -> 1.2.0
-|    |    |    \--- androidx.arch.core:core-common:2.0.0 -> 2.1.0 (*)
-|    |    +--- androidx.arch.core:core-runtime:2.0.0 -> 2.1.0 (*)
-|    |    +--- androidx.lifecycle:lifecycle-runtime:2.0.0 -> 2.4.1 (*)
-|    |    +--- androidx.lifecycle:lifecycle-livedata:2.0.0 -> 2.4.1 (*)
-|    |    \--- androidx.recyclerview:recyclerview:1.0.0 -> 1.1.0 (*)
-|    +--- com.goterl:lazysodium-android:5.0.2
-|    +--- net.java.dev.jna:jna:5.5.0
-|    +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 (*)
-|    +--- org.jetbrains.kotlin:kotlin-android-extensions-runtime:1.6.10 (*)
-|    +--- androidx.appcompat:appcompat:1.0.2 -> 1.3.1 (*)
-|    +--- androidx.recyclerview:recyclerview:1.0.0 -> 1.1.0 (*)
-|    +--- androidx.exifinterface:exifinterface:1.0.0 -> 1.3.3 (*)
-|    +--- com.squareup.okhttp3:okhttp-urlconnection:4.9.0 -> 4.9.2
-|    |    +--- com.squareup.okhttp3:okhttp:4.9.2 (*)
-|    |    \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.4.10 -> 1.6.10 (*)
-|    +--- com.google.code.gson:gson:2.8.5
-|    +--- org.apache.commons:commons-text:1.1 (*)
-|    +--- androidx.room:room-runtime:2.4.2 (*)
-|    +--- androidx.room:room-ktx:2.4.2
-|    |    +--- androidx.room:room-common:2.4.2 (*)
-|    |    +--- androidx.room:room-runtime:2.4.2 (*)
-|    |    +--- org.jetbrains.kotlin:kotlin-stdlib:1.6.10 (*)
-|    |    \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.5.2 (*)
-|    +--- com.google.dagger:dagger:2.42 (*)
-|    +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.3.9 -> 1.5.2 (*)
-|    \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.3.9 -> 1.5.2 (*)
++--- org.wordpress:fluxc:{strictly trunk-7e1f7406753a72cbed61feab1204ca52ba7f0f77} -> trunk-7e1f7406753a72cbed61feab1204ca52ba7f0f77
+|    +--- org.wordpress:wellsql:1.7.0
+|    |    \--- org.wordpress.wellsql:wellsql-annotations:1.7.0
+|    +--- org.wordpress.fluxc:fluxc-annotations:trunk-7e1f7406753a72cbed61feab1204ca52ba7f0f77
+|    +--- org.greenrobot:eventbus:3.3.1 (*)
+|    +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.9.2 (*)
+|    +--- com.android.volley:volley:1.1.1 -> 1.2.0
+|    +--- androidx.paging:paging-runtime:2.1.2
+|    |    +--- androidx.paging:paging-common:2.1.2
+|    |    |    +--- androidx.annotation:annotation:1.0.0 -> 1.2.0
+|    |    |    \--- androidx.arch.core:core-common:2.0.0 -> 2.1.0 (*)
+|    |    +--- androidx.arch.core:core-runtime:2.0.0 -> 2.1.0 (*)
+|    |    +--- androidx.lifecycle:lifecycle-runtime:2.0.0 -> 2.4.1 (*)
+|    |    +--- androidx.lifecycle:lifecycle-livedata:2.0.0 -> 2.4.1 (*)
+|    |    \--- androidx.recyclerview:recyclerview:1.0.0 -> 1.1.0 (*)
+|    +--- com.goterl:lazysodium-android:5.0.2
+|    +--- net.java.dev.jna:jna:5.5.0
+|    +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 (*)
+|    +--- org.jetbrains.kotlin:kotlin-android-extensions-runtime:1.6.10 (*)
+|    +--- androidx.appcompat:appcompat:1.0.2 -> 1.3.1 (*)
+|    +--- androidx.recyclerview:recyclerview:1.0.0 -> 1.1.0 (*)
+|    +--- androidx.exifinterface:exifinterface:1.0.0 -> 1.3.3 (*)
+|    +--- com.squareup.okhttp3:okhttp-urlconnection:4.9.0 -> 4.9.2
+|    |    +--- com.squareup.okhttp3:okhttp:4.9.2 (*)
+|    |    \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.4.10 -> 1.6.10 (*)
+|    +--- com.google.code.gson:gson:2.8.5
+|    +--- org.apache.commons:commons-text:1.1 (*)
+|    +--- androidx.room:room-runtime:2.4.2 (*)
+|    +--- androidx.room:room-ktx:2.4.2
+|    |    +--- androidx.room:room-common:2.4.2 (*)
+|    |    +--- androidx.room:room-runtime:2.4.2 (*)
+|    |    +--- org.jetbrains.kotlin:kotlin-stdlib:1.6.10 (*)
+|    |    \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.5.2 (*)
+|    +--- com.google.dagger:dagger:2.42 (*)
+|    +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.3.9 -> 1.5.2 (*)
+|    \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.3.9 -> 1.5.2 (*)
 \--- org.wordpress:login:1.0.0
-     \--- org.wordpress:fluxc:2.0.0 -> trunk-5a4f8132de39ae666f841edc4522f87b1b02a265 (*)
+     \--- org.wordpress:fluxc:2.0.0 -> trunk-7e1f7406753a72cbed61feab1204ca52ba7f0f77 (*)

Please review and act accordingly

wpmobilebot · 2022-11-21T19:42:37Z

📲 You can test these changes on WordPress by downloading wordpress-installable-build-pr17496-7df6d32.apk

	💡 Scan this QR code with your Android phone to download and install the APK directly on it.
	App	`WordPress`
	Build Flavor	`Jalapeno`
	Build Type	`Debug`
	Commit	`7df6d32`

Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

wpmobilebot · 2022-11-21T19:43:30Z

📲 You can test these changes on Jetpack by downloading jetpack-installable-build-pr17496-7df6d32.apk

	💡 Scan this QR code with your Android phone to download and install the APK directly on it.
	App	`Jetpack`
	Build Flavor	`Jalapeno`
	Build Type	`Debug`
	Commit	`7df6d32`

Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

ravishanker

Nice work 👍

Feel free to merge it after resolving build issues

ravishanker · 2022-11-22T03:56:30Z

.../src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsActivity.kt

+        // Set up primary toolbar
+        setUpToolbar()
+
+        // Set up main switch


comment is redundant here

I just copied Java while converting. But I agree. The code is already explaining itself.
Removed! 7df6d32

ravishanker · 2022-11-22T03:57:08Z

.../src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsActivity.kt

+        sharedPreferences = PreferenceManager.getDefaultSharedPreferences(this@NotificationsSettingsActivity)
+
+        // Set up primary toolbar
+        setUpToolbar()


Could be renamed to setupPrimaryToolbar() and remove the comment

Done! 7df6d32

ravishanker · 2022-11-22T03:57:52Z

.../src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsActivity.kt

+        fragmentContainer = findViewById(id.fragment_container)
+
+        // Get shared preferences for main switch.
+        sharedPreferences = PreferenceManager.getDefaultSharedPreferences(this@NotificationsSettingsActivity)


Same here, renaming it to mainSwitchPrefs would remove the need for comment

Actually, sharedPreferences doesn't have to be a class variable. I moved it to the function. 7df6d32

ravishanker · 2022-11-22T03:58:29Z

RELEASE-NOTES.txt

@@ -2,6 +2,7 @@

 21.3
 -----
+* [*] Disable local notifications with Notification Settings switch. [https://github.com/wordpress-mobile/WordPress-Android/pull/17496]


Should this be marked as [internal]

I think an important issue was fixed. This is not only about Jetpack Focus. With this PR, the Notifications Settings switch will affect local notifications too. So, I think it should not be internal.

ravishanker · 2022-11-22T03:59:54Z

.../src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsActivity.kt

+import javax.inject.Named
+
+@AndroidEntryPoint
+class NotificationsSettingsActivity : LocaleAwareActivity(), MainSwitchToolbarListener {


🎉 Kudos for converting it to Kotlin

ravishanker · 2022-11-22T04:04:31Z

.../src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsActivity.kt

+        hideDisabledView(isMainChecked)
+    }
+
+    override fun onMainSwitchCheckedChanged(buttonView: CompoundButton?, isChecked: Boolean) {


This was removed in a previous PR right?

No, onMainSwitchCheckedChanged was being called unnecessarily at launch. I fixed that in #17498. It didn't change this class.

irfano added 5 commits November 20, 2022 01:17

Convert NotificationsSettingsActivity to Kotlin

e354a9b

Update FluxC version

b085873

Add UpdateNotificationSettingsUseCase

a055a19

Use UpdateNotificationSettingsUseCase in NotificationsSettingsActivity

86725a7

Use UpdateNotificationSettingsUseCase in JetpackAppInstallReceiver

f72f456

irfano added [Type] Bug Notifications [Status] Not Ready for Merge labels Nov 20, 2022

irfano added this to the 21.3 milestone Nov 20, 2022

irfano mentioned this pull request Nov 20, 2022

Add getAll function to BloggingRemindersStore wordpress-mobile/WordPress-FluxC-Android#2571

Merged

irfano added 2 commits November 20, 2022 18:13

Fetch blogging reminders once in UpdateNotificationSettingsUseCase

09a7618

Update release notes for PR 17496

f600aa1

Base automatically changed from Jetpack-Focus-Disable-notifications-for-WP-on-JP-Install to trunk November 21, 2022 01:34

irfano added 2 commits November 21, 2022 21:57

Merge branch 'trunk' into issue/15816-manage-local-notifs-with-switch

f93fc56

# Conflicts: # build.gradle

Update FluxC version

62ce8c7

Prevent scheduling local notifications if switch is off

4be1196

irfano removed the [Status] Not Ready for Merge label Nov 21, 2022

irfano requested a review from ravishanker November 21, 2022 20:42

irfano marked this pull request as ready for review November 21, 2022 20:43

ravishanker approved these changes Nov 22, 2022

View reviewed changes

ravishanker reviewed Nov 22, 2022

View reviewed changes

irfano added 2 commits November 22, 2022 18:58

Update CreateSiteNotificationHandlerTest

fad1f9f

Refactor NotificationsSettingsActivity

7df6d32

irfano merged commit cb9c999 into trunk Nov 22, 2022

irfano deleted the issue/15816-manage-local-notifs-with-switch branch November 22, 2022 21:08

irfano mentioned this pull request Nov 22, 2022

Weekly roundup notification despite notifications off #15816

Closed

ravishanker mentioned this pull request Nov 23, 2022

Disable notifications on WP app in migration flow on JP app #17513

Merged

3 tasks

irfano mentioned this pull request Nov 24, 2022

Fix disabling create site notification issue #17534

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable local notifications with Notification Settings switch #17496

Disable local notifications with Notification Settings switch #17496

irfano commented Nov 20, 2022 •

edited

Loading

wpmobilebot commented Nov 21, 2022

wpmobilebot commented Nov 21, 2022 •

edited

Loading

wpmobilebot commented Nov 21, 2022 •

edited

Loading

ravishanker left a comment

ravishanker Nov 22, 2022

irfano Nov 22, 2022

ravishanker Nov 22, 2022

irfano Nov 22, 2022

ravishanker Nov 22, 2022

irfano Nov 22, 2022

ravishanker Nov 22, 2022

irfano Nov 22, 2022

ravishanker Nov 22, 2022

ravishanker Nov 22, 2022

irfano Nov 22, 2022

Disable local notifications with Notification Settings switch #17496

Disable local notifications with Notification Settings switch #17496

Conversation

irfano commented Nov 20, 2022 • edited Loading

To test:

Regression Notes

wpmobilebot commented Nov 21, 2022

The PR caused the following dependency changes:

wpmobilebot commented Nov 21, 2022 • edited Loading

wpmobilebot commented Nov 21, 2022 • edited Loading

ravishanker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irfano commented Nov 20, 2022 •

edited

Loading

wpmobilebot commented Nov 21, 2022 •

edited

Loading

wpmobilebot commented Nov 21, 2022 •

edited

Loading