Add base RemoteSpecsEngine class, add logging for errors in all remote specs #44775

ilyasfoo · 2024-02-19T16:07:16Z

Changes proposed in this Pull Request:

Closes #44248.

This PR:

Adds base RemoteSpecsEngine that's extended by all Remote Specs
Adds logging for development environments for all errors from evaluating specs
Change evaluate to private to ensure it's not called from outside since it's possible to fatal

How to test the changes in this Pull Request:

Test remote specs still work as expected

Ensure your environment's PHP version is > 8.1
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
Select I'm already selling and No, I'm selling 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 (non-JN, JN)

Test logging

In wp-config.php, ensure WP_ENVIRONMENT_TYPE is set to local or development
In your local, modify the file RuleEvaluator.php and add in_array( '', '', true ); to simulate errors in PHP 8+.
Ensure logging is enabled in WooCommerce > Status > Logs > Settings (screenshot)
Go to WooCommerce > Homescreen
Go to Tools > WCA Test Helper > Tools > Run wc_admin_daily job
Go to WooCommerce > Status > Logs and look for logs named remotespecsengine-errors
In the logs, ensure there are errors that came from the following classes:

Automattic\\WooCommerce\\Internal\\Admin\\WCPayPromotion\\Init
Automattic\\WooCommerce\\Internal\\Admin\\RemoteFreeExtensions\\Init
Automattic\\WooCommerce\\Admin\\Features\\PaymentGatewaySuggestions\\Init
Automattic\\WooCommerce\\Internal\\Admin\\WCPayPromotion\\Init
Automattic\\WooCommerce\\Admin\\RemoteInboxNotifications\\RemoteInboxNotificationsEngine

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-19T16:13:00Z

Hi @chihsuan, @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

github-actions · 2024-02-19T17:50:10Z

Test Results Summary

Commit SHA: 31d6e42

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	259	0	0	2	0	261	0m 37s
E2E Tests	319	0	0	21	0	340	12m 46s

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.

ilyasfoo · 2024-02-20T01:12:20Z

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

@@ -20,16 +20,13 @@ class EvaluateExtension {
 	 * @param object $extension The extension to evaluate.
 	 * @return object The evaluated extension.
 	 */
-	public static function evaluate( $extension ) {
+	private static function evaluate( $extension ) {


Set to private since this can now produce fatal errors.

ilyasfoo · 2024-02-20T01:12:31Z

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

 use Automattic\WooCommerce\Admin\Features\PaymentGatewaySuggestions\EvaluateSuggestion;
-use Automattic\WooCommerce\Admin\Features\PaymentGatewaySuggestions\PaymentGatewaySuggestionsDataSourcePoller as PaymentGatewaySuggestionsDataSourcePoller;


Unused imports

chihsuan

Tested well! 👍 Just left a question about the transient.

Select I'm just starting my business and continue

Sorry, I found that my previous PR test instructions were not accurate. I think to display the Square as shown in the screenshot, we need to select "I'm already selling" and "I'm selling both online and offline." (or I'm selling offline.)

Additionally, if testing via the live branch, the plugin folder name would need to be changed from /srv/htdocs/wp-content/plugins/wc_beta_tester_live_branch_* to /srv/htdocs/wp-content/plugins/woocommerce for WCPay to appear in the set up payment task.

chihsuan · 2024-02-20T03:12:21Z

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

 			PaymentGatewaySuggestionsDataSourcePoller::get_instance()->set_specs_transient( array( $locale => DefaultPaymentGateways::get_all() ), 3 * HOUR_IN_SECONDS );
-
-			return EvaluateSuggestion::evaluate_specs( DefaultPaymentGateways::get_all() )['suggestions'];
+			$specs_to_return = EvaluateSuggestion::evaluate_specs( DefaultPaymentGateways::get_all() )['suggestions'];


Since we don't return the specs here, when suggestions are empty and errors exist, we will set the transient to $specs in line 53. Is this an intentional change?

Thanks for catching it! It's not intentional, the logic went over my head 😵

@chihsuan I've updated the specs logic with c39de1a

…g-remote-specs

ilyasfoo · 2024-02-20T06:45:00Z

Thanks for the review, @chihsuan!

Sorry, I found that my previous PR test instructions were not accurate.
Additionally, if testing via the live branch, the plugin folder name would need to be changed

I've updated the instructions to reflect actual use cases!

chihsuan

Thanks! LGTM 💯

rjchow

Thanks for refactoring!

Optional comment: since it's now extending a base class, I wonder if it would make sense to take it one step further and extract the common logic in get_suggestions?

ilyasfoo · 2024-02-21T01:31:51Z

Thanks, @rjchow!

since it's now extending a base class, I wonder if it would make sense to take it one step further and extract the common logic in get_suggestions?

I've been thinking that too! However, the get_suggestions is not identical for all remote spec engines (only 3 works similarly). I still think there can be a common denominator, but will require a small refactor, and out of the scope of this PR.

ilyasfoo added 2 commits February 20, 2024 00:06

Add basic RemoteSpecEngine class, add logging for errors

6243127

Changelog

4ddb97a

ilyasfoo requested review from a team, chihsuan and rjchow February 19, 2024 16:11

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

ilyasfoo commented Feb 20, 2024

View reviewed changes

chihsuan reviewed Feb 20, 2024

View reviewed changes

ilyasfoo added 3 commits February 20, 2024 13:22

Fix logic error

c39de1a

Add log name

4b982fa

Merge remote-tracking branch 'origin/trunk' into fix/44248-add-loggin…

31d6e42

…g-remote-specs

ilyasfoo requested a review from chihsuan February 20, 2024 06:45

chihsuan approved these changes Feb 20, 2024

View reviewed changes

rjchow approved these changes Feb 20, 2024

View reviewed changes

ilyasfoo merged commit 42aa027 into trunk Feb 21, 2024
40 checks passed

ilyasfoo deleted the fix/44248-add-logging-remote-specs branch February 21, 2024 01:33

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

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

ilyasfoo mentioned this pull request Feb 21, 2024

Wrangle MarketingRecommendations as RemoteSpecsEngine #44828

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add base RemoteSpecsEngine class, add logging for errors in all remote specs #44775

Add base RemoteSpecsEngine class, add logging for errors in all remote specs #44775

ilyasfoo commented Feb 19, 2024 •

edited

github-actions bot commented Feb 19, 2024 •

edited

github-actions bot commented Feb 19, 2024 •

edited

ilyasfoo Feb 20, 2024

ilyasfoo Feb 20, 2024

chihsuan left a comment

chihsuan Feb 20, 2024

ilyasfoo Feb 20, 2024 •

edited

ilyasfoo Feb 20, 2024

ilyasfoo commented Feb 20, 2024

chihsuan left a comment

rjchow left a comment

ilyasfoo commented Feb 21, 2024

		use Automattic\WooCommerce\Admin\Features\PaymentGatewaySuggestions\EvaluateSuggestion;
		use Automattic\WooCommerce\Admin\Features\PaymentGatewaySuggestions\PaymentGatewaySuggestionsDataSourcePoller as PaymentGatewaySuggestionsDataSourcePoller;

Add base RemoteSpecsEngine class, add logging for errors in all remote specs #44775

Add base RemoteSpecsEngine class, add logging for errors in all remote specs #44775

Conversation

ilyasfoo commented Feb 19, 2024 • edited

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Changelog entry

Significance

Type

Message

Comment

github-actions bot commented Feb 19, 2024 • edited

github-actions bot commented Feb 19, 2024 • edited

Test Results Summary

ilyasfoo Feb 20, 2024

Choose a reason for hiding this comment

ilyasfoo Feb 20, 2024

Choose a reason for hiding this comment

chihsuan left a comment

Choose a reason for hiding this comment

chihsuan Feb 20, 2024

Choose a reason for hiding this comment

ilyasfoo Feb 20, 2024 • edited

Choose a reason for hiding this comment

ilyasfoo Feb 20, 2024

Choose a reason for hiding this comment

ilyasfoo commented Feb 20, 2024

chihsuan left a comment

Choose a reason for hiding this comment

rjchow left a comment

Choose a reason for hiding this comment

ilyasfoo commented Feb 21, 2024

ilyasfoo commented Feb 19, 2024 •

edited

github-actions bot commented Feb 19, 2024 •

edited

github-actions bot commented Feb 19, 2024 •

edited

ilyasfoo Feb 20, 2024 •

edited