-
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
Hijack the feature eligibility checks on WordPress.com sites #23189
Conversation
Caution: This PR has changes that must be merged to WordPress.com |
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:
|
…res a paid plan in WP.com)
// Manually mapping WordPress.com features to Jetpack module slugs. | ||
foreach ( $plan['features']['active'] as $wpcom_feature ) { | ||
switch ( $wpcom_feature ) { | ||
case 'wordads-jetpack': |
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 we no longer need this, since in D75985-code we're reporting wordads
as the active feature (along with wordads-jetpack
for old Jetpack sites).
…ks-wpcom-sites # Conflicts: # projects/plugins/jetpack/modules/simple-payments/simple-payments.php
// Jetpack_Plan does not exist on WordPress.com. | ||
if ( class_exists( 'Jetpack_Plan' ) ) { | ||
// Jetpack_Plan::get does not exist on WordPress.com. | ||
if ( method_exists( 'Jetpack_Plan', 'get' ) ) { |
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 prevents fatals on WP.com since we're adding a Jetpack_Plan
class there with only the supports
method (see D75906-code).
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.
Would it be better to fully sync Jetpack_Plan to wpcom instead, so when p9dueE-4mF-p2 and later work happens we don't have to figure out how to fix things then?
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 would this is okay and something we can fix properly when Jetpack is on WP.com.
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.
fully sync Jetpack_Plan to wpcom instead
That'd require to refactor the Jetpack_Plan
class in order to ensure its methods also work in WP.com. I took a briefly look at what would be needed, and it didn't seem trivial, so I decided to keep things simple for this PR.
projects/plugins/jetpack/modules/videopress/class.videopress-gutenberg.php
Show resolved
Hide resolved
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 on Jetpack and wpcom simple. I wasn't able to test on Atomic though (for some reason I see no wpcom-features/
directory in my copy of wpcomsh`, so I'll leave that to somebody else.
If you used an existing WoA dev blog created a while ago, that may explain it, since
Already tested in #23189 (review) 🙂 |
Great news! One last step: head over to your WordPress.com diff, D75906-code, and commit it. Thank you! |
r241836-wpcom |
Fixes Automattic/wp-calypso#51593.
Fixes Automattic/wp-calypso#51826.
Fixes Automattic/wp-calypso#61499.
Changes proposed in this Pull Request:
Hijacks the
Jetpack_Plan::supports
function with a call towpcom_site_has_feature
(if exists) to make sure that Jetpack features are gated correctly on WP.com sites.That allows us to simplify some of the existing feature eligibility checks since we can use now
Jetpack_Plan::supports
on all sites.Jetpack product discussion
pdG1ka-46-p2
Does this pull request change what data or activity we track or use?
N/A.
Testing instructions:
Jetpack sites:
sudo global-http enable
.JETPACK__SANDBOX_DOMAIN
.Simple sites:
Atomic sites:
wp-admin/post-new.php
.wp-admin/upload.php
.(*) I first tried this by actually removing the plan from SA, but that left the site on a broken state because Jetpack was deactivated. Didn't look deeply into the issue, but apparently it's caused by a conflict between Jetpack Beta and WoA dev blogs since I can remove a plan without problems when Jetpack Beta is not involved.