-
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
Introduce "data service" for Forms #5521
Conversation
ee4483a
to
cc7ccd1
Compare
dea13b1
to
b932c8c
Compare
@grzesiek2010 are you able to prioritise viewing this? I'd like to get it merged near the beginning of the release cycle. |
Ok I will review it tomorrow. |
collect_app/src/main/java/org/odk/collect/android/formmanagement/FormsDataService.kt
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/formmanagement/FormsDataService.kt
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/formmanagement/FormsDataService.kt
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/formmanagement/FormsDataService.kt
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/formmanagement/FormsDataService.kt
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/formmanagement/FormsDataService.kt
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/formmanagement/FormsDataService.kt
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/formmanagement/FormsDataService.kt
Outdated
Show resolved
Hide resolved
collect_app/src/androidTest/java/org/odk/collect/android/instrumented/forms/FormUtilsTest.java
Show resolved
Hide resolved
Like the approach being used in getodk#5521
@seadowg there are conflicts in this pr. |
@grzesiek2010 fixed! |
We’re not able to verify if issues #5499 and #5500 appear on this PR since we can’t upgrade older version with Google Drive project to CircleCi PR app build (so that there is a GD project in the PR version). If it was possible another question would be should we test it before or after merging PR#5675 |
On Android 10 and 13 there isn't a progress bar in "Start new form" while the forms are being downloaded. On the master version the progress bar appears. Steps to reproduce: |
@dbemke do you see a progress bar when you hit the refresh button? |
Yes, after tapping the refresh button the progress bar appears |
@dbemke ok that should be fixed. I realised that the sync with the server and the sync with the local disk could run at the same time which shouldn't be allowed (the latter was hiding the process bar before the former actually finished). |
The progress bar appears but the way forms "appear" changed a bit- don't know if it's better or not? |
Yeah good spot. Before the forms were being synced on disk before the full server sync had finished so if you happen to go to "Start new form" while the first sync is happening you'd might see some of the forms earlier. I think the new behaviour is better as the user does not a see a partial list of forms which is not great when the project is set up so that the forms "Exactly match server". |
Tested with Success! Verified on Android 13 Verified cases:
|
Tested with Success! Verified on Android 10 |
Like the approach being used in getodk#5521
Like the approach being used in getodk#5521
Like the approach being used in getodk#5521
Like the approach being used in getodk#5521
Like the approach being used in getodk#5521
Like the approach being used in getodk#5521
Like the approach being used in getodk#5521
Blocked by #5456Might reopen #5499Might reopen #5500This is something we've talked about for a while, and after working on #5456 it felt like we were at a good point. The recommended architecture for Android Apps is layered like so: UI Layer -> Domain Layer -> Data Layer. For us that breaks down like this:
FormDeleter
orServerFormDetailsFetcher
)FormSource
)This has led to "chunky" ViewModel classes that deal directly with Use Case or Data Layer classes and also means that we didn't have a very good story around shared reactive state between ViewModels (
BlankFormListViewModel
being our best example). Android's Architecture guide generally suggests using "Repository" objects to abstract over DBs and other data sources and to handle shared reactive state and domain logic (and Use Cases). I do like the idea of this kind of object, but generally we've trended to using a more (in my experience) common version of Repositories that allow simple synchronous access to data where the holder of the data itself is abstracted (likeFormsRepository
). The building blocks of what we needed for something similar to the Android App Architecture version of a Repository was there inFormsUpdater
- the rework to get there really just involved pushing most of the logic and state fromBlankFormListViewModel
in there. I thought about calling this kind of object a "Service", but Android's already overloaded this, so I went with "DataService". Naming is hard, and I'm happy to discuss.I've ended up in a place where I have a class that is responsible for both exposing the current set of forms and the status of any syncing that's happening around them (the "state"), as well as the actual modifications of the underlying forms data (the "actions"). This is not unlike the structure used in Redux - the key difference is that here a set of state, actions and reducers that belong to a particular "domain" have all been bucketed into one class.
What has been done to verify that this works as intended?
New and existing tests, verified manually.
Why is this the best possible solution? Were any other approaches considered?
Discussed above.
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?
I think a general check of managing forms (downloading, updating, deleting etc) is worth while here. Form Entry itself shouldn't be at risk. I've also noted a couple of issues I'm worried I might have created regressions ("Might reopen" at the top) around here as well so rechecking them specifically would be good.
Before submitting this PR, please make sure you have:
./gradlew checkAll
and confirmed all checks still pass OR confirm CircleCI build passes and run./gradlew connectedDebugAndroidTest
locally.