-
Notifications
You must be signed in to change notification settings - Fork 110
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
(fix) Replace the existing name instead of creating a new name #546
base: master
Are you sure you want to change the base?
(fix) Replace the existing name instead of creating a new name #546
Conversation
Functionally, it's incorrect to assume a patient should only have one name. We can, however, try voiding any names that are not in the list. (The UI defaults to the preferred name for a client, but that doesn't mean additional names are not important). |
Hey @ibacher , thanks for the feddback. You're right. I'm trying to implement a logic to update the name instead of setting the others to voided. I'm still investigating another possible solution, which is why I put the PR in draft. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #546 +/- ##
============================================
+ Coverage 77.84% 77.87% +0.03%
- Complexity 2683 2706 +23
============================================
Files 239 239
Lines 7452 7511 +59
Branches 901 919 +18
============================================
+ Hits 5801 5849 +48
Misses 1115 1115
- Partials 536 547 +11 ☔ View full report in Codecov by Sentry. |
@denniskigen @ibacher would it be possible for you to review this PR ? Thank you cc @icrc-jofrancisco @icrc-fdeniger @gracepotma |
existingName = currentPatient.getNames().stream().filter(n -> n.getUuid().equals(name.getId())).findFirst() | ||
.orElse(null); | ||
} else { | ||
existingName = currentPatient.getPersonName(); |
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.
This behaviour still strikes me as incorrectly assuming that a patient has only a single name...
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'm on board with matching existing names based on UUIDs, but I'm not sold that this is a good idea.
Description of what I changed
Problem
When updating a patient's name using the FHIR endpoint, a new person_name entry is created in the database instead of voiding the existing one and creating a new entry. This results in multiple active names for a single patient, which is incorrect and leads to data inconsistencies. Additionally, OpenMRS does not consider more than one name and always picks the first one, so users cannot see any changes in the UI.
Solution
This fix addresses the problem by:
If the person's name doesn't contain an
id
for some reason (in a normal case it will), it will “ replace ” the preferred name for the person.By implementing this approach, we ensure the UI reflects the latest changes accurately.
Thnaks,
Issue I worked on
see https://issues.openmrs.org/browse/FM2-
Checklist: I completed these to help reviewers :)
My IDE is configured to follow the code style of this project.
I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)
I ran
mvn clean package
right before creating this pull request andadded all formatting changes to my commit.
All new and existing tests passed.
My pull request is based on the latest changes of the master branch.