feat: gaec migration #1783
feat: gaec migration #1783
Conversation
WalkthroughThe changes focus on updating the configuration files for Google Ads Enhanced Conversions. The Changes
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1783 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 53 53
Branches 7 7
=========================================
Hits 53 53 ☔ View full report in Codecov by Sentry. |
aashishmalik
force-pushed
the
feat.gaec-migration
branch
4 times, most recently
from
d05f09b to
929ec90
Compare
aashishmalik
force-pushed
the
feat.gaec-migration
branch
from
929ec90 to
53f1f79
Compare
aashishmalik
requested review from
lvrach,
cisse21,
debanjan97,
am6010,
ssbeefeater,
AchuthaSourabhC,
ruchiramoitra,
ItsSudip,
shrouti1507,
sandeepdsvs,
krishna2020 and
1abhishekpandey
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/configurations/destinations/google_adwords_enhanced_conversions/db-config.json (1)
45-45: Document the migration process.Since this is part of the GAEC migration (INT-XXX), please add documentation that covers:
- The new configuration structure using
configData- Migration steps for existing implementations
- Any required changes in the destination setup process
src/configurations/destinations/google_adwords_enhanced_conversions/ui-config.json (1)
16-29: Consider adding validation feedback.The current implementation could benefit from additional UX improvements:
- Add loading state indication during API calls
- Include error state handling for API failures
- Add validation feedback for selected accounts
Consider adding these properties to enhance user feedback:
{ "type": "nestedSelect", "label": "Select account", "apiName": "getConfigData", "configKey": "configData", "placeholder": "Select account details", + "loadingLabel": "Fetching accounts...", + "errorLabel": "Failed to load accounts. Please try again.", + "validation": { + "required": true, + "messages": { + "required": "Please select an account" + } + }, "preRequisites": {src/configurations/destinations/google_adwords_enhanced_conversions/schema.json (2)
12-15: Add documentation for requireHash property.The purpose and implications of this flag aren't clear. Please add:
- Description field explaining when hashing is required
- Documentation about the impact of this setting
- Examples of when it should be true/false
"requireHash": { "type": "boolean", - "default": true + "default": true, + "description": "Specify whether enhanced conversion values should be hashed before sending to Google Ads" },
16-27: Enhance validation for listOfConversions.The current implementation could benefit from additional constraints:
- No limits on array size could lead to performance issues
- Pattern allows very short strings that might be invalid
- Missing description of expected conversion format
"listOfConversions": { "type": "array", + "minItems": 1, + "maxItems": 100, + "description": "List of conversion actions to be tracked", "items": { "type": "object", "properties": { "conversions": { "type": "string", - "pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{0,100})$" + "pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{3,100})$", + "description": "Conversion action ID or template expression" } }, + "required": ["conversions"], + "additionalProperties": false } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/configurations/destinations/google_adwords_enhanced_conversions/db-config.json(1 hunks)src/configurations/destinations/google_adwords_enhanced_conversions/schema.json(1 hunks)src/configurations/destinations/google_adwords_enhanced_conversions/ui-config.json(1 hunks)
🔇 Additional comments (5)
src/configurations/destinations/google_adwords_enhanced_conversions/db-config.json (2)
45-45: Ensure secure handling of sensitive data.
The consolidation of account configuration into configData requires careful handling of sensitive information. Please verify:
- The
configDatafield is properly encrypted in storage - No sensitive information is exposed in logs
#!/bin/bash
# Description: Check if configData is included in the secretKeys array
# in any related configuration files
echo "Checking for proper handling of configData..."
rg -A 10 '"secretKeys":\s*\[' \
--glob 'src/configurations/destinations/google_adwords_enhanced_conversions/*.json'45-45:
Verify backward compatibility of the configuration changes.
The removal of direct configuration fields (customerId, subAccount, loginCustomerId) in favor of configData could potentially impact existing implementations. Please ensure:
- Existing destinations continue to work after this change
- A migration path exists for existing configurations
✅ Verification successful
Based on the comprehensive analysis of the configuration files, I can now provide the final response:
Configuration changes are backward compatible and well-structured
The changes to the Google Ads Enhanced Conversions configuration are safe and backward compatible for the following reasons:
- The removed fields (
customerId,subAccount,loginCustomerId) are now encapsulated within the newconfigDatafield, which has a default value that maintains the customer ID structure:{"customerId": "123"} - The configuration is obtained through an API endpoint (
getConfigData), ensuring proper account selection through the UI - The schema enforces proper validation with required fields and maintains the same pattern validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing usage of removed fields in other configuration files
# and deployment manifests to assess the impact.
echo "Checking for usage of removed fields..."
rg -l "customerId|subAccount|loginCustomerId" \
--type json \
--glob '!**/node_modules/**' \
--glob '!**/dist/**' \
--glob '!**/build/**'
Length of output: 900
Script:
#!/bin/bash
# Let's try a more comprehensive search to understand the configuration changes
# 1. Search for any references to the removed fields in the entire codebase
echo "Searching for references to removed fields..."
rg -A 5 "customerId|subAccount|loginCustomerId" \
--type-add 'config:*.{json,js,ts,jsx,tsx}' \
--type config \
--glob '!**/node_modules/**' \
--glob '!**/dist/**' \
--glob '!**/build/**'
# 2. Check the specific destination configuration structure
echo -e "\nChecking destination configuration structure..."
cat src/configurations/destinations/google_adwords_enhanced_conversions/db-config.json
# 3. Look for any migration or upgrade related files
echo -e "\nSearching for migration documentation..."
fd -e md -e txt . | xargs rg -l "google.*enhanced.*conversion.*migrat|GAEC.*migrat"
Length of output: 65656
Script:
#!/bin/bash
# Let's check the schema and UI configuration to understand how configData is structured
echo "Checking schema and UI configuration..."
cat src/configurations/destinations/google_adwords_enhanced_conversions/schema.json
echo -e "\nChecking UI configuration..."
cat src/configurations/destinations/google_adwords_enhanced_conversions/ui-config.json
# Also check for any related test files that might show the migration path
echo -e "\nChecking for related test files..."
fd -t f -e json -e js -e ts . | grep -i "google.*adwords.*enhanced.*conversion.*test"
Length of output: 16657
src/configurations/destinations/google_adwords_enhanced_conversions/ui-config.json (2)
16-20: Improved UI for account selection.
The change from text input to a nested select with API integration is a good improvement that will reduce user error and provide a better user experience.
24-25: Verify migration path for existing configurations.
The change from manual Customer ID input to API-based selection needs careful consideration for existing configurations.
Let's verify the presence of existing configurations that might be affected:
Also applies to: 28-29
✅ Verification successful
Migration path for existing configurations is handled appropriately
The git history shows that this is a new destination configuration file (added in commit 53f1f79) where the customerId field was directly replaced with the API-based account selection in the same commit. Since this is the initial commit adding this destination, there are no existing configurations that would be affected by this change.
The change from manual Customer ID input to API-based selection is safe because:
- This is a new destination being introduced
- The change was made in the initial commit itself
- The API endpoint will handle the customer ID selection through
configDatafield, maintaining backward compatibility through the schema default value:{"customerId": "123"}
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing configurations using the old customerId field
rg -l '"customerId"' src/configurations/destinations/google_adwords_enhanced_conversions/
Length of output: 91
Script:
#!/bin/bash
# Let's check the git history for any changes related to customerId in this configuration
git log -p -- src/configurations/destinations/google_adwords_enhanced_conversions/ui-config.json
# Also search for any references to customerId in the entire codebase
rg "customerId" -A 2 -B 2
Length of output: 57066
src/configurations/destinations/google_adwords_enhanced_conversions/schema.json (1)
5-6:
Breaking changes detected in schema requirements.
The changes introduce two breaking changes that contradict the PR objectives:
- Making
configDatarequired will break existing configurations that don't have this field - Setting
additionalPropertiesto false will reject configurations with any custom fields
Consider:
- Adding a migration plan for existing configurations
- Documenting these breaking changes in the PR description
- Potentially making these changes backward compatible
Let's verify if there are existing configurations that would break:
src/configurations/destinations/google_adwords_enhanced_conversions/schema.json
Show resolved
Hide resolved
src/configurations/destinations/google_adwords_enhanced_conversions/ui-config.json
Show resolved
Hide resolved
What are the changes introduced in this PR?
Write a brief explainer on your code changes.
What is the related Linear task?
Resolves INT-XXX
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
Documentation