-
Notifications
You must be signed in to change notification settings - Fork 50
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
introduce woocommerce_paypal_payments_product_supports_prb filter #353
introduce woocommerce_paypal_payments_product_supports_prb filter #353
Conversation
Hi team, can we get some eyes on this? |
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.
Thanks for the contribution. I left a note about some changes that would make the hook more flexible for other plugins.
&& ( | ||
$product->is_type( array( 'external', 'grouped' ) ) | ||
|| ! $product->is_in_stock() | ||
&& ( ! apply_filters( 'woocommerce_paypal_payments_product_supports_prb', ! $product->is_type( array( 'external', 'grouped' ) ) && $product->is_in_stock(), $product ) |
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.
It would be better if we did apply_filters( 'woocommerce_paypal_payments_product_supports_prb', $product)
instead. This would allow the plugin adding the filter to have access to more info in the product. Also we should add the apply filter as the last part of the if statement. So we don't call the hook unless the other checks passed.
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.
Also I tend to dislike abbreviations as sometimes they usually need more context to understand. So I would suggest to change the hook name to woocommerce_paypal_payments_product_supports_payment_request_button
even though it's quite long.
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.
Not sure I understand the first comment. Do you mean something like:
$supports = ! is_checkout() && is_a( $product, \WC_Product::class ) && ! $product->is_type( array( 'external', 'grouped' ) ) && $product->is_in_stock();
$supports = apply_filters( 'woocommerce_paypal_payments_product_supports_payment_request_button', $product );
if ( ! $supports ) {
return;
}
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.
Oops. I misread the )
here. So we are actually good to go. Just changing the filter name would be enough.
I've tested this and I can still see the PayPal button in the cart when a product that does not support it is present. It should probably not show up there right? |
…ment_request_button to avoid abbreviates
As far as I know, it's fine there. For example, in my Name Your Price plugin... the payment request buttons don't work on the single product page because there's an extra input where the user enters their price. So users can click the PRB before entering a price and bypass the minimum price checks! Ideally, I'd love to enable/disable the buttons when the price is valid, but that's not possible yet. Once the Name Your Price product is in the cart though... it has a price. So in the cart the PRB don't cause any trouble. |
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.
Thanks @helgatheviking for contributing in this one. Keep rockin'
Glad to see this merged! @leonardola one more thing... what would be the easiest way to test this plugin is active? Like a |
I usually check the class like you posted. But you can also use this function. I'll probably do a release tomorrow so keep an eye open to update any code running on that hook on your sites |
Just got pinged that we'll need to postpone the release to after May 20 |
No problem, I'm not in a rush. Which class would you test for? |
You could check this class as it seems to be loaded on every request |
Description
Add a filter that will allow other plugins to conditionally disable the payment request buttons for single products that cannot support them.
Steps to test:
Should turn off payment request buttons on all single products even if they are enabled in the admin.
Because the
$product
object is passed, can also disable conditionally, for example, in the case of Mix and Match productsChangelog entry
Add
woocommerce_paypal_payments_product_supports_prb
filterCloses #234.
cc @jimjasson