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

Additional alias highlighting #276

Merged
merged 5 commits into from
Aug 30, 2023
Merged

Conversation

elsaperelli
Copy link
Contributor

Summary

Fixes #275
This PR fixes the highlighting issue with the ValidEncounter alias in the above issue. This PR does not fix the clause coverage calculation issue and highlighting for reasonCode, that is being done in this PR in cql-execution.

New behavior

Added new handling to findAllLocalIdsInStatement to account for aliases that are nested within queries from CQL that was authored using QICore.

Code changes

  • ClauseResultsHelpers.ts - added branch to findAllLocalIdsInStatement to handle query expressions that have a source that is an array with a single value but that value does not have a localId. In this case (that seems to occur when the CQL is authored using QICore), the alias is within this source[0].expression.scope. The alias localId is then going to be one less than the localId of the query statement.
  • ClauseResultsHelpers.test.ts - added a unit test for this new handling that uses CQL query expression authored using QICore.
  • QICoreQuery.cql / QICoreQuery.json - test fixtures for the above unit test

Testing guidance

  • npm run check, npm run test:integration, run regression tests (all should pass)
  • Run detailed calculation on the above issue with a measurement period start of 2024-01-01 and a measurement period end of 2024-12-31 and the debug option off.
  • Look at the overall clause coverage html (cd debug/html). ValidEncounter should be highlighted (again, reasonCode will not be highlighted)
  • Look at the measure logic highlighting. ValidEncounter should be highlighted green (reasonCode will be highlighted red)

@elsaperelli elsaperelli marked this pull request as ready for review August 23, 2023 19:37
@github-actions
Copy link

github-actions bot commented Aug 23, 2023

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements
86.58% (+0.09% 🔼)
2355/2720
🟡 Branches
73.88% (+0.1% 🔼)
2189/2963
🟢 Functions 89.05% 423/475
🟢 Lines
86.94% (+0.1% 🔼)
2276/2618

Test suite run success

447 tests passing in 31 suites.

Report generated by 🧪jest coverage report action from 928086f

@sarahmcdougall sarahmcdougall self-requested a review August 24, 2023 13:24
@sarahmcdougall sarahmcdougall self-assigned this Aug 24, 2023
Copy link
Contributor

@sarahmcdougall sarahmcdougall left a comment

Choose a reason for hiding this comment

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

Great work on this - tested via several different methods (highlighting inspection, integration tests, unit tests, etc.) and everything looks good. Just have one small suggested change

Something we could consider in the future is refactoring the test/unit/fixtures/elm/queries directory since we have been adding a lot of files to it recently. Could put each test case in its own subdirectory (ex. a QICoreQuery subdirectory for the CQL and ELM used for the test). This is something we can keep in mind for end-of-sprint work, not something that needs to be addressed here

src/calculation/ClauseResultsHelpers.ts Show resolved Hide resolved
test/unit/ClauseResultsHelper.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@sarahmcdougall sarahmcdougall left a comment

Choose a reason for hiding this comment

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

The changes look good, and good call with adding the check for length 1 on the array 👍

@sarahmcdougall sarahmcdougall merged commit 98cc01c into master Aug 30, 2023
5 checks passed
@sarahmcdougall sarahmcdougall deleted the alias-coverage-highlight branch August 30, 2023 20:50
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.

Encounter.reasonCode is not recognized
3 participants