Skip to content
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

Merged
merged 8 commits into from
Oct 14, 2023

Conversation

s-aga-r
Copy link
Contributor

@s-aga-r s-aga-r commented Sep 21, 2023

frappe/erpnext#37182

  • GoCardless Settings
  • GoCardless Mandate
  • GoCardless Templates
  • Mpesa Settings
  • stripe_integration.py
  • Dependencies
  • Fix Imports
  • Fix Unit Tests

@s-aga-r s-aga-r marked this pull request as draft September 21, 2023 02:05
@s-aga-r s-aga-r changed the title feat: get payment gateways from ERPNext refactor: get payment gateways from ERPNext Sep 21, 2023
@ankush

This comment was marked as resolved.

@s-aga-r s-aga-r force-pushed the GET-PAYMENTS-GATEWAYS branch 4 times, most recently from 03ab790 to cbe8458 Compare September 25, 2023 05:26
@s-aga-r s-aga-r force-pushed the GET-PAYMENTS-GATEWAYS branch 7 times, most recently from 9d8ee31 to 3307d25 Compare September 27, 2023 05:58
@s-aga-r s-aga-r marked this pull request as ready for review September 27, 2023 07:43
@s-aga-r s-aga-r requested a review from ankush September 27, 2023 07:45
@blaggacao
Copy link
Collaborator

blaggacao commented Oct 14, 2023

Maybe validate_transaction_currency weirdly called by erpnext.accounts.doctype.payment_request.payment_request.{get_request_acount,get_payment_url} and validate_minimum_transaction_amount called by [idem].get_payment_url should both be folded into on_payment_request_submission, instead?

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.

Copy link
Collaborator

@blaggacao blaggacao left a 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 for validate_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.

@s-aga-r
Copy link
Contributor Author

s-aga-r commented Oct 14, 2023

@blaggacao do changes separately once this PR is merged.

@s-aga-r s-aga-r merged commit 3fc3f68 into frappe:develop Oct 14, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants