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
🎉 Source Amazon Ads : use optional config report_record_types #18677
Changes from 62 commits
4ea218d
4dc3df5
5a57c67
b559c1a
ddfc8d5
b14a827
5b3261f
559447c
f1c0628
e080398
18b28dc
8367e32
8b919dc
a601922
bf3a425
995c78f
3c5fdb9
ab8a412
f1876a0
838de4d
6b28639
0df73da
ab77c36
a5d0b43
e1815da
587e4e2
81155bf
c2551c8
465920c
e73ae2e
68cfe5e
fc00a4c
e6a4720
f9ec87c
a9eb708
141f49b
15f7864
549eece
6d3158a
bee8928
c87573c
641dca2
64ec3a6
eb13adb
da68abe
e62c6ec
4f95353
45aba9d
976e009
e80a2e5
7a5be40
98d7011
d931d57
45117c7
ca1a104
3ed9aa0
eb2c393
8a0cf6e
88569f6
6ff35e0
a9efcb9
baeff65
6c190c5
fa5ece1
c83c309
8d5ca20
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -122,6 +122,8 @@ def __init__(self, config: Mapping[str, Any], profiles: List[Profile], authentic | |||||
# Maximum retries Airbyte will attempt for fetching report data. Default is 5. | ||||||
self.report_generation_maximum_retries: int = get_typed_env("REPORT_GENERATION_MAX_RETRIES", 5) | ||||||
|
||||||
self._report_record_types = config.get("report_record_types", []) | ||||||
ganpatagarwal marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
@property | ||||||
def model(self) -> CatalogModel: | ||||||
return self._model | ||||||
|
@@ -364,6 +366,9 @@ def _init_reports(self, profile: Profile, report_date: str) -> List[ReportInfo]: | |||||
""" | ||||||
report_infos = [] | ||||||
for record_type, metrics in self.metrics_map.items(): | ||||||
if len(self._report_record_types) > 0 and record_type not in self._report_record_types: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
When There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's on |
||||||
continue | ||||||
|
||||||
report_init_body = self._get_init_report_body(report_date, record_type, profile) | ||||||
if not report_init_body: | ||||||
continue | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -486,7 +486,6 @@ def test_read_incremental_with_records(config): | |
stream = SponsoredDisplayReportStream(config, profiles, authenticator=mock.MagicMock()) | ||
|
||
with freeze_time("2021-01-02 12:00:00") as frozen_datetime: | ||
|
||
state = {} | ||
records = list(read_incremental(stream, state)) | ||
assert state == {"1": {"reportDate": "20210102"}} | ||
|
@@ -531,7 +530,6 @@ def test_read_incremental_without_records_start_date(config): | |
stream = SponsoredDisplayReportStream(config, profiles, authenticator=mock.MagicMock()) | ||
|
||
with freeze_time("2021-01-02 12:00:00") as frozen_datetime: | ||
|
||
state = {} | ||
reportDates = ["20201231", "20210101", "20210102", "20210103", "20210104"] | ||
for reportDate in reportDates: | ||
|
@@ -554,7 +552,6 @@ def test_read_incremental_with_records_start_date(config): | |
stream = SponsoredDisplayReportStream(config, profiles, authenticator=mock.MagicMock()) | ||
|
||
with freeze_time("2021-01-02 12:00:00") as frozen_datetime: | ||
|
||
state = {} | ||
records = list(read_incremental(stream, state)) | ||
|
||
|
@@ -643,3 +640,148 @@ def test_streams_state_filter(mocker, config, state_filter, stream_class): | |
assert params["stateFilter"] == ",".join(state_filter) | ||
else: | ||
assert state_filter is None | ||
|
||
|
||
@responses.activate | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a test for Sponsored Products report and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have added tests. Please review |
||
@pytest.mark.parametrize( | ||
"custom_record_types, flag_match_error", | ||
[ | ||
( | ||
["campaigns"], | ||
True | ||
), | ||
( | ||
["campaigns", "adGroups"], | ||
True | ||
), | ||
( | ||
[], | ||
False | ||
), | ||
( | ||
["invalid_record_type"], | ||
True | ||
) | ||
] | ||
) | ||
def test_display_report_stream_with_custom_record_types(config_gen, custom_record_types, flag_match_error): | ||
setup_responses( | ||
init_response=REPORT_INIT_RESPONSE, | ||
status_response=REPORT_STATUS_RESPONSE, | ||
metric_response=METRIC_RESPONSE, | ||
) | ||
|
||
profiles = make_profiles() | ||
|
||
stream = SponsoredDisplayReportStream(config_gen(report_record_types=custom_record_types), profiles, authenticator=mock.MagicMock()) | ||
stream_slice = {"profile": profiles[0], "reportDate": "20210725"} | ||
records = list(stream.read_records(SyncMode.incremental, stream_slice=stream_slice)) | ||
for record in records: | ||
if record['recordType'] not in custom_record_types: | ||
if flag_match_error: | ||
assert False | ||
|
||
|
||
@responses.activate | ||
@pytest.mark.parametrize( | ||
"custom_record_types, expected_record_types, flag_match_error", | ||
[ | ||
( | ||
["campaigns"], | ||
["campaigns"], | ||
True | ||
), | ||
( | ||
["asins_keywords"], | ||
["asins_keywords"], | ||
True | ||
), | ||
( | ||
["asins_targets"], | ||
["asins_targets"], | ||
True | ||
), | ||
( | ||
["campaigns", "adGroups"], | ||
["campaigns", "adGroups"], | ||
True | ||
), | ||
( | ||
[], | ||
[], | ||
False | ||
), | ||
( | ||
["invalid_record_type"], | ||
[], | ||
True | ||
) | ||
] | ||
) | ||
def test_products_report_stream_with_custom_record_types(config_gen, custom_record_types, expected_record_types, flag_match_error): | ||
setup_responses( | ||
init_response_products=REPORT_INIT_RESPONSE, | ||
status_response=REPORT_STATUS_RESPONSE, | ||
metric_response=METRIC_RESPONSE, | ||
) | ||
|
||
profiles = make_profiles(profile_type="vendor") | ||
|
||
stream = SponsoredProductsReportStream(config_gen(report_record_types=custom_record_types), profiles, authenticator=mock.MagicMock()) | ||
stream_slice = {"profile": profiles[0], "reportDate": "20210725", "retry_count": 3} | ||
records = list(stream.read_records(SyncMode.incremental, stream_slice=stream_slice)) | ||
for record in records: | ||
print(record) | ||
if record['recordType'] not in expected_record_types: | ||
if flag_match_error: | ||
assert False | ||
|
||
|
||
@responses.activate | ||
@pytest.mark.parametrize( | ||
"custom_record_types, expected_record_types, flag_match_error", | ||
[ | ||
( | ||
["campaigns"], | ||
["campaigns"], | ||
True | ||
), | ||
( | ||
["asins"], | ||
["asins"], | ||
True | ||
), | ||
( | ||
["campaigns", "adGroups"], | ||
["campaigns", "adGroups"], | ||
True | ||
), | ||
( | ||
[], | ||
[], | ||
False | ||
), | ||
( | ||
["invalid_record_type"], | ||
[], | ||
True | ||
) | ||
] | ||
) | ||
def test_brands_video_report_with_custom_record_types(config_gen, custom_record_types, expected_record_types, flag_match_error): | ||
setup_responses( | ||
init_response_brands=REPORT_INIT_RESPONSE, | ||
status_response=REPORT_STATUS_RESPONSE, | ||
metric_response=METRIC_RESPONSE, | ||
) | ||
|
||
profiles = make_profiles() | ||
|
||
stream = SponsoredBrandsVideoReportStream(config_gen(report_record_types=custom_record_types), profiles, authenticator=mock.MagicMock()) | ||
stream_slice = {"profile": profiles[0], "reportDate": "20210725"} | ||
records = list(stream.read_records(SyncMode.incremental, stream_slice=stream_slice)) | ||
for record in records: | ||
print(record) | ||
if record['recordType'] not in expected_record_types: | ||
if flag_match_error: | ||
assert False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it
enum
just likestate_filter
. Then, it will be easier for the users to pick predefined values from the list and prevent them from entering invalid ones.NB! There are
asins_keywords
andasins_targets
for Sponsored Products. Make sure that they both work. Add test cases forasins
,asins_keywords
, andasins_targets
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since all report types are not supported by each category of reports, making them enum doesn't make sense.
Also, this would be an optional config option, just like
profiles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, any value can be entered, even nonexistent in any report type. Enum would limit possible values to those that exist at least in one report type. Anyway, your code now ignores invalid values so that nothing would change on the code side.