-
Notifications
You must be signed in to change notification settings - Fork 10.4k
fix: calculate net profit amount from root node accounts #51513
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #51513 +/- ##
===========================================
- Coverage 79.11% 79.10% -0.01%
===========================================
Files 1179 1179
Lines 121360 121359 -1
===========================================
- Hits 96009 96007 -2
- Misses 25351 25352 +1
🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughThe Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-12-16T05:33:58.723ZApplied to files:
⏰ 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)
🔇 Additional comments (2)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
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/gross_and_net_profit_report/gross_and_net_profit_report.py (1)
175-201: Apply the same aggregation fix toget_profitfor consistency.The
get_profitfunction still uses the old single-row approach on lines 189-190:gross_income_for_period = flt(gross_income[0].get(key, 0)) if gross_income else 0 gross_expense_for_period = flt(gross_expense[0].get(key, 0)) if gross_expense else 0This mirrors the bug fixed in
get_net_profit. If a company has multiple root-level accounts marked withinclude_in_gross=1, only the first is included in the Gross Profit calculation. Theget_net_profitfunction already demonstrates the correct approach (lines 222-233):gross_income_roots = [row for row in (gross_income or []) if not flt(row.get("indent"))] gross_income_for_period = sum(flt(row.get(key, 0)) for row in gross_income_roots)Apply this same pattern to
get_profitto aggregate all root accounts correctly.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
erpnext/accounts/report/gross_and_net_profit_report/gross_and_net_profit_report.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-16T05:33:58.723Z
Learnt from: Abdeali099
Repo: frappe/erpnext PR: 51078
File: erpnext/accounts/doctype/financial_report_template/financial_report_engine.py:486-491
Timestamp: 2025-12-16T05:33:58.723Z
Learning: In ERPNext/Frappe codebase, query.run(as_dict=True) returns frappe._dict objects that support both dict-style access (obj["key"]) and attribute-style access (obj.key). Therefore, attribute access on query results is valid and will not raise AttributeError. When reviewing Python code, prefer attribute access (obj.key) for readability where the key is known to exist, but ensure existence checks or fallback handling if there is any doubt about missing keys.
Applied to files:
erpnext/accounts/report/gross_and_net_profit_report/gross_and_net_profit_report.py
⏰ 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 (2)
erpnext/accounts/report/gross_and_net_profit_report/gross_and_net_profit_report.py (2)
230-233: Aggregation logic correctly sums across all root accounts.The change from single-row access (
gross_income[0]) to summing all roots (sum(flt(row.get(key, 0)) for row in gross_income_roots)) correctly addresses the issue described in the PR objectives.The implementation is safe:
- Empty root lists:
sum()returns 0- Missing period keys:
.get(key, 0)provides safe fallback- None values:
flt(None)returns 0.0
222-225: LGTM! Root filtering logic correctly identifies root-level accounts.The filtering using
not flt(row.get("indent"))correctly captures rows with indent == 0 (root accounts). The logic handles edge cases well:
- Missing
indentfield: treated as root (indent 0)- Empty lists: handled by
or []fallback- None values:
flt(None)returns 0.0The commit message confirms this is an intentional fix: "calculate net profit amount from root node accounts", so the behavior is as designed.
…-51513 fix: calculate net profit amount from root node accounts (backport #51513)
…-51513 fix: calculate net profit amount from root node accounts (backport #51513)
Issue: Net Profit value in the Gross and Net Profit Report shows incorrect value because total income and expense amounts are calculated using only the zero-indexed row.
This leads a discrepancy, when the Net Profit value is compared with the Profit and Loss Statement, especially when multiple root-level accounts exist.
Ref: 56322
Solution: Calculate income and expense totals by summing all root-level accounts.
Before:
After:
Bacport Needed v15