From 06d1ee52908d7cb81958b54b41f064969fbf126a Mon Sep 17 00:00:00 2001 From: Don Kendall Date: Sat, 21 Mar 2026 21:58:13 -0400 Subject: [PATCH 1/3] [18.0][FIX] mis_builder: guard against falsy drilldown action When a user clicks a computed KPI cell (e.g. ``profit - loss``), ``mis_report_instance.drilldown()`` returns ``False`` because the expression has no account variable. The JS handler passed this falsy value to ``doAction()`` which threw: No view found for act_window action undefined Add a truthy check before calling ``doAction()``. Closes #776 Co-Authored-By: Claude Opus 4.6 (1M context) --- mis_builder/static/src/components/mis_report_widget.esm.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/mis_builder/static/src/components/mis_report_widget.esm.js b/mis_builder/static/src/components/mis_report_widget.esm.js index c00d6177a..fff21988e 100644 --- a/mis_builder/static/src/components/mis_report_widget.esm.js +++ b/mis_builder/static/src/components/mis_report_widget.esm.js @@ -137,7 +137,9 @@ export class MisReportWidget extends Component { [this._instanceId(), drilldown], {context: this.context} ); - this.action.doAction(action); + if (action) { + this.action.doAction(action); + } } async refresh() { From d3e8b503ae53ba4668ebabe2a308838c23f3ba98 Mon Sep 17 00:00:00 2001 From: Don Kendall Date: Mon, 23 Mar 2026 10:01:45 -0400 Subject: [PATCH 2/3] [18.0][IMP] mis_builder: enable drilldown on computed KPIs When a computed KPI references other KPIs with account variables (e.g. `expenses + equip`), the cell is now clickable and drills down to the combined journal entries from all referenced KPIs. Changes: - mis_report.py: detect computed KPIs referencing account-var KPIs and set drilldown_arg so the cell becomes clickable in the widget - mis_report_instance.py: drilldown() resolves referenced KPI expressions and combines their AML domains with OR - mis_report_widget.esm.js: use event.currentTarget for reliable dataset access (fixes undefined drilldown data when clicking child elements inside the anchor) - Tests for computed KPI drilldown, falsy args, and matrix output Co-Authored-By: Claude Opus 4.6 (1M context) --- mis_builder/models/mis_report.py | 36 ++++++++-- mis_builder/models/mis_report_instance.py | 70 ++++++++++++++++++- .../src/components/mis_report_widget.esm.js | 3 +- mis_builder/tests/test_mis_report_instance.py | 64 +++++++++++++++++ 4 files changed, 165 insertions(+), 8 deletions(-) diff --git a/mis_builder/models/mis_report.py b/mis_builder/models/mis_report.py index c4442f239..288a5e944 100644 --- a/mis_builder/models/mis_report.py +++ b/mis_builder/models/mis_report.py @@ -642,6 +642,26 @@ def _fetch_queries(self, date_from, date_to, get_additional_query_filter=None): res[query.name] = s return res + def _get_computed_drilldown_arg(self, expr, col_key, kpi): + """Return a drilldown_arg for a computed KPI if it references + other KPIs that have account variables. + + This enables drilldown on summary rows like ``expenses + equip`` + by combining the journal entry domains of the referenced KPIs. + """ + referenced_kpis = self.kpi_ids.filtered( + lambda k: k.name in expr and k.id != kpi.id + ) + has_account_child = any( + AEP.has_account_var(e.name) + for k in referenced_kpis + for e in k.expression_ids + if e.name + ) + if has_account_child: + return {"expr": expr, "period_id": col_key, "kpi_id": kpi.id} + return None + def _declare_and_compute_col( # noqa: C901 (TODO simplify this fnction) self, expression_evaluator, @@ -688,11 +708,19 @@ def _declare_and_compute_col( # noqa: C901 (TODO simplify this fnction) drilldown_args, name_error, ) = expression_evaluator.eval_expressions(expressions, locals_dict) - for drilldown_arg in drilldown_args: - if not drilldown_arg: + for i, drilldown_arg in enumerate(drilldown_args): + if drilldown_arg: + drilldown_arg["period_id"] = col_key + drilldown_arg["kpi_id"] = kpi.id continue - drilldown_arg["period_id"] = col_key - drilldown_arg["kpi_id"] = kpi.id + # For computed KPIs without account vars, check if the + # expression references other KPIs that have account vars + # so we can enable drilldown on the computed total. + expr = expressions[i] and expressions[i].name + if expr and not name_error: + drilldown_args[i] = self._get_computed_drilldown_arg( + expr, col_key, kpi + ) if name_error: recompute_queue.append(kpi) diff --git a/mis_builder/models/mis_report_instance.py b/mis_builder/models/mis_report_instance.py index 9acf6fe63..e15e4f60d 100644 --- a/mis_builder/models/mis_report_instance.py +++ b/mis_builder/models/mis_report_instance.py @@ -967,8 +967,10 @@ def drilldown(self, arg): period_id = arg.get("period_id") expr = arg.get("expr") account_id = arg.get("account_id") - if period_id and expr and AEP.has_account_var(expr): - period = self.env["mis.report.instance.period"].browse(period_id) + if not (period_id and expr): + return False + period = self.env["mis.report.instance.period"].browse(period_id) + if AEP.has_account_var(expr): aep = AEP( self.query_company_ids, self.currency_id, self.report_id.account_model ) @@ -992,8 +994,70 @@ def drilldown(self, arg): "target": "current", "context": {"active_test": False}, } + # For computed KPIs, resolve referenced KPI expressions and combine + domain = self._get_computed_kpi_drilldown_domain(expr, period, account_id) + if domain is not None: + views = self._get_drilldown_model_views(period.source_aml_model_name) + return { + "name": self._get_drilldown_action_name(arg), + "domain": domain, + "type": "ir.actions.act_window", + "res_model": period.source_aml_model_name, + "views": [[False, view] for view in views], + "view_mode": ",".join(view for view in views), + "target": "current", + "context": {"active_test": False}, + } + return False + + def _get_computed_kpi_drilldown_domain(self, expr, period, account_id): + """Build a combined AML domain for computed KPIs. + + When a KPI expression references other KPIs (e.g. ``expenses + + equip``), resolve each referenced KPI to its account expression and + combine the resulting domains with OR so the user sees all journal + entries that contribute to the computed value. + """ + report = self.report_id + kpi_by_name = {kpi.name: kpi for kpi in report.kpi_ids} + # Collect account-var expressions from referenced KPIs + account_exprs = [] + for kpi_name, kpi in kpi_by_name.items(): + if kpi_name not in expr: + continue + for kpi_expr in kpi.expression_ids: + if kpi_expr.name and AEP.has_account_var(kpi_expr.name): + account_exprs.append(kpi_expr.name) + if not account_exprs: + return None + # Build individual domains and combine with OR + domains = [] + for acct_expr in account_exprs: + aep = AEP( + self.query_company_ids, + self.currency_id, + report.account_model, + ) + aep.parse_expr(acct_expr) + aep.done_parsing() + domain = aep.get_aml_domain_for_expr( + acct_expr, + period.date_from, + period.date_to, + account_id, + ) + domains.append(domain) + if len(domains) == 1: + combined = domains[0] else: - return False + # Combine with OR: ['|', domain1, '|', domain2, domain3] + combined = [] + for i, domain in enumerate(domains): + if i < len(domains) - 1: + combined.append("|") + combined.extend(domain) + combined.extend(period._get_additional_move_line_filter()) + return combined def _get_drilldown_action_name(self, arg): kpi_id = arg.get("kpi_id") diff --git a/mis_builder/static/src/components/mis_report_widget.esm.js b/mis_builder/static/src/components/mis_report_widget.esm.js index fff21988e..5ad6cd477 100644 --- a/mis_builder/static/src/components/mis_report_widget.esm.js +++ b/mis_builder/static/src/components/mis_report_widget.esm.js @@ -130,7 +130,8 @@ export class MisReportWidget extends Component { } async drilldown(event) { - const drilldown = JSON.parse(event.target.dataset.drilldown); + const el = event.currentTarget || event.target; + const drilldown = JSON.parse(el.dataset.drilldown); const action = await this.orm.call( "mis.report.instance", "drilldown", diff --git a/mis_builder/tests/test_mis_report_instance.py b/mis_builder/tests/test_mis_report_instance.py index ba8d83aed..e73a8105a 100644 --- a/mis_builder/tests/test_mis_report_instance.py +++ b/mis_builder/tests/test_mis_report_instance.py @@ -490,6 +490,70 @@ def test_drilldown_views(self): [[False, "list"], [False, "form"], [False, "pivot"], [False, "graph"]], ) + def test_drilldown_computed_kpi(self): + """Computed KPIs that reference account-var KPIs should be drillable.""" + # k4 = k1 + k2 + k3 ; k1 and k2 have balp[200%] expressions + k4 = self.env["mis.report.kpi"].search( + [("report_id", "=", self.report.id), ("name", "=", "k4")] + ) + period = self.report_instance.period_ids[0] + action = self.report_instance.drilldown( + {"expr": "k1 + k2 + k3", "period_id": period.id, "kpi_id": k4.id} + ) + self.assertTrue(action, "Computed KPI referencing account KPIs should drilldown") + self.assertEqual(action["type"], "ir.actions.act_window") + self.assertEqual(action["res_model"], "account.move.line") + # Domain should contain account_id filters from both k1 and k2 + domain_str = str(action["domain"]) + self.assertIn("account_id", domain_str) + + def test_drilldown_computed_kpi_no_account_refs(self): + """Computed KPIs with no account-var references should return False.""" + # Create a KPI that only references constants + kpi_const = self.env["mis.report.kpi"].create( + { + "report_id": self.report.id, + "description": "constant total", + "name": "k_const_total", + "multi": False, + "expression": "k3", + } + ) + period = self.report_instance.period_ids[0] + action = self.report_instance.drilldown( + {"expr": "k3", "period_id": period.id, "kpi_id": kpi_const.id} + ) + self.assertFalse( + action, "Computed KPI with no account refs should not drilldown" + ) + + def test_drilldown_falsy_args(self): + """Drilldown with missing period or expr should return False.""" + self.assertFalse(self.report_instance.drilldown({})) + self.assertFalse(self.report_instance.drilldown({"expr": "balp[200%]"})) + self.assertFalse( + self.report_instance.drilldown( + {"period_id": self.report_instance.period_ids[0].id} + ) + ) + + def test_computed_kpi_clickable_in_matrix(self): + """Computed KPIs referencing account KPIs should have drilldown_arg.""" + matrix = self.report_instance.compute() + # Find the k4 row (computed: k1 + k2 + k3) + k4_row = None + for row in matrix.get("body", []): + if row.get("kpi_name") == "k4": + k4_row = row + break + self.assertTrue(k4_row, "k4 row should be in the report body") + # At least one cell should have drilldown_arg since k1/k2 have account vars + has_drilldown = any("drilldown_arg" in cell for cell in k4_row.get("cells", [])) + self.assertTrue( + has_drilldown, + "Computed KPI k4 should be clickable since it references k1/k2", + ) + def test_qweb(self): self.report_instance.print_pdf() # get action test_reports.try_report( From 59409a116c3a3a3ad2f0a5b01dc1fb6edd0f2a55 Mon Sep 17 00:00:00 2001 From: Don Kendall Date: Mon, 23 Mar 2026 11:17:04 -0400 Subject: [PATCH 3/3] fix: ruff format + fix matrix test to use row label key --- mis_builder/models/mis_report.py | 2 +- mis_builder/models/mis_report_instance.py | 17 ++++++---------- mis_builder/tests/test_mis_report_instance.py | 20 +++++++++++-------- 3 files changed, 19 insertions(+), 20 deletions(-) diff --git a/mis_builder/models/mis_report.py b/mis_builder/models/mis_report.py index 288a5e944..6711c9148 100644 --- a/mis_builder/models/mis_report.py +++ b/mis_builder/models/mis_report.py @@ -619,7 +619,7 @@ def _fetch_queries(self, date_from, date_to, get_additional_query_filter=None): v = data[0][field_name] except KeyError: _logger.error( - "field %s not found in read_group " "for %s; not summable?", + "field %s not found in read_group for %s; not summable?", field_name, model._name, ) diff --git a/mis_builder/models/mis_report_instance.py b/mis_builder/models/mis_report_instance.py index e15e4f60d..1e3f06534 100644 --- a/mis_builder/models/mis_report_instance.py +++ b/mis_builder/models/mis_report_instance.py @@ -423,8 +423,7 @@ def _check_mode_source(self): if rec.mode == MODE_NONE: raise DateFilterRequired( self.env._( - "A date filter is mandatory for this source " - "in column %s.", + "A date filter is mandatory for this source in column %s.", rec.name, ) ) @@ -432,8 +431,7 @@ def _check_mode_source(self): if rec.mode != MODE_NONE: raise DateFilterForbidden( self.env._( - "No date filter is allowed for this source " - "in column %s.", + "No date filter is allowed for this source in column %s.", rec.name, ) ) @@ -460,8 +458,7 @@ def _check_source_cmpcol(self): ): raise ValidationError( self.env._( - "Columns to compare must belong to the same report " - "in %s", + "Columns to compare must belong to the same report in %s", rec.name, ) ) @@ -499,7 +496,7 @@ def _compute_pivot_date(self): sequence = fields.Integer(default=10) description = fields.Char(related="report_id.description") date = fields.Date( - string="Base date", help="Report base date " "(leave empty to use current date)" + string="Base date", help="Report base date (leave empty to use current date)" ) pivot_date = fields.Date(compute="_compute_pivot_date") report_id = fields.Many2one("mis.report", required=True, string="Report") @@ -765,9 +762,7 @@ def get_views(self, views, options=None): context.get("from_dashboard") and context.get("active_model") == "mis.report.instance" ): - view_id = self.env.ref( - "mis_builder." "mis_report_instance_result_view_form" - ) + view_id = self.env.ref("mis_builder.mis_report_instance_result_view_form") mis_report_form_view = view_id and [view_id.id, "form"] for view in views: if view and view[1] == "form": @@ -778,7 +773,7 @@ def get_views(self, views, options=None): def preview(self): self.ensure_one() - view_id = self.env.ref("mis_builder." "mis_report_instance_result_view_form") + view_id = self.env.ref("mis_builder.mis_report_instance_result_view_form") return { "type": "ir.actions.act_window", "res_model": "mis.report.instance", diff --git a/mis_builder/tests/test_mis_report_instance.py b/mis_builder/tests/test_mis_report_instance.py index e73a8105a..c3f7614d7 100644 --- a/mis_builder/tests/test_mis_report_instance.py +++ b/mis_builder/tests/test_mis_report_instance.py @@ -500,7 +500,10 @@ def test_drilldown_computed_kpi(self): action = self.report_instance.drilldown( {"expr": "k1 + k2 + k3", "period_id": period.id, "kpi_id": k4.id} ) - self.assertTrue(action, "Computed KPI referencing account KPIs should drilldown") + self.assertTrue( + action, + "Computed KPI referencing account KPIs should drilldown", + ) self.assertEqual(action["type"], "ir.actions.act_window") self.assertEqual(action["res_model"], "account.move.line") # Domain should contain account_id filters from both k1 and k2 @@ -538,20 +541,21 @@ def test_drilldown_falsy_args(self): ) def test_computed_kpi_clickable_in_matrix(self): - """Computed KPIs referencing account KPIs should have drilldown_arg.""" + """Computed KPIs referencing account KPIs should have + drilldown_arg.""" matrix = self.report_instance.compute() - # Find the k4 row (computed: k1 + k2 + k3) + # Find the k4 row (description "kpi 4", computed: k1+k2+k3) k4_row = None for row in matrix.get("body", []): - if row.get("kpi_name") == "k4": + if row.get("label") == "kpi 4": k4_row = row break self.assertTrue(k4_row, "k4 row should be in the report body") - # At least one cell should have drilldown_arg since k1/k2 have account vars - has_drilldown = any("drilldown_arg" in cell for cell in k4_row.get("cells", [])) + # At least one cell should have drilldown_arg + has_dd = any("drilldown_arg" in c for c in k4_row.get("cells", [])) self.assertTrue( - has_drilldown, - "Computed KPI k4 should be clickable since it references k1/k2", + has_dd, + "Computed KPI k4 should be clickable", ) def test_qweb(self):