Repository navigation
Feature: Exchange rate addition - #223
Conversation
Resolves conflicts in dbt_project.yml/integration_tests/dbt_project.yml (kept 1.10.0), account_data.csv (combined both branches' new seed accounts), and CHANGELOG.md (kept newest-first order: 1.10.0, 1.9.2, 1.9.1).
Merge branch 'feature/exchange-rate' of https://github.com/fivetran/dbt_quickbooks into feature/exchange-rate
fivetran-jamie
left a comment
There was a problem hiding this comment.
Looks good, just some doc-related comments
…bt_quickbooks into feature/exchange-rate
fivetran-avinash
left a comment
There was a problem hiding this comment.
@fivetran-jamie Ready for re-review!
| type: string | ||
| value: "" | ||
| description: "The primary reporting currency, use the same 3-letter ISO currency code (e.g., USD, EUR, GBP, CAD) that appears in your QuickBooks `currency_id` field. Relevant for organizations with multiple currencies, this field defaults to an empty string; when not set, the package applies exchange rate conversions to all transactions, regardless of currency." | ||
| description: "The primary reporting currency, use the same 3-letter ISO currency code (e.g., USD, EUR, GBP, CAD) that appears in your QuickBooks `currency_id` field. Relevant for organizations with multiple currencies, this field defaults to an empty string; when not set, the package applies exchange rate conversions to all transactions, regardless of currency. Required if 'Enable Report-Date Balance Sheet FX Conversion' is enabled." |
There was a problem hiding this comment.
| description: "The primary reporting currency, use the same 3-letter ISO currency code (e.g., USD, EUR, GBP, CAD) that appears in your QuickBooks `currency_id` field. Relevant for organizations with multiple currencies, this field defaults to an empty string; when not set, the package applies exchange rate conversions to all transactions, regardless of currency. Required if 'Enable Report-Date Balance Sheet FX Conversion' is enabled." | |
| description: "The primary reporting currency, use the same 3-letter ISO currency code (e.g., USD, EUR, GBP, CAD) that appears in your QuickBooks `currency_id` field. Relevant for organizations with multiple currencies, this field defaults to an empty string; when not set, the package applies exchange rate conversions to all transactions, regardless of currency. Required if 'Enable Report-Date Balance Sheet FX Conversion' is true." |
| {"name": "rate", "datatype": dbt.type_float()}, | ||
| {"name": "created_at", "datatype": dbt.type_timestamp()}, | ||
| {"name": "updated_at", "datatype": dbt.type_timestamp()}, | ||
| {"name": "_fivetran_deleted", "datatype": "boolean"} |
There was a problem hiding this comment.
| {"name": "_fivetran_deleted", "datatype": "boolean"} | |
| {"name": "_fivetran_deleted", "datatype": dbt.type_boolean()} |
| coalesce(period_ending_converted_balance_starter, | ||
| first_value(period_ending_converted_balance_starter) over (partition by gl_partition {{ fivetran_utils.partition_by_source_relation(package_name='quickbooks') }} | ||
| order by period_last_day rows unbounded preceding)) as period_ending_converted_balance | ||
| period_beginning_converted_balance_final as period_beginning_converted_balance, |
There was a problem hiding this comment.
Claude suggests that this is using the legacy cumulative sum and needs to be updated to align with period_ending_converted_balance. Summary below:
Accounting identity breaks when report-date FX is enabled
In the final CTE, when using_report_date_fx_conversion: true:
period_ending_converted_balance=period_ending_balance_final * report_date_rate(report-date rate)period_beginning_converted_balance=period_beginning_converted_balance_final(legacy cumulative transaction-date sum)
These use different methodologies in the same row. period_beginning[N+1] ≠ period_ending[N]. Any user computing net converted movement from these two columns gets a number that doesn't reconcile.
Fix: add a CTE that materializes period_ending_converted_balance, then derive period_beginning_converted_balance as lag() of it:
gl_report_date_converted as (
select
*,
case
when report_date_rate is not null
then period_ending_balance_final * report_date_rate
else period_ending_converted_balance_legacy
end as period_ending_converted_balance_final
from gl_report_date_rate
),
# Then in final:
{% if report_date_fx_enabled %}
lag(period_ending_converted_balance_final) over (
partition by account_id, source_relation, coalesce(class_id, '0')
order by period_last_day
) as period_beginning_converted_balance,
period_ending_converted_balance_final as period_ending_converted_balance
{% else %}
period_beginning_converted_balance_final as period_beginning_converted_balance,
period_ending_converted_balance_legacy as period_ending_converted_balance
{% endif %}First-period lag() returning null is correct — no prior period exists.
There was a problem hiding this comment.
Took the above suggestion and applied some changes to account for this, let me know your thoughts!
| @@ -1,3 +1,17 @@ | |||
| # dbt_quickbooks v1.10.0 (--full-refresh required when enabling `using_report_date_fx_conversion`) | |||
There was a problem hiding this comment.
I would move the full refresh note from the title
fivetran-avinash
left a comment
There was a problem hiding this comment.
@fivetran-jamie Ready for another look!
fivetran-jamie
left a comment
There was a problem hiding this comment.
LGTM! Two minor suggestions
Co-authored-by: Jamie Rodriguez <65564846+fivetran-jamie@users.noreply.github.com>
| @@ -1,3 +1,17 @@ | |||
| # dbt_quickbooks v1.10.0 | |||
|
|
||
| If a matching exchange rate isn't available for an account's currency as of the period's last day, the package falls back to the legacy transaction-date summation method for that account and period. | ||
|
|
||
| > IMPORTANT: If you enable `using_report_date_fx_conversion`, you need to run a `--full-refresh` for the change to take effect on historical data. |
There was a problem hiding this comment.
If users need to run a full refresh to get the conversion on historical data, we should call this out in the changelog.
There was a problem hiding this comment.
Added it to the schema change section
| description: Timestamp of the date which the exchange rate record was last updated. | ||
| - name: source_relation | ||
| description: "{{ doc('source_relation') }}" | ||
|
|
There was a problem hiding this comment.
Missing _fivetran_deleted
fivetran-avinash
left a comment
There was a problem hiding this comment.
@fivetran-savage 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