feat(payments): Add PaymentController v2 gateway support#51723
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds v2 gateway support to PaymentRequest. Introduces Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 2
🤖 Fix all issues with AI agents
In @erpnext/accounts/doctype/payment_request/payment_request.py:
- Around line 47-51: The current try/except around payment_app_import_guard()
won't catch the real exception because payment_app_import_guard converts
ImportError into a frappe.throw (frappe.ValidationError); update the import
handling so that you either (A) import payments.controllers.PaymentController
directly inside a try/except ImportError and return False on ImportError, or (B)
keep the guard but also catch frappe.ValidationError (or frappe.ValidationError
and ImportError) and return False; reference the existing
payment_app_import_guard and PaymentController symbols and ensure the function
returns False when the payments app is absent.
🧹 Nitpick comments (2)
erpnext/accounts/doctype/payment_request/payment_request.py (2)
52-55: Consider narrowing the exception catch or logging failures.The broad
except Exceptionis flagged by static analysis (Ruff BLE001). While this is acceptable for a detection function, silently swallowing all exceptions could hide unexpected issues during gateway resolution.♻️ Optional: Log the exception for debugging
try: controller = _get_payment_gateway_controller(payment_gateway) - except Exception: + except Exception as e: + frappe.log_error(f"Failed to resolve gateway {payment_gateway}: {e}", "V2 Gateway Detection") return False
248-255: Consider adding error handling forPaymentControllercalls.If
PaymentController.initiate()orget_payment_url()fails (e.g., gateway API error, network issue), the exception will propagate and potentially leave the Payment Request in an inconsistent state (submitted but without a payment URL).♻️ Suggested: Wrap in try-except with meaningful error
def _process_v2_gateway(self): """Process payment using the new PaymentController interface (v2 gateways).""" tx_data = self.get_tx_data() with payment_app_import_guard(): from payments.controllers import PaymentController - _controller, psl_name = PaymentController.initiate(tx_data, self.payment_gateway) - self.payment_url = PaymentController.get_payment_url(psl_name) + try: + _controller, psl_name = PaymentController.initiate(tx_data, self.payment_gateway) + self.payment_url = PaymentController.get_payment_url(psl_name) + except Exception as e: + frappe.throw( + _("Failed to initiate payment with gateway {0}: {1}").format(self.payment_gateway, str(e)), + title=_("Payment Gateway Error") + )
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
erpnext/accounts/doctype/payment_request/payment_request.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-16T05:33:58.723Z
Learnt from: Abdeali099
Repo: frappe/erpnext PR: 51078
File: erpnext/accounts/doctype/financial_report_template/financial_report_engine.py:486-491
Timestamp: 2025-12-16T05:33:58.723Z
Learning: In ERPNext/Frappe codebase, query.run(as_dict=True) returns frappe._dict objects that support both dict-style access (obj["key"]) and attribute-style access (obj.key). Therefore, attribute access on query results is valid and will not raise AttributeError. When reviewing Python code, prefer attribute access (obj.key) for readability where the key is known to exist, but ensure existence checks or fallback handling if there is any doubt about missing keys.
Applied to files:
erpnext/accounts/doctype/payment_request/payment_request.py
🧬 Code graph analysis (1)
erpnext/accounts/doctype/payment_request/payment_request.py (1)
erpnext/utilities/__init__.py (1)
payment_app_import_guard(44-53)
🪛 Ruff (0.14.10)
erpnext/accounts/doctype/payment_request/payment_request.py
54-54: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (2)
erpnext/accounts/doctype/payment_request/payment_request.py (2)
229-243: Control flow looks correct with clear separation of v2, phone, and legacy URL paths.The branching logic properly routes:
- v2 gateways →
_process_v2_gateway()→ email sent- Phone channel →
request_phone_payment()→ returns early (no email, as expected for phone)- Legacy URL →
set_payment_request_url()→ email sent
279-288: The review comment raises a valid concern about whetherloyalty_pointsanddiscount_amountshould be populated from the reference document inget_tx_data(). However, verification is inconclusive: the Payment Request itself doesn't store these fields, and loyalty adjustments are already deducted from thegrand_totalbefore being passed to PaymentController. Without access to the externalfrappe-paymentspackage's TxData interface definition, it cannot be confirmed whether populating these fields is required, optional, or intentionally omitted.
0spinboson
force-pushed
the
feat/payment-controller-v2-support
branch
2 times, most recently
from
81c8ebf to
593ea56
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
erpnext/accounts/doctype/payment_request/payment_request.py (2)
51-54: Consider logging unexpected exceptions for debuggability.The broad
except Exceptionis intentional for graceful fallback, but silently swallowing all exceptions could mask configuration errors or bugs in the gateway controller. Consider logging at debug level before returning False.Proposed improvement
try: controller = _get_payment_gateway_controller(payment_gateway) except Exception: + frappe.logger().debug(f"Could not get controller for gateway {payment_gateway}", exc_info=True) return False
264-279: Consider extracting the contact/address lookup into a helper.The Customer and Supplier branches have nearly identical logic. While acceptable, extracting this pattern could reduce duplication and make it easier to support additional party types in the future.
Proposed refactor
+ def _get_party_contact_and_address(self, party, contact_field, address_field): + """Helper to fetch primary contact and address for a party.""" + payer_contact = {} + payer_address = {} + if contact_val := getattr(party, contact_field, None): + if frappe.db.exists("Contact", contact_val): + payer_contact = frappe.get_doc("Contact", contact_val).as_dict() + if address_val := getattr(party, address_field, None): + if frappe.db.exists("Address", address_val): + payer_address = frappe.get_doc("Address", address_val).as_dict() + return payer_contact, payer_address + def get_tx_data(self): ... if party and self.party_type == "Customer": - if party.customer_primary_contact and frappe.db.exists("Contact", party.customer_primary_contact): - payer_contact = frappe.get_doc("Contact", party.customer_primary_contact).as_dict() - if party.customer_primary_address and frappe.db.exists("Address", party.customer_primary_address): - payer_address = frappe.get_doc("Address", party.customer_primary_address).as_dict() + payer_contact, payer_address = self._get_party_contact_and_address( + party, "customer_primary_contact", "customer_primary_address" + ) elif party and self.party_type == "Supplier": - if party.supplier_primary_contact and frappe.db.exists("Contact", party.supplier_primary_contact): - payer_contact = frappe.get_doc("Contact", party.supplier_primary_contact).as_dict() - if party.supplier_primary_address and frappe.db.exists("Address", party.supplier_primary_address): - payer_address = frappe.get_doc("Address", party.supplier_primary_address).as_dict() + payer_contact, payer_address = self._get_party_contact_and_address( + party, "supplier_primary_contact", "supplier_primary_address" + )
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
erpnext/accounts/doctype/payment_request/payment_request.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-16T05:33:58.723Z
Learnt from: Abdeali099
Repo: frappe/erpnext PR: 51078
File: erpnext/accounts/doctype/financial_report_template/financial_report_engine.py:486-491
Timestamp: 2025-12-16T05:33:58.723Z
Learning: In ERPNext/Frappe codebase, query.run(as_dict=True) returns frappe._dict objects that support both dict-style access (obj["key"]) and attribute-style access (obj.key). Therefore, attribute access on query results is valid and will not raise AttributeError. When reviewing Python code, prefer attribute access (obj.key) for readability where the key is known to exist, but ensure existence checks or fallback handling if there is any doubt about missing keys.
Applied to files:
erpnext/accounts/doctype/payment_request/payment_request.py
🪛 Ruff (0.14.10)
erpnext/accounts/doctype/payment_request/payment_request.py
53-53: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (3)
erpnext/accounts/doctype/payment_request/payment_request.py (3)
228-242: Logic is correct; consider adding a comment for clarity.The control flow is sound: v2 gateways and v1 URL payments both proceed to send emails, while v1 Phone payments return early (skipping email). The asymmetry is intentional but may confuse future readers. A brief inline comment explaining this would help.
281-290: The hardcodedNonevalues are correct.loyalty_pointsare processed duringmake_payment_request()before the Payment Request is created, and their adjustment is already reflected ingrand_total. Sinceget_tx_data()is called during payment processing (in_process_v2_gateway()), the payment system needs these fields inTxDatabut they should beNoneas the values have already been applied to the transaction amount.Likely an incorrect or invalid review comment.
247-254: Add error handling for failed payment initiation.If
PaymentController.initiate()fails or returnsNoneforpsl_name,get_payment_url(psl_name)may produce an invalid URL or raise an exception. Consider validatingpsl_namebefore proceeding.Proposed fix
def _process_v2_gateway(self): """Process payment using the new PaymentController interface (v2 gateways).""" tx_data = self.get_tx_data() with payment_app_import_guard(): from payments.controllers import PaymentController _controller, psl_name = PaymentController.initiate(tx_data, self.payment_gateway) + if not psl_name: + frappe.throw(_("Failed to initiate payment session for gateway {0}").format(self.payment_gateway)) self.payment_url = PaymentController.get_payment_url(psl_name)
0spinboson
force-pushed
the
feat/payment-controller-v2-support
branch
2 times, most recently
from
05b4a81 to
487f4c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
erpnext/accounts/doctype/payment_request/payment_request.py (1)
248-255: Consider error handling for PaymentController.initiate() failure.If
PaymentController.initiate()raises an exception, the submission will fail withoutpayment_urlbeing set. This is likely intentional (fail fast), but you may want to confirm this aligns with desired UX—users would see a generic error rather than a payment-specific message.If graceful handling is preferred:
🔧 Optional: Add explicit error handling
def _process_v2_gateway(self): """Process payment using the new PaymentController interface (v2 gateways).""" tx_data = self.get_tx_data() with payment_app_import_guard(): from payments.controllers import PaymentController - _controller, psl_name = PaymentController.initiate(tx_data, self.payment_gateway) - self.payment_url = PaymentController.get_payment_url(psl_name) + try: + _controller, psl_name = PaymentController.initiate(tx_data, self.payment_gateway) + self.payment_url = PaymentController.get_payment_url(psl_name) + except Exception as e: + frappe.throw(_("Failed to initiate payment: {0}").format(str(e)))erpnext/accounts/doctype/payment_request/test_payment_request.py (2)
913-971: Good coverage of tx_data and party contact/address handling.The tests verify:
- All required TxData fields are present
- Values map correctly from PaymentRequest
- Missing party returns empty dicts
- Nonexistent party handles gracefully
Consider adding a test for the positive case where a Customer with primary contact/address exists, to verify the data is actually fetched correctly.
973-1028: Flow routing tests verify correct branching.The tests correctly verify:
- v2 gateways trigger
_process_v2_gateway- v1 gateways trigger
set_payment_request_urlThe
prvariables (lines 987, 1019) are flagged as unused by static analysis. This is intentional since the tests verify method calls during creation, not the returned object. You can silence the linter with underscore prefix:- pr = make_payment_request( + _pr = make_payment_request(The v1 test has deep nesting (6 context managers). Consider using
contextlib.ExitStackfor readability in future tests:🔧 Optional: Reduce nesting with ExitStack
from contextlib import ExitStack def test_v1_gateway_uses_legacy_flow(self): so = make_sales_order(currency="INR") with ExitStack() as stack: stack.enter_context(patch( "...payment_request._is_v2_gateway", return_value=False)) mock_set_url = stack.enter_context(patch( "...PaymentRequest.set_payment_request_url")) stack.enter_context(patch( "...PaymentRequest.get_payment_url", return_value=PAYMENT_URL)) stack.enter_context(patch( "..._get_payment_gateway_controller")) stack.enter_context(patch( "...PaymentRequest.send_email")) make_payment_request(...) mock_set_url.assert_called_once()
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/accounts/doctype/payment_request/payment_request.pyerpnext/accounts/doctype/payment_request/test_payment_request.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-16T05:33:58.723Z
Learnt from: Abdeali099
Repo: frappe/erpnext PR: 51078
File: erpnext/accounts/doctype/financial_report_template/financial_report_engine.py:486-491
Timestamp: 2025-12-16T05:33:58.723Z
Learning: In ERPNext/Frappe codebase, query.run(as_dict=True) returns frappe._dict objects that support both dict-style access (obj["key"]) and attribute-style access (obj.key). Therefore, attribute access on query results is valid and will not raise AttributeError. When reviewing Python code, prefer attribute access (obj.key) for readability where the key is known to exist, but ensure existence checks or fallback handling if there is any doubt about missing keys.
Applied to files:
erpnext/accounts/doctype/payment_request/payment_request.pyerpnext/accounts/doctype/payment_request/test_payment_request.py
🧬 Code graph analysis (1)
erpnext/accounts/doctype/payment_request/test_payment_request.py (1)
erpnext/accounts/doctype/payment_request/payment_request.py (4)
_is_v2_gateway(39-58)get_tx_data(257-274)_get_party_contact_and_address(276-306)make_payment_request(628-766)
🪛 Ruff (0.14.10)
erpnext/accounts/doctype/payment_request/payment_request.py
53-53: Do not catch blind exception: Exception
(BLE001)
erpnext/accounts/doctype/payment_request/test_payment_request.py
987-987: Local variable pr is assigned to but never used
Remove assignment to unused variable pr
(F841)
1019-1019: Local variable pr is assigned to but never used
Remove assignment to unused variable pr
(F841)
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (5)
erpnext/accounts/doctype/payment_request/payment_request.py (4)
39-58: LGTM! Defensive v2 detection with graceful fallback.The function handles all edge cases appropriately:
- Falsy gateway check prevents unnecessary processing
- ImportError fallback ensures v1 compatibility when payments module lacks PaymentController
- The broad exception catch at line 53 is justified here since
_get_payment_gateway_controllercan fail in various ways (gateway not found, misconfiguration, etc.), and falling back to v1 is the correct behavior- Type vs instance check handles both class-based and instantiated controller returns
229-243: LGTM! Clean integration with preserved backward compatibility.The branching logic correctly:
- Routes v2 gateways to the new flow
- Preserves phone channel's early return (no email) behavior
- Maintains legacy URL flow for v1 gateways
- Applies email/communication logic consistently for both v2 and legacy URL flows
257-274: LGTM! Properly structured TxData payload.The tx_data dict correctly maps PaymentRequest fields to the expected TxData structure. The use of
self.doctypeandself.nameas reference fields is appropriate since the Payment Session Log should reference the PaymentRequest.Note:
loyalty_pointsanddiscount_amountare hardcoded toNone. If these should be populated from the reference document (e.g., Sales Order's loyalty program data), consider adding that in a follow-up.
276-306: LGTM! Robust party contact/address retrieval.The implementation handles edge cases well:
- Missing party_type/party returns empty dicts
- Deleted party handled via DoesNotExistError
- Existence checks before fetching Contact/Address prevent secondary errors
- Explicit field_map limits support to Customer/Supplier (appropriate for payment context)
erpnext/accounts/doctype/payment_request/test_payment_request.py (1)
874-911: Mock pattern may not work when payments module is absent.The tests patch
payments.controllers.PaymentControllerwithcreate=True, but the code under test does:from payments.controllers import PaymentControllerIf the
paymentsmodule isn't installed, this import raisesImportErrorbefore the patch takes effect. Thecreate=Trueflag creates the attribute, not the module itself.Consider patching the import mechanism to make the test self-contained:
🔧 More robust mock approach
def test_is_v2_gateway_detects_v2_controller(self): """_is_v2_gateway returns True for PaymentController subclasses.""" from erpnext.accounts.doctype.payment_request.payment_request import _is_v2_gateway + import sys + from unittest.mock import MagicMock # Create mock PaymentController class MockPaymentController: pass class MockV2Gateway(MockPaymentController): pass + # Mock the payments.controllers module + mock_module = MagicMock() + mock_module.PaymentController = MockPaymentController + sys.modules["payments.controllers"] = mock_module + + try: with patch( "erpnext.accounts.doctype.payment_request.payment_request._get_payment_gateway_controller", return_value=MockV2Gateway, ): - with patch("payments.controllers.PaymentController", MockPaymentController, create=True): - result = _is_v2_gateway("_Test Gateway") - self.assertTrue(result) + result = _is_v2_gateway("_Test Gateway") + self.assertTrue(result) + finally: + sys.modules.pop("payments.controllers", None)If the payments app is always installed in the test environment, this may work as-is. Please confirm the test passes in CI.
0spinboson
force-pushed
the
feat/payment-controller-v2-support
branch
2 times, most recently
from
c865a76 to
261173f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
erpnext/accounts/doctype/payment_request/payment_request.py (2)
254-256: Use underscore for intentionally unused variable.The
_controllervariable is assigned but never used. By convention, use a single underscore to indicate an intentionally discarded value.♻️ Suggested fix
- _controller, psl_name = PaymentController.initiate(tx_data, self.payment_gateway) + _, psl_name = PaymentController.initiate(tx_data, self.payment_gateway)
311-317: Consider combining existence check with document fetch.The pattern of
frappe.db.exists()followed byfrappe.get_doc()makes two database round trips. You could consolidate into a single call with exception handling.♻️ Suggested refactor
contact_name = getattr(party, contact_field, None) - if contact_name and frappe.db.exists("Contact", contact_name): - payer_contact = frappe.get_doc("Contact", contact_name).as_dict() + if contact_name: + try: + payer_contact = frappe.get_doc("Contact", contact_name).as_dict() + except frappe.DoesNotExistError: + pass address_name = getattr(party, address_field, None) - if address_name and frappe.db.exists("Address", address_name): - payer_address = frappe.get_doc("Address", address_name).as_dict() + if address_name: + try: + payer_address = frappe.get_doc("Address", address_name).as_dict() + except frappe.DoesNotExistError: + pass
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/accounts/doctype/payment_request/payment_request.pyerpnext/accounts/doctype/payment_request/test_payment_request.py
🚧 Files skipped from review as they are similar to previous changes (1)
- erpnext/accounts/doctype/payment_request/test_payment_request.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-16T05:33:58.723Z
Learnt from: Abdeali099
Repo: frappe/erpnext PR: 51078
File: erpnext/accounts/doctype/financial_report_template/financial_report_engine.py:486-491
Timestamp: 2025-12-16T05:33:58.723Z
Learning: In ERPNext/Frappe codebase, query.run(as_dict=True) returns frappe._dict objects that support both dict-style access (obj["key"]) and attribute-style access (obj.key). Therefore, attribute access on query results is valid and will not raise AttributeError. When reviewing Python code, prefer attribute access (obj.key) for readability where the key is known to exist, but ensure existence checks or fallback handling if there is any doubt about missing keys.
Applied to files:
erpnext/accounts/doctype/payment_request/payment_request.py
🪛 Ruff (0.14.10)
erpnext/accounts/doctype/payment_request/payment_request.py
53-53: Do not catch blind exception: Exception
(BLE001)
256-256: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (3)
erpnext/accounts/doctype/payment_request/payment_request.py (3)
39-58: LGTM - Gateway detection logic is well-structured.The function handles both the ImportError case (when payments package lacks PaymentController) and controller retrieval failures gracefully. The broad exception catch at line 53 is contextually appropriate since various errors can occur during gateway controller resolution, and falling back to v1 behavior is the correct recovery strategy.
The dual check for both
issubclass(class-based controllers) andisinstance(instantiated controllers) on lines 56-58 correctly handles both gateway implementation patterns.
229-243: Verify: V2 gateways with Phone channel now send emails.With v1 gateways, Phone payments return early (line 236) and skip email/communication. With v2 gateways, all channels fall through to the email block (lines 241-243).
If a v2 gateway is configured with
payment_channel == "Phone", this represents a behavioral change. Please confirm this is intentional for v2 gateway implementations.
268-287: LGTM - Transaction data structure is well-formed.The method correctly prepares the TxData payload. The
loyalty_pointsanddiscount_amountbeingNoneis appropriate since loyalty adjustments are applied togrand_totalupstream inmake_payment_request()before the PaymentRequest is created.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
erpnext/accounts/doctype/payment_request/test_payment_request.py (1)
1007-1037: Consider usingExitStackto reduce nesting depth.The test correctly verifies v1 flow routing, but the 5-level deep context manager nesting hurts readability.
♻️ Optional: Flatten with ExitStack
def test_v1_gateway_uses_legacy_flow(self): """v1 gateways should use set_payment_request_url flow.""" + from contextlib import ExitStack so = make_sales_order(currency="INR") - with patch( - "erpnext.accounts.doctype.payment_request.payment_request._is_v2_gateway", - return_value=False, - ): - with patch( - "erpnext.accounts.doctype.payment_request.payment_request.PaymentRequest.set_payment_request_url" - ) as mock_set_url: - with patch( - "erpnext.accounts.doctype.payment_request.payment_request.PaymentRequest.get_payment_url", - return_value=PAYMENT_URL, - ): - with patch( - "erpnext.accounts.doctype.payment_request.payment_request._get_payment_gateway_controller" - ): - with patch( - "erpnext.accounts.doctype.payment_request.payment_request.PaymentRequest.send_email" - ): - make_payment_request(...) - mock_set_url.assert_called_once() + with ExitStack() as stack: + stack.enter_context(patch( + "erpnext.accounts.doctype.payment_request.payment_request._is_v2_gateway", + return_value=False, + )) + mock_set_url = stack.enter_context(patch( + "erpnext.accounts.doctype.payment_request.payment_request.PaymentRequest.set_payment_request_url" + )) + stack.enter_context(patch( + "erpnext.accounts.doctype.payment_request.payment_request.PaymentRequest.get_payment_url", + return_value=PAYMENT_URL, + )) + stack.enter_context(patch( + "erpnext.accounts.doctype.payment_request.payment_request._get_payment_gateway_controller" + )) + stack.enter_context(patch( + "erpnext.accounts.doctype.payment_request.payment_request.PaymentRequest.send_email" + )) + + make_payment_request( + dt="Sales Order", + dn=so.name, + recipient_id="test@example.com", + mute_email=True, + submit_doc=True, + return_doc=True, + ) + mock_set_url.assert_called_once()
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/accounts/doctype/payment_request/payment_request.pyerpnext/accounts/doctype/payment_request/test_payment_request.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-16T05:33:58.723Z
Learnt from: Abdeali099
Repo: frappe/erpnext PR: 51078
File: erpnext/accounts/doctype/financial_report_template/financial_report_engine.py:486-491
Timestamp: 2025-12-16T05:33:58.723Z
Learning: In ERPNext/Frappe codebase, query.run(as_dict=True) returns frappe._dict objects that support both dict-style access (obj["key"]) and attribute-style access (obj.key). Therefore, attribute access on query results is valid and will not raise AttributeError. When reviewing Python code, prefer attribute access (obj.key) for readability where the key is known to exist, but ensure existence checks or fallback handling if there is any doubt about missing keys.
Applied to files:
erpnext/accounts/doctype/payment_request/payment_request.pyerpnext/accounts/doctype/payment_request/test_payment_request.py
🧬 Code graph analysis (2)
erpnext/accounts/doctype/payment_request/payment_request.py (1)
erpnext/utilities/__init__.py (1)
payment_app_import_guard(44-53)
erpnext/accounts/doctype/payment_request/test_payment_request.py (1)
erpnext/accounts/doctype/payment_request/payment_request.py (4)
_is_v2_gateway(39-58)get_tx_data(268-287)_get_party_contact_and_address(289-325)make_payment_request(647-785)
🪛 Ruff (0.14.10)
erpnext/accounts/doctype/payment_request/payment_request.py
53-53: Do not catch blind exception: Exception
(BLE001)
256-256: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (7)
erpnext/accounts/doctype/payment_request/payment_request.py (5)
39-59: Well-structured v2 gateway detection with appropriate defensive handling.The function correctly handles:
- Missing
paymentsapp (ImportError fallback)- Gateway resolution failures (broad exception catch with debug logging)
- Both class and instance checks for PaymentController
The broad
Exceptioncatch on line 53 is intentional here since_get_payment_gateway_controllercan raise various exceptions (gateway not found, misconfigured, etc.), and the goal is graceful fallback to v1 behavior. This matches the pattern used inpayment_app_import_guard().
229-243: Clean branching logic preserving backward compatibility.The control flow correctly:
- Routes v2 gateways to
_process_v2_gateway()- Preserves legacy phone payment flow with early return (no email)
- Falls back to legacy URL flow for v1 gateways
- Sends email/communication for non-phone flows when not muted
248-266: Solid v2 gateway processing with proper error handling.The method correctly:
- Uses
payment_app_import_guard()for import safety- Catches initiation failures and provides user-friendly localized error messages
- Validates that
psl_nameis returned before proceedingThe broad
Exceptioncatch on line 256 is appropriate here sincePaymentController.initiate()could fail for various gateway-specific reasons, and the goal is to surface a meaningful error to the user.
289-325: LGTM!Well-implemented with:
- Defensive early returns for missing party data
- Graceful
DoesNotExistErrorhandling for deleted parties/contacts/addresses- Clean mapping of party type to field names
- Individual error handling for contact vs address fetches
This follows the refactoring pattern mentioned in the commit message (consolidating existence check with document fetch).
268-287: Verify intention behindloyalty_pointsanddiscount_amountbeingNone.The
get_tx_data()method returnsloyalty_pointsanddiscount_amountasNone. While the test validates that these fields are present, it does not assert expected values. Loyalty points are processed separately inmake_payment_request()(lines 666-671) through a different workflow. If these fields should contain values for the PaymentController.initiate() call, they need to be populated here; otherwise, ensure the comment clarifies that these are intentionally handled outside the transaction data flow.erpnext/accounts/doctype/payment_request/test_payment_request.py (2)
5-7: LGTM!The added imports (
sysandMagicMock) are necessary for the mocking strategy that injectspayments.controllersviasys.modules, enabling tests to run regardless of whether the payments app is installed.
857-981: Good test coverage for v2 gateway detection and helpers.The tests effectively cover:
- Edge cases for
_is_v2_gateway(None, empty, nonexistent, v2 vs v1)- Required TxData fields in
get_tx_data- Graceful handling of missing/deleted parties
The
sys.modulespatching approach is smart—it enables testing the v2 detection logic without requiring thepaymentsapp to be installed, which aligns with the commit message about CI compatibility.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@erpnext/accounts/doctype/payment_request/payment_request.py`:
- Around line 296-353: In _get_party_contact_and_address ensure all
payer_contact values are normalized to strings so None doesn't leak into
tx_data: change the construction of payer_contact in the Contact block (the code
that sets "first_name": contact.first_name, etc.) to default first_name to an
empty string like the other fields (e.g., "first_name": contact.first_name or
""), leaving the rest of the None-safe fallbacks intact and keeping the
try/except around frappe.get_doc("Contact", ...).
- Around line 39-51: The _is_v2_gateway function only catches
frappe.ValidationError when importing payments, but any exception raised by
payments.utils.is_v2_gateway (or during import) can bubble up and break
before_submit; update _is_v2_gateway (which calls payment_app_import_guard and
payments.utils.is_v2_gateway) to catch all exceptions (e.g., Exception) as a
fallback, log the unexpected error (use frappe.log_error or process logger) for
diagnostics, and return False to ensure the code gracefully falls back to the v1
flow instead of crashing.
In `@erpnext/accounts/doctype/payment_request/test_payment_request.py`:
- Around line 900-964: Tests create a Payment Request (pr) and call
pr.insert(ignore_permissions=True) without setting mandatory fields (e.g.,
company), causing flaky failures; instead instantiate a valid Payment Request
before calling get_tx_data. Fix by either populating required fields on pr (set
"company" and any required gateway/doctype fields) before pr.insert, or replace
manual creation with the helper make_payment_request(..., return_doc=True,
submit_doc=False) and then call get_tx_data() on the returned doc; update
references in the test functions test_get_tx_data_returns_required_fields and
test_get_tx_data_uses_request_amount_not_grand_total to use the valid PR
creation approach.
- Around line 857-899: The tests for _is_v2_gateway currently patch only
"payments.utils" which causes the parent package import to fail; update the two
delegation tests (test_is_v2_gateway_delegates_to_payments_util and
test_is_v2_gateway_returns_false_when_payments_util_returns_false) to patch both
the parent package and the submodule in sys.modules (e.g. put a MagicMock for
"payments" and for "payments.utils") so that "from payments.utils import
is_v2_gateway" succeeds and then assert the behavior and mock calls as before.
🧹 Nitpick comments (2)
erpnext/accounts/doctype/payment_request/payment_request.py (1)
242-268: Log tracebacks in_process_v2_gateway()and consider narrowing/structuring the catch.
frappe.log_error(..., message=f"...{e}")drops the traceback; also logging raw exception strings can unintentionally include sensitive details depending on upstream errors.Proposed diff
try: _controller, psl_name = PaymentController.initiate(tx_data, self.payment_gateway) except Exception as e: # Log full exception for debugging, show generic message to user frappe.log_error( title=_("Payment Initialization Failed"), - message=f"Gateway: {self.payment_gateway}, Error: {e}", + message=f"Gateway: {self.payment_gateway}\n{frappe.get_traceback()}", ) frappe.throw(erpnext/accounts/doctype/payment_request/test_payment_request.py (1)
965-1017: PII minimization assertions are good; consider making the fixture deterministic.Right now the test passes even when no primary contact/address exists (because it conditionally asserts). If you want this to be a strong regression test, set up a customer with known
customer_primary_contact/customer_primary_addressand assert exact keys/values.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/accounts/doctype/payment_request/payment_request.pyerpnext/accounts/doctype/payment_request/test_payment_request.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-16T05:33:58.723Z
Learnt from: Abdeali099
Repo: frappe/erpnext PR: 51078
File: erpnext/accounts/doctype/financial_report_template/financial_report_engine.py:486-491
Timestamp: 2025-12-16T05:33:58.723Z
Learning: In ERPNext/Frappe codebase, query.run(as_dict=True) returns frappe._dict objects that support both dict-style access (obj["key"]) and attribute-style access (obj.key). Therefore, attribute access on query results is valid and will not raise AttributeError. When reviewing Python code, prefer attribute access (obj.key) for readability where the key is known to exist, but ensure existence checks or fallback handling if there is any doubt about missing keys.
Applied to files:
erpnext/accounts/doctype/payment_request/payment_request.pyerpnext/accounts/doctype/payment_request/test_payment_request.py
🧬 Code graph analysis (1)
erpnext/accounts/doctype/payment_request/test_payment_request.py (1)
erpnext/accounts/doctype/payment_request/payment_request.py (5)
_is_v2_gateway(39-51)get_tx_data(269-294)get_request_amount(372-387)_get_party_contact_and_address(296-352)make_payment_request(674-812)
🪛 Ruff (0.14.11)
erpnext/accounts/doctype/payment_request/payment_request.py
250-250: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (4)
erpnext/accounts/doctype/payment_request/payment_request.py (2)
222-238:before_submit()v2/v1 branching looks consistent and preserves the legacy “Phone” early-return.The ordering ensures: v2 routing happens when detected; legacy phone requests don’t emit email/communication (Line 226–230); legacy URL flow remains intact.
269-295: The field names inget_tx_data()are validated by the test suite.The test
test_get_tx_data_returns_required_fields()explicitly verifies all returned fields includingreference_docname. The test confirms the field is present and correctly set to the Payment Request name. If you need absolute confirmation that these field names match the currentpayments.types.TxDatadefinition, consult the payments module documentation or source directly for the currently installed version.erpnext/accounts/doctype/payment_request/test_payment_request.py (2)
4-8: Imports are fine; the new tests rely on module-injection, so keepsysonly if you fix the guard interaction below.No functional concerns with the imports themselves.
1018-1073: The duplicate class definition check is unnecessary; there's only oneTestPaymentRequestV2Gatewayclass in the file.Consider patching
PaymentRequest.make_communication_entryin these tests for robustness. Whilemute_email=Truecurrently guards against its execution (sincemake_communication_entry()is only called whennot (self.mute_email or self.flags.mute_email)), patching it would protect against unexpected database side effects if the mute email logic changes later.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@erpnext/accounts/doctype/payment_request/test_payment_request.py`:
- Around line 953-983: The test
test_get_party_contact_and_address_returns_whitelisted_fields_only can be
vacuous because the assertions are skipped if no primary Contact/Address exists;
ensure the test always exercises _get_party_contact_and_address by creating and
linking a Contact and Address for the Customer fixture before calling
pr._get_party_contact_and_address (or assert the fixtures contain a primary
contact/address). Specifically, create new Contact and Address docs (or fetch
and mark existing ones as primary) and link them to the Customer "_Test
Customer", save them, then call pr._get_party_contact_and_address so the
subsequent whitelist assertions run unconditionally.
- Around line 1006-1061: Tests rely on implicit/default payment gateway
selection which causes flakiness; update both tests to pass a known
payment_gateway_account (e.g., "_Test Gateway - INR - _TC") into
make_payment_request to pin the gateway, and add the complementary negative
assertion to each test so the non-selected flow is explicitly not called — in
test_v2_gateway_uses_process_v2_gateway pass payment_gateway_account="_Test
Gateway - INR - _TC" to make_payment_request and also assert
PaymentRequest.set_payment_request_url was not called; in
test_v1_gateway_uses_legacy_flow pass the same payment_gateway_account and
assert PaymentRequest._process_v2_gateway was not called; keep the existing
patches of _is_v2_gateway, PaymentRequest._process_v2_gateway,
PaymentRequest.set_payment_request_url, and PaymentRequest.send_email as-is.
♻️ Duplicate comments (1)
erpnext/accounts/doctype/payment_request/test_payment_request.py (1)
875-899: Fixsys.modulespatching: mock the parentpaymentspackage too (current tests likely fail).
from payments.utils import is_v2_gatewayrequires the parentpaymentsmodule to exist; patching only"payments.utils"is insufficient. This was already raised previously.Proposed fix
def test_is_v2_gateway_delegates_to_payments_util(self): """_is_v2_gateway delegates to payments.utils.is_v2_gateway.""" from erpnext.accounts.doctype.payment_request.payment_request import _is_v2_gateway # Mock the payments.utils module with is_v2_gateway returning True mock_utils = MagicMock() mock_utils.is_v2_gateway = MagicMock(return_value=True) - with patch.dict(sys.modules, {"payments.utils": mock_utils}): + mock_payments = MagicMock() + mock_payments.utils = mock_utils + with patch.dict(sys.modules, {"payments": mock_payments, "payments.utils": mock_utils}): result = _is_v2_gateway("_Test Gateway") self.assertTrue(result) mock_utils.is_v2_gateway.assert_called_once_with("_Test Gateway") def test_is_v2_gateway_returns_false_when_payments_util_returns_false(self): """_is_v2_gateway returns False when payments.utils.is_v2_gateway returns False.""" from erpnext.accounts.doctype.payment_request.payment_request import _is_v2_gateway # Mock the payments.utils module with is_v2_gateway returning False mock_utils = MagicMock() mock_utils.is_v2_gateway = MagicMock(return_value=False) - with patch.dict(sys.modules, {"payments.utils": mock_utils}): + mock_payments = MagicMock() + mock_payments.utils = mock_utils + with patch.dict(sys.modules, {"payments": mock_payments, "payments.utils": mock_utils}): result = _is_v2_gateway("_Test Gateway") self.assertFalse(result)
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
erpnext/accounts/doctype/payment_request/test_payment_request.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-16T05:33:58.723Z
Learnt from: Abdeali099
Repo: frappe/erpnext PR: 51078
File: erpnext/accounts/doctype/financial_report_template/financial_report_engine.py:486-491
Timestamp: 2025-12-16T05:33:58.723Z
Learning: In ERPNext/Frappe codebase, query.run(as_dict=True) returns frappe._dict objects that support both dict-style access (obj["key"]) and attribute-style access (obj.key). Therefore, attribute access on query results is valid and will not raise AttributeError. When reviewing Python code, prefer attribute access (obj.key) for readability where the key is known to exist, but ensure existence checks or fallback handling if there is any doubt about missing keys.
Applied to files:
erpnext/accounts/doctype/payment_request/test_payment_request.py
🧬 Code graph analysis (1)
erpnext/accounts/doctype/payment_request/test_payment_request.py (1)
erpnext/accounts/doctype/payment_request/payment_request.py (4)
_is_v2_gateway(39-51)get_tx_data(269-294)get_request_amount(372-387)_get_party_contact_and_address(296-352)
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (2)
erpnext/accounts/doctype/payment_request/test_payment_request.py (2)
4-8: Imports look fine (andsysis actually used).
857-1061: No duplication exists—the AI summary was incorrect. The classTestPaymentRequestV2Gatewayappears only once (line 857).However, the flow-selection tests
test_v2_gateway_uses_process_v2_gatewayandtest_v1_gateway_uses_legacy_flow(lines 1006 and 1031) don't configure an actual payment gateway account. They rely entirely on mocking_is_v2_gateway, which makes it impossible to verify that the gateway selection logic correctly routes to either_process_v2_gatewayorset_payment_request_urlbased on real gateway configuration. Consider adding a test variant that sets up an actual payment gateway document with v2 or v1 configuration before callingmake_payment_request.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@erpnext/accounts/doctype/payment_request/test_payment_request.py`:
- Around line 1044-1045: The test's allowed_contact_fields set in
test_payment_request.py is missing the "email" key used by
_get_party_contact_and_address(); update the allowed_contact_fields set (used in
the assertion around contact.keys()) to include both "email_id" and "email" so
the subset assertion passes when the contact dict contains the alias "email".
♻️ Duplicate comments (1)
erpnext/accounts/doctype/payment_request/payment_request.py (1)
337-343: Normalizefirst_nameto preventNonefrom leaking into tx_data.All other contact fields use
or ""for null-safety, butfirst_nameis passed through as-is. If the contact has nofirst_name, this will passNoneto the payment gateway, which may cause validation issues if it expects strings.🐛 Proposed fix
payer_contact = { - "first_name": contact.first_name, + "first_name": contact.first_name or "", "last_name": contact.last_name or "", "email_id": contact.email_id or "", "email": contact.email_id or "", # Alias for gateway compatibility "phone": contact.phone or contact.mobile_no or "", }
🧹 Nitpick comments (1)
erpnext/accounts/doctype/payment_request/test_payment_request.py (1)
1083-1152: Comprehensive flow selection tests with proper isolation.Both tests:
- Pass explicit
payment_gateway_accountto avoid fixture coupling- Assert the expected flow is called
- Assert the alternative flow is NOT called (mutual exclusivity)
The nested
withstatements could be simplified withcontextlib.ExitStack, but functionality is correct.♻️ Optional: Simplify nested context managers
from contextlib import ExitStack def test_v2_gateway_uses_process_v2_gateway(self): """v2 gateways should use _process_v2_gateway flow, not legacy flow.""" so = make_sales_order(currency="INR") with ExitStack() as stack: stack.enter_context(patch( "erpnext.accounts.doctype.payment_request.payment_request._is_v2_gateway", return_value=True, )) mock_process_v2 = stack.enter_context(patch( "erpnext.accounts.doctype.payment_request.payment_request.PaymentRequest._process_v2_gateway" )) mock_set_url = stack.enter_context(patch( "erpnext.accounts.doctype.payment_request.payment_request.PaymentRequest.set_payment_request_url" )) stack.enter_context(patch( "erpnext.accounts.doctype.payment_request.payment_request.PaymentRequest.send_email" )) make_payment_request(...) mock_process_v2.assert_called_once() mock_set_url.assert_not_called()
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/accounts/doctype/payment_request/payment_request.pyerpnext/accounts/doctype/payment_request/test_payment_request.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-16T16:00:08.157Z
Learnt from: barredterra
Repo: frappe/erpnext PR: 50159
File: erpnext/public/js/utils/sales_common.js:122-125
Timestamp: 2025-11-16T16:00:08.157Z
Learning: In ERPNext sales transactions (erpnext/public/js/utils/sales_common.js and erpnext/controllers/selling_controller.py), the company_contact_person field has different update behaviors: Frontend set_default_company_contact_person() should update the contact when the company field changes (intentional override on user action), while backend set_company_contact_person() in set_missing_values() should only fill empty fields (no override during save operations).
Applied to files:
erpnext/accounts/doctype/payment_request/payment_request.py
📚 Learning: 2025-12-16T05:33:58.723Z
Learnt from: Abdeali099
Repo: frappe/erpnext PR: 51078
File: erpnext/accounts/doctype/financial_report_template/financial_report_engine.py:486-491
Timestamp: 2025-12-16T05:33:58.723Z
Learning: In ERPNext/Frappe codebase, query.run(as_dict=True) returns frappe._dict objects that support both dict-style access (obj["key"]) and attribute-style access (obj.key). Therefore, attribute access on query results is valid and will not raise AttributeError. When reviewing Python code, prefer attribute access (obj.key) for readability where the key is known to exist, but ensure existence checks or fallback handling if there is any doubt about missing keys.
Applied to files:
erpnext/accounts/doctype/payment_request/payment_request.pyerpnext/accounts/doctype/payment_request/test_payment_request.py
🧬 Code graph analysis (2)
erpnext/accounts/doctype/payment_request/payment_request.py (1)
erpnext/utilities/__init__.py (1)
payment_app_import_guard(44-53)
erpnext/accounts/doctype/payment_request/test_payment_request.py (1)
erpnext/accounts/doctype/payment_request/payment_request.py (5)
_is_v2_gateway(39-57)make_payment_request(685-823)get_tx_data(279-304)get_request_amount(383-398)_get_party_contact_and_address(306-363)
🪛 Ruff (0.14.11)
erpnext/accounts/doctype/payment_request/payment_request.py
53-53: Do not catch blind exception: Exception
(BLE001)
256-256: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (9)
erpnext/accounts/doctype/payment_request/payment_request.py (4)
39-57: Well-structured defensive error handling for gateway detection.The function properly delegates to
payments.utils.is_v2_gateway()with robust fallback behavior:
- Catches
frappe.ValidationErrorwhen the payments app is missing (frompayment_app_import_guard)- Catches unexpected exceptions with logging to prevent submission failures
- Returns
Falseon any error to gracefully fall back to v1 flowThe catch-all
Exceptionhandler (flagged by Ruff BLE001) is intentional here—this is a detection utility where failing silently to v1 is the correct behavior to preserve backward compatibility.
228-243: LGTM - Clean routing logic with proper flow separation.The branching correctly handles:
- v2 gateways →
_process_v2_gateway()then email/communication- Phone channel →
request_phone_payment()with early return (no email)- Legacy v1 →
set_payment_request_url()then email/communicationThe early
returnon line 236 for Phone payments is correct since phone payments don't generate email/link communications.
248-277: Good error handling and defensive coding for external gateway integration.The method correctly:
- Logs detailed error info for debugging while showing user-friendly messages
- Validates that
psl_nameis returned before proceeding- Uses
hasattrcheck forpayment_session_logto handle environments where the custom field hasn't been added yetThe catch-all
Exception(Ruff BLE001) is appropriate here sincePaymentController.initiate()is an external call that could raise various unpredictable exceptions.
279-304: LGTM - Clean tx_data preparation with good documentation.The design decision to have
reference_doctype/reference_docnamepoint to the Payment Request (not the underlying document) is well-documented and makes sense for callback handling and reconciliation.Using
self.get_request_amount()correctly supports partial payment scenarios.erpnext/accounts/doctype/payment_request/test_payment_request.py (5)
5-7: LGTM - Required imports for mocking.The
sysimport is needed forpatch.dict(sys.modules, ...)andMagicMockis used for the mock utilities helper.
860-870: Well-designed helper for mocking the payments module hierarchy.The helper correctly mocks both the parent
paymentspackage andpayments.utilssubmodule, which is required for Python's import mechanism to work. The docstring clearly explains the rationale.
872-941: Comprehensive tests for_is_v2_gatewayedge cases and delegation.Good coverage including:
- Falsy inputs (None, empty string)
- Nonexistent gateway
- Proper delegation to
payments.utils.is_v2_gateway- Exception handling for unexpected errors
The tests properly use the module mocking helper to isolate behavior.
942-993: LGTM - Good test setup usingmake_payment_requesthelper.The tests properly use
make_payment_request()to create valid Payment Request documents with all mandatory fields populated. The partial payment test correctly verifies thatget_request_amount()is used rather thangrand_total.
1061-1081: Good edge case tests for missing/deleted party handling.These tests verify the graceful fallback behavior when the party doesn't exist or is deleted, matching the
DoesNotExistErrorhandling in the implementation.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@erpnext/accounts/doctype/payment_request/test_payment_request.py`:
- Around line 857-871: Add a tearDown method to TestPaymentRequestV2Gateway to
mirror TestPaymentRequest and ensure test isolation: implement def
tearDown(self): and call frappe.db.rollback() (optionally call
super().tearDown() if the base class defines it) so that any Contact/Address or
_Test Customer changes made in tests are rolled back after each test.
🧹 Nitpick comments (1)
erpnext/accounts/doctype/payment_request/test_payment_request.py (1)
1096-1165: LGTM!Flow selection tests correctly verify:
- v2 gateways use
_process_v2_gatewayand skip legacy URL flow- v1 gateways use
set_payment_request_urland skip v2 flow- Both tests use explicit
payment_gateway_accountfor deterministic behaviorOptional: Consider using
@patchdecorators orExitStackto reduce the deep nesting, though the current approach is functionally correct.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
erpnext/accounts/doctype/payment_request/test_payment_request.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-16T16:00:08.157Z
Learnt from: barredterra
Repo: frappe/erpnext PR: 50159
File: erpnext/public/js/utils/sales_common.js:122-125
Timestamp: 2025-11-16T16:00:08.157Z
Learning: In ERPNext sales transactions (erpnext/public/js/utils/sales_common.js and erpnext/controllers/selling_controller.py), the company_contact_person field has different update behaviors: Frontend set_default_company_contact_person() should update the contact when the company field changes (intentional override on user action), while backend set_company_contact_person() in set_missing_values() should only fill empty fields (no override during save operations).
Applied to files:
erpnext/accounts/doctype/payment_request/test_payment_request.py
📚 Learning: 2025-12-16T05:33:58.723Z
Learnt from: Abdeali099
Repo: frappe/erpnext PR: 51078
File: erpnext/accounts/doctype/financial_report_template/financial_report_engine.py:486-491
Timestamp: 2025-12-16T05:33:58.723Z
Learning: In ERPNext/Frappe codebase, query.run(as_dict=True) returns frappe._dict objects that support both dict-style access (obj["key"]) and attribute-style access (obj.key). Therefore, attribute access on query results is valid and will not raise AttributeError. When reviewing Python code, prefer attribute access (obj.key) for readability where the key is known to exist, but ensure existence checks or fallback handling if there is any doubt about missing keys.
Applied to files:
erpnext/accounts/doctype/payment_request/test_payment_request.py
🧬 Code graph analysis (1)
erpnext/accounts/doctype/payment_request/test_payment_request.py (1)
erpnext/accounts/doctype/payment_request/payment_request.py (5)
_is_v2_gateway(39-57)make_payment_request(685-823)get_tx_data(279-304)get_request_amount(383-398)_get_party_contact_and_address(306-363)
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (6)
erpnext/accounts/doctype/payment_request/test_payment_request.py (6)
5-7: LGTM!The
sysandMagicMockimports are necessary for the module-level mocking pattern used in the v2 gateway tests.
873-883: LGTM!Good helper pattern that properly mocks both the parent
paymentsmodule and the childpayments.utilsmodule. This addresses Python's module import requirements and enables clean test isolation.
885-954: LGTM!Comprehensive test coverage for
_is_v2_gateway:
- Edge cases (None, empty string, nonexistent gateway) properly delegate to the mocked utility
- Delegation tests verify correct interaction with
payments.utils.is_v2_gateway- Exception handling test confirms graceful fallback behavior
The module mocking pattern correctly addresses Python's import requirements.
955-1006: LGTM!Well-structured tests for
get_tx_data:
- Uses
make_payment_requestto properly construct valid Payment Requests- Verifies all TxData-required fields are present
- Correctly tests partial payment support by mocking
get_request_amount
1008-1072: LGTM!Comprehensive whitelist test that:
- Creates fixture data to ensure assertions always run (addresses past review)
- Includes both
email_idand- Verifies sensitive fields are excluded from the response
The fixture creation pattern is acceptable for integration tests with transaction rollback.
1074-1094: LGTM!Good edge case coverage for
_get_party_contact_and_address:
- Missing party (None values) returns empty dicts
- Deleted/nonexistent party handles
DoesNotExistErrorgracefully
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
0spinboson
force-pushed
the
feat/payment-controller-v2-support
branch
from
5c9e518 to
6ed0a8a
Compare
|
okay, should be done now, apologies for the earlier noise, should've done a PR in my own fork first. |
@0spinboson backport v15 v16 ? |
should work fine in v15 as well I think, aside from the payments CI updates that were needed for develop/v16. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #51723 +/- ##
===========================================
+ Coverage 79.78% 79.86% +0.07%
===========================================
Files 1159 1159
Lines 126362 126852 +490
===========================================
+ Hits 100822 101310 +488
- Misses 25540 25542 +2
🚀 New features to boost your workflow:
|
Adds documentation for the new PaymentController v2 architecture in the payments app, covering: - Architecture overview and components - Integration with ERPNext Payment Request - Implementing v2 payment gateways - Stripe v2 configuration guide - API reference - Troubleshooting guide Related: frappe/erpnext#51723, frappe/payments#192
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@erpnext/accounts/doctype/payment_request/test_payment_request.py`:
- Around line 1388-1395: The test unpacks two values from Payment
Request._get_party_contact_and_address but never uses the second one causing a
lint error; change the unpack target from "address" to a dummy name like "_" or
"_address" so the value is ignored (e.g., contact, _ =
pr._get_party_contact_and_address()), keeping the call to
_get_party_contact_and_address and the assert on contact intact.
- Around line 1409-1416: The test unpacks pr._get_party_contact_and_address()
into an unused variable named contact which triggers lint; change the unpack to
use a dummy/underscore name (e.g., _contact or _) instead of contact so the
address assertion remains the same. Locate the unpack in test_payment_request.py
where _get_party_contact_and_address() is called and replace the first variable
(contact) with a dummy name to satisfy Ruff while leaving the subsequent address
assertion untouched.
- Around line 1425-1450: The test assigns the return of make_payment_request to
an unused local variable pr in test_v2_gateway_sends_email_when_not_muted;
remove the unused assignment by calling make_payment_request(...) without
assigning its result (or replace pr with a throwaway name) so the function is
still invoked but no unused local remains, updating the
test_v2_gateway_sends_email_when_not_muted function accordingly.
f45179c to
5187e97
Compare
Thanks, will do. To clarify, how would you prefer I split up the PRs for the payments side, and/or what would you consider unrelated code? Like so?:
or would you want it to be split up further? |
0spinboson
force-pushed
the
feat/payment-controller-v2-support
branch
3 times, most recently
from
54632e9 to
232a1b5
Compare
Start with just the payment controller changes. Once the ERPNext side and Payments side controller changes are done, then others can be handled in separate PR.
Is this needed? |
|
Okay, will do in a day or two, thank you |
|
should be done |
|
This pull request has been automatically marked as inactive because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing. |
|
should be active in the background |
|
This pull request has been automatically marked as inactive because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing. |
|
Hiya. Hope you are well. any idea when I can expect feedback? |
|
what shall I do about the test failures do to the change to ledger entries (now submittable)? Leave it for now? |
Resolve the conflicts so CI can run. So that we can find out which tests break. |
0spinboson
force-pushed
the
feat/payment-controller-v2-support
branch
2 times, most recently
from
b721df4 to
13fdf35
Compare
|
done |
|
Will review by this week |
|
@0spinboson |
Add support for the new PaymentController interface from frappe/payments, enabling Payment Request to work with v2 gateways while maintaining backward compatibility with v1. Related: frappe/payments#192
…act/address handling
0spinboson
force-pushed
the
feat/payment-controller-v2-support
branch
from
13fdf35 to
b9e40a4
Compare
|
done |
|
@ruthra-kumar @0spinboson backport v-15 v-16 ? |
3 participants
Summary
Adds support for the new
PaymentControllerinterface from frappe/payments, enabling Payment Request to work with v2 payment gateways while maintaining backward compatibility with existing v1 gateways.Related: frappe/payments#192
Documentation: frappe/frappe_io#332
Background
This PR is the ERPNext-side integration for the PaymentController architecture introduced in frappe/payments. It builds on the foundation laid by @blaggacao in:
Rather than refactoring existing Payment Request logic, this PR adds a branch that detects v2 gateways and routes them through
PaymentController.initiate()while leaving v1 behavior untouched.Changes
Gateway detection:
_is_v2_gateway()- Thin wrapper aroundpayments.utils.is_v2_gateway()with import guardNew v2 flow methods:
_process_v2_gateway()- CallsPaymentController.initiate()and sets payment URLget_tx_data()- Prepares transaction data matching theTxDatainterface_get_party_contact_and_address()- Extracts payer contact/address infoModified
before_submit:Design Decisions
Delegation to payments app: Gateway detection logic lives in
payments.utils.is_v2_gateway()to avoid duplication and ensure consistency.Request amount: Uses
get_request_amount()(notgrand_total) to support partial payment scenarios, consistent with existing phone payment flow.Reference semantics:
tx_data.reference_doctype/docnamepoints to Payment Request (not the underlying invoice/order) because Payment Request handles callbacks and reconciliation.Contact/address fields: Returns only payment-relevant fields rather than full document data.
Flow
Dependencies
Requires frappe/payments with
PaymentControllersupport (#192).Falls back gracefully when payments app lacks v2 support.
Test Plan
_is_v2_gateway()returns True for v2 gateways, False for v1PaymentController.initiate()mute_emailflag for both flows