fix(salary_slip): update round() for remaining sub period#4243
fix(salary_slip): update round() for remaining sub period#4243nareshkannasln wants to merge 1 commit intofrappe:developfrom
Conversation
nareshkannasln
force-pushed
the
remaining-sub-periods
branch
2 times, most recently
from
d130c70 to
1432cdd
Compare
nareshkannasln
requested review from
AyshaHakeem and
ruchamahabal
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe 🚥 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. 📝 Coding Plan
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 Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@hrms/payroll/doctype/salary_slip/salary_slip.py`:
- Around line 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.
- Around line 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.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d82baa36-c0da-40cd-b350-c1d5d555d444
📒 Files selected for processing (1)
hrms/payroll/doctype/salary_slip/salary_slip.py
| 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) |
There was a problem hiding this comment.
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.
🧩 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.pyRepository: 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.pyRepository: 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 -60Repository: 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 -80Repository: 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.pyRepository: 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.pyRepository: 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.pyRepository: 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.
nareshkannasln
force-pushed
the
remaining-sub-periods
branch
from
1432cdd to
ed9291b
Compare
1 participant
Ref: 62117
Issue: When running payroll with fortnightly frequency for payroll period 2025-26, the future_structured_taxable_earnings and future_structured_taxable_earnings_before_exemption are incorrectly calculated.
The system calculates remaining_sub_periods as 365 / 14 = 26.07. Using ceil(26.07) converts this to 27, resulting in 27 - 1 = 26 future periods being projected instead of the correct 25. This inflates the CTC and annual taxable amount, causing incorrect income tax deduction.
Before:
Screencast.from.2026-03-19.11-11-41.webm
After:
Screencast.from.2026-03-19.11-10-30.webm
Backport needed for v-15, v-16
Summary by CodeRabbit