fix: add ignore_permissions flag to make_salary_slip function#3333
fix: add ignore_permissions flag to make_salary_slip function#3333elshafei-developer wants to merge 18 commits intofrappe:developfrom
Conversation
WalkthroughThe SalarySlip.pull_sal_struct path was changed so that when Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-10-07T05:10:53.235ZApplied to files:
🧬 Code graph analysis (1)hrms/payroll/doctype/salary_slip/salary_slip.py (1)
⏰ 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). (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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
hrms/payroll/doctype/salary_slip/salary_slip.py (1)
415-417: Harden attribute access and ensure a strict bool is passedMinor robustness: if ignore_permissions isn’t set on flags, getattr + bool avoids a possible None and enforces the expected boolean type. Safe and no behavior change when the flag isn’t set.
Apply this diff within the call:
- make_salary_slip( - self._salary_structure_doc.name, self, ignore_permissions=self.flags.ignore_permissions - ) + make_salary_slip( + self._salary_structure_doc.name, + self, + ignore_permissions=bool(getattr(self.flags, "ignore_permissions", False)), + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
hrms/payroll/doctype/salary_slip/salary_slip.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
hrms/payroll/doctype/salary_slip/salary_slip.py (1)
hrms/payroll/doctype/salary_structure/salary_structure.py (1)
make_salary_slip(359-401)
⏰ 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). (3)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Summary
🔇 Additional comments (1)
hrms/payroll/doctype/salary_slip/salary_slip.py (1)
415-417: Good fix: make_salary_slip now respects flags.ignore_permissionsPassing ignore_permissions from the document’s flags unblocks the intended admin/system flows without changing default behavior. This aligns with the signature in salary_structure.make_salary_slip and fixes the earlier override.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
hrms/payroll/doctype/salary_slip/salary_slip.py (1)
439-441: Add tests for permission-bypass toggle (default vs bypass, timesheet vs non-timesheet).Repo is labeled needs-tests; please cover both paths and assert the flag reaches get_mapped_doc.
I can draft tests in hrms/payroll/doctype/salary_slip/test_salary_slip.py that stub get_mapped_doc and validate ignore_permissions for both modes—want me to proceed?
🧹 Nitpick comments (1)
hrms/payroll/doctype/salary_slip/salary_slip.py (1)
439-441: Coerce to bool and guard for missing attribute.Avoid passing None and handle edge cases if flags is absent; pass a strict boolean.
Apply:
- make_salary_slip( - self._salary_structure_doc.name, self, ignore_permissions=self.flags.ignore_permissions - ) + make_salary_slip( + self._salary_structure_doc.name, + self, + ignore_permissions=bool(getattr(self.flags, "ignore_permissions", False)), + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
hrms/payroll/doctype/salary_slip/salary_slip.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
hrms/payroll/doctype/salary_slip/salary_slip.py (1)
hrms/payroll/doctype/salary_structure/salary_structure.py (1)
make_salary_slip(359-401)
⏰ 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). (3)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Summary
🔇 Additional comments (1)
hrms/payroll/doctype/salary_slip/salary_slip.py (1)
439-441: Good fix: propagate ignore_permissions to mapping.Passing the flag through to make_salary_slip aligns behavior with self.flags.ignore_permissions and fixes the override issue.
|
This pull request is being marked as inactive because of no recent activity. It will be closed in 3 days if no further activity occurs. |
|
This pull request is being marked as inactive because of no recent activity. It will be closed in 3 days if no further activity occurs. |
|
This pull request is being marked as inactive because of no recent activity. It will be closed in 3 days if no further activity occurs. |
|
Hi @asmitahase |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
hrms/payroll/doctype/salary_slip/salary_slip.py (1)
453-455: Test coverage needed for permission-bypass behavior.As noted in the previous review, this change requires test coverage for:
- Default behavior (flag absent or False): mapping runs with normal permissions
- Bypass behavior (flag True): mapping succeeds for contexts that would normally fail permission checks
- Both timesheet-based and non-timesheet-based salary slip generation paths
The existing review comment provides detailed guidance on implementing these tests in
hrms/payroll/doctype/salary_slip/test_salary_slip.py.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hrms/payroll/doctype/salary_slip/salary_slip.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: AyshaHakeem
Repo: frappe/hrms PR: 3607
File: hrms/payroll/doctype/salary_slip/salary_slip.py:1520-1523
Timestamp: 2025-10-07T05:10:53.235Z
Learning: In the `_get_claim_based_benefit_payout` method in `hrms/payroll/doctype/salary_slip/salary_slip.py`, the negative adjustment to `current_period_benefit` when `total_paid > total_accrued` is intentional. This handles the case where an employee claims a benefit that includes the current month's entitlement and then takes unpaid leave afterward - the next accrual is reduced to account for the overpayment.
📚 Learning: 2025-10-07T05:10:53.235Z
Learnt from: AyshaHakeem
Repo: frappe/hrms PR: 3607
File: hrms/payroll/doctype/salary_slip/salary_slip.py:1520-1523
Timestamp: 2025-10-07T05:10:53.235Z
Learning: In the `_get_claim_based_benefit_payout` method in `hrms/payroll/doctype/salary_slip/salary_slip.py`, the negative adjustment to `current_period_benefit` when `total_paid > total_accrued` is intentional. This handles the case where an employee claims a benefit that includes the current month's entitlement and then takes unpaid leave afterward - the next accrual is reduced to account for the overpayment.
Applied to files:
hrms/payroll/doctype/salary_slip/salary_slip.py
🧬 Code graph analysis (1)
hrms/payroll/doctype/salary_slip/salary_slip.py (1)
hrms/payroll/doctype/salary_structure/salary_structure.py (1)
make_salary_slip(334-379)
⏰ 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). (2)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (2)
| make_salary_slip( | ||
| self._salary_structure_doc.name, self, ignore_permissions=self.flags.ignore_permissions | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Handle undefined ignore_permissions flag explicitly.
The code assumes self.flags.ignore_permissions is set. If the flag is absent, self.flags.ignore_permissions evaluates to None (frappe._dict behavior), which is falsy but not the same as explicit False. This could lead to subtle bugs or unexpected behavior.
Apply this diff for more defensive handling:
- make_salary_slip(
- self._salary_structure_doc.name, self, ignore_permissions=self.flags.ignore_permissions
- )
+ make_salary_slip(
+ self._salary_structure_doc.name,
+ self,
+ ignore_permissions=getattr(self.flags, "ignore_permissions", False)
+ )📝 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.
| make_salary_slip( | |
| self._salary_structure_doc.name, self, ignore_permissions=self.flags.ignore_permissions | |
| ) | |
| make_salary_slip( | |
| self._salary_structure_doc.name, | |
| self, | |
| ignore_permissions=getattr(self.flags, "ignore_permissions", False) | |
| ) |
🤖 Prompt for AI Agents
In hrms/payroll/doctype/salary_slip/salary_slip.py around lines 453 to 455, the
call to make_salary_slip passes self.flags.ignore_permissions directly which can
be None rather than an explicit boolean; update the call to explicitly resolve
the flag to a boolean (e.g., use getattr(self.flags, "ignore_permissions",
False) or self.flags.get("ignore_permissions", False) and cast to bool if
needed) so make_salary_slip always receives True or False.
|
This pull request is being marked as inactive because of no recent activity. It will be closed in 3 days if no further activity occurs. |
|
This issue occurs when running code like this: salary = frappe.new_doc("Salary Slip")
salary.flags.ignore_permissions = True
salary.data = ...
salary.save()Even with |
|
This pull request is being marked as inactive because of no recent activity. It will be closed in 3 days if no further activity occurs. |
3 participants
When using
self.flags.ignore_permissionsto ignore salary generation permissions for an employee, themake_salary_slipfunction prevents salary generation because it automatically sets theignore_permissionsparameter to False. This overridesself.flags.ignore_permissions.backport version-15-hotfix
backport version-16-beta
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.