-
Notifications
You must be signed in to change notification settings - Fork 798
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add site lifecycle status guess to My Jetpack #35815
Conversation
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped. Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Backup plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Boost plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Search plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Social plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Starter Plugin plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Protect plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Videopress plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Migration plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
@robertsreberski i am guessing (based on the code) I should be looking at |
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.
"Brand New Sites"
May or may not come from a known host (we can do our best to guess)
I don't see any check for this yet
"New Sites"
Have 3 or fewer Jetpack modules activated
I think we still need to check this, the way it is right now they would be labelled as "New" if there are no purchases regardless of the amount of modules
When I'm thinking about it, it's not really a check I suppose, I don't see how it can distinguish "Brand New Sites" 🤔 We just have that information in the state, but don't include it in lifecycle status. |
I was referencing "may or may not come from a known host" since there doesn't seem to be any best guess type code as it pertains to that. I am not sure if that exists though |
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.
Thank you for the changes! I had a few questions about the condition for "new" and "brand new" users. It seems, at least with my testing, we could be mislabelling some users with these conditions
getUserStats( state ); | ||
|
||
// 'new' = no purchases + less than 3 features + less than 3 modules | ||
if ( purchases.length === 0 && features.length < 3 && modules.length < 3 ) { |
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.
One note about the features being checked here. I created a new jurassic ninja site without anything pre-downloaded (other than whatever comes with the Jetpack plugin normally) and as soon as I connected my user account, I had 17 features in my features array
I am not 100% sure this is the behavior for all jetpack sites, but if it is, that would mean that as soon as a user connects their site, they'd automatically be labelled as "established" since they don't have a purchase yet and wouldn't get stopped with the "settling-in" condition
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 catching that; indeed, some features are enabled by default. I'll omit them in the features array!
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've updated the code to add hardcoded array of default features. Unfortunately, I don't see any easy access to them; they are essentially features from the WPCOM_Features
that include the JETPACK_ALL_SITES
slug.
Not sure if that solution is good enough or if we should update the get_available_features_for_blog_uncached()
method (fbhepr%2Skers%2Sjcpbz%2Sjc%2Qpbagrag%2Snqzva%2Qcyhtvaf%2Sjcpbz%2Qovyyvat%2Sfgber%2Qcebqhpg%2Qyvfg.cuc%3Se%3Q1prs6727%232141-og) to, e.g., include default_features
array in the response. It might be an overkill, though, unnecessarily increasing the size of every payload. And the array of default features isn't likely to change often or in the near future 🤔
What do you think?
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.
Hmmmm, even with that added defaults array, I still had 4 features active as soon as I connected my site
(Ignore scan, that is because I purchased Protect)
I think those social features may be active by default? And I am also wondering if there are other default features added depending on the host?
Do you think it might be better to just leave the features out of these calculations? The modules and purchases may be enough if we can't confidently know what and how many features are active by default
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 think I contributed to this 😬 In the task description I used the word "features" to mean "Jetpack modules", not thinking about the wpcom_features array.
I agree with @CodeyGuyDylan - let's simplify this by dropping the features check and just use purchases and Jetpack modules.
So sorry for the mislead there @robertsreberski 🤦
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.
Yeah, that simplifies things 😅 sounds good, thanks for your feedback here 👍
I've been working on such code last month (#34864), and it's already accessible as |
Oh ok I misunderstood, I thought there was going to be some sort of host check when determining how new users are. This makes sense, thank you |
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 think I created some trouble in the initial description - my apologies!
I think this will be a quick cleanup to remove "features" from the data gathered and checks. Then this should be good to go.
getUserStats( state ); | ||
|
||
// 'new' = no purchases + less than 3 features + less than 3 modules | ||
if ( purchases.length === 0 && features.length < 3 && modules.length < 3 ) { |
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 think I contributed to this 😬 In the task description I used the word "features" to mean "Jetpack modules", not thinking about the wpcom_features array.
I agree with @CodeyGuyDylan - let's simplify this by dropping the features check and just use purchases and Jetpack modules.
So sorry for the mislead there @robertsreberski 🤦
} | ||
|
||
// 'settling-in' = 1 purchase and less than 10 features | ||
if ( purchases.length === 1 && features.length < 10 ) { |
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.
Related to the comment above, I think this can be a modules check instead of a features check
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 think this looks good now, thanks for all the back and forth about the conditions 😅 . I left one non-blocking comment about a comment, everything else looks good
return 'settling-in'; | ||
} | ||
|
||
// 'established' = 2 or more purchases and 10 or more modules |
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 think this is inaccurate, the user will be marked as 'established' if they have 2 or more purchases OR 10 or more modules
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 updates - LGTM!
Proposed changes:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
window.myJetpackInitialState.userStats
includes correct informationgetGuessedSiteLifecycleStatus()
whether it matches the following conditions: