fix(cash_flow): correct Total Row Calculation and Spacer For add_tota…#53073
Open
AhmedAbokhatwa wants to merge 4 commits intofrappe:developfrom
Open
fix(cash_flow): correct Total Row Calculation and Spacer For add_tota…#53073AhmedAbokhatwa wants to merge 4 commits intofrappe:developfrom
AhmedAbokhatwa wants to merge 4 commits intofrappe:developfrom
Conversation
Contributor
📝 WalkthroughWalkthroughRefactors cash flow generation in erpnext/accounts/report/cash_flow/cash_flow.py: internal control flow and data shapes reflowed, total-row creation now uses Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@erpnext/accounts/report/cash_flow/cash_flow.py`:
- Around line 265-271: The subtotal aggregation in add_total_row_account() is
only running for rows with "parent_account" but the rows generated in this
module use "parent_section"; update the guard in the for-row loop inside
add_total_row_account() to check for "parent_section" (or accept either
"parent_section" or "parent_account") so total_row and summary_data accumulate
the intended rows for each period from period_list when computing totals
(respecting consolidated logic and existing keys like period["key"]).
- Line 309: Replace the direct empty dict appended to out
(out.extend([opening_balance, net_change_in_cash, closing_balance, {}])) with
the same explicit spacer row structure used in add_total_row_account(); locate
the out.extend call in cash_flow.py and either construct the spacer using the
identical keys/shape returned by add_total_row_account() or call that helper to
produce the spacer so the opening/closing path uses the same structured spacer
row instead of {}.
Comment on lines
+265
to
+271
| for row in data: | ||
| if row.get("parent_account"): | ||
| for period in period_list: | ||
| key = period if consolidated else period["key"] | ||
| total_row.setdefault(key, 0.0) | ||
| total_row[key] += row.get(key, 0.0) | ||
| summary_data[label] += row.get(key) |
Contributor
There was a problem hiding this comment.
Fix total aggregation guard key (parent_account vs parent_section).
add_total_row_account() currently aggregates only rows with parent_account, but rows created in this file use parent_section (Line 91, Line 116). This makes subtotal/summary computation skip intended rows.
💡 Proposed fix
- for row in data:
- if row.get("parent_account"):
+ for row in data:
+ if row.get("parent_section"):
for period in period_list:
key = period if consolidated else period["key"]
total_row.setdefault(key, 0.0)
total_row[key] += row.get(key, 0.0)
summary_data[label] += row.get(key)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@erpnext/accounts/report/cash_flow/cash_flow.py` around lines 265 - 271, The
subtotal aggregation in add_total_row_account() is only running for rows with
"parent_account" but the rows generated in this module use "parent_section";
update the guard in the for-row loop inside add_total_row_account() to check for
"parent_section" (or accept either "parent_section" or "parent_account") so
total_row and summary_data accumulate the intended rows for each period from
period_list when computing totals (respecting consolidated logic and existing
keys like period["key"]).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
erpnext/accounts/report/cash_flow/cash_flow.py (1)
1-442:⚠️ Potential issue | 🟡 MinorCI is currently blocked by formatting.
Pipeline reports Ruff reformatted this file. Please run formatting locally before merge (
pre-commit run --all-filesorruff format erpnext/accounts/report/cash_flow/cash_flow.py).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/report/cash_flow/cash_flow.py` around lines 1 - 442, The file formatting doesn't match the project's linter (Ruff) and CI is failing; run the formatter locally and re-commit the reformatted file. Specifically, run either "pre-commit run --all-files" or "ruff format erpnext/accounts/report/cash_flow/cash_flow.py" and then stage and push the changes; this touches the module functions such as execute, get_cash_flow_accounts, get_account_type_based_data, and get_chart_data so the diff will be limited to formatting only.
♻️ Duplicate comments (1)
erpnext/accounts/report/cash_flow/cash_flow.py (1)
265-271:⚠️ Potential issue | 🔴 CriticalTotals are skipped due to wrong parent-key guard.
On Line 266, aggregation checks
parent_account, but rows added in this report useparent_section(Line 91, Line 116). This causes section totals to miss expected rows.💡 Proposed fix
- for row in data: - if row.get("parent_account"): + for row in data: + if row.get("parent_section") or row.get("parent_account"): for period in period_list: key = period if consolidated else period["key"] total_row.setdefault(key, 0.0) total_row[key] += row.get(key, 0.0) - summary_data[label] += row.get(key) + summary_data[label] += row.get(key, 0.0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/report/cash_flow/cash_flow.py` around lines 265 - 271, The loop currently skips rows because it checks row.get("parent_account") but the report rows use "parent_section"; change the guard in the aggregation loop (the for row in data block that updates total_row and summary_data) to check row.get("parent_section") (or accept either key: row.get("parent_section") or row.get("parent_account")) so section rows are included when summing period values (retain use of period_list, consolidated, key, total_row and summary_data[label] as before).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@erpnext/accounts/report/cash_flow/cash_flow.py`:
- Around line 277-278: The spacer row appended to out uses spacer =
{"account_name": "", "currency": "", "total": None} but lacks per-period keys,
causing shape mismatch; update the spacer construction (the variable named
spacer before out.append(spacer)) to include each period key set to None
(mirroring the per-period keys used in the later block around lines creating
period keys at lines 309-312) so the spacer has the same structure as other
numeric rows.
---
Outside diff comments:
In `@erpnext/accounts/report/cash_flow/cash_flow.py`:
- Around line 1-442: The file formatting doesn't match the project's linter
(Ruff) and CI is failing; run the formatter locally and re-commit the
reformatted file. Specifically, run either "pre-commit run --all-files" or "ruff
format erpnext/accounts/report/cash_flow/cash_flow.py" and then stage and push
the changes; this touches the module functions such as execute,
get_cash_flow_accounts, get_account_type_based_data, and get_chart_data so the
diff will be limited to formatting only.
---
Duplicate comments:
In `@erpnext/accounts/report/cash_flow/cash_flow.py`:
- Around line 265-271: The loop currently skips rows because it checks
row.get("parent_account") but the report rows use "parent_section"; change the
guard in the aggregation loop (the for row in data block that updates total_row
and summary_data) to check row.get("parent_section") (or accept either key:
row.get("parent_section") or row.get("parent_account")) so section rows are
included when summing period values (retain use of period_list, consolidated,
key, total_row and summary_data[label] as before).
Comment on lines
+277
to
+278
| spacer = {"account_name": "", "currency": "", "total": None} | ||
| out.append(spacer) |
Contributor
There was a problem hiding this comment.
Spacer row is still incomplete for period columns.
Line 277 creates a spacer without per-period keys. In numeric report columns, missing keys can still render as 0.00. Align this spacer shape with Line 309-312 by setting each period key to None.
💡 Proposed fix
- spacer = {"account_name": "", "currency": "", "total": None}
+ spacer = {"account_name": "", "section": "", "currency": "", "total": None}
+ for period in period_list:
+ key = period if consolidated else period["key"]
+ spacer[key] = None
out.append(spacer)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| spacer = {"account_name": "", "currency": "", "total": None} | |
| out.append(spacer) | |
| spacer = {"account_name": "", "section": "", "currency": "", "total": None} | |
| for period in period_list: | |
| key = period if consolidated else period["key"] | |
| spacer[key] = None | |
| out.append(spacer) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@erpnext/accounts/report/cash_flow/cash_flow.py` around lines 277 - 278, The
spacer row appended to out uses spacer = {"account_name": "", "currency": "",
"total": None} but lacks per-period keys, causing shape mismatch; update the
spacer construction (the variable named spacer before out.append(spacer)) to
include each period key set to None (mirroring the per-period keys used in the
later block around lines creating period keys at lines 309-312) so the spacer
has the same structure as other numeric rows.
AhmedAbokhatwa
force-pushed
the
fix-cash-flow
branch
from
d9165e2 to
9227696
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
erpnext/accounts/report/cash_flow/cash_flow.py (2)
277-278:⚠️ Potential issue | 🟡 MinorSpacer row missing period keys may still render as
0.00.The spacer lacks per-period keys. Compare to lines 309-312 where
spacer[period["key"]] = Noneis set for each period. Without these keys, numeric columns may still display0.00.🛠️ Proposed fix - add period keys to spacer
out.append(total_row) - spacer = {"account_name": "", "currency": "", "total": None} + spacer = {"account_name": "", "section": "", "currency": "", "total": None} + for period in period_list: + key = period if consolidated else period["key"] + spacer[key] = None out.append(spacer)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/report/cash_flow/cash_flow.py` around lines 277 - 278, The spacer dict created as spacer = {"account_name": "", "currency": "", "total": None} is missing per-period keys so numeric columns can still render as 0.00; update the spacer before appending by iterating the periods collection (use the same variable used elsewhere, e.g., periods) and set spacer[period["key"]] = None for each period key (mirroring the logic at lines where spacer[period["key"]] is set), then call out.append(spacer) so the spacer contains all period keys.
265-274:⚠️ Potential issue | 🔴 CriticalTotal aggregation is broken:
parent_accountvsparent_sectionmismatch.The guard at line 266 checks for
parent_account, but data rows are created withparent_section(see lines 91 and 116). This means the aggregation loop never finds matching rows, resulting in all totals being0.0.🐛 Proposed fix - align the guard with the data key
for row in data: - if row.get("parent_account"): + if row.get("parent_section"): for period in period_list: key = period if consolidated else period["key"] total_row.setdefault(key, 0.0) total_row[key] += row.get(key, 0.0) summary_data[label] += row.get(key) total_row.setdefault("total", 0.0) total_row["total"] += row["total"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/report/cash_flow/cash_flow.py` around lines 265 - 274, The aggregation loop in the cash flow report is skipping rows because it checks row.get("parent_account") but rows are populated with "parent_section" (see row creation at lines where parent_section is set), so update the guard in the aggregation block (the for row in data loop that updates total_row and summary_data) to check for "parent_section" instead of "parent_account"; also ensure you use row.get(key, 0.0) and safely initialize summary_data[label] before adding to avoid None/KeyError when incrementing totals.
🧹 Nitpick comments (1)
erpnext/accounts/report/cash_flow/cash_flow.py (1)
227-236: Existing SQL construction pattern flagged by static analysis.Ruff flags potential SQL injection (S608) due to string concatenation with
{cond}. The current code usesfrappe.db.escape()for escaping, which provides protection, but this pattern is inherently fragile.This appears to be pre-existing code, not introduced by this PR. Consider migrating to Frappe's query builder in a follow-up for better maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/report/cash_flow/cash_flow.py` around lines 227 - 236, The SQL is built with an f-string that injects {cond} into the query (gl_sum using frappe.db.sql_list), which risks SQL injection; change this to a parameterized query by removing the f-string interpolation and instead attach any dynamic predicates via query placeholders and the filters dict (or by building a safe clause with placeholders and extending filters) so that frappe.db.sql_list(query, filters) receives only a static query string with %(...)s parameters; specifically update the code that constructs gl_sum to stop embedding {cond} directly and instead add the condition terms to filters and reference them with parameter placeholders (or build the clause using safe placeholders) before calling frappe.db.sql_list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@erpnext/accounts/report/cash_flow/cash_flow.py`:
- Around line 253-257: The code is translating the placeholder string
_("{0}").format(label) which is meaningless for translators; replace the
redundant translation wrapper by using the already-translated label directly
when building total_row (e.g., set "account_name" and "section" to the quoted
label value instead of _("{0}").format(label)). Remove the extra _("{0}")
translation call and keep the existing quoting/concatenation logic around the
label variable so total_row uses label (which is already translated via
_("...")).
---
Duplicate comments:
In `@erpnext/accounts/report/cash_flow/cash_flow.py`:
- Around line 277-278: The spacer dict created as spacer = {"account_name": "",
"currency": "", "total": None} is missing per-period keys so numeric columns can
still render as 0.00; update the spacer before appending by iterating the
periods collection (use the same variable used elsewhere, e.g., periods) and set
spacer[period["key"]] = None for each period key (mirroring the logic at lines
where spacer[period["key"]] is set), then call out.append(spacer) so the spacer
contains all period keys.
- Around line 265-274: The aggregation loop in the cash flow report is skipping
rows because it checks row.get("parent_account") but rows are populated with
"parent_section" (see row creation at lines where parent_section is set), so
update the guard in the aggregation block (the for row in data loop that updates
total_row and summary_data) to check for "parent_section" instead of
"parent_account"; also ensure you use row.get(key, 0.0) and safely initialize
summary_data[label] before adding to avoid None/KeyError when incrementing
totals.
---
Nitpick comments:
In `@erpnext/accounts/report/cash_flow/cash_flow.py`:
- Around line 227-236: The SQL is built with an f-string that injects {cond}
into the query (gl_sum using frappe.db.sql_list), which risks SQL injection;
change this to a parameterized query by removing the f-string interpolation and
instead attach any dynamic predicates via query placeholders and the filters
dict (or by building a safe clause with placeholders and extending filters) so
that frappe.db.sql_list(query, filters) receives only a static query string with
%(...)s parameters; specifically update the code that constructs gl_sum to stop
embedding {cond} directly and instead add the condition terms to filters and
reference them with parameter placeholders (or build the clause using safe
placeholders) before calling frappe.db.sql_list.
Comment on lines
253
to
257
| total_row = { | ||
| "account_name": "'" + _("{0}").format(label) + "'", | ||
| "section": "'" + _("{0}").format(label) + "'", | ||
| "currency": currency, | ||
| } |
Contributor
There was a problem hiding this comment.
Fix translation pattern causing pipeline failure.
The pipeline fails because _("{0}").format(label) translates a placeholder string "{0}", which is meaningless to translators. Since label is already a translated string (e.g., _("Net Cash from Operations")), the extra translation wrapper is redundant.
🛠️ Proposed fix
total_row = {
- "account_name": "'" + _("{0}").format(label) + "'",
- "section": "'" + _("{0}").format(label) + "'",
+ "account_name": "'" + label + "'",
+ "section": "'" + label + "'",
"currency": currency,
}🧰 Tools
🪛 GitHub Actions: Linters
[error] 254-254: Blocking code rule fired: frappe-semgrep-rules.rules.frappe-translation-variable-only. Tries to translate variables that are not known to translators. (line 254)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@erpnext/accounts/report/cash_flow/cash_flow.py` around lines 253 - 257, The
code is translating the placeholder string _("{0}").format(label) which is
meaningless for translators; replace the redundant translation wrapper by using
the already-translated label directly when building total_row (e.g., set
"account_name" and "section" to the quoted label value instead of
_("{0}").format(label)). Remove the extra _("{0}") translation call and keep the
existing quoting/concatenation logic around the label variable so total_row uses
label (which is already translated via _("...")).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
erpnext/accounts/report/cash_flow/cash_flow.py (1)
227-238: Consider parameterized query construction.Static analysis flagged potential SQL injection (S608). While
frappe.db.escape()is used for values incond, f-string interpolation of SQL fragments is a maintenance risk. Consider using Frappe's query builder for safer query construction.This is pre-existing code and not a blocker for this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/report/cash_flow/cash_flow.py` around lines 227 - 238, The SQL in the function that computes gl_sum uses f-string interpolation for the {cond} fragment (in the query that selects sum(credit) - sum(debit) from `tabGL Entry`) which risks injection; change the implementation to build the WHERE clause with parameterized filters or Frappe's query builder instead of string interpolation: move any dynamic conditions into the filters dict (or construct a safe list of SQL fragments using frappe.qb or validated/static mappings) and call frappe.db.sql with named parameters (or use frappe.qb.from_('GL Entry').select(...).where(...) with proper bindings) so the query no longer injects raw {cond} via f-strings and uses only bound parameters for company, start_date, end_date and account_type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.pre-commit-config.yaml:
- Line 2: The change setting default_stages: [manual] disables automatic
pre-commit execution and should be reverted or narrowed: either remove the
default_stages line (restoring normal commit-time hooks) or set default_stages
back to include the commit stage (e.g., default_stages: [commit]) and, if you
needed to bypass a specific hook for CI, instead add a per-hook "stages" or
"files"/"exclude" rule for that hook in .pre-commit-config.yaml (or set SKIP for
that specific hook), referencing the default_stages key and any specific hook
id(s) you want to target.
---
Nitpick comments:
In `@erpnext/accounts/report/cash_flow/cash_flow.py`:
- Around line 227-238: The SQL in the function that computes gl_sum uses
f-string interpolation for the {cond} fragment (in the query that selects
sum(credit) - sum(debit) from `tabGL Entry`) which risks injection; change the
implementation to build the WHERE clause with parameterized filters or Frappe's
query builder instead of string interpolation: move any dynamic conditions into
the filters dict (or construct a safe list of SQL fragments using frappe.qb or
validated/static mappings) and call frappe.db.sql with named parameters (or use
frappe.qb.from_('GL Entry').select(...).where(...) with proper bindings) so the
query no longer injects raw {cond} via f-strings and uses only bound parameters
for company, start_date, end_date and account_type.
| @@ -1,5 +1,5 @@ | |||
| exclude: 'node_modules|.git' | |||
| default_stages: [pre-commit] | |||
| default_stages: [manual] | |||
Contributor
There was a problem hiding this comment.
Changing default_stages to [manual] disables automatic pre-commit hooks.
This prevents hooks from running automatically on git commit, requiring developers to explicitly run pre-commit run. This could allow commits to bypass linting and formatting checks.
Is this intentional? If this is needed to unblock CI, consider a more targeted fix (e.g., skipping specific hooks) rather than disabling automatic execution entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.pre-commit-config.yaml at line 2, The change setting default_stages:
[manual] disables automatic pre-commit execution and should be reverted or
narrowed: either remove the default_stages line (restoring normal commit-time
hooks) or set default_stages back to include the commit stage (e.g.,
default_stages: [commit]) and, if you needed to bypass a specific hook for CI,
instead add a per-hook "stages" or "files"/"exclude" rule for that hook in
.pre-commit-config.yaml (or set SKIP for that specific hook), referencing the
default_stages key and any specific hook id(s) you want to target.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix cash flow report: total row calculation and spacer