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 Stripe: Add Invoice Discounts Expandable #37748

Closed
wants to merge 6 commits into from

Conversation

aasimsani
Copy link

@aasimsani aasimsani commented May 1, 2024

What

Stripe has deprecated the old way to access discounts through invoice.discount. It now uses invoice.discounts and now supports multiple discounts on one invoice.

How

Added an expandable item to the invoice datastream.

Review guide

Files changed:

  • airbyte-integrations/connectors/source-stripe/source_stripe/source.py
  • airbyte-integrations/connectors/source-stripe/source_stripe/schema/invoice.json

Bumps to versions and release information

  • airbyte-integrations/connectors/source-stripe/metadata.yaml
  • airbyte-integrations/connectors/source-stripe/pyproject.toml
  • docs/integrations/sources/stripe.md

Local unit tests passed and visually confirmed that on a Stripe account that has discounts on invoices that the array invoice.discounts no longer comprises of just discount ids.

User Impact

Users can now access multiple discounts that are present on Stripe's invoices.
Fixes: #29162
No negatives

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 ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview May 8, 2024 10:34pm

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.

Left a comment. Thank you for your contribution, @aasimsani This is a update to an Airbyte critical connector, so our team will review it in a future sprint. We ask for your patience until then. Don't hesitate to contact me here or on Slack for updates.

@@ -110,7 +110,7 @@
"description": "Any discounts applied to the invoice",
"type": ["null", "array"],
"items": {
"type": ["null", "string"]
"type": ["null", "object"]
Copy link
Member

Choose a reason for hiding this comment

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

Can you provide link to Stripe documentation showing the change in the data type?
If it now is a object you must specify the property additionalProperties: true

Copy link
Author

@aasimsani aasimsani May 6, 2024

Choose a reason for hiding this comment

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

Hi @marcosmarxm - here's the link to invoice.discounts now being an array of strings but with how expandables work in Stripe it would turn into an array of discounts object - here's the reference to Stripe's discount object

Copy link
Collaborator

Choose a reason for hiding this comment

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

The docs says it should be the array of strings?
Reading docs more resulted in the fact the discounts should be the array of objects, with the following structure: https://docs.stripe.com/api/discounts/object

This means we need to declare this in the schema as well. The changes are needed or the explanation of why we leave the object only and don't expand the schema otherwise.

Thank you.

Copy link
Contributor

@maxi297 maxi297 May 7, 2024

Choose a reason for hiding this comment

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

Two concerns:

  • Using expandable, I still get an array of string. Yes, they look a lot like strings but if Stripe consider them as string in their API doc, I wouldn't mess with that as they could be sending strings that do not represent objects. Trace from our test account for documentation purposes:
            "discounts": [
                "{'id': 'di_1PDuEkEcXtiJtvvhSoPOEqOo', 'object': 'discount', 'checkout_session': None, 'coupon': {'id': 'iJ6qlwM5', 'object': 'coupon', 'amount_off': None, 'created': 1674208993, 'currency': None, 'duration': 'forever', 'duration_in_months': None, 'livemode': False, 'max_redemptions': None, 'metadata': {}, 'name': '\u0415\u0443\u0456\u0435', 'percent_off': 10.0, 'redeem_by': None, 'times_redeemed': 5, 'valid': True}, 'customer': 'cus_NGoTFiJFVbSsvZ', 'end': None, 'invoice': 'in_1PDuE1EcXtiJtvvhwv4F8wEK', 'invoice_item': None, 'promotion_code': None, 'start': 1715112222, 'subscription': None, 'subscription_item': None}"
            ],
  • A little bit of background on how incremental syncs work: Stripe does not allow to filter on the resources endpoints using "updated" as a parameter. Instead, they provide another endpoint which is the events endpoint. That being said: I'm very confused about Stripe behavior regarding this field as poking the resource directly, we get the snippet I share in the previous point. However, when querying the events endpoints, I get the following:
          "discounts": [
            "di_1PDuEkEcXtiJtvvhSoPOEqOo"
          ],

I can't explain the discrepancy but would like you to confirm this behavior on your side: Do you have the same discounts values in full_refresh and incremental? I'll contact Stripe regarding this but they already told us that events [...] provide a summary of the object's data at the time of the event, without including any expanded properties. so I don't expect them to support those. It seems like the pattern we use for charges surprisingly so I'll try to get more information from them as to why.

EDIT: For documentation purposes internally, please refer to https://groups.google.com/a/airbyte.io/g/integration-test/c/C9COjdmCwPo/m/m8mi2TQKAwAJ

Copy link
Author

Choose a reason for hiding this comment

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

Strange behaviour from Stripe - unsure why this happens but I'll revert the schema back - I only noticed it now that I looked at the output again.

@aasimsani aasimsani temporarily deployed to community-ci-auto May 6, 2024 18:47 — with GitHub Actions Inactive
@aasimsani aasimsani had a problem deploying to community-ci-auto May 6, 2024 18:47 — with GitHub Actions Failure
@aasimsani aasimsani requested review from a team as code owners May 6, 2024 19:25
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.

Can we answer the question of why there are 240+ files changed? Is this intentional?

@@ -110,7 +110,7 @@
"description": "Any discounts applied to the invoice",
"type": ["null", "array"],
"items": {
"type": ["null", "string"]
"type": ["null", "object"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

The docs says it should be the array of strings?
Reading docs more resulted in the fact the discounts should be the array of objects, with the following structure: https://docs.stripe.com/api/discounts/object

This means we need to declare this in the schema as well. The changes are needed or the explanation of why we leave the object only and don't expand the schema otherwise.

Thank you.

@octavia-squidington-iii octavia-squidington-iii removed the CDK Connector Development Kit label May 8, 2024
@aasimsani aasimsani had a problem deploying to community-ci-auto May 8, 2024 22:34 — with GitHub Actions Failure
@aasimsani aasimsani had a problem deploying to community-ci-auto May 8, 2024 22:34 — with GitHub Actions Failure
@aasimsani
Copy link
Author

Can we answer the question of why there are 240+ files changed? Is this intentional?

@bazarnov - Accidental git sync on my branch - just reverted it back to what it should be.

@marcosmarxm
Copy link
Member

@bazarnov @maxi297 are we going to continue this?

@bazarnov
Copy link
Collaborator

@bazarnov @maxi297 are we going to continue this?

Hello @marcosmarxm
I think this comment was not addressed yet: #37748 (comment)
Thus I cannot approve, without this requested change.

@marcosmarxm
Copy link
Member

@aasimsani did you check latest comments? Are you still thinking to finish this?

@AGPapa
Copy link
Contributor

AGPapa commented Jul 10, 2024

Is the 'properties' configuration the only thing left?

I have these changes on a local branch - I copied the properties from the existing "discount" column to where it needs to be and everything seems to work. The 'discounts' column now has full json objects instead of having only ids.

@bazarnov
Copy link
Collaborator

Is the 'properties' configuration the only thing left?

@AGPapa It seems like it is.

@AGPapa
Copy link
Contributor

AGPapa commented Jul 16, 2024

I have create a new PR #41985 that takes the changes here and adds the requested 'properties' section. If there are any other changes needed then let me know and I can make them!

Thank you @aasimsani for putting up this PR!

@marcosmarxm
Copy link
Member

Closed as inactive and #41985 is continuing the implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Source Stripe: Disounts not expanded for Invoice Line Items
7 participants