From 78693299f34de2e8c1cfc3ebaad86cddfbd51629 Mon Sep 17 00:00:00 2001 From: Evan Masseau <> Date: Thu, 11 Apr 2024 13:20:42 -0400 Subject: [PATCH 1/4] Added some layers of value validation to reduce duplicative or error prone API calls - Trim whitespace from identifiers - Reject with warning any blank string value for identifiers - Setting identifiers with the fluent setters to the same value as before will not enqueue an API request --- .../java/com/klaviyo/analytics/Klaviyo.kt | 83 ++++++++++++++----- .../com/klaviyo/analytics/model/BaseModel.kt | 2 + .../com/klaviyo/analytics/model/ProfileKey.kt | 7 ++ .../java/com/klaviyo/analytics/KlaviyoTest.kt | 81 ++++++++++++++++++ 4 files changed, 153 insertions(+), 20 deletions(-) diff --git a/sdk/analytics/src/main/java/com/klaviyo/analytics/Klaviyo.kt b/sdk/analytics/src/main/java/com/klaviyo/analytics/Klaviyo.kt index 479e8c61e..738dfa68f 100644 --- a/sdk/analytics/src/main/java/com/klaviyo/analytics/Klaviyo.kt +++ b/sdk/analytics/src/main/java/com/klaviyo/analytics/Klaviyo.kt @@ -7,6 +7,7 @@ import android.content.Intent import com.klaviyo.analytics.model.Event import com.klaviyo.analytics.model.EventKey import com.klaviyo.analytics.model.EventMetric +import com.klaviyo.analytics.model.PROFILE_IDENTIFIERS import com.klaviyo.analytics.model.Profile import com.klaviyo.analytics.model.ProfileKey import com.klaviyo.analytics.networking.ApiClient @@ -67,7 +68,9 @@ object Klaviyo { /** * Assign new identifiers and attributes to the currently tracked profile. - * If a profile has already been identified it will be overwritten by calling [resetProfile]. + * If a profile has already been identified, it will be overwritten by calling [resetProfile]. + * + * Whitespace will be trimmed from all identifier values. * * The SDK keeps track of current profile details to * build analytics requests with profile identifiers @@ -82,15 +85,38 @@ object Klaviyo { resetProfile() } - UserInfo.externalId = profile.externalId ?: "" - UserInfo.email = profile.email ?: "" - UserInfo.phoneNumber = profile.phoneNumber ?: "" - profileOperationQueue.debounceProfileUpdate(profile) + // Copy the profile object, so we aren't mutating the argument + val mutableProfile = Profile().merge(profile) + + // Route identifiers to the explicit setter functions to re-use that validator logic + mutableProfile.externalId?.let { + setExternalId(it) + mutableProfile.externalId = null + } + + mutableProfile.email?.let { + setEmail(it) + mutableProfile.email = null + } + + mutableProfile.phoneNumber?.let { + setPhoneNumber(it) + mutableProfile.phoneNumber = null + } + + // Enqueue any remaining profile attributes + if (mutableProfile.count() > 0) { + profileOperationQueue.debounceProfileUpdate(mutableProfile) + } } /** * Assigns an email address to the currently tracked Klaviyo profile * + * Whitespace will be trimmed from the value. + * Calling this method with an empty string or the same value as currently set + * will be ignored. To clear identifiers, use [resetProfile]. + * * The SDK keeps track of current profile details to * build analytics requests with profile identifiers * @@ -113,6 +139,10 @@ object Klaviyo { * NOTE: Phone number format is not validated, but should conform to Klaviyo formatting * see (documentation)[https://help.klaviyo.com/hc/en-us/articles/360046055671-Accepted-phone-number-formats-for-SMS-in-Klaviyo] * + * Whitespace will be trimmed from the value. + * Calling this method with an empty string or the same value as currently set + * will be ignored. To clear identifiers, use [resetProfile]. + * * The SDK keeps track of current profile details to * build analytics requests with profile identifiers * @@ -139,6 +169,10 @@ object Klaviyo { * NOTE: Please consult (documentation)[https://help.klaviyo.com/hc/en-us/articles/12902308138011-Understanding-identity-resolution-in-Klaviyo-] * to familiarize yourself with identity resolution before using this identifier. * + * Whitespace will be trimmed from the value. + * Calling this method with an empty string or the same value as currently set + * will be ignored. To clear identifiers, use [resetProfile]. + * * The SDK keeps track of current profile details to * build analytics requests with profile identifiers * @@ -195,22 +229,31 @@ object Klaviyo { * @return Returns [Klaviyo] for call chaining */ fun setProfileAttribute(propertyKey: ProfileKey, value: String): Klaviyo = safeApply { - when (propertyKey) { - ProfileKey.EMAIL -> { - UserInfo.email = value - profileOperationQueue.debounceProfileUpdate(UserInfo.getAsProfile()) - } - ProfileKey.EXTERNAL_ID -> { - UserInfo.externalId = value - profileOperationQueue.debounceProfileUpdate(UserInfo.getAsProfile()) - } - ProfileKey.PHONE_NUMBER -> { - UserInfo.phoneNumber = value - profileOperationQueue.debounceProfileUpdate(UserInfo.getAsProfile()) - } - else -> { - profileOperationQueue.debounceProfileUpdate(Profile(mapOf(propertyKey to value))) + if (PROFILE_IDENTIFIERS.contains(propertyKey)) { + value.trim().ifEmpty { + Registry.log.warning( + "Empty string for $propertyKey will be ignored. To clear identifiers use resetProfile." + ) + null + }?.also { validatedIdentifier -> + var property by when (propertyKey) { + ProfileKey.EXTERNAL_ID -> UserInfo::externalId + ProfileKey.EMAIL -> UserInfo::email + ProfileKey.PHONE_NUMBER -> UserInfo::phoneNumber + else -> return@safeApply + } + + if (property != validatedIdentifier) { + property = validatedIdentifier + profileOperationQueue.debounceProfileUpdate(UserInfo.getAsProfile()) + } else { + Registry.log.info( + "$propertyKey value was unchanged, the update will be ignored." + ) + } } + } else { + profileOperationQueue.debounceProfileUpdate(Profile(mapOf(propertyKey to value))) } } diff --git a/sdk/analytics/src/main/java/com/klaviyo/analytics/model/BaseModel.kt b/sdk/analytics/src/main/java/com/klaviyo/analytics/model/BaseModel.kt index 37a0fac86..fecbf309f 100644 --- a/sdk/analytics/src/main/java/com/klaviyo/analytics/model/BaseModel.kt +++ b/sdk/analytics/src/main/java/com/klaviyo/analytics/model/BaseModel.kt @@ -29,6 +29,8 @@ abstract class BaseModel(properties: Map?) this.setProperty(key, value) } + fun count(): Int = propertyMap.count() + /** * Convert this data model into a simple map */ diff --git a/sdk/analytics/src/main/java/com/klaviyo/analytics/model/ProfileKey.kt b/sdk/analytics/src/main/java/com/klaviyo/analytics/model/ProfileKey.kt index 87318cba4..936904608 100644 --- a/sdk/analytics/src/main/java/com/klaviyo/analytics/model/ProfileKey.kt +++ b/sdk/analytics/src/main/java/com/klaviyo/analytics/model/ProfileKey.kt @@ -58,3 +58,10 @@ sealed class ProfileKey(name: String) : Keyword(name) { else -> name } } + +internal val PROFILE_IDENTIFIERS = arrayOf( + ProfileKey.EXTERNAL_ID, + ProfileKey.EMAIL, + ProfileKey.PHONE_NUMBER, + ProfileKey.ANONYMOUS_ID +) diff --git a/sdk/analytics/src/test/java/com/klaviyo/analytics/KlaviyoTest.kt b/sdk/analytics/src/test/java/com/klaviyo/analytics/KlaviyoTest.kt index e1b9a5288..034aae936 100644 --- a/sdk/analytics/src/test/java/com/klaviyo/analytics/KlaviyoTest.kt +++ b/sdk/analytics/src/test/java/com/klaviyo/analytics/KlaviyoTest.kt @@ -183,6 +183,87 @@ internal class KlaviyoTest : BaseTest() { assertEquals(stubMiddleName, profile[stubMiddleNameKey]) } + @Test + fun `Whitespace is trimmed off of profile identifiers for fluent setters`() { + Klaviyo.setExternalId("\t$EXTERNAL_ID \n") + .setEmail("$EMAIL \t\n") + .setPhoneNumber(" $PHONE \t\n") + + verifyProfileDebounced() + val profile = capturedProfile.captured + assertEquals(EXTERNAL_ID, profile.externalId) + assertEquals(EMAIL, profile.email) + assertEquals(PHONE, profile.phoneNumber) + } + + @Test + fun `Whitespace is trimmed off of profile identifiers for setProfile`() { + Klaviyo.setProfile( + Profile( + externalId = "\t$EXTERNAL_ID \n", + email = "$EMAIL \t\n", + phoneNumber = " $PHONE \t\n" + ) + ) + + verifyProfileDebounced() + val profile = capturedProfile.captured + assertEquals(EXTERNAL_ID, profile.externalId) + assertEquals(EMAIL, profile.email) + assertEquals(PHONE, profile.phoneNumber) + } + + @Test + fun `Empty identifiers are ignored with warning`() { + Klaviyo.setProfile( + Profile( + externalId = EXTERNAL_ID, + email = EMAIL, + phoneNumber = PHONE + ) + ) + + verifyProfileDebounced() + val profile = capturedProfile.captured + assertEquals(EXTERNAL_ID, profile.externalId) + assertEquals(EMAIL, profile.email) + assertEquals(PHONE, profile.phoneNumber) + + Klaviyo.setExternalId("") + Klaviyo.setEmail("") + Klaviyo.setPhoneNumber("") + + verifyProfileDebounced() // Should not have enqueued a new request + verify(exactly = 3) { logSpy.warning(any(), null) } + assertEquals(EXTERNAL_ID, UserInfo.externalId) + assertEquals(EMAIL, UserInfo.email) + assertEquals(PHONE, UserInfo.phoneNumber) + } + + @Test + fun `Unchanged profile identifiers do not enqueue new API requests`() { + Klaviyo.setProfile( + Profile( + externalId = EXTERNAL_ID, + email = EMAIL, + phoneNumber = PHONE + ) + ) + + verifyProfileDebounced() + val profile = capturedProfile.captured + assertEquals(EXTERNAL_ID, profile.externalId) + assertEquals(EMAIL, profile.email) + assertEquals(PHONE, profile.phoneNumber) + + Klaviyo.setExternalId(EXTERNAL_ID) + Klaviyo.setEmail(EMAIL) + Klaviyo.setPhoneNumber(PHONE) + + // We should not have enqueued a new request, so still should only have been called once + verifyProfileDebounced() + } + @Test fun `setProfile is debounced`() { Klaviyo.setProfile(Profile().setEmail(EMAIL)) From 9167f1d48560dab00d246f8243d97d9c4baf68d3 Mon Sep 17 00:00:00 2001 From: Evan Masseau <> Date: Thu, 11 Apr 2024 13:30:03 -0400 Subject: [PATCH 2/4] Missed readme AC from the ticket. --- README.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/README.md b/README.md index 91b3fcfe9..b62281e77 100644 --- a/README.md +++ b/README.md @@ -157,6 +157,9 @@ Klaviyo.setEmail("robin@example.com") .setProfileAttribute(ProfileKey.FIRST_NAME, "Robin") ``` +**Note:** We strip whitespace from identifier values, and empty strings will be ignored with a logged warning. +If you are trying to remove an identifier's value, use `setProfile` or `resetProfile`. + ### Anonymous Tracking Klaviyo will track unidentified users with an autogenerated ID whenever a push token is set or an event is created. That way, you can collect push tokens and track events prior to collecting profile identifiers such as email or From 0d233bdd85ccd29f98c4d20a768318de68b9f02c Mon Sep 17 00:00:00 2001 From: Evan Masseau <> Date: Thu, 11 Apr 2024 13:39:50 -0400 Subject: [PATCH 3/4] wording --- README.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index b62281e77..0ad860990 100644 --- a/README.md +++ b/README.md @@ -157,8 +157,9 @@ Klaviyo.setEmail("robin@example.com") .setProfileAttribute(ProfileKey.FIRST_NAME, "Robin") ``` -**Note:** We strip whitespace from identifier values, and empty strings will be ignored with a logged warning. -If you are trying to remove an identifier's value, use `setProfile` or `resetProfile`. +**Note:** We trim leading and trailing whitespace off of identifier values. +Empty strings will be ignored with a logged warning. If you are trying to remove an identifier's value, +use `setProfile` or `resetProfile`. ### Anonymous Tracking Klaviyo will track unidentified users with an autogenerated ID whenever a push token is set or an event is created. From a5b52e4fbc908eaef0111588fb512a6cd147d613 Mon Sep 17 00:00:00 2001 From: Evan C Masseau <5167687+evan-masseau@users.noreply.github.com> Date: Thu, 11 Apr 2024 16:22:00 -0400 Subject: [PATCH 4/4] Apply suggestions from code review --- sdk/analytics/src/main/java/com/klaviyo/analytics/Klaviyo.kt | 2 +- .../src/main/java/com/klaviyo/analytics/model/BaseModel.kt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/analytics/src/main/java/com/klaviyo/analytics/Klaviyo.kt b/sdk/analytics/src/main/java/com/klaviyo/analytics/Klaviyo.kt index 738dfa68f..248a796b9 100644 --- a/sdk/analytics/src/main/java/com/klaviyo/analytics/Klaviyo.kt +++ b/sdk/analytics/src/main/java/com/klaviyo/analytics/Klaviyo.kt @@ -105,7 +105,7 @@ object Klaviyo { } // Enqueue any remaining profile attributes - if (mutableProfile.count() > 0) { + if (mutableProfile.propertyCount() > 0) { profileOperationQueue.debounceProfileUpdate(mutableProfile) } } diff --git a/sdk/analytics/src/main/java/com/klaviyo/analytics/model/BaseModel.kt b/sdk/analytics/src/main/java/com/klaviyo/analytics/model/BaseModel.kt index fecbf309f..298f169de 100644 --- a/sdk/analytics/src/main/java/com/klaviyo/analytics/model/BaseModel.kt +++ b/sdk/analytics/src/main/java/com/klaviyo/analytics/model/BaseModel.kt @@ -29,7 +29,7 @@ abstract class BaseModel(properties: Map?) this.setProperty(key, value) } - fun count(): Int = propertyMap.count() + fun propertyCount(): Int = propertyMap.count() /** * Convert this data model into a simple map