Stripe updates #59
Stripe updates #59
Conversation
There was a problem hiding this comment.
@fivetran-reneeli we are getting close! Please see my comments below and let me know if you want to discuss any in more detail. Specifically let me know if you want to chat about the consolidation of the stg_stripe__pricing model.
Additionally, there were two or so comments from my previous review that were missed. Please look through the previous comments above for any unresolved ones to still be addressed.
Co-authored-by: Joe Markiewicz <[email protected]>
There was a problem hiding this comment.
@fivetran-reneeli I think this is the absolute final review with only a few suggestions and change requests. This shouldn't take too long to update. Let me know once the changes are applied and I can approve this PR.
Additionally, once you make these changes be sure to regen the docs for this package one last time. We will then be good to go!
fivetran-reneeli
changed the base branch from
feature/stripe-updates-q4-2022
to
main
Co-authored-by: Joe Markiewicz <[email protected]>
Co-authored-by: Joe Markiewicz <[email protected]>
Co-authored-by: Joe Markiewicz <[email protected]>
There was a problem hiding this comment.
@fivetran-reneeli thanks for working through all my comments and addressing the requested updates!
I have two small comments regarding the README and CHANGELOG; however, they are relatively small and shouldn't need a re-review. As such, I am approving this PR. Thanks for all your hard work!
| @@ -129,7 +163,7 @@ Alternatively, if you only have strings in your JSON object, the metadata variab | |||
|
|
|||
| ```yml | |||
| vars: | |||
| stripe__plan_metadata: ['the', 'list', 'of', 'property', 'fields'] # Note: this is case-SENSITIVE and must match the casing of the property as it appears in the JSON | |||
| stripe__subscription_metadata: ['the', 'list', 'of', 'property', 'fields'] # Note: this is case-SENSITIVE and must match the casing of the property as it appears in the JSON | |||
There was a problem hiding this comment.
The above note needs to reference the stripe__subscription_metadata variable instead of the old stripe__plan_metadata variable.
| stripe__plan_metadata: | ||
| - name: rename | ||
| - alias: renamed_field | ||
| stripe__price_plan_metadata: ## Used for both Price and Plan sources |
There was a problem hiding this comment.
My apologies for not catching this, but we will need to update the CHANGELOG that this variable was renamed. Actually, it looks like we didn't include any of the metadata variables in the previous -> new variable names. Can you add those there.
There was a problem hiding this comment.
@fivetran-joemarkiewicz it looks like the other metadata variables were already prefixed with stripe, so I don't think they were renamed?
There was a problem hiding this comment.
Ah good catch. Thanks for calling that out. If that is the case then we can just call out the price_plan change.
There was a problem hiding this comment.
Yup -- added that
stripe__plan_metadatavariable has been renamed tostripe__price_plan_metadata
…pe_source into stripe_updates
Are you a current Fivetran customer?
Internal
What change(s) does this PR introduce?
Q4 Stripe updates
pricetable replacing theplantable for new customersDid you update the CHANGELOG?
Does this PR introduce a breaking change?
Will need a full refresh to have all the new columns/ tables
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.