-
Notifications
You must be signed in to change notification settings - Fork 799
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
SEO Tools: use new feature gating for WPcom sites #23685
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 (other than "Required review") appearing at the bottom of this PR are passing or skipped. Jetpack plugin:
|
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 know this is still a draft, but I went ahead and left a drive-by review anyway. Looks good!
Also tracking this here: c/iUNrEVZP-tr
* @return bool True if SEO tools are enabled, false otherwise. | ||
*/ | ||
public static function is_enabled_jetpack_seo( $site_id = 0 ) { | ||
public static function is_enabled_jetpack_seo() { |
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.
Technically I guess you could say this is an API change, but I confirmed nothing in WPCOM is calling it with a site ID passed in, and just in case something else out there is, it wouldn't actually change the behavior anyway (because the function always returns true
for Jetpack).
So 👍 from me at least.
projects/plugins/jetpack/modules/seo-tools/class-jetpack-seo-utils.php
Outdated
Show resolved
Hide resolved
return has_any_blog_stickers( array( 'business-plan', 'ecommerce-plan' ), $site_id ); | ||
// For WPcom sites. | ||
if ( defined( 'IS_WPCOM' ) && IS_WPCOM && method_exists( 'Jetpack_Plan', 'supports' ) ) { | ||
return Jetpack_Plan::supports( 'advanced-seo' ); |
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 checked the feature code and confirmed this looks like the correct one to call, and that it is supported for Pro/Business/eCommerce, which is exactly what we want.
6f72fed
to
0740781
Compare
This could maybe use a code comment adjustment (see my earlier comment) but is otherwise ready to go -- consider it approved. We can switch this back to the main "Needs Review" label once the tests pass (for the Jetpack Crew), I guess? I've already tested this on WPCOM and am approving D77860-code now also (we should be able to deploy that before this one to get the bugfix out before the Pro plan is launched). |
0740781
to
550b9c8
Compare
👍 |
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.
This tests well for me in Jetpack. 👍
In #23189, we started using the new feature gating for WPcom sites in
Jetpack_Plan::supports()
. Given upcoming Dotcom pricing changes, this moves the feature gating for the SEO Tools to useJetpack_Plan
on WPcom sites.See: pdgrnI-wV-p2#comment-1061
Changes proposed in this Pull Request:
Jetpack_Plan::supports()
instead of blog stickers for the SEO Tools gatingDoes this pull request change what data or activity we track or use?
N/A
Testing instructions:
Jetpack sites:
Simple sites:
Atomic sites: