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

🐛Source Amazon Seller Partner: Fix GET_VENDOR_TRAFFIC_REPORT when reportPeriod=DAY #37732

Conversation

tianchen98
Copy link

@tianchen98 tianchen98 commented May 1, 2024

What

issue: #37729

How

Added availablity_sla_days=3 and fixed_period_in_days=1 to ensure that all data in the lookback window can be fetched and that dataEndTime doesn't violate Amazon Vendor Central's SLA, which is 72 hours after the close of the period.

Review guide

streams.py

User Impact

Syncing GET_VENDOR_TRAFFIC_REPORT will not result in errors

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

Copy link

vercel bot commented May 1, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 7, 2024 4:40pm

@CLAassistant
Copy link

CLAassistant commented May 1, 2024

CLA assistant check
All committers have signed the CLA.

@tianchen98 tianchen98 temporarily deployed to community-ci-auto May 1, 2024 00:47 — with GitHub Actions Inactive
@tianchen98 tianchen98 changed the title Source Amazon Seller Partner: Fix GET_VENDOR_TRAFFIC_REPORT when reportPeriod=DAY 🐛Source Amazon Seller Partner: Fix GET_VENDOR_TRAFFIC_REPORT when reportPeriod=DAY May 1, 2024
@tianchen98
Copy link
Author

tianchen98 commented May 1, 2024

what's the action that I need to take now? Should I update connector version?

@katmarkham katmarkham requested a review from askarpets May 1, 2024 14:42
@darynaishchenko
Copy link
Collaborator

darynaishchenko commented May 2, 2024

what's the action that I need to take now? Should I update connector version?
@tianchen98
Thanks for the changes.

Did you test it on your data, everything is fine?

Please update connector version in metedata.yaml and pyproject.toml and add changelog record in docs/integrations/sources/amazon-seller-partner.md. Thanks.

@tianchen98
Copy link
Author

tianchen98 commented May 2, 2024

Does PR branch need to stay up-to-date with master in order to merge? @darynaishchenko

@marcosmarxm marcosmarxm temporarily deployed to community-ci-auto May 6, 2024 17:45 — with GitHub Actions Inactive
@marcosmarxm marcosmarxm temporarily deployed to community-ci-auto May 6, 2024 17:45 — with GitHub Actions Inactive
@marcosmarxm
Copy link
Member

Running tests @tianchen98

Copy link
Member

@marcosmarxm marcosmarxm left a comment

Choose a reason for hiding this comment

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

@tianchen98 it looks some unit tests are failing. Can you fix them?

@tianchen98
Copy link
Author

@tianchen98 it looks some unit tests are failing. Can you fix them?

I can't really see the failed tests. It's returning Forbidden 403

@darynaishchenko
Copy link
Collaborator

@tianchen98 it looks some unit tests are failing. Can you fix them?

I can't really see the failed tests. It's returning Forbidden 403

@tianchen98, You can run poetry install --with dev and poetry run pytest unit_tests from connector's folder to run unit tests. Most of tests failed due to changes in request url so old request mocks are not match actual requests.

FAILED unit_tests/integration/test_report_based_streams.py::TestFullRefresh::test_given_report_when_read_then_return_records[GET_VENDOR_TRAFFIC_REPORT-json]
FAILED unit_tests/integration/test_report_based_streams.py::TestFullRefresh::test_given_compressed_report_when_read_then_return_records[GET_VENDOR_TRAFFIC_REPORT-json]
FAILED unit_tests/integration/test_report_based_streams.py::TestFullRefresh::test_given_http_status_500_then_200_when_create_report_then_retry_and_return_records[GET_VENDOR_TRAFFIC_REPORT-json]
FAILED unit_tests/integration/test_report_based_streams.py::TestFullRefresh::test_given_http_status_500_then_200_when_retrieve_report_then_retry_and_return_records[GET_VENDOR_TRAFFIC_REPORT-json]
FAILED unit_tests/integration/test_report_based_streams.py::TestFullRefresh::test_given_http_status_500_then_200_when_get_document_url_then_retry_and_return_records[GET_VENDOR_TRAFFIC_REPORT-json]
FAILED unit_tests/integration/test_report_based_streams.py::TestFullRefresh::test_given_http_status_500_then_200_when_download_document_then_retry_and_return_records[GET_VENDOR_TRAFFIC_REPORT-json]
FAILED unit_tests/integration/test_report_based_streams.py::TestFullRefresh::test_given_report_access_forbidden_when_read_then_no_records_and_error_logged[GET_VENDOR_TRAFFIC_REPORT-json]
FAILED unit_tests/integration/test_report_based_streams.py::TestFullRefresh::test_given_report_status_cancelled_when_read_then_stream_completed_successfully_and_warn_about_cancellation[GET_VENDOR_TRAFFIC_REPORT-json]
FAILED unit_tests/integration/test_report_based_streams.py::TestFullRefresh::test_given_report_status_fatal_when_read_then_exception_raised[GET_VENDOR_TRAFFIC_REPORT-json]
FAILED unit_tests/integration/test_report_based_streams.py::TestFullRefresh::test_given_http_error_500_on_create_report_when_read_then_no_records_and_error_logged[GET_VENDOR_TRAFFIC_REPORT-json]
FAILED unit_tests/integration/test_report_based_streams.py::TestIncremental::test_given_report_when_read_then_default_cursor_field_added_to_every_record[GET_VENDOR_TRAFFIC_REPORT-json]
FAILED unit_tests/integration/test_report_based_streams.py::TestIncremental::test_given_report_when_read_then_state_message_produced_and_state_match_latest_record[GET_VENDOR_TRAFFIC_REPORT-json]

@tianchen98
Copy link
Author

Test can be passed if I remove fixed_period_in_days = 1 from the stream config, but that'll break the stream when reportPeriod is set to DAY

@darynaishchenko
Copy link
Collaborator

Test can be passed if I remove fixed_period_in_days = 1 from the stream config, but that'll break the stream when reportPeriod is set to DAY

Yes, I understood. You are allowed to change unit tests implementation if source code was changed.

@marcosmarxm
Copy link
Member

Thanks for your contribution @tianchen98 the code was introduced in #38210. I'm going to close this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation community connectors/source/amazon-seller-partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants