From be0db23e70b6442dde59d11ce0a2ba6640c7a27c Mon Sep 17 00:00:00 2001 From: Shobhit Agarwal Date: Sun, 1 Dec 2024 15:08:55 +0530 Subject: [PATCH 1/6] Add helper method to count number of draft submissions in local db Needed for unit tests --- .../persistence/local/room/dao/DraftSubmissionDao.kt | 2 ++ .../local/room/stores/RoomSubmissionStore.kt | 10 ++++++---- .../persistence/local/stores/LocalSubmissionStore.kt | 3 +++ .../android/ground/repository/SubmissionRepository.kt | 2 ++ 4 files changed, 13 insertions(+), 4 deletions(-) diff --git a/ground/src/main/java/com/google/android/ground/persistence/local/room/dao/DraftSubmissionDao.kt b/ground/src/main/java/com/google/android/ground/persistence/local/room/dao/DraftSubmissionDao.kt index 03a286d8cf..495208660e 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/local/room/dao/DraftSubmissionDao.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/local/room/dao/DraftSubmissionDao.kt @@ -27,4 +27,6 @@ interface DraftSubmissionDao : BaseDao { suspend fun findById(draftSubmissionId: String): DraftSubmissionEntity? @Query("DELETE FROM draft_submission") fun delete() + + @Query("SELECT COUNT(*) FROM draft_submission") suspend fun countAll(): Int } diff --git a/ground/src/main/java/com/google/android/ground/persistence/local/room/stores/RoomSubmissionStore.kt b/ground/src/main/java/com/google/android/ground/persistence/local/room/stores/RoomSubmissionStore.kt index 5c57aa86b7..df09e3038a 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/local/room/stores/RoomSubmissionStore.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/local/room/stores/RoomSubmissionStore.kt @@ -44,7 +44,6 @@ import com.google.android.ground.persistence.local.room.fields.MutationEntityTyp import com.google.android.ground.persistence.local.room.fields.UserDetails import com.google.android.ground.persistence.local.stores.LocalSubmissionStore import com.google.android.ground.util.Debug.logOnFailure -import com.google.firebase.crashlytics.FirebaseCrashlytics import javax.inject.Inject import javax.inject.Singleton import kotlinx.collections.immutable.toPersistentList @@ -211,9 +210,10 @@ class RoomSubmissionStore @Inject internal constructor() : LocalSubmissionStore apply(mutation) enqueue(mutation) } catch (e: LocalDataStoreException) { - FirebaseCrashlytics.getInstance() - .log("Error enqueueing ${mutation.type} mutation for submission ${mutation.submissionId}") - FirebaseCrashlytics.getInstance().recordException(e) + Timber.e( + e, + "Error enqueueing ${mutation.type} mutation for submission ${mutation.submissionId}", + ) throw e } } @@ -269,4 +269,6 @@ class RoomSubmissionStore @Inject internal constructor() : LocalSubmissionStore override suspend fun deleteDraftSubmissions() { draftSubmissionDao.delete() } + + override suspend fun countDraftSubmissions(): Int = draftSubmissionDao.countAll() } diff --git a/ground/src/main/java/com/google/android/ground/persistence/local/stores/LocalSubmissionStore.kt b/ground/src/main/java/com/google/android/ground/persistence/local/stores/LocalSubmissionStore.kt index c3be233866..b5e86d5f5d 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/local/stores/LocalSubmissionStore.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/local/stores/LocalSubmissionStore.kt @@ -78,4 +78,7 @@ interface LocalSubmissionStore : LocalMutationStore Date: Sun, 1 Dec 2024 15:19:39 +0530 Subject: [PATCH 2/6] Access draftId only via repository Will be used in unit tests --- .../google/android/ground/repository/SubmissionRepository.kt | 2 ++ .../com/google/android/ground/ui/home/HomeScreenViewModel.kt | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/ground/src/main/java/com/google/android/ground/repository/SubmissionRepository.kt b/ground/src/main/java/com/google/android/ground/repository/SubmissionRepository.kt index 4397be3e87..d0c5c58f4d 100644 --- a/ground/src/main/java/com/google/android/ground/repository/SubmissionRepository.kt +++ b/ground/src/main/java/com/google/android/ground/repository/SubmissionRepository.kt @@ -79,6 +79,8 @@ constructor( suspend fun countDraftSubmissions() = localSubmissionStore.countDraftSubmissions() + fun getDraftSubmissionsId() = localValueStore.draftSubmissionId ?: "" + suspend fun saveDraftSubmission( jobId: String, loiId: String?, diff --git a/ground/src/main/java/com/google/android/ground/ui/home/HomeScreenViewModel.kt b/ground/src/main/java/com/google/android/ground/ui/home/HomeScreenViewModel.kt index 89de4ed2aa..7cbebfb955 100644 --- a/ground/src/main/java/com/google/android/ground/ui/home/HomeScreenViewModel.kt +++ b/ground/src/main/java/com/google/android/ground/ui/home/HomeScreenViewModel.kt @@ -82,10 +82,10 @@ internal constructor( /** Attempts to return draft submission for the currently active survey. */ suspend fun getDraftSubmission(): DraftSubmission? { - val draftId = localValueStore.draftSubmissionId + val draftId = submissionRepository.getDraftSubmissionsId() val survey = surveyRepository.activeSurvey - if (draftId.isNullOrEmpty() || survey == null) { + if (draftId.isEmpty() || survey == null) { // Missing draft submission return null } From b84af1aaafccfd7a0d8b21c75ba5f99b51e3cfa9 Mon Sep 17 00:00:00 2001 From: Shobhit Agarwal Date: Sun, 1 Dec 2024 15:31:01 +0530 Subject: [PATCH 3/6] Make getPendingCreateCount function public --- .../google/android/ground/repository/SubmissionRepository.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ground/src/main/java/com/google/android/ground/repository/SubmissionRepository.kt b/ground/src/main/java/com/google/android/ground/repository/SubmissionRepository.kt index d0c5c58f4d..9d695c3696 100644 --- a/ground/src/main/java/com/google/android/ground/repository/SubmissionRepository.kt +++ b/ground/src/main/java/com/google/android/ground/repository/SubmissionRepository.kt @@ -107,7 +107,7 @@ constructor( suspend fun getTotalSubmissionCount(loi: LocationOfInterest) = loi.submissionCount + getPendingCreateCount(loi.id) - getPendingDeleteCount(loi.id) - private suspend fun getPendingCreateCount(loiId: String) = + suspend fun getPendingCreateCount(loiId: String) = localSubmissionStore.getPendingCreateCount(loiId) private suspend fun getPendingDeleteCount(loiId: String) = From 4190b6eae9cb6abbe981696f57302e7eac740f44 Mon Sep 17 00:00:00 2001 From: Shobhit Agarwal Date: Sun, 1 Dec 2024 15:37:38 +0530 Subject: [PATCH 4/6] Replace mock SubmissionRepository with actual implementation in DataCollectionFragmentTest --- .../DataCollectionFragmentTest.kt | 159 +++++++++--------- 1 file changed, 82 insertions(+), 77 deletions(-) diff --git a/ground/src/test/java/com/google/android/ground/ui/datacollection/DataCollectionFragmentTest.kt b/ground/src/test/java/com/google/android/ground/ui/datacollection/DataCollectionFragmentTest.kt index 692fcc9c4a..0248a301c8 100644 --- a/ground/src/test/java/com/google/android/ground/ui/datacollection/DataCollectionFragmentTest.kt +++ b/ground/src/test/java/com/google/android/ground/ui/datacollection/DataCollectionFragmentTest.kt @@ -19,9 +19,11 @@ package com.google.android.ground.ui.datacollection import android.os.Bundle import com.google.android.ground.BaseHiltTest import com.google.android.ground.R -import com.google.android.ground.capture import com.google.android.ground.domain.usecases.survey.ActivateSurveyUseCase import com.google.android.ground.launchFragmentWithNavController +import com.google.android.ground.model.mutation.Mutation +import com.google.android.ground.model.mutation.SubmissionMutation +import com.google.android.ground.model.submission.DraftSubmission import com.google.android.ground.model.submission.MultipleChoiceTaskData import com.google.android.ground.model.submission.TextTaskData import com.google.android.ground.model.submission.ValueDelta @@ -31,15 +33,18 @@ import com.google.android.ground.model.task.MultipleChoice import com.google.android.ground.model.task.Option import com.google.android.ground.model.task.Task import com.google.android.ground.persistence.local.room.converter.SubmissionDeltasConverter -import com.google.android.ground.persistence.uuid.OfflineUuidGenerator +import com.google.android.ground.persistence.local.room.fields.MutationEntitySyncStatus +import com.google.android.ground.repository.MutationRepository import com.google.android.ground.repository.SubmissionRepository +import com.google.android.ground.repository.UserRepository import com.google.common.truth.Truth.assertThat import com.sharedtest.FakeData import com.sharedtest.FakeData.LOCATION_OF_INTEREST import com.sharedtest.FakeData.LOCATION_OF_INTEREST_NAME +import com.sharedtest.FakeData.USER import com.sharedtest.persistence.remote.FakeRemoteDataStore -import dagger.hilt.android.testing.BindValue import dagger.hilt.android.testing.HiltAndroidTest +import java.util.Date import javax.inject.Inject import kotlinx.collections.immutable.persistentListOf import kotlinx.coroutines.ExperimentalCoroutinesApi @@ -47,12 +52,6 @@ import kotlinx.coroutines.runBlocking import kotlinx.coroutines.test.advanceUntilIdle import org.junit.Test import org.junit.runner.RunWith -import org.mockito.ArgumentCaptor -import org.mockito.Captor -import org.mockito.Mock -import org.mockito.kotlin.eq -import org.mockito.kotlin.times -import org.mockito.kotlin.verify import org.robolectric.RobolectricTestRunner import org.robolectric.shadows.ShadowToast @@ -63,16 +62,14 @@ class DataCollectionFragmentTest : BaseHiltTest() { @Inject lateinit var activateSurvey: ActivateSurveyUseCase @Inject lateinit var fakeRemoteDataStore: FakeRemoteDataStore - @BindValue @Mock lateinit var submissionRepository: SubmissionRepository - @Captor lateinit var deltaCaptor: ArgumentCaptor> + @Inject lateinit var mutationRepository: MutationRepository + @Inject lateinit var submissionRepository: SubmissionRepository + @Inject lateinit var userRepository: UserRepository + lateinit var fragment: DataCollectionFragment - @Inject lateinit var uuidGenerator: OfflineUuidGenerator - lateinit var collectionId: String override fun setUp() = runBlocking { super.setUp() - collectionId = uuidGenerator.generateUuid() - setupSubmission() setupFragment() } @@ -123,20 +120,7 @@ class DataCollectionFragmentTest : BaseHiltTest() { fun `Next click saves draft`() = runWithTestDispatcher { runner().inputText(TASK_1_RESPONSE).clickNextButton() - // Validate that previous drafts were cleared - verify(submissionRepository, times(1)).deleteDraftSubmission() - - // Validate that new draft was created - verify(submissionRepository, times(1)) - .saveDraftSubmission( - eq(JOB.id), - eq(LOCATION_OF_INTEREST.id), - eq(SURVEY.id), - capture(deltaCaptor), - eq(LOCATION_OF_INTEREST_NAME), - ) - - listOf(TASK_1_VALUE_DELTA).forEach { value -> assertThat(deltaCaptor.value).contains(value) } + assertDraftSaved(listOf(TASK_1_VALUE_DELTA)) } @Test @@ -147,20 +131,7 @@ class DataCollectionFragmentTest : BaseHiltTest() { .selectMultipleChoiceOption(TASK_2_OPTION_LABEL) .clickPreviousButton() - // Both deletion and creating happens twice as we do it on every previous/next step - verify(submissionRepository, times(2)).deleteDraftSubmission() - verify(submissionRepository, times(2)) - .saveDraftSubmission( - eq(JOB.id), - eq(LOCATION_OF_INTEREST.id), - eq(SURVEY.id), - capture(deltaCaptor), - eq(LOCATION_OF_INTEREST_NAME), - ) - - listOf(TASK_1_VALUE_DELTA, TASK_2_VALUE_DELTA).forEach { value -> - assertThat(deltaCaptor.value).contains(value) - } + assertDraftSaved(listOf(TASK_1_VALUE_DELTA, TASK_2_VALUE_DELTA)) } @Test @@ -210,25 +181,19 @@ class DataCollectionFragmentTest : BaseHiltTest() { .selectMultipleChoiceOption(TASK_2_OPTION_LABEL) .clickDoneButton() // Click "done" on final task - verify(submissionRepository) - .saveSubmission( - eq(SURVEY.id), - eq(LOCATION_OF_INTEREST.id), - capture(deltaCaptor), - eq(collectionId), - ) - - listOf(TASK_1_VALUE_DELTA, TASK_2_VALUE_DELTA).forEach { value -> - assertThat(deltaCaptor.value).contains(value) - } + assertSubmissionSaved(listOf(TASK_1_VALUE_DELTA, TASK_2_VALUE_DELTA)) } @Test fun `Clicking back button on first task clears the draft and returns false`() = runWithTestDispatcher { - runner().pressBackButton(false) + runner() + .inputText(TASK_1_RESPONSE) + .clickNextButton() + .pressBackButton(true) + .pressBackButton(false) - verify(submissionRepository, times(1)).deleteDraftSubmission() + assertNoDraftSaved() } @Test @@ -245,17 +210,10 @@ class DataCollectionFragmentTest : BaseHiltTest() { .inputText(TASK_CONDITIONAL_RESPONSE) .clickDoneButton() - verify(submissionRepository) - .saveSubmission( - eq(SURVEY.id), - eq(LOCATION_OF_INTEREST.id), - capture(deltaCaptor), - eq(collectionId), - ) - // Conditional task data is submitted. - listOf(TASK_1_VALUE_DELTA, TASK_2_CONDITIONAL_VALUE_DELTA, TASK_CONDITIONAL_VALUE_DELTA) - .forEach { value -> assertThat(deltaCaptor.value).contains(value) } + assertSubmissionSaved( + listOf(TASK_1_VALUE_DELTA, TASK_2_CONDITIONAL_VALUE_DELTA, TASK_CONDITIONAL_VALUE_DELTA) + ) } @Test @@ -279,21 +237,68 @@ class DataCollectionFragmentTest : BaseHiltTest() { .clickDoneButton() .validateTextIsNotDisplayed(TASK_CONDITIONAL_NAME) - verify(submissionRepository) - .saveSubmission( - eq(SURVEY.id), - eq(LOCATION_OF_INTEREST.id), - capture(deltaCaptor), - eq(collectionId), - ) - // Conditional task data is not submitted. - listOf(TASK_1_VALUE_DELTA, TASK_2_VALUE_DELTA).forEach { value -> - assertThat(deltaCaptor.value).contains(value) - } + assertSubmissionSaved(listOf(TASK_1_VALUE_DELTA, TASK_2_VALUE_DELTA)) } + private suspend fun assertSubmissionSaved(valueDeltas: List) { + assertNoDraftSaved() + + val loiId = LOCATION_OF_INTEREST.id + + // Exactly 1 submission should be saved. + assertThat(submissionRepository.getPendingCreateCount(loiId)).isEqualTo(1) + + val testDate = Date() + val mutation = + (mutationRepository.getMutations(loiId, MutationEntitySyncStatus.PENDING)[0] + as SubmissionMutation) + .copy(clientTimestamp = testDate) // seed dummy test date + + assertThat(mutation) + .isEqualTo( + SubmissionMutation( + id = 1, + type = Mutation.Type.CREATE, + syncStatus = Mutation.SyncStatus.PENDING, + surveyId = SURVEY.id, + locationOfInterestId = loiId, + userId = USER.id, + clientTimestamp = testDate, + collectionId = "TEST UUID", + job = JOB, + submissionId = "TEST UUID", + deltas = valueDeltas, + ) + ) + } + + private suspend fun assertDraftSaved(valueDeltas: List) { + val draftId = submissionRepository.getDraftSubmissionsId() + assertThat(draftId).isNotEmpty() + + // Exactly 1 draft should be present always. + assertThat(submissionRepository.countDraftSubmissions()).isEqualTo(1) + assertThat(submissionRepository.getDraftSubmission(draftId, SURVEY)) + .isEqualTo( + DraftSubmission( + id = draftId, + jobId = JOB.id, + loiId = LOCATION_OF_INTEREST.id, + loiName = LOCATION_OF_INTEREST_NAME, + surveyId = SURVEY.id, + deltas = valueDeltas, + ) + ) + } + + private suspend fun assertNoDraftSaved() { + assertThat(submissionRepository.getDraftSubmissionsId()).isEmpty() + assertThat(submissionRepository.countDraftSubmissions()).isEqualTo(0) + } + private fun setupSubmission() = runWithTestDispatcher { + userRepository.saveUserDetails(USER) fakeRemoteDataStore.surveys = listOf(SURVEY) fakeRemoteDataStore.predefinedLois = listOf(LOCATION_OF_INTEREST) activateSurvey(SURVEY.id) From f9ec45c5264029e6f402294ff1c2d13691848a71 Mon Sep 17 00:00:00 2001 From: Shobhit Agarwal Date: Sun, 1 Dec 2024 15:38:54 +0530 Subject: [PATCH 5/6] Remove unused var --- .../com/google/android/ground/ui/home/HomeScreenViewModel.kt | 2 -- 1 file changed, 2 deletions(-) diff --git a/ground/src/main/java/com/google/android/ground/ui/home/HomeScreenViewModel.kt b/ground/src/main/java/com/google/android/ground/ui/home/HomeScreenViewModel.kt index 7cbebfb955..4f5065dba2 100644 --- a/ground/src/main/java/com/google/android/ground/ui/home/HomeScreenViewModel.kt +++ b/ground/src/main/java/com/google/android/ground/ui/home/HomeScreenViewModel.kt @@ -20,7 +20,6 @@ import androidx.lifecycle.MutableLiveData import androidx.lifecycle.viewModelScope import com.google.android.ground.model.mutation.Mutation.SyncStatus.COMPLETED import com.google.android.ground.model.submission.DraftSubmission -import com.google.android.ground.persistence.local.LocalValueStore import com.google.android.ground.persistence.sync.MediaUploadWorkManager import com.google.android.ground.persistence.sync.MutationSyncWorkManager import com.google.android.ground.repository.MutationRepository @@ -43,7 +42,6 @@ import timber.log.Timber class HomeScreenViewModel @Inject internal constructor( - private val localValueStore: LocalValueStore, private val navigator: Navigator, private val offlineAreaRepository: OfflineAreaRepository, private val submissionRepository: SubmissionRepository, From 2c52a5ab92c2c0a39377a29e91c1c5b6116e547e Mon Sep 17 00:00:00 2001 From: Shobhit Agarwal Date: Thu, 5 Dec 2024 00:27:31 +0530 Subject: [PATCH 6/6] Fix failing test after merge --- .../ground/ui/datacollection/DataCollectionFragmentTest.kt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ground/src/test/java/com/google/android/ground/ui/datacollection/DataCollectionFragmentTest.kt b/ground/src/test/java/com/google/android/ground/ui/datacollection/DataCollectionFragmentTest.kt index 0248a301c8..4abc5f1721 100644 --- a/ground/src/test/java/com/google/android/ground/ui/datacollection/DataCollectionFragmentTest.kt +++ b/ground/src/test/java/com/google/android/ground/ui/datacollection/DataCollectionFragmentTest.kt @@ -33,7 +33,6 @@ import com.google.android.ground.model.task.MultipleChoice import com.google.android.ground.model.task.Option import com.google.android.ground.model.task.Task import com.google.android.ground.persistence.local.room.converter.SubmissionDeltasConverter -import com.google.android.ground.persistence.local.room.fields.MutationEntitySyncStatus import com.google.android.ground.repository.MutationRepository import com.google.android.ground.repository.SubmissionRepository import com.google.android.ground.repository.UserRepository @@ -251,8 +250,9 @@ class DataCollectionFragmentTest : BaseHiltTest() { val testDate = Date() val mutation = - (mutationRepository.getMutations(loiId, MutationEntitySyncStatus.PENDING)[0] - as SubmissionMutation) + mutationRepository + .getIncompleteUploads()[0] + .submissionMutation!! .copy(clientTimestamp = testDate) // seed dummy test date assertThat(mutation)