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

[Pre-release] Bug fix for net income adjustment for no revenue/expense lines #147

Conversation

fivetran-avinash
Copy link
Contributor

@fivetran-avinash fivetran-avinash commented Nov 18, 2024

Test branch for the following changes. This is a pre-release.

Bug Fix

  • Updated the logic in int_quickbooks__retained_earnings to ensure accounting periods with no revenue and expense class lines were accounted for.
    • This will ensure the net income adjustment is available regardless of existing revenue or expenses.

@fivetran-avinash fivetran-avinash self-assigned this Nov 18, 2024
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.

This looks good, but are we planning for this to be a pre-release or an official release? If pre-release please make the two changes requested below.

CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@fivetran-avinash fivetran-avinash left a comment

Choose a reason for hiding this comment

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

@fivetran-joemarkiewicz Thanks for the quick checks! This PR is ready for re-review.

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.

LGTM with the one small adjustment in the README. This will be fine to start the release process for the pre-release tomorrow.

README.md Outdated
@@ -73,7 +73,7 @@ Include the following QuickBooks package version in your `packages.yml` file.
```yaml
packages:
- package: fivetran/quickbooks
version: [">=0.16.0", "<0.17.0"] # we recommend using ranges to capture non-breaking changes automatically
version: v0.17.0-a1 # we recommend using ranges to capture non-breaking changes automatically
Copy link
Contributor

Choose a reason for hiding this comment

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

The official version will not include the v once it's on the dbt hub.

Suggested change
version: v0.17.0-a1 # we recommend using ranges to capture non-breaking changes automatically
version: 0.17.0-a1 # we recommend using ranges to capture non-breaking changes automatically

Copy link
Contributor

@fivetran-catfritz fivetran-catfritz left a comment

Choose a reason for hiding this comment

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

lgtm

@fivetran-avinash fivetran-avinash merged commit c0ca2ea into main Nov 21, 2024
8 checks passed
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.

3 participants