From 0fd8df52abf5fc37baef5bb882b8f90b14956098 Mon Sep 17 00:00:00 2001 From: Stefan Ceriu Date: Mon, 20 Jan 2025 15:43:47 +0200 Subject: [PATCH] Improve how alias settings are handled, add unit tests (#3686) --- .../Sources/Mocks/JoinedRoomProxyMock.swift | 3 +- .../EditRoomAddressScreenModels.swift | 4 +- .../EditRoomAddressScreenViewModel.swift | 38 +++++++---- .../SecurityAndPrivacyScreenViewModel.swift | 12 +++- .../Sources/Services/Room/RoomInfoProxy.swift | 31 +++++++++ ...tRoomAddressScreen-iPad-en-GB.No-alias.png | 4 +- ...RoomAddressScreen-iPad-pseudo.No-alias.png | 4 +- ...AddressScreen-iPhone-16-en-GB.No-alias.png | 4 +- ...ddressScreen-iPhone-16-pseudo.No-alias.png | 4 +- .../EditRoomAddressScreenViewModelTests.swift | 64 ++++++++++++++++++- 10 files changed, 144 insertions(+), 24 deletions(-) diff --git a/ElementX/Sources/Mocks/JoinedRoomProxyMock.swift b/ElementX/Sources/Mocks/JoinedRoomProxyMock.swift index c709769f3..c14d414e0 100644 --- a/ElementX/Sources/Mocks/JoinedRoomProxyMock.swift +++ b/ElementX/Sources/Mocks/JoinedRoomProxyMock.swift @@ -25,6 +25,7 @@ struct JoinedRoomProxyMockConfiguration { var isEncrypted = true var hasOngoingCall = true var canonicalAlias: String? + var alternativeAliases: [String] = [] var pinnedEventIDs: Set = [] var timelineStartReached = false @@ -146,7 +147,7 @@ extension RoomInfo { isTombstoned: false, isFavourite: false, canonicalAlias: configuration.canonicalAlias, - alternativeAliases: [], + alternativeAliases: configuration.alternativeAliases, membership: .joined, inviter: configuration.inviter.map { RoomMember(userId: $0.userID, displayName: $0.displayName, diff --git a/ElementX/Sources/Screens/EditRoomAddressScreen/EditRoomAddressScreenModels.swift b/ElementX/Sources/Screens/EditRoomAddressScreen/EditRoomAddressScreenModels.swift index 1b969e476..db442d698 100644 --- a/ElementX/Sources/Screens/EditRoomAddressScreen/EditRoomAddressScreenModels.swift +++ b/ElementX/Sources/Screens/EditRoomAddressScreen/EditRoomAddressScreenModels.swift @@ -22,11 +22,11 @@ struct EditRoomAddressScreenViewState: BindableState { !bindings.desiredAliasLocalPart.isEmpty } - var bindings: EditRoomAddressScreenViewStateBindings + var bindings = EditRoomAddressScreenViewStateBindings() } struct EditRoomAddressScreenViewStateBindings { - var desiredAliasLocalPart: String + var desiredAliasLocalPart = "" } enum EditRoomAddressScreenViewAction { diff --git a/ElementX/Sources/Screens/EditRoomAddressScreen/EditRoomAddressScreenViewModel.swift b/ElementX/Sources/Screens/EditRoomAddressScreen/EditRoomAddressScreenViewModel.swift index 7ec9066db..ec430775a 100644 --- a/ElementX/Sources/Screens/EditRoomAddressScreen/EditRoomAddressScreenViewModel.swift +++ b/ElementX/Sources/Screens/EditRoomAddressScreen/EditRoomAddressScreenViewModel.swift @@ -31,23 +31,33 @@ class EditRoomAddressScreenViewModel: EditRoomAddressScreenViewModelType, EditRo self.clientProxy = clientProxy self.userIndicatorController = userIndicatorController - var aliasLocalPart = "" - if let canonicalAlias = roomProxy.infoPublisher.value.canonicalAlias { - aliasLocalPart = canonicalAlias.dropFirst().split(separator: ":").first.flatMap(String.init) ?? "" - } else if let displayName = roomProxy.infoPublisher.value.displayName { - aliasLocalPart = roomAliasNameFromRoomDisplayName(roomName: displayName) - } - if let initialViewState { super.init(initialViewState: initialViewState) } else { - super.init(initialViewState: EditRoomAddressScreenViewState(serverName: clientProxy.userIDServerName ?? "", - currentAliasLocalPart: aliasLocalPart, - bindings: .init(desiredAliasLocalPart: aliasLocalPart))) + super.init(initialViewState: EditRoomAddressScreenViewState(serverName: clientProxy.userIDServerName ?? "")) + + state.currentAliasLocalPart = localPartForMatchingAlias(computeFromDisplayName: false) + state.bindings.desiredAliasLocalPart = localPartForMatchingAlias(computeFromDisplayName: true) ?? "" } + setupSubscriptions() } + /// Give priority to aliases from the current user's homeserver as remote ones + /// cannot be edited. If none match then don't fallback and show an empty alias + /// instead so that the user can add one sepecific to this homeserver. + private func localPartForMatchingAlias(computeFromDisplayName: Bool) -> String? { + if let matchingAlias = roomProxy.infoPublisher.value.firstAliasMatching(serverName: clientProxy.userIDServerName, useFallback: false) { + return matchingAlias.aliasLocalPart + } + + guard computeFromDisplayName, let displayName = roomProxy.infoPublisher.value.displayName else { + return nil + } + + return roomAliasNameFromRoomDisplayName(roomName: displayName) + } + // MARK: - Public override func process(viewAction: EditRoomAddressScreenViewAction) { @@ -136,7 +146,7 @@ class EditRoomAddressScreenViewModel: EditRoomAddressScreenViewModelType, EditRo return } - let oldAlias = roomProxy.infoPublisher.value.canonicalAlias + let oldAlias = roomProxy.infoPublisher.value.firstAliasMatching(serverName: clientProxy.userIDServerName, useFallback: false) // First publish the new alias if case .failure = await roomProxy.publishRoomAliasInRoomDirectory(canonicalAlias) { @@ -172,3 +182,9 @@ class EditRoomAddressScreenViewModel: EditRoomAddressScreenViewModelType, EditRo userIndicatorController.retractIndicatorWithId(Self.loadingIndicatorIdentifier) } } + +private extension String { + var aliasLocalPart: String { + dropFirst().split(separator: ":").first.flatMap(String.init) ?? "" + } +} diff --git a/ElementX/Sources/Screens/SecurityAndPrivacyScreen/SecurityAndPrivacyScreenViewModel.swift b/ElementX/Sources/Screens/SecurityAndPrivacyScreen/SecurityAndPrivacyScreenViewModel.swift index 467844113..5a38d60b1 100644 --- a/ElementX/Sources/Screens/SecurityAndPrivacyScreen/SecurityAndPrivacyScreenViewModel.swift +++ b/ElementX/Sources/Screens/SecurityAndPrivacyScreen/SecurityAndPrivacyScreenViewModel.swift @@ -79,8 +79,18 @@ class SecurityAndPrivacyScreenViewModel: SecurityAndPrivacyScreenViewModelType, } .store(in: &cancellables) + let userIDServerName = clientProxy.userIDServerName + roomProxy.infoPublisher - .map(\.canonicalAlias) + .compactMap { roomInfo in + guard let userIDServerName else { + return nil + } + + // Give priority to aliases from the current user's homeserver as remote ones + // cannot be edited. + return roomInfo.firstAliasMatching(serverName: userIDServerName, useFallback: true) + } .removeDuplicates() .receive(on: DispatchQueue.main) .weakAssign(to: \.state.canonicalAlias, on: self) diff --git a/ElementX/Sources/Services/Room/RoomInfoProxy.swift b/ElementX/Sources/Services/Room/RoomInfoProxy.swift index f1ab84651..75924b850 100644 --- a/ElementX/Sources/Services/Room/RoomInfoProxy.swift +++ b/ElementX/Sources/Services/Room/RoomInfoProxy.swift @@ -67,6 +67,37 @@ struct RoomInfoProxy: BaseRoomInfoProxyProtocol { var pinnedEventIDs: Set { Set(roomInfo.pinnedEventIds) } var joinRule: JoinRule? { roomInfo.joinRule } var historyVisibility: RoomHistoryVisibility { roomInfo.historyVisibility } + + /// Find the first alias that matches the given homeserver + /// - Parameters: + /// - serverName: the homserver in question + /// - useFallback: whether to return any alias if none match + func firstAliasMatching(serverName: String?, useFallback: Bool) -> String? { + guard let serverName else { return nil } + + // Check if the canonical alias matches the homeserver + if let canonicalAlias = roomInfo.canonicalAlias, + canonicalAlias.range(of: serverName) != nil { + return canonicalAlias + } + + // Otherwise check the alternative aliases and return the first one that matches + if let matchingAlternativeAlias = roomInfo.alternativeAliases.filter({ $0.range(of: serverName) != nil }).first { + return matchingAlternativeAlias + } + + guard useFallback else { + return nil + } + + // Or just return the canonical alias if any + if let canonicalAlias = roomInfo.canonicalAlias { + return canonicalAlias + } + + // And finally return whatever the first alternative alias is + return roomInfo.alternativeAliases.first + } } struct RoomPreviewInfoProxy: BaseRoomInfoProxyProtocol { diff --git a/PreviewTests/Sources/__Snapshots__/PreviewTests/test_editRoomAddressScreen-iPad-en-GB.No-alias.png b/PreviewTests/Sources/__Snapshots__/PreviewTests/test_editRoomAddressScreen-iPad-en-GB.No-alias.png index 71a688937..09c241b2a 100644 --- a/PreviewTests/Sources/__Snapshots__/PreviewTests/test_editRoomAddressScreen-iPad-en-GB.No-alias.png +++ b/PreviewTests/Sources/__Snapshots__/PreviewTests/test_editRoomAddressScreen-iPad-en-GB.No-alias.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:dd9afbafee47a33dfa211d290b3737873fb55e8706ec7b6cf3c74f1820da86ce -size 103048 +oid sha256:b37dd8a18de99e48b75dee74174e64a096db0362d66abf7162906f5e9138e174 +size 103033 diff --git a/PreviewTests/Sources/__Snapshots__/PreviewTests/test_editRoomAddressScreen-iPad-pseudo.No-alias.png b/PreviewTests/Sources/__Snapshots__/PreviewTests/test_editRoomAddressScreen-iPad-pseudo.No-alias.png index 31d81148b..aadf21da3 100644 --- a/PreviewTests/Sources/__Snapshots__/PreviewTests/test_editRoomAddressScreen-iPad-pseudo.No-alias.png +++ b/PreviewTests/Sources/__Snapshots__/PreviewTests/test_editRoomAddressScreen-iPad-pseudo.No-alias.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:ef1930d51d5f0634aa06250abe7740029b3d8fc2f3ba319653e54aaf3bacccfb -size 111971 +oid sha256:6fb2d86369c003b01ba5647c2bcbd2fdd19dcc652bdbdd4e824f8cc706da8e32 +size 112083 diff --git a/PreviewTests/Sources/__Snapshots__/PreviewTests/test_editRoomAddressScreen-iPhone-16-en-GB.No-alias.png b/PreviewTests/Sources/__Snapshots__/PreviewTests/test_editRoomAddressScreen-iPhone-16-en-GB.No-alias.png index 79d366683..c3f6afd60 100644 --- a/PreviewTests/Sources/__Snapshots__/PreviewTests/test_editRoomAddressScreen-iPhone-16-en-GB.No-alias.png +++ b/PreviewTests/Sources/__Snapshots__/PreviewTests/test_editRoomAddressScreen-iPhone-16-en-GB.No-alias.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:b8bb258d0a65f5ab6bd36ce68909417e220b7336bac504ed9ba1c9ca77e3dc8b -size 54967 +oid sha256:825e6d71ddfad3f418a251f5dbd4a117867f2b9c098a2624ca43fb4b508651cf +size 54829 diff --git a/PreviewTests/Sources/__Snapshots__/PreviewTests/test_editRoomAddressScreen-iPhone-16-pseudo.No-alias.png b/PreviewTests/Sources/__Snapshots__/PreviewTests/test_editRoomAddressScreen-iPhone-16-pseudo.No-alias.png index 0d840eff2..3d4fa801f 100644 --- a/PreviewTests/Sources/__Snapshots__/PreviewTests/test_editRoomAddressScreen-iPhone-16-pseudo.No-alias.png +++ b/PreviewTests/Sources/__Snapshots__/PreviewTests/test_editRoomAddressScreen-iPhone-16-pseudo.No-alias.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:7dc96671de7a8e204694533122addb9a64f705bf1818d6a0f46a1ba535571a1e -size 68334 +oid sha256:03760870affe24024075c15eabfe8ffe8b3dfcd9c24a99473dedc9818e106df1 +size 68418 diff --git a/UnitTests/Sources/EditRoomAddressScreenViewModelTests.swift b/UnitTests/Sources/EditRoomAddressScreenViewModelTests.swift index 40129a546..fa4acd243 100644 --- a/UnitTests/Sources/EditRoomAddressScreenViewModelTests.swift +++ b/UnitTests/Sources/EditRoomAddressScreenViewModelTests.swift @@ -17,5 +17,67 @@ class EditRoomAddressScreenViewModelTests: XCTestCase { viewModel.context } - override func setUpWithError() throws { } + func testCanonicalAliasChosen() async throws { + let roomProxy = JoinedRoomProxyMock(.init(name: "Room Name", canonicalAlias: "#room-name:matrix.org", + alternativeAliases: ["#beta:homeserver.io", + "#alternative-room-name:matrix.org"])) + + viewModel = EditRoomAddressScreenViewModel(roomProxy: roomProxy, + clientProxy: ClientProxyMock(.init(userIDServerName: "matrix.org")), + userIndicatorController: UserIndicatorControllerMock()) + + let deferred = deferFulfillment(context.$viewState) { state in + state.bindings.desiredAliasLocalPart == "room-name" + } + + try await deferred.fulfill() + } + + /// Priority should be given to aliases from the current user's homeserver as they can edit those. + func testAlternativeAliasChosen() async throws { + let roomProxy = JoinedRoomProxyMock(.init(name: "Room Name", canonicalAlias: "#alpha:homeserver.io", + alternativeAliases: ["#beta:homeserver.io", + "#room-name:matrix.org", + "#alternative-room-name:matrix.org"])) + + viewModel = EditRoomAddressScreenViewModel(roomProxy: roomProxy, + clientProxy: ClientProxyMock(.init(userIDServerName: "matrix.org")), + userIndicatorController: UserIndicatorControllerMock()) + + let deferred = deferFulfillment(context.$viewState) { state in + state.bindings.desiredAliasLocalPart == "room-name" + } + + try await deferred.fulfill() + } + + func testBuildAliasFromDisplayName() async throws { + let roomProxy = JoinedRoomProxyMock(.init(name: "Room Name")) + + viewModel = EditRoomAddressScreenViewModel(roomProxy: roomProxy, + clientProxy: ClientProxyMock(.init(userIDServerName: "matrix.org")), + userIndicatorController: UserIndicatorControllerMock()) + + let deferred = deferFulfillment(context.$viewState) { state in + state.bindings.desiredAliasLocalPart == "room-name" + } + + try await deferred.fulfill() + } + + func testCorrectMethodsCalledOnSave() async throws { + let clientProxy = ClientProxyMock(.init(userIDServerName: "matrix.org")) + clientProxy.isAliasAvailableReturnValue = .success(true) + + let roomProxy = JoinedRoomProxyMock(.init(name: "Room Name")) + roomProxy.publishRoomAliasInRoomDirectoryReturnValue = .success(true) + roomProxy.updateCanonicalAliasAltAliasesReturnValue = .success(()) + roomProxy.removeRoomAliasFromRoomDirectoryReturnValue = .success(true) + + viewModel = EditRoomAddressScreenViewModel(roomProxy: roomProxy, + clientProxy: clientProxy, + userIndicatorController: UserIndicatorControllerMock()) + + context.send(viewAction: .save) + } }