Jetpack Focus: Disable Notifications For WordPress app when Jetpack notifications enabled - Part 1 #19531

staskus · 2022-10-27T16:28:24Z

Description

A part of solution for disabling notifications on WordPress app during Jetpack Focus.

It is missing enable [Notification Filtering Entitlement](https://developer.apple.com/documentation/bundleresources/entitlements/com_apple_developer_usernotifications_filtering and Local Notifications](#19560). They will be added on the other PRs.

All the changes are under the feature flag and won't affect production.

Solution

Solution relies on Notification Filtering Entitlement , that allows to decide not to show notification at the moment of the arrival.

Add WPNotificationsEnabledKey key
Disable WPNotificationsEnabledKey when Jetpack notifications are enabled
Check for WPNotificationsEnabledKey in NotificationService and skip showing notification if it's false
Additionally creating a switch inside Notification Settings (similar to Android), that allow to reenable notification (see video)

Testing instructions

Case 1: WordPress + Receive Notifications

Install and log in to the WordPress app
Enable notifications
Confirm that push notifications are received

Case 2: WordPress + Feature Flag

Install and log in to the WordPress app
Enable "Disable WordPress app notifications when Jetpack is installed" (App Settings -> Debug) feature flag
Go to Notifications
Go to App Notification Settings
Confirm that "Allow Notifications" switch is visible

Case 3: Jetpack + Feature Flag

Repeat Case 2 for Jetpack app
Confirm that "Allow Notifications" switch is not visible

Case 4: WordPress + Jetpack + Jetpack Enable Notifications + Receive Notifications

Install and log in to the WordPress app, enable feature flag, enable notifications
Install and log in to the Jetpack app, enable feature flag, enable notifications
Receive push notifications
Jetpack notifications should appear, WordPress notifications should be filtered

Case 5: WordPress Reenable Notifications

Continue Case 4
Go to WordPress
Notifications
Notifications Settings
Enable "Allow Notifications" Switch
Receive push notifications
Both Jetpack and WordPress notifications should appear

Case 6: WordPress Disable Feature Flag

Continue Case 4
Disable "Disable WordPress app notifications when Jetpack is installed" (App Settings -> Debug) feature flag
Receive push notifications
Both Jetpack and WordPress notifications should appear

Case 7: WordPress + Jetpack + Jetpack No Notifications + Receive Notifications

Install and log in to the WordPress app, enable feature flag, enable notifications
Install and log in to the Jetpack app, enable feature flag, don't enable notifications
Receive push notifications
WordPress notifications should appear, Jetpack notifications shouldn't appear

Regression Notes

Potential unintended areas of impact

Disabling WordPress notifications without indenting to

What I did to test those areas of impact (or what existing automated tests I relied on)

Moved all the logic to NotificationFilteringService and tested the cases

What automated tests I added (or what prevented me from doing so)

NotificationFilteringServiceTests to test all the underlying logic that determines when we should allow filtering or filter notifications.

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.

Images & Videos

Testing.notifications.small.mov

…enabled

…bling notifications on WordPress

WordPress/WordPressNotificationServiceExtension/Sources/NotificationService.swift

staskus · 2022-10-27T16:30:25Z

@guarani

Here's the draft. You can take a look at the code or the video if you find it interesting.

wpmobilebot · 2022-10-27T18:34:43Z

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 pr19531-52b80d8 on your iPhone

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

wpmobilebot · 2022-10-27T18:35:05Z

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 pr19531-52b80d8 on your iPhone

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

…ification filtering logic

staskus · 2022-11-04T14:58:45Z

@sla8c, @guarani

Opening PR.

I tried to move most of the logic into one class so it could be tested and also be ready to be more easily removed once the migration is complete.

This is also open for testing. Have in mind that filtered notifications appear with "Filtered" message for now and won't appear once the entitlement is added. I still think it's good to test it to find problems and potential uncovered edge cases.

staskus · 2022-11-04T15:08:27Z

I created a separate task for local notifications. It could be implemented in separate PR and then merged into this branch.

guarani · 2022-11-04T15:23:20Z

Thanks for this, @staskus! The test cases and video look good. I'll review today.

guarani · 2022-11-05T02:01:09Z

One thing I noticed is that push notifications don't work on the installable builds. This rings a bell but caught me out.
Notifications work when built from Xcode 👍

guarani

Leaving a partial review here, I tested but haven't dived into the code:

✅ Case 1
✅ Case 2 - the feature flag was already turned on, is that what you see?
✅ Case 3 - shouldn't this be called "Jetpack + Feature Flag"?
✅ Case 4
✅ Case 5
✅ Case 6
✅ Case 7

Judging from the test, this is looking like a promising solution to disabling WP notifications when users migrate to JP (and enable its notifications). Nice work!

sla8c

@staskus This LGTM! All test cases pass for me. I have just left a couple of comments thanks

sla8c · 2022-11-08T11:08:43Z

WordPress/Classes/Services/NotificationFilteringService.swift

+    private var notificationsEnabled: Bool = false
+    private let allowDisablingWPNotifications: Bool
+    private let isWordPress: Bool
+    private let userDefaults = UserDefaults(suiteName: WPAppGroupName)


Perhaps we should add a comment here that this class and solution uses App Groups which allows JetPack app to set the state of wordPressNotificationsEnabled since both WordPress and JetPack are using the same AppGroup name? While testing + reviewing the solution it took me awhile to understand what was going on until I saw WPAppGroupName

I agree. Adding it to the description. 👍

sla8c · 2022-11-08T11:09:14Z

WordPress/Classes/Utility/InteractiveNotificationsManager.swift

+    /// A temporary setting to allow controlling WordPress notifications when they are disabled after Jetpack installation
+    /// Disable WordPress notifications when they are enabled on Jetpack
+    func disableWordPressNotificationsIfNeeded() {
+        let notificationFilteringService = NotificationFilteringService()


nit - perhaps we don't need to assign the local variable here anymore?

I'm going to keep it as it is for now.

I think it's more natural readability to have

notificationFilteringService.disableWordPressNotificationsIfNeeded() rather than NotificationFilteringService().disableWordPressNotificationsIfNeeded() although of course it's subjective and there're no clear guidelines regarding that.

mokagio · 2022-11-11T13:51:15Z

Given this PR still has "WIP" in the title on the Friday before the upcoming 21.2 code freeze on Monday, I'll move it to the 21.3 milestone.

Feel free to push back to 21.2 if this gets finished and reviewed in the meantime 😄

…pp groups

This reverts commit 323fdb1.

staskus · 2022-11-12T19:35:42Z

Updated the PR by including remoteKey, identical to Android, following the example of another feature flags PR.

sla8c

I tested the changes with 'filtered' / mock notification and as discussed with @staskus this doesn't need re-testing.

staskus · 2022-11-14T12:02:53Z

Merging the changes with the feature flag disabled. It does not affect production right now.

Local notification and entitlement changes will be done in other PRs.

hassaanelgarem · 2022-11-24T01:16:11Z

WordPress/Classes/Services/NotificationFilteringService.swift

+    }
+
+    init(notificationSettingsLoader: NotificationSettingsLoader = UNUserNotificationCenter.current(),
+         allowDisablingWPNotifications: Bool = FeatureFlag.allowDisablingWPNotifications.enabled,


Hey @staskus 👋
I noticed you've added a remote feature flag. Just wanted to note that directly using FeatureFlag.enabled does not return the remote value, instead, it returns the flag's default value.

We're aware that it's a bit confusing and we'll try to make it more intuitive in the next iteration.

For now, in order to retrieve the remote value, you'll have to do this:

Suggested change

allowDisablingWPNotifications: Bool = FeatureFlag.allowDisablingWPNotifications.enabled,

allowDisablingWPNotifications: Bool = RemoteFeatureFlagStore().value(for: FeatureFlag.allowDisablingWPNotifications),

Thank you for letting me know! I'll make a change. I thought setting a remoteKey is enough.

@hassaanelgarem

To clarify, remote feature flags cannot be overridden in the Debug menu, right?

This ability has just been added through #19645 👍

🫡 thanks!

staskus added 4 commits October 27, 2022 19:21

Add a feature flag to allow disabling WP notifications

e236969

Disabling WordPress notification flag when Jetpack notifications are …

ddad502

…enabled

Add a Allow Notification cell to Notification Settings to allow reena…

9002960

…bling notifications on WordPress

Filter notifications in NotificationsService

0077667

staskus commented Oct 27, 2022

View reviewed changes

WordPress/WordPressNotificationServiceExtension/Sources/NotificationService.swift Outdated Show resolved Hide resolved

staskus added this to the 21.2 milestone Oct 27, 2022

staskus added 6 commits November 3, 2022 16:52

Merge branch 'trunk' into task/disable-notifications-for-wordpress-app

f7ac0df

Create NotificationFilteringService with tests to contain all the not…

665712c

…ification filtering logic

Merge branch 'trunk' into task/disable-notifications-for-wordpress-app

900c853

Improve allow notifications explanation

9e11f71

Remove excessive comments in NotificationFilteringService

5a0206a

Remove top comment from NotificationFilteringServiceTests

0514782

staskus added Notifications Jetpack Jetpack Focus labels Nov 4, 2022

staskus requested review from guarani and sla8c November 4, 2022 14:56

staskus marked this pull request as ready for review November 4, 2022 14:56

staskus changed the title ~~[Draft] Jetpack Focus: Disable Notifications For WordPress app when Jetpack notifications enabled~~ [WIP] Jetpack Focus: Disable Notifications For WordPress app when Jetpack notifications enabled Nov 4, 2022

staskus mentioned this pull request Nov 4, 2022

Jetpack Focus: Disable Local Notifications #19560

Closed

guarani reviewed Nov 5, 2022

View reviewed changes

sla8c reviewed Nov 8, 2022

View reviewed changes

mokagio modified the milestones: 21.2, 21.3 Nov 11, 2022

staskus added 6 commits November 12, 2022 09:50

Added NotificationsFilteringService comment explaining the usage of a…

42b4680

…pp groups

Use already created Jetpack migration remote feature flags

323fdb1

Revert "Use already created Jetpack migration remote feature flags"

d3e06b6

This reverts commit 323fdb1.

Add remoteKey for allowDisablingWPNotifications flag

8f2ab92

Make remote feature flag key identical to Android

3fd2f5c

Disable default value of allowDisablingWPNotifications feature flag

e565373

Remove incomplete code that filters notifications

52b80d8

staskus changed the title ~~[WIP] Jetpack Focus: Disable Notifications For WordPress app when Jetpack notifications enabled~~ Jetpack Focus: Disable Notifications For WordPress app when Jetpack notifications enabled - Part 1 Nov 14, 2022

staskus requested a review from sla8c November 14, 2022 11:12

sla8c approved these changes Nov 14, 2022

View reviewed changes

staskus merged commit 48633ce into trunk Nov 14, 2022

staskus deleted the task/disable-notifications-for-wordpress-app branch November 14, 2022 12:04

This was referenced Nov 17, 2022

Jetpack Focus: Disable WordPress app notifications from Jetpack app by using URL schemes #19616

Merged

Jetpack Focus: Disable WordPress notifications when notifications are enabled on Jetpack app #19619

Closed

hassaanelgarem reviewed Nov 24, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jetpack Focus: Disable Notifications For WordPress app when Jetpack notifications enabled - Part 1 #19531

Jetpack Focus: Disable Notifications For WordPress app when Jetpack notifications enabled - Part 1 #19531

staskus commented Oct 27, 2022 •

edited

Loading

staskus commented Oct 27, 2022

wpmobilebot commented Oct 27, 2022 •

edited

Loading

wpmobilebot commented Oct 27, 2022 •

edited

Loading

staskus commented Nov 4, 2022 •

edited

Loading

staskus commented Nov 4, 2022

guarani commented Nov 4, 2022

guarani commented Nov 5, 2022

guarani left a comment

sla8c left a comment

sla8c Nov 8, 2022

staskus Nov 12, 2022

sla8c Nov 8, 2022

staskus Nov 12, 2022

mokagio commented Nov 11, 2022

staskus commented Nov 12, 2022

sla8c left a comment

staskus commented Nov 14, 2022

hassaanelgarem Nov 24, 2022

staskus Nov 24, 2022

staskus Nov 24, 2022

hassaanelgarem Nov 24, 2022

staskus Nov 24, 2022

	allowDisablingWPNotifications: Bool = FeatureFlag.allowDisablingWPNotifications.enabled,
	allowDisablingWPNotifications: Bool = RemoteFeatureFlagStore().value(for: FeatureFlag.allowDisablingWPNotifications),

Jetpack Focus: Disable Notifications For WordPress app when Jetpack notifications enabled - Part 1 #19531

Jetpack Focus: Disable Notifications For WordPress app when Jetpack notifications enabled - Part 1 #19531

Conversation

staskus commented Oct 27, 2022 • edited Loading

Description

Solution

Testing instructions

Case 1: WordPress + Receive Notifications

Case 2: WordPress + Feature Flag

Case 3: Jetpack + Feature Flag

Case 4: WordPress + Jetpack + Jetpack Enable Notifications + Receive Notifications

Case 5: WordPress Reenable Notifications

Case 6: WordPress Disable Feature Flag

Case 7: WordPress + Jetpack + Jetpack No Notifications + Receive Notifications

Regression Notes

Images & Videos

staskus commented Oct 27, 2022

wpmobilebot commented Oct 27, 2022 • edited Loading

wpmobilebot commented Oct 27, 2022 • edited Loading

staskus commented Nov 4, 2022 • edited Loading

staskus commented Nov 4, 2022

guarani commented Nov 4, 2022

guarani commented Nov 5, 2022

guarani left a comment

Choose a reason for hiding this comment

sla8c 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

mokagio commented Nov 11, 2022

staskus commented Nov 12, 2022

sla8c left a comment

Choose a reason for hiding this comment

staskus commented Nov 14, 2022

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

staskus commented Oct 27, 2022 •

edited

Loading

wpmobilebot commented Oct 27, 2022 •

edited

Loading

wpmobilebot commented Oct 27, 2022 •

edited

Loading

staskus commented Nov 4, 2022 •

edited

Loading