Prevent module activation when plan doesn't support them #8983

gravityrail · 2018-03-05T21:48:17Z

Some plans support different modules. This change declares plan support in the module header, and checks for that support when rendering the "active plan" information.

This allows us to easily render inactive rows in the jetpack_modules page and elsewhere, so that users know which modules are truly available.

This change also adds some basic checking in WP-cli and elsewhere for whether plan activation was successful.

Changes proposed in this Pull Request:

Declare modules with a Plan: business, premium, personal (default is free) attribute
Render full list of supported modules and plugins (in the case of vaultpress/akismet) in the Jetpack::get_active_plan() response
Render rows as inactive in the jetpack_modules list if they are not supported by the current plan

The goal is NOT to prevent users from being able to hack the code to enable modules they shouldn't (since that is impossible anyway) but to help users understand what features their plan supports, and prevent them from accidentally enabling features that it doesn't.

Further work:

Allow users to click to upgrade to a supporting plan from within the jetpack_modules page.

Testing instructions:

On a site with a free plan:
- Go to /wp-admin/admin.php?page=jetpack_modules
- Ads, Data Backups, Google Analytics, SEO Tools, Search and VideoPress should be greyed out. Only Data Backups should be clickable, with a Configure link.
- Go to /settings/traffic/{site_domain} in Calypso
  - Attempt to enable SEO tools using the Enable link next to SEO Tools module is disabled in Jetpack.
  - You should see a message There was a problem saving your changes. Please try again. (TODO: a better error message - in the network response you will see the actual error is {error: "not_supported", message: "The requested Jetpack module is not supported by your plan."})
On a site with a Premium plan
- VideoPress should become available
On a site with a Professional plan
- SEO Tools, Google Analytics and Search should become available

Proposed changelog entry for your changes:

Show correct available status in Jetpack modules list
Return error in wp-cli if activating a module that is unsupported by your plan

beaulebens · 2018-03-05T22:07:24Z

class.jetpack-modules-list-table.php

@@ -231,8 +220,10 @@ function single_row( $item ) {
 		if ( ! empty( $item['activated'] )  )
 			$row_class .= ' active';

-		if ( ! $this->is_module_available( $item ) )
+		if ( ! Jetpack_Admin::is_module_available( $item ) ) {
+			error_log($item['module']." is NOT available");


This shouldn't be here.

beaulebens · 2018-03-05T22:09:57Z

class.jetpack.php

@@ -4033,7 +4051,9 @@ function admin_page_load() {
 				$module = stripslashes( $_GET['module'] );
 				check_admin_referer( "jetpack_activate-$module" );
 				Jetpack::log( 'activate', $module );
-				Jetpack::activate_module( $module );
+				if ( ! Jetpack::activate_module( $module ) ) {
+					Jetpack::state( 'error', sprintf( __( 'Could not activated %s', 'jetpack' ), $module ) );


Typo: Could not activate

beaulebens · 2018-03-05T22:10:35Z

class.jetpack.php

-				'akismet',
-				'vaultpress',
-				'wordads',
-			);


Did you mean to remove videopress and wordads from here?

Yes - they are modules, the others are plugins. Modules are automatically added to the list based on metadata.

beaulebens · 2018-03-05T22:10:54Z

class.jetpack.php

-				'google-analytics',
-				'wordads',
-				'search',
-			);


As above, did you mean to remove some items from this list?

As above - yes ;)

beaulebens · 2018-03-05T23:59:34Z

On a Free plan
Not a blocker, but noticed that all of the paid options are added to the bottom of the list, vs being in amongst the other items (in alpha order).

I don't have any configuration options on "Data Backups" (on Free plan), although maybe that's correct? I'm not sure what I'd actually configure at this point.

I don't get the

SEO Tools module is disabled in Jetpack.

banner in Calypso (correctly), so can't test that bit. I just get a banner asking me to upgrade to get access to that feature.

On Personal
No apparent differences compared to Free.

I'd expect Data Backups to be available, but it doesn't appear to be.

On Premium
Backups, Google Analytics, SEO and Search remain unavailable (VideoPress becomes available, as expected)

On Professional
Everything was "available" except "Data Backups". I could see the "Configure" link when I hovered, but the module was still "disabled" in the list and could not be Activated via link or checkbox.

gravityrail · 2018-03-06T05:09:38Z

@beaulebens - thanks for the detailed feedback.

Not a blocker, but noticed that all of the paid options are added to the bottom of the list, vs being in amongst the other items (in alpha order).

That appears to be a deliberate feature that was added to the modules list at some point - not a thing I added.

I don't have any configuration options on "Data Backups" (on Free plan), although maybe that's correct? I'm not sure what I'd actually configure at this point.

I'd expect Data Backups to be available, but it doesn't appear to be.

Everything was "available" except "Data Backups". I could see the "Configure" link when I hovered, but the module was still "disabled" in the list and could not be Activated via link or checkbox.

These are existing bugs unrelated to my changes, and should be handled in a separate PR. They also probably need some design/UX work to figure out how things need to behave.

Having now dealt with the code for this change, I actually recommend throwing away the whole modules list code and either just doing without it, or replacing it with a cleanroom implementation. In its current form it is very hard to understand what's going on. Case in point: The HTML implementation that is rendered out as a table appears to be completely replaced in the DOM by a Javascript implementation using JS templates at load time - all the data is being rendered twice, and it is impossible to tell why your changes are having no effect until you realise this is what's going on.

I would encourage you, if you could, to confirm that the Data Backups behaviour is actually identical in master and then either approve this PR as-is, or add fixing that bug as a requirement for this branch. From a cursory look at the code, it appears to be a wholly different kettle of fish.

beaulebens · 2018-03-06T18:38:25Z

Gotcha -- I hadn't realized that "Data Backups" was already in the list; I figured you'd added it as part of this. I've just tested master and am seeing it there as well, with all the same issues as mentioned above.

I've just gone through with that in mind and done some more testing, and this seems to handle things much better and more explicitly than the current master.

beaulebens · 2018-03-06T18:58:01Z

class.jetpack.php

@@ -1452,6 +1452,13 @@ public static function refresh_active_plan_from_wpcom() {
 	 * @return array Active Jetpack plan details
 	 */
 	public static function get_active_plan() {
+		global $active_plan_cache;


Is there any better way to do this than introducing another global?

Unfortunately I couldn't think of one.

To be clear: We could refactor the whole way that we list out modules, and then we'd only need to call this one - but that requires a lot more time than "hack week" necessarily allows.

beaulebens · 2018-03-06T18:58:38Z

class.jetpack.php

-		if ( ! isset( $plan['supports'] ) ) {
-			$plan['supports'] = array();
+		// get available features
+		foreach( self::get_available_modules() as $module_slug ) {


Spacing -- foreach (

beaulebens

This does seem to work in all of my testing, so I'm going to go ahead and approve. I agree that this entire module-listing UI probably needs to be gutted and simplified at some point.

jeherve · 2018-03-12T12:53:55Z

Related: #9033

@tiagonoronha

* Changelog 6.0: create base for changelog. * Add #8938 to changelog * Add #8962 to changelog * Add #8974 to changelog * Add #8975 to changelog * Add #8978 to changelog * Add #8867 to changelog * Add #8937 to changelog * Add #8961 to changelog * Add #8855 to changelog * Add #8944 to changelog * Add #8973 to changelog * Add #8977 to changelog * Add #8979 to changelog * Add #8980 to changelog * Add #8982 to changelog * Add #8983 to changelog * Add #8984 to changelog * Add #8986 to changelog * Add #9005 to changelog * Add #9010 to changelog * Add #9012 to changelog * Add #9021 to changelog * Add #9022 to changelog * Add #9056 to changelog * Add #9061 to changelog * Add #9079 to changelog * Add #9080 to changelog * Add #9088 to changelog * Add #9096 to changelog * Add #9097 to changelog * Add #9100 to changelog * Add #9107 to changelog * Add #8969 to changelog * Add #8993 to changelog * Add #9003 to changelog * Add #9031 to changelog * Add #8945 to changelog * Add #9052 to changelog * Add #9058 to changelog * Add #9066 to changelog * Add #9076 to changelog * Add #9053 to changelog * Add #9108 to changelog * Add #9135 to changelog * Add #9148 to changelog * Add #9125 to changelog * Add #9137 to changelog * Added testing instructions for 6.0. * Added IS testing instructions, huge props to @tiagonoronha. * Added #8498 to changelog. * Added #8954 to changelog. * Added #8985 to changelog. * add #9027 * add #9112 to changelog * add #9136 to changelog * add #9102 to changelog * add #9093 to changelog * add #9062 to changelog * add #9172 to changelog

gravityrail added 2 commits March 5, 2018 12:28

Deactivate search when downgrading Jetpack plan

8730bf1

Centralise active plan checking and use all modules in 'supports' list

43c6662

gravityrail requested a review from a team as a code owner March 5, 2018 21:48

gravityrail added the [Focus] FixTheFlows label Mar 5, 2018

gravityrail self-assigned this Mar 5, 2018

gravityrail added this to the 6.0 milestone Mar 5, 2018

gravityrail added 2 commits March 5, 2018 13:53

Add 'free' to plan classes, always

7477f02

Fix bug showing correct plan support in premium

1905189

gravityrail added Admin Page React-powered dashboard under the Jetpack menu Plans [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Mar 5, 2018

Return better error message via API

ff8fce0

beaulebens reviewed Mar 5, 2018

View reviewed changes

gravityrail added 2 commits March 5, 2018 14:51

Remove rogue error_log

9b2712c

Fix typo

5ed8a9f

beaulebens reviewed Mar 6, 2018

View reviewed changes

Fix spacing

0850e6a

beaulebens approved these changes Mar 6, 2018

View reviewed changes

gravityrail merged commit d2fc8b9 into master Mar 7, 2018

matticbot added the [Status] Needs Changelog label Mar 7, 2018

gravityrail deleted the fix/disable-seo-tools-module-list branch March 7, 2018 00:28

jeherve mentioned this pull request Mar 12, 2018

REST API: Cannot activate some Settings ("The requested Jetpack module is not supported by your plan.") #9033

Closed

oskosk added a commit that referenced this pull request Mar 23, 2018

Add #8983 to changelog

84f195d

oskosk added [Status] Has Changelog and removed [Status] Needs Changelog labels Mar 23, 2018

kraftbj removed the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Sep 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent module activation when plan doesn't support them #8983

Prevent module activation when plan doesn't support them #8983

gravityrail commented Mar 5, 2018 •

edited

Loading

beaulebens Mar 5, 2018

gravityrail Mar 5, 2018

beaulebens Mar 5, 2018

gravityrail Mar 5, 2018

beaulebens Mar 5, 2018

gravityrail Mar 5, 2018

beaulebens Mar 5, 2018

gravityrail Mar 5, 2018

beaulebens commented Mar 5, 2018 •

edited

Loading

gravityrail commented Mar 6, 2018 •

edited

Loading

beaulebens commented Mar 6, 2018

beaulebens Mar 6, 2018

gravityrail Mar 6, 2018

gravityrail Mar 6, 2018

beaulebens Mar 6, 2018

beaulebens left a comment

jeherve commented Mar 12, 2018

Prevent module activation when plan doesn't support them #8983

Prevent module activation when plan doesn't support them #8983

Conversation

gravityrail commented Mar 5, 2018 • edited Loading

Changes proposed in this Pull Request:

Further work:

Testing instructions:

Proposed changelog entry for your changes:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beaulebens commented Mar 5, 2018 • edited Loading

gravityrail commented Mar 6, 2018 • edited Loading

beaulebens commented Mar 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beaulebens left a comment

Choose a reason for hiding this comment

jeherve commented Mar 12, 2018

gravityrail commented Mar 5, 2018 •

edited

Loading

beaulebens commented Mar 5, 2018 •

edited

Loading

gravityrail commented Mar 6, 2018 •

edited

Loading