Skip to content

fix: multi-tax detection and net_amount calculation#256

Open
0xD0M1M0 wants to merge 10 commits into
alyf-de:developfrom
0xD0M1M0:net_amount_calc
Open

fix: multi-tax detection and net_amount calculation#256
0xD0M1M0 wants to merge 10 commits into
alyf-de:developfrom
0xD0M1M0:net_amount_calc

Conversation

@0xD0M1M0

Copy link
Copy Markdown
Collaborator

PR Description

This PR addresses two connected issues when generating e-invoices for Sales Invoices with multiple tax rows or configurations where a row in Sales Taxes and Charges is present but not applicable.

Example: Sales Invoice ACC-SINV-2025-00035

# Item Net Amount Item Tax Template
1 PM0003 105.00 19% - CP
2 PM0001 103.00 7% - CP
3 PM0001 114.00 19% - CP
# Account Rate (row) Tax Amount
1 3806 - Umsatzsteuer 19% 0 41.61
2 3801 - Umsatzsteuer 7% 0 7.21

Expected e-invoice settlement lines:

  • 19% basis = 105 + 114 = 219.00, tax = 41.61
  • 7% basis = 103 = 103.00, tax = 7.21

1) Basis amount for multi-tax "On Net Total" rows

In multi-tax environments, ERPNext v15 does not provide a net_amount on tax rows for use as the taxable basis. The existing logic relied on fallback options (hasattr(tax, "net_amount")) that could match an uninitialized or zero-valued field, resulting in basis_amount = 0.

Without this PR:

  • If hasattr(tax, "net_amount") evaluated to True (for example due to a Custom Field or attribute initialization on the site), the basis could become 0 or any other value for both tax lines, producing an incorrect e-invoice tax basis.

With this PR:

  • The basis is first derived from tax_amount / rate * 100 (here: 41.61 / 19 * 100 = 219.0 and 7.21 / 7 * 100 = 103.0).
  • If the derived value is exact at 2 decimals, it is used directly.
  • If not exact (rounding would be required), it falls back to summing line-item net_amount values whose resolved VAT rate matches the tax row's rate.

This keeps the fast path for exact results and guarantees a correct basis when rounding would otherwise distort the taxable amount.

2) Highest non-zero rate from Item Tax Template

When multiple accounts are listed in Sales Taxes and Charges and an Item Tax Template contains a matching account with 0% tax, the old logic returned the first match, which could be 0%.

Without this PR:

  • A template could contain:
    • Row A: tax_type = "3806 - Umsatzsteuer 19%", tax_rate = 0
    • Row B: tax_type = "3801 - Umsatzsteuer 7%", tax_rate = 7
  • Because both accounts are present in the invoice tax table, the old first-match behavior could return 0% for a line that should be 7%.

With this PR:

  • Matching entries with tax_rate = 0 are excluded in the applicable-account matching path.
  • The highest non-zero matching tax_rate is returned.
  • If income_account is available, direct account matching is still preferred.

This avoids false 0% assignments and keeps mixed-tax invoices consistent.

@greptile-apps

greptile-apps Bot commented Apr 30, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes two bugs in e-invoice generation for Sales Invoices with multiple tax rows: incorrect basis-amount calculation for multi-rate "On Net Total" rows, and false 0% VAT rate assignment when an Item Tax Template contains a zero-rate entry for a matched account.

  • Basis amount: replaces the unreliable hasattr(tax, "net_amount") fast path with _sum_item_net_matching_on_net_total_rate, which sums net_amount of items whose resolved VAT rate matches the current tax row, falling back to the calculated tax_amount / rate * 100 approach.
  • Rate resolution: get_item_rate now skips zero-rate template rows on ERPNext v15 and rows flagged not_applicable on v16+, preventing false 0% assignments; also upgrades from frappe.get_doc to frappe.get_cached_doc to reduce per-item DB hits.
  • Row skipping: adds an early continue for On Net Total rows where tax_amount == 0 and tax_rate != 0, avoiding validation errors for inapplicable tax rows.

Confidence Score: 4/5

Safe to merge for the common case the PR targets, but invoices that mix items with and without Item Tax Templates under multiple tax rates can get a wrong BasisAmount without any error signal

The _sum_item_net_matching_on_net_total_rate method silently returns a partial sum when some items lack item_tax_template in a multi-tax invoice, and because the fallback chain is or-chained, a non-zero partial sum prevents the reliable calculated fallback from firing. The emitted BasisAmount would be lower than the actual taxable amount for that rate, potentially causing downstream schema validation failures.

eu_einvoice/european_e_invoice/custom/sales_invoice.py — specifically the or-chain in _add_taxes_and_charges (lines 574–581) and the _sum_item_net_matching_on_net_total_rate method that feeds it

Important Files Changed

Filename Overview
eu_einvoice/european_e_invoice/custom/sales_invoice.py Refactors multi-tax basis-amount calculation and VAT rate resolution; introduces _sum_item_net_matching_on_net_total_rate which can emit a partial sum when items lack item_tax_template, silently suppressing the calculated fallback

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[_add_taxes_and_charges loop] --> B{charge_type == On Net Total?}
    B -- No --> Z[Other charge types]
    B -- Yes --> C{tax_amount == 0 AND tax_rate != 0?}
    C -- Yes --> SKIP[continue / skip row]
    C -- No --> D[len taxes == 1?]
    D -- Yes --> E[basis = invoice.net_total]
    D -- No --> F[_sum_item_net_matching_on_net_total_rate]
    F --> G{sum > 0?}
    G -- Yes --> H[basis = item net sum]
    G -- No --> I{hasattr net_amount?}
    I -- Yes --> J[basis = tax.net_amount]
    I -- No --> K{hasattr custom_net_amount?}
    K -- Yes --> L[basis = tax.custom_net_amount]
    K -- No --> M{tax_amount and rate?}
    M -- Yes --> N[basis = tax_amount / rate * 100]
    M -- No --> O[basis = 0]

    subgraph get_item_rate
        P[item_tax_template provided?] -- Yes --> Q[frappe.get_cached_doc template]
        Q --> R{v16 not_applicable field?}
        R -- Yes --> S[match: tax_type in accounts AND NOT not_applicable]
        R -- No --> T[match: tax_type in accounts AND tax_rate != 0]
        S --> U[return first matching rate]
        T --> U
        P -- No --> V{exactly 1 On Net Total row?}
        V -- Yes --> W[return that row rate]
        V -- No --> X[return None]
    end
Loading

Fix All in Cursor

Reviews (2): Last reviewed commit: "Merge branch 'develop' into net_amount_c..." | Re-trigger Greptile

Comment thread eu_einvoice/european_e_invoice/custom/sales_invoice.py Outdated
Comment thread eu_einvoice/european_e_invoice/custom/sales_invoice.py Outdated
Comment thread eu_einvoice/european_e_invoice/custom/sales_invoice.py Outdated
0xD0M1M0 and others added 6 commits April 30, 2026 10:32
In V16+ not_applicable is set, so it can be used as the "stronger" link, that this row
should be skipped. In V15 and below, if a tax_rate is 0, it is skipped.
* feat(eu_einvoice): configurable base name for auto-attached XRechnung XML

Add optional pattern on E Invoice Settings (auto_name_format_for_xml_file) for the
file base name when auto-attaching XML on Sales Invoice submit. Empty pattern keeps
using the document name; otherwise use naming-series-style segments (dot-separated,
parse_naming_series, {field} placeholders, date tokens), strip leading # per
segment so counter-only parts do not consume Series, sanitize with get_safe_file_name,
and fall back to the document name on error.

Colocate get_xml_attachment_file_base_name with Sales Invoice custom logic and cover
naming edge cases in test_sales_invoice.py.

Refresh E Invoice Settings controller and de / template catalogs for the new strings.

* refactor(eu_einvoice): enhance download_xrechnung to use configurable base name

Update the download_xrechnung function to retrieve the Sales Invoice document
and check permissions before generating the XML file.
The filename now utilizes a configurable base name from E Invoice Settings,
improving flexibility for file naming.
This change ensures that the generated XML file adheres to the specified naming format.

* chore(eu_einvoice): update E Invoice Settings and localization files

Rearrange fields in e_invoice_settings.json for better organization, adding a label for
the XML Settings section. Update localization files (de.po and main.pot) to reflect
changes in field names and add new translations for XML Settings.
Adjust POT creation dates for consistency.

* refactor(eu_einvoice): centralize sales invoice xml stem naming

Resolve the downloadable XML filename stem in get_xml_attachment_file_base_name:
read *Auto name format for XML file* from **E Invoice Settings** when the pattern
argument is omitted, accept an optional keyword-only pattern override, and build
the stem with parse_naming_series plus a no-op number generator so naming has no
series counter DB side effects. Sanitize with get_safe_file_name and fall back to
doc.name when the pattern is empty or fails.

Call sites use the helper without duplicating settings reads. Tests pass pattern=
for empty or whitespace patterns, and assert the settings-backed path by patching
frappe.get_single_value only for **E Invoice Settings** / auto_name_format_for_xml_file.

---------

Co-authored-by: Daniel Rose <26166128+dafrose@users.noreply.github.com>
* chore: ignore translatable strings from frappe and erpnext

* chore: update translations
@barredterra

Copy link
Copy Markdown
Member

@greptileai

Comment thread eu_einvoice/european_e_invoice/custom/sales_invoice.py
@dafrose

dafrose commented Jun 5, 2026

Copy link
Copy Markdown
Member

Commitlint complains about a commit message body line being too long. This can be fixed with the following command:

FILTER_BRANCH_SQUELCH_WARNING=1 git filter-branch -f --msg-filter '
if test "$GIT_COMMIT" = "3171c881365067f1561dd9e7dfdb818913fc67ab"; then
  cat <<EOF
chore: return to standard

In V16+ not_applicable is set, so it can be used as the "stronger" link, that this row
should be skipped. In V15 and below, if a tax_rate is 0, it is skipped.
EOF
else
  cat
fi
' 3171c88^..HEAD

followed by force-push:

git push --force-with-lease origin net_amount_calc

@barredterra: Does this need to be fixed or can the pull request be merged regardless?

@barredterra

Copy link
Copy Markdown
Member

Does this need to be fixed or can the pull request be merged regardless?

We usually squash+merge, so it should be fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants