address comments

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

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
         }

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>) -> Double) -> PrivacyConfigurationFeatureState
 
     func cohorts(for subfeature: any PrivacySubfeature) -> [PrivacyConfigurationData.Cohort]?

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
     }
 

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

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)