Fixed displaying saved forms to delete #6107
Conversation
There was a problem hiding this comment.
It looks like there's some conflicts blocking this.
I could have edited InstancesDataService by adding a new flow specifically for undelete instances (and read them from the repository) but:
- This would make testing more difficult than what I did because I would need to rework the view model test completely so that it tests a real InstancesDataService object.
- I'm not sure if exposing different flows for different scenarios (all instances, undelete instances, etc) is what we want to have in our data service. Maybe exposing just one like now is clearer.
Good points. I guess it's a trade off, if we expose different Flows/LiveData values for different "filters" over the data then there is definitely more complexity in the data service to manage this and more API to care about from clients. On the other hand, exposing "everything" and then filtering in memory will definitely be less efficient than pushing that work all the way to the DB. I think sticking with the least change for now (what you've done) is a good idea. If we do need to change it later once we're using the data service for all our lists, we'll be better informed to make that decision then.
| @@ -142,7 +142,7 @@ class DeleteFormsActivity : LocalizedActivity() { | |||
| showAllVersions = true | |||
| ) | |||
|
|
|||
| SavedFormListViewModel::class.java -> SavedFormListViewModel( | |||
| DeleteSavedFormListViewModel::class.java -> DeleteSavedFormListViewModel( | |||
There was a problem hiding this comment.
I'm not sure about this rename. I was envisioning that we'd be able to use SavedFormListViewModel in other places (like "Drafts") in the future. But, maybe it makes more sense to have this (DeleteSavedFormListViewModel) name now and rename in the future if we do end up generalising it?
There was a problem hiding this comment.
Yeah, I thought that with the new changes that view model could not be shared but that's not true in Drafts as you said we would also need to filter out deleted forms. I've removed that change.
grzesiek2010
force-pushed
the
COLLECT-6017
branch
from
14f2de0 to
2d30b34
Compare
|
Tested with Success! Verified on device with Android: 11 Verified cases:
|
|
Tested with Success! Verified on device with Android: 10 |
|
Tested with Success! Verified on device with Android: 13 |
Closes #6017
Why is this the best possible solution? Were any other approaches considered?
I could have edited
InstancesDataServiceby adding a new flow specifically for undelete instances (and read them from the repository) but:InstancesDataServiceobject.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?
This should fix filtering forms to delete so that the soft-deleted ones (forms that have already been deleted but still exist in the database so that we can keep track of sent forms for example). are not displayed on the list.
Do we need any specific form for testing your changes? If so, please attach one.
No.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No.
Before submitting this PR, please make sure you have:
./gradlew connectedAndroidTest(or./gradlew testLab) and confirmed all checks still pass