Skip to content
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

AT: Disable Jetpack and Vaultpress plugin actions #11817

Merged
merged 9 commits into from
Mar 13, 2017

Conversation

jhnstn
Copy link
Member

@jhnstn jhnstn commented Mar 7, 2017

This hides the plugin actions for Jetpack and Vaultpress on automated transfer sites for the following routes:
http://calypso.localhost:3000/plugins/:site
http://calypso.localhost:3000/plugins/:plugin/:site

plugins/:site

The targeted plugins no longer display the activation or auto update toggles. There is also added messaging: {plugin} is automatically managed on this site

screen shot 2017-03-06 at 5 28 22 pm

plugins/:plugin/:site

Users can still click through the plugin in the list to access details page. Here the actions again are not displayed but there is no additional messaging.

screen shot 2017-03-06 at 5 28 35 pm

The PR does not address bulk changes or non site selected pages ( /plugins or /plugins/:plugin).

The changes here were guided from the feedback in #11541. In particular:

  1. Take a more generalized approach to disabling plugin actions.
  2. Do not grey out or disable clicking through to the detail page.
  3. Consider using server side logic for filtering plugin actions

On the last point this PR does use existing logic in calypso to check for automated transfer sites but the check is done further up the component hierarchy. It should be simple to refactor getAllowedPluginActions and remove the AT site selector after the new attributes are available from the plugins api.

Related PR : #11541
Fixes 84-gh-automated-transfer

@Automattic/stark
cc @johnHackworth @enejb

@matticbot
Copy link
Contributor

@matticbot matticbot added the [Size] L Large sized issue label Mar 7, 2017
@jhnstn jhnstn requested review from retrofox, gwwar, artpi and lamosty March 7, 2017 06:12
@jhnstn jhnstn added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 7, 2017
@jhnstn jhnstn requested a review from rralian March 7, 2017 06:13
@jhnstn jhnstn added the [Feature] Plugins Features related to plugins on WordPress.com, including search, management, and installation. label Mar 7, 2017
getAllowedPluginActions( ) {
return {
autoupdate: true,
activation: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually, we use trailing comma. Not a big deal, though.

@@ -160,6 +160,15 @@ module.exports = React.createClass( {
);
}
}

if ( this.props.isAutoManaged ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this component doesn't have defined rightly any property, I suggest defining isAutoManaged and allowedActions (type and default value) on this patch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated.

notices={ this.props.notices } wporg={ this.props.plugin.wporg } isMock={ this.props.isMock } />
<PluginRemoveButton plugin={ this.props.plugin } site={ this.props.selectedSite }
notices={ this.props.notices } isMock={ this.props.isMock } />
{ canToggleActivation && <PluginActivateToggle plugin={ this.props.plugin } site={ this.props.selectedSite }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lines are increasing too much. Could we separate the properties a little to improve reading? Something like ...

{ canToggleActivation && <PluginActivateToggle
  plugin={ this.props.plugin }
  site={ this.props.selectedSite }
  notices={ this.props.notices }
  isMock={ this.props.isMock } />
}

Not a big deal. Feel free to ignore this comment.

return {
selectedSite,
selectedSiteSlug: getSelectedSiteSlug( state ),
isSiteAutomatedTransfer: isSiteAutomatedTransfer( state, get( selectedSite, 'ID' ) )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing trailing comma

@retrofox
Copy link
Contributor

retrofox commented Mar 7, 2017

The information about why these features are disabled maybe are too generic.

We could add more context information in another patch, though.

Ignore my comments here. I was taking a look to the wrong place. My bad.

@retrofox
Copy link
Contributor

retrofox commented Mar 7, 2017

I wonder if have we get rid of apply transparency to the disabled plugins? cc @rralian #11541 (comment)

if not I think they should be applied on this PR.

return (
<div className="plugin-item__actions">
<PluginActivateToggle
{ canToggleActivation && <PluginActivateToggle
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could hide these components in the same way that they are disabled. Instead of ...

{ canToggleAutoupdate && <PluginAutoupdateToggle
  isMock={ this.props.isMock }
  // ...
  /> }
}

... implement something like ...

<PluginAutoupdateToggle
  isMock={ this.props.isMock }
  hide={ ! canToggleAutoupdate }
  // ...
/>

I think it looks more readable, but I don't wanna bothering you with too many details. Again, feel free to ignore this.

@johnHackworth
Copy link
Contributor

Code-wise, looks good

@retrofox
Copy link
Contributor

retrofox commented Mar 7, 2017

I've tested the implementation, and it looks great. It works like expected.
Beyond some comments that I left, I think it is almost done. Thanks, @jhnstn for this PR. Let's wait for other 👀.

@enejb
Copy link
Member

enejb commented Mar 7, 2017

Lets also update the README.md files if we are updating the props.

For example https://github.com/Automattic/wp-calypso/blob/master/client/my-sites/plugins/plugins-list/README.md

@matticbot matticbot added the [Size] L Large sized issue label Mar 7, 2017
@gwwar
Copy link
Contributor

gwwar commented Mar 8, 2017

We can follow up on this in other PRs, but I think we should add some similar this is managed messaging on plugins/:plugin/:site

We also confusingly show the installation tab
screen shot 2017-03-08 at 11 07 29 am

@gwwar
Copy link
Contributor

gwwar commented Mar 8, 2017

If folks are happy with changes here, let's 🚢 this and follow up in additional PRs. Since this is a larger changeset let's take care to test main flows on stage in addition to the plugin changes here.

@rralian
Copy link
Contributor

rralian commented Mar 10, 2017

Good job! This looks and works great for me, so I say :shipit:. I don't have a strong feeling about removing the "Installation" tab from the plugin info. I agree that we could remove the "installation" tab for installed plugins, but that's a Jetpack issue, not an Automated Transfer issue. I added a separate issue to ask the question for Jetpack in general. #12008

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Plugins Features related to plugins on WordPress.com, including search, management, and installation. [Size] L Large sized issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants