Plugins: Fix/plugins not available on any site

[enejb]

Currently if you have no business plan sites and visit /plugins you end up on this page .

This PR fixes this by

1. Removing the Plugins link in the sidebar if you don't have any sites that can manage plugins.
2. Add a placeholder on the plugins page.
   

cc: @drewblaisdell, @johnHackworth, @rickybanister

[jordwest]

I'm logged in as a test user without a business plan, but I'm having trouble reproducing the first screenshot. I still get the Want to add a store to your site? message on master. Could there be another condition I'm missing? Maybe the abtest in the if statement above is causing me to see a different result.

It also looks like the pluginPageError assignment is similar to the if statement above (with the exception of the actionURL property) - could we extract that out to avoid duplication?

[enejb]

To see the current screen you should be able to just visit
https://wordpress.com/plugins/

It could be that the AB test it causing you not to see same screenshot as me.

Yes you are right about the pluginPageError code. Will fix it soon

[enejb]

@jordwest can you take another look?
Let me know what I can do to move forward with this PR.

[jordwest]

@enejb: A couple of thoughts:

• I'm just thinking through the conditions where this would appear. If the user has a business plan site but no plugins, would sites.hasSiteWithPlugins be true? Would it be better to simply remove the abtest here, since the hasErrorCondition( site, 'noBusinessPlan' ) seems to check the same thing? I'm not too familiar with this part of Calypso; @johnHackworth, @klimeryk, @breezyskies - any thoughts here?
• Kind of nitpicking, but could we capitalize the P for plugin in WpCompluginPageError? Or how about wpcomPluginPageError?
• Is there any particular reason the actionUrl leaves out the site slug? If it's left in, we could use the same as above without re-assigning the actionUrl.
• It looks like a rebase might have brought in a bunch of unrelated commits introduced a syntax error caused by a merge conflict: line 178 of /client/my-sites/sidebar/sidebar.jsx

[enejb]

@jordwest I just:

• update the variable name to be wpcomPluginPageError.
• update the actionUrl so that we add the site.url instead of take it away because it produced plans/undefined url path.
• remove the breaking rebase/eslint fixed commit that was breaking this build.

Regarding the hasErrorCondition( site, 'noBusinessPlan' ) checking the same thing.
Yeah I think it is a bit confusing why one check is happening in access-control.js and the other check is
main.js? I saw a note in the PR that added it. To clear the code later when one of the version wins.

[enejb]

I think that might be a good approach since currently it would be quite a big change to refactor it.

[klimeryk]

I think the order of the conditions should be reversed here - otherwise, if site is undefined, an exception will be thrown.

Scratch that, I thought it was site twice, but it's sites and site.

[enejb]

@klimeryk I think I fixed all the suggestions that you had. Thanks for the review? Do you think this is ready to merge? After a rebase and squashing of some of the commits.

[klimeryk]

Yup, the code looks good now. OK, one more comment.
But testing it on my account, I'm getting a blank page on http://calypso.localhost:3000/plugins, just like in the first screenshot in this issue.
After investigation it looks like this is what's happening in hasRestrictedAccess: site is set to false (as it should be, since no site has been selected), but this causes every condition to fail. Especially the last one: ! sites.hasSiteWithPlugins() && ! site, since I have a site with a Premium plan.
Maybe I just shouldn't be seeing the Plugins link at all in the sidebar. The condition for not seeing it right now is ! this.props.sites.hasSiteWithPlugins() && ! this.isSingle(). That doesn't seem right to me - this.props.sites.hasSiteWithPlugins() && ! this.isSingle() seems more in line with how hasRestrictedAccess behaves. BTW, why the check for isSingle here? :)

[klimeryk]

What if siteSlug is undefined, like in this line?

[jordwest]

This could be handled using an ES6 default parameter:

const getWpcomPluginPageError = ( siteSlug = '' ) => {
  ...
}

[klimeryk]

Ohhh, nice 👍 Gotta remember this ES6 goodness!

[enejb]

@klimeryk and @jordwest anything else before this ready to merge?

[klimeryk]

Thanks for addressing all my comments regarding code. The only thing that's left is the issue I've described in my previous comment.

[enejb]

Thanks for all the reviews @klimeryk very much approached.

The idea behind ! sites.hasSiteWithPlugins() is to show the same error seen in the second screenshot. For users that don't any sites that they can manage. Trying to avoid the first screen.

I think it would be good to remove the plugins link in the sidebar for users that don't have any sites that can manage plugins. But keep the link always for single sites since they would have the screen to upgrade the site for example.

Let me know if the latest changed that I pushed fix the issue you had earlier.

[klimeryk]

Yup, that did it! Thanks for sticking with me while we got to the bottom of this.
A well deserved: !