Skip to content

[MAIN] [STRATCONN-2841] Added event_id in page action of Pinterest Tag #801

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

Merged
merged 5 commits into from
Jun 3, 2024

Conversation

AnkitSegment
Copy link
Contributor

@AnkitSegment AnkitSegment commented May 30, 2024

What does this PR do?
JIRA TICKET: https://segment.atlassian.net/browse/STRATCONN-2841

Create mapping for Pinterest event_id as segment messageId.

  1. When mapMessageIdToEventId is disabled/false
Screenshot 2024-05-30 at 11 57 30 AM Screenshot 2024-05-30 at 11 56 12 AM
  1. When mapMessageIdToEventId is enabled/true
Screenshot 2024-05-30 at 11 29 53 AM Screenshot 2024-05-30 at 11 52 59 AM

Are there breaking changes in this PR?

Testing

  • Testing completed successfully using stage

Any background context you want to provide?

Is there parity with the server-side/android/iOS integration components (if applicable)?

Does this require a new integration setting? If so, please explain how the new setting works

Links to helpful docs and other external resources

Copy link
Contributor

@harsh-joshi99 harsh-joshi99 left a comment

Choose a reason for hiding this comment

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

LGTM ✨

name: page.name() || ''
};

var eventKeys = ['event_id', 'eid', 'eventID'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we search for eid, eventID these specific attributes? How did we decide these three values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@varadarajan-tw varadarajan-tw May 30, 2024

Choose a reason for hiding this comment

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

Okay. So, do we expect customers to provide these values in properties? Is this consistent with how we can handle track? Should we add supprot for eid, eventID in track as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, for track events, mappings are generated through the generatePropertiesObject function. Using forLoop, when the user adds any of these three event IDs, that value directly gets attached to the mappings.

Copy link
Contributor

@varadarajan-tw varadarajan-tw left a comment

Choose a reason for hiding this comment

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

Looks good. Left a few questions.

Also - could you bump the package version in package.json?

@varadarajan-tw varadarajan-tw self-requested a review June 3, 2024 07:43
@AnkitSegment AnkitSegment merged commit e09a394 into master Jun 3, 2024
@AnkitSegment AnkitSegment deleted the STRATCONN-2841-event-id-pinterest branch June 3, 2024 14:32
@AnkitSegment AnkitSegment restored the STRATCONN-2841-event-id-pinterest branch June 3, 2024 14:32
@AnkitSegment AnkitSegment deleted the STRATCONN-2841-event-id-pinterest branch June 3, 2024 14:32
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.

3 participants