Skip to content
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.

Sign up for GitHub

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

Order form is dropping second last names unexpectedly #3113

Merged
merged 21 commits into from
Nov 27, 2024
Merged

Order form is dropping second last names unexpectedly #3113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
e885fec
Refactor first name and last name detection of customer logic
AnirudhBhat Nov 25, 2024
0266ff2
Add test to verify that proper first name is returned given customer …
AnirudhBhat Nov 25, 2024
8f355b0
Remove unnecessary annotation
AnirudhBhat Nov 25, 2024
6e3f63b
add test to verify proper last name is detected
AnirudhBhat Nov 25, 2024
2b2a46d
add test to verify empty string is returned when customer name is null
AnirudhBhat Nov 25, 2024
6bc6314
add test to verify full string is returned when customer name has no …
AnirudhBhat Nov 25, 2024
27f2158
add test to verify empty string is returned for last name when custom…
AnirudhBhat Nov 25, 2024
bba2b8d
add test to verify empty string is returned for last name when custom…
AnirudhBhat Nov 25, 2024
4ca0bc6
add test to verify proper string is returned for first name when cust…
AnirudhBhat Nov 25, 2024
d76f381
add test to verify proper string is returned for last name when custo…
AnirudhBhat Nov 25, 2024
3b5294e
Refactor logic to handle multiple spaces, leading and trailing spaces…
AnirudhBhat Nov 26, 2024
2276918
Add test to verify leading space in customer name will still give us …
AnirudhBhat Nov 26, 2024
1700a00
Add test to verify trailing space in customer name will still give us…
AnirudhBhat Nov 26, 2024
c4e267a
Add test to verify multiple in between space in customer name will st…
AnirudhBhat Nov 26, 2024
2455c15
Add test to verify multiple in between space in customer name will st…
AnirudhBhat Nov 26, 2024
8f338d2
Comment given, when, then in capitals to maintain consistency in file
AnirudhBhat Nov 26, 2024
e085d0c
Add comment
AnirudhBhat Nov 26, 2024
7b6d579
Fix checkstyle error
AnirudhBhat Nov 26, 2024
651e2b9
Fix checkstyle error
AnirudhBhat Nov 26, 2024
a243e08
Fix checkstyle error
AnirudhBhat Nov 26, 2024
2aca001
Refactor first name and last name extraction logic
AnirudhBhat Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
180 changes: 180 additions & 0 deletions example/src/test/java/org/wordpress/android/fluxc/model/customer/WCCustomerMapperTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,4 +273,184 @@ class WCCustomerMapperTest {
// THEN
assertThat(result.isPayingCustomer).isEqualTo(false)
}

@Test
fun `given customer name, then first name is properly assigned`() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// GIVEN
val siteId = 23
val site = SiteModel().apply { id = siteId }

val customerDTO = CustomerFromAnalyticsDTO(name = "firstname lastname")

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

// THEN
assertEquals("firstname", result.firstName)
}

@Test
fun `given customer name as null, then first name returns empty string`() {
// GIVEN
val siteId = 23
val site = SiteModel().apply { id = siteId }

val customerDTO = CustomerFromAnalyticsDTO(name = null)

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

// THEN
assertEquals("", result.firstName)
}

@Test
fun `given customer name with no space, then first name returns full string`() {
// GIVEN
val siteId = 23
val site = SiteModel().apply { id = siteId }

val customerDTO = CustomerFromAnalyticsDTO(name = "firstnamelastname")

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

// THEN
assertEquals("firstnamelastname", result.firstName)
}

@Test
fun `given customer name, then last name is properly assigned`() {
// GIVEN
val siteId = 23
val site = SiteModel().apply { id = siteId }

val customerDTO = CustomerFromAnalyticsDTO(name = "firstname lastname")

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

// THEN
assertEquals("lastname", result.lastName)
}

@Test
fun `given customer name as null, then last name returns empty string`() {
// GIVEN
val siteId = 23
val site = SiteModel().apply { id = siteId }

val customerDTO = CustomerFromAnalyticsDTO(name = null)

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

// THEN
assertEquals("", result.lastName)
}

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

val customerDTO = CustomerFromAnalyticsDTO(name = "firstnamelastname")

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

// THEN
assertEquals("", result.lastName)
}

@Test
fun `given customer name has multiple spaces, then first 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("firstname", result.firstName)
}

@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)
}

@Test
fun `given customer name has leading space, then first 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("firstname", result.firstName)
}

@Test
fun `given customer name has trailing space, 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)
}

@Test
fun `given customer name has multiple in between space, then first 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("firstname", result.firstName)
}

@Test
fun `given customer name has multiple in between space, 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)
}
}
8 changes: 6 additions & 2 deletions ...oocommerce/src/main/kotlin/org/wordpress/android/fluxc/model/customer/WCCustomerMapper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,10 @@ class WCCustomerMapper @Inject constructor() {
}
}

private fun String?.firstNameFromName() = this?.split(" ")?.firstOrNull() ?: ""
private fun String?.lastNameFromName() = this?.split(" ")?.lastOrNull() ?: ""
// Please refer WCCustomerMapperTest file which serves as documentation of how this function behaves.
private fun String?.firstNameFromName(): String =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it do the trick in both cases?

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: ""

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: 2aca001

this?.trim()?.substringBefore(' ')?.trim().orEmpty()

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