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.

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

Update TikTok Pixel Destination #1937

Merged

Conversation

jae-rhee-tiktok
Copy link
Contributor

Update TikTok Pixel Destination to support all TikTok standard web events & event properties.

@jae-rhee-tiktok jae-rhee-tiktok marked this pull request as ready for review March 27, 2024 16:18
@jae-rhee-tiktok jae-rhee-tiktok requested review from a team as code owners March 27, 2024 16:18
@jae-rhee-tiktok jae-rhee-tiktok requested a review from a team March 27, 2024 16:18
},
useExistingPixel: {
// TODO: HOW TO DELETE (reusing will not include Segment Partner name)
label: '[Deprecated]Use Existing Pixel',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
label: '[Deprecated]Use Existing Pixel',
label: '[Deprecated] Use Existing Pixel',

*/
event: string
/**
* Any hashed ID that can identify a unique user/session.
*/
event_id?: string
/**
* Phone number of the user who triggered the conversion event, in E.164 standard format, e.g. +14150000000. Segment will hash this value before sending to TikTok.
* A single phone number in E.164 standard format. TikTok Pixel will hash this value before sending to TikTok. e.g. +14150000000. Segment will hash this value before sending to TikTok.
Copy link
Contributor

Choose a reason for hiding this comment

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

The Offline Conversions desription for this field also includes the following:

At least one phone number value is required if both Email and External ID fields are empty.

Should this apply for TikTok pixel as well?

/**
* Email address of the user who triggered the conversion event. Segment will hash this value before sending to TikTok.
* A single email address. TikTok Pixel will be hash this value before sending to TikTok.
Copy link
Contributor

Choose a reason for hiding this comment

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

The same field in the TikTok Offline Conversions Destination also includes the following:

At least one email value is required if both Phone Number and External ID fields are empty.

Should this apply for TikTok pixel as well?

*/
shop_id?: string
/**
* Uniquely identifies the user who triggered the conversion event. TikTok Pixel will hash this value before sending to TikTok.
Copy link
Contributor

Choose a reason for hiding this comment

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

TikTok Offline Conversions also includes the following:

TikTok Offline Conversions Destination supports both string and string[] types for sending external ID(s). At least one external ID value is required if both Email and Phone Number fields are empty.

Should something similar apply for TikTok Pixel?

description:
'Uniquely identifies the user who triggered the conversion event. TikTok Pixel will hash this value before sending to TikTok.',
type: 'string',
multiple: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you are changing this from a string to be a string array. It should be fine, but is this deliberate?

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 changed this to be a string array to match the s2s destination mappings, so that clients can re-use the same mappings for both Pixel & S2S destinations.

Comment on lines +20 to +22
email: handleArrayInput(payload.email),
phone_number: formatPhone(handleArrayInput(payload.phone_number)),
external_id: handleArrayInput(payload.external_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Has this been tested so that it works if a single string or an array of strings has been passed in? I'm thinking it might need to be able to handle both?

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 thought it would only need to handle string array since the parameter type defined as string array in the common_fields - will those variables come in as string types?

Comment on lines 20 to 23
export function handleArrayInput(array: string[] | undefined): string {
if (!array) return ''
return array[0]
}
Copy link
Contributor

@joe-ayoub-segment joe-ayoub-segment Apr 3, 2024

Choose a reason for hiding this comment

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

Should we also handle the case where 'array' is empty?
Should we also handle the case where array is a single string? i.e not an array?
Is it intentional that only the first item from the array is being forwarded on to TikTok?

@joe-ayoub-segment joe-ayoub-segment merged commit 698a7aa into segmentio:main Apr 15, 2024
8 of 11 checks passed
harsh-joshi99 pushed a commit that referenced this pull request Apr 19, 2024
* add all presets for pixel sdk & update event properties support

* update default values & global settings

* use existing pixel setting

* revert pixel init script

* update

---------

Co-authored-by: Jaehyuk Rhee <jaehyuk.rhee@bytedance.com>
@joe-ayoub-segment
Copy link
Contributor

PR deployed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants