Fixed enable auto-send schedules submission of completed forms #3980 #4757

TomRCummings · 2021-07-30T00:20:09Z

What has been done to verify that this works as intended?

Tested manually with device and added both Roboelectric and Espresso tests.

Why is this the best possible solution? Were any other approaches considered?

The desired behavior results from a change in the Auto-send form management preference, so it seems appropriate to handle that behavior with onSettingChange listener function in FormManagementPreferencesFragment.

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?

With this change, any time the Auto-send preference is toggled to something other than "off" a new submitSchedule call is made to the InstanceSubmitScheduler. I don't think this carries any risk with it, but I am not totally sure. Is it necessary to call cancelSubmit if a submission has been scheduled but not yet completed and the user toggles Auto-send to something different?

Do we need any specific form for testing your changes? If so, please attach one.

No specific form required.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No documentation changes required.

Before submitting this PR, please make sure you have:

run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
verified that any code or assets from external sources are properly credited in comments and/or in the about file.
verified that any new UI elements use theme colors. UI Components Style guidelines

This commit solves the issue where finalized forms are not sent when the auto send preference is enabled, until another form is finalized. Resolves getodk#3980.

The test starts a blank form and finalizes it, enables auto send, then asserts that the form is listed in the "Sent Forms" and there is a notification about the auto send.

Added a function that simplifies the Espresso test for issue getodk#3980.

lognaturel · 2021-07-31T21:16:39Z

Thanks so much for taking this on, @TomRCummings! I took a quick look and it looks ready for review to me. Is there a reason it's still a draft?

TomRCummings · 2021-08-01T19:33:55Z

Excellent, I'll open it for review! I opened it as a draft out of an excess of caution.

seadowg · 2021-08-06T10:47:57Z

@TomRCummings apologies on the delay in getting this reviewed. We're just finishing off our 2021.2 release (which was a bigger one than usual). I'm hoping we can start looking at PRs again next week.

seadowg

This is looking great! I think we should tweak the approach to testing slightly.

seadowg · 2021-08-10T08:47:03Z

collect_app/src/androidTest/java/org/odk/collect/android/support/pages/FormManagementPage.java

@@ -19,4 +19,9 @@ public FormManagementPage assertOnPage() {
        clickOnString(R.string.form_update_frequency_title);
        return new ListPreferenceDialog<>(R.string.form_update_frequency_title, this).assertOnPage();
    }
+
+    public ListPreferenceDialog<FormManagementPage> clickAutoSend() {


This is unused. Were you planning to use it in MainMenuPage#enableAutoSend?

I was yes, but ended up not. Do you think it is still worthwhile to keep to use for testing in the future? Or get rid of it since we got rid of the MainMenuPage#enableAutoSend test?

Good question. I guess it might come in handy some day! Given it's test code not app code it's not going to contribute to the binary size or anything so nice to have around.

seadowg · 2021-08-10T08:58:12Z

...st/java/org/odk/collect/android/preferences/screens/FormManagementPreferencesFragmentTest.kt

+
+        val scenario = FragmentScenario.launch(FormManagementPreferencesFragment::class.java)
+        scenario.onFragment { fragment: FormManagementPreferencesFragment ->
+            generalSettings.save(ProjectKeys.KEY_AUTOSEND, "wifi")


Instead of changing the value in the underlying Settings (where settings are stored) we probably want to try simulating changing the Android Preference (the UI component) value. That could be like:

fragment.findPreference<ListPreference>(ProjectKeys.KEY_AUTOSEND)!!.value = "wifi"

That will just check that the UI is hooked up correctly as well as the listeners on settings.

seadowg · 2021-08-10T08:59:06Z

...st/java/org/odk/collect/android/preferences/screens/FormManagementPreferencesFragmentTest.kt

@@ -487,4 +499,15 @@ class FormManagementPreferencesFragmentTest {
            assertThat(fragment.findPreference<PreferenceCategory>("form_import")!!.isVisible, `is`(true))
        }
    }
+
+    @Test
+    fun `When Auto send preference is enabled, finalized forms should be scheduled for submission`() {


I think in addition to this we should have a test that checks that when I switch auto send off it doesn't schedule submissions. This prevents us from forgetting to have the if wrapped around the submission scheduling in the implementation.

seadowg · 2021-08-10T09:04:42Z

...pp/src/androidTest/java/org/odk/collect/android/feature/instancemanagement/AutoSendTest.java

@@ -65,4 +65,25 @@ public void whenFormHasAutoSend_fillingAndFinalizingForm_sendsFormAndNotifiesUse
        notificationDrawerRule.open()
                .assertAndDismissNotification("ODK Collect", "ODK auto-send results", "Success");
    }
+
+    @Test
+    public void whenFormIsFinalized_enablingAutoSend_schedulesSubmitFinalizedForm() {


I think we have doubled up test coverage here between this and the test in FormManagementPreferencesFragmentTest. More than happy to discuss different approaches (here or on Slack), as obviously there are no right answers here, but I'd suggest removing this test and sticking with the coverage at the Fragment level for this feature.

As a side note, I would often start with a test like this for a feature and then if I discover I can write a more focused test that covers all the same bases at a lower level later, I'd get rid of my high level one. I've found I've been doing that more in Collect than I would have in other apps (where I might just have kept both tests) as the app has so many features that the sheer number of tests is hard to keep under control.

This makes sense, especially alongside changing the other test to change the Auto send preference using the Android preference component.

Previously, this test changed the underlying generalSettings object to simulate a user changing the Auto send preference. By using the Android preference object in the test instead, it also checks that the UI component behaves properly, along with the underlying settings change listener.

Test checks that no submissions are scheduled when the Auto send preference is disabled.

Previously in the Auto send tests, the GeneralSettings container was changed directly to set a pre-test value for the Auto send preference. This was unnecessary, since the behavior required after changing the preference does not depend on its previous state.

This test overlapped with the Roboelectric tests in FormManagementPreferencesFragmentTest.kt. Since the behavior being tested is fairly low-level, the Roboelectric test makes more sense.

seadowg

Looks ace! Thanks for the working on the suggested changes.

seadowg · 2021-08-11T10:52:04Z

Is it necessary to call cancelSubmit if a submission has been scheduled but not yet completed and the user toggles Auto-send to something different?

I think it's fine not to. Here's my thinking: there is a case where the user has finished filling in forms while offline, and then they switch off autosend, come back online and the autosend task runs (because it was scheduled before we disabled autosend) and sends forms. I think it's hard to say whether the enumerator would expect that or not, but it feels worse to me to not send forms if they expect them to send than the other way around. Does that make sense?

TomRCummings · 2021-08-11T13:01:00Z

@seadowg Since writing that, I've had a closer look at AutoSendTaskSpec#getTask. In the case you outline, if the user has their general autosend preference set to "off", even if a submission was scheduled while the preference was set to something different, when the user comes back online the form will not send since AutoSendTaskSpec#networkTypeMatchesAutoSendSetting checks the current value of autosend, not the value of the preference when the submission was scheduled. I could be totally wrong about that!

seadowg · 2021-08-11T13:17:08Z

@TomRCummings no you're totally right! I always forget that the check gets made again. It's all a little confusing, as really that check should happen in WorkManager (we can tell it to only run under certain network conditions). That code is a lot older than WorkManager though. I think we're all good as there won't be a behaviour change here when disabling auto send (just when enabling it).

Thanks for working on this! Let me know (or post on Slack) if you'd like more issues to work on.

kkrawczyk123 · 2021-08-16T13:50:41Z

Tested with success!
Verified on Androids 5.1 and 11.

Tested scenarios:

wifi, cellular, both
different servers: Aggregate, Central, Ona, Kobo, Google drive
autosend in form management
autosend defined in form
internet connection turned off
internet connection restored
airplane mode
autosend when app working in background

mmarciniak90 · 2021-08-16T20:00:16Z

Tested with success also on Android versions 8.1, 9.0, 10.0

grzesiek2010 · 2021-08-16T20:05:40Z

Thanks @TomRCummings

TomRCummings added 5 commits July 29, 2021 00:24

Schedule submit when autosend is enabled. Fix getodk#3980

0270523

This commit solves the issue where finalized forms are not sent when the auto send preference is enabled, until another form is finalized. Resolves getodk#3980.

Add Espresso test for getodk#3980

e13853a

The test starts a blank form and finalizes it, enables auto send, then asserts that the form is listed in the "Sent Forms" and there is a notification about the auto send.

Add Roboelectric test for getodk#3980

a0a9d5a

Add clickAutoSend to FormManagementPage

6f4b9c8

Added a function that simplifies the Espresso test for issue getodk#3980.

Remove unused import

9442a52

TomRCummings marked this pull request as ready for review August 1, 2021 19:39

seadowg requested changes Aug 10, 2021

View reviewed changes

seadowg reviewed Aug 10, 2021

View reviewed changes

TomRCummings added 5 commits August 10, 2021 07:57

Add test for Auto send disable

7709468

Test checks that no submissions are scheduled when the Auto send preference is disabled.

Remove unnecessary Espresso test

77a6660

This test overlapped with the Roboelectric tests in FormManagementPreferencesFragmentTest.kt. Since the behavior being tested is fairly low-level, the Roboelectric test makes more sense.

Fix import order

174931c

seadowg approved these changes Aug 11, 2021

View reviewed changes

seadowg added the needs testing label Aug 11, 2021

mmarciniak90 added behavior verified and removed needs testing labels Aug 16, 2021

grzesiek2010 merged commit d34dd0f into getodk:master Aug 16, 2021

TomRCummings deleted the Fix#3980 branch August 17, 2021 11:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed enable auto-send schedules submission of completed forms #3980 #4757

Fixed enable auto-send schedules submission of completed forms #3980 #4757

TomRCummings commented Jul 30, 2021

lognaturel commented Jul 31, 2021

TomRCummings commented Aug 1, 2021

seadowg commented Aug 6, 2021 •

edited

Loading

seadowg left a comment

seadowg Aug 10, 2021

TomRCummings Aug 10, 2021

seadowg Aug 11, 2021

seadowg Aug 10, 2021

seadowg Aug 10, 2021

seadowg Aug 10, 2021

seadowg Aug 10, 2021

TomRCummings Aug 10, 2021

seadowg left a comment

seadowg commented Aug 11, 2021 •

edited

Loading

TomRCummings commented Aug 11, 2021

seadowg commented Aug 11, 2021

kkrawczyk123 commented Aug 16, 2021

mmarciniak90 commented Aug 16, 2021

grzesiek2010 commented Aug 16, 2021

Fixed enable auto-send schedules submission of completed forms #3980 #4757

Fixed enable auto-send schedules submission of completed forms #3980 #4757

Conversation

TomRCummings commented Jul 30, 2021

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

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?

Do we need any specific form for testing your changes? If so, please attach one.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Before submitting this PR, please make sure you have:

lognaturel commented Jul 31, 2021

TomRCummings commented Aug 1, 2021

seadowg commented Aug 6, 2021 • edited Loading

seadowg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seadowg left a comment

Choose a reason for hiding this comment

seadowg commented Aug 11, 2021 • edited Loading

TomRCummings commented Aug 11, 2021

seadowg commented Aug 11, 2021

kkrawczyk123 commented Aug 16, 2021

mmarciniak90 commented Aug 16, 2021

grzesiek2010 commented Aug 16, 2021

seadowg commented Aug 6, 2021 •

edited

Loading

seadowg commented Aug 11, 2021 •

edited

Loading