From c42adeaa34f52c1570b792c238242de398827b48 Mon Sep 17 00:00:00 2001 From: sergeibakhtiarov Date: Wed, 18 Dec 2024 09:58:46 +0100 Subject: [PATCH] fix: infinite profile loading screen when user cannot be added (WPB-14842) (#3187) Co-authored-by: sergei.bakhtiarov --- .../kalium/logic/data/user/UserRepository.kt | 12 +++--- .../message/MessageSendFailureHandler.kt | 2 +- .../feature/user/ObserveUserInfoUseCase.kt | 10 ++++- .../prekey/MessageSendFailureHandlerTest.kt | 2 +- .../conversation/mls/OneOnOneResolverTest.kt | 6 +-- .../user/ObserveUserInfoUseCaseTest.kt | 37 ++++++++++++++++++- .../repository/UserRepositoryArrangement.kt | 4 +- 7 files changed, 58 insertions(+), 15 deletions(-) diff --git a/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/user/UserRepository.kt b/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/user/UserRepository.kt index e8760f83b79..64459a70784 100644 --- a/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/user/UserRepository.kt +++ b/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/user/UserRepository.kt @@ -100,7 +100,7 @@ interface UserRepository { * Fetches user information for all of users id stored in the DB */ suspend fun fetchAllOtherUsers(): Either - suspend fun fetchUsersByIds(qualifiedUserIdList: Set): Either + suspend fun fetchUsersByIds(qualifiedUserIdList: Set): Either suspend fun fetchUsersIfUnknownByIds(ids: Set): Either suspend fun observeSelfUser(): Flow suspend fun observeSelfUserWithTeam(): Flow> @@ -266,7 +266,7 @@ internal class UserDataSource internal constructor( override suspend fun fetchAllOtherUsers(): Either { val ids = userDAO.allOtherUsersId().map(UserIDEntity::toModel).toSet() - return fetchUsersByIds(ids) + return fetchUsersByIds(ids).map { } } override suspend fun fetchUserInfo(userId: UserId) = @@ -328,8 +328,8 @@ internal class UserDataSource internal constructor( } } - override suspend fun fetchUsersByIds(qualifiedUserIdList: Set): Either = - fetchUsersByIdsReturningListUsersDTO(qualifiedUserIdList).map { } + override suspend fun fetchUsersByIds(qualifiedUserIdList: Set): Either = + fetchUsersByIdsReturningListUsersDTO(qualifiedUserIdList).map { it.usersFound.isNotEmpty() } private suspend fun fetchTeamMembersByIds(userProfileList: List): Either> { val selfUserDomain = selfUserId.domain @@ -411,7 +411,7 @@ internal class UserDataSource internal constructor( qualifiedIDList.filterNot { knownUsers.any { userEntity -> userEntity.id == it && !userEntity.name.isNullOrBlank() } } }.flatMap { missingIds -> if (missingIds.isEmpty()) Either.Right(Unit) - else fetchUsersByIds(missingIds.map { it.toModel() }.toSet()) + else fetchUsersByIds(missingIds.map { it.toModel() }.toSet()).map { } } @OptIn(ExperimentalCoroutinesApi::class) @@ -629,7 +629,7 @@ internal class UserDataSource internal constructor( }.flatMap { usersWithoutMetadata -> kaliumLogger.d("Numbers of users to refresh: ${usersWithoutMetadata.size}") val userIds = usersWithoutMetadata.map { it.id.toModel() }.toSet() - fetchUsersByIds(userIds) + fetchUsersByIds(userIds).map { } } override suspend fun removeUserBrokenAsset(qualifiedID: QualifiedID) = wrapStorageRequest { diff --git a/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/message/MessageSendFailureHandler.kt b/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/message/MessageSendFailureHandler.kt index 1ee884d653e..0a6ad334583 100644 --- a/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/message/MessageSendFailureHandler.kt +++ b/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/message/MessageSendFailureHandler.kt @@ -111,7 +111,7 @@ class MessageSendFailureHandlerImpl internal constructor( private suspend fun syncUserIds(userId: Set): Either { return if (userId.isEmpty()) Either.Right(Unit) - else userRepository.fetchUsersByIds(userId) + else userRepository.fetchUsersByIds(userId).map { } } private suspend fun addMissingClients(missingClients: Map>): Either = diff --git a/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/user/ObserveUserInfoUseCase.kt b/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/user/ObserveUserInfoUseCase.kt index 5688d266312..404110e096f 100644 --- a/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/user/ObserveUserInfoUseCase.kt +++ b/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/user/ObserveUserInfoUseCase.kt @@ -77,7 +77,15 @@ internal class ObserveUserInfoUseCaseImpl( either.fold({ storageFailure -> if (storageFailure is StorageFailure.DataNotFound) { userRepository.fetchUsersByIds(setOf(userId)) - .fold({ ObserveOtherUserResult(fetchUserError = it) }) { ObserveOtherUserResult() } + .fold({ ObserveOtherUserResult(fetchUserError = it) }) { usersFound -> + if (usersFound) { + // Fetched users are persisted + ObserveOtherUserResult() + } else { + // Users cannot be found + ObserveOtherUserResult(fetchUserError = StorageFailure.DataNotFound) + } + } } else { ObserveOtherUserResult(getKnownUserError = storageFailure) } diff --git a/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/prekey/MessageSendFailureHandlerTest.kt b/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/prekey/MessageSendFailureHandlerTest.kt index fce96785379..22fcd306f95 100644 --- a/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/prekey/MessageSendFailureHandlerTest.kt +++ b/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/prekey/MessageSendFailureHandlerTest.kt @@ -400,7 +400,7 @@ class MessageSendFailureHandlerTest { suspend fun withFetchUsersByIdSuccess() = apply { coEvery { userRepository.fetchUsersByIds(any()) - }.returns(Either.Right(Unit)) + }.returns(Either.Right(true)) } suspend fun withFetchUsersByIdFailure(failure: CoreFailure) = apply { diff --git a/logic/src/commonTest/kotlin/com/wire/kalium/logic/feature/conversation/mls/OneOnOneResolverTest.kt b/logic/src/commonTest/kotlin/com/wire/kalium/logic/feature/conversation/mls/OneOnOneResolverTest.kt index 8d8a02ac310..573083d1951 100644 --- a/logic/src/commonTest/kotlin/com/wire/kalium/logic/feature/conversation/mls/OneOnOneResolverTest.kt +++ b/logic/src/commonTest/kotlin/com/wire/kalium/logic/feature/conversation/mls/OneOnOneResolverTest.kt @@ -101,7 +101,7 @@ class OneOnOneResolverTest { // given val oneOnOneUser = TestUser.OTHER.copy(id = TestUser.OTHER_USER_ID) val (arrangement, resolver) = arrange { - withFetchUsersByIdReturning(Either.Right(Unit)) + withFetchUsersByIdReturning(Either.Right(true)) withGetProtocolForUser(Either.Right(SupportedProtocol.MLS)) withMigrateToMLSReturns(Either.Right(TestConversation.ID)) } @@ -128,7 +128,7 @@ class OneOnOneResolverTest { // given val oneOnOneUser = TestUser.OTHER.copy(id = TestUser.OTHER_USER_ID) val (arrangement, resolver) = arrange { - withFetchUsersByIdReturning(Either.Right(Unit)) + withFetchUsersByIdReturning(Either.Right(true)) withGetProtocolForUser(Either.Right(SupportedProtocol.MLS)) withMigrateToMLSReturns(Either.Right(TestConversation.ID)) } @@ -149,7 +149,7 @@ class OneOnOneResolverTest { val oneOnOneUser = TestUser.OTHER.copy(id = TestUser.OTHER_USER_ID) val (arrangement, resolver) = arrange { withGetKnownUserReturning(flowOf(oneOnOneUser)) - withFetchUsersByIdReturning(Either.Right(Unit)) + withFetchUsersByIdReturning(Either.Right(true)) withGetProtocolForUser(Either.Right(SupportedProtocol.MLS)) withMigrateToMLSReturns(Either.Right(TestConversation.ID)) } diff --git a/logic/src/commonTest/kotlin/com/wire/kalium/logic/feature/user/ObserveUserInfoUseCaseTest.kt b/logic/src/commonTest/kotlin/com/wire/kalium/logic/feature/user/ObserveUserInfoUseCaseTest.kt index 715110ba89b..686be4c5da1 100644 --- a/logic/src/commonTest/kotlin/com/wire/kalium/logic/feature/user/ObserveUserInfoUseCaseTest.kt +++ b/logic/src/commonTest/kotlin/com/wire/kalium/logic/feature/user/ObserveUserInfoUseCaseTest.kt @@ -20,6 +20,7 @@ package com.wire.kalium.logic.feature.user import app.cash.turbine.test import com.wire.kalium.logic.CoreFailure +import com.wire.kalium.logic.StorageFailure import com.wire.kalium.logic.data.team.TeamRepository import com.wire.kalium.logic.data.user.UserId import com.wire.kalium.logic.data.user.UserRepository @@ -231,6 +232,32 @@ class ObserveUserInfoUseCaseTest { } } + @Test + fun givenAUserIdWhichIsNotInDBAndNotOnServer_whenInvokingObserveUserInfo_thenErrorIsReturned() = runTest { + // given + val (arrangement, useCase) = arrangement + .withFailingUserRetrieveFromDB() + .withSuccessfulTeamRetrieve(localTeamPresent = true) + .withSuccessfulUserFetchingNoUsersFound() + .arrange() + + // when + useCase(userId).test { + val result = awaitItem() + + // then + assertIs(result) + + with(arrangement) { + coVerify { + userRepository.fetchUsersByIds(any()) + }.wasInvoked(once) + } + + awaitComplete() + } + } + private class ObserveUserInfoUseCaseTestArrangement { @Mock @@ -276,7 +303,15 @@ class ObserveUserInfoUseCaseTest { suspend fun withSuccessfulUserFetching(): ObserveUserInfoUseCaseTestArrangement { coEvery { userRepository.fetchUsersByIds(any()) - }.returns(Either.Right(Unit)) + }.returns(Either.Right(true)) + + return this + } + + suspend fun withSuccessfulUserFetchingNoUsersFound(): ObserveUserInfoUseCaseTestArrangement { + coEvery { + userRepository.fetchUsersByIds(any()) + }.returns(Either.Right(false)) return this } diff --git a/logic/src/commonTest/kotlin/com/wire/kalium/logic/util/arrangement/repository/UserRepositoryArrangement.kt b/logic/src/commonTest/kotlin/com/wire/kalium/logic/util/arrangement/repository/UserRepositoryArrangement.kt index 25fecc6da7c..e106b3290a6 100644 --- a/logic/src/commonTest/kotlin/com/wire/kalium/logic/util/arrangement/repository/UserRepositoryArrangement.kt +++ b/logic/src/commonTest/kotlin/com/wire/kalium/logic/util/arrangement/repository/UserRepositoryArrangement.kt @@ -72,7 +72,7 @@ internal interface UserRepositoryArrangement { suspend fun withFetchUserInfoReturning(result: Either) suspend fun withFetchUsersByIdReturning( - result: Either, + result: Either, userIdList: Matcher> = AnyMatcher(valueOf()) ) @@ -186,7 +186,7 @@ internal open class UserRepositoryArrangementImpl : UserRepositoryArrangement { } override suspend fun withFetchUsersByIdReturning( - result: Either, + result: Either, userIdList: Matcher> ) { coEvery {