feat(garl): move GARL destination to new form builder #1715
Conversation
sandeepdsvs
requested review from
lvrach,
cisse21,
debanjan97,
am6010,
ssbeefeater,
AchuthaSourabhC,
ruchiramoitra,
ItsSudip,
shrouti1507,
krishna2020 and
1abhishekpandey
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## release/v1.97.0 #1715 +/- ##
=================================================
Coverage 100.00% 100.00%
=================================================
Files 2 2
Lines 53 53
Branches 7 7
=================================================
Hits 53 53 ☔ View full report in Codecov by Sentry. |
|
This PR is considered to be stale. It has been open for 20 days with no further activity thus it is going to be closed in 7 days. To avoid such a case please consider removing the stale label manually or add a comment to the PR. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes involve modifications to three configuration files for Google Ads Remarketing Lists. In Changes
Possibly related PRs
Suggested reviewers
Poem
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: 2
🧹 Outside diff range and nitpick comments (3)
src/configurations/destinations/google_adwords_remarketing_lists/ui-config.json (3)
89-206: Approve consent-related additions and suggest improvement for Schema FieldsThe addition of user data consent and personalization consent fields is a positive change, aligning with modern data privacy practices and regulations.
The conditional display of "Schema Fields" based on the "List Type" improves the user experience by showing only relevant options.
Consider adding a brief explanation or tooltip for each schema field option to help users understand what each field represents and its implications. For example:
{ "label": "Email", - "value": "email" + "value": "email", + "description": "User's email address for remarketing" }, { "label": "Phone Number", - "value": "phone" + "value": "phone", + "description": "User's phone number for remarketing" }, { "label": "Address Info", - "value": "addressInfo" + "value": "addressInfo", + "description": "User's address information for location-based remarketing" }This addition would enhance user understanding and help them make informed decisions when configuring the destination.
Line range hint
207-350: Approve consent settings changes and suggest improvementsThe updates to the consent settings, particularly the change to
tagInputfor OneTrust and Ketch configurations, simplify the input process and improve user experience.Consider the following improvements:
For the empty
groupsarray in the "Consent settings" section:{ "id": "consentSettings", "title": "Consent settings", "note": "Configure consent settings for each provider here", "icon": "settings", - "groups": [] + "groups": [], + "footerNote": "Additional consent configuration options will be available in future updates." }For the custom consent management configuration:
- Add input validation for the "Enter consent category ID's" field to ensure proper formatting.
- Consider adding a help link or tooltip explaining the difference between "AND" and "OR" consent logic.
{ "type": "tagInput", "label": "Enter consent category ID's", "note": "Input your consent category IDs by pressing 'Enter' after each entry. The support for category names is deprecated. We recommend using the category IDs instead of the names as IDs are unique and less likely to change over time, making them a more reliable choice.", "configKey": "consents", "tagKey": "consent", "placeholder": "e.g: Marketing", + "regex": "^[A-Za-z0-9_-]{1,50}$", + "regexErrorMessage": "Consent category ID should be 1-50 alphanumeric characters, underscores, or hyphens", "default": [ { "consent": "" } ] }These changes would enhance user guidance and data validation in the consent management section.
217-221: Approve addition of sdkTemplate and suggest clarity improvementThe addition of the
sdkTemplatesection is a good preparation for future SDK-specific configurations.To improve clarity and maintainability, consider adding a more descriptive comment about the purpose of this section and when it might be populated. For example:
"sdkTemplate": { "title": "SDK settings", - "note": "not visible in the ui", + "note": "This section is reserved for future SDK-specific configurations and is not visible in the UI. It will be populated as new SDK features are implemented.", "fields": [] },This change would provide better context for other developers who might work on this configuration in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/configurations/destinations/google_adwords_remarketing_lists/db-config.json (1 hunks)
- src/configurations/destinations/google_adwords_remarketing_lists/ui-config.json (5 hunks)
🧰 Additional context used
🔇 Additional comments (1)
src/configurations/destinations/google_adwords_remarketing_lists/ui-config.json (1)
Line range hint
1-353: Summary: Approve overall changes and alignment with PR objectivesThe restructuring of the
ui-config.jsonfile for the Google AdWords Remarketing Lists destination aligns well with the PR objective of moving GARL destination to a new form builder. The changes introduce several improvements:
- Enhanced organization with a hierarchical structure (
baseTemplateandsdkTemplate).- Improved user guidance through additional
notefields and clearer labeling.- Integration of consent management options, aligning with modern data privacy practices.
- Preparation for future expansions with placeholder sections like
sdkTemplate.These changes should result in a more maintainable configuration and an improved user experience when setting up the GARL destination.
To ensure these changes don't introduce any breaking changes or inconsistencies, please run the following verification:
This script will help identify any potential issues with the transition to the new structure.
src/configurations/destinations/google_adwords_remarketing_lists/db-config.json
Show resolved
Hide resolved
src/configurations/destinations/google_adwords_remarketing_lists/ui-config.json
Outdated
Show resolved
Hide resolved
|
attach screenshots or loom video |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/configurations/destinations/google_adwords_remarketing_lists/schema.json (2)
39-48: Consider documenting consent state implicationsThe consent management configuration is well-structured, but would benefit from documentation explaining:
- The difference between
userDataConsentandpersonalizationConsent- The implications of each consent state (UNSPECIFIED, UNKNOWN, GRANTED, DENIED)
- How these settings affect data handling in Google Ads
Would you like me to help create documentation for these consent settings?
22-25: Consider a more descriptive property nameThe
subAccountboolean property could be more descriptive. Consider renaming toisSubAccountoruseSubAccountto better indicate its purpose.- "subAccount": { + "isSubAccount": { "type": "boolean", "default": false },src/configurations/destinations/google_adwords_remarketing_lists/ui-config.json (1)
Line range hint
238-249: Simplify duplicate feature flag prerequisitesThe same feature flag prerequisites are repeated multiple times. Consider extracting these common conditions into a shared configuration.
Example approach:
{ "commonPrerequisites": { "featureFlags": [ { "configKey": "AMP_enable-gcm", "value": false }, { "configKey": "AMP_enable-gcm" } ], "featureFlagsCondition": "or" } }Then reference this shared configuration where needed.
Also applies to: 265-276
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/configurations/destinations/google_adwords_remarketing_lists/schema.json (1 hunks)
- src/configurations/destinations/google_adwords_remarketing_lists/ui-config.json (5 hunks)
🔇 Additional comments (7)
src/configurations/destinations/google_adwords_remarketing_lists/schema.json (3)
6-9: LGTM! Good security defaults and data type restrictionsThe configuration for user data handling is well-structured with secure defaults:
isHashRequired: trueensures data hashing by defaultuserSchemaproperly restricts to known data typestypeOfListaligns with Google Ads remarketing list typesAlso applies to: 26-38
6-48: Verify required properties and consent integrationPlease consider:
- Should any of the new properties be marked as required? For example,
customerIdmight be mandatory for Google Ads integration.- How do the new consent properties (
userDataConsent,personalizationConsent) interact with existing consent mechanisms (oneTrustCookieCategories,ketchConsentPurposes)?#!/bin/bash # Search for usage of consent-related code rg -g '*.{js,jsx,ts,tsx}' -A 5 'oneTrustCookieCategories|ketchConsentPurposes|userDataConsent|personalizationConsent'
10-21: Verify ID field patterns against Google Ads API requirementsThe regex pattern for
audienceId,customerId, andloginCustomerIdallows for template variables, environment variables, and direct values up to 100 characters. Please verify that this aligns with Google Ads API requirements for these identifiers.✅ Verification successful
Let me gather more specific information about these fields.
Based on the search results, I can now generate the final response since we have sufficient context from both the schema and UI configuration files.
ID field patterns are correctly implemented and validated
The patterns for
audienceId,customerId, andloginCustomerIdare properly implemented with:
- Consistent validation across schema.json and ui-config.json
- Appropriate example formats in placeholders (e.g., "9693X29833")
- Clear error messages for invalid inputs
- Support for template variables and environment variables as required
- 100-character limit that aligns with typical Google Ads ID formats
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing usage of these fields to ensure pattern compatibility rg -g '*.{js,jsx,ts,tsx}' -A 2 'audienceId|customerId|loginCustomerId'Length of output: 4994
Script:
#!/bin/bash # Search for implementation details and usage patterns of these specific fields rg -g '*.{js,jsx,ts,tsx,json}' -A 5 'audienceId|customerId|loginCustomerId' src/configurations/destinations/google_adwords_remarketing_lists/Length of output: 5409
src/configurations/destinations/google_adwords_remarketing_lists/ui-config.json (4)
2-88: LGTM: Well-structured connection settingsThe new hierarchical structure with proper field validation and prerequisites handling is well-implemented. The connection settings group provides clear organization of related fields.
89-216: LGTM: Comprehensive configuration settingsThe configuration settings section is well-structured with:
- Clear list type options
- Proper consent management fields
- Conditional schema fields based on list type
217-221: Verify if empty SDK template section is neededThe SDK template section is marked as "not visible in the ui" and contains no fields. Please confirm if this section is necessary for future use or can be removed.
Line range hint
222-351: LGTM: Improved consent management implementationThe transition to tagInput for consent management and support for multiple consent providers is well-implemented. The deprecation notice for category names is clear and provides good guidance.
src/configurations/destinations/google_adwords_remarketing_lists/ui-config.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/configurations/destinations/google_adwords_remarketing_lists/schema.json (2)
7-26: Document the regex pattern constraintsThe string fields use a common regex pattern that allows:
- Template variables:
{{...||...}}- Environment variables:
env.*- Direct values (limited to 100 characters)
Consider adding JSDoc comments to explain these constraints and provide examples.
Example documentation:
"audienceId": { + "description": "Audience ID for remarketing list. Supports template variables ({{var||default}}), environment variables (env.*), or direct values (max 100 chars).", + "examples": ["{{audienceId||123}}", "env.AUDIENCE_ID", "123456789"], "type": "string", "pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{0,100})$" },
35-49: Review default values for consent fieldsSetting
UNSPECIFIEDas the default for consent fields (userDataConsentandpersonalizationConsent) might not align with privacy-first practices. Consider:
- The implications of processing data with unspecified consent
- Whether a more restrictive default like
DENIEDwould be more appropriate- If consent should be explicitly set rather than having a default
Consider these privacy-focused improvements:
- Remove defaults for consent fields to force explicit consent decisions
- Add validation to ensure consent is specified when certain features are used
- Document the privacy implications of each consent value
test/data/validation/destinations/google_adwords_remarketing_lists.json (1)
4-5: Use realistic test data for customerId field.The
customerIdvalue "dfashethdetyjetyj" appears to be a placeholder. Consider using realistic test data that matches Google Ads customer ID format (e.g., "123-456-7890") to make the tests more meaningful.- "customerId": "dfashethdetyjetyj", + "customerId": "123-456-7890",Also applies to: 57-58
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/configurations/destinations/google_adwords_remarketing_lists/schema.json (1 hunks)
- test/data/validation/destinations/google_adwords_remarketing_lists.json (5 hunks)
🔇 Additional comments (3)
src/configurations/destinations/google_adwords_remarketing_lists/schema.json (2)
27-34: 🛠️ Refactor suggestionConsider adding format validation for user schema fields
While the schema defines allowed values for user data (
phone,addressInfo), it doesn't specify format validation for these fields. This could lead to invalid data being accepted.Let's check if format validation exists elsewhere:
#!/bin/bash # Search for any validation logic related to these fields rg -A 5 -B 5 'email|phone|addressInfo' --type ts --type jsConsider adding format validation:
"userSchema": { "type": "array", "items": { "type": "string", "enum": ["email", "phone", "addressInfo"] + "if": { "const": "email" }, + "then": { + "format": "email" + }, + "else": { + "if": { "const": "phone" }, + "then": { + "pattern": "^\\+[1-9]\\d{1,14}$" + } + } }, "default": ["email"] },
5-5:⚠️ Potential issue
⚠️ Breaking Change: New required fields addedAdding
customerIdandsubAccountas required fields is a breaking change that could affect existing configurations. This contradicts the PR objective stating "no breaking changes".Let's verify the impact:
Consider one of these approaches:
- Make these fields optional to maintain backward compatibility
- Provide migration scripts for existing configurations
- Document this as a breaking change and increment the major version
test/data/validation/destinations/google_adwords_remarketing_lists.json (1)
Line range hint
1-197: Verify schema consistency across configuration files.Let's ensure the test cases align with the schema definition and UI configuration.
src/configurations/destinations/google_adwords_remarketing_lists/db-config.json
Show resolved
Hide resolved
What are the changes introduced in this PR?
Move GARL destination to new form builder
What is the related Linear task?
Resolves INT-2806
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
Summary by CodeRabbit
New Features
audienceIdfield for improved audience identification.Improvements