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

Validate if values are whitespace only #639

Merged
merged 2 commits into from Nov 8, 2023
Merged

Conversation

aaron-collier
Copy link
Contributor

NOTE: Changes to openapi.yml require updating openapi.yml for sdr-api and dor-services-app and generating models - see README.

Why was this change made? 🤔

This is draft while wrapping up styling/refactoring...

How was this change tested? 🤨

⚡ ⚠ If this change has cross service impact, run integration tests and/or test in [stage|qa] environment, in addition to specs. ⚡


raise ValidationError, "Multiple value, groupedValue, structuredValue, and parallelValue in description: #{error_paths.join(', ')}"
raise ValidationError, "Multiple value, groupedValue, structuredValue, and parallelValue in description: #{error_paths_multiple.join(', ')}" unless error_paths_multiple.empty?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should it be able to return both kinds of errors at once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that's possible, at least as written. The multiples requires the top level value to be present and it's not when blank. Perhaps this is a topic for follow up discussion with @andrewjbtw or @arcadiafalcone

@aaron-collier aaron-collier force-pushed the validate-blank-values branch 2 times, most recently from 3234b0c to 63c7075 Compare November 1, 2023 20:58
@aaron-collier aaron-collier changed the title [DRAFT] Validate if values are whitespace only Validate if values are whitespace only Nov 1, 2023
@aaron-collier aaron-collier marked this pull request as ready for review November 1, 2023 21:08
@mjgiarlo
Copy link
Member

mjgiarlo commented Nov 1, 2023

Wondering if this is the sort of change we might want to run against a sizable subset of prod data to see how this affects validation.

@justinlittman
Copy link
Contributor

This should be validated against all items in all environments else chaos ensues.

@aaron-collier aaron-collier changed the title Validate if values are whitespace only [HOLD] Validate if values are whitespace only Nov 2, 2023
@aaron-collier
Copy link
Contributor Author

@mjgiarlo / @justinlittman agreed. I've marked as hold.

@jcoyne
Copy link
Collaborator

jcoyne commented Nov 3, 2023

@aaron-collier how long will this take to validate? We're getting hammered with these errors again/still.

@aaron-collier aaron-collier force-pushed the validate-blank-values branch 3 times, most recently from 9ef7bd3 to 0ef861f Compare November 3, 2023 19:20
@aaron-collier
Copy link
Contributor Author

@andrewjbtw here is a list of the druids that vail cocina validation with this change. 1073 objects. Should these be remediated before this change goes in?

validate-cocina.csv

@aaron-collier aaron-collier changed the title [HOLD] Validate if values are whitespace only Validate if values are whitespace only Nov 6, 2023
@aaron-collier
Copy link
Contributor Author

Removing hold per @andrewjbtw:

I think these are all fixed. I’m not sure what will happen if MARC-origin updates trigger this issue after we turn on the validation, but I don’t see how we can chase down where those past errors came from. It’s worth turning on the validation for spreadsheet imports and then seeing how we can catch the MARC-related issues if they happen again.

@mjgiarlo / @justinlittman ready for a follow up review.

@aaron-collier aaron-collier merged commit a474ff1 into main Nov 8, 2023
6 checks passed
@aaron-collier aaron-collier deleted the validate-blank-values branch November 8, 2023 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants