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

PE-88 Snap products - add product array support for Snapchat Conversions API #1557

Merged
merged 17 commits into from
Sep 26, 2023

Conversation

joe-ayoub-segment
Copy link
Contributor

Update the Snapchat Conversions API Destination so that it can handle a Segment products array.

Specifically a new array field added to support this type of structure:

"products": {
{"product_id": "12345", "price": 100, "category": "toys", "brand": "Hasbro", "quantity": 5},
{"product_id": "98765", "price": 76, "category": "games", "brand": "Mattel", "quantity": 2},
}

Testing

Tested using the Actions Tester connected to Snapchat. Test results here.

Unit test added to test that arrays of products are accepted.

@joe-ayoub-segment joe-ayoub-segment requested a review from a team as a code owner September 6, 2023 11:24
@joe-ayoub-segment joe-ayoub-segment requested a review from a team September 6, 2023 11:24
@joe-ayoub-segment joe-ayoub-segment self-assigned this Sep 6, 2023
@joe-ayoub-segment joe-ayoub-segment changed the title Snap products - add product array support for Snapchat Conversions API PE-88 Snap products - add product array support for Snapchat Conversions API Sep 6, 2023
Copy link

@dcasey-sc dcasey-sc left a comment

Choose a reason for hiding this comment

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

Hey Joe, thanks for all the hard work on this. Just a few nitpicks regarding code style.

Comment on lines 438 to 442
let item_ids: string | undefined = undefined
let number_items: string | undefined = undefined
let item_category: string | undefined = undefined
let price: string | undefined = undefined
let brands: string[] | undefined = undefined

Choose a reason for hiding this comment

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

This should have the same impact of the current code but a bit more simple. Specifically removing the need for manual type specification and later nullish coalescing. All of these items would still be replaced if payload?.products is an array with non-zero length.

Suggested change
let item_ids: string | undefined = undefined
let number_items: string | undefined = undefined
let item_category: string | undefined = undefined
let price: string | undefined = undefined
let brands: string[] | undefined = undefined
let item_ids = payload?. item_ids
let number_items = payload?. number_items
let item_category = payload?.item_category
let price = payload?.price
let brands = payload?.brands

Comment on lines 471 to 472
number_items: number_items ?? payload?.number_items,
price: price ?? payload?.price,

Choose a reason for hiding this comment

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

Continuation of lines 438-442 suggestion.

Suggested change
number_items: number_items ?? payload?.number_items,
price: price ?? payload?.price,
number_items,
price,

@@ -331,6 +406,18 @@ export const hash = (value: string | undefined): string | undefined => {

const isHashedEmail = (email: string): boolean => new RegExp(/[0-9abcdef]{64}/gi).test(email)

// eslint-disable-next-line
const transformProperty = (property: string, items: Array<any>): string =>

Choose a reason for hiding this comment

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

Should be able to do items: Array<Record<string, string | number | undefined>> to avoid disabling the eslint rule.

@dcasey-sc
Copy link

@joe-ayoub-segment given that none of the comments above are logical issues we can handle them in another PR so we don't block the customer if desired. Let me know what you think.

@joe-ayoub-segment
Copy link
Contributor Author

@joe-ayoub-segment given that none of the comments above are logical issues we can handle them in another PR so we don't block the customer if desired. Let me know what you think.

Thanks @dcasey-sc , let's go ahead with the PR then, thanks for verifying everything!!

@joe-ayoub-segment
Copy link
Contributor Author

Testing after adding new field allow price calculation to be toggled

TEST 1: calculates price from product array details.
image
image

@joe-ayoub-segment
Copy link
Contributor Author

TEST 2: takes price from "Price" field
image

@joe-ayoub-segment
Copy link
Contributor Author

TEST 3: excluding product array falls back to using individual fields
image
image

Copy link

@dcasey-sc dcasey-sc left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @joe-ayoub-segment

@joe-ayoub-segment joe-ayoub-segment merged commit 0df35e7 into main Sep 26, 2023
5 of 6 checks passed
@joe-ayoub-segment joe-ayoub-segment deleted the snap-products branch September 26, 2023 09:18
@joe-ayoub-segment
Copy link
Contributor Author

Hi @dcasey-sc this PR has been deployed!

wpride pushed a commit to wpride/action-destinations that referenced this pull request Oct 12, 2023
…ons API (segmentio#1557)

* adding snap product array support

* test and fix type

* more changes

* fixing test

* minor changes post testing

* minor changes

* minor changes

* type description  changes

* addressing linting issue

* minor recommended changes

* adding options for how to track purchase price

* label changes

* making function more strongly typed

* adding tests

* Updating mappings for Price field

Updating mappings for Price field to match Segment Spec

* changing price and number_items

* removing files which were added by accident
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants