feat(cdp): support nested google ad accounts #28531
Conversation
MarconLP
changed the title
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
MarconLP
added
waiting
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
|
Size Change: -195 kB (-1.97%) Total Size: 9.73 MB
|
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Added support for nested Google Ads accounts by implementing parent-child account relationships and indirect access handling across the integration stack.
- Added DFS traversal in
list_google_ads_accessible_accountsto discover nested accounts up to level 5, with duplicate filtering that prioritizes direct access - Modified
customerIdformat to include parent ID (e.g.1231231234/5675675678), used for thelogin-customer-idheader when accessing indirect accounts - Added validation in
list_google_ads_accessible_accountsto only include enabled accounts and proper error handling for API responses - Updated frontend components to handle new account structure with parent_id and level fields while maintaining backward compatibility
- Potential issue: Updating hog function from old version will reset customer ID field, requiring migration handling
7 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
| if (requiresFieldValue) { | ||
| loadGoogleAdsConversionActions(requiresFieldValue) | ||
| loadGoogleAdsConversionActions(requiresFieldValue.split('/')[0], requiresFieldValue.split('/')[1]) | ||
| } |
There was a problem hiding this comment.
style: Add error handling for malformed requiresFieldValue that doesn't contain '/'
| if (requiresFieldValue) { | |
| loadGoogleAdsConversionActions(requiresFieldValue) | |
| loadGoogleAdsConversionActions(requiresFieldValue.split('/')[0], requiresFieldValue.split('/')[1]) | |
| } | |
| if (requiresFieldValue?.includes('/')) { | |
| loadGoogleAdsConversionActions(requiresFieldValue.split('/')[0], requiresFieldValue.split('/')[1]) | |
| } |
posthog/models/integration.py
Outdated
| # if nested_account["customerClient"].get("manager") == True and int(nested_account["customerClient"].get("level")) >= 1: | ||
| # accounts = dfs(nested_account["customerClient"].get("clientCustomer").split("/")[1], accounts, parent_id) |
There was a problem hiding this comment.
logic: Commented-out DFS recursion code suggests incomplete implementation of nested account traversal. Either remove or implement the recursive traversal.
| if nested_account["customerClient"].get("status") != "ENABLED": | ||
| continue |
There was a problem hiding this comment.
style: Consider logging disabled accounts for debugging purposes instead of silently skipping
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
Problem
At the moment we only support accounts on level 0.
Changes
parent_idas part of thecustomer_id. We'll need to include this value in thelogin-customer-idheader, when accessing accounts with indirect access (level 2 or greater).Does this work well for both Cloud and self-hosted?
How did you test this code?
Did manual testing to ensure sending events back into Google Ads is still working.