Always keep sign-in button enabled and prevent race conditions.

google · Mar 1, 2024 · d63f883 · d63f883
1 parent 3dcff36
commit d63f883
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 7 deletions.
diff --git a/ground/src/main/java/com/google/android/ground/ui/signin/SignInFragment.kt b/ground/src/main/java/com/google/android/ground/ui/signin/SignInFragment.kt
@@ -59,7 +59,6 @@ class SignInFragment : AbstractFragment(), BackPressListener {
         if (!connected) {
           displayNetworkError()
         }
-        binding.signInButton.isEnabled = connected
       }
     }
   }

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
@@ -19,13 +19,16 @@ import androidx.lifecycle.viewModelScope
 import com.google.android.ground.repository.UserRepository
 import com.google.android.ground.system.NetworkManager
 import com.google.android.ground.system.NetworkStatus
+import com.google.android.ground.system.auth.SignInState
 import com.google.android.ground.ui.common.AbstractViewModel
-import javax.inject.Inject
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.flow.Flow
 import kotlinx.coroutines.flow.SharingStarted
+import kotlinx.coroutines.flow.first
 import kotlinx.coroutines.flow.mapLatest
 import kotlinx.coroutines.flow.shareIn
+import kotlinx.coroutines.launch
+import javax.inject.Inject
 
 class SignInViewModel
 @Inject
@@ -41,6 +44,12 @@ internal constructor(
       .shareIn(viewModelScope, SharingStarted.Lazily, replay = 0)
 
   fun onSignInButtonClick() {
-    userRepository.signIn()
+    viewModelScope.launch {
+      val signInState = userRepository.signInState.first()
+      when (signInState.state) {
+        SignInState.State.SIGNED_OUT, SignInState.State.ERROR -> userRepository.signIn()
+        else -> {}
+      }
+    }
   }
 }
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
@@ -19,7 +19,7 @@ import androidx.test.espresso.Espresso.onView
 import androidx.test.espresso.action.ViewActions.click
 import androidx.test.espresso.assertion.ViewAssertions.matches
 import androidx.test.espresso.matcher.ViewMatchers.isDisplayed
-import androidx.test.espresso.matcher.ViewMatchers.isNotEnabled
+import androidx.test.espresso.matcher.ViewMatchers.isEnabled
 import androidx.test.espresso.matcher.ViewMatchers.withId
 import androidx.test.espresso.matcher.ViewMatchers.withText
 import app.cash.turbine.test
@@ -34,16 +34,17 @@ import com.sharedtest.FakeData
 import com.sharedtest.system.auth.FakeAuthenticationManager
 import dagger.hilt.android.testing.BindValue
 import dagger.hilt.android.testing.HiltAndroidTest
-import javax.inject.Inject
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.flow.flowOf
 import kotlinx.coroutines.test.advanceUntilIdle
 import org.hamcrest.Matchers.allOf
+import org.junit.Before
 import org.junit.Test
 import org.junit.runner.RunWith
 import org.mockito.Mock
 import org.mockito.kotlin.whenever
 import org.robolectric.RobolectricTestRunner
+import javax.inject.Inject
 
 @OptIn(ExperimentalCoroutinesApi::class)
 @HiltAndroidTest
@@ -54,6 +55,7 @@ class SignInFragmentTest : BaseHiltTest() {
 
   @Inject lateinit var fakeAuthenticationManager: FakeAuthenticationManager
 
+  @Before
   override fun setUp() {
     super.setUp()
     fakeAuthenticationManager.setState(SignInState.signedOut())
@@ -76,12 +78,12 @@ class SignInFragmentTest : BaseHiltTest() {
     }
 
   @Test
-  fun `Sign-in button should be disabled when network is not available`() = runWithTestDispatcher {
+  fun `Sign-in button should be enabled when network is not available`() = runWithTestDispatcher {
     whenever(networkManager.networkStatusFlow).thenReturn(flowOf(NetworkStatus.UNAVAILABLE))
     launchFragmentInHiltContainer<SignInFragment>()
     fakeAuthenticationManager.setUser(TEST_USER)
 
-    onView(allOf(withId(R.id.sign_in_button), isDisplayed(), isNotEnabled())).perform(click())
+    onView(allOf(withId(R.id.sign_in_button), isDisplayed(), isEnabled())).perform(click())
 
     // Assert that the sign-in state is still signed out
     fakeAuthenticationManager.signInState.test {
@@ -93,6 +95,23 @@ class SignInFragmentTest : BaseHiltTest() {
       .check(matches(withText(R.string.network_error_when_signing_in)))
   }
 
+  @Test
+  fun `Sign-in should only execute once when clicked multiple times`() = runWithTestDispatcher {
+    whenever(networkManager.networkStatusFlow).thenReturn(flowOf(NetworkStatus.AVAILABLE))
+    launchFragmentInHiltContainer<SignInFragment>()
+    fakeAuthenticationManager.setUser(TEST_USER)
+
+    onView(withId(R.id.sign_in_button)).perform(click())
+    onView(withId(R.id.sign_in_button)).perform(click())
+    advanceUntilIdle()
+
+    fakeAuthenticationManager.signInState.test {
+      assertThat(awaitItem())
+        .isEqualTo(SignInState(SignInState.State.SIGNED_IN, Result.success(TEST_USER)))
+      // Fails if there are further emitted sign-in events.
+    }
+  }
+
   @Test
   fun `Back press should finish activity`() {
     launchFragmentInHiltContainer<SignInFragment> {
-Original file line number
+Diff line change
@@ Expand Up @@
             if (!connected) {
               displayNetworkError()
             }
-            binding.signInButton.isEnabled = connected
           }
         }
       }
@@ Expand Down @@