Various improvements to HPOS settings

[jorgeatorres]

Submission Review Guidelines:

• I have followed the WooCommerce Contributing Guidelines and the WordPress Coding Standards.
• I have checked to ensure there aren't other open Pull Requests for the same update/change.
• I have reviewed my code for security best practices.
• Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate.

Changes proposed in this Pull Request:

This PR makes a few changes to the HPOS settings screen (and some related functions). This is based on feedback we've received, but also things we've been notices (such as duplicate queries, unused functions, etc.).

Some of the changes contained in this PR:

• In 902e1ec, we remove a hard-coded style for settings and make it a CSS class instead, which is more flexible and also plays better with colors defined elsewhere. This might seem unrelated to HPOS, but was added specifically in the context of HPOS settings, so seems reasonable to update that here too.

• Removal of an internal sync option (woocommerce_initial_orders_pending_sync_count) that is not used at all in the sync code. This allows us to deprecate DataSynchronizer::get_sync_status(), which at this point, is nothing but an unnecessary wrapper over methods counting the number of orders pending sync.

• When generating settings/labels for HPOS, we now use the "cached" version of the orders pending sync count function, which removes query duplication on this screen.

• Some UI changes such as removal of an unnecessary tooltip, and proper formatting for numbers. Also, screens were tweaked as described below:

  Before	Now	Why?
  		The "pending sync" wording has been reported (several times) as confusing users, as even with compatibility mode disabled, it seems to imply you have to sync orders. I've changed this for "out of sync" and moved the bit about changing datastores first, to make it clear that it's only when changing datastores that this is relevant.
  		It's now possible to stop the manual sync, which means merchants no longer have to wait until it completes if they clicked it by mistake.

Closes #43268.

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

1. Make sure your site has a few orders or use Smooth Generator (if needed).
2. Go to WC > Settings > Advanced > Features and make sure HPOS is selected as order data storage and that compatibility mode is disabled.
   If necessary, use wp wc hpos sync to first sync orders so that all options become available.
3. Check that there's no synchronization query duplication.
   1. Install and activate Query Monitor to be able to detect duplicate queries.
   2. Go to WC > Settings > Advanced > Features and make sure that Query Monitor doesn't report any duplication of queries related to Automattic\WooCommerce\Internal\DataStores\Orders\DataSynchronizer.
   3. (Optional) Contrast this with trunk where duplication is reported:
      
4. Run wp wc hpos cleanup all so that order storage is no longer in sync.
5. Go to WC > Settings > Advanced > Features and confirm that you see a message about orders being out of sync.
6. Check that sync can be manually started and stopped.
   i. On WC > Settings > Advanced > Features click "Sync orders now" to start a manual order sync.
   ii. Confirm that after the page refreshes, you see a message about orders being synced and there's a link to "Stop sync".
   iii. Refresh the page until you see number of unsynced orders go down (might take a bit depending on when A-S runs).
   iv. Quickly click "Stop sync".
   v. Refresh the page and confirm that orders have stopped syncing and that "Sync orders now" appears once again.
7. Run wp wc hpos cleanup all again to make sure the datastores are not in sync.
8. Enable compatibility mode on the WC > Settings > Advanced > Features screen, and confirm that orders are syncing and that you can't choose a different data storage until the process is complete.

Test description_is_error

This PR also introduces a change in how settings with description_is_error are treated. Instead of hardcoding the styles in the HTML tags, we now use a CSS class that uses the color defined via SCSS.

1. Build WC so that legacy assets are properly built and linked.
2. Make sure datastores are not in sync. Run wp wc hpos cleanup all if necessary.
3. Go to WC > Settings > Advanced > Features and you should see the following message inside the HPOS section:
   
4. Activate the following snippet on your site:

   add_filter( 'wc_allow_changing_orders_storage_while_sync_is_pending', '__return_true' );

5. Go back to WC > Settings > Advanced > Features and the message from step 3, should've changed to the following and be displayed in red:
   

Changelog entry

• Automatically create a changelog entry from the details below.

Significance

• Patch
• Minor
• Major

Type

• Fix - Fixes an existing bug
• Add - Adds functionality
• Update - Update existing functionality
• Dev - Development related task
• Tweak - A minor adjustment to the codebase
• Performance - Address performance issues
• Enhancement - Improvement to existing functionality

Message

Comment

[github-actions]

Test using WordPress Playground

The changes in this pull request can be previewed and tested using a WordPress Playground instance.
WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Test this pull request with WordPress Playground.

Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit.

[github-actions]

Hi @naman03malhotra,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

[naman03malhotra]

Thanks for all these awesome imporvements to HPOS! 🎉

I did one pass of review and overall it looks great, but I am yet to test a few minor things which I will take up next week.

Overall, I am super glad to see this PR, thank you for working on it 🙇.

[naman03malhotra]

Great work cleaning this up. Can you please clarify the instructions on how to test this change? It would also be helpful when Solaris or our external partners tests this PR.

[jorgeatorres]

I've updated the testing instructions with a section on this specific change.

[naman03malhotra]

Maybewe can extract the common message out, wdyt? For a moment, I was confused as to why we had two similar messages, and then I realized.

Seems duplicate, can be extracted out.

<strong>Switching data storage while sync is incomplete is dangerous and can lead to order data corruption or loss!</strong>

[jorgeatorres]

I guess there might be some i18n issues with separating the strings, but I did it anyways in d55c61c. We can reassess later if this ever becomes a problem for translations, which it most likely won't.

[naman03malhotra]

Out of context of this PR probably, the sync status doesn't get updates unless the page is fully reloaded. Probably not even needed, but good to have enhancement if we can update the status as well either by polling or websocket.

[jorgeatorres]

Hey @naman03malhotra! Thanks for the review. I've responded / addressed your comments. Please let me know what you think!

Out of context of this PR probably, the sync status doesn't get updates unless the page is fully reloaded. Probably not even needed, but good to have enhancement if we can update the status as well either by polling or websocket.

I agree that this isn't the ideal UI, but this would require some changes that are probably not worth the effort right now for a screen that is seldom used. Polling (or similar) to get an updated count is not necessarily the most problematic aspect (though it does involve some heavy queries), but we would also need to properly update all the UI depending on the result (which is much more involved as you see there are a ton of different messages that can appear and interactions between the different settings). There's also the issue of nonce invalidation, etc.

[naman03malhotra]

@jorgeatorres just to confirm this is fine correct? the duplicates of individual query shouldn't happen.

[jorgeatorres]

@naman03malhotra: Yep. That's correct.

[naman03malhotra]

Thanks for making all the imporant changes as part of this PR. I've tested and it works well. I've left some comments none of which are blocking for this PR to be merged.