Update/my jetpack status handling #37217
Conversation
…r free and plan checks
|
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?
|
github-actions
bot
added
[Plugin] Backup
There was a problem hiding this comment.
The change feels very good 😌 tests without issues, was trying to activate different plugins - all works according to the test plan!
"Inactive" sounds a bit vague, and can hint as plugin being not activated in WordPress (e.g. if you install & activate Jetpack Backup but don't purchase the subscription). No matter the wording, everything seems more consistent than it was before - and that's the point of this PR.
There was a problem hiding this comment.
When the Search standalone plugin is installed with or without the Jetpack plugin and whether or not the user is connected, when I go to My Jetpack, I get a blank screen with an error trying to read properties of null
This does not happen with any other standalone plugins, and I did confirm this only happens on this branch
Overall I really like the goal of this PR, the statuses seem to make a lot more sense than they used to and more consistent as well
| size: 'small', | ||
| variant: 'primary', | ||
| weight: 'regular', | ||
| label: __( 'Sign In', 'jetpack-my-jetpack' ), |
There was a problem hiding this comment.
I like the wording of "Sign In" much more, but I noticed we've never really called in a sign-in anywhere else
In the connection footer we just call it "Connect"
and on the connection screen we say "Connect your user account"
on the auth screen if we are already logged in we just say "Approve"
and on the login screen we call it "log in" not "sign in"
and when connected, in the connection footer we say "Connected as" not "signed in as"
Overall it seems like we lack consistency in what we call a user connection on the frontend, and I don't think we'll fix that here, but we might want to go with one of the existing names for it rather than introducing yet another name for it, even though I do like "Sign in" much more than a lot of the others as far as clarity. Maybe we could just have it as "Connect" (since that's most of our messaging around it in My Jetpack) or "Log In" (since we use that on the actual auth screen when logging in)
I may be putting too much thought into this, let me know what you think!
There was a problem hiding this comment.
I like the wording of "Sign In" much more, but I noticed we've never really called in a sign-in anywhere else
Good call - this is probably not the diff to change that in :) I'll update that to use the "connect" and "needs user connection" language.
| } else { | ||
| } | ||
| // Check specifically for inactive modules, which will prevent a product from being active | ||
| } elseif ( property_exists( static::class, 'module_name' ) && static::$module_name && ! static::is_module_active() ) { |
There was a problem hiding this comment.
Looks like you have a few PHP errors here
There was a problem hiding this comment.
oopsie :) Thanks.
| $status = 'needs_purchase_or_free'; | ||
| if ( ! static::$has_free_offering ) { | ||
| $status = 'needs_purchase'; | ||
| } |
There was a problem hiding this comment.
Nitpick: This could be shortened to this
| $status = 'needs_purchase_or_free'; | |
| if ( ! static::$has_free_offering ) { | |
| $status = 'needs_purchase'; | |
| } | |
| $status = static::$has_free_offering ? 'needs_purchase_or_free' : 'needs_purchase'; |
There was a problem hiding this comment.
nice optimization 👍
| if ( ! \Jetpack_Options::get_option( 'id' ) ) { | ||
| $status = 'needs_first_site_connection'; | ||
| } else { | ||
| $status = 'site_connection_error'; | ||
| } |
There was a problem hiding this comment.
I see this exact conditional above in the previous if statement. Not sure its necessary since it's only used twice but we could potentially introduce some reusable variable or function for this check
There was a problem hiding this comment.
Might not be needed at this point - I think I'll leave these duplicated instances for now. Thanks for the suggestion though!
| */ | ||
| public static function has_required_plan() { | ||
| $search_state = static::get_state_from_wpcom(); | ||
| return ! empty( $search_state->supports_search ) || ! empty( $search_state->supports_instant_search ); |
There was a problem hiding this comment.
I see in this PR you're removing these different kind of ways to determine if a site has a required plan to run these products. Are there any instances where, in this case Jetpack Search, would be running without any sort of plan, Free or paid? Or below for VideoPress, would there be any instances where the site does have the feature VideoPress but does not have a plan for it? Just making sure we don't break the functionality in certain situations that we may be only vaguely familiar with
There was a problem hiding this comment.
Great question, my goal was to cover those scenarios with the $requires_plan property on the product object. This would indicate if the product needed a plan (free or paid) to run or if it could run in some form with just a plugin installed. $requires_plan is used in the main is_active check on the base product class.
Does this cover the cases you were thinking of?
There was a problem hiding this comment.
I think this does make sense to me, and from all my testing everything mostly makes sense to me 😄
Ahh, good catch. There was an inconsistency in the status slug. |
There was a problem hiding this comment.
This all looks good to me now. I ran through many different scenarios in my testing and didn't see any glaring issues. It's a shame that Search needs a "Free plan" to run because if it weren't for that, we wouldn't have to have a user connection in order to have it Active here
| */ | ||
| public static function has_required_plan() { | ||
| $search_state = static::get_state_from_wpcom(); | ||
| return ! empty( $search_state->supports_search ) || ! empty( $search_state->supports_instant_search ); |
There was a problem hiding this comment.
I think this does make sense to me, and from all my testing everything mostly makes sense to me 😄
Proposed changes:
has_required_planfunction has been removed in favor ofhas_paid_plan_for_product,has_free_plan_for_productandhas_any_plan_for_productOther information:
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Using the Jetpack Beta plugin, apply this PR on a brand new Jetpack site that has the main Jetpack plugin.
Before connecting the site, visit My Jetpack. Confirm that all the product cards are showing as "inactive". This should be true since the site has never been connected before.
Now, connect the site
Return to My Jetpack. Now, any feature that is contained within the main Jetpack plugin (Creator, Stats) will show as "Active". AI will show as "Needs Sign in". Akismet may be active as well depending on your test site and if an API key has been added for Akismet.
If you connect your user account, the AI product should show as "Active" as well.
If you disconnect Jetpack, all products that were active before will now show "Needs Connection"
Test purchasing and activating other products to make sure that the active/inactive statuses make sense