551 google data ingress #628
Conversation
aaryansinha16
force-pushed
the
551-google-data-ingress
branch
3 times, most recently
from
7fa5830
to
f3b1bbc
Compare
aaryansinha16
force-pushed
the
551-google-data-ingress
branch
5 times, most recently
from
f7e5f41
to
927692c
Compare
aaryansinha16
force-pushed
the
551-google-data-ingress
branch
2 times, most recently
from
376c0d7
to
c20d736
Compare
|
|
||
| const url = `https://googleads.googleapis.com/${apiVersion}/customers/${customerId ?? managerId}/googleAds:search`; | ||
|
|
||
| const defaultQuery = ` |
There was a problem hiding this comment.
I think in both cases you are passing a query so let's delete it and make query mandatatory. Same with customerId
There was a problem hiding this comment.
let's rename it to something more appropriate, like customerQuery
aaryansinha16
force-pushed
the
551-google-data-ingress
branch
2 times, most recently
from
0523677
to
bcb024b
Compare
aaryansinha16
force-pushed
the
551-google-data-ingress
branch
2 times, most recently
from
9865bc7
to
cea5684
Compare
aaryansinha16
force-pushed
the
551-google-data-ingress
branch
from
cea5684
to
0be2850
Compare
There was a problem hiding this comment.
resolved the vast majoriy of the past comments - good job.
added some new, and we have pending that:
- when i tried to ingress data, my insights were not saved
- add the call back url in the channel prod
- make sure that both credential work (we may need to add ourselved as test users)
| if (validatedResponse.results) { | ||
| const managers = validatedResponse.results | ||
| .map((result) => result.customerClient) | ||
| .filter((custoemer) => custoemer.manager) |
| } | ||
|
|
||
| private static async findManagerAccount(accessToken: string, customerIds: string[]): Promise<string | AError> { | ||
| for (const customerId of customerIds) { |
There was a problem hiding this comment.
I think when we have one customerId we don't need to go through this process. Even if this customerId is not a manager, they will have access to their own account.
| dbAccount: DbAdAccount, | ||
| initial: boolean, | ||
| ): Promise<AError | undefined> { | ||
| if (!integration.refreshToken) return new AError('Refresh token is required'); |
There was a problem hiding this comment.
do we need this line here?
|
|
||
| for (const range of ranges) { | ||
| try { | ||
| const refreshedIntegration = await this.refreshedIntegration(integration); |
There was a problem hiding this comment.
we should do this inside the handlePagination, applicable else where.
| customer_client.level, | ||
| customer_client.manager, | ||
| customer_client.descriptive_name |
There was a problem hiding this comment.
I think you don't need level and descriptive_name?
| integrationData.accessTokenExpiresAt = new Date(tokens.accessTokenExpiresAt ?? new Date()); | ||
| integrationData.refreshTokenExpiresAt = tokens.refreshTokenExpiresAt | ||
| ? new Date(tokens.refreshTokenExpiresAt) | ||
| : undefined; |
There was a problem hiding this comment.
my understanding if that if access/refresh token has not expiration date, then it does not expire. You have found a contradictory example?
| return res; | ||
| } | ||
|
|
||
| private static parseRequest(signedRequest: string, secret: string): string | AError { |
There was a problem hiding this comment.
in both places it seems that is doing the same thing and is used from the same function, the signOutCallback. My thought process was to move to channel-utils and call it from there.
| }); | ||
| } | ||
|
|
||
| private static async refreshedIntegration(integration: Integration): Promise<Integration | AError> { |
There was a problem hiding this comment.
I think there are places that we can move it a bit more nested still, for example in handlePagination
| asset.resource_name IN (${el.adGroupAd.ad.videoResponsiveAd.videos.map((c) => `'${c.asset}'`).join(',')}) | ||
| `; | ||
|
|
||
| const response = await fetch(url, { |
There was a problem hiding this comment.
can we use Google.handlePangination here? I think the url is the same...
|
|
||
| const url = `https://googleads.googleapis.com/${apiVersion}/customers/${customerId ?? managerId}/googleAds:search`; | ||
|
|
||
| const defaultQuery = ` |
There was a problem hiding this comment.
let's rename it to something more appropriate, like customerQuery
No description provided.