Skip to content

fix: rebase POS sales invoice GL entries on ERPNext 16#209

Open
my-dev-jour wants to merge 1 commit intoBrainWise-DEV:developfrom
my-dev-jour:fix/191-rebase-pos-gl-entries
Open

fix: rebase POS sales invoice GL entries on ERPNext 16#209
my-dev-jour wants to merge 1 commit intoBrainWise-DEV:developfrom
my-dev-jour:fix/191-rebase-pos-gl-entries

Conversation

@my-dev-jour
Copy link
Copy Markdown
Contributor

Summary

  • rebase POSNext's Sales Invoice POS GL override on the ERPNext 16 core make_pos_gl_entries flow
  • restore ERPNext 16's against_voucher guard and transaction-currency fields
  • keep the wallet-specific party_type/party behavior on payment-mode accounts

Why

POSNext's override had drifted from ERPNext 16 core and could produce incorrect paid/outstanding behavior on submitted POS invoices.

Testing

  • python3 -m py_compile pos_next/overrides/sales_invoice.py
  • local verification was limited to py_compile in this workspace

Refs #191

@my-dev-jour
Copy link
Copy Markdown
Contributor Author

Staging validation on ERPNext 16.11.0 showed the immediate root cause for the unpaid submitted invoice path was payment rows being overwritten during draft creation in pos_next/api/invoices.py, not the GL override alone.

The confirmed fix is now in PR #215, which preserves cashier-entered payment rows across set_missing_values() and was validated on staging with a real POS flow.

Closing or deprioritizing this PR in favor of #215 would avoid parallel review on the wrong root-cause path.

Copy link
Copy Markdown
Contributor

@engahmed1190 engahmed1190 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — REQUEST CHANGES ❌ (BLOCKER)

Summary

Rebases make_pos_gl_entries override on ERPNext 16 core flow. Adds against_voucher guard, transaction_currency fields, and removes the v15/v16 compatibility shim.

Test Results

No tests included.

CRITICAL BUG: v15 Breakage

The PR removes the hasattr(self, 'get_gle_for_change_amount') compatibility check and unconditionally calls self.get_gle_for_change_amount() (line 122).

The installed ERPNext on this bench is v15. In ERPNext v15, this method is called make_gle_for_change_amount (with a different signature — it takes gl_entries as a parameter). get_gle_for_change_amount does not exist in v15.

This will cause an AttributeError at runtime whenever a POS invoice with change amount is submitted. This is a hard crash in the invoice submission path.

The hasattr guard that was removed existed precisely for this reason — it allowed POSNext to work on both v15 and v16.

Other Changes (Correct)

The non-breaking changes in this PR are good:

  • against_voucher pre-computation: Cleaner than the inline ternary. Correct for returns.
  • payment_mode.base_amount gating: Correct — matches ERPNext 16 core and handles multi-currency properly
  • credit_in_transaction_currency / debit_in_transaction_currency: Correct for multi-currency GL entries. Harmless on v15 (get_gl_dict ignores unknown keys)
  • Docstring update: Appropriate

Required Fix

Either:

  1. Restore the hasattr guard:
    if hasattr(self, 'get_gle_for_change_amount'):
        gl_entries.extend(self.get_gle_for_change_amount())
    else:
        self.make_gle_for_change_amount(gl_entries)
  2. OR defer this entire PR until the ERPNext 16 upgrade is confirmed and executed

Additional Notes

  • PR #215 description says it "supersedes the earlier GL-focused hypothesis from PR #209" — the payment-wiping bug was ultimately fixed at the set_missing_values level, not the GL level
  • The _get_post_change_gl_entries_setting() helper is preserved (good)

Verdict

The v15 compatibility removal is a runtime crash blocker. The other changes are correct and should be preserved, but the hasattr guard must be restored unless this project has committed to dropping v15 support.

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.

2 participants