-
Notifications
You must be signed in to change notification settings - Fork 68
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
Enable the use of saved SEPA payments #1540
Conversation
19e5fb2
to
a4c0527
Compare
Hey @harriswong ! Have you requested a review for this within your team? There is a recent change in approach discussed in slack (p1618348326004200-slack-C01JC3PBZL6). So @Automattic/gamma will be happy to review it at the final stage 🙂 cc @c-shultz |
Hi @vbelolapotkov ! Nope, not yet. I will wait for our team to review first, then I will add gamma back to the final stage. Thanks! |
3b65b61
to
7b12572
Compare
This has been renamed
7b12572
to
89e04f2
Compare
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.
I have tested different scenarios and all worked:
- Save SEPA for existing customer
- Save SEPA for guest customer creating account on checkout
- Payment using the saved method
- Verify CC was still working
The code is also good, the structure is similar to what Stripe plugin is doing, which makes sense.
A couple of points:
- Compatibility with Stripe plugin. Thanks for pointing it out. We can create a separate issue but I think we need to address it.
- On My Account: Add SEPA Payment method, delete payment method and set as default will be handled on Fix Add payment method on My Account for SEPA #1538
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.
Thank you for enabling the saved SEPA payment @harriswong! Worked like a charm 🎉 !
Architecture-wise looks also good. I left a few comments, which should make the changes consistent with the existing codebase, but overall great job here!
/** | ||
* Controls the output for SEPA on the my account page. | ||
* | ||
* @param array $item Individual list item from woocommerce_saved_payment_methods_list. |
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.
Doc-block alignment.
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.
I meant to add one more space in $item
(before 'Individual'), so the descriptions get aligned.
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.
Woops. Fixed. Added spaces so variable and description are aligned vertically.
Also updated some phpdoc block spacings.
On it 👀 |
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.
@harriswong: LGTM,
PS: See also my responses on doc-blocks.
@kalessil Thanks for taking another look. I added some spaces between the variable name and description. Now it should be vertically aligned. |
@harriswong, thanks! Let's merge it then 🎉 |
Fixes #1471
Changes proposed in this Pull Request
Payment_Method
class forcard
,sepa_debit
. Planning on addingsofort
andgiropay
later.Concerns
Testing instructions
Similar to the setup in #1537
_wcpay_feature_sepa
to1
/wp-admin/admin.php?page=wc-settings&tab=checkout§ion=woocommerce_payments