Skip to content

Commit 1410563

Browse files
ganfraganfra
authored
Merge pull request #5832 from element-hq/feature/fga/fix_owner_admin_displayed_twice
fix: usersWithRole(Owner) returns creators only if privilegedCreatorRole is true
2 parents e9e699e + a371fe9 commit 1410563

File tree

10 files changed

+84
-122
lines changed

10 files changed

+84
-122
lines changed

‎features/leaveroom/impl/src/main/kotlin/io/element/android/features/leaveroom/impl/LeaveRoomPresenter.kt‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -96,10 +96,7 @@ class LeaveRoomPresenter(
9696
} else {
9797
val hasPrivilegedCreatorRole = roomInfoFlow.value.privilegedCreatorRole
9898
if (!hasPrivilegedCreatorRole) return false
99-
100-
val creators = usersWithRole(RoomMember.Role.Owner(isCreator = true)).first()
101-
val superAdmins = usersWithRole(RoomMember.Role.Owner(isCreator = false)).first()
102-
val owners = creators + superAdmins
99+
val owners = usersWithRole { role -> role is RoomMember.Role.Owner }.first()
103100
return owners.size == 1 && owners.first().userId == sessionId
104101
}
105102
}

‎features/rolesandpermissions/impl/src/main/kotlin/io/element/android/features/rolesandpermissions/impl/roles/ChangeRolesPresenter.kt‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -79,18 +79,13 @@ class ChangeRolesPresenter(
7979
val usersWithRole = produceState<ImmutableList<MatrixUser>>(initialValue = persistentListOf()) {
8080
// If the role is admin, we need to include the owners as well since they implicitly have admin role
8181
val owners = if (role == RoomMember.Role.Admin) {
82-
combine(
83-
room.usersWithRole(RoomMember.Role.Owner(isCreator = true)),
84-
room.usersWithRole(RoomMember.Role.Owner(isCreator = false)),
85-
) { creators, superAdmins ->
86-
creators + superAdmins
87-
}
82+
room.usersWithRole { role -> role is RoomMember.Role.Owner }
8883
} else {
8984
emptyFlow()
9085
}
9186
combine(
9287
owners,
93-
room.usersWithRole(role),
88+
room.usersWithRole { it == role },
9489
) { owners, users ->
9590
owners + users
9691
}.map { members -> members.map { it.toMatrixUser() } }

‎features/rolesandpermissions/impl/src/main/kotlin/io/element/android/features/rolesandpermissions/impl/root/RolesAndPermissionsPresenter.kt‎

Lines changed: 7 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,10 @@ import io.element.android.libraries.architecture.AsyncAction
2222
import io.element.android.libraries.architecture.Presenter
2323
import io.element.android.libraries.architecture.runUpdatingState
2424
import io.element.android.libraries.core.coroutine.CoroutineDispatchers
25-
import io.element.android.libraries.matrix.api.core.UserId
2625
import io.element.android.libraries.matrix.api.room.JoinedRoom
27-
import io.element.android.libraries.matrix.api.room.RoomInfo
2826
import io.element.android.libraries.matrix.api.room.RoomMember
29-
import io.element.android.libraries.matrix.api.room.activeRoomMembers
3027
import io.element.android.libraries.matrix.api.room.powerlevels.UserRoleChange
28+
import io.element.android.libraries.matrix.api.room.powerlevels.userCountWithRole
3129
import io.element.android.libraries.matrix.ui.model.roleOf
3230
import io.element.android.services.analytics.api.AnalyticsService
3331
import kotlinx.coroutines.CoroutineScope
@@ -43,32 +41,14 @@ class RolesAndPermissionsPresenter(
4341
override fun present(): RolesAndPermissionsState {
4442
val coroutineScope = rememberCoroutineScope()
4543
val roomInfo by room.roomInfoFlow.collectAsState()
46-
val roomMembers by room.membersStateFlow.collectAsState()
47-
// Get the list of active room members (joined or invited), in order to filter members present in the power
48-
// level state Event.
49-
val activeRoomMemberIds by remember {
50-
derivedStateOf {
51-
roomMembers.activeRoomMembers().map { it.userId }
52-
}
53-
}
5444
val moderatorCount by remember {
55-
derivedStateOf {
56-
roomInfo.userCountWithRole(activeRoomMemberIds, RoomMember.Role.Moderator)
57-
}
58-
}
45+
room.userCountWithRole { role -> role is RoomMember.Role.Moderator }
46+
}.collectAsState(null)
47+
5948
val adminCount by remember {
60-
derivedStateOf {
61-
val admins = roomInfo.userCountWithRole(activeRoomMemberIds, RoomMember.Role.Admin)
62-
val ownersCount = if (roomInfo.privilegedCreatorRole) {
63-
val superAdmins = roomInfo.userCountWithRole(activeRoomMemberIds, RoomMember.Role.Owner(isCreator = false))
64-
val creators = roomInfo.userCountWithRole(activeRoomMemberIds, RoomMember.Role.Owner(isCreator = true))
65-
superAdmins + creators
66-
} else {
67-
0
68-
}
69-
admins + ownersCount
70-
}
71-
}
49+
room.userCountWithRole { role -> role is RoomMember.Role.Admin || role is RoomMember.Role.Owner }
50+
}.collectAsState(null)
51+
7252
val canDemoteSelf = remember { derivedStateOf { roomInfo.roleOf(room.sessionId) !is RoomMember.Role.Owner } }
7353
val changeOwnRoleAction = remember { mutableStateOf<AsyncAction<Unit>>(AsyncAction.Uninitialized) }
7454
val resetPermissionsAction = remember { mutableStateOf<AsyncAction<Unit>>(AsyncAction.Uninitialized) }
@@ -122,8 +102,4 @@ class RolesAndPermissionsPresenter(
122102
room.resetPowerLevels()
123103
}
124104
}
125-
126-
private fun RoomInfo.userCountWithRole(userIds: List<UserId>, role: RoomMember.Role): Int {
127-
return usersWithRole(role).filter { it in userIds }.size
128-
}
129105
}

‎features/rolesandpermissions/impl/src/main/kotlin/io/element/android/features/rolesandpermissions/impl/root/RolesAndPermissionsState.kt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,8 @@ import io.element.android.libraries.architecture.AsyncAction
1212

1313
data class RolesAndPermissionsState(
1414
val roomSupportsOwnerRole: Boolean,
15-
val adminCount: Int,
16-
val moderatorCount: Int,
15+
val adminCount: Int?,
16+
val moderatorCount: Int?,
1717
val canDemoteSelf: Boolean,
1818
val changeOwnRoleAction: AsyncAction<Unit>,
1919
val resetPermissionsAction: AsyncAction<Unit>,

‎features/rolesandpermissions/impl/src/main/kotlin/io/element/android/features/rolesandpermissions/impl/root/RolesAndPermissionsView.kt‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,13 +63,17 @@ fun RolesAndPermissionsView(
6363
ListItem(
6464
headlineContent = { Text(adminsTitle) },
6565
leadingContent = ListItemContent.Icon(IconSource.Vector(CompoundIcons.Admin())),
66-
trailingContent = ListItemContent.Text("${state.adminCount}"),
66+
trailingContent = state.adminCount?.let { adminCount ->
67+
ListItemContent.Text("$adminCount")
68+
},
6769
onClick = { rolesAndPermissionsNavigator.openAdminList() },
6870
)
6971
ListItem(
7072
headlineContent = { Text(stringResource(R.string.screen_room_roles_and_permissions_moderators)) },
7173
leadingContent = ListItemContent.Icon(IconSource.Vector(CompoundIcons.ChatProblem())),
72-
trailingContent = ListItemContent.Text("${state.moderatorCount}"),
74+
trailingContent = state.moderatorCount?.let { moderationCount ->
75+
ListItemContent.Text("$moderationCount")
76+
},
7377
onClick = { rolesAndPermissionsNavigator.openModeratorList() },
7478
)
7579
if (state.canDemoteSelf) {

‎features/rolesandpermissions/impl/src/test/kotlin/io/element/android/features/rolesandpermissions/impl/roles/ChangeRolesPresenterTest.kt‎

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -18,19 +18,23 @@ import io.element.android.libraries.matrix.api.core.UserId
1818
import io.element.android.libraries.matrix.api.room.RoomMember
1919
import io.element.android.libraries.matrix.api.room.RoomMembersState
2020
import io.element.android.libraries.matrix.api.room.powerlevels.RoomPowerLevels
21+
import io.element.android.libraries.matrix.api.room.toMatrixUser
2122
import io.element.android.libraries.matrix.api.user.MatrixUser
23+
import io.element.android.libraries.matrix.test.A_SESSION_ID
2224
import io.element.android.libraries.matrix.test.A_USER_ID
2325
import io.element.android.libraries.matrix.test.A_USER_ID_2
2426
import io.element.android.libraries.matrix.test.A_USER_ID_3
2527
import io.element.android.libraries.matrix.test.room.FakeBaseRoom
2628
import io.element.android.libraries.matrix.test.room.FakeJoinedRoom
2729
import io.element.android.libraries.matrix.test.room.aRoomInfo
2830
import io.element.android.libraries.matrix.test.room.aRoomMember
31+
import io.element.android.libraries.matrix.test.room.anAlice
2932
import io.element.android.libraries.matrix.test.room.defaultRoomPowerLevelValues
3033
import io.element.android.libraries.previewutils.room.aRoomMemberList
3134
import io.element.android.services.analytics.test.FakeAnalyticsService
3235
import io.element.android.tests.testutils.test
3336
import io.element.android.tests.testutils.testCoroutineDispatchers
37+
import kotlinx.collections.immutable.persistentListOf
3438
import kotlinx.collections.immutable.persistentMapOf
3539
import kotlinx.collections.immutable.toImmutableList
3640
import kotlinx.collections.immutable.toImmutableMap
@@ -63,7 +67,7 @@ class ChangeRolesPresenterTest {
6367
}
6468
val presenter = createChangeRolesPresenter(room = room)
6569
presenter.test {
66-
skipItems(1)
70+
skipItems(2)
6771
assertThat(awaitItem().searchResults).isInstanceOf(SearchBarResultState.Results::class.java)
6872
}
6973
}
@@ -161,13 +165,13 @@ class ChangeRolesPresenterTest {
161165
}
162166

163167
@Test
164-
fun `present - when modifying admins, creators are displayed too`() = runTest {
168+
fun `present - when modifying admins, creators are displayed too - privilegedCreatorRole is true`() = runTest {
165169
val room = FakeJoinedRoom().apply {
166170
val creatorUserId = UserId("@creator:matrix.org")
167171
val memberList = aRoomMemberList()
168172
.plus(aRoomMember(displayName = "CREATOR", role = RoomMember.Role.Owner(isCreator = true), userId = creatorUserId))
169173
.toImmutableList()
170-
givenRoomInfo(aRoomInfo(roomCreators = listOf(creatorUserId)))
174+
givenRoomInfo(aRoomInfo(roomCreators = listOf(creatorUserId), privilegedCreatorRole = true))
171175
givenRoomMembersState(RoomMembersState.Ready(memberList))
172176
}
173177
val presenter = createChangeRolesPresenter(room = room)
@@ -190,6 +194,7 @@ class ChangeRolesPresenterTest {
190194
}
191195
val presenter = createChangeRolesPresenter(room = room)
192196
presenter.test {
197+
skipItems(1)
193198
val initialState = awaitItem()
194199

195200
initialState.eventSink(ChangeRolesEvent.ToggleSearchActive)
@@ -207,6 +212,7 @@ class ChangeRolesPresenterTest {
207212
}
208213
val presenter = createChangeRolesPresenter(room = room)
209214
presenter.test {
215+
skipItems(1)
210216
val initialState = awaitItem()
211217
val initialResults = (awaitItem().searchResults as? SearchBarResultState.Results)?.results
212218
assertThat(initialResults?.members).hasSize(8)
@@ -231,7 +237,7 @@ class ChangeRolesPresenterTest {
231237
}
232238
val presenter = createChangeRolesPresenter(room = room)
233239
presenter.test {
234-
skipItems(1)
240+
skipItems(2)
235241
val initialResults = (awaitItem().searchResults as? SearchBarResultState.Results)?.results
236242
assertThat(initialResults?.members).hasSize(8)
237243
assertThat(initialResults?.moderators).hasSize(1)
@@ -478,17 +484,14 @@ class ChangeRolesPresenterTest {
478484
@Test
479485
fun `present - Save will just save the changes if the current user is a room creator and the selected users are not`() = runTest {
480486
val analyticsService = FakeAnalyticsService()
487+
val alice = anAlice()
488+
val me = aRoomMember(displayName = "CREATOR", role = RoomMember.Role.Owner(isCreator = true), userId = A_SESSION_ID)
481489
val room = FakeJoinedRoom(
482490
updateUserRoleResult = { Result.success(Unit) },
483491
baseRoom = FakeBaseRoom(updateMembersResult = { Result.success(Unit) }),
484492
).apply {
485-
givenRoomMembersState(RoomMembersState.Ready(aRoomMemberList()))
486-
givenRoomInfo(
487-
aRoomInfo(
488-
roomCreators = listOf(sessionId),
489-
roomPowerLevels = roomPowerLevelsWithRole(role = RoomMember.Role.Admin, userId = A_USER_ID_2)
490-
)
491-
)
493+
val roomMemberList = persistentListOf(alice, me)
494+
givenRoomMembersState(RoomMembersState.Ready(roomMemberList))
492495
}
493496
val presenter = createChangeRolesPresenter(
494497
role = RoomMember.Role.Admin,
@@ -499,7 +502,7 @@ class ChangeRolesPresenterTest {
499502
skipItems(2)
500503
val initialState = awaitItem()
501504
assertThat(initialState.selectedUsers).hasSize(2)
502-
initialState.eventSink(ChangeRolesEvent.UserSelectionToggled(MatrixUser(A_USER_ID_2)))
505+
initialState.eventSink(ChangeRolesEvent.UserSelectionToggled(alice.toMatrixUser()))
503506
awaitItem().also {
504507
assertThat(it.selectedUsers).hasSize(1)
505508
it.eventSink(ChangeRolesEvent.Save)

‎features/rolesandpermissions/impl/src/test/kotlin/io/element/android/features/rolesandpermissions/impl/root/RolesAndPermissionPresenterTest.kt‎

Lines changed: 35 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -8,16 +8,17 @@
88

99
package io.element.android.features.rolesandpermissions.impl.root
1010

11-
import app.cash.molecule.RecompositionMode
12-
import app.cash.molecule.moleculeFlow
13-
import app.cash.turbine.test
1411
import com.google.common.truth.Truth.assertThat
1512
import im.vector.app.features.analytics.plan.RoomModeration
1613
import io.element.android.libraries.architecture.AsyncAction
1714
import io.element.android.libraries.core.coroutine.CoroutineDispatchers
1815
import io.element.android.libraries.matrix.api.room.RoomMember
16+
import io.element.android.libraries.matrix.api.room.RoomMembersState
17+
import io.element.android.libraries.matrix.test.room.FakeBaseRoom
1918
import io.element.android.libraries.matrix.test.room.FakeJoinedRoom
19+
import io.element.android.libraries.matrix.test.room.aRoomMemberList
2020
import io.element.android.services.analytics.test.FakeAnalyticsService
21+
import io.element.android.tests.testutils.test
2122
import io.element.android.tests.testutils.testCoroutineDispatchers
2223
import kotlinx.coroutines.ExperimentalCoroutinesApi
2324
import kotlinx.coroutines.test.StandardTestDispatcher
@@ -30,12 +31,10 @@ class RolesAndPermissionPresenterTest {
3031
@Test
3132
fun `present - initial state`() = runTest {
3233
val presenter = createRolesAndPermissionsPresenter()
33-
moleculeFlow(RecompositionMode.Immediate) {
34-
presenter.present()
35-
}.test {
34+
presenter.test {
3635
with(awaitItem()) {
37-
assertThat(adminCount).isEqualTo(0)
38-
assertThat(moderatorCount).isEqualTo(0)
36+
assertThat(adminCount).isNull()
37+
assertThat(moderatorCount).isNull()
3938
assertThat(changeOwnRoleAction).isEqualTo(AsyncAction.Uninitialized)
4039
}
4140
}
@@ -44,12 +43,9 @@ class RolesAndPermissionPresenterTest {
4443
@Test
4544
fun `present - ChangeOwnRole presents a confirmation dialog`() = runTest {
4645
val presenter = createRolesAndPermissionsPresenter()
47-
moleculeFlow(RecompositionMode.Immediate) {
48-
presenter.present()
49-
}.test {
46+
presenter.test {
5047
val initialState = awaitItem()
5148
initialState.eventSink(RolesAndPermissionsEvents.ChangeOwnRole)
52-
5349
assertThat(awaitItem().changeOwnRoleAction).isEqualTo(AsyncAction.ConfirmingNoParams)
5450
}
5551
}
@@ -60,12 +56,11 @@ class RolesAndPermissionPresenterTest {
6056
val presenter = createRolesAndPermissionsPresenter(
6157
dispatchers = testCoroutineDispatchers(),
6258
room = FakeJoinedRoom(
59+
baseRoom = FakeBaseRoom(updateMembersResult = {}),
6360
updateUserRoleResult = { Result.success(Unit) }
6461
),
6562
)
66-
moleculeFlow(RecompositionMode.Immediate) {
67-
presenter.present()
68-
}.test {
63+
presenter.test {
6964
val initialState = awaitItem()
7065
initialState.eventSink(RolesAndPermissionsEvents.DemoteSelfTo(RoomMember.Role.Moderator))
7166

@@ -81,12 +76,11 @@ class RolesAndPermissionPresenterTest {
8176
@Test
8277
fun `present - DemoteSelfTo can handle failures and clean them`() = runTest(StandardTestDispatcher()) {
8378
val room = FakeJoinedRoom(
79+
baseRoom = FakeBaseRoom(updateMembersResult = {}),
8480
updateUserRoleResult = { Result.failure(Exception("Failed to update role")) }
8581
)
8682
val presenter = createRolesAndPermissionsPresenter(room = room, dispatchers = testCoroutineDispatchers())
87-
moleculeFlow(RecompositionMode.Immediate) {
88-
presenter.present()
89-
}.test {
83+
presenter.test {
9084
val initialState = awaitItem()
9185
initialState.eventSink(RolesAndPermissionsEvents.DemoteSelfTo(RoomMember.Role.Moderator))
9286

@@ -104,9 +98,7 @@ class RolesAndPermissionPresenterTest {
10498
@Test
10599
fun `present - CancelPendingAction dismisses confirmation dialog too`() = runTest {
106100
val presenter = createRolesAndPermissionsPresenter()
107-
moleculeFlow(RecompositionMode.Immediate) {
108-
presenter.present()
109-
}.test {
101+
presenter.test {
110102
val initialState = awaitItem()
111103
initialState.eventSink(RolesAndPermissionsEvents.ChangeOwnRole)
112104
awaitItem().eventSink(RolesAndPermissionsEvents.CancelPendingAction)
@@ -121,12 +113,11 @@ class RolesAndPermissionPresenterTest {
121113
val presenter = createRolesAndPermissionsPresenter(
122114
analyticsService = analyticsService,
123115
room = FakeJoinedRoom(
116+
baseRoom = FakeBaseRoom(updateMembersResult = {}),
124117
resetPowerLevelsResult = { Result.success(Unit) }
125118
)
126119
)
127-
moleculeFlow(RecompositionMode.Immediate) {
128-
presenter.present()
129-
}.test {
120+
presenter.test {
130121
val initialState = awaitItem()
131122
initialState.eventSink(RolesAndPermissionsEvents.ResetPermissions)
132123
// Confirmation
@@ -141,9 +132,7 @@ class RolesAndPermissionPresenterTest {
141132
@Test
142133
fun `present - ResetPermissions confirmation can be cancelled`() = runTest {
143134
val presenter = createRolesAndPermissionsPresenter()
144-
moleculeFlow(RecompositionMode.Immediate) {
145-
presenter.present()
146-
}.test {
135+
presenter.test {
147136
val initialState = awaitItem()
148137
initialState.eventSink(RolesAndPermissionsEvents.ResetPermissions)
149138
awaitItem().eventSink(RolesAndPermissionsEvents.CancelPendingAction)
@@ -152,8 +141,26 @@ class RolesAndPermissionPresenterTest {
152141
}
153142
}
154143

144+
@Test
145+
fun `present - admins and moderator counts are updated when members changes`() = runTest {
146+
val room = FakeJoinedRoom(
147+
baseRoom = FakeBaseRoom(updateMembersResult = {}),
148+
)
149+
val presenter = createRolesAndPermissionsPresenter(room = room)
150+
presenter.test {
151+
val initialState = awaitItem()
152+
assertThat(initialState.adminCount).isNull()
153+
assertThat(initialState.moderatorCount).isNull()
154+
room.givenRoomMembersState(state = RoomMembersState.Ready(aRoomMemberList()))
155+
skipItems(1)
156+
val finalState = awaitItem()
157+
assertThat(finalState.adminCount).isEqualTo(1)
158+
assertThat(finalState.moderatorCount).isEqualTo(1)
159+
}
160+
}
161+
155162
private fun TestScope.createRolesAndPermissionsPresenter(
156-
room: FakeJoinedRoom = FakeJoinedRoom(),
163+
room: FakeJoinedRoom = FakeJoinedRoom(baseRoom = FakeBaseRoom(updateMembersResult = {})),
157164
dispatchers: CoroutineDispatchers = testCoroutineDispatchers(),
158165
analyticsService: FakeAnalyticsService = FakeAnalyticsService()
159166
): RolesAndPermissionsPresenter {

‎libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/RoomInfo.kt‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -79,15 +79,4 @@ data class RoomInfo(
7979
) {
8080
val aliases: List<RoomAlias>
8181
get() = listOfNotNull(canonicalAlias) + alternativeAliases
82-
83-
/**
84-
* Returns the list of users with the given [role] in this room.
85-
*/
86-
fun usersWithRole(role: RoomMember.Role): List<UserId> {
87-
return if (role is RoomMember.Role.Owner && role.isCreator) {
88-
this.creators
89-
} else {
90-
this.roomPowerLevels?.usersWithRole(role).orEmpty().toList()
91-
}
92-
}
9382
}

0 commit comments

Comments
 (0)