Repository navigation
feat: let integrations narrow the PE transfer-method options - #51
Conversation
Add a `payment_transfer_method_options` hook: onload resolves the first registered resolver's non-empty list of allowed methods for the PE, and the PE client JS narrows the Select to it (resetting a now-invalid draft value to the first). No resolver / none installed -> the full default set, so razorpayx is unaffected; a resolver error degrades to the full set rather than breaking the form. Lets a bank integration hide UPI/Link its banks don't support without a global, install-order-dependent property setter. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Tick the box to add this pull request to the merge queue (same as
|
Confidence Score: 4/5Safe to merge after confirming the party_bank_account handler interaction is acceptable or addressed. The new server-side hook resolver and client-side narrowing logic are well-structured. The primary concern — the payment_integration_utils/payment_integration_utils/client_overrides/form/payment_entry.js — specifically the
|
| Filename | Overview |
|---|---|
| payment_integration_utils/payment_integration_utils/server_overrides/doctype/payment_entry.py | Adds _transfer_method_options helper and wires it into onload. First-resolver-wins logic, error isolation via try/except, and None fallback are all correct. No new issues beyond the already-flagged party_bank_account handler interaction. |
| payment_integration_utils/payment_integration_utils/client_overrides/form/payment_entry.js | Adds apply_transfer_method_options called only in refresh. The party_bank_account handler (line 116-122) still unconditionally sets payment_transfer_method to LINK or NEFT without checking the narrowed options, leaving the field in an invalid state until the next refresh — this pre-existing handler directly undermines the new narrowing feature. |
Reviews (3): Last reviewed commit: "fix: keep transfer-method resolver conve..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis change adds server-side resolution of Changes
Sequence Diagram(s)sequenceDiagram
participant PaymentEntry as PaymentEntry
participant Hooks as payment_transfer_method_options hooks
participant Frm as Payment Entry Form
PaymentEntry->>Hooks: call hook resolvers with doc
Hooks-->>PaymentEntry: return methods list or error
PaymentEntry-->>Frm: onload.payment_transfer_method_options
Frm->>Frm: apply_transfer_method_options(frm)
Frm->>Frm: set payment_transfer_method options
Frm->>Frm: reset invalid draft value
Related issues: None provided. Related PRs: None provided. Suggested labels: enhancement, payment-entry Suggested reviewers: None specified. Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
payment_integration_utils/payment_integration_utils/server_overrides/doctype/payment_entry.py (1)
46-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the failing hook path in the error log title.
With multiple registered resolvers,
frappe.log_error(title="payment_transfer_method_options resolver failed")gives no way to tell which hook path failed from the Error Log list view.♻️ Suggested improvement
except Exception: - frappe.log_error(title="payment_transfer_method_options resolver failed") + frappe.log_error(title=f"payment_transfer_method_options resolver failed: {path}") continue
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 81807c37-8dce-4b84-9348-b2f84e072893
📒 Files selected for processing (2)
payment_integration_utils/payment_integration_utils/client_overrides/form/payment_entry.jspayment_integration_utils/payment_integration_utils/server_overrides/doctype/payment_entry.py
…s as list
- _transfer_method_options: move `list(methods)` inside the try so a
malformed resolver return degrades to the default set instead of
crashing onload, matching the documented behaviour.
- payment_entry.js: pass options directly to set_df_property instead of
join("\n").
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01B7vdkobbjBpanpq9mK1gXq
| if methods: | ||
| return list(methods) | ||
| except Exception: | ||
| frappe.log_error(title="payment_transfer_method_options resolver failed") |
There was a problem hiding this comment.
@vorasmit should we add message for which path it failed and trackback
|
@vorasmit release this right now or wait? Also required to backport to version-15? |
…ix/pr-51 feat: let integrations narrow the PE transfer-method options (backport #51)
3 participants
Add a
payment_transfer_method_optionshook: onload resolves the first registered resolver's non-empty list of allowed methods for the PE, and the PE client JS narrows the Select to it (resetting a now-invalid draft value to the first). No resolver / none installed -> the full default set, so razorpayx is unaffected; a resolver error degrades to the full set rather than breaking the form. Lets a bank integration hide UPI/Link its banks don't support without a global, install-order-dependent property setter.