fix: v16 compatibility for item_wise_tax_detail in _build_amounts_per_vat_rate

[TRS-WOB]

Summary

• In ERPNext v16, the JSON field item_wise_tax_detail on Sales Taxes and Charges rows no longer exists. Tax detail data is now stored in the Child Table item_wise_tax_details (DocType: "Item Wise Tax Detail") on the invoice document.
• This causes _build_amounts_per_vat_rate() to silently skip all tax rows (line 243: if not item_wise_tax_detail_json: continue), resulting in "Could not derive VAT amounts" errors when creating TSE transactions on v16.
• The fix adds a v16 fallback path: if the JSON field is empty, the function reads from the Child Table instead. The v15 JSON path is preserved as primary, so this is fully backwards-compatible.

Details

Problem: In v16, tax_row.item_wise_tax_detail is always None because ERPNext moved this data to a proper Child Table. The original code requires this JSON field and fails without it.

Solution: Before iterating tax rows, the function now:

1. Reads the item_wise_tax_details Child Table from the invoice (in-memory first, DB fallback)
2. Builds a lookup dict (tax_row_name, item_code) → {rate, amount}
3. For each tax row: tries the v15 JSON path first, falls back to the v16 Child Table lookup

Tested with:

• ERPNext v16 (frappe/erpnext version-16 branch)
• fiskaly cloud TSE (test environment)
• POS Invoice with 19% tax → TSE transaction signed successfully

Test plan

• v16: Create POS Invoice with 19% items → TSE transaction should complete
• v16: Create POS Invoice with mixed 19%/7% items → correct VAT breakdown
• v15 (if available): Verify JSON path still works unchanged

🤖 Generated with Claude Code

[Rocket-Quack]

Thanks for your help and this PR 👍

Have you been able to test all the cases listed in the test plan and verify that the changes work as expected in version-16?

[TRS-WOB]

Thanks! Yes, we have tested the cases from the test plan on v16:

• v16: POS Invoice with 19% items → TSE transaction signed successfully ✅
• v16: POS Invoice with 7% items → TSE transaction signed successfully ✅
• v16: POS Invoice with mixed 19% + 7% items → correct VAT breakdown, TSE transaction signed successfully ✅

Unfortunately we don't have a v15 environment to test backwards compatibility, but the code preserves the v15 JSON path as the primary lookup and only falls back to the v16 Child Table — so it should be safe.

On a separate note: any thoughts on the two RFCs we opened (#75 SUBMIT DE / ELSTER and #76 SAFE flex)? Would love to hear if these are something you'd consider for the project.

[Rocket-Quack]

Tahnks and cool for your help😄👍
I noticed one more thing that I would have liked to clarify explicitly before the merge.

In the current code, the old v15 path is processed via item_wise_tax_detail, i.e., using the JSON-based format.
In v16, however, the information comes from the Child Table Item Wise Tax Detail, which is modeled at the row level and uses fields like item_row and tax_row.

That’s why I’m wondering whether it’s really correct in the v16 fallback to reduce this Child Table data back to item_code.

The specific case I’m referring to here would be a v16 POS Invoice with the same item_code on multiple separate invoice lines, for example:

POS Invoice Item Row A: name = ROW-1, item_code = COFFEE, net_amount = 10.00
POS Invoice Item Row B: name = ROW-2, item_code = COFFEE, net_amount = 20.00
Item Wise Tax Detail 1: tax_row = TAX-1, item_row = ROW-1, rate = 19, amount = 1.90
Item-wise Tax Detail 2: tax_row = TAX-1, item_row = ROW-2, rate = 19, amount = 3.80

Here, ROW-1 and ROW-2 are technically two separate invoice lines, even though both have the same item_code.
Therefore, before merging, I would like to ensure that the new fallback does not unintentionally combine multiple separate lines under a common item_code key when constructing the lookups and totals.

The “happy paths” you tested with 19% / 7% / mixed rates are helpful for this, but I think this specific case—where the same item_code appears multiple times across several item rows—needs to be tested separately, because this is where the new v16 data structure differs most significantly from the old JSON logic.

A second point concerns the error handling for tax rows: In the previous code, tax rows without usable item_wise_tax_detail were effectively skipped before VAT mapping was enforced. In the current version, it appears as though VAT mapping is resolved as soon as account_head is present—that is, even before it is determined whether the tax row provides any usable detail data for the calculation.

Still, great work so far on this important update for full support of v16 👍
What is your take on this? 😄

[TRS-WOB]

Thanks for the thorough review and great catch on the duplicate item_code issue! You were absolutely right — the v16 fallback was reducing the Child Table data back to item_code as key, which would silently merge separate invoice lines.

Fixes Applied

We've updated our v16 compatibility patch to address both points:

1. Duplicate item_code on multiple invoice lines (your main concern):

• The v16 lookup key is now (tax_row_name, item_row_name) instead of (tax_row_name, item_code) — each invoice line is processed individually
• A separate net_amount_by_row_name dict tracks net amounts per row (unique), while net_amount_by_item_code uses += instead of = for the v15 path
• The v16 iteration walks net_amount_by_row_name.items() instead of net_amount_by_item_code

2. Error handling for tax rows without usable data:

• VAT mapping resolution (frappe.db.get_value) is now deferred until we actually have data to process
• v15 path: resolved after JSON parse, before item loop
• v16 path: resolved lazily on first relevant detail row (has_data flag)
• Tax rows without data are silently skipped — consistent with the original behavior (line 243: if not tax_account or not item_wise_tax_detail_json: continue)

Tested with 6 scenarios (all via HTTP with the compat patch active via before_request):

• 19% only ✅
• 7% only ✅
• Mixed 19% + 7% ✅
• Duplicate item_code 19% (2x same item, different amounts) ✅
• Duplicate item_code 7% ✅
• Duplicate 19% + single 7% (3 lines, 2 with same item_code) ✅

Full Module Review

While fixing these, we took the opportunity to review the entire erpnext_tse module from a code quality and security perspective. Here are our findings — some are v16-specific, others apply to the original code as well:

Potential Issues in Original Code

#	Location	Finding	Severity
1	tse_transaction.py:206	net_amount_by_item_code[item_code] = float(...) overwrites instead of accumulating. If the same item_code appears on multiple invoice lines, only the last net amount is kept. Whether this causes wrong amounts on older ERPNext versions depends on the old JSON format — we can't verify since we don't have a v14 system.	Medium
2	dsfinv_k_cash_point_closing.py:362	Same overwrite pattern as above.	Medium
3	tse_transaction.py:398	datetime.fromtimestamp() without timezone — creates naive datetime. Fiskaly returns UTC timestamps, so on servers in non-UTC timezones this could produce incorrect times. Consider datetime.fromtimestamp(ts, tz=timezone.utc).	Low
4	tse_transaction.py:404-407	insert() followed by save() — after insert the doc is already persisted. The separate save() is redundant (no harm, just unnecessary).	Cosmetic
5	fiskaly.py:197	frappe.db.commit() inside _store_auth_error — breaks atomicity when called from a before_submit hook. We see the "Commit during event" warning in our tests.	Low
6	dsfinv_k_cash_point_closing.py:200	Counter increment via db_set() without locking — theoretical race condition on last_cash_point_closing_export_id with concurrent closings. Irrelevant in practice (one closing per register per day).	Negligible
7	Recovery modules	sync_transactions(), sync_clients() etc. are @frappe.whitelist() without explicit role checks — any Desk user could call them. Should probably be restricted to System Manager.	Medium
8	tse_transaction.py:268	Rate filter (19.0, 7.0) is hardcoded (noted as TODO in code). 0% items and future tax rates would be silently skipped.	Low
9	tse_transaction.py:421	tx_revision + 1 — the revision is not read from the start_transaction response (noted as TODO). Works but isn't robust.	Low

Security Assessment

Positive:

• Credentials in encrypted password fields (get_password())
• Admin PIN generated with secrets.choice() (cryptographically secure)
• Token refresh with 60s skew tolerance + proper cleanup on 401
• Audit logging via provider_events child tables
• State machine validation (TSS: INITIALIZED, Client: REGISTERED)
• No raw SQL, only ORM
• Fail-fast on missing mappings — no risk of unsigned/wrong signatures
• Timeouts on all HTTP calls (15s/30s)

No critical security issues found. The module is well-structured and follows Frappe best practices.

Code Quality

Overall the architecture is solid — clean provider abstraction, proper error handling, idempotency checks, deduplication on background jobs. The two _build_amounts_per_vat_* functions share ~90% of their logic and could benefit from a shared utility, but that's a refactoring suggestion, not a bug.

Happy to discuss any of these findings! 😄

[Rocket-Quack]

That's great 👍
Do you want to commit those changes?

[TRS-WOB]

Done — pushed commit c885349 to fix/v16-item-wise-tax-details. Both review points are now in the PR branch:

1. Duplicate item_code on multiple invoice lines

• v16 lookup key: (tax_row_name, item_row_name) instead of (tax_row_name, item_code)
• Separate net_amount_by_row_name dict (unique per row) drives the v16 iteration
• v15 path: net_amount_by_item_code now accumulates with += instead of overwriting on duplicates

2. Lazy VAT mapping resolution

• _resolve_vat_code / _resolve_vat_id are deferred until we actually have relevant detail data
• Tax rows without usable item_wise_tax_detail no longer force a mapping lookup — matches the original pre-v16 behavior of silently skipping such rows

Applied symmetrically in _build_amounts_per_vat_rate and _build_amounts_per_vat_definition. Re-tested on v16 with the 6 scenarios listed in the previous comment (single-rate, mixed, and duplicate-item_code cases) — all pass ✅.

---

Regarding the 9 findings from the full-module review: we haven't fixed those yet, since they're separate observations on the original code and weren't part of this PR's scope. We'd be happy to prepare separate PRs for them if you'd like — but only for the ones you actually consider bugs. Some might be intentional design choices we don't have the context for (e.g. the hardcoded (19.0, 7.0) filter is marked as TODO in the code, the tx_revision + 1 may be a deliberate trade-off, the naive datetime.fromtimestamp() might be intentional if the server is always in UTC). So before we touch anything, could you let us know which of the 9 findings you'd want fixed and which are by design? Then we can split them into focused PRs.

[Rocket-Quack]

Regarding the 9 findings from the full-module review: we haven't fixed those yet, since they're separate observations on the original code and weren't part of this PR's scope. We'd be happy to prepare separate PRs for them if you'd like — but only for the ones you actually consider bugs. Some might be intentional design choices we don't have the context for (e.g. the hardcoded (19.0, 7.0) filter is marked as TODO in the code, the tx_revision + 1 may be a deliberate trade-off, the naive datetime.fromtimestamp() might be intentional if the server is always in UTC). So before we touch anything, could you let us know which of the 9 findings you'd want fixed and which are by design? Then we can split them into focused PRs.

If you have good suggestions for new PRs feel free to submit them then we can discuss them and their scope. 🙂👍

[Rocket-Quack]

Automated tests for communicating with Fiskaly and ensuring functionality for future updates would be helpful if you want to tackle those