Hide saved payment methods if their gateway is disabled #2975
Conversation
Aljullu
added
status: needs review
type: bug
|
Size Change: +348 B (0%) Total Size: 1.66 MB
ℹ️ View Unchanged
|
Aljullu
force-pushed
the
fix/2850-hide-saved-card-if-stripe-disabled
branch
from
e8cdb66
to
9bf9b36
Compare
| @@ -8,6 +8,7 @@ import { | |||
| usePaymentMethodDataContext, | |||
| } from '@woocommerce/base-context'; | |||
| import RadioControl from '@woocommerce/base-components/radio-control'; | |||
| import { PAYMENT_GATEWAY_SORT_ORDER } from '@woocommerce/block-settings'; | |||
There was a problem hiding this comment.
SavedPaymentMethodOptions is already consuming the Payment Method data context via usePaymentMethodDataContext. So you can grab the paymentMethods and paymentMethodsInitialized properties from there and use those to match against whether saved payment methods should display or not.
I even wonder if this should be handled upstream in the payment method data context and filter the available customerPaymentMethod by what is actually enabled in the paymentMethods object (which could be done via a effect in the context provider). That way this logic wouldn't need to exist in this component.
PAYMENT_GATEWAY_SORT_ORDER creates a coupling to server provided values that I think we should try to limit as much as possible in our components so down the road if this data is provided over REST or a transport other than via a WordPress host there's less refactoring involved.
There was a problem hiding this comment.
💯 good idea. Should be fixed now.
| /** | ||
| * Returns true if the payment gateway is enabled. | ||
| * | ||
| * @param object $gateway Payment gateway. | ||
| * @return boolean | ||
| */ | ||
| private function is_payment_gateway_enabled( $gateway ) { | ||
| return filter_var( $gateway->enabled, FILTER_VALIDATE_BOOLEAN ); | ||
| } | ||
|
|
||
| /** | ||
| * Add payment method data to Asset Registry. | ||
| */ | ||
| public function add_payment_method_script_data() { | ||
| // Enqueue the order of enabled gateways as `paymentGatewaySortOrder`. | ||
| if ( ! $this->asset_registry->exists( 'paymentGatewaySortOrder' ) ) { | ||
| $available_gateways = WC()->payment_gateways->payment_gateways(); | ||
| $this->asset_registry->add( 'paymentGatewaySortOrder', array_keys( $available_gateways ) ); | ||
| $payment_gateways = WC()->payment_gateways->payment_gateways(); | ||
| $enabled_gateways = array_filter( $payment_gateways, array( $this, 'is_payment_gateway_enabled' ) ); | ||
| $this->asset_registry->add( 'paymentGatewaySortOrder', array_keys( $enabled_gateways ) ); | ||
| } |
There was a problem hiding this comment.
Won't this break the logic that was implemented as a result of this comment?
If you implement the changes I suggested in an earlier comment, there won't be need for this.
There was a problem hiding this comment.
Not really: enabled and available are different concepts. For example, COD might be enabled but not available depending on the shipping method.
While this code is no longer needed for this PR, what do you think about keeping it? While I see you discussed that was not an information leak, at the same time I don't see any benefit on exposing disabled payment methods in the frontend, so I think we could keep the filter. Thoughts?
There was a problem hiding this comment.
For example, COD might be enabled but not available depending on the shipping method.
Ahh, right that makes sense. I'm fine with keeping this but you should make sure testing steps for the work done in #2157 are done to cover any unexpected regressions (don't expect any but still...)
There was a problem hiding this comment.
Nope, if you're happy with that, it works for me!
Aljullu
force-pushed
the
fix/2850-hide-saved-card-if-stripe-disabled
branch
from
40db3ee
to
a6281d1
Compare
|
Thanks for the review @nerrad. This is ready for another look. I see e2e tests are failing but I think it's not related to this pull. |
Fix #2850.
How to test the changes in this Pull Request:
Steps from #2850.
Enable Payment via Saved Cards.Save payment information to my account for future purchases.checkbox on checkout.WooCommerce > Settings > Paymentsand disable Stripe CC payment method.Visa ending in 4242 (expires 02/22)) is not there.Also, test there are no regressions with the testing steps introduced in #2934.
Changelog