From 1ad916c8831e231e871d7fbca24ec72f9f4fc4ac Mon Sep 17 00:00:00 2001 From: Mohamad Jaara Date: Tue, 5 Dec 2023 09:22:07 +0100 Subject: [PATCH] feat: fetch full list of team members when kalium consumers needs to (#2284) * Revert "refactor: remove the fetch of full team members during slow sync (#2253)" This reverts commit 1bdd10cd839ec194a52bd44f1a96a19a74a91f37. * add flag to fetchAllTeamMembersEagerly * add flag to fetchAllTeamMembersEagerly * fetch all team members pages * fetch all team members pages * MayBeConst * fix tests --- .../kalium/logic/data/team/TeamRepository.kt | 45 ++++++++- .../kalium/logic/feature/UserSessionScope.kt | 33 ++++--- .../logic/feature/team/SyncSelfTeamUseCase.kt | 21 +++-- .../logic/featureFlags/KaliumConfigs.kt | 3 +- .../logic/data/team/TeamRepositoryTest.kt | 64 +++++++++++-- .../feature/team/SyncSelfTeamUseCaseTest.kt | 92 ++++++++++++++++--- .../com/wire/kalium/monkeys/homeDirectory.kt | 1 + 7 files changed, 216 insertions(+), 43 deletions(-) diff --git a/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/team/TeamRepository.kt b/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/team/TeamRepository.kt index a75644216b8..8f2f022ad32 100644 --- a/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/team/TeamRepository.kt +++ b/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/team/TeamRepository.kt @@ -31,6 +31,7 @@ import com.wire.kalium.logic.di.MapperProvider import com.wire.kalium.logic.functional.Either import com.wire.kalium.logic.functional.flatMap import com.wire.kalium.logic.functional.map +import com.wire.kalium.logic.functional.onFailure import com.wire.kalium.logic.functional.onSuccess import com.wire.kalium.logic.sync.receiver.handler.legalhold.LegalHoldHandler import com.wire.kalium.logic.sync.receiver.handler.legalhold.LegalHoldRequestHandler @@ -50,6 +51,7 @@ import kotlinx.coroutines.flow.map interface TeamRepository { suspend fun fetchTeamById(teamId: TeamId): Either + suspend fun fetchMembersByTeamId(teamId: TeamId, userDomain: String): Either suspend fun getTeam(teamId: TeamId): Flow suspend fun deleteConversation(conversationId: ConversationId, teamId: TeamId): Either suspend fun updateMemberRole(teamId: String, userId: String, permissionCode: Int?): Either @@ -81,11 +83,45 @@ internal class TeamDataSource( }.map { teamDTO -> teamMapper.fromDtoToEntity(teamDTO) }.flatMap { teamEntity -> - wrapStorageRequest { teamDAO.insertTeam(team = teamEntity) }.map { + wrapStorageRequest { + teamDAO.insertTeam(teamEntity) + }.map { teamMapper.fromDaoModelToTeam(teamEntity) } } + override suspend fun fetchMembersByTeamId(teamId: TeamId, userDomain: String): Either { + var hasMore = true + var error: CoreFailure? = null + while (hasMore && error == null) { + wrapApiRequest { + teamsApi.getTeamMembers( + teamId = teamId.value, + limitTo = FETCH_TEAM_MEMBER_PAGE_SIZE + ) + }.onSuccess { + hasMore = it.hasMore + }.map { + it.members.map { teamMember -> + val userId = QualifiedIDEntity(teamMember.nonQualifiedUserId, userDomain) + val userType = userTypeEntityTypeMapper.teamRoleCodeToUserType(teamMember.permissions?.own) + userId to userType + } + }.flatMap { teamMembers -> + wrapStorageRequest { + userDAO.upsertTeamMemberUserTypes(teamMembers.toMap()) + } + }.onFailure { + error = it + } + } + return if (error != null) { + Either.Left(error!!) + } else { + Either.Right(Unit) + } + } + override suspend fun getTeam(teamId: TeamId): Flow = teamDAO.getTeamById(teamId.value) .map { @@ -147,6 +183,7 @@ internal class TeamDataSource( eventContentDTO = EventContentDTO.User.LegalHoldEnabledDTO(id = selfUserId.toString()) ) ) + LegalHoldStatusDTO.DISABLED -> legalHoldHandler.handleDisable( eventMapper.legalHoldDisabled( id = LocalId.generate(), @@ -155,6 +192,7 @@ internal class TeamDataSource( eventContentDTO = EventContentDTO.User.LegalHoldDisabledDTO(id = selfUserId.toString()) ) ) + LegalHoldStatusDTO.PENDING -> legalHoldRequestHandler.handle( eventMapper.legalHoldRequest( @@ -168,7 +206,12 @@ internal class TeamDataSource( ) ) ) + LegalHoldStatusDTO.NO_CONSENT -> Either.Right(Unit) }.map { legalHoldStatusMapper.fromApiModel(response.legalHoldStatusDTO) } } + + private companion object { + const val FETCH_TEAM_MEMBER_PAGE_SIZE = 200 + } } diff --git a/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/UserSessionScope.kt b/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/UserSessionScope.kt index bf4e2de904c..7bfa72767c4 100644 --- a/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/UserSessionScope.kt +++ b/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/UserSessionScope.kt @@ -875,7 +875,9 @@ class UserSessionScope internal constructor( private val syncSelfTeamUseCase: SyncSelfTeamUseCase get() = SyncSelfTeamUseCaseImpl( - userRepository = userRepository, teamRepository = teamRepository + userRepository = userRepository, + teamRepository = teamRepository, + fetchAllTeamMembersEagerly = kaliumConfigs.fetchAllTeamMembersEagerly ) private val joinExistingMLSConversationUseCase: JoinExistingMLSConversationUseCase @@ -1088,22 +1090,23 @@ class UserSessionScope internal constructor( ) }) - internal val mlsMigrationWorker get() = - MLSMigrationWorkerImpl( - userConfigRepository, - featureConfigRepository, - mlsConfigHandler, - mlsMigrationConfigHandler, - mlsMigrator, - ) + internal val mlsMigrationWorker + get() = + MLSMigrationWorkerImpl( + userConfigRepository, + featureConfigRepository, + mlsConfigHandler, + mlsMigrationConfigHandler, + mlsMigrator, + ) internal val mlsMigrationManager: MLSMigrationManager = MLSMigrationManagerImpl( - kaliumConfigs, - featureSupport, - incrementalSyncRepository, - lazy { clientRepository }, - lazy { users.timestampKeyRepository }, - lazy { mlsMigrationWorker } + kaliumConfigs, + featureSupport, + incrementalSyncRepository, + lazy { clientRepository }, + lazy { users.timestampKeyRepository }, + lazy { mlsMigrationWorker } ) private val mlsPublicKeysRepository: MLSPublicKeysRepository diff --git a/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/team/SyncSelfTeamUseCase.kt b/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/team/SyncSelfTeamUseCase.kt index be5d8003135..ea665c53615 100644 --- a/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/team/SyncSelfTeamUseCase.kt +++ b/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/team/SyncSelfTeamUseCase.kt @@ -23,7 +23,7 @@ import com.wire.kalium.logic.CoreFailure import com.wire.kalium.logic.data.team.TeamRepository import com.wire.kalium.logic.data.user.UserRepository import com.wire.kalium.logic.functional.Either -import com.wire.kalium.logic.functional.map +import com.wire.kalium.logic.functional.flatMap import com.wire.kalium.logic.functional.onSuccess import com.wire.kalium.logic.kaliumLogger import kotlinx.coroutines.flow.first @@ -34,18 +34,27 @@ internal interface SyncSelfTeamUseCase { internal class SyncSelfTeamUseCaseImpl( private val userRepository: UserRepository, - private val teamRepository: TeamRepository + private val teamRepository: TeamRepository, + private val fetchAllTeamMembersEagerly: Boolean ) : SyncSelfTeamUseCase { override suspend fun invoke(): Either { val user = userRepository.observeSelfUser().first() return user.teamId?.let { teamId -> - teamRepository.fetchTeamById(teamId = teamId) - .map { } - .onSuccess { - teamRepository.syncServices(teamId = teamId) + teamRepository.fetchTeamById(teamId = teamId).flatMap { + if (fetchAllTeamMembersEagerly) { + kaliumLogger.withFeatureId(SYNC).i("Fetching all team members eagerly") + teamRepository.fetchMembersByTeamId( + teamId = teamId, + userDomain = user.id.domain + ) + } else { + Either.Right(Unit) } + }.onSuccess { + teamRepository.syncServices(teamId = teamId) + } } ?: run { kaliumLogger.withFeatureId(SYNC).i("Skipping team sync because user doesn't belong to a team") Either.Right(Unit) diff --git a/logic/src/commonMain/kotlin/com/wire/kalium/logic/featureFlags/KaliumConfigs.kt b/logic/src/commonMain/kotlin/com/wire/kalium/logic/featureFlags/KaliumConfigs.kt index 4aea8d10954..9bb7ae67576 100644 --- a/logic/src/commonMain/kotlin/com/wire/kalium/logic/featureFlags/KaliumConfigs.kt +++ b/logic/src/commonMain/kotlin/com/wire/kalium/logic/featureFlags/KaliumConfigs.kt @@ -44,7 +44,8 @@ data class KaliumConfigs( val kaliumMockEngine: KaliumMockEngine? = null, val mockNetworkStateObserver: NetworkStateObserver? = null, // Interval between attempts to advance the proteus to MLS migration - val mlsMigrationInterval: Duration = 24.hours + val mlsMigrationInterval: Duration = 24.hours, + val fetchAllTeamMembersEagerly: Boolean = false, ) sealed interface BuildFileRestrictionState { diff --git a/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/team/TeamRepositoryTest.kt b/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/team/TeamRepositoryTest.kt index 1f74861879a..566295e6efc 100644 --- a/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/team/TeamRepositoryTest.kt +++ b/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/team/TeamRepositoryTest.kt @@ -49,6 +49,7 @@ import com.wire.kalium.persistence.dao.UserDAO import com.wire.kalium.persistence.dao.unread.UserConfigDAO import io.mockative.Mock import io.mockative.any +import io.mockative.anything import io.mockative.classOf import io.mockative.configure import io.mockative.eq @@ -58,11 +59,13 @@ import io.mockative.mock import io.mockative.once import io.mockative.oneOf import io.mockative.verify +import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.test.runTest import kotlin.test.Test import kotlin.test.assertEquals +@OptIn(ExperimentalCoroutinesApi::class) class TeamRepositoryTest { @Test fun givenSelfUserExists_whenFetchingTeamInfo_thenTeamInfoShouldBeSuccessful() = runTest { @@ -103,6 +106,52 @@ class TeamRepositoryTest { } } + @Test + fun givenTeamIdAndUserDomain_whenFetchingTeamMembers_thenTeamMembersShouldBeSuccessful() = runTest { + val teamMember = TestTeam.memberDTO( + nonQualifiedUserId = "teamMember1" + ) + + val teamMembersList = TeamsApi.TeamMemberList( + hasMore = false, + members = listOf( + teamMember + ) + ) + + val (arrangement, teamRepository) = Arrangement() + .withGetTeamMembers(NetworkResponse.Success(teamMembersList, mapOf(), 200)) + .arrange() + + val result = teamRepository.fetchMembersByTeamId(teamId = TeamId("teamId"), userDomain = "userDomain") + + // Verifies that userDAO insertUsers was called with the correct mapped values + verify(arrangement.userDAO) + .suspendFunction(arrangement.userDAO::upsertTeamMemberUserTypes) + .with(any()) + .wasInvoked(exactly = once) + + // Verifies that when fetching members by team id, it succeeded + result.shouldSucceed() + } + + @Test + fun givenTeamApiFails_whenFetchingTeamMembers_thenTheFailureIsPropagated() = runTest { + val (arrangement, teamRepository) = Arrangement() + .arrange() + + given(arrangement.teamsApi) + .suspendFunction(arrangement.teamsApi::getTeamMembers) + .whenInvokedWith(any(), anything()) + .thenReturn(NetworkResponse.Error(KaliumException.ServerError(ErrorResponse(500, "error_message", "error_label")))) + + val result = teamRepository.fetchMembersByTeamId(teamId = TeamId("teamId"), userDomain = "userDomain") + + result.shouldFail { + assertEquals(it::class, NetworkFailure.ServerMiscommunication::class) + } + } + @Test fun givenSelfUserExists_whenGettingTeamById_thenTeamDataShouldBePassed() = runTest { val teamEntity = TeamEntity(id = "teamId", name = "teamName", icon = "icon") @@ -364,13 +413,6 @@ class TeamRepositoryTest { .then { NetworkResponse.Success(value = teamDTO, headers = mapOf(), httpCode = 200) } } - fun withApiGetTeamMemberSuccess(teamMemberDTO: TeamsApi.TeamMemberDTO) = apply { - given(teamsApi) - .suspendFunction(teamsApi::getTeamMember) - .whenInvokedWith(any(), any()) - .thenReturn(NetworkResponse.Success(value = teamMemberDTO, headers = mapOf(), httpCode = 200)) - } - fun withFetchWhiteListedServicesSuccess() = apply { given(teamsApi) .suspendFunction(teamsApi::whiteListedServices) @@ -407,6 +449,14 @@ class TeamRepositoryTest { .thenReturn(Either.Right(Unit)) } + fun withGetTeamMembers(result: NetworkResponse) = apply { + given(teamsApi) + .suspendFunction(teamsApi::getTeamMembers) + .whenInvokedWith(any(), any()) + .thenReturn(result) + + } + fun arrange() = this to teamRepository companion object { diff --git a/logic/src/commonTest/kotlin/com/wire/kalium/logic/feature/team/SyncSelfTeamUseCaseTest.kt b/logic/src/commonTest/kotlin/com/wire/kalium/logic/feature/team/SyncSelfTeamUseCaseTest.kt index 2f8d1e7c436..89e6344bc0f 100644 --- a/logic/src/commonTest/kotlin/com/wire/kalium/logic/feature/team/SyncSelfTeamUseCaseTest.kt +++ b/logic/src/commonTest/kotlin/com/wire/kalium/logic/feature/team/SyncSelfTeamUseCaseTest.kt @@ -38,9 +38,9 @@ import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.test.runTest +import kotlin.properties.Delegates import kotlin.test.Test -@OptIn(ExperimentalCoroutinesApi::class) class SyncSelfTeamUseCaseTest { @Test @@ -54,6 +54,7 @@ class SyncSelfTeamUseCaseTest { val (arrangement, syncSelfTeamUseCase) = Arrangement() .withSelfUser(selfUserFlow) + .witFetchAllTeamMembersEagerly(false) .arrange() // when @@ -64,7 +65,10 @@ class SyncSelfTeamUseCaseTest { .suspendFunction(arrangement.teamRepository::fetchTeamById) .with(any()) .wasNotInvoked() - + verify(arrangement.teamRepository) + .suspendFunction(arrangement.teamRepository::fetchMembersByTeamId) + .with(any(), any()) + .wasNotInvoked() verify(arrangement.teamRepository) .suspendFunction(arrangement.teamRepository::syncServices) .with(any()) @@ -72,13 +76,15 @@ class SyncSelfTeamUseCaseTest { } @Test - fun givenSelfUserHasValidTeam_whenSyncingSelfTeam_thenTeamInfoAndServicesAreRequestedSuccessfully() = runTest { + fun givenSelfUserHasValidTeamAndFetchAllTeamMembersEagerlyIsTrue_whenSyncingSelfTeam_thenTeamInfoAndServicesAreRequestedSuccessfully() = runTest { // given val selfUserFlow = flowOf(TestUser.SELF) val (arrangement, syncSelfTeamUseCase) = Arrangement() .withSelfUser(selfUserFlow) + .witFetchAllTeamMembersEagerly(true) .withTeam() + .withTeamMembers() .withServicesSync() .arrange() @@ -90,7 +96,13 @@ class SyncSelfTeamUseCaseTest { .suspendFunction(arrangement.teamRepository::fetchTeamById) .with(eq(TestUser.SELF.teamId)) .wasInvoked(exactly = once) - + verify(arrangement.teamRepository) + .suspendFunction(arrangement.teamRepository::fetchMembersByTeamId) + .with( + eq(TestUser.SELF.teamId), + eq(TestUser.SELF.id.domain) + ) + .wasInvoked(exactly = once) verify(arrangement.teamRepository) .suspendFunction(arrangement.teamRepository::syncServices) .with(eq(TestUser.SELF.teamId)) @@ -104,6 +116,7 @@ class SyncSelfTeamUseCaseTest { val (arrangement, syncSelfTeamUseCase) = Arrangement() .withSelfUser(selfUserFlow) + .witFetchAllTeamMembersEagerly(false) .withFailingTeamInfo() .arrange() @@ -115,11 +128,20 @@ class SyncSelfTeamUseCaseTest { .suspendFunction(arrangement.teamRepository::fetchTeamById) .with(eq(TestUser.SELF.teamId)) .wasInvoked(exactly = once) - + verify(arrangement.teamRepository) + .suspendFunction(arrangement.teamRepository::fetchMembersByTeamId) + .with(any(), any()) + .wasNotInvoked() verify(arrangement.teamRepository) .suspendFunction(arrangement.teamRepository::syncServices) .with(any()) .wasNotInvoked() + verify(arrangement.teamRepository) + .suspendFunction(arrangement.teamRepository::fetchMembersByTeamId) + .with( + eq(TestUser.SELF.teamId), + eq(TestUser.SELF.id.domain) + ).wasNotInvoked() } @Test @@ -129,7 +151,9 @@ class SyncSelfTeamUseCaseTest { val (_, syncSelfTeamUseCase) = Arrangement() .withSelfUser(selfUserFlow) + .witFetchAllTeamMembersEagerly(false) .withTeam() + .withTeamMembers() .withFailingServicesSync() .arrange() @@ -140,18 +164,50 @@ class SyncSelfTeamUseCaseTest { result.shouldSucceed() } + @Test + fun givenSelfUserHasValidTeamAndFetchAllTeamMembersEagerlyIsFalse_whenSyncingSelfTeam_thenTeamInfoAndServicesAreRequestedSuccessfully() = runTest { + // given + val selfUserFlow = flowOf(TestUser.SELF) + + val (arrangement, syncSelfTeamUseCase) = Arrangement() + .withSelfUser(selfUserFlow) + .witFetchAllTeamMembersEagerly(false) + .withTeam() + .withServicesSync() + .arrange() + + // when + syncSelfTeamUseCase.invoke() + + // then + verify(arrangement.teamRepository) + .suspendFunction(arrangement.teamRepository::fetchTeamById) + .with(eq(TestUser.SELF.teamId)) + .wasInvoked(exactly = once) + verify(arrangement.teamRepository) + .suspendFunction(arrangement.teamRepository::fetchMembersByTeamId) + .with(any(), any()) + .wasNotInvoked() + verify(arrangement.teamRepository) + .suspendFunction(arrangement.teamRepository::syncServices) + .with(eq(TestUser.SELF.teamId)) + .wasInvoked(exactly = once) + } + private class Arrangement { + var fetchAllTeamMembersEagerly by Delegates.notNull() + @Mock val userRepository = mock(classOf()) @Mock val teamRepository = mock(classOf()) - val syncSelfTeamUseCase = SyncSelfTeamUseCaseImpl( - userRepository = userRepository, - teamRepository = teamRepository - ) + private lateinit var syncSelfTeamUseCase: SyncSelfTeamUseCase + fun witFetchAllTeamMembersEagerly(result: Boolean) = apply { + fetchAllTeamMembersEagerly = result + } fun withSelfUser(selfUserFlow: Flow) = apply { given(userRepository) @@ -174,6 +230,13 @@ class SyncSelfTeamUseCaseTest { .thenReturn(Either.Left(NetworkFailure.ServerMiscommunication(TestNetworkException.badRequest))) } + fun withTeamMembers() = apply { + given(teamRepository) + .suspendFunction(teamRepository::fetchMembersByTeamId) + .whenInvokedWith(any(), any()) + .thenReturn(Either.Right(Unit)) + } + fun withServicesSync() = apply { given(teamRepository) .suspendFunction(teamRepository::syncServices) @@ -188,10 +251,13 @@ class SyncSelfTeamUseCaseTest { .thenReturn(Either.Left(NetworkFailure.ServerMiscommunication(TestNetworkException.accessDenied))) } - fun arrange() = this to syncSelfTeamUseCase - - companion object { - + fun arrange(): Pair { + syncSelfTeamUseCase = SyncSelfTeamUseCaseImpl( + userRepository = userRepository, + teamRepository = teamRepository, + fetchAllTeamMembersEagerly + ) + return this to syncSelfTeamUseCase } } } diff --git a/monkeys/src/main/kotlin/com/wire/kalium/monkeys/homeDirectory.kt b/monkeys/src/main/kotlin/com/wire/kalium/monkeys/homeDirectory.kt index 452d27dff97..f0c76cd4db1 100644 --- a/monkeys/src/main/kotlin/com/wire/kalium/monkeys/homeDirectory.kt +++ b/monkeys/src/main/kotlin/com/wire/kalium/monkeys/homeDirectory.kt @@ -35,6 +35,7 @@ fun coreLogic( encryptProteusStorage = true, isMLSSupportEnabled = true, wipeOnDeviceRemoval = true, + fetchAllTeamMembersEagerly = true, ), "Wire Infinite Monkeys" ) coreLogic.updateApiVersionsScheduler.scheduleImmediateApiVersionUpdate()