CDCgov / prime-reportstream

ReportStream is a public intermediary tool for delivery of data between different parts of the healthcare ecosystem.
https://reportstream.cdc.gov
Creative Commons Zero v1.0 Universal
72 stars 40 forks source link

Inconsistent warning messages when posting reports #1066

Closed tlosbo closed 3 years ago

tlosbo commented 3 years ago

Describe the bug When posting a report that has unexpected column headers or invalid/unexpected data, the messages provided in the Json response use internal field names that are confusing to the client posting the report. Example #1

"warnings" : [ {
    "scope" : "ITEM",
    "id" : "737324",
    "details" : "Invalid code: 'sdfsdfdsf' is not a display value in altValues set for 'patient_ethnicity'"
  }, {
    "scope" : "ITEM",
    "id" : "737324",
    "details" : "Invalid date: 'baasasdfs' for element 'patient_dob'"
  } ]

The posted CSV report has ‘patientEthnicity’ and ‘patientDOB_pii’ respectively for those two fields.

Example #2

"warnings" : [ {
    "scope" : "ITEM",
    "id" : "testId24ODSBLs_0",
    "details" : "Invalid Code: 'patientRacevVn7JRow_0' does not match any codes for 'patient_race'"
  }, {
    "scope" : "ITEM",
    "id" : "testId24ODSBLs_0",
    "details" : "Invalid code: 'patientEthnicityMWvxRhOg_0' is not a display value in altValues set for 'patient_ethnicity'"
  }, {
    "scope" : "ITEM",
    "id" : "testId24ODSBLs_0",
    "details" : "Invalid Code: 'patientSexaQcW2VWn_0' does not match any codes for 'patient_gender'"
  }, {
    "scope" : "ITEM",
    "id" : "testId24ODSBLs_0",
    "details" : "Invalid postal code 'PatZippmW74WJ0_0'"
  }, {
    "scope" : "ITEM",
    "id" : "testId24ODSBLs_0",
    "details" : "Invalid date: 'patientDob_piiQxxfVWxj_0' for element 'patient_dob'"
  }, {
    "scope" : "ITEM",
    "id" : "testId24ODSBLs_0",
    "details" : "Invalid phone number 'patientPhone_piimc8kiy90_0' for 'patient_phone_number'"
  }]

These results are from using the attached CSV report. All warnings are using internal field names. The message should include the report field name and internal mapped field name:

{
    "scope" : "ITEM",
    "id" : "testId24ODSBLs_0",
    "details" : "Invalid phone number 'patientPhone_piimc8kiy90_0' for 'patientPhone_pii' mapped to  'patient_phone_number'"
}

Impact Internal field names are confusing to client posting the report and makes it more difficult for them to determine what data element is causing the problem.

To Reproduce Post a report with bad column headers and invalid data and observe the response warning/error messages. Bad data file attached. gh-reportstream-crafted-from-headers.csv

Expected behavior The warning/error messages should make sense to the posting client with headers/field names they are using in their report.

jimduff-usds commented 3 years ago

This:

{
    "scope" : "REPORT",
    "id" : "",
    "details" : "Missing 'patientAge' header"
  }

has the opposite problem: it shows the customer-facing header name, but not the internal report stream name. It should include both patientAge and patient_age

jimduff-usds commented 3 years ago

Here's another one I just noticed:

{
    "scope" : "ITEM",
    "id" : "2021051121-dsr^2021051121-12cf19c87761487e9a338416fb99e408",
    "details" : "Invalid code: 'U' is not a display value in altValues set for 'patient_ethnicity'"
  }, 

It should list both the internal patient_ethnicity, and also the customer facing name - in the case of Waters its 'patientEthnicity'.

jimduff-usds commented 3 years ago

@tlosbo, note that we allow one internal name to map to multiple customer facing external csvField names.

Although we do this extremely rarely, you will need to handle this case.

tlosbo commented 3 years ago

@jimduff-usds it looks like there are lots of custom messages in the Element class that use $name which referenced the internal field name. Debating on creating a generic string to use in all of the current messages and drop that in place of $name. For example:

  {
    "scope" : "ITEM",
    "id" : "testId24ODSBLs_0",
    "details" : "Invalid Code: 'patientSexaQcW2VWn_0' does not match any codes for 'patient_gender'"
  }, {
    "scope" : "ITEM",
    "id" : "testId24ODSBLs_0",
    "details" : "Invalid postal code 'PatZippmW74WJ0_0'"
  }, {
    "scope" : "ITEM",
    "id" : "testId24ODSBLs_0",
    "details" : "Invalid date: 'patientDob_piiQxxfVWxj_0' for element 'patient_dob'"
  }, {
    "scope" : "ITEM",
    "id" : "testId24ODSBLs_0",
    "details" : "Invalid phone number 'patientPhone_piimc8kiy90_0' for 'patient_phone_number'"
  }

would change to

  {
    "scope" : "ITEM",
    "id" : "testId24ODSBLs_0",
    "details" : "Invalid Code: 'patientSexaQcW2VWn_0' does not match any codes for 'patient_gender' mapped from 'patientSex'"
  }, {
    "scope" : "ITEM",
    "id" : "testId24ODSBLs_0",
    "details" : "Invalid postal code 'PatZippmW74WJ0_0' for element 'patient_zip_code' mapped from 'PatZip'"
  }, {
    "scope" : "ITEM",
    "id" : "testId24ODSBLs_0",
    "details" : "Invalid date: 'patientDob_piiQxxfVWxj_0' for element 'patient_dob' mapped from 'patientDob_pii'"
  }, {
    "scope" : "ITEM",
    "id" : "testId24ODSBLs_0",
    "details" : "Invalid phone number 'patientPhone_piimc8kiy90_0' for 'patient_phone_number' mapped from 'patientPhone_pii'"
  }

Some of the messages don't mention the field (i.e. the Zip Code check)

jimduff-usds commented 3 years ago

I definitely like your idea of a printableName that can just be used everywhere.

Can we put the customer-facing name first? I'm not sure the "mapped from" will be meaningful to a non-programmer. I want to call it the "ReportStream Internal Name but that's ridiculously verbose. Uhhh...

...for element 'PatZip' (In ReportStream: 'patient_zip_code'). ...for element 'PatZip' (aka 'patient_zip_code'). ...for element 'PatZip' (tracked as: 'patient_zip_code'). ...for element 'PatZip' (RS: 'patient_zip_code').

IMO, all of those are worse than simply ....for element 'PatZip' ('patient_zip_code').

tlosbo commented 3 years ago

I have added a property to the Element class named fieldMapping: fieldMapping = "'${this.csvFields?.joinToString { it -> it.name }}' ('${this.name}')"

Still working on updating all the error message checks but here's an example of the warnings:

  "warnings" : [ {
    "scope" : "ITEM",
    "id" : "testId24ODSBLs_0",
    "details" : "Invalid Code: 'patientRacevVn7JRow_0' does not match any codes for 'patientRace' ('patient_race')"
  }, {
    "scope" : "ITEM",
    "id" : "testId24ODSBLs_0",
    "details" : "Invalid code: 'patientEthnicityMWvxRhOg_0' is not a display value in altValues set for 'patientEthnicity' ('patient_ethnicity')"
  }, {
    "scope" : "ITEM",
    "id" : "testId24ODSBLs_0",
    "details" : "Invalid Code: 'patientSexaQcW2VWn_0' does not match any codes for 'patientSex' ('patient_gender')"
  }, {
    "scope" : "ITEM",
    "id" : "testId24ODSBLs_0",
    "details" : "Invalid postal code 'PatZippmW74WJ0_0' for 'PatZip' ('patient_zip_code')"
  }, {
    "scope" : "ITEM",
    "id" : "testId24ODSBLs_0",
    "details" : "Invalid date: 'patientDob_piiQxxfVWxj_0' for element 'patientDob_pii' ('patient_dob')"
  }, {
    "scope" : "ITEM",
    "id" : "testId24ODSBLs_0",
    "details" : "Invalid phone number 'patientPhone_piimc8kiy90_0' for 'patientPhone_pii' ('patient_phone_number')"
  } ]

Wanted to show the example and get some feedback while I work on the remaining messages in Element. The headers are in the Serializer class and I'll update those as well.

jimduff-usds commented 3 years ago

@tlosbo - this is done?

tlosbo commented 3 years ago

Merged with PR #1080, closing.