fix: modify voucher type based on account type

[elshafei-developer]

Incorrect voucher_type assignment

Payroll Entry was always creating Journal Entries with voucher_type = "Bank Entry" even when the selected payment_account is a Cash account.
The updated logic checks the account_type of payment_account and sets the voucher_type accordingly (Bank Entry for bank accounts, Cash Entry for cash accounts).

backport version-15-hotfix
backport version-16-hotfix

Summary by CodeRabbit

• Bug Fixes

  • Payroll now recognizes both bank and cash payment entries when linking transactions, ensuring payments are correctly associated.
  • Payment handling selects the appropriate entry type (bank vs. cash) dynamically based on the payment account’s configuration.

• Tests

  • Test coverage updated to validate multi-method payment aggregation and selection logic.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Payroll Entry logic and tests were changed to treat both "Bank Entry" and "Cash Entry" journal entries as payment sources. The has_bank_entries check now accepts Journal Entries with voucher_type equal to "Bank Entry" or "Cash Entry" when linked to a Payroll Entry. set_accounting_entries_for_bank_entry sets voucher_type to "Cash Entry" if the payment account's account_type is "Cash`, otherwise to "Bank Entry". Tests' SQL filters were updated accordingly.

🚥 Pre-merge checks | ✅ 2 | ❌ 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 (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: modify voucher type based on account type' directly reflects the core change: updating logic to set voucher_type based on whether the payment_account is a Bank or Cash account.

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

📝 Coding Plan

• Generate coding plan for human review comments

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

Tip

CodeRabbit can generate a title for your PR based on the changes with custom instructions.

Set the reviews.auto_title_instructions setting to generate a title for your PR based on the changes in the PR with custom instructions.

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

hrms/payroll/doctype/payroll_entry/payroll_entry.py (1)

1009-1100: Add server-side validation for payment_account account type.

The code at lines 1093-1095 assumes payment_account is either a "Bank" or "Cash" account without validation. Although client-side filtering exists in the form, direct API calls or database updates could bypass this, resulting in an incorrect voucher type being set for unsupported account types.

Add this validation method:

def validate_payment_account(self):
	if not self.payment_account:
		return
	
	payment_account_type = frappe.db.get_value(
		"Account", self.payment_account, "account_type"
	)
	if payment_account_type not in ("Bank", "Cash"):
		frappe.throw(
			_(
				"Account type should be either {0} or {1} for payment account {2}, please set and try again"
			).format(
				frappe.bold("Bank"),
				frappe.bold("Cash"),
				frappe.bold(get_link_to_form("Account", self.payment_account)),
			)
		)

Then call it in before_submit():

 	def before_submit(self):
 		self.validate_existing_salary_slips()
 		self.validate_payroll_payable_account()
+		self.validate_payment_account()
 		if self.get_employees_with_unmarked_attendance():
 			frappe.throw(_("Cannot submit. Attendance is not marked for some employees."))

🧹 Nitpick comments (1)

hrms/payroll/doctype/payroll_entry/payroll_entry.py (1)

882-904: Consider renaming for clarity.

The method name has_bank_entries and the return key "has_bank_entries" are now misleading since the logic checks for both "Bank Entry" and "Cash Entry" voucher types. Consider renaming to better reflect the actual behavior (e.g., has_payment_entries or has_bank_or_cash_entries).

Apply this diff to improve clarity:

 	@frappe.whitelist()
-	def has_bank_entries(self) -> dict[str, bool]:
+	def has_payment_entries(self) -> dict[str, bool]:
 		je = frappe.qb.DocType("Journal Entry")
 		jea = frappe.qb.DocType("Journal Entry Account")
 
-		bank_entries = (
+		payment_entries = (
 			frappe.qb.from_(je)
 			.inner_join(jea)
 			.on(je.name == jea.parent)
 			.select(je.name)
 			.where(
 				((je.voucher_type == "Bank Entry") | (je.voucher_type == "Cash Entry"))
 				& (jea.reference_name == self.name)
 				& (jea.reference_type == "Payroll Entry")
 			)
 		).run(as_dict=True)
 
 		return {
-			"has_bank_entries": bool(bank_entries),
-			"has_bank_entries_for_withheld_salaries": not any(
+			"has_payment_entries": bool(payment_entries),
+			"has_payment_entries_for_withheld_salaries": not any(
 				employee.is_salary_withheld for employee in self.employees
 			),
 		}

Note: This change would require updating any client-side code that calls this method or uses the returned keys.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a08190c and b55b675.

📒 Files selected for processing (1)

• hrms/payroll/doctype/payroll_entry/payroll_entry.py (2 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: asmitahase
Repo: frappe/hrms PR: 3448
File: hrms/hr/doctype/employee_advance/employee_advance.py:109-117
Timestamp: 2025-08-07T11:26:49.671Z
Learning: In the Employee Advance system, `set_total_advance_paid` is only called after advance payment ledger entries are created, which already validates that the account type is either "Receivable" or "Payable". Additional validation for account type in this method would be redundant.

⏰ 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: build
• GitHub Check: Summary

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

hrms/payroll/doctype/payroll_entry/payroll_entry.py (1)

1093-1103: LGTM! The voucher type is now correctly determined based on account type.

The logic properly sets "Bank Entry" for bank accounts and "Cash Entry" otherwise. Using get_cached_value is efficient.

Note that payment_account field lacks explicit validation to restrict it to Bank or Cash account types (unlike payroll_payable_account which validates account_type). The runtime conditional serves as a defensive fallback, but consider adding field-level validation or a doctype validation method to enforce that payment_account must be Bank or Cash type.

🧹 Nitpick comments (1)

hrms/payroll/doctype/payroll_entry/payroll_entry.py (1)

886-907: Method name is now slightly misleading but functionally correct.

The method has_bank_entries and its return key has_bank_entries now also check for "Cash Entry" journal entries. The logic change is correct for the fix, but the naming could be more accurate (e.g., has_payment_entries).

Consider renaming for clarity in a future refactor, though this may require changes to callers.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b55b675 and b244d1e.

📒 Files selected for processing (1)

• hrms/payroll/doctype/payroll_entry/payroll_entry.py (2 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: asmitahase
Repo: frappe/hrms PR: 3448
File: hrms/hr/doctype/employee_advance/employee_advance.py:109-117
Timestamp: 2025-08-07T11:26:49.671Z
Learning: In the Employee Advance system, `set_total_advance_paid` is only called after advance payment ledger entries are created, which already validates that the account type is either "Receivable" or "Payable". Additional validation for account type in this method would be redundant.

⏰ 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

[asmitahase]

Can you check failing tests?

[elshafei-developer]

Can you check failing tests?

@asmitahase
I updated the test and cleaned up the branch
Because of the cleanup, the PR is closed
Please reopen

[github-actions]

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!

[elshafei-developer]

@asmitahase
The awaiting response from contributor label is still present.
Please let me know if any further changes are required from my side.

[iamkhanraheel]

@asmitahase The awaiting response from contributor label is still present. Please let me know if any further changes are required from my side.

Change was already requested, please check the comments above.

[elshafei-developer]

Change was already requested, please check the comments above.

I have already pushed the commits to fix the failing tests as requested above, and resolved the issue.
Please let me know if I missed any specific inline comments or if further changes are needed.

[github-actions]

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!