-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Handle entities that have been removed on the server #6132
Conversation
collect_app/src/test/java/org/odk/collect/android/entities/JsonFileEntitiesRepositoryTest.kt
Outdated
Show resolved
Hide resolved
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Tested with success! Verified on a device with Android 13 Verified cases:
|
Tested with success! Verified on a device with Android 14 |
Tested with success! Verified on a device with Android 10 |
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