Skip to content

feature/exchange-gain-loss#215

Open
fivetran-catfritz wants to merge 6 commits into
mainfrom
feature/exchange-gain-loss
Open

feature/exchange-gain-loss#215
fivetran-catfritz wants to merge 6 commits into
mainfrom
feature/exchange-gain-loss

Conversation

@fivetran-catfritz

@fivetran-catfritz fivetran-catfritz commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

PR Overview

Package version introduced in this PR:

  • v1.9.0-a1

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

  • GA-1026684

Summary of changes:

  • dds realized exchange gain/loss entries to int_quickbooks__bill_payment_double_entry and int_quickbooks__payment_double_entry to capture the currency impact when a foreign currency transaction is settled at a different exchange rate than when it was originally recorded. Requires an ExchangeGainOrLoss account in QuickBooks to generate entries.

Submission Checklist

  • 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-catfritz fivetran-catfritz marked this pull request as draft June 26, 2026 17:45
@fivetran-catfritz fivetran-catfritz self-assigned this Jun 30, 2026
@fivetran-catfritz fivetran-catfritz marked this pull request as ready for review June 30, 2026 19:08

@fivetran-savage fivetran-savage left a comment

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.

Just one question about the changelog but nothing to hold off approval for a pre-release.

Comment thread CHANGELOG.md Outdated
| `quickbooks__general_ledger`<br>`quickbooks__general_ledger_by_period` | New records | — | Exchange gain/loss entries | |

## Feature Update
- Adds realized exchange gain/loss entries to `int_quickbooks__bill_payment_double_entry` and `int_quickbooks__payment_double_entry` to capture the currency impact when a foreign currency transaction is settled at a different exchange rate than when it was originally recorded. Requires an `ExchangeGainOrLoss` account in QuickBooks to generate entries.

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.

In the validation, it appears to be both an account_name value and an account_sub_type value. Do we need to call that out explicitly?

@fivetran-catfritz fivetran-catfritz Jun 30, 2026

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.

Good question. It's the account_sub_type that is a pre-set list coming from Quickbook, and the name should follow, so yes I should clarify what field we're talking about here.

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.

Updated!

Comment thread CHANGELOG.md Outdated
@fivetran-catfritz fivetran-catfritz added the pre-release Triggers the auto-releaser workflow. label Jun 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pre-release Triggers the auto-releaser workflow.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants