fix(billing): EN16931 XML remaining gaps — payment means, tax categories, document-level charges (#158)#160
Conversation
|
@mostlyvirtual — this PR is ready for review. |
There was a problem hiding this comment.
Pull request overview
Improves Romanian e-Factura (EN16931/CIUS-RO) XML generation by adding payment means mapping, structured payment terms, tax exemption reason elements, and document-level allowance/charge support; also includes a small hardening in SES webhook signature verification and test updates.
Changes:
- Extend e-Factura XML builder with payment means codes, tax exemption reason output, document-level allowances/charges, and structured payment terms notes.
- Update portal login lockout tests to execute through the real HMAC middleware test stack.
- Harden SES SNS signature verification by rejecting non-RSA public keys; adjust an orders edit test expectation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| services/platform/apps/billing/efactura/xml_builder.py | Adds EN16931-related XML fields (payment means, tax exemption reasons, allowances/charges, payment terms) and adjusts monetary totals. |
| services/platform/apps/notifications/webhooks.py | Adds RSA public-key type check before verifying AWS SNS signatures for SES webhooks. |
| services/platform/tests/api/test_api_users_security.py | Runs portal login lockout tests through HMAC middleware with dedicated test helpers/settings. |
| services/platform/tests/orders/test_orders_edit.py | Updates draft editable-fields test to assert against Order._SAFE_DRAFT_FIELDS. |
Comments suppressed due to low confidence (1)
services/platform/apps/billing/efactura/xml_builder.py:735
- Document-level allowances/charges are subtracted/added into
TaxExclusiveAmount, butTaxTotal/TaxableAmount,TaxTotal/TaxAmount,TaxInclusiveAmount, andPayableAmountstill come from the unadjusted invoice fields. Ifmeta['allowances']/meta['charges']are populated, the totals in the XML will become internally inconsistent and may fail CIUS-RO/EN16931 validation. Update the calculation flow so document-level allowance/charge amounts (and their taxes) are reflected consistently acrossTaxTotalandLegalMonetaryTotal, or ensure these meta amounts are already included ininvoice.subtotal_cents/tax_total_cents/total_centsbefore building the XML.
# Document-level allowances/charges (BT-107/BT-108)
allowance_total, charge_total = self._get_document_level_totals()
if allowance_total > 0:
allow_elem = self._add_cbc(monetary_total, "AllowanceTotalAmount", self._format_amount(allowance_total))
allow_elem.set("currencyID", currency)
if charge_total > 0:
charge_elem = self._add_cbc(monetary_total, "ChargeTotalAmount", self._format_amount(charge_total))
charge_elem.set("currencyID", currency)
# Tax Exclusive Amount (line extension - allowances + charges)
tax_exclusive = subtotal - allowance_total + charge_total
tax_excl = self._add_cbc(monetary_total, "TaxExclusiveAmount", self._format_amount(tax_exclusive))
tax_excl.set("currencyID", currency)
# Tax Inclusive Amount (total with tax)
total = Decimal(self.invoice.total_cents or 0) / 100
tax_incl = self._add_cbc(monetary_total, "TaxInclusiveAmount", self._format_amount(total))
tax_incl.set("currencyID", currency)
# Payable Amount (amount to be paid)
payable = self._add_cbc(monetary_total, "PayableAmount", self._format_amount(total))
payable.set("currencyID", currency)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mostlyvirtual
left a comment
There was a problem hiding this comment.
Re-review: REWORK — 2 blocking EN16931 compliance issues + merge conflict
Must fix before merge (blocking)
1. `TaxExemptionReasonCode` emits category code instead of VATEX code (BT-121)
`xml_builder.py:639,988` — Emits `"AE"`, `"E"`, `"K"` etc. into `TaxExemptionReasonCode`, but BT-121 requires VATEX codes (`"VATEX-EU-AE"`, `"VATEX-EU-IC"`, etc.). ANAF Schematron will reject every non-standard-rate invoice. Either map to proper VATEX codes or omit BT-121 (it's optional when BT-120 free-text reason is present, which it is).
2. `AllowanceChargeReasonCode "FC"` invalid for document charges
`xml_builder.py:673` — `"FC"` is not in UNCL4465 code list. Use `"ZZZ"` (mutually defined) or accept from meta: `charge.get("reason_code", "ZZZ")`. BIS Billing 3.0 rule UBL-SR-44 will reject this.
3. Merge conflict from 3 unrelated files
`webhooks.py`, `test_api_users_security.py`, `test_orders_edit.py` are already on master. Reset these to master versions before merging: `git checkout master -- `.
Should fix
4. `_get_tax_category` classifies non-EU customers as reverse charge
`xml_builder.py:265` — `customer.country_code != "RO" and customer.tax_id` returns `AE` for US/GB customers with tax IDs. Need EU country code check.
5. Credit note `unitCode` hardcoded to `"C62"`
`xml_builder.py:1026` — Invoice builder uses `_get_unit_code(line)` but credit note builder always emits `"C62"` (piece). EN16931 BT-130 requires credited quantity unit to match invoiced unit.
6. Unvalidated JSON meta in allowances/charges
`xml_builder.py:645-683` — `meta["allowances"]` / `meta["charges"]` read without schema validation. Invalid `amount_cents` crashes with `InvalidOperation`, missing fields silently produce zero amounts. Validate each entry and skip bad ones with logging.
7. Totals computed independently from emitted elements
`_add_document_allowances_charges` and `_get_document_level_totals` iterate meta separately — if one skips bad entries and the other doesn't, EN16931 BR-CO-11 (totals must match) fails. Extract a shared validated list.
8. `_get_tax_category` uses unordered `.first()`
`xml_builder.py:254` — Multi-line invoices with mixed tax categories get a non-deterministic document-level category. Add `.order_by("sort_order", "id")` or validate all lines share the same category.
What's good
- Safe XML construction throughout — lxml auto-escaping, no f-string concatenation
- Historical tax rate preservation from frozen `line.tax_rate`
- Payment means codes correct per UNCL4461 (30/48/68/49/10)
- `_validate_invoice()` is thorough with collected error list
- None-safe tax total handling preserves zero-rated invoices correctly
- No new tests needed for merge (new code paths not yet used in production), but should be added before the feature goes live
…ax category logic, safe Decimal - Use VATEX codelist (VATEX-EU-AE, etc.) for TaxExemptionReasonCode instead of misusing UNCL5305 category codes - Only prefer line-level tax_category_code when explicitly non-default (not "S"), allowing customer/amount-based detection for AE/O/K/E categories - Use Decimal(str(...)) for JSON-sourced amounts to avoid float rounding issues Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ax category logic, safe Decimal - Use VATEX codelist (VATEX-EU-AE, etc.) for TaxExemptionReasonCode instead of misusing UNCL5305 category codes - Only prefer line-level tax_category_code when explicitly non-default (not "S"), allowing customer/amount-based detection for AE/O/K/E categories - Use Decimal(str(...)) for JSON-sourced amounts to avoid float rounding issues Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
326fd66 to
94223ec
Compare
Review feedback addressed (326fd66, rebased as 94223ec)
|
…imal - Payment means code mapping based on Stripe/bank/card/PayPal methods - Tax category detection from customer location and tax amounts - Document-level allowances/charges (BG-20/BG-21) - Use VATEX codelist (VATEX-EU-AE, etc.) for TaxExemptionReasonCode - Only prefer line-level tax_category_code when non-default (not "S") - Use Decimal(str(...)) for JSON-sourced amounts to avoid rounding Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ciprian Radulescu <craps2003@gmail.com>
94223ec to
5fc9c9e
Compare
Summary
Closes the 4 remaining EN16931 compliance gaps in the e-Factura XML builder:
Payment.payment_methodinstead of hardcoding30. Maps: stripe→48, bank→30, paypal→68, cash→10invoice.meta['allowances']andinvoice.meta['charges']. IncludeAllowanceTotalAmount/ChargeTotalAmountinLegalMonetaryTotal, adjustTaxExclusiveAmountFiles changed (1)
services/platform/apps/billing/efactura/xml_builder.py— +164/-13Test plan
48when invoice has a Stripe paymentTaxExemptionReasonin XMLmeta['allowances']is populatedCloses #158
🤖 Generated with Claude Code