Don't allow deleting saved forms that have created/updated entities #6311
Conversation
seadowg
force-pushed
the
delete-instances
branch
from
5daf5b5
to
e24fb63
Compare
seadowg
force-pushed
the
delete-instances
branch
from
e24fb63
to
ffb9a23
Compare
| db.execSQL("CREATE TABLE IF NOT EXISTS " + INSTANCES_TABLE_NAME + " (" | ||
| + _ID + " integer primary key autoincrement, " | ||
| + DISPLAY_NAME + " text not null, " | ||
| + SUBMISSION_URI + " text, " | ||
| + CAN_EDIT_WHEN_COMPLETE + " text, " | ||
| + CAN_DELETE_BEFORE_SEND + " text, " |
There was a problem hiding this comment.
I went with text here as that's what we use for CAN_EDIT_WHEN_COMPLETE, but I think it'd make more sense to store both of these as integer to save space (as SQLite doesn't have a boolean type). That feels like a change for another time though?
There was a problem hiding this comment.
I don't have strong preferences here but if you want to change anything it's better do do that now to avoid testing db upgrades twice.
collect_app/src/main/java/org/odk/collect/android/formentry/FormEntryUseCases.kt
Outdated
Show resolved
Hide resolved
...t_app/src/main/java/org/odk/collect/android/database/instances/InstanceDatabaseMigrator.java
Show resolved
Hide resolved
...t_app/src/main/java/org/odk/collect/android/database/instances/InstanceDatabaseMigrator.java
Outdated
Show resolved
Hide resolved
|
@seadowg Which version/versions of Collect should we use to check the upgrades? |
The store version works! |
We won't be able to upgrade the store (released version) version to the master (not released). We've got some apks from a previous PR from Grzegorz - it's 2024.3.0 beta 0 apk and 2024.2.1. And I've got apks of the master version made a few days ago. Which of these version would work best to test changes in entities? |
When testing upgrading, we always want to use (at least) an APK for the last released version as that's what people will be upgrading from (not from |
I think no one was assigned to do it. We (testers) assumed that either you or Grzegorz will create the apks cause we're not familliar with what happens just before creating the released version or when/ how to create the apks so they're the same as the released version. I'll add this topic to our weekly |
|
Sorry. I removed the label by accident cause I wanted to add "testing blocked" label but it doesn't exist. |
|
Tested with Success Verified on device with Android 14 Verified cases:
|
|
Tested with Success Verified on device with Android 10 |
Closes #6012
Blocked by #6290Why is this the best possible solution? Were any other approaches considered?
I considered a few different options here, but ultimately tracking this state in the instances DB seemed to make the most sense. Tracking it in forms, would mean blocking forms that can create/update entities but don't from deletion and tracking from entities would require a potentially very slow query.
I went with a boolean flag rather than storing the entity ID as we currently don't need that info, and I'd rather not do "foreign keys" between different DBs unless we need to.
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?
The biggest risk would be that we end up blocking deletion for forms that should be deletable. Checking that saved forms can/can't be deleted when expected is probably the best focus here.
Also, this involves migrating the instances DB so checking upgrades work correctly is important.
Before submitting this PR, please make sure you have:
./gradlew connectedAndroidTest(or./gradlew testLab) and confirmed all checks still passDateFormatsTest