🎉 Source Amazon Ads : use optional config report_record_types #18677
Conversation
| @@ -355,6 +356,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.
| if len(self._report_record_types) > 0 and record_type not in self._report_record_types: | |
| if record_type not in self._report_record_types: |
When self._report_record_types is an empty array, which is by default, not in will return True for any value.
There was a problem hiding this comment.
it's on and condition. If array is empty, the complete condition would always be false
|
|
||
|
|
||
| @responses.activate | ||
| def test_display_report_stream_single_record_type(config_gen): |
There was a problem hiding this comment.
-
Both tests are the same except for the
custom_record_typesparameter. Deduplicate code by using@pytest.mark.parametrize. Seetest_streams_state_filterfor an example. -
Add two more test cases:
custom_record_types = Noneandcustom_record_types = ["invalid_record_type"]to make the test more exhaustive.
There was a problem hiding this comment.
sure, will do
| stream = SponsoredDisplayReportStream(config_gen(report_record_types=custom_record_types), profiles, authenticator=mock.MagicMock()) | ||
| stream_slice = {"profile": profiles[0], "reportDate": "20210725"} | ||
| metrics = [m for m in stream.read_records(SyncMode.incremental, stream_slice=stream_slice)] | ||
| assert len(metrics) == len(custom_record_types) * len(stream.metrics_map) |
There was a problem hiding this comment.
Assertion for the length doesn't ensure that the retrieved record types are the same as requested. Instead, assert whether the set of retrieved record["recordType"] is equal to the set of custom_record_types.
|
|
||
| stream = SponsoredDisplayReportStream(config_gen(report_record_types=custom_record_types), profiles, authenticator=mock.MagicMock()) | ||
| stream_slice = {"profile": profiles[0], "reportDate": "20210725"} | ||
| metrics = [m for m in stream.read_records(SyncMode.incremental, stream_slice=stream_slice)] |
There was a problem hiding this comment.
a_list = list(generator) is a cleaner way to materialize a generator into a list.
| metrics = [m for m in stream.read_records(SyncMode.incremental, stream_slice=stream_slice)] | |
| metrics = list(stream.read_records(SyncMode.incremental, stream_slice=stream_slice)) |
| order: 10 | ||
| type: array | ||
| items: | ||
| type: string |
There was a problem hiding this comment.
Make it enum just like state_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 and asins_targets for Sponsored Products. Make sure that they both work. Add test cases for asins, asins_keywords, and asins_targets.
There was a problem hiding this comment.
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.
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.
| @@ -612,3 +609,43 @@ 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.
Add a test for Sponsored Products report and asins_keywords and asins_targets record types. They're treated differently than other types.
There was a problem hiding this comment.
Have added tests. Please review
| @@ -612,3 +609,43 @@ 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.
Have added tests. Please review
|
Could you please refactor the spec input type to enum? See #18677 (comment) |
|
/test connector=connectors/source-amazon-ads
Build FailedTest summary info: |
|
@grubberr I have updated the tests as per new Please review again |
|
/test connector=connectors/source-amazon-ads
Build PassedTest summary info: |
|
/test connector=connectors/source-amazon-ads
Build PassedTest summary info: |
...ions/connectors/source-amazon-ads/source_amazon_ads/streams/report_streams/report_streams.py
Outdated
Show resolved
Hide resolved
|
/test connector=connectors/source-amazon-ads
Build PassedTest summary info: |
|
@marcosmarxm @natalyjazzviolin Please help in merging this PR. Thanks !! |
|
/publish connector=connectors/source-amazon-ads
if you have connectors that successfully published but failed definition generation, follow step 4 here |
…ehq#18677) * use optional config report_record_types * update spec * update image version * update * correct indentation * remove lint changes * add tests for sponsored products report * update spec * add test for video report * increment amazon-ads connector version * add test_strictness_level as high * gradle format update * update * update tests for new METRIC_RESPONSE * align validation and transformation * bump connector version * auto-bump connector version --------- Co-authored-by: Juozas V <monai@cure.lt> Co-authored-by: Sajarin <sajarindider@gmail.com> Co-authored-by: Nataly Merezhuk <65251165+natalyjazzviolin@users.noreply.github.com> Co-authored-by: Serhii Chvaliuk <grubberr@gmail.com> Co-authored-by: sh4sh <6833405+sh4sh@users.noreply.github.com> Co-authored-by: marcosmarxm <marcosmarxm@gmail.com> Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
* use optional config report_record_types * update spec * update image version * update * correct indentation * remove lint changes * add tests for sponsored products report * update spec * add test for video report * increment amazon-ads connector version * add test_strictness_level as high * gradle format update * update * update tests for new METRIC_RESPONSE * align validation and transformation * bump connector version * auto-bump connector version --------- Co-authored-by: Juozas V <monai@cure.lt> Co-authored-by: Sajarin <sajarindider@gmail.com> Co-authored-by: Nataly Merezhuk <65251165+natalyjazzviolin@users.noreply.github.com> Co-authored-by: Serhii Chvaliuk <grubberr@gmail.com> Co-authored-by: sh4sh <6833405+sh4sh@users.noreply.github.com> Co-authored-by: marcosmarxm <marcosmarxm@gmail.com> Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
* use optional config report_record_types * update spec * update image version * update * correct indentation * remove lint changes * add tests for sponsored products report * update spec * add test for video report * increment amazon-ads connector version * add test_strictness_level as high * gradle format update * update * update tests for new METRIC_RESPONSE * align validation and transformation * bump connector version * auto-bump connector version --------- Co-authored-by: Juozas V <monai@cure.lt> Co-authored-by: Sajarin <sajarindider@gmail.com> Co-authored-by: Nataly Merezhuk <65251165+natalyjazzviolin@users.noreply.github.com> Co-authored-by: Serhii Chvaliuk <grubberr@gmail.com> Co-authored-by: sh4sh <6833405+sh4sh@users.noreply.github.com> Co-authored-by: marcosmarxm <marcosmarxm@gmail.com> Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
What
How
*Adding an optional configuration option to read a list of selective report types from user. When the optional config is empty, it will fetch for all record types ( default behaviour )
Recommended reading order
source_amazon_ads/spec.yamlsource_amazon_ads/streams/report_streams/report_streams.py🚨 User Impact 🚨
Not a breaking change
Pre-merge Checklist
Expand the relevant checklist and delete the others.
Updating a connector
Community member or Airbyter
airbyte_secret./gradlew :airbyte-integrations:connectors:<name>:integrationTest.README.mdbootstrap.md. See description and examplesdocs/integrations/<source or destination>/<name>.mdincluding changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>command is passing/publishcommand described hereTests
Unit
Results (3.80s):
70 passed
Name Stmts Miss Cover Missing
source_amazon_ads/init.py 2 0 100%
source_amazon_ads/constants.py 1 0 100%
source_amazon_ads/schemas/init.py 7 0 100%
source_amazon_ads/schemas/attribution_report.py 21 0 100%
source_amazon_ads/schemas/common.py 51 1 98% 26
source_amazon_ads/schemas/profile.py 16 0 100%
source_amazon_ads/schemas/sponsored_brands.py 22 0 100%
source_amazon_ads/schemas/sponsored_display.py 31 0 100%
source_amazon_ads/schemas/sponsored_products.py 37 0 100%
source_amazon_ads/source.py 44 1 98% 129
source_amazon_ads/streams/init.py 7 0 100%
source_amazon_ads/streams/attribution_report.py 81 0 100%
source_amazon_ads/streams/common.py 79 1 99% 157
source_amazon_ads/streams/profiles.py 21 0 100%
source_amazon_ads/streams/report_streams/init.py 5 0 100%
source_amazon_ads/streams/report_streams/brands_report.py 10 0 100%
source_amazon_ads/streams/report_streams/brands_video_report.py 10 0 100%
source_amazon_ads/streams/report_streams/display_report.py 16 0 100%
source_amazon_ads/streams/report_streams/products_report.py 18 0 100%
source_amazon_ads/streams/report_streams/report_streams.py 243 19 92% 151, 204-205, 257-258, 380-381, 413-415, 420-421, 430-436
source_amazon_ads/streams/sponsored_brands.py 26 0 100%
source_amazon_ads/streams/sponsored_display.py 31 0 100%
source_amazon_ads/streams/sponsored_products.py 41 0 100%
source_amazon_ads/utils.py 9 0 100%
unit_tests/init.py 0 0 100%
unit_tests/conftest.py 37 0 100%
unit_tests/test_attribution_report.py 66 1 98% 47
unit_tests/test_report_streams.py 315 0 100%
unit_tests/test_source.py 47 0 100%
unit_tests/test_streams.py 126 1 99% 77
unit_tests/utils.py 32 0 100%
TOTAL 1452 24 98%