From 3610c70be16ee7f98f38ae9ae6a45e3ad0c4f5d1 Mon Sep 17 00:00:00 2001 From: Christopher Brind Date: Fri, 12 Jul 2024 14:50:52 +0100 Subject: [PATCH] remove unused gating logic for history roll out (#3075) --- Core/DefaultVariantManager.swift | 5 -- Core/FeatureFlag.swift | 3 - Core/HistoryManager.swift | 36 +--------- DuckDuckGo/AppDelegate.swift | 7 +- DuckDuckGoTests/HistoryManagerTests.swift | 88 +++++++---------------- 5 files changed, 30 insertions(+), 109 deletions(-) diff --git a/Core/DefaultVariantManager.swift b/Core/DefaultVariantManager.swift index 1be1051020..c8018ff0c2 100644 --- a/Core/DefaultVariantManager.swift +++ b/Core/DefaultVariantManager.swift @@ -26,7 +26,6 @@ extension FeatureName { // Define your feature e.g.: // public static let experimentalFeature = FeatureName(rawValue: "experimentalFeature") - public static let history = FeatureName(rawValue: "history") } public struct VariantIOS: Variant { @@ -62,10 +61,6 @@ public struct VariantIOS: Variant { VariantIOS(name: "sd", weight: doNotAllocate, isIncluded: When.always, features: []), VariantIOS(name: "se", weight: doNotAllocate, isIncluded: When.always, features: []), - // This needs to stay until we finish rolling out history to all users... - // This ensures that users who previously had do not lose it. - VariantIOS(name: "md", weight: doNotAllocate, isIncluded: When.inEnglish, features: [.history]), - returningUser ] diff --git a/Core/FeatureFlag.swift b/Core/FeatureFlag.swift index 30d6911a51..bb05e0df29 100644 --- a/Core/FeatureFlag.swift +++ b/Core/FeatureFlag.swift @@ -32,7 +32,6 @@ public enum FeatureFlag: String { case incontextSignup case autoconsentOnByDefault case history - case historyRollout case newTabPageSections case duckPlayer } @@ -62,8 +61,6 @@ extension FeatureFlag: FeatureFlagSourceProviding { return .remoteReleasable(.subfeature(AutoconsentSubfeature.onByDefault)) case .history: return .remoteReleasable(.feature(.history)) - case .historyRollout: - return .remoteReleasable(.subfeature(HistorySubFeature.onByDefault)) case .newTabPageSections: return .internalOnly case .duckPlayer: diff --git a/Core/HistoryManager.swift b/Core/HistoryManager.swift index a6ef0a799d..692d76efea 100644 --- a/Core/HistoryManager.swift +++ b/Core/HistoryManager.swift @@ -33,20 +33,9 @@ public protocol HistoryManaging { } -// Used for controlling incremental rollout -public enum HistorySubFeature: String, PrivacySubfeature { - public var parent: PrivacyFeature { - .history - } - - case onByDefault -} - public class HistoryManager: HistoryManaging { let privacyConfigManager: PrivacyConfigurationManaging - let variantManager: VariantManager - let internalUserDecider: InternalUserDecider let dbCoordinator: HistoryCoordinator public var historyCoordinator: HistoryCoordinating { @@ -66,15 +55,11 @@ public class HistoryManager: HistoryManaging { /// Use `make()` init(privacyConfigManager: PrivacyConfigurationManaging, - variantManager: VariantManager, - internalUserDecider: InternalUserDecider, dbCoordinator: HistoryCoordinator, isAutocompleteEnabledByUser: @autoclosure @escaping () -> Bool, isRecentlyVisitedSitesEnabledByUser: @autoclosure @escaping () -> Bool) { self.privacyConfigManager = privacyConfigManager - self.variantManager = variantManager - self.internalUserDecider = internalUserDecider self.dbCoordinator = dbCoordinator self.isAutocompleteEnabledByUser = isAutocompleteEnabledByUser self.isRecentlyVisitedSitesEnabledByUser = isRecentlyVisitedSitesEnabledByUser @@ -82,23 +67,7 @@ public class HistoryManager: HistoryManaging { /// Determines if the history feature is enabled. This code will need to be cleaned up once the roll out is at 100% public func isHistoryFeatureEnabled() -> Bool { - guard privacyConfigManager.privacyConfig.isEnabled(featureKey: .history) else { - // Whatever happens if this is disabled then disable the feature - return false - } - - if internalUserDecider.isInternalUser { - // Internal users get the feature - return true - } - - if variantManager.isSupported(feature: .history) { - // Users in the experiment get the feature - return true - } - - // Handles incremental roll out to everyone else - return privacyConfigManager.privacyConfig.isSubfeatureEnabled(HistorySubFeature.onByDefault) + return privacyConfigManager.privacyConfig.isEnabled(featureKey: .history) } public func removeAllHistory() async { @@ -234,7 +203,6 @@ extension HistoryManager { /// Should only be called once in the app public static func make(isAutocompleteEnabledByUser: @autoclosure @escaping () -> Bool, isRecentlyVisitedSitesEnabledByUser: @autoclosure @escaping () -> Bool, - internalUserDecider: InternalUserDecider, privacyConfigManager: PrivacyConfigurationManaging) -> Result { let database = HistoryDatabase.make() @@ -251,8 +219,6 @@ extension HistoryManager { let dbCoordinator = HistoryCoordinator(historyStoring: HistoryStore(context: context, eventMapper: HistoryStoreEventMapper())) let historyManager = HistoryManager(privacyConfigManager: privacyConfigManager, - variantManager: DefaultVariantManager(), - internalUserDecider: internalUserDecider, dbCoordinator: dbCoordinator, isAutocompleteEnabledByUser: isAutocompleteEnabledByUser(), isRecentlyVisitedSitesEnabledByUser: isRecentlyVisitedSitesEnabledByUser()) diff --git a/DuckDuckGo/AppDelegate.swift b/DuckDuckGo/AppDelegate.swift index ec895f2abc..69dc0b2f90 100644 --- a/DuckDuckGo/AppDelegate.swift +++ b/DuckDuckGo/AppDelegate.swift @@ -114,7 +114,7 @@ import WebKit } } - // swiftlint:disable:next function_body_length cyclomatic_complexity + // swiftlint:disable:next function_body_length func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey: Any]?) -> Bool { // SKAD4 support @@ -237,10 +237,8 @@ import WebKit variantManager.assignVariantIfNeeded { _ in // MARK: perform first time launch logic here DaxDialogs.shared.primeForUse() - historyMessageManager.dismiss() - } - if variantManager.isSupported(feature: .history) { + // New users don't see the message historyMessageManager.dismiss() } @@ -380,7 +378,6 @@ import WebKit switch HistoryManager.make(isAutocompleteEnabledByUser: settings.autocomplete, isRecentlyVisitedSitesEnabledByUser: settings.recentlyVisitedSites, - internalUserDecider: AppDependencyProvider.shared.internalUserDecider, privacyConfigManager: ContentBlocking.shared.privacyConfigurationManager) { case .failure(let error): diff --git a/DuckDuckGoTests/HistoryManagerTests.swift b/DuckDuckGoTests/HistoryManagerTests.swift index 74414ae853..8ce0afd4ee 100644 --- a/DuckDuckGoTests/HistoryManagerTests.swift +++ b/DuckDuckGoTests/HistoryManagerTests.swift @@ -28,73 +28,43 @@ final class HistoryManagerTests: XCTestCase { let privacyConfig = MockPrivacyConfiguration() let privacyConfigManager = MockPrivacyConfigurationManager() - var variantManager = MockVariantManager() - let internalUserStore = MockInternalUserStoring() - - func test() { - - struct Condition { - - let privacyConfig: Bool - let variant: Bool - let inRollOut: Bool - let internalUser: Bool - let expected: Bool + func testWhenEnabledInPrivacyConfig_ThenFeatureIsEnabled() { + privacyConfig.isFeatureKeyEnabled = { feature, _ in + XCTAssertEqual(feature, .history) + return true } - let conditions = [ - // Users in the experiment should get the feature - Condition(privacyConfig: true, variant: true, inRollOut: false, internalUser: false, expected: true), - Condition(privacyConfig: true, variant: true, inRollOut: true, internalUser: false, expected: true), - - // If not previously in the experiment then check for the rollout - Condition(privacyConfig: true, variant: false, inRollOut: false, internalUser: false, expected: false), - Condition(privacyConfig: true, variant: false, inRollOut: true, internalUser: false, expected: true), - - // Internal users also get the feature - Condition(privacyConfig: true, variant: false, inRollOut: false, internalUser: true, expected: true), - Condition(privacyConfig: true, variant: false, inRollOut: true, internalUser: true, expected: true), - - // Privacy config is the ultimate on/off switch though - Condition(privacyConfig: false, variant: true, inRollOut: true, internalUser: true, expected: false), - ] - - for index in conditions.indices { - let condition = conditions[index] - privacyConfig.isFeatureKeyEnabled = { feature, _ in - XCTAssertEqual(feature, .history) - return condition.privacyConfig - } - - privacyConfig.isSubfeatureKeyEnabled = { subFeature, _ in - XCTAssertEqual(subFeature as? HistorySubFeature, HistorySubFeature.onByDefault) - return condition.inRollOut - } - - internalUserStore.isInternalUser = condition.internalUser - privacyConfigManager.privacyConfig = privacyConfig - variantManager.isSupportedReturns = condition.variant + let model = CoreDataDatabase.loadModel(from: History.bundle, named: "BrowsingHistory")! + let db = CoreDataDatabase(name: "Test", containerLocation: tempDBDir(), model: model) + db.loadStore() - let model = CoreDataDatabase.loadModel(from: History.bundle, named: "BrowsingHistory")! - let db = CoreDataDatabase(name: "Test", containerLocation: tempDBDir(), model: model) - db.loadStore() + let historyManager = makeHistoryManager(db) { + XCTFail("DB Error \($0)") + } - let historyManager = makeHistoryManager(db) { - XCTFail("DB Error \($0)") - } + XCTAssertTrue(historyManager.isHistoryFeatureEnabled()) + XCTAssertTrue(historyManager.historyCoordinator is HistoryCoordinator) + } - let result = historyManager.isHistoryFeatureEnabled() - XCTAssertEqual(condition.expected, result, "\(index): \(condition)") + func testWhenDisabledInPrivacyConfig_ThenFeatureIsDisabled() { + privacyConfig.isFeatureKeyEnabled = { feature, _ in + XCTAssertEqual(feature, .history) + return false + } + + privacyConfigManager.privacyConfig = privacyConfig - if condition.expected { - XCTAssertTrue(historyManager.historyCoordinator is HistoryCoordinator) - } else { - XCTAssertTrue(historyManager.historyCoordinator is NullHistoryCoordinator) - } + let model = CoreDataDatabase.loadModel(from: History.bundle, named: "BrowsingHistory")! + let db = CoreDataDatabase(name: "Test", containerLocation: tempDBDir(), model: model) + db.loadStore() + let historyManager = makeHistoryManager(db) { + XCTFail("DB Error \($0)") } + XCTAssertFalse(historyManager.isHistoryFeatureEnabled()) + XCTAssertTrue(historyManager.historyCoordinator is NullHistoryCoordinator) } func test_WhenUserHasDisabledAutocompleteSitesSetting_ThenDontStoreOrLoadHistory() { @@ -104,7 +74,6 @@ final class HistoryManagerTests: XCTestCase { return true } - internalUserStore.isInternalUser = true privacyConfigManager.privacyConfig = privacyConfig autocompleteEnabledByUser = false @@ -126,7 +95,6 @@ final class HistoryManagerTests: XCTestCase { return true } - internalUserStore.isInternalUser = true privacyConfigManager.privacyConfig = privacyConfig recentlyVisitedSitesEnabledByUser = false @@ -147,8 +115,6 @@ final class HistoryManagerTests: XCTestCase { let dbCoordinator = HistoryCoordinator(historyStoring: store) return HistoryManager(privacyConfigManager: privacyConfigManager, - variantManager: variantManager, - internalUserDecider: DefaultInternalUserDecider(mockedStore: internalUserStore), dbCoordinator: dbCoordinator, isAutocompleteEnabledByUser: self.autocompleteEnabledByUser, isRecentlyVisitedSitesEnabledByUser: self.recentlyVisitedSitesEnabledByUser)