Added more optional parameters to Snap Conversions API destination #1546

sebashine-branly · 2023-09-04T15:35:05Z

Following the work that has been done in this other PR, this new PR adds more optional fields for the following Destination: Snapchat Conversions API (Actions).

It is based on the official documentation from Snapchat: https://marketingapi.snapchat.com/docs/conversion.html#conversion-parameters

More specifically, it adds the following parameters:

hashed_first_name_sha
hashed_middle_name_sha
hashed_last_name_sha
hashed_city_sha
hashed_state_sha
hashed_zip
hashed_dob_month
hashed_dob_day
country
region

(Command yarn types has been run)

This PR adds checks around the usage of country and/or region based on the rules given by Snapchat.

🐛 This PR also fixes a bug around the usage of idfv being turned into hashed_idfv, as it should be normalized (lowercased) before being hashed (like it's the case for hashed_mobile_ad_id).

Given that the Destination is currently active in Production, the following guide has been followed: https://github.com/segmentio/action-destinations/blob/main/CONTRIBUTING.md#submitting-changes-after-your-integration-is-already-live

Documentation

For quick documentation through test purposes, the hash values in the unit tests correspond to those present in the Snapchat documentation

Name of param	Raw string value	Hashed value
`hashed_first_name_sha`	`John`	`a8cfcd74832004951b4408cdb0a5dbcd8c7e52d43f7fe244bf720582e05241da`
`hashed_middle_name_sha`	`Middle`	`d93006ec2e4339d770a7afd068c1f1e789a52df12f595e529fd0f302fc1e5ec7`
`hashed_last_name_sha`	`Doe`	`fd53ef835b15485572a6e82cf470dcb41fd218ae5751ab7531c956a2a6bcd3c7`
`hashed_city_sha`	`Santa Monica`	`ced2e77f732fbf327f53e1f9748078c778b190ed9cf376a7df469a92d2ad62d3`
`hashed_state_sha`	`CA`	`4b650e5c4785025dee7bd65e3c5c527356717d7a1c0bfef5b4ada8ca1e9cbe17`
`hashed_zip`	`90405`	`e222c384dd83ac669bcd1da281ffea2e60bab298f8c0673d35bc0b704e345282`
`hashed_dob_month`	`January`	`37082e68df858e0ba76442174128811135890ae4c2c5df8b6f31aef5885d0be7`
`hashed_dob_day`	`26`	`5f9c4ab08cac7457e9111a30e4664920607ea2c115a1433d7be98e97e64244ca`

Limitations

This PR does not implement the five parameters finishing with _sdx suffix (Soundex). In the future, if a PR does so, it will have to check for the existence of one parameter (_sha suffix) or the other (_sdx), but not both. For instance, if a PR implements hashed_first_name_sdx in the future, said PR will have to make sure it is not being used at the same time as hashed_first_name_sha.

Testing

Include any additional information about the testing you have completed to
ensure your changes behave as expected. For a speedy review, please check
any of the tasks you completed below during your testing.

For unit tests the following commands have been run:

yarn cloud test --testPathPattern=src/destinations/snap-conversions-api
yarn cloud test --testPathPattern=src/destinations/snap-conversions-api -u (for updating the snapshots)
Added unit tests for new functionality
Tested end-to-end using the local server
[Segmenters] Tested in the staging environment

While I didn't fully test the E2E flow by sending the event onto Snapchat, I ran the Actions Tester to verify two points:

By default, there is no path set for the new params	Setting a path successfully adds the params to the event

…h hashed_dob_day

sebashine-branly · 2023-09-04T16:12:50Z

...ges/destination-actions/src/destinations/snap-conversions-api/reportConversionEvent/index.ts

+    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())) {


I wonder if for this message, we should send Misconfigured optional field rather than Misconfigured required field, as I think currency isn't mandatory.

hi @sebashine-branly , yes I agree. The error can better be explained as "Misconfigured optional field".

However, could we throw a PayloadValidationError instead please?

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#L76

Error to indicate the payload is missing fields that are required.

The 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-api folder.

https://github.com/segmentio/action-destinations/blob/main/CONTRIBUTING.md#submitting-changes-after-your-integration-is-already-live

Do not raise a PR containing changes for more than 1 Integration. For example if you need to make changes to a Cloud Mode and Device Mode Integration you should raise separate PRs.

I also noticed that PayloadValidationError uses ErrorCodes.PAYLOAD_VALIDATION_FAILED behind the scenes but it seems like ErrorCodes.INVALID_CURRENCY_CODE would be more fitting.

joe-ayoub-segment · 2023-09-05T10:02:35Z

hi @sebashine-branly I see this is no longer a draft - are you ready for me to review it?

sebashine-branly · 2023-09-05T10:25:48Z

Hi @joe-ayoub-segment , thank you for your quick reply. Yes, this PR is ready for a review when you get a chance. Thank you for your help.

joe-ayoub-segment · 2023-09-08T11:27:47Z

hi @sebashine-branly I'm about to review your PR. Can you please provide proof that you are from Snap please, or that you have authorization for this change? you can email me at joe.ayoub@segment.com .

joe-ayoub-segment

hi @sebashine-branly , PR looks really good! Few minor additions needed.

joe-ayoub-segment · 2023-09-08T11:37:34Z

...ges/destination-actions/src/destinations/snap-conversions-api/reportConversionEvent/index.ts

+    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())) {


hi @sebashine-branly , yes I agree. The error can better be explained as "Misconfigured optional field".

However, could we throw a PayloadValidationError instead please?

joe-ayoub-segment · 2023-09-08T11:39:07Z

...ges/destination-actions/src/destinations/snap-conversions-api/reportConversionEvent/index.ts

@@ -76,6 +97,22 @@ const action: ActionDefinition<Settings, Payload> = {
      )
    }

+    if (data.payload.country && !COUNTRY_ISO_3166_CODES.has(data.payload.country)) {
+      throw new IntegrationError(


Hi @sebashine-branly can we throw a PayloadValidationError error instead please?

Same concern regarding the fact this field is not required, contrary to what is assumed by the comment above PayloadValidationError definition.

joe-ayoub-segment · 2023-09-08T11:40:54Z

...ges/destination-actions/src/destinations/snap-conversions-api/reportConversionEvent/index.ts

+      )
+    }
+
+    if (data.payload.country === 'US' && data.payload.region && data.payload.region.length !== 2) {


Can this be a PayloadValidationError please?

Same concern regarding the fact this field is not required, contrary to what is assumed by the comment above PayloadValidationError definition.

However, I noticed that this other case could use PayloadValidationError without any confusion as it is really about required fields, if you agree?
https://github.com/segmentio/action-destinations/blob/main/packages/destination-actions/src/destinations/snap-conversions-api/reportConversionEvent/index.ts#L85

packages/destination-actions/src/destinations/snap-conversions-api/snap-capi-properties.ts

sebashine-branly · 2023-09-08T14:00:21Z

@joe-ayoub-segment thank you very much for your help for this contribution. I sent you an email and wrote a few comments here on GitHub to share a few concerns about the suggested changes. Based on your opinion, I'm happy to perform any change.

joe-ayoub-segment · 2023-09-08T14:08:43Z

Thanks for implementing the feedback @sebashine-branly , and for pointing out the incorrect mapping suggestions!

joe-ayoub-segment · 2023-10-03T10:52:53Z

Hi @dcasey-sc, cc @sebashine-branly ,

Now that we have #1557 out of the way, would you be interested in getting this PR approved?

joe-ayoub-segment · 2023-10-10T12:42:55Z

hi @sebashine-branly and @dcasey-sc, checking in on this again to see if you are interested in progressing with this change?

sebashine-branly · 2023-10-13T09:36:59Z

Hi @joe-ayoub-segment and @dcasey-sc, thanks for the follow-up.
From what I can see, #1557 is complimentary to this PR as they both add more properties but in a different scope: array of products for #1557 whereas this PR adds more customer information.
For this reason I'd still be interested in getting this PR approved. I'm planning to solve the code conflict either today or early next week.
Thank you!

sebashine-branly · 2023-10-13T13:51:57Z

ℹ️ Fixed the code conflict and verified that test suite is still ✅

sebashine-branly added 8 commits September 1, 2023 17:52

feat: add hashed_first_name_sha

c1631d5

feat: add hashed_middle_name_sha

0fac111

feat: add hashed_last_name_sha

2dbe361

feat: add hashed_city_sha hashed_state_sha hashed_zip hashed_dob_mont…

fa6035e

…h hashed_dob_day

fix: lowercase idfv

02d267c

feat: add country region

ad395d1

feat: add validation and tests for region

a4af32b

feat: add validation and tests for country

07a16df

sebashine-branly commented Sep 4, 2023

View reviewed changes

joe-ayoub-segment added not-partner-eng-review (cloud) and removed not-partner-eng-review (cloud) labels Sep 4, 2023

joe-ayoub-segment self-assigned this Sep 4, 2023

joe-ayoub-segment added the partner-eng-review (cloud) label Sep 4, 2023

sebashine-branly mentioned this pull request Sep 4, 2023

Added more hashed parameters to list for Snapchat Conversions API (Actions) segmentio/segment-docs#5294

Closed

feat: add self-review

2f81842

sebashine-branly marked this pull request as ready for review September 5, 2023 08:18

sebashine-branly requested review from a team as code owners September 5, 2023 08:18

joe-ayoub-segment requested changes Sep 8, 2023

View reviewed changes

sebashine-branly commented Sep 8, 2023

View reviewed changes

packages/destination-actions/src/destinations/snap-conversions-api/snap-capi-properties.ts Show resolved Hide resolved

sebashine-branly added 2 commits September 8, 2023 17:32

feat: include feedback regarding default rules

dd56b56

test: use default values

aaad210

sebashine-branly requested a review from joe-ayoub-segment September 8, 2023 15:51

Merge branch 'main' into snap-conversions-api-additional-params

8564c68

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added more optional parameters to Snap Conversions API destination #1546

Added more optional parameters to Snap Conversions API destination #1546

sebashine-branly commented Sep 4, 2023 •

edited

sebashine-branly Sep 4, 2023

joe-ayoub-segment Sep 8, 2023

sebashine-branly Sep 8, 2023

sebashine-branly Sep 8, 2023

joe-ayoub-segment commented Sep 5, 2023

sebashine-branly commented Sep 5, 2023

joe-ayoub-segment commented Sep 8, 2023

joe-ayoub-segment left a comment

joe-ayoub-segment Sep 8, 2023

joe-ayoub-segment Sep 8, 2023

sebashine-branly Sep 8, 2023

joe-ayoub-segment Sep 8, 2023

sebashine-branly Sep 8, 2023

sebashine-branly Sep 8, 2023

sebashine-branly commented Sep 8, 2023

joe-ayoub-segment commented Sep 8, 2023

joe-ayoub-segment commented Oct 3, 2023

joe-ayoub-segment commented Oct 10, 2023

sebashine-branly commented Oct 13, 2023 •

edited

sebashine-branly commented Oct 13, 2023

Added more optional parameters to Snap Conversions API destination #1546

Are you sure you want to change the base?

Added more optional parameters to Snap Conversions API destination #1546

Conversation

sebashine-branly commented Sep 4, 2023 • edited

Documentation

Limitations

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joe-ayoub-segment commented Sep 5, 2023

sebashine-branly commented Sep 5, 2023

joe-ayoub-segment commented Sep 8, 2023

joe-ayoub-segment left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebashine-branly commented Sep 8, 2023

joe-ayoub-segment commented Sep 8, 2023

joe-ayoub-segment commented Oct 3, 2023

joe-ayoub-segment commented Oct 10, 2023

sebashine-branly commented Oct 13, 2023 • edited

sebashine-branly commented Oct 13, 2023

sebashine-branly commented Sep 4, 2023 •

edited

sebashine-branly commented Oct 13, 2023 •

edited