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

Coding display validation should consider all possible designations #3712

Closed
lmsurpre opened this issue Jun 15, 2022 · 5 comments
Closed

Coding display validation should consider all possible designations #3712

lmsurpre opened this issue Jun 15, 2022 · 5 comments
Assignees
Labels
bug Something isn't working enhancement New feature or request P2 Priority 2 - Should Have waiting-for-spec-clarification

Comments

@lmsurpre
Copy link
Member

Is your feature request related to a problem? Please describe.
From https://hl7.org/fhir/datatypes.html#codesystem

If the 'display' element is populated, the string used in display SHALL be one of the display strings defined for that code by the code system (code systems may define multiple display strings for a single code). If one of the available display strings is labeled as preferred, it SHOULD be used.

However, CodeSystem.concept.display is 0..1. Does it means you can use any of the "designations" for the Coding.display?
Currently, our implementation only checks against CodeSystem.concept.display and not the corresponding designations.

This is in FHIRTermService.validateDisplay.

Describe the solution you'd like
Instead of checking against just the CodeSystem.concept.display, check against any existing designation values as well.

Describe alternatives you've considered

Acceptance Criteria

  1. GIVEN a CodeSystem with a concept that has one or more designations
    AND a resource instance that has a Coding from that concept in this CodeSystem
    WHEN you validate that instance
    THEN it passes validation

Additional context
I opened https://jira.hl7.org/browse/FHIR-37603 to get some clarification from the spec authors.

@lmsurpre lmsurpre added enhancement New feature or request waiting-for-spec-clarification P2 Priority 2 - Should Have bug Something isn't working labels Jun 15, 2022
@lmsurpre
Copy link
Member Author

Performance question: will doing the designationValues.contains dramatically increase the time to validate large instances?

@lmsurpre lmsurpre self-assigned this Jun 20, 2022
lmsurpre added a commit that referenced this issue Jul 11, 2022
1. Split `validateDisplay` into two flavors; now
validateCode(CodeSystem, Coding, ValidationParameters) will use the
passed CodeSystem to determine case sensitivity (instead of looking it
up via the registry)

2. When the Coding.display doesn't match the lookup display, we now look
for a match within the CodesSystem.concept.designation values before
calling it invalid.

3. Added a designation to a couple sample code systems and added
testValidateDisplay tests for both case-sensitive and case-insensitive
behavior.

Signed-off-by: Lee Surprenant <lmsurpre@merative.com>
lmsurpre added a commit that referenced this issue Jul 13, 2022
* issue #3712 - check all CodeSystem concept designations

1. Split `validateDisplay` into two flavors; now
validateCode(CodeSystem, Coding, ValidationParameters) will use the
passed CodeSystem to determine case sensitivity (instead of looking it
up via the registry)

2. When the Coding.display doesn't match the lookup display, we now look
for a match within the CodesSystem.concept.designation values before
calling it invalid.

3. Added a designation to a couple sample code systems and added
testValidateDisplay tests for both case-sensitive and case-insensitive
behavior.

Signed-off-by: Lee Surprenant <lmsurpre@merative.com>

* Apply suggestions from code review

Signed-off-by: Lee Surprenant <lmsurpre@merative.com>
@lmsurpre
Copy link
Member Author

lmsurpre commented Aug 4, 2022

QA:

  1. find or create a CodeSystem that has a concept which has one or more "designations" (that don't match that concept's "display")
  2. find or create a StructureDefinition (base resourceType or profile) that has a binding to a valueset which includes that concept
  3. create an instance of that resourceType/profile and use the value from the designation
codeableConcept: {
  coding: [
{
  system: ""
  code: ""
  display: "THIS MUST MATCH THE DESIGNATION"
}
]
...

Before this change, the server would say thats invalid. Now that should succeed.

@PrasannaHegde1
Copy link
Collaborator

PrasannaHegde1 commented Aug 30, 2022

The Patient resource type has a binding with CodeSystem for "Medical Record Number".
"Medical Record Number" has display value "Medical Record Number" and designation value "Krankenaktennummer"
Tried to create an instance of Patient resource type with below request :

{
    "resourceType" : "Patient",
    "active" : true,
    "identifier": [		
       {
			"type": {
				"coding": [
					{
						"system": "http://terminology.hl7.org/CodeSystem/v2-0203",
						"code": "MR",
						**"display": "Krankenaktennummer"**
					}
				],
				"text": "Medical Record Number"
			},
			"system": "http://hospital.smarthealthit.org",
			"value": "6db064db-2005-6b05-aeb2-b72a36df1a6f"
		}
	],
    "name" : [ {
        "family" : "Malan",
        "given" : [ "David" ]
    } ],
    "gender" : "male"
}

and the FHIRTermService.validateDisplay returned below outcome

[ display={
        "string": "Medical record number"
    }, message=null, result={
        "boolean": true
    }
]

Tried to create an instance of Patient with display value other than the CodeSystem display and designation values for "Medical Record Number"
Request:

{
    "resourceType" : "Patient",
    "active" : true,
    "identifier": [		
       {
			"type": {
				"coding": [
					{
						"system": "http://terminology.hl7.org/CodeSystem/v2-0203",
						"code": "MR",
						**"display": "MR123"**
					}
				],
				"text": "Medical Record Number"
			},
			"system": "http://hospital.smarthealthit.org",
			"value": "6db064db-2005-6b05-aeb2-b72a36df1a6f"
		}
	],
    "name" : [ {
        "family" : "Malan5",
        "given" : [ "David5" ]
    } ],
    "gender" : "male"
}

and the FHIRTermService.validateDisplay returned below outcome

ValidationOutcome [display={
    "string": "Medical record number"
}, message={
    "string": "The display 'MR123' is incorrect for code 'MR' from code system 'http://terminology.hl7.org/CodeSystem/v2-0203'"
}, result={
    "boolean": false
}]

@lmsurpre
Copy link
Member Author

Prasanna found that even in the latter case (The display 'MR123' is incorrect for code 'MR') the severity of the issue is "information" whereas I would have expected it to be "error".

The intention was for it to be informational only when we couldn't validate the code.

@lmsurpre
Copy link
Member Author

lmsurpre commented Sep 1, 2022

I dug deeper into the above and I don't think its a regression.
This particular message is always informational.
But in addition, it results in the corresponding memberOf(valueSet, bindingStrength) constraint to return false.
When the binding strength is "Required" that results in us rejecting the transaction.
But when the binding strength is "Extensible" (or "Preferred") we don't let this constraint failure prevent us from accepting the data.

QUESTION: should we fail the transaction if a coding uses an invalid display value irregardless of the the binding strength?!

It would be awful strict to reject something just because the display value is wrong when we would have accepted it if the code were missing altogether.

Further, we only do this display value validation for concepts that exists in a bound valueset that has binding strength of preferred or higher.
That means we would only catch cases where the display value is invalid when it exists in a bound valueset AND we have a corresponding "complete" CodeSystem to complete the validation.

I think that could appear inconsistent/confusing to our users if we added the strict display code checking for just this one type of codes. But with that said, we do have an older / outstanding issue for adding Concept validation more generally (i.e. not just for codeable elements bound to a valueset). Therefor, I'll add the above commentary to that one (#1217) and close this one out.

@lmsurpre lmsurpre closed this as completed Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request P2 Priority 2 - Should Have waiting-for-spec-clarification
Projects
None yet
Development

No branches or pull requests

2 participants