HANS-298: HL7v2 Improvements #6
Conversation
There was a problem hiding this comment.
Three main things:
- We need to document how to run the tests locally.
- Timezones will bite us on the ankle if we let them. I've had far too many experiences where tests pass for a team in one country but fail for those in another to let there be ambiguity there.
- Getting the NHS number out of the HL7 message feels really fragile to me.
Wider point: it feels like we want an abstraction between hl7.parse and the HL7MessageController. I'd much rather the controller was calling self.hl7_message.family_name() than self._extract_field("PID", 0, 5, 0, 0). Feels like having the knowledge about how to pull the hl7 message apart separated from translating it into FHIR will make things easier to test and understand.
| ): | ||
| """https://hl7-definition.caristix.com/v2/HL7v2.8/Segments/MSH""" | ||
|
|
||
| timestamp = datetime.now().strftime("%Y%m%d%H%M%S") |
There was a problem hiding this comment.
I don't think this format is right.. We're not specifying a timezone, which according to the spec means it defaults to being interpreted by anything that speaks HL7v2 as the local timezone. That means if we pass the output of this method to to_fhir_datetime below, which formats everything as UTC, we're actually an hour wrong for half the year. If we specify +0000 on the tail of the format string I think that removes any potential ambiguity.
| + ":" | ||
| + hl7_DTM[12:14] | ||
| + "Z" | ||
| return datetime.strptime(hl7_datetime, "%Y%m%d%H%M%S").strftime( |
There was a problem hiding this comment.
See above. I'm pretty sure we do need to care about the timezone here.
| name | ||
| for name in ( | ||
| self._extract_field("PID", 0, 5, 0, 1), | ||
| self._extract_field("PID", 0, 5, 0, 2), |
There was a problem hiding this comment.
Possibly stupid question, but do we know we're only getting two given names?
There was a problem hiding this comment.
Ah that's a good spot - we need to space separate the result of _extract_field("PID", 0, 5, 0, 2) (if it exists)
These could be space separated according to the spec on Caristix
| raise MissingNHSNumberError | ||
|
|
||
| try: | ||
| return next( |
There was a problem hiding this comment.
This feels fragile. We're identifying whether NHSNMBR appears anywhere in the pid_segment then scanning for any 10-digit sequence that might be anywhere else in the pid_segment - there's no reason for the two to be coming from the same place. In our example below if the SIMULATOR MRN value happened to pass the NHS number checksum calculation, that's what we'd get returned.
There was a problem hiding this comment.
Yep this is definitely not what we want - @michal-kras can you look at the current get_nhs_number() method and extract the logic from there?
|
Some additional functionality that needs fixing from current version to do with which fields to complain about when missing might be worth dealing with now too (as appropriate) and adding in regression tests: |
I think this is really good idea, I am jumping right on it. |
No description provided.