VED-282-284-226 Delta refactoring extractor#416
Conversation
robertnovac1
changed the title
| # Conversion engine for expression-based field transformation | ||
| class ConversionChecker: | ||
| # checker settings | ||
| summarise = False |
There was a problem hiding this comment.
I believe summarise and report_unexpected_exception can be removed now
There was a problem hiding this comment.
Kept only report_unexpected_exception, summarise has been removed as it wasn't used
delta_backend/src/delta.py
Outdated
| FHIRConverter = Converter(json.dumps(resource_json)) | ||
| flat_json = FHIRConverter.runConversion(resource_json) # Get the flat JSON | ||
| error_records = FHIRConverter.getErrorRecords() | ||
| FHIRConverter = Converter(json.dumps(resource_json), action_flag = action_flag) |
There was a problem hiding this comment.
Assuming this is going to be cleaned up, but why are we parsing, then stringifying, then parsing again?
There was a problem hiding this comment.
Yeah that looks strange, I've fixed this
delta_backend/src/fhir_parser.py
Outdated
|
|
||
| # get the value for a key | ||
| def getKeyValue(self, fieldName, flatFieldName, expression_type: str = "", expression_rule = ""): | ||
| def get_key_value(self, fieldName, flatFieldName, expression_type: str = "", expression_rule = ""): |
There was a problem hiding this comment.
I think we can remove this entire file now 🙌
There was a problem hiding this comment.
fhir_parser has been removed
delta_backend/src/schema_parser.py
Outdated
| # Moved from file loading to JSON string better for elasticache | ||
|
|
||
|
|
||
| class SchemaParser: |
There was a problem hiding this comment.
I don't think this needs to be a class. Could just do conversion_layout["conversions"] where we're currently calling get_conversions
There was a problem hiding this comment.
schema_parser has been removed
| "expression": { | ||
| "expressionName": "Look Up", | ||
| "expressionType": "LOOKUP", | ||
| "expressionType": "NORMAL", |
There was a problem hiding this comment.
Should we just use the real conversion_layout.py in the tests?
There was a problem hiding this comment.
This file has been removed
delta_backend/src/converter.py
Outdated
|
|
||
| class Converter: | ||
|
|
||
| def __init__(self, fhir_data, action_flag = "UPDATE", report_unexpected_exception=True): |
There was a problem hiding this comment.
Can use ActionFlag.UPDATE
robertnovac1
changed the title
Akol125
left a comment
Akol125
left a comment
There was a problem hiding this comment.
It's actually more straightforward, rather than use switch cases and expressionRules. Extractor is called in layout and it directly handles conversion logic, nice. The default postcode with zz, is that how you want to handle the default address if none, because it gives an impression that obfuscation was done on the address, which might be misleading, what do you think?
nhsdevws
left a comment
nhsdevws
left a comment
There was a problem hiding this comment.
Field management - use enums / constants is pretty much a should, magic strings can cause problems
|
| contained = self.fhir_json_data.get("contained", []) | ||
| return next((c for c in contained if isinstance(c, dict) and c.get("resourceType") == "Patient"), "") | ||
|
|
||
| def _get_valid_names(self, names, occurrence_time): |
There was a problem hiding this comment.
The spec has one extra rule - only consider names which have a given and family. I feel like we should question this though - follow up ticket?
| return "", "" | ||
|
|
||
| practitioner_names = practitioner.get("name", []) | ||
| valid_practitioner_names = [n for n in practitioner_names if "given" in n or "family" in n] |
There was a problem hiding this comment.
The spec says we should have a given AND family. I feel like we should question this though - follow up ticket?
| if not isinstance(performers, list) or not performers: | ||
| return "", "" | ||
|
|
||
| valid_performers = [p for p in performers if "actor" in p and "identifier" in p["actor"]] |
There was a problem hiding this comment.
The spec says we should prefer performers with a system unless there's only one specified - follow up ticket?
There was a problem hiding this comment.
I think this is being handled below
|
|
||
| valid_addresses = [a for a in addresses if "postalCode" in a and self._is_current_period(a, occurrence_time)] | ||
| if not valid_addresses: | ||
| return self.DEFAULT_POSTCODE |
There was a problem hiding this comment.
If there's only one address, the spec says to use it even if it's not current at the time of vaccination - follow up ticket?
|
|
||
| except Exception as e: | ||
| message = "DateTime conversion error [%s]: %s" % (e.__class__.__name__, e) | ||
| error = self._log_error("DATE_AND_TIME", message, e, code=exception_messages.UNEXPECTED_EXCEPTION) |
There was a problem hiding this comment.
Could use field name constants here and a few other places in this file
|
|
||
| def extract_unique_id(self): | ||
| identifier = self.fhir_json_data.get("identifier", []) | ||
| if identifier and len(identifier) == 1: |
There was a problem hiding this comment.
Not sure we need the length check?
|
|
||
| if isinstance(primary_source, bool): | ||
| return primary_source | ||
| if str(primary_source).strip().lower() == "true": |
There was a problem hiding this comment.
Don't think we should ever get something other than a boolean
There was a problem hiding this comment.
Correct - it's always boolean in backend api
mfjarvis
left a comment
mfjarvis
left a comment
There was a problem hiding this comment.
Looks really good. Couple of minor differences from the spec, but happy for these to be looked at later
4 participants
Summary
Major overhaul of the delta JSON extraction logic.
Removed the following:
Added:
Reviews Required
Review Checklist
ℹ️ This section is to be filled in by the reviewer.