Skip to content

Fix government share double-posting; restrict Close Sales Order to Managers#47

Merged
av-dev2 merged 1 commit into
version-15from
backport-46-to-version-15
Jun 8, 2026
Merged

Fix government share double-posting; restrict Close Sales Order to Managers#47
av-dev2 merged 1 commit into
version-15from
backport-46-to-version-15

Conversation

@github-actions

@github-actions github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown

Backport of #46 to version-15.


  • Fixes a deferred-revenue accounting bug where completing a Plot Contract
    double-posted (and in some cases miscalculated) the government share to
    Government Payable, on top of the incremental per-payment postings already
    made as the customer pays.
  • Restricts the "Close Sales Order" action — button and whitelisted server
    method — to users with the LandMS Sales Manager role, both in the UI
    and at the API level.

Details

Government share double-posting & miscalculation (plot_contract.py, payment_sync.py)

Two compounding bugs in the deferred-revenue model:

  1. Double-posting: Plot Contract._post_completion_entries re-credited
    the full government_fee_withheld to Government Payable at contract
    completion — on top of the incremental credits payment_sync already
    posts as each payment comes in. Every completed contract's government
    share was effectively doubled.
  2. Wrong calculation base: the incremental postings used the Payment
    Entry's full paid_amount (which can include unallocated extra cash
    bundled into the same transaction) instead of the allocated_amount
    actually applied to the plot-sale invoice/order — and didn't sum
    allocated_amount across multiple reference rows when Frappe split one
    payment's allocation across several rows to the same invoice.

Fix: completion now recognizes only net_revenue (government share is
posted solely incrementally), and the incremental postings sum
allocated_amount across every relevant reference row as the tax base.

Verified end-to-end with two fresh full-payment-cycle tests through the
real TCB IPN flow (including a split-allocation edge case), and corrected
the 3 genuinely affected completed contracts in live data for
LACQ-2026-0002 via audit-safe reversing Journal Entries
(ACC-JV-2026-17252/53/54, totaling 10,699,700.29 — verified diff = 0.00
against government_fee_withheld for all three).

Close Sales Order — Manager-only restriction (sales_order.js, sales_order_hooks.py)

The custom "Close Sales Order" action had no role check — any user with
Sales Order access could trigger its cascade (forfeiture JE, invoice/
contract cleanup, TCB control-number decline, application and plot release)
directly from the UI, or by calling the whitelisted method via the API.

  • Client: frappe.user.has_role('LandMS Sales Manager') hides the custom
    button from non-Managers (the standard Close/Hold buttons remain hidden
    for everyone, funneling all users through the safe flow).
  • Server: frappe.only_for("LandMS Sales Manager") enforces the same
    restriction in close_sales_order, so it can't be bypassed by calling
    the method directly.

…red revenue

Plot Contract completion re-credited the full government_fee_withheld to
Government Payable on top of the incremental per-payment postings already
made by payment_sync, exactly doubling the government share for every
completed contract. The incremental postings also used the Payment Entry's
full paid_amount (which can include unallocated extra cash) rather than the
allocated_amount actually applied to the plot-sale invoice/order, and didn't
sum allocated_amount across multiple reference rows when Frappe split one
payment's allocation across several rows to the same invoice.

Completion now recognizes only net_revenue; government share is posted
solely incrementally, computed from the summed allocated_amount across every
relevant reference row.
fix: restrict Close Sales Order to LandMS Sales Manager role

The custom "Close Sales Order" action (button + whitelisted server method)
had no role check, so any user with Sales Order access could trigger its
cascade — forfeiture JE, invoice/contract cleanup, TCB decline, application
and plot release — directly from the UI or via the API. Hide the button from
non-Managers client-side and enforce the same restriction server-side with
frappe.only_for so the whitelisted method can't be invoked directly by
anyone else.

(cherry picked from commit 8ca2024)
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