🐛 Source Braintree: Fixes subscription data #40693
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
| The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
|
octavia-squidington-iii added area/connectors 
natikgadzhi reviewed Jul 3, 2024
Contributor
natikgadzhi left a comment
natikgadzhi left a comment There was a problem hiding this comment.
Thank you for the PR, and for moving to Poetry and base image too!
@ChristoGrab let's take a look at this. Let's see what CI tells us, and perform a regression test, take a look at the report. Perhaps that helps us move the source forward, it's pretty high usage.
@AGPapa, would you be willing to guide us through logs / screenshots of the resulting records to verify it works? I'm afraid we don't have a sandbox account seeded with the records to verify this easily.
| | ||
| | ||
| class Subscription(CatalogModel): | ||
| add_ons: List[AddOn] |
Contributor
There was a problem hiding this comment.
Pretty wild to see Python schemas — we switched away from them quite some time ago it seems. But hey, if this works, I'm ok.
I'm worried that the side effect of this could be that we lose some data (the time portion?), but if the API changed and the responses only have dates, that's ok.
Contributor Author
There was a problem hiding this comment.
That first Airbyte issue was posted in June 2022, so if the API did change from a datetime to a date then it was at least two years ago.
Contributor Author
Absolutely, let me know what you need. Here are the error logs from the current version of the connector. This is the original error log: and this is the error log from fixing the billing start/end dates, but not the paid through date: And here's a screenshot of it all working with both changes.  If you need any other screenshots or logs then let me know and I will attach them. I can make adjustments to this PR too if we need it! |
ChristoGrab temporarily deployed to community-ci-auto 
GitHub Actions Inactive ChristoGrab temporarily deployed to community-ci-auto GitHub Actions Inactive 
Contributor
| @AGPapa Thanks for the solid contribution, and for sharing the logs so thoroughly! There were a couple failures in our pipeline that were unrelated to your changes. I merged the latest changes from master which should resolve the issue. @natikgadzhi All LGTM. I also had a chance to peek through the Braintree docs to verify, they're a bit sneaky as the fields themselves are listed as "Time" fields, but the actual descriptions do clearly specify the values simply as dates 🙄 Only thing I'm not sure about is how to approve the |
Contributor Author
| In other testing I did this morning I ran across a problem when I tried an incremental load (instead of full refresh). The problem is similar to one mentioned in this issue: #31061 The error I had was I think that this PR fixes everything for full reloads but might still have an unrelated issue for incremental loads. I should be able to fix that error too over the next day or so, I think I just need to tweak the datetime_format in the yml, should I include the change in this same PR here? Should we hold off on merging until we can figure that out? |
Contributor
| Okay, I see two errors in CI, let's fix them up real quick. @AGPapa one trick you can do to have a nice report is to run
We should fix both, it's a high usage connector. PyAirbyte failureAcceptance test in incrementals |
Contributor
| Oh I think I know what's up with pyairbyte. Braintree is still on Dockerfile + setup.py. We're moving all connectors to a unified base image and poetry, and this is one of the last outliers. |
Contributor
| @AGPapa mind adding me to your fork so I can push up a few commits to this PR? Courtesy of Claude:
|
Contributor
| Oh my lordy I see now. Braintree used Airbyte CDK 0.1. It's a bit older than I am. I guess I'll pin to the latest 0.1 available to see if it helps with tests. |
Contributor Author
| I've added a commit to add cursor_datetime_formats - that resolved the error I had when running incremental syncs. Running Hopefully pinning that version will fix the PyAirbyte validation tests - do you think it will fix the Acceptance tests as well? Or is there something else we need to update? This is the error I'm seeing I'm thinking that these tests are an older style and need to be updated? Not sure |
2 tasks
Contributor
| The failure that you see with I think I've fixed pyairbyte and integration tests problems in #40722, let's see. |
Contributor Author
| Closing this PR since #40722 was merged in it's place |
Contributor
| @AGPapa thank you for fixing up Source Braintree! <3 Great work. Is there anything else that you're looking to improve / clean up? What's the best way for me and the team to support you in using Airbyte? |
Contributor Author
I see there's also this issue about the braintree connector only loading 50 records at a time since this was updated to a "low code" connector. Perhaps this is because of pagination in the API? My sandbox environment doesn't have 50 records, so I'll need to make some more to be able to test it. If I can reproduce the issue then what's the best way to fix it? Is there another connector that I can look at to see how to handle this pagination? This data type issue was simple enough, but I'm not sure where to start with that other issue. |
Contributor
| It definitely looks like a broken paginator, but I am not certain why — haven't looked at the code. I don't think we have a sandbox account, but we can perform a regression test with one of our customers using Braintree. If you're interested in investigating and checking the difference in how 0.1.5 approached pagination and how 0.2.0 does it and putting together a PR, I will be happy to review, @AGPapa |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
What
For several columns in the subscriptions stream the type is incorrectly "date-time" when it should be "date". This causes syncs involving subscriptions to fail with this message:
invalid type; expected datetime, string, bytes, int or float (type=type_error)This has been brought up in at least two issues:
#15547
#14186
How
I switched the types to 'dates' instead of 'date-times' and it works! I also added in a missing column to the subscriptions stream - billing_period_end_date.
I tested this by running a connection from braintree's sandbox to a local CSV file. It failed before the change and works after it.
I also switched it from using the Dockerfile approach to using poetry instead (as directed by the test suite).
Review guide
User Impact
Users with subscription data will be able to sync it using airbyte. Users without subscription data should be unaffected, although there might be a datatype change in their empty subscription tables.
Can this PR be safely reverted and rolled back?