feat: add config for hash to bingads offline conversions #1843
Conversation
ItsSudip
requested review from
lvrach,
cisse21,
debanjan97,
am6010,
ssbeefeater,
ruchiramoitra,
shrouti1507,
sandeepdsvs,
krishna2020 and
1abhishekpandey
as code owners
WalkthroughThe pull request introduces modifications to the configuration files for the Bing Ads Offline Conversions destination. Key changes include the addition of a new property, Changes
Possibly related PRs
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1843 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 53 53
Branches 7 7
=========================================
Hits 53 53 ☔ 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: 0
🧹 Outside diff range and nitpick comments (5)
src/configurations/destinations/bingads_offline_conversions/schema.json (1)
15-18: Consider adding validation for PII fields when hashing is enabledWhile the schema definition for
isHashRequiredis correct, consider adding validation rules for email and phone fields when hashing is enabled to ensure they meet the Microsoft Advertising requirements for hashed values.Add validation patterns for hashed values:
"isHashRequired": { "type": "boolean", "default": false - } + }, + "allOf": [ + { + "if": { + "properties": { + "isHashRequired": { "const": true } + } + }, + "then": { + "properties": { + "email": { + "pattern": "^[a-fA-F0-9]{64}$" // SHA256 hash pattern + }, + "phone": { + "pattern": "^[a-fA-F0-9]{64}$" // SHA256 hash pattern + } + } + } + } + ]src/configurations/destinations/bingads_offline_conversions/ui-config.json (2)
71-72: Improve checkbox label clarityThe current label "Enable it, if you are not sending hashed data" could be clearer. Consider rewording to better explain the action and its consequence.
Suggested label change:
- "label": "Enable it, if you are not sending hashed data.", + "label": "Automatically hash email and phone data before sending",
61-92: Consider additional security measures for PII handlingWhile adding hashing capability is good, consider these additional security measures:
- Add a warning in the UI about sending unhashed PII data
- Consider implementing server-side validation to prevent accidental PII exposure
- Add logging/monitoring for when unhashed data is detected
test/data/validation/destinations/bingads_offline_conversions.json (2)
218-218: Add more test cases forisHashRequired.While the current test cases cover valid boolean and invalid string scenarios, consider adding tests for:
- Missing
isHashRequiredfield (default behavior)isHashRequired: falsevalueisHashRequired: nullvalueisHashRequired: undefinedvalueThis ensures comprehensive validation coverage for all possible scenarios.
Here's a suggested test case to add:
[ { "config": { "customerAccountId": "434342324", "customerId": "447477473", "isHashRequired": false }, "result": true }, + { + "config": { + "customerAccountId": "434342324", + "customerId": "447477473", + "isHashRequired": null + }, + "result": false, + "err": ["isHashRequired must be boolean"] + } ]
Line range hint
185-243: Consider adding validation for hashed data format.The PR objective mentions this feature is for hashing email and phone data, but the current validation only checks if
isHashRequiredis a boolean. Consider adding validation to ensure that whenisHashRequiredis true, the relevant fields actually contain hashed values (e.g., checking for common hash formats like SHA-256).This would prevent accidental PII exposure by ensuring the data is actually hashed when required.
Example validation patterns for common hash formats:
- MD5: 32 hexadecimal characters
- SHA-256: 64 hexadecimal characters
- SHA-512: 128 hexadecimal characters
Would you like me to propose additional test cases that validate the hash format?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/configurations/destinations/bingads_offline_conversions/db-config.json(1 hunks)src/configurations/destinations/bingads_offline_conversions/schema.json(1 hunks)src/configurations/destinations/bingads_offline_conversions/ui-config.json(1 hunks)test/data/validation/destinations/bingads_offline_conversions.json(3 hunks)
🔇 Additional comments (4)
src/configurations/destinations/bingads_offline_conversions/db-config.json (1)
28-29: LGTM! Verify configuration consistency
The addition of isHashRequired to defaultConfig is appropriate and follows the existing pattern.
Let's verify that this configuration key is consistently used across all related files:
✅ Verification successful
Let me check the content of these files to ensure the configuration is consistent across them.
Configuration is consistent across all files ✅
The isHashRequired configuration is properly defined and consistent across all related files:
- In
db-config.json: Added todefaultConfigarray - In
ui-config.json: Defined as a config key with default valuefalse - In
schema.json: Properly typed as boolean with default valuefalse
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for isHashRequired usage across configuration files
rg "isHashRequired" src/configurations/destinations/bingads_offline_conversions/
Length of output: 475
Script:
#!/bin/bash
# Check the content of the files to verify configuration consistency
echo "=== ui-config.json ==="
cat src/configurations/destinations/bingads_offline_conversions/ui-config.json | grep -A 5 -B 5 "isHashRequired"
echo -e "\n=== schema.json ==="
cat src/configurations/destinations/bingads_offline_conversions/schema.json | grep -A 5 -B 1 "isHashRequired"
Length of output: 951
src/configurations/destinations/bingads_offline_conversions/ui-config.json (1)
73-85: LGTM! Well-documented feature
Excellent inclusion of relevant documentation links to Microsoft's official documentation for both hashed email and phone number requirements.
test/data/validation/destinations/bingads_offline_conversions.json (2)
185-185: LGTM! Good test case for invalid type.
The test case correctly validates that isHashRequired must be a boolean and not a string "true". This helps prevent type-related bugs in production.
Also applies to: 209-209
Line range hint 185-209: Verify PII handling in validation tests.
Given that this feature is specifically for hashing email and phone data (as per PR objectives), we should verify that the test data doesn't contain any actual PII.
Also applies to: 218-243
✅ Verification successful
Let me check the specific file under review for any PII patterns.
Let me check the actual content of the file to verify the test data.
No PII data found in test cases
After reviewing the test file content, I can confirm that it only contains:
- Mock account IDs (e.g., "434342324", "447477473")
- Test validation scenarios with dummy data
- Configuration validation patterns
- No actual email addresses, phone numbers, or other PII
The test data appropriately uses non-sensitive information to validate the schema and functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential PII patterns in test files
rg -i '(\b[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Z|a-z]{2,}\b)|(\b\d{3}[-.]?\d{3}[-.]?\d{4}\b)' test/data/validation/destinations/
Length of output: 14693
Script:
#!/bin/bash
# Check the specific file for PII patterns
rg -i '(\b[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Z|a-z]{2,}\b)|(\b\d{3}[-.]?\d{3}[-.]?\d{4}\b)' test/data/validation/destinations/bingads_offline_conversions.json
Length of output: 162
Script:
#!/bin/bash
# View the content of the specific file
cat test/data/validation/destinations/bingads_offline_conversions.json
Length of output: 7053
What are the changes introduced in this PR?
We are adding a new configuration to enable hashing of email and phone.
What is the related Linear task?
Resolves INT-3019
Please explain the objectives of your changes below
Put down any required details on the broader aspect of your changes. If there are any dependent changes, mandatorily mention them here
Any changes to existing capabilities/behaviour, mention the reason & what are the changes ?
N/A
Any new dependencies introduced with this change?
N/A
Any new checks got introduced or modified in test suites. Please explain the changes.
N/A
Developer checklist
My code follows the style guidelines of this project
No breaking changes are being introduced.
All related docs linked with the PR?
All changes manually tested?
Any documentation changes needed with this change?
I have executed schemaGenerator tests and updated schema if needed
Are sensitive fields marked as secret in definition config?
My test cases and placeholders use only masked/sample values for sensitive fields
Is the PR limited to 10 file changes & one task?
Reviewer checklist
Is the type of change in the PR title appropriate as per the changes?
Verified that there are no credentials or confidential data exposed with the changes.
Summary by CodeRabbit
New Features
Bug Fixes