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

Return false instead of crashing when asking if a repeat can be added at a deleted position #734

Merged
merged 3 commits into from
Nov 16, 2023

Conversation

grzesiek2010
Copy link
Member

Closes #getodk/collect#5539

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

I've added tests and also tested the fix with ODK Collect.

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

In ODK Collect when we remove a repeat we need to find the last relevant index. In this case we navigate back and check every possible index. If something does not exist we don't want any exceptions but just simple false value.

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?

It's a safe fix so testing the described scenario should be enough.

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

any form with nested repeats and repeat_count like the one attached in getodk/collect#5539

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

No.

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.

I initially was concerned that this change might negatively affect certain cases but I've spent some time trying to come up with an example and I can't. I confirmed that it does fix the issue that was filed.

@lognaturel lognaturel changed the title Fixed canCreateRepeat method to avoid having exceptions when looking for the last relevant index Return false instead of crashing when asking if a repeat can be added at a deleted position Nov 16, 2023
@lognaturel lognaturel merged commit 9ae4796 into master Nov 16, 2023
3 checks passed
@lognaturel lognaturel deleted the canCreateRepeat branch November 16, 2023 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants