Add woocommerce_suggest_jetpack filter to exlude Jetpack suggestion in OBW #38286

moon0326 · 2023-05-13T01:21:37Z

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 adds woocommerce_suggest_jetpack filter to the free extensions REST endpoint to exclude Jetpack suggestion in OBW.

How to test the changes in this Pull Request:

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

Download this plugin and activate it
Start with a fresh site or make sure to remove Jetpack completely
Start OBW
Confirm Jetpack is not listed in Business Details -> Free features

github-actions · 2023-05-13T01:22:06Z

Hi @adrianduffell, @rjchow, @woocommerce/ghidorah

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 · 2023-05-13T01:49:21Z

Test Results Summary

Commit SHA: af67291

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	267	0	0	2	0	269	1m 13s
E2E Tests	189	0	0	10	0	199	14m 18s

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.

codecov · 2023-05-13T01:54:16Z

Codecov Report

Merging #38286 (af67291) into trunk (6ed8fb4) will increase coverage by 0.3%.
The diff coverage is 58.8%.

Additional details and impacted files

@@            Coverage Diff             @@
##             trunk   #38286     +/-   ##
==========================================
+ Coverage     51.2%    51.6%   +0.3%     
- Complexity   17439    17444      +5     
==========================================
  Files          440      440             
  Lines        80722    80297    -425     
==========================================
+ Hits         41346    41403     +57     
+ Misses       39376    38894    -482

Impacted Files	Coverage Δ
...ommerce/includes/abstracts/abstract-wc-product.php	`88.0% <0.0%> (-0.3%)`	⬇️
...ins/woocommerce/includes/class-wc-form-handler.php	`5.6% <0.0%> (-<0.1%)`	⬇️
...ugins/woocommerce/includes/class-wc-post-types.php	`1.8% <ø> (+0.8%)`	⬆️
...woocommerce/includes/class-wc-product-variable.php	`75.7% <0.0%> (-0.7%)`	⬇️
...ins/woocommerce/includes/wc-template-functions.php	`12.2% <0.0%> (-<0.1%)`	⬇️
...lugins/woocommerce/includes/class-wc-countries.php	`95.4% <100.0%> (ø)`
...rs/Version2/class-wc-rest-orders-v2-controller.php	`94.4% <100.0%> (ø)`
...llers/Version3/class-wc-rest-orders-controller.php	`80.5% <100.0%> (ø)`
plugins/woocommerce/includes/wc-core-functions.php	`63.6% <100.0%> (ø)`

... and 5 files with indirect coverage changes

plugins/woocommerce/src/Admin/API/OnboardingFreeExtensions.php

chihsuan · 2023-05-18T03:38:53Z

plugins/woocommerce/src/Admin/API/OnboardingFreeExtensions.php

+		 *
+		 * @since 7.8
+		*/
+		if ( false === apply_filters( 'woocommerce_suggest_jetpack', true ) ) {


I'm wondering if "woocommerce_onboarding_suggest_jetpack" would be more explicit. What do you think?

That was my first thought as well. However, I think we plan to use woocommerce_suggest_jetpack on other pages where we also suggest Jetpack.

Yup we should roll this out everywhere Jetpack is suggested 👍

I'll audit wc-admin for this and create a new issue.

chihsuan

Thanks for adding the filter!

Left a two minor suggestions. Others look good! 👍

Co-authored-by: Chi-Hsuan Huang <[email protected]>

adrianduffell

LGTM @moon0326 🚀

I left a nitpick possible improvement, but feel free to merge without addressing it.

adrianduffell · 2023-05-21T22:59:33Z

plugins/woocommerce/src/Admin/API/OnboardingFreeExtensions.php

+		*/
+		if ( false === apply_filters( 'woocommerce_suggest_jetpack', true ) ) {
+			foreach ( $extensions as &$extension ) {
+				$extension['plugins'] = array_filter(


Nitpick: Should the array_filter be used on $extensions so that it outright removes the extension completely?

I think at the moment, only the jetpack plugin slug would be removed from the extension data, but the extension still remains. I think this still works, but is dependent on the consuming code having a defensive check for an empty plugin array.

@adrianduffell Good point. Let's merge it for now and get back to it later. I'm not sure if the client code is checking for the extension. We need to test it further if we want to remove the list completely.

Sounds good!

jonathansadowski · 2023-05-23T20:50:14Z

plugins/woocommerce/src/Admin/API/OnboardingFreeExtensions.php

-		return RemoteFreeExtensions::get_extensions();
+		$extensions = RemoteFreeExtensions::get_extensions();
+		/**
+		* Remove Jetpack when woocommerce_suggest_jetpack is false.


Just a note: we use these comments to auto-generate docs, as well as lists of new filters introduced in a release. It'd be helpful in the future if the description more completely described the filter, such as "When false, Jetpack is removed from the list of suggested extensions."

No changes immediately required here, but please keep this in mind when adding these comments to future filters and when reviewing PRs that include new filters.

/cc @chihsuan and @adrianduffell as reviewers as well

@moon0326 I fixed the description used in the release post, but after some discussion, we'd still like this description improved in the code. Please create a PR to clean this up when you get a change. No need to request an exception to get it into the 7.8 cycle, but please do work toward getting it cleaned up in trunk.

Thank you! @jonathansadowski

cc @woocommerce/ghidorah for the rest of the team

Great callout, @jonathansadowski!

jonathansadowski · 2023-05-23T20:56:03Z

plugins/woocommerce/changelog/add-38285-add-filter-to-free-extensions-rest-endpoint

+Significance: minor
+Type: add
+
+Add a filter to free extensions REST endpoint


Same comment here as below for the filter comment. This isn't very useful to somebody glancing over the changelog what the change entry means. Please consider the audience for the change entry. What does the filter do?

moon added 2 commits May 12, 2023 18:18

Add filter for free extensions endpoint

e4d6bf8

Add changelog

8d8069c

moon0326 requested review from a team, adrianduffell and rjchow May 13, 2023 01:21

github-actions bot added focus: react admin [team:Ghidorah] plugin: woocommerce Issues related to the WooCommerce Core plugin. labels May 13, 2023

Add filter doc

c0ac103

moon0326 changed the title ~~Add a filter to free extensions REST endpoint~~ Add woocommerce_suggest_jetpack filter to exlude Jetpack suggestion in OBW May 15, 2023

Support woocommerce_suggest_jetpack filter to exlude Jetpack

0cd7c09

moon0326 force-pushed the add/38285-add-filter-to-free-extensions-rest-endpoint branch from d1fcefe to 0cd7c09 Compare May 15, 2023 19:50

chihsuan reviewed May 18, 2023

View reviewed changes

plugins/woocommerce/src/Admin/API/OnboardingFreeExtensions.php Outdated Show resolved Hide resolved

chihsuan reviewed May 18, 2023

View reviewed changes

chihsuan previously approved these changes May 18, 2023

View reviewed changes

Update plugins/woocommerce/src/Admin/API/OnboardingFreeExtensions.php

af67291

Co-authored-by: Chi-Hsuan Huang <[email protected]>

moon0326 dismissed chihsuan’s stale review via af67291 May 18, 2023 18:33

adrianduffell approved these changes May 21, 2023

View reviewed changes

moon0326 merged commit 6d40140 into trunk May 21, 2023

moon0326 deleted the add/38285-add-filter-to-free-extensions-rest-endpoint branch May 21, 2023 23:07

github-actions bot added this to the 7.8.0 milestone May 21, 2023

jonathansadowski reviewed May 23, 2023

View reviewed changes

chihsuan mentioned this pull request May 29, 2023

Update woocommerce_suggest_jetpack filter description #38501

Merged

Add woocommerce_suggest_jetpack filter to exlude Jetpack suggestion in OBW #38286

Add woocommerce_suggest_jetpack filter to exlude Jetpack suggestion in OBW #38286

Uh oh!

Conversation

moon0326 commented May 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Submission Review Guidelines:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Uh oh!

github-actions bot commented May 13, 2023

Uh oh!

github-actions bot commented May 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results Summary

Uh oh!

codecov bot commented May 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chihsuan left a comment

Choose a reason for hiding this comment

Uh oh!

adrianduffell left a comment

Choose a reason for hiding this comment

Uh oh!

adrianduffell May 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

moon0326 commented May 13, 2023 •

edited

Loading

github-actions bot commented May 13, 2023 •

edited

Loading

codecov bot commented May 13, 2023 •

edited

Loading

adrianduffell May 21, 2023 •

edited

Loading