Skip to content

Commit

Permalink
Revert "refactor: remove the fetch of full team members during slow s…
Browse files Browse the repository at this point in the history
…ync (#2253)"

This reverts commit 1bdd10c.
  • Loading branch information
MohamadJaara committed Dec 4, 2023
1 parent 8e02923 commit 1c3b234
Show file tree
Hide file tree
Showing 4 changed files with 119 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import kotlinx.coroutines.flow.map

interface TeamRepository {
suspend fun fetchTeamById(teamId: TeamId): Either<CoreFailure, Team>
suspend fun fetchMembersByTeamId(teamId: TeamId, userDomain: String): Either<CoreFailure, Unit>
suspend fun getTeam(teamId: TeamId): Flow<Team?>
suspend fun deleteConversation(conversationId: ConversationId, teamId: TeamId): Either<CoreFailure, Unit>
suspend fun updateMemberRole(teamId: String, userId: String, permissionCode: Int?): Either<CoreFailure, Unit>
Expand Down Expand Up @@ -80,9 +81,35 @@ internal class TeamDataSource(
teamsApi.getTeamInfo(teamId = teamId.value)
}.map { teamDTO ->
teamMapper.fromDtoToEntity(teamDTO)
}.flatMap { teamEntity ->
wrapStorageRequest { teamDAO.insertTeam(team = teamEntity) }.map {
teamMapper.fromDaoModelToTeam(teamEntity)
}.map { teamEntity ->
teamDAO.insertTeam(team = teamEntity)

teamMapper.fromDaoModelToTeam(teamEntity)
}

override suspend fun fetchMembersByTeamId(teamId: TeamId, userDomain: String): Either<CoreFailure, Unit> = wrapApiRequest {
teamsApi.getTeamMembers(
teamId = teamId.value,
limitTo = null
)
}.map { teamMemberList ->
/**
* If hasMore is true, then this result should be discarded and not stored locally,
* otherwise the user will see random team members when opening the search UI.
* If the result has has_more field set to false, then these users are stored locally to be used in a search later.
*/
if (teamMemberList.hasMore.not()) {
teamMemberList.members.map { teamMember ->
val userId = QualifiedIDEntity(teamMember.nonQualifiedUserId, userDomain)
val userType = userTypeEntityTypeMapper.teamRoleCodeToUserType(teamMember.permissions?.own)
userId to userType
}
} else {
listOf()
}
}.flatMap { teamMembers ->
wrapStorageRequest {
userDAO.upsertTeamMemberUserTypes(teamMembers.toMap())
}
}

Expand All @@ -102,11 +129,9 @@ internal class TeamDataSource(

override suspend fun updateMemberRole(teamId: String, userId: String, permissionCode: Int?): Either<CoreFailure, Unit> {
return wrapStorageRequest {
userDAO.upsertTeamMemberUserTypes(
mapOf(
QualifiedIDEntity(userId, selfUserId.domain) to userTypeEntityTypeMapper.teamRoleCodeToUserType(permissionCode)
)
)
userDAO.upsertTeamMemberUserTypes(mapOf(
QualifiedIDEntity(userId, selfUserId.domain) to userTypeEntityTypeMapper.teamRoleCodeToUserType(permissionCode)
))
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -41,11 +41,14 @@ internal class SyncSelfTeamUseCaseImpl(
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 {
teamRepository.fetchMembersByTeamId(
teamId = teamId,
userDomain = user.id.domain
)
}.onSuccess {
teamRepository.syncServices(teamId = teamId)
}
} ?: run {
kaliumLogger.withFeatureId(SYNC).i("Skipping team sync because user doesn't belong to a team")
Either.Right(Unit)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 {
Expand Down Expand Up @@ -103,6 +106,56 @@ 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()
.arrange()

given(arrangement.teamsApi)
.suspendFunction(arrangement.teamsApi::getTeamMembers)
.whenInvokedWith(oneOf("teamId"), oneOf(null))
.thenReturn(NetworkResponse.Success(value = teamMembersList, headers = mapOf(), httpCode = 200))

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")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,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())
Expand All @@ -79,6 +82,7 @@ class SyncSelfTeamUseCaseTest {
val (arrangement, syncSelfTeamUseCase) = Arrangement()
.withSelfUser(selfUserFlow)
.withTeam()
.withTeamMembers()
.withServicesSync()
.arrange()

Expand All @@ -90,7 +94,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))
Expand All @@ -115,7 +125,10 @@ 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())
Expand All @@ -130,6 +143,7 @@ class SyncSelfTeamUseCaseTest {
val (_, syncSelfTeamUseCase) = Arrangement()
.withSelfUser(selfUserFlow)
.withTeam()
.withTeamMembers()
.withFailingServicesSync()
.arrange()

Expand Down Expand Up @@ -174,6 +188,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)
Expand Down

0 comments on commit 1c3b234

Please sign in to comment.