Change payment processing for subscriptions #3686
Conversation
opr
added
category: extensibility
|
Size Change: +137 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
opr
force-pushed
the
fix/allow-0-payment-on-crossgrade
branch
2 times, most recently
from
fd6e4a1 to
e08c56a
Compare
opr
requested review from
budzanowski and
senadir
and removed request for
a team
opr
changed the title
opr
changed the title
With the check removed, we will surface the saved payment methods even if they are not allowed. If we would want to remove the check here we should ensure that the payment methods are not sending the saved options otherwise we are exposing them to the user even if they are not working. This is how I understand the problem. |
I agree with Bartek here. Thomas did you confirm that the server is only surfacing saved payment methods for payment methods that are active? If not, we will need to make sure this is fixed on the server side. |
| ( { method: { gateway } } ) => { | ||
| const isAvailable = gateway in availablePaymentMethods; | ||
| return ( | ||
| isAvailable && | ||
| availablePaymentMethods[ gateway ].supports?.savePaymentInfo | ||
| ); | ||
| } |
There was a problem hiding this comment.
It's still fuzzy to me what this check does exactly? and why did it had supports?.savePaymentInfo?
| /** | ||
| * WooCommerce Blocks Checkout Update Order Meta (experimental). | ||
| * | ||
| * This hook gives extensions the chance to add or update meta data on the $order. | ||
| * | ||
| * This is similar to existing core hook woocommerce_checkout_update_order_meta. | ||
| * We're using a new action: | ||
| * - To keep the interface focused (only pass $order, not passing request data). | ||
| * - This also explicitly indicates these orders are from checkout block/StoreAPI. | ||
| * | ||
| * @see https://github.com/woocommerce/woocommerce-gutenberg-products-block/pull/3686 | ||
| * @internal This Hook is experimental and may change or be removed. | ||
| * | ||
| * @param WC_Order $order Order object. | ||
| */ | ||
| do_action( '__experimental_woocommerce_blocks_checkout_update_order_meta', $this->order ); | ||
|
|
There was a problem hiding this comment.
We did a lot of testing yesterday in our call so I'm confident that this works, I can't comment yet on the payment change until I understand it a bit better but approving this part.
github-actions
bot
added
status: ready to merge
and removed
status: needs review
labels
| } | ||
| if ( | ||
| config.supports && | ||
| typeof config.supports.requiresSaving !== 'undefined' && |
There was a problem hiding this comment.
If you use optional chaining here you can get rid of the previous line.
There was a problem hiding this comment.
Great idea, let's add this as a cooldown task to tidy this up so we don't pollute the PR. Thanks for the tip!
There was a problem hiding this comment.
Never mind, I see how small this change is, I thought it was going to be much larger. I've added it to the PR!
| * @param {boolean} props.allowsSaving Whether that payment method allows saving | ||
| * the data for future purchases. | ||
| * @param {boolean} props.requiresSaving Whether the payment method should display the option to save | ||
| * the details entered by the customer. |
There was a problem hiding this comment.
nit, comment block spacing here seems off.
| <PaymentMethodErrorBoundary isEditor={ isEditor }> | ||
| { children } | ||
| { customerId > 0 && allowsSaving && ( | ||
| { customerId > 0 && allowsSaving && ! requiresSaving && ( |
There was a problem hiding this comment.
hmm this conditional seems a bit wonky to me. Don't we want something like:
| { customerId > 0 && allowsSaving && ! requiresSaving && ( | |
| { customerId > 0 && ( allowsSaving || requiresSaving ) && ( |
In other words, "If a customer is logged in, and the payment method allows saving or requires saving, then show the checkbox".
| $saved_cards = isset( $this->settings['saved_cards'] ) ? $this->settings['saved_cards'] : false; | ||
| return isset( $this->settings['saved_cards'] ) ? 'yes' === $this->settings['saved_cards'] : false; |
There was a problem hiding this comment.
nice little improvement here :)
src/Payments/Integrations/Stripe.php
Outdated
| // See https://github.com/woocommerce/woocommerce-gateway-stripe/blob/ad19168b63df86176cbe35c3e95203a245687640/includes/class-wc-gateway-stripe.php#L271 and | ||
| // https://github.com/woocommerce/woocommerce/wiki/Payment-Token-API . | ||
| return apply_filters( 'wc_stripe_display_save_payment_method_checkbox', filter_var( $saved_cards, FILTER_VALIDATE_BOOLEAN ) ); | ||
| return ! apply_filters( 'wc_stripe_display_save_payment_method_checkbox', filter_var( $saved_cards, FILTER_VALIDATE_BOOLEAN ) ); |
There was a problem hiding this comment.
hmm this seems to indicate to me that I might be confused about what requiresSaving means. I'm a bit wary of the implicit connection between requiresSaving and allowsSavedCards. So essentially what we're saying here is that if anything filters this option so it returns false, that means the the saved payment methods ARE required (double negative resulting in that). I wonder if we should introduce a new filter that allows for explicitly setting this rather than implicitly?
There was a problem hiding this comment.
I wonder if we should introduce a new filter that allows for explicitly setting this rather than implicitly?
double down on this.
There was a problem hiding this comment.
Thomas and I chatted about this in our call and reviewed existing behaviour in the shortcode flow. It turns out we think we can improve this by retaining the explicit use of the filtered value and non filtered value and update the name of the properties to make it more clear how things are used. He's in the process of doing that now. My concerns and wariness with the current approach ended up being more an artifact of how the filtered value was returned (switching boolean) and the naming of the properties/values involved.
|
Thanks for the feedback everyone, I have updated the testing instructions and PR body to include more information about what changes I have made. Would you mind taking another look and giving it a quick spin? |
This is needed because some extensions rely on this action to add their information to the metadata of order items.
This is no longer needed.
This adds displaySavePaymentMethodCheckbox which will be used to determine if the checkbox to save payment methods should display.
This will determine whether the "Save payment information" checkbox will be displayed.
This is informed by the saved_cards option and the result of the wc_stripe_display_save_payment_method_checkbox filter.
We are going to rename the properties we use to determine whether saved cards are shown, or whether the save payment method checkbox is shown, so that their names are more descriptive of what they are for.
This is so we can hide the checkbox independently of hiding the saved payment methods.
This is because we are leaving it in to enable backward compatibility but payment methods registering using this should be informed of the change in case it gets removed.
This will allow us to show the save checkbox only if the payment method says it should be shown.
This makes the code a little tidier :)
…g.js Co-authored-by: Seghir Nadir <nadir.seghir@gmail.com>
senadir
force-pushed
the
fix/allow-0-payment-on-crossgrade
branch
from
99de9e5 to
e8ac0ee
Compare
mikejolley
added
the
skip-changelog
Saved payment methods are now correctly displayed - previously they were hidden based on whether or not the 'Save payment method' checkbox was visible, by running the
wc_stripe_display_save_payment_method_checkboxfilter.The checkbox display was ultimately decided by the Stripe extension, which set the checkbox to not display if a subscription product was in the cart. The logic was because the payment method should always be saved when buying a subscription, so displaying the checkbox was unnecessary.
To fix this I have changed the naming of some props used when registering payment methods:
savePaymentInfoto inform the UI whether we should show saved cards and the checkbox.showSavedCardsandshowSaveOptionto give more granular control over the UI to the payment methods. In our Stripe integration, we setshowSavedCardsbased on the value of thesaved_cardsoption in the Stripe extension, and we setshowSaveOptionbased on the result of thewc_stripe_display_save_payment_method_checkboxfilter.get_allow_saved_cardsmethod in theStripeclass to be namedget_show_saved_cardsand return whether thesaved_cardsoption is enabled, without running the checkbox filter mentioned above.get_show_save_optionwhich will return the result ofget_show_saved_cardsfiltered throughwc_stripe_display_save_payment_method_checkboxPaymentMethodTabcomponent to accept a booleanshowSaveOptionprop. It is this prop that determines if the checkbox should be displayed or not.supports.showSavedCardsto the same value assupports.savePaymentInfofor backward compatibility. If any payment methods are registering and still usingsavePaymentInfothen a deprecation notice will be emitted using@wordpress/deprecated.I have also modified the Checkout API so that it will fire a new action
__experimental_woocommerce_blocks_checkout_update_order_metawhich is handled in Subscriptions - this is required as extensions should be given the chance to update the metadata for the orders.Fixes #3616
Fixes #3620
Fixes #3271
Related PR in subs: 3963-gh-woocommerce/woocommerce-subscriptions
How to test the changes in this Pull Request
add/actions-hooks-for-update-meta-from-blocksonwoocommerce-subscriptionsSave payment methodcheckbox shows.