From e0a5d6bbf40641fbb9dabbd9d2b76caccb9efc13 Mon Sep 17 00:00:00 2001 From: Dave Date: Tue, 5 Mar 2024 08:13:34 -0500 Subject: [PATCH] Snap misc fixes (#1900) * set the app_id to undefined if empty. Set advertiser_tracking_enabled to 0 if the app_id is defined * add comment on advertiser_tracking_enabled field usage * tweak app_data and extinfo build * update box function to return undefined if the input string is empty or undefined instead of an empty array * fix tests * fix typo * simplify action_source logic * update tests to validate advertiser_tracking_enabled and changes to box function * cleaner implementation of emptyStringtoUndefined * simpler implementation of box * remove duplicate util function * fix function name * log errors as critical * add prefix to logger entries --------- Co-authored-by: David Bordoley --- .../_tests_/capiV3tests.ts | 25 +++--- .../reportConversionEvent/index.ts | 2 +- .../reportConversionEvent/snap-capi-v3.ts | 85 ++++++++++++------- .../reportConversionEvent/utils.ts | 9 +- 4 files changed, 73 insertions(+), 48 deletions(-) diff --git a/packages/destination-actions/src/destinations/snap-conversions-api/_tests_/capiV3tests.ts b/packages/destination-actions/src/destinations/snap-conversions-api/_tests_/capiV3tests.ts index c52234e38e..e1cca58775 100644 --- a/packages/destination-actions/src/destinations/snap-conversions-api/_tests_/capiV3tests.ts +++ b/packages/destination-actions/src/destinations/snap-conversions-api/_tests_/capiV3tests.ts @@ -82,7 +82,6 @@ export const capiV3tests = () => const { integration, event_name, event_time, user_data, custom_data, action_source, app_data } = data[0] const { em, ph } = user_data const { brands, content_category, content_ids, currency, num_items, value } = custom_data - const { app_id } = app_data expect(integration).toBe('segment') expect(event_name).toBe('PURCHASE') @@ -93,7 +92,8 @@ export const capiV3tests = () => expect(currency).toBe('USD') expect(value).toBe(15) expect(action_source).toBe('website') - expect(app_id).toBe('test123') + // app_data is only defined when action_source is app + expect(app_data).toBeUndefined() expect(brands).toEqual(['Hasbro', 'Mattel']) expect(content_category).toEqual(['games', 'games']) @@ -133,7 +133,6 @@ export const capiV3tests = () => data[0] const { client_ip_address, client_user_agent, em, ph } = user_data const { currency, value } = custom_data - const { app_id } = app_data expect(integration).toBe('segment') expect(event_name).toBe('PURCHASE') @@ -148,7 +147,8 @@ export const capiV3tests = () => expect(currency).toBe('USD') expect(value).toBe(15) expect(action_source).toBe('website') - expect(app_id).toBe('test123') + // app_data is only defined when action_source is app + expect(app_data).toBeUndefined() }) it('should fail web event without pixel_id', async () => { @@ -234,7 +234,6 @@ export const capiV3tests = () => data[0] const { client_ip_address, client_user_agent, em, ph } = user_data const { currency, value } = custom_data - const { app_id } = app_data expect(integration).toBe('segment') expect(event_name).toBe('SAVE') @@ -248,8 +247,10 @@ export const capiV3tests = () => expect(ph[0]).toBe('dc008fda46e2e64002cf2f82a4906236282d431c4f75e5b60bfe79fc48546383') expect(currency).toBe('USD') expect(value).toBe(15) - expect(action_source).toBe('other') - expect(app_id).toBe('test123') + expect(action_source).toBe('OFFLINE') + + // App data is only defined for app events + expect(app_data).toBeUndefined() }) it('should handle a mobile app event conversion type', async () => { @@ -288,6 +289,7 @@ export const capiV3tests = () => data[0] const { client_ip_address, client_user_agent, em, ph } = user_data const { currency, value } = custom_data + const { extinfo, advertiser_tracking_enabled } = app_data expect(integration).toBe('segment') expect(event_name).toBe('SAVE') @@ -302,7 +304,8 @@ export const capiV3tests = () => expect(currency).toBe('USD') expect(value).toBe(15) expect(action_source).toBe('app') - expect(app_data.extinfo).toEqual(['i2', '', '', '', '17.2', 'iPhone12,1', '', '', '', '', '', '', '', '', '', '']) + expect(extinfo).toEqual(['i2', '', '', '', '17.2', 'iPhone12,1', '', '', '', '', '', '', '', '', '', '']) + expect(advertiser_tracking_enabled).toBe(0) }) it('should fail invalid currency', async () => { @@ -393,7 +396,7 @@ export const capiV3tests = () => data[0] const { client_ip_address, client_user_agent, em, ph } = user_data const { currency, value } = custom_data - const { app_id } = app_data + const { app_id, advertiser_tracking_enabled } = app_data expect(integration).toBe('segment') expect(event_name).toBe('CUSTOM_EVENT_5') @@ -409,6 +412,7 @@ export const capiV3tests = () => expect(value).toBe(15) expect(action_source).toBe('app') expect(app_id).toBe('123') + expect(advertiser_tracking_enabled).toBe(0) }) it('should fail event missing all Snap identifiers', async () => { @@ -472,12 +476,13 @@ export const capiV3tests = () => expect(data.length).toBe(1) const { integration, event_name, event_time, user_data, action_source } = data[0] - const { em } = user_data + const { em, ph } = user_data expect(integration).toBe('segment') expect(event_name).toBe('PURCHASE') expect(event_time).toBe(1652368875449) expect(em[0]).toBe('cc779c04191c2e736d89e45c11339c8382832bcaf70383f7df94e3d08ba7a6d9') + expect(ph).toBeUndefined() expect(action_source).toBe('website') }) diff --git a/packages/destination-actions/src/destinations/snap-conversions-api/reportConversionEvent/index.ts b/packages/destination-actions/src/destinations/snap-conversions-api/reportConversionEvent/index.ts index e7d3d08548..c5558f5cdd 100644 --- a/packages/destination-actions/src/destinations/snap-conversions-api/reportConversionEvent/index.ts +++ b/packages/destination-actions/src/destinations/snap-conversions-api/reportConversionEvent/index.ts @@ -90,7 +90,7 @@ const action: ActionDefinition = { // record. Instead log the errors so that we can identify issues and resolve them. // FIXME: Should we add sampling here? - data.logger?.info(String(e)) + data.logger?.crit(`snap-capi-v3\n\n${String(e)}`) } })() ]) diff --git a/packages/destination-actions/src/destinations/snap-conversions-api/reportConversionEvent/snap-capi-v3.ts b/packages/destination-actions/src/destinations/snap-conversions-api/reportConversionEvent/snap-capi-v3.ts index 694fba5656..34e410b4e4 100644 --- a/packages/destination-actions/src/destinations/snap-conversions-api/reportConversionEvent/snap-capi-v3.ts +++ b/packages/destination-actions/src/destinations/snap-conversions-api/reportConversionEvent/snap-capi-v3.ts @@ -4,13 +4,13 @@ import { Settings } from '../generated-types' import { box, emptyObjectToUndefined, - emptyToUndefined, hash, hashEmailSafe, isNullOrUndefined, splitListValueToArray, raiseMisconfiguredRequiredFieldErrorIf, - raiseMisconfiguredRequiredFieldErrorIfNullOrUndefined + raiseMisconfiguredRequiredFieldErrorIfNullOrUndefined, + emptyStringToUndefined } from './utils' import { CURRENCY_ISO_4217_CODES } from '../snap-capi-properties' @@ -33,15 +33,22 @@ export const validatePayload = (payload: Payload): Payload => { const eventConversionTypeToActionSource: { [k in string]?: string } = { WEB: 'website', - MOBILE_APP: 'app' + MOBILE_APP: 'app', + + // Use the snap event_conversion_type for offline events + OFFLINE: 'OFFLINE' } const iosAppIDRegex = new RegExp('^[0-9]+$') export const formatPayload = (payload: Payload, settings: Settings, isTest = true): object => { - const action_source = eventConversionTypeToActionSource[payload.event_conversion_type] ?? 'other' + const app_id = emptyStringToUndefined(settings.app_id) + + // event_conversion_type is a required parameter whose value is enforced as + // always OFFLINE, WEB, or MOBILE_APP, so in practice action_source will always have a value. + const action_source = eventConversionTypeToActionSource[payload.event_conversion_type] - const event_id = emptyToUndefined(payload.client_dedup_id) + const event_id = emptyStringToUndefined(payload.client_dedup_id) // Removes all leading and trailing whitespace and converts all characters to lowercase. const email = hashEmailSafe(payload.email?.replace(/\s/g, '').toLowerCase()) @@ -70,9 +77,44 @@ export const formatPayload = (payload: Payload, settings: Settings, isTest = tru num_items: payload.number_items } - const { app_id } = settings + // FIXME: Ideally advertisers on iOS 14.5+ would pass the ATT_STATUS from the device. + // However the field is required for app events, so hardcode the value to false (0) + // for any events sent that include app_data. + const advertiser_tracking_enabled = !isNullOrUndefined(app_id) ? 0 : undefined const extInfoVersion = iosAppIDRegex.test((app_id ?? '').trim()) ? 'i2' : 'a2' + // extinfo needs to be defined whenever app_data is included in the data payload + const extinfo = !isNullOrUndefined(app_id) + ? [ + extInfoVersion, // required per spec version must be a2 for Android, must be i2 for iOS + '', // app package name + '', // short version + '', // long version + payload.os_version ?? '', // os version + payload.device_model ?? '', // device model name + '', // local + '', // timezone abbr + '', // carrier + '', //screen width + '', // screen height + '', // screen density + '', // cpu core + '', // external storage size + '', // freespace in external storage size + '' // device time zone + ] + : undefined + + // Only set app data for app events + const app_data = + action_source === 'app' + ? emptyObjectToUndefined({ + app_id, + advertiser_tracking_enabled, + extinfo + }) + : undefined + const result = { data: [ { @@ -100,37 +142,14 @@ export const formatPayload = (payload: Payload, settings: Settings, isTest = tru content_ids, currency: payload.currency, num_items, - order_id: emptyToUndefined(payload.transaction_id), + order_id: emptyStringToUndefined(payload.transaction_id), search_string: payload.search_string, sign_up_method: payload.sign_up_method, value: payload.price }), action_source, - - app_data: emptyObjectToUndefined({ - app_id, - extinfo: !isNullOrUndefined(payload.os_version ?? payload.device_model) - ? [ - extInfoVersion, // required per spec version must be a2 for Android, must be i2 for iOS - '', // app package name - '', // short version - '', // long version - payload.os_version ?? '', // os version - payload.device_model ?? '', // device model name - '', // local - '', // timezone abbr - '', // carrier - '', //screen width - '', // screen height - '', // screen density - '', // cpu core - '', // external storage size - '', // freespace in external storage size - '' // device time zone - ] - : undefined - }) + app_data } ], ...(isTest ? { test_event_code: 'segment_test' } : {}) @@ -141,8 +160,8 @@ export const formatPayload = (payload: Payload, settings: Settings, isTest = tru export const validateAppOrPixelID = (settings: Settings, event_conversion_type: string): string => { const { snap_app_id, pixel_id } = settings - const snapAppID = emptyToUndefined(snap_app_id) - const snapPixelID = emptyToUndefined(pixel_id) + const snapAppID = emptyStringToUndefined(snap_app_id) + const snapPixelID = emptyStringToUndefined(pixel_id) const appOrPixelID = snapAppID ?? snapPixelID raiseMisconfiguredRequiredFieldErrorIfNullOrUndefined(appOrPixelID, 'Missing valid app or pixel ID') diff --git a/packages/destination-actions/src/destinations/snap-conversions-api/reportConversionEvent/utils.ts b/packages/destination-actions/src/destinations/snap-conversions-api/reportConversionEvent/utils.ts index bd9fb51507..1948026c8d 100644 --- a/packages/destination-actions/src/destinations/snap-conversions-api/reportConversionEvent/utils.ts +++ b/packages/destination-actions/src/destinations/snap-conversions-api/reportConversionEvent/utils.ts @@ -30,9 +30,6 @@ export const transformProperty = ( export const hashEmailSafe = (email: string | undefined): string | undefined => isHashedEmail(String(email)) ? email : hash(email) -export const emptyToUndefined = (str: string | undefined): string | undefined => - str != null && str === '' ? undefined : str - export const raiseMisconfiguredRequiredFieldErrorIf = (condition: boolean, message: string) => { if (condition) { throw new IntegrationError(message, 'Misconfigured required field', 400) @@ -48,7 +45,8 @@ export const raiseMisconfiguredRequiredFieldErrorIfNullOrUndefined: S['raiseMisc (v: T | undefined, message: string): asserts v is T => raiseMisconfiguredRequiredFieldErrorIf(isNullOrUndefined(v), message) -export const box = (v: T | undefined): readonly T[] => (!isNullOrUndefined(v) ? [v] : []) +export const box = (v: string | undefined): readonly string[] | undefined => + (v ?? '').length > 0 ? [v as string] : undefined export const emptyObjectToUndefined = (v: { [k in string]?: unknown }) => { const properties = Object.getOwnPropertyNames(v) @@ -78,3 +76,6 @@ export const splitListValueToArray = (input: string): readonly string[] | undefi return result.length > 0 ? result : undefined } + +export const emptyStringToUndefined = (v: string | undefined): string | undefined => + (v ?? '').length > 0 ? v : undefined