From ca5166217a5c2edb1ac3bf4dca34daa9620990a6 Mon Sep 17 00:00:00 2001 From: Shobhit Agarwal Date: Mon, 24 Jun 2024 18:02:39 +0530 Subject: [PATCH] Convert SignInState to sealed data class (#2506) * Convert SignInState to sealed data class * Use the value from SignedIn state when saving the user instead of getting from auth manager * Use the same instance for refreshing remote profile as well * Fix import order --- .../google/android/ground/MainViewModel.kt | 23 ++++++++----------- .../ground/repository/UserRepository.kt | 12 +++++----- .../auth/AnonymousAuthenticationManager.kt | 10 ++++---- .../system/auth/BaseAuthenticationManager.kt | 8 ++----- .../auth/GoogleAuthenticationManager.kt | 12 +++++----- .../android/ground/system/auth/SignInState.kt | 20 ++++------------ .../ground/ui/signin/SignInViewModel.kt | 8 +++---- .../google/android/ground/MainActivityTest.kt | 6 ++--- .../android/ground/MainViewModelTest.kt | 7 +++--- .../LocationOfInterestRepositoryTest.kt | 2 +- .../ground/repository/UserRepositoryTest.kt | 9 +++----- .../ground/ui/signin/SignInFragmentTest.kt | 11 ++++----- .../system/auth/FakeAuthenticationManager.kt | 8 +++---- 13 files changed, 54 insertions(+), 82 deletions(-) diff --git a/ground/src/main/java/com/google/android/ground/MainViewModel.kt b/ground/src/main/java/com/google/android/ground/MainViewModel.kt index 029bad4c98..b84879af8c 100644 --- a/ground/src/main/java/com/google/android/ground/MainViewModel.kt +++ b/ground/src/main/java/com/google/android/ground/MainViewModel.kt @@ -21,6 +21,7 @@ import androidx.lifecycle.viewModelScope import androidx.navigation.NavDirections import com.google.android.ground.coroutines.IoDispatcher import com.google.android.ground.domain.usecases.survey.ReactivateLastSurveyUseCase +import com.google.android.ground.model.User import com.google.android.ground.persistence.local.room.LocalDatabase import com.google.android.ground.repository.SurveyRepository import com.google.android.ground.repository.TermsOfServiceRepository @@ -79,18 +80,14 @@ constructor( private suspend fun onSignInStateChange(signInState: SignInState): NavDirections? { // Display progress only when signing in. - signInProgressDialogVisibility.postValue(signInState.state == SignInState.State.SIGNING_IN) + signInProgressDialogVisibility.postValue(signInState == SignInState.SigningIn) - return signInState.result.fold( - { - when (signInState.state) { - SignInState.State.SIGNED_IN -> onUserSignedIn() - SignInState.State.SIGNED_OUT -> onUserSignedOut() - else -> null - } - }, - { onUserSignInError(it) }, - ) + return when (signInState) { + is SignInState.Error -> onUserSignInError(signInState.error) + is SignInState.SignedIn -> onUserSignedIn(signInState.user) + is SignInState.SignedOut -> onUserSignedOut() + is SignInState.SigningIn -> null + } } private suspend fun onUserSignInError(error: Throwable): NavDirections? { @@ -118,9 +115,9 @@ constructor( return SignInFragmentDirections.showSignInScreen() } - private suspend fun onUserSignedIn(): NavDirections? = + private suspend fun onUserSignedIn(user: User): NavDirections? = try { - userRepository.saveUserDetails() + userRepository.saveUserDetails(user) val tos = termsOfServiceRepository.getTermsOfService() if (tos == null || termsOfServiceRepository.isTermsOfServiceAccepted) { reactivateLastSurvey() diff --git a/ground/src/main/java/com/google/android/ground/repository/UserRepository.kt b/ground/src/main/java/com/google/android/ground/repository/UserRepository.kt index e83617fa55..0e68b5b454 100644 --- a/ground/src/main/java/com/google/android/ground/repository/UserRepository.kt +++ b/ground/src/main/java/com/google/android/ground/repository/UserRepository.kt @@ -52,19 +52,19 @@ constructor( suspend fun getAuthenticatedUser() = authenticationManager.getAuthenticatedUser() - /** Stores the current user's profile details into the local and remote dbs. */ - suspend fun saveUserDetails() { - localUserStore.insertOrUpdateUser(getAuthenticatedUser()) - updateRemoteUserInfo() + /** Stores the given user to the local and remote dbs. */ + suspend fun saveUserDetails(user: User) { + localUserStore.insertOrUpdateUser(user) + updateRemoteUserInfo(user) } /** Attempts to refresh current user's profile in remote database if network is available. */ - private suspend fun updateRemoteUserInfo() { + private suspend fun updateRemoteUserInfo(user: User) { if (!networkManager.isNetworkConnected()) { Timber.d("Skipped refreshing user profile as device is offline.") return } - if (!getAuthenticatedUser().isAnonymous) { + if (!user.isAnonymous) { runCatching { remoteDataStore.refreshUserProfile() } .fold( { Timber.i("Profile refreshed") }, diff --git a/ground/src/main/java/com/google/android/ground/system/auth/AnonymousAuthenticationManager.kt b/ground/src/main/java/com/google/android/ground/system/auth/AnonymousAuthenticationManager.kt index d4d7f5ac43..3aad021923 100644 --- a/ground/src/main/java/com/google/android/ground/system/auth/AnonymousAuthenticationManager.kt +++ b/ground/src/main/java/com/google/android/ground/system/auth/AnonymousAuthenticationManager.kt @@ -43,8 +43,8 @@ constructor( override fun initInternal() { setState( - if (firebaseAuth.currentUser == null) SignInState.signedOut() - else SignInState.signedIn(anonymousUser) + if (firebaseAuth.currentUser == null) SignInState.SignedOut + else SignInState.SignedIn(anonymousUser) ) } @@ -53,13 +53,13 @@ constructor( } override fun signIn() { - setState(SignInState.signingIn()) + setState(SignInState.SigningIn) externalScope.launch { firebaseAuth.signInAnonymously().await() } - setState(SignInState.signedIn(anonymousUser)) + setState(SignInState.SignedIn(anonymousUser)) } override fun signOut() { firebaseAuth.signOut() - setState(SignInState.signedOut()) + setState(SignInState.SignedOut) } } diff --git a/ground/src/main/java/com/google/android/ground/system/auth/BaseAuthenticationManager.kt b/ground/src/main/java/com/google/android/ground/system/auth/BaseAuthenticationManager.kt index b35ea71528..b970e63f7c 100644 --- a/ground/src/main/java/com/google/android/ground/system/auth/BaseAuthenticationManager.kt +++ b/ground/src/main/java/com/google/android/ground/system/auth/BaseAuthenticationManager.kt @@ -16,9 +16,8 @@ package com.google.android.ground.system.auth import com.google.android.ground.model.User -import kotlinx.coroutines.flow.filter +import kotlinx.coroutines.flow.filterIsInstance import kotlinx.coroutines.flow.first -import kotlinx.coroutines.flow.mapNotNull abstract class BaseAuthenticationManager : AuthenticationManager { @@ -36,9 +35,6 @@ abstract class BaseAuthenticationManager : AuthenticationManager { init() } - return signInState - .filter { it.state == SignInState.State.SIGNED_IN } - .mapNotNull { it.result.getOrNull() } - .first() + return signInState.filterIsInstance().first().user } } diff --git a/ground/src/main/java/com/google/android/ground/system/auth/GoogleAuthenticationManager.kt b/ground/src/main/java/com/google/android/ground/system/auth/GoogleAuthenticationManager.kt index ff1bfc57fe..8035bec4cb 100644 --- a/ground/src/main/java/com/google/android/ground/system/auth/GoogleAuthenticationManager.kt +++ b/ground/src/main/java/com/google/android/ground/system/auth/GoogleAuthenticationManager.kt @@ -71,7 +71,7 @@ constructor( override fun initInternal() { firebaseAuth.addAuthStateListener { auth -> val user = auth.currentUser?.toUser() - setState(if (user == null) SignInState.signedOut() else SignInState.signedIn(user)) + setState(if (user == null) SignInState.SignedOut else SignInState.SignedIn(user)) } } @@ -80,7 +80,7 @@ constructor( } override fun signIn() { - setState(SignInState.signingIn()) + setState(SignInState.SigningIn) showSignInDialog() } @@ -92,7 +92,7 @@ constructor( override fun signOut() { firebaseAuth.signOut() - setState(SignInState.signedOut()) + setState(SignInState.SignedOut) activityStreams.withActivity { getGoogleSignInClient(it).signOut() } } @@ -108,7 +108,7 @@ constructor( googleSignInTask.getResult(ApiException::class.java)?.let { onGoogleSignIn(it) } } catch (e: ApiException) { Timber.e(e, "Sign in failed") - setState(SignInState.error(e)) + setState(SignInState.Error(e)) } } @@ -116,10 +116,10 @@ constructor( firebaseAuth .signInWithCredential(getFirebaseAuthCredential(googleAccount)) .addOnSuccessListener { authResult: AuthResult -> onFirebaseAuthSuccess(authResult) } - .addOnFailureListener { setState(SignInState.error(it)) } + .addOnFailureListener { setState(SignInState.Error(it)) } private fun onFirebaseAuthSuccess(authResult: AuthResult) { - setState(SignInState.signedIn(authResult.user!!.toUser())) + setState(SignInState.SignedIn(authResult.user!!.toUser())) } private fun getFirebaseAuthCredential(googleAccount: GoogleSignInAccount): AuthCredential = diff --git a/ground/src/main/java/com/google/android/ground/system/auth/SignInState.kt b/ground/src/main/java/com/google/android/ground/system/auth/SignInState.kt index 8df6c0802f..52b98f0a26 100644 --- a/ground/src/main/java/com/google/android/ground/system/auth/SignInState.kt +++ b/ground/src/main/java/com/google/android/ground/system/auth/SignInState.kt @@ -17,23 +17,13 @@ package com.google.android.ground.system.auth import com.google.android.ground.model.User -data class SignInState(val state: State, val result: Result) { +sealed class SignInState { - enum class State { - SIGNED_OUT, - SIGNING_IN, - SIGNED_IN, - ERROR, - } + data object SignedOut : SignInState() - companion object { + data object SigningIn : SignInState() - fun signedOut() = SignInState(State.SIGNED_OUT, Result.success(null)) + data class SignedIn(val user: User) : SignInState() - fun signingIn() = SignInState(State.SIGNING_IN, Result.success(null)) - - fun signedIn(user: User) = SignInState(State.SIGNED_IN, Result.success(user)) - - fun error(error: Throwable) = SignInState(State.ERROR, Result.failure(error)) - } + data class Error(val error: Throwable) : SignInState() } diff --git a/ground/src/main/java/com/google/android/ground/ui/signin/SignInViewModel.kt b/ground/src/main/java/com/google/android/ground/ui/signin/SignInViewModel.kt index 04f4350a3c..340ce53994 100644 --- a/ground/src/main/java/com/google/android/ground/ui/signin/SignInViewModel.kt +++ b/ground/src/main/java/com/google/android/ground/ui/signin/SignInViewModel.kt @@ -45,11 +45,9 @@ internal constructor( fun onSignInButtonClick() { viewModelScope.launch { - val signInState = userRepository.getSignInState().first() - when (signInState.state) { - SignInState.State.SIGNED_OUT, - SignInState.State.ERROR -> userRepository.signIn() - else -> {} + val state = userRepository.getSignInState().first() + if (state is SignInState.SignedOut || state is SignInState.Error) { + userRepository.signIn() } } } diff --git a/ground/src/test/java/com/google/android/ground/MainActivityTest.kt b/ground/src/test/java/com/google/android/ground/MainActivityTest.kt index bc33c8e99e..d751ec61d8 100644 --- a/ground/src/test/java/com/google/android/ground/MainActivityTest.kt +++ b/ground/src/test/java/com/google/android/ground/MainActivityTest.kt @@ -47,7 +47,7 @@ class MainActivityTest : BaseHiltTest() { controller.setup() // Moves Activity to RESUMED state activity = controller.get() - fakeAuthenticationManager.setState(SignInState.signingIn()) + fakeAuthenticationManager.setState(SignInState.SigningIn) advanceUntilIdle() assertThat(ShadowProgressDialog.getLatestDialog().isShowing).isTrue() @@ -60,8 +60,8 @@ class MainActivityTest : BaseHiltTest() { controller.setup() // Moves Activity to RESUMED state activity = controller.get() - fakeAuthenticationManager.setState(SignInState.signingIn()) - fakeAuthenticationManager.setState(SignInState.signedOut()) + fakeAuthenticationManager.setState(SignInState.SigningIn) + fakeAuthenticationManager.setState(SignInState.SignedOut) advanceUntilIdle() assertThat(ShadowProgressDialog.getLatestDialog().isShowing).isFalse() diff --git a/ground/src/test/java/com/google/android/ground/MainViewModelTest.kt b/ground/src/test/java/com/google/android/ground/MainViewModelTest.kt index c65279333b..effe4f2bd3 100644 --- a/ground/src/test/java/com/google/android/ground/MainViewModelTest.kt +++ b/ground/src/test/java/com/google/android/ground/MainViewModelTest.kt @@ -20,8 +20,7 @@ import app.cash.turbine.test import com.google.android.ground.persistence.local.room.LocalDataStoreException import com.google.android.ground.repository.TermsOfServiceRepository import com.google.android.ground.repository.UserRepository -import com.google.android.ground.system.auth.SignInState.Companion.error -import com.google.android.ground.system.auth.SignInState.Companion.signingIn +import com.google.android.ground.system.auth.SignInState import com.google.android.ground.ui.common.Navigator import com.google.android.ground.ui.signin.SignInFragmentDirections import com.google.common.truth.Truth.assertThat @@ -95,7 +94,7 @@ class MainViewModelTest : BaseHiltTest() { @Test fun testSignInStateChanged_onSigningIn() = runWithTestDispatcher { testNoNavigation(navigator.getNavigateRequests()) { - fakeAuthenticationManager.setState(signingIn()) + fakeAuthenticationManager.setState(SignInState.SigningIn) } verifyProgressDialogVisible(true) @@ -180,7 +179,7 @@ class MainViewModelTest : BaseHiltTest() { setupUserPreferences() testNavigateTo(navigator.getNavigateRequests(), SignInFragmentDirections.showSignInScreen()) { - fakeAuthenticationManager.setState(error(Exception())) + fakeAuthenticationManager.setState(SignInState.Error(Exception())) } verifyProgressDialogVisible(false) diff --git a/ground/src/test/java/com/google/android/ground/repository/LocationOfInterestRepositoryTest.kt b/ground/src/test/java/com/google/android/ground/repository/LocationOfInterestRepositoryTest.kt index f0ca1b5279..17e9edff4c 100644 --- a/ground/src/test/java/com/google/android/ground/repository/LocationOfInterestRepositoryTest.kt +++ b/ground/src/test/java/com/google/android/ground/repository/LocationOfInterestRepositoryTest.kt @@ -66,7 +66,7 @@ class LocationOfInterestRepositoryTest : BaseHiltTest() { runWithTestDispatcher { // Setup user fakeAuthenticationManager.setUser(TEST_USER) - userRepository.saveUserDetails() + userRepository.saveUserDetails(TEST_USER) // Setup survey and LOIs fakeRemoteDataStore.surveys = listOf(TEST_SURVEY) diff --git a/ground/src/test/java/com/google/android/ground/repository/UserRepositoryTest.kt b/ground/src/test/java/com/google/android/ground/repository/UserRepositoryTest.kt index 39211bd33b..b4f940519a 100644 --- a/ground/src/test/java/com/google/android/ground/repository/UserRepositoryTest.kt +++ b/ground/src/test/java/com/google/android/ground/repository/UserRepositoryTest.kt @@ -59,20 +59,18 @@ class UserRepositoryTest : BaseHiltTest() { @Test fun `saveUserDetails() updates local user profile`() = runWithTestDispatcher { - fakeAuthenticationManager.setUser(FakeData.USER) whenever(networkManager.isNetworkConnected()).thenReturn(true) - userRepository.saveUserDetails() + userRepository.saveUserDetails(FakeData.USER) assertThat(localUserStore.getUser(FakeData.USER.id)).isEqualTo(FakeData.USER) } @Test fun `saveUserDetails() updates remote user profile`() = runWithTestDispatcher { - fakeAuthenticationManager.setUser(FakeData.USER) whenever(networkManager.isNetworkConnected()).thenReturn(true) - userRepository.saveUserDetails() + userRepository.saveUserDetails(FakeData.USER) assertThat(fakeRemoteDataStore.userProfileRefreshCount).isEqualTo(1) } @@ -80,10 +78,9 @@ class UserRepositoryTest : BaseHiltTest() { @Test fun `saveUserDetails() doesn't update remote user profile when offline `() = runWithTestDispatcher { - fakeAuthenticationManager.setUser(FakeData.USER) whenever(networkManager.isNetworkConnected()).thenReturn(false) - userRepository.saveUserDetails() + userRepository.saveUserDetails(FakeData.USER) assertThat(fakeRemoteDataStore.userProfileRefreshCount).isEqualTo(0) } diff --git a/ground/src/test/java/com/google/android/ground/ui/signin/SignInFragmentTest.kt b/ground/src/test/java/com/google/android/ground/ui/signin/SignInFragmentTest.kt index 6867ffac10..8b45b29ffb 100644 --- a/ground/src/test/java/com/google/android/ground/ui/signin/SignInFragmentTest.kt +++ b/ground/src/test/java/com/google/android/ground/ui/signin/SignInFragmentTest.kt @@ -59,7 +59,7 @@ class SignInFragmentTest : BaseHiltTest() { @Before override fun setUp() { super.setUp() - fakeAuthenticationManager.setState(SignInState.signedOut()) + fakeAuthenticationManager.setState(SignInState.SignedOut) } @Test @@ -73,8 +73,7 @@ class SignInFragmentTest : BaseHiltTest() { advanceUntilIdle() fakeAuthenticationManager.signInState.test { - assertThat(expectMostRecentItem()) - .isEqualTo(SignInState(SignInState.State.SIGNED_IN, Result.success(TEST_USER))) + assertThat(expectMostRecentItem()).isEqualTo(SignInState.SignedIn(TEST_USER)) } } @@ -88,8 +87,7 @@ class SignInFragmentTest : BaseHiltTest() { // Assert that the sign-in state is still signed out fakeAuthenticationManager.signInState.test { - assertThat(expectMostRecentItem()) - .isEqualTo(SignInState(SignInState.State.SIGNED_OUT, Result.success(null))) + assertThat(expectMostRecentItem()).isEqualTo(SignInState.SignedOut) } onView(withId(com.google.android.material.R.id.snackbar_text)) @@ -107,8 +105,7 @@ class SignInFragmentTest : BaseHiltTest() { advanceUntilIdle() fakeAuthenticationManager.signInState.test { - assertThat(awaitItem()) - .isEqualTo(SignInState(SignInState.State.SIGNED_IN, Result.success(TEST_USER))) + assertThat(awaitItem()).isEqualTo(SignInState.SignedIn(TEST_USER)) // Fails if there are further emitted sign-in events. } } diff --git a/sharedTest/src/main/kotlin/com/sharedtest/system/auth/FakeAuthenticationManager.kt b/sharedTest/src/main/kotlin/com/sharedtest/system/auth/FakeAuthenticationManager.kt index af0ad19a38..1e48dc018a 100644 --- a/sharedTest/src/main/kotlin/com/sharedtest/system/auth/FakeAuthenticationManager.kt +++ b/sharedTest/src/main/kotlin/com/sharedtest/system/auth/FakeAuthenticationManager.kt @@ -19,8 +19,6 @@ import com.google.android.ground.coroutines.ApplicationScope import com.google.android.ground.model.User import com.google.android.ground.system.auth.BaseAuthenticationManager import com.google.android.ground.system.auth.SignInState -import com.google.android.ground.system.auth.SignInState.Companion.signedIn -import com.google.android.ground.system.auth.SignInState.Companion.signedOut import com.sharedtest.FakeData import javax.inject.Inject import javax.inject.Singleton @@ -52,9 +50,9 @@ constructor(@ApplicationScope private val externalScope: CoroutineScope) : externalScope.launch { _signInStateFlow.emit(state) } } - override fun initInternal() = setState(signedIn(currentUser)) + override fun initInternal() = setState(SignInState.SignedIn(currentUser)) - override fun signIn() = setState(signedIn(currentUser)) + override fun signIn() = setState(SignInState.SignedIn(currentUser)) - override fun signOut() = setState(signedOut()) + override fun signOut() = setState(SignInState.SignedOut) }