-
Notifications
You must be signed in to change notification settings - Fork 151
Improve force update to also suspend firebase requests #3936
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
47856d7
de776f8
139d5bc
ab178b6
89790bb
c70ad34
b75c979
3260492
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,8 @@ import dagger.Module | |
| import dagger.Provides | ||
| import dagger.hilt.InstallIn | ||
| import dagger.hilt.components.SingletonComponent | ||
| import org.groundplatform.android.BuildConfig | ||
| import org.groundplatform.domain.repository.AppConfigRepositoryInterface | ||
| import org.groundplatform.domain.repository.LocationOfInterestRepositoryInterface | ||
| import org.groundplatform.domain.repository.MapStateRepositoryInterface | ||
| import org.groundplatform.domain.repository.OfflineAreaRepositoryInterface | ||
|
|
@@ -125,4 +127,11 @@ object UseCaseModule { | |
| surveyRepository: SurveyRepositoryInterface, | ||
| userRepository: UserRepositoryInterface, | ||
| ) = ListAvailableSurveysUseCase(networkManager, surveyRepository, userRepository) | ||
|
|
||
| @Provides | ||
| fun providesShouldForceUpdateUseCase(appConfigRepositoryInterface: AppConfigRepositoryInterface) = | ||
| org.groundplatform.domain.usecases.ShouldForceUpdateUseCase( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Can we import the use case instead of using fully qualified imports? |
||
| appConfigRepository = appConfigRepositoryInterface, | ||
| currentVersion = BuildConfig.VERSION_NAME, | ||
| ) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| /* | ||
| * Copyright 2026 Google LLC | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.groundplatform.android.repository | ||
|
|
||
| import com.google.firebase.remoteconfig.FirebaseRemoteConfig | ||
| import javax.inject.Inject | ||
| import javax.inject.Singleton | ||
| import org.groundplatform.domain.model.AppConfig | ||
| import org.groundplatform.domain.repository.AppConfigRepositoryInterface | ||
|
|
||
| /** Reads the [AppConfig] from Firebase Remote Config. */ | ||
| @Singleton | ||
| class AppConfigRepository @Inject constructor(private val remoteConfig: FirebaseRemoteConfig) : | ||
| AppConfigRepositoryInterface { | ||
|
|
||
| override fun getAppConfig(): AppConfig = | ||
| AppConfig( | ||
| minAppVersion = remoteConfig.getString("min_app_version"), | ||
| forceUpdate = remoteConfig.getBoolean("force_update"), | ||
| ) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -161,7 +161,7 @@ class MainActivity : AbstractActivity() { | |
|
|
||
| override fun onResume() { | ||
| super.onResume() | ||
| if (viewModel.isAppUpdateAvailable()) { | ||
| if (viewModel.isAppUpdateRequired()) { | ||
| showForceUpdateDialog() | ||
|
Comment on lines
+164
to
165
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please see the other comment. |
||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,6 @@ package org.groundplatform.android.ui.main | |
|
|
||
| import android.net.Uri | ||
| import androidx.lifecycle.viewModelScope | ||
| import com.google.firebase.remoteconfig.FirebaseRemoteConfig | ||
| import javax.inject.Inject | ||
| import kotlinx.coroutines.CoroutineDispatcher | ||
| import kotlinx.coroutines.channels.Channel | ||
|
|
@@ -26,7 +25,7 @@ import kotlinx.coroutines.flow.MutableStateFlow | |
| import kotlinx.coroutines.flow.receiveAsFlow | ||
| import kotlinx.coroutines.launch | ||
| import kotlinx.coroutines.withContext | ||
| import org.groundplatform.android.BuildConfig | ||
| import org.groundplatform.android.data.remote.UpdateRequiredException | ||
| import org.groundplatform.android.di.coroutines.IoDispatcher | ||
| import org.groundplatform.android.system.auth.AuthenticationManager | ||
| import org.groundplatform.android.system.deeplink.PlayInstallReferrerService | ||
|
|
@@ -37,6 +36,7 @@ import org.groundplatform.domain.model.User | |
| import org.groundplatform.domain.model.auth.SignInState | ||
| import org.groundplatform.domain.repository.TermsOfServiceRepositoryInterface | ||
| import org.groundplatform.domain.repository.UserRepositoryInterface | ||
| import org.groundplatform.domain.usecases.ShouldForceUpdateUseCase | ||
| import org.groundplatform.domain.usecases.survey.ReactivateLastSurveyUseCase | ||
| import org.groundplatform.domain.usecases.user.ClearUserSessionUseCase | ||
| import timber.log.Timber | ||
|
|
@@ -52,7 +52,7 @@ constructor( | |
| private val reactivateLastSurvey: ReactivateLastSurveyUseCase, | ||
| private val surveyDeepLinkParser: SurveyDeepLinkParser, | ||
| @IoDispatcher private val ioDispatcher: CoroutineDispatcher, | ||
| private val remoteConfig: FirebaseRemoteConfig, | ||
| private val shouldForceUpdateUseCase: ShouldForceUpdateUseCase, | ||
| authenticationManager: AuthenticationManager, | ||
| private val playInstallReferrerService: PlayInstallReferrerService, | ||
| ) : AbstractViewModel() { | ||
|
|
@@ -114,6 +114,9 @@ constructor( | |
| } | ||
| } | ||
| } | ||
| } catch (_: UpdateRequiredException) { | ||
| // Popup prompting the user to update is displayed, so we don't need to do anything here. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of silently consuming the exception, can we instead create a new UiEvent which the ui layer can then listen to for showing the dialog? |
||
| return | ||
| } catch (e: Throwable) { | ||
| Timber.e(e) | ||
| // TODO: Display some error dialog to the user with a helpful user-readable message. | ||
|
|
@@ -126,28 +129,6 @@ constructor( | |
| /** Returns true if the user has already accepted the Terms of Service. */ | ||
| private fun isTosAccepted(): Boolean = termsOfServiceRepository.isTermsOfServiceAccepted | ||
|
|
||
| private fun isOlderVersion(current: String, minRequired: String): Boolean { | ||
| fun String.toSegments() = split('.').map { it.toIntOrNull() ?: 0 } | ||
|
|
||
| val currentParts = current.toSegments() | ||
| val requiredParts = minRequired.toSegments() | ||
| val maxLength = maxOf(currentParts.size, requiredParts.size) | ||
|
|
||
| for (i in 0 until maxLength) { | ||
| val curr = currentParts.getOrElse(i) { 0 } | ||
| val req = requiredParts.getOrElse(i) { 0 } | ||
| if (curr != req) return curr < req | ||
| } | ||
|
|
||
| return false | ||
| } | ||
|
|
||
| fun isAppUpdateAvailable(currentVersion: String = BuildConfig.VERSION_NAME): Boolean { | ||
| val forceUpdate = remoteConfig.getBoolean("force_update") | ||
| val latestVersion = remoteConfig.getString("min_app_version") | ||
|
|
||
| return forceUpdate && | ||
| latestVersion.isNotBlank() && | ||
| isOlderVersion(currentVersion, latestVersion) | ||
| } | ||
| /** Returns true if this build must be updated before it may be used. */ | ||
| fun isAppUpdateRequired(): Boolean = shouldForceUpdateUseCase() | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| /* | ||
| * Copyright 2026 Google LLC | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.groundplatform.android.data.remote.firebase | ||
|
|
||
| import kotlin.test.assertFailsWith | ||
| import kotlinx.coroutines.flow.first | ||
| import kotlinx.coroutines.test.UnconfinedTestDispatcher | ||
| import kotlinx.coroutines.test.runTest | ||
| import org.groundplatform.android.FakeData | ||
| import org.groundplatform.android.data.remote.UpdateRequiredException | ||
| import org.groundplatform.domain.model.AppConfig | ||
| import org.groundplatform.domain.usecases.ShouldForceUpdateUseCase | ||
| import org.groundplatform.testing.FakeAppConfigRepository | ||
| import org.junit.Test | ||
| import org.junit.runner.RunWith | ||
| import org.mockito.kotlin.mock | ||
| import org.mockito.kotlin.verifyNoInteractions | ||
| import org.robolectric.RobolectricTestRunner | ||
|
|
||
| @RunWith(RobolectricTestRunner::class) | ||
| class FirestoreDataStoreTest { | ||
|
|
||
| private val firestoreProvider: FirebaseFirestoreProvider = mock() | ||
| private val appConfigRepository = | ||
| FakeAppConfigRepository().apply { | ||
| config = AppConfig(minAppVersion = "2.0.0", forceUpdate = true) | ||
| } | ||
| private val dataStore = | ||
| FirestoreDataStore( | ||
| firebaseFunctions = mock(), | ||
| firestoreProvider = firestoreProvider, | ||
| shouldForceUpdate = ShouldForceUpdateUseCase(appConfigRepository, currentVersion = "1.0.0"), | ||
| ioDispatcher = UnconfinedTestDispatcher(), | ||
| ) | ||
|
|
||
| @Test | ||
| fun `Refuses one-off reads when an app update is required`() = runTest { | ||
| assertFailsWith<UpdateRequiredException> { dataStore.loadSurvey("surveyId") } | ||
| verifyNoInteractions(firestoreProvider) | ||
| } | ||
|
|
||
| @Test | ||
| fun `Refuses listeners when an app update is required`() = runTest { | ||
| assertFailsWith<UpdateRequiredException> { dataStore.getPublicSurveyList().first() } | ||
| verifyNoInteractions(firestoreProvider) | ||
| } | ||
|
|
||
| @Test | ||
| fun `Refuses uploads when an app update is required`() = runTest { | ||
| assertFailsWith<UpdateRequiredException> { | ||
| dataStore.applyMutations(emptyList(), FakeData.USER) | ||
| } | ||
| verifyNoInteractions(firestoreProvider) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename to
AppUpdateRequiredException?