feat: criteo add support for multiple hash methods #1812
Conversation
WalkthroughWalkthroughThe recent changes introduce a new function, Changes
Possibly related 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 your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1812 +/- ##
===========================================
+ Coverage 56.67% 56.70% +0.02%
===========================================
Files 473 473
Lines 16108 16118 +10
Branches 3223 3224 +1
===========================================
+ Hits 9129 9139 +10
+ Misses 5735 5717 -18
- Partials 1244 1262 +18 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/analytics-js-integrations/src/integrations/Criteo/utils.js (3 hunks)
Additional context used
Learnings (2)
Common learnings
Learnt from: pierreavizou PR: rudderlabs/rudder-sdk-js#1613 File: packages/analytics-js-integrations/src/integrations/Criteo/utils.js:36-42 Timestamp: 2024-03-27T17:02:27.599Z Learning: The implementation for handling SHA256 email hashes in the Criteo integration sends both md5 and sha256 hashes regardless of the `hashMethod` selection, as per Criteo's recommendation to comply with new regulations and ensure compatibility during their system transition.packages/analytics-js-integrations/src/integrations/Criteo/utils.js (1)
Learnt from: pierreavizou PR: rudderlabs/rudder-sdk-js#1613 File: packages/analytics-js-integrations/src/integrations/Criteo/utils.js:36-42 Timestamp: 2024-03-27T17:02:27.599Z Learning: The implementation for handling SHA256 email hashes in the Criteo integration sends both md5 and sha256 hashes regardless of the `hashMethod` selection, as per Criteo's recommendation to comply with new regulations and ensure compatibility during their system transition.
GitHub Check: codecov/patch
packages/analytics-js-integrations/src/integrations/Criteo/utils.js
[warning] 37-37: packages/analytics-js-integrations/src/integrations/Criteo/utils.js#L37
Added line #L37 was not covered by tests
[warning] 40-40: packages/analytics-js-integrations/src/integrations/Criteo/utils.js#L40
Added line #L40 was not covered by tests
[warning] 44-44: packages/analytics-js-integrations/src/integrations/Criteo/utils.js#L44
Added line #L44 was not covered by tests
Additional comments not posted (2)
packages/analytics-js-integrations/src/integrations/Criteo/utils.js (2)
33-56: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
getEmailHashesare correctly implemented in the codebase.Verification successful
The function usage of
getEmailHashesis correctly implemented.
- The function is used in
packages/analytics-js-integrations/src/integrations/Criteo/utils.jsand matches the new signature.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `getEmailHashes` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type python -A 5 $'getEmailHashes'Length of output: 69
Script:
#!/bin/bash # Description: Verify all function calls to `getEmailHashes` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type js -A 5 'getEmailHashes'Length of output: 1240
Tools
GitHub Check: codecov/patch
[warning] 37-37: packages/analytics-js-integrations/src/integrations/Criteo/utils.js#L37
Added line #L37 was not covered by tests
[warning] 40-40: packages/analytics-js-integrations/src/integrations/Criteo/utils.js#L40
Added line #L40 was not covered by tests
[warning] 44-44: packages/analytics-js-integrations/src/integrations/Criteo/utils.js#L44
Added line #L44 was not covered by tests
77-78: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
handleCommonFieldsare correctly implemented in the codebase.Verification successful
Let's correct the file type and re-run the script to verify the function calls to
handleCommonFields.
LGTM! All function calls to
handleCommonFieldsare correctly implemented.The function calls to
handleCommonFieldsmatch the new signature and are consistent throughout the codebase.
packages/analytics-js-integrations/src/integrations/Criteo/browser.jspackages/analytics-js-integrations/__tests__/integrations/Criteo/utils.test.jsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `handleCommonFields` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type python -A 5 $'handleCommonFields'Length of output: 73
Script:
#!/bin/bash # Description: Verify all function calls to `handleCommonFields` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type js -A 5 $'handleCommonFields'Length of output: 5734
size-limit report 📦
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/analytics-js-integrations/tests/integrations/Criteo/utils.test.js (2 hunks)
- packages/analytics-js-integrations/src/integrations/Criteo/utils.js (4 hunks)
Additional context used
Learnings (3)
Common learnings
Learnt from: pierreavizou PR: rudderlabs/rudder-sdk-js#1613 File: packages/analytics-js-integrations/src/integrations/Criteo/utils.js:36-42 Timestamp: 2024-03-27T17:02:27.599Z Learning: The implementation for handling SHA256 email hashes in the Criteo integration sends both md5 and sha256 hashes regardless of the `hashMethod` selection, as per Criteo's recommendation to comply with new regulations and ensure compatibility during their system transition.packages/analytics-js-integrations/__tests__/integrations/Criteo/utils.test.js (1)
Learnt from: pierreavizou PR: rudderlabs/rudder-sdk-js#1613 File: packages/analytics-js-integrations/src/integrations/Criteo/utils.js:36-42 Timestamp: 2024-03-27T17:02:27.599Z Learning: The implementation for handling SHA256 email hashes in the Criteo integration sends both md5 and sha256 hashes regardless of the `hashMethod` selection, as per Criteo's recommendation to comply with new regulations and ensure compatibility during their system transition.packages/analytics-js-integrations/src/integrations/Criteo/utils.js (1)
Learnt from: pierreavizou PR: rudderlabs/rudder-sdk-js#1613 File: packages/analytics-js-integrations/src/integrations/Criteo/utils.js:36-42 Timestamp: 2024-03-27T17:02:27.599Z Learning: The implementation for handling SHA256 email hashes in the Criteo integration sends both md5 and sha256 hashes regardless of the `hashMethod` selection, as per Criteo's recommendation to comply with new regulations and ensure compatibility during their system transition.
GitHub Check: codecov/patch
packages/analytics-js-integrations/src/integrations/Criteo/utils.js
[warning] 40-40: packages/analytics-js-integrations/src/integrations/Criteo/utils.js#L40
Added line #L40 was not covered by tests
[warning] 44-44: packages/analytics-js-integrations/src/integrations/Criteo/utils.js#L44
Added line #L44 was not covered by tests
Additional comments not posted (5)
packages/analytics-js-integrations/__tests__/integrations/Criteo/utils.test.js (3)
Line range hint
9-34:
LGTM! The test cases forhandleCommonFieldsare comprehensive.The test cases cover different scenarios for various hash methods, ensuring the function behaves as expected.
Line range hint
36-182:
LGTM! The test cases forhandleProductAddedare comprehensive.The test cases cover different scenarios for various input conditions, ensuring the function behaves as expected.
185-214: LGTM! The test cases forgetEmailHashesare comprehensive.The test cases cover different scenarios for various hash methods and input conditions, ensuring the function behaves as expected.
packages/analytics-js-integrations/src/integrations/Criteo/utils.js (2)
Line range hint
57-84:
LGTM! The updates tohandleCommonFieldsare well-implemented.The changes align with the new functionality introduced by
getEmailHashes.
26-56: Ensure test coverage for all branches and conditions ingetEmailHashes.The static analysis tools indicate that some lines are not covered by tests. Ensure that all branches and conditions are tested.
Tools
GitHub Check: codecov/patch
[warning] 40-40: packages/analytics-js-integrations/src/integrations/Criteo/utils.js#L40
Added line #L40 was not covered by tests
[warning] 44-44: packages/analytics-js-integrations/src/integrations/Criteo/utils.js#L44
Added line #L44 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/analytics-js-integrations/tests/integrations/Criteo/utils.test.js (2 hunks)
Additional context used
Learnings (2)
Common learnings
Learnt from: pierreavizou PR: rudderlabs/rudder-sdk-js#1613 File: packages/analytics-js-integrations/src/integrations/Criteo/utils.js:36-42 Timestamp: 2024-03-27T17:02:27.599Z Learning: The implementation for handling SHA256 email hashes in the Criteo integration sends both md5 and sha256 hashes regardless of the `hashMethod` selection, as per Criteo's recommendation to comply with new regulations and ensure compatibility during their system transition.packages/analytics-js-integrations/__tests__/integrations/Criteo/utils.test.js (1)
Learnt from: pierreavizou PR: rudderlabs/rudder-sdk-js#1613 File: packages/analytics-js-integrations/src/integrations/Criteo/utils.js:36-42 Timestamp: 2024-03-27T17:02:27.599Z Learning: The implementation for handling SHA256 email hashes in the Criteo integration sends both md5 and sha256 hashes regardless of the `hashMethod` selection, as per Criteo's recommendation to comply with new regulations and ensure compatibility during their system transition.
Additional comments not posted (4)
packages/analytics-js-integrations/__tests__/integrations/Criteo/utils.test.js (4)
185-195: LGTM!The test case for
getEmailHasheswith hashMethod 'both' is well-written and covers the scenario adequately.
197-206: LGTM!The test case for
getEmailHasheswith empty email input is well-written and covers the scenario adequately.
208-213: LGTM!The test case for
getEmailHasheswith hashMethod 'sha256' is well-written and covers the scenario adequately.
215-219: LGTM!The test case for
getEmailHasheswith hashMethod 'none' is well-written and covers the scenario adequately.
packages/analytics-js-integrations/__tests__/integrations/Criteo/utils.test.js
Show resolved
Hide resolved
|
PR Description
Supporting multiple hashes for email for criteo in one call driven through UI option for hashing.
Criteo Docs: https://help.criteo.com/kb/guide/en/intro-to-the-criteo-onetag-8fjCDwCENw/Steps/775595,853887,2616497,2616569,2616738,2617588
Please include a summary of the change along with the relevant motivation and context.
Linear task (optional)
https://linear.app/rudderstack/issue/INT-2438/add-double-hashing-method-in-criteo-device-mode
Linear task link
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit
New Features
Tests
These changes provide users with greater control and flexibility in managing email properties within the integration.