[Prerelease] Apply home currency exchange rate to fix unneeded multicurrency issues. #188
[Prerelease] Apply home currency exchange rate to fix unneeded multicurrency issues. #188fivetran-avinash wants to merge 9 commits intomainfrom
Conversation
fivetran-joemarkiewicz
left a comment
There was a problem hiding this comment.
A few comments before pre-release approval
| case | ||
| when invoices.currency_id = '{{ var('quickbooks__home_currency', '') }}' | ||
| then case when invoice_lines.bundle_id is not null and invoices.total_amount = 0 | ||
| then invoices.total_amount | ||
| else coalesce(invoice_bundles.amount, invoice_lines.amount) | ||
| end | ||
| when invoice_lines.bundle_id is not null and invoices.total_amount = 0 | ||
| then invoices.total_amount * coalesce(invoices.exchange_rate, 1) | ||
| else coalesce(invoice_bundles.amount, invoice_lines.amount) * coalesce(invoices.exchange_rate, 1) |
There was a problem hiding this comment.
In an effort to not maintain two versions of the same original case statement I believe we should only use the variable case when on the original lines 215 and 216 where it's required. Otherwise, I worry about us updating one of these case statements and not updating the other in a future update.
There was a problem hiding this comment.
For example, I'm thinking something along the lines of the following (this will need to be verified):
| case | |
| when invoices.currency_id = '{{ var('quickbooks__home_currency', '') }}' | |
| then case when invoice_lines.bundle_id is not null and invoices.total_amount = 0 | |
| then invoices.total_amount | |
| else coalesce(invoice_bundles.amount, invoice_lines.amount) | |
| end | |
| when invoice_lines.bundle_id is not null and invoices.total_amount = 0 | |
| then invoices.total_amount * coalesce(invoices.exchange_rate, 1) | |
| else coalesce(invoice_bundles.amount, invoice_lines.amount) * coalesce(invoices.exchange_rate, 1) | |
| case when invoice_lines.bundle_id is not null and invoices.total_amount = 0 | |
| then | |
| case when invoices.currency_id = '{{ var('quickbooks__home_currency', '') }}' | |
| then invoices.total_amount | |
| else (invoices.total_amount * coalesce(invoices.exchange_rate, 1)) | |
| else | |
| case when invoices.currency_id = '{{ var('quickbooks__home_currency', '') }}' | |
| then coalesce(invoice_bundles.amount, invoice_lines.amount) | |
| else (coalesce(invoice_bundles.amount, invoice_lines.amount) * coalesce(invoices.exchange_rate, 1)) |
This way the original case statement persists with no repeated logic other than the variable case statement.
There was a problem hiding this comment.
I was realizing this code is a bit repetitive, so I tried and simplified it to separate out the bundle component of this logic and the exchange rate application. Whiteboarded it and I believe it retains the initial logic. Let me know your thoughts.
| case when sales_receipts.currency_id = '{{ var('quickbooks__home_currency', '') }}' | ||
| then case when sales_receipt_lines.discount_account_id is not null | ||
| then sales_receipt_lines.amount * (-1) | ||
| else sales_receipt_lines.amount | ||
| end | ||
| when sales_receipt_lines.discount_account_id is not null | ||
| then sales_receipt_lines.amount * coalesce(-sales_receipts.exchange_rate, -1) | ||
| else sales_receipt_lines.amount * coalesce(sales_receipts.exchange_rate, 1) |
There was a problem hiding this comment.
Same comment as the other multi case statement and leaning more towards only maintaining one version of the original case statement.
There was a problem hiding this comment.
Also simplified the logic here, let me know your thoughts.
models/double_entry_transactions/int_quickbooks__bill_double_entry.sql
Outdated
Show resolved
Hide resolved
fivetran-avinash
left a comment
There was a problem hiding this comment.
@fivetran-joemarkiewicz Ready for re-review.
PR Overview
Package version introduced in this PR:
This PR addresses the following Issue/Feature(s):
Summary of changes:
Submission Checklist
Changelog