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

Jump to bottom

Added null check for Offline LOI #2788

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Added null check for Offline LOI #2788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9f5432d
added null check
anandwana001 Oct 10, 2024
9d35999
return count fix
anandwana001 Oct 10, 2024
7819f36
Merge branch 'master' into anandwana001/2773/offline-loi-check
gino-m Nov 4, 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
14 changes: 11 additions & 3 deletions ground/src/main/java/com/google/android/ground/repository/LocationOfInterestRepository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.flow.firstOrNull
import kotlinx.coroutines.flow.map
import timber.log.Timber

/**
* Coordinates persistence and retrieval of [LocationOfInterest] instances from remote, local, and
Expand Down Expand Up @@ -88,9 +89,16 @@
}

/** This only works if the survey and location of interests are already cached to local db. */
suspend fun getOfflineLoi(surveyId: String, loiId: String): LocationOfInterest {
val survey = localSurveyStore.getSurveyById(surveyId) ?: error("Survey not found: $surveyId")
return localLoiStore.getLocationOfInterest(survey, loiId) ?: error("LOI not found: $loiId")
suspend fun getOfflineLoi(surveyId: String, loiId: String): LocationOfInterest? {
val survey = localSurveyStore.getSurveyById(surveyId)
val locationOfInterest = survey?.let { localLoiStore.getLocationOfInterest(it, loiId) }

if (survey == null) {
Timber.e("LocationOfInterestRepository", "Survey not found: $surveyId")

Check warning on line 97 in ground/src/main/java/com/google/android/ground/repository/LocationOfInterestRepository.kt

View check run for this annotation

Codecov / codecov/patch

ground/src/main/java/com/google/android/ground/repository/LocationOfInterestRepository.kt#L97 

Added line #L97 was not covered by tests
} else if (locationOfInterest == null) {
Timber.e("LocationRepository", "LOI not found for survey $surveyId: LOI ID $loiId")

Check warning on line 99 in ground/src/main/java/com/google/android/ground/repository/LocationOfInterestRepository.kt

View check run for this annotation

Codecov / codecov/patch

ground/src/main/java/com/google/android/ground/repository/LocationOfInterestRepository.kt#L99 

Added line #L99 was not covered by tests
}
return locationOfInterest
}

/** Saves a new LOI in the local db and enqueues a sync worker. */
Expand Down
30 changes: 16 additions & 14 deletions ground/src/main/java/com/google/android/ground/repository/SubmissionRepository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,20 +56,22 @@
) {
val newId = uuidGenerator.generateUuid()
val userId = userRepository.getAuthenticatedUser().id
val job = locationOfInterestRepository.getOfflineLoi(surveyId, locationOfInterestId).job
val mutation =
SubmissionMutation(
job = job,
submissionId = newId,
deltas = deltas,
type = Mutation.Type.CREATE,
syncStatus = SyncStatus.PENDING,
surveyId = surveyId,
locationOfInterestId = locationOfInterestId,
userId = userId,
collectionId = collectionId,
)
applyAndEnqueue(mutation)
val job = locationOfInterestRepository.getOfflineLoi(surveyId, locationOfInterestId)?.job
job?.let {
val mutation =
SubmissionMutation(
job = job,
submissionId = newId,
deltas = deltas,
type = Mutation.Type.CREATE,
syncStatus = SyncStatus.PENDING,
surveyId = surveyId,
locationOfInterestId = locationOfInterestId,
userId = userId,
collectionId = collectionId,

Check warning on line 71 in ground/src/main/java/com/google/android/ground/repository/SubmissionRepository.kt

View check run for this annotation

Codecov / codecov/patch

ground/src/main/java/com/google/android/ground/repository/SubmissionRepository.kt#L61-L71 

Added lines #L61 - L71 were not covered by tests
)
applyAndEnqueue(mutation)
}

Check warning on line 74 in ground/src/main/java/com/google/android/ground/repository/SubmissionRepository.kt

View check run for this annotation

Codecov / codecov/patch

ground/src/main/java/com/google/android/ground/repository/SubmissionRepository.kt#L73-L74 

Added lines #L73 - L74 were not covered by tests
}

suspend fun getDraftSubmission(draftSubmissionId: String, survey: Survey): DraftSubmission? =
Expand Down
7 changes: 4 additions & 3 deletions ground/src/main/java/com/google/android/ground/ui/datacollection/DataCollectionViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,9 +121,10 @@ internal constructor(
} else
// LOI name pulled from LOI properties, if it exists.
flow {
val loi = locationOfInterestRepository.getOfflineLoi(surveyId, loiId)
val label = locationOfInterestHelper.getDisplayLoiName(loi)
emit(label)
locationOfInterestRepository.getOfflineLoi(surveyId, loiId)?.let {
val label = locationOfInterestHelper.getDisplayLoiName(it)
emit(label)
}
})
.stateIn(viewModelScope, SharingStarted.Lazily, "")

Expand Down
10 changes: 8 additions & 2 deletions ground/src/main/java/com/google/android/ground/ui/syncstatus/SyncStatusViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import kotlinx.coroutines.flow.filterNotNull
import kotlinx.coroutines.flow.flatMapLatest
import kotlinx.coroutines.flow.map
import timber.log.Timber

/**
* View model for the offline area manager fragment. Handles the current list of downloaded areas.
Expand Down Expand Up @@ -61,11 +62,16 @@

private suspend fun loadLocationsOfInterestAndPair(
mutations: List<Mutation>
): List<MutationDetail> = mutations.map { toMutationDetail(it) }
): List<MutationDetail> = mutations.mapNotNull { toMutationDetail(it) }

private suspend fun toMutationDetail(mutation: Mutation): MutationDetail {
private suspend fun toMutationDetail(mutation: Mutation): MutationDetail? {

Check warning on line 67 in ground/src/main/java/com/google/android/ground/ui/syncstatus/SyncStatusViewModel.kt

View check run for this annotation

Codecov / codecov/patch

ground/src/main/java/com/google/android/ground/ui/syncstatus/SyncStatusViewModel.kt#L67 

Added line #L67 was not covered by tests
val loi =
locationOfInterestRepository.getOfflineLoi(mutation.surveyId, mutation.locationOfInterestId)
if (loi == null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any idea why this might be happening? Was the LOI removed remotely, or perhaps the ID in the local db changed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nvm, I see the LocationOfInterestMutation references the LOI by UUID, not local DB primary key.

If this happens, shouldn't we show the mutation as "failed" in the sync status screen? To do that, we may need to introduce a new type of error state in SyncStatus, UNRECOVERABLE_ERROR for ex. We could also rename FAILED to PENDING_RETRY or something clearer and update its current incorrect ktdoc in SyncStatus.

// If LOI is null, return null to avoid proceeding
Timber.e("MutationDetail", "LOI not found for mutation ${mutation.id}")
return null

Check warning on line 73 in ground/src/main/java/com/google/android/ground/ui/syncstatus/SyncStatusViewModel.kt

View check run for this annotation

Codecov / codecov/patch

ground/src/main/java/com/google/android/ground/ui/syncstatus/SyncStatusViewModel.kt#L72-L73 

Added lines #L72 - L73 were not covered by tests
}
val user = userRepository.getAuthenticatedUser()
return MutationDetail(
user = user.displayName,
Expand Down