-
Notifications
You must be signed in to change notification settings - Fork 35
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
Feature/stripe updates q4 2022 #56
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fivetran-reneeli a few more comments, but we are getting close!
Additionally, I noticed the .DS_Store file is in this repo. Would you mind removing that since it is not technically necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fivetran-reneeli thanks for making all the changes I requested and responding to my questions.
I believe this will be the last review where I request changes! I have a few comments and requests in my review. Additionally, please apply the same CHANGELOG and README updates from the source review into this one.
Finally, the biggest request I have is to address the issue in the daily_overview model where the weekends (days without transactions) are showing as null
. We have a good roadmap in Recurly on how to address this issue. Let me know if you would like for me to elaborate more on this piece.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fivetran-reneeli thanks for making those final updates! After reviewing, this looks ready to go! I just have a few super minor comments and suggestions, but otherwise this is good to ship!
| using_invoices | stripe__using_invoices | ||
| using_credit_notes | stripe__using_credit_notes | ||
| using_payment_method | stripe__using_payment_method | ||
| using_livemode | stripe__using_livemode | ||
| using_invoice_line_sub_filter | stripe__using_invoice_line_sub_filter | ||
| using_subscriptions | stripe__using_subscriptions | ||
| using_subscription_history | stripe__using_subscription_history | ||
| using_price | stripe__using_price |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also include the name change for the stripe__plan_metadata variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added-- mentioned in its own bullet point since these rows are just for variables that have been prefixed with stripe__
select | ||
date_spine.date_day, | ||
date_spine.account_id, | ||
balance_transaction.source_relation, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This field is showing as null for the days without transactions. To be consistent, let's use the date_spine source_relation field. Let me know if you have any concerns with this suggestion.
balance_transaction.source_relation, | |
date_spine.source_relation, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh good catch, updated
packages.yml
Outdated
# - package: fivetran/stripe_source | ||
# version: [">=0.8.0", "<0.9.0"] | ||
- git: https://github.com/fivetran/dbt_stripe_source.git | ||
revision: stripe_updates | ||
warn-unpinned: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder to switch before release
packages.yml
Outdated
# - package: fivetran/stripe_source | ||
# version: [">=0.8.0", "<0.9.0"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# - package: fivetran/stripe_source | |
# version: [">=0.8.0", "<0.9.0"] | |
# - package: fivetran/stripe_source | |
# version: [">=0.9.0", "<0.10.0"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
Are you a current Fivetran customer?
Internal
What change(s) does this PR introduce?
Incoming stripe updates include the following:
pricing
source table which may replace the plan table depending on whether customer migrated to the Price API (some column names have changed instripe__invoice_line_items
)stripe__invoice_details
andstripe__account_daily_overview
stripe__invoice_line_items
with pricing data andsubscription_item_id
stripe__subscription_line_items
modelFor more info see PRs #54 and #55
Did you update the CHANGELOG?
Does this PR introduce a breaking change?
Did you update the dbt_project.yml files with the version upgrade (please leverage standard semantic versioning)? (In both your main project and integration_tests)
Is this PR in response to a previously created Bug or Feature Request
How did you test the PR changes?
Select which warehouse(s) were used to test the PR
Provide an emoji that best describes your current mood
💃
Feedback
We are so excited you decided to contribute to the Fivetran community dbt package! We continue to work to improve the packages and would greatly appreciate your feedback on our existing dbt packages or what you'd like to see next.