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

Relax validation constraint on Observation value field #4539

Closed
mlm483 opened this issue May 18, 2023 · 2 comments · Fixed by #4567
Closed

Relax validation constraint on Observation value field #4539

mlm483 opened this issue May 18, 2023 · 2 comments · Fixed by #4567
Assignees
Labels
Priority: High Issue/PR significantly impacts users. Type: Implementation Issue proposes a non-feature change to implementation.

Comments

@mlm483
Copy link

mlm483 commented May 18, 2023

Expected Behavior

When Observations are sent to BreedBase via BrAPI, any string should be valid for the Observation value field.

Details

In /lib/CXGN/Phenotypes/ParseUpload/Plugin/Observations.pm, on line 227, the following regular expression is used to check for invalid characters: [^a-zA-Z0-9,.\-\/\_:;\s]. As a result, POSTing Observations with characters such as "?" in the value field produces an error.

After discussing this with everyone at the lab meeting today, I believe this whole validation of the value field should be removed. The only remaining concern is whether there are other parts of the code that rely on this constraint.

See the BrAPI spec for Observation.

For Bugs:

Environment

Steps to Reproduce

@nmenda nmenda self-assigned this May 18, 2023
@nickpalladino
Copy link
Contributor

@nmenda I had a similar issue in the phenotype file saving code. It was causing issues for us when trying to do Field Book file uploads due to not allowing spaces. I removed the alphanumeric validation constraint and made a few other changes to support our use case of creating variables with BrAPI and using those to sync with Field Book using BrAPI or through Breedbase file export / import. You can review the PR at Breeding-Insight#117 and let me know if you'd be interested in having those changes upstream.

@mlm483
Copy link
Author

mlm483 commented May 24, 2023

@nmenda I also opened a PR on Breeding Insight's fork (because this is a blocker for us), if you'd like to take a look: Breeding-Insight#118.

@lukasmueller lukasmueller added Type: Implementation Issue proposes a non-feature change to implementation. Priority: High Issue/PR significantly impacts users. labels May 25, 2023
@nmenda nmenda closed this as completed Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High Issue/PR significantly impacts users. Type: Implementation Issue proposes a non-feature change to implementation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants