onc-healthit / onc-certification-g10-test-kit

ONC Certification (g)(10) Standardized API Tests
Apache License 2.0
34 stars 11 forks source link

FI-2532: Add feature flag to toggle using HL7 validator wrapper #497

Closed dehall closed 6 months ago

dehall commented 7 months ago

This PR adds a new feature flag to toggle between using the current Inferno validator wrapper and the HL7 validator wrapper. Set env var USE_HL7_RESOURCE_VALIDATOR = true to switch over to the HL7 validator, leave it blank/unset or set to anything else and it will use the current Inferno validator.

Summary of code changes

To make this a "one-click" toggle we'll need to have both validators enabled in the docker-compose file and nginx. With that in mind some of the changes here are just reverting changes from the WIP branch on #488 to re-enable the Inferno validator, so it may be useful to review the diff against main as well. If we're ok with the 3-step directions of "change env var, uncomment this service in docker-compose.yml, uncomment this section in nginx.conf" then we could leave the the HL7 validator entries commented out by default.

The real logic that handles the toggle is in onc_certification_g10_test_kit.rb - just a simple if so that the cliContext and igs settings aren't called for the inferno validator, and a conditional to call validator vs fhir_resource_validator. I also noticed an instance where the capitalization of one error message made it slip through the filter so I updated that here as well.

Most of the remaining changes are for testing purposes. I didn't want to just skip tests depending on an env var, so I duplicated the resource_validation_spec.rb tests to run once for each type of validator. To make that work I needed a way to "reset" the validators so they reload appropriately per the env var. The simplest approach was to break the validator setup logic out into its own function that could be invoked later. Unfortunately rubocop checks cyclomatic complexity on method but not arbitrary blocks in the class definition so that didn't pass rubocop. Rather than a more significant refactor I just disabled that check in that one spot. If there's a better way to do this kind of "class reload" let me know. Then there's another test that I didn't know if it was worth a more significant refactor: bulk_data_group_export_validation_spec.rb, so this test just checks the current state of the env var and handles it appropriately. Let me know if this is worth digging more into.

Testing Guidance

No special guidance needed - as currently configured this should require only the one env var to be changed to toggle the desired result. You don't even need to restart services after toggling the option.

Jammjammjamm commented 7 months ago

With the feature flag, I think we should leave all the config defaulting to the old validator. Stick the new env var in .env, set it to false, and put the additional instructions for enabling the new validator there in .env so you see it when you're trying to change the validator.

dehall commented 6 months ago

Updated - everything defaults to the old validator now. Again that makes the diff on this PR against the earlier "HL7 validator" pr look a little confusing, so looking at the diff with main may be helpful to confirm where things aren't really changing.