-
Notifications
You must be signed in to change notification settings - Fork 145
Add performance tracks to onboarding flow #8042
Conversation
Adding a new track sounds good to me. When do you plan to release this? WC 6.0? |
This would be part of 6.2 I believe, as it would be in WCA 3.1, given we already cut the RC version of 3.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, @louwie17! Left a couple minor comments, but nothing really blocking depending on your thoughts on them.
@@ -59,27 +59,81 @@ export const filterBusinessExtensions = ( extensionInstallationOptions ) => { | |||
); | |||
}; | |||
|
|||
const timeBoxes = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Timebox is an interesting name. Not super opinionated on this, but maybe timeFrames
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done here: 8ca96a0
{ name: '10-15s', max: 15 }, | ||
{ name: '>15s' }, | ||
]; | ||
function getTimebox( timeInMs ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple quick tests around this would be really awesome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I added them here: 8ca96a0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could optionally also rename to getTimeFrame
to keep these consisten.
const key = | ||
fieldKey === 'woocommerce-payments' | ||
? 'wcpay' | ||
: `${ fieldKey.replace( /-/g, '_' ) }`.split( ':', 1 )[ 0 ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're doing this enough that it may warrant a shared util or some sanitization on the backend even before hitting components, but not a blocker for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done here, thanks for the suggestion: cbf184d
$upgrader = new \Plugin_Upgrader( new \Automatic_Upgrader_Skin() ); | ||
$result = $upgrader->install( $api->download_link ); | ||
$results[ $plugin ] = $result; | ||
$install_time[ $plugin ] = round( ( microtime( true ) - $start_time ) * 1000 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 💯
355843c
to
cbf184d
Compare
@joshuatf thanks for the review, I updated this PR with your feedback, it should be ready for a re-review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @louwie17- thanks for the changes!
Had a couple more thoughts on this that I left below.
@@ -45,7 +46,7 @@ describe( 'BusinessDetails', () => { | |||
jetpack: false, | |||
'google-listings-and-ads': true, | |||
'mailchimp-for-woocommerce': false, | |||
'woocommerce-payments': true, | |||
'woocommerce-payments:us': true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thinking 👍
{ name: '10-15s', max: 15 }, | ||
{ name: '>15s' }, | ||
]; | ||
function getTimebox( timeInMs ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could optionally also rename to getTimeFrame
to keep these consisten.
@@ -0,0 +1,12 @@ | |||
export function getPluginSlug( id: string ): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice sharing this as utils!
I think it would be great to handle this entirely server-side at some point, but this is a great short-term solution.
for ( let [ fieldKey ] of Object.entries( extensionInstallationOptions ) ) { | ||
fieldKey = getPluginSlug( fieldKey ); | ||
const key = getPluginTrackKey( fieldKey ); | ||
if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshuatf is this because these plugins were already installed and just needed to be activated?
This condition is triggered for me, when an installation is required (this does depend on changes in the data package, so maybe that didn't get updated?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was with plugins completely removed. I can try again later today and rebuild all packages to see if that makes a difference.
{ name: '2-5s', max: 5 }, | ||
{ name: '5-10s', max: 10 }, | ||
{ name: '10-15s', max: 15 }, | ||
{ name: '>15s' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added several more time frames all the way up to >60s
, I would be quite concerned if we hit the >60s
often (I could even stop at >30s)
); | ||
|
||
recordEvent( | ||
'storeprofiler_store_business_features_installed_and_activated', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the logic, so we don't call the install and activate hook when all the selected extensions are already activated and installed. This will prevent the above scenario. The changes for this are here: 6e47c4c
cbf184d
to
6e47c4c
Compare
@joshuatf this will be ready for a re-review whenever you have time, or of-course anyone else on @woocommerce/mothra . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes, @louwie17!
This looks good and is testing well in my environments. Most seemed to come in between 20-40s so i think the timeframes are adequate coverage.
6e47c4c
to
3b6b370
Compare
…min#8042) * Add payment_setup track for WooCommerce Payments * Add timing props to tracks in onboarding when installing extensions * Update extension key logic * Add changelog * Add tests for extension installation data for tracks * Add util function for plugin slug parsing * Only install and activate plugins that are not already active
Fixes #7919
This adds one extra onboarding track for the business details free extensions step to add info about installation time.
Also adding one extra prop for the product types step as we sometime install WC Pay there.
Screenshots
cc @pmcpinto & @becdetat I added a new track
storeprofiler_store_business_features_installed_and_activated
for the install time props, see the example above. Does this look ok, or would you prefer to tag it on to one of the existing tracks. I decided on a new track mostly to keep thinks semi clean and allowing for easy removal if we decide we don't want it anymore.Detailed test instructions:
localStorage.setItem( 'debug', 'wc-admin:*' );
storeprofiler_store_business_features_installed_and_activated
success
set to false and afailed_extensions
list.