Bugfix/v0.12.5 #124

fivetran-reneeli · 2024-05-03T21:15:09Z

PR Overview

This PR will address the following Issue/Feature:
#122
#80
Zendesk ticket 200711

This PR will result in the following new package version:

v0.13.0

Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:

🚨 Breaking Changes 🚨:

PR #124 includes the following updates:

Bug Fixes

Updates the int_quickbooks__invoice_join and downstream quickbooks__ap_ar_enhanced models to include the using_payments config. Previously, this model would fail if the payment or the payment_line source tables did not exist.
Requires quickbooks__ap_ar_enhanced to have using_payments enabled in order to run, otherwise the model is limited in its use.
Updates the quickbooks__profit_and_loss model to include both period_first_day and period_last_day. This allows users to have greater optionality in choosing which date to aggregate records upon.
Updates the logic for amount in int_quickbooks__invoice_double_entry to use invoice.total_amount only on the condition there exists bundle_id and total_amount = 0, otherwise use invoice_lines.amount. This avoids double counting when aggregating invoice_line items and accounts for the edge cases where a bundle_id is involved.

PR Checklist

Basic Validation

Please acknowledge that you have successfully performed the following commands locally:

dbt run –full-refresh && dbt test
dbt run (if incremental models are present) && dbt test

Before marking this PR as "ready for review" the following have been applied:

The appropriate issue has been linked, tagged, and properly assigned
All necessary documentation and version upgrades have been applied
docs were regenerated (unless this PR does not include any code or yml updates)
BuildKite integration tests are passing
Detailed validation steps have been provided below

Detailed Validation

Please share any and all of your validation steps:

Customer for [Bug] Invoices with 0 balance due because of deposit being applied result in incorrect revenue on P&L #122 has confirmed the amount update works
Issue calendar date in the profit and loss model should be the last day of the month #80 is straightforward with adding new fields in quickbooks__profit_and_loss
Zendesk ticket 200711 involved adding using_payments config to select models, and this was validated by running with the following vars enabled/disabled:

  using_payment: true
  using_bill: true
  using_invoice: true

If you had to summarize this PR in an emoji, which would it be?

💃

…and last, and adding more using_payment configs downstream

fivetran-joemarkiewicz

@fivetran-reneeli great job working through these changes and confirming the updates work for the customer. I do have a few comments and suggestions before approving. Let me know if you have any questions. Thanks!

dbt_project.yml

models/quickbooks__profit_and_loss.sql

models/quickbooks__ap_ar_enhanced.sql

fivetran-joemarkiewicz · 2024-05-14T15:10:07Z

models/intermediate/int_quickbooks__invoice_join.sql

@@ -21,6 +21,7 @@ estimates as (
 ),
 {% endif %}

+{% if var('using_payment', True) %}


The int_quickbooks__invoice_join model is only used as an upstream model in the quickbooks__ap_ar_enhanced model.

Additionally, this model is not materialized and is ephemeral in our config.

Because of this, would it make more sense to take the same approach you took for the quickbooks__ap_ar_enhanced model and just entirely disable this model if payments are disabled? This would allow us to reduce the code changes and not run code in the customers warehouse if it is not entirely necessary.

Ah I think this is a good point-- There's no point in running int_quickbooks__invoice_join if the downstream quickbooks__ap_ar_enhanced is disabled. I added the same configs (invoice and payments enabled) to this model.

Looks great! Technically, you can revert your changes from before where the specific {% if var('using_payment', True) %} conditionals are included. Now that this model won't be run there is no need for these added conditionals.

You can safely use the previous code from this model, but only make the following change and that should return the desired results

{{ config(enabled=var('using_invoice') and var('using_payment', True)) }}

ah good point-- updated to revert and remove those original changes

CHANGELOG.md

Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>

fivetran-joemarkiewicz

@fivetran-reneeli great work on this PR! I have two final suggestions, but they don't need to hold back the approval. Once you make those final changes and regen the docs, you can open this for release review!

Let me know if you have any questions. Thanks!

CHANGELOG.md

fivetran-joemarkiewicz · 2024-05-14T18:34:56Z

models/intermediate/int_quickbooks__invoice_join.sql

@@ -21,6 +21,7 @@ estimates as (
 ),
 {% endif %}

+{% if var('using_payment', True) %}


Looks great! Technically, you can revert your changes from before where the specific {% if var('using_payment', True) %} conditionals are included. Now that this model won't be run there is no need for these added conditionals.

fivetran-joemarkiewicz · 2024-05-14T18:41:45Z

models/intermediate/int_quickbooks__invoice_join.sql

@@ -21,6 +21,7 @@ estimates as (
 ),
 {% endif %}

+{% if var('using_payment', True) %}


You can safely use the previous code from this model, but only make the following change and that should return the desired results

{{ config(enabled=var('using_invoice') and var('using_payment', True)) }}

fivetran-joemarkiewicz

See the above comment for approval notes

Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>

fivetran-jamie

looks great! just some doc-related comments around deprecating calendar_date

CHANGELOG.md

fivetran-jamie · 2024-05-14T22:12:42Z

CHANGELOG.md

+- Corrects the misspelled `customer_vendor_webiste` to `customer_vendor_website` in `quickbooks__ap_ar_enhanced`.
+
+## Bug Fixes
+- Updates the [quickbooks__profit_and_loss](https://github.com/fivetran/dbt_quickbooks/blob/main/models/quickbooks__profit_and_loss.sql) and [quickbooks__balance_sheet](https://github.com/fivetran/dbt_quickbooks/blob/main/models/quickbooks__balance_sheet.sql) models to include both `period_first_day` and `period_last_day`. This allows users to have greater flexibility in choosing which date to aggregate records upon.


is this a bug fix or feature update?

also are we intending on deprecating calendar_date? if so, maybe we should mention it here (and maybe in calendar_date's yml description) and recommend using one of the new fields instead

Thanks @fivetran-jamie , moved that to its own feature update section and added the deprecation notice for calendar_date:

"This is slated to be deprecated, and the fields period_first_day and period_last_day are both offered as replacements, depending on how your company performs their financial reporting."

that sounds great! do you have the feature_update section added locally?

fivetran-jamie · 2024-05-14T22:22:45Z

models/quickbooks__balance_sheet.sql

@@ -8,6 +8,8 @@ with general_ledger_by_period as (
 final as (
    select
        period_first_day as calendar_date,
+        period_first_day,


maybe we should note here as well in a comment that calendar_date will be deprecated

good point! added

fivetran-jamie · 2024-05-14T22:22:59Z

models/quickbooks__profit_and_loss.sql

@@ -8,6 +8,8 @@ with general_ledger_by_period as (
 final as (
    select
        period_first_day as calendar_date,
+        period_first_day,


same comment as above

Co-authored-by: Jamie Rodriguez <65564846+fivetran-jamie@users.noreply.github.com>

sbrudz · 2024-05-17T13:59:38Z

models/quickbooks__profit_and_loss.sql

@@ -7,7 +7,7 @@ with general_ledger_by_period as (

 final as (
    select
-        period_first_day as calendar_date,
+        period_first_day as calendar_date, --  Slated to be deprecated; we recommend using `period_first_day` or `period_last_day`


I just noticed that period_first_day and period_last_day aren't included in the quickbooks__profit_and_loss SQL even though the yml definition says the columns exist.

Ah @sbrudz you're right, this was mistakenly removed during the PR. We will have this fix ready first thing next week!

Thanks @sbrudz for catching this, the package has been updated!

fivetran-reneeli added 2 commits May 3, 2024 17:14

initial updates

4ae8707

oops

c337de5

fivetran-reneeli self-assigned this May 3, 2024

fivetran-reneeli added 10 commits May 3, 2024 17:25

more updates

758df4e

more updates to changelog, calendar date revert and add period first …

de555fe

…and last, and adding more using_payment configs downstream

fix

96ff2ae

updates

b4bd9be

updates

36c6cef

add payment config and fix typo

06c2754

more updates

3c01a15

try limiting to dbt adapter less than 1.8.0

60b3ce9

updates

0e19e72

docs

b14cdb4

This was linked to issues May 13, 2024

calendar date in the profit and loss model should be the last day of the month #80

Closed

[Bug] Invoices with 0 balance due because of deposit being applied result in incorrect revenue on P&L #122

Closed

fivetran-joemarkiewicz self-requested a review May 14, 2024 14:52

fivetran-joemarkiewicz requested changes May 14, 2024

View reviewed changes

fivetran-reneeli and others added 2 commits May 14, 2024 13:41

Update CHANGELOG.md

4a0211f

Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>

updates

44b2010

fivetran-joemarkiewicz reviewed May 14, 2024

View reviewed changes

fivetran-joemarkiewicz approved these changes May 14, 2024

View reviewed changes

fivetran-reneeli and others added 3 commits May 14, 2024 17:36

Update CHANGELOG.md

55fa537

Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>

remove individual payment config

e3ee3d4

additional

d7d564a

fivetran-jamie reviewed May 14, 2024

View reviewed changes

fivetran-reneeli and others added 2 commits May 15, 2024 10:58

Update CHANGELOG.md

2383bdc

Co-authored-by: Jamie Rodriguez <65564846+fivetran-jamie@users.noreply.github.com>

update

7337bc9

fivetran-jamie approved these changes May 15, 2024

View reviewed changes

docs

5372dfa

fivetran-reneeli merged commit a10932c into main May 15, 2024
8 checks passed

fivetran-reneeli deleted the bugfix/v0.12.5 branch May 15, 2024 18:56

fivetran-reneeli linked an issue May 15, 2024 that may be closed by this pull request

first and last day columns for the general ledger model #79

Open

4 tasks

fivetran-reneeli removed a link to an issue May 15, 2024

first and last day columns for the general ledger model #79

Open

4 tasks

sbrudz reviewed May 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix/v0.12.5 #124

Bugfix/v0.12.5 #124

fivetran-reneeli commented May 3, 2024 •

edited

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz May 14, 2024

fivetran-reneeli May 14, 2024

fivetran-joemarkiewicz May 14, 2024

fivetran-joemarkiewicz May 14, 2024

fivetran-reneeli May 14, 2024

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz May 14, 2024

fivetran-joemarkiewicz May 14, 2024

fivetran-joemarkiewicz left a comment

fivetran-jamie left a comment

fivetran-jamie May 14, 2024

fivetran-jamie May 14, 2024

fivetran-reneeli May 15, 2024

fivetran-jamie May 15, 2024

fivetran-jamie May 14, 2024

fivetran-reneeli May 15, 2024

fivetran-jamie May 14, 2024

fivetran-reneeli May 15, 2024

sbrudz May 17, 2024

fivetran-reneeli May 17, 2024

sbrudz May 17, 2024

fivetran-reneeli May 20, 2024

Bugfix/v0.12.5 #124

Bugfix/v0.12.5 #124

Conversation

fivetran-reneeli commented May 3, 2024 • edited

PR Overview

🚨 Breaking Changes 🚨:

Bug Fixes

PR Checklist

Basic Validation

Detailed Validation

If you had to summarize this PR in an emoji, which would it be?

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

fivetran-jamie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-reneeli commented May 3, 2024 •

edited