From 2eacce47c016d5e9346a5087c964fb8729fb2b7c Mon Sep 17 00:00:00 2001 From: Jorge Martin Espinosa Date: Thu, 4 Sep 2025 14:56:46 +0200 Subject: [PATCH] Reload member list after moderation actions (#5268) * Reload member list after moderation actions The previous `runActionAndWaitForMembershipChange` logic wasn't really doing anything, as the modified flow was never used. * Make sure we always set the value in the member list state flow, even if the underlying coroutine scope is no longer there. With `emit`, the `Ready` state was not emitted if the member list was loaded way too fast. --- .../impl/members/RoomMemberListPresenter.kt | 14 ++++----- .../members/RoomMemberListPresenterTest.kt | 3 -- .../impl/RoomMemberModerationPresenter.kt | 18 +++++++++-- .../impl/RoomMemberModerationPresenterTest.kt | 31 ++++++++++++++++--- .../impl/room/member/RoomMemberListFetcher.kt | 12 +++---- .../matrix/test/room/FakeBaseRoom.kt | 6 +++- 6 files changed, 61 insertions(+), 23 deletions(-) diff --git a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/RoomMemberListPresenter.kt b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/RoomMemberListPresenter.kt index fac98da36f..315a98248a 100644 --- a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/RoomMemberListPresenter.kt +++ b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/RoomMemberListPresenter.kt @@ -38,6 +38,7 @@ import kotlinx.collections.immutable.ImmutableMap import kotlinx.collections.immutable.persistentMapOf import kotlinx.collections.immutable.toImmutableList import kotlinx.collections.immutable.toPersistentMap +import kotlinx.coroutines.flow.collectLatest import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.launchIn @@ -66,11 +67,6 @@ class RoomMemberListPresenter @Inject constructor( val syncUpdateFlow = room.syncUpdateFlow.collectAsState() val canInvite by room.canInviteAsState(syncUpdateFlow.value) val roomModerationState = roomMembersModerationPresenter.present() - val activeRoomMemberCount by produceState(0L) { - room.roomInfoFlow.map { it.activeMembersCount } - .distinctUntilChanged() - .collect { value = it } - } val roomMemberIdentityStates by produceState(persistentMapOf()) { room.roomMemberIdentityStateChange(waitForEncryption = true) @@ -81,8 +77,12 @@ class RoomMemberListPresenter @Inject constructor( } // Update the room members when the screen is loaded or the active member count changes - LaunchedEffect(activeRoomMemberCount) { - room.updateMembers() + LaunchedEffect(Unit) { + room.roomInfoFlow.map { it.activeMembersCount } + .distinctUntilChanged() + .collectLatest { + room.updateMembers() + } } LaunchedEffect(membersState, roomMemberIdentityStates) { diff --git a/features/roomdetails/impl/src/test/kotlin/io/element/android/features/roomdetails/impl/members/RoomMemberListPresenterTest.kt b/features/roomdetails/impl/src/test/kotlin/io/element/android/features/roomdetails/impl/members/RoomMemberListPresenterTest.kt index 0491c5d437..03555fb967 100644 --- a/features/roomdetails/impl/src/test/kotlin/io/element/android/features/roomdetails/impl/members/RoomMemberListPresenterTest.kt +++ b/features/roomdetails/impl/src/test/kotlin/io/element/android/features/roomdetails/impl/members/RoomMemberListPresenterTest.kt @@ -116,9 +116,6 @@ class RoomMemberListPresenterTest { } } - // Wait for the update to be processed - skipItems(1) - // Update the room members state as `Room.updateMembers()` would have done with the actual implementation room.givenRoomMembersState(RoomMembersState.Ready(persistentListOf())) // Wait for another update diff --git a/features/roommembermoderation/impl/src/main/kotlin/io/element/android/features/roommembermoderation/impl/RoomMemberModerationPresenter.kt b/features/roommembermoderation/impl/src/main/kotlin/io/element/android/features/roommembermoderation/impl/RoomMemberModerationPresenter.kt index f0861a735e..71b7640e66 100644 --- a/features/roommembermoderation/impl/src/main/kotlin/io/element/android/features/roommembermoderation/impl/RoomMemberModerationPresenter.kt +++ b/features/roommembermoderation/impl/src/main/kotlin/io/element/android/features/roommembermoderation/impl/RoomMemberModerationPresenter.kt @@ -38,10 +38,12 @@ import kotlinx.collections.immutable.PersistentList import kotlinx.collections.immutable.persistentListOf import kotlinx.collections.immutable.toPersistentList import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.delay import kotlinx.coroutines.flow.drop -import kotlinx.coroutines.flow.take +import kotlinx.coroutines.flow.first import kotlinx.coroutines.launch import javax.inject.Inject +import kotlin.time.Duration.Companion.milliseconds class RoomMemberModerationPresenter @Inject constructor( private val room: JoinedRoom, @@ -82,6 +84,9 @@ class RoomMemberModerationPresenter @Inject constructor( ) } is RoomMemberModerationEvents.ProcessAction -> { + // First, hide any list of existing actions that could be displayed + moderationActions.value = persistentListOf() + when (event.action) { is ModerationAction.DisplayProfile -> Unit is ModerationAction.KickUser -> { @@ -118,6 +123,7 @@ class RoomMemberModerationPresenter @Inject constructor( } is InternalRoomMemberModerationEvents.Reset -> { selectedUser = null + moderationActions.value = persistentListOf() kickUserAsyncAction.value = AsyncAction.Uninitialized banUserAsyncAction.value = AsyncAction.Uninitialized unbanUserAsyncAction.value = AsyncAction.Uninitialized @@ -204,7 +210,15 @@ class RoomMemberModerationPresenter @Inject constructor( action.runUpdatingState { val result = block() if (result.isSuccess) { - room.membersStateFlow.drop(1).take(1) + // We wait a bit to ensure the server has processed the membership change + delay(50.milliseconds) + + // Update the members to ensure we have the latest state + launch { room.updateMembers() } + + // Wait for the membership change to be processed and returned + // We drop the first emission as it's the current state + room.membersStateFlow.drop(1).first() } result } diff --git a/features/roommembermoderation/impl/src/test/kotlin/io/element/android/features/roommembermoderation/impl/RoomMemberModerationPresenterTest.kt b/features/roommembermoderation/impl/src/test/kotlin/io/element/android/features/roommembermoderation/impl/RoomMemberModerationPresenterTest.kt index 2d1bf77fe0..3b647c6478 100644 --- a/features/roommembermoderation/impl/src/test/kotlin/io/element/android/features/roommembermoderation/impl/RoomMemberModerationPresenterTest.kt +++ b/features/roommembermoderation/impl/src/test/kotlin/io/element/android/features/roommembermoderation/impl/RoomMemberModerationPresenterTest.kt @@ -29,6 +29,7 @@ import io.element.android.services.analytics.test.FakeAnalyticsService import io.element.android.tests.testutils.WarmUpRule import io.element.android.tests.testutils.test import io.element.android.tests.testutils.testCoroutineDispatchers +import kotlinx.collections.immutable.persistentListOf import kotlinx.collections.immutable.toPersistentList import kotlinx.coroutines.test.TestScope import kotlinx.coroutines.test.runTest @@ -217,7 +218,14 @@ class RoomMemberModerationPresenterTest { @Test fun `present - do kick user with success`() = runTest { - createRoomMemberModerationPresenter(room = aJoinedRoom()).test { + val room = aJoinedRoom() + room.baseRoom.givenUpdateMembersResult { + // Simulate the member list being updated + room.givenRoomMembersState(RoomMembersState.Ready( + persistentListOf(aRoomMember()) + )) + } + createRoomMemberModerationPresenter(room = room).test { val initialState = awaitState() initialState.eventSink( RoomMemberModerationEvents.ProcessAction( @@ -238,7 +246,14 @@ class RoomMemberModerationPresenterTest { @Test fun `present - do ban user with success`() = runTest { - createRoomMemberModerationPresenter(room = aJoinedRoom()).test { + val room = aJoinedRoom() + room.baseRoom.givenUpdateMembersResult { + // Simulate the member list being updated + room.givenRoomMembersState(RoomMembersState.Ready( + persistentListOf(aRoomMember()) + )) + } + createRoomMemberModerationPresenter(room = room).test { val initialState = awaitState() initialState.eventSink( RoomMemberModerationEvents.ProcessAction( @@ -259,7 +274,14 @@ class RoomMemberModerationPresenterTest { @Test fun `present - do unban user with success`() = runTest { - createRoomMemberModerationPresenter(room = aJoinedRoom()).test { + val room = aJoinedRoom() + room.baseRoom.givenUpdateMembersResult { + // Simulate the member list being updated + room.givenRoomMembersState(RoomMembersState.Ready( + persistentListOf(aRoomMember()) + )) + } + createRoomMemberModerationPresenter(room = room).test { val initialState = awaitState() initialState.eventSink( RoomMemberModerationEvents.ProcessAction( @@ -326,7 +348,7 @@ class RoomMemberModerationPresenterTest { banUserResult: Result = Result.success(Unit), unBanUserResult: Result = Result.success(Unit), targetRoomMember: RoomMember? = null, - ): JoinedRoom { + ): FakeJoinedRoom { return FakeJoinedRoom( kickUserResult = { _, _ -> kickUserResult }, banUserResult = { _, _ -> banUserResult }, @@ -335,6 +357,7 @@ class RoomMemberModerationPresenterTest { canBanResult = { _ -> Result.success(canBan) }, canKickResult = { _ -> Result.success(canKick) }, userRoleResult = { Result.success(myUserRole) }, + updateMembersResult = { Result.success(Unit) } ), ).apply { val roomMembers = listOfNotNull(targetRoomMember).toPersistentList() diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/member/RoomMemberListFetcher.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/member/RoomMemberListFetcher.kt index 187bead11e..fc394e4ce7 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/member/RoomMemberListFetcher.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/member/RoomMemberListFetcher.kt @@ -79,35 +79,35 @@ internal class RoomMemberListFetcher( Timber.i("Loading cached members for room $roomId") try { // Send current member list with pending state to notify the UI that we are loading new members - emit(pendingWithCurrentMembers()) + value = pendingWithCurrentMembers() val members = parseAndEmitMembers(room.membersNoSync()) val newState = if (asPendingState) { RoomMembersState.Pending(prevRoomMembers = members) } else { RoomMembersState.Ready(members) } - emit(newState) + value = newState } catch (exception: CancellationException) { Timber.d("Cancelled loading cached members for room $roomId") throw exception } catch (exception: Exception) { Timber.e(exception, "Failed to load cached members for room $roomId") - emit(RoomMembersState.Error(exception, _membersFlow.value.roomMembers()?.toImmutableList())) + value = RoomMembersState.Error(exception, _membersFlow.value.roomMembers()?.toImmutableList()) } } private suspend fun MutableStateFlow.fetchRemoteRoomMembers() { try { // Send current member list with pending state to notify the UI that we are loading new members - emit(pendingWithCurrentMembers()) + value = pendingWithCurrentMembers() // Start loading new members - emit(RoomMembersState.Ready(parseAndEmitMembers(room.members()))) + value = RoomMembersState.Ready(parseAndEmitMembers(room.members())) } catch (exception: CancellationException) { Timber.d("Cancelled loading updated members for room $roomId") throw exception } catch (exception: Exception) { Timber.e(exception, "Failed to load updated members for room $roomId") - emit(RoomMembersState.Error(exception, _membersFlow.value.roomMembers()?.toImmutableList())) + value = RoomMembersState.Error(exception, _membersFlow.value.roomMembers()?.toImmutableList()) } } diff --git a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/room/FakeBaseRoom.kt b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/room/FakeBaseRoom.kt index 872481a860..2a61c48e50 100644 --- a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/room/FakeBaseRoom.kt +++ b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/room/FakeBaseRoom.kt @@ -59,7 +59,7 @@ class FakeBaseRoom( private val markAsReadResult: (ReceiptType) -> Result = { Result.success(Unit) }, private val powerLevelsResult: () -> Result = { lambdaError() }, private val leaveRoomLambda: () -> Result = { lambdaError() }, - private val updateMembersResult: () -> Unit = { lambdaError() }, + private var updateMembersResult: () -> Unit = { lambdaError() }, private val getMembersResult: (Int) -> Result> = { lambdaError() }, private val saveComposerDraftLambda: (ComposerDraft) -> Result = { _: ComposerDraft -> Result.success(Unit) }, private val loadComposerDraftLambda: () -> Result = { Result.success(null) }, @@ -223,6 +223,10 @@ class FakeBaseRoom( override suspend fun reportRoom(reason: String?) = reportRoomResult(reason) override fun predecessorRoom(): PredecessorRoom? = predecessorRoomResult() + + fun givenUpdateMembersResult(result: () -> Unit) { + updateMembersResult = result + } } fun defaultRoomPowerLevelValues() = RoomPowerLevelsValues(