TRUNK-6529: Ensure patient has all required identifiers #5728
Conversation
|
Hi @ibacher, This PR addresses TRUNK-6529 by ensuring that a patient must have identifiers for all required, non-retired PatientIdentifierTypes before validation passes. I’ve added validation logic and updated unit tests to cover missing, present, voided, and retired required identifiers. All tests pass locally (mvn clean test). Please review when convenient.Thanks! |
|
Hi @ibacher, |
|
@Binayak490-cyber, kindly mention the ticket which is related to this PR! |
@kmvipin, it is the issue ticket TRUNK-6529 i.e; https://openmrs.atlassian.net/browse/TRUNK-6529?focusedCommentId=184800 |
|
Hi @ibacher, |
ibacher
left a comment
ibacher
left a comment
There was a problem hiding this comment.
Thanks! This needs some work. Also, instead of the O(n^2) algorithm you might be able to do something clever with sets to just get the difference between the "set of required identifiers" and the "set of identifiers the patient has", but it's not a big deal as the set of identifiers is normally very small.
| Collection<PatientIdentifierType> identifierTypes = Context.getPatientService().getAllPatientIdentifierTypes(false); | ||
|
|
||
| for (PatientIdentifierType type : identifierTypes) { | ||
| if (Boolean.TRUE.equals(type.getRequired()) && !type.getRetired()) { |
There was a problem hiding this comment.
We already know that these types are not retired, so the check here is redundant.
There was a problem hiding this comment.
Okk @ibacher, I removed the !type.getRetired() check as getAllPatientIdentifierTypes(false) already guarantees that only non-retired types are returned, making the additional check redundant and misleading.
| if (!found) { | ||
| errors.rejectValue( | ||
| "identifiers", | ||
| "Patient.missingRequiredIdentifier", |
There was a problem hiding this comment.
You need to actually add this string to the message.properties file.
There was a problem hiding this comment.
@ibacher, I’ve added Patient.missingRequiredIdentifier to messages.properties and pushed the update.
| errors.rejectValue( | ||
| "identifiers", | ||
| "Patient.missingRequiredIdentifier", | ||
| "Missing required patient identifier: " + type.getName() |
There was a problem hiding this comment.
Please lookup how you add parameters to a Spring errors object. Setting the default string isn't the right thing to do.
There was a problem hiding this comment.
@ibacher, I have updated to use Spring Errors.rejectValue with message parameters and added the message to messages.properties.
Removed the default message as suggested.
|
|
||
| for (PatientIdentifierType type : identifierTypes) { | ||
| if (Boolean.TRUE.equals(type.getRequired()) && !type.getRetired()) { | ||
| boolean found = false; |
There was a problem hiding this comment.
This isn't a case where we want to fail-fast. Instead we want to collect all missing required identifiers and send that back to the user as a single string.
There was a problem hiding this comment.
okk @ibacher, for not getting fail-fast, i have updated the validator to collect all missing required identifier types and report them back as a single validation error.
The error now uses Spring’s Errors.rejectValue with parameters.
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
| import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
| import static org.junit.Assert.*; |
There was a problem hiding this comment.
@ibacher, yepp i mistakenly added the full imported package which is not at all required for my added tests also and therefore, i have removed it.
|
|
Hi @ibacher, |
|
Hi @ibacher, |
|
Hi @ibacher, |
3 participants
This PR enhances PatientValidator to ensure that a patient must have identifiers for all required and non-retired PatientIdentifierTypes before validation passes.
Changes included:
Added validation logic to check that every required, non-retired identifier type has a corresponding non-voided patient identifier.
Updated and added unit tests to cover the following scenarios:
Patient missing a required identifier (validation fails)
Patient with required identifier present (validation passes)
Required identifier is voided (validation fails)
Required identifier type is retired (validation ignored)
This change ensures data consistency and aligns patient validation with the configured identifier requirements.All tests pass locally.