-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Add placement event mapping for event attributes #57
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
base: development
Are you sure you want to change the base?
feat: Add placement event mapping for event attributes #57
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Expands the Rokt kit’s placement event mapping to also support mapping event attributes (with optional conditional operators) to Rokt local session attributes, while keeping the existing event-type/category/name mapping behavior intact.
Changes:
- Added
placementEventAttributeMappingLookupplus rule evaluation (exists/equals/contains) and application duringprocessEvent. - Parsed new
placementEventAttributeMappingsetting and exposed a test helper to build the lookup. - Added unit tests for lookup generation and conditional attribute mapping behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/Rokt-Kit.js |
Implements attribute-based mapping rules and applies them during event processing. |
test/src/tests.js |
Adds unit tests covering lookup generation and condition evaluation for attribute mappings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (operator === 'equals') { | ||
| return String(actualValue) === String(expectedValue); | ||
| } | ||
|
|
||
| if (operator === 'contains') { | ||
| return String(actualValue).indexOf(String(expectedValue)) !== -1; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would consider adding some additional test cases to add some potential edge cases. Something like 2 === "2" would likley pass as intended but what if you're testing true === 'True'or even"TRUE" === 'True'"`?
I think you have some guard clauses that should catch this, but also make sure something like "0" === "false" or "0" === "" is covered in tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Currently in the Rokt Kit, you can only map specific events (Event Type, Event Category, & Event Name) to a Rokt attribute.

This PR expands the existing functionality to map event attributes matching certain conditions to a Rokt attribute.
Example: "Events with url attribute containing
salewill map tosaleseekerRokt attribute.The existing functionality still works as expected. The conditions are stored in a separate object called placementEventAttributeMappingLookup. It will map an array of conditions to the mapped attribute key.
Testing Plan
Testing locally, unit tests, going to make changes to the connection level to add the JSON custom field to allow for the placement event mapping rules