fix: split_invoices_based_on_payment_terms

[marination]

Issues:

• Invoices were in the wrong order due to the logic. The invoices with payment terms were added first and the rest after.
• Overly long function with unnecessary loops (reduced to one main loop) and complexity

Before:

• "ACC-SINV-2023-00038" is allocated first inspite having the longest due date, as it is an invoice with payment terms which was processed first
• The order makes a difference when one of the invoices is partially allocated. The expectation would be the oldest invoices get fully allocated first. Which does not happen in this case
• Invoice "ACC-SINV-2023-00037" is partially allocated when we wanted "ACC-SINV-2023-00038" to be partially allocated as it is the newest invoice with a longer due date
  

After:

• "ACC-SINV-2023-00038" is partially allocated
• The order of instalments among split rows of the invoice is also maintained
  

Misc:

• Extracted major parts of split_invoices_based_on_payment_terms into get_currency_data and get_split_invoice_rows
• Use one single loop to process invoices. In the loop if the invoice can be broken down by payment terms then do that, else add the invoice row as is. In this way the order of invoices is maintained.
• Instead of creating a default dict for payment terms against invoices, fetch the details in the loop dynamically

Note: Encountered this issue while trying to reuse this API for the new Bank Reco tool. Even there, the order of invoices passed to this API highly matters as the invoices are ordered by rank and the allocation must happen exactly like that.

[codecov]

Codecov Report

Merging #37859 (4b4b176) into develop (6e3e094) will decrease coverage by 0.01%.
Report is 36 commits behind head on develop.
The diff coverage is 68.64%.

❗ Current head 4b4b176 differs from pull request most recent head 1fc5844. Consider uploading reports for the commit 1fc5844 to get more accurate results

@@             Coverage Diff             @@
##           develop   #37859      +/-   ##
===========================================
- Coverage    67.40%   67.39%   -0.01%     
===========================================
  Files          757      757              
  Lines        60337    60375      +38     
===========================================
+ Hits         40668    40688      +20     
- Misses       19669    19687      +18     

Files	Coverage Δ
..._receivable_summary/accounts_receivable_summary.py	92.15% <100.00%> (+0.07%)	⬆️
...tax_withholding_details/tax_withholding_details.py	79.87% <ø> (ø)
erpnext/accounts/utils.py	73.22% <100.00%> (+0.06%)	⬆️
erpnext/stock/doctype/stock_entry/stock_entry.py	82.41% <ø> (ø)
erpnext/stock/get_item_details.py	83.03% <ø> (ø)
.../report/accounts_receivable/accounts_receivable.py	90.09% <96.15%> (+0.13%)	⬆️
erpnext/accounts/report/financial_statements.py	80.42% <0.00%> (ø)
...t/accounts/report/general_ledger/general_ledger.py	70.45% <0.00%> (ø)
...ext/accounts/report/trial_balance/trial_balance.py	75.14% <0.00%> (ø)
...eport/fixed_asset_register/fixed_asset_register.py	0.00% <0.00%> (ø)
... and 6 more

[bosue]

Sorry for bothering you again.

The order makes a difference when one of the invoices is partially allocated. The expectation would be the oldest invoices get fully allocated first. Which does not happen in this case

In general, this is indeed the expectation, so this PR definitely improves things. I would actually consider it a bugfix, name it accordingly, and make sure it’s backported, although refactoring may be part of the solution.

However, there are quite many situation where the allocation order needs to be different:

• There may be an explicit or more often: implicit instruction by the customer which invoices the payment should be applied to. (§ 366 BGB in Germany)
• Costs, then interests, then principle rules (§ 367 BGB in Germany)
• In rental contracts etc., the current invoice ( < 3 weeks or the current month) needs to be allocated first as only then it is considered a “cash-like transaction”, at least according to the German insolvency clawback law.
• etc.

While your PR does implement a sensible default, in followups we should iterate over this to simplify switching the allocation order or even to implement allocation rules.

[marination]

@bosue Sounds good. Unfortunately I don't have the bandwidth to take this up, but I'd suggest moving your suggestion to a separate issue where it can be tracked and worked on. Thanks for taking the effort :)

[bosue]

A pleasure. Think we need a basic test to fail ahead of the bugfix while passing with it, and then this should be merged and backported. Probably even as “fix!” as it may break previous expectations.

[ruthra-kumar]

"ACC-SINV-2023-00038" is partially allocated

@marination This(allocating the oldest first) doesn't happen.

SINV-23-20599 is the oldest with term based allocation in below scenario.

term_based_allocation.mp4

[bosue]

@ruthra-kumar:

I can reproduce though:

With adapted columns (added Due Date) it’s easier to spot.

[marination]

@ruthra-kumar I worded this wrongly. My apologies

• So the issue was that the invoices passed to split_invoices_based_on_payment_terms would lose their order.
• Due to how this function processed payment terms first and then added invoices without terms later, we would always have invoices with payment terms added first to the references table (irrespective of due date), which is fixed now by the refactor.
• In ERPNEXT split_invoices_based_on_payment_terms gets invoices from get_outstanding_invoices, which orders the invoices by due_date.
• The Payment terms themselves were also fetched haphazardly earlier (the last term was allocated first). I've added an order_by to the Payment Schedule query to fix the order.

What @bosue has shown in his last comment is the issue. The invoices are fetched as per due date but split_invoices_based_on_payment_terms messes up the order.

[marination]

Also I think we should have the Due Date in the Payment References child table. If space is a problem, maybe even instead of the Grand Total. Outstanding and Allocated amounts should be sufficient.

@bosue I'm personally not sure/very opinionated on what columns users would deem important although I agree with your suggestion. Let this be tackled in a separate PR/issue

[ruthra-kumar]

Ignoring unrelated test failure https://github.com/frappe/erpnext/actions/runs/6812843279/job/18526119760?pr=37859