From a123580bb383de3fca711640b31414326f996add Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Saleniuk?= Date: Fri, 26 Apr 2024 14:11:31 +0200 Subject: [PATCH] fix: get identities only for newly emitted members [WPB-8753] --- ...serveParticipantsForConversationUseCase.kt | 20 +++- ...eParticipantsForConversationUseCaseTest.kt | 98 ++++++++++++++++++- 2 files changed, 114 insertions(+), 4 deletions(-) diff --git a/app/src/main/kotlin/com/wire/android/ui/home/conversations/details/participants/usecase/ObserveParticipantsForConversationUseCase.kt b/app/src/main/kotlin/com/wire/android/ui/home/conversations/details/participants/usecase/ObserveParticipantsForConversationUseCase.kt index ad0121a9681..5819b7dcbb0 100644 --- a/app/src/main/kotlin/com/wire/android/ui/home/conversations/details/participants/usecase/ObserveParticipantsForConversationUseCase.kt +++ b/app/src/main/kotlin/com/wire/android/ui/home/conversations/details/participants/usecase/ObserveParticipantsForConversationUseCase.kt @@ -27,12 +27,16 @@ import com.wire.kalium.logic.data.conversation.Conversation.Member import com.wire.kalium.logic.data.id.ConversationId import com.wire.kalium.logic.data.user.OtherUser import com.wire.kalium.logic.data.user.SelfUser +import com.wire.kalium.logic.data.user.UserId import com.wire.kalium.logic.data.user.type.UserType import com.wire.kalium.logic.feature.conversation.ObserveConversationMembersUseCase +import com.wire.kalium.logic.feature.e2ei.CertificateStatus import com.wire.kalium.logic.feature.e2ei.usecase.GetMembersE2EICertificateStatusesUseCase import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.drop import kotlinx.coroutines.flow.flowOn import kotlinx.coroutines.flow.map +import kotlinx.coroutines.flow.scan import javax.inject.Inject class ObserveParticipantsForConversationUseCase @Inject constructor( @@ -50,7 +54,9 @@ class ObserveParticipantsForConversationUseCase @Inject constructor( isAdmin && !isService } } - .map { sortedMemberList -> + .scan( + ConversationParticipantsData() to emptyMap() + ) { (_, previousMlsVerificationMap), sortedMemberList -> val allAdminsWithoutServices = sortedMemberList.getOrDefault(true, listOf()) val visibleAdminsWithoutServices = allAdminsWithoutServices.limit(limit) val allParticipants = sortedMemberList.getOrDefault(false, listOf()) @@ -59,7 +65,13 @@ class ObserveParticipantsForConversationUseCase @Inject constructor( val visibleUserIds = visibleParticipants.map { it.userId } .plus(visibleAdminsWithoutServices.map { it.userId }) - val mlsVerificationMap = getMembersE2EICertificateStatuses(conversationId, visibleUserIds) + // only fetch certificate statuses for newly emitted users and get the rest from previous iterations + val newlyEmittedVisibleUserIds = visibleUserIds - previousMlsVerificationMap.keys + val mlsVerificationMap = previousMlsVerificationMap.plus( + if (newlyEmittedVisibleUserIds.isEmpty()) emptyMap() + else getMembersE2EICertificateStatuses(conversationId, newlyEmittedVisibleUserIds) + ) + ConversationParticipantsData( admins = visibleAdminsWithoutServices .map { uiParticipantMapper.toUIParticipant(it.user, mlsVerificationMap[it.user.id]) }, @@ -68,8 +80,10 @@ class ObserveParticipantsForConversationUseCase @Inject constructor( allAdminsCount = allAdminsWithoutServices.size, allParticipantsCount = allParticipants.size, isSelfAnAdmin = allAdminsWithoutServices.any { it.user is SelfUser } - ) + ) to mlsVerificationMap } + .drop(1) // ignore the initial value from scan + .map { (data, _) -> data } .flowOn(dispatchers.io()) private fun List.limit(limit: Int = -1) = when { diff --git a/app/src/test/kotlin/com/wire/android/ui/home/conversations/details/participants/usecase/ObserveParticipantsForConversationUseCaseTest.kt b/app/src/test/kotlin/com/wire/android/ui/home/conversations/details/participants/usecase/ObserveParticipantsForConversationUseCaseTest.kt index 000ef758f72..13ced313138 100644 --- a/app/src/test/kotlin/com/wire/android/ui/home/conversations/details/participants/usecase/ObserveParticipantsForConversationUseCaseTest.kt +++ b/app/src/test/kotlin/com/wire/android/ui/home/conversations/details/participants/usecase/ObserveParticipantsForConversationUseCaseTest.kt @@ -27,16 +27,19 @@ import com.wire.android.util.ui.WireSessionImageLoader import com.wire.kalium.logic.data.conversation.Conversation.Member import com.wire.kalium.logic.data.conversation.MemberDetails import com.wire.kalium.logic.data.id.ConversationId +import com.wire.kalium.logic.data.user.UserId import com.wire.kalium.logic.data.user.type.UserType import com.wire.kalium.logic.feature.conversation.ObserveConversationMembersUseCase import com.wire.kalium.logic.feature.e2ei.usecase.GetMembersE2EICertificateStatusesUseCase import io.mockk.MockKAnnotations import io.mockk.coEvery +import io.mockk.coVerify import io.mockk.impl.annotations.MockK import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.channels.Channel import kotlinx.coroutines.flow.consumeAsFlow import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.test.advanceUntilIdle import kotlinx.coroutines.test.runTest import org.amshove.kluent.internal.assertEquals import org.junit.jupiter.api.Test @@ -82,6 +85,99 @@ class ObserveParticipantsForConversationUseCaseTest { assert(data.allParticipantsCount == members.size) } } + + @Test + fun givenGroup_whenVisibleMembersListChanges_thenGetE2EICertificateOnlyForNewOnes() = runTest { + // Given + val members: List = buildList { + for (i in 1..6) { + add(MemberDetails(testOtherUser(i).copy(userType = UserType.INTERNAL), Member.Role.Member)) + } + } + val members1 = members.subList(0, 4) + val members2 = members.subList(4, 6) + val (arrangement, useCase) = ObserveParticipantsForConversationUseCaseArrangement() + .withConversationParticipantsUpdate(members1) + .arrange() + // When - Then + useCase(ConversationId("", "")).test { + val result1 = awaitItem() + assertEquals(result1.participants.size, members1.size) + assertEquals(result1.allParticipantsCount, members1.size) + // emit updated members list with new members that should also be visible because there is no limit set + arrangement.withConversationParticipantsUpdate(members1 + members2) + advanceUntilIdle() + val result2 = awaitItem() + assertEquals(result2.participants.size, members1.size + members2.size) + assertEquals(result2.allParticipantsCount, members1.size + members2.size) + + // certificate statuses are fetched once for the initial visible members list + coVerify(exactly = 1) { arrangement.getMembersE2EICertificateStatuses(any(), eq(members1.map { it.user.id })) } + // and then second time only for newly emitted visible members + coVerify(exactly = 1) { arrangement.getMembersE2EICertificateStatuses(any(), eq(members2.map { it.user.id })) } + } + } + + @Test + fun givenGroup_whenVisibleMembersDoesNotChange_thenDoNotGetE2EICertificateForNonVisibleMembers() = runTest { + // Given + val members: List = buildList { + for (i in 1..6) { + add(MemberDetails(testOtherUser(i).copy(userType = UserType.INTERNAL), Member.Role.Member)) + } + } + val limit = 4 + val members1 = members.subList(0, limit) + val members2 = members.subList(limit, limit + 2) + val (arrangement, useCase) = ObserveParticipantsForConversationUseCaseArrangement() + .withConversationParticipantsUpdate(members1) + .arrange() + // When - Then + useCase(ConversationId("", ""), limit = limit).test { + val result1 = awaitItem() + assertEquals(result1.participants.size, members1.size) + assertEquals(result1.allParticipantsCount, members1.size) + // emit updated members list with new members that should not be visible because there is no limit set + arrangement.withConversationParticipantsUpdate(members1 + members2) + advanceUntilIdle() + val result2 = awaitItem() + assertEquals(result2.participants.size, members1.size) + assertEquals(result2.allParticipantsCount, members1.size + members2.size) + + // certificate statuses are fetched once for the initial visible members list + coVerify(exactly = 1) { arrangement.getMembersE2EICertificateStatuses(any(), eq(members1.map { it.user.id })) } + // and then newly emitted members are non-visible (because of the limit) and should not be fetched + coVerify(exactly = 0) { arrangement.getMembersE2EICertificateStatuses(any(), eq(members2.map { it.user.id })) } + } + } + + @Test + fun givenGroup_whenVisibleMembersDoesNotChange_thenDoNotGetE2EICertificateAgainForTheSameList() = runTest { + // Given + val members: List = buildList { + for (i in 1..6) { + add(MemberDetails(testOtherUser(i).copy(userType = UserType.INTERNAL), Member.Role.Member)) + } + } + val (arrangement, useCase) = ObserveParticipantsForConversationUseCaseArrangement() + .withConversationParticipantsUpdate(members) + .arrange() + // When - Then + useCase(ConversationId("", "")).test { + val result1 = awaitItem() + assertEquals(result1.participants.size, members.size) + assertEquals(result1.allParticipantsCount, members.size) + // emit the same members list again + arrangement.withConversationParticipantsUpdate(members) + advanceUntilIdle() + val result2 = awaitItem() + assertEquals(result2.participants.size, members.size) + assertEquals(result2.allParticipantsCount, members.size) + + // executed only once for visible members list even if the same list was emitted twice + coVerify(exactly = 1) { arrangement.getMembersE2EICertificateStatuses(any(), eq(members.map { it.user.id })) } + } + } } internal class ObserveParticipantsForConversationUseCaseArrangement { @@ -110,7 +206,7 @@ internal class ObserveParticipantsForConversationUseCaseArrangement { MockKAnnotations.init(this, relaxUnitFun = true) // Default empty values coEvery { observeConversationMembersUseCase(any()) } returns flowOf() - coEvery { getMembersE2EICertificateStatuses(any(), any()) } returns mapOf() + coEvery { getMembersE2EICertificateStatuses(any(), any()) } answers { secondArg>().associateWith { null } } } suspend fun withConversationParticipantsUpdate(members: List): ObserveParticipantsForConversationUseCaseArrangement {