OBPIH-6825 Prioritize active persons when fetching by name or email by ewaterman · Pull Request #4971 · openboxes/openboxes

ewaterman · 2024-12-04T23:52:16Z

✨ Description of Change

A concise summary of what is being changed. Please provide enough context for reviewers to be able to understand the change and why it is necessary. If the issue/ticket already provides enough information, you can put "See ticket" as the description.

Link to GitHub issue or Jira ticket: https://pihemr.atlassian.net/browse/OBPIH-6825

Description: Because we don't enforce any uniqueness on name or email in our system for entries in the Person table, when fetching people by name and email, there's a chance we'll find more than one. When this happens, we simply pick the first one we get. This can cause inconsistent behaviour when we have two identical users, one active and one inactive.

This was uncovered in PO import but is relevant to all flows that fetch a single Person.

This change is to prioritize returning active Person entries in the case where we have duplicates.

…e we have duplicates

ewaterman · 2024-12-04T23:53:48Z

Marking this as do not merge for now since it's not essential to the release as it's not a new bug. If we feel confident about it though, I'm happy to try fitting it in.

ewaterman · 2024-12-04T23:55:49Z

grails-app/services/org/pih/warehouse/data/PersonService.groovy

+     *        format can be provided during data imports.
+     * @return A Person who has the provided name and email. Will be a new Person if they didn't previously exist.
+     */
+    Person getOrCreatePersonByRecipient(String recipient) {


for the most part this method is copied from ShipmentService. I did refactor it though (and created tests for it)

codecov · 2024-12-04T23:57:36Z

Codecov Report

Attention: Patch coverage is 54.00000% with 23 lines in your changes missing coverage. Please review.

Project coverage is 7.76%. Comparing base (d67b64f) to head (82f0037).
Report is 155 commits behind head on develop.

Files with missing lines	Patch %	Lines
...rvices/org/pih/warehouse/data/PersonService.groovy	64.28%	5 Missing and 10 partials ⚠️
.../warehouse/shipping/CombinedShipmentService.groovy	0.00%	3 Missing ⚠️
.../org/pih/warehouse/shipping/ShipmentService.groovy	0.00%	3 Missing ⚠️
...rvices/org/pih/warehouse/order/OrderService.groovy	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             develop   #4971      +/-   ##
============================================
+ Coverage       7.61%   7.76%   +0.14%     
- Complexity       817     845      +28     
============================================
  Files            601     601              
  Lines          42300   42315      +15     
  Branches       10277   10281       +4     
============================================
+ Hits            3222    3284      +62     
+ Misses         38610   38545      -65     
- Partials         468     486      +18

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

alannadolny · 2024-12-05T08:56:41Z

grails-app/services/org/pih/warehouse/data/PersonService.groovy

+    /**
+     * @return The first person we can find who has the given email, giving preference to active people.
+     */
+    private Person getPersonByEmail(String email) {


I am not feeling fully comfortable with this function name, because, at the last line, we are returning a list of active people, so I would like to add this information in the naming because at this moment the name suggests we are just returning all matching people by email, but I don't have any better idea right now

the find only returns a single active person. To me, if we were going to return a list of person I'd name the method getAllPersonByEmail or getPersonsByEmail or getPeopleByEmail

grails-app/services/org/pih/warehouse/data/PersonService.groovy

alannadolny · 2024-12-05T09:08:33Z

grails-app/services/org/pih/warehouse/data/PersonService.groovy

+            return null
+        }
+
+        InternetAddress internetAddress


I know you are not the original author of this, but maybe you can explain how and why we use InternetAdress?

Internet address is an email of the format: "Justin Miranda <[email protected]>". So this code parses that in and outputs a person with:

email: [email protected]

firstName: Justin

lastName: Miranda

I don't know why that format was the original requirement. Maybe @jmiranda has the answer. This format is only used during the import packing list flow.

Yeah, i think i was originally trying to impose a data standard here https://www.ietf.org/rfc/rfc822.txt but was quickly overruled (within 10 days) when met with reality.

alannadolny · 2024-12-05T09:09:14Z

grails-app/services/org/pih/warehouse/data/PersonService.groovy

+        }
+        String[] names = extractNames(internetAddress.personal)
+        person = new Person(firstName: names[0], lastName: names[1], email: internetAddress.address)
+        if (!person.save(flush: true)) {


is this flush needed?

probably not but it was in the original code so I didn't want to change it without knowing why it was added

Theoretically it shouldn't be necessary. But practically, it depends on where this code is being invoked. For adding a single person to the system, probably not. But in the previous version of Grails, things were less predictable when we were persisting lots of data all at once. Like say in a data import. It might be possible to remove the flush now (and I would be happy with that refactoring) but I appreciate Evan's instinct to leave well enough alone for now. And if/when we do make the change we need to ensure we have some integration tests around it to make sure it works for all use cases:

add a single person in an admin page

add a single person per line item and assign them as recipient on the shipment item

Again it should work without the flush, but we should perform a discovery before making changes. This might be a good scenario to add to the Cascading Behavior on Associations tech debt that we've discussed in the past.

alannadolny · 2024-12-05T09:10:13Z

grails-app/services/org/pih/warehouse/data/PersonService.groovy

-                throw new ValidationException("Cannot save recipient ${combinedNames} due to errors", person.errors)
-            }
+        person = new Person(firstName: names[0], lastName: names[1])
+        if (!person.save(flush: true)) {


and this one flush?

Same answer. It was in the original code so I didn't want to change it without knowing why it was added

alannadolny · 2024-12-05T09:20:03Z

grails-app/services/org/pih/warehouse/shipping/CombinedShipmentService.groovy

-                        recipient = Person.findByFirstNameAndLastName(firstName, lastName)
-                    } else if (names.length == 1) {
-                        recipient = Person.findByEmail(names[0])
+                    if (line.recipient.contains(" ")) {


I don't get it, why do we check if line.recipient contains " "?

You can just use the ternary operator for such an assignment

It's checking how many words are in the recipient string. If there are no spaces, it's an email, if there's one or more spaces, it's a name. It's kind of clunky, but we don't really have a better method of distinguishing between the formats

ewaterman · 2024-12-05T15:32:21Z

src/test/groovy/unit/org/pih/warehouse/person/PersonServiceSpec.groovy

+        ''              || null
+        'a b'           || '2'
+        '<[email protected]>'     || '2'
+        'e f <[email protected]>' || '3'  // email takes priority, even if name doesn't match and user is inactive.


This is an interesting quirk of the system. This is how the logic was working originally so I didn't change it, but it could be worth validating that both the name and email match the user if we're given both

awalkowiak

Approved on the tech huddle

when fetching people by name and email, prioritize active ones in cas…

97ba1a6

…e we have duplicates

ewaterman requested review from alannadolny, awalkowiak and kchelstowski December 4, 2024 23:52

ewaterman self-assigned this Dec 4, 2024

github-actions bot added type: bug Addresses unintended behaviours of the app domain: backend Changes or discussions relating to the backend server labels Dec 4, 2024

ewaterman added warn: do not merge Marks a pull request that is not yet ready to be merged and removed type: bug Addresses unintended behaviours of the app domain: backend Changes or discussions relating to the backend server labels Dec 4, 2024

ewaterman commented Dec 4, 2024

View reviewed changes

ewaterman added type: bug Addresses unintended behaviours of the app domain: backend Changes or discussions relating to the backend server labels Dec 5, 2024

alannadolny reviewed Dec 5, 2024

View reviewed changes

ewaterman commented Dec 5, 2024

View reviewed changes

formatting fixes

82f0037

ewaterman removed the warn: do not merge Marks a pull request that is not yet ready to be merged label Dec 11, 2024

awalkowiak approved these changes Dec 16, 2024

View reviewed changes

awalkowiak merged commit bcf3a9b into develop Dec 16, 2024

awalkowiak deleted the bug/OBPIH-6825-prioritize-active-person-when-duplicates branch December 16, 2024 16:33

Uh oh!

Conversation

ewaterman commented Dec 4, 2024

✨ Description of Change

Uh oh!

ewaterman commented Dec 4, 2024

Uh oh!

ewaterman Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ewaterman Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awalkowiak left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ewaterman Dec 4, 2024 •

edited

Loading

codecov bot commented Dec 4, 2024 •

edited

Loading

ewaterman Dec 5, 2024 •

edited

Loading