Skip to content
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

Improved handling invalid geo answers #5533

Merged
merged 4 commits into from
Mar 31, 2023
Merged

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Mar 30, 2023

Closes #5532

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

I've added automated tests.

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

I wasn't able to reproduce the issue in real world (only hardcoding invalid answers or using tests). I believe the problem was in setData() methods where we didn't protect from injecting invalid answers. I've fixed those methods and also fixed reading answers when widgets are created.
I think that after introducing those improvements this issue https://console.firebase.google.com/u/1/project/api-project-322300403941/crashlytics/app/android:org.odk.collect.android/issues/324b108b1880c264d9c3f01101110e03?time=last-seven-days&versions=v2023.1.0%20(4611)&types=crash&sessionEventKey=642570CE003900014975C8483B90F492_1794743691754173217 was also handled but just in case I've added improvements there too in the last commit.

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 wasn't able to reproduce the issue so we can focus on testing the GeoPoint and GeoPointMapWidget widgets for regression.

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:

  • 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

@grzesiek2010 grzesiek2010 force-pushed the COLLECT-5532 branch 2 times, most recently from 0ca4e31 to 901818b Compare March 30, 2023 13:01
@seadowg seadowg added this to the v2023.1.x milestone Mar 30, 2023
@grzesiek2010 grzesiek2010 changed the title Collect 5532 Improved handling invalid geo answers Mar 30, 2023
@grzesiek2010 grzesiek2010 force-pushed the COLLECT-5532 branch 2 times, most recently from 380283b to 3b050d4 Compare March 30, 2023 16:05
@grzesiek2010 grzesiek2010 marked this pull request as ready for review March 30, 2023 16:30
@seadowg
Copy link
Member

seadowg commented Mar 30, 2023

I thought I'd be able to reproduce this by adding an invalid default to a geopoint question. So in all-widgets.xml I change it so that we have the following in the instance:

<geopoint_widget>blah</geopoint_widget>

That weirdly doesn't cause the issue though. If I set a valid default (like "51.5074 -0.1278") that does work though. I can't see anything that Collect does to clean that up. Can @lognaturel or @grzesiek2010 explain what's happening there?

@grzesiek2010
Copy link
Member Author

grzesiek2010 commented Mar 30, 2023

I've tried defaults and calculations to no avail. This is handled in javarosa. I don't know where exactly because I don't need it but it is. That's why I think invalid data can be only passed via the setData method, for example an external GPS device might be used that sometimes returns invalid data.

@grzesiek2010 grzesiek2010 added the high priority Should be looked at before other PRs/issues label Mar 31, 2023
@srujner
Copy link

srujner commented Mar 31, 2023

Tested with Success!

Verified on device with Android: 12, 13

Verified cases:
• Regression checks for GeoPoint, GeoTrace, GeoShape widgets;
• Select Map forms
• Google Maps, Mapbox, OSM;
• Collecting Location in background;
• View Submissions on map;
• Screen rotation, minimize app, using backward button

@seadowg seadowg merged commit 15b2688 into getodk:master Mar 31, 2023
@seadowg seadowg removed this from the v2023.1.x milestone Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behavior verified high priority Should be looked at before other PRs/issues
Projects
Status: done
Development

Successfully merging this pull request may close these issues.

NullPointerException: Attempt to get length of null array in GeoPointWidget.java:80
3 participants