fix: item wise tax details and net amounts

[blaggacao]

Developer SLA: my promise is to keep a close eye on this for two weeks after this merge and resolve any issues within 3 working days from the time of duly documented identification and notification

Companions / Fixes: (backporting dependencies)

• fix(migration): to new item_wise_tax_detail #44171
• fix: partial fix for #44291 #44229
• fix: net_amount calculation #44231
  • feat: get_meta can derive meta from dict frappe#28525
  • fix: meta signature frappe#28526

Net Amount Field Addition for Tax Doctype

This pull request introduces a new "Net Amount" field to various tax-related doctypes in ERPNext, enhancing the granularity of tax calculations and reporting.

Changes Overview

1. Added "Net Amount" and "Base Net Amount" fields to:

   • Advance Taxes and Charges
   • Purchase Taxes and Charges
   • Sales Taxes and Charges
   • PR to HRMS for Expense Taxes and Charges → feat: add net amount to taxes and charges (follow erpnext) hrms#2230

2. Updated the calculate_taxes_and_totals class to incorporate the new net amount fields in tax calculations.

3. Modified the get_current_tax_amount method to get_current_tax_and_net_amount, now returning both net and tax amounts.

4. Adjusted the item-wise tax calculation to include net amount information.

Key Updates

• New Fields: "Net Amount" and "Base Net Amount" added to tax doctypes for more detailed financial tracking[1].
• Calculation Logic: Updated to handle net amounts separately from tax amounts, providing more accurate tax breakdowns[1].
• Item-wise Tax Details: Now includes net amount information, offering a more comprehensive view of tax application per item[1].

Impact

This change will provide users with more detailed tax information, allowing for:

• Better tracking of net amounts before tax application
• More accurate reporting of tax breakdowns
• Enhanced visibility into tax calculations at the item level

Testing Recommendations

• Verify that net amounts are correctly calculated and displayed in all relevant doctypes
• Ensure that existing tax calculations remain accurate with the introduction of the new fields
• Test various tax scenarios to confirm the correct interaction between net amounts and tax amounts

This update significantly improves the granularity and accuracy of tax-related information in ERPNext, providing users with more detailed financial insights. Additionally, it enhances compatibility with UBL2.1 (Universal Business Language) electronic documents, allowing for more precise representation of net amounts and tax calculations in standardized e-invoicing formats. This improvement facilitates better interoperability with various e-invoicing systems and ensures compliance with digital reporting requirements in jurisdictions that mandate UBL2.1 format for electronic invoices.

Migration Recommendations

• DO NOT migrate while you have a POS Invoice Merge pending: the change in json datastructure will be incompatible. Make sure to migrate after a completed merge to only merge invoices with compatible data structures. Data is now migrated via migration script

cc/ @barredterra

Example interaction

Prior

tax_data = json.loads(tax.item_wise_tax_detail)
rate, amount = tax_data

Now

tax_data = json.loads(tax.item_wise_tax_detail)
rate, amount, net_amount = tax_data.values()
# or
rate = tax_data.get("tax_rate")
amount = tax_data.get("tax_amount")
net_amount = tax_data.get("net_amount") # not migrated, only new records
# or (preferred)
tax_data = ItemWiseTaxDetail(**tax_data) # helpful to clearly signal to future maintainers "I'm using this here"
rate = tax_data.tax_rate
amount = tax_data.tax_amount
net_amount = tax_data.net_amount # not migrated, only new records

[barredterra]

This is also a requirement for the UN/CEFACT Cross Industry Invoice, not only UBL.

Btw, the India Compliance app already calculates net amounts per Item (in case that is helpful here).

[barredterra]

DO NOT migrate while you have a POS Invoice Merge pending: the change in json datastructure will be incompatible. Make sure to migrate after a completed merge to only merge invoices with compatible data structures.

This restriction seems hard to enforce (or even communicate). We'll probably have to find a workaround.

[blaggacao]

Indeed, I added a migration script yesterday (wasn't yet able to test). It's not reconstructing the missing value, because that is very difficult to accurately infer in all cases, but it conforms the structure and removes that limitation.

[ruthra-kumar]

@blaggacao

Can you provide a simple example scenario with expected outcomes, along with

1. How the current system handles it
2. How the proposed change will help address it.

[blaggacao]

I've added a unit example of how the new datastructure can be worked with to the end of the PR description.

The key is the addition of net_amount so that UBL2.1 standard of representing item-wise tax breakup in edi documents can be implemented without brittle inference and rather reliably using the source values from the time of first calculation.

Several localization apps, even in-tree ones, have used brittle inference in the past to determine net_amount, but this may not meet the quality requirements for other implementations.

The change from a list to a _dict was overloaded onto this PR in order to use this API change and the baseline necessity for migration and also sanitize and future-proof this datastructure at the occasion. In part, this aided with validating a correct implementation of the migration, because it is easier to audit the explicit datastructure across the repository than a very subtle list and index based logic that is easy to misread.

So to sum it up:

• Higher quality localization code
• Future Proofing the datastructure
• Remove a long standing code bottleneck

Maybe a word on brittleness:

• Rounding logic has evolved over time
• Localization apps didn't catch up (or didn't bother to soundly implement in the first place)
• As a result, already today, certain rounding settings in certain localizations will produce inconsistent results (wether always consequential or not is an entirely different question)

[barredterra]

Several localization apps, even in-tree ones, have used brittle inference in the past to determine net_amount

Can confirm 😁

https://github.com/alyf-de/eu_einvoice/blob/a2fa848d20a55b920eb6c07fab87300543377edc/eu_einvoice/european_e_invoice/custom/sales_invoice.py#L180-L182

[blaggacao]

This proposal to deprecate has two goals:

1. Prevent new usage of this method; nudging users to use the more reliable and stable means implemented by this PR
2. Eventually remove the liability and cost to maintain these lines of codes

The main goal is point 1, therefore, we also can also set the graduation version beyond v17.

A good graduation point would be something that matches the natural rate of change and evolution of code within the ecosystem.

Given that current downstream users have made use of brittle work around for the previous limitations, it might be a good thing to prompt them to revisit this code within the span of roughly 1-2 years.

[blaggacao]

@ruthra-kumar even if we wouldn't be able to merge it right away, could we lock in an eventual agreement on the design principle, namely:

• Extension of the datastructure by net_amount
• Modification of the datastructure to better future proof the implementation, incl poor-mans typing via type alias

This would currently help a lot as ecosystem downstream work is conditioned (and currently halted) on these hypotheses.

[ruthra-kumar]

We can go ahead with this.

As with most complex logic, edge cases will only come out the more it gets used. Will have to tackle it as and when those arise.

[blaggacao]

Thanks @ruthra-kumar

I've added a priority developer SLA pledge to the beginning of the PR description for the next two weeks.