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 Shopify: implement BalanceTransactions stream #10204

Merged
merged 17 commits into from
Mar 22, 2022

Conversation

harshithmullapudi
Copy link
Contributor

What

closes #9850

@github-actions github-actions bot added area/connectors Connector related issues area/documentation Improvements or additions to documentation labels Feb 9, 2022
@harshithmullapudi
Copy link
Contributor Author

harshithmullapudi commented Feb 9, 2022

/test connector=connectors/source-shopify

🕑 connectors/source-shopify https://github.com/airbytehq/airbyte/actions/runs/1816604909
❌ connectors/source-shopify https://github.com/airbytehq/airbyte/actions/runs/1816604909
🐛 https://gradle.com/s/tbpzk2db4xnai
Python short test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestBasicRead::test_read[inputs0] - docker.errors.Contai...
FAILED test_full_refresh.py::TestFullRefresh::test_sequential_reads[inputs0]
FAILED test_incremental.py::TestIncremental::test_two_sequential_reads[inputs0]
FAILED test_incremental.py::TestIncremental::test_state_with_abnormally_large_values[inputs0]
================== 4 failed, 34 passed in 1570.73s (0:26:10) ===================

@harshithmullapudi harshithmullapudi temporarily deployed to more-secrets February 9, 2022 06:58 Inactive
@codecov
Copy link

codecov bot commented Feb 9, 2022

Codecov Report

❗ No coverage uploaded for pull request base (master@1f98286). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 3c957ac differs from pull request most recent head 8373e94. Consider uploading reports for the commit 8373e94 to get more accurate results

@@            Coverage Diff            @@
##             master   #10204   +/-   ##
=========================================
  Coverage          ?   72.04%           
=========================================
  Files             ?        5           
  Lines             ?      415           
  Branches          ?        0           
=========================================
  Hits              ?      299           
  Misses            ?      116           
  Partials          ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f98286...8373e94. Read the comment docs.

@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets February 9, 2022 07:00 Inactive
@harshithmullapudi
Copy link
Contributor Author

harshithmullapudi commented Mar 10, 2022

/test connector=connectors/source-shopify

🕑 connectors/source-shopify https://github.com/airbytehq/airbyte/actions/runs/1962357657
❌ connectors/source-shopify https://github.com/airbytehq/airbyte/actions/runs/1962357657
🐛 https://gradle.com/s/vvvnsrcsmvinm

@harshithmullapudi
Copy link
Contributor Author

harshithmullapudi commented Mar 21, 2022

/test connector=connectors/source-shopify

🕑 connectors/source-shopify https://github.com/airbytehq/airbyte/actions/runs/2014261075
❌ connectors/source-shopify https://github.com/airbytehq/airbyte/actions/runs/2014261075
🐛 https://gradle.com/s/y3r67ew43egoe
Python short test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestBasicRead::test_read[inputs0] - AssertionError: All ...
================== 1 failed, 37 passed in 2208.29s (0:36:48) ===================

@harshithmullapudi
Copy link
Contributor Author

harshithmullapudi commented Mar 22, 2022

/test connector=connectors/source-shopify

🕑 connectors/source-shopify https://github.com/airbytehq/airbyte/actions/runs/2020905533
✅ connectors/source-shopify https://github.com/airbytehq/airbyte/actions/runs/2020905533
Python tests coverage:

Name                                                 Stmts   Miss  Cover
------------------------------------------------------------------------
source_acceptance_test/utils/__init__.py                 6      0   100%
source_acceptance_test/tests/__init__.py                 4      0   100%
source_acceptance_test/__init__.py                       2      0   100%
source_acceptance_test/tests/test_full_refresh.py       52      2    96%
source_acceptance_test/utils/asserts.py                 37      2    95%
source_acceptance_test/config.py                        74      6    92%
source_acceptance_test/utils/json_schema_helper.py     105     13    88%
source_acceptance_test/utils/common.py                  70     17    76%
source_acceptance_test/utils/compare.py                 62     23    63%
source_acceptance_test/tests/test_core.py              275    106    61%
source_acceptance_test/base.py                          10      4    60%
source_acceptance_test/utils/connector_runner.py       110     48    56%
source_acceptance_test/tests/test_incremental.py        69     38    45%
------------------------------------------------------------------------
TOTAL                                                  876    259    70%
Name                          Stmts   Miss  Cover
-------------------------------------------------
source_shopify/__init__.py        2      0   100%
source_shopify/transform.py      58      3    95%
source_shopify/utils.py          58      6    90%
source_shopify/auth.py           20      4    80%
source_shopify/source.py        292    112    62%
-------------------------------------------------
TOTAL                           430    125    71%

@harshithmullapudi harshithmullapudi temporarily deployed to more-secrets March 22, 2022 07:35 Inactive
@harshithmullapudi harshithmullapudi temporarily deployed to more-secrets March 22, 2022 07:36 Inactive
@harshithmullapudi
Copy link
Contributor Author

@bazarnov @ron-damon I made the changes here to reduce the back and forth. Kindly look at them

@ron-damon
Copy link
Contributor

I'd refactored the incremental logic for streams based purely on id (does not support Incremental Refresh based on datetime fields) because it's exactly the same behavior for all "purely id" streams, but it's your call! 😃
LGTM! 🚀

@harshithmullapudi
Copy link
Contributor Author

@ron-damon are there more streams similar to collects and balance transactions if so I think we can go with what you have suggested. wdyt?

@bazarnov
Copy link
Collaborator

@harshithmullapudi @ron-damon
Please, checkout the latest commit, as my suggestion to cover new id-based streams and cleared a code a bit.

@bazarnov bazarnov temporarily deployed to more-secrets March 22, 2022 10:30 Inactive
@airbytehq airbytehq deleted a comment from harshithmullapudi Mar 22, 2022
@bazarnov bazarnov temporarily deployed to more-secrets March 22, 2022 10:30 Inactive
@bazarnov bazarnov changed the title Publish source shopify: added new stream 🎉 Source Shopify: implement BalanceTransactions stream Mar 22, 2022
@bazarnov bazarnov temporarily deployed to more-secrets March 22, 2022 10:38 Inactive
@bazarnov
Copy link
Collaborator

bazarnov commented Mar 22, 2022

/test connector=connectors/source-shopify

🕑 connectors/source-shopify https://github.com/airbytehq/airbyte/actions/runs/2022396569
✅ connectors/source-shopify https://github.com/airbytehq/airbyte/actions/runs/2022396569
Python tests coverage:

Name                                                 Stmts   Miss  Cover
------------------------------------------------------------------------
source_acceptance_test/utils/__init__.py                 6      0   100%
source_acceptance_test/tests/__init__.py                 4      0   100%
source_acceptance_test/__init__.py                       2      0   100%
source_acceptance_test/tests/test_full_refresh.py       52      2    96%
source_acceptance_test/utils/asserts.py                 37      2    95%
source_acceptance_test/config.py                        74      6    92%
source_acceptance_test/utils/json_schema_helper.py     105     13    88%
source_acceptance_test/utils/common.py                  70     17    76%
source_acceptance_test/utils/compare.py                 62     23    63%
source_acceptance_test/tests/test_core.py              275    106    61%
source_acceptance_test/base.py                          10      4    60%
source_acceptance_test/utils/connector_runner.py       110     48    56%
source_acceptance_test/tests/test_incremental.py        69     38    45%
------------------------------------------------------------------------
TOTAL                                                  876    259    70%
Name                          Stmts   Miss  Cover
-------------------------------------------------
source_shopify/__init__.py        2      0   100%
source_shopify/transform.py      58      3    95%
source_shopify/utils.py          58      6    90%
source_shopify/auth.py           20      4    80%
source_shopify/source.py        281    103    63%
-------------------------------------------------
TOTAL                           419    116    72%

@bazarnov
Copy link
Collaborator

@ron-damon

I think we should remove the order parameter when using since_id, because it's not allowed to use both. The output of a request with order and since_id parameters

It's should be fine, if the order_field is id, otherwise, the endpoint returns the error. The tests are passed as expected.

@bazarnov bazarnov self-requested a review March 22, 2022 13:37
Copy link
Collaborator

@bazarnov bazarnov left a comment

Choose a reason for hiding this comment

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

We are good to go)

@bazarnov bazarnov temporarily deployed to more-secrets March 22, 2022 15:10 Inactive
@bazarnov bazarnov temporarily deployed to more-secrets March 22, 2022 15:11 Inactive
@bazarnov bazarnov temporarily deployed to more-secrets March 22, 2022 15:15 Inactive
@bazarnov bazarnov temporarily deployed to more-secrets March 22, 2022 15:15 Inactive
@bazarnov bazarnov temporarily deployed to more-secrets March 22, 2022 15:22 Inactive
@bazarnov bazarnov temporarily deployed to more-secrets March 22, 2022 15:22 Inactive
@bazarnov bazarnov temporarily deployed to more-secrets March 22, 2022 17:38 Inactive
@bazarnov bazarnov temporarily deployed to more-secrets March 22, 2022 17:43 Inactive
@bazarnov bazarnov temporarily deployed to more-secrets March 22, 2022 18:05 Inactive
@bazarnov
Copy link
Collaborator

bazarnov commented Mar 22, 2022

/publish connector=connectors/source-shopify

🕑 connectors/source-shopify https://github.com/airbytehq/airbyte/actions/runs/2024127143
✅ connectors/source-shopify https://github.com/airbytehq/airbyte/actions/runs/2024127143

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants