-
Notifications
You must be signed in to change notification settings - Fork 39
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
Add account class ordering to balance profit loss models #65
Add account class ordering to balance profit loss models #65
Conversation
swapping credit and debit accounts
…vetran/dbt_quickbooks into feature/backwards-credit-card
…vetran/dbt_quickbooks into feature/backwards-credit-card
Feature/backwards credit card
Update packages
…ation Updates for dbt-utils to dbt-core cross-db macro migration
…ount fields, updated README and CHANGELOG
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-avinash this is looking great, but currently this customization will not work for customers. They will not be able to edit the seed file which then removes the flexibility that we are trying to configure. Additionally, the customer needs to run a dbt build
or dbt seed
command in order to take advantage of these ordinals. That is a pretty big road block from the majority of our customers who are just running dbt run
.
I have a number of comments included in this review that I feel we should modify with this approach in order for the outcome to work in the way we want. Ultimately we want there to be a way for customers to leverage this feature if they want, but not have to worry about it if they don't care to take advantage of it. Currently, regardless if they want it or not, they need to configure change their workflow to account for this feature. We need to add to this feature in order to ensure a seamless workflow for customers that want to use it, and if they don't want to customize the ordinals.
I tried to be thorough in my suggestions, but feel free to reach out if it is not clear. Once we get this working for both situations I think this will be a super powerful feature!
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-avinash thanks so much for reviewing my comments diligently and applying the updates! The code looks great, I only have a few minor requests to update the README logic.
Once those are applied, I will re-review and we can fold into your updates branch!
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
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.
Thanks for applying these updates and they look great! LGTM 🙌
Are you a current Fivetran customer?
What change(s) does this PR introduce?
account_class_ordinal
toquickbooks__balance_sheet
andquickbooks__profit_and_loss
to allow customers to order their data by account class type.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.