Added more optional parameters to Snap Conversions API destination #1546
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,11 +26,22 @@ import { | |
| page_url, | ||
| sign_up_method, | ||
| formatPayload, | ||
| COUNTRY_ISO_3166_CODES, | ||
| CURRENCY_ISO_4217_CODES, | ||
| conversionType, | ||
| device_model, | ||
| os_version, | ||
| click_id, | ||
| first_name, | ||
| middle_name, | ||
| last_name, | ||
| city, | ||
| state, | ||
| zip, | ||
| dob_month, | ||
| dob_day, | ||
| country, | ||
| region | ||
| } from '../snap-capi-properties' | ||
|
|
||
| const CONVERSION_EVENT_URL = 'https://tr.snapchat.com/v2/conversion' | ||
|
|
@@ -65,7 +76,17 @@ const action: ActionDefinition<Settings, Payload> = { | |
| sign_up_method: sign_up_method, | ||
| os_version: os_version, | ||
| device_model: device_model, | ||
| click_id: click_id, | ||
| first_name: first_name, | ||
| middle_name: middle_name, | ||
| last_name: last_name, | ||
| city: city, | ||
| state: state, | ||
| zip: zip, | ||
| dob_month: dob_month, | ||
| dob_day: dob_day, | ||
| country: country, | ||
| region: region | ||
| }, | ||
| perform: (request, data) => { | ||
| if (data.payload.currency && !CURRENCY_ISO_4217_CODES.has(data.payload.currency.toUpperCase())) { | ||
|
|
@@ -76,6 +97,22 @@ const action: ActionDefinition<Settings, Payload> = { | |
| ) | ||
| } | ||
|
|
||
| if (data.payload.country && !COUNTRY_ISO_3166_CODES.has(data.payload.country)) { | ||
| throw new IntegrationError( | ||
|
There was a problem hiding this comment. Hi @sebashine-branly can we throw a PayloadValidationError error instead please? There was a problem hiding this comment. Same concern regarding the fact this field is not required, contrary to what is assumed by the comment above |
||
| `${data.payload.country} is not a valid country code. It must be provided as a two letter ISO 3166 alpha-2 country code.`, | ||
| 'Misconfigured optional field', | ||
| 400 | ||
| ) | ||
| } | ||
|
|
||
| if (data.payload.country === 'US' && data.payload.region && data.payload.region.length !== 2) { | ||
|
There was a problem hiding this comment. Can this be a PayloadValidationError please? There was a problem hiding this comment. Same concern regarding the fact this field is not required, contrary to what is assumed by the comment above There was a problem hiding this comment. However, I noticed that this other case could use |
||
| throw new IntegrationError( | ||
| `${data.payload.region} is not a valid region code. Given that country is US, region should be a two letter State code.`, | ||
| 'Misconfigured optional field', | ||
| 400 | ||
| ) | ||
| } | ||
|
|
||
| if ( | ||
| !data.payload.email && | ||
| !data.payload.phone_number && | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if for this message, we should send
Misconfigured optional fieldrather thanMisconfigured required field, as I thinkcurrencyisn't mandatory.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @sebashine-branly , yes I agree. The error can better be explained as "Misconfigured optional field".
However, could we throw a PayloadValidationError instead please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am happy to make the change but I have some concerns about the comment above the definition of
PayloadValidationError: https://github.com/segmentio/action-destinations/blob/main/packages/core/src/errors.ts#L76The field here is not required so I am not sure whether the new error type is fitting. I don't know if the comment should be updated instead, to also include the case of wrongly formatted optional parameters, but if needed I would prefer to have it in a separate PR as the contribution would be outside of the
snap-conversions-apifolder.https://github.com/segmentio/action-destinations/blob/main/CONTRIBUTING.md#submitting-changes-after-your-integration-is-already-live
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also noticed that
PayloadValidationErrorusesErrorCodes.PAYLOAD_VALIDATION_FAILEDbehind the scenes but it seems likeErrorCodes.INVALID_CURRENCY_CODEwould be more fitting.