-
Notifications
You must be signed in to change notification settings - Fork 189
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
(chore) O3-2120: Move OpenMRS ID
patient search identifier to defaultIdentifierTypes
#1061
base: main
Are you sure you want to change the base?
Changes from 1 commit
35cc1fe
e3347d9
e986080
caae423
dbb81b6
ef45c65
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,12 +44,11 @@ export const configSchema = { | |
_description: | ||
'A list of identifier types to be displayed in the patient search results as banner tags. If no defaultIdentifierTypes are provided, the defaultIdentifier will be displayed.', | ||
// These values correspond to the Patient Clinic Number and National Unique Patient Identifier (NUPI) identifier types respectively | ||
_default: ['b4d66522-11fc-45c7-83e3-39a1af21ae0d', 'f85081e2-b4be-4e48-b3a4-7994b69bb101'], | ||
}, | ||
defaultIdentifier: { | ||
_type: Type.String, | ||
_description: 'Identifer type to be displayed when no defaultIdentifierTypes are provided. Default is OpenMRS ID.', | ||
_default: 'OpenMRS ID', | ||
_default: [ | ||
'b4d66522-11fc-45c7-83e3-39a1af21ae0d', | ||
'f85081e2-b4be-4e48-b3a4-7994b69bb101', | ||
'05a29f94-c0ed-11e2-94be-8c13b969e334', | ||
], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO we shouldn't be hardcoding any identifier uuids here but this should come from the ref app configuration, but I know there's not agreement on that (and that this is existing code, not a change introduced as part of this code) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @mogoodrich for the feedback. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What are the other identifiers then? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, these first two identifiers are Palladium-specific. Those are the Patient Clinic Number and the NUPI number. They should be provided via distro configuration instead of being hardcoded in the Patient Search config schema. @donaldkibet, can @usamaidrsk proceed to remove them from here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the config file there is this comment, for the first two.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @denniskigen ! In fairness to @usamaidrsk looks like those two identifiers constants existed prior to this PR, but if they are implementation-specific, they definitely should be removed as defaults. @donaldkibet There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks right @mogoodrich |
||
}, | ||
}; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the configuration still specifies no patient identifiers? It seems like we still need some fallback here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ibacher how do you suggest we handle this callback, given that from the previous implementations it was checking for a static string
OpenMRS ID
if it existed in the patient identifiersThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@usamaidrsk Technically, it's checking for a user-configurable value,
identifierName
, right? I'm not sure there's a saner implementation than that. Maybe if that's also unset we could look for thepreferred
identifier?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is a configurable value, which makes it not safe.
I do agree with the idea of the
preferred
identifier, how do we get to itcc @ibacher
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a property of
patient.identifiers
or available as one (if it's excluded via a custom representation).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have pushed an updated respective to that.
cc @ibacher