Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: adding clevertap segment support #1822

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

shrouti1507
Copy link
Contributor

@shrouti1507 shrouti1507 commented Nov 27, 2024

What are the changes introduced in this PR?

Write a brief explainer on your code changes.

What is the related Linear task?

Resolves INT-2765

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

    • Introduced new configuration files for the CleverTap segment, enabling enhanced connection and consent management.
    • Added a comprehensive validation schema for configuration settings, ensuring proper structure and data integrity.
    • Developed a user interface configuration for managing connection settings and consent management dynamically.
  • Tests

    • Implemented test configurations to validate consent management settings across multiple platforms, improving error handling and validation robustness.

Copy link

coderabbitai bot commented Nov 27, 2024

Walkthrough

A series of new JSON configuration and schema files have been added for the "CLEVERTAP_SEGMENT" destination. These include a main configuration file detailing connection settings and consent management, a schema for validating configuration structures, and a user interface configuration. Additionally, test configurations for validating consent management settings across multiple platforms have been introduced. The changes establish a structured framework for managing and validating configurations related to the CleverTap integration.

Changes

File Path Change Summary
src/configurations/destinations/clevertap_segment/db-config.json Introduced a new configuration file for "CLEVERTAP_SEGMENT" with settings for transformation, audience support, and required parameters.
src/configurations/destinations/clevertap_segment/schema.json Added a new schema file defining validation rules for configuration settings, including required properties and regex patterns.
src/configurations/destinations/clevertap_segment/ui-config.json Created a new UI configuration file outlining connection settings, consent management, and SDK settings.
test/data/validation/destinations/clevertap_segment.json Introduced test configurations for validating consent management settings across various platforms, including error handling scenarios.

Suggested reviewers

  • lvrach
  • am6010
  • AchuthaSourabhC
  • krishna2020
  • ssbeefeater
  • debanjan97
  • cisse21
  • ruchiramoitra
  • 1abhishekpandey
  • ItsSudip
  • sandeepdsvs

🐰 In the garden, we hop and play,
New configs bloom, brightening the day.
CleverTap's here, with settings so neat,
Consent and keys, all ready to greet!
With schemas and tests, our work is done,
Hopping with joy, let’s have some fun! 🌼


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between d4ac4b9 and ce20dd1.

📒 Files selected for processing (1)
  • src/configurations/destinations/clevertap_segment/db-config.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/configurations/destinations/clevertap_segment/db-config.json

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Experiment)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

codecov bot commented Nov 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (0e55f6a) to head (ce20dd1).
Report is 19 commits behind head on develop.

Additional details and impacted files
@@            Coverage Diff            @@
##           develop     #1822   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines           53        53           
  Branches         7         7           
=========================================
  Hits            53        53           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (8)
src/configurations/destinations/clevertap_segment/schema.json (2)

65-98: Standardize consent provider configurations

The oneTrustCookieCategories and ketchConsentPurposes sections have similar structures but different validation rules. Consider:

  1. Standardizing the validation requirements
  2. Adding required field constraints
  3. Enforcing minimum array items

Consider applying these improvements:

   "oneTrustCookieCategories": {
     "type": "object",
     "properties": {
       "warehouse": {
         "type": "array",
+        "minItems": 1,
         "items": {
           "type": "object",
           "properties": {
             "oneTrustCookieCategory": {
               "type": "string",
               "pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{0,100})$"
             }
           },
+          "required": ["oneTrustCookieCategory"]
         }
       }
     },
+    "required": ["warehouse"]
   },

Apply similar changes to ketchConsentPurposes section.


2-100: Improve schema maintainability and strictness

Consider these schema-wide improvements:

  1. Prevent additional properties to ensure strict validation
  2. Define common patterns as reusable definitions
  3. Add schema description and examples

Consider applying these enhancements:

   "configSchema": {
     "$schema": "http://json-schema.org/draft-07/schema#",
+    "description": "Configuration schema for CleverTap segment destination",
     "type": "object",
+    "additionalProperties": false,
+    "definitions": {
+      "envVarPattern": {
+        "type": "string",
+        "pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{0,100})$"
+      }
+    },
     "properties": {
       "appKey": {
-        "type": "string",
-        "pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{1,100})$"
+        "$ref": "#/definitions/envVarPattern"
       },
test/data/validation/destinations/clevertap_segment.json (3)

1-11: Add test title for better clarity

The first test case lacks a descriptive title. Consider adding a testTitle field to clearly indicate that this is testing the minimal valid configuration.

 {
+  "testTitle": "Should accept minimal valid configuration",
   "config": {
     "appKey": "dfregth",

128-160: Consider adding more test coverage for consent management

The multiple consent providers test case could be enhanced by:

  1. Adding validation for empty consents array in the Ketch provider
  2. Including a test case for the 'and' resolution strategy
  3. Testing the interaction between different providers' consent states

161-230: Add test cases for additional error scenarios

Consider adding test cases for:

  1. Invalid consent format or structure
  2. Duplicate provider configurations
  3. Mixed case provider names (e.g., "OneTrust" vs "oneTrust")
  4. Invalid environment variable references

This will ensure robust validation of the configuration schema.

src/configurations/destinations/clevertap_segment/ui-config.json (3)

20-23: Document the regex pattern requirements

The regex pattern (^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{1,100})$ is complex and would benefit from documentation explaining the accepted formats:

  • Template variables: {{...||...}}
  • Environment variables: env.VARIABLE
  • Direct values: Up to 100 characters

79-90: Refactor duplicate feature flag conditions

The feature flag prerequisites are duplicated in both OneTrust and Ketch sections. Consider extracting these into a shared configuration to improve maintainability.

 {
+  "commonPreRequisites": {
+    "featureFlags": [
+      {
+        "configKey": "AMP_enable-gcm",
+        "value": false
+      },
+      {
+        "configKey": "AMP_enable-gcm"
+      }
+    ],
+    "featureFlagsCondition": "or"
+  },
   "consentSettingsTemplate": {
     // ... other config ...
     "fields": [
       {
         // ... OneTrust config ...
-        "preRequisites": {
-          "featureFlags": [
-            {
-              "configKey": "AMP_enable-gcm",
-              "value": false
-            },
-            {
-              "configKey": "AMP_enable-gcm"
-            }
-          ],
-          "featureFlagsCondition": "or"
-        }
+        "preRequisites": "$ref: #/commonPreRequisites"
       },
       {
         // ... Ketch config ...
-        "preRequisites": {
-          "featureFlags": [
-            {
-              "configKey": "AMP_enable-gcm",
-              "value": false
-            },
-            {
-              "configKey": "AMP_enable-gcm"
-            }
-          ],
-          "featureFlagsCondition": "or"
-        }
+        "preRequisites": "$ref: #/commonPreRequisites"
       }
     ]
   }
 }

Also applies to: 104-115


170-171: Fix inconsistent apostrophe usage

Change "ID's" to "IDs" for consistency with other occurrences in the file.

-          "label": "Enter consent category ID's",
+          "label": "Enter consent category IDs",
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 0e55f6a and 570fbf8.

📒 Files selected for processing (4)
  • src/configurations/destinations/clevertap_segment/db-config.json (1 hunks)
  • src/configurations/destinations/clevertap_segment/schema.json (1 hunks)
  • src/configurations/destinations/clevertap_segment/ui-config.json (1 hunks)
  • test/data/validation/destinations/clevertap_segment.json (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • src/configurations/destinations/clevertap_segment/db-config.json
🔇 Additional comments (6)
src/configurations/destinations/clevertap_segment/schema.json (1)

1-101: Verify schema against CleverTap's API requirements

Please ensure that the schema aligns with CleverTap's API requirements, particularly:

  1. Field naming conventions
  2. Required vs optional fields
  3. Value constraints and limitations
✅ Verification successful

Schema aligns with CleverTap's API requirements

The schema validation is correct based on the test data which demonstrates:

  • Required fields appKey and accessToken are properly enforced
  • Pattern validation for string fields limits to 100 characters
  • Proper structure for consent management with supported providers (custom, ketch, oneTrust)
  • Correct validation of resolution strategies for custom consent provider
  • Proper handling of environment variables and template strings
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for CleverTap integration tests or examples in the codebase
rg -g '!node_modules' -g '*.{json,js,ts}' -i 'clevertap.*test|test.*clevertap'

# Look for existing CleverTap configurations
fd -g '*clevertap*.json' -E 'node_modules'

Length of output: 229


Script:

#!/bin/bash
# Check test data for CleverTap configuration examples
cat test/data/validation/destinations/clevertap_segment.json test/data/validation/destinations/clevertap.json

# Search for CleverTap API-related code or documentation
rg -g '!node_modules' -g '!*.test.*' -g '*.{js,ts}' -i 'clevertap.*api|api.*clevertap' -A 5

# Look for any CleverTap transformation logic
ast-grep --pattern 'function $FUNC($$$) {
  $$$
  clevertap
  $$$
}'

Length of output: 17013

test/data/validation/destinations/clevertap_segment.json (2)

12-127: Verify template expression compatibility and add documentation

A few suggestions to improve the platform-specific consent configurations:

  1. The template expression uses single quotes which might cause issues in some environments:
-            "oneTrustCookieCategory": "{{ event.properties.prop1 || 'val' }}"
+            "oneTrustCookieCategory": "{{ event.properties.prop1 || \"val\" }}"
  1. Empty arrays for Unity and Cordova platforms should be documented to explain if this is intentional.

  2. Consider extracting duplicate platform configurations into shared test fixtures to improve maintainability.


4-5: Ensure test credentials are clearly marked as dummy values

The test file uses dummy values for appKey and accessToken. Consider:

  1. Adding a comment to explicitly indicate these are test values
  2. Using more obvious dummy values (e.g., "DUMMY_APP_KEY", "DUMMY_ACCESS_TOKEN")

Also applies to: 131-132

✅ Verification successful

Let me check the content of some of these test files to verify if they contain real credentials or properly marked test values.


Let me check the specific file in question to see if there are any comments or patterns around the credentials.


Test credentials should be more clearly marked as dummy values

Looking at the test file content, the credentials are clearly non-real test values ("dfregth", "345fghyj"), but they could be made more explicit:

  • Replace "dfregth" with "DUMMY_APP_KEY"
  • Replace "345fghyj" with "DUMMY_ACCESS_TOKEN"
  • Add a comment at the top of the file indicating these are test credentials

The current values, while obviously fake, could be made more self-documenting to prevent any confusion for new contributors.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for potential real credentials in test files
rg -l '[A-Za-z0-9]{32,}' --type json test/

Length of output: 11642


Script:

#!/bin/bash
# Sample a few test files to check credential patterns and comments
for file in test/data/validation/destinations/{clevertap,facebook_pixel,google_adwords_enhanced_conversions,salesforce}.json; do
  echo "=== $file ==="
  rg -A 2 -B 2 '(key|token|secret|password|credential|apiKey)' "$file" || true
done

Length of output: 4789


Script:

#!/bin/bash
# Check the specific clevertap_segment.json file
cat test/data/validation/destinations/clevertap_segment.json

Length of output: 5123

src/configurations/destinations/clevertap_segment/ui-config.json (3)

1-194: Security review approved

The configuration properly handles sensitive information:

  • Sensitive fields are correctly marked as secret
  • Input validation is in place for all fields
  • Consent management structure follows best practices

182-189: Document feature flag behavior

The feature flag prerequisite for the dynamic custom form has different conditions compared to the individual provider sections. Consider adding a note explaining why AMP_enable-gcm needs to be true here but has different logic elsewhere.


50-51: Empty fields array in connection mode section

The connection mode section has an empty fields array. Is this intentional? If not, consider adding relevant connection mode options.

✅ Verification successful

Based on the search results, I can see that having an empty fields array in the connection mode section is actually a common pattern across many destination configurations in the codebase. This appears to be the standard implementation when no specific connection mode fields are needed, as evidenced by similar empty fields arrays in destinations like LinkedIn Ads, Zoho, X Audience, Wunderkind, and many others.

Empty fields array is intentional for connection mode section

The empty fields array is the expected configuration when no additional connection mode options are required. This is consistent with the destination's configuration pattern.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if other destinations have connection mode fields defined
rg -l '"connection mode"' src/configurations/destinations/

Length of output: 60


Script:

#!/bin/bash
# Let's check for any fields array in the destination configs and their structure
rg -U --multiline '"fields": \[.*?\]' -A 5 src/configurations/destinations/

Length of output: 55540


Script:

#!/bin/bash
# Let's specifically look at the clevertap_segment config file
cat src/configurations/destinations/clevertap_segment/ui-config.json

Length of output: 6228


Script:

#!/bin/bash
# Let's check other destination configs for similar patterns
fd . src/configurations/destinations/ -e json -x cat {}

Length of output: 114083

Comment on lines +6 to +13
"appKey": {
"type": "string",
"pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{1,100})$"
},
"accessToken": {
"type": "string",
"pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{1,100})$"
},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Mark accessToken as sensitive and consider additional validation

The accessToken field should be marked as sensitive to ensure proper handling of credentials. Consider adding:

  1. The secret: true property for the accessToken field
  2. Additional pattern validation for direct values to enforce minimum security requirements

Apply this diff to enhance security:

   "accessToken": {
     "type": "string",
+    "secret": true,
     "pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{1,100})$"
   },
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
"appKey": {
"type": "string",
"pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{1,100})$"
},
"accessToken": {
"type": "string",
"pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{1,100})$"
},
"appKey": {
"type": "string",
"pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{1,100})$"
},
"accessToken": {
"type": "string",
"secret": true,
"pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{1,100})$"
},

Comment on lines +14 to +64
"consentManagement": {
"type": "object",
"properties": {
"warehouse": {
"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"]
}
}
]
}
}
}
},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance consent management validation rules

The consent management structure could be improved with additional validation rules:

  1. Missing required fields at the root level
  2. No minimum items validation for arrays
  3. No validation for empty consent arrays

Consider applying these enhancements:

   "consentManagement": {
     "type": "object",
     "properties": {
       "warehouse": {
         "type": "array",
+        "minItems": 1,
         "items": {
           "type": "object",
           "properties": {
             "provider": {
               "type": "string",
               "enum": ["custom", "ketch", "oneTrust"],
               "default": "oneTrust"
             },
             "consents": {
               "type": "array",
+              "minItems": 1,
               "items": {
                 "type": "object",
                 "properties": {
                   "consent": {
                     "type": "string",
                     "pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{0,100})$"
                   }
                 },
+                "required": ["consent"]
               }
             }
           },
+          "required": ["provider", "consents"]
         }
       }
     },
+    "required": ["warehouse"]
   },
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
"consentManagement": {
"type": "object",
"properties": {
"warehouse": {
"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"]
}
}
]
}
}
}
},
"consentManagement": {
"type": "object",
"properties": {
"warehouse": {
"type": "array",
"minItems": 1,
"items": {
"type": "object",
"properties": {
"provider": {
"type": "string",
"enum": ["custom", "ketch", "oneTrust"],
"default": "oneTrust"
},
"consents": {
"type": "array",
"minItems": 1,
"items": {
"type": "object",
"properties": {
"consent": {
"type": "string",
"pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{0,100})$"
}
},
"required": ["consent"]
}
}
},
"required": ["provider", "consents"],
"allOf": [
{
"if": {
"properties": {
"provider": {
"const": "custom"
}
},
"required": ["provider"]
},
"then": {
"properties": {
"resolutionStrategy": {
"type": "string",
"enum": ["and", "or"]
}
},
"required": ["resolutionStrategy"]
}
}
]
}
}
},
"required": ["warehouse"]
},

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
src/configurations/destinations/clevertap_segment/schema.json (2)

71-87: Add required field validation for OneTrust cookie categories

Consider adding required field validation to ensure oneTrustCookieCategory is provided when the object is present.

   "oneTrustCookieCategories": {
     "type": "object",
     "properties": {
       "warehouse": {
         "type": "array",
         "items": {
           "type": "object",
           "properties": {
             "oneTrustCookieCategory": {
               "type": "string",
               "pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{0,100})$"
             }
           },
+          "required": ["oneTrustCookieCategory"]
         }
       }
     }
   },

88-104: Add required field validation for Ketch consent purposes

Consider adding required field validation to ensure purpose is provided when the object is present.

   "ketchConsentPurposes": {
     "type": "object",
     "properties": {
       "warehouse": {
         "type": "array",
         "items": {
           "type": "object",
           "properties": {
             "purpose": {
               "type": "string",
               "pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{0,100})$"
             }
           },
+          "required": ["purpose"]
         }
       }
     }
   }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 0e97915 and d4ac4b9.

📒 Files selected for processing (4)
  • src/configurations/destinations/clevertap_segment/db-config.json (1 hunks)
  • src/configurations/destinations/clevertap_segment/schema.json (1 hunks)
  • src/configurations/destinations/clevertap_segment/ui-config.json (1 hunks)
  • test/data/validation/destinations/clevertap_segment.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • test/data/validation/destinations/clevertap_segment.json
  • src/configurations/destinations/clevertap_segment/db-config.json
🔇 Additional comments (7)
src/configurations/destinations/clevertap_segment/schema.json (3)

1-11: LGTM! Root schema configuration is well-structured.

The schema correctly defines required fields and region enumeration matches CleverTap's available regions.


12-19: Mark accessToken as sensitive and consider additional validation

The accessToken field should be marked as sensitive to ensure proper handling of credentials.


20-70: Enhance consent management validation rules

The consent management structure could be improved with additional validation rules.

src/configurations/destinations/clevertap_segment/ui-config.json (4)

1-89: LGTM! Base template configuration is well-structured.

The connection settings are properly configured with appropriate security measures and validation.


95-148: LGTM! Consent settings template is comprehensive.

The configuration properly handles both OneTrust and Ketch consent management with appropriate feature flag controls.


149-222: LGTM! Dynamic custom form configuration is well-implemented.

The form properly handles consent management configuration with appropriate validation and conditional fields.


90-94: Verify if SDK template configuration is intentionally empty

The SDK template section has no fields and is marked as "not visible in the ui". Please verify if this is intentional or if SDK-specific configuration will be added later.

✅ Verification successful

Empty SDK template configuration is consistent with other destinations

The empty SDK template configuration with "not visible in the ui" note appears to be intentional, as other major destinations (Facebook Pixel, GA4, Braze) either have null or empty SDK template sections. This is likely a standard pattern for destinations that don't require SDK-specific configurations in the UI.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if other destinations have SDK template configurations
rg -l '"sdkTemplate":\s*{[^}]+fields":\s*\[[^\]]+\]' src/configurations/destinations/

Length of output: 232


Script:

#!/bin/bash
# Check if other destinations have SDK template configurations with non-empty fields
rg --json -g 'ui-config.json' '"sdkTemplate"' src/configurations/destinations/ | jq -r '.data.path'

# Get content of SDK template sections from other destinations
rg -U --multiline '"sdkTemplate"[\s\S]*?}(?=,|\s*})' src/configurations/destinations/

Length of output: 14599


Script:

#!/bin/bash
# Check content of a few sample destination configs to understand the pattern
cat src/configurations/destinations/facebook_pixel/ui-config.json | jq '.sdkTemplate'
cat src/configurations/destinations/ga4/ui-config.json | jq '.sdkTemplate'
cat src/configurations/destinations/braze/ui-config.json | jq '.sdkTemplate'

Length of output: 252

Co-authored-by: Sandeep Digumarty <sandeepdsvs@gmail.com>
Comment on lines +19 to +21
"oneTrustCookieCategories",
"ketchConsentPurposes",
"consentManagement"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you confirm if consent management fields apply to this destination?

@@ -0,0 +1,226 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add appropriate note to all the fields instead of leaving it blank

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants