Fixed the problem with SelectOneWidget and minimal + search appearance in field-list #3181

grzesiek2010 · 2019-06-27T23:38:58Z

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

I tested the form attached to the issue.

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

I solved the issue in the first commit 62cae79
The problem was that the code in onItemSelected() was performed even if we set the item programmatically (during initiating the view) so widgetValueChanged() was called.
If we use that widget in a field-list group and with search appearance (086f852) it caused that such a widget was recreated every time, and again onItemSelected() was called (endless loop).

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?

Changes are related only to that one widget (SelectOneWidget with minimal appearance) so it's not risky and testing the widget would be enough.

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

The form attached to the issue.

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:

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

lognaturel

Thank you for that cleanup, especially expanding the 1-character names!!

collect_app/src/main/java/org/odk/collect/android/widgets/SpinnerWidget.java

lognaturel · 2019-06-28T03:08:58Z

Oh, how about adding a case in FieldListUpdateTest? I gave you write access to the test form so you can easily add to the bottom of it.

A value from the second widget was used in both spinners

grzesiek2010 · 2019-07-02T12:21:22Z

@lognaturel I added one test, you can review and add needs testing label if everything is fine.

lognaturel

Up to you whether you want to share the CSV data file. I'm a little more concerned about the sleep/InterruptedException.

Either way, these are test-only changes so don't need to be figured out before QA.

lognaturel · 2019-07-03T18:27:17Z

collect_app/src/androidTest/java/org/odk/collect/android/formentry/FieldListUpdateTest.java

+    public void search_function_in_field_list() throws InterruptedException {
+        jumpToGroupWithText("Search in field-list");
+        onView(withText(startsWith("Source15"))).perform(click());
+        Thread.sleep(1000);


Why are this sleep and the InterrputedException throwing needed? Espresso should be able to detect when the UI thread is idle. Is it related to database lookups? ExternalCsvSearchTest does something similar so I'm surprised.

It's because the spinner is not clickable immediately and the test didn't work.

lognaturel · 2019-07-03T18:28:56Z

collect_app/src/androidTest/java/org/odk/collect/android/formentry/FieldListUpdateTest.java

@@ -83,7 +84,7 @@
                    Manifest.permission.WRITE_EXTERNAL_STORAGE,
                    Manifest.permission.CAMERA)
            )
-            .around(new CopyFormRule(FIELD_LIST_TEST_FORM));
+            .around(new CopyFormRule(FIELD_LIST_TEST_FORM, "", Collections.singletonList("fruits.csv")));


Would you consider using the existing external-csv-search-produce.csv data file? I think it would be appropriate to move fieldlist-updates.xml to the forms subdirectory as well. Not a big deal if you prefer to keep them separate for now.

bognasoldev89 · 2019-07-05T11:11:48Z

Tested with success

Verified on Android: 4.2, 4.4, 5.1, 6.0, 7.0, 8.1

Verified cases:

SelectOneWidget with minimal + search appearance search list
Rotating during testing the form
Swiping while a mobile phone was vertically
Closing the app during testing the form
Minimizing the app during testing the form
Blocking mobile phones during testing
Saving changes
Dismissing changes

@opendatakit-bot unlabel "needs testing"
@opendatakit-bot label "behavior verified"

lognaturel · 2019-07-09T03:00:31Z

Thanks for testing, @bognasoldev89! @grzesiek2010, can you please fix the new conflict and merge? We can see if there's another option for the test when someone has a chance to dig deeper.

grzesiek2010 · 2019-07-09T10:24:22Z

can you please fix the new conflict and merge?

FIxed, but I can't merge without at least one approvement.

grzesiek2010 added 7 commits June 28, 2019 00:06

Call onItemSelected() only if it's a user's action

62cae79

Factored out SpinnerAdapter class

cf15b66

Removed redundant parameters from SpinnerAdapter

d66a8ed

Code improvements in SpinnerAdapter class

b6204d2

Improvements in SpinnerWidget class

50a8d65

Fixed pmd errors

d722004

Code improvements

2f21cac

lognaturel reviewed Jun 28, 2019

View reviewed changes

collect_app/src/main/java/org/odk/collect/android/widgets/SpinnerWidget.java Outdated Show resolved Hide resolved

grzesiek2010 added 4 commits July 2, 2019 09:47

Added tests

cc85890

Fixed the bug with two spinner widget on the same page

80025c3

A value from the second widget was used in both spinners

Improved the test

3de9fd6

Code improvements

0cf5aad

lognaturel reviewed Jul 3, 2019

View reviewed changes

lognaturel added the needs testing label Jul 3, 2019

getodk-bot added behavior verified and removed needs testing labels Jul 5, 2019

Merge branch 'master' into COLLECT-3180

e067a66

lognaturel approved these changes Jul 10, 2019

View reviewed changes

lognaturel merged commit 6de271c into getodk:master Jul 10, 2019

lognaturel mentioned this pull request Jul 10, 2019

Investigate why a sleep is needed for Espresso test with minimal select in field list #3203

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed the problem with SelectOneWidget and minimal + search appearance in field-list #3181

Fixed the problem with SelectOneWidget and minimal + search appearance in field-list #3181

grzesiek2010 commented Jun 27, 2019 •

edited

lognaturel left a comment

lognaturel commented Jun 28, 2019

grzesiek2010 commented Jul 2, 2019

lognaturel left a comment

lognaturel Jul 3, 2019

grzesiek2010 Jul 4, 2019

lognaturel Jul 3, 2019

bognasoldev89 commented Jul 5, 2019

lognaturel commented Jul 9, 2019

grzesiek2010 commented Jul 9, 2019

Fixed the problem with SelectOneWidget and minimal + search appearance in field-list #3181

Fixed the problem with SelectOneWidget and minimal + search appearance in field-list #3181

Conversation

grzesiek2010 commented Jun 27, 2019 • edited

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 left a comment

Choose a reason for hiding this comment

lognaturel commented Jun 28, 2019

grzesiek2010 commented Jul 2, 2019

lognaturel left a comment

Choose a reason for hiding this comment

lognaturel Jul 3, 2019

Choose a reason for hiding this comment

grzesiek2010 Jul 4, 2019

Choose a reason for hiding this comment

lognaturel Jul 3, 2019

Choose a reason for hiding this comment

bognasoldev89 commented Jul 5, 2019

lognaturel commented Jul 9, 2019

grzesiek2010 commented Jul 9, 2019

grzesiek2010 commented Jun 27, 2019 •

edited