I don't understand that part

Here we use InternetAddress only to detect whether the value entered in the recipient column is an email address (e.g. alannadolny@gmail.com or Alan Nadolny <alannadolny@gmail.com>). Based on that, we decide whether to check the person by email (getActivePersonByEmail method) or by full name (getActivePersonByName method). You can read more about this here: https://docs.oracle.com/javaee/5/api/javax/mail/internet/InternetAddress.html

I was inspired by @ewaterman code:

    Person getOrCreatePersonByRecipient(String recipient) {
        if (StringUtils.isBlank(recipient)) {
            return null
        }

        InternetAddress internetAddress
        try {
            internetAddress = new InternetAddress(recipient, false)
        }
        catch (AddressException ignored) {
            // If recipient isn't a valid internet address, it must be a regular name.
            return getOrCreatePersonFromNames(recipient)
        }

        // If a person exists with the given email, return them. Note that this can return inactive people so
        // the caller must properly handle that case. We do this to avoid creating duplicate users.
        Person person = getPersonByEmail(internetAddress.address)
        if (person) {
            return person
        }

        // Otherwise create a new person.
        if (!internetAddress.personal) {
            throw new RuntimeException("Cannot save new recipient without a name: ${recipient}")
        }
        String[] names = extractNames(internetAddress.personal)
        person = new Person(firstName: names[0], lastName: names[1], email: internetAddress.address)
        if (!person.save(flush: true)) {
            throw new ValidationException("Cannot save recipient ${recipient} due to errors", person.errors)
        }
        return person
    }

I forgot I wrote that 😅 I think the code in PersonService can be refactored so that we have cleaner methods:

• getPersonByName(String) -> split into firstname, lastname and query
• getPersonByEmail(String) -> simply query on Person.email
• getPerson(String) -> calls both getPersonByEmail and getPersonByName to find the Person
• getActivePerson(String) -> calls getPerson then returns them if active == true
• getOrCreatePerson(String) -> calls getPerson and if null, creates a Person

but I won't make you do that.

The refactor looks simple enough, so I can try tomorrow

btw, the internet address looks weird to me, and I am confused every time I see that code 😄 Maybe we can try to do some kind of validator for that? (https://grails.apache.org/docs/7.0.4/ref/Constraints/email.html)

@ewaterman

the problem is that when we import from a csv or xls file, we have just one field representing the Person (in this case "recipient"). And to be user friendly, we want to accept either name or email in that field. So we can't just wrap the field in a simple built-in validator. The String field that we get is either an email or a name and we need to figure out which it is here

you can assign the params["recipient"] to a variable

looks good! I know you didn't introduce it, but "recipient" doesn't make much sense here IMO because this can be used by any flow, not just orders and shipments. Really it's just a string for identifying a person.

Can you please change the "recipient" variables here to "identifier" or "personIdentifier" or something similar?

yes, I think that "identifier" makes more sense. I will change that

Nice tests! I feel you could be more exhaustive with inputs here. Both these methods could test:

- 'Justin'
- 'Justin Miranda'
- 'Justin Justy Miranda'
- 'justin@openboxes.com'
- 'unknown@openboxes.com'
- 'Justin Miranda <justin@openboxes.com>'
- 'Justin Miranda <unknown@openboxes.com>'
- 'Unknown Name <justin@openboxes.com>'
- 'Justin Inactive'
- null

I agree it’s a good idea to test more cases. I haven’t written many tests in my career, so thanks for the guidance on what exactly to test 🚀

👏

I think that this test case is curious. In this test case: Justin Miranda <unknown@openboxes.com> the result is null. This happens because the method detects an email format and only searches by the email address. Even though the name 'Justin Miranda' is correct, the system doesn't find the person because the email is unknown.

On the other hand, if the email is correct but the name is wrong (e.g. Unknown Name <justin@openboxes.com>), the system finds the person.

Maybe somehow we should improve this? If the email search fails, then we should also check by name or maybe we should stay as it is and always prioritize email 🤔

hmm it's an interesting point. I don't know if there's a clear answer.

Another argument is that if you provide both, both should match. Person isn't actually unique on email or on name so just because the name or email match, it doesn't mean you're getting the user that you want. Providing both could mean you want to be very specific. But idk, that could be overly restrictive.

I think it's more important that the email match because email far more likely to be unique. Two people can have the same name, but I'd be surprised if we have two people with the same email (unless the same person is in the system multiple times).

So given that, I guess the current logic makes sense to prioritize email, but I suppose we could have it try name afterwards if the email doesn't match.

That said, I don't actually know if anyone is providing strings like Justin Miranda <unknown@openboxes.com>. I imagine 99% of the time it's either just name or just email. So maybe it's not a problem that we need to deal with

@kchelstowski @alannadolny what do you think about this?

I believe it's not a problem because it's a hardcore edge case. But maybe as a simple solution, we can show the user an error like: "We found a person by the name that you provided, but the email doesn't match. Please check your data."

hmmmm I think it's better to leave it as it is 🤔