Skip to content

fix(salary_slip): update round() for remaining sub period #4243

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.

Sign up for GitHub

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

Open
nareshkannasln wants to merge 1 commit into
base: develop
Choose a base branch
from
+2 −2
Open

fix(salary_slip): update round() for remaining sub period #4243

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions hrms/payroll/doctype/salary_slip/salary_slip.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1018,15 +1018,15 @@ def compute_current_and_future_taxable_earnings(self):
# get taxable_earnings for current period (all days)
self.current_taxable_earnings = self.get_taxable_earnings(self.tax_slab.allow_tax_exemption)
self.future_structured_taxable_earnings = self.current_taxable_earnings.taxable_earnings * (
ceil(self.remaining_sub_periods) - 1
round(self.remaining_sub_periods) - 1
)

current_taxable_earnings_before_exemption = (
self.current_taxable_earnings.taxable_earnings
+ self.current_taxable_earnings.amount_exempted_from_income_tax
)
self.future_structured_taxable_earnings_before_exemption = (
current_taxable_earnings_before_exemption * (ceil(self.remaining_sub_periods) - 1)
current_taxable_earnings_before_exemption * (round(self.remaining_sub_periods) - 1)
Comment on lines 1020 to +1029
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Please add regression coverage for the new multiplier.

hrms/payroll/doctype/salary_slip/test_salary_slip.py Lines 1385-1400 still derive prev_period with math.ceil(remaining_sub_periods). Please update that path and cover both the 26.07 fortnightly case from this ticket and one exact .5 case, otherwise this fix is easy to regress.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@hrms/payroll/doctype/salary_slip/salary_slip.py` around lines 1020 - 1029,
Update the test derivation of prev_period in
hrms/payroll/doctype/salary_slip/test_salary_slip.py so it matches the new
production logic (use round(remaining_sub_periods) - 1 instead of
math.ceil(remaining_sub_periods)), and add two regression cases: one for the
26.07 fortnightly scenario and one for an exact .5 remaining_sub_periods case;
for each case assert the computed prev_period and that
future_structured_taxable_earnings and
future_structured_taxable_earnings_before_exemption on the SalarySlip instance
match the expected values derived using round(self.remaining_sub_periods) - 1.
Ensure the tests reference remaining_sub_periods, prev_period,
future_structured_taxable_earnings, and
future_structured_taxable_earnings_before_exemption so the change cannot
regress.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
python - <<'PY'
for value in (1.5, 2.5, 10.5, 26.07):
    print(f"{value}: round={round(value)}")
PY

rg -n -C2 'round\(self\.remaining_sub_periods\)|ceil\(self\.remaining_sub_periods\)' hrms/payroll/doctype/salary_slip/salary_slip.py

Repository: frappe/hrms

Length of output: 1190

🏁 Script executed:

# Check test cases for remaining_sub_periods and future period calculations
fd test_salary_slip hrms/payroll/doctype/salary_slip/

Repository: frappe/hrms

Length of output: 107

🏁 Script executed:

# Understand remaining_sub_periods and get_period_factor
rg -A10 "def get_period_factor" hrms/payroll/doctype/payroll_period/

Repository: frappe/hrms

Length of output: 888

🏁 Script executed:

# Check all usages of remaining_sub_periods in salary_slip.py
rg -n "remaining_sub_periods" hrms/payroll/doctype/salary_slip/salary_slip.py

Repository: frappe/hrms

Length of output: 549

🏁 Script executed:

# Check test file for assertions on ceil vs round behavior
rg -n -A5 "remaining_sub_periods|future_structured_taxable" hrms/payroll/doctype/salary_slip/test_salary_slip.py | head -60

Repository: frappe/hrms

Length of output: 410

🏁 Script executed:

# Get full get_period_factor implementation to verify it returns fractional values
rg -A50 "def get_period_factor" hrms/payroll/doctype/payroll_period/payroll_period.py | head -80

Repository: frappe/hrms

Length of output: 1447

🏁 Script executed:

# Check if there are recent test modifications for this change
rg -n "round\(remaining_sub_periods\)" hrms/payroll/doctype/salary_slip/test_salary_slip.py

Repository: frappe/hrms

Length of output: 37

🏁 Script executed:

# Look for comments around the changed lines explaining the intent
sed -n '1015,1035p' hrms/payroll/doctype/salary_slip/salary_slip.py

Repository: frappe/hrms

Length of output: 950

🏁 Script executed:

# Check if there are related changes in other test scenarios
rg -B5 -A10 "prev_period.*ceil" hrms/payroll/doctype/salary_slip/test_salary_slip.py

Repository: frappe/hrms

Length of output: 413

Use an explicit half-up future-period count here, not bare round().

get_period_factor() returns fractional values (e.g., remaining days ÷ salary days), and Python's round() is half-to-even, so round(2.5) becomes 2. This under-projects future earnings by ~50% in some cases. Meanwhile, this file still uses ceil(self.remaining_sub_periods) at Line 1147 and Lines 1200–1203, creating inconsistent multipliers across taxable-earnings, exempted-deduction, and future-period paths. The test at Line 1394 still asserts the old ceil behavior, indicating this inconsistency is unintended. Compute one shared future-period count with explicit half-up semantics and reuse it across all affected call sites (Lines 1021, 1029, 1147, 1200, 1203).

🛠️ Proposed change for this block 
 		self.current_taxable_earnings = self.get_taxable_earnings(self.tax_slab.allow_tax_exemption)
+		future_sub_period_count = max(0, int(self.remaining_sub_periods + 0.5) - 1)
 		self.future_structured_taxable_earnings = self.current_taxable_earnings.taxable_earnings * (
-			round(self.remaining_sub_periods) - 1
+			future_sub_period_count
 		)
@@
 		self.future_structured_taxable_earnings_before_exemption = (
-			current_taxable_earnings_before_exemption * (round(self.remaining_sub_periods) - 1)
+			current_taxable_earnings_before_exemption * future_sub_period_count
 		)

Then apply the same future_sub_period_count to Lines 1147, 1200, and 1203, and update the test expectation at Line 1394.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@hrms/payroll/doctype/salary_slip/salary_slip.py` around lines 1020 - 1029,
The code uses Python's round() on self.remaining_sub_periods (derived from
get_period_factor()) which is half-to-even and inconsistent with other ceil
usage; replace those disparate calls by computing a single
future_sub_period_count using explicit half-up semantics (e.g., Decimal quantize
with ROUND_HALF_UP or a small helper that does half-up on the fractional value)
based on self.remaining_sub_periods and reuse that variable when computing
future_structured_taxable_earnings,
future_structured_taxable_earnings_before_exemption and in the places currently
using ceil (the sites referenced by future_structured_taxable_earnings, the
before_exemption calculation, and the other two future-period multipliers); also
update the related test expectation (the assertion around Line 1394) to match
the new half-up behavior.

)

# get taxable_earnings, addition_earnings for current actual payment days
Expand Down
Loading