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(cdp): support nested google ad accounts #28531

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
10 changes: 5 additions & 5 deletions frontend/src/lib/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -847,13 +847,13 @@ class ApiRequest {

public integrationGoogleAdsConversionActions(
id: IntegrationType['id'],
customerId: string,
params: { customerId: string; parentId: string },
teamId?: TeamType['id']
): ApiRequest {
return this.integrations(teamId)
.addPathComponent(id)
.addPathComponent('google_conversion_actions')
.withQueryString({ customerId })
.withQueryString(params)
}

public integrationLinkedInAdsAccounts(id: IntegrationType['id'], teamId?: TeamType['id']): ApiRequest {
Expand Down Expand Up @@ -2701,14 +2701,14 @@ const api = {
},
async googleAdsAccounts(
id: IntegrationType['id']
): Promise<{ accessibleAccounts: { id: string; name: string }[] }> {
): Promise<{ accessibleAccounts: { id: string; name: string; level: string; parent_id: string }[] }> {
return await new ApiRequest().integrationGoogleAdsAccounts(id).get()
},
async googleAdsConversionActions(
id: IntegrationType['id'],
customerId: string
params: { customerId: string; parentId: string }
): Promise<{ conversionActions: GoogleAdsConversionActionType[] }> {
return await new ApiRequest().integrationGoogleAdsConversionActions(id, customerId).get()
return await new ApiRequest().integrationGoogleAdsConversionActions(id, params).get()
},
async linkedInAdsAccounts(id: IntegrationType['id']): Promise<{ adAccounts: LinkedInAdsAccountType[] }> {
return await new ApiRequest().integrationLinkedInAdsAccounts(id).get()
Expand Down
8 changes: 4 additions & 4 deletions frontend/src/lib/integrations/GoogleAdsIntegrationHelpers.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ import { GoogleAdsConversionActionType, IntegrationType } from '~/types'
import { googleAdsIntegrationLogic } from './googleAdsIntegrationLogic'

const getGoogleAdsAccountOptions = (
googleAdsAccounts?: { id: string; name: string }[] | null
googleAdsAccounts?: { id: string; name: string; parent_id: string; level: string }[] | null
): LemonInputSelectOption[] | null => {
return googleAdsAccounts
? googleAdsAccounts.map((customer) => ({
key: customer.id,
key: `${customer.id}/${customer.parent_id}`,
labelComponent: (
<span className="flex items-center">
{customer.name} ({customer.id.replace(/(\d{3})(\d{3})(\d{4})/, '$1-$2-$3')})
Expand Down Expand Up @@ -65,7 +65,7 @@ export function GoogleAdsConversionActionPicker({

useEffect(() => {
if (requiresFieldValue) {
loadGoogleAdsConversionActions(requiresFieldValue)
loadGoogleAdsConversionActions(requiresFieldValue.split('/')[0], requiresFieldValue.split('/')[1])
}
Comment on lines 67 to 69
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Add error handling for malformed requiresFieldValue that doesn't contain '/'

Suggested change
if (requiresFieldValue) {
loadGoogleAdsConversionActions(requiresFieldValue)
loadGoogleAdsConversionActions(requiresFieldValue.split('/')[0], requiresFieldValue.split('/')[1])
}
if (requiresFieldValue?.includes('/')) {
loadGoogleAdsConversionActions(requiresFieldValue.split('/')[0], requiresFieldValue.split('/')[1])
}

}, [loadGoogleAdsConversionActions, requiresFieldValue])

Expand All @@ -78,7 +78,7 @@ export function GoogleAdsConversionActionPicker({
!googleAdsConversionActions &&
!googleAdsConversionActionsLoading &&
requiresFieldValue &&
loadGoogleAdsConversionActions(requiresFieldValue)
loadGoogleAdsConversionActions(requiresFieldValue.split('/')[0], requiresFieldValue.split('/')[1])
}
disabled={disabled}
mode="single"
Expand Down
17 changes: 13 additions & 4 deletions frontend/src/lib/integrations/googleAdsIntegrationLogic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,30 @@ export const googleAdsIntegrationLogic = kea<googleAdsIntegrationLogicType>([
key((props) => props.id),
path((key) => ['lib', 'integrations', 'googleAdsIntegrationLogic', key]),
actions({
loadGoogleAdsConversionActions: (customerId: string) => customerId,
loadGoogleAdsConversionActions: (customerId: string, parentId: string) => ({ customerId, parentId }),
loadGoogleAdsAccessibleAccounts: true,
}),
loaders(({ props }) => ({
googleAdsConversionActions: [
null as GoogleAdsConversionActionType[] | null,
{
loadGoogleAdsConversionActions: async (customerId: string) => {
const res = await api.integrations.googleAdsConversionActions(props.id, customerId)
loadGoogleAdsConversionActions: async ({
customerId,
parentId,
}: {
customerId: string
parentId: string
}) => {
const res = await api.integrations.googleAdsConversionActions(props.id, {
customerId,
parentId,
})
return res.conversionActions
},
},
],
googleAdsAccessibleAccounts: [
null as { id: string; name: string }[] | null,
null as { id: string; level: string; parent_id: string; name: string }[] | null,
{
loadGoogleAdsAccessibleAccounts: async () => {
const res = await api.integrations.googleAdsAccounts(props.id)
Expand Down
5 changes: 3 additions & 2 deletions posthog/api/integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,9 @@ def conversion_actions(self, request: Request, *args: Any, **kwargs: Any) -> Res
instance = self.get_object()
google_ads = GoogleAdsIntegration(instance)
customer_id = request.query_params.get("customerId")
parent_id = request.query_params.get("parentId")

conversion_actions = google_ads.list_google_ads_conversion_actions(customer_id)
conversion_actions = google_ads.list_google_ads_conversion_actions(customer_id, parent_id)

if len(conversion_actions) == 0:
return Response({"conversionActions": []})
Expand All @@ -126,7 +127,7 @@ def conversion_actions(self, request: Request, *args: Any, **kwargs: Any) -> Res
"name": conversionAction["conversionAction"]["name"],
"resourceName": conversionAction["conversionAction"]["resourceName"],
}
for conversionAction in google_ads.list_google_ads_conversion_actions(customer_id)[0]["results"]
for conversionAction in google_ads.list_google_ads_conversion_actions(customer_id, parent_id)[0]["results"]
]

return Response({"conversionActions": conversion_actions})
Expand Down
7 changes: 4 additions & 3 deletions posthog/cdp/templates/google_ads/template_google_ads.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
'conversions': [
{
'gclid': inputs.gclid,
'conversion_action': f'customers/{replaceAll(inputs.customerId, '-', '')}/conversionActions/{inputs.conversionActionId}',
'conversion_action': f'customers/{splitByString('/', inputs.customerId)[1]}/conversionActions/{inputs.conversionActionId}',
'conversion_date_time': inputs.conversionDateTime
}
],
Expand All @@ -38,11 +38,12 @@
body.conversions[1].order_id := inputs.orderId
}

let res := fetch(f'https://googleads.googleapis.com/v18/customers/{replaceAll(inputs.customerId, '-', '')}:uploadClickConversions', {
let res := fetch(f'https://googleads.googleapis.com/v18/customers/{splitByString('/', inputs.customerId)[1]}:uploadClickConversions', {
'method': 'POST',
'headers': {
'Authorization': f'Bearer {inputs.oauth.access_token}',
'Content-Type': 'application/json'
'Content-Type': 'application/json',
'login-customer-id': splitByString('/', inputs.customerId)[2]
},
'body': body
})
Expand Down
3 changes: 2 additions & 1 deletion posthog/cdp/templates/google_ads/test_template_google_ads.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def _inputs(self, **kwargs):
"oauth": {
"access_token": "oauth-1234",
},
"customerId": "123-123-1234",
"customerId": "1231231234/5675675678",
"conversionActionId": "123456789",
"gclid": "89y4thuergnjkd34oihroh3uhg39uwhgt9",
"conversionDateTime": "2024-10-10 16:32:45+02:00",
Expand All @@ -32,6 +32,7 @@ def test_function_works(self):
"headers": {
"Authorization": "Bearer oauth-1234",
"Content-Type": "application/json",
"login-customer-id": "5675675678",
},
"body": {
"conversions": [
Expand Down
61 changes: 47 additions & 14 deletions posthog/models/integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,7 @@ def __init__(self, integration: Integration) -> None:
def client(self) -> WebClient:
return WebClient(self.integration.sensitive_config["access_token"])

def list_google_ads_conversion_actions(self, customer_id) -> list[dict]:
def list_google_ads_conversion_actions(self, customer_id, parent_id=None) -> list[dict]:
response = requests.request(
"POST",
f"https://googleads.googleapis.com/v18/customers/{customer_id}/googleAds:searchStream",
Expand All @@ -490,6 +490,7 @@ def list_google_ads_conversion_actions(self, customer_id) -> list[dict]:
"Content-Type": "application/json",
"Authorization": f"Bearer {self.integration.sensitive_config['access_token']}",
"developer-token": settings.GOOGLE_ADS_DEVELOPER_TOKEN,
**({"login-customer-id": parent_id} if parent_id else {}),
},
)

Expand All @@ -501,6 +502,8 @@ def list_google_ads_conversion_actions(self, customer_id) -> list[dict]:

return response.json()

# Google Ads manager accounts can have access to other accounts (including other manager accounts).
# Filter out duplicates where a user has direct access and access through a manager account, while prioritizing direct access.
def list_google_ads_accessible_accounts(self) -> list[dict[str, str]]:
response = requests.request(
"GET",
Expand All @@ -516,35 +519,65 @@ def list_google_ads_accessible_accounts(self) -> list[dict[str, str]]:
capture_exception(Exception(f"GoogleAdsIntegration: Failed to list accessible accounts: {response.text}"))
raise Exception(f"There was an internal error")

accounts = response.json()
accounts_with_name = []
accessible_accounts = response.json()
all_accounts: list[dict[str, str]] = []

for account in accounts["resourceNames"]:
def dfs(account_id, accounts=None, parent_id=None) -> list[dict]:
if accounts is None:
accounts = []
response = requests.request(
"POST",
f"https://googleads.googleapis.com/v18/customers/{account.split('/')[1]}/googleAds:searchStream",
f"https://googleads.googleapis.com/v18/customers/{account_id}/googleAds:searchStream",
json={
"query": "SELECT customer_client.descriptive_name, customer_client.client_customer FROM customer_client WHERE customer_client.level <= 1"
"query": "SELECT customer_client.descriptive_name, customer_client.client_customer, customer_client.level, customer_client.manager, customer_client.status FROM customer_client WHERE customer_client.level <= 5"
},
headers={
"Content-Type": "application/json",
"Authorization": f"Bearer {self.integration.sensitive_config['access_token']}",
"developer-token": settings.GOOGLE_ADS_DEVELOPER_TOKEN,
**({"login-customer-id": parent_id} if parent_id else {}),
},
)

if response.status_code != 200:
continue
return []

data = response.json()
accounts_with_name.append(
{
"id": account.split("/")[1],
"name": data[0]["results"][0]["customerClient"].get("descriptiveName", "Google Ads account"),
}
)

return accounts_with_name
for nested_account in data[0]["results"]:
if any(
account["id"] == nested_account["customerClient"]["clientCustomer"].split("/")[1]
and account["level"] > nested_account["customerClient"]["level"]
for account in accounts
):
accounts = [
account
for account in accounts
if account["id"] != nested_account["customerClient"]["clientCustomer"].split("/")[1]
]
elif any(
account["id"] == nested_account["customerClient"]["clientCustomer"].split("/")[1]
and account["level"] < nested_account["customerClient"]["level"]
for account in accounts
):
continue
if nested_account["customerClient"].get("status") != "ENABLED":
continue
Comment on lines +564 to +565
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider logging disabled accounts for debugging purposes instead of silently skipping

accounts.append(
{
"parent_id": parent_id,
"id": nested_account["customerClient"].get("clientCustomer").split("/")[1],
"level": nested_account["customerClient"].get("level"),
"name": nested_account["customerClient"].get("descriptiveName", "Google Ads account"),
}
)

return accounts

for account in accessible_accounts["resourceNames"]:
all_accounts = dfs(account.split("/")[1], all_accounts, account.split("/")[1])

return all_accounts


class GoogleCloudIntegration:
Expand Down
Loading