diff --git a/Riot/Modules/Analytics/Analytics.swift b/Riot/Modules/Analytics/Analytics.swift index 6f6a67597..48716f78e 100644 --- a/Riot/Modules/Analytics/Analytics.swift +++ b/Riot/Modules/Analytics/Analytics.swift @@ -108,7 +108,6 @@ import AnalyticsEvents !RiotSettings.shared.isIdentifiedForAnalytics else { return } - let userProperties = makeUserProperties(for: session) let service = AnalyticsService(session: session) self.service = service @@ -117,7 +116,7 @@ import AnalyticsEvents switch result { case .success(let settings): - self.identify(with: settings, and: userProperties) + self.identify(with: settings) self.service = nil case .failure: MXLog.error("[Analytics] Failed to use analytics settings. Will continue to run without analytics ID.") @@ -150,30 +149,17 @@ import AnalyticsEvents /// Identify (pseudonymously) any future events with the ID from the analytics account data settings. /// - Parameter settings: The settings to use for identification. The ID must be set *before* calling this method. - /// - Parameter userProperties: Any user properties that should be set for creating cohorts etc. - private func identify(with settings: AnalyticsSettings, and userProperties: AnalyticsEvent.UserProperties) { + private func identify(with settings: AnalyticsSettings) { guard let id = settings.id else { MXLog.error("[Analytics] identify(with:) called before an ID has been generated.") return } - client.identify(id: id, userProperties: userProperties) + client.identify(id: id) MXLog.debug("[Analytics] Identified.") RiotSettings.shared.isIdentifiedForAnalytics = true } - /// Returns the user properties for use when identifying a session. - /// - Parameter session: The session to gather any user properties from. - /// - Returns: The properties to be set. - private func makeUserProperties(for session: MXSession) -> AnalyticsEvent.UserProperties { - var useCaseSelection: AnalyticsEvent.UserProperties.FtueUseCaseSelection? - if let userId = session.credentials.userId, let userSession = UserSessionsService.shared.userSession(withUserId: userId) { - useCaseSelection = userSession.userProperties.useCase?.analyticsName - } - - return AnalyticsEvent.UserProperties(ftueUseCaseSelection: useCaseSelection, numSpaces: nil) - } - /// Capture an event in the `client`. /// - Parameter event: The event to capture. private func capture(event: AnalyticsEventProtocol) { diff --git a/Riot/Modules/Analytics/AnalyticsClientProtocol.swift b/Riot/Modules/Analytics/AnalyticsClientProtocol.swift index 5ae391f61..e16b962e5 100644 --- a/Riot/Modules/Analytics/AnalyticsClientProtocol.swift +++ b/Riot/Modules/Analytics/AnalyticsClientProtocol.swift @@ -26,10 +26,7 @@ protocol AnalyticsClientProtocol { /// Associate the client with an ID. This is persisted until `reset` is called. /// - Parameter id: The ID to associate with the user. - /// - Parameter userProperties: Any user properties that should be included. - /// - /// Only non-nil user properties will be updated when calling this method. - func identify(id: String, userProperties: AnalyticsEvent.UserProperties) + func identify(id: String) /// Reset all stored properties and any event queues on the client. Note that /// the client will remain active, but in a fresh unidentified state. @@ -52,6 +49,8 @@ protocol AnalyticsClientProtocol { /// Updates any user properties to help with creating cohorts. /// - Parameter userProperties: The user properties to be updated. /// - /// Only non-nil properties will be updated when calling this method. + /// Only non-nil properties will be updated when calling this method. There might + /// be a delay when updating user properties as these are cached to be included + /// as part of the next event that gets captured. func updateUserProperties(_ userProperties: AnalyticsEvent.UserProperties) } diff --git a/Riot/Modules/Analytics/PostHogAnalyticsClient.swift b/Riot/Modules/Analytics/PostHogAnalyticsClient.swift index 69724a5d0..434ce978e 100644 --- a/Riot/Modules/Analytics/PostHogAnalyticsClient.swift +++ b/Riot/Modules/Analytics/PostHogAnalyticsClient.swift @@ -22,6 +22,9 @@ class PostHogAnalyticsClient: AnalyticsClientProtocol { /// The PHGPostHog object used to report events. private var postHog: PHGPostHog? + /// Any user properties to be included with the next captured event. + private(set) var pendingUserProperties: AnalyticsEvent.UserProperties? + var isRunning: Bool { postHog?.enabled ?? false } func start() { @@ -35,13 +38,19 @@ class PostHogAnalyticsClient: AnalyticsClientProtocol { postHog?.enable() } - func identify(id: String, userProperties: AnalyticsEvent.UserProperties) { - // As user properties overwrite old ones, compactMap the dictionary to avoid resetting any missing properties - postHog?.identify(id, properties: userProperties.properties.compactMapValues { $0 }) + func identify(id: String) { + if let userProperties = pendingUserProperties { + // As user properties overwrite old ones, compactMap the dictionary to avoid resetting any missing properties + postHog?.identify(id, properties: userProperties.properties.compactMapValues { $0 }) + pendingUserProperties = nil + } else { + postHog?.identify(id) + } } func reset() { postHog?.reset() + pendingUserProperties = nil } func stop() { @@ -56,15 +65,38 @@ class PostHogAnalyticsClient: AnalyticsClientProtocol { } func capture(_ event: AnalyticsEventProtocol) { - postHog?.capture(event.eventName, properties: event.properties) + postHog?.capture(event.eventName, properties: attachUserProperties(to: event.properties)) } func screen(_ event: AnalyticsScreenProtocol) { - postHog?.screen(event.screenName.rawValue, properties: event.properties) + postHog?.screen(event.screenName.rawValue, properties: attachUserProperties(to: event.properties)) } func updateUserProperties(_ userProperties: AnalyticsEvent.UserProperties) { + guard let pendingUserProperties = pendingUserProperties else { + pendingUserProperties = userProperties + return + } + + // Merge the updated user properties with the existing ones + self.pendingUserProperties = AnalyticsEvent.UserProperties(ftueUseCaseSelection: userProperties.ftueUseCaseSelection ?? pendingUserProperties.ftueUseCaseSelection, + numSpaces: userProperties.numSpaces ?? pendingUserProperties.numSpaces) + } + + // MARK: - Private + + /// Given a dictionary containing properties from an event, this method will return those properties + /// with any pending user properties included under the `$set` key. + /// - Parameter properties: A dictionary of properties from an event. + /// - Returns: The `properties` dictionary with any user properties included. + private func attachUserProperties(to properties: [String: Any]) -> [String: Any] { + guard isRunning, let userProperties = pendingUserProperties else { return properties } + + var properties = properties + // As user properties overwrite old ones via $set, compactMap the dictionary to avoid resetting any missing properties - postHog?.capture("$identify", properties: ["$set": userProperties.properties.compactMapValues { $0 }]) + properties["$set"] = userProperties.properties.compactMapValues { $0 } + pendingUserProperties = nil + return properties } } diff --git a/Riot/Modules/Onboarding/OnboardingCoordinator.swift b/Riot/Modules/Onboarding/OnboardingCoordinator.swift index 7be3f4370..d123d7abc 100644 --- a/Riot/Modules/Onboarding/OnboardingCoordinator.swift +++ b/Riot/Modules/Onboarding/OnboardingCoordinator.swift @@ -229,11 +229,8 @@ final class OnboardingCoordinator: NSObject, OnboardingCoordinatorProtocol { // Store the value in the user's session userSession.userProperties.useCase = useCase - // Capture the use case if analytics are running. - // Otherwise it will be included when identifying if opted in. - if Analytics.shared.isRunning { - Analytics.shared.updateUserProperties(ftueUseCase: useCase) - } + // Update the analytics user properties with the use case + Analytics.shared.updateUserProperties(ftueUseCase: useCase) } } } diff --git a/RiotTests/AnalyticsTests.swift b/RiotTests/AnalyticsTests.swift index 5e15bf523..148a23bc8 100644 --- a/RiotTests/AnalyticsTests.swift +++ b/RiotTests/AnalyticsTests.swift @@ -16,6 +16,7 @@ import XCTest @testable import Riot +import AnalyticsEvents class AnalyticsTests: XCTestCase { func testAnalyticsPromptNewUser() { @@ -70,4 +71,68 @@ class AnalyticsTests: XCTestCase { // Then no prompt should be shown. XCTAssertFalse(showPrompt, "A prompt should not be shown any more.") } + + func testAddingUserProperties() { + // Given a client with no user properties set + let client = PostHogAnalyticsClient() + XCTAssertNil(client.pendingUserProperties, "No user properties should have been set yet.") + + // When updating the user properties + client.updateUserProperties(AnalyticsEvent.UserProperties(ftueUseCaseSelection: .PersonalMessaging, numSpaces: 5)) + + // Then the properties should be cached + XCTAssertNotNil(client.pendingUserProperties, "The user properties should be cached.") + XCTAssertEqual(client.pendingUserProperties?.ftueUseCaseSelection, .PersonalMessaging, "The use case selection should match.") + XCTAssertEqual(client.pendingUserProperties?.numSpaces, 5, "The number of spaces should match.") + } + + func testMergingUserProperties() { + // Given a client with a cached use case user properties + let client = PostHogAnalyticsClient() + client.updateUserProperties(AnalyticsEvent.UserProperties(ftueUseCaseSelection: .PersonalMessaging, numSpaces: nil)) + + XCTAssertNotNil(client.pendingUserProperties, "The user properties should be cached.") + XCTAssertEqual(client.pendingUserProperties?.ftueUseCaseSelection, .PersonalMessaging, "The use case selection should match.") + XCTAssertNil(client.pendingUserProperties?.numSpaces, "The number of spaces should not be set.") + + // When updating the number of spaced + client.updateUserProperties(AnalyticsEvent.UserProperties(ftueUseCaseSelection: nil, numSpaces: 5)) + + // Then the new properties should be updated and the existing properties should remain unchanged + XCTAssertNotNil(client.pendingUserProperties, "The user properties should be cached.") + XCTAssertEqual(client.pendingUserProperties?.ftueUseCaseSelection, .PersonalMessaging, "The use case selection shouldn't have changed.") + XCTAssertEqual(client.pendingUserProperties?.numSpaces, 5, "The number of spaces should have been updated.") + } + + func testSendingUserProperties() { + // Given a client with user properties set + let client = PostHogAnalyticsClient() + client.updateUserProperties(AnalyticsEvent.UserProperties(ftueUseCaseSelection: .PersonalMessaging, numSpaces: nil)) + client.start() + + XCTAssertNotNil(client.pendingUserProperties, "The user properties should be cached.") + XCTAssertEqual(client.pendingUserProperties?.ftueUseCaseSelection, .PersonalMessaging, "The use case selection should match.") + + // When sending an event (tests run under Debug configuration so this is sent to the development instance) + client.screen(AnalyticsEvent.Screen(durationMs: nil, screenName: .Home)) + + // Then the properties should be cleared + XCTAssertNil(client.pendingUserProperties, "The user properties should be cleared.") + } + + func testSendingUserPropertiesWithIdentify() { + // Given a client with user properties set + let client = PostHogAnalyticsClient() + client.updateUserProperties(AnalyticsEvent.UserProperties(ftueUseCaseSelection: .PersonalMessaging, numSpaces: nil)) + client.start() + + XCTAssertNotNil(client.pendingUserProperties, "The user properties should be cached.") + XCTAssertEqual(client.pendingUserProperties?.ftueUseCaseSelection, .PersonalMessaging, "The use case selection should match.") + + // When calling identify (tests run under Debug configuration so this is sent to the development instance) + client.identify(id: UUID().uuidString) + + // Then the properties should be cleared + XCTAssertNil(client.pendingUserProperties, "The user properties should be cleared.") + } }