Updated WooPayments MultiCurrency integration with Product Add-Ons #9070
Conversation
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: +9.7 kB (+1%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
bborman22
requested review from
a team and
lovo-h
and removed request for
a team
There was a problem hiding this comment.
Thank you for addressing these issues 🙏
The code changes are looking good, but I was not able to observe some of the behavior, as described. I also left a few nit comments that we may want to apply before merging 😎
Testing
Bugs
- ✅ In the cart, notice that the Flat Fee is added to the name of the add-on and is displayed as $0.00.
- ❌ Then, increase the quantity -- notice that the product price, which represents the price of the single unit, changes (this doesn't happen for products without add-ons).
- I did notice this bug, but only with the changes in this PR.
Fixes
- ✅ In the cart, the $0.00 should not be visible.
- ✅ In the cart, notice that B's price shows up after the add-on value, like so (+$xx).
- ✅ Change the currency and notice that B's price is converted.
- ❌ Increase the product quantity and ensure that the product price doesn't change.
- The product price, which represents the price of the single unit, changes.
- ❌ In WooCommerce > Orders find the order you submitted -- ensure that B's price shows up correctly there.
- ❌ The product price is the total divided by the quantity.
- The add-on doesn't display the price next to it. But I don't know if it should.
- ❌ By default, quantity and percentage based prices do not show up in the cart.
- They show up even without the snippets activated.
- ❌ Ensure that the prices in the cart are correct, even when changing the currency.
- ❌ Add-on C - The base price plus the add-on price adds up to the total price, but the displayed add-on price is incorrect.
- ✅ Add-on D - The base price plus the add-on price add up to the total price and the displayed add-on price is correct.
- ✅ Ensure that both the product price and the add-on prices are correct when increasing the product quantity.
Other Tests
- ❌ Ensure that the prices in the cart are correct for Quantity add-ons with Adjust Price set to
Quantity Based.
includes/multi-currency/Compatibility/WooCommerceProductAddOns.php
Outdated
Show resolved
Hide resolved
| // Quantity/multiplier add on needs to be split, calculated, then multiplied by input value. | ||
| $addon_price = $this->multi_currency->get_price( $addon['price'] / $addon['value'], 'product' ) * $addon['value']; | ||
| } | ||
| // phpcs:ignore |
There was a problem hiding this comment.
Nit: Can we remove this phpcs:ignore and @psalm-suppress if we make the following change, above:
if ( class_exists( 'WC_Product_Addons_Helper' ) ) {
includes/multi-currency/Compatibility/WooCommerceProductAddOns.php
Outdated
Show resolved
Hide resolved
includes/multi-currency/Compatibility/WooCommerceProductAddOns.php
Outdated
Show resolved
Hide resolved
includes/multi-currency/Compatibility/WooCommerceProductAddOns.php
Outdated
Show resolved
Hide resolved
| $addon_price = wc_price( \WC_Product_Addons_Helper::get_product_addon_price_for_display( $addon['price'], $cart_item['data'], true ) ); | ||
| /* translators: %1$s custom addon price in cart */ | ||
| $value .= sprintf( _x( ' (%1$s)', 'custom price addon price in cart', 'woocommerce-payments' ), $addon_price ); | ||
| $addon['display'] = $value; |
There was a problem hiding this comment.
Nit: Should we add a comment indicating that if display is set, it overrides value? I'm not certain it does but from my testing, it looks like it does.
There was a problem hiding this comment.
This is also 1-1 identical to how Product Add-Ons handles the display key -- in general, the display key is not actively used, but is there for backwards compatibility.
includes/multi-currency/Compatibility/WooCommerceProductAddOns.php
Outdated
Show resolved
Hide resolved
| $addon['value'] = $addon['price']; | ||
| if ( 'flat_fee' === $addon['price_type'] && $addon['price'] && $add_price_to_value ) { | ||
| /* translators: %1$s flat fee addon price in order */ | ||
| $value .= sprintf( _x( ' (+ %1$s)', 'flat fee addon price in order', 'woocommerce-payments' ), $price ); |
There was a problem hiding this comment.
We may want to consider handling things more gracefully when $price is not defined. Which can happen if class_exists( '\WC_Product_Addons_Helper' ) resolves to false.
There was a problem hiding this comment.
I thought about this, but couldn't find a case where the WC_Product_Addons_Helper wouldn't be defined -- do you have a specific case in mind?
Right now, if this class is not defined, the the add-on value will be the same as the one set by Product Add-Ons, i.e. not converted by the selected currency. Would you prefer to have a different fallback here?
There was a problem hiding this comment.
Although it's unlikely, this is one scenario where this may happen:
- We remove
WC_Product_Addons_Helperin a future version ofwoocommerce-product-addons. - The merchant installs the future version of
woocommerce-product-addons. - The merchant installs this version of WooPayments.
Although this is possible, it's unlikely, so we can proceed without making modifications.
|
Thank you @lovo-h for the thorough review! 🙏 I pushed some fixes both on this branch and on: https://github.com/woocommerce/woocommerce-product-addons/pull/1048, so feel free to pull both branches and have another look!
This bug is a bit random, but you are right that it was not completely fixed by the time you tested this PR. With the latest stable WooPayments and Product Add-Ons versions active, I am able to replicate this issue by:
Screen.Capture.on.2024-07-15.at.12-52-05.mp4Don't worry too much if you cannot replicate it -- let's make sure that you don't see it after the latest changes. |
There was a problem hiding this comment.
Great work! Code changes look good and test well 👍
Fixes
- 🟢 In the cart, the $0.00 should not be visible.
- 🟢 In the cart, notice that B's price shows up after the add-on value, like so (+$xx).
- 🟢 Change the currency and notice that B's price is converted.
- ✅ Increase the product quantity and ensure that the product price doesn't change.
- ✅ In WooCommerce > Orders find the order you submitted -- ensure that B's price shows up correctly there.
- ❓ The add-on doesn't display the price next to it. I'm not sure if it should.
- ✅ By default, quantity and percentage based prices do not show up in the cart. To make them show up to confirm that they are correct use these snippets.
- ✅ Ensure that the prices in the cart are correct, even when changing the currency.
- 🟢 Ensure that both the product price and the add-on prices are correct when increasing the product quantity.
Other Tests
- ✅ Ensure that the prices in the cart are correct for Quantity add-ons with Adjust Price set to
Quantity Based. - ✅
npm run test:php.
There was a problem hiding this comment.
Hi @jimjasson
I added a couple of comments, but I'm trying to test it on the jurassic ninja site and having problems getting the numbers to add up.
Using a simple product with price 21 and the following addons:
The calculation in the cart differs from what I see on the single product page even with the default currency ($):
It seems to me the total should be $57.30, with the product price being $47.30.
The calculation on the single product page is what I'd expect and it also aligns with what I see in PAO trunk locally:
| */ | ||
| $add_price_to_value = apply_filters( 'woocommerce_addons_add_cart_price_to_value', false, $cart_item ); | ||
|
|
||
| if ( 0.0 === (float) $addon['price'] ) { |
There was a problem hiding this comment.
This might be good enough here, but comparing floats for equality might cause subtle problems. Perhaps 0 is a special case here, but I think price can often be filtered and calculated, so maybe not totally safe?
There was a problem hiding this comment.
In the Product Add-Ons repo, we do this instead: https://github.com/woocommerce/woocommerce-product-addons/blob/trunk/includes/class-wc-product-addons-cart.php#L769, but Woo Payments repo doesn't allow non-strict comparisons and the float method was previously used here: https://github.com/Automattic/woocommerce-payments/blob/develop/includes/multi-currency/Compatibility/WooCommerceProductAddOns.php#L237
|
|
||
| if ( 0.0 === (float) $addon['price'] ) { | ||
| $value .= ''; | ||
| } elseif ( 'percentage_based' === $addon['price_type'] && 0.0 === (float) $price ) { |
There was a problem hiding this comment.
Same here with comparing floats for equality.
| } | ||
| } else { | ||
| } elseif ( 'percentage_based' === $addon['price_type'] && $addon['price'] && $add_price_to_value ) { | ||
| // Get the percentage cost in the currency in use, and set the meta data on the product that the value was converted. | ||
| $_product = wc_get_product( $cart_item['product_id'] ); | ||
| $price = $this->multi_currency->get_price( $price, 'product' ); | ||
| $_product->set_price( $price * ( $addon['price'] / 100 ) ); |
There was a problem hiding this comment.
Out of scope for this PR, but interesting that we update the product price with percentage-based add-ons, but don't update it for quantity-based add-ons.
There was a problem hiding this comment.
Yes, this happens because the percentage amount needs to be calculated on top of the converted product price. Flat fees and quantity based add-ons are added on top of the product price and they don't need the product price to make any calculation about the actual add-on value.
There was a problem hiding this comment.
Thanks for the explanation.
| @@ -110,37 +110,64 @@ public function product_addons_params( array $params ): array { | |||
| */ | |||
| public function get_item_data( $addon_data, $addon, $cart_item ): array { | |||
There was a problem hiding this comment.
Since this method is very similar to \WC_Product_Addons_Cart::get_item_data, would it be worth it to add a comment to PAO to reflect changes in \WC_Product_Addons_Cart::get_item_data here in Woo Payments, too, so we can avoid things being out of sync in the future? (and similarly with other methods here that have counterparts in PAO)
Alternatively, we could refactor \WC_Product_Addons_Cart::get_item_data (or extract the calculation part out to a separate method) and call it from WooPayments, it's essentially a static method anyway, right?
There was a problem hiding this comment.
Added some comments here.
There was a problem hiding this comment.
After the changes, the numbers now match. 🎉
I wasn't able to check out with product qty > 1 due to the rounding problem with Level 3 data, but I will create a separate issue for that, since it seems that this problem also happens with the latest stable WooPayments.
There was a problem hiding this comment.
Tested and reviewed from different angles, seems to be working as expected now.
The remaining issue was created as #9114.
|
Thank you Peter! Will merge this PR after: https://github.com/woocommerce/woocommerce-product-addons/pull/1048 gets released, by the end of July. |
Fixes #8911
Changes proposed in this Pull Request
This PR updates the integration between WooPayments MultiCurrency and Product Add-Ons which hasn't been reviewed after the Product Add-Ons v6.x.x releases.
One of the major changes in the newer Product Add-Ons versions is that the way add-on prices show up in the cart has changed. Add-on prices are no longer added after the add-on name, but are added after the add-on's value. Moreover, Product Add-Ons has simplified how flat fee prices show up in the cart to make it more evident to shoppers how the cart total is calculated.
Therefore, this PR:
Testing instructions
First, set up a MultiCurrency environment:
trunkor the latest stable version.Now, let's see some bugs:
Finally, let's see some fixes:
npm run changelogto add a changelog file, choosepatchto 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