-
Notifications
You must be signed in to change notification settings - Fork 69
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
Register only reusable payment methods when adding a payment method #4427
Register only reusable payment methods when adding a payment method #4427
Conversation
👀 |
If I enabled giropay, sofort and sepa inside UPE, then visiting http://localhost:8082/my-account/add-payment-method/ should show cc, giropay, sofort and sepa right? |
Also, do we need to worry about the failed tests, or is this something we need to address separately in the |
In my http://localhost:8082/my-account/add-payment-method/, I see credit card and SEPA, but I don't see giropay and sofort. My checkout page does show those though. Is this the same case for you? |
@harriswong Yes, that's the correct behavior. Only reusable payment gateways will be available here, so SEPA and CC should be the only gateways that appear when adding a payment method for future use. The undefined UPEElement error you mentioned earlier is now resolved, but we're now getting these errors when submitting the form:
|
It turns out that the selectors used for payment gateways on the Add Payment Method page differs from those used on checkout and other pages, so @harriswong After rebuilding JS, this should be working well now, as in, both credit card and SEPA payment methods can be added. It does surface another issue where SEPA tokens are appearing in the list of credit card tokens(where they've always appeared because it was the only place) when they should appear inside the SEPA gateway. That warrants a separate issue so I think we can push forward with this one. |
selectedGatewayId = $( | ||
'li.wc_payment_method input.input-radio:checked' | ||
).attr( 'id' ); | ||
} else if ( $( 'li.woocommerce-PaymentMethod' ).length ) { |
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.
Ah I see, 'li.woocommerce-PaymentMethod'
is used inside http://localhost:8082/my-account/add-payment-method/. I was trying to look for this on the http://localhost:8082/checkout/ but I can't find it.
I wonder if it's worth adding a comment here so we don't accidentally remove it in the future.
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.
Yes! This was a head-scratcher for me as well. Comment added.
if ( | ||
'woocommerce_payments' !== | ||
$( | ||
"#add_payment_method input:checked[name='payment_method']" |
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.
Are we removing this because in http://localhost:8082/my-account/add-payment-method/, it will no longer be only "Credit Card/debit card"?
Do you know why we needed to return;
early before?
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.
Are we removing this because in http://localhost:8082/my-account/add-payment-method/, it will no longer be only "Credit Card/debit card"?
Yep. woocommerce_payments
is the attribute we'll see for Credit Card/debit card while other gateways will append the gateway name, like woocommerce_payments_sepa_debit
.
Do you know why we needed to return; early before?
I don't exactly 🤔.
} | ||
|
||
if ( is_add_payment_method_page() ) { | ||
return $reusable_methods; |
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 think it makes sense that http://localhost:8082/my-account/add-payment-method/ only display payment methods that are saved, but not every single payment method we support. 👍
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 tested this on both checkout and my my-account/add-payment-method/
. I have some questions but LGTM otherwise!
With multiple gateways being registered, the Add Payment Method page is broken with the error,
This payment method does not support recurring payments.
and new payment methods cannot be added.Fixes #4396
Changes proposed in this Pull Request
This ensures that only reusable payment methods are available on the My Account > Payment Methods > Add Payment Method page rather than all UPE methods.
Testing instructions
npm run start
, enable WooPay and the UPE.npm run changelog
to add a changelog file, choosepatch
to leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge