Skip to content
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/Cash Flow statement #69

Merged

Conversation

fivetran-avinash
Copy link
Contributor

@fivetran-avinash fivetran-avinash commented Jan 10, 2023

Are you a current Fivetran customer?

What change(s) does this PR introduce?

Did you update the CHANGELOG?

  • Yes

Does this PR introduce a breaking change?

  • Yes (please provide breaking change details below.)
  • No (please provide an explanation as to how the change is non-breaking below.)

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)

  • Yes (Will update version on feature/quickbooks-updates-project

Is this PR in response to a previously created Bug or Feature Request

How did you test the PR changes?

  • BuildKite
  • Local (please provide additional testing details below)

Select which warehouse(s) were used to test the PR

  • BigQuery
  • Redshift
  • Snowflake
  • Postgres
  • Databricks
  • Other (provide details below)

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.

@fivetran-avinash fivetran-avinash changed the title Feature/cash flow statement Feature/Cash flow statement Jan 11, 2023
@fivetran-avinash fivetran-avinash changed the title Feature/Cash flow statement Feature/Cash Flow statement Jan 11, 2023
@fivetran-avinash fivetran-avinash marked this pull request as ready for review January 11, 2023 16:30
@fivetran-avinash fivetran-avinash self-assigned this Jan 11, 2023
Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fivetran-avinash I have a few comments and suggestions below. This is looking great though!!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
fivetran-avinash and others added 2 commits January 11, 2023 14:12
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the previously requested changes @fivetran-avinash!

I just finished the model code review and found it worked great when using the default behavior. However, when using the ordinal seed file I found a fanout to occur. Would you be able to review my notes and adjust the model to resolve the fanout.

models/quickbooks.yml Show resolved Hide resolved
Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a 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 and we are so close to closing it out!

I have a few small suggestions and comments included below. Additionally, one last thought I had was the location of the example seed files are pretty hard to find. I know I requested they be put in the integration_tests seeds folder, but as I was working with the package it was a very confusing experience (especially since we have so many other seed files).

What if we instead created a new folder example_ordinal_seeds (or something like that) in the root directory of the package and put the example seeds there. This way it is very clear to customers where the examples are, and when you follow a hyper link it doesn't take you into the deep recesses of the package. Let me know your thoughts!

dbt_project.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@fivetran-avinash
Copy link
Contributor Author

@fivetran-avinash this is looking great and we are so close to closing it out!

I have a few small suggestions and comments included below. Additionally, one last thought I had was the location of the example seed files are pretty hard to find. I know I requested they be put in the integration_tests seeds folder, but as I was working with the package it was a very confusing experience (especially since we have so many other seed files).

What if we instead created a new folder example_ordinal_seeds (or something like that) in the root directory of the package and put the example seeds there. This way it is very clear to customers where the examples are, and when you follow a hyper link it doesn't take you into the deep recesses of the package. Let me know your thoughts!

@fivetran-joemarkiewicz Changes applied!

@fivetran-joemarkiewicz fivetran-joemarkiewicz added type:enhancement New functionality or enhancement status:in_review Currently in review labels Jan 12, 2023
@fivetran-joemarkiewicz fivetran-joemarkiewicz added the update_type:models Primary focus requires model updates label Jan 12, 2023
Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me!! Thanks for applying all the updates and working with me on the back and forth 😄. Great job @fivetran-avinash

@fivetran-avinash fivetran-avinash linked an issue Jan 13, 2023 that may be closed by this pull request
2 tasks
@fivetran-avinash fivetran-avinash added priority:p2 Affects most users; fix needed and removed priority:p2 Affects most users; fix needed type:enhancement New functionality or enhancement status:in_review Currently in review update_type:models Primary focus requires model updates labels Jan 13, 2023
@fivetran-avinash fivetran-avinash merged commit 8bd4697 into feature/quickbooks-updates-project Jan 13, 2023
@fivetran-avinash
Copy link
Contributor Author

Looks great to me!! Thanks for applying all the updates and working with me on the back and forth 😄. Great job @fivetran-avinash

Thanks for all the input @fivetran-joemarkiewicz!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FEATURE - Cashflow Statement
2 participants