-
Notifications
You must be signed in to change notification settings - Fork 558
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
fix: dynamic messages - pass merchant id from cpnw callback, flush logger #2381
base: main
Are you sure you want to change the base?
Conversation
src/zoid/buttons/component.jsx
Outdated
const { message, buttonSessionID, currency, merchantID } = props; | ||
|
||
// override with server id if partner does not exist | ||
getLogger().addTrackingBuilder(() => ({ | ||
[FPTI_KEY.SELLER_ID]: merchantID?.toString() || serverMerchantId, |
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.
this already exists in sdk-client
: https://github.com/paypal/paypal-sdk-client/blob/main/src/tracking.js#L138
I would consider changing this to
if (serverMerchantId) {
getLogger().addTrackingBuilder(() => ({
[FPTI_KEY.SELLER_ID]: serverMerchantId,
}))
}
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 me get this added
src/zoid/buttons/util.js
Outdated
@@ -396,7 +396,7 @@ export const getModal: ( | |||
|
|||
return window[namespace].MessagesModal({ | |||
account: `client-id:${clientID}`, | |||
merchantId: merchantID?.join(",") || undefined, | |||
merchantId: merchantID?.join() || undefined, |
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.
did you mean to drop the ","
in the join?
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 think by default it makes it comma delimited but i can add it back in for clarity
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.
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 went ahead and added it back in just in case
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 addressed the comments
Description
we are passing merchantID from a callback handled in an internal service. We are also flushing the logger that way on page navigation we can send up logger events
Why are we making these changes? Include references to any related Jira tasks or GitHub Issues
Reproduction Steps (if applicable)
Screenshots (if applicable)
Dependent Changes (if applicable)
Groups who should review (if applicable)
❤️ Thank you!