-
Notifications
You must be signed in to change notification settings - Fork 2
feat: SDKE-482 support custom product attributes #49
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: SDKE-482 support custom product attributes #49
Conversation
|
Can you add tests? |
src/FacebookEventForwarder.js
Outdated
| // Resolve a simple product key by checking top-level first, then Attributes bag | ||
| function getField(product, key) { | ||
| if (!product || !key) return undefined; | ||
| if (Object.prototype.hasOwnProperty.call(product, key)) { |
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.
It looks like this is prioritizing the value of the attribute at the Product level vs the Product.Attributes level. I don't think this matters because it is likely that if a customer puts a product.attribute that is the same as the Product level attribute, that the value would be the same.
Just to call out though - for the customer who requested this, the launchpad/chats say that they want to pass in the following:
product_type: product.custom_attributes.category
This is interesting, because category is actually one of our product level keys as well (see docs here). However, when you call mParticle.eCommerce.createProduct(), what is returned is a product with a Titlecase key, ie:
var product1 = mParticle.eCommerce.createProduct(
'Double Room - Econ Rate', // Name
'econ-1', // SKU
100.00, // Price
4, // Quantity
'variant', // Variant
'room', // Category
'lodge-o-rama', // Brand
'banner', // Position
null, // Coupon code
customProductAttributes // The custom attributes defined in a separate variable above
);
makes:
So if the customer puts category, since it's lowercase it will overlook Product.Category and then look at product.Attributes.category.
But I wouldn't expect it to be a different value at the end of the day. This comment is more digging in deeper as an FYI
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.
Thanks for the detailed explanation!
Our implementation prioritizes standard product fields over custom attributes in the mapping logic. The case-sensitive key matching naturally separates these two namespaces, allowing customers to map both without conflicts. I've added test coverage for both Product.Category and Product.Attributes.category to validate this behavior and ensure both mapping types work correctly.
// Check product field first
if (product.hasOwnProperty(sourceField)) {
value = product[sourceField];
}
// Then check custom attributes
else if (product.Attributes && product.Attributes[sourceField]) {
value = product.Attributes[sourceField];
}
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.
Added all my comments. Please review and lmk if you have any questions
test/tests.js
Outdated
| productAttributeMapping: JSON.stringify({ | ||
| 'Name': 'custom_name', | ||
| 'Brand': 'custom_brand', | ||
| 'Price': 'custom_price', | ||
| 'category': 'custom_attribute_category', | ||
| 'Category': 'custom_category', | ||
|
|
||
| }) |
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 use a real copied and pasted setting, like from https://jssdk-origin-qa1.qa.corp.mparticle.com/js/v2/9e77ff19b9fa2b4fa33fa0ef322addf4/config
"productAttributeMapping": "[{"jsmap":"3373707","map":"Name","maptype":"ProductAttributeSelector.Name","value":"test_name"},{"jsmap":"93997959","map":"Brand","maptype":"ProductAttributeSelector.Name","value":"test_brand"},{"jsmap":"3322014","map":"list","maptype":"ProductAttributeSelector.Name","value":"test_list"}]"
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.
You can change the map and value in there, but at least this way you have the structure of a rela string that is returned.
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 agree, and this helped with debugging as well! Updated the tests to use real settings.
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.
one minor comment
Co-authored-by: Robert Ing <rmi22186@gmail.com>
|
@jaissica12 - LGTM. Only thing is please change the title from |
Instructions
developmentSummary
Add Product Attribute Mapping for Facebook Purchase Events
- Implement
buildProductContents()to create nestedcontentsarray for Purchase events- Add
productAttributeMappingsetting for configurable field mapping- Map
TransactionIdtoorder_idparameter- Support mapping both product fields and custom attributes
- Add test coverage for the changes
Testing Plan
Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)