Feature: Multicurrency support (plus some bug fixes for double entry models) #134

fivetran-avinash · 2024-07-08T10:52:07Z

PR Overview

This PR will address the following Issue/Feature: [#128] (duplicate issue: #115), as well as bugfix [#135]. (Issue #127 was also validated/merged into this PR branch as well)

This PR will result in the following new package version:

v0.14.0

The introduction of new fields to support multicurrency makes this a breaking change.

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

🚨 Breaking Changes 🚨

Feature Requests: Multicurrency Support

We have introduced multicurrency support to the following models by providing these new fields that convert transaction amounts by their exchange rates. (PR #134)
We have kept the existing cash value fields that provides amounts and balances to ensure full coverage to customers regardless of their currency setup. (PR #134)
The new multicurrency fields that fulfill the same function as the respective existing fields is below:

Model	New Multicurrency Fields	Respective Single Currency Fields
quickbooks__general_ledger	`adjusted_converted_amount`, `running_converted_balance`	`adjusted_amount`, `running_balance`
quickbooks__general_ledger_by_period	`period_net_converted_change`, `period_beginning_converted_balance`, `period_ending_converted_balance`	`period_net_change`, `period_beginning_balance`, `period_ending_balance`
quickbooks__profit_and_loss	`converted_amount`	`amount`
quickbooks__balance_sheet	`converted_amount`	`amount`
quickbooks__cash_flow_statement	`cash_converted_ending_period`, `cash_converted_beginning_period`, `cash_converted_net_period`	`cash_ending_period`, `cash_beginning_period`, `cash_net_period`
quickbooks__ap_ar_enhanced	`total_converted_amount`, `estimate_total_converted_amount`, `total_current_converted_payment`	`total_amount`, `estimate_total_amount`, `total_current_payment`
quickbooks__expenses_sales_enhanced	`total_converted_amount`, `converted_amount`	`total_amount`, `amount`

Introduced *_converted_* type fields in our intermediate models to convert amounts where exchange rates exist for those transactions. If there is no exchange rate, these *_converted_* fields will default back to the already existing fields created for single currency, and all downstream calculations should match the single currency amount, balance and cash values. (PR #134)
For double-entry models that applied a cross-join to either AP/AR accounts, we added joins based on the currency_id value in the accounts source table for those transactions. (PR #134)
In the analysis folder, added the converted_balance to the quickbooks__balance_sheet and ending_converted_balance to the quickbooks__income_statement models. (PR #134)

Bug Fixes

Adjusted logic for discount sales receipt lines in int_quickbooks__sales_receipt_double_entry model to bring in these values properly as negative adjusted amounts in the quickbooks__general_ledger.
(PR #130)
Applied filter in int_quickbooks__invoice_double_entry to filter out 'Accounts Receivable' accounts that are inactive. (PR #134)

Under the Hood

Added consistency and integrity tests within integration tests for the quickbooks__general_ledger, quickbooks__general_ledger_by_period, quickbooks__balance_sheet, quickbooks__cash_flow_statement and quickbooks__profit_and_loss models. (PR #130) & (PR 134)

Documentation Update

Updated README to reflect the new multicurrency support. (PR #134)
Added yml documentation with the new multicurrency fields and descriptions. (PR #134)

PR Checklist

Basic Validation

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

dbt run –full-refresh && dbt test
[NA] 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:

A host of new validation tests have been added to our most used end models, as mentioned above. These will check for consistency of amounts between dev and prod (we only look at the regular amount for now, as converted_amount doesn't exist in prod at this moment--we will probably need to update this in a future round of validation testing), and then integrity tests assessing no changes on amounts and converted amounts for the various end model grains.

There were also regular consistency tests looking at row values for ap_ar_enhanced and expense_sales_enhanced. These tests will be modified before deploying to main.

One thing to call out is you will have to configure the vars in the integration_tests/dbt_project.yml to get the enabled_union_models macro to work properly in the general_ledger_amounts_match test model.

vars:
using_bill: true
using_credit_card_payment_txn: true

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

💴 💷 💶

… validation tests

fivetran-joemarkiewicz

@fivetran-avinash fantastic work on this PR! I have a few questions, comments, and suggestions below. Let me know if you want to sync on any of my comments. Thanks!

integration_tests/tests/consistency/consistency_balance_sheet_amount.sql

README.md

models/double_entry_transactions/int_quickbooks__sales_receipt_double_entry.sql

models/intermediate/int_quickbooks__bill_join.sql

fivetran-joemarkiewicz · 2024-07-17T19:40:47Z

models/intermediate/int_quickbooks__invoice_join.sql

        cast(null as {{ dbt.type_string() }}) as estimate_status,

        {% endif %}

        invoice_link.due_date as due_date,
+        (payments.total_amount * coalesce(payments.exchange_rate, 1)) as total_current_converted_payment,


Similar comment to a previous question, is there a related field to this which is available in prod?

See my comment on int_quickbooks__invoice_join for the update

models/quickbooks__general_ledger.sql

models/quickbooks.yml

fivetran-joemarkiewicz · 2024-07-17T20:13:07Z

packages.yml

+  #   version: [">=0.10.0", "<0.11.0"]
+
+  - git: https://github.com/fivetran/dbt_quickbooks_source.git
+    revision: feature/add-home-total-amount-to-deposit
+    warn-unpinned: false


Reminder to swap before merging

CHANGELOG.md

fivetran-avinash

@fivetran-joemarkiewicz Addressed all PR notes/reran tests/regenerated docs. Should be ready for re-review.

fivetran-avinash · 2024-07-17T10:53:27Z

integration_tests/tests/consistency/consistency_expenses_sales_enhanced.sql

+dev as (
+    select *
+    --remove the below line before merging to main
+    except(converted_amount, total_converted_amount)


@fivetran-avinash Reminder to remove except clause before merging.

fivetran-avinash · 2024-07-17T10:53:42Z

integration_tests/tests/consistency/consistency_ap_ar_enhanced.sql

+dev as (
+    select *
+    --remove the below line before merging to main
+    except (total_converted_amount, estimate_total_converted_amount, total_current_converted_payment)


@fivetran-avinash Reminder to remove except clause before merging.

integration_tests/tests/consistency/consistency_balance_sheet_amount.sql

README.md

models/quickbooks.yml

CHANGELOG.md

fivetran-joemarkiewicz

@fivetran-avinash great work on this PR and thanks for addressing the outstanding items. I have a few very minor comments. Otherwise this PR is good to go!

fivetran-joemarkiewicz · 2024-07-18T21:00:35Z

README.md


-> Please be aware that the [dbt_quickbooks](https://github.com/fivetran/dbt_quickbooks) and [dbt_quickbooks_source](https://github.com/fivetran/dbt_quickbooks_source) packages were developed with single currency company data. As such, the package models will not reflect accurate totals if your QuickBooks account has Multi-Currency enabled.
+> [dbt_quickbooks](https://github.com/fivetran/dbt_quickbooks) and [dbt_quickbooks_source](https://github.com/fivetran/dbt_quickbooks_source) now supports multicurrency by bringing in values by specifying `*_converted_*` values for cash. More details are [available in the DECISIONLOG](https://github.com/fivetran/dbt_quickbooks/blob/main/DECISIONLOG.md#multicurrency-vs-single-currency-configuration).


Small wording suggestion update.

Suggested change

> [dbt_quickbooks](https://github.com/fivetran/dbt_quickbooks) and [dbt_quickbooks_source](https://github.com/fivetran/dbt_quickbooks_source) now supports multicurrency by bringing in values by specifying `*_converted_*` values for cash. More details are [available in the DECISIONLOG](https://github.com/fivetran/dbt_quickbooks/blob/main/DECISIONLOG.md#multicurrency-vs-single-currency-configuration).

> [dbt_quickbooks](https://github.com/fivetran/dbt_quickbooks) and [dbt_quickbooks_source](https://github.com/fivetran/dbt_quickbooks_source) now supports multicurrency by bringing in values by specifying `*_converted_*` values for cash amounts. More details are [available in the DECISIONLOG](https://github.com/fivetran/dbt_quickbooks/blob/main/DECISIONLOG.md#multicurrency-vs-single-currency-configuration).

fivetran-joemarkiewicz · 2024-07-18T21:01:03Z

README.md

-> Please be aware that the [dbt_quickbooks](https://github.com/fivetran/dbt_quickbooks) and [dbt_quickbooks_source](https://github.com/fivetran/dbt_quickbooks_source) packages were developed with single currency company data. As such, the package models will not reflect accurate totals if your QuickBooks account has Multi-Currency enabled.
+> [dbt_quickbooks](https://github.com/fivetran/dbt_quickbooks) and [dbt_quickbooks_source](https://github.com/fivetran/dbt_quickbooks_source) now supports multicurrency by bringing in values by specifying `*_converted_*` values for cash. More details are [available in the DECISIONLOG](https://github.com/fivetran/dbt_quickbooks/blob/main/DECISIONLOG.md#multicurrency-vs-single-currency-configuration).
+
+There are specific limitations if you need converted amounts for credit card payments and transfers, as we do not currently have the ability to validate a best approach to convert the original amounts into their proper home currency. [Please open an issue with us](https://github.com/fivetran/dbt_quickbooks/issues/new/choose) if this is critical and/or you believe you can help contribute to scoping out adding that functionality in..


I really like this section! One small edit.

Suggested change

There are specific limitations if you need converted amounts for credit card payments and transfers, as we do not currently have the ability to validate a best approach to convert the original amounts into their proper home currency. [Please open an issue with us](https://github.com/fivetran/dbt_quickbooks/issues/new/choose) if this is critical and/or you believe you can help contribute to scoping out adding that functionality in..

There are specific limitations if you need converted amounts for credit card payments and transfers, as we do not currently have the ability to validate a best approach to convert the original amounts into their proper home currency. [Please open an issue with us](https://github.com/fivetran/dbt_quickbooks/issues/new/choose) if this is critical and/or you believe you can help contribute to scoping out adding that functionality in.

models/intermediate/int_quickbooks__bill_join.sql

fivetran-catfritz

@fivetran-avinash This is generally looking great--I just have one small question!

fivetran-catfritz · 2024-07-19T20:43:55Z

integration_tests/dbt_project.yml

+  # For validation testing.  
+  # This below vars configuration is needed for the `general_ledger_amounts_match` macro.
+  using_bill: true
+  using_credit_card_payment_txn: true


Since the default for using_credit_card_payment_txn is false but we're setting it true here, should we set it to false in this line so one run tests true and the other false? Or, you could just comment out this L70 when sending to buildkite?

Also I'm not sure if it would be related to this PR, it looks like the default value is set as false everywhere except for here. Is that intentional?

@fivetran-catfritz Oh good catch. This should have been commented out, will fix now.

In the second link, I believe this is false by default? It's conditional on the variable being true.

@fivetran-avinash According to the docs, {% var('using_credit_card_payment_txn', True) %} means the default value is True.
The line {% if var('using_credit_card_payment_txn', True) %} simply evaluates if the variable value provided is true/false.

@fivetran-catfritz Ahh, good catch! Yes it should probably be consistent then.

Interestingly, I don't think the behavior changes very much unless the variable is not set, but since it is set by default the cases that it wouldn't be set are probably rare, which is why this hasn't been flagged.

fivetran-catfritz

Awesome! LGTM.

Add multicurrency support to end models

91c99dd

fivetran-avinash changed the base branch from main to release/v0.14.0 July 8, 2024 10:52

fivetran-avinash added 2 commits July 9, 2024 03:52

add multicurrency support to additional models, update documentation,…

6948e95

… validation tests

Bump schema

f85f5a2

fivetran-joemarkiewicz mentioned this pull request Jul 11, 2024

[Bug] Invoice Double Entry not filtering inactive/sub account AR #135

Closed

4 tasks

fivetran-avinash linked an issue Jul 13, 2024 that may be closed by this pull request

[Bug] Invoice Double Entry not filtering inactive/sub account AR #135

Closed

4 tasks

fivetran-avinash self-assigned this Jul 16, 2024

This was linked to issues Jul 16, 2024

dbt_quickbooks package: quickbooks__profit_and_loss - 'amount' column is mixing currencies #115

Closed

[Bug] Currency Conversion not occurring in GL transformation #128

Closed

fivetran-avinash added 2 commits July 16, 2024 01:25

testing/documentation

58df6c7

testing/documentation

4cf0ad9

fivetran-avinash mentioned this pull request Jul 16, 2024

Feature: Add home_total_amount to the stg_quickbooks__deposit model fivetran/dbt_quickbooks_source#53

Merged

5 tasks

fivetran-avinash added 4 commits July 16, 2024 02:32

CHANGELOG

8156cda

bump schema

0448f59

fix and add tests

07eb792

fix tests

7ae35c7

fivetran-avinash changed the title ~~Feature/multicurrency support~~ Feature: Multicurrency support (plus some bug fixes for double entry models) Jul 17, 2024

fivetran-avinash marked this pull request as ready for review July 17, 2024 16:31

fivetran-avinash requested a review from fivetran-joemarkiewicz July 17, 2024 16:33

fivetran-joemarkiewicz requested changes Jul 17, 2024

View reviewed changes

fivetran-joemarkiewicz reviewed Jul 17, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

fivetran-joemarkiewicz reviewed Jul 17, 2024

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

fivetran-avinash added 3 commits July 17, 2024 23:20

PR review notes

03022b7

fixes

bffceda

schema

258fdf8

fivetran-avinash commented Jul 18, 2024

View reviewed changes

fivetran-joemarkiewicz approved these changes Jul 18, 2024

View reviewed changes

final PR notes

ae46b2e

fivetran-catfritz reviewed Jul 19, 2024

View reviewed changes

vars config

03ad811

fivetran-avinash added 4 commits July 19, 2024 17:26

release review updates

ac946b6

regen docs

044a815

schema

fe35f20

schema

804d155

fivetran-catfritz approved these changes Jul 22, 2024

View reviewed changes

pre-merge updates

93bbe5d

fivetran-avinash merged commit e76e353 into release/v0.14.0 Jul 22, 2024
8 checks passed

fivetran-avinash mentioned this pull request Jul 22, 2024

Release/v0.14.0 #136

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Multicurrency support (plus some bug fixes for double entry models) #134

Feature: Multicurrency support (plus some bug fixes for double entry models) #134

fivetran-avinash commented Jul 8, 2024 •

edited

Loading

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Jul 17, 2024

fivetran-avinash Jul 18, 2024

fivetran-joemarkiewicz Jul 17, 2024

fivetran-avinash left a comment

fivetran-avinash Jul 17, 2024

fivetran-avinash Jul 17, 2024

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Jul 18, 2024

fivetran-avinash Jul 18, 2024

fivetran-joemarkiewicz Jul 18, 2024

fivetran-avinash Jul 18, 2024

fivetran-catfritz left a comment

fivetran-catfritz Jul 19, 2024

fivetran-avinash Jul 19, 2024 •

edited

Loading

fivetran-catfritz Jul 19, 2024

fivetran-avinash Jul 19, 2024

fivetran-catfritz left a comment


		> Please be aware that the [dbt_quickbooks](https://github.com/fivetran/dbt_quickbooks) and [dbt_quickbooks_source](https://github.com/fivetran/dbt_quickbooks_source) packages were developed with single currency company data. As such, the package models will not reflect accurate totals if your QuickBooks account has Multi-Currency enabled.
		> [dbt_quickbooks](https://github.com/fivetran/dbt_quickbooks) and [dbt_quickbooks_source](https://github.com/fivetran/dbt_quickbooks_source) now supports multicurrency by bringing in values by specifying `_converted_` values for cash. More details are [available in the DECISIONLOG](https://github.com/fivetran/dbt_quickbooks/blob/main/DECISIONLOG.md#multicurrency-vs-single-currency-configuration).

Feature: Multicurrency support (plus some bug fixes for double entry models) #134

Feature: Multicurrency support (plus some bug fixes for double entry models) #134

Conversation

fivetran-avinash commented Jul 8, 2024 • edited Loading

PR Overview

🚨 Breaking Changes 🚨

Feature Requests: Multicurrency Support

Bug Fixes

Under the Hood

Documentation Update

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

fivetran-avinash 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

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-catfritz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-avinash Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-catfritz left a comment

Choose a reason for hiding this comment

fivetran-avinash commented Jul 8, 2024 •

edited

Loading

fivetran-avinash Jul 19, 2024 •

edited

Loading