Skip to content
This repository was archived by the owner on Jan 20, 2026. It is now read-only.

Bugfix/add credit card txn transfer exchange rates#72

Merged
fivetran-avinash merged 10 commits into
mainfrom
bugfix/add-credit-card-txn-transfer-exchange-rates
Sep 11, 2025
Merged

Bugfix/add credit card txn transfer exchange rates#72
fivetran-avinash merged 10 commits into
mainfrom
bugfix/add-credit-card-txn-transfer-exchange-rates

Conversation

@fivetran-avinash
Copy link
Copy Markdown
Contributor

@fivetran-avinash fivetran-avinash commented Sep 10, 2025

PR Overview

Package version introduced in this PR:

  • v0.14.1

This PR addresses the following Issue/Feature(s):

Summary of changes:

  • Added credit card + transfer exchange rates to converted amount calculations

Submission Checklist

  • [NA] Alignment meeting with the reviewer (if needed)
    • Timeline and validation requirements discussed
  • Provide validation details:
    • Validation Steps: Check for unintentional effects (e.g., add/run consistency & integrity tests)
    • Testing Instructions: Confirm the change addresses the issue(s)
    • Focus Areas: Complex logic or queries that need extra attention
  • Merge any relevant open PRs into this PR

Changelog

  • Draft changelog for PR
  • Final changelog for release review

@fivetran-avinash fivetran-avinash self-assigned this Sep 10, 2025
@fivetran-avinash fivetran-avinash added the docs:ready Triggers the docs generator workflow. label Sep 10, 2025
Copy link
Copy Markdown
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.

A few small change requests before approval.

Comment thread CHANGELOG.md Outdated
Comment on lines +12 to +15
## Bug Fixes
- Added currency and exchange rate fields for transfers and credit card payment transactions to complete support for Quickbooks multicurrency.
- This will ensure accurate calculations in the downstream transform `quickbooks__general_ledger` model for multicurrency accounts leveraging these transactions. [See the v0.21.1 release of `dbt_quickbooks` for more details](https://github.com/fivetran/dbt_quickbooks/releases/tag/v0.21.1).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would say we don't need this section. I would instead say let's consolidate this information in the Notes section of the schema updates table as right now they are a bit duplicative.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Consolidated.

Comment thread models/stg_quickbooks.yml Outdated
Comment on lines +650 to +675
- name: stg_quickbooks__invoice_tax_line
description: "{{ doc('invoice_tax_line_table') }}"
columns:
- name: invoice_id
description: "{{ doc('id') }} invoice associated with this tax line."
tests:
- not_null
- name: index
description: "{{ doc('tax_index') }} invoice."
tests:
- not_null
- name: tax_rate_id
description: "{{ doc('tax_rate_id') }}"
- name: amount
description: "{{ doc('tax_amount') }}"
- name: tax_percent
description: "{{ doc('tax_percent') }}"
- name: net_amount_taxable
description: "{{ doc('net_amount_taxable') }} invoice line amount."
- name: override_delta_amount
description: "{{ doc('override_delta_amount') }}"
- name: percent_based
description: "{{ doc('percent_based') }}"
- name: tax_inclusive_amount
description: "{{ doc('tax_inclusive_amount') }}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I assume this was a left over from the tax lines branch. We shouldn't include these updates in this release. Please remove the tax table docs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ack. Removed.

Copy link
Copy Markdown
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 Ready for re-review!

@fivetran-avinash fivetran-avinash added docs:ready Triggers the docs generator workflow. and removed docs:ready Triggers the docs generator workflow. labels Sep 11, 2025
Copy link
Copy Markdown
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

@fivetran-avinash fivetran-avinash merged commit 31c370c into main Sep 11, 2025
8 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

docs:ready Triggers the docs generator workflow.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants