Refine form saving language #5569
Conversation
collect_app/src/androidTest/java/org/odk/collect/android/support/pages/FormEndPage.java
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/formentry/FormEndViewFactory.kt
Outdated
Show resolved
Hide resolved
| binding.finalize.setOnClickListener { | ||
| listener.onSaveClicked(true) | ||
| } | ||
| if (formEndViewModel.shouldFormBeSentAutomatically(form)) { |
There was a problem hiding this comment.
I think it would be good for FormEndViewModel to encapsulate any accesses to Form here so that we don't need to pass one in to FormEndView for this. The ViewModel could use a similar method to FormEntryViewModel, FormSaveViewModel etc to load the form from the FormSessionRepository.
I think the formTitle param should really be dropped and should also be something the view gets from the ViewModel. This might require quite a chunk of rework however (as this data gets passed around between ViewModels), so I could understand it feeling like something we should punt on for the moment.
There was a problem hiding this comment.
I agree regarding the first part but I wasn't sure how to handle this to make the code clean. What do you think about 477b025 and keeping a formController and a form objects in the same repository? I think creating a separate repository just for the form object would be weird, they are both kind of a formSession so probably we should keep the both in the same place.
There was a problem hiding this comment.
That makes a lot of sense to me! I could see that we'll probably also want Instance (probably optional) on the form session eventually.
| @@ -566,7 +573,7 @@ private void setupViewModels(FormEntryViewModelFactory formEntryViewModelFactory | |||
| } | |||
|
|
|||
| private void formControllerAvailable(@NonNull FormController formController) { | |||
| formSessionRepository.set(sessionId, formController); | |||
| formSessionRepository.set(sessionId, formController, formsRepository.getOneByPath(formPath)); | |||
There was a problem hiding this comment.
I was wondering if we could avoid the extra DB lookup here, but it seems like that would be awkward. I'd hope that eventually the whole form load (loading from the DB and creating the JavaRosa objects) will happen in one neat place that could then set the form session after it's done.
There was a problem hiding this comment.
Yeah, I was thinking about passing form/instance objects from FormUriActivity to FormFillingActivity so that FormFillingActivity does not have to deal with that since FormUriActivity needs to read the database and validate the output either way. But that's something for the future.
collect_app/src/main/java/org/odk/collect/android/formentry/FormEndViewModel.kt
Outdated
Show resolved
Hide resolved
|
@getodk/testers I've merged this pr to continue work on other related issues please test it on master. |
|
Tested with Success! Verified on device with Android 10 Verified cases:
|
|
Tested with Success! Verified on device with Android 13 |
Closes #5417
What has been done to verify that this works as intended?
I've tested the changes manually and updated automated tests.
Why is this the best possible solution? Were any other approaches considered?
There is nothing important to discuss here.
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?
Please test the changes according to the acceptance criteria described in the issue. The biggest source of bugs might be mixing
autosendsettings - disabling/enablingautosendin settings and then doing the same on a form level. Then we can end up with: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 checkAlland confirmed all checks still pass OR confirm CircleCI build passes and run./gradlew connectedDebugAndroidTestlocally.