From bbfc6c1e17770097442e0b05e12c12bca93567ab Mon Sep 17 00:00:00 2001 From: Mauro <34335419+Velin92@users.noreply.github.com> Date: Tue, 1 Apr 2025 18:06:40 +0200 Subject: [PATCH] Add alerts with reason for kick and ban (#3967) * feat: added alerts with reason for kick and ban in the room members list screen * fix tests --- .../en-US.lproj/Localizable.strings | 4 + .../en.lproj/Localizable.strings | 4 + ElementX/Sources/Generated/Strings.swift | 8 ++ .../Mocks/Generated/GeneratedMocks.swift | 96 +++++++++---------- .../Sources/Mocks/JoinedRoomProxyMock.swift | 4 +- .../RoomMembersListScreenModels.swift | 2 + .../RoomMembersListScreenViewModel.swift | 34 +++++-- .../RoomMembersListManageMemberSheet.swift | 12 +-- .../Services/Room/JoinedRoomProxy.swift | 8 +- .../Services/Room/RoomProxyProtocol.swift | 4 +- .../RoomMembersListScreenViewModelTests.swift | 30 ++++-- 11 files changed, 125 insertions(+), 81 deletions(-) diff --git a/ElementX/Resources/Localizations/en-US.lproj/Localizable.strings b/ElementX/Resources/Localizations/en-US.lproj/Localizable.strings index adb38c09f..9bdc31459 100644 --- a/ElementX/Resources/Localizations/en-US.lproj/Localizable.strings +++ b/ElementX/Resources/Localizations/en-US.lproj/Localizable.strings @@ -208,6 +208,7 @@ "common_privacy_policy" = "Privacy policy"; "common_reaction" = "Reaction"; "common_reactions" = "Reactions"; +"common_reason" = "Reason"; "common_recovery_key" = "Recovery key"; "common_refreshing" = "Refreshing…"; "common_replying_to" = "Replying to %1$@"; @@ -875,6 +876,9 @@ "screen_room_member_list_ban_member_confirmation_title" = "Are you sure you want to ban this member?"; "screen_room_member_list_banned_empty" = "There are no banned users in this room."; "screen_room_member_list_banning_user" = "Banning %1$@"; +"screen_room_member_list_kick_member_confirmation_action" = "Remove"; +"screen_room_member_list_kick_member_confirmation_description" = "They will be able to join this room again if invited."; +"screen_room_member_list_kick_member_confirmation_title" = "Are you sure you want to remove this member?"; "screen_room_member_list_manage_member_ban" = "Remove and ban member"; "screen_room_member_list_manage_member_remove" = "Remove from room"; "screen_room_member_list_manage_member_remove_confirmation_kick" = "Only remove member"; diff --git a/ElementX/Resources/Localizations/en.lproj/Localizable.strings b/ElementX/Resources/Localizations/en.lproj/Localizable.strings index e3740d0bc..cc74ffbb2 100644 --- a/ElementX/Resources/Localizations/en.lproj/Localizable.strings +++ b/ElementX/Resources/Localizations/en.lproj/Localizable.strings @@ -208,6 +208,7 @@ "common_privacy_policy" = "Privacy policy"; "common_reaction" = "Reaction"; "common_reactions" = "Reactions"; +"common_reason" = "Reason"; "common_recovery_key" = "Recovery key"; "common_refreshing" = "Refreshing…"; "common_replying_to" = "Replying to %1$@"; @@ -875,6 +876,9 @@ "screen_room_member_list_ban_member_confirmation_title" = "Are you sure you want to ban this member?"; "screen_room_member_list_banned_empty" = "There are no banned users in this room."; "screen_room_member_list_banning_user" = "Banning %1$@"; +"screen_room_member_list_kick_member_confirmation_action" = "Remove"; +"screen_room_member_list_kick_member_confirmation_description" = "They will be able to join this room again if invited."; +"screen_room_member_list_kick_member_confirmation_title" = "Are you sure you want to remove this member?"; "screen_room_member_list_manage_member_ban" = "Remove and ban member"; "screen_room_member_list_manage_member_remove" = "Remove from room"; "screen_room_member_list_manage_member_remove_confirmation_kick" = "Only remove member"; diff --git a/ElementX/Sources/Generated/Strings.swift b/ElementX/Sources/Generated/Strings.swift index 0701b9bb4..bf0224cad 100644 --- a/ElementX/Sources/Generated/Strings.swift +++ b/ElementX/Sources/Generated/Strings.swift @@ -478,6 +478,8 @@ internal enum L10n { internal static var commonReaction: String { return L10n.tr("Localizable", "common_reaction") } /// Reactions internal static var commonReactions: String { return L10n.tr("Localizable", "common_reactions") } + /// Reason + internal static var commonReason: String { return L10n.tr("Localizable", "common_reason") } /// Recovery key internal static var commonRecoveryKey: String { return L10n.tr("Localizable", "common_recovery_key") } /// Refreshing… @@ -2050,6 +2052,12 @@ internal enum L10n { internal static func screenRoomMemberListHeaderTitle(_ p1: Int) -> String { return L10n.tr("Localizable", "screen_room_member_list_header_title", p1) } + /// Remove + internal static var screenRoomMemberListKickMemberConfirmationAction: String { return L10n.tr("Localizable", "screen_room_member_list_kick_member_confirmation_action") } + /// They will be able to join this room again if invited. + internal static var screenRoomMemberListKickMemberConfirmationDescription: String { return L10n.tr("Localizable", "screen_room_member_list_kick_member_confirmation_description") } + /// Are you sure you want to remove this member? + internal static var screenRoomMemberListKickMemberConfirmationTitle: String { return L10n.tr("Localizable", "screen_room_member_list_kick_member_confirmation_title") } /// Remove and ban member internal static var screenRoomMemberListManageMemberBan: String { return L10n.tr("Localizable", "screen_room_member_list_manage_member_ban") } /// Remove from room diff --git a/ElementX/Sources/Mocks/Generated/GeneratedMocks.swift b/ElementX/Sources/Mocks/Generated/GeneratedMocks.swift index 47411e300..8075e3cdf 100644 --- a/ElementX/Sources/Mocks/Generated/GeneratedMocks.swift +++ b/ElementX/Sources/Mocks/Generated/GeneratedMocks.swift @@ -9077,15 +9077,15 @@ class JoinedRoomProxyMock: JoinedRoomProxyProtocol, @unchecked Sendable { } //MARK: - kickUser - var kickUserUnderlyingCallsCount = 0 - var kickUserCallsCount: Int { + var kickUserReasonUnderlyingCallsCount = 0 + var kickUserReasonCallsCount: Int { get { if Thread.isMainThread { - return kickUserUnderlyingCallsCount + return kickUserReasonUnderlyingCallsCount } else { var returnValue: Int? = nil DispatchQueue.main.sync { - returnValue = kickUserUnderlyingCallsCount + returnValue = kickUserReasonUnderlyingCallsCount } return returnValue! @@ -9093,29 +9093,29 @@ class JoinedRoomProxyMock: JoinedRoomProxyProtocol, @unchecked Sendable { } set { if Thread.isMainThread { - kickUserUnderlyingCallsCount = newValue + kickUserReasonUnderlyingCallsCount = newValue } else { DispatchQueue.main.sync { - kickUserUnderlyingCallsCount = newValue + kickUserReasonUnderlyingCallsCount = newValue } } } } - var kickUserCalled: Bool { - return kickUserCallsCount > 0 + var kickUserReasonCalled: Bool { + return kickUserReasonCallsCount > 0 } - var kickUserReceivedUserID: String? - var kickUserReceivedInvocations: [String] = [] + var kickUserReasonReceivedArguments: (userID: String, reason: String?)? + var kickUserReasonReceivedInvocations: [(userID: String, reason: String?)] = [] - var kickUserUnderlyingReturnValue: Result! - var kickUserReturnValue: Result! { + var kickUserReasonUnderlyingReturnValue: Result! + var kickUserReasonReturnValue: Result! { get { if Thread.isMainThread { - return kickUserUnderlyingReturnValue + return kickUserReasonUnderlyingReturnValue } else { var returnValue: Result? = nil DispatchQueue.main.sync { - returnValue = kickUserUnderlyingReturnValue + returnValue = kickUserReasonUnderlyingReturnValue } return returnValue! @@ -9123,39 +9123,39 @@ class JoinedRoomProxyMock: JoinedRoomProxyProtocol, @unchecked Sendable { } set { if Thread.isMainThread { - kickUserUnderlyingReturnValue = newValue + kickUserReasonUnderlyingReturnValue = newValue } else { DispatchQueue.main.sync { - kickUserUnderlyingReturnValue = newValue + kickUserReasonUnderlyingReturnValue = newValue } } } } - var kickUserClosure: ((String) async -> Result)? + var kickUserReasonClosure: ((String, String?) async -> Result)? - func kickUser(_ userID: String) async -> Result { - kickUserCallsCount += 1 - kickUserReceivedUserID = userID + func kickUser(_ userID: String, reason: String?) async -> Result { + kickUserReasonCallsCount += 1 + kickUserReasonReceivedArguments = (userID: userID, reason: reason) DispatchQueue.main.async { - self.kickUserReceivedInvocations.append(userID) + self.kickUserReasonReceivedInvocations.append((userID: userID, reason: reason)) } - if let kickUserClosure = kickUserClosure { - return await kickUserClosure(userID) + if let kickUserReasonClosure = kickUserReasonClosure { + return await kickUserReasonClosure(userID, reason) } else { - return kickUserReturnValue + return kickUserReasonReturnValue } } //MARK: - banUser - var banUserUnderlyingCallsCount = 0 - var banUserCallsCount: Int { + var banUserReasonUnderlyingCallsCount = 0 + var banUserReasonCallsCount: Int { get { if Thread.isMainThread { - return banUserUnderlyingCallsCount + return banUserReasonUnderlyingCallsCount } else { var returnValue: Int? = nil DispatchQueue.main.sync { - returnValue = banUserUnderlyingCallsCount + returnValue = banUserReasonUnderlyingCallsCount } return returnValue! @@ -9163,29 +9163,29 @@ class JoinedRoomProxyMock: JoinedRoomProxyProtocol, @unchecked Sendable { } set { if Thread.isMainThread { - banUserUnderlyingCallsCount = newValue + banUserReasonUnderlyingCallsCount = newValue } else { DispatchQueue.main.sync { - banUserUnderlyingCallsCount = newValue + banUserReasonUnderlyingCallsCount = newValue } } } } - var banUserCalled: Bool { - return banUserCallsCount > 0 + var banUserReasonCalled: Bool { + return banUserReasonCallsCount > 0 } - var banUserReceivedUserID: String? - var banUserReceivedInvocations: [String] = [] + var banUserReasonReceivedArguments: (userID: String, reason: String?)? + var banUserReasonReceivedInvocations: [(userID: String, reason: String?)] = [] - var banUserUnderlyingReturnValue: Result! - var banUserReturnValue: Result! { + var banUserReasonUnderlyingReturnValue: Result! + var banUserReasonReturnValue: Result! { get { if Thread.isMainThread { - return banUserUnderlyingReturnValue + return banUserReasonUnderlyingReturnValue } else { var returnValue: Result? = nil DispatchQueue.main.sync { - returnValue = banUserUnderlyingReturnValue + returnValue = banUserReasonUnderlyingReturnValue } return returnValue! @@ -9193,26 +9193,26 @@ class JoinedRoomProxyMock: JoinedRoomProxyProtocol, @unchecked Sendable { } set { if Thread.isMainThread { - banUserUnderlyingReturnValue = newValue + banUserReasonUnderlyingReturnValue = newValue } else { DispatchQueue.main.sync { - banUserUnderlyingReturnValue = newValue + banUserReasonUnderlyingReturnValue = newValue } } } } - var banUserClosure: ((String) async -> Result)? + var banUserReasonClosure: ((String, String?) async -> Result)? - func banUser(_ userID: String) async -> Result { - banUserCallsCount += 1 - banUserReceivedUserID = userID + func banUser(_ userID: String, reason: String?) async -> Result { + banUserReasonCallsCount += 1 + banUserReasonReceivedArguments = (userID: userID, reason: reason) DispatchQueue.main.async { - self.banUserReceivedInvocations.append(userID) + self.banUserReasonReceivedInvocations.append((userID: userID, reason: reason)) } - if let banUserClosure = banUserClosure { - return await banUserClosure(userID) + if let banUserReasonClosure = banUserReasonClosure { + return await banUserReasonClosure(userID, reason) } else { - return banUserReturnValue + return banUserReasonReturnValue } } //MARK: - unbanUser diff --git a/ElementX/Sources/Mocks/JoinedRoomProxyMock.swift b/ElementX/Sources/Mocks/JoinedRoomProxyMock.swift index b21a2ca49..dfba5ebf2 100644 --- a/ElementX/Sources/Mocks/JoinedRoomProxyMock.swift +++ b/ElementX/Sources/Mocks/JoinedRoomProxyMock.swift @@ -107,8 +107,8 @@ extension JoinedRoomProxyMock { canUserJoinCallUserIDReturnValue = .success(configuration.canUserJoinCall) canUserPinOrUnpinUserIDReturnValue = .success(configuration.canUserPin) - kickUserReturnValue = .success(()) - banUserReturnValue = .success(()) + kickUserReasonReturnValue = .success(()) + banUserReasonReturnValue = .success(()) unbanUserReturnValue = .success(()) let widgetDriver = ElementCallWidgetDriverMock() diff --git a/ElementX/Sources/Screens/RoomMemberListScreen/RoomMembersListScreenModels.swift b/ElementX/Sources/Screens/RoomMemberListScreen/RoomMembersListScreenModels.swift index cfd436e49..5f15e3c66 100644 --- a/ElementX/Sources/Screens/RoomMemberListScreen/RoomMembersListScreenModels.swift +++ b/ElementX/Sources/Screens/RoomMemberListScreen/RoomMembersListScreenModels.swift @@ -111,4 +111,6 @@ enum RoomMembersListScreenViewAction { enum RoomMembersListScreenAlertType: Hashable { case unbanConfirmation(RoomMemberDetails) + case kickConfirmation + case banConfirmation } diff --git a/ElementX/Sources/Screens/RoomMemberListScreen/RoomMembersListScreenViewModel.swift b/ElementX/Sources/Screens/RoomMemberListScreen/RoomMembersListScreenViewModel.swift index 68a833d7c..fbf3441f9 100644 --- a/ElementX/Sources/Screens/RoomMemberListScreen/RoomMembersListScreenViewModel.swift +++ b/ElementX/Sources/Screens/RoomMemberListScreen/RoomMembersListScreenViewModel.swift @@ -51,9 +51,31 @@ class RoomMembersListScreenViewModel: RoomMembersListScreenViewModelType, RoomMe case .showMemberDetails(let member): showMemberDetails(member) case .kickMember(let member): - Task { await kickMember(member) } + var reason: String? + let binding: Binding = .init(get: { reason ?? "" }, + set: { reason = $0.isEmpty ? nil : $0 }) + state.bindings.alertInfo = .init(id: .kickConfirmation, + title: L10n.screenRoomMemberListKickMemberConfirmationTitle, + message: L10n.screenRoomMemberListKickMemberConfirmationDescription, + primaryButton: .init(title: L10n.actionCancel, role: .cancel) { }, + secondaryButton: .init(title: L10n.screenRoomMemberListKickMemberConfirmationAction) { [weak self] in Task { await self?.kickMember(member, reason: reason) } }, + textFields: [.init(placeholder: L10n.commonReason, + text: binding, + autoCapitalization: .sentences, + autoCorrectionDisabled: false)]) case .banMember(let member): - Task { await banMember(member) } + var reason: String? + let binding: Binding = .init(get: { reason ?? "" }, + set: { reason = $0.isEmpty ? nil : $0 }) + state.bindings.alertInfo = .init(id: .banConfirmation, + title: L10n.screenRoomMemberListBanMemberConfirmationTitle, + message: L10n.screenRoomMemberListBanMemberConfirmationDescription, + primaryButton: .init(title: L10n.actionCancel, role: .cancel) { }, + secondaryButton: .init(title: L10n.screenRoomMemberListBanMemberConfirmationAction) { [weak self] in Task { await self?.banMember(member, reason: reason) } }, + textFields: [.init(placeholder: L10n.commonReason, + text: binding, + autoCapitalization: .sentences, + autoCorrectionDisabled: false)]) case .unbanMember(let member): Task { await unbanMember(member) } case .invite: @@ -183,11 +205,11 @@ class RoomMembersListScreenViewModel: RoomMembersListScreenViewModelType, RoomMe // MARK: - Member Management - private func kickMember(_ member: RoomMemberDetails) async { + private func kickMember(_ member: RoomMemberDetails, reason: String?) async { let indicatorTitle = L10n.screenRoomMemberListRemovingUser(member.name ?? member.id) showManageMemberIndicator(title: indicatorTitle) - switch await roomProxy.kickUser(member.id) { + switch await roomProxy.kickUser(member.id, reason: reason) { case .success: state.bindings.memberToManage = nil hideManageMemberIndicator(title: indicatorTitle) @@ -197,11 +219,11 @@ class RoomMembersListScreenViewModel: RoomMembersListScreenViewModelType, RoomMe } } - private func banMember(_ member: RoomMemberDetails) async { + private func banMember(_ member: RoomMemberDetails, reason: String?) async { let indicatorTitle = L10n.screenRoomMemberListBanningUser(member.name ?? member.id) showManageMemberIndicator(title: indicatorTitle) - switch await roomProxy.banUser(member.id) { + switch await roomProxy.banUser(member.id, reason: reason) { case .success: state.bindings.memberToManage = nil hideManageMemberIndicator(title: indicatorTitle) diff --git a/ElementX/Sources/Screens/RoomMemberListScreen/View/RoomMembersListManageMemberSheet.swift b/ElementX/Sources/Screens/RoomMemberListScreen/View/RoomMembersListManageMemberSheet.swift index 775c29143..7632c9099 100644 --- a/ElementX/Sources/Screens/RoomMemberListScreen/View/RoomMembersListManageMemberSheet.swift +++ b/ElementX/Sources/Screens/RoomMemberListScreen/View/RoomMembersListManageMemberSheet.swift @@ -14,8 +14,6 @@ struct RoomMembersListManageMemberSheet: View { @ObservedObject var context: RoomMembersListScreenViewModel.Context - @State private var isPresentingBanConfirmation = false - var body: some View { Form { AvatarHeaderView(member: member, @@ -44,7 +42,7 @@ struct RoomMembersListManageMemberSheet: View { icon: \.block, role: .destructive), kind: .button { - isPresentingBanConfirmation = true + context.send(viewAction: .banMember(member)) }) } } @@ -53,14 +51,6 @@ struct RoomMembersListManageMemberSheet: View { .scrollBounceBehavior(.basedOnSize) .presentationDragIndicator(.visible) .presentationDetents([.large, .fraction(0.54)]) // Maybe find a way to use the ideal height somehow? - .alert(L10n.screenRoomMemberListBanMemberConfirmationTitle, isPresented: $isPresentingBanConfirmation) { - Button(L10n.actionCancel, role: .cancel) { } - Button(L10n.screenRoomMemberListBanMemberConfirmationAction) { - context.send(viewAction: .banMember(member)) - } - } message: { - Text(L10n.screenRoomMemberListBanMemberConfirmationDescription) - } } } diff --git a/ElementX/Sources/Services/Room/JoinedRoomProxy.swift b/ElementX/Sources/Services/Room/JoinedRoomProxy.swift index 50a1e6273..213d04cb0 100644 --- a/ElementX/Sources/Services/Room/JoinedRoomProxy.swift +++ b/ElementX/Sources/Services/Room/JoinedRoomProxy.swift @@ -623,9 +623,9 @@ class JoinedRoomProxy: JoinedRoomProxyProtocol { // MARK: - Moderation - func kickUser(_ userID: String) async -> Result { + func kickUser(_ userID: String, reason: String?) async -> Result { do { - try await room.kickUser(userId: userID, reason: nil) + try await room.kickUser(userId: userID, reason: reason) return .success(()) } catch { MXLog.error("Failed kicking \(userID) with error: \(error)") @@ -633,9 +633,9 @@ class JoinedRoomProxy: JoinedRoomProxyProtocol { } } - func banUser(_ userID: String) async -> Result { + func banUser(_ userID: String, reason: String?) async -> Result { do { - try await room.banUser(userId: userID, reason: nil) + try await room.banUser(userId: userID, reason: reason) return .success(()) } catch { MXLog.error("Failed banning \(userID) with error: \(error)") diff --git a/ElementX/Sources/Services/Room/RoomProxyProtocol.swift b/ElementX/Sources/Services/Room/RoomProxyProtocol.swift index 1e135decc..48cae0671 100644 --- a/ElementX/Sources/Services/Room/RoomProxyProtocol.swift +++ b/ElementX/Sources/Services/Room/RoomProxyProtocol.swift @@ -159,8 +159,8 @@ protocol JoinedRoomProxyProtocol: RoomProxyProtocol { // MARK: - Moderation - func kickUser(_ userID: String) async -> Result - func banUser(_ userID: String) async -> Result + func kickUser(_ userID: String, reason: String?) async -> Result + func banUser(_ userID: String, reason: String?) async -> Result func unbanUser(_ userID: String) async -> Result // MARK: - Element Call diff --git a/UnitTests/Sources/RoomMembersListScreenViewModelTests.swift b/UnitTests/Sources/RoomMembersListScreenViewModelTests.swift index 7f118d39c..3ae5633e5 100644 --- a/UnitTests/Sources/RoomMembersListScreenViewModelTests.swift +++ b/UnitTests/Sources/RoomMembersListScreenViewModelTests.swift @@ -249,28 +249,42 @@ class RoomMembersListScreenViewModelTests: XCTestCase { func testKickMember() async throws { setup(with: .allMembersAsAdmin) - let deferred = deferFulfillment(context.$viewState) { !$0.visibleJoinedMembers.isEmpty } + var deferred = deferFulfillment(context.$viewState) { !$0.visibleJoinedMembers.isEmpty } try await deferred.fulfill() + let expectation = XCTestExpectation(description: "Ban member") + roomProxy.kickUserReasonClosure = { _, _ in + defer { expectation.fulfill() } + return .success(()) + } + context.send(viewAction: .kickMember(viewModel.state.visibleJoinedMembers[0].member)) - // Calling the mock won't actually change any view state, so sleep instead. - try await Task.sleep(for: .milliseconds(100)) + deferred = deferFulfillment(context.$viewState) { $0.bindings.alertInfo != nil } + try await deferred.fulfill() - XCTAssert(roomProxy.kickUserCalled) + context.alertInfo?.secondaryButton?.action?() + await fulfillment(of: [expectation]) } func testBanMember() async throws { setup(with: .allMembersAsAdmin) - let deferred = deferFulfillment(context.$viewState) { !$0.visibleJoinedMembers.isEmpty } + var deferred = deferFulfillment(context.$viewState) { !$0.visibleJoinedMembers.isEmpty } try await deferred.fulfill() + let expectation = XCTestExpectation(description: "Ban member") + roomProxy.banUserReasonClosure = { _, _ in + defer { expectation.fulfill() } + return .success(()) + } + context.send(viewAction: .banMember(viewModel.state.visibleJoinedMembers[0].member)) - // Calling the mock won't actually change any view state, so sleep instead. - try await Task.sleep(for: .milliseconds(100)) + deferred = deferFulfillment(context.$viewState) { $0.bindings.alertInfo != nil } + try await deferred.fulfill() - XCTAssert(roomProxy.banUserCalled) + context.alertInfo?.secondaryButton?.action?() + await fulfillment(of: [expectation]) } func testUnbanMember() async throws {