Fix government share double-posting; restrict Close Sales Order to Managers#46
Merged
av-dev2 merged 1 commit intoJun 8, 2026
Merged
Conversation
…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.
|
Successfully created backport PR for |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
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:
Plot Contract._post_completion_entriesre-creditedthe full
government_fee_withheldto Government Payable at contractcompletion — on top of the incremental credits
payment_syncalreadyposts as each payment comes in. Every completed contract's government
share was effectively doubled.
Entry's full
paid_amount(which can include unallocated extra cashbundled into the same transaction) instead of the
allocated_amountactually applied to the plot-sale invoice/order — and didn't sum
allocated_amountacross multiple reference rows when Frappe split onepayment's allocation across several rows to the same invoice.
Fix: completion now recognizes only
net_revenue(government share isposted solely incrementally), and the incremental postings sum
allocated_amountacross 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_withheldfor 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.
frappe.user.has_role('LandMS Sales Manager')hides the custombutton from non-Managers (the standard Close/Hold buttons remain hidden
for everyone, funneling all users through the safe flow).
frappe.only_for("LandMS Sales Manager")enforces the samerestriction in
close_sales_order, so it can't be bypassed by callingthe method directly.