feat(accounts): support multi-company financial report templates#53992
feat(accounts): support multi-company financial report templates#53992barredterra wants to merge 1 commit intofrappe:developfrom
Conversation
📝 WalkthroughWalkthroughThis PR implements multi-company support for financial report templates. Three new public functions— Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Detailed notesThe core refactoring of 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (3)
erpnext/accounts/doctype/financial_report_template/financial_report_engine.py (1)
230-248: Consider handling missing company records gracefully.If a company in
report_companiesdoesn't exist,frappe.get_cached_valuereturnsNone, which would be added to thedefault_currenciesset. This could result in a set like{None, "INR"}with length 2, triggering a misleading "same default currency" error instead of a "company not found" error.♻️ Proposed fix to filter out None values
def get_report_currency(filters: dict[str, Any]) -> str | None: report_companies = get_report_companies(filters) if not report_companies: return None default_currencies = { - frappe.get_cached_value("Company", company, "default_currency") for company in report_companies + currency + for company in report_companies + if (currency := frappe.get_cached_value("Company", company, "default_currency")) } if len(default_currencies) > 1:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/doctype/financial_report_template/financial_report_engine.py` around lines 230 - 248, The get_report_currency function builds default_currencies using frappe.get_cached_value which can return None for missing Company records; filter out None results from the set and if any None were present, raise a clear error listing the missing company identifiers (or a message like "Company not found") instead of letting a mixed set produce the misleading currency mismatch error; update get_report_currency to collect results from frappe.get_cached_value("Company", company, "default_currency"), track which companies returned None (from report_companies), remove None before checking len(default_currencies), and if any missing companies exist raise frappe.throw with a descriptive message referencing the affected company identifiers.erpnext/accounts/doctype/financial_report_template/test_financial_report_multi_company.py (2)
66-70: Minor: PotentialIndexErrorif no cost center exists.The helper assumes at least one non-group cost center exists for the company. While this is typically true for newly created companies, consider adding a guard or assertion for clearer test failure messages.
♻️ Optional defensive improvement
`@staticmethod` def _get_cost_center(company: str) -> str: - return frappe.get_all( + cost_centers = frappe.get_all( "Cost Center", filters={"company": company, "is_group": 0}, pluck="name", order_by="lft", limit=1 - )[0] + ) + if not cost_centers: + raise AssertionError(f"No cost center found for company {company}") + return cost_centers[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/doctype/financial_report_template/test_financial_report_multi_company.py` around lines 66 - 70, The helper _get_cost_center currently indexes the result of frappe.get_all directly which can raise IndexError if no non-group cost center exists; update _get_cost_center to check the returned list from frappe.get_all (called in _get_cost_center) and either assert/raise a clear error like "No non-group Cost Center found for company X" or create a fallback cost center for the given company so tests fail with a descriptive message instead of an IndexError.
161-233: Test template is not cleaned up after test execution.The dynamically created template (line 207) is not deleted in the
finallyblock. While the unique hash name prevents conflicts, this accumulates test data over repeated runs.♻️ Proposed fix to clean up template
try: journal_entries.append(self._make_journal_entry(company_a, asset_a, income_a, 200, "2027-04-05")) # ... rest of test finally: for journal_entry in reversed(journal_entries): journal_entry.cancel() + if template: + template.delete()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/doctype/financial_report_template/test_financial_report_multi_company.py` around lines 161 - 233, The created report template (variable template from create_test_template_with_rows) isn't removed after the test; update the finally block to also delete the template when present (e.g., if template: call template.delete()/frappe.delete_doc for the Financial Report Template) after cancelling journal_entries to avoid accumulating test fixtures; reference the template variable and create_test_template_with_rows to locate where to add the cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@erpnext/accounts/doctype/financial_report_template/financial_report_engine.py`:
- Around line 230-248: The get_report_currency function builds
default_currencies using frappe.get_cached_value which can return None for
missing Company records; filter out None results from the set and if any None
were present, raise a clear error listing the missing company identifiers (or a
message like "Company not found") instead of letting a mixed set produce the
misleading currency mismatch error; update get_report_currency to collect
results from frappe.get_cached_value("Company", company, "default_currency"),
track which companies returned None (from report_companies), remove None before
checking len(default_currencies), and if any missing companies exist raise
frappe.throw with a descriptive message referencing the affected company
identifiers.
In
`@erpnext/accounts/doctype/financial_report_template/test_financial_report_multi_company.py`:
- Around line 66-70: The helper _get_cost_center currently indexes the result of
frappe.get_all directly which can raise IndexError if no non-group cost center
exists; update _get_cost_center to check the returned list from frappe.get_all
(called in _get_cost_center) and either assert/raise a clear error like "No
non-group Cost Center found for company X" or create a fallback cost center for
the given company so tests fail with a descriptive message instead of an
IndexError.
- Around line 161-233: The created report template (variable template from
create_test_template_with_rows) isn't removed after the test; update the finally
block to also delete the template when present (e.g., if template: call
template.delete()/frappe.delete_doc for the Financial Report Template) after
cancelling journal_entries to avoid accumulating test fixtures; reference the
template variable and create_test_template_with_rows to locate where to add the
cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: b2f9c2e5-3eb9-4b39-b854-63ccf4189ad4
📒 Files selected for processing (6)
erpnext/accounts/doctype/financial_report_template/financial_report_engine.pyerpnext/accounts/doctype/financial_report_template/test_financial_report_multi_company.pyerpnext/accounts/report/balance_sheet/balance_sheet.jserpnext/accounts/report/custom_financial_statement/custom_financial_statement.jserpnext/accounts/report/profit_and_loss_statement/profit_and_loss_statement.jserpnext/public/js/financial_statements.js
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #53992 +/- ##
===========================================
+ Coverage 79.05% 79.27% +0.21%
===========================================
Files 1159 1160 +1
Lines 124773 124934 +161
===========================================
+ Hits 98644 99045 +401
+ Misses 26129 25889 -240
🚀 New features to boost your workflow:
|
1 participant
Summary
This change unblocks consolidated reporting use cases for template-backed financial statements, especially intercompany elimination setups where rows from one company are intended to offset rows from another.
companiesmulti-select filter for template-backed Balance Sheet, Profit and Loss Statement, Cash Flow, and Custom Financial Statement reportscompanyfilterWhy
Today, custom financial report templates are effectively blocked from consolidated multi-company reporting because the report flow assumes a single company throughout execution.
With this change, users can build consolidated templates where intercompany income and expense rows cancel each other out and are hidden through the existing template row and formula system.
Testing
bench --site alyf-dev run-tests --module erpnext.accounts.doctype.financial_report_template.test_financial_report_multi_company