From 46bc489f1fa42e1fad4433504a58ee827d4b51f9 Mon Sep 17 00:00:00 2001 From: David Perez Date: Thu, 1 Feb 2024 09:42:40 -0600 Subject: [PATCH] Polling on pending requests screen (#942) --- .../data/auth/repository/AuthRepository.kt | 6 ++ .../auth/repository/AuthRepositoryImpl.kt | 14 +++++ .../model/AuthRequestsUpdatesResult.kt | 18 ++++++ .../PendingRequestsViewModel.kt | 26 ++++---- .../auth/repository/AuthRepositoryTest.kt | 43 +++++++++++++ .../PendingRequestsViewModelTest.kt | 61 ++++++++----------- 6 files changed, 119 insertions(+), 49 deletions(-) create mode 100644 app/src/main/java/com/x8bit/bitwarden/data/auth/repository/model/AuthRequestsUpdatesResult.kt diff --git a/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepository.kt b/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepository.kt index 5844f7913..a78323490 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepository.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepository.kt @@ -7,6 +7,7 @@ import com.x8bit.bitwarden.data.auth.repository.model.AuthRequest import com.x8bit.bitwarden.data.auth.repository.model.AuthRequestResult import com.x8bit.bitwarden.data.auth.repository.model.AuthRequestUpdatesResult import com.x8bit.bitwarden.data.auth.repository.model.AuthRequestsResult +import com.x8bit.bitwarden.data.auth.repository.model.AuthRequestsUpdatesResult import com.x8bit.bitwarden.data.auth.repository.model.AuthState import com.x8bit.bitwarden.data.auth.repository.model.BreachCountResult import com.x8bit.bitwarden.data.auth.repository.model.CreateAuthRequestResult @@ -237,6 +238,11 @@ interface AuthRepository : AuthenticatorProvider { */ fun getAuthRequestByIdFlow(requestId: String): Flow + /** + * Get all auth request and emits updates over time. + */ + fun getAuthRequestsWithUpdates(): Flow + /** * Get a list of the current user's [AuthRequest]s. */ diff --git a/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt index f60d6cb44..3cddcb820 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt @@ -36,6 +36,7 @@ import com.x8bit.bitwarden.data.auth.repository.model.AuthRequest import com.x8bit.bitwarden.data.auth.repository.model.AuthRequestResult import com.x8bit.bitwarden.data.auth.repository.model.AuthRequestUpdatesResult import com.x8bit.bitwarden.data.auth.repository.model.AuthRequestsResult +import com.x8bit.bitwarden.data.auth.repository.model.AuthRequestsUpdatesResult import com.x8bit.bitwarden.data.auth.repository.model.AuthState import com.x8bit.bitwarden.data.auth.repository.model.BreachCountResult import com.x8bit.bitwarden.data.auth.repository.model.CreateAuthRequestResult @@ -824,6 +825,19 @@ class AuthRepositoryImpl( mutableSsoCallbackResultFlow.tryEmit(result) } + override fun getAuthRequestsWithUpdates(): Flow = flow { + while (currentCoroutineContext().isActive) { + when (val result = getAuthRequests()) { + AuthRequestsResult.Error -> emit(AuthRequestsUpdatesResult.Error) + + is AuthRequestsResult.Success -> { + emit(AuthRequestsUpdatesResult.Update(authRequests = result.authRequests)) + } + } + delay(timeMillis = PASSWORDLESS_APPROVER_INTERVAL_MILLIS) + } + } + @Suppress("LongMethod") override fun createAuthRequestWithUpdates( email: String, diff --git a/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/model/AuthRequestsUpdatesResult.kt b/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/model/AuthRequestsUpdatesResult.kt new file mode 100644 index 000000000..a6435694a --- /dev/null +++ b/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/model/AuthRequestsUpdatesResult.kt @@ -0,0 +1,18 @@ +package com.x8bit.bitwarden.data.auth.repository.model + +/** + * Models result of an authorization approval request. + */ +sealed class AuthRequestsUpdatesResult { + /** + * Models the data returned when creating an auth request. + */ + data class Update( + val authRequests: List, + ) : AuthRequestsUpdatesResult() + + /** + * There was an error getting the user's auth requests. + */ + data object Error : AuthRequestsUpdatesResult() +} diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/pendingrequests/PendingRequestsViewModel.kt b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/pendingrequests/PendingRequestsViewModel.kt index 4ef0f1a05..3cb5a2b41 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/pendingrequests/PendingRequestsViewModel.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/pendingrequests/PendingRequestsViewModel.kt @@ -5,12 +5,13 @@ import androidx.lifecycle.SavedStateHandle import androidx.lifecycle.viewModelScope import com.x8bit.bitwarden.data.auth.repository.AuthRepository import com.x8bit.bitwarden.data.auth.repository.model.AuthRequest -import com.x8bit.bitwarden.data.auth.repository.model.AuthRequestsResult +import com.x8bit.bitwarden.data.auth.repository.model.AuthRequestsUpdatesResult import com.x8bit.bitwarden.data.platform.repository.SettingsRepository import com.x8bit.bitwarden.ui.platform.base.BaseViewModel import com.x8bit.bitwarden.ui.platform.base.util.Text import com.x8bit.bitwarden.ui.platform.base.util.isOverFiveMinutesOld import dagger.hilt.android.lifecycle.HiltViewModel +import kotlinx.coroutines.Job import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.onEach @@ -38,6 +39,8 @@ class PendingRequestsViewModel @Inject constructor( isPullToRefreshSettingEnabled = settingsRepository.getPullToRefreshEnabledFlow().value, ), ) { + private var authJob: Job = Job().apply { complete() } + private val dateTimeFormatter get() = DateTimeFormatter .ofPattern("M/d/yy hh:mm a") @@ -126,8 +129,8 @@ class PendingRequestsViewModel @Inject constructor( private fun handleAuthRequestsResultReceived( action: PendingRequestsAction.Internal.AuthRequestsResultReceive, ) { - when (val result = action.authRequestsResult) { - is AuthRequestsResult.Success -> { + when (val result = action.authRequestsUpdatesResult) { + is AuthRequestsUpdatesResult.Update -> { val requests = result .authRequests .filterRespondedAndExpired() @@ -160,7 +163,7 @@ class PendingRequestsViewModel @Inject constructor( } } - AuthRequestsResult.Error -> { + AuthRequestsUpdatesResult.Error -> { mutableStateFlow.update { it.copy( authRequests = emptyList(), @@ -173,13 +176,12 @@ class PendingRequestsViewModel @Inject constructor( } private fun updateAuthRequestList() { - viewModelScope.launch { - trySendAction( - PendingRequestsAction.Internal.AuthRequestsResultReceive( - authRequestsResult = authRepository.getAuthRequests(), - ), - ) - } + authJob.cancel() + authJob = authRepository + .getAuthRequestsWithUpdates() + .map { PendingRequestsAction.Internal.AuthRequestsResultReceive(it) } + .onEach(::sendAction) + .launchIn(viewModelScope) } } @@ -332,7 +334,7 @@ sealed class PendingRequestsAction { * Indicates that a new auth requests result has been received. */ data class AuthRequestsResultReceive( - val authRequestsResult: AuthRequestsResult, + val authRequestsUpdatesResult: AuthRequestsUpdatesResult, ) : Internal() } } diff --git a/app/src/test/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryTest.kt index 95dbcd825..19985a073 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryTest.kt @@ -46,6 +46,7 @@ import com.x8bit.bitwarden.data.auth.repository.model.AuthRequest import com.x8bit.bitwarden.data.auth.repository.model.AuthRequestResult import com.x8bit.bitwarden.data.auth.repository.model.AuthRequestUpdatesResult import com.x8bit.bitwarden.data.auth.repository.model.AuthRequestsResult +import com.x8bit.bitwarden.data.auth.repository.model.AuthRequestsUpdatesResult import com.x8bit.bitwarden.data.auth.repository.model.AuthState import com.x8bit.bitwarden.data.auth.repository.model.BreachCountResult import com.x8bit.bitwarden.data.auth.repository.model.CreateAuthRequestResult @@ -97,7 +98,9 @@ import io.mockk.mockkStatic import io.mockk.runs import io.mockk.unmockkStatic import io.mockk.verify +import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.test.advanceTimeBy import kotlinx.coroutines.test.runTest import kotlinx.serialization.ExperimentalSerializationApi import kotlinx.serialization.json.buildJsonObject @@ -3496,6 +3499,46 @@ class AuthRepositoryTest { } } + @OptIn(ExperimentalCoroutinesApi::class) + @Suppress("MaxLineLength") + @Test + fun `getAuthRequestsWithUpdates should emit error then success and not cancel flow when getAuthRequests fails then succeeds`() = + runTest { + val threeMinutes = 3L * 60L * 1_000L + val authRequests = listOf(AUTH_REQUEST) + val authRequestsResponseJson = AuthRequestsResponseJson( + authRequests = listOf(AUTH_REQUESTS_RESPONSE_JSON_AUTH_RESPONSE), + ) + val expectedOne = AuthRequestsUpdatesResult.Error + val expectedTwo = AuthRequestsUpdatesResult.Update(authRequests = authRequests) + coEvery { + authRequestsService.getAuthRequests() + } returns Throwable("Fail").asFailure() andThen authRequestsResponseJson.asSuccess() + coEvery { + authSdkSource.getUserFingerprint( + email = EMAIL, + publicKey = PUBLIC_KEY, + ) + } returns Result.success(FINGER_PRINT) + fakeAuthDiskSource.userState = SINGLE_USER_STATE_1 + + repository + .getAuthRequestsWithUpdates() + .test { + assertEquals(expectedOne, awaitItem()) + advanceTimeBy(threeMinutes) + expectNoEvents() + advanceTimeBy(threeMinutes) + assertEquals(expectedTwo, awaitItem()) + advanceTimeBy(threeMinutes) + cancelAndIgnoreRemainingEvents() + } + + coVerify(exactly = 2) { + authRequestsService.getAuthRequests() + } + } + @Test fun `getAuthRequests should return failure when service returns failure`() = runTest { coEvery { diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/pendingrequests/PendingRequestsViewModelTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/pendingrequests/PendingRequestsViewModelTest.kt index 329378fed..db0c3c9ef 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/pendingrequests/PendingRequestsViewModelTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/pendingrequests/PendingRequestsViewModelTest.kt @@ -6,13 +6,13 @@ import com.x8bit.bitwarden.data.auth.repository.AuthRepository import com.x8bit.bitwarden.data.auth.repository.model.AuthRequest import com.x8bit.bitwarden.data.auth.repository.model.AuthRequestResult import com.x8bit.bitwarden.data.auth.repository.model.AuthRequestsResult +import com.x8bit.bitwarden.data.auth.repository.model.AuthRequestsUpdatesResult import com.x8bit.bitwarden.data.platform.repository.SettingsRepository +import com.x8bit.bitwarden.data.platform.repository.util.bufferedMutableSharedFlow import com.x8bit.bitwarden.ui.platform.base.BaseViewModelTest -import io.mockk.awaits import io.mockk.coEvery import io.mockk.coVerify import io.mockk.every -import io.mockk.just import io.mockk.mockk import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.test.runTest @@ -26,9 +26,11 @@ import java.util.TimeZone class PendingRequestsViewModelTest : BaseViewModelTest() { + private val mutableAuthRequestsWithUpdatesFlow = + bufferedMutableSharedFlow() private val authRepository = mockk { // This is called during init, anything that cares about this will handle it - coEvery { getAuthRequests() } just awaits + coEvery { getAuthRequestsWithUpdates() } returns mutableAuthRequestsWithUpdatesFlow } private val mutablePullToRefreshStateFlow = MutableStateFlow(false) private val settingsRepository = mockk { @@ -48,16 +50,13 @@ class PendingRequestsViewModelTest : BaseViewModelTest() { } @Test - fun `initial state should be correct and trigger a getAuthRequests call`() { - coEvery { - authRepository.getAuthRequests() - } returns AuthRequestsResult.Success( - authRequests = emptyList(), + fun `init should call getAuthRequestsWithUpdates`() { + createViewModel(state = null) + mutableAuthRequestsWithUpdatesFlow.tryEmit( + AuthRequestsUpdatesResult.Update(authRequests = emptyList()), ) - val viewModel = createViewModel(state = null) - assertEquals(DEFAULT_STATE, viewModel.stateFlow.value) coVerify { - authRepository.getAuthRequests() + authRepository.getAuthRequestsWithUpdates() } } @@ -122,11 +121,6 @@ class PendingRequestsViewModelTest : BaseViewModelTest() { fingerprint = "fingerprint", ), ) - coEvery { - authRepository.getAuthRequests() - } returns AuthRequestsResult.Success( - authRequests = requestList, - ) val expected = DEFAULT_STATE.copy( authRequests = requestList, viewState = PendingRequestsState.ViewState.Content( @@ -140,6 +134,9 @@ class PendingRequestsViewModelTest : BaseViewModelTest() { ), ) val viewModel = createViewModel() + mutableAuthRequestsWithUpdatesFlow.tryEmit( + AuthRequestsUpdatesResult.Update(authRequests = requestList), + ) assertEquals(expected, viewModel.stateFlow.value) } @@ -159,13 +156,11 @@ class PendingRequestsViewModelTest : BaseViewModelTest() { @Test fun `getPendingResults failure with error should update state`() { - coEvery { - authRepository.getAuthRequests() - } returns AuthRequestsResult.Error val expected = DEFAULT_STATE.copy( viewState = PendingRequestsState.ViewState.Error, ) val viewModel = createViewModel() + mutableAuthRequestsWithUpdatesFlow.tryEmit(AuthRequestsUpdatesResult.Error) assertEquals(expected, viewModel.stateFlow.value) } @@ -184,7 +179,7 @@ class PendingRequestsViewModelTest : BaseViewModelTest() { viewModel.trySendAction(PendingRequestsAction.RefreshPull) coVerify(exactly = 2) { // This should be called twice since we also call it on init - authRepository.getAuthRequests() + authRepository.getAuthRequestsWithUpdates() } } @@ -242,14 +237,6 @@ class PendingRequestsViewModelTest : BaseViewModelTest() { originUrl = "www.bitwarden.com", fingerprint = "pantry-overdue-survive-sleep-jab", ) - coEvery { - authRepository.getAuthRequests() - } returns AuthRequestsResult.Success( - authRequests = listOf( - authRequest1, - authRequest2, - ), - ) coEvery { authRepository.updateAuthRequest( requestId = "2", @@ -275,6 +262,9 @@ class PendingRequestsViewModelTest : BaseViewModelTest() { ), ) val viewModel = createViewModel() + mutableAuthRequestsWithUpdatesFlow.tryEmit( + AuthRequestsUpdatesResult.Update(authRequests = listOf(authRequest1, authRequest2)), + ) viewModel.actionChannel.trySend(PendingRequestsAction.DeclineAllRequestsConfirm) coVerify { @@ -343,20 +333,17 @@ class PendingRequestsViewModelTest : BaseViewModelTest() { fingerprint = "pantry-overdue-survive-sleep-jab", ), ) - coEvery { - authRepository.getAuthRequests() - } returns AuthRequestsResult.Success(emptyList()) val viewModel = createViewModel() - + mutableAuthRequestsWithUpdatesFlow.tryEmit( + AuthRequestsUpdatesResult.Update(authRequests = emptyList()), + ) assertEquals( DEFAULT_STATE, viewModel.stateFlow.value, ) - coEvery { - authRepository.getAuthRequests() - } returns AuthRequestsResult.Success( - authRequests = requestList, + mutableAuthRequestsWithUpdatesFlow.tryEmit( + AuthRequestsUpdatesResult.Update(authRequests = requestList), ) val expected = DEFAULT_STATE.copy( authRequests = requestList, @@ -379,7 +366,7 @@ class PendingRequestsViewModelTest : BaseViewModelTest() { assertEquals(expected, viewModel.stateFlow.value) coVerify(exactly = 2) { - authRepository.getAuthRequests() + authRepository.getAuthRequestsWithUpdates() } }