Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Hide saved payment methods if their gateway is disabled #2975

Merged
merged 7 commits into from
Aug 11, 2020
Merged

Hide saved payment methods if their gateway is disabled #2975

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a3c15d7
Hide saved payment methods if their gateway is disabled
Aljullu Aug 6, 2020
fc2dce9
Fix wrong type-def
Aljullu Aug 6, 2020
39f1518
Remove extra blank line
Aljullu Aug 6, 2020
6bf687c
Rename var
Aljullu Aug 6, 2020
9bf9b36
Use FILTER_VALIDATE_BOOLEAN instead of comparing to 'yes'
Aljullu Aug 6, 2020
a6281d1
Use data from Payment Method context instead of reading setting from …
Aljullu Aug 10, 2020
b512e00
Use data from Payment Method context instead of reading setting from …
Aljullu Aug 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 20 additions & 11 deletions assets/js/base/components/payment-methods/saved-payment-method-options.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 good idea. Should be fixed now.


/**
* @typedef {import('@woocommerce/type-defs/contexts').CustomerPaymentMethod} CustomerPaymentMethod
Expand Down Expand Up @@ -104,9 +105,15 @@ const SavedPaymentMethodOptions = ( { onSelect } ) => {
if ( paymentMethodKeys.length > 0 ) {
paymentMethodKeys.forEach( ( type ) => {
const paymentMethods = customerPaymentMethods[ type ];
if ( paymentMethods.length > 0 ) {
const enabledPaymentMethods = paymentMethods.filter(
( paymentMethod ) =>
PAYMENT_GATEWAY_SORT_ORDER.includes(
paymentMethod.method.gateway
)
);
if ( enabledPaymentMethods.length > 0 ) {
options = options.concat(
paymentMethods.map( ( paymentMethod ) => {
enabledPaymentMethods.map( ( paymentMethod ) => {
const option =
type === 'cc' || type === 'echeck'
? getCcOrEcheckPaymentMethodOption(
Expand All @@ -131,15 +138,17 @@ const SavedPaymentMethodOptions = ( { onSelect } ) => {
);
}
} );
currentOptions.current = options;
currentOptions.current.push( {
value: '0',
label: __(
'Use a new payment method',
'woo-gutenberg-product-blocks'
),
name: `wc-saved-payment-method-token-new`,
} );
if ( options.length > 0 ) {
currentOptions.current = options;
currentOptions.current.push( {
value: '0',
label: __(
'Use a new payment method',
'woo-gutenberg-product-blocks'
),
name: `wc-saved-payment-method-token-new`,
} );
}
}
}, [
customerPaymentMethods,
Expand Down
6 changes: 3 additions & 3 deletions assets/js/type-defs/contexts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,9 +256,9 @@
/**
* @typedef {Object} EditorDataContext
*
* @property {number} isEditor Indicates whether in the editor context.
* @property {number} currentPostId The post ID being edited.
* @property {Object} previewData Object containing preview data for the editor.
* @property {boolean} isEditor Indicates whether in the editor context.
* @property {number} currentPostId The post ID being edited.
* @property {Object} previewData Object containing preview data for the editor.
*/

/**
Expand Down
15 changes: 13 additions & 2 deletions src/Payments/Api.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,14 +76,25 @@ public function add_payment_method_script_dependencies( $dependencies, $handle )
return array_merge( $dependencies, $this->payment_method_registry->get_all_registered_script_handles() );
}

/**
* 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 ) );
}
Comment on lines +79 to 98
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I consider testing steps in #2934 already cover any potential regression introduced in this PR. I updated the testing steps in this PR to also point to testing steps in #2934. Do you think we need to add anything else?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, if you're happy with that, it works for me!


// Enqueue all registered gateway data (settings/config etc).
Expand Down