fix: changes as per review

erpnext/accounts/doctype/payment_entry/payment_entry.js

@@ -7,8 +7,6 @@ cur_frm.cscript.tax_table = "Advance Taxes and Charges";
 erpnext.accounts.taxes.setup_tax_validations("Payment Entry");
 erpnext.accounts.taxes.setup_tax_filters("Advance Taxes and Charges");
 
-const FAULTY_VALUES = ["", null, undefined, 0];
-
 frappe.ui.form.on("Payment Entry", {
 	onload: function (frm) {
 		frm.ignore_doctypes_on_cancel_all = [
@@ -1027,8 +1025,6 @@ frappe.ui.form.on("Payment Entry", {
 								total_negative_outstanding - total_positive_outstanding
 							);
 					}
-
-					frm.events.set_matched_payment_requests(frm);
 				}
 
 				frm.events.allocate_party_amount_against_ref_docs(
@@ -1134,6 +1130,7 @@ frappe.ui.form.on("Payment Entry", {
 		});
 
 		frm.refresh_fields();
+		if (frappe.flags.allocate_payment_amount) frm.call("set_matched_payment_requests");
 		frm.events.set_total_allocated_amount(frm);
 	},
 
@@ -1681,11 +1678,6 @@ frappe.ui.form.on("Payment Entry", {
 
 		return current_tax_amount;
 	},
-
-	set_matched_payment_requests: async function (frm) {
-		await frappe.after_ajax();
-		frm.call("set_matched_payment_requests");
-	},
 });
 
 frappe.ui.form.on("Payment Entry Reference", {
@@ -1731,15 +1723,10 @@ frappe.ui.form.on("Payment Entry Reference", {
 	allocated_amount: function (frm, cdt, cdn) {
 		frm.events.set_total_allocated_amount(frm);
 
-		const row = locals[cdt][cdn];
+		const row = frappe.get_doc(cdt, cdn);
 
-		// if payment_request already set then return
+		if (row.payment_request || !row.reference_name || !row.reference_doctype || !row.allocated_amount)
-		if (row.payment_request) return;
+			return;
-
-		const references = [row.reference_name, row.reference_doctype, row.allocated_amount];
-
-		// if any of the reference fields are faulty, it returns
-		if (FAULTY_VALUES.some((el) => references.includes(el))) return;
 
 		frm.call("set_matched_payment_request", { row_idx: row.idx });
 	},

erpnext/accounts/doctype/payment_entry/payment_entry.py

@@ -181,7 +181,7 @@ class PaymentEntry(AccountsController):
 		self.set_total_in_words()
 
 	def before_save(self):
-		self.check_payment_requests()
+		self.check_references_for_unset_payment_request()
 
 	def on_submit(self):
 		if self.difference_amount:
@@ -190,9 +190,8 @@ class PaymentEntry(AccountsController):
 		self.update_outstanding_amounts()
 		self.update_advance_paid()
 		self.update_payment_schedule()
-		self.set_payment_req_outstanding_amount()
+		self.update_payment_requests()
-		self.set_payment_req_status()
+		self.update_references_advance_payment_status()
-		self.set_reference_advance_payment_status()
 		self.set_status()
 
 	def set_liability_account(self):
@@ -267,33 +266,25 @@ class PaymentEntry(AccountsController):
 		self.update_advance_paid()
 		self.delink_advance_entry_references()
 		self.update_payment_schedule(cancel=1)
-		self.set_payment_req_outstanding_amount(cancel=True)
+		self.update_payment_requests(cancel=True)
-		self.set_payment_req_status()
+		self.update_references_advance_payment_status()
-		self.set_reference_advance_payment_status()
 		self.set_status()
 
-	def set_payment_req_outstanding_amount(self, cancel=False):
+	def update_payment_requests(self, cancel=False):
 		from erpnext.accounts.doctype.payment_request.payment_request import (
-			update_payment_req_outstanding_amount,
+			update_payment_requests_as_per_pe_references,
 		)
 
-		update_payment_req_outstanding_amount(self, cancel=cancel)
+		update_payment_requests_as_per_pe_references(self.references, cancel=cancel)
 
-	def set_payment_req_status(self):
+	def update_references_advance_payment_status(self):
-		from erpnext.accounts.doctype.payment_request.payment_request import update_payment_req_status
-
-		update_payment_req_status(self, None)
-
-	# todo: need to optimize
-	def set_reference_advance_payment_status(self):
 		advance_payment_doctypes = frappe.get_hooks("advance_payment_receivable_doctypes") + frappe.get_hooks(
 			"advance_payment_payable_doctypes"
 		)
 
 		for ref in self.get("references"):
-			ref_doc = frappe.get_doc(ref.reference_doctype, ref.reference_name)
 			if ref.reference_doctype in advance_payment_doctypes:
-				# set advance payment status
+				ref_doc = frappe.get_doc(ref.reference_doctype, ref.reference_name)
 				ref_doc.set_advance_payment_status()
 
 	def update_outstanding_amounts(self):
@@ -334,7 +325,7 @@ class PaymentEntry(AccountsController):
 		if self.payment_type == "Internal Transfer":
 			return
 
-		self.validate_allocated_amount_as_of_pr()
+		self.validate_allocated_amount_as_per_payment_request()
 
 		if self.party_type in ("Customer", "Supplier"):
 			self.validate_allocated_amount_with_latest_data()
@@ -348,7 +339,7 @@ class PaymentEntry(AccountsController):
 				if flt(d.allocated_amount) < 0 and flt(d.allocated_amount) < flt(d.outstanding_amount):
 					frappe.throw(fail_message.format(d.idx))
 
-	def validate_allocated_amount_as_of_pr(self):
+	def validate_allocated_amount_as_per_payment_request(self):
 		from erpnext.accounts.doctype.payment_request.payment_request import (
 			get_outstanding_amount_of_payment_entry_references as get_outstanding_amounts,
 		)
@@ -358,9 +349,10 @@ class PaymentEntry(AccountsController):
 		for ref in self.references:
 			if ref.payment_request and ref.allocated_amount > outstanding_amounts[ref.payment_request]:
 				frappe.throw(
-					_("Allocated Amount cannot be greater than Outstanding Amount of {0}").format(
+					msg=_(
-						get_link_to_form("Payment Request", ref.payment_request)
+						"Row #{0}: Allocated Amount cannot be greater than Outstanding Amount of Payment Request {1}"
-					)
+					).format(ref.idx, get_link_to_form("Payment Request", ref.payment_request)),
+					title=_("Invalid Allocated Amount"),
 				)
 
 	def term_based_allocation_enabled_for_reference(
@@ -1735,59 +1727,69 @@ class PaymentEntry(AccountsController):
 
 		return current_tax_fraction
 
-	def check_payment_requests(self):
+	def check_references_for_unset_payment_request(self):
 		if not self.references:
 			return
 
-		not_set_count = sum(1 for row in self.references if not row.payment_request)
+		matched_payment_requests = get_matched_payment_requests_of_references(
+			[row for row in self.references if not row.payment_request]
+		)
 
-		if not_set_count == 0:
+		unset_pr_rows = {}
-			return
+
-		elif not_set_count == 1:
+		for row in self.references:
-			msg = _("{0} {1} is not set in {2}").format(
+			if row.payment_request:
-				not_set_count,
+				continue
-				frappe.bold("Payment Request"),
+
-				frappe.bold("Payment References"),
+			matched_pr = matched_payment_requests.get(
-			)
+				(row.reference_doctype, row.reference_name, row.allocated_amount)
-		else:
-			msg = _("{0} {1} are not set in {2}").format(
-				not_set_count,
-				frappe.bold("Payment Request"),
-				frappe.bold("Payment References"),
 			)
 
-		frappe.msgprint(msg=msg, alert=True, indicator="orange")
+			if matched_pr:
+				unset_pr_rows[row.idx] = matched_pr
+
+		if unset_pr_rows:
+			message = _("Matched Payment Requests found for references, but not set. <br><br>")
+			message += _("<details><summary><strong>View Details</strong></summary><ul>")
+			for idx, pr in unset_pr_rows.items():
+				message += _("<li>Row #{0}: {1}</li>").format(idx, get_link_to_form("Payment Request", pr))
+			message += _("</ul></details>")
+
+			frappe.msgprint(
+				msg=message,
+				indicator="yellow",
+			)
 
-	# todo: can be optimize
 	@frappe.whitelist()
 	def set_matched_payment_requests(self):
 		if not self.references:
 			return
 
+		matched_payment_requests = get_matched_payment_requests_of_references(self.references)
+
 		matched_count = 0
 
 		for row in self.references:
-			if row.payment_request or (
+			if (
-				not row.reference_doctype or not row.reference_name or not row.allocated_amount
+				row.payment_request
+				or not row.reference_doctype
+				or not row.reference_name
+				or not row.allocated_amount
 			):
 				continue
 
-			row.payment_request = get_matched_payment_request(
+			row.payment_request = matched_payment_requests.get(
-				row.reference_doctype, row.reference_name, row.allocated_amount
+				(row.reference_doctype, row.reference_name, row.allocated_amount)
 			)
 
 			if row.payment_request:
 				matched_count += 1
 
-		if matched_count == 0:
+		if not matched_count:
 			return
-		elif matched_count == 1:
-			msg = _("{0} matched {1} is set").format(matched_count, frappe.bold("Payment Request"))
-		else:
-			msg = _("{0} matched {1} are set").format(matched_count, frappe.bold("Payment Request"))
 
 		frappe.msgprint(
-			msg=msg,
+			msg=_("Setting {0} matched Payment Request(s)").format(matched_count),
 			alert=True,
 		)
 
@@ -1799,38 +1801,61 @@ class PaymentEntry(AccountsController):
 			frappe.throw(_("Row #{0} not found").format(row_idx), title=_("Row Not Found"))
 
 		# if payment entry already set then do not set it again
-		if row.payment_request:
+		if (
+			row.payment_request
+			or not row.reference_doctype
+			or not row.reference_name
+			or not row.allocated_amount
+		):
 			return
 
-		row.payment_request = get_matched_payment_request(
+		matched_pr = get_matched_payment_requests_of_references([row])
-			row.reference_doctype, row.reference_name, row.allocated_amount
+
+		if not matched_pr:
+			return
+
+		row.payment_request = matched_pr[(row.reference_doctype, row.reference_name, row.allocated_amount)]
+
+		frappe.msgprint(
+			msg=_("Setting matched Payment Request"),
+			alert=True,
 		)
 
-		if row.payment_request:
-			frappe.msgprint(
-				msg=_("Matched {0} is set").format(frappe.bold("Payment Request")),
-				alert=True,
-			)
 
+# FIXME: can be optimize and use query builder
+def get_matched_payment_requests_of_references(references=None):
+	if not references:
+		return
 
-# todo: can be optimize
+	refs = [
-def get_matched_payment_request(reference_doctype, reference_name, outstanding_amount):
+		(row.reference_doctype, row.reference_name, row.allocated_amount)
-	payment_requests = frappe.get_all(
+		for row in references
-		doctype="Payment Request",
+		if row.reference_doctype and row.reference_name and row.allocated_amount
-		filters={
+	]
-			"reference_doctype": reference_doctype,
+
-			"reference_name": reference_name,
+	if not refs:
-			"outstanding_amount": outstanding_amount,
+		return
-			"status": ["!=", "Paid"],
+
-			"docstatus": 1,
+	all_matched_prs = frappe.db.sql(
-		},
+		"""
-		pluck="name",
+        select name, reference_doctype, reference_name, outstanding_amount
+        from `tabPayment Request`
+        where (reference_doctype, reference_name, outstanding_amount) in %s
+        and status != 'Paid' and docstatus = 1
+        """,
+		(refs,),
+		as_dict=True,
 	)
 
-	if len(payment_requests) == 1:
+	single_matched_prs = {}
-		return payment_requests[0]
+	for pr in all_matched_prs:
+		key = (pr.reference_doctype, pr.reference_name, pr.outstanding_amount)
+		if key in single_matched_prs:
+			single_matched_prs[key].append(pr.name)
+		else:
+			single_matched_prs[key] = [pr.name]
 
-	return None
+	return {key: names[0] for key, names in single_matched_prs.items() if len(names) == 1}
 
 
 def validate_inclusive_tax(tax, doc):

erpnext/accounts/doctype/payment_request/payment_request.json

@@ -134,7 +134,8 @@
    "no_copy": 1,
    "options": "reference_doctype",
    "print_hide": 1,
-   "read_only": 1
+   "read_only": 1,
+   "search_index": 1
   },
   {
    "fieldname": "transaction_details",
@@ -147,7 +148,8 @@
    "fieldtype": "Currency",
    "label": "Amount",
    "non_negative": 1,
-   "options": "currency"
+   "options": "currency",
+   "reqd": 1
   },
   {
    "default": "0",
@@ -403,7 +405,7 @@
    "read_only": 1
   },
   {
-   "depends_on": "eval:doc.docstatus==1",
+   "depends_on": "eval: doc.docstatus === 1",
    "fieldname": "outstanding_amount",
    "fieldtype": "Currency",
    "label": "Outstanding Amount",
@@ -415,7 +417,7 @@
  "index_web_pages_for_search": 1,
  "is_submittable": 1,
  "links": [],
- "modified": "2024-07-20 17:54:33.064658",
+ "modified": "2024-07-23 19:02:07.754296",
  "modified_by": "Administrator",
  "module": "Accounts",
  "name": "Payment Request",

erpnext/accounts/doctype/payment_request/payment_request.py

@@ -103,7 +103,10 @@ class PaymentRequest(Document):
 
 	def validate_payment_request_amount(self):
 		if self.grand_total == 0:
-			frappe.throw(_("Total Payment Request cannot be zero"))
+			frappe.throw(
+				_("{0} cannot be zero").format(self.get_label_from_fieldname("grand_total")),
+				title=_("Invalid Amount"),
+			)
 
 		existing_payment_request_amount = flt(
 			get_existing_payment_request_amount(self.reference_doctype, self.reference_name)
@@ -267,7 +270,7 @@ class PaymentRequest(Document):
 		)
 
 	def set_as_paid(self):
-		if self.payment_channel == "Phone" and self.status != "Paid":
+		if self.payment_channel == "Phone":
 			self.db_set({"status": "Paid", "outstanding_amount": 0})
 
 		else:
@@ -279,8 +282,6 @@ class PaymentRequest(Document):
 
 	def create_payment_entry(self, submit=True):
 		"""create entry"""
-		if self.payment_channel == "Phone":
-			frappe.throw(_("Payment Entry cannot be created for Phone Payment"))
 		frappe.flags.ignore_account_permission = True
 
 		ref_doc = frappe.get_doc(self.reference_doctype, self.reference_name)
@@ -297,8 +298,8 @@ class PaymentRequest(Document):
 		bank_amount = self.outstanding_amount
 
 		if party_account_currency == ref_doc.company_currency and party_account_currency != self.currency:
-			total = ref_doc.get("rounded_total") or ref_doc.grand_total
+			total = ref_doc.get("rounded_total") or ref_doc.get("grand_total")
-			base_total = ref_doc.get("base_rounded_total") or ref_doc.base_grand_total
+			base_total = ref_doc.get("base_rounded_total") or ref_doc.get("base_grand_total")
 			party_amount = flt(self.outstanding_amount / total * base_total, self.precision("grand_total"))
 		else:
 			party_amount = self.outstanding_amount
@@ -314,7 +315,6 @@ class PaymentRequest(Document):
 		payment_entry.update(
 			{
 				"mode_of_payment": self.mode_of_payment,
-				"reference_no": self.name,
 				"reference_date": nowdate(),
 				"remarks": "Payment Entry against {} {} via Payment Request {}".format(
 					self.reference_doctype, self.reference_name, self.name
@@ -323,7 +323,7 @@ class PaymentRequest(Document):
 		)
 
 		# Add reference of Payment Request
-		payment_entry.get("references")[0].payment_request = self.name
+		payment_entry.references[0].payment_request = self.name
 
 		# Update dimensions
 		payment_entry.update(
@@ -333,9 +333,6 @@ class PaymentRequest(Document):
 			}
 		)
 
-		payment_entry.received_amount = self.outstanding_amount
-		payment_entry.get("references")[0].allocated_amount = self.outstanding_amount
-
 		for dimension in get_accounting_dimensions():
 			payment_entry.update({dimension: self.get(dimension)})
 
@@ -420,12 +417,11 @@ class PaymentRequest(Document):
 			return create_stripe_subscription(gateway_controller, data)
 
 	def update_reference_advance_payment_status(self):
-		ref_doc = frappe.get_doc(self.reference_doctype, self.reference_name)
 		advance_payment_doctypes = frappe.get_hooks("advance_payment_receivable_doctypes") + frappe.get_hooks(
 			"advance_payment_payable_doctypes"
 		)
 		if self.reference_doctype in advance_payment_doctypes:
-			# set advance payment status
+			ref_doc = frappe.get_doc(self.reference_doctype, self.reference_name)
 			ref_doc.set_advance_payment_status()
 
 
@@ -475,6 +471,9 @@ def make_payment_request(**args):
 	if existing_payment_request_amount:
 		grand_total -= existing_payment_request_amount
 
+		if not grand_total:
+			frappe.throw(_("Payment Request is already created"))
+
 	if draft_payment_request:
 		frappe.db.set_value(
 			"Payment Request", draft_payment_request, "grand_total", grand_total, update_modified=False
@@ -571,29 +570,22 @@ def get_amount(ref_doc, payment_account=None):
 	return grand_total
 
 
-def get_existing_payment_request_amount(ref_dt, ref_dn, only_paid=False):
+def get_existing_payment_request_amount(ref_dt, ref_dn):
 	"""
-	Return the total amount of Payment Requests against a reference document. \n
+	Return the total amount of Payment Requests against a reference document.
-	If `only_paid` is True, it will return the total amount of paid Payment Requests. \n
-	Else, it will return the total amount of all Payment Requests.
 	"""
 	PR = frappe.qb.DocType("Payment Request")
 
-	if only_paid:
-		select = Sum(PR.grand_total - PR.outstanding_amount)
-	else:
-		select = Sum(PR.grand_total)
-
 	response = (
 		frappe.qb.from_(PR)
-		.select(select)
+		.select(Sum(PR.grand_total))
 		.where(PR.reference_doctype == ref_dt)
 		.where(PR.reference_name == ref_dn)
 		.where(PR.docstatus == 1)
 		.run()
 	)
 
-	return response[0][0] or 0
+	return response[0][0] if response else 0
 
 
 def get_gateway_details(args):  # nosemgrep
@@ -635,40 +627,13 @@ def make_payment_entry(docname):
 	return doc.create_payment_entry(submit=False).as_dict()
 
 
-def update_payment_req_outstanding_amount(pe_doc, cancel=False):
+def update_payment_requests_as_per_pe_references(references=None, cancel=False):
-	outstanding_amounts = get_outstanding_amount_of_payment_entry_references(pe_doc.references)
+	if not references:
+		return
 
-	for ref in pe_doc.references:
-		if not ref.payment_request:
-			continue
-
-		old_outstanding_amount = outstanding_amounts[ref.payment_request]
-
-		new_outstanding_amount = (
-			old_outstanding_amount + ref.allocated_amount
-			if cancel
-			else old_outstanding_amount - ref.allocated_amount
-		)
-
-		if not cancel and new_outstanding_amount < 0:
-			frappe.throw(
-				_(
-					"The allocated amount is greater than the outstanding amount of Payment Request {0}"
-				).format(ref.payment_request)
-			)
-
-		frappe.db.set_value(
-			"Payment Request",
-			ref.payment_request,
-			"outstanding_amount",
-			new_outstanding_amount,
-		)
-
-
-def update_payment_req_status(pe_doc, method):
 	payment_requests = frappe.get_all(
 		"Payment Request",
-		filters={"name": ["in", get_referenced_payment_requests(pe_doc.references)]},
+		filters={"name": ["in", get_referenced_payment_requests(references)]},
 		fields=[
 			"name",
 			"grand_total",
@@ -679,24 +644,40 @@ def update_payment_req_status(pe_doc, method):
 
 	payment_requests = {pr.name: pr for pr in payment_requests}
 
-	for ref in pe_doc.references:
+	for ref in references:
 		if not ref.payment_request:
 			continue
 
 		payment_request = payment_requests[ref.payment_request]
 
-		if payment_request["outstanding_amount"] == payment_request["grand_total"]:
+		# update outstanding amount
+		new_outstanding_amount = (
+			payment_request["outstanding_amount"] + ref.allocated_amount
+			if cancel
+			else payment_request["outstanding_amount"] - ref.allocated_amount
+		)
+
+		if not cancel and new_outstanding_amount < 0:
+			frappe.throw(
+				msg=_(
+					"The allocated amount is greater than the outstanding amount of Payment Request {0}"
+				).format(ref.payment_request),
+				title=_("Invalid Allocated Amount"),
+			)
+
+		# update status
+		if new_outstanding_amount == payment_request["grand_total"]:
 			status = "Initiated" if payment_request["payment_request_type"] == "Outward" else "Requested"
-		elif payment_request["outstanding_amount"] == 0:
+		elif new_outstanding_amount == 0:
 			status = "Paid"
-		elif payment_request["outstanding_amount"] > 0:
+		elif new_outstanding_amount > 0:
 			status = "Partially Paid"
 
+		# update database
 		frappe.db.set_value(
 			"Payment Request",
 			ref.payment_request,
-			"status",
+			{"outstanding_amount": new_outstanding_amount, "status": status},
-			status,
 		)
 
 
@@ -724,7 +705,7 @@ def get_dummy_message(doc):
 {%- else %}<p>Hello,</p>{% endif %}
 
 <p>{{ _("Requesting payment against {0} {1} for amount {2}").format(doc.doctype,
-    doc.name, doc.get_formatted("grand_total")) }}</p>
+	doc.name, doc.get_formatted("grand_total")) }}</p>
 
 <a href="{{ payment_url }}">{{ _("Make Payment") }}</a>
 

erpnext/controllers/accounts_controller.py

@@ -1943,35 +1943,27 @@ class AccountsController(TransactionBase):
 
 		self.set_advance_payment_status()
 
-	# todo: need to optimize
-	# todo: modularize
 	def set_advance_payment_status(self):
-		from erpnext.accounts.doctype.payment_request.payment_request import (
-			get_existing_payment_request_amount as get_paid_amount,
-		)
-
 		new_status = None
-		available_payment_requests = frappe.db.count(
-			"Payment Request",
-			{"reference_doctype": self.doctype, "reference_name": self.name, "docstatus": 1},
-		)
-		total_amount = self.get("rounded_total") or self.grand_total
-		paid_amount = get_paid_amount(self.doctype, self.name, only_paid=True)
 
-		if paid_amount == total_amount:
+		paid_amount = frappe.get_value(
-			new_status = "Fully Paid"
+			doctype="Payment Request",
-		elif paid_amount > 0 and paid_amount < total_amount:
+			filters={
-			new_status = "Partially Paid"
+				"reference_doctype": self.doctype,
-		elif self.doctype in frappe.get_hooks("advance_payment_receivable_doctypes"):
+				"reference_name": self.name,
-			if not available_payment_requests:
+				"docstatus": 1,
-				new_status = "Not Requested"
+			},
-			elif paid_amount == 0 or paid_amount == 0.0:
+			fieldname="sum(grand_total - outstanding_amount)",
-				new_status = "Requested"
+		)
-		elif self.doctype in frappe.get_hooks("advance_payment_payable_doctypes"):
+
-			if not available_payment_requests:
+		if not paid_amount:
-				new_status = "Not Initiated"
+			if self.doctype in frappe.get_hooks("advance_payment_receivable_doctypes"):
-			elif paid_amount == 0 or paid_amount == 0.0:
+				new_status = "Not Requested" if paid_amount is None else "Requested"
-				new_status = "Initiated"
+			elif self.doctype in frappe.get_hooks("advance_payment_payable_doctypes"):
+				new_status = "Not Initiated" if paid_amount is None else "Initiated"
+		else:
+			total_amount = self.get("rounded_total") or self.get("grand_total")
+			new_status = "Fully Paid" if paid_amount == total_amount else "Partially Paid"
 
 		if new_status == self.advance_payment_status:
 			return