-
Notifications
You must be signed in to change notification settings - Fork 217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: get payment gateways from ERPNext #44
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
03ab790
to
cbe8458
Compare
d50f51a
to
340b78c
Compare
9d8ee31
to
3307d25
Compare
3307d25
to
02b2c73
Compare
02b2c73
to
f9cd282
Compare
Maybe This would reduce the api interface to the necessary minimum and along with frappe/erpnext#37503 would make this refactoring a bit more polished. However so, at the same time, we need an API expansion in another place (frappe/erpnext#37502) to support known use cases (e.g. Lyra / PayZen) which I'm happy to contribute once stable. |
payments/payment_gateways/doctype/gocardless_settings/gocardless_settings.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless this refactoring is barred from orthogonal improvements / changes. According to above comment, I suggest to:
- Standardize around
on_payment_request_submission
hook forvalidate_transaction_currency
&validate_minimum_transaction_amount
(and all other relevant validations) - Consequentially, throw on
payment_gateway_validation
instead of casting any errors into silence by default (callers can do so if they so require). - Amplify the API with
on_payment_authorized_redirect
as described in the linked PR - Maybe even reduce the interface around
controller.request_for_payment(**payment_record)
If this refactoring is barred from these changes, in order to facilitate a coordinated implementation, I'd suggest to address these issues before the refactoring is completed in order to avoid having to coordinate an API refactoring across repositories later.
@blaggacao do changes separately once this PR is merged. |
frappe/erpnext#37182