🐛 Source Stripe: fix taxes-related tables in CheckoutSessionsLineItems + delete redundant secret key validation #8182

grkhr · 2021-11-22T18:41:29Z

What

Fixed taxes expanding in Checkout Sessions Line items implemented in this PR 🎉 Source Stripe: add checkout_sessions stream, checkout_sessions_line_item stream and promotion_codes stream #7345. There were empty tables after sync.
Deleted secret key regexp validation

How

Added taxes param to stripe stream request. There were schema for this table but not in the request to Stripe.
Also, I've deleted regexp validation, causing bugs mentioned in Slack (e.g. it was unable to delete this source, there is some bug with validation itself). I've found this redundant because there is a clear example in doc and Stripe can change key-generating logic at any moment.

Pre-merge Checklist

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.

root@airbyte-dev:/home/eduard/airbyte/airbyte-integrations/connectors/source-stripe# python3 -m pytest integration_tests --full-trace
======================================================================================== test session starts ========================================================================================
platform linux -- Python 3.7.3, pytest-6.2.5, py-1.11.0, pluggy-1.0.0 -- /usr/bin/python3
cachedir: .pytest_cache
rootdir: /home/eduard/airbyte, configfile: pytest.ini
collected 1 item

integration_tests/test_dummy.py::test_dummy PASSED

========================================================================================= 1 passed in 0.01s =========================================================================================

root@airbyte-dev:/home/eduard/airbyte/airbyte-integrations/connectors/source-stripe# python3 -m pytest unit_tests
======================================================================================== test session starts ========================================================================================
platform linux -- Python 3.7.3, pytest-6.2.5, py-1.11.0, pluggy-1.0.0 -- /usr/bin/python3
cachedir: .pytest_cache
rootdir: /home/eduard/airbyte, configfile: pytest.ini
collected 4 items

unit_tests/test_source.py::test_lookback_window[None-1637606827.766514-1637606827.766514-if lookback_window_days is not set should not affect cursor value] PASSED
unit_tests/test_source.py::test_lookback_window[0-1637606827.766514-1637606827.766514-if lookback_window_days is not set should not affect cursor value] PASSED
unit_tests/test_source.py::test_lookback_window[10-1637606827.766514-1636742827-Should calculate cursor value as expected] {"type": "LOG", "log": {"level": "INFO", "message": "Applying lookback window of 10 days to stream invoices"}}
PASSED
unit_tests/test_source.py::test_lookback_window[-10-1637606827.766514-1636742827-Should not care for the sign, use the module] {"type": "LOG", "log": {"level": "INFO", "message": "Applying lookback window of -10 days to stream invoices"}}
PASSED

========================================================================================= warnings summary ==========================================================================================
airbyte-integrations/connectors/source-stripe/unit_tests/test_source.py::test_lookback_window[None-1637606827.766514-1637606827.766514-if lookback_window_days is not set should not affect cursor value]
airbyte-integrations/connectors/source-stripe/unit_tests/test_source.py::test_lookback_window[0-1637606827.766514-1637606827.766514-if lookback_window_days is not set should not affect cursor value]
airbyte-integrations/connectors/source-stripe/unit_tests/test_source.py::test_lookback_window[10-1637606827.766514-1636742827-Should calculate cursor value as expected]
airbyte-integrations/connectors/source-stripe/unit_tests/test_source.py::test_lookback_window[-10-1637606827.766514-1636742827-Should not care for the sign, use the module]
  /usr/local/lib/python3.7/dist-packages/airbyte_cdk/sources/streams/http/http.py:37: DeprecationWarning: Call to deprecated class NoAuth. (Set `authenticator=None` instead) -- Deprecated since version 0.1.20.
    self._authenticator = NoAuth()

airbyte-integrations/connectors/source-stripe/unit_tests/test_source.py::test_lookback_window[None-1637606827.766514-1637606827.766514-if lookback_window_days is not set should not affect cursor value]
airbyte-integrations/connectors/source-stripe/unit_tests/test_source.py::test_lookback_window[0-1637606827.766514-1637606827.766514-if lookback_window_days is not set should not affect cursor value]
airbyte-integrations/connectors/source-stripe/unit_tests/test_source.py::test_lookback_window[10-1637606827.766514-1636742827-Should calculate cursor value as expected]
airbyte-integrations/connectors/source-stripe/unit_tests/test_source.py::test_lookback_window[-10-1637606827.766514-1636742827-Should not care for the sign, use the module]
  /usr/local/lib/python3.7/dist-packages/deprecated/classic.py:173: DeprecationWarning: Call to deprecated class HttpAuthenticator. (Use requests.auth.AuthBase instead) -- Deprecated since version 0.1.20.
    return old_new1(cls, *args, **kwargs)

-- Docs: https://docs.pytest.org/en/stable/warnings.html
=================================================================================== 4 passed, 8 warnings in 0.27s ===================================================================================

❗️Also I've build docker image and tested it by adding custom connector in UI. It can be found here and easily added to existing Airbyte instance.

Here is screenshot from database after testing

Code reviews completed
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
Credentials added to Github CI. Instructions.
/test connector=connectors/<name> command is passing.
New Connector version released on Dockerhub by running the /publish command described here
After the new connector version is published, connector version bumped in the seed directory as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

…lems

CLAassistant · 2021-11-22T18:41:35Z

All committers have signed the CLA.

This reverts commit 5086442.

sherifnada · 2021-11-24T16:29:21Z

airbyte-integrations/connectors/source-stripe/source_stripe/streams.py

@@ -377,7 +377,7 @@ def read_records(self, stream_slice: Optional[Mapping[str, Any]] = None, **kwarg

    def request_params(self, stream_slice: Mapping[str, Any] = None, **kwargs):
        params = super().request_params(stream_slice=stream_slice, **kwargs)
-        params["expand[]"] = "data.discounts"
+        params["expand[]"] = ["data.discounts", "data.taxes"]


doesn't this change the output schema? if so, we should update the .json schema file for this stream in order for normalization to process it correctly

Schema was already in .json, you can check here, for some reason developer forgot to add this param. That's all.

sherifnada · 2021-11-24T16:29:50Z

airbyte-integrations/connectors/source-stripe/source_stripe/spec.json

@@ -9,7 +9,6 @@
    "properties": {
      "client_secret": {
        "type": "string",
-        "pattern": "^(s|r)k_(live|test)_[a-zA-Z0-9]+$",


what's the reason to remove this? it's better to give feedback instantly if we know it's correct

I've found that I'm unable to delete Stripe source in Airbyte UI (and I'm not alone). Also, I found this redundant because there is a clear example in the settings page of the connector for this key and Stripe can change key-generating logic at any moment.

Seems that the real reason for that bug is somewhere in backend logic which isn't related to stripe connector, so it's easier to delete excessive validation. Btw you can reproduce it — just add the latest stripe source, configure It, try to delete and open web inspector in your browser to see errors.

thanks for the context. this seems like a fair workaround then. Ive created an issue to address the root cause: #8244

grkhr · 2021-11-24T18:33:22Z

Do I need to bump the version and changelog? I did it here but reverted because of confusion of a review process

sherifnada · 2021-11-24T18:59:06Z

nope, you're good to go. Thanks @grkhr !

Co-authored-by: grkhr <[email protected]>

…s + delete redundant secret key validation (airbytehq#8182)

Co-authored-by: grkhr <[email protected]>

grkhr added 3 commits November 19, 2021 12:49

Source Stripe: add taxes to request params in CheckoutSessionsLineItems

23ed9b0

Source Stripe: delete pattern from spec to avoid source deleting prob…

5df1add

…lems

Source Stripe: update version in Dockerfile

159b13e

github-actions bot added the area/connectors Connector related issues label Nov 22, 2021

octavia-squidington-iii added the community label Nov 22, 2021

grkhr marked this pull request as ready for review November 22, 2021 19:09

Source Stripe: bump connector version

5086442

github-actions bot added the area/documentation Improvements or additions to documentation label Nov 24, 2021

grkhr marked this pull request as draft November 24, 2021 10:22

grkhr marked this pull request as ready for review November 24, 2021 10:23

grkhr added 2 commits November 24, 2021 13:24

fix whitespace

4bab990

Revert "Source Stripe: bump connector version"

bda8176

This reverts commit 5086442.

github-actions bot removed the area/documentation Improvements or additions to documentation label Nov 24, 2021

grkhr marked this pull request as draft November 24, 2021 11:44

grkhr marked this pull request as ready for review November 24, 2021 12:51

sherifnada reviewed Nov 24, 2021

View reviewed changes

sherifnada mentioned this pull request Nov 24, 2021

publish #8182 #8246

Merged

sherifnada temporarily deployed to more-secrets November 24, 2021 17:47 Inactive

jrhizor temporarily deployed to more-secrets November 24, 2021 17:48 Inactive

jrhizor temporarily deployed to more-secrets November 24, 2021 18:05 Inactive

sherifnada merged commit dae0d77 into airbytehq:master Nov 24, 2021

sherifnada added a commit that referenced this pull request Nov 24, 2021

publish #8182 (#8246)

b94256b

Co-authored-by: grkhr <[email protected]>

schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022

🐛 Source Stripe: fix taxes-related tables in CheckoutSessionsLineItem…

d5050e9

…s + delete redundant secret key validation (airbytehq#8182)

schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022

publish airbytehq#8182 (airbytehq#8246)

519fdd9

Co-authored-by: grkhr <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Source Stripe: fix taxes-related tables in CheckoutSessionsLineItems + delete redundant secret key validation #8182

🐛 Source Stripe: fix taxes-related tables in CheckoutSessionsLineItems + delete redundant secret key validation #8182

grkhr commented Nov 22, 2021 •

edited by sherifnada

Loading

CLAassistant commented Nov 22, 2021 •

edited

Loading

sherifnada Nov 24, 2021

grkhr Nov 24, 2021

sherifnada Nov 24, 2021

grkhr Nov 24, 2021

sherifnada Nov 24, 2021

grkhr commented Nov 24, 2021

sherifnada commented Nov 24, 2021

🐛 Source Stripe: fix taxes-related tables in CheckoutSessionsLineItems + delete redundant secret key validation #8182

🐛 Source Stripe: fix taxes-related tables in CheckoutSessionsLineItems + delete redundant secret key validation #8182

Conversation

grkhr commented Nov 22, 2021 • edited by sherifnada Loading

What

How

Pre-merge Checklist

Community member or Airbyter

Airbyter

CLAassistant commented Nov 22, 2021 • edited Loading

sherifnada Nov 24, 2021

Choose a reason for hiding this comment

grkhr Nov 24, 2021

Choose a reason for hiding this comment

sherifnada Nov 24, 2021

Choose a reason for hiding this comment

grkhr Nov 24, 2021

Choose a reason for hiding this comment

sherifnada Nov 24, 2021

Choose a reason for hiding this comment

grkhr commented Nov 24, 2021

sherifnada commented Nov 24, 2021

grkhr commented Nov 22, 2021 •

edited by sherifnada

Loading

CLAassistant commented Nov 22, 2021 •

edited

Loading