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

Fixed sending forms with repeat groups to Google Sheets #2897

Merged
merged 1 commit into from
Feb 21, 2019

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Feb 20, 2019

Closes #2896

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

I tested a form with repeat groups inside a regular group.

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

It's just a bug that has existed from the beginning. We missed the case when a repeat group is inside a regular group.

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?

The pr introduces some changes to InstanceGoogleSheetsUploader so sending forms to Google Sheets should be tested. @mmarciniak90 I count on you and as I described below please test some complex forms: forms with repeat groups, repeat groups inside regular groups, nested repeat groups etc.

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

Any form with repeat groups, repeat groups inside regular groups, nested repeat groups etc.

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
Copy link
Member Author

@zestyping @cooperka
It would be great to add it to v1.20 (since it's an important issue) so please review.

@yanokwa yanokwa added this to the v1.20 milestone Feb 21, 2019
Copy link
Contributor

@cooperka cooperka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some complex logic in this file but it's a simple change; if the testing pans out then LGTM

@opendatakit-bot label "needs testing"

@cooperka
Copy link
Contributor

I guess 🤖 doesn't see reviews... @opendatakit-bot label "needs testing"

@mmarciniak90
Copy link
Contributor

Tested with success

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

I was able to reproduce the problem.
Sheet for the repeated group was not created and hyperlink shows null in URL.
Problem is not visible with the fix.

Verified forms:

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

@yanokwa yanokwa removed the request for review from zestyping February 21, 2019 17:01
@yanokwa yanokwa merged commit cac7a60 into getodk:master Feb 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants