Keep live rates enabled even with WC Shipping active #2812

waclawjacek · 2024-09-11T09:37:06Z

Description

Context for development decisions: p1726023174650669-slack-C04KWSNPSE5

This PR keeps live rates enabled even when both WCS&T and WCShip are active. It does this by bringing back the registration of live rates shipping method registration, as well as bringing back the WCS&T settings page (hidden but available through the shipping method settings and in the future, also through the WCShipping settings page).

Additionally, it uses package data from WCShipping if a store has been migrated from WCS&T to WCShipping. Since the two extensions use different package data formats, this PR introduces remapping of package data formats on both reads and writes to wc_connect_options.

Breaking changes

The changes described below do not affect how WCShipping stores data, only how WCS&T remaps WCShipping's package data for its own purposes.

However, since WCS&T is writing data to WCShipping's options field when an update is requested from the WCS&T settings page that this PR reinstates for package management, it does affect the values that end up in wcshipping_options[packages].

To clean up the package data in WCShipping (not a required step; just for the sake of clarity), separate PRs will have to be implemented there.

1) Outer dimensions of custom packages

This is a feature that seems long deprecated but still supported in the codebase.

In WCS&T, predefined packages have outer dimensions (dimensions of the box used for calculating shipping rates) and inner dimensions (dimensions of the inside of the box used for box packing cart contents).

The WCS&T wc_connect_options option allows the key "outer_dimensions" to be defined on custom packages too. This used to be a feature of the extension but it was removed in Sep 2019.

Although it is no longer possible to set outer dimensions in WCS&T, the field was never unset if a custom package had it set (AFAIK). This means that some stores might have this key lying around for some old custom packages, although it is not presented anywhere anymore.

We could continue passing this key around however since the package edit modal in both WCS&T and WCShipping does not allow you to edit outer dimensions - it only supports inner dimensions (labelled simply as "Dimensions") - there is no way to edit them. This means that merchants effectively have no way to edit the dimensions used for calculating live rates for some very old packages, if they have any still around.

Because of the above, I decided to not support this field in this PR and to only carry over the inner_dimensions field which is the one that merchants can edit (and is the only one set for custom packages created after Sep 2019).

2) Custom package type value stored under `type`/`isLetter`/`is_letter`

WCS&T used to define packages as a boolean is_letter. If a package is a letter, it means it's an envelope. Otherwise, it's a box.

WCShipping introduces a new field called type which can have the value of box or envelope. To support old packages, the field is_letter is still kept around, as isLetter. In addition to that, a package might also have is_letter set.

Since it seems these are generally in sync for the packages defined by WCShipping, I have opted to only keep the type field after consulting with the team (p1726489562638679-slack-C04KWSNPSE5).

Testing instructions

Testing live rates pre-migration:

Install WCS&T on a non-migrated-to-WCShipping site (or reset the WCShip migration wizard by running the following code):

delete_option( 'wcshipping_migration_state' );
delete_option( 'wcst_data_migration_required' );
delete_option( 'wcshipping_installation_completed_shown' );
delete_option( 'wcst_data_migration_processes_to_run' );
delete_option( 'wcshipping_version' );

Add some packages in WC > Settings > WooCommerce Shipping.
Ensure your site is eligible for USPS live rates.
Add a shipping zone with a live rates shipping method.
Verify you can access and edit the shipping method's settings.
Install and activate WooCommerce Shipping.
Verify that live rates are working on the frontend.
- Please check both the blocks and the classic version of the cart and checkout pages.
Verify that label purchases are working.
- The label purchase modal should display the appropriate shipping costs based on the selected live rates shipping method.
Verify the status pages are working for both WCS&T and WCShip.
Verify plugin settings are working for both WCS&T and WCShip.

Testing live rates post-migration (after going through all of the above tests):

Migrate the settings from WCS&T to WCShip.
Go to admin.php?page=wc-settings&tab=shipping&section=woocommerce-services-settings and verify you can see the packages you previously had in WCS&T.
Add packages of different dimensions/weight/types on the new live rates settings page (WC > Settings > Shipping > WooCommerce Shipping > Live rates > Select packages). Verify it shows up correctly in the "purchase label" modal.
Add packages of different dimensions/weight/types in the "purchase label" modal. Verify it shows up correctly in the live rates settings.
Verify that live rates are working on the frontend.
- Please check both the blocks and the classic version of the cart and checkout pages.
Verify that label purchases are working.
- The label purchase modal should display the appropriate shipping costs based on the selected live rates shipping method.
Verify the status pages are working for both WCS&T and WCShip.
Verify plugin settings are working for both WCS&T and WCShip.

Testing WCShipping deactivation (after going through all of the above tests):

Verify that after deactivating WCShipping, WCS&T displays the packages added/modified/removed during your testing.

Checklist

unit tests

changelog.txt entry added
readme.txt entry added

waclawjacek · 2024-09-11T09:39:56Z

woocommerce-services.php

-		public function init_core_wizard_shipping_config() {
-			if ( $this->is_wc_shipping_activated() ) {
-				return;
-			}


This methods looks for options set by the WC setup wizard to preconfigure shipping methods. The options it looks for were removed in WC 3.6 and are no longer set by newer versions of WC. This plugin requires WC ≥ 8.8.

Instead of conditionally loading this, I removed it altogether.

waclawjacek · 2024-09-11T09:52:40Z

woocommerce-services.php

-
-				// Initialize user choices from the core setup wizard.
-				// Note: Avoid doing so on non-primary requests so we don't duplicate efforts.
-				if ( ! defined( 'DOING_AJAX' ) && is_admin() && ! isset( $_GET['noheader'] ) ) {
-					$this->init_core_wizard_shipping_config();
-				}


No longer needed. See https://github.com/Automattic/woocommerce-services/pull/2812/files#r1753818509

waclawjacek · 2024-09-11T11:08:59Z

There is another thing that needs to be figured out in this PR - support for available packages which are taken into account when calculating live rates. I'm currently working on that.

waclawjacek · 2024-09-11T11:21:17Z

One more thing we need to figure out is that when live rates are calculated, existing packages are taken into account.

When WCShipping is installed, it migrates packages from WCS&T and stores them under a new option name.

With WCShipping present, WCS&T's settings page is no longer registered, and so merchants can no longer edit the available packages on that page. Even if they could, WCS&T would be updating the old option which is still referenced by the live rates logic (provided by WCS&T) but not by the label purchase logic (provided by WCShipping).

In WCShip, the only way to edit packages is in the label purchase modal.

@Automattic/escargot How do you suggest we approach this? Some ideas that came to mind are:

Point merchants to the label purchase modal for editing labels and update WCS&T to use WCShip's package list when WCShip is installed.
- Drawback: Requires going to a random order edit screen to access.
Keep WCS&T's package configuration screen but make it reference the new settings.
- Drawback: Requires restoring the package manager to the settings page. Should be easy when done on a separate settings page, conditionally linked to from WCShip's settings.
- Drawback: The data structure of package settings might be different now.

kloon · 2024-09-11T11:23:54Z

How about we remove the link, and then just add a check to use the new saved packages instead of the old one for the calculations if it is present? Seem like the most minimal amount of changes?

waclawjacek · 2024-09-11T11:28:51Z

I'll try this approach. 👍

waclawjacek · 2024-09-11T11:51:31Z

Unfortunately, another issue is that in the label purchase modal in WCShipping, there is no notion of enabled/disabled package - all are available.

In WCS&T, you could enable selected carrier-provided and define custom packages. Your choices would then be reflected in which packages are presented in WCS&T's label purchase modal, as well as sent to the Connect Server to decide what packages are used for box packing during live rates calculation.

A way to define these packages seems necessary. How do you think we should implement it?

kallehauge · 2024-09-11T12:07:13Z

add a check to use the new saved packages instead of the old one for the calculations if it is present?

I created this "normalise" method for our custom packages that maps new field names and removes all new package keys since WCS will reject the requests if it finds unknown key/parameters:

https://github.com/woocommerce/woocommerce-shipping/pull/744/files#diff-5afc41abf1925e1fbd86294fa5dddcbaadb6d48cf1ce7e8e6442ccc75f16a00bR441-R471

waclawjacek · 2024-09-11T16:30:44Z

This PR is ready for review again.

Please verify especially the packages data structure to make sure the data format is correct (item no. 8 in the description).

waclawjacek · 2024-09-11T16:38:46Z

package.json

@@ -3,6 +3,7 @@
 	"version": "2.8.1",
 	"scripts": {
 		"start": "cross-env NODE_ENV=development CALYPSO_CLIENT=true webpack-dev-server --hot --inline --watch --content-base dist --port 8085 --host 0.0.0.0",
+		"watch": "cross-env NODE_ENV=development CALYPSO_CLIENT=true webpack --watch",


I needed this for my local dev. 😅 Can be removed.

No way! Keeper for sure! 😄

waclawjacek · 2024-09-19T12:48:35Z

This is now ready for review again.

harriswong

Thanks for the PR! I have 2 questions but otherwise tested ok for me!

Tested live rates with new packages 👍

Tested checking out the order and the shipping cost there 👍

Confirmed the packages are brought over after migration 👍

Questions:
Will updating "service packages" for live rates going to be a problem?

harriswong · 2024-09-19T21:03:58Z

classes/class-wc-connect-compatibility-wcshipping-packages.php

+	 * @return void
+	 */
+	public static function register_option_overwriting_hooks() {
+		// Intercept reads of "wc_connect_options[packages]" and "wc_connect_options[predefined_packages]".


These comments are helpful, thank you!

harriswong · 2024-09-19T21:43:59Z

classes/class-wc-connect-compatibility-wcshipping-packages.php

+
+		// Intercept updates to "wc_connect_options[packages]" and "wc_connect_options[predefined_packages]".
+		add_action( 'update_option_wc_connect_options', array( self::class, 'intercept_packages_update' ), 10, 2 );
+		add_action( 'update_option_wc_connect_options', array( self::class, 'intercept_predefined_packages_update' ), 10, 2 );


The 2 update_ functions here will always overwrite WooShipping's packages and predefined packages if the user edits live rate settings under WCS&T's code, regardless if there are any new changes from WCShipping. This may "delete" any new "saved templates" (the blue star) created from WCShipping's side.

If WCS&T can update wc_connect_options and can overwrite wcshipping_options but WooShipping can only update wcshipping_options but can't modify wc_connect_options, then losing saved templates (blue star) is the only problem. We can fix this by adding another level of array_merge to combine them. However, if WooShipping can also update wc_connect_options, then this will get really messy and we should avoid doing that.

Also, when I clicked "edit" in one of my custom box via http://localhost/wp-admin/admin.php?page=wc-settings&tab=shipping&section=woocommerce-services-settings, these 2 hooks don't get fired. Is that the same for you too?

The 2 update_ functions here will always overwrite WooShipping's packages and predefined packages if the user edits live rate settings under WCS&T's code, regardless if there are any new changes from WCShipping. This may "delete" any new "saved templates" (the blue star) created from WCShipping's side.

They won't! If you look at this method, it first gets WCShip's current options value and then it only replaces the packages key.

Whatever WCS&T is writing there is based on current WCShip packages, too, because the read is intercepted as well.

If WCS&T can update wc_connect_options and can overwrite wcshipping_options but WooShipping can only update wcshipping_options but can't modify wc_connect_options, then losing saved templates (blue star) is the only problem.

WCS&T modifies its entire wc_connect_options but the value of the packages key is replaced with whatever is in wcshipping_options[packages]. In pseudocode, it is doing:

wc_connect_options = { ...wc_connect_options, packages: wcshipping_options.packages, };

And after a merchant is done modifying the packages in the above object (note they came from the WCShip options), when WCS&T is writing to wc_connect_options, it also writes to just the packages field of WCShip:

const wcshipping_options = get_from_db( 'wcshipping_options' ) save( 'wc_connect_options', submitted_form_data ); save( 'wcshipping_options', { ...wcshipping_options, packages: submitted_form_data.packages, } );

Also, when I clicked "edit" in one of my custom box via http://localhost/wp-admin/admin.php?page=wc-settings&tab=shipping&section=woocommerce-services-settings, these 2 hooks don't get fired. Is that the same for you too?

Please make sure you also have the WC Shipping plugin enabled. Without it, this compatibility layer will not be registered at all.

Whatever WCS&T is writing there is based on current WCShip packages, too, because the read is intercepted as well.
... it also writes to just the packages field of WCShip:

Interesting... that's brilliant! Both read & write are intercepted and but the read-inside-the-write is also intercepted first, so you are reading the WCShipping, turns it to WCS&T settings, then write it, essentially syncing the new database config back to the old ones before writing. 👍

Ok I need a moment to visualize this...

https://xkcd.com/555/

waclawjacek · 2024-09-20T13:16:08Z

@harriswong Thank you for the review!

Will updating "service packages" for live rates going to be a problem?

Could you please elaborate on what you mean by updating service packages?

I've responded to the other comments in places where you have posted them.

harriswong · 2024-09-20T17:20:20Z

Could you please elaborate on what you mean by updating service packages?

Yes i can. I was thinking if someone goes to http://localhost/wp-admin/admin.php?page=wc-settings&tab=shipping&section=woocommerce-services-settings and update the "Service packages" there, not "custom package".

Then, this will save and update the WCS&T's configuration because this UI is loaded from WCS&T. That means the changes here won't get updated back in WCShipping's config.

But the explanation you have above #2812 (comment) covered this. Every save action in WCS&T will saves a copy to WCShipping as well. 👍

waclawjacek · 2024-09-20T17:26:00Z

That's right! An update to "Service packages" (a.k.a. carrier packages) changes the predefined_packages field the reads of/writes to which we are also intercepting.

harriswong · 2024-09-20T17:31:50Z

Please make sure you also have the WC Shipping plugin enabled. Without it, this compatibility layer will not be registered at all.

Double checked again and this time I ensured $is_migration_to_wcshipping_completed is true. Confirmed update is called. 👍

harriswong · 2024-09-20T17:36:45Z

tests/php/classes/test-class-wc-connect-compatibility-wcshipping-packages.php

+					'predefined_packages' => self::EXAMPLE_WCSHIPPING_OPTIONS['predefined_packages'],
+				)
+			),
+			get_option( 'wc_connect_options' )


Nice test, the setup starts with something in both wc_connect_options and wcshipping_options. This merges the two together when wc_connect_options is requested. 👍

harriswong

Tested again and looks good to me. 👍

I have 1 suggestion! What do you think about adding another test for this situation:
For this one, the test starts with
wc_connect_options = [101, 201]
wcshipping_options = [301, 401, 501]

Then we click "save" and add a new package, update_option(wc_connect_options, [601]); // adding a completely new config

Then, we verify the following:
wc_connect_options = [101, 201, 301, 401, 501, 601]
wcshipping_options = [101, 201, 301, 401, 501, 601]

This will help make sure a new item introduced will get saved in both configs.

Just a suggestion though, please feel free to merge!

harriswong · 2024-09-20T17:45:27Z

tests/php/classes/test-class-wc-connect-compatibility-wcshipping-packages.php

+					'predefined_packages' => self::EXAMPLE_WC_CONNECT_OPTIONS['predefined_packages'],
+				)
+			),
+			get_option( 'wcshipping_options' )


@harriswong

Props @harriswong

waclawjacek · 2024-09-20T19:22:00Z

Thanks for the review and testing! I've added the test case you suggested. Just a note that if the starting state is:

wc_connect_options[packages] == [101, 201]
wcshipping_options[packages] == [301, 401, 501]

The expected result will be 301, 401, 501, 601 and will not include 101, 201.

This is because we read from wcshipping_options with the changes in this PR active. We don't merge the options on-the-fly.

In a real-world example, if packages are first migrated from WCS&T to WCShip, WCShip's package list should be equal to WCS&T's so then the list would be [101, 201, 301, 401, 501]. 😄

Does that make sense?

By the way there is one quirk of this PR that I thought might not require fixing in this iteration - we can do it separately if needed. That quirk is that if you update the packages in WCShip's purchase modal, the WCS&T option will not get updated. It is not a concern as long as this PR applies because we read WCShip's options anyway but if you disable WCShip, the package list in it will be from the last time you clicked "Save" in the WCS&T package manager we've reintroduced, not when you last clicked save in the WCShip purchase modal.

Please LMK if this sounds concerning. I don't think it is. :)

One way to fix this would be to either prevent changes to wc_connect_options[packages] so that one stays "frozen" until WCShip is uninstalled, or to make sure it is always updated when wcshipping_options is updated. We could go the former way but that would introduce an extra DB write.

waclawjacek · 2024-09-20T21:23:06Z

@harriswong I managed to work around it. :) I've rewritten the filter of update_option( 'wc_connect_options' ) so it updates wcshipping_options with the new packages and predefined packages, and it does still update the wc_connect_options setting, but not with the new packages/predefined packages. This way when this PR goes out, the package setting will remain "frozen". If WCShipping is uninstalled, everything goes back to the pre-WCShipping installation state.

Could you please give it another look?

harriswong · 2024-09-23T17:28:58Z

tests/php/classes/test-class-wc-connect-compatibility-wcshipping-packages.php

+		remove_filter( 'option_wc_connect_options', array( WC_Connect_Compatibility_WCShipping_Packages::class, 'intercept_packages_read' ) );
+		remove_filter( 'option_wc_connect_options', array( WC_Connect_Compatibility_WCShipping_Packages::class, 'intercept_predefined_packages_read' ) );
+
+		// Assert the original `wc_connect_options` remain unchanged throughout the entire test.


harriswong

Thanks for the updates! I tested a migration and unserialized the before-and-after for wc_connect_options and wcshipping_options, I confirmed that updating WCShipping doesn't update WCS&T's options.

I added a comment regarding the PHP docs but the last 3 commits look good to me.

harriswong · 2024-09-23T17:29:14Z

classes/class-wc-connect-compatibility-wcshipping-packages.php

 	 */
-	public static function intercept_predefined_packages_update( $old_wc_connect_options, $wc_connect_options ) {
+	public static function intercept_predefined_packages_update( $wc_connect_options, $old_wc_connect_options ) {


We should probably update the @param comments

Done! Thanks.

harriswong · 2024-09-23T20:02:28Z

last commit LGTM

PS. Who would have thought that the update_ and pre_update_ hooks have the reverse parameter between the value and the old_value. Nice catch too!

Keep live rates enabled even with WC Shipping active

32c332c

waclawjacek added [Type] Bug [Pri] CRITICAL labels Sep 11, 2024

waclawjacek self-assigned this Sep 11, 2024

waclawjacek commented Sep 11, 2024

View reviewed changes

Add comments. Remove old logic.

6d71d5f

waclawjacek commented Sep 11, 2024

View reviewed changes

waclawjacek added 5 commits September 11, 2024 11:54

Remove unneeded classes

15752bb

Reorder methods for a cleaner diff

88de5ae

Merge branch 'trunk' into tweak/keep-live-rates-enabled-if-available

e9ea0ad

Restore continents controller to keep changes to a minimum

73fc282

Add changelog entry

a181ddd

waclawjacek marked this pull request as ready for review September 11, 2024 10:43

waclawjacek requested review from a team and kloon and removed request for a team September 11, 2024 10:43

waclawjacek marked this pull request as draft September 11, 2024 11:09

waclawjacek added 3 commits September 11, 2024 14:39

Make is_wc_shipping_activated() static

d7990ae

Restore access to live rates settings when WCShip is active

10b5e17

Display breadcrumbs

c50b969

waclawjacek marked this pull request as ready for review September 11, 2024 16:29

waclawjacek requested a review from kallehauge September 11, 2024 16:30

waclawjacek commented Sep 11, 2024

View reviewed changes

waclawjacek added 8 commits September 19, 2024 11:49

Add tests

026a6ea

Add test for mapping WCS&T save -> WCShipping

3fe8f0f

More tests

d18ed50

Revert classes/class-wc-connect-shipping-method.php

045a41b

Update phpdoc

897786e

phpdoc

2ceb236

Merge branch 'trunk' into tweak/keep-live-rates-enabled-if-available

724cee4

Add is_user_defined to allowed keys. Remove outer_dimensions

cf4e455

waclawjacek marked this pull request as ready for review September 19, 2024 12:48

harriswong reviewed Sep 19, 2024

View reviewed changes

Statically call ::maybe_enable()

125d372

harriswong reviewed Sep 20, 2024

View reviewed changes

harriswong approved these changes Sep 20, 2024

View reviewed changes

Add test

5f67b48

Props @harriswong

Ensure wc_connect_options remains unchanged

de08672

Remove unnecessary filter deregistration

7ed27a2

harriswong reviewed Sep 23, 2024

View reviewed changes

harriswong approved these changes Sep 23, 2024

View reviewed changes

Update PHPDoc. Rename variables for clarity

0248545

waclawjacek merged commit 5cd7545 into trunk Sep 23, 2024
9 checks passed

waclawjacek deleted the tweak/keep-live-rates-enabled-if-available branch September 23, 2024 20:08

waclawjacek mentioned this pull request Sep 25, 2024

Restore migration banner display on sites using live rates #2820

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep live rates enabled even with WC Shipping active #2812

Keep live rates enabled even with WC Shipping active #2812

waclawjacek commented Sep 11, 2024 •

edited

Loading

waclawjacek Sep 11, 2024

waclawjacek Sep 11, 2024

waclawjacek commented Sep 11, 2024

waclawjacek commented Sep 11, 2024 •

edited

Loading

kloon commented Sep 11, 2024

waclawjacek commented Sep 11, 2024

waclawjacek commented Sep 11, 2024

kallehauge commented Sep 11, 2024

waclawjacek commented Sep 11, 2024

waclawjacek Sep 11, 2024

kallehauge Sep 11, 2024

waclawjacek commented Sep 19, 2024

harriswong left a comment

harriswong Sep 19, 2024

harriswong Sep 19, 2024

waclawjacek Sep 20, 2024

harriswong Sep 20, 2024

waclawjacek commented Sep 20, 2024

harriswong commented Sep 20, 2024

waclawjacek commented Sep 20, 2024

harriswong commented Sep 20, 2024

harriswong Sep 20, 2024

harriswong left a comment

harriswong Sep 20, 2024

waclawjacek commented Sep 20, 2024 •

edited

Loading

waclawjacek commented Sep 20, 2024

harriswong Sep 23, 2024

harriswong left a comment

harriswong Sep 23, 2024

waclawjacek Sep 23, 2024

harriswong Sep 23, 2024

harriswong commented Sep 23, 2024

Keep live rates enabled even with WC Shipping active #2812

Keep live rates enabled even with WC Shipping active #2812

Conversation

waclawjacek commented Sep 11, 2024 • edited Loading

Description

Breaking changes

1) Outer dimensions of custom packages

2) Custom package type value stored under type/isLetter/is_letter

Testing instructions

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waclawjacek commented Sep 11, 2024

waclawjacek commented Sep 11, 2024 • edited Loading

kloon commented Sep 11, 2024

waclawjacek commented Sep 11, 2024

waclawjacek commented Sep 11, 2024

kallehauge commented Sep 11, 2024

waclawjacek commented Sep 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waclawjacek commented Sep 19, 2024

harriswong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waclawjacek commented Sep 20, 2024

harriswong commented Sep 20, 2024

waclawjacek commented Sep 20, 2024

harriswong commented Sep 20, 2024

Choose a reason for hiding this comment

harriswong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waclawjacek commented Sep 20, 2024 • edited Loading

waclawjacek commented Sep 20, 2024

Choose a reason for hiding this comment

harriswong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harriswong commented Sep 23, 2024

waclawjacek commented Sep 11, 2024 •

edited

Loading

2) Custom package type value stored under `type`/`isLetter`/`is_letter`

waclawjacek commented Sep 11, 2024 •

edited

Loading

waclawjacek commented Sep 20, 2024 •

edited

Loading