Automattic / woocommerce-subscriptions-core

Subscriptions core package for WooCommerce
Other
80 stars 29 forks source link

Override the place order button text on the block checkout with subscription-context overrides #587

Closed james-allan closed 3 months ago

james-allan commented 3 months ago

Fixes #https://github.com/woocommerce/woocommerce-subscriptions/issues/4628

Description

When you renew, switch or purchase a new subscription the place order button text on the checkout it changed to give the customer more context.

Sign up * Switch Renew
Screenshot 2024-03-26 at 5 01 10 pm Screenshot 2024-03-26 at 4 57 06 pm Screenshot 2024-03-26 at 4 58 10 pm

Screenshot 2024-03-26 at 5 05 16 pm
* The default "Sign up now" is changeable by Subscription admin settings.

These overrides don't currently apply to the block checkout. This PR fixes that.

[!important] To test the switch case you will need to use the issue/4628 branch of the Woo Subscriptions repo. Ref: https://github.com/woocommerce/woocommerce-subscriptions/pull/4637

How to test this PR

  1. Run npm run build as this PR has changes to built JS files.
  2. Go to WooCommerce → Settings → Subscriptions
  3. Scroll down to the Button Text section and change the default place order button text.
  4. Add a standard (non-subscription) product to your cart.
  5. Go to the block checkout and note that the place order button is still the default "Place Order".
  6. Add a subscription product to you cart.
  7. View the block checkout.
    • On trunk it will still say "Place Order".
    • On this branch the place order button text will be replaced with the text you entered in step 2.
  8. Purchase a variable subscription product and from the My Account Subscription page click the button to switch (upgrade/downgrade).
  9. View the block checkout.
    • On trunk it will still say "Place Order".
    • On this branch the place order button text will be replaced with "Switch subscription"
  10. Create a failed/pending renewal order and attempt to pay for it by adding it to the cart.
  11. View the block checkout.
    • On trunk it will still say "Place Order".
    • On this branch the place order button text will be replaced with "Renew subscription"
james-allan commented 3 months ago

@mattallan I'd be curious to know what your thoughts are on this approach. All the cart related functions which hook onto the woocommerce_order_button_text filter are unaccessible because all our instances of those classes (which aren't static) aren't stored and retrievable anywhere.

This PR adds a method to make them accessible so we can call functions on those instances now. I don't really have any concerns about that. We could add static get_instance() versions instead. Those classes not being accessible is a bit of pain anyway.

An alternative would be to just trigger the woocommerce_order_button_text filter and inject into the JS whatever is returned, however, that would mean anyone else hooking onto it would also be affected by that and in that case I think I'd prefer WooCommerce Core just honoured the woocommerce_order_button_text themselves.

james-allan commented 3 months ago

We could add static get_instance() versions instead. Those classes not being accessible is a bit of pain anyway

After discussions with @mattallan we're going to go with this approach as it's cleaner and easier to understand.

james-allan commented 3 months ago

After discussions with @mattallan we're going to go with this approach as it's cleaner and easier to understand

So I ended up running into issues with that approach. Namely adding a get_instance() function to the WCS_Cart_Renewal class like this

class WCS_Cart_Renewal {

    /**
     * The single instance of the class.
     *
     * @var WCS_Cart_Renewal
     */
    protected static $instance = null;

    /**
     * Main WCS_Cart_Renewal Instance.
     *
     * Ensures only one instance of WCS_Cart_Renewal is loaded or can be loaded.
     *
     * @return WCS_Cart_Renewal - Main instance.
     */
    public static function get_instance() {
        if ( is_null( static::$instance ) ) {
            static::$instance = new static();
        }

        return static::$instance;
    }

Would mean that all child classes inherit that static variable and I'd need to define an $instance variable in each child class and that's not great.

I'm going to improve the current approach to incorporate some of Matt's other feedback. Namely the eye sore that is $this->cart_handlers['renewal'].

james-allan commented 3 months ago

@mattallan I've updated this and the accompanying PR over on Woo Subscriptions inline with the changes we spoke about.