From bef1eda533f8131404ffb394837b8af5dc003842 Mon Sep 17 00:00:00 2001 From: Doug <6060466+pixlwave@users.noreply.github.com> Date: Mon, 18 Nov 2024 11:38:20 +0000 Subject: [PATCH] Fix a bug where the security banner has the wrong state when out of sync. (#3511) --- .../Screens/HomeScreen/HomeScreenModels.swift | 26 ++++- .../HomeScreen/HomeScreenViewModel.swift | 15 ++- .../HomeScreen/View/HomeScreenContent.swift | 6 +- ...eScreenRecoveryKeyConfirmationBanner.swift | 42 +++++-- .../Sources/HomeScreenViewModelTests.swift | 110 ++++++++++++++++-- 5 files changed, 168 insertions(+), 31 deletions(-) diff --git a/ElementX/Sources/Screens/HomeScreen/HomeScreenModels.swift b/ElementX/Sources/Screens/HomeScreen/HomeScreenModels.swift index 2e58c17ae..7671b3774 100644 --- a/ElementX/Sources/Screens/HomeScreen/HomeScreenModels.swift +++ b/ElementX/Sources/Screens/HomeScreen/HomeScreenModels.swift @@ -66,10 +66,28 @@ enum HomeScreenRoomListMode: CustomStringConvertible { } } -enum HomeScreenBannerMode { +enum HomeScreenSecurityBannerMode: Equatable { case none case dismissed - case show + case show(HomeScreenRecoveryKeyConfirmationBanner.State) + + var isDismissed: Bool { + switch self { + case .dismissed: true + default: false + } + } + + var isShown: Bool { + switch self { + case .show: true + default: false + } + } +} + +enum HomeScreenMigrationBannerMode { + case none, show, dismissed } struct HomeScreenViewState: BindableState { @@ -77,8 +95,8 @@ struct HomeScreenViewState: BindableState { var userDisplayName: String? var userAvatarURL: URL? - var securityBannerMode = HomeScreenBannerMode.none - var slidingSyncMigrationBannerMode = HomeScreenBannerMode.none + var securityBannerMode = HomeScreenSecurityBannerMode.none + var slidingSyncMigrationBannerMode = HomeScreenMigrationBannerMode.none var requiresExtraAccountSetup = false diff --git a/ElementX/Sources/Screens/HomeScreen/HomeScreenViewModel.swift b/ElementX/Sources/Screens/HomeScreen/HomeScreenViewModel.swift index c1b21cf4f..9bd00e2b2 100644 --- a/ElementX/Sources/Screens/HomeScreen/HomeScreenViewModel.swift +++ b/ElementX/Sources/Screens/HomeScreen/HomeScreenViewModel.swift @@ -55,16 +55,15 @@ class HomeScreenViewModel: HomeScreenViewModelType, HomeScreenViewModelProtocol .sink { [weak self] securityState in guard let self else { return } - switch (securityState.verificationState, securityState.recoveryState) { - case (.verified, .disabled): + switch securityState.recoveryState { + case .disabled: state.requiresExtraAccountSetup = true - state.securityBannerMode = .show - case (.verified, .incomplete): - state.requiresExtraAccountSetup = true - - if state.securityBannerMode != .dismissed { - state.securityBannerMode = .show + if !state.securityBannerMode.isDismissed { + state.securityBannerMode = .show(.setUpRecovery) } + case .incomplete: + state.requiresExtraAccountSetup = true + state.securityBannerMode = .show(.recoveryOutOfSync) default: state.securityBannerMode = .none state.requiresExtraAccountSetup = false diff --git a/ElementX/Sources/Screens/HomeScreen/View/HomeScreenContent.swift b/ElementX/Sources/Screens/HomeScreen/View/HomeScreenContent.swift index 68c7408b9..969d1882a 100644 --- a/ElementX/Sources/Screens/HomeScreen/View/HomeScreenContent.swift +++ b/ElementX/Sources/Screens/HomeScreen/View/HomeScreenContent.swift @@ -120,7 +120,7 @@ struct HomeScreenContent: View { private var topSection: some View { // An empty VStack causes glitches within the room list if context.viewState.shouldShowFilters || - context.viewState.securityBannerMode == .show || + context.viewState.securityBannerMode.isShown || context.viewState.slidingSyncMigrationBannerMode == .show { VStack(spacing: 0) { if context.viewState.shouldShowFilters { @@ -129,8 +129,8 @@ struct HomeScreenContent: View { if context.viewState.slidingSyncMigrationBannerMode == .show { HomeScreenSlidingSyncMigrationBanner(context: context) - } else if context.viewState.securityBannerMode == .show { - HomeScreenRecoveryKeyConfirmationBanner(requiresExtraAccountSetup: context.viewState.requiresExtraAccountSetup, context: context) + } else if case let .show(state) = context.viewState.securityBannerMode { + HomeScreenRecoveryKeyConfirmationBanner(state: state, context: context) } } .background(Color.compound.bgCanvasDefault) diff --git a/ElementX/Sources/Screens/HomeScreen/View/HomeScreenRecoveryKeyConfirmationBanner.swift b/ElementX/Sources/Screens/HomeScreen/View/HomeScreenRecoveryKeyConfirmationBanner.swift index 1adf1fac7..344276289 100644 --- a/ElementX/Sources/Screens/HomeScreen/View/HomeScreenRecoveryKeyConfirmationBanner.swift +++ b/ElementX/Sources/Screens/HomeScreen/View/HomeScreenRecoveryKeyConfirmationBanner.swift @@ -10,13 +10,37 @@ import Compound import SwiftUI struct HomeScreenRecoveryKeyConfirmationBanner: View { - let requiresExtraAccountSetup: Bool + enum State { case setUpRecovery, recoveryOutOfSync } + let state: State var context: HomeScreenViewModel.Context - var title: String { requiresExtraAccountSetup ? L10n.bannerSetUpRecoveryTitle : L10n.confirmRecoveryKeyBannerTitle } - var message: String { requiresExtraAccountSetup ? L10n.bannerSetUpRecoveryContent : L10n.confirmRecoveryKeyBannerMessage } - var actionTitle: String { requiresExtraAccountSetup ? L10n.bannerSetUpRecoverySubmit : L10n.confirmRecoveryKeyBannerPrimaryButtonTitle } - var primaryAction: HomeScreenViewAction { requiresExtraAccountSetup ? .setupRecovery : .confirmRecoveryKey } + var title: String { + switch state { + case .setUpRecovery: L10n.bannerSetUpRecoveryTitle + case .recoveryOutOfSync: L10n.confirmRecoveryKeyBannerTitle + } + } + + var message: String { + switch state { + case .setUpRecovery: L10n.bannerSetUpRecoveryContent + case .recoveryOutOfSync: L10n.confirmRecoveryKeyBannerMessage + } + } + + var actionTitle: String { + switch state { + case .setUpRecovery: L10n.bannerSetUpRecoverySubmit + case .recoveryOutOfSync: L10n.confirmRecoveryKeyBannerPrimaryButtonTitle + } + } + + var primaryAction: HomeScreenViewAction { + switch state { + case .setUpRecovery: .setupRecovery + case .recoveryOutOfSync: .confirmRecoveryKey + } + } var body: some View { VStack(spacing: 16) { @@ -37,7 +61,7 @@ struct HomeScreenRecoveryKeyConfirmationBanner: View { .foregroundColor(.compound.textPrimary) .frame(maxWidth: .infinity, alignment: .leading) - if requiresExtraAccountSetup { + if state == .setUpRecovery { Button { context.send(viewAction: .skipRecoveryKeyConfirmation) } label: { @@ -63,7 +87,7 @@ struct HomeScreenRecoveryKeyConfirmationBanner: View { .buttonStyle(.compound(.primary, size: .medium)) .accessibilityIdentifier(A11yIdentifiers.homeScreen.recoveryKeyConfirmationBannerContinue) - if !requiresExtraAccountSetup { + if state == .recoveryOutOfSync { Button { context.send(viewAction: .resetEncryption) } label: { @@ -81,10 +105,10 @@ struct HomeScreenRecoveryKeyConfirmationBanner_Previews: PreviewProvider, Testab static let viewModel = buildViewModel() static var previews: some View { - HomeScreenRecoveryKeyConfirmationBanner(requiresExtraAccountSetup: true, + HomeScreenRecoveryKeyConfirmationBanner(state: .setUpRecovery, context: viewModel.context) .previewDisplayName("Set up recovery") - HomeScreenRecoveryKeyConfirmationBanner(requiresExtraAccountSetup: false, + HomeScreenRecoveryKeyConfirmationBanner(state: .recoveryOutOfSync, context: viewModel.context) .previewDisplayName("Out of sync") } diff --git a/UnitTests/Sources/HomeScreenViewModelTests.swift b/UnitTests/Sources/HomeScreenViewModelTests.swift index e48584479..1361b8303 100644 --- a/UnitTests/Sources/HomeScreenViewModelTests.swift +++ b/UnitTests/Sources/HomeScreenViewModelTests.swift @@ -20,13 +20,6 @@ class HomeScreenViewModelTests: XCTestCase { override func setUpWithError() throws { cancellables.removeAll() - roomSummaryProvider = RoomSummaryProviderMock(.init(state: .loaded(.mockRooms))) - clientProxy = ClientProxyMock(.init(userID: "@mock:client.com", roomSummaryProvider: roomSummaryProvider)) - viewModel = HomeScreenViewModel(userSession: UserSessionMock(.init(clientProxy: clientProxy)), - analyticsService: ServiceLocator.shared.analytics, - appSettings: ServiceLocator.shared.settings, - selectedRoomPublisher: CurrentValueSubject(nil).asCurrentValuePublisher(), - userIndicatorController: ServiceLocator.shared.userIndicatorController) } override func tearDown() { @@ -34,6 +27,8 @@ class HomeScreenViewModelTests: XCTestCase { } func testSelectRoom() async throws { + setupViewModel() + let mockRoomId = "mock_room_id" var correctResult = false var selectedRoomId = "" @@ -57,6 +52,8 @@ class HomeScreenViewModelTests: XCTestCase { } func testTapUserAvatar() async throws { + setupViewModel() + var correctResult = false viewModel.actions @@ -76,6 +73,8 @@ class HomeScreenViewModelTests: XCTestCase { } func testLeaveRoomAlert() async throws { + setupViewModel() + let mockRoomId = "1" clientProxy.roomForIdentifierClosure = { _ in .joined(JoinedRoomProxyMock(.init(id: mockRoomId, name: "Some room"))) } @@ -92,6 +91,8 @@ class HomeScreenViewModelTests: XCTestCase { } func testLeaveRoomError() async throws { + setupViewModel() + let mockRoomId = "1" let room = JoinedRoomProxyMock(.init(id: mockRoomId, name: "Some room")) room.leaveRoomClosure = { .failure(.sdkError(ClientProxyMockError.generic)) } @@ -110,6 +111,8 @@ class HomeScreenViewModelTests: XCTestCase { } func testLeaveRoomSuccess() async throws { + setupViewModel() + let mockRoomId = "1" var correctResult = false let expectation = expectation(description: #function) @@ -136,6 +139,8 @@ class HomeScreenViewModelTests: XCTestCase { } func testShowRoomDetails() async throws { + setupViewModel() + let mockRoomId = "1" var correctResult = false viewModel.actions @@ -155,6 +160,8 @@ class HomeScreenViewModelTests: XCTestCase { } func testFilters() async throws { + setupViewModel() + context.filtersState.activateFilter(.people) try await Task.sleep(for: .milliseconds(100)) XCTAssertEqual(roomSummaryProvider.roomListPublisher.value.count, 2) @@ -162,6 +169,8 @@ class HomeScreenViewModelTests: XCTestCase { } func testSearch() async throws { + setupViewModel() + context.isSearchFieldFocused = true context.searchQuery = "lude to Found" try await Task.sleep(for: .milliseconds(100)) @@ -170,6 +179,8 @@ class HomeScreenViewModelTests: XCTestCase { } func testFiltersEmptyState() async throws { + setupViewModel() + context.filtersState.activateFilter(.people) context.filtersState.activateFilter(.favourites) try await Task.sleep(for: .milliseconds(100)) @@ -177,4 +188,89 @@ class HomeScreenViewModelTests: XCTestCase { context.isSearchFieldFocused = true XCTAssertFalse(context.viewState.shouldShowEmptyFilterState) } + + func testSetUpRecoveryBannerState() async throws { + // Given a view model without a visible security banner. + let securityStateStateSubject = CurrentValueSubject(.init(verificationState: .verified, recoveryState: .unknown)) + setupViewModel(securityStatePublisher: securityStateStateSubject.asCurrentValuePublisher()) + XCTAssertEqual(context.viewState.securityBannerMode, .none) + + // When the recovery state comes through as disabled. + var deferred = deferFulfillment(context.$viewState) { $0.requiresExtraAccountSetup == true } + securityStateStateSubject.send(.init(verificationState: .verified, recoveryState: .disabled)) + try await deferred.fulfill() + + // Then the banner should be shown to set up recovery. + XCTAssertEqual(context.viewState.securityBannerMode, .show(.setUpRecovery)) + + // When the recovery is enabled. + deferred = deferFulfillment(context.$viewState) { $0.requiresExtraAccountSetup == false } + securityStateStateSubject.send(.init(verificationState: .verified, recoveryState: .enabled)) + try await deferred.fulfill() + + // Then the banner should no longer be shown. + XCTAssertEqual(context.viewState.securityBannerMode, .none) + } + + func testDismissSetUpRecoveryBannerState() async throws { + // Given a view model with the setup recovery banner shown. + let securityStateStateSubject = CurrentValueSubject(.init(verificationState: .verified, recoveryState: .unknown)) + setupViewModel(securityStatePublisher: securityStateStateSubject.asCurrentValuePublisher()) + var deferred = deferFulfillment(context.$viewState) { $0.securityBannerMode == .show(.setUpRecovery) } + securityStateStateSubject.send(.init(verificationState: .verified, recoveryState: .disabled)) + try await deferred.fulfill() + + // When the banner is dismissed. + deferred = deferFulfillment(context.$viewState) { $0.securityBannerMode == .dismissed } + context.send(viewAction: .skipRecoveryKeyConfirmation) + + // Then the banner should no longer be shown. + try await deferred.fulfill() + + // And when the recovery state comes through a second time the banner should still not be shown. + let failure = deferFailure(context.$viewState, timeout: 1) { $0.securityBannerMode != .dismissed } + securityStateStateSubject.send(.init(verificationState: .verified, recoveryState: .disabled)) + try await failure.fulfill() + } + + func testOutOfSyncRecoveryBannerState() async throws { + // Given a view model without a visible security banner. + let securityStateStateSubject = CurrentValueSubject(.init(verificationState: .verified, recoveryState: .unknown)) + setupViewModel(securityStatePublisher: securityStateStateSubject.asCurrentValuePublisher()) + XCTAssertEqual(context.viewState.securityBannerMode, .none) + + // When the recovery state comes through as incomplete. + var deferred = deferFulfillment(context.$viewState) { $0.requiresExtraAccountSetup == true } + securityStateStateSubject.send(.init(verificationState: .verified, recoveryState: .incomplete)) + try await deferred.fulfill() + + // Then the banner should be shown for out of sync recovery. + XCTAssertEqual(context.viewState.securityBannerMode, .show(.recoveryOutOfSync)) + + // When the recovery is enabled. + deferred = deferFulfillment(context.$viewState) { $0.requiresExtraAccountSetup == false } + securityStateStateSubject.send(.init(verificationState: .verified, recoveryState: .enabled)) + try await deferred.fulfill() + + // Then the banner should no longer be shown. + XCTAssertEqual(context.viewState.securityBannerMode, .none) + } + + // MARK: - Helpers + + private func setupViewModel(securityStatePublisher: CurrentValuePublisher? = nil) { + roomSummaryProvider = RoomSummaryProviderMock(.init(state: .loaded(.mockRooms))) + clientProxy = ClientProxyMock(.init(userID: "@mock:client.com", + roomSummaryProvider: roomSummaryProvider)) + let userSession = UserSessionMock(.init(clientProxy: clientProxy)) + if let securityStatePublisher { + userSession.sessionSecurityStatePublisher = securityStatePublisher + } + + viewModel = HomeScreenViewModel(userSession: userSession, + analyticsService: ServiceLocator.shared.analytics, + appSettings: ServiceLocator.shared.settings, + selectedRoomPublisher: CurrentValueSubject(nil).asCurrentValuePublisher(), + userIndicatorController: ServiceLocator.shared.userIndicatorController) + } }