feat: Add "Accumulated Values in Group Company" checkbox to Budget Variance Report (#50608) #50610
Conversation
📝 WalkthroughWalkthroughAdds an "accumulated_in_group_company" filter and frontend logic to show/hide it based on whether the selected company is a group company. Frontend registers onload and on_change handlers to update the filter and refresh the report. Backend adds get_companies_for_report(filters) and, when the accumulated flag is true, iterates companies to merge per-company month-by-account dimension maps into a consolidated cam_map by summing target/actual values and recomputing variances; otherwise existing per-company behavior remains. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
erpnext/accounts/report/budget_variance_report/budget_variance_report.js (2)
21-45: Add on_change handler for company filter.The
onloadhandler checks the company'sis_groupstatus only once when the report loads. If the user changes the company filter after the report has loaded, the visibility of theaccumulated_in_group_companycheckbox won't update dynamically.Consider adding an
on_changehandler to the company filter definition:{ fieldname: "company", label: __("Company"), fieldtype: "Link", options: "Company", default: frappe.defaults.get_user_default("Company"), reqd: 1, on_change: function () { let company = frappe.query_report.get_filter_value("company"); if (!company) return; frappe.call({ method: "frappe.client.get_value", args: { doctype: "Company", filters: { name: company }, fieldname: "is_group", }, callback: function(r) { const filter = frappe.query_report.get_filter("accumulated_in_group_company"); if (!filter) return; if (r.message && r.message.is_group) { filter.df.hidden = 0; } else { filter.df.hidden = 1; frappe.query_report.set_filter_value("accumulated_in_group_company", 0); } filter.refresh(); }, }); }, },
137-143: Fix indentation inconsistency.Lines 137-143 use spaces for indentation, while the rest of the file uses tabs. This inconsistency should be corrected for code uniformity.
Apply this diff to use tabs consistently:
- { - fieldname: "accumulated_in_group_company", - label: __("Accumulated Values in Group Company"), - fieldtype: "Check", - default: 0, - hidden: 1, - }, + { + fieldname: "accumulated_in_group_company", + label: __("Accumulated Values in Group Company"), + fieldtype: "Check", + default: 0, + hidden: 1, + },
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/accounts/report/budget_variance_report/budget_variance_report.js(2 hunks)erpnext/accounts/report/budget_variance_report/budget_variance_report.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
erpnext/accounts/report/budget_variance_report/budget_variance_report.py (1)
erpnext/accounts/report/budget_variance_report/budget_variance_report.js (2)
filters(67-144)company(22-22)
🪛 Ruff (0.14.5)
erpnext/accounts/report/budget_variance_report/budget_variance_report.py
48-48: Unexpected indentation
(invalid-syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (1)
erpnext/accounts/report/budget_variance_report/budget_variance_report.py (1)
13-31: LGTM! Clean implementation for company retrieval.The function correctly handles both single-company and consolidated modes, retrieving child companies and ensuring the parent is always included.
erpnext/accounts/report/budget_variance_report/budget_variance_report.js
Outdated
Show resolved
Hide resolved
erpnext/accounts/report/budget_variance_report/budget_variance_report.py
Outdated
Show resolved
Hide resolved
erpnext/accounts/report/budget_variance_report/budget_variance_report.py
Outdated
Show resolved
Hide resolved
erpnext/accounts/report/budget_variance_report/budget_variance_report.py
Show resolved
Hide resolved
… consolidation fixes
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
erpnext/accounts/report/budget_variance_report/budget_variance_report.py (2)
13-28: Scope ofget_companies_for_reportmay miss deeper company treesThe helper returns the selected company plus its direct children (
parent_company = company). If group structures can be more than one level deep (group → sub‑group → operating company), those deeper descendants won’t be included in consolidation.If you expect multi‑level hierarchies, consider reusing an existing helper that returns all descendant companies (or iteratively traversing
parent_company) so the consolidated view truly covers the full group structure. Otherwise, documenting that only direct children are included would avoid surprises.
47-83: Minor: avoid redundantvariancecomputation in the consolidated mapInside the consolidation loop, you recompute and store
["variance"]for each month (Lines 75-78), butget_final_datalater derives variance asbudget - actualand overwritestotals[2]accordingly. The stored variance values incam_mapare effectively unused for the final numbers.You can simplify and slightly reduce work by only aggregating
"target"and"actual", skipping the"variance"assignment in this block. That keeps the data structure cleaner and avoids confusion about which variance is “authoritative”.erpnext/accounts/report/budget_variance_report/budget_variance_report.js (1)
103-112: Ensure companyon_changestill refreshes the reportThe new
on_changehandler for thecompanyfilter now only callsupdate_group_company_checkbox(company). Depending on Frappe’s default behavior, adding this handler may override any implicit auto-refresh that previously occurred when the company changed.Please confirm whether query reports still refresh automatically when a filter has a custom
on_change. If not, consider explicitly refreshing:{ fieldname: "company", @@ - on_change: function () { - let company = frappe.query_report.get_filter_value("company"); - if (company) update_group_company_checkbox(company); - }, + on_change: function () { + const company = frappe.query_report.get_filter_value("company"); + if (company) { + update_group_company_checkbox(company); + } + frappe.query_report.refresh(); + },(Optionally also hide/reset the checkbox when
companybecomes empty, for extra safety.)
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/accounts/report/budget_variance_report/budget_variance_report.js(3 hunks)erpnext/accounts/report/budget_variance_report/budget_variance_report.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: vorasmit
Repo: frappe/erpnext PR: 49098
File: erpnext/accounts/doctype/financial_report_row/financial_report_row.json:118-124
Timestamp: 2025-11-14T04:52:35.353Z
Learning: In ERPNext Financial Report Row (erpnext/accounts/doctype/financial_report_row), when data_source is "Account Data" and advanced_filtering is false, the filters_editor field provides a UI that automatically populates the calculation_formula field behind the scenes. The calculation_formula field can be hidden but mandatory in this case because it gets set automatically before validation occurs.
📚 Learning: 2025-11-14T04:52:35.353Z
Learnt from: vorasmit
Repo: frappe/erpnext PR: 49098
File: erpnext/accounts/doctype/financial_report_row/financial_report_row.json:118-124
Timestamp: 2025-11-14T04:52:35.353Z
Learning: In ERPNext Financial Report Row (erpnext/accounts/doctype/financial_report_row), when data_source is "Account Data" and advanced_filtering is false, the filters_editor field provides a UI that automatically populates the calculation_formula field behind the scenes. The calculation_formula field can be hidden but mandatory in this case because it gets set automatically before validation occurs.
Applied to files:
erpnext/accounts/report/budget_variance_report/budget_variance_report.js
📚 Learning: 2025-10-14T04:57:50.118Z
Learnt from: vorasmit
Repo: frappe/erpnext PR: 49098
File: erpnext/accounts/financial_report_template/horizontal_profit_and_loss_(columnar)/horizontal_profit_and_loss_(columnar).json:212-225
Timestamp: 2025-10-14T04:57:50.118Z
Learning: In ERPNext Financial Report Template calculation formulas for filtering service-related accounts, use `account_name` field with a "like" operator (e.g., `account_name like "Service"`) rather than `account_type`, as the backend automatically adds SQL wildcards during evaluation.
Applied to files:
erpnext/accounts/report/budget_variance_report/budget_variance_report.js
🧬 Code graph analysis (1)
erpnext/accounts/report/budget_variance_report/budget_variance_report.py (2)
erpnext/accounts/report/budget_variance_report/budget_variance_report.js (3)
filters(72-151)company(23-23)company(110-110)erpnext/controllers/trends.py (1)
get_period_month_ranges(330-342)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (3)
erpnext/accounts/report/budget_variance_report/budget_variance_report.js (3)
7-15: Formatter null-guards look goodAdding checks for
column.fieldnameanddatabefore testing variance columns hardens the formatter against missing metadata/rows without changing behavior for valid data. Negative and positive variances will still be color‑coded as before.
22-50: Group-company checkbox toggling is robustThe
onload+update_group_company_checkboxflow correctly:
- Reads the current
companyfilter.- Fetches
is_groupviafrappe.client.get_value.- Shows the
accumulated_in_group_companyfilter only for group companies and hides + resets it to0otherwise.This keeps the server-side flag in sync with the selected company and avoids stale consolidated-state leakage when switching between group and non-group companies.
144-150: Newaccumulated_in_group_companyfilter wiring matches backendThe added
accumulated_in_group_companyCheck filter (hidden by default) aligns with the Python flag of the same name and is correctly controlled byupdate_group_company_checkbox. Naming and behavior are consistent with the server-side consolidation logic.
| columns = get_columns(filters) | ||
|
|
||
| if filters.get("budget_against_filter"): | ||
| dimensions = filters.get("budget_against_filter") | ||
| else: | ||
| dimensions = get_cost_centers(filters) | ||
|
|
||
| period_month_ranges = get_period_month_ranges(filters["period"], filters["from_fiscal_year"]) | ||
| cam_map = get_dimension_account_month_map(filters) | ||
| period_month_ranges = get_period_month_ranges( | ||
| filters["period"], filters["from_fiscal_year"] | ||
| ) |
There was a problem hiding this comment.
Consolidated cam_map can drop child-company dimensions when no explicit filter is set
In execute:
dimensionsis initialized once (Lines 38-41) using eitherbudget_against_filterorget_cost_centers(filters), i.e., cost centers/projects for the initial company.- In consolidated mode,
cam_mapis then built by iteratingcompanies(Lines 47-83) and mergingget_dimension_account_month_map(filters)for each company. - Later, you iterate
for dimension in dimensions:and accesscam_map.get(dimension)(Lines 85-92).
If accumulated_in_group_company is enabled and no budget_against_filter is provided, dimensions only contains entries from the original company, but cam_map can also contain dimensions from child companies. Any dimension that exists only in a child company will never be visited and its data will be silently omitted from the consolidated report.
Concrete effect: consolidated totals will under‑report amounts for cost centers/projects that exist solely in child companies unless the user explicitly includes them in budget_against_filter.
A minimal fix, keeping existing semantics when a filter is set, would be:
# consolidate dimension_account_month map across companies
companies = get_companies_for_report(filters)
if filters.get("accumulated_in_group_company"):
@@
- filters["company"] = original_company
- cam_map = consolidated_cam_map
+ filters["company"] = original_company
+ cam_map = consolidated_cam_map
+
+ # in consolidated mode without an explicit dimension filter,
+ # derive the dimension list from the consolidated map so
+ # child-company dimensions are not dropped
+ if not filters.get("budget_against_filter"):
+ dimensions = list(cam_map.keys())
else:
cam_map = get_dimension_account_month_map(filters)You can further sort dimensions if ordering matters. This keeps filtered behavior unchanged while ensuring consolidated mode includes all dimensions that have data.
Also applies to: 47-83, 85-97
🤖 Prompt for AI Agents
In erpnext/accounts/report/budget_variance_report/budget_variance_report.py
around lines 36-45 (and related logic in 47-83, 85-97), dimensions are built
once from the initial company which omits dimensions that exist only in child
companies when consolidated; change the logic so that if
filters.get("budget_against_filter") is present you keep current behavior, but
when it is not present you collect dimensions from all companies by taking the
union of keys from each company's get_dimension_account_month_map (the cam_map
entries) after building/merging cam_map — then set dimensions to that union
(optionally sorted) so every dimension with data in any company is iterated and
included in the consolidated totals.
1 participant
Description
This pull request adds the "Accumulated Values in Group Company" feature to the Budget Variance Report, enabling consolidated reporting across parent and child companies similar to the Consolidated Financial Statement reports.
Changes Made
accumulated_values_in_group_companyinbudget_variance_report.js.budget_variance_report.pyto aggregate actual, budget, and variance values across child companies.Problem Solved
Previously, the Budget Variance Report only displayed data for a single company.
With this feature:
Testing & Validation
Screenshots / GIFs
(Include before/after screenshots or GIFs showing the checkbox and consolidated output)
Issue Reference
Closes #50608