feat: onboard tune destination #1744
Conversation
aanshi07
requested review from
lvrach,
cisse21,
debanjan97,
am6010,
ssbeefeater,
AchuthaSourabhC,
ruchiramoitra,
ItsSudip,
shrouti1507,
sandeepdsvs,
krishna2020 and
1abhishekpandey
as code owners
WalkthroughA set of new configuration files has been introduced for the TUNE destination, enhancing the management of its settings and integration. This includes a 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 #1744 +/- ##
=========================================
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 (13)
src/configurations/destinations/tune/db-config.json (1)
69-71: LGTM: Beta status is correctly set.The beta status is correctly set for this new destination integration. This aligns well with the PR objectives.
Suggestion:
Consider adding a note in the PR description about the beta status of this integration. This will help other developers and stakeholders understand the current state of the TUNE destination.
test/data/validation/destinations/tune.json (5)
58-79: LGTM: Correct validation of invalid connectionModeThis test case properly identifies and rejects an invalid connectionMode value. The error message is clear and specific.
Consider adding a comment in the test case to explicitly state the purpose of testing an invalid connectionMode. This would enhance the test's readability and maintainability.
80-101: LGTM: Consistent validation of another invalid connectionModeThis test case correctly identifies and rejects another invalid connectionMode value. The error message is consistent with the previous test case.
For consistency with other test cases, consider adding a
testTitlefield to this test case, similar to the ones used in subsequent test cases. This would improve the overall structure and readability of the test suite.
102-120: LGTM: Thorough validation of invalid consent management configurationThis test case effectively validates an invalid consent management configuration with an incorrect resolutionStrategy. The error messages are specific and helpful.
To improve clarity, consider adding a comment or expanding the testTitle to explain what a valid resolutionStrategy value would be. This would make the test case more self-explanatory and easier to maintain.
121-153: LGTM: Comprehensive validation of consent management edge casesThese test cases effectively validate two important scenarios for consent management configuration:
- Missing resolutionStrategy
- Invalid provider value
The error messages are specific and helpful in both cases.
For improved structure and readability, consider splitting these two scenarios into separate test objects within the JSON array. This would make each test case more distinct and easier to maintain or extend in the future.
1-154: Overall: Well-structured and comprehensive test suite for TUNE destinationThis test suite provides a thorough set of validation cases for the TUNE destination configuration, covering both valid and invalid scenarios for connection modes and consent management. The test cases are generally well-structured with clear expected results and specific error messages.
To further improve the test suite:
- Consider adding test cases for edge cases or boundary values that may not be covered yet. For example:
- Maximum/minimum allowed values for certain fields
- Empty string values where applicable
- Special characters in URLs or event names
- Implement a consistent structure across all test cases, including:
- Using
testTitlefor all test cases- Grouping related test cases (e.g., all connection mode tests together)
- Ensuring each test case is a separate object in the JSON array
These improvements would enhance the comprehensiveness and maintainability of the test suite.
src/configurations/destinations/tune/ui-config.json (3)
3-148: Review the "Initial setup" section in the baseTemplateThe "Initial setup" section in the baseTemplate is well-structured but currently lacks any fields for user input. Consider adding necessary fields for initial configuration, such as API keys or other essential connection parameters.
Would you like assistance in defining appropriate fields for the "Initial setup" section?
150-154: Clarify the purpose of the sdkTemplate sectionThe sdkTemplate section is present but currently empty, with a note indicating it's not visible in the UI. Could you provide more context on the intended use of this section? If it's for future expansion, consider adding a TODO comment with more details.
263-265: Consider enhancing URL validationThe URL validation regex is comprehensive but could be improved to handle more edge cases. For instance, it might reject valid URLs with certain special characters in the query string.
Consider using a more robust URL validation library or expanding the regex to cover more valid URL formats while still preventing common mistakes.
src/configurations/destinations/tune/schema.json (4)
15-54: LGTM: Well-defined event mapping structureThe
eventsMappingproperty is well-structured, allowing for flexible mapping of custom events to predefined TUNE events. The use of an enum for thetoproperty ensures that only valid TUNE events are used.Consider adding a comment or documentation explaining the purpose of the empty string in the
toenum. This could help clarify whether it's used for optional mappings or has another specific purpose.
55-95: LGTM: Comprehensive event filtering optionsThe event filtering options are well-structured, providing flexible control over event tracking. The inclusion of both whitelisting and blacklisting offers comprehensive management of tracked events, which is crucial for data privacy and compliance.
Consider adding a conditional validation to ensure that when
eventFilteringOptionis set to "whitelistedEvents" or "blacklistedEvents", the corresponding array is not empty. This could prevent misconfiguration where filtering is enabled but no events are specified.Example addition to the schema:
"if": { "properties": { "eventFilteringOption": { "const": "whitelistedEvents" } } }, "then": { "required": ["whitelistedEvents"], "properties": { "whitelistedEvents": { "minItems": 1 } } }, "else": { "if": { "properties": { "eventFilteringOption": { "const": "blacklistedEvents" } } }, "then": { "required": ["blacklistedEvents"], "properties": { "blacklistedEvents": { "minItems": 1 } } } }This addition would ensure that the appropriate event list is provided and non-empty when filtering is enabled.
96-606: LGTM: Comprehensive and flexible consent management configurationThe consent management configuration is well-structured, providing platform-specific settings and support for multiple consent management providers. The conditional logic for custom providers requiring a
resolutionStrategyis a good practice for handling complex consent scenarios.To reduce code duplication and improve maintainability, consider extracting the common consent management structure into a reusable definition. You can use JSON Schema's
$refkeyword to reference this definition for each platform. For example:"definitions": { "consentManagementPlatform": { "type": "array", "items": { "type": "object", "properties": { "provider": { "type": "string", "enum": ["custom", "ketch", "oneTrust"], "default": "oneTrust" }, "consents": { "type": "array", "items": { "type": "object", "properties": { "consent": { "type": "string", "pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{0,100})$" } } } } }, "allOf": [ { "if": { "properties": { "provider": { "const": "custom" } }, "required": ["provider"] }, "then": { "properties": { "resolutionStrategy": { "type": "string", "enum": ["and", "or"] } }, "required": ["resolutionStrategy"] } } ] } } }, "properties": { "consentManagement": { "type": "object", "properties": { "cloud": { "$ref": "#/definitions/consentManagementPlatform" }, "warehouse": { "$ref": "#/definitions/consentManagementPlatform" }, "android": { "$ref": "#/definitions/consentManagementPlatform" }, // ... other platforms } } }This approach would significantly reduce the schema's size and make it easier to maintain and update the consent management configuration across all platforms.
607-658: LGTM: Consistent connection mode configurationThe
connectionModeconfiguration ensures that all platforms use the cloud connection mode, which promotes consistency and simplifies integration.While the current configuration is appropriate for the cloud-only approach, consider the potential need for extensibility in the future. If there's a possibility that other connection modes might be introduced later, it may be beneficial to structure the schema in a way that allows for easy addition of new modes. This could involve using an enum with a single value for now, but designed to be easily expanded:
"connectionMode": { "type": "object", "properties": { "android": { "type": "string", "enum": ["cloud"], "description": "Currently only cloud mode is supported. This may be expanded in the future." }, // ... other platforms } }This approach maintains the current functionality while documenting the potential for future expansion, making it easier to add new connection modes if needed without breaking existing configurations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/configurations/destinations/tune/db-config.json (1 hunks)
- src/configurations/destinations/tune/schema.json (1 hunks)
- src/configurations/destinations/tune/ui-config.json (1 hunks)
- test/data/validation/destinations/tune.json (1 hunks)
🧰 Additional context used
🔇 Additional comments (11)
src/configurations/destinations/tune/db-config.json (4)
1-7: LGTM: Basic configuration looks good.The basic configuration for the TUNE destination is well-structured and follows best practices:
- Appropriate naming
- CDK V2 enabled
- Transformation set to "router"
- Destination response saving enabled (useful for debugging and analytics)
46-68: Verify: Confirm if any keys should be marked as secret.The destination configuration looks well-structured:
- Appropriate default configuration for event tracking.
- Consistent configuration across all source types.
However, there's a potential security concern to address:
The
secretKeysarray is empty. Please verify if this is intentional or if any keys should be marked as secret. Run the following script to check for potential secret keys:#!/bin/bash # Description: Check for potential secret keys that should be marked as such # List of common secret key names potential_secret_keys=("apiKey" "secretKey" "token" "password" "accessKey" "privateKey") # Test 1: Check if any potential secret keys are present in the configuration for key in "${potential_secret_keys[@]}"; do if grep -q "\"$key\"" src/configurations/destinations/tune/db-config.json; then echo "Warning: Potential secret key '$key' found in the configuration. Verify if it should be in secretKeys." fi done # Test 2: Verify that secretKeys is empty as expected secret_keys=$(jq -r '.config.secretKeys[]' src/configurations/destinations/tune/db-config.json) if [ -z "$secret_keys" ]; then echo "Confirmed: secretKeys array is empty." else echo "Warning: secretKeys array is not empty as expected. Contents: $secret_keys" fi
8-18: Verify: Confirm proper handling of sensitive information.The keys configuration looks appropriate:
- Relevant keys for event tracking and configuration are included.
- No keys are excluded, which is acceptable if all are necessary.
However, please ensure that sensitive information is properly handled:
To verify the handling of sensitive information, particularly related to consent management, run the following script:
✅ Verification successful
Verified: Proper Handling of Sensitive Information
consentManagementis included in the configuration.secretKeysis defined in the configuration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for proper handling of sensitive information in the configuration # Test 1: Verify that consentManagement is included in the configuration if grep -q '"consentManagement"' src/configurations/destinations/tune/db-config.json; then echo "consentManagement is included in the configuration." else echo "Warning: consentManagement is not found in the configuration." fi # Test 2: Check if there are any sensitive keys that should be in secretKeys sensitive_keys=("apiKey" "token" "password" "secret") for key in "${sensitive_keys[@]}"; do if grep -q "\"$key\"" src/configurations/destinations/tune/db-config.json; then echo "Warning: Potential sensitive key '$key' found. Verify if it should be in secretKeys." fi done # Test 3: Verify that secretKeys is defined (even if empty) if grep -q '"secretKeys":' src/configurations/destinations/tune/db-config.json; then echo "secretKeys is defined in the configuration." else echo "Warning: secretKeys is not found in the configuration." fiLength of output: 1144
Script:
#!/bin/bash # Description: Check for proper handling of sensitive information in the configuration # Test 1: Verify that consentManagement is included in the configuration if grep -q '"consentManagement"' src/configurations/destinations/tune/db-config.json; then echo "consentManagement is included in the configuration." else echo "Warning: consentManagement is not found in the configuration." fi # Test 2: Check if there are any sensitive keys that should be in secretKeys sensitive_keys=("apiKey" "token" "password" "secret") for key in "${sensitive_keys[@]}"; do if grep -q "\"$key\"" src/configurations/destinations/tune/db-config.json; then echo "Warning: Potential sensitive key '$key' found. Verify if it should be in secretKeys." fi done # Test 3: Verify that secretKeys is defined (even if empty) if grep -q '"secretKeys":' src/configurations/destinations/tune/db-config.json; then echo "secretKeys is defined in the configuration." else echo "Warning: secretKeys is not found in the configuration." fiLength of output: 864
19-45: Verify: Confirm if limited message type support is intentional.The configuration for supported types and modes looks comprehensive:
- Wide range of supported source types, enhancing compatibility.
- Consistent use of "cloud" connection mode across all sources.
However, there's a potential limitation to verify:
Please confirm if the limited message type support (only "track" for cloud sources) is intentional. Run the following script to check for any discrepancies:
test/data/validation/destinations/tune.json (2)
1-22: LGTM: Valid test case for basic cloud configurationThis test case correctly validates a basic cloud configuration for the TUNE destination. It includes a single event mapping and expects a successful validation result.
23-57: LGTM: Comprehensive test case for multiple event mappingsThis test case effectively validates a more complex cloud configuration for the TUNE destination. It includes multiple event mappings with different URLs and event names, providing a thorough check of the configuration's flexibility.
src/configurations/destinations/tune/ui-config.json (3)
3-148: LGTM: Well-structured baseTemplateThe overall structure of the baseTemplate is well-organized and provides clear guidance for users. It effectively covers initial setup, configuration settings, and event mapping. The inclusion of client-side event filtering options in the "Configuration settings" section is a good feature for customization.
155-389: LGTM: Comprehensive event mapping configurationThe redirectGroups section provides a detailed and flexible configuration for mapping RudderStack events to Tune events. The use of a dynamicCustomForm allows for customizable mappings, which is excellent for accommodating various use cases.
The default "Purchase Event" mapping is extensive and covers many common properties. This provides a good starting point for users.
1-391: LGTM: Well-structured and comprehensive Tune destination configurationThe overall structure of the ui-config.json file for the Tune destination is well-organized, comprehensive, and follows a logical flow. It covers all major aspects of the destination setup, including initial configuration, settings management, and detailed event mapping.
Key strengths:
- Clear separation of concerns between different configuration sections.
- Flexible event mapping with a wide range of predefined fields.
- Inclusion of client-side event filtering options.
Recommendations for future improvements:
- Complete the "Initial setup" section with necessary connection fields.
- Clarify the purpose and future plans for the sdkTemplate section.
- Consider enhancing URL validation in the event mapping section.
The configuration provides a solid foundation for integrating the Tune destination. It balances guided setup with flexibility for customization, which should cater to various user needs.
src/configurations/destinations/tune/schema.json (2)
1-14: LGTM: Well-structured schema with appropriate validationThe schema is well-structured and follows JSON Schema draft-07 standards. The
urlandeventNameproperties have appropriate regex patterns that allow for flexibility, including the use of environment variables. This is a good practice for configuration management.
1-658: Overall: Well-designed and comprehensive schema for TUNE destinationThis schema provides a robust and flexible configuration structure for the TUNE destination. It covers all necessary aspects including URL and event name validation, event mapping, filtering options, comprehensive consent management across multiple platforms, and connection mode settings.
The use of JSON Schema features such as regex patterns, enums, and conditional logic ensures that configurations will be validated effectively, reducing the likelihood of runtime errors due to misconfiguration.
While some suggestions have been made for potential optimizations (such as reducing duplication in the consent management section and considering future extensibility for connection modes), these are refinements to an already solid foundation.
Great job on creating a thorough and well-structured configuration schema!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/configurations/destinations/tune/ui-config.json (2)
3-35: Consider adding fields to Connection Settings and Connection mode groups.The baseTemplate section provides a good structure for initial setup. However, both the "Connection Settings" and "Connection mode" groups have empty
fieldsarrays. Consider adding relevant fields to these groups to capture necessary connection information from users.
155-384: LGTM: Comprehensive conversionMapping configuration with a minor suggestion.The conversionMapping group in the redirectGroups section is well-structured and provides a flexible way to map RudderStack events and properties to Tune events. The default mappings cover a wide range of common fields, which will be helpful for users.
One minor suggestion:
Consider updating the URL regex to allow for localhost and IP addresses, which could be useful for testing purposes. You could modify the regex as follows:- "regex": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|(?!.*\\.ngrok\\.io)^(?:http(s)?:\\/\\/)?[\\w.-]+(?:\\.[\\w\\.-]+)+[\\w\\-\\._~:/?#[\\]@!\\$&'\\(\\)\\*\\+,;=.]+$|^$", + "regex": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|(?!.*\\.ngrok\\.io)^(?:http(s)?:\\/\\/)?(?:localhost|\\d{1,3}(?:\\.\\d{1,3}){3}|[\\w.-]+(?:\\.[\\w\\.-]+)+)[\\w\\-\\._~:/?#[\\]@!\\$&'\\(\\)\\*\\+,;=.]+$|^$",This change allows for localhost and IP addresses while maintaining the existing validation rules.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/configurations/destinations/tune/ui-config.json (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
src/configurations/destinations/tune/ui-config.json (3)
36-126: LGTM: Well-structured Configuration settings section.The Configuration settings section is well-designed, particularly the client-side event filtering options. The use of prerequisites to control field visibility based on user selections is a good practice. This structure ensures a clear and logical flow for users setting up event filtering.
127-148: LGTM: Efficient Event mapping structure.The Event mapping section effectively uses a redirect to the "conversionMapping" group. This approach allows for a clean and maintainable structure, separating the complex mapping configuration from the main UI layout.
150-154: Clarify the purpose of the sdkTemplate section.The sdkTemplate section is marked as "not visible in the ui" and contains an empty fields array. Could you please clarify the intended purpose of this section? If it's a placeholder for future development, consider adding a comment to that effect. If it's not needed, it might be better to remove it to keep the configuration clean.
…onfig into feat.onboard_tune_destination
What are the changes introduced in this PR?
Onboarded Tune destination
What is the related Linear task?
Resolves INT-282
Ui:
Research Doc: https://www.notion.so/rudderstacks/Tune-0ae922ff65be4915897e437ac25c311c#b05d3640f14b4d4a84b176c922a9bc92
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
Tests