From a4c9135b58c8f3c8e2df364e1efe86894e6c320e Mon Sep 17 00:00:00 2001 From: Stefan Ceriu Date: Thu, 30 May 2024 14:10:51 +0300 Subject: [PATCH] Decouple the bug report service from the analytics one --- .../Sources/Application/AppCoordinator.swift | 16 ++++++-- .../Mocks/Generated/GeneratedMocks.swift | 35 ------------------ .../View/AnalyticsSettingsScreen.swift | 4 +- .../Services/Analytics/AnalyticsService.swift | 24 +++++++----- .../Analytics/PostHogAnalyticsClient.swift | 2 +- .../Services/BugReport/BugReportService.swift | 7 +--- .../BugReport/BugReportServiceProtocol.swift | 2 - .../UITests/UITestsAppCoordinator.swift | 8 ++-- .../UnitTests/UnitTestsAppCoordinator.swift | 8 ++-- ...nalyticsSettingsScreenViewModelTests.swift | 3 +- UnitTests/Sources/AnalyticsTests.swift | 37 +++++++------------ .../ComposerToolbarViewModelTests.swift | 6 ++- 12 files changed, 58 insertions(+), 94 deletions(-) diff --git a/ElementX/Sources/Application/AppCoordinator.swift b/ElementX/Sources/Application/AppCoordinator.swift index 5eb5570e71..d4631a40c7 100644 --- a/ElementX/Sources/Application/AppCoordinator.swift +++ b/ElementX/Sources/Application/AppCoordinator.swift @@ -93,6 +93,17 @@ class AppCoordinator: AppCoordinatorProtocol, AuthenticationFlowCoordinatorDeleg Self.setupServiceLocator(appSettings: appSettings) + ServiceLocator.shared.analytics.isRunningPublisher + .removeDuplicates() + .sink { isRunning in + if isRunning { + ServiceLocator.shared.bugReportService.start() + } else { + ServiceLocator.shared.bugReportService.stop() + } + } + .store(in: &cancellables) + ServiceLocator.shared.analytics.startIfEnabled() stateMachine = AppCoordinatorStateMachine() @@ -337,10 +348,9 @@ class AppCoordinator: AppCoordinatorProtocol, AuthenticationFlowCoordinatorDeleg sdkGitSHA: sdkGitSha(), maxUploadSize: appSettings.bugReportMaxUploadSize)) let posthogAnalyticsClient = PostHogAnalyticsClient() - posthogAnalyticsClient.updateSuperProperties(AnalyticsEvent.SuperProperties(appPlatform: nil, cryptoSDK: .Rust, cryptoSDKVersion: sdkGitSha())) + posthogAnalyticsClient.updateSuperProperties(AnalyticsEvent.SuperProperties(appPlatform: .EXI, cryptoSDK: .Rust, cryptoSDKVersion: sdkGitSha())) ServiceLocator.shared.register(analytics: AnalyticsService(client: posthogAnalyticsClient, - appSettings: appSettings, - bugReportService: ServiceLocator.shared.bugReportService)) + appSettings: appSettings)) } /// Perform any required migrations for the app to function correctly. diff --git a/ElementX/Sources/Mocks/Generated/GeneratedMocks.swift b/ElementX/Sources/Mocks/Generated/GeneratedMocks.swift index 5c664ea3f2..aa8ca64fa3 100644 --- a/ElementX/Sources/Mocks/Generated/GeneratedMocks.swift +++ b/ElementX/Sources/Mocks/Generated/GeneratedMocks.swift @@ -1856,41 +1856,6 @@ class BugReportServiceMock: BugReportServiceProtocol { stopCallsCount += 1 stopClosure?() } - //MARK: - reset - - var resetUnderlyingCallsCount = 0 - var resetCallsCount: Int { - get { - if Thread.isMainThread { - return resetUnderlyingCallsCount - } else { - var returnValue: Int? = nil - DispatchQueue.main.sync { - returnValue = resetUnderlyingCallsCount - } - - return returnValue! - } - } - set { - if Thread.isMainThread { - resetUnderlyingCallsCount = newValue - } else { - DispatchQueue.main.sync { - resetUnderlyingCallsCount = newValue - } - } - } - } - var resetCalled: Bool { - return resetCallsCount > 0 - } - var resetClosure: (() -> Void)? - - func reset() { - resetCallsCount += 1 - resetClosure?() - } //MARK: - submitBugReport var submitBugReportProgressListenerUnderlyingCallsCount = 0 diff --git a/ElementX/Sources/Screens/Settings/AnalyticsSettingsScreen/View/AnalyticsSettingsScreen.swift b/ElementX/Sources/Screens/Settings/AnalyticsSettingsScreen/View/AnalyticsSettingsScreen.swift index 35be839f29..dd31147f12 100644 --- a/ElementX/Sources/Screens/Settings/AnalyticsSettingsScreen/View/AnalyticsSettingsScreen.swift +++ b/ElementX/Sources/Screens/Settings/AnalyticsSettingsScreen/View/AnalyticsSettingsScreen.swift @@ -49,9 +49,7 @@ struct AnalyticsSettingsScreen_Previews: PreviewProvider, TestablePreview { static var previews: some View { let appSettings = AppSettings() let viewModel = AnalyticsSettingsScreenViewModel(appSettings: appSettings, - analytics: AnalyticsService(client: AnalyticsClientMock(), - appSettings: appSettings, - bugReportService: BugReportServiceMock())) + analytics: ServiceLocator.shared.analytics) AnalyticsSettingsScreen(context: viewModel.context) } } diff --git a/ElementX/Sources/Services/Analytics/AnalyticsService.swift b/ElementX/Sources/Services/Analytics/AnalyticsService.swift index 0e363abefa..2caf474da9 100644 --- a/ElementX/Sources/Services/Analytics/AnalyticsService.swift +++ b/ElementX/Sources/Services/Analytics/AnalyticsService.swift @@ -15,6 +15,7 @@ // import AnalyticsEvents +import Combine import PostHog /// A class responsible for managing a variety of analytics clients @@ -34,20 +35,23 @@ class AnalyticsService { /// The analytics client to send events with. private let client: AnalyticsClientProtocol private let appSettings: AppSettings - private let bugReportService: BugReportServiceProtocol /// A signpost client for performance testing the app. This client doesn't respect the /// `isRunning` state or behave any differently when `start`/`reset` are called. let signpost = Signposter() + + /// Whether or not the object is enabled and sending events to the server. + private let isRunningSubject: CurrentValueSubject = .init(false) + var isRunningPublisher: CurrentValuePublisher { + isRunningSubject.asCurrentValuePublisher() + } - init(client: AnalyticsClientProtocol, appSettings: AppSettings, bugReportService: BugReportServiceProtocol) { + init(client: AnalyticsClientProtocol, appSettings: AppSettings) { self.client = client self.appSettings = appSettings - self.bugReportService = bugReportService - } - /// Whether or not the object is enabled and sending events to the server. - var isRunning: Bool { client.isRunning } + isRunningSubject.send(client.isRunning) + } /// Whether to show the user the analytics opt in prompt. var shouldShowAnalyticsPrompt: Bool { @@ -72,20 +76,21 @@ class AnalyticsService { // The order is important here. PostHog ignores the reset if stopped. reset() client.stop() - bugReportService.stop() + + isRunningSubject.send(false) MXLog.info("Stopped.") } /// Starts the analytics client if the user has opted in, otherwise does nothing. func startIfEnabled() { - guard isEnabled, !isRunning else { return } + guard isEnabled, !isRunningPublisher.value else { return } client.start(analyticsConfiguration: appSettings.analyticsConfiguration) - bugReportService.start() // Sanity check in case something went wrong. guard client.isRunning else { return } + isRunningSubject.send(true) MXLog.info("Started.") } @@ -95,7 +100,6 @@ class AnalyticsService { /// Note: **MUST** be called before stopping PostHog or the reset is ignored. func reset() { client.reset() - bugReportService.reset() MXLog.info("Reset.") } diff --git a/ElementX/Sources/Services/Analytics/PostHogAnalyticsClient.swift b/ElementX/Sources/Services/Analytics/PostHogAnalyticsClient.swift index 61b81a5c96..83ed3469c6 100644 --- a/ElementX/Sources/Services/Analytics/PostHogAnalyticsClient.swift +++ b/ElementX/Sources/Services/Analytics/PostHogAnalyticsClient.swift @@ -53,7 +53,7 @@ class PostHogAnalyticsClient: AnalyticsClientProtocol { // Add super property cryptoSDK to the captured events, to allow easy // filtering of events across different client by using same filter. - superProperties = AnalyticsEvent.SuperProperties(appPlatform: nil, cryptoSDK: .Rust, cryptoSDKVersion: nil) + superProperties = AnalyticsEvent.SuperProperties(appPlatform: .EXI, cryptoSDK: .Rust, cryptoSDKVersion: nil) postHog?.optIn() } diff --git a/ElementX/Sources/Services/BugReport/BugReportService.swift b/ElementX/Sources/Services/BugReport/BugReportService.swift index ca8ab5b65e..09cae3a9b7 100644 --- a/ElementX/Sources/Services/BugReport/BugReportService.swift +++ b/ElementX/Sources/Services/BugReport/BugReportService.swift @@ -105,12 +105,7 @@ class BugReportService: NSObject, BugReportServiceProtocol { SentrySDK.close() MXLog.info("Stopped.") } - - func reset() { - lastCrashEventId = nil - MXLog.info("Reset.") - } - + // swiftlint:disable:next cyclomatic_complexity func submitBugReport(_ bugReport: BugReport, progressListener: CurrentValueSubject) async -> Result { diff --git a/ElementX/Sources/Services/BugReport/BugReportServiceProtocol.swift b/ElementX/Sources/Services/BugReport/BugReportServiceProtocol.swift index dae5f1661e..4139523b5d 100644 --- a/ElementX/Sources/Services/BugReport/BugReportServiceProtocol.swift +++ b/ElementX/Sources/Services/BugReport/BugReportServiceProtocol.swift @@ -62,8 +62,6 @@ protocol BugReportServiceProtocol { func stop() - func reset() - func submitBugReport(_ bugReport: BugReport, progressListener: CurrentValueSubject) async -> Result } diff --git a/ElementX/Sources/UITests/UITestsAppCoordinator.swift b/ElementX/Sources/UITests/UITestsAppCoordinator.swift index 516179136e..cfe7b31ec0 100644 --- a/ElementX/Sources/UITests/UITestsAppCoordinator.swift +++ b/ElementX/Sources/UITests/UITestsAppCoordinator.swift @@ -45,9 +45,11 @@ class UITestsAppCoordinator: AppCoordinatorProtocol, SecureWindowManagerDelegate AppSettings.resetAllSettings() ServiceLocator.shared.register(appSettings: AppSettings()) ServiceLocator.shared.register(bugReportService: BugReportServiceMock()) - ServiceLocator.shared.register(analytics: AnalyticsService(client: AnalyticsClientMock(), - appSettings: ServiceLocator.shared.settings, - bugReportService: ServiceLocator.shared.bugReportService)) + + let analyticsClient = AnalyticsClientMock() + analyticsClient.isRunning = false + ServiceLocator.shared.register(analytics: AnalyticsService(client: analyticsClient, + appSettings: ServiceLocator.shared.settings)) } func start() { diff --git a/ElementX/Sources/UnitTests/UnitTestsAppCoordinator.swift b/ElementX/Sources/UnitTests/UnitTestsAppCoordinator.swift index 6c669d8a2b..689d672580 100644 --- a/ElementX/Sources/UnitTests/UnitTestsAppCoordinator.swift +++ b/ElementX/Sources/UnitTests/UnitTestsAppCoordinator.swift @@ -27,9 +27,11 @@ class UnitTestsAppCoordinator: AppCoordinatorProtocol { AppSettings.resetAllSettings() ServiceLocator.shared.register(appSettings: AppSettings()) ServiceLocator.shared.register(bugReportService: BugReportServiceMock()) - ServiceLocator.shared.register(analytics: AnalyticsService(client: AnalyticsClientMock(), - appSettings: ServiceLocator.shared.settings, - bugReportService: ServiceLocator.shared.bugReportService)) + + let analyticsClient = AnalyticsClientMock() + analyticsClient.isRunning = false + ServiceLocator.shared.register(analytics: AnalyticsService(client: analyticsClient, + appSettings: ServiceLocator.shared.settings)) } func start() { } diff --git a/UnitTests/Sources/AnalyticsSettingsScreenViewModelTests.swift b/UnitTests/Sources/AnalyticsSettingsScreenViewModelTests.swift index bb2fa19f66..fa3c01f209 100644 --- a/UnitTests/Sources/AnalyticsSettingsScreenViewModelTests.swift +++ b/UnitTests/Sources/AnalyticsSettingsScreenViewModelTests.swift @@ -34,8 +34,7 @@ class AnalyticsSettingsScreenViewModelTests: XCTestCase { let analyticsClient = AnalyticsClientMock() analyticsClient.isRunning = false ServiceLocator.shared.register(analytics: AnalyticsService(client: analyticsClient, - appSettings: appSettings, - bugReportService: ServiceLocator.shared.bugReportService)) + appSettings: appSettings)) viewModel = AnalyticsSettingsScreenViewModel(appSettings: appSettings, analytics: ServiceLocator.shared.analytics) diff --git a/UnitTests/Sources/AnalyticsTests.swift b/UnitTests/Sources/AnalyticsTests.swift index 9dcb69eb18..dff89453dc 100644 --- a/UnitTests/Sources/AnalyticsTests.swift +++ b/UnitTests/Sources/AnalyticsTests.swift @@ -22,21 +22,16 @@ import XCTest class AnalyticsTests: XCTestCase { private var appSettings: AppSettings! private var analyticsClient: AnalyticsClientMock! - private var bugReportService: BugReportServiceMock! private var posthogMock: PHGPostHogMock! override func setUp() { AppSettings.resetAllSettings() appSettings = AppSettings() - bugReportService = BugReportServiceMock() - bugReportService.isRunning = false - ServiceLocator.shared.register(bugReportService: bugReportService) analyticsClient = AnalyticsClientMock() analyticsClient.isRunning = false ServiceLocator.shared.register(analytics: AnalyticsService(client: analyticsClient, - appSettings: appSettings, - bugReportService: ServiceLocator.shared.bugReportService)) + appSettings: appSettings)) posthogMock = PHGPostHogMock() posthogMock.configureMockBehavior() @@ -78,12 +73,11 @@ class AnalyticsTests: XCTestCase { } func testAnalyticsPromptNotDisplayed() { - // Given a fresh install of the app both Analytics and BugReportService should be disabled + // Given a fresh install of the app Analytics should be disabled XCTAssertEqual(appSettings.analyticsConsentState, .unknown) XCTAssertFalse(ServiceLocator.shared.analytics.isEnabled) - XCTAssertFalse(ServiceLocator.shared.analytics.isRunning) + XCTAssertFalse(ServiceLocator.shared.analytics.isRunningPublisher.value) XCTAssertFalse(analyticsClient.startAnalyticsConfigurationCalled) - XCTAssertFalse(bugReportService.startCalled) } func testAnalyticsOptOut() { @@ -93,12 +87,10 @@ class AnalyticsTests: XCTestCase { // Then analytics should be disabled XCTAssertEqual(appSettings.analyticsConsentState, .optedOut) XCTAssertFalse(ServiceLocator.shared.analytics.isEnabled) - XCTAssertFalse(ServiceLocator.shared.analytics.isRunning) + XCTAssertFalse(ServiceLocator.shared.analytics.isRunningPublisher.value) XCTAssertFalse(analyticsClient.isRunning) - XCTAssertFalse(bugReportService.isRunning) - // Analytics client and the bug report service should have been stopped + // Analytics client should have been stopped XCTAssertTrue(analyticsClient.stopCalled) - XCTAssertTrue(bugReportService.stopCalled) } func testAnalyticsOptIn() { @@ -108,9 +100,8 @@ class AnalyticsTests: XCTestCase { // The analytics should be enabled XCTAssertEqual(appSettings.analyticsConsentState, .optedIn) XCTAssertTrue(ServiceLocator.shared.analytics.isEnabled) - // Analytics client and the bug report service should have been started + // Analytics client should have been started XCTAssertTrue(analyticsClient.startAnalyticsConfigurationCalled) - XCTAssertTrue(bugReportService.startCalled) } func testAnalyticsStartIfNotEnabled() { @@ -120,17 +111,15 @@ class AnalyticsTests: XCTestCase { XCTAssertFalse(ServiceLocator.shared.analytics.isEnabled) ServiceLocator.shared.analytics.startIfEnabled() XCTAssertFalse(analyticsClient.startAnalyticsConfigurationCalled) - XCTAssertFalse(bugReportService.startCalled) } func testAnalyticsStartIfEnabled() { - // Given an existing install of the app where the user previously accpeted the tracking + // Given an existing install of the app where the user previously accepted the tracking appSettings.analyticsConsentState = .optedIn // Analytics should start XCTAssertTrue(ServiceLocator.shared.analytics.isEnabled) ServiceLocator.shared.analytics.startIfEnabled() XCTAssertTrue(analyticsClient.startAnalyticsConfigurationCalled) - XCTAssertTrue(bugReportService.startCalled) } func testAddingUserProperties() { @@ -232,7 +221,7 @@ class AnalyticsTests: XCTestCase { client.start(analyticsConfiguration: appSettings.analyticsConfiguration) client.updateSuperProperties( - AnalyticsEvent.SuperProperties(appPlatform: "A thing", + AnalyticsEvent.SuperProperties(appPlatform: .EXI, cryptoSDK: .Rust, cryptoSDKVersion: "000") ) @@ -246,7 +235,7 @@ class AnalyticsTests: XCTestCase { // All the super properties should have been added XCTAssertEqual(screenEvent?.properties?["cryptoSDK"] as? String, AnalyticsEvent.SuperProperties.CryptoSDK.Rust.rawValue) - XCTAssertEqual(screenEvent?.properties?["appPlatform"] as? String, "A thing") + XCTAssertEqual(screenEvent?.properties?["appPlatform"] as? String, "EXI") XCTAssertEqual(screenEvent?.properties?["cryptoSDKVersion"] as? String, "000") // It should be the same for any event @@ -267,13 +256,13 @@ class AnalyticsTests: XCTestCase { // All the super properties should have been added XCTAssertEqual(capturedEvent?.properties?["cryptoSDK"] as? String, AnalyticsEvent.SuperProperties.CryptoSDK.Rust.rawValue) - XCTAssertEqual(capturedEvent?.properties?["appPlatform"] as? String, "A thing") + XCTAssertEqual(capturedEvent?.properties?["appPlatform"] as? String, "EXI") XCTAssertEqual(capturedEvent?.properties?["cryptoSDKVersion"] as? String, "000") // Updating should keep the previously set properties client.updateSuperProperties( - AnalyticsEvent.SuperProperties(appPlatform: nil, - cryptoSDK: nil, + AnalyticsEvent.SuperProperties(appPlatform: .EXI, + cryptoSDK: .Rust, cryptoSDKVersion: "001") ) @@ -282,7 +271,7 @@ class AnalyticsTests: XCTestCase { // All the super properties should have been added, with the one udpated XCTAssertEqual(capturedEvent2?.properties?["cryptoSDK"] as? String, AnalyticsEvent.SuperProperties.CryptoSDK.Rust.rawValue) - XCTAssertEqual(capturedEvent2?.properties?["appPlatform"] as? String, "A thing") + XCTAssertEqual(capturedEvent2?.properties?["appPlatform"] as? String, "EXI") XCTAssertEqual(capturedEvent2?.properties?["cryptoSDKVersion"] as? String, "001") } diff --git a/UnitTests/Sources/ComposerToolbarViewModelTests.swift b/UnitTests/Sources/ComposerToolbarViewModelTests.swift index 6258f772ee..d35cf60752 100644 --- a/UnitTests/Sources/ComposerToolbarViewModelTests.swift +++ b/UnitTests/Sources/ComposerToolbarViewModelTests.swift @@ -37,7 +37,8 @@ class ComposerToolbarViewModelTests: XCTestCase { completionSuggestionService: completionSuggestionServiceMock, mediaProvider: MockMediaProvider(), appSettings: appSettings, - mentionDisplayHelper: ComposerMentionDisplayHelper.mock) + mentionDisplayHelper: ComposerMentionDisplayHelper.mock, + analyticsService: ServiceLocator.shared.analytics) viewModel.context.composerFormattingEnabled = true } @@ -113,7 +114,8 @@ class ComposerToolbarViewModelTests: XCTestCase { completionSuggestionService: mockCompletionSuggestionService, mediaProvider: MockMediaProvider(), appSettings: ServiceLocator.shared.settings, - mentionDisplayHelper: ComposerMentionDisplayHelper.mock) + mentionDisplayHelper: ComposerMentionDisplayHelper.mock, + analyticsService: ServiceLocator.shared.analytics) XCTAssertEqual(viewModel.state.suggestions, suggestions) }