segmentio
Added more optional parameters to Snap Conversions API destination
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_shahashed_middle_name_shahashed_last_name_shahashed_city_shahashed_state_shahashed_ziphashed_dob_monthhashed_dob_daycountryregion
(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) -
[x] 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 |
|---|---|
hi @sebashine-branly I see this is no longer a draft - are you ready for me to review it?
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.
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 [email protected] .
@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.
Thanks for implementing the feedback @sebashine-branly , and for pointing out the incorrect mapping suggestions!
Hi @dcasey-sc, cc @sebashine-branly ,
Now that we have https://github.com/segmentio/action-destinations/pull/1557 out of the way, would you be interested in getting this PR approved?
hi @sebashine-branly and @dcasey-sc, checking in on this again to see if you are interested in progressing with this change?
Hi @joe-ayoub-segment and @dcasey-sc, thanks for the follow-up. From what I can see, https://github.com/segmentio/action-destinations/pull/1557 is complimentary to this PR as they both add more properties but in a different scope: array of products for https://github.com/segmentio/action-destinations/pull/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!
ℹ️ Fixed the code conflict and verified that test suite is still ✅