[jinscoe]

Resolves #224

Please provide your name and company

Jack, Nexus

Describe what this PR changes and how it addresses the linked issue or feature request.

Thanks for getting back to me so quickly on #224! Here's the fix. It's three small commits, so it should be easy to pick apart if you only want some of it.

The join fix. Bundle components now join to their own parent line (invoice_line_index = invoice_lines.index) rather than to every line on the invoice. That's the actual bug: each component posts once again, and the regular lines on the same invoice keep their own amounts.

The component index. Once the join was fixed, the general ledger's unique_id test started failing on a realistic bundled invoice. Component indexes start over at 0 under each bundle line, so a component can end up with the same index as a regular line (or as a component in a second bundle on the same invoice). I gave component rows 20000 + parent line index * 1000 + component index, similar to how tax lines already use index + 10000.

It looks like #175 ran into this as well. Its seed update (6b6ba9f) renumbered the components on invoice 40113 so they ran 0 to 8 instead of repeating, which hid the collision. The new 60002 seed puts the real shape back.

One heads-up: this gives every bundle component row a new transaction_index and unique_id, even on invoices that were already correct. If anyone snapshots the general ledger or builds incrementally on unique_id, those rows will show up as removed and re-added once.

Seeds and a test (feel free to drop these if you'd rather write your own). I added two made-up invoices: 60001 is a bundle plus a regular line, like in the issue, and 60002 sells the same bundle on two lines. There's also an integrity test that checks each account on a bundled invoice gets credited what the components and item lines add up to. It catches this bug, and it also catches the reverse, where components stop matching and the bundle line posts with no account. I also put a class on one component and added a zero-amount tax line to 60002, so the older component class behavior and the tax line offset both have coverage.

There's one related thing I noticed but left alone to keep this focused. If a bundle's items map to more than one Sales of Product Income account, the bundle_income_accounts join still duplicates the components. Only joining it when no component matched fixed it when I tried it. Happy to add that here or open a separate issue, whichever you'd prefer.

I didn't touch the versions, CHANGELOG or docs since it looks like you all handle those. Here's a rough CHANGELOG entry in case it saves you a minute:

• Bug fix: invoice bundle components now match their own parent line instead of every line on the invoice, so bundled invoices no longer duplicate component amounts or lose their other lines in int_quickbooks__invoice_double_entry and downstream models (#224).
• Data change: bundle component rows get a new index / transaction_index (20000 + parent line index * 1000 + component index), which changes their unique_id in quickbooks__general_ledger.

How did you validate the changes introduced within this PR, and what warehouse did you use for testing?

DuckDB, using only the integration test seeds (no real data). I ran v1.10.0 and this branch on the same seeds, in both the Default and Tax Lines scenarios, and checked the new test against the other scenarios in test_scenarios.yml.

Credits per account on the new invoices (the same in both scenarios):

Invoice	Account	Expected	v1.10.0	This branch
60001	999	150.00	200.00	150.00
60001	1999	200.00	400.00	200.00
60002	999	300.00	600.00	300.00
60002	1999	600.00	1,200.00	600.00

Tests:

	v1.10.0	This branch
dbt test, Default	44 of 45 (unique_id fails)	45 of 45
dbt test, Tax Lines	64 of 65 (unique_id fails)	65 of 65
New integrity test	fails	passes

With only the join fix, the amounts come out right but unique_id still fails, which is why the index commit is there. I also broke the fix on purpose a few ways to make sure the tests would notice, and they did.

Nothing else moved. Only the invoice double entry model and the general ledger changed row counts, and every changed ledger row is on a bundled invoice (40113, 40116, 60001, 60002). The report models kept the same row counts.

A few things about the existing seeds that might be useful:

• Four of 40113's components point at a line 3 that doesn't exist, so they don't post anymore.
• 40116's only component points at its subtotal line, so its 500.00 item line now posts instead of a 0.00 component.
• balance_sheet_amounts_match fails for me on DuckDB even on a clean v1.10.0.

Let me know if you'd like anything changed!

[fivetran-savage]

@jinscoe apologies for the delay! Thank you for opening this PR. It looks great. Regarding this additional issue you noticed: "If a bundle's items map to more than one Sales of Product Income account, the bundle_income_accounts join still duplicates the components." We will want to address that in this release as well, so if you want to add it to this PR, that would be great, otherwise, someone from the team will add it onto the PR once this is brought into a release branch. We are at the end of a sprint, so this will likely be addressed in the coming weeks. Thank you so much for your contribution!

[jinscoe]

Thanks @fivetran-savage! Made those changes.

I just pushed two more commits:

• The bundle's income accounts are now just a last resort, used only when nothing else (the component, the line, or its item) gives the line an account. So components stop doubling up when a bundle's items map to more than one Sales of Product Income account.
• The integrity test now checks that each bundled invoice's credits add up and that none are missing an account, instead of guessing which account each one should land on. That way the model's own fallbacks don't trip it.

I also added a couple of seed rows for this: bundle 149 now maps to two Sales of Product Income accounts, and a small new invoice (60003) where the fallback actually kicks in, so both paths get tested. Everything passes in Default and Tax Lines, and 60001 and 60002 still come out to the same numbers as the table above.

One thing I left alone: if nothing resolves the account and the bundle maps to several income accounts, it still posts once per account, same as today. There's no one right account to pick there, so I didn't want to make a pure "guess"

[fivetran-savage]

@jinscoe Thanks for adding the additional commits! Just a heads up that I'm going on PTO tomorrow through Monday, so I won't have a chance to review until Tuesday, but at that point, I'll pull your branch into a release branch with all of your commits, review and make edits as needed from there. I'll comment here once there's a PR for you to watch. Thanks so much for being a contributing member of the dbt package community. We couldn't do this without you!

[fivetran-savage]

@jinscoe apologies for the delay! I will pull your PR into a release branch and take it from here. When I have a release PR for you to follow, I'll post it here. Thanks again for raising the issue and solution!

[jinscoe]

@jinscoe apologies for the delay! I will pull your PR into a release branch and take it from here. When I have a release PR for you to follow, I'll post it here. Thanks again for raising the issue and solution!

Terrific. I hope it helps!
Thanks for the follow ups.

I look forward to tracking this through release!

[fivetran-savage]

PR is #227. Planning to release today. FYI, I removed the additional integration test you added, not because there was anything wrong with it but because we are in the process of rethinking the way that we integrate testing into our packages as a general rule, so we are not adding new tests at the moment. Thank you for your contributions!

[jinscoe]

Sounds great. Thank you!