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 removing repeat groups #3327

Merged
merged 8 commits into from
Sep 13, 2019
Merged

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Aug 30, 2019

Closes #3033

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

I tested implemented changes manually and added automated tests.

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

It was a regression, we fixed one issue 2 years ago #1034 which introduced this problem so I just combined two solutions to handle both cases.

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 change is not very risky, I added automated tests so testing the form attached to this pr would be enough but @mmarciniak90 you can try some more complicated forms of course as well to be sure everything is fine.

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

The form attached to this pr https://github.com/opendatakit/collect/compare/master...grzesiek2010:COLLECT-3033?expand=1#diff-fe55d3110049e63221b4a8a2a49067cb

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

Copy link
Member

@lognaturel lognaturel left a comment

Choose a reason for hiding this comment

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

Fix looks good but the tests will take a long time to run. I've asked some questions and made suggestions inline.

Copy link
Member

@lognaturel lognaturel left a comment

Choose a reason for hiding this comment

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

The test split looks good to me. I think on aggregate it probably takes longer to run but I think what's more important is that they're more likely to help us identify and fix an issue this way.

Some of the cases have incorrect names.

I also can't run the full test suite. I get java.lang.IllegalStateException: #init was called twice in a row. Make sure to call #release after every #init and haven't looked any deeper yet.

@grzesiek2010
Copy link
Member Author

I also can't run the full test suite. I get java.lang.IllegalStateException: #init was called twice in a row. Make sure to call #release after every #init

I haven't noticed anything like that.

@lognaturel
Copy link
Member

I still see the issue running the tests on Pixel 2 / API27 but if you and @seadowg can't reproduce it's probably something about my state.

@seadowg
Copy link
Member

seadowg commented Sep 13, 2019

@lognaturel hmmm. That's weird. Have could you do a wipe on the emulator and then try?

@bognasoldev89
Copy link

Tested with success

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

Verified cases:

  • Fixed removing repeat groups
  • Rotating phone
  • Swiping back and forth
  • Blocking phone
  • Minimizing phone
  • Saving and opening saved form
  • Finalizing and opening finalized form
  • Sending form to Aggregate

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

@lognaturel lognaturel merged commit 82b9db8 into getodk:master Sep 13, 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.

When repeat with field-list appearance is deleted, the wrong repeat instance is removed
5 participants