diff --git a/budget_control/models/account_move.py b/budget_control/models/account_move.py index 759415b9..18804d5d 100644 --- a/budget_control/models/account_move.py +++ b/budget_control/models/account_move.py @@ -99,7 +99,9 @@ def write(self, vals): if vals.get("state") in ("posted", "cancel", "draft") or vals.get( "not_affect_budget" ): - self.mapped("invoice_line_ids").recompute_budget_move() + self.mapped("invoice_line_ids")._call_budget_method_guarded( + "recompute_budget_move" + ) return res def _filtered_move_check_budget(self): diff --git a/budget_control/models/base_budget_move.py b/budget_control/models/base_budget_move.py index 09c71cca..4ba9cf5b 100644 --- a/budget_control/models/base_budget_move.py +++ b/budget_control/models/base_budget_move.py @@ -186,6 +186,26 @@ def _budget_model(self): def _budget_field(self): return self.env.context.get("alt_budget_move_field") or self._budget_move_field + def _call_budget_method_guarded(self, method_name): + """Call () unless it's already in progress for this + exact recordset higher up the current call stack (write() MRO can + re-enter for the same records). + """ + if not self: + return + in_progress = getattr(self.env.cr, "_budget_recompute_in_progress", None) + if in_progress is None: + in_progress = set() + self.env.cr._budget_recompute_in_progress = in_progress + key = (self._name, method_name, frozenset(self.ids)) + if key in in_progress: + return + in_progress.add(key) + try: + getattr(self, method_name)() + finally: + in_progress.discard(key) + def _valid_commit_state(self): raise ValidationError(self.env._("No implementation error!")) diff --git a/budget_control/models/budget_period.py b/budget_control/models/budget_period.py index 23fbd0d9..80a2e612 100644 --- a/budget_control/models/budget_period.py +++ b/budget_control/models/budget_period.py @@ -1,6 +1,8 @@ # Copyright 2020 Ecosoft Co., Ltd. (http://ecosoft.co.th) # License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl). +from collections import defaultdict + from odoo import api, fields, models from odoo.exceptions import UserError, ValidationError from odoo.tools import SQL, float_compare, format_amount @@ -135,29 +137,19 @@ def _get_budget_constraint(self): ) @api.model - def check_budget(self, doclines, doc_type="account"): - """ - Check the budget based on the input budget moves, i.e., account_move_line. - 1. Get a valid budget period (how budget is being controlled). - 2. Determine which account (KPI) and analytic - to control based on (1) and doclines. - 3. Check for negative budget and return warnings - based on (2) and the KPI matrix. - """ - if self._context.get("force_no_budget_check"): - return - doclines = doclines.filtered("can_commit") - if not doclines: - return - self = self.sudo() - budget_constraints = self._get_budget_constraint() - all_analytics = doclines.mapped(doclines._budget_analytic_field) + def _group_doclines_by_analytic(self, doclines): + """Group document lines by every analytic in their distribution. - # Get All Analytic Account - all_analytic_ids = set() - for data_dict in all_analytics: + Odoo stores analytics from multiple plans in comma-separated JSON keys, + for example ``{"12,34": 100}``. Build all groups in one pass so those + keys are handled correctly without repeatedly filtering every docline. + """ + grouped_line_ids = defaultdict(list) + analytic_field = doclines._budget_analytic_field + for line in doclines: + distribution = line[analytic_field] or {} # Check percent analytic account must be 100% only - total_sum = sum(data_dict.values()) + total_sum = sum(distribution.values()) if ( float_compare( total_sum, @@ -172,61 +164,101 @@ def check_budget(self, doclines, doc_type="account"): "Please check again." ) ) - all_analytic_ids.update( - int(aa) for key in data_dict.keys() for aa in key.split(",") - ) + analytic_ids = { + int(analytic_id) + for key in distribution + for analytic_id in key.split(",") + } + for analytic_id in sorted(analytic_ids): + grouped_line_ids[analytic_id].append(line.id) + return [ + (analytic_id, doclines.browse(line_ids)) + for analytic_id, line_ids in grouped_line_ids.items() + ] + + @api.model + def check_budget(self, doclines, doc_type="account"): + """ + Check the budget based on the input budget moves, i.e., account_move_line. + 1. Get a valid budget period (how budget is being controlled). + 2. Determine which account (KPI) and analytic + to control based on (1) and doclines. + 3. Check for negative budget and return warnings + based on (2) and the KPI matrix. + """ + if self._context.get("force_no_budget_check"): + return + doclines = doclines.filtered("can_commit") + if not doclines: + return + self = self.sudo() + budget_constraints = self._get_budget_constraint() # Check budget by group analytic. For case many budget periods in one document. - for aa in all_analytic_ids: - if isinstance(aa, int): - doclines = doclines.filtered( - lambda line, aa=aa, doclines=doclines: line[ - doclines._budget_analytic_field - ].get(str(aa)) - ) - else: - doclines = doclines.filtered( - lambda line, aa=aa, doclines=doclines: line[ - doclines._budget_analytic_field - ] - == aa - ) + # Pass 1: resolve each analytic account's group, no DB hit yet. + groups = [] + period_cache = {} + controls_cache = {} + for analytic_id, aa_doclines in self._group_doclines_by_analytic(doclines): # Find active budget.period based on latest doclines date_commit - date_commit = doclines.filtered("date_commit").mapped("date_commit") + date_commit = aa_doclines.filtered("date_commit").mapped("date_commit") if not date_commit: - return + continue date_commit = max(date_commit) - budget_period = self._get_eligible_budget_period( - date_commit, doc_type=doc_type - ) + if date_commit not in period_cache: + period_cache[date_commit] = self._get_eligible_budget_period( + date_commit, doc_type=doc_type + ) + budget_period = period_cache[date_commit] if not budget_period: - return - # Find combination of account (KPI) + analytic (i.e., project) to control - controls = self._prepare_controls(budget_period, doclines) + continue + # Find KPI controls only for this analytic. A line may contain several + # analytics and consequently several budget moves. + controls_key = (budget_period.id, tuple(aa_doclines.ids)) + if controls_key not in controls_cache: + controls_cache[controls_key] = self._prepare_controls( + budget_period, aa_doclines + ) + controls = [ + control + for control in controls_cache[controls_key] + if control["analytic_id"] == analytic_id + ] if not controls: - return - # The budget_control of these analytics must be active - if isinstance(aa, int): - analytic_ids = all_analytic_ids - else: - analytic_ids = [x["analytic_id"] for x in controls] - analytics = self.env["account.analytic.account"].browse(analytic_ids) - analytics._check_budget_control_status(budget_period_id=budget_period.id) + continue + groups.append((aa_doclines, date_commit, budget_period, controls)) + if not groups: + return + # Validate each period's analytics in one query before the monitor scan. + analytic_ids_by_period = defaultdict(set) + for _doclines, _date, budget_period, controls in groups: + analytic_ids_by_period[budget_period.id].update( + control["analytic_id"] for control in controls + ) + for budget_period_id, analytic_ids in analytic_ids_by_period.items(): + self.env["account.analytic.account"].browse( + sorted(analytic_ids) + )._check_budget_control_status(budget_period_id=budget_period_id) + # Pass 2: one shared prefetch instead of one scan per control. + all_controls = [c for group in groups for c in group[3]] + avail_cache = self._prefetch_budget_available(all_controls) + self = self.with_context(_budget_avail_cache=avail_cache) + for aa_doclines, date_commit, budget_period, controls in groups: # Check budget on each control element against each KPI/avail (period) currency = ( - "currency_id" in doclines - and doclines.mapped("currency_id")[:1] + "currency_id" in aa_doclines + and aa_doclines.mapped("currency_id")[:1] or self.env.context.get("doc_currency", self.env.company.currency_id) ) warnings = self.with_context( - date_commit=date_commit, doc_currency=currency, doclines=doclines + date_commit=date_commit, doc_currency=currency, doclines=aa_doclines )._check_budget_available(controls, budget_period) if warnings: msg = "\n".join(["Budget not sufficient,", "\n".join(warnings)]) raise UserError(msg) # Check budget constraint following your customize condition - elif doclines and budget_constraints and budget_period: - self.check_budget_constraint(budget_constraints, doclines) + elif aa_doclines and budget_constraints and budget_period: + self.check_budget_constraint(budget_constraints, aa_doclines) return @api.model @@ -429,9 +461,71 @@ def _get_budget_monitor_report(self): """Hook for add context""" return self.env["budget.monitor.report"] + def _can_use_budget_available_cache(self, template_lines): + """Whether the shared cache represents this availability query. + + Extensions adding dimensions to ``_get_where_domain()`` should override + this hook and return ``False`` for template-line models that need those + extra filters. + """ + return not template_lines or template_lines._name == "budget.template.line" + + def _prefetch_budget_available(self, controls): + """One monitor-report scan for all controls' analytic accounts, keyed + by analytic_id. ``check_budget()`` can then serve every control from + this dict instead of running one SQL query per control. + + Only the base filter shape (``analytic_account_id IN (...)``) is + served here; ``_get_where_domain()`` overrides that add extra clauses + (e.g. budget_plan_detail's ``fund_id``) are not covered and must keep + using the per-control ``_get_budget_avaiable()`` path. + """ + analytic_ids = sorted( + {c["analytic_id"] for c in controls if c.get("analytic_id")} + ) + if not analytic_ids: + return {} + self.env.flush_all() + report_sql = self._get_budget_monitor_report()._table_query + self.env.cr.execute( + SQL( + """ + SELECT + analytic_account_id, + kpi_id, + budget_period_id, + amount_type, + amount + FROM (%s) report + WHERE analytic_account_id IN %s + """, + SQL(report_sql), + tuple(analytic_ids), + ) + ) + cache = {} + for row in self.env.cr.dictfetchall(): + cache.setdefault(row["analytic_account_id"], []).append(row) + return cache + def _get_budget_avaiable(self, analytic_id, template_lines): # Callers that batch many queries can set env.context['skip_budget_flush'] # after flushing once themselves, avoiding a flush_all() per control. + # check_budget() additionally pre-scans every analytic once and passes + # the rows through env.context['_budget_avail_cache']; we then filter + # by kpi_id in Python when control_level != "analytic". + cache = self.env.context.get("_budget_avail_cache") + # The cache only covers the base filter shape; custom _get_where_domain + # overrides (e.g. budget_plan_detail's fund_id) fall through to a query. + if cache is not None and self._can_use_budget_available_cache(template_lines): + rows = cache.get(analytic_id, []) + if ( + template_lines + and self._context.get("control_level", False) != "analytic" + ): + kpi_ids = set(template_lines.kpi_id.ids) + rows = [r for r in rows if r.get("kpi_id") in kpi_ids] + return rows if not self.env.context.get("skip_budget_flush"): self.env.flush_all() self.env.cr.execute( @@ -462,11 +556,11 @@ def _check_budget_available(self, controls, budget_period): company = self.env.user.company_id doc_currency = self.env.context.get("doc_currency") date_commit = self.env.context.get("date_commit") - # Flush once for all controls. Budget moves are not written while this - # loop runs (it only reads and raises/returns), so a single flush before - # the loop is enough and avoids repeating the expensive flush_all() per - # control when checking many analytics/KPIs. - self.env.flush_all() + # A shared cache is built only after flushing, and nothing is written + # while the control groups are checked. Without a cache, flush once here + # before the per-control fallback queries. + if self.env.context.get("_budget_avail_cache") is None: + self.env.flush_all() self = self.with_context(skip_budget_flush=True) for control in controls: analytic_id = control["analytic_id"] diff --git a/budget_control/tests/test_budget_control.py b/budget_control/tests/test_budget_control.py index 57d5a2f7..15366da1 100644 --- a/budget_control/tests/test_budget_control.py +++ b/budget_control/tests/test_budget_control.py @@ -1660,3 +1660,57 @@ def _add_invoice_line(bill, account, not_affect_budget=False): self.assertEqual( sum(len(line.budget_move_ids) for line in bill3.invoice_line_ids), 0 ) + + @freeze_time("2001-02-01") + def test_27_multi_analytic_budget_check(self): + """Every analytic used by a document must be budget checked.""" + self.budget_period.control_budget = True + self.budget_period.control_level = "analytic" + budget_controls = self.budget_control | self.budget_control2 + budget_controls.action_submit() + budget_controls.action_done() + + bill = self._create_simple_bill( + {self.costcenter1.id: 100}, self.account_kpi1, 100 + ) + second_line_vals = { + "quantity": 1, + "account_id": self.account_kpi1.id, + "price_unit": 100000, + "analytic_distribution": {self.costcenterX.id: 100}, + } + if getattr(self, "check_plan_detail_installed", False): + second_line_vals.update( + { + "fund_id": self.fund1_g1.id, + "analytic_tag_ids": [Command.link(self.analytic_tag1.id)], + } + ) + bill.write({"invoice_line_ids": [Command.create(second_line_vals)]}) + + with self.assertRaisesRegex(UserError, "Budget not sufficient"): + bill.action_post() + + @freeze_time("2001-02-01") + def test_28_combined_analytic_distribution_key(self): + """Comma-separated analytic keys must not bypass budget checks.""" + self.budget_period.control_budget = True + self.budget_period.control_level = "analytic" + budget_controls = self.budget_control | self.budget_control2 + budget_controls.action_submit() + budget_controls.action_done() + + distribution_key = f"{self.costcenter1.id},{self.costcenterX.id}" + bill = self._create_simple_bill( + {distribution_key: 100}, self.account_kpi1, 100000 + ) + + grouped_analytics = { + analytic_id + for analytic_id, _lines in self.env[ + "budget.period" + ]._group_doclines_by_analytic(bill.invoice_line_ids) + } + self.assertEqual(grouped_analytics, {self.costcenter1.id, self.costcenterX.id}) + with self.assertRaisesRegex(UserError, "Budget not sufficient"): + bill.action_post() diff --git a/budget_control_expense/models/account_move.py b/budget_control_expense/models/account_move.py index a653d65a..b7d226ce 100644 --- a/budget_control_expense/models/account_move.py +++ b/budget_control_expense/models/account_move.py @@ -11,5 +11,7 @@ def write(self, vals): """Uncommit budget for source expense document.""" res = super().write(vals) if vals.get("state") in ("draft", "posted", "cancel"): - self.mapped("line_ids.expense_id").recompute_budget_move() + self.mapped("line_ids.expense_id")._call_budget_method_guarded( + "recompute_budget_move" + ) return res diff --git a/budget_control_expense/models/hr_expense_sheet.py b/budget_control_expense/models/hr_expense_sheet.py index c36ec594..5e48f78d 100644 --- a/budget_control_expense/models/hr_expense_sheet.py +++ b/budget_control_expense/models/hr_expense_sheet.py @@ -16,6 +16,8 @@ class HRExpenseSheet(models.Model): @api.constrains("expense_line_ids") def recompute_budget_move(self): + if self.env.context.get("_skip_auto_recompute_budget_move"): + return self.mapped("expense_line_ids").recompute_budget_move() def close_budget_move(self): @@ -33,7 +35,13 @@ def write(self, vals): - "cancel" = Canceled - False = To Submit (Draft) """ - res = super().write(vals) + # The write() below re-runs recompute_budget_move() explicitly, so skip + # the @api.constrains auto-trigger for those states to avoid doing it + # twice (the constrains fires on expense_line_ids writes done by super). + ctx_self = self + if vals.get("approval_state") in ("approve", "cancel", False): + ctx_self = self.with_context(_skip_auto_recompute_budget_move=True) + res = super(HRExpenseSheet, ctx_self).write(vals) if vals.get("approval_state") in ("approve", "cancel", False): doclines = self.mapped("expense_line_ids") if vals.get("approval_state") in ("cancel", False): diff --git a/budget_control_purchase/models/account_move.py b/budget_control_purchase/models/account_move.py index 07447bb0..d20b4a16 100644 --- a/budget_control_purchase/models/account_move.py +++ b/budget_control_purchase/models/account_move.py @@ -15,7 +15,7 @@ def write(self, vals): # invoice reversal. Update only the reversal owned by these invoice # lines; draft/cancel removes it and posted creates it. invoice_lines = self.mapped("invoice_line_ids").filtered("purchase_line_id") - invoice_lines.uncommit_purchase_budget() + invoice_lines._call_budget_method_guarded("uncommit_purchase_budget") # Budget totals are non-stored SQL aggregates and their declared # dependency is the budget plan lines. Incremental PO reversals do # not touch those lines, so explicitly drop any totals cached earlier diff --git a/budget_control_purchase/models/purchase.py b/budget_control_purchase/models/purchase.py index 11fd6f31..c1b24b50 100644 --- a/budget_control_purchase/models/purchase.py +++ b/budget_control_purchase/models/purchase.py @@ -17,6 +17,8 @@ class PurchaseOrder(models.Model): # Allow trigger, because purchase order line is editable even when approved. @api.constrains("order_line") def recompute_budget_move(self): + if self.env.context.get("_skip_auto_recompute_budget_move"): + return self.mapped("order_line").recompute_budget_move() def close_budget_move(self): @@ -27,7 +29,13 @@ def write(self, vals): - Commit budget when state changes to purchase - Cancel/Draft document should delete all budget commitment """ - res = super().write(vals) + # The write() below re-runs recompute_budget_move() explicitly, so skip + # the @api.constrains auto-trigger for those states to avoid doing it + # twice (the constrains fires on order_line writes done by super().write). + ctx_self = self + if vals.get("state") in ("purchase", "cancel", "draft"): + ctx_self = self.with_context(_skip_auto_recompute_budget_move=True) + res = super(PurchaseOrder, ctx_self).write(vals) if vals.get("state") in ("purchase", "cancel", "draft"): doclines = self.mapped("order_line") if vals.get("state") in ("cancel", "draft"): diff --git a/budget_control_purchase/tests/test_budget_purchase.py b/budget_control_purchase/tests/test_budget_purchase.py index 9c6f687b..a17c2957 100644 --- a/budget_control_purchase/tests/test_budget_purchase.py +++ b/budget_control_purchase/tests/test_budget_purchase.py @@ -269,6 +269,7 @@ def test_03_budget_recompute_and_close_budget_move(self): # PO Commit = 700, INV Actual = 0 self.assertAlmostEqual(self.budget_control.amount_purchase, 700.0) self.assertAlmostEqual(self.budget_control.amount_actual, 0.0) + # Create and post invoice purchase.action_create_invoice() self.assertEqual(purchase.invoice_status, "invoiced") @@ -299,3 +300,40 @@ def test_03_budget_recompute_and_close_budget_move(self): self.budget_control.invalidate_recordset() self.assertAlmostEqual(self.budget_control.amount_purchase, 0.0) self.assertAlmostEqual(self.budget_control.amount_actual, 0.0) + + @freeze_time("2001-02-01") + def test_04_confirmed_line_edit_and_manual_recompute(self): + """State-write guard must not suppress later intentional recomputes.""" + self.budget_control.action_submit() + self.budget_control.action_done() + + analytic_distribution = {str(self.costcenter1.id): 100} + purchase = self._create_purchase( + [ + { + "product_id": self.product1, + "product_qty": 2, + "price_unit": 100, + "analytic_distribution": analytic_distribution, + } + ] + ).with_context(force_date_commit=datetime.today()) + purchase.button_confirm() + self.assertAlmostEqual(self.budget_control.amount_purchase, 200.0) + + # Editing an approved PO through its order_line commands must trigger + # the constraint normally; the skip flag is set only on state writes. + purchase.write( + { + "order_line": [ + Command.update(purchase.order_line.id, {"price_unit": 150}) + ] + } + ) + self.assertAlmostEqual(self.budget_control.amount_purchase, 300.0) + + # The object button invokes this method without the internal skip + # context and must always rebuild the commitment from current values. + purchase.order_line.write({"price_unit": 175}) + purchase.recompute_budget_move() + self.assertAlmostEqual(self.budget_control.amount_purchase, 350.0) diff --git a/budget_control_purchase_request/models/purchase_request.py b/budget_control_purchase_request/models/purchase_request.py index fdf81bd6..2492ba81 100644 --- a/budget_control_purchase_request/models/purchase_request.py +++ b/budget_control_purchase_request/models/purchase_request.py @@ -18,6 +18,8 @@ class PurchaseRequest(models.Model): # Allow trigger, because purchase request line is editable even when approved. @api.constrains("line_ids") def recompute_budget_move(self): + if self.env.context.get("_skip_auto_recompute_budget_move"): + return self.mapped("line_ids").recompute_budget_move() def close_budget_move(self): @@ -28,7 +30,13 @@ def write(self, vals): - Commit budget when state changes to approved - Cancel/Draft document should delete all budget commitment """ - res = super().write(vals) + # The write() below re-runs recompute_budget_move() explicitly, so skip + # the @api.constrains auto-trigger for those states to avoid doing it + # twice (the constrains fires on line_ids writes done by super().write). + ctx_self = self + if vals.get("state") in ("approved", "rejected", "draft"): + ctx_self = self.with_context(_skip_auto_recompute_budget_move=True) + res = super(PurchaseRequest, ctx_self).write(vals) if vals.get("state") in ("approved", "rejected", "draft"): doclines = self.mapped("line_ids") if vals.get("state") in ("rejected", "draft"): diff --git a/budget_plan_detail/models/budget_period.py b/budget_plan_detail/models/budget_period.py index a2289817..fb2a5ec9 100644 --- a/budget_plan_detail/models/budget_period.py +++ b/budget_plan_detail/models/budget_period.py @@ -7,6 +7,11 @@ class BudgetPeriod(models.Model): _inherit = "budget.period" + def _can_use_budget_available_cache(self, template_lines): + if template_lines and template_lines._name == "budget.plan.line.detail": + return False + return super()._can_use_budget_available_cache(template_lines) + def _get_where_domain(self, analytic_id, template_lines): if template_lines._name == "budget.plan.line.detail": unique_fund_ids = template_lines.mapped("fund_id")