Handle entities that have been removed on the server #6132
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
seadowg
commented
May 13, 2024
collect_app/src/test/java/org/odk/collect/android/entities/JsonFileEntitiesRepositoryTest.kt
Outdated
Show resolved
Hide resolved
seadowg
commented
May 13, 2024
grzesiek2010
requested changes
May 17, 2024
collect_app/src/main/java/org/odk/collect/android/entities/JsonFileEntitiesRepository.kt
Outdated
Show resolved
Hide resolved
entities/src/main/java/org/odk/collect/entities/InMemEntitiesRepository.kt
Outdated
Show resolved
Hide resolved
collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt
Outdated
Show resolved
Hide resolved
collect_app/src/androidTest/java/org/odk/collect/android/support/pages/EntitiesPage.kt
Outdated
Show resolved
Hide resolved
collect_app/src/androidTest/java/org/odk/collect/android/support/pages/EntitiesPage.kt
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/entities/JsonFileEntitiesRepository.kt
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/entities/JsonFileEntitiesRepository.kt
Outdated
Show resolved
Hide resolved
collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt
Outdated
Show resolved
Hide resolved
collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt
Outdated
Show resolved
Hide resolved
"local" entities are ones stored locally and can include "offline" and "online" ones.
grzesiek2010
requested changes
May 21, 2024
collect_app/src/androidTest/java/org/odk/collect/android/support/pages/EntityListPage.kt
Outdated
Show resolved
Hide resolved
collect_app/src/test/java/org/odk/collect/android/entities/JsonFileEntitiesRepositoryTest.kt
Outdated
Show resolved
Hide resolved
| } | ||
| } catch (e: Exception) { | ||
| entitiesFile.delete() | ||
| entitiesFile.createNewFile() |
There was a problem hiding this comment.
If I comment those two lines out this test still passes: https://github.com/getodk/collect/pull/6132/files#diff-f7e8798917d3ccfe8e394d1d714eb9535648a9c63a42c41b5ec30d32006cbd2cR35
There was a problem hiding this comment.
Ah good spot! We don't actually need to delete the file itself as we just treat the file as empty and then override next time we mutate (as we always do anyway). I can remove the delete/createNewFile call.
We write the whole file again when mutating, so treating the file as empty is good enough.
grzesiek2010
approved these changes
May 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #6011
Why is this the best possible solution? Were any other approaches considered?
The big change here is that our locally stored entities (of type
Entity) can now be "offline" (they only exist locally) or "online" (they exist locally and on the server). This concept refers to the entities ID rather than its version: we might have made an update locally that's not on the server yet to an entity we know is "online". Entities become "online" once they've been seen in a form's entity list. As part of this change, I've updated the language we use in the code to reflect it better.As noted in the issue, there isn't a way that we could handle one of the cases (a form deleted on the server that has never been "seen" by the client in an entity list) without adding new communication with the server. This is something that's being actively discussed with the Central team.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Not much has changed other than entities so testing can probably be restricted to that.
One thing to note is that entities will likely be cleared when upgrading from one version of Collect to another with the current implementation. As discussed here, this is just a temporary caveat while the features are experimental so allow us to iterate faster.
Before submitting this PR, please make sure you have:
./gradlew connectedAndroidTest(or./gradlew testLab) and confirmed all checks still pass