feat: shopify: ecom fix and enhancement #2842
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #2842 +/- ##
===========================================
+ Coverage 87.20% 87.41% +0.20%
===========================================
Files 775 799 +24
Lines 28923 29596 +673
Branches 6788 6910 +122
===========================================
+ Hits 25222 25871 +649
- Misses 3359 3380 +21
- Partials 342 345 +3 ☔ View full report in Codecov by Sentry. |
| }); | ||
| return lineItems; | ||
| } | ||
| return 'EMPTY'; |
There was a problem hiding this comment.
Return null apart from a string.
There was a problem hiding this comment.
We decided to use this as this would be stored in redis.
We can easily differentiate and know is line_items is truly EMPTY or nor using this.
In current version as well we are doing the same
| updatedProduct.id = lineItemID; | ||
| events.push(this.ecomPayloadBuilder(updatedProduct, 'product_removed')); |
There was a problem hiding this comment.
Shouldn't we do a if condition check that updatedProduct id not present in prvious line items id list?
There was a problem hiding this comment.
This is from previouslineItems only.
First we are checking for the commmon ones and comparing the quantity to decide if its Product Added or Product Removed then we deleting that id from prev cart state (L190-l213)
Then we are checking for the items that were present in the previous cart only as common ones are already deleted from previous step (Product Removed)(L215- l222)
| } | ||
| if (events.length > 0) { | ||
| await this.updateCartState( | ||
| getLineItemsToStore(event), |
There was a problem hiding this comment.
This function is expecting a cart object but we are passing the entire event?
There was a problem hiding this comment.
For cart update event the whole event is cart object only
There was a problem hiding this comment.
Changed the param name
| }, | ||
| { | ||
| "sourceKeys": "tax_exempt", | ||
| "destKeys": "traits.taxExempt" |
There was a problem hiding this comment.
For all values which we are storing in traits should also be stored in context.traits
|
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThe changes involve updates to the Shopify source integration in the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ? TipsChat with CodeRabbit Bot (
|
| describe('setExtraNonEcomProperties() unit test cases', () => { | ||
| it('Non ecom event passed', () => { | ||
| const output = enrichPayload.setExtraNonEcomProperties({}, {}, 'non_ecom_event'); | ||
| expect(output).toEqual({}); // empty object as input would be returned as it is | ||
| }); | ||
|
|
||
| it('Valid Ecom Event -> Order Cancelled', () => { | ||
| const message = { | ||
| event: 'Order Cancelled', | ||
| properties: { | ||
| order_id: '1234', | ||
| total: 100, | ||
| revenue: 70, | ||
| tax: 30, | ||
| shipping: 10.2, | ||
| discount: 5, | ||
| coupon: 'code1, code2', | ||
| currency: 'USD', | ||
| products, | ||
| }, | ||
| }; | ||
|
|
||
| const expectedOutput = { | ||
| event: 'Order Cancelled', | ||
| properties: { | ||
| order_id: '1234', | ||
| total: 100, | ||
| revenue: 70, | ||
| tax: 30, | ||
| shipping_lines: [ | ||
| { id: 1, price: 7 }, | ||
| { id: 2, price: 3.2 }, | ||
| ], | ||
| shipping: 10.2, | ||
| discount: 5, | ||
| discount_codes: [{ code: 'code1' }, { code: 'code2' }], | ||
| coupon: 'code1, code2', | ||
| currency: 'USD', | ||
| extra_property: 'ExtraValue', | ||
| products, | ||
| }, | ||
| }; | ||
|
|
||
| const output = enrichPayload.setExtraNonEcomProperties( | ||
| message, | ||
| ordersCancelledWebhook, | ||
| 'orders_cancelled', | ||
| ); | ||
| expect(output).toEqual(expectedOutput); | ||
| }); |
There was a problem hiding this comment.
Can we add more edge case scenarios if possible (use codium)
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 23
Configuration used: CodeRabbit UI
Files ignored due to filter (14)
- src/util/redis/testData/shopify_v1_source.json
- src/v0/util/testdata/handleSourceKeysOperation.json
- src/v1/sources/shopify/data/CheckoutStartedConfig.json
- src/v1/sources/shopify/data/CheckoutStepCompletedConfig.json
- src/v1/sources/shopify/data/CheckoutStepViewedConfig.json
- src/v1/sources/shopify/data/OrderCancelledConfig.json
- src/v1/sources/shopify/data/OrderCompletedConfig.json
- src/v1/sources/shopify/data/OrderUpdatedConfig.json
- src/v1/sources/shopify/data/PaymentInfoEnteredConfig.json
- src/v1/sources/shopify/data/ProductAddedOrRemovedConfig.json
- src/v1/sources/shopify/data/identifyMapping.json
- src/v1/sources/shopify/data/lineItemsMapping.json
- test/mocks/data/sources/shopify_v1/response.json
- test/tests/data/shopify_v1.json
Files selected for processing (20)
- src/util/redis/redisConnector.test.js (4 hunks)
- src/v0/util/index.js (1 hunks)
- src/v1/sources/shopify/mocks/data.js (1 hunks)
- src/v1/sources/shopify/commonUtils.js (1 hunks)
- src/v1/sources/shopify/commonUtils.test.js (1 hunks)
- src/v1/sources/shopify/config.js (1 hunks)
- src/v1/sources/shopify/enrichmentLayer.js (1 hunks)
- src/v1/sources/shopify/enrichmentLayer.test.js (1 hunks)
- src/v1/sources/shopify/identifierEventsLayer.js (1 hunks)
- src/v1/sources/shopify/identifierEventsLayer.test.js (1 hunks)
- src/v1/sources/shopify/identifyEventsLayer.js (1 hunks)
- src/v1/sources/shopify/identifyEventsLayer.test.js (1 hunks)
- src/v1/sources/shopify/identityResolutionLayer.js (1 hunks)
- src/v1/sources/shopify/identityResolutionLayer.test.js (1 hunks)
- src/v1/sources/shopify/trackEventLayer.test.js (1 hunks)
- src/v1/sources/shopify/trackEventsLayer.js (1 hunks)
- src/v1/sources/shopify/transform.js (1 hunks)
- test/mocks/redis.js (2 hunks)
- test/tests/shopify_source.test.js (1 hunks)
- test/tests/shopify_v1_source.test.js (1 hunks)
Additional comments: 23
src/v1/sources/shopify/config.js (1)
- 83-83: Ensure that the
getMappingConfigfunction is robust and handles any potential errors, as it is critical for the correct mapping of Shopify events to RudderStack events.src/v1/sources/shopify/enrichmentLayer.test.js (1)
- 1-55: The test cases provided in the hunk appear to be correctly structured and logically sound. They test both the non-ecommerce event handling and the enrichment of valid e-commerce events, which aligns with the summary description of the changes made to the Shopify integration.
src/v1/sources/shopify/identifierEventsLayer.js (1)
- 20-42: The structure of the value being set in Redis is an array, which is atypical for Redis storage. Redis typically stores key-value pairs. Please verify if this is the intended structure for Redis storage in this context, as it may lead to unexpected behavior when retrieving and processing this data later.
src/v1/sources/shopify/identifyEventsLayer.js (1)
- 14-14: Verify that the assignment of
message.traitstocontext.traitsis intentional and thatmessage.traitsis populated with the correct data before this operation.src/v1/sources/shopify/identifyEventsLayer.test.js (1)
- 4-11: The test case provided ensures that the
identifyPayloadBuilderfunction creates a new Message object with the appropriate integration constant. This is a good practice to ensure that the function behaves as expected.src/v1/sources/shopify/identityResolutionLayer.js (3)
54-54: Verify that the
generateUUIDfunction generates a version 5 UUID to maintain consistency with the use ofv5UUIDs elsewhere in the code.64-64: Verify that the
v5.URLnamespace is appropriate for hashing thecartTokento generate theanonymousId.77-77: Verify that the
setPropertymethod exists on theupdatedMessageobject. If not, this should be corrected to use the appropriate method for setting properties.src/v1/sources/shopify/trackEventsLayer.js (10)
27-38: The function
getProductsListFromLineItemsis well-implemented and modular, extracting product information from line items and handling custom fields. This promotes code reusability and maintainability.41-52: The function
createPropertiesForEcomEventcorrectly maps the payload for e-commerce events based on the Shopify topic. It also handles line items when necessary, which is a good practice for data consistency.61-74: The function
ecomPayloadBuilderis responsible for creating the payload for e-commerce events. It ensures that properties are cleaned of undefined and null values and enriches the payload with additional properties. This is a good practice for data quality.83-108: The function
nonEcomPayloadBuilderbuilds payloads for non-e-commerce events. It correctly excludes certain keys from the event and adds product information when available. This separation of e-commerce and non-e-commerce event handling is good for maintainability.118-126: The function
getUpdatedEventNameForCheckoutUpateEventdetermines the correct event name based on the presence of certain fields in the event. This logic is clear and concise.137-160: The function
mapCustomerDetailsenriches the payload with customer details, including email, shipping and billing addresses, and user identifiers. It also resolves identifiers for tracking events. This is crucial for accurate event tracking and user identification.170-176: The function
getUpdatedProductPropertiesupdates product properties with the cart token and quantity. This is a simple and effective way to ensure product properties are up-to-date.188-243: The function
generateProductAddedAndRemovedEventscompares the current and previous cart states to generate product added or removed events. It also updates the cart state in Redis. This is a complex function that handles multiple scenarios and is central to the functionality of the integration.263-279: The function
updateCartStateupdates the cart state in Redis. It includes error handling and logging, which is important for debugging and monitoring.282-316: The function
processTrackEventorchestrates the processing of track events, handling different scenarios such as cart updates and checkout updates. It ensures that the correct payload is built and customer details are mapped. This function is the entry point for event processing and is well-structured.test/__mocks__/redis.js (2)
3-7: The addition of "shopify_v1_test" to the
directoryMapobject is consistent with the summary.25-26: The expansion of the
setCallKeysForErrorarray to include "shopify_v1_test_set_map_fail" and "shopify_v1_test_set_redis_error" is consistent with the summary.test/__tests__/shopify_source.test.js (2)
3-3: The summary from the diff mentions the removal of the
versionconstant fromredisConnector.test.jsandredis.js, but it is still present in this file. Please verify if this is intentional or an oversight.8-8: The dynamic path construction for requiring the transformer module using the
versionvariable is a good approach for handling different versions of the integration.test/__tests__/shopify_v1_source.test.js (1)
- 1-33: The test setup and cases in
shopify_v1_source.test.jsappear to be correctly implemented and should effectively test the Shopify v1 source transformation logic.
| case 'concatenationInArray': { | ||
| const { propertyKey, arrayKey } = args[0]; | ||
| const arrayValues = get(message, arrayKey); | ||
| if (lodash.isArray(arrayValues) && isNotEmpty(arrayValues)) { | ||
| result = arrayValues.map((item) => get(item, propertyKey, '')).join(', '); | ||
| return result; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
In the 'concatenationInArray' case, similar to 'additionInArray', it would be prudent to add error handling to ensure that the propertyKey exists within the items of the array and that the array itself is present in the message. This will prevent the function from returning incorrect results or failing silently.
+ // Handle cases where arrayKey is not present or not an array
+ else {
+ throw new Error(`arrayKey not found or is not an array: ${arrayKey}`);
+ }Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| case 'concatenationInArray': { | |
| const { propertyKey, arrayKey } = args[0]; | |
| const arrayValues = get(message, arrayKey); | |
| if (lodash.isArray(arrayValues) && isNotEmpty(arrayValues)) { | |
| result = arrayValues.map((item) => get(item, propertyKey, '')).join(', '); | |
| return result; | |
| } | |
| return null; | |
| case 'concatenationInArray': { | |
| const { propertyKey, arrayKey } = args[0]; | |
| const arrayValues = get(message, arrayKey); | |
| if (lodash.isArray(arrayValues) && isNotEmpty(arrayValues)) { | |
| result = arrayValues.map((item) => get(item, propertyKey, '')).join(', '); | |
| return result; | |
| } | |
| // Handle cases where arrayKey is not present or not an array | |
| else { | |
| throw new Error(`arrayKey not found or is not an array: ${arrayKey}`); | |
| } | |
| return null; |
There was a problem hiding this comment.
Looks like humans are talking. I'll be quiet for now. If you need me again, just tag @coderabbitai in a new comment.
| }); | ||
| return lineItems; | ||
| } | ||
| return 'EMPTY'; |
There was a problem hiding this comment.
We decided to use this as this would be stored in redis.
We can easily differentiate and know is line_items is truly EMPTY or nor using this.
In current version as well we are doing the same
| updatedProduct.id = lineItemID; | ||
| events.push(this.ecomPayloadBuilder(updatedProduct, 'product_removed')); |
There was a problem hiding this comment.
This is from previouslineItems only.
First we are checking for the commmon ones and comparing the quantity to decide if its Product Added or Product Removed then we deleting that id from prev cart state (L190-l213)
Then we are checking for the items that were present in the previous cart only as common ones are already deleted from previous step (Product Removed)(L215- l222)
| } | ||
| if (events.length > 0) { | ||
| await this.updateCartState( | ||
| getLineItemsToStore(event), |
There was a problem hiding this comment.
For cart update event the whole event is cart object only
| case 'concatenationInArray': { | ||
| const { propertyKey, arrayKey } = args[0]; | ||
| const arrayValues = get(message, arrayKey); | ||
| if (lodash.isArray(arrayValues) && isNotEmpty(arrayValues)) { | ||
| result = arrayValues.map((item) => get(item, propertyKey, '')).join(', '); | ||
| return result; | ||
| } | ||
| return null; |
chore: add date mock to fix timestamp validation
* feat: initial commit * feat: making sure existing functionality is intact * fix: edits for exclusion keys * fix: edits for supporting property paths * fix: delete wrong test case * fix: test cases * fix: removed unnecessary code * fix: adding unit test cases for trimTraits * fix: changing the order of priority in property mapping * fix: edited distinct id logic * fix: small edit * fix: review comments addressed * fix: adding dedicated mappingJson for setOnce * fix: adding all the fields to the dedicated json * fix: addressing review comments * feat: review comments addressed
Co-authored-by: Sudip Paul <[email protected]>
Co-authored-by: Sudip Paul <[email protected]>
Co-authored-by: Sudip Paul <[email protected]>
* feat: update facebook destinations API version to v18.0 * feat: updated fb_pixel tests to pick version dynamically from config.js * feat: updated fb tests to pick version dynamically from config.js * feat: updated fb_custom_audience tests to pick version dynamically from config.js
* feat: cm360 transformerproxy, V1 features.json update * feat: add support for both v0,v1 in handler for old server support * fix: addressed comments * chore: added tests for V1 too
* fix: encode &, < and > to html counterparts in adobe * fix: use encodeurl on url valuez
* fix: timestamp microseconds input cm360 * feat: batching in cm360 * chore: addressed comments * chore: addressed comments * fix: return aborted inside cath block * feat: update batching partial events * feat: update batching partial events * feat: update batching partial events * feat: update batching partial events * chore: removed hardcoded response * feat: added new error class * chore: develop merge * feat: handle metadata as obj * feat: handle metadata as obj * chore: removed commented code * fix: merged develop * fix: merged develop * chore: added tests for batching * chore: added tests for batching
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| } | ||
| const metricMetadata = { | ||
| writeKey: event.query_parameters?.writeKey?.[0], | ||
| sourceId: source.ID, |
There was a problem hiding this comment.
For writeKey why not use the source object?
There was a problem hiding this comment.
We will need to change it for all metrics then I will change it
anantjain45823
force-pushed
the
feat.shopifyEcom
branch
from
6773d66 to
0cda8d1
Compare
|
Kudos, SonarCloud Quality Gate passed! |
|
This PR is considered to be stale. It has been open for 20 days with no further activity thus it is going to be closed in 7 days. To avoid such a case please consider removing the stale label manually or add a comment to the PR. |
Description of the change
Resolve INT-360
We will provide a toggle in the UI to use the new or old version.
With this old version would be deprecated.
Events changed (split/merged):
Events Added:
Product Added
Product Removed
Type of change
Related issues
Checklists
Development
Code review
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Tests
Chores