From b0574316b9b7d318d16ddbc659b1b74659d2efef Mon Sep 17 00:00:00 2001 From: Sabrina Tardio Date: Thu, 28 Nov 2024 22:51:42 +0100 Subject: [PATCH] address comments --- .../ExperimentCohortsManager.swift | 23 +++++++++---------- .../FeatureFlagger/FeatureFlagger.swift | 10 ++++---- .../PrivacyConfig/PrivacyConfiguration.swift | 2 +- .../ContentBlocker/WebViewTestHelper.swift | 2 +- .../DefaultFeatureFlaggerTests.swift | 2 +- .../ExperimentCohortsManagerTests.swift | 16 ++++++------- .../Resources/privacy-reference-tests | 2 +- 7 files changed, 27 insertions(+), 30 deletions(-) diff --git a/Sources/BrowserServicesKit/FeatureFlagger/ExperimentCohortsManager.swift b/Sources/BrowserServicesKit/FeatureFlagger/ExperimentCohortsManager.swift index 86a196334..8129cc0a6 100644 --- a/Sources/BrowserServicesKit/FeatureFlagger/ExperimentCohortsManager.swift +++ b/Sources/BrowserServicesKit/FeatureFlagger/ExperimentCohortsManager.swift @@ -29,8 +29,8 @@ public typealias SubfeatureID = String public typealias ParentFeatureID = String public struct ExperimentData: Codable, Equatable { - public let parentID: String - public let cohortID: String + public let parentID: ParentFeatureID + public let cohortID: CohortID public let enrollmentDate: Date } @@ -46,26 +46,26 @@ public protocol ExperimentCohortsManaging { /// for the specified experiment. If the assigned cohort is valid (i.e., it matches /// one of the experiment's defined cohorts), the method returns the assigned cohort. /// Otherwise, the invalid cohort is removed, and a new cohort is assigned if - /// `isAssignCohortEnabled` is `true`. + /// `allowCohortReassignment` is `true`. /// /// - Parameters: /// - experiment: The `ExperimentSubfeature` representing the experiment and its associated cohorts. - /// - isAssignCohortEnabled: A Boolean value indicating whether cohort assignment is allowed + /// - allowCohortReassignment: A Boolean value indicating whether cohort assignment is allowed /// if the user is not already assigned to a valid cohort. /// /// - Returns: The valid `CohortID` assigned to the user for the experiment, or `nil` - /// if no valid cohort exists and `isAssignCohortEnabled` is `false`. + /// if no valid cohort exists and `allowCohortReassignment` is `false`. /// /// - Behavior: /// 1. Retrieves the currently assigned cohort for the experiment using the `subfeatureID`. /// 2. Validates if the assigned cohort exists within the experiment's cohort list: /// - If valid, the assigned cohort is returned. /// - If invalid, the cohort is removed from storage. - /// 3. If cohort assignment is enabled (`isAssignCohortEnabled` is `true`), a new cohort + /// 3. If cohort assignment is enabled (`allowCohortReassignment` is `true`), a new cohort /// is assigned based on the experiment's cohort weights and saved in storage. /// - Cohort assignment is probabilistic, determined by the cohort weights. /// - func resolveCohort(for experiment: ExperimentSubfeature, isAssignCohortEnabled: Bool) -> CohortID? + func resolveCohort(for experiment: ExperimentSubfeature, allowCohortReassignment: Bool) -> CohortID? } public class ExperimentCohortsManager: ExperimentCohortsManaging { @@ -87,15 +87,14 @@ public class ExperimentCohortsManager: ExperimentCohortsManaging { self.randomizer = randomizer } - public func resolveCohort(for experiment: ExperimentSubfeature, isAssignCohortEnabled: Bool) -> CohortID? { + public func resolveCohort(for experiment: ExperimentSubfeature, allowCohortReassignment: Bool) -> CohortID? { queue.sync { let assignedCohort = cohort(for: experiment.subfeatureID) if experiment.cohorts.contains(where: { $0.name == assignedCohort }) { - return (assignedCohort) - } else { - removeCohort(from: experiment.subfeatureID) + return assignedCohort } - return isAssignCohortEnabled ? assignCohort(to: experiment) : nil + removeCohort(from: experiment.subfeatureID) + return allowCohortReassignment ? assignCohort(to: experiment) : nil } } } diff --git a/Sources/BrowserServicesKit/FeatureFlagger/FeatureFlagger.swift b/Sources/BrowserServicesKit/FeatureFlagger/FeatureFlagger.swift index b609860ee..3ecb53d1e 100644 --- a/Sources/BrowserServicesKit/FeatureFlagger/FeatureFlagger.swift +++ b/Sources/BrowserServicesKit/FeatureFlagger/FeatureFlagger.swift @@ -278,8 +278,8 @@ public class DefaultFeatureFlagger: FeatureFlagger { } public func getAllActiveExperiments() -> Experiments { + guard let enrolledExperiments = experimentManager?.experiments else { return [:] } var activeExperiments = [String: ExperimentData]() - guard let enrolledExperiments = experimentManager?.experiments else { return activeExperiments } let config = privacyConfigManager.privacyConfig for (subfeatureID, experimentData) in enrolledExperiments { @@ -288,7 +288,7 @@ public class DefaultFeatureFlagger: FeatureFlagger { let cohorts = config.cohorts(subfeatureID: subfeatureID, parentFeatureID: experimentData.parentID) ?? [] let experimentSubfeature = ExperimentSubfeature(parentID: experimentData.parentID, subfeatureID: subfeatureID, cohorts: cohorts) - if experimentManager?.resolveCohort(for: experimentSubfeature, isAssignCohortEnabled: false) == experimentData.cohortID { + if experimentManager?.resolveCohort(for: experimentSubfeature, allowCohortReassignment: false) == experimentData.cohortID { activeExperiments[subfeatureID] = experimentData } } @@ -301,8 +301,6 @@ public class DefaultFeatureFlagger: FeatureFlagger { return nil case .internalOnly(let cohort): return cohort - case .remoteDevelopment where !internalUserDecider.isInternalUser: - return nil case .remoteReleasable(let featureType), .remoteDevelopment(let featureType) where internalUserDecider.isInternalUser: if case .subfeature(let subfeature) = featureType { @@ -323,9 +321,9 @@ public class DefaultFeatureFlagger: FeatureFlagger { let experiment = ExperimentSubfeature(parentID: subfeature.parent.rawValue, subfeatureID: subfeature.rawValue, cohorts: cohorts ?? []) switch featureState { case .enabled: - return experimentManager?.resolveCohort(for: experiment, isAssignCohortEnabled: true) + return experimentManager?.resolveCohort(for: experiment, allowCohortReassignment: true) case .disabled(.targetDoesNotMatch): - return experimentManager?.resolveCohort(for: experiment, isAssignCohortEnabled: false) + return experimentManager?.resolveCohort(for: experiment, allowCohortReassignment: false) default: return nil } diff --git a/Sources/BrowserServicesKit/PrivacyConfig/PrivacyConfiguration.swift b/Sources/BrowserServicesKit/PrivacyConfig/PrivacyConfiguration.swift index 358ce21e9..8cb98866f 100644 --- a/Sources/BrowserServicesKit/PrivacyConfig/PrivacyConfiguration.swift +++ b/Sources/BrowserServicesKit/PrivacyConfig/PrivacyConfiguration.swift @@ -105,7 +105,7 @@ public protocol PrivacyConfiguration { /// Adds given domain to locally unprotected list. func userDisabledProtection(forDomain: String) - // APIs used for Exmpriments + // APIs used for Experiments func stateFor(subfeatureID: SubfeatureID, parentFeatureID: ParentFeatureID, versionProvider: AppVersionProvider, randomizer: (Range) -> Double) -> PrivacyConfigurationFeatureState func cohorts(for subfeature: any PrivacySubfeature) -> [PrivacyConfigurationData.Cohort]? diff --git a/Tests/BrowserServicesKitTests/ContentBlocker/WebViewTestHelper.swift b/Tests/BrowserServicesKitTests/ContentBlocker/WebViewTestHelper.swift index d3a2348fc..bc979233e 100644 --- a/Tests/BrowserServicesKitTests/ContentBlocker/WebViewTestHelper.swift +++ b/Tests/BrowserServicesKitTests/ContentBlocker/WebViewTestHelper.swift @@ -227,7 +227,7 @@ final class WebKitTestHelper { } class MockExperimentCohortsManager: ExperimentCohortsManaging { - func resolveCohort(for experiment: ExperimentSubfeature, isAssignCohortEnabled: Bool) -> CohortID? { + func resolveCohort(for experiment: ExperimentSubfeature, allowCohortReassignment: Bool) -> CohortID? { return nil } diff --git a/Tests/BrowserServicesKitTests/FeatureFlagging/DefaultFeatureFlaggerTests.swift b/Tests/BrowserServicesKitTests/FeatureFlagging/DefaultFeatureFlaggerTests.swift index c8e17ac2d..6f5fffbd4 100644 --- a/Tests/BrowserServicesKitTests/FeatureFlagging/DefaultFeatureFlaggerTests.swift +++ b/Tests/BrowserServicesKitTests/FeatureFlagging/DefaultFeatureFlaggerTests.swift @@ -374,7 +374,7 @@ class MockExperimentManager: ExperimentCohortsManaging { var cohortToReturn: CohortID? var experiments: BrowserServicesKit.Experiments? - func resolveCohort(for experiment: BrowserServicesKit.ExperimentSubfeature, isAssignCohortEnabled: Bool) -> CohortID? { + func resolveCohort(for experiment: BrowserServicesKit.ExperimentSubfeature, allowCohortReassignment: Bool) -> CohortID? { return cohortToReturn } } diff --git a/Tests/BrowserServicesKitTests/FeatureFlagging/ExperimentCohortsManagerTests.swift b/Tests/BrowserServicesKitTests/FeatureFlagging/ExperimentCohortsManagerTests.swift index 80ffa7594..48fc85355 100644 --- a/Tests/BrowserServicesKitTests/FeatureFlagging/ExperimentCohortsManagerTests.swift +++ b/Tests/BrowserServicesKitTests/FeatureFlagging/ExperimentCohortsManagerTests.swift @@ -94,8 +94,8 @@ final class ExperimentCohortsManagerTests: XCTestCase { mockStore.experiments = [subfeatureName1: experimentData1, subfeatureName2: experimentData2] // WHEN - let result1 = experimentCohortsManager.resolveCohort(for: ExperimentSubfeature(parentID: experimentData1.parentID, subfeatureID: subfeatureName1, cohorts: [cohort1, cohort2]), isAssignCohortEnabled: false) - let result2 = experimentCohortsManager.resolveCohort(for: ExperimentSubfeature(parentID: experimentData2.parentID, subfeatureID: subfeatureName2, cohorts: [cohort2, cohort3]), isAssignCohortEnabled: false) + let result1 = experimentCohortsManager.resolveCohort(for: ExperimentSubfeature(parentID: experimentData1.parentID, subfeatureID: subfeatureName1, cohorts: [cohort1, cohort2]), allowCohortReassignment: false) + let result2 = experimentCohortsManager.resolveCohort(for: ExperimentSubfeature(parentID: experimentData2.parentID, subfeatureID: subfeatureName2, cohorts: [cohort2, cohort3]), allowCohortReassignment: false) // THEN XCTAssertEqual(result1, experimentData1.cohortID) @@ -109,7 +109,7 @@ final class ExperimentCohortsManagerTests: XCTestCase { let experiment = ExperimentSubfeature(parentID: experimentData1.parentID, subfeatureID: subfeatureName1, cohorts: cohorts) // WHEN - let result = experimentCohortsManager.resolveCohort(for: experiment, isAssignCohortEnabled: true) + let result = experimentCohortsManager.resolveCohort(for: experiment, allowCohortReassignment: true) // THEN XCTAssertNotNil(result) @@ -123,7 +123,7 @@ final class ExperimentCohortsManagerTests: XCTestCase { let experiment = ExperimentSubfeature(parentID: experimentData1.parentID, subfeatureID: subfeatureName1, cohorts: cohorts) // WHEN - let result = experimentCohortsManager.resolveCohort(for: experiment, isAssignCohortEnabled: false) + let result = experimentCohortsManager.resolveCohort(for: experiment, allowCohortReassignment: false) // THEN XCTAssertNil(result) @@ -135,7 +135,7 @@ final class ExperimentCohortsManagerTests: XCTestCase { let experiment = ExperimentSubfeature(parentID: "TestParent", subfeatureID: "NonExistentSubfeature", cohorts: []) // WHEN - let result = experimentCohortsManager.resolveCohort(for: experiment, isAssignCohortEnabled: true) + let result = experimentCohortsManager.resolveCohort(for: experiment, allowCohortReassignment: true) // THEN XCTAssertNil(result) @@ -146,7 +146,7 @@ final class ExperimentCohortsManagerTests: XCTestCase { mockStore.experiments = [subfeatureName1: experimentData1] // WHEN - let result1 = experimentCohortsManager.resolveCohort(for: ExperimentSubfeature(parentID: experimentData1.parentID, subfeatureID: subfeatureName1, cohorts: [cohort2, cohort3]), isAssignCohortEnabled: true) + let result1 = experimentCohortsManager.resolveCohort(for: ExperimentSubfeature(parentID: experimentData1.parentID, subfeatureID: subfeatureName1, cohorts: [cohort2, cohort3]), allowCohortReassignment: true) // THEN XCTAssertEqual(result1, experimentData3.cohortID) @@ -157,7 +157,7 @@ final class ExperimentCohortsManagerTests: XCTestCase { mockStore.experiments = [subfeatureName1: experimentData1] // WHEN - let result1 = experimentCohortsManager.resolveCohort(for: ExperimentSubfeature(parentID: experimentData1.parentID, subfeatureID: subfeatureName1, cohorts: [cohort2, cohort3]), isAssignCohortEnabled: false) + let result1 = experimentCohortsManager.resolveCohort(for: ExperimentSubfeature(parentID: experimentData1.parentID, subfeatureID: subfeatureName1, cohorts: [cohort2, cohort3]), allowCohortReassignment: false) // THEN XCTAssertNil(result1) @@ -177,7 +177,7 @@ final class ExperimentCohortsManagerTests: XCTestCase { ) // WHEN - let result = experimentCohortsManager.resolveCohort(for: experiment, isAssignCohortEnabled: true) + let result = experimentCohortsManager.resolveCohort(for: experiment, allowCohortReassignment: true) // THEN XCTAssertEqual(result, experimentData3.cohortID) diff --git a/Tests/BrowserServicesKitTests/Resources/privacy-reference-tests b/Tests/BrowserServicesKitTests/Resources/privacy-reference-tests index 6133e7d9d..a603ff9af 160000 --- a/Tests/BrowserServicesKitTests/Resources/privacy-reference-tests +++ b/Tests/BrowserServicesKitTests/Resources/privacy-reference-tests @@ -1 +1 @@ -Subproject commit 6133e7d9d9cd5f1b925cab1971b4d785dc639df7 +Subproject commit a603ff9af22ca3ff7ce2e7ffbfe18c447d9f23e8