Add CodeableReference keyword #1292

jafeltra · 2023-06-15T13:35:08Z

This PR adds a CodeableReference keyword, which can be used in Type Rules to constrain a type to a CodeableReference or a specific set of CodeableReference resource/profiles and Add Element Rules to add a CodeableReference element.

As of right now, the CodeableReference keyword cannot be used in AssignmentRules. To assign a portion of a CodeableReference element, you can assign the concept and/or reference portion directly, as you do now in the current version of SUSHI. If it turns out we do want to add AssignmentRule use for the CodeableReference PR, I'll either close this PR or mark it as draft while making the updates.

mint-thompson

This looks really good, and should definitely help with using CodeableReference types. I have a few small suggestions.

mint-thompson · 2023-06-15T16:06:41Z

src/fhirtypes/ElementDefinition.ts

+    if (
+      targetTypes.some(t => t.code === 'CodeableReference') &&
+      !targetTypes.some(t => t.code === 'Reference') &&
+      rule.types.every(t => t.isReference)
+    ) {


I think this condition may need to be slightly adjusted. It should only be necessary for one of the rule's types to be Reference for this warning to be useful. Here is an example of what I mean:

Extension: MyExt * value[x] only boolean or CodeableReference * value[x] only boolean or Reference(Patient)

The first OnlyRule removes the Reference type, so the second OnlyRule constrains the target profile for CodeableReference. But, in this case, no warning is logged.

Ah, that's a good point, and a case I hadn't thought about. If I just change the condition here, the warning message actually constructs the wrong example because it tries to create CodeableReference(boolean or Patient), which is wrong. So I think I'll also just remove the example, especially since the line number is included as well.

I updated the condition and the warning message and included a test in 20614c4.

mint-thompson · 2023-06-15T16:10:08Z

src/fhirtypes/ElementDefinition.ts

@@ -1171,7 +1199,7 @@ export class ElementDefinition {
        // one of the targetProfiles.  If the targetProfile property is null, that means any
        // reference is allowed.
        // When 'Reference' keyword is used, prefer to match on the 'Reference' type over the
-        // 'CodeableReference' type if they both exist on the element.
+        // 'CodeableReference' type if they both exist on the element, but issue a warning.


Is this referring to the warning in constrainType, or is there supposed to be a new warning here?

opps, yes I moved the warning and forgot about the comment!

Removed in 20614c4

mint-thompson · 2023-06-15T16:21:19Z

test/fhirtypes/ElementDefinition.constrainType.test.ts

It could be useful to add some tests that constrain an element to a combination of Reference/CodeableReference and non-reference types.

Added in 20614c4

mint-thompson · 2023-06-15T16:22:56Z

test/import/FSHImporter.SDRules.test.ts

@@ -1319,6 +1319,58 @@ describe('FSHImporter', () => {
        );
      });

+      it('should parse an only rule with a CodeableReference to one type', () => {


Are these new tests identical to the new tests in FSHImporter.Profile.test.ts?

So.. I think.. maybe? I thought they seemed to be covering the same cases, but I was going off of the existing tests, and there seem to be a good deal of overlap in those sets of tests. So I sort of just figured there was a reason we had them in each place. But maybe, if you agree they seem to cover exactly the same cases, this is the time to stop duplicating tests and I can remove either set.

If you can confirm that these are exactly the same, I'm fine with removing them. We may have overlapping tests due to previous refactoring... Or in a few cases, I think we have a token test cases in broad test files and then many detailed ones in more specific test files. Maybe that is what's going on here? Either way, duplicate tests aren't really helpful so I'm fine w/ them going away.

That said, if there are lots of duplicate tests in this file in general, then it may be worth making a separate task to clean the whole thing up.

I double checked, and as far as I can tell, they are exactly the same. Neither set of tests sets up anything different in describe and/or beforeAll/beforeEach blocks.

There do seem to be large amounts of duplicates between these two files. The FSHImporter.SDRules.tests.ts file does mention at the top that it is designed to cover the overlap between the four different StructureDefinition entities (profiles, extensions, resources, and logicals). I looked at the OnlyRule section, where the tests I added were, as well as a few other sections. There is a large amount of overlap between the tests, but unfortunately there are a few tests that were newly added in both files. For example, the CaretValueRule section has 4 tests shared between the two files, 1 new test added in the FSHImporter.Profile file, and 1 new test added in the FSHImporter.SDRules file.

So that is... unfortunate. It seems like the files might have been refactored when Logicals and Resources were added, so I wonder if something got messed up in the rebasing/merging back then. Who knows.

All that said, I think what makes sense is to add the three new tests I included here into the FSHImporter.SDRules.test.ts file and remove them from the FSHImporter.Profile.test.ts file since they apply to any StructureDefinition and that will reduce clutter for now. And then it probably is a good idea to clean up these files so there are fewer shared tests between them.

I checked the InsertRule block and there's a whole bunch of tests in the FSHImporter.Profile.test.ts that aren't in the FSHImporter.SDRules.test.ts (~12 tests). So I actually don't know for sure which file we should be trying to maintain, but I think the FSHImporter.SDRules.test.ts makes more sense as a centralized place to hold the overlapping tests. So I'll still plan to move my tests to be in FSHImporter.SDRules.test.ts, but I can be convinced otherwise if either of you disagree on the file to hold them.

Removed the duplicate tests in 7d6c098, but I can swap the location once you have a chance to process the novels I wrote above.

I don't know. On one hand, I understand the approach of having tests for logic that applies to any SD being in FSHImporter.SDRules.test.ts. On the other hand, when you write the test, you do have to choose a specific kind of SD, right? But when we're talking about importing rules, I guess it shouldn't matter if it is a profile, logical, etc... So... ???

I think one of the reasons we split into FSHImporter.Profile.test.ts and others was to keep FSHImporter.SDRules.test.ts from becoming massive. But at the same time, if we put a rule import test only in FSHImporter.Profile.test.ts, then it's not easy to know where to look for it and people might thing that type of rule is not tested at all.

All this to say, I think you can justify either way. But I guess maybe if I had to lean, I'd lean toward FSHImporter.SDRules.test.ts? Maybe we should refactor into a separate file for every rule type instead. That actually might make more sense, but is way outside the scope of this PR.

On the other hand, when you write the test, you do have to choose a specific kind of SD, right?

Yes, you do. It seems like the majority of the tests in FSHImporter.SDRules.test.ts are testing Profile, with a handful for Resource when testing Resource/Logical Model specific rules.

I was looking at the file history trying to figure out which file came first, and it seemed like SDRules came second, which is why I thought maybe it was trying to ease the number of tests in the Profile file, but that was really just an inference.

But when we're talking about importing rules, I guess it shouldn't matter if it is a profile, logical, etc... So... ???

Also, when looking at the Importer code, we do specifically have a visitSdRule function that is called when importing all four types of StructureDefintions. So maybe to align with the code, the tests in FSHImporter.SDRules.test.ts should align with the functionality in visitSdRule and any extra functionality, like the rules specific to Logical Models and Resources, are included in separate files for those entities. It feels like maybe that was what we tried to do and things got messy. I'm not sure. And I guess I don't need to be sure for right now.

… multiple types

mint-thompson

Thank you for the updates! I have a suggestion about the warning message, but everything else looks good.

mint-thompson · 2023-06-16T12:30:44Z

src/fhirtypes/ElementDefinition.ts

+      rule.types.some(t => t.isReference)
+    ) {
+      logger.warn(
+        'The CodeableReference() syntax should be used to constrain references of a CodeableReference',


I think this message might read a little better if it said keyword instead of syntax:

The CodeableReference() keyword should be used to constrain references of a CodeableReference

I like it. Added in 9c780bd.

cmoesel · 2023-06-21T13:05:35Z

FYI - I've discussed with Mark Kramer and he agrees with the approach here: CodeableConcept() is suitable for constraining types but should not be used for assignments.

cmoesel

This looks good and works well. I've mainly just made some small suggestions for the tests.

cmoesel · 2023-06-21T13:27:41Z

test/fhirtypes/ElementDefinition.constrainType.test.ts

-        addresses.constrainType(onlyRule, fisher);
-      }).toThrow(/"Reference\(Patient\).*Reference\(.*Condition\)/);
+    describe('Reference() keyword', () => {
+      it('should allow a CodeableReference to multiple resource types to be constrained to a reference to a subset', () => {


I'd suggest rewording the tests that use Reference to constrain a CodeableConcept so that the test summary indicates that a warning should be logged. E.g.:

Suggested change

it('should allow a CodeableReference to multiple resource types to be constrained to a reference to a subset', () => {

it('should warn when a CodeableReference to multiple resource types is constrained to a Reference to a subset', () => {

Updated in 9f88612, along with the few other tests that needed this update.

cmoesel · 2023-06-21T13:29:56Z

test/fhirtypes/ElementDefinition.constrainType.test.ts

+        );
+      });
+
+      it('should allow a CodeableReference to multiple resource types to be constrained to a reference to a single profile', () => {


Suggested change

it('should allow a CodeableReference to multiple resource types to be constrained to a reference to a single profile', () => {

it('should allow a CodeableReference to multiple resource types to be constrained to a reference to multiple profiles', () => {

Updated in 9f88612

cmoesel · 2023-06-21T13:31:39Z

test/fhirtypes/ElementDefinition.constrainType.test.ts

+        expect(loggerSpy.getAllMessages('warn')).toHaveLength(0);
+      });
+
+      it('should allow a CodeableReference to multiple resource types to be constrained to a reference to a single profile using CodeableReference keyword', () => {


Suggested change

it('should allow a CodeableReference to multiple resource types to be constrained to a reference to a single profile using CodeableReference keyword', () => {

it('should allow a CodeableReference to multiple resource types to be constrained to a reference to multiple profiles using CodeableReference keyword', () => {

Updated in 9f88612

cmoesel · 2023-06-21T13:33:34Z

test/import/FSHImporter.Profile.test.ts

+        const input = leftAlign(`
+        Profile: ObservationProfile
+        Parent: Observation
+        * performer only CodeableReference(   Organization    or  CareTeam)


Just to be sure we're allowing white space in all the places, I'd suggest adding some before the last ) too:

Suggested change

* performer only CodeableReference( Organization or CareTeam)

* performer only CodeableReference( Organization or CareTeam )

Updated in 9f88612, but then promptly removed in favor of the tests in FSHImporter.SDRules.test.ts. But don't worry, they have the space as well!

cmoesel · 2023-06-21T13:37:19Z

test/import/FSHImporter.SDRules.test.ts

@@ -1319,6 +1319,58 @@ describe('FSHImporter', () => {
        );
      });

+      it('should parse an only rule with a CodeableReference to one type', () => {


If you can confirm that these are exactly the same, I'm fine with removing them. We may have overlapping tests due to previous refactoring... Or in a few cases, I think we have a token test cases in broad test files and then many detailed ones in more specific test files. Maybe that is what's going on here? Either way, duplicate tests aren't really helpful so I'm fine w/ them going away.

That said, if there are lots of duplicate tests in this file in general, then it may be worth making a separate task to clean the whole thing up.

…ules

cmoesel

Two thumbs up. Would recommend to a friend.

Add CodeableReference keyword for Only Rules and Add Element Rules

fe81260

jafeltra requested review from cmoesel and mint-thompson as code owners June 15, 2023 13:35

Fix InvalidTypeError

314a875

jafeltra force-pushed the codeable-reference-keyword branch from 9a84c47 to 314a875 Compare June 15, 2023 15:40

mint-thompson requested changes Jun 15, 2023

View reviewed changes

Fix condition to log warning with. Add test to ensure assignment with…

20614c4

… multiple types

mint-thompson requested changes Jun 16, 2023

View reviewed changes

Update warning message

9c780bd

jafeltra force-pushed the codeable-reference-keyword branch from 6ef48c8 to 9c780bd Compare June 20, 2023 21:10

mint-thompson previously approved these changes Jun 21, 2023

View reviewed changes

cmoesel requested changes Jun 21, 2023

View reviewed changes

Update test summaries and include more spaces in importer test

9f88612

jafeltra dismissed mint-thompson’s stale review via 9f88612 June 21, 2023 20:42

Don't duplicate tests between FSHImporter.Profile and FSHImporter.SDR…

7d6c098

…ules

cmoesel approved these changes Jun 21, 2023

View reviewed changes

Merge branch 'master' into codeable-reference-keyword

2a3bdbf

mint-thompson approved these changes Jun 22, 2023

View reviewed changes

jafeltra merged commit d58c5e7 into master Jun 22, 2023

jafeltra deleted the codeable-reference-keyword branch June 22, 2023 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CodeableReference keyword #1292

Add CodeableReference keyword #1292

jafeltra commented Jun 15, 2023

mint-thompson left a comment

mint-thompson Jun 15, 2023

jafeltra Jun 15, 2023

jafeltra Jun 15, 2023

mint-thompson Jun 15, 2023

jafeltra Jun 15, 2023

jafeltra Jun 15, 2023

mint-thompson Jun 15, 2023

jafeltra Jun 15, 2023

mint-thompson Jun 15, 2023

jafeltra Jun 15, 2023

cmoesel Jun 21, 2023

jafeltra Jun 21, 2023

jafeltra Jun 21, 2023

jafeltra Jun 21, 2023

cmoesel Jun 21, 2023

jafeltra Jun 21, 2023

mint-thompson left a comment

mint-thompson Jun 16, 2023

jafeltra Jun 20, 2023 •

edited

Loading

cmoesel commented Jun 21, 2023 •

edited

Loading

cmoesel left a comment

cmoesel Jun 21, 2023

jafeltra Jun 21, 2023

cmoesel Jun 21, 2023

jafeltra Jun 21, 2023

cmoesel Jun 21, 2023

jafeltra Jun 21, 2023

cmoesel Jun 21, 2023

jafeltra Jun 21, 2023

jafeltra Jun 21, 2023 •

edited

Loading

cmoesel Jun 21, 2023

cmoesel left a comment

	it('should allow a CodeableReference to multiple resource types to be constrained to a reference to a subset', () => {
	it('should warn when a CodeableReference to multiple resource types is constrained to a Reference to a subset', () => {

	* performer only CodeableReference( Organization or CareTeam)
	* performer only CodeableReference( Organization or CareTeam )

Add CodeableReference keyword #1292

Add CodeableReference keyword #1292

Conversation

jafeltra commented Jun 15, 2023

mint-thompson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mint-thompson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jafeltra Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

cmoesel commented Jun 21, 2023 • edited Loading

cmoesel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jafeltra Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmoesel left a comment

Choose a reason for hiding this comment

jafeltra Jun 20, 2023 •

edited

Loading

cmoesel commented Jun 21, 2023 •

edited

Loading

jafeltra Jun 21, 2023 •

edited

Loading