feat: accoil analytics ui #1807
Conversation
accoilmj
requested review from
lvrach,
cisse21,
debanjan97,
am6010,
ssbeefeater,
ruchiramoitra,
ItsSudip,
shrouti1507,
sandeepdsvs,
krishna2020 and
1abhishekpandey
as code owners
WalkthroughThe pull request introduces three new JSON configuration files for the Accoil Analytics destination: 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: 3
🧹 Outside diff range and nitpick comments (7)
src/configurations/destinations/accoil_analytics/db-config.json (2)
14-14: Consider documenting the empty excludeKeys arrayThe
excludeKeysarray is empty. If this is intentional, consider adding a comment in the documentation PR (rudderlabs/rudderstack-docs#1196) explaining why no keys need to be excluded for this destination.
44-115: Consider reducing configuration duplicationThe platform-specific configurations contain identical settings for most platforms. Consider refactoring to use a common configuration template to improve maintainability.
"destConfig": { "defaultConfig": ["apiKey", "blacklistedEvents", "whitelistedEvents", "eventFilteringOption"], + "commonConfig": [ + "connectionMode", + "consentManagement", + "oneTrustCookieCategories", + "ketchConsentPurposes" + ], "web": [ "useNativeSDK", - "connectionMode", - "consentManagement", - "oneTrustCookieCategories", - "ketchConsentPurposes" + ...commonConfig ], // Apply similar pattern to other platformssrc/configurations/destinations/accoil_analytics/ui-config.json (3)
60-64: Clarify purpose of empty SDK Template.The SDK template section is marked as "not visible in the ui" and contains no fields. If this section is not needed, consider removing it to avoid confusion in future maintenance.
81-92: Simplify duplicate feature flag conditions.The OneTrust and Ketch sections have identical feature flag conditions. Consider extracting these to a shared configuration to improve maintainability.
+ "consentFeatureFlags": { + "featureFlags": [ + { + "configKey": "AMP_enable-gcm", + "value": false + }, + { + "configKey": "AMP_enable-gcm" + } + ], + "featureFlagsCondition": "or" + },Then reference this shared configuration in both sections.
Also applies to: 106-117
172-173: Fix inconsistent apostrophe usage.Change
ID'stoIDsfor consistency with other occurrences in the file.-"label": "Enter consent category ID's", +"label": "Enter consent category IDs",src/configurations/destinations/accoil_analytics/schema.json (2)
7-10: Consider strengthening the API key pattern validationThe current pattern
(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{1,100})$allows any string up to 100 characters. Consider adding more specific validation for the API key format to prevent invalid keys."apiKey": { "type": "string", - "pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{1,100})$" + "pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^([a-zA-Z0-9_-]{32,100})$", + "description": "Accoil Analytics API key. Must be 32-100 characters long and contain only alphanumeric characters, underscores, and hyphens." }
845-858: Consider adding validation for native SDK configurationThe
useNativeSDKconfiguration lacks constraints that might be important:
- No default values are specified
- No description of the implications of enabling/disabling native SDK
- No validation to ensure consistent settings across platforms
"useNativeSDK": { "type": "object", + "description": "Configuration for native SDK usage per platform", "properties": { "web": { - "type": "boolean" + "type": "boolean", + "default": false, + "description": "Enable/disable native SDK for web platform" }, "android": { - "type": "boolean" + "type": "boolean", + "default": false, + "description": "Enable/disable native SDK for Android platform" }, "ios": { - "type": "boolean" + "type": "boolean", + "default": false, + "description": "Enable/disable native SDK for iOS platform" } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/configurations/destinations/accoil_analytics/db-config.json(1 hunks)src/configurations/destinations/accoil_analytics/schema.json(1 hunks)src/configurations/destinations/accoil_analytics/ui-config.json(1 hunks)
🔇 Additional comments (9)
src/configurations/destinations/accoil_analytics/db-config.json (5)
116-120: LGTM! Security configuration and beta status are properly set
The API key is correctly marked as secret and the beta status is appropriately indicated.
28-30: Verify supported message types configuration
Message types are only defined for the "cloud" source. Please verify if this is intentional and aligns with the destination implementation in rudderlabs/rudder-transformer#3873.
#!/bin/bash
# Check message type configurations in other destinations
rg -A 5 '"supportedMessageTypes"' src/configurations/destinations/47-47: Verify native SDK support configuration
The useNativeSDK option is only available for web, android, and ios platforms. Please verify this aligns with the actual SDK capabilities in the destination implementation (rudderlabs/rudder-transformer#3873).
#!/bin/bash
# Check native SDK support patterns in other destinations
rg '"useNativeSDK"' src/configurations/destinations/Also applies to: 54-54, 61-61
1-121: Verify schema validation
Please ensure that the configuration aligns with the validation rules defined in the accompanying schema.json file, particularly for required fields and value constraints.
✅ Verification successful
Configuration validated successfully
The db-config.json aligns perfectly with the schema.json requirements:
- Required
apiKeyfield is present in the defaultConfig - All optional fields (oneTrustCookieCategories, connectionMode, consentManagement, ketchConsentPurposes) are properly structured
- Connection modes are correctly set to "cloud" across all platforms
- Platform-specific configurations follow the defined schema patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find and check the corresponding schema file
fd -t f schema.json src/configurations/destinations/accoil_analytics/
Length of output: 24843
Script:
#!/bin/bash
# Let's check the schema file for Accoil Analytics specifically
cat src/configurations/destinations/accoil_analytics/schema.json
Length of output: 25654
7-7: Verify privacy implications of saving destination responses
The configuration enables saving destination responses. Please ensure this doesn't inadvertently store sensitive user data or PII.
✅ Verification successful
Saving destination responses is a common and safe practice
The search results show that saveDestinationResponse is a standard configuration used across many destinations, with most destinations having it enabled (true). This is a built-in feature of the platform that helps with debugging and monitoring integrations. Only a few destinations like GA4, Firebase, GTM, and Pinterest Tag explicitly disable this feature.
The configuration follows the established pattern and doesn't introduce any privacy concerns as:
- It's a system-level configuration for debugging purposes
- The platform has built-in controls through
includeKeysandexcludeKeysto manage what data gets saved - Many sensitive destinations (payment, analytics, etc.) safely use this configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check other destinations for similar settings and any associated privacy controls
rg -l "saveDestinationResponse.*true" src/configurations/destinations/
Length of output: 24843
Script:
#!/bin/bash
# Let's search for saveDestinationResponse configuration in other destinations
rg -A 2 '"saveDestinationResponse"' src/configurations/destinations/
Length of output: 63033
src/configurations/destinations/accoil_analytics/ui-config.json (3)
50-56: Verify empty consent settings section.
The consent settings section has an empty groups array, while detailed consent configurations exist in consentSettingsTemplate. Consider:
- Either removing this empty section if it's not needed
- Or moving the relevant groups from
consentSettingsTemplatehere
#!/bin/bash
# Check if other destinations follow similar pattern
fd -e json -x jq -r 'select(.uiConfig.baseTemplate[].sections[].id == "consentSettings") | input_filename' {} \; 2>/dev/null184-191:
Review conflicting feature flag conditions.
The feature flag AMP_enable-gcm appears with different values across the file:
falsein OneTrust/Ketch sectionstruein the dynamicCustomForm section
This could lead to unexpected behavior where some consent management features are hidden while others are visible.
#!/bin/bash
# Check for other occurrences of this feature flag
rg -l "AMP_enable-gcm" | xargs jq -r 'paths as $p | select(getpath($p) == "AMP_enable-gcm") | $p | join(".")'39-40:
Connection mode section requires configuration.
The fields array is empty, but the section is present with a title and note. This could lead to confusion as users might expect configuration options here.
src/configurations/destinations/accoil_analytics/schema.json (1)
796-844: Verify connection mode constraints
The connection mode is restricted to "cloud" for all platforms. Verify if this is intentional and if other modes might be needed in the future.
✅ Verification successful
Connection mode restriction is appropriate for this destination
The "cloud" only restriction for all platforms in accoil_analytics is consistent with most other destinations. While Adobe Analytics supports both "cloud" and "device" modes for some platforms, this appears to be an exception rather than the norm. Other major destinations like Zendesk, Salesforce, and Snowflake also restrict their connection modes to "cloud" only, indicating this is a standard pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other destinations support additional connection modes
# Search for connection mode configurations in other destination schemas
fd . src/configurations/destinations -e json -x grep -l "connectionMode"
Length of output: 23737
Script:
#!/bin/bash
# Let's check the connection mode configurations in other destination schemas
# We'll sample a few schemas to see if they have different connection modes
# Check Adobe Analytics schema
echo "=== Adobe Analytics Connection Mode ==="
rg -A 10 '"connectionMode":\s*{' src/configurations/destinations/adobe_analytics/schema.json
echo -e "\n=== Zendesk Connection Mode ==="
rg -A 10 '"connectionMode":\s*{' src/configurations/destinations/zendesk/schema.json
echo -e "\n=== Salesforce Connection Mode ==="
rg -A 10 '"connectionMode":\s*{' src/configurations/destinations/salesforce/schema.json
echo -e "\n=== Snowflake Connection Mode ==="
rg -A 10 '"connectionMode":\s*{' src/configurations/destinations/snowflake/schema.json
Length of output: 1120
Script:
#!/bin/bash
# Let's fix the regex and check connection modes in other destination schemas
# Check Adobe Analytics schema
echo "=== Adobe Analytics Connection Mode ==="
rg -A 10 '"connectionMode"' src/configurations/destinations/adobe_analytics/schema.json
echo -e "\n=== Zendesk Connection Mode ==="
rg -A 10 '"connectionMode"' src/configurations/destinations/zendesk/schema.json
echo -e "\n=== Salesforce Connection Mode ==="
rg -A 10 '"connectionMode"' src/configurations/destinations/salesforce/schema.json
echo -e "\n=== Snowflake Connection Mode ==="
rg -A 10 '"connectionMode"' src/configurations/destinations/snowflake/schema.json
Length of output: 1752
src/configurations/destinations/accoil_analytics/ui-config.json
Outdated
Show resolved
Hide resolved
accoilmj
force-pushed
the
accoil-analytics-destination-ui
branch
from
20993ae to
cfcf256
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/configurations/destinations/accoil_analytics/schema.json (1)
796-844: Simplify connection mode configurationThe connection mode configuration can be simplified since all platforms use the same "cloud" enum.
Apply this refactor:
{ "configSchema": { "properties": { "connectionMode": { "type": "object", + "additionalProperties": { + "type": "string", + "enum": ["cloud"] + }, "properties": { - "cloud": { - "type": "string", - "enum": ["cloud"] - }, - "android": { - "type": "string", - "enum": ["cloud"] - }, // ... remove all platform-specific definitions } } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/configurations/destinations/accoil_analytics/db-config.json(1 hunks)src/configurations/destinations/accoil_analytics/schema.json(1 hunks)src/configurations/destinations/accoil_analytics/ui-config.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/configurations/destinations/accoil_analytics/db-config.json
- src/configurations/destinations/accoil_analytics/ui-config.json
🔇 Additional comments (4)
src/configurations/destinations/accoil_analytics/schema.json (4)
1-10: LGTM: Root schema and API key configuration
The schema version and API key validation are properly configured. The pattern allows for environment variables and template strings while limiting the key length to 100 characters.
845-858: LGTM: Native SDK configuration
The Native SDK configuration is well-structured and appropriately limited to web, android, and ios platforms.
11-147: 🛠️ Refactor suggestion
Reduce code duplication in platform configurations
The OneTrust cookie categories configuration is identical across all platforms, leading to significant code duplication.
Apply this refactor to improve maintainability:
{
"configSchema": {
"$schema": "http://json-schema.org/draft-07/schema#",
+ "definitions": {
+ "oneTrustCookieCategory": {
+ "type": "array",
+ "items": {
+ "type": "object",
+ "properties": {
+ "oneTrustCookieCategory": {
+ "type": "string",
+ "pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{0,100})$"
+ }
+ }
+ }
+ }
+ },
"properties": {
"oneTrustCookieCategories": {
"type": "object",
"properties": {
- "cloud": {
- "type": "array",
- "items": {
- "type": "object",
- "properties": {
- "oneTrustCookieCategory": {
- "type": "string",
- "pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{0,100})$"
- }
- }
- }
- },
+ "cloud": { "$ref": "#/definitions/oneTrustCookieCategory" },
+ "android": { "$ref": "#/definitions/oneTrustCookieCategory" },
// ... repeat for other platforms
}
}
}
}
}285-795: 🛠️ Refactor suggestion
Improve consent management configuration structure
The consent management configuration has significant duplication and could be better structured.
Apply this refactor:
{
"configSchema": {
"definitions": {
+ "consentConfig": {
+ "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": {
- "type": "array",
- "items": { ... }
- },
+ "cloud": { "$ref": "#/definitions/consentConfig" },
+ "android": { "$ref": "#/definitions/consentConfig" },
// ... repeat for other platforms
}
}
}
}
}
src/configurations/destinations/accoil_analytics/db-config.json
Outdated
Show resolved
Hide resolved
src/configurations/destinations/accoil_analytics/db-config.json
Outdated
Show resolved
Hide resolved
src/configurations/destinations/accoil_analytics/db-config.json
Outdated
Show resolved
Hide resolved
src/configurations/destinations/accoil_analytics/ui-config.json
Outdated
Show resolved
Hide resolved
Co-authored-by: Manish Kumar <[email protected]>
Co-authored-by: Manish Kumar <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/configurations/destinations/accoil_analytics/schema.json (1)
285-795: Consider adding validation for empty consent arrays.While the conditional logic for resolution strategy is well implemented, consider adding a minimum length requirement for the consents array when a provider is specified. This would prevent configurations with a provider but no actual consents.
Example validation to add (shown for one platform, apply to all):
{ "if": { "properties": { "provider": { "type": "string" } }, "required": ["provider"] }, "then": { "properties": { "consents": { "minItems": 1 } }, "required": ["consents"] } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/configurations/destinations/accoil_analytics/db-config.json(1 hunks)src/configurations/destinations/accoil_analytics/schema.json(1 hunks)src/configurations/destinations/accoil_analytics/ui-config.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/configurations/destinations/accoil_analytics/ui-config.json
- src/configurations/destinations/accoil_analytics/db-config.json
🧰 Additional context used
📓 Learnings (1)
src/configurations/destinations/accoil_analytics/schema.json (3)
Learnt from: accoilmj
PR: rudderlabs/rudder-integrations-config#1807
File: src/configurations/destinations/accoil_analytics/schema.json:148-284
Timestamp: 2024-12-09T04:17:28.675Z
Learning: The file `src/configurations/destinations/accoil_analytics/schema.json` is autogenerated. Avoid suggesting changes to autogenerated files.
Learnt from: accoilmj
PR: rudderlabs/rudder-integrations-config#1807
File: src/configurations/destinations/accoil_analytics/schema.json:285-795
Timestamp: 2024-12-09T04:17:16.931Z
Learning: The file `src/configurations/destinations/accoil_analytics/schema.json` is autogenerated.
Learnt from: accoilmj
PR: rudderlabs/rudder-integrations-config#1807
File: src/configurations/destinations/accoil_analytics/schema.json:11-147
Timestamp: 2024-12-09T04:17:05.013Z
Learning: The file `src/configurations/destinations/accoil_analytics/schema.json` is autogenerated and should not be manually modified.
🔇 Additional comments (3)
src/configurations/destinations/accoil_analytics/schema.json (3)
1-10: LGTM! Root schema configuration is well-structured.
The schema correctly enforces API key requirements with a secure pattern that supports both direct values and environment variables.
796-844: LGTM! Connection mode configuration is consistent.
The schema correctly enforces cloud-only mode across all platforms, which is appropriate for this analytics destination.
845-858: LGTM! Native SDK configuration is appropriately scoped.
The schema correctly limits native SDK configuration to relevant platforms (web, android, ios) with simple boolean toggles.
|
Hi @manish339k just checking in if there's anything further I need to work on here? Thanks. |
What are the changes introduced in this PR?
Adds new UI for 'Accoil Analytics' destination.
Please explain the objectives of your changes below
Adds UI for Accoil Analytics.
Destination code PR: rudderlabs/rudder-transformer#3873
Documentation PR: rudderlabs/rudderstack-docs#1196
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
Release Notes
These updates enhance user experience and streamline the integration of Accoil Analytics.