feat: onboarding intercom v2 destination #1655
Conversation
manish339k
requested review from
lvrach,
cisse21,
debanjan97,
am6010,
ssbeefeater,
AchuthaSourabhC,
ruchiramoitra,
ItsSudip,
shrouti1507,
sandeepdsvs,
krishna2020 and
1abhishekpandey
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1655 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 53 53
Branches 7 7
=========================================
Hits 53 53 ☔ View full report in Codecov by Sentry. |
WalkthroughA set of new configuration files has been introduced for the Intercom V2 integration, including 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 (
|
![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 (6)
src/configurations/destinations/intercom_v2/ui-config.json (2)
15-36: Add documentation link for API Server selection.Consider adding a link to Intercom's regional hosting documentation in the note field to help users make an informed decision.
{ "type": "singleSelect", "label": "API Server", "configKey": "apiServer", "options": [ {"label": "US", "value": "US"}, {"label": "Europe", "value": "Europe"}, {"label": "Australia", "value": "Australia"} ], "default": "US", - "note": "Select your Intercom API Server", + "note": ["Select your Intercom API Server. ", { + "text": "Learn more about regional hosting", + "link": "https://developers.intercom.com/docs/build-an-integration/learn-more/rest-apis#regional-hosting" + }], "preRequisites": {} }
203-204: Fix apostrophe usage in labels and notes.For consistency, avoid using apostrophes in plural forms.
- "label": "Enter consent category ID's", + "label": "Enter consent category IDs",test/data/validation/destinations/intercom_v2.json (2)
1-20: Enhance API server validation test coverage.While the basic validation is covered, consider adding these test cases for
apiServer:
- Empty string value
- Null value
- Case sensitivity validation (e.g., "us" vs "US")
Would you like me to provide the additional test cases?
21-118: Add edge cases for consent management validation.Consider adding these test cases:
- Empty provider array validation
- Duplicate provider validation
- Maximum number of consents validation
- Mixed case provider names (e.g., "OneTrust" vs "onetrust")
Would you like me to provide the additional test cases?
src/configurations/destinations/intercom_v2/schema.json (2)
1-6: Consider defining required properties in the schema.The schema currently doesn't enforce any required properties. Consider making essential properties like
apiServerandconnectionModerequired to prevent invalid configurations."configSchema": { "$schema": "http://json-schema.org/draft-07/schema#", - "required": [], + "required": ["apiServer", "connectionMode"], "type": "object", "properties": {
7-8: Add descriptions for configuration properties.Consider adding descriptions to improve schema documentation:
"properties": { - "apiServer": { "type": "string", "enum": ["US", "Europe", "Australia"], "default": "US" }, - "sendAnonymousId": { "type": "boolean", "default": false }, + "apiServer": { + "type": "string", + "enum": ["US", "Europe", "Australia"], + "default": "US", + "description": "Specifies the regional API server for data residency compliance" + }, + "sendAnonymousId": { + "type": "boolean", + "default": false, + "description": "Controls whether anonymous user IDs should be sent to Intercom" + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/configurations/destinations/intercom_v2/db-config.json (1 hunks)
- src/configurations/destinations/intercom_v2/schema.json (1 hunks)
- src/configurations/destinations/intercom_v2/ui-config.json (1 hunks)
- test/data/validation/destinations/intercom_v2.json (1 hunks)
🔇 Additional comments (10)
src/configurations/destinations/intercom_v2/db-config.json (4)
6-6: Consider the performance impact of saving destination responses.Setting
saveDestinationResponseto true will store all responses from Intercom, which might impact performance and storage. Consider enabling this only if required for debugging or compliance purposes.
13-13: Remove unnecessary excludeKeys array.Since this integration only supports cloud mode (no device mode support), the
excludeKeysarray is unnecessary and can be removed.
98-104: LGTM! Proper feature flag implementation.The feature flag implementation allows for controlled testing in production environments, which is crucial for a beta release.
96-96: Verify no sensitive fields are missing from secretKeys.The
secretKeysarray is empty, which appears correct as OAuth handles authentication separately. However, please verify that no other sensitive fields (like API keys or tokens) will be added to this configuration in the future.✅ Verification successful
Let me try a different approach to verify the security configuration.
After analyzing both the schema.json and db-config.json files, I can now provide a definitive response about the security configuration.
Empty secretKeys array is correct for this configuration
The empty
secretKeysarray is appropriate because:
- Authentication is handled via OAuth as specified in the
authconfiguration- The schema shows no sensitive fields in the configuration - only standard settings like
apiServer,connectionMode, consent management configurations, and feature flags- All configurable fields are either enums, boolean flags, or consent-related string patterns that don't contain sensitive information
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any potential sensitive field patterns in the schema rg -i "key|token|secret|password|credential" src/configurations/destinations/intercom_v2/schema.jsonLength of output: 102
Script:
#!/bin/bash # First, let's check if the schema.json file exists and see its content fd schema.json src/configurations/destinations/intercom_v2/ --exec cat {} # Also check the db-config.json for context fd db-config.json src/configurations/destinations/intercom_v2/ --exec cat {}Length of output: 23484
src/configurations/destinations/intercom_v2/ui-config.json (3)
41-56: LGTM!The connection mode section is well-structured with appropriate documentation link for user guidance.
59-89: LGTM!The configuration settings section is well-structured with appropriate defaults and clear labeling.
81-87: Verify the relationship between consent settings sections.There are two sections related to consent settings:
- The empty "Consent settings" group in the main configuration
- The detailed "consentSettingsTemplate" at the root level
Please verify if this is intentional or if these should be consolidated.
Also applies to: 96-225
✅ Verification successful
Let me try a different search strategy to understand the consent settings pattern better.
This will help us:
- Find all occurrences of both consent-related sections
- See their context to understand the relationship
- Determine if this pattern is consistent across the codebase
Based on the search results, I can now provide a definitive response about the relationship between the consent settings sections.
The dual consent settings structure is intentional and consistent across destinations
The search results reveal a clear, consistent pattern across all destination configurations:
- The empty
consentSettingsgroup in the main configuration serves as a UI container/section- The
consentSettingsTemplateat the root level contains the actual implementation with fields (typically tagInput type)This separation appears to be a deliberate architectural decision where:
- The empty group provides the UI structure and metadata (title, icon, note)
- The template defines the actual consent configuration fields and behavior
This pattern is consistently implemented across 50+ destination configurations, indicating it's a standardized approach rather than an oversight.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other consent settings implementations to verify the pattern rg -g '*.json' -l 'consentSettingsTemplate.*Consent settings'Length of output: 63
Script:
#!/bin/bash # Search for consentSettings and consentSettingsTemplate separately in JSON files echo "=== Files with consentSettings ===" rg -g '*.json' '"consentSettings"' -A 5 echo -e "\n=== Files with consentSettingsTemplate ===" rg -g '*.json' '"consentSettingsTemplate"' -A 5Length of output: 65880
test/data/validation/destinations/intercom_v2.json (2)
155-156: Verify template expression security.The test cases allow template expressions that could potentially access sensitive event properties. Consider:
- Adding validation for allowed property paths
- Implementing expression sanitization
- Adding tests for malicious expressions
#!/bin/bash # Search for template expressions in configuration files echo "Searching for template expressions..." rg "\{\{.*\}\}" --type json # Look for any sensitive patterns in template expressions echo "Checking for sensitive patterns in templates..." rg "\{\{.*(?i)(secret|key|token|password|credential).*\}\}" --type jsonAlso applies to: 208-209
150-151: Verify security implications of environment variable references.The test cases allow environment variable references in cookie categories and consent purposes. This could potentially expose sensitive data if not properly sanitized.
Please ensure that:
- Only approved environment variables are accessible
- Environment variables are properly sanitized before use
- Access to sensitive environment variables is restricted
Also applies to: 174-175, 203-204, 227-228
src/configurations/destinations/intercom_v2/schema.json (1)
1-697: Verify OAuth configuration handling.The schema enforces cloud-only mode which is required for OAuth, but there are no explicit OAuth configuration properties. Please verify if OAuth credentials and configuration are handled elsewhere (e.g., in
db-config.jsonor through environment variables).✅ Verification successful
Let me check the OAuth configuration in the relevant files to understand how it's handled.
Based on the results, I can now provide a final response as we have clear evidence about how OAuth is handled for Intercom V2.
OAuth configuration is correctly handled through db-config.json
The schema.json correctly enforces cloud-only mode which aligns with the OAuth implementation. The OAuth configuration is properly handled in the destination's db-config.json where it's defined with:
"auth": { "type": "OAuth", "role": "intercom", "rudderScopes": ["delivery"] }The schema.json doesn't need explicit OAuth properties because:
- OAuth credentials are managed through the authentication system defined in db-config.json
- The schema only requires user-configurable settings (
apiServerandsendAnonymousId)- Cloud-only connection mode is enforced, which is a prerequisite for OAuth
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for OAuth-related configurations in other files echo "Searching for OAuth-related configurations..." rg -l "oauth" --type jsonLength of output: 757
Script:
#!/bin/bash # Check the db-config schema to understand OAuth configuration structure cat src/schemas/sources/db-config-schema.json # Also check the actual db-config for Intercom echo "\nChecking Intercom db-config:" fd "db-config.json" --type f | grep -i "intercom" | xargs catLength of output: 11132
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/configurations/destinations/intercom_v2/schema.json (4)
1-8: Add descriptions to improve schema documentation.Consider adding descriptions to help users understand the purpose of each configuration option:
{ "configSchema": { "$schema": "http://json-schema.org/draft-07/schema#", + "description": "Configuration schema for Intercom v2 destination with OAuth support", "required": ["apiServer"], "type": "object", "properties": { - "apiServer": { "type": "string", "enum": ["US", "Europe", "Australia"], "default": "US" }, + "apiServer": { + "type": "string", + "enum": ["US", "Europe", "Australia"], + "default": "US", + "description": "Specifies the Intercom API server region to use for data routing" + }, - "sendAnonymousId": { "type": "boolean", "default": false }, + "sendAnonymousId": { + "type": "boolean", + "default": false, + "description": "Controls whether anonymous user IDs should be sent to Intercom" + },
19-19: Document the consent pattern regex.The pattern
(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{0,100})$accepts:
- Template variables:
{{variable||default}}- Environment variables:
env.VARIABLE_NAME- Direct strings up to 100 characters
Consider adding this documentation in a description field.
148-168: Document resolution strategy configuration.The resolution strategy configuration needs better documentation:
"allOf": [ { "if": { "properties": { "provider": { "const": "custom" } }, "required": ["provider"] }, "then": { "properties": { "resolutionStrategy": { "type": "string", "enum": ["and", "or"], + "description": "Specifies how multiple consents are combined: 'and' requires all consents to be granted, 'or' requires at least one consent to be granted", + "examples": [ + "and - User must consent to all specified categories", + "or - User must consent to at least one category" + ] } }, "required": ["resolutionStrategy"] } } ]
654-694: Document cloud-only mode restriction.Since this integration only supports cloud mode for OAuth authentication, add a description to clarify this limitation:
"connectionMode": { + "description": "Specifies the connection mode for each platform. Currently only cloud mode is supported for OAuth authentication.", "type": "object", "properties": { "web": { "type": "string", - "enum": ["cloud"] + "enum": ["cloud"], + "description": "Only cloud mode is supported for OAuth-based authentication" }, // Apply similar changes for other platforms
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/configurations/destinations/intercom_v2/schema.json (1 hunks)
🔇 Additional comments (1)
src/configurations/destinations/intercom_v2/schema.json (1)
1-697: Schema structure looks good!The schema correctly defines all necessary configurations for the Intercom v2 destination with OAuth support. The structure is valid and includes all required fields for proper integration.
What are the changes introduced in this PR?
Onboarding intercom v2 destination which will support oauth.
What is the related Linear task?
Resolves INT-2474
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