Improve remote specifications transient handling and error management #44384

chihsuan · 2024-02-06T07:54:55Z

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:

Closes #44261.

Improve remote specifications transient handling and error management

For PaymentGatewaySuggestions and RemoteFreeExtensions,

When "suggestions" is empty, replace transient with defaults and save for 3 hours
When "suggestions" is not empty but has errors, save transient for 3 hours

For WCPayPromotion:

When it has errors, save the transient for 3 hours
We don't need to replace the transient with defaults because the default is an empty array. DataSourcePoller will always re-fetch the data when it's an empty array.

Adds unit tests

How to test the changes in this Pull Request:

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

Testing

Test remote specs still work as expected

If using an existing site, clear transients by running wp transient delete --all
Ensure WooCommerce > Settings > Advanced > Woo.com > Display suggestions within WooCommerce is checked
Go to Setup Wizard, /wp-admin/admin.php?page=wc-admin&path=/setup-wizard
Click continue
In the Which one of these best describes you? step, select I'm already selling and I'm selling both online and offline
Select Australia — Australian Capital Territory and continue
Skip plugin installation
Observe no fatal when loading homescreen
Go to Tools > WCA Test Helper > Tools, Run wc_admin_daily job
Ensure the job is run successfully
Go to /wp-admin/admin.php?page=wc-admin&task=payments
Confirm it displays the gateways like below

Run wp transient get timeout_woocommerce_admin_payment_gateway_suggestions_specs
Confirm it returns a timestamp that is about 7 days from now
Run wp transient get timeout_woocommerce_admin_remote_free_extensions_specs
Confirm it returns a timestamp that is about 7 days from now
Run wp transient get timeout_woocommerce_admin_payment_method_promotion_specs
Confirm it returns a timestamp that is about 7 days from now

Has errors

Setup:

Cherry-pick the commit a2bf028 from this branch
Or manually set the following APIs
- PaymentGatewaySuggestionsDataSourcePoller (file): https://api.npoint.io/83d63d29d478ececdbc5
- RemoteFreeExtensionsDataSourcePoller (file): https://api.npoint.io/34e72d8b75957e47ff1e
- WCPayPromotionDataSourcePoller (file): https://api.npoint.io/6eed3f7093fa2ca8fd1f
- make the !contains' and 'in' to throw an exception

throw new Exception( 'Fail' );

Runwp transient delete --all
Ensure WooCommerce > Settings > Advanced > Woo.com > Display suggestions within WooCommerce is checked
Go to Tools > WCA Test Helper > Tools, Run wc_admin_daily job
Ensure the job is run successfully
Go to /wp-admin/admin.php?page=wc-admin&task=payments
Observe that it shows Airwallex Payments gateway

Go to /wp-admin/admin.php?page=wc-admin&task=marketing
Observe that it shows Google Listings & Ads, Pinterest for WooCommerce, and TikTok for WooCommerce in GROW YOUR STOR section.
Run wp transient get woocommerce_admin_payment_gateway_suggestions_specs --format=json
Observe that it returns default specs instead of specs from https://api.npoint.io/83d63d29d478ececdbc5

{"en_US":[{"id":"affirm","title":"Affirm","content":"Affirm\u2019s tailored Buy Now Pay Later programs remove price as a barrier, turning browsers into buyers, increasing average order value, and expanding your customer base."......

Run wp transient get timeout_woocommerce_admin_payment_gateway_suggestions_specs
Confirm it returns a timestamp that is less than 3 hours from now
Run wp transient get woocommerce_admin_remote_free_extensions_specs --format=json
Observe that it returns default specs instead of specs from https://api.npoint.io/34e72d8b75957e47ff1e

{"en_US":[{"key":"obw\/basics","title":"Get the basics","plugins":[{"name":"WooPayments","image_url":"http:\/\/localhost:8885\/mywordpress\/wp-content\/plugins\/woocommerce\/assets\/images\/onboarding\/wcpay.svg","description":"Accept credit cards and other popular payment methods with <a href=\"https:\/\/woo.com\/products\/woocommerce-payments\" target=\"_blank\">WooPayments<\/a>","is_visible":[{"type":"or","operands":[{"type":"base_location_country","value":"US","operation":"="},{"type":"base_location_country","value":"PR","operation":"="},{"type":"base_location_country","value":"AU","operation":"="},{"type":"base_location_country","value":"CA","operation":"="},....

Run wp transient get timeout_woocommerce_admin_remote_free_extensions_specs
Confirm it returns a timestamp that is less than 3 hours from now.
Run wp transient get woocommerce_admin_payment_method_promotion_specs --format=json
Observe that it returns same data as https://api.npoint.io/6eed3f7093fa2ca8fd1f
Run wp transient get timeout_woocommerce_admin_payment_method_promotion_specs
Confirm it returns a timestamp that is less than 3 hours from now.

Test

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 · 2024-02-06T08:03:23Z

Test Results Summary

Commit SHA: 0ae2595

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	259	0	0	2	0	261	0m 36s
E2E Tests	254	0	0	60	0	314	6m 22s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

chihsuan · 2024-02-06T08:35:09Z

plugins/woocommerce/src/Admin/DataSourcePoller.php

@@ -133,7 +133,8 @@ public function read_specs_from_data_sources() {
 			$this->merge_specs( $specs_from_data_source, $specs, $url );
 		}

-		$specs_group            = get_transient( $this->args['transient_name'] ) ?? array();
+		$specs_group            = get_transient( $this->args['transient_name'] );
+		$specs_group            = is_array( $specs_group ) ? $specs_group : array();


Fix PHP 8.1+ Automatic conversion of false to array is deprecated

github-actions · 2024-02-06T08:36:08Z

Hi @adrianduffell, @rjchow,

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

ilyasfoo

Apologies can I change my mind, it seems like the solution is a bit weird since we return $suggestions in the first attempt, but after that we'll return the defaults for 3 hours 😅

Perhaps instead, could we do this instead?

When $suggestions is empty, replace it with defaults and save for 3 hours
When $suggestions is not empty but has errors, save it for 3 hours

plugins/woocommerce/src/Admin/DataSourcePoller.php

moon0326 · 2024-02-06T19:13:18Z

plugins/woocommerce/src/Admin/Features/PaymentGatewaySuggestions/Init.php

 			}
 		}

+		if ( $has_error ) {


What do you think about moving this code into the catch block? I don't think we need $has_error if it's only used once.

I originally wanted to do that too, but because it's a loop, we might end up calling set_specs_transient many times. 🤔

Oh my bad 🤦 I did not see the loop.

Co-authored-by: Ilyas Foo <foo.ilyas@gmail.com>

chihsuan · 2024-02-07T01:36:40Z

Apologies can I change my mind, it seems like the solution is a bit weird since we return $suggestions in the first attempt, but after that we'll return the defaults for 3 hours 😅

Perhaps instead, could we do this instead?

When $suggestions is empty, replace it with defaults and save for 3 hours

When $suggestions is not empty but has errors, save it for 3 hours

Sure! I will update my PR.

chihsuan · 2024-02-07T10:47:53Z

@ilyasfoo I've made the changes. Could you please review this PR when you get a chance? Thanks. 🙏

I think we could create a parent Init class to provide some helpful methods and reduce duplicates, but I think that would be a follow-up.

plugins/woocommerce/src/Internal/Admin/RemoteFreeExtensions/Init.php

ilyasfoo

Awesome, tested well!

Minor comments, pre-approving 🚢

plugins/woocommerce/src/Admin/Features/PaymentGatewaySuggestions/EvaluateSuggestion.php

ilyasfoo · 2024-02-08T08:53:36Z

...tests/woocommerce-admin/features/payment-gateway-suggestions/payment-gateway-suggestions.php

+		$this->assertEquals( count( $stored_transients['en_US'] ), count( DefaultPaymentGateways::get_all() ) );
+
+		$expires = (int) get_transient( '_transient_timeout_woocommerce_admin_' . PaymentGatewaySuggestionsDataSourcePoller::ID . '_specs' );
+		$this->assertTrue( ( $expires - time() ) < 3 * HOUR_IN_SECONDS );


I think changing it to <= might be safer to accommodate for supercomputers running unit tests 😄

Changed in 0ae2595. Thanks! 😆

…ns/EvaluateSuggestion.php Co-authored-by: Ilyas Foo <foo.ilyas@gmail.com>

rjchow

One question and one minor nit - would it be possible to add a test case for the error flow?

rjchow · 2024-02-07T14:36:55Z

plugins/woocommerce/src/Admin/DataSourcePoller.php

@@ -183,7 +197,7 @@ protected static function read_data_source( $url ) {
 			// phpcs:ignore
 			$logger->error( print_r( $response, true ), $logger_context );

-			return [];
+			return array();


I couldn't quite figure out the intent of changing [] for array(), is it a lint issue?

Yes, it's a lint issue.

Short array syntax is not allowedPHP_CodeSniffer(Generic.Arrays.DisallowShortArraySyntax.Found)

chihsuan · 2024-02-13T02:43:45Z

One question and one minor nit - would it be possible to add a test case for the error flow?

@rjchow I wanted to add unit tests for that too, but since it's a static method, I can't mock it to return errors. 😞 I've tried different approaches, but none of them seem to work.

There are also many discussions on Slack about this, but still couldn't find any solutions.

A related issue:
sebastianbergmann/phpunit-documentation#77

Add set_specs_transient helper method

9dcb641

github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Feb 6, 2024

chihsuan changed the title ~~Store default specs into transient when spec rules fails~~ Store default specs into transient when spec rules fail Feb 6, 2024

chihsuan self-assigned this Feb 6, 2024

chihsuan added 2 commits February 6, 2024 16:00

Store default specs into transient when spec rules fails

13a60b3

Add changelog

d824646

chihsuan force-pushed the update/store-default-specs-when-fails branch from b50ab27 to d824646 Compare February 6, 2024 08:00

Fix automatic conversion of false to array is deprecated

03cc602

chihsuan commented Feb 6, 2024

View reviewed changes

chihsuan requested review from a team, adrianduffell and rjchow February 6, 2024 08:35

ilyasfoo requested changes Feb 6, 2024

View reviewed changes

plugins/woocommerce/src/Admin/DataSourcePoller.php Show resolved Hide resolved

plugins/woocommerce/src/Admin/DataSourcePoller.php Outdated Show resolved Hide resolved

moon0326 reviewed Feb 6, 2024

View reviewed changes

Update plugins/woocommerce/src/Admin/DataSourcePoller.php

96e27d3

Co-authored-by: Ilyas Foo <foo.ilyas@gmail.com>

chihsuan added 2 commits February 7, 2024 13:31

Use set_specs_transient

a36e36d

Handle evaluation error properly

bc61c9c

chihsuan force-pushed the update/store-default-specs-when-fails branch from 9b220f7 to 9199e6a Compare February 7, 2024 09:12

chihsuan changed the title ~~Store default specs into transient when spec rules fail~~ Improved remote specifications transient handling and error management Feb 7, 2024

chihsuan changed the title ~~Improved remote specifications transient handling and error management~~ Improve remote specifications transient handling and error management Feb 7, 2024

chihsuan requested a review from ilyasfoo February 7, 2024 09:53

chihsuan added 4 commits February 7, 2024 19:01

Add unit tests

1cb0457

Update init

142f8d2

Update changelog

bfdf2ae

Update changelog

b088329

chihsuan force-pushed the update/store-default-specs-when-fails branch from c5b8bee to b088329 Compare February 7, 2024 11:01

ilyasfoo requested changes Feb 8, 2024

View reviewed changes

plugins/woocommerce/src/Internal/Admin/RemoteFreeExtensions/Init.php Show resolved Hide resolved

ilyasfoo approved these changes Feb 8, 2024

View reviewed changes

chihsuan and others added 2 commits February 8, 2024 18:50

Update plugins/woocommerce/src/Admin/Features/PaymentGatewaySuggestio…

e794793

…ns/EvaluateSuggestion.php Co-authored-by: Ilyas Foo <foo.ilyas@gmail.com>

Change expires time compare operator to <= for safety

0ae2595

rjchow reviewed Feb 9, 2024

View reviewed changes

chihsuan requested a review from rjchow February 14, 2024 00:37

chihsuan merged commit a5711d0 into trunk Feb 15, 2024
39 checks passed

chihsuan deleted the update/store-default-specs-when-fails branch February 15, 2024 03:00

github-actions bot added this to the 8.7.0 milestone Feb 15, 2024

github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Feb 15, 2024

Stojdza added needs: internal testing Indicates if the PR requires further testing conducted by Solaris status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Feb 16, 2024

barryhughes mentioned this pull request Feb 27, 2024

Use ternary instead of null coalescing operator to prevent PHP 8.1 deprecation notice #39852

Closed

chihsuan mentioned this pull request Mar 1, 2024

Automatic conversion of false to array is deprecated #38860

Closed

5 tasks

adrianduffell mentioned this pull request Apr 5, 2024

Debug notice when updating to 8.5.2: Automatic conversion of false to array is deprecated #44103

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve remote specifications transient handling and error management #44384

Improve remote specifications transient handling and error management #44384

chihsuan commented Feb 6, 2024 •

edited

github-actions bot commented Feb 6, 2024 •

edited

chihsuan Feb 6, 2024

github-actions bot commented Feb 6, 2024 •

edited

ilyasfoo left a comment

moon0326 Feb 6, 2024 •

edited

chihsuan Feb 7, 2024

moon0326 Feb 7, 2024

chihsuan commented Feb 7, 2024

chihsuan commented Feb 7, 2024

ilyasfoo left a comment

ilyasfoo Feb 8, 2024

chihsuan Feb 8, 2024

rjchow left a comment

rjchow Feb 7, 2024

chihsuan Feb 13, 2024

chihsuan commented Feb 13, 2024 •

edited

Improve remote specifications transient handling and error management #44384

Improve remote specifications transient handling and error management #44384

Conversation

chihsuan commented Feb 6, 2024 • edited

Submission Review Guidelines:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Testing

Test

Changelog entry

Significance

Type

Message

Comment

github-actions bot commented Feb 6, 2024 • edited

Test Results Summary

chihsuan Feb 6, 2024

Choose a reason for hiding this comment

github-actions bot commented Feb 6, 2024 • edited

ilyasfoo left a comment

Choose a reason for hiding this comment

moon0326 Feb 6, 2024 • edited

Choose a reason for hiding this comment

chihsuan Feb 7, 2024

Choose a reason for hiding this comment

moon0326 Feb 7, 2024

Choose a reason for hiding this comment

chihsuan commented Feb 7, 2024

chihsuan commented Feb 7, 2024

ilyasfoo left a comment

Choose a reason for hiding this comment

ilyasfoo Feb 8, 2024

Choose a reason for hiding this comment

chihsuan Feb 8, 2024

Choose a reason for hiding this comment

rjchow left a comment

Choose a reason for hiding this comment

rjchow Feb 7, 2024

Choose a reason for hiding this comment

chihsuan Feb 13, 2024

Choose a reason for hiding this comment

chihsuan commented Feb 13, 2024 • edited

chihsuan commented Feb 6, 2024 •

edited

github-actions bot commented Feb 6, 2024 •

edited

github-actions bot commented Feb 6, 2024 •

edited

moon0326 Feb 6, 2024 •

edited

chihsuan commented Feb 13, 2024 •

edited