From 69dcd10f86345b48f60937523faa6f722eac9ad0 Mon Sep 17 00:00:00 2001 From: hossenlopp Date: Fri, 1 Nov 2024 10:56:52 -0400 Subject: [PATCH] Relationship alias coverage fixes (#317) * Initial incomplete fix for relationship alias coverage * Added code to clause results helpers to find the relationship expression alias id in annotation structure, replacing old +1 hack * reworked html generation tests to use input more reflective of actual usage * Update comments for clarity * Check for undefined alias id and update iterator variable * Added UNHIT results to the HTMLBuilder Tests --------- Co-authored-by: lmd59 --- src/calculation/HTMLBuilder.ts | 6 +- src/helpers/ClauseResultsHelpers.ts | 89 +++++++++++++++-- test/unit/HTMLBuilder.test.ts | 99 ++++++++++++++----- .../html/simpleCoverageAnnotation.html | 17 +++- .../html/simpleCoverageAnnotation2.html | 41 ++++---- .../fixtures/html/simpleFalseAnnotation.html | 29 ------ .../html/simpleHighlightingAnnotation.html | 37 +++++++ .../fixtures/html/simpleTrueAnnotation.html | 29 ------ 8 files changed, 235 insertions(+), 112 deletions(-) delete mode 100644 test/unit/fixtures/html/simpleFalseAnnotation.html create mode 100644 test/unit/fixtures/html/simpleHighlightingAnnotation.html delete mode 100644 test/unit/fixtures/html/simpleTrueAnnotation.html diff --git a/src/calculation/HTMLBuilder.ts b/src/calculation/HTMLBuilder.ts index ef1a81c7..caef87dc 100644 --- a/src/calculation/HTMLBuilder.ts +++ b/src/calculation/HTMLBuilder.ts @@ -84,11 +84,11 @@ Handlebars.registerHelper('highlightCoverage', (localId, context) => { const libraryName: string = context.data.root.libraryName; const clauseResults: ClauseResult[] = context.data.root.clauseResults; - const clauseResult = clauseResults.filter(result => result.libraryName === libraryName && result.localId === localId); + const clauseResult = clauseResults.find(result => result.libraryName === libraryName && result.localId === localId); if (clauseResult) { - if (clauseResult.some(c => c.final === FinalResult.TRUE)) { + if (clauseResult.final === FinalResult.TRUE) { return objToCSS(cqlLogicClauseCoveredStyle); - } else if (clauseResult.every(c => c.final === FinalResult.FALSE || c.final === FinalResult.UNHIT)) { + } else if (clauseResult.final === FinalResult.FALSE || clauseResult.final === FinalResult.UNHIT) { return objToCSS(cqlLogicUncoveredClauseStyle); } } diff --git a/src/helpers/ClauseResultsHelpers.ts b/src/helpers/ClauseResultsHelpers.ts index f53de0cd..03bd40c1 100644 --- a/src/helpers/ClauseResultsHelpers.ts +++ b/src/helpers/ClauseResultsHelpers.ts @@ -1,4 +1,4 @@ -import { ELMProperty } from '../types/ELMTypes'; +import { Annotation, AnnotationStatement, ELMProperty } from '../types/ELMTypes'; import { ELMFunctionRef } from '../types/ELMTypes'; import { ELM, ELMBinaryExpression, ELMStatement } from '../types/ELMTypes'; @@ -33,7 +33,7 @@ export function findLocalIdsInStatementByName(libraryElm: ELM, statementName: st for (const alias of Array.from(emptyResultClauses)) { // Only do it if we have a clause for where the result should be fetched from // and have a localId for the clause that the result should map to - if (localIds[alias.expressionLocalId] != null && alias.aliasLocalId != null) { + if (localIds[alias.expressionLocalId] != null && alias.aliasLocalId) { localIds[alias.aliasLocalId] = { localId: alias.aliasLocalId, sourceLocalId: alias.expressionLocalId @@ -132,11 +132,16 @@ export function findAllLocalIdsInStatement( aliasMap[v] = statement.expression.localId; // Determine the localId for this alias. if (statement.localId) { - // Older translator versions require with statements to use the statement.expression.localId + 1 as the alias Id - // even if the statement already has a localId. There is not a clear mapping for alias with statements in the new - // translator, so they will go un highlighted but this will not affect coverage calculation + // There is not a clear mapping for `With` and `Without` relationship aliases with statements in newer + // translator versions. The node in the annotation that contains the alias has a local id that doesn't + // exist in the elm. We have to find this localId by looking for it in the annotation structure. if (statement.type === 'With' || statement.type === 'Without') { - alId = (parseInt(statement.expression.localId, 10) + 1).toString(); + if (annotation) { + const id = findRelationshipAliasAnnotationId(annotation, statement.expression.localId); + if (id) { + alId = id; + } + } } else { alId = statement.localId; } @@ -347,6 +352,78 @@ export function findLocalIdsForComparisonOperators( } } +/** + * Helper function to kick off the recursive search for the relationship (With, Without) source's localId in the annotation + * structure. If found this returns the localId of the parent node of the given source localId. Null is returned if not + * found. + */ +function findRelationshipAliasAnnotationId(annotation: Annotation[], sourceLocalId: string): string | null { + for (const a of annotation) { + const id = findRelationshipAliasNodeAnnotationId([a.s], sourceLocalId); + if (id) { + return id; + } + } + return null; +} + +/** + * Recursively looks through the annotation structure for the source localId and grabs its parent id which also contains + * the alias. This search is valid to be used with relationship (With, Without) clauses and allows us to tag the found + * localId, which only exists in the annotation, as the alias localId for the source so it can be highlighted when the + * source clause has a result. + * + * For example in the below snippet. We are looking for the source localId 355 and want to return 301 which includes + * the alias. + * + * { + * "r": "301", + * "s": [ + * { + * "r": "355", + * "s": [ + * { + * "s": [ + * { + * "value": [ + * "\"Bladder Cancer Diagnosis\"" + * ] + * } + * ] + * } + * ] + * }, + * { + * "value": [ + * " ", + * "BladderCancer" + * ] + * } + * ] + * } + * + * @returns id of the node that is parent to the source localId if found + */ +function findRelationshipAliasNodeAnnotationId( + annotation: AnnotationStatement[], + sourceLocalId: string +): string | null { + for (const node of annotation) { + // if this node has a list of more nodes in s, look at the first one and see if it matches the sourceLocalId + if (node.r && node.s && node.s[0]?.r === sourceLocalId) { + // return this localId which is the parent of the sourceLocalId + return node.r; + } else if (node.s) { + // otherwise, keep recursing and return the alias localId if found + const id = findRelationshipAliasNodeAnnotationId(node.s, sourceLocalId); + if (id) { + return id; + } + } + } + return null; +} + /** * Find the localId of the library reference in the JSON elm annotation. This recursively searches the annotation structure * for the clause of the library ref. When that is found it knows where to look inside of that for where the library diff --git a/test/unit/HTMLBuilder.test.ts b/test/unit/HTMLBuilder.test.ts index 5d933a45..8d98cfd1 100644 --- a/test/unit/HTMLBuilder.test.ts +++ b/test/unit/HTMLBuilder.test.ts @@ -20,11 +20,13 @@ import { getELMFixture, getHTMLFixture, getJSONFixture } from './helpers/testHel describe('HTMLBuilder', () => { let elm = {}; - let simpleExpression: ELMStatement | undefined; + let denominatorExpression: ELMStatement | undefined; + let numeratorExpression: ELMStatement | undefined; let statementResults: StatementResult[]; let trueClauseResults: ClauseResult[]; let falseClauseResults: ClauseResult[]; - const desiredLocalId = '119'; + const denominatorLocalId = '119'; + const numeratorLocalId = '135'; const trueStyleString = objToCSS(cqlLogicClauseTrueStyle); const falseStyleString = objToCSS(cqlLogicClauseFalseStyle); const coverageStyleString = objToCSS(cqlLogicClauseCoveredStyle); @@ -114,54 +116,105 @@ describe('HTMLBuilder', () => { beforeEach(() => { elm = getELMFixture('elm/CMS723v0.json'); - simpleExpression = elm.library.statements.def.find(d => d.localId === desiredLocalId); // Simple expression for Denominator + denominatorExpression = elm.library.statements.def.find(d => d.localId === denominatorLocalId); // Simple expression for Denominator + numeratorExpression = elm.library.statements.def.find(d => d.localId === numeratorLocalId); // Simple expression for Denominator + // statementResults = [ { - statementName: simpleExpression?.name ?? '', + statementName: denominatorExpression?.name ?? '', libraryName: elm.library.identifier.id, final: FinalResult.TRUE, relevance: Relevance.TRUE, - localId: desiredLocalId + localId: denominatorLocalId + }, + { + statementName: numeratorExpression?.name ?? '', + libraryName: elm.library.identifier.id, + final: FinalResult.UNHIT, + relevance: Relevance.FALSE, + localId: numeratorLocalId } ]; trueClauseResults = [ { - statementName: simpleExpression?.name ?? '', + statementName: denominatorExpression?.name ?? '', libraryName: elm.library.identifier.id, - localId: desiredLocalId, + localId: denominatorLocalId, final: FinalResult.TRUE, raw: true + }, + { + statementName: denominatorExpression?.name ?? '', + libraryName: elm.library.identifier.id, + localId: '118', + final: FinalResult.TRUE, + raw: [{ resourceType: 'foo' }] + }, + { + statementName: denominatorExpression?.name ?? '', + libraryName: elm.library.identifier.id, + localId: '116', + final: FinalResult.TRUE, + raw: [{ resourceType: 'foo' }] + }, + { + statementName: denominatorExpression?.name ?? '', + libraryName: elm.library.identifier.id, + localId: '115', + final: FinalResult.TRUE, + raw: [{ resourceType: 'foo' }] } ]; falseClauseResults = [ { - statementName: simpleExpression?.name ?? '', + statementName: denominatorExpression?.name ?? '', libraryName: elm.library.identifier.id, - localId: desiredLocalId, + localId: denominatorLocalId, final: FinalResult.FALSE, raw: false + }, + { + statementName: denominatorExpression?.name ?? '', + libraryName: elm.library.identifier.id, + localId: '117', + final: FinalResult.FALSE, + raw: [] + }, + // specifically not including this result to make this clause have no coverage styling. + // This simulates a clause that only exists in the annotation. + // { + // statementName: simpleExpression?.name ?? '', + // libraryName: elm.library.identifier.id, + // localId: '101', + // final: FinalResult.FALSE, + // raw: [] + // } + { + statementName: numeratorExpression?.name ?? '', + libraryName: elm.library.identifier.id, + localId: numeratorLocalId, + final: FinalResult.UNHIT, + raw: false } ]; }); - test('simple HTML with generation with true clause', () => { + test('simple HTML with generation with mix of false and true clauses', () => { // Ignore tabs and new lines - const expectedHTML = getHTMLFixture('simpleTrueAnnotation.html').replace(/\s/g, ''); - const res = generateHTML(simpleMeasure, [elm], statementResults, trueClauseResults, 'test'); + const expectedHTML = getHTMLFixture('simpleHighlightingAnnotation.html').replace(/\s/g, ''); + const res = generateHTML( + simpleMeasure, + [elm], + statementResults, + [...trueClauseResults, ...falseClauseResults], + 'test' + ); expect(res.replace(/\s/g, '')).toEqual(expectedHTML); expect(res.includes(trueStyleString)).toBeTruthy(); - }); - - test('simple HTML with generation with false clause', () => { - // Ignore tabs and new lines - const expectedHTML = getHTMLFixture('simpleFalseAnnotation.html').replace(/\s/g, ''); - const res = generateHTML(simpleMeasure, [elm], statementResults, falseClauseResults, 'test'); - - expect(res.replace(/\s/g, '')).toEqual(expectedHTML); expect(res.includes(falseStyleString)).toBeTruthy(); }); @@ -213,7 +266,7 @@ describe('HTMLBuilder', () => { detailedResults: [ { statementResults: statementResults, - clauseResults: [trueClauseResults[0], falseClauseResults[0]], + clauseResults: [...trueClauseResults, ...falseClauseResults], groupId: 'test' } ] @@ -235,12 +288,12 @@ describe('HTMLBuilder', () => { detailedResults: [ { statementResults: statementResults, - clauseResults: [trueClauseResults[0], falseClauseResults[0]], + clauseResults: [...trueClauseResults, ...falseClauseResults], groupId: 'test' }, { statementResults: statementResults, - clauseResults: [trueClauseResults[0], falseClauseResults[0]], + clauseResults: [...trueClauseResults, ...falseClauseResults], groupId: 'test2' } ] diff --git a/test/unit/fixtures/html/simpleCoverageAnnotation.html b/test/unit/fixtures/html/simpleCoverageAnnotation.html index 8b46bd65..855ae6c1 100644 --- a/test/unit/fixtures/html/simpleCoverageAnnotation.html +++ b/test/unit/fixtures/html/simpleCoverageAnnotation.html @@ -1,5 +1,5 @@
-

test Clause Coverage: 100%

+

test Clause Coverage: 66.7%

test Clause Coverage: 100%
       
         
           define "Denominator": 
-          
-            
-              
+          
+            
+              
                 "Encounter with Atrial Ablation Procedure"
               
               union
-              
+              
                 "History of Atrial FibrillationFlutter"
               
             
@@ -26,4 +26,11 @@ 
test Clause Coverage: 100%
+ 
+    
+    define "Numerator": "Denominator" NonElectiveEncounter
+        with "Anticoagulant Therapy at Discharge" Anticoagulant
+            such that Anticoagulant.authorDatetime during NonElectiveEncounter.relevantPeriod
+
diff --git a/test/unit/fixtures/html/simpleCoverageAnnotation2.html b/test/unit/fixtures/html/simpleCoverageAnnotation2.html index 7d4f528c..c76fb7bb 100644 --- a/test/unit/fixtures/html/simpleCoverageAnnotation2.html +++ b/test/unit/fixtures/html/simpleCoverageAnnotation2.html @@ -1,29 +1,36 @@
-

test2 Clause Coverage: 100%

+

test2 Clause Coverage: 66.7%

-        
-          
-            define "Denominator": 
-            
-              
-                
-                  "Encounter with Atrial Ablation Procedure"
-                
-                union
-                
-                  "History of Atrial FibrillationFlutter"
-                
+      
+        
+          define "Denominator": 
+          
+            
+              
+                "Encounter with Atrial Ablation Procedure"
               
               union
-              
-                "Current Diagnosis Atrial FibrillationFlutter"
+              
+                "History of Atrial FibrillationFlutter"
               
             
+            union
+            
+              "Current Diagnosis Atrial FibrillationFlutter"
+            
           
-        
-
+ + + + 
+    
+    define "Numerator": "Denominator" NonElectiveEncounter
+        with "Anticoagulant Therapy at Discharge" Anticoagulant
+            such that Anticoagulant.authorDatetime during NonElectiveEncounter.relevantPeriod
+
diff --git a/test/unit/fixtures/html/simpleFalseAnnotation.html b/test/unit/fixtures/html/simpleFalseAnnotation.html deleted file mode 100644 index cf1d4bb5..00000000 --- a/test/unit/fixtures/html/simpleFalseAnnotation.html +++ /dev/null @@ -1,29 +0,0 @@ -
-

Population Group: test

- 
-    
-      
-        define "Denominator": 
-        
-          
-            
-              "Encounter with Atrial Ablation Procedure"
-            
-            union
-            
-              "History of Atrial FibrillationFlutter"
-            
-          
-          union
-          
-            "Current Diagnosis Atrial FibrillationFlutter"
-          
-        
-      
-    
-
-
diff --git a/test/unit/fixtures/html/simpleHighlightingAnnotation.html b/test/unit/fixtures/html/simpleHighlightingAnnotation.html new file mode 100644 index 00000000..034e8806 --- /dev/null +++ b/test/unit/fixtures/html/simpleHighlightingAnnotation.html @@ -0,0 +1,37 @@ +
+

Population Group: test

+ 
+    
+      
+        define "Denominator": 
+        
+          
+            
+              "Encounter with Atrial Ablation Procedure"
+            
+            union
+            
+              "History of Atrial FibrillationFlutter"
+            
+          
+          union
+          
+            "Current Diagnosis Atrial FibrillationFlutter"
+          
+        
+      
+    
+
+ + 
+  
+  define "Numerator": "Denominator" NonElectiveEncounter
+      with "Anticoagulant Therapy at Discharge" Anticoagulant
+          such that Anticoagulant.authorDatetime during NonElectiveEncounter.relevantPeriod
+
+
diff --git a/test/unit/fixtures/html/simpleTrueAnnotation.html b/test/unit/fixtures/html/simpleTrueAnnotation.html deleted file mode 100644 index f1b616eb..00000000 --- a/test/unit/fixtures/html/simpleTrueAnnotation.html +++ /dev/null @@ -1,29 +0,0 @@ -
-

Population Group: test

- 
-    
-      
-        define "Denominator": 
-        
-          
-            
-              "Encounter with Atrial Ablation Procedure"
-            
-            union
-            
-              "History of Atrial FibrillationFlutter"
-            
-          
-          union
-          
-            "Current Diagnosis Atrial FibrillationFlutter"
-          
-        
-      
-    
-
-