-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Add User Filtering to Kit #9
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
feat: Add User Filtering to Kit #9
Conversation
BrandonStalnaker
left a comment
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.
The logic looks good to me
rmi22186
left a comment
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.
looks good. mostly nits. tests?
src/Rokt-Kit.js
Outdated
| service, | ||
| testMode, | ||
| trackerId, | ||
| _userAttributes |
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.
let's label this filteredUserAttributes so it's distinct (and that's what it is regardless of what this was originally named)
| _userAttributes | |
| filteredUserAttributes |
src/Rokt-Kit.js
Outdated
| self.filters = window.mParticle.Rokt.filters; | ||
|
|
||
| // Attaches the launcher and kit to the Rokt manager | ||
| window.mParticle.Rokt.attachLauncher(launcher); |
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.
we don't need to attach the launcher anymore here. instead it should still notify mParticle.Rokt that it is now available and the rokt kit is ready so that the core sdk can fire of the queued events
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.
seems like when you attach the kit it then calls processMessageQueue on the core SDK, so i think this is fine and we can remove attachLauncher completely.
src/Rokt-Kit.js
Outdated
|
|
||
| function selectPlacements(options) { | ||
| var attributes = (options && options.attributes) || {}; | ||
| var placementAttributes = mergeObjects(attributes, self.userAttributes); |
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 flip these, so that the attributes passed in from the customer override anything we have cached.
63f464b to
a791b35
Compare
Summary
Add User Filtering logic to Rokt Kit
Testing Plan
window.mParticle.Rokt.selectPlacementthat have been filtered via the mParticle UI and verify the filtered attributes are not passed through.