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

Disallow string downcast for groups and repeats #765

Merged
merged 1 commit into from
Feb 13, 2023
Merged

Conversation

ktuite
Copy link
Member

@ktuite ktuite commented Feb 13, 2023

Closes #381

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

Unskipping some tests that existed before.

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

It was easy to add this to the downcast checking function!

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?

Shouldn't have a big impact on users unless they're changing their field types a lot. Should help people keep their data cleaner.

Does this change require updates to the API documentation? If so, please update docs/api.md as part of this PR.

I don't think so.

Before submitting this PR, please make sure you have:

  • run make test-full and confirmed all checks still pass OR confirm CircleCI build passes

@matthew-white matthew-white self-requested a review February 13, 2023 19:04
@ktuite ktuite merged commit c90025b into master Feb 13, 2023
@ktuite ktuite deleted the ktuite/downcast branch February 13, 2023 19:36
@ktuite
Copy link
Member Author

ktuite commented Feb 28, 2023

Hi QA!
Here are two forms that represent the change from a group field to a string with the same name as a group:

group downcast.xlsx

group downcast (group is now string).xlsx

I just tried on https://test.getodk.cloud/#/projects/445 (not updated? at v2023.1.) and it is allowed.

Also, here's the message that mentions 'now in different groups or repeats' that it detected but didn't stop:
Screen Shot 2023-02-27 at 5 08 58 PM

It should not be allowed in staging.

@dbemke
Copy link

dbemke commented Feb 28, 2023

Hi. Thank you for the information, it was really helpful to know where the change to string happened. I changed the forms a bit and tried to upload them to Central. I've noticed that if I change the group/repeat field to a string and put them inside a repeat/group I'm still able to upload the one changed to a string. I don't know if it's a valid case - should it also be prevented.
insiderepeatgroup
group.downcast.xlsx
group.downcast.group.is.now.string.xlsx

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.

Disallow string downcast for groups and repeat groups
3 participants