Skip to content

Commit

Permalink
Decouple the bug report service from the analytics one
Browse files Browse the repository at this point in the history
  • Loading branch information
stefanceriu committed May 30, 2024
1 parent c3571f4 commit a4c9135
Show file tree
Hide file tree
Showing 12 changed files with 58 additions and 94 deletions.
16 changes: 13 additions & 3 deletions ElementX/Sources/Application/AppCoordinator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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.
Expand Down
35 changes: 0 additions & 35 deletions ElementX/Sources/Mocks/Generated/GeneratedMocks.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
24 changes: 14 additions & 10 deletions ElementX/Sources/Services/Analytics/AnalyticsService.swift
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
//

import AnalyticsEvents
import Combine
import PostHog

/// A class responsible for managing a variety of analytics clients
Expand All @@ -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<Bool, Never> = .init(false)
var isRunningPublisher: CurrentValuePublisher<Bool, Never> {
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 {
Expand All @@ -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.")
}

Expand All @@ -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.")
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}

Expand Down
7 changes: 1 addition & 6 deletions ElementX/Sources/Services/BugReport/BugReportService.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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<Double, Never>) async -> Result<SubmitBugReportResponse, BugReportServiceError> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,6 @@ protocol BugReportServiceProtocol {

func stop()

func reset()

func submitBugReport(_ bugReport: BugReport,
progressListener: CurrentValueSubject<Double, Never>) async -> Result<SubmitBugReportResponse, BugReportServiceError>
}
8 changes: 5 additions & 3 deletions ElementX/Sources/UITests/UITestsAppCoordinator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
8 changes: 5 additions & 3 deletions ElementX/Sources/UnitTests/UnitTestsAppCoordinator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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() { }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
37 changes: 13 additions & 24 deletions UnitTests/Sources/AnalyticsTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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() {
Expand All @@ -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() {
Expand All @@ -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() {
Expand All @@ -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() {
Expand Down Expand Up @@ -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")
)
Expand All @@ -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
Expand All @@ -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")
)

Expand All @@ -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")
}

Expand Down
6 changes: 4 additions & 2 deletions UnitTests/Sources/ComposerToolbarViewModelTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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)
}
Expand Down

0 comments on commit a4c9135

Please sign in to comment.