From 8ca20240a72bb6cdeb0be4715bee43ab76bfd771 Mon Sep 17 00:00:00 2001 From: Basilisa Kornel Date: Mon, 8 Jun 2026 23:22:36 +0300 Subject: [PATCH] fix: stop double-posting and miscalculating government share in deferred revenue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../doctype/plot_contract/plot_contract.py | 31 ++++------ .../landms/doctype/plot_master/plot_master.js | 9 +++ .../landms/doctype/plot_master/plot_master.py | 15 +++++ .../report/landms_revenue/landms_revenue.py | 58 ++++++++++++------- landms/payment_sync.py | 56 +++++++++++++----- landms/public/js/sales_order.js | 2 + landms/sales_order_hooks.py | 2 + 7 files changed, 117 insertions(+), 56 deletions(-) diff --git a/landms/landms/doctype/plot_contract/plot_contract.py b/landms/landms/doctype/plot_contract/plot_contract.py index 2d604a0..091ad48 100644 --- a/landms/landms/doctype/plot_contract/plot_contract.py +++ b/landms/landms/doctype/plot_contract/plot_contract.py @@ -606,27 +606,20 @@ def _post_completion_entries(self, settings): if selling_price <= 0: return None - accounts = [{ - "account": settings.customer_advance_account, - "debit_in_account_currency": selling_price, - "cost_center": settings.cost_center, - "land_acquisition": self.land_acquisition, - }] - - if govt_fee > 0: - accounts.append({ - "account": settings.government_payable_account, - "credit_in_account_currency": govt_fee, + accounts = [ + { + "account": settings.customer_advance_account, + "debit_in_account_currency": net_revenue, "cost_center": settings.cost_center, "land_acquisition": self.land_acquisition, - }) - - accounts.append({ - "account": settings.revenue_account, - "credit_in_account_currency": net_revenue, - "cost_center": settings.cost_center, - "land_acquisition": self.land_acquisition, - }) + }, + { + "account": settings.revenue_account, + "credit_in_account_currency": net_revenue, + "cost_center": settings.cost_center, + "land_acquisition": self.land_acquisition, + }, + ] je = frappe.get_doc({ "doctype": "Journal Entry", diff --git a/landms/landms/doctype/plot_master/plot_master.js b/landms/landms/doctype/plot_master/plot_master.js index ce46b70..6018908 100644 --- a/landms/landms/doctype/plot_master/plot_master.js +++ b/landms/landms/doctype/plot_master/plot_master.js @@ -5,6 +5,15 @@ frappe.ui.form.on('Plot Master', { frm.set_query('land_acquisition', () => ({ filters: { status: ['in', ['Approved', 'Subdivided']] } })); + + // Only show plot types that have a selling rate on the current LA + frm.set_query('plot_type', () => { + if (!frm.doc.land_acquisition) return {}; + return { + query: 'landms.landms.doctype.plot_master.plot_master.get_plot_types_for_la', + filters: { land_acquisition: frm.doc.land_acquisition } + }; + }); }, refresh(frm) { diff --git a/landms/landms/doctype/plot_master/plot_master.py b/landms/landms/doctype/plot_master/plot_master.py index dd1ba6e..281356b 100644 --- a/landms/landms/doctype/plot_master/plot_master.py +++ b/landms/landms/doctype/plot_master/plot_master.py @@ -306,3 +306,18 @@ def _build_plot_stock_entry_doc( } ], } + + +@frappe.whitelist() +@frappe.validate_and_sanitize_search_inputs +def get_plot_types_for_la(doctype, txt, searchfield, start, page_len, filters): + la = filters.get("land_acquisition") if isinstance(filters, dict) else None + if not la: + return [] + return frappe.db.sql(""" + SELECT DISTINCT plot_type + FROM `tabLand Acquisition Plot Type Rate` + WHERE parent = %s AND plot_type LIKE %s + ORDER BY plot_type + LIMIT %s OFFSET %s + """, (la, f"%{txt}%", page_len, start)) diff --git a/landms/landms/report/landms_revenue/landms_revenue.py b/landms/landms/report/landms_revenue/landms_revenue.py index b153d79..b7386ea 100644 --- a/landms/landms/report/landms_revenue/landms_revenue.py +++ b/landms/landms/report/landms_revenue/landms_revenue.py @@ -30,51 +30,65 @@ def get_data(filters): if grouping == "Weekly": period_expr = "CONCAT('Week ', LPAD(WEEK(pe.posting_date, 1), 2, '0'), ' — ', YEAR(pe.posting_date))" sort_expr = "YEARWEEK(pe.posting_date, 1)" + je_sort = "YEARWEEK(je.posting_date, 1)" else: period_expr = "DATE_FORMAT(pe.posting_date, '%%M %%Y')" sort_expr = "DATE_FORMAT(pe.posting_date, '%%Y%%m')" + je_sort = "DATE_FORMAT(je.posting_date, '%%Y%%m')" - rows = frappe.db.sql(f""" + params = {"from_date": from_date, "to_date": to_date} + + pay_rows = frappe.db.sql(f""" SELECT - {period_expr} AS period, - {sort_expr} AS sort_key, - COUNT(DISTINCT pe.name) AS payment_count, - COUNT(DISTINCT COALESCE(pc.name, so.plot_contract)) AS contract_count, - SUM(per.allocated_amount) AS total_collected, - SUM( - CASE - WHEN pc.contract_status = 'Completed' - THEN per.allocated_amount * pc.government_share_percent / 100 - ELSE 0 - END - ) AS govt_fee + {period_expr} AS period, + {sort_expr} AS sort_key, + COUNT(DISTINCT pe.name) AS payment_count, + COUNT(DISTINCT COALESCE(pc.name, so.plot_contract)) AS contract_count, + SUM(per.allocated_amount) AS total_collected FROM `tabPayment Entry` pe INNER JOIN `tabPayment Entry Reference` per - ON per.parent = pe.name - AND per.reference_doctype = 'Sales Invoice' + ON per.parent = pe.name + AND per.reference_doctype = 'Sales Invoice' INNER JOIN `tabSales Invoice` si ON si.name = per.reference_name AND si.docstatus = 1 AND si.is_plot_sale_invoice = 1 LEFT JOIN `tabSales Order` so - ON so.plot_sales_invoice = si.name - AND so.docstatus = 1 + ON so.plot_sales_invoice = si.name + AND so.docstatus = 1 LEFT JOIN `tabPlot Contract` pc - ON pc.name = COALESCE(NULLIF(si.plot_contract, ''), so.plot_contract) - AND pc.docstatus != 2 + ON pc.name = COALESCE(NULLIF(si.plot_contract, ''), so.plot_contract) + AND pc.docstatus != 2 WHERE pe.party_type = 'Customer' AND pe.docstatus = 1 AND pe.posting_date BETWEEN %(from_date)s AND %(to_date)s GROUP BY sort_key, period ORDER BY sort_key - """, {"from_date": from_date, "to_date": to_date}, as_dict=True) + """, params, as_dict=True) + + je_rows = frappe.db.sql(f""" + SELECT + {je_sort} AS sort_key, + SUM(jea.credit_in_account_currency) AS govt_fee + FROM `tabJournal Entry` je + INNER JOIN `tabJournal Entry Account` jea + ON jea.parent = je.name + AND jea.credit_in_account_currency > 0 + WHERE je.docstatus = 1 + AND je.lms_payment_entry IS NOT NULL + AND je.lms_payment_entry != '' + AND je.posting_date BETWEEN %(from_date)s AND %(to_date)s + GROUP BY sort_key + """, params, as_dict=True) + + je_by_key = {str(r.sort_key): flt(r.govt_fee) for r in je_rows} data = [] grand_collected = grand_govt = grand_payments = grand_contracts = 0.0 - for row in rows: + for row in pay_rows: collected = flt(row.total_collected) - govt = flt(row.govt_fee) + govt = je_by_key.get(str(row.sort_key), 0.0) net = collected - govt grand_collected += collected grand_govt += govt diff --git a/landms/payment_sync.py b/landms/payment_sync.py index 8accf46..b20ea31 100644 --- a/landms/payment_sync.py +++ b/landms/payment_sync.py @@ -696,12 +696,18 @@ def _post_government_share_je(pe_doc): Dr Customer Advances Account (reducing deferred revenue) Cr Government Payable Account (recording govt share payable) - Amount = government_share_percent × paid_amount + Amount = government_share_percent × allocated_amount + + allocated_amount (not the PE's total paid_amount) is used because a single + payment can bundle in extra cash beyond the plot price — e.g. customers + often pay government fees alongside the plot price in the same transaction. + Only the portion actually allocated to the plot sale is land-sale revenue + subject to the government's share. """ if pe_doc.docstatus != 1: return - so_name, govt_pct, land_acquisition = _get_govt_share_fields_from_pe(pe_doc) + so_name, govt_pct, land_acquisition, allocated_amount = _get_govt_share_fields_from_pe(pe_doc) if not so_name or flt(govt_pct) <= 0: return @@ -709,7 +715,7 @@ def _post_government_share_je(pe_doc): if not settings.government_payable_account or not settings.customer_advance_account: return - govt_amount = flt(pe_doc.paid_amount) * flt(govt_pct) / 100.0 + govt_amount = flt(allocated_amount) * flt(govt_pct) / 100.0 if govt_amount <= 0: return @@ -756,8 +762,24 @@ def _cancel_government_share_je(pe_doc): def _get_govt_share_fields_from_pe(pe_doc): - """Return (sales_order_name, government_share_percent, land_acquisition) from PE references.""" + """Return (sales_order_name, government_share_percent, land_acquisition, allocated_amount) + from PE references. + + allocated_amount is the SUM of allocated amounts across every reference row that + resolves to the plot sale — its Sales Order directly, or a Sales Invoice flagged + as the plot sale invoice for that Sales Order. Frappe can split one payment's + allocation to the same invoice/order across multiple reference rows, so a single + matching row is not enough. This is not the Payment Entry's total paid_amount, + which can include extra cash the customer bundled into the same transaction (e.g. + for government fees) that was never allocated to the plot sale invoice/order.""" + so_name = None + govt_pct = 0 + land_acquisition = "" + allocated_amount = 0.0 + for row in pe_doc.get("references") or []: + row_so_name = None + if row.reference_doctype == "Sales Invoice": si = frappe.db.get_value( "Sales Invoice", row.reference_name, @@ -766,24 +788,28 @@ def _get_govt_share_fields_from_pe(pe_doc): ) if not si or not si.is_plot_sale_invoice: continue - so_name = frappe.db.get_value( + row_so_name = frappe.db.get_value( "Sales Invoice Item", {"parent": row.reference_name}, "sales_order", ) - if not so_name: - continue + elif row.reference_doctype == "Sales Order": + row_so_name = row.reference_name + + if not row_so_name: + continue + + if so_name is None: + so_name = row_so_name govt_pct, land_acquisition = frappe.db.get_value( "Sales Order", so_name, ["government_share_percent", "land_acquisition"], ) or (0, "") - return so_name, flt(govt_pct), land_acquisition - if row.reference_doctype == "Sales Order": - govt_pct, land_acquisition = frappe.db.get_value( - "Sales Order", row.reference_name, - ["government_share_percent", "land_acquisition"], - ) or (0, "") - return row.reference_name, flt(govt_pct), land_acquisition + if row_so_name == so_name: + allocated_amount += flt(row.allocated_amount) + + if not so_name: + return None, 0, "", 0 - return None, 0, "" + return so_name, flt(govt_pct), land_acquisition, allocated_amount diff --git a/landms/public/js/sales_order.js b/landms/public/js/sales_order.js index 75889b4..5cb33bc 100644 --- a/landms/public/js/sales_order.js +++ b/landms/public/js/sales_order.js @@ -76,6 +76,8 @@ function _render_so_close_button(frm) { frm.remove_custom_button(__('Hold'), __('Status')); }, 10); + if (!frappe.user.has_role('LandMS Sales Manager')) return; + frm.add_custom_button(__('Close Sales Order'), () => { frappe.confirm( __('Close this Sales Order? The plot will be released and the TCB reference declined. This cannot be undone.'), diff --git a/landms/sales_order_hooks.py b/landms/sales_order_hooks.py index eaa1828..526e2da 100644 --- a/landms/sales_order_hooks.py +++ b/landms/sales_order_hooks.py @@ -181,6 +181,8 @@ def cancel_sales_order(doc, method=None): @frappe.whitelist() def close_sales_order(sales_order_name): """Close a plot Sales Order with no payments — releases plot, declines TCB CN, cancels application.""" + frappe.only_for("LandMS Sales Manager") + doc = frappe.get_doc("Sales Order", sales_order_name) if not _is_landms_sales_order(doc): frappe.throw("This Sales Order is not a LandMS plot sale.")