Skip to content

fix(hr): cancel linked Additional Salary on Overtime Slip cancellation - #4999

Open
pratheep-bit wants to merge 1 commit into
frappe:developfrom
pratheep-bit:fix/overtime-slip-cancel-additional-salary
Open

fix(hr): cancel linked Additional Salary on Overtime Slip cancellation#4999
pratheep-bit wants to merge 1 commit into
frappe:developfrom
pratheep-bit:fix/overtime-slip-cancel-additional-salary

Conversation

@pratheep-bit

Copy link
Copy Markdown
Contributor

Summary of Changes

  • Implement on_cancel(self) hook in OvertimeSlip controller to find and cancel linked Additional Salary records (ref_doctype="Overtime Slip", ref_docname=self.name).
  • Prevents cancelled overtime slips from leaving active submitted Additional Salary records in payroll processing.
  • Includes unit test test_overtime_slip_cancel_cancels_additional_salary in test_overtime_slip.py.

@greptile-apps

greptile-apps Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 5/5

Safe to merge; the new cancel hook correctly scopes its query and the test covers the end-to-end flow.

The implementation is narrowly scoped: it only touches submitted Additional Salaries linked by ref_doctype/ref_docname and cancels them in order. The test exercises the full submit-then-cancel path with a real salary structure assigned.

Files Needing Attention: No files require special attention.

Reviews (4): Last reviewed commit: "fix(hr): cancel linked Additional Salary..." | Re-trigger Greptile

Comment on lines +63 to +75
def unlink_and_cancel_additional_salary(self):
additional_salaries = frappe.get_all(
"Additional Salary",
filters={
"ref_doctype": "Overtime Slip",
"ref_docname": self.name,
"docstatus": 1,
},
pluck="name",
)
for name in additional_salaries:
doc = frappe.get_doc("Additional Salary", name)
doc.cancel()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Missing guard for salary-slip-linked Additional Salaries

If an Additional Salary was already pulled into a submitted Salary Slip, calling doc.cancel() here raises a raw LinkExistsError and rolls back the entire cancel, leaving the user with no actionable message. Consider checking frappe.db.exists("Salary Detail", {"additional_salary": name}) before attempting cancellation and surfacing a clear frappe.throw pointing users to cancel the Salary Slip first.

Comment thread hrms/hr/doctype/overtime_slip/test_overtime_slip.py
Comment thread hrms/hr/doctype/overtime_slip/test_overtime_slip.py
@mergify

mergify Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

pluck="name",
)
for name in additional_salaries:
doc = frappe.get_doc("Additional Salary", name)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Better to add a comment on those documents which got cancelled

@pratheep-bit
pratheep-bit force-pushed the fix/overtime-slip-cancel-additional-salary branch from 17dafd0 to 5e00d8a Compare July 28, 2026 02:12
@pratheep-bit

Copy link
Copy Markdown
Contributor Author

Hi @thomasantony12,

Thanks for the review!

Updated the cancellation handler in to add an audit comment () to each linked document before cancellation.

Also updated to assert that the cancellation comment is logged.

Force-pushed a single clean commit.

@github-actions

Copy link
Copy Markdown

This pull request is being marked as inactive because of no recent activity.
If your PR hasn't been reviewed, it's likely because it doesn't fullfill the contribution guidelines. Please read them carefully and fix the pull request. When you are sure all items are checked, please ping relevant codeowner in the comment. Be nice, they have a lot on their plate too.

It will be closed in 3 days if no further activity occurs.
Thank you for contributing!

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants