Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Fix Play services install and sign in flow #2924

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
+82 −76
Draft

[WIP] Fix Play services install and sign in flow #2924

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ef6981f
Add logging, syntactic sugar
gino-m Dec 13, 2024
2ed559c
Add debug logging
gino-m Dec 13, 2024
def1b29
Recast "main UI nav request" as "main UI state"
gino-m Dec 13, 2024
0625122
Ensure state is restored on resume
gino-m Dec 13, 2024
576235f
ktfmt
gino-m Dec 13, 2024
6f81a45
Catch cancellation exceptions
gino-m Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 21 additions & 26 deletions ground/src/main/java/com/google/android/ground/MainActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,17 @@ import androidx.compose.runtime.remember
import androidx.compose.runtime.setValue
import androidx.compose.ui.platform.ComposeView
import androidx.core.view.WindowInsetsCompat
import androidx.lifecycle.Lifecycle
import androidx.lifecycle.lifecycleScope
import androidx.lifecycle.repeatOnLifecycle
import androidx.navigation.NavDirections
import androidx.navigation.fragment.NavHostFragment
import com.google.android.ground.MainUiState.HomeScreen
import com.google.android.ground.MainUiState.PermissionDeniedDialog
import com.google.android.ground.MainUiState.SignInProgressDialog
import com.google.android.ground.MainUiState.SignInScreen
import com.google.android.ground.MainUiState.SurveySelector
import com.google.android.ground.MainUiState.TermsOfService
import com.google.android.ground.databinding.MainActBinding
import com.google.android.ground.repository.UserRepository
import com.google.android.ground.system.ActivityCallback
Expand Down Expand Up @@ -83,34 +91,25 @@ class MainActivity : AbstractActivity() {
viewModel = viewModelFactory[this, MainViewModel::class.java]

lifecycleScope.launch {
viewModel.navigationRequests.filterNotNull().collect { updateUi(binding.root, it) }
repeatOnLifecycle(Lifecycle.State.RESUMED) {
viewModel.mainUiState.filterNotNull().collect { updateUi(binding.root, it) }
}
}
}

private fun updateUi(viewGroup: ViewGroup, uiState: MainUiState) {
when (uiState) {
MainUiState.OnPermissionDenied -> {
showPermissionDeniedDialog(viewGroup)
}
MainUiState.OnUserSignedOut -> {
navigateTo(SignInFragmentDirections.showSignInScreen())
}
MainUiState.TosNotAccepted -> {
navigateTo(SignInFragmentDirections.showTermsOfService(false))
}
MainUiState.NoActiveSurvey -> {
navigateTo(SurveySelectorFragmentDirections.showSurveySelectorScreen(true))
}
MainUiState.ShowHomeScreen -> {
navigateTo(HomeScreenFragmentDirections.showHomeScreen())
}
MainUiState.OnUserSigningIn -> {
onSignInProgress(true)
}
Timber.d("UI state changed: $uiState")
if (uiState != SignInProgressDialog) {
dismissSignInDialog()
}

if (uiState != MainUiState.OnUserSigningIn) {
onSignInProgress(false)
when (uiState) {
PermissionDeniedDialog -> showPermissionDeniedDialog(viewGroup)
SignInScreen -> navigateTo(SignInFragmentDirections.showSignInScreen())
TermsOfService -> navigateTo(SignInFragmentDirections.showTermsOfService(false))
SurveySelector -> navigateTo(SurveySelectorFragmentDirections.showSurveySelectorScreen(true))
HomeScreen -> navigateTo(HomeScreenFragmentDirections.showHomeScreen())
SignInProgressDialog -> showSignInDialog()
}
}

Expand Down Expand Up @@ -193,10 +192,6 @@ class MainActivity : AbstractActivity() {
return currentFragment is BackPressListener && currentFragment.onBack()
}

private fun onSignInProgress(visible: Boolean) {
if (visible) showSignInDialog() else dismissSignInDialog()
}

private fun showSignInDialog() {
if (signInProgressDialog == null) {
signInProgressDialog = modalSpinner(this, layoutInflater, R.string.signing_in)
Expand Down
12 changes: 6 additions & 6 deletions ground/src/main/java/com/google/android/ground/MainUiState.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,15 @@ package com.google.android.ground

sealed class MainUiState {

data object OnPermissionDenied : MainUiState()
data object PermissionDeniedDialog : MainUiState()

data object OnUserSignedOut : MainUiState()
data object SignInScreen : MainUiState()

data object OnUserSigningIn : MainUiState()
data object SignInProgressDialog : MainUiState()

data object TosNotAccepted : MainUiState()
data object TermsOfService : MainUiState()

data object NoActiveSurvey : MainUiState()
data object SurveySelector : MainUiState()

data object ShowHomeScreen : MainUiState()
data object HomeScreen : MainUiState()
}
55 changes: 28 additions & 27 deletions ground/src/main/java/com/google/android/ground/MainViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import androidx.lifecycle.MutableLiveData
import androidx.lifecycle.viewModelScope
import com.google.android.gms.auth.api.signin.GoogleSignInStatusCodes.SIGN_IN_CANCELLED
import com.google.android.gms.common.api.ApiException
import com.google.android.ground.MainUiState.*
import com.google.android.ground.coroutines.IoDispatcher
import com.google.android.ground.domain.usecases.survey.ReactivateLastSurveyUseCase
import com.google.android.ground.model.User
Expand All @@ -29,14 +30,15 @@ import com.google.android.ground.repository.TermsOfServiceRepository
import com.google.android.ground.repository.UserRepository
import com.google.android.ground.system.auth.AuthenticationManager
import com.google.android.ground.system.auth.SignInState
import com.google.android.ground.system.auth.SignInState.*
import com.google.android.ground.ui.common.AbstractViewModel
import com.google.android.ground.ui.common.SharedViewModel
import com.google.android.ground.util.isPermissionDeniedException
import javax.inject.Inject
import kotlinx.coroutines.CoroutineDispatcher
import kotlinx.coroutines.flow.MutableSharedFlow
import kotlinx.coroutines.flow.SharedFlow
import kotlinx.coroutines.flow.asSharedFlow
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.flow.asStateFlow
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
import timber.log.Timber
Expand All @@ -55,37 +57,36 @@ constructor(
authenticationManager: AuthenticationManager,
) : AbstractViewModel() {

private val _navigationRequests: MutableSharedFlow<MainUiState?> = MutableSharedFlow()
var navigationRequests: SharedFlow<MainUiState?> = _navigationRequests.asSharedFlow()
private val _mainUiState = MutableStateFlow<MainUiState?>(null)
var mainUiState: StateFlow<MainUiState?> = _mainUiState.asStateFlow()

/** The window insets determined by the activity. */
val windowInsets: MutableLiveData<WindowInsetsCompat> = MutableLiveData()

init {
viewModelScope.launch {
// TODO: Check auth status whenever fragments resumes
authenticationManager.signInState.collect {
_navigationRequests.emit(onSignInStateChange(it))
}
}
viewModelScope.launch { authenticationManager.signInState.collect { onSignInStateChange(it) } }
}

private suspend fun onSignInStateChange(signInState: SignInState): MainUiState =
/** Reacts to changes to the authentication state, updating the main UI state accordingly. */
private suspend fun onSignInStateChange(signInState: SignInState) {
Timber.d("Sign in state changed. New state: $signInState")
when (signInState) {
is SignInState.Error -> onUserSignInError(signInState.error)
is SignInState.SignedIn -> onUserSignedIn(signInState.user)
is SignInState.SignedOut -> onUserSignedOut()
is SignInState.SigningIn -> MainUiState.OnUserSigningIn
is SigningIn -> _mainUiState.value = SignInProgressDialog
is SignedIn -> onUserSignedIn(signInState.user)
is SignedOut -> onUserSignedOut()
is Error -> onUserSignInError(signInState.error)
}
}

private fun onUserSignInError(error: Throwable): MainUiState {
Timber.e(error, "Sign in failed")
return if (error.isPermissionDeniedException()) {
MainUiState.OnPermissionDenied
private fun onUserSignInError(error: Throwable) {
if (error.isPermissionDeniedException()) {
Timber.d(error, "User does not have permission to sign in")
_mainUiState.value = PermissionDeniedDialog
} else if (error.isSignInCancelledException()) {
Timber.d("User cancelled sign in")
MainUiState.OnUserSignedOut
_mainUiState.value = SignInScreen
} else {
Timber.d(error, "Unknown sign in error")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make it Timber.e?

// TODO(#1808): Display some error dialog to the user with a helpful user-readable message.
onUserSignedOut()
}
Expand All @@ -94,7 +95,7 @@ constructor(
private fun Throwable.isSignInCancelledException() =
this is ApiException && statusCode == SIGN_IN_CANCELLED

private fun onUserSignedOut(): MainUiState {
private fun onUserSignedOut() {
// Scope of subscription is until view model is cleared. Dispose it manually otherwise, firebase
// attempts to maintain a connection even after user has logged out and throws an error.
surveyRepository.clearActiveSurvey()
Expand All @@ -104,19 +105,19 @@ constructor(
// currently being done to prevent one user's data to be submitted as another user after
// re-login.
viewModelScope.launch { withContext(ioDispatcher) { localDatabase.clearAllTables() } }
return MainUiState.OnUserSignedOut
_mainUiState.value = SignInScreen
}

private suspend fun onUserSignedIn(user: User): MainUiState =
private suspend fun onUserSignedIn(user: User) =
try {
userRepository.saveUserDetails(user)
if (!isTosAccepted()) {
MainUiState.TosNotAccepted
_mainUiState.value = TermsOfService
} else if (!attemptToReactiveLastActiveSurvey()) {
MainUiState.NoActiveSurvey
_mainUiState.value = SurveySelector
} else {
// Everything is fine, show the home screen
MainUiState.ShowHomeScreen
_mainUiState.value = HomeScreen
}
} catch (e: Throwable) {
onUserSignInError(e)
Expand Down
10 changes: 7 additions & 3 deletions ground/src/main/java/com/google/android/ground/system/auth/GoogleAuthenticationManager.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ import com.google.android.ground.coroutines.ApplicationScope
import com.google.android.ground.model.User
import com.google.android.ground.system.ActivityResult
import com.google.android.ground.system.ActivityStreams
import com.google.android.ground.system.auth.SignInState.SignedIn
import com.google.android.ground.system.auth.SignInState.SignedOut
import com.google.firebase.auth.AuthCredential
import com.google.firebase.auth.AuthResult
import com.google.firebase.auth.FirebaseAuth
Expand Down Expand Up @@ -69,9 +71,11 @@ constructor(
override val signInState: Flow<SignInState> = _signInStateFlow.asStateFlow().filterNotNull()

override fun initInternal() {
Timber.d("Listening to Firebase auth state")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we cleanup debug logs if they aren't needed anymore?

firebaseAuth.addAuthStateListener { auth ->
Timber.d("Firebase auth state changed")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

val user = auth.currentUser?.toUser()
setState(if (user == null) SignInState.SignedOut else SignInState.SignedIn(user))
setState(if (user == null) SignedOut else SignedIn(user))
}
}

Expand All @@ -92,7 +96,7 @@ constructor(

override fun signOut() {
firebaseAuth.signOut()
setState(SignInState.SignedOut)
setState(SignedOut)
activityStreams.withActivity { getGoogleSignInClient(it).signOut() }
}

Expand All @@ -119,7 +123,7 @@ constructor(
.addOnFailureListener { setState(SignInState.Error(it)) }

private fun onFirebaseAuthSuccess(authResult: AuthResult) {
setState(SignInState.SignedIn(authResult.user!!.toUser()))
setState(SignedIn(authResult.user!!.toUser()))
}

private fun getFirebaseAuthCredential(googleAccount: GoogleSignInAccount): AuthCredential =
Expand Down
3 changes: 3 additions & 0 deletions ground/src/main/java/com/google/android/ground/ui/startup/StartupFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import com.google.android.ground.ui.common.AbstractFragment
import com.google.android.ground.ui.common.EphemeralPopups
import dagger.hilt.android.AndroidEntryPoint
import javax.inject.Inject
import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.launch
import timber.log.Timber

Expand Down Expand Up @@ -53,6 +54,8 @@ class StartupFragment : AbstractFragment() {
viewLifecycleOwner.lifecycleScope.launch {
try {
viewModel.initializeLogin()
} catch (e: CancellationException) {
Timber.d(e, "Startup job cancelled")
} catch (t: Throwable) {
onInitFailed(t)
}
Expand Down
3 changes: 3 additions & 0 deletions ground/src/main/java/com/google/android/ground/ui/startup/StartupViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import com.google.android.ground.repository.UserRepository
import com.google.android.ground.system.GoogleApiManager
import com.google.android.ground.ui.common.AbstractViewModel
import javax.inject.Inject
import timber.log.Timber

class StartupViewModel
@Inject
Expand All @@ -29,7 +30,9 @@ internal constructor(

/** Initializes the login flow, installing Google Play Services if necessary. */
suspend fun initializeLogin() {
Timber.d("Checking for Play services")
googleApiManager.installGooglePlayServices()
Timber.d("Initializing user repository")
userRepository.init()
}
}
28 changes: 14 additions & 14 deletions ground/src/test/java/com/google/android/ground/MainViewModelTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,11 @@ class MainViewModelTest : BaseHiltTest() {
fun testSignInStateChanged_onSignedOut() = runWithTestDispatcher {
setupUserPreferences()

viewModel.navigationRequests.test {
viewModel.mainUiState.test {
fakeAuthenticationManager.signOut()
advanceUntilIdle()

assertThat(awaitItem()).isEqualTo(MainUiState.OnUserSignedOut)
assertThat(awaitItem()).isEqualTo(MainUiState.SignInScreen)
verifyUserPreferencesCleared()
verifyUserNotSaved()
assertThat(tosRepository.isTermsOfServiceAccepted).isFalse()
Expand All @@ -88,11 +88,11 @@ class MainViewModelTest : BaseHiltTest() {

@Test
fun testSignInStateChanged_onSigningIn() = runWithTestDispatcher {
viewModel.navigationRequests.test {
viewModel.mainUiState.test {
fakeAuthenticationManager.setState(SignInState.SigningIn)
advanceUntilIdle()

assertThat(awaitItem()).isEqualTo(MainUiState.OnUserSigningIn)
assertThat(awaitItem()).isEqualTo(MainUiState.SignInProgressDialog)
verifyUserNotSaved()
assertThat(tosRepository.isTermsOfServiceAccepted).isFalse()
}
Expand All @@ -106,11 +106,11 @@ class MainViewModelTest : BaseHiltTest() {
tosRepository.isTermsOfServiceAccepted = false
fakeRemoteDataStore.termsOfService = Result.success(FakeData.TERMS_OF_SERVICE)

viewModel.navigationRequests.test {
viewModel.mainUiState.test {
fakeAuthenticationManager.signIn()
advanceUntilIdle()

assertThat(awaitItem()).isEqualTo(MainUiState.TosNotAccepted)
assertThat(awaitItem()).isEqualTo(MainUiState.TermsOfService)
verifyUserSaved()
assertThat(tosRepository.isTermsOfServiceAccepted).isFalse()
}
Expand All @@ -121,11 +121,11 @@ class MainViewModelTest : BaseHiltTest() {
tosRepository.isTermsOfServiceAccepted = false
fakeRemoteDataStore.termsOfService = null

viewModel.navigationRequests.test {
viewModel.mainUiState.test {
fakeAuthenticationManager.signIn()
advanceUntilIdle()

assertThat(awaitItem()).isEqualTo(MainUiState.TosNotAccepted)
assertThat(awaitItem()).isEqualTo(MainUiState.TermsOfService)
verifyUserSaved()
assertThat(tosRepository.isTermsOfServiceAccepted).isFalse()
}
Expand All @@ -142,12 +142,12 @@ class MainViewModelTest : BaseHiltTest() {
)
)

viewModel.navigationRequests.test {
viewModel.mainUiState.test {
fakeAuthenticationManager.signIn()
advanceUntilIdle()
// TODO(#2667): Update these implementation to make it clearer why this would be the case.
assertThat(tosRepository.isTermsOfServiceAccepted).isFalse()
assertThat(awaitItem()).isEqualTo(MainUiState.TosNotAccepted)
assertThat(awaitItem()).isEqualTo(MainUiState.TermsOfService)
}
}

Expand All @@ -157,24 +157,24 @@ class MainViewModelTest : BaseHiltTest() {
tosRepository.isTermsOfServiceAccepted = false
fakeRemoteDataStore.termsOfService = Result.failure(Error("user error"))

viewModel.navigationRequests.test {
viewModel.mainUiState.test {
fakeAuthenticationManager.signIn()
advanceUntilIdle()

assertThat(tosRepository.isTermsOfServiceAccepted).isFalse()
assertThat(awaitItem()).isEqualTo(MainUiState.TosNotAccepted)
assertThat(awaitItem()).isEqualTo(MainUiState.TermsOfService)
}
}

@Test
fun testSignInStateChanged_onSignInError() = runWithTestDispatcher {
setupUserPreferences()

viewModel.navigationRequests.test {
viewModel.mainUiState.test {
fakeAuthenticationManager.setState(SignInState.Error(Exception()))
advanceUntilIdle()

assertThat(awaitItem()).isEqualTo(MainUiState.OnUserSignedOut)
assertThat(awaitItem()).isEqualTo(MainUiState.SignInScreen)
verifyUserPreferencesCleared()
verifyUserNotSaved()
assertThat(tosRepository.isTermsOfServiceAccepted).isFalse()
Expand Down
Loading