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

Feature(IDE-1636): Scaffold StackAdapt action destination #1

Closed
wants to merge 5 commits into from

Conversation

AlliterativeAlice
Copy link

@AlliterativeAlice AlliterativeAlice commented Jan 11, 2024

Why?

IDE-1636

We currently have a destination function developed for Segment users to forward Segment events to the StackAdapt pixel endpoint, enabling advertisers who are users of both Segment and StackAdapt to track conversions based on Segment events without needing to install a StackAdapt pixel on their website. We want to convert this destination function into an action destination in Segment's catalog so that Segment users can more easily discover it and can use it without needing to manage code.

What

  • Set up "authentication" (entering Pixel ID) and configured fields to read from Segment.
  • Ported destination function code from https://github.com/StackAdapt/sa-tags/pull/5/files to convert read data fields into a request to the StackAdapt pixel endpoint.
  • Added unit tests as required by Segment

Testing

  • Added unit tests for new functionality
  • Tested end-to-end using the local server
  • [Segmenters] Tested in the staging environment

Notes

  • Failing checks are expected. They are failing because they are trying to run on our action environment (since this PR is against our fork) which doesn't have access to Segment's private packages.
  • Snapshot files are auto-generated by Segment

'@if': {
exists: { '@path': '$.traits.email' },
then: { '@path': '$.traits.email' },
else: { '@path': '$.properties.email' }
Copy link
Author

Choose a reason for hiding this comment

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

For PII fields we fallback to trying to load them from properties when they are not present in traits - this is different from the destination function code that only took them from traits. Let me know if there's a particular reason to read from traits only.

Copy link
Collaborator

@omfgitsjack omfgitsjack left a comment

Choose a reason for hiding this comment

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

lgtm aside from a question on the snapshot

Comment on lines +3 to +5
exports[`Testing snapshot for actions-stackadapt destination: forwardEvent action - all fields 1`] = `""`;

exports[`Testing snapshot for actions-stackadapt destination: forwardEvent action - required fields 1`] = `""`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does forwardActions have any required fields? The snapshot seems to be returning no data

Copy link
Author

Choose a reason for hiding this comment

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

There is one required field - user_id (which can be derived from either user ID or anonymous ID)

The snapshot files are all auto-generated by Segment. The data in the snapshot file is the expected request body and we don't have one because we make GET requests with all data passed in the query string.

Copy link
Collaborator

Choose a reason for hiding this comment

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

gotcha makes sense. would it be worthwhile snapshotting the expected get request with data in query string for some of the event types we explicitly support in our CDP ? I'm thinking it'd double as documentation for downstream teams that want to understand how we map segment events (i.e. the ecommerce events were supporting in Shopify v1 can be cross referenced with what's supported in segment) back into pixel service calls or alternatively we can maintain a separate guide/doc to explain the mapping strategy for different event types?

Copy link
Author

@AlliterativeAlice AlliterativeAlice Jan 23, 2024

Choose a reason for hiding this comment

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

No issue with putting the expected query strings for the index.test.ts tests into a snapshot file, but I don't think the tests or any other files in this repo should be used for the purpose of documentation for our internal teams because it belongs to Segment and we will always need their permission to merge any changes upstream.

Copy link
Author

Choose a reason for hiding this comment

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

On second thought I actually think it's safer to force developers to manually update the expected query params when they update the code rather than just regenerating a snapshot and hopefully reviewing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants