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

Enable toggling display issues as warnings or errors #61

Merged
merged 4 commits into from
Jul 24, 2023

Conversation

dehall
Copy link
Contributor

@dehall dehall commented Jun 21, 2023

Summary

Addresses FI-1994 and FI-1995

This PR bumps the HL7 validator dependency to 6.0.21 so that we can toggle whether code display errors from the terminology server are reported as warnings or errors. This also checks for a new environment variable DISPLAY_ISSUES_ARE_WARNINGS to toggle it on and off per instance. (Unfortunately it's a setting on the validator instance not a parameter of the validate function, so given the current architecture we can't easily toggle it per-request.) The env var was chosen to match the setting name used on the official validator CLI. The default matches the default of the official validator, where code display issues are errors.

Note 1: this PR is marked as WIP while I continue to test it out to ensure nothing breaks elsewhere

Note 2: I pushed the 2 changes here to separate branches so that if we want to start with just the bumped dependency version we can do that -- see branch bump_hl7_validator

Note 3: CI is still failing, see #60 to resolve that

Testing Guidance

I have two minimal sample resources to test with in src/test/resources

  1. Test the validator by launching it without the new env var and validating some resources. Resources with a display issue should return errors. Resources without issues should still be fine
  2. Test the validator by launching it with DISPLAY_ISSUES_ARE_WARNINGS=true and validating resources. Resources with a display issue should return warnings. Resources without issues should still be fine
  3. Test the validator by running G10 tests against it and make sure nothing unexpected breaks

@dehall
Copy link
Contributor Author

dehall commented Jun 26, 2023

Update: even with DISABLE_TX=true the updated version of the HL7 validator produces some additional errors when validating the sample patients from the reference server, see snippet below. This breaks the (g)(10) test kit. I'll make a separate PR for the reference server data which should go live before this so that everything continues to work in prod.
image
Edit: this is resolved by using the "DISPLAY_ISSUES_ARE_WARNINGS" flag

@@ -64,7 +77,7 @@ public Validator(String igDir) throws Exception {
// The two lines below turn off URL resolution checking in the validator.
// This eliminates the need to silence these errors elsewhere in Inferno
// And also keeps contained resources from failing validation based solely on URL errors
ValidationControl vc = new BaseValidator(null, null)
ValidationControl vc = new BaseValidator(null, null, false)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dehall dehall marked this pull request as ready for review July 12, 2023 14:37
@dehall dehall changed the title WIP: Enable toggling display issues as warnings or errors Enable toggling display issues as warnings or errors Jul 12, 2023
@dehall dehall requested a review from Jammjammjamm July 12, 2023 14:42
@Jammjammjamm
Copy link
Contributor

We'll have to make g10 updates to deal with these errors:

Screenshot 2023-07-13 at 9 37 26 AM

Screenshot 2023-07-13 at 9 37 41 AM

@dehall
Copy link
Contributor Author

dehall commented Jul 13, 2023

@Jammjammjamm Yes apologies I forgot to link the associated g10 PR -- that change is made here: onc-healthit/onc-certification-g10-test-kit#449

@dehall dehall merged commit 8474832 into master Jul 24, 2023
@dehall dehall deleted the toggle_display_warnings branch July 24, 2023 20:09
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.

2 participants