chore: adding salesforce oauth sandbox support for closed testing #1732
Conversation
shrouti1507
requested review from
lvrach,
cisse21,
debanjan97,
am6010,
ssbeefeater,
AchuthaSourabhC,
ruchiramoitra,
ItsSudip,
sandeepdsvs,
krishna2020 and
1abhishekpandey
as code owners
WalkthroughA new configuration for the Salesforce OAuth Sandbox has been introduced, comprising two new files: Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (7)
src/configurations/destinations/salesforce_oauth_sandbox/db-config.json (2)
1-6: Remove trailing space in display nameThe
displayNamevalue has a trailing space, which might be unintentional. Consider removing it for consistency.- "displayName": "Salesforce Sandbox V2 ", + "displayName": "Salesforce Sandbox V2",
105-113: Options configuration looks good, consider documenting AMP feature flagThe options configuration, including the beta status and hidden feature flag for AMP integration, is appropriate. However, consider adding a comment or documentation about the purpose and usage of the
AMP_salesforce_sandbox_acornsfeature flag to improve maintainability.Consider adding a comment explaining the AMP feature flag:
"options": { "hidden": { "featureFlagName": "AMP_salesforce_sandbox_acorns", - "featureFlagValue": false + "featureFlagValue": false, + "_comment": "This feature flag controls the availability of AMP integration for Salesforce Sandbox" }, "beta": true }test/data/validation/destinations/salesforce_oauth_sandbox.json (4)
56-109: LGTM: Comprehensive test cases for invalid consent management configurations.These test cases effectively cover important edge cases for consent management configuration:
- Invalid resolutionStrategy value (lines 56-75)
- Missing resolutionStrategy value (lines 76-94)
- OneTrust provider without resolutionStrategy (lines 95-109)
The expected results and error messages are appropriate for each case.
Suggestion for improvement: Consider adding a test case for a valid custom provider configuration with a correct resolutionStrategy value to ensure the validation logic works correctly for both valid and invalid cases.
126-244: LGTM: Comprehensive test for oneTrustCookieCategories and ketchConsentPurposes.This test case effectively covers a wide range of valid configurations for both oneTrustCookieCategories and ketchConsentPurposes across multiple platforms. It includes various scenarios such as empty arrays, environment variables, and template expressions.
Suggestion for improvement: Consider breaking this large test case into smaller, more focused test cases for each platform or specific scenario. This would improve maintainability and make it easier to identify issues if they arise in the future.
For example:
{ "testTitle": "Valid oneTrustCookieCategories for Android", "config": { "mapProperties": true, "useContactId": true, "oneTrustCookieCategories": { "android": [ { "oneTrustCookieCategory": "C0001" }, { "oneTrustCookieCategory": "C0002" } ] } }, "result": true }, { "testTitle": "Valid ketchConsentPurposes with environment variable", "config": { "mapProperties": true, "useContactId": true, "ketchConsentPurposes": { "amp": [ { "purpose": "env.ENVIRONMENT_VARIABLE" } ] } }, "result": true }This approach would make the test suite more modular and easier to maintain.
269-344: LGTM: Comprehensive test for invalid configurations in consent-related properties.This test case effectively covers a wide range of invalid configurations for both oneTrustCookieCategories and ketchConsentPurposes. It includes scenarios such as overly long strings, non-string values, and incorrect array structures. The expected false result and the specific error messages are appropriate and helpful for identifying the exact issues in each part of the configuration.
Suggestion for improvement: Consider breaking this large test case into smaller, more focused test cases for each type of invalid configuration. This would improve maintainability and make it easier to identify and debug specific issues in the future.
For example:
{ "testTitle": "Invalid oneTrustCookieCategory - String too long", "config": { "mapProperties": true, "useContactId": true, "oneTrustCookieCategories": { "android": [ { "oneTrustCookieCategory": "more than 100 characters string - AAAA..." } ] } }, "result": false, "err": [ "oneTrustCookieCategories.android.0.oneTrustCookieCategory must match pattern \"(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{0,100})$\"" ] }, { "testTitle": "Invalid ketchConsentPurposes - Non-string value", "config": { "mapProperties": true, "useContactId": true, "ketchConsentPurposes": { "ios": [ { "purpose": { "not": "a string" } } ] } }, "result": false, "err": [ "ketchConsentPurposes.ios.0.purpose must be string" ] }This approach would make the test suite more modular and easier to maintain, while still covering all the necessary invalid configurations.
1-344: Overall, a comprehensive and well-structured test suite with room for minor improvements.This test suite for the Salesforce OAuth Sandbox configuration validation is thorough and covers a wide range of scenarios, including both valid and invalid configurations. It effectively tests various aspects such as consent management, OneTrust cookie categories, and Ketch consent purposes across multiple platforms.
Main suggestions for improvement:
- Consolidate identical test cases (lines 1-22) into a single test case with a descriptive title.
- Break down larger test cases (e.g., lines 126-244 and 269-344) into smaller, more focused tests for better maintainability and easier debugging.
- Consider adding a test case for a valid custom provider configuration with a correct resolutionStrategy value (as suggested in the comment for lines 56-109).
These improvements will enhance the overall quality and maintainability of the test suite while maintaining its comprehensive coverage.
src/configurations/destinations/salesforce_oauth_sandbox/schema.json (1)
1-778: Overall assessment: Comprehensive schema with room for improvement.The schema provides a detailed structure for configuring a Salesforce OAuth sandbox, covering various aspects such as property mapping, contact ID usage, cookie categories, consent management, connection modes, and consent purposes across multiple platforms.
While functional, the schema could benefit from the following improvements:
- Refactoring repeated structures (like platform-specific configurations) to reduce duplication.
- Simplifying overly complex structures (like
connectionMode).- Considering the addition of required properties at the root level.
- Evaluating the use of
additionalProperties: trueat the root level.Implementing these changes would enhance the schema's maintainability, reduce the potential for errors, and improve its overall robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/configurations/destinations/salesforce_oauth_sandbox/db-config.json (1 hunks)
- src/configurations/destinations/salesforce_oauth_sandbox/schema.json (1 hunks)
- src/configurations/destinations/salesforce_oauth_sandbox/ui-config.json (1 hunks)
- test/data/validation/destinations/salesforce_oauth_sandbox.json (1 hunks)
🧰 Additional context used
🔇 Additional comments (13)
src/configurations/destinations/salesforce_oauth_sandbox/db-config.json (4)
7-15: Authentication and transformation configuration looks goodThe authentication setup, transformation configuration, and key management are well-defined and appropriate for a Salesforce OAuth Sandbox integration. The inclusion of consent management and cookie category keys demonstrates attention to privacy considerations.
1-113: Overall, the configuration is well-structured and appropriateThe Salesforce OAuth Sandbox configuration is comprehensive and well-organized. It covers all major platforms, includes necessary consent management features, and is appropriately marked as beta. The suggestions provided in the review are minor and aimed at improving clarity and considering potential enhancements. Please address the comments and verify the points raised to ensure the configuration is robust and future-proof.
29-92: Destination configuration looks good, verify empty secretKeysThe destination configuration is well-structured and consistent across all platforms. The inclusion of consent management and cookie categories is commendable. However, please verify if the empty
secretKeysarray is intentional or if any secret keys should be added for secure communication with Salesforce.To verify the
secretKeysconfiguration, run the following script:#!/bin/bash # Description: Check secretKeys configuration in other Salesforce integrations # Test: Search for secretKeys in other Salesforce configurations rg --type json 'secretKeys' src/configurations/destinations/salesforce*
16-28: 🛠️ Refactor suggestionConsider supporting additional message types
The configuration currently only supports the "identify" message type. For a more comprehensive Salesforce integration, consider adding support for "track" and "group" message types as well. This would allow for more versatile data syncing capabilities.
- "supportedMessageTypes": ["identify"], + "supportedMessageTypes": ["identify", "track", "group"],To verify the impact of this change, run the following script:
#!/bin/bash # Description: Check if other Salesforce configurations support additional message types # Test: Search for supportedMessageTypes in other Salesforce configurations rg --type json 'supportedMessageTypes' src/configurations/destinations/salesforce*src/configurations/destinations/salesforce_oauth_sandbox/ui-config.json (5)
1-20: LGTM: "Other Settings" section is well-structured and clear.The "Other Settings" section is appropriately configured with two checkbox fields:
- "Map Rudder Properties to Salesforce Properties" (default: true)
- "Use contactId for converted leads" (default: false)
The labels are clear, and the footer note for the second option provides helpful guidance for users.
21-51: LGTM: OneTrust Consent Category IDs configuration is well-structured.The configuration for OneTrust Consent Category IDs is clear and includes:
- Appropriate labeling and placeholder
- A helpful footer note about using category IDs instead of names
- A regex pattern for validation
- Feature flag prerequisites for conditional rendering
52-78: LGTM: Ketch Consent Purpose IDs configuration is consistent with OneTrust.The Ketch Consent Purpose IDs configuration mirrors the structure of the OneTrust section, maintaining consistency in the UI. It includes:
- Appropriate labeling and placeholder
- The same regex pattern for validation
- Similar feature flag prerequisites for conditional rendering
79-157: LGTM: Consent management settings are comprehensive and well-structured.The consent management settings section provides a flexible and comprehensive configuration:
- It allows selection of the consent management provider (Custom, Ketch, OneTrust)
- Includes a required field for consent logic (AND/OR) when using custom provider
- Provides a dynamic form for entering consent category IDs
- Uses appropriate feature flags for conditional rendering
The structure and options provided align well with the PR objective of adding Salesforce OAuth sandbox support for closed testing.
1-161: Overall, the ui-config.json file is well-structured and comprehensive.The file provides a thorough UI configuration for the Salesforce OAuth Sandbox, covering both general settings and consent management options. Key points:
- Consistent use of feature flags for conditional rendering
- Clear labeling and helpful footer notes
- Comprehensive consent management settings
- Appropriate use of dynamic custom forms
The configuration aligns well with the PR objective of adding Salesforce OAuth sandbox support for closed testing. No issues or inconsistencies were found.
test/data/validation/destinations/salesforce_oauth_sandbox.json (3)
23-55: LGTM: Well-structured test case for multiple consent management providers.This test case effectively covers a scenario with multiple consent management providers, including custom, oneTrust, and ketch. The configuration is comprehensive and the expected result is appropriate.
110-125: LGTM: Effective test for invalid consent management provider.This test case correctly validates that only allowed provider values are accepted in the consent management configuration. The expected false result and the specific error message are appropriate for this scenario.
245-268: LGTM: Effective test for invalid structure of consent-related properties.This test case correctly validates that oneTrustCookieCategories and ketchConsentPurposes must be objects, not arrays. The expected false result and the specific error messages are appropriate for this scenario, ensuring proper validation of the structure of these properties.
src/configurations/destinations/salesforce_oauth_sandbox/schema.json (1)
7-14: LGTM: Well-defined boolean properties.The
mapPropertiesanduseContactIdboolean properties are clearly defined with appropriate default values.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
src/configurations/destinations/salesforce_oauth_sandbox/schema.json (2)
1-14: Consider making essential properties required.The schema structure looks good, and the use of default values for
mapPropertiesanduseContactIdis appropriate. However, the emptyrequiredarray at the schema root level means that no properties are mandatory. Consider if any properties should be required for a valid configuration.
1-777: Overall schema structure is comprehensive but could benefit from refactoring.The schema for the Salesforce OAuth sandbox configuration is well-structured and covers all necessary aspects, including properties for mapping, contact ID usage, cookie categories, consent management, connection mode, and consent purposes. It provides good flexibility with support for multiple platforms and consent management providers.
The use of string patterns allowing for environment variables and placeholders is a good practice for configuration management. However, the main area for improvement is the repetition of similar structures across different properties (
oneTrustCookieCategories,consentManagement,ketchConsentPurposes).To enhance maintainability and reduce the risk of inconsistencies, consider implementing the refactoring suggestions provided in the previous comments. This would involve creating reusable schema definitions for the repeated structures and referencing them for each platform.
These changes would make the schema more concise, easier to maintain, and less prone to errors when updates are needed across multiple platforms.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/configurations/destinations/salesforce_oauth_sandbox/db-config.json (1 hunks)
- src/configurations/destinations/salesforce_oauth_sandbox/schema.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/configurations/destinations/salesforce_oauth_sandbox/db-config.json
🧰 Additional context used
📓 Learnings (1)
src/configurations/destinations/salesforce_oauth_sandbox/schema.json (1)
Learnt from: shrouti1507 PR: rudderlabs/rudder-integrations-config#1732 File: test/data/validation/destinations/salesforce_oauth_sandbox.json:1-22 Timestamp: 2024-10-08T12:13:04.607Z Learning: In the `rudder-integrations-config` repository, for closed testing destinations, identical test cases without consolidation are acceptable.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1732 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 53 53
Branches 7 7
=========================================
Hits 53 53 ☔ View full report in Codecov by Sentry. |
What are the changes introduced in this PR?
Write a brief explainer on your code changes.
What is the related Linear task?
Resolves INT-2736
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
Tests