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.

Sign up for GitHub

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

Source Amazon Ads: added adId to product report stream #11660

Merged
merged 8 commits into from
Apr 26, 2022
Merged

Source Amazon Ads: added adId to product report stream #11660

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
01c3c88
added adId to product report stream
agroh1 Apr 1, 2022
376c56f
Merge branch 'airbytehq:master' into issue-11659
agroh1 Apr 4, 2022
00ccf78
simplified removal of adId
agroh1 Apr 8, 2022
44ae708
removed duplicate comment
agroh1 Apr 8, 2022
b9c7dc1
Bumped dockerfile version
agroh1 Apr 8, 2022
46ddcf7
Merge branch 'issue-11659' of github.com:agroh1/airbyte into marcos/t…
marcosmarxm Apr 20, 2022
5f87976
bump connector version
marcosmarxm Apr 20, 2022
0dfa74a
Merge branch 'master' into marcos/test-pr-11659
marcosmarxm Apr 21, 2022
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
11 changes: 11 additions & 0 deletions .../connectors/source-amazon-ads/source_amazon_ads/streams/report_streams/products_report.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@
"campaignId",
"adGroupName",
"adGroupId",
"adId",
"impressions",
"clicks",
"cost",
Expand Down Expand Up @@ -156,6 +157,7 @@
"adGroupId",
"keywordId",
"keywordText",
"adId",
"asin",
"otherAsin",
"sku",
Expand All @@ -179,6 +181,7 @@
"campaignId",
"adGroupName",
"adGroupId",
"adId",
"asin",
"otherAsin",
"sku",
Expand Down Expand Up @@ -255,9 +258,17 @@ def _get_init_report_body(self, report_date: str, record_type: str, profile):
body = {
"reportDate": report_date,
}

# adId is automatically added to the report by amazon and requesting adId causes an amazon error
if RecordType.ASINS in record_type:
body["campaignType"] = "sponsoredProducts"
metrics_list = copy(metrics_list)
metrics_list.remove("adId")
if profile.accountInfo.type == "vendor":
metrics_list = copy(metrics_list)
metrics_list.remove("sku")
elif RecordType.PRODUCTADS in record_type:
metrics_list = copy(metrics_list)
metrics_list.remove("adId")

Copy link
Member

Choose a reason for hiding this comment

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

    # adId is automatically....
    if "adId" in metrics_list:
        metrics_list.remove("adId")

This way if you add a new report you don't need to change the code again to make it works.
Why is the reason you're using copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that is probably better. It does assume that all product reports have this problem but I think that is probably correct.

I made a copy because the old code that removed "sku" also made a copy and I assumed that it was probably the right thing.

Copy link
Member

Choose a reason for hiding this comment

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

For those cases we can improve the if logic

if "adId" in metrics_list and not SpecialReport:
   metrics_list.remove("adId")

because right now both need to remove it right?

return {**body, "metrics": ",".join(metrics_list)}