Order form is dropping second last names unexpectedly

[AnirudhBhat]

closes #11236

This PR fixes the logic to retrieve first name and last name given customer full name.

Before this PR, we were considering first word to be the first name and last word to be the last name. This caused problem when there were multiple words in customers name.
Full Name : Firstname and a very long last name
firstname : Firstname
lastname : name

After this PR, we are considering first word to be the first name and all other words as last names
Full Name : Firstname and a very long last name
firstname : Firstname
lastname : and a very long last name

customer_last_name.mp4

[kidinov]

np: in all tests, maybe assert both first and last names, because they kinda related and deducted from one string?

[AnirudhBhat]

I agree, my reasoning for splitting the test cases is mainly to understand why the test fails in future. There must be only one reason for the failure of tests IMHO. If we combine both, we need to see if the test failed because of logical error in the extraction of first name or last name which could be avoided if the test asserts just one thing.

[kidinov]

In my opinion, the one reason here - "if correct logic used in splitting the string". The current implementation, which consists of two private extensions, should not affect the test's logic. Currently, the test does not validate whether the splitter is functioning correctly; instead, it only checks that the first word is designated as the first name.

[kidinov]

np: the comments are upper case in the other tests

[AnirudhBhat]

Done: 8f338d2

[kidinov]

Does this handle extra spaces correctly? I wrote a test and it doesn't pass

    @Test
    fun `given customer name has multiple spaces, then last name returns proper string`() {
        // GIVEN
        val siteId = 23
        val site = SiteModel().apply { id = siteId }

        val customerDTO = CustomerFromAnalyticsDTO(name = "firstname   and a very long last name")

        // WHEN
        val result = mapper.mapToModel(site, customerDTO)

        // THEN
        assertEquals("and a very long last name", result.lastName)
        assertEquals("firstname", result.firstName)
    }

[AnirudhBhat]

Good catch! I've fixed this

1. Multiple in-between spaces
2. Leading spaces
3. Trailing spaces

3b5294e

[kidinov]

@AnirudhBhat, thanks for the improvement! I left a couple of nitpicks and one more potential bug. Please take a look!

[kidinov]

Wouldn't it do the trick in both cases?

.substringBefore(" ").trim() ?: ""

You implementation works, but both (maybe) slow and not easy to read

[AnirudhBhat]

This wouldn't work as it fails for names that has leading spaces, multiple spaces in between.

For example: First Name:
.substringBefore(" "): Returns "" because the first substring before a space is empty due to leading spaces.
.trim(): Has no effect since the result is already an empty string.
Result: ""

[kidinov]

Considering that we run this code 6 times per 1 customer mapped I'd propose something simple like that:

    private fun String?.firstNameFromName(): String =
        this?.trim()?.substringBefore(' ')?.trim().orEmpty()

    private fun String?.lastNameFromName(): String =
        this?.trim()?.substringAfter(' ', "")?.trim().orEmpty()

It won't remove spaces inside of the first and last name, but IMO, its not what we even want and we should leave it up to the users and the way they want to write their names

[AnirudhBhat]

Done: 2aca001

[kidinov]

LGTM!