Skip to content

fix(profit-loss-report): handle zero base values and prevent null% display#54684

Open
AhmedAbokhatwa wants to merge 3 commits intofrappe:developfrom
AhmedAbokhatwa:profit-loss-report
Open

fix(profit-loss-report): handle zero base values and prevent null% display#54684
AhmedAbokhatwa wants to merge 3 commits intofrappe:developfrom
AhmedAbokhatwa:profit-loss-report

Conversation

@AhmedAbokhatwa
Copy link
Copy Markdown
Contributor

fix margin calculation and prevent invalid percentage display in Profit and Loss report

  • If both base_value and curr_value are 0 → the result will be 0%
  • If base_value is 0 and curr_value is not 0 → the result will be displayed as "N/A" (None) the UI will return it 0%
  • If either value is None → display "N/A" (None) Ensured consistent data contract between backend (Python) and frontend (HTML template)
    Before
image After image

@github-actions github-actions Bot added the needs-tests This PR needs automated unit-tests. label Apr 30, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 30, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 0b9df8a6-df43-4790-ad4e-d3d57b52ca51

📥 Commits

Reviewing files that changed from the base of the PR and between aac3ccd and 7335011.

📒 Files selected for processing (1)
  • erpnext/accounts/report/financial_statements.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • erpnext/accounts/report/financial_statements.py

📝 Walkthrough

Walkthrough

The compute_margin_view_data function in the financial statements report was updated to use .get() when reading both base_value and curr_value. The function now treats missing inputs (None) by setting the result cell to None and continuing. When base_value == 0, it returns 0 only if curr_value == 0; otherwise it returns None. Margin percentage is calculated only when both values are present and base_value is non-zero.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the main change: handling zero base values and preventing null% display in the profit-loss report margin calculations.
Description check ✅ Passed The description clearly explains the fix, outlines the specific rules implemented for margin calculation, and includes before/after visual comparisons demonstrating the change.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share
Review rate limit: 6/8 reviews remaining, refill in 7 minutes and 49 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

@AhmedAbokhatwa AhmedAbokhatwa changed the title Profit loss report fix(profit-loss-report): handle zero base values and prevent null% display Apr 30, 2026
@AhmedAbokhatwa AhmedAbokhatwa force-pushed the profit-loss-report branch 2 times, most recently from f3a5430 to aac3ccd Compare April 30, 2026 17:49
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 30, 2026

Codecov Report

❌ Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.75%. Comparing base (9e5d94c) to head (7335011).
⚠️ Report is 174 commits behind head on develop.

Files with missing lines Patch % Lines
erpnext/accounts/report/financial_statements.py 0.00% 8 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #54684      +/-   ##
===========================================
+ Coverage    79.27%   79.75%   +0.47%     
===========================================
  Files         1158     1160       +2     
  Lines       125495   126451     +956     
===========================================
+ Hits         99492   100853    +1361     
+ Misses       26003    25598     -405     
Files with missing lines Coverage Δ
erpnext/accounts/report/financial_statements.py 76.11% <0.00%> (-1.08%) ⬇️

... and 111 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-tests This PR needs automated unit-tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant