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.

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

Added null check for Offline LOI #2788

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

Conversation

anandwana001
Copy link
Collaborator

@anandwana001 anandwana001 commented Oct 10, 2024

Fixes #2773

Earlier app crash when any offline LOI is not available.
Added Null check of such LOI access logic

@shobhitagarwal1612 @sufyanAbbasi PTAL?

Copy link

codecov bot commented Oct 11, 2024

Codecov Report

Attention: Patch coverage is 21.87500% with 25 lines in your changes missing coverage. Please review.

Project coverage is 61.89%. Comparing base (c3ad147) to head (7819f36).

Files with missing lines Patch % Lines
.../android/ground/repository/SubmissionRepository.kt 0.00% 15 Missing ⚠️
.../ground/repository/LocationOfInterestRepository.kt 37.50% 2 Missing and 3 partials ⚠️
...ndroid/ground/ui/syncstatus/SyncStatusViewModel.kt 20.00% 4 Missing ⚠️
...round/ui/datacollection/DataCollectionViewModel.kt 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2788      +/-   ##
============================================
- Coverage     61.96%   61.89%   -0.08%     
  Complexity     1166     1166              
============================================
  Files           265      265              
  Lines          6223     6235      +12     
  Branches        877      883       +6     
============================================
+ Hits           3856     3859       +3     
- Misses         1840     1847       +7     
- Partials        527      529       +2     
Files with missing lines Coverage Δ
...round/ui/datacollection/DataCollectionViewModel.kt 71.57% <75.00%> (-0.37%) ⬇️
...ndroid/ground/ui/syncstatus/SyncStatusViewModel.kt 45.45% <20.00%> (-7.18%) ⬇️
.../ground/repository/LocationOfInterestRepository.kt 50.87% <37.50%> (-0.11%) ⬇️
.../android/ground/repository/SubmissionRepository.kt 19.51% <0.00%> (-1.01%) ⬇️

@gino-m
Copy link
Collaborator

gino-m commented Oct 11, 2024

Earlier app crash when any offline LOI is not available.

Do you mean "when no LOIs are available offline"?

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.

@kenstershiro
Copy link
Collaborator

@sufyanAbbasi to try to take a look at the PR approval in @gino-m 's absence

Copy link
Contributor

@sufyanAbbasi sufyanAbbasi left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, I think the code looks good! As @gino-m pointed out, we will definitely want to indicate irreversibly failing mutations, maybe by taking the diff between the map and mapNotNull and marking those in the UI as broken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LocationOfInterestRepository.getOfflineLoi java.lang.IllegalStateException - LOI not found
5 participants