Additional changes for the core profiler plugins page #38616

moon0326 · 2023-06-06T01:10:00Z

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 #38436 .

This PR addresses changes listed in the issue, except for the Jetpack redirection and InstallAndActivatePlugins refactor. I'll handle Jetpack redirection in a separate issue, as it requires a bit of context switching. @rjchow is working on refactoring InstallAndActivatePlugins in #38577. For the reference, my previous attempt at refactoring is 94e548c

How to test the changes in this Pull Request:

For the label and description, please refer to Figma design Y5pUYSJPsGEud1vknUZhi8-fi-300-14366 or use the screenshot:

Make sure to turn off Show Suggestions from WooCommerce -> Settings -> Advanced -> WooCommerce.com to show the local version of the suggestions.

Go to the Plugins page
Confirm the plugins have the correct label and description.
Confirm the plugins have a `learn more link
Select some plugins and click Continue
Confirm the plugins get installed in order of install_priority
Confirm the change requests from the label and description

For now, I've set install_priority value by plugin size.

Plugin	Priority	Size
Tiktok for Business	1	54 KB
Pinterest for WooCommerce	2	985 KB
WooCommerce Shipping	3	1.6 MB
WooCommerce Tax	4	1.6 MB
WooCommerce Payments	5	5 MB
Google Listing and Ads	6	7.2MB
Mailpoet	7	7.4MB
Jetpack	8	25.7MB

github-actions · 2023-06-06T01:10:22Z

Hi @adrianduffell, @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-06-06T01:10:23Z

Hi @ilyasfoo, @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-06-06T01:33:48Z

Test Results Summary

Commit SHA: f852515

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	259	0	0	2	0	261	0m 51s
E2E Tests	195	0	0	10	0	205	16m 41s

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

I'm not sure what I'm doing wrong, but I wasn't able to get to the plugins page in core profiler. After business_info step, I get redirected straight to loading screen 🤔

moon0326 · 2023-06-06T21:35:54Z

I'm not sure what I'm doing wrong, but I wasn't able to get to the plugins page in core profiler. After business_info step, I get redirected straight to loading screen 🤔

My apologies 🙏 We need to turn off Show Suggestions to use the updated extension list.

I've updated the test instructions.

Make sure to turn off `Show Suggestions` from
WooCommerce -> Settings -> Advanced -> WooCommerce.com 
to show the local version of the suggestions.

ilyasfoo

Thanks for working on this, @moon0326! I wasn't able to review the code properly since I think there's some unrelated commits in the PR, perhaps I'll defer reviewing them after RJ's PR is merged.

I'll review the UI and functionality first:

It seems the extension is displayed as Installed even though it's deactivated. Is this intended?
In smaller screens, Learn more should be hidden (screenshot)
I think it's also good to arrange the extensions as displayed in Figma, cc @verofasulo if you intend the extensions arrangement should follow design, or could it be random?
Probably not related to this PR in particular, I observed some plugins were installed but not activated. I selected multiple plugins (Jetpack, MailPoet, GLA, Pinterest, TikTok, Shipping) and only some plugins were activated (Pinterest, TikTok). The action scheduler job woocommerce_plugins_install_callback was complete. There was some PHP warnings in debug.log related to Pinterest, no fatal errors.

plugins/woocommerce-admin/client/core-profiler/style.scss

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

moon0326 · 2023-06-07T19:22:21Z

Thank you for the review!

It seems the extension is displayed as Installed even though it's deactivated. Is this intended?

I think so, but let's check with @verofasulo ✋

In smaller screens, Learn more should be hidden

Updated in cb255e7

I think it's also good to arrange the extensions as displayed in Figma, cc @verofasulo if you intend the extensions arrangement should follow design, or could it be random?

According to Y5pUYSJPsGEud1vknUZhi8-fi-1698-36088 I think the order is correct.

Do you see different order by any chance?

Probably not related to this PR in particular, I observed some plugins were installed but not activated. I selected multiple plugins (Jetpack, MailPoet, GLA, Pinterest, TikTok, Shipping) and only some plugins were activated (Pinterest, TikTok). The action scheduler job woocommerce_plugins_install_callback was complete. There was some PHP warnings in debug.log related to Pinterest, no fatal errors.

~~I'll double check on that.~~ You're correct on this. I've copied the refactored version of InstallAndActivatePlugins from RJ's PR, which doesn't use InstallAndActivatePluginsAsync. I'll review RJ's PR today so that he can merge it, allowing me to rebase and update the code in this PR to use installAndActivatePlugins.

verofasulo · 2023-06-08T09:17:50Z

if you intend the extensions arrangement should follow design, or could it be random?

@ilyasfoo Yes, please, let's follow the order in the designs 🙂 The marketing team has reviewed it and wants to showcase the extensions in that particular order.

verofasulo · 2023-06-08T09:19:11Z

It seems the extension is displayed as Installed even though it's deactivated. Is this intended?
I think so, but let's check with @verofasulo ✋

@moon0326 I'm not sure what we're referring to 🙂 Could you please clarify? Thanks!

ilyasfoo · 2023-06-08T13:49:20Z

@moon0326 I'm not sure what we're referring to 🙂 Could you please clarify? Thanks!

Hey, @verofasulo! Moon was referring to the extensions having Installed label when it's installed, but not activated. In my opinion, I think we have to label it Installed only when it's both installed & activated. If the plugin is inactive, the user may not know how to activate it, thus it could become broken UX down the line.

ilyasfoo

According to Y5pUYSJPsGEud1vknUZhi8-fi-1698-36088 I think the order is correct.

Do you see different order by any chance?

My bad, I may have referred to an outdated design. Sorry! Thanks for addressing the learn more change. Functionality wise this is looking good! I'll try to review them as soon as RJ's PR is merged and rebased

verofasulo · 2023-06-09T06:38:33Z

Moon was referring to the extensions having Installed label when it's installed, but not activated. In my opinion, I think we have to label it Installed only when it's both installed & activated. If the plugin is inactive, the user may not know how to activate it, thus it could become broken UX down the line.

@ilyasfoo, good point! I didn't think about this specific scenario but it makes sense. Let's show the label only if the extension is active 🙂 Thank you 🙏

* Updated WC payment description * Updated logo images

* Change CTA font size from 13px to 14px * Change spacing between the chebox and logo to 24px * Change heading font-weight to 500

moon0326 · 2023-06-11T02:06:15Z

Thanks @verofasulo and @ilyasfoo ❤️

@ilyasfoo // I've rebased the latest main, which has RJ's changes.

ilyasfoo

I'm getting the same issue with @rjchow here, where plugins list is inconsistent. I think I may have found the way to reproduce it:

Go through the profiler until the plugins step.
In another tab or CLI, delete the option woocommerce_default_country or replace with empty string.
Refresh the plugins list.
Observe the plugin list is not complete.

I'm also getting this error when pressing Continue in the plugins page after the rebase:

Uncaught TypeError: Cannot read properties of undefined (reading 'find')
    at installAndActivatePlugins.ts:198:43
    at Array.sort (<anonymous>)
    at pluginsInstallationQueue (installAndActivatePlugins.ts:197:41)
    at utils.js:417:1
    at Array.reduce (<anonymous>)
    at updateContext (utils.js:399:1)
    at handleAction (actions.js:584:41)
    at processBlock (actions.js:625:1)
    at Array.map (<anonymous>)
    at resolveActions (actions.js:650:32)

Also from my previous review, the plugin is still labelled as Installed when deactivated.

moon0326 · 2023-06-12T23:07:14Z

I'm getting the same issue with @rjchow #38577 (comment), where plugins list is inconsistent. I think I may have found the way to reproduce it:

Thank you for the link. I totally missed the ping from the PR. @rjchow Are you still able to reproduce the issue? I've tried calling the endpoint multiple times and it all came back the same for me.

In another tab or CLI, delete the option woocommerce_default_country or replace with empty string.

As far as I know, woocommerce_default_country always has a value. The plugin list is incomplete because some plugins are only available for certain countries. I think the behavior is expected when woocommerce_default_country value is missing.

I'm also getting this error when pressing Continue in the plugins page after the rebase:

@ilyasfoo Fixed in fcb6b67

Also from my previous review, the plugin is still labelled as Installed when deactivated.

Fixed in e09db7e

rjchow · 2023-06-12T23:19:52Z

I'm getting the same issue with @rjchow #38577 (comment), where plugins list is inconsistent. I think I may have found the way to reproduce it:

Thank you for the link. I totally missed the ping from the PR. @rjchow Are you still able to reproduce the issue? I've tried calling the endpoint multiple times and it all came back the same for me.

In another tab or CLI, delete the option woocommerce_default_country or replace with empty string.

As far as I know, woocommerce_default_country always has a value. The plugin list is incomplete because some plugins are only available for certain countries. I think the behavior is expected when woocommerce_default_country value is missing.

If that's the case I suspect what's going on is that the getFreeExtensions() dispatch is happening before the persistBusinessInfo
call completes

I can look into it when I'm back from AFK tomorrow

ilyasfoo

Thanks for fixing them, @moon0326! I confirmed that plugins are considered Installed only when it's activated.

Probably not related to this PR in particular, I observed some plugins were installed but not activated. I selected multiple plugins (Jetpack, MailPoet, GLA, Pinterest, TikTok, Shipping) and only some plugins were activated (Pinterest, TikTok). The action scheduler job woocommerce_plugins_install_callback was complete. There was some PHP warnings in debug.log related to Pinterest, no fatal errors.

I'm still having the above issue, it seems only 2 out of 5 plugins installed get activated. Not sure why, the scheduled action is completed. I don't mind to have this addressed in a follow-up if you feel like it though.

Not sure if this is expected, but it seems we're using the old APIs, /wp-json/wc-admin/plugins/install?_locale=user and /wp-json/wc-admin/plugins/activate?_locale=user? 🤔

Some minor comments:

ilyasfoo · 2023-06-13T03:46:56Z

packages/js/data/src/onboarding/actions.ts

 		throw new Error();
 	}
 }



I think it'll be good to document the async behaviour, i.e.: how it works and how it's handled in the frontend. Not sure if this is the best place, but I digress. What do you think?

I think we can document it in both frontend and backend. I'll add them 👍

packages/js/data/src/onboarding/types.ts

ilyasfoo · 2023-06-13T04:16:42Z

plugins/woocommerce-admin/client/core-profiler/index.tsx

 						},
+						PLUGINS_LEARN_MORE_LINK_CLICKED: {
+							actions: [
+								'recordTracksPluginsLearnMoreLinkClicked',


It seems recordTracksPluginsLearnMoreLinkClicked isn't defined anywhere? 🤔

🤦 I think I missed it from the last rebase.

Updated in 31f8600

ilyasfoo · 2023-06-13T04:45:43Z

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

 	 *
 	 * @return array
 	 */
 	public static function with_core_profiler_fields( array $plugins ) {


Perhaps we should comment about how how install_priority is handled here such as the ordering since I think maintainers going to need to know what to assign when adding new extensions.

Good idea. I'll copy the table from this PR to the comment.

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

ilyasfoo · 2023-06-13T04:52:50Z

plugins/woocommerce-admin/client/core-profiler/services/installAndActivatePlugins.ts

 			assignPluginsInstallationQueue: assign( {
 				pluginsInstallationQueue: ( ctx ) => {
-					return ctx.selectedPlugins;
+					return ctx.selectedPlugins.slice().sort( ( a, b ) => {


I'd love it if there's a comment on the sorting that is done here, i.e: ascending or descending

Co-authored-by: Ilyas Foo <[email protected]>

…faultFreeExtensions.php Co-authored-by: Ilyas Foo <[email protected]>

moon0326 · 2023-06-13T05:36:30Z

I'm still having the above issue, it seems only 2 out of 5 plugins installed get activated. Not sure why, the scheduled action is completed. I don't mind to have this addressed in a follow-up if you feel like it though.

You're correct. I did not change the endpoint after the last rebase 🤦 ~~I'll handle it first thing tomorrow morning.~~

Updated in 5fb6863

moon0326 · 2023-06-13T05:49:47Z

@ilyasfoo I've addressed missing recordTracksPluginsLearnMoreLinkClicked and activation issues.

Could we address the documentation changes in a follow-up PR?

ilyasfoo

@ilyasfoo I've addressed missing recordTracksPluginsLearnMoreLinkClicked and activation issues.

Thanks! Tests well.

Could we address the documentation changes in a follow-up PR?

Sounds good! 🚢

ilyasfoo · 2023-06-13T07:54:16Z

plugins/woocommerce-admin/client/core-profiler/actions/tracks.tsx

+	{ action }: { action: unknown }
+) => {
+	const { step } = action as { step: string };
+	recordEvent( `storeprofiler_${ step }_learn_more_link_clicked` );


Do we need to track which extension "Learn more" was clicked on? cc @verofasulo if you have input 🙏

I think we should. I'll add it 👍

Updated in 175deec

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

moon0326 requested review from a team, ilyasfoo and rjchow June 6, 2023 01:10

github-actions bot added focus: react admin [team:Ghidorah] package: @woocommerce/data issues related to @woocommerce/data plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Jun 6, 2023

moon0326 force-pushed the update/38436-additional-changes-for-plugins-page branch from 9deb1b4 to e1138d1 Compare June 6, 2023 03:32

github-actions bot added focus: e2e tests Issues related to e2e tests package: @woocommerce/onboarding issues related to @woocommerce/onboarding labels Jun 6, 2023

moon0326 force-pushed the update/38436-additional-changes-for-plugins-page branch 3 times, most recently from 53d73f3 to 9deb1b4 Compare June 6, 2023 04:06

github-actions bot removed focus: e2e tests Issues related to e2e tests package: @woocommerce/onboarding issues related to @woocommerce/onboarding labels Jun 6, 2023

ilyasfoo reviewed Jun 6, 2023

View reviewed changes

moon0326 force-pushed the update/38436-additional-changes-for-plugins-page branch from 1f8da1f to 26c26d6 Compare June 7, 2023 00:27

ilyasfoo reviewed Jun 7, 2023

View reviewed changes

plugins/woocommerce-admin/client/core-profiler/style.scss Outdated Show resolved Hide resolved

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

ilyasfoo reviewed Jun 8, 2023

View reviewed changes

rjchow requested review from a team and adrianduffell and removed request for rjchow June 9, 2023 07:19

rjchow and others added 9 commits June 10, 2023 18:59

ts fixes

bf6d53f

Sort plugins by install_priority

071ecbb

Make sure WooCommerce Shipping is always visible

3d8483b

Update free extension list content

7bc389d

* Updated WC payment description * Updated logo images

Visual changes on the plugin page

c29b492

* Change CTA font size from 13px to 14px * Change spacing between the chebox and logo to 24px * Change heading font-weight to 500

Fix css lint error

a2506ca

Fix gray-900 variable name

9725e3f

Hide learn more link on mobile view

5676c45

Add back learn more link that was removed from rebase

6bc1133

moon0326 force-pushed the update/38436-additional-changes-for-plugins-page branch from cb255e7 to 6bc1133 Compare June 11, 2023 02:05

ilyasfoo reviewed Jun 12, 2023

View reviewed changes

Send pluginsAvailable to pluginInstallermachine

fcb6b67

Use is_activated to determine plugin availability and install status

e09db7e

ilyasfoo suggested changes Jun 13, 2023

View reviewed changes

Moon and others added 2 commits June 12, 2023 22:30

Update packages/js/data/src/onboarding/types.ts

a09dbfd

Co-authored-by: Ilyas Foo <[email protected]>

Update plugins/woocommerce/src/Internal/Admin/RemoteFreeExtensions/De…

de530a5

…faultFreeExtensions.php Co-authored-by: Ilyas Foo <[email protected]>

moon added 2 commits June 12, 2023 22:47

Add back recordTracksPluginsLearnMoreLinkClicked

31f8600

Use install-and-activate-plugins-async when timer is up

5fb6863

ilyasfoo approved these changes Jun 13, 2023

View reviewed changes

moon added 3 commits June 13, 2023 14:07

Record plugin and link with learn more linked clicked event

175deec

Fix failign tests

f45f695

Add comment for install_priority

f852515

moon0326 merged commit 620ff93 into trunk Jun 13, 2023

moon0326 deleted the update/38436-additional-changes-for-plugins-page branch June 13, 2023 22:03

github-actions bot added this to the 7.9.0 milestone Jun 13, 2023

Additional changes for the core profiler plugins page #38616

Additional changes for the core profiler plugins page #38616

Uh oh!

Conversation

moon0326 commented Jun 6, 2023 • edited by adrianduffell 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 Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 6, 2023

Uh oh!

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

Uh oh!

Test Results Summary

Uh oh!

ilyasfoo left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

ilyasfoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

verofasulo commented Jun 8, 2023

Uh oh!

verofasulo commented Jun 8, 2023

Uh oh!

ilyasfoo commented Jun 8, 2023

Uh oh!

ilyasfoo left a comment

Choose a reason for hiding this comment

Uh oh!

verofasulo commented Jun 9, 2023

Uh oh!

moon0326 commented Jun 11, 2023

Uh oh!

ilyasfoo left a comment • 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!

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

Uh oh!

Uh oh!

rjchow commented Jun 12, 2023

Uh oh!

ilyasfoo left a comment

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!

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!

ilyasfoo Jun 13, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

moon0326 commented Jun 6, 2023 •

edited by adrianduffell

Loading

github-actions bot commented Jun 6, 2023 •

edited

Loading

github-actions bot commented Jun 6, 2023 •

edited

Loading

moon0326 commented Jun 6, 2023 •

edited

Loading

moon0326 commented Jun 7, 2023 •

edited

Loading

ilyasfoo left a comment •

edited

Loading

moon0326 commented Jun 12, 2023 •

edited

Loading

ilyasfoo Jun 13, 2023 •

edited

Loading

moon0326 commented Jun 13, 2023 •

edited

Loading