From 33a635afc2de1aace06cfa61c53d4a58606b1283 Mon Sep 17 00:00:00 2001 From: Vitor Hugo Schwaab Date: Tue, 7 Nov 2023 16:20:20 +0100 Subject: [PATCH] fix: update all users when getting their data [AR-5057] (#2200) This commit removes distinction between federated and non-federated users in UserRepository when getting known users. All users are now treated in the same manner when getting them from the database. This implies that their data will be refreshed from the API regardless of the user type. Adjustments have been made to the UserRepositoryTest to reflect this change in behavior. --- .../kalium/logic/data/user/UserRepository.kt | 42 ++++++++++++------- .../logic/data/user/UserRepositoryTest.kt | 14 +++---- 2 files changed, 35 insertions(+), 21 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 c989ca40438..7593219e01f 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 @@ -37,7 +37,6 @@ import com.wire.kalium.logic.data.team.Team import com.wire.kalium.logic.data.team.TeamMapper import com.wire.kalium.logic.data.user.type.DomainUserTypeMapper import com.wire.kalium.logic.data.user.type.UserEntityTypeMapper -import com.wire.kalium.logic.data.user.type.isFederated import com.wire.kalium.logic.di.MapperProvider import com.wire.kalium.logic.failure.SelfUserDeleted import com.wire.kalium.logic.feature.SelfTeamIdProvider @@ -116,6 +115,7 @@ internal interface UserRepository { * when backends stops federating. */ suspend fun defederateUser(userId: UserId): Either + // TODO: move to migration repo suspend fun insertUsersIfUnknown(users: List): Either suspend fun fetchUserInfo(userId: UserId): Either @@ -153,12 +153,12 @@ internal class UserDataSource internal constructor( ) : UserRepository { /** - * In case of federated users, we need to refresh their info every time. - * Since the current backend implementation at wire does not emit user events across backends. + * Stores the last time a user's details were fetched from remote. * - * This is an in-memory cache, to help avoid unnecessary requests in a time window. + * @see Event.User.Update + * @see USER_DETAILS_MAX_AGE */ - private val federatedUsersExpirationCache = ConcurrentMap() + private val userDetailsRefreshInstantCache = ConcurrentMap() override suspend fun fetchSelfUser(): Either = wrapApiRequest { selfApi.getSelfInfo() } .flatMap { userDTO -> @@ -184,20 +184,25 @@ internal class UserDataSource internal constructor( .map { userEntity -> userEntity?.let { publicUserMapper.fromUserEntityToOtherUser(userEntity) } }.onEach { otherUser -> - processFederatedUserRefresh(userId, otherUser) + if (otherUser != null) { + refreshUserDetailsIfNeeded(userId) + } } /** - * Only in case of federated users and if it's expired or not cached, we fetch and refresh the user info. + * Only refresh user profiles if it wasn't fetched recently. + * + * @see userDetailsRefreshInstantCache + * @see USER_DETAILS_MAX_AGE */ - private suspend fun processFederatedUserRefresh(userId: UserId, otherUser: OtherUser?) { - if (otherUser != null && otherUser.userType.isFederated() - && federatedUsersExpirationCache[userId]?.let { DateTimeUtil.currentInstant() > it } != false - ) { + private suspend fun refreshUserDetailsIfNeeded(userId: UserId) { + val now = DateTimeUtil.currentInstant() + val wasFetchedRecently = userDetailsRefreshInstantCache[userId]?.let { now < it + USER_DETAILS_MAX_AGE } ?: false + if (!wasFetchedRecently) { fetchUserInfo(userId).also { - kaliumLogger.d("Federated user, refreshing user info from API after $FEDERATED_USER_TTL") + kaliumLogger.d("Federated user, refreshing user info from API after $USER_DETAILS_MAX_AGE") } - federatedUsersExpirationCache[userId] = DateTimeUtil.currentInstant().plus(FEDERATED_USER_TTL) + userDetailsRefreshInstantCache[userId] = now } } @@ -472,7 +477,16 @@ internal class UserDataSource internal constructor( companion object { internal const val SELF_USER_ID_KEY = "selfUserID" - internal val FEDERATED_USER_TTL = 5.minutes + + /** + * Maximum age for user details. + * + * The USER_DETAILS_MAX_AGE constant represents the maximum age in minutes that user details can be considered valid. After + * this duration, the user details should be refreshed. + * + * This is needed because some users don't get `user.update` events, so we need to refresh their details every so often. + */ + internal val USER_DETAILS_MAX_AGE = 5.minutes internal const val BATCH_SIZE = 500 } } diff --git a/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/user/UserRepositoryTest.kt b/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/user/UserRepositoryTest.kt index fbb3aaabf9f..b9363f70f23 100644 --- a/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/user/UserRepositoryTest.kt +++ b/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/user/UserRepositoryTest.kt @@ -353,9 +353,9 @@ class UserRepositoryTest { } @Test - fun givenAKnownNOTFederatedUser_whenGettingFromDb_thenShouldNotRefreshItsDataFromAPI() = runTest { + fun givenAKnownUser_whenGettingFromDb_thenShouldRefreshItsDataFromAPI() = runTest { val (arrangement, userRepository) = Arrangement() - .withUserDaoReturning(TestUser.ENTITY.copy(userType = UserTypeEntity.STANDARD)) + .withUserDaoReturning(TestUser.ENTITY) .withSuccessfulGetUsersInfo() .arrange() @@ -365,22 +365,22 @@ class UserRepositoryTest { verify(arrangement.userDetailsApi) .suspendFunction(arrangement.userDetailsApi::getUserInfo) .with(any()) - .wasNotInvoked() + .wasInvoked(exactly = once) verify(arrangement.userDAO) .suspendFunction(arrangement.userDAO::upsertTeamMembers) .with(any()) - .wasNotInvoked() + .wasInvoked(exactly = once) verify(arrangement.userDAO) .suspendFunction(arrangement.userDAO::upsertUsers) .with(any()) - .wasNotInvoked() + .wasInvoked(exactly = once) } } @Test - fun givenAKnownFederatedUser_whenGettingFromDbAndCacheValid_thenShouldNOTRefreshItsDataFromAPI() = runTest { + fun givenAKnownUser_whenGettingFromDbAndCacheValid_thenShouldNOTRefreshItsDataFromAPI() = runTest { val (arrangement, userRepository) = Arrangement() - .withUserDaoReturning(TestUser.ENTITY.copy(userType = UserTypeEntity.FEDERATED)) + .withUserDaoReturning(TestUser.ENTITY) .withSuccessfulGetUsersInfo() .arrange()