Fix various bugs in the moderation feature. (#2608)

* Fix search field string. * Show your own user as an Admin when changing roles. * Also show invited users when changing roles. * Don't allow admin's to kick/ban other admins or themselves. * Fix a bug when left members were counted as admins/moderators. * Show when a member is pending. * Add sections to the change role screen.
2026-11-03 06:55:59 +00:00 · 2024-03-27 10:33:40 +00:00
parent bb725db6bb
commit e424a02a2b
51 changed files with 413 additions and 148 deletions
@@ -1263,8 +1263,18 @@ internal enum L10n {
  internal static var screenRoomChangeRoleConfirmDemoteSelfDescription: String { return L10n.tr("Localizable", "screen_room_change_role_confirm_demote_self_description") }
  /// Demote yourself?
  internal static var screenRoomChangeRoleConfirmDemoteSelfTitle: String { return L10n.tr("Localizable", "screen_room_change_role_confirm_demote_self_title") }
+  /// %1$@ (Pending)
+  internal static func screenRoomChangeRoleInvitedMemberName(_ p1: Any) -> String {
+    return L10n.tr("Localizable", "screen_room_change_role_invited_member_name", String(describing: p1))
+  }
  /// Edit Moderators
  internal static var screenRoomChangeRoleModeratorsTitle: String { return L10n.tr("Localizable", "screen_room_change_role_moderators_title") }
+  /// Admins
+  internal static var screenRoomChangeRoleSectionAdministrators: String { return L10n.tr("Localizable", "screen_room_change_role_section_administrators") }
+  /// Moderators
+  internal static var screenRoomChangeRoleSectionModerators: String { return L10n.tr("Localizable", "screen_room_change_role_section_moderators") }
+  /// Members
+  internal static var screenRoomChangeRoleSectionUsers: String { return L10n.tr("Localizable", "screen_room_change_role_section_users") }
  /// You have unsaved changes.
  internal static var screenRoomChangeRoleUnsavedChangesDescription: String { return L10n.tr("Localizable", "screen_room_change_role_unsaved_changes_description") }
  /// Save changes?
@@ -53,7 +53,7 @@ extension RoomMemberProxyMock {
    
    static var mockMeAdmin: RoomMemberProxyMock {
        RoomMemberProxyMock(with: .init(userID: "@me:matrix.org",
-                                        displayName: "Me admin",
+                                        displayName: "Me",
                                        avatarURL: URL.picturesDirectory,
                                        membership: .join,
                                        powerLevel: 100,
@@ -24,8 +24,13 @@ enum RoomChangeRolesScreenViewModelAction {
 struct RoomChangeRolesScreenViewState: BindableState {
    /// The screen's current mode (which role we are promoting/demoting users to/from.
    let mode: RoomMemberDetails.Role
-    /// All of the room's members.
-    var members: [RoomMemberDetails]
+    /// All of the room's members who are currently admins.
+    var administrators: [RoomMemberDetails]
+    /// All of the room's members who are currently moderators.
+    var moderators: [RoomMemberDetails]
+    /// All of the room's members who are currently neither an admin or moderator.
+    var users: [RoomMemberDetails]
+    
    var bindings: RoomChangeRolesScreenViewStateBindings
    
    /// The members selected for promotion to the current role.
@@ -48,19 +53,24 @@ struct RoomChangeRolesScreenViewState: BindableState {
        }
    }
    
-    /// The visible members in the screen (after searching).
-    var visibleMembers: [RoomMemberDetails] {
-        guard !bindings.searchQuery.isEmpty else { return members }
-        
-        return members.filter { member in
-            member.name?.localizedStandardContains(bindings.searchQuery) == true
-                || member.id.localizedStandardContains(bindings.searchQuery)
-        }
+    /// The visible admins in the screen (after searching).
+    var visibleAdministrators: [RoomMemberDetails] {
+        administrators.filter { $0.matches(searchQuery: bindings.searchQuery) }
+    }
+    
+    /// The visible mods in the screen (after searching).
+    var visibleModerators: [RoomMemberDetails] {
+        moderators.filter { $0.matches(searchQuery: bindings.searchQuery) }
+    }
+    
+    /// The visible regular users in the screen (after searching).
+    var visibleUsers: [RoomMemberDetails] {
+        users.filter { $0.matches(searchQuery: bindings.searchQuery) }
    }
    
    /// All of the members who will gain/keep this screen's role after saving any changes.
    var membersWithRole: [RoomMemberDetails] {
-        members.filter(isMemberSelected)
+        administrators.filter(isMemberSelected) + moderators.filter(isMemberSelected) + users.filter(isMemberSelected)
    }
    
    /// Whether or not any changes have been made to the members.
@@ -40,7 +40,9 @@ class RoomChangeRolesScreenViewModel: RoomChangeRolesScreenViewModelType, RoomCh
        self.analytics = analytics
        
        super.init(initialViewState: RoomChangeRolesScreenViewState(mode: mode,
-                                                                    members: [],
+                                                                    administrators: [],
+                                                                    moderators: [],
+                                                                    users: [],
                                                                    bindings: .init()))
        
        roomProxy.membersPublisher
@@ -82,10 +84,27 @@ class RoomChangeRolesScreenViewModel: RoomChangeRolesScreenViewModelType, RoomCh
    // MARK: - Private
    
    private func updateMembers(_ members: [RoomMemberProxyProtocol]) {
-        state.members = members.sorted().compactMap { member in
-            guard member.membership == .join, member.userID != roomProxy.ownUserID else { return nil }
-            return RoomMemberDetails(withProxy: member)
+        var administrators = [RoomMemberDetails]()
+        var moderators = [RoomMemberDetails]()
+        var users = [RoomMemberDetails]()
+        
+        for member in members.sorted() {
+            guard member.isActive else { continue }
+            let memberDetails = RoomMemberDetails(withProxy: member)
+            
+            switch member.role {
+            case .administrator:
+                administrators.append(memberDetails)
+            case .moderator:
+                moderators.append(memberDetails)
+            case .user:
+                users.append(memberDetails)
+            }
        }
+        
+        state.administrators = administrators
+        state.moderators = moderators
+        state.users = users
    }
    
    private func toggleMember(_ member: RoomMemberDetails) {
@@ -56,34 +56,20 @@ struct RoomChangeRolesScreen: View {
                    }
                }
                
-                membersSection
+                RoomChangeRolesScreenSection(members: context.viewState.administrators,
+                                             title: L10n.screenRoomChangeRoleSectionAdministrators,
+                                             context: context)
+                RoomChangeRolesScreenSection(members: context.viewState.moderators,
+                                             title: L10n.screenRoomChangeRoleSectionModerators,
+                                             context: context)
+                RoomChangeRolesScreenSection(members: context.viewState.users,
+                                             title: L10n.screenRoomChangeRoleSectionUsers,
+                                             context: context)
            }
        }
    }
    
-    @ViewBuilder
-    private var membersSection: some View {
-        if !context.viewState.visibleMembers.isEmpty {
-            Section {
-                ForEach(context.viewState.visibleMembers, id: \.id) { member in
-                    RoomChangeRolesScreenRow(member: member,
-                                             imageProvider: context.imageProvider,
-                                             isSelected: context.viewState.isMemberSelected(member)) {
-                        context.send(viewAction: .toggleMember(member))
-                    }
-                    .disabled(member.role == .administrator)
-                }
-            } header: {
-                Text(L10n.screenRoomMemberListRoomMembersHeaderTitle)
-                    .compoundListSectionHeader()
-            }
-        } else {
-            Section.empty
-        }
-    }
-    
    @ScaledMetric private var cellWidth: CGFloat = 72
-
    private var membersWithRoleSection: some View {
        ScrollView(.horizontal, showsIndicators: false) {
            ScrollViewReader { scrollView in
@@ -28,10 +28,20 @@ struct RoomChangeRolesScreenRow: View {
    let action: () -> Void
    
    var body: some View {
-        ListRow(label: .avatar(title: member.name ?? member.id,
+        ListRow(label: .avatar(title: memberName,
                               description: member.name == nil ? nil : member.id,
                               icon: avatar),
-                kind: isEnabled ? .multiSelection(isSelected: isSelected, action: action) : .label)
+                kind: .multiSelection(isSelected: isSelected, action: action))
+    }
+    
+    var memberName: String {
+        let name = member.name ?? member.id
+        
+        return if member.isInvited {
+            L10n.screenRoomChangeRoleInvitedMemberName(name)
+        } else {
+            name
+        }
    }
    
    var avatar: LoadableAvatarImage {
@@ -58,6 +68,11 @@ struct RoomChangeRolesScreenRow_Previews: PreviewProvider, TestablePreview {
                                     isSelected: false,
                                     action: action)
            
+            RoomChangeRolesScreenRow(member: .init(withProxy: RoomMemberProxyMock.mockInvited),
+                                     imageProvider: MockMediaProvider(),
+                                     isSelected: false,
+                                     action: action)
+            
            RoomChangeRolesScreenRow(member: .init(withProxy: RoomMemberProxyMock.mockCharlie),
                                     imageProvider: MockMediaProvider(),
                                     isSelected: true,
@@ -0,0 +1,43 @@
+//
+// Copyright 2024 New Vector Ltd
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+
+import Compound
+import SwiftUI
+
+struct RoomChangeRolesScreenSection: View {
+    let members: [RoomMemberDetails]
+    let title: String
+    
+    @ObservedObject var context: RoomChangeRolesScreenViewModel.Context
+    
+    var body: some View {
+        if !members.isEmpty {
+            Section {
+                ForEach(members, id: \.id) { member in
+                    RoomChangeRolesScreenRow(member: member,
+                                             imageProvider: context.imageProvider,
+                                             isSelected: context.viewState.isMemberSelected(member)) {
+                        context.send(viewAction: .toggleMember(member))
+                    }
+                    .disabled(member.role == .administrator)
+                }
+            } header: {
+                Text(title)
+                    .compoundListSectionHeader()
+            }
+        }
+    }
+}
@@ -19,6 +19,15 @@ import Foundation
 enum RoomMembersListScreenViewModelAction {
    case selectMember(_ member: RoomMemberProxyProtocol)
    case invite
+    
+    var isSelectMember: Bool {
+        switch self {
+        case .selectMember:
+            true
+        default:
+            false
+        }
+    }
 }

 /// The different modes that the screen can be in.
@@ -54,7 +63,7 @@ struct RoomMembersListScreenViewState: BindableState {
        self.bannedMembers = bannedMembers
        self.bindings = bindings
    }
-
+    
    var visibleJoinedMembers: [RoomMemberDetails] {
        joinedMembers
            .filter { $0.matches(searchQuery: bindings.searchQuery) }
@@ -76,12 +85,25 @@ struct RoomMembersListScreenViewStateBindings {
    /// The current mode the screen is in.
    var mode: RoomMembersListScreenMode = .members
    /// A selected member to kick, ban, promote etc.
-    var memberToManage: RoomMemberDetails?
+    var memberToManage: RoomMembersListScreenManagementDetails?

    /// Information describing the currently displayed alert.
    var alertInfo: AlertInfo<RoomMembersListScreenAlertType>?
 }

+/// Information about managing a particular room member.
+struct RoomMembersListScreenManagementDetails: Identifiable {
+    var id: String { member.id }
+    
+    /// The member that is being managed.
+    let member: RoomMemberDetails
+    
+    /// A management action that can be performed on the member.
+    enum Action { case kick, ban }
+    /// The management actions available for `member`.
+    let actions: [Action]
+}
+
 enum RoomMembersListScreenViewAction {
    case selectMember(RoomMemberDetails)
    case showMemberDetails(RoomMemberDetails)
@@ -94,13 +116,3 @@ enum RoomMembersListScreenViewAction {
 enum RoomMembersListScreenAlertType: Hashable {
    case unbanConfirmation(RoomMemberDetails)
 }
-
-private extension RoomMemberDetails {
-    func matches(searchQuery: String) -> Bool {
-        guard !searchQuery.isEmpty else {
-            return true
-        }
-        
-        return id.localizedCaseInsensitiveContains(searchQuery) || name?.localizedCaseInsensitiveContains(searchQuery) ?? false
-    }
-}
@@ -149,18 +149,32 @@ class RoomMembersListScreenViewModel: RoomMembersListScreenViewModelType, RoomMe
    }
    
    private func selectMember(_ member: RoomMemberDetails) {
-        if appSettings.roomModerationEnabled, state.canKickUsers || state.canBanUsers {
-            if member.isBanned {
-                state.bindings.alertInfo = AlertInfo(id: .unbanConfirmation(member),
-                                                     title: L10n.screenRoomMemberListManageMemberUnbanTitle,
-                                                     message: L10n.screenRoomMemberListManageMemberUnbanMessage,
-                                                     primaryButton: .init(title: L10n.screenRoomMemberListManageMemberUnbanAction) { [weak self] in
-                                                         self?.context.send(viewAction: .unbanMember(member))
-                                                     },
-                                                     secondaryButton: .init(title: L10n.actionCancel, role: .cancel) { })
-            } else {
-                state.bindings.memberToManage = member
-            }
+        guard appSettings.roomModerationEnabled else {
+            showMemberDetails(member)
+            return
+        }
+        
+        if member.isBanned { // No need to check canBan here, banned users are only shown when it is true.
+            state.bindings.alertInfo = AlertInfo(id: .unbanConfirmation(member),
+                                                 title: L10n.screenRoomMemberListManageMemberUnbanTitle,
+                                                 message: L10n.screenRoomMemberListManageMemberUnbanMessage,
+                                                 primaryButton: .init(title: L10n.screenRoomMemberListManageMemberUnbanAction) { [weak self] in
+                                                     self?.context.send(viewAction: .unbanMember(member))
+                                                 },
+                                                 secondaryButton: .init(title: L10n.actionCancel, role: .cancel) { })
+            return
+        }
+        
+        var actions = [RoomMembersListScreenManagementDetails.Action]()
+        if state.canKickUsers, member.role != .administrator {
+            actions.append(.kick)
+        }
+        if state.canBanUsers, member.role != .administrator {
+            actions.append(.ban)
+        }
+        
+        if !actions.isEmpty {
+            state.bindings.memberToManage = .init(member: member, actions: actions)
        } else {
            showMemberDetails(member)
        }
@@ -19,6 +19,8 @@ import SwiftUI

 struct RoomMembersListManageMemberSheet: View {
    let member: RoomMemberDetails
+    let actions: [RoomMembersListScreenManagementDetails.Action]
+    
    @ObservedObject var context: RoomMembersListScreenViewModel.Context
    
    @State private var isPresentingBanConfirmation = false
@@ -38,7 +40,7 @@ struct RoomMembersListManageMemberSheet: View {
                            context.send(viewAction: .showMemberDetails(member))
                        })
                
-                if context.viewState.canKickUsers, !member.isBanned {
+                if actions.contains(.kick) {
                    ListRow(label: .default(title: L10n.screenRoomMemberListManageMemberRemove,
                                            icon: \.close),
                            kind: .button {
@@ -46,7 +48,7 @@ struct RoomMembersListManageMemberSheet: View {
                            })
                }
                
-                if context.viewState.canBanUsers, !member.isBanned {
+                if actions.contains(.ban) {
                    ListRow(label: .default(title: L10n.screenRoomMemberListManageMemberBan,
                                            icon: \.block,
                                            role: .destructive),
@@ -76,11 +78,13 @@ struct RoomMembersListManageMemberSheet_Previews: PreviewProvider, TestablePrevi
    
    static var previews: some View {
        RoomMembersListManageMemberSheet(member: .init(withProxy: RoomMemberProxyMock.mockDan),
+                                         actions: [.kick, .ban],
                                         context: viewModel.context)
            .previewDisplayName("Joined")
            .snapshot(delay: 0.2)
        
        RoomMembersListManageMemberSheet(member: .init(withProxy: RoomMemberProxyMock.mockBanned[3]),
+                                         actions: [],
                                         context: viewModel.context)
            .previewDisplayName("Banned")
            .snapshot(delay: 0.2)
@@ -94,6 +98,7 @@ struct RoomMembersListManageMemberSheetLive_Previews: PreviewProvider {
        Color.clear
            .sheet(isPresented: .constant(true)) {
                RoomMembersListManageMemberSheet(member: .init(withProxy: RoomMemberProxyMock.mockDan),
+                                                 actions: [.kick, .ban],
                                                 context: viewModel.context)
            }
            .previewDisplayName("Sheet")
@@ -49,13 +49,15 @@ struct RoomMembersListScreen: View {
                    .background(.compound.bgCanvasDefault)
            }
        }
-        .searchable(text: $context.searchQuery, placement: .navigationBarDrawer(displayMode: .always))
+        .searchable(text: $context.searchQuery,
+                    placement: .navigationBarDrawer(displayMode: .always),
+                    prompt: L10n.commonSearchForSomeone)
        .compoundSearchField()
        .autocorrectionDisabled()
        .background(Color.compound.bgCanvasDefault.ignoresSafeArea())
        .navigationTitle(L10n.commonPeople)
        .sheet(item: $context.memberToManage) {
-            RoomMembersListManageMemberSheet(member: $0, context: context)
+            RoomMembersListManageMemberSheet(member: $0.member, actions: $0.actions, context: context)
        }
        .alert(item: $context.alertInfo)
        .toolbar { toolbar }
@@ -94,8 +94,8 @@ class RoomRolesAndPermissionsScreenViewModel: RoomRolesAndPermissionsScreenViewM
    // MARK: - Members
    
    private func updateMembers(_ members: [RoomMemberProxyProtocol]) {
-        state.administratorCount = members.filter { $0.role == .administrator }.count
-        state.moderatorCount = members.filter { $0.role == .moderator }.count
+        state.administratorCount = members.filter { $0.role == .administrator && $0.isActive }.count
+        state.moderatorCount = members.filter { $0.role == .moderator && $0.isActive }.count
    }
    
    private func updateOwnRole(_ role: RoomMemberDetails.Role) async {
@@ -23,11 +23,17 @@ struct RoomMemberDetails: Identifiable, Hashable {
    let avatarURL: URL?
    let permalink: URL?
    
+    var isInvited: Bool
    var isIgnored: Bool
    var isBanned: Bool
    
    enum Role { case administrator, moderator, user }
    let role: Role
+    
+    func matches(searchQuery: String) -> Bool {
+        guard !searchQuery.isEmpty else { return true }
+        return id.localizedStandardContains(searchQuery) || name?.localizedStandardContains(searchQuery) == true
+    }
 }

 extension RoomMemberDetails {
@@ -37,6 +43,7 @@ extension RoomMemberDetails {
        avatarURL = proxy.avatarURL
        permalink = proxy.permalink
        
+        isInvited = proxy.membership == .invite
        isIgnored = proxy.isIgnored
        isBanned = proxy.membership == .ban
        role = .init(proxy.role)
@@ -48,6 +55,7 @@ extension RoomMemberDetails {
        avatarURL = nil
        permalink = nil
        
+        isInvited = false
        isIgnored = false
        isBanned = false
        role = .user
@@ -31,6 +31,11 @@ protocol RoomMemberProxyProtocol: AnyObject {
 }

 extension RoomMemberProxyProtocol {
+    /// The member is active in the room (joined or invited).
+    var isActive: Bool {
+        membership == .join || membership == .invite
+    }
+    
    var permalink: URL? {
        try? PermalinkBuilder.permalinkTo(userIdentifier: userID,
                                          baseURL: ServiceLocator.shared.settings.permalinkBaseURL)