Add union schema to quickbooks package #62
Conversation
There was a problem hiding this comment.
Hey @fivetran-avinash, great work on this PR! Overall, PR looks good. I left a couple of comments in-line, if you could review those - I can go through and mark them as resolved and approve this PR after they have been answered
packages.yml
Outdated
| revision: feature/add-class-ids-to-source-models | ||
| warn-unpinned: false | ||
| - git: https://github.com/fivetran/dbt_quickbooks_source.git | ||
| revision: feature/quickbooks-updates-project |
There was a problem hiding this comment.
reminder to switch this to the appropriate versions as necessary
There was a problem hiding this comment.
With packages.yml I should keep it the way it is for now since it's dependent on features/quickbook-updates-project for the time being, but will change when the branch is ready to merge.
…pass tests, updated CHANGELOG
CHANGELOG.md
Outdated
| # New Features (In Progress) | ||
|
|
||
| ## 🚨 Breaking Changes 🚨: | ||
| - Added `transaction_source` to surrogate key function to fix `unique_id` uniqueness issues. Customer will need to do either a full refresh or add in the variable to their modeling to produce up-to-date `unique_id` values. |
There was a problem hiding this comment.
Sorry for the back and forth for this. This is just a pretty big change that I think should be highlighted to the best of our ability.
If we release this as part of the migration branch, I think the messaging should be very similar to what we currently have in the changelog re: generating surrogate keys.
dbt_utils.surrogate_key has also been updated to dbt_utils.generate_surrogate_key. Since the method for creating surrogate keys differ, we suggest all users do a full-refresh for the most accurate data. For more information, please refer to dbt-utils release notes for this update.
Maybe this specific update could be a sub-bullet of the surrogate key note above from the migration branch. I mostly want to make sure that customers know that there is a variable they may need to toggle.
There was a problem hiding this comment.
Hey! Sorry for the confusion.
This branch will not be merged into migration, it'll be merged into feature/quickbooks-updates-project, which will then get merged onto main.
So either migration will get merged first, or feature/quickbooks-updates-project. I think if the latter occurs, it makes sense to do the following above. But right now I think the overall plan is to merge this after migration, which was the reason Joe created this separate branch to work off of.
Apologies if this is a bit confusing. Happy to discuss more at standup.
There was a problem hiding this comment.
Okay, that's fine! In that case "add in the variable" sounds ambiguous here, you can just reference the dbt-utils release with the variable reference that way they can just look into it on their own. I tend to try to be as specific as possible - helps give both of us and the end user a quick tltr and not have to click through all the PRs. Something like the below should do.
- Added
transaction_sourcetogenerate_surrogate_keyfunction to fixunique_iduniqueness issues in thequickbooks__general_ledgermodel. A full refresh is recommended for accurate and consistent surrogate keys, for more information please refer to dbt-utils release notes regardinggenerate_surrogate_key.
There was a problem hiding this comment.
Thanks for this Sheri! I'll do my best to be more thorough in the future.
dbt_project.yml
Outdated
| @@ -2,7 +2,7 @@ config-version: 2 | |||
| name: 'quickbooks' | |||
|
|
|||
| version: '0.6.0' | |||
| require-dbt-version: [">=1.3.0", "<2.0.0"] | |||
| require-dbt-version: [">=1.0.0", "<2.0.0"] | |||
There was a problem hiding this comment.
this should be 1.3 for post migration merges
There was a problem hiding this comment.
Good catch, updating!
Are you a current Fivetran customer?
What change(s) does this PR introduce?
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.