Remote Messaging Framework for macOS #876

ayoy · 2024-07-03T15:49:42Z

Please review the release process for BrowserServicesKit here.

Required:

Task/Issue URL: https://app.asana.com/0/72649045549333/1202913520695928/f
iOS PR: duckduckgo/iOS#3031
macOS PR: duckduckgo/macos-browser#2937
What kind of version bump will this require?: Major

Optional:

CC: @amddg44 @samsymons

Description:
This change adds support for RMF on macOS.

Remaining RMF functionality previously present in iOS codebase was moved to BSK
(most importantly RemoteMessagingProcessing protocol).
remoteMessaging feature flag was added.
RemoteMessagingConfigMatcherProviding protocol was added – it's implemented by client apps
to create and return the config matcher filled with current values of matching attributes.
Support for platform-specific user attribute matchers was added. CommonUserAttributeMatcher was added
to handle attributes that are the supported on both iOS and macOS. Mobile and Desktop user attribute matcher
structs were added to support platform-specific attributes (currently it's only isWidgetInstalled for iOS).
RemoteMessagingRequest was replaced with RemoteMessagingConfigFetcher that uses Configuration Store
and supports Etags.
All RMF tests (and mocks) were moved out of BrowserServicesKit(Tests) into RemoteMessagingTestsUtils
and RemoteMessagingTests (that also mandated creating EmailTestsUtils).
RemoteMessagingStore was updated to keep a reference to the database and create contexts on demand,
to ensure it always gets the freshest data from the persistent store.

Steps to test this PR:
See iOS PR for steps to test.

OS Testing:

Internal references:

Software Engineering Expectations
Technical Design Template

…ding

…ocol

…toring

ayoy · 2024-07-03T21:23:12Z

Package.swift

@@ -166,6 +166,12 @@ let package = Package(
                .define("DEBUG", .when(configuration: .debug))
            ]
        ),
+        .target(
+            name: "EmailTestsUtils",


EmailTestUtils contains EmailManager related mocks that are moved over from BrowserServicesKit so as to avoid making RemoteMessagingTests dependent on BrowserServicesKit

EDIT: I renamed it as BrowserServicesKitTestsUtils to indicate that these are mocks of classes from BrowserServicesKit module. Also moved there StatisticsStore, Variant and VariantManager mocks.

ayoy · 2024-07-03T21:23:47Z

Package.swift

@@ -272,6 +279,13 @@ let package = Package(
                .define("DEBUG", .when(configuration: .debug))
            ]
        ),
+        .target(
+            name: "RemoteMessagingTestsUtils",


RemoteMessagingTestsUtils contains mocks for RemoteMessaging related protocols

ayoy · 2024-07-03T21:24:27Z

Package.swift

@@ -400,7 +414,7 @@ let package = Package(
            name: "BrowserServicesKitTests",
            dependencies: [
                "BrowserServicesKit",
-                "RemoteMessaging", // Move tests later (lots of test dependencies in BSK)


RemoteMessaging has its own tests target now.

ayoy · 2024-07-03T21:26:34Z

Sources/Bookmarks/BookmarkUtils.swift

@@ -184,6 +184,31 @@ public struct BookmarkUtils {
        return result.compactMap { $0[#keyPath(BookmarkEntity.title)] as? String }
    }

+    public static func numberOfBookmarks(in context: NSManagedObjectContext) -> Int {


These 2 functions were used exclusively for RMF in the iOS repo. Now they're in BookmarkUtils and can be reused by macOS.

ayoy · 2024-07-03T21:28:35Z

Sources/EmailTestsUtils/MockEmailManagerRequestDelegate.swift

The 2 files in EmailTestUtils are moved 1:1 from EmailManagerTests.swift in BrowserServicesKit

ayoy · 2024-07-03T21:41:45Z

Sources/RemoteMessaging/Matchers/UserAttributeMatcher.swift

+
+}
+
+public struct CommonUserAttributeMatcher: AttributeMatching {


Common matcher defines matching mechanism for all attributes that are supported by both iOS and macOS.

ayoy · 2024-07-03T21:42:55Z

Sources/RemoteMessaging/Model/MatchingAttributes.swift

@@ -27,7 +27,7 @@ private enum RuleAttributes {
    static let since = "since"
 }

-protocol MatchingAttribute {
+public protocol MatchingAttribute {


AttributeMatching was made public. Since it uses MatchingAttribute, this one needs to be made public as well.

ayoy · 2024-07-03T21:43:27Z

Sources/RemoteMessaging/RemoteMessageRequest.swift

This has been replaced by RemoteMessagingConfigFetcher that uses ConfigurationFetcher/Store.

ayoy · 2024-07-03T22:08:31Z

Sources/RemoteMessaging/RemoteMessagingStore.swift

-    let context: NSManagedObjectContext
+    let database: CoreDataDatabase


This change makes the functions in RemoteMessagingStore create context as needed, ensuring that they have the fresh data all the time. We use a similar approach in Sync Bookmarks provider.

ayoy · 2024-07-03T22:11:04Z

Sources/RemoteMessagingTestsUtils/MockRemoteMessagePercentileStore.swift

@@ -19,12 +19,17 @@
 import Foundation
 import RemoteMessaging

-class MockRemoteMessagePercentileStore: RemoteMessagingPercentileStoring {
+public class MockRemoteMessagePercentileStore: RemoteMessagingPercentileStoring {


Mocks moved to a dedicated mocks target (RemoteMessagingTestsUtils in this case) need to be made public so that the module could be imported without @testable

…n RMF becomes disabled

…ilablePublisher

jaceklyp

looks solid :)

jaceklyp · 2024-07-08T11:02:33Z

Sources/RemoteMessaging/RemoteMessagingProcessing.swift

+            return
+        }
+        do {
+            let statusResponse = try await configurationFetcher.fetchRemoteMessagingConfig()


I think this might be some old naming, where this func used to return status code or something? But now it is config indeed, isn't it?

I would actually suggest a couple of helper func here to make this functionality here more readable, e.g:
private func fetchRemoteMessagingConfig() async throws -> RemoteMessagingConfig
private func processRemoteMessagingConfig(_ config: RemoteMessagingConfig, using store: RemoteMessagingStoring) async throws -> ProcessedResult

where first func just implements: try await configurationFetcher.fetchRemoteMessagingConfig()
second: pretty much old the rest

Thanks! Yeah, I agree it's a bit messy. This function used to be called fetchRemoteMessages. Now I've renamed it but failed to notice that there are two fetchRemoteMessagingConfig functions being called here, from 2 different protocols 🤦 and they do 2 different things, first one fetches the config from the server, and the second one fetches config metadata from the local database. 😬

Let me try to make it right :)

jaceklyp · 2024-07-08T11:04:44Z

Sources/RemoteMessaging/RemoteMessagingProcessing.swift

+            if let processorResult = processor.process(jsonRemoteMessagingConfig: statusResponse, currentConfig: config) {
+                store.saveProcessedResult(processorResult)
+            }


perhaps this should be guard that throws some processing failed error? otherwise we're not getting any debug info what happened

This method calls a bunch of Store private methods inside, each of which can throw an error that is handled by sending a pixel (via EventMapping) and printing a console log. So I think it's acceptable. I agree that it's not ideal though.

Also, this function's error is silenced at the call site (with try? – again probably acceptable in this specific case because it effectively silences only network and JSON parsing errors). I may propose an update to this piece of code but it has been a status quo for past 2 years so I'd rather handle it separately.

don't bother, makes sense to me

… abit more clear

* main: Remove unused VPN session utilities (#898) Add new deprecated Mac remote message attribute. (#903) Resetting all state for the VPN will cancel the tunnel and stop the monitors (#900) Add support for skipping sending usage pixels for remote messages (#902) Bump Tests/BrowserServicesKitTests/Resources/privacy-reference-tests (#896) Removes the listen port from the wireguard client (#901) Be explicit when performing developer redirects (#884) C-S-S cross origin fixes Update C-S-S version (#892) Add a debug menu action to reset Remote Messages on macOS (#891) Add desktop specific RMF attributes (#883) Upload exception message to Sentry (#856) Add locale to broken site report (#889) Add new subfeature for duckplayer (#885) Swiftlint refactoring (#882) Remote Messaging Framework for macOS (#876)

ayoy added 21 commits July 2, 2024 15:59

Add numberOfBookmarks and numberOfFavorites to BookmarkUtils

a7adf0f

Add RemoteMessagingClientBase class

419e6df

Update data source

dfe0f9c

Update RemoteMessagingClientBase.fetchAndProcess

fcdfa1d

Fix formatting

861d19c

Rename RemoteMessagingDataSource as RemoteMessagingConfigMatcherProvi…

59935c9

…ding

Replace RemoteMessagingClientBase with RemoteMessagingProcessing prot…

cc6afff

…ocol

Make some RemoteMessagingConfigProcessor APIs private

61f6379

Use ConfigurationFetcher for fetching RMF config

65e1d06

Use proper ConfigurationFetcher API

d484ebc

Add mobile and desktop user attribute matchers

80a8a79

Abstract attribute matchers in RemoteMessagingConfigMatcher

088bf37

Move all RMF tests to RemoteMessagingTests

172e980

Clean up imports in tests

70aa7b1

Add remoteMessaging feature flag

37f51ef

Add privacyConfigurationManager to RMF store

ccb40d0

Add more mocks for Remote Messaging

df1d513

Add PrivacyConfigurationRemoteMessagingAvailabilityProvider

3a9e5dd

Replace UserDefaults with KeyValueStore in RemoteMessagingPercentileS…

1181fa6

…toring

Add RemoteMessagingProcessingTests

2d6d684

Add feature flag tests to RemoteMessagingStore

c8778cb

ayoy self-assigned this Jul 3, 2024

ayoy added 8 commits July 3, 2024 17:58

Fix SwiftLint violations

fd1358c

Fix more SwiftLint violations

687cdcc

Fix more SwiftLint violations

687bfba

Add unit tests for numberOfBookmarks and numberOfFavorites

9644245

Revert a change to EmailUrls.Url.emailAlias

aadcd91

AttributeMatcher -> AttributeMatching

7fd8d07

Add documentation to RemoteMessagingAvailabilityProviding

8334115

Remote commented out code

99023c9

Add documentation

e7c7b09

ayoy commented Jul 3, 2024

View reviewed changes

Merge branch 'main' into dominik/rmf-macos

7c39e61

ayoy marked this pull request as ready for review July 4, 2024 07:19

ayoy requested a review from jaceklyp July 4, 2024 07:19

ayoy assigned jaceklyp and unassigned ayoy Jul 4, 2024

ayoy added 5 commits July 4, 2024 11:06

Merge branch 'main' into dominik/rmf-macos

4d7daa3

EmailTestsUtils -> BrowserServicesKitTestsUtils

23b0dc1

Add RMF availability flag publisher and delete scheduled messages whe…

8e742e3

…n RMF becomes disabled

Add documentation for isRemoteMessagingAvailablePublisher

4d111ca

Drop first PrivacyConfigurationManager update in isRemoteMessagingAva…

324baf8

…ilablePublisher

jaceklyp approved these changes Jul 8, 2024

View reviewed changes

ayoy added 4 commits July 8, 2024 13:35

Rename configurationFetcher as configFetcher and make fetchAndProcess…

1cc19c3

… abit more clear

Id -> ID in RemoteMessagingStoring APIs

d27f306

Merge branch 'main' into dominik/rmf-macos

2319ec8

Add DefaultRemoteMessagingSurveyURLBuilder (moved from client repos)

ed3cf77

jaceklyp approved these changes Jul 8, 2024

View reviewed changes

ayoy merged commit bfabf45 into main Jul 9, 2024
7 checks passed

ayoy deleted the dominik/rmf-macos branch July 9, 2024 06:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remote Messaging Framework for macOS #876

Remote Messaging Framework for macOS #876

ayoy commented Jul 3, 2024 •

edited

Loading

ayoy Jul 3, 2024

ayoy Jul 5, 2024

ayoy Jul 3, 2024

ayoy Jul 3, 2024

ayoy Jul 3, 2024

ayoy Jul 3, 2024

ayoy Jul 3, 2024

ayoy Jul 3, 2024

ayoy Jul 3, 2024

ayoy Jul 3, 2024

ayoy Jul 3, 2024

jaceklyp left a comment

jaceklyp Jul 8, 2024

ayoy Jul 8, 2024

jaceklyp Jul 8, 2024

ayoy Jul 8, 2024

jaceklyp Jul 8, 2024


		}

		public struct CommonUserAttributeMatcher: AttributeMatching {

		let context: NSManagedObjectContext
		let database: CoreDataDatabase

Remote Messaging Framework for macOS #876

Remote Messaging Framework for macOS #876

Conversation

ayoy commented Jul 3, 2024 • edited Loading

Internal references:

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

jaceklyp 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

ayoy commented Jul 3, 2024 •

edited

Loading