Automattic / woocommerce-subscriptions-core

Subscriptions core package for WooCommerce
Other
87 stars 33 forks source link

Add filter to allow recurring cart shipping taxes to be copied over from standard package rates. #430

Closed ecairol closed 1 year ago

ecairol commented 1 year ago

Description

This enhancement fixes an error "Invalid recurring payment method" that happens if:

The WCS plugin does a validation process (validate_recurring_shipping_methods()) and compares whether the recurring_cart package rates are the same as the standard package rates.

The expected behavior is that they both are the same, however, the standard package rates contain some data from Avatax and that fails the condition to be true as expected.

This is an example of the contents of $standard_packages[ $package_index ]['rates'], the Array that contains some data from Avatax:

(
    [flat_rate:2] => WC_Shipping_Rate Object
        (
            [data:protected] => Array
                (
                    [id] => flat_rate:2
                    [method_id] => flat_rate
                    [instance_id] => 2
                    [label] => Flat rate
                    [cost] => 15.00
                    [taxes] => Array
                        (
                            [AVATAX-Standard-] => 1.16
                        )
                )
            [meta_data:protected] => Array
                ( [Items] => test)
        )
)

The proposed solution is to add a filter and let the theme copy over the taxes from one object to the other, before the if condition, so that it result being true and the Subscription Order can be created.

Testing instructions

  1. On your mobile phone, go to https://novoslabs.com/product/novos-boost/
  2. Select a Monthly or any Subscription
  3. Pay with either Google Pay or Apple Pay.
  4. The user receives an error "Invalid recurring payment method"

Product impact

Dev notes

File: includes/class-wc-subscriptions-cart.php Function: validate_recurring_shipping_methods()

This if statement ends up being false if an Avatax rate is setup, unless we apply a filter to modify $package['rates']

// ↓ Proposed solution starts 
if ( isset( $standard_packages[ $package_index ] ) ) {
    $package['rates'] = apply_filters( 'wcs_cart_package_use_rates_from_standard_package', $package['rates'], $standard_packages[ $package_index ]['rates'] );
}
// ↑ Proposed solution ends

if ( ( isset( $standard_packages[ $package_index ] ) && $package['rates'] == $standard_packages[ $package_index ]['rates'] ) ) {
    // the recurring package rates match the initial package rates, there won't be a selected shipping method for this recurring cart package move on to the next package.
    if ( apply_filters( 'wcs_cart_totals_shipping_html_price_only', true, $package, $recurring_cart ) ) {
        continue;
    }
}

Additional context

james-allan commented 1 year ago

I’m not opposed to adding a filter as suggested but it does feel like a gap or weakness in Subscriptions logic that we’re band-aiding with a filter.


The logic in question: $package['rates'] == $standard_packages[ $package_index ]['rates'] ) is suppose to be a copy of similar logic in the wcs_cart_totals_shipping_html() function (code link). Both of these logic statements are designed to compare the subscription rates with the standard cart rates so it knows if we need to:

  1. display the UI in the cart and checkout to offer different shipping options to the subscription (wcs_cart_totals_shipping_html())
  2. validate the subscription chosen rate (the function raised by this issue). If the rates match then there won't be a subscription-specific shipping method chosen.

So the questions I have are:

In this circumstance, are shipping method options for the subscription being displayed to the user? These two logic statements around displaying and validating shipping methods should remain in sync so if we filter the rates here, we'd need to apply a similar change to the other function

Is this specific to Google or Apple Pay because they bypass normal checkout flows?

From the issue description it looks as though the AVATAX- bit of the taxes array is what is causing this. I don't know why the tax figure would be different. Do we know why the avatax bit is missing from the subscription rates? If the expectation is that they match in this circumstance, I wondering if there's a gap somewhere that is leading to them being different.

I was going to suggest that we compare the arrays on a more deeper level (ie just check that the rate IDs match), but I think that may lead to issues if the rate IDs were the same but the costs were different.

What's the core purpose behind the proposed hook? Is it to set/update the package rates or is just to get around this logic? I'd prefer to avoid avatax or another plugin having to just copy or set things with the goal of hoping that it will pass the logical checks. Ie would a filter like this resolve the issue?

$package_rates_match = apply_filters( 'wcs_recurring_shipping_package_rates_match_standard_rates' $package['rates'] == $standard_packages[ $package_index ]['rates'], $package['rates'], $standard_packages[ $package_index ]['rates'], $recurring_cart_key );

if ( isset( $standard_packages[ $package_index ] ) && $package_rates_match ) {
    // the recurring package rates match the initial package rates, there won't be a selected shipping method for this recurring cart package move on to the next package.
    if ( apply_filters( 'wcs_cart_totals_shipping_html_price_only', true, $package, $recurring_cart ) ) {
        continue;
    }
}
ecairol commented 1 year ago

Thanks for providing all this context @james-allan!

I'll do some research today to answer the first set of questions, however, I wanted to clarify that:

function wcs_cart_package_use_rates_from_standard_package( $package_rates, $standard_rates ) {
    foreach ( $standard_rates as $key => $s_package_rate ) {
        if ( isset( $package_rates[ $key ] ) ) {
            $standard_taxes = $s_package_rate->get_taxes();
            $package_rates[ $key ]->set_taxes( $standard_taxes );
        }
    }
    return $package_rates;
}
add_filter( 'wcs_cart_package_use_rates_from_standard_package', 'wcs_cart_package_use_rates_from_standard_package', 10, 2 );

However, I understand what you're saying about keeping this logic in sync with the that exists on wcs-cart-functions.php, and I'd like to avoid increasing the risk of something else failling.

Ie would a filter like this resolve the issue?

Yes, I think it would! If I understand correctly:

Please confirm that's the idea. That would work.

Though I think the check if ( isset( $standard_packages[ $package_index ] ) ) would need to go first, before the apply_filters()

ecairol commented 1 year ago

Opened a PR with suggested filter here: https://github.com/Automattic/woocommerce-subscriptions-core/pull/435