Coming Soon: check to see if the ETK plugin is installed and hide the setting if so #47569

ramonjd · 2020-11-19T04:19:13Z

Changes proposed in this Pull Request

Hi friends!

Because the new coming soon functionality lives in the WordPress.com Editing Toolkit, if an Atomic users wishes to deactivate or delete that plugin we don't wish to allow them to toggle Coming Soon in the WPCOM settings (because it won't have any effect!)

So we're going to hide the coming soon option when the ETK plugin is disabled/deleted.

Testing instructions

Create an Atomic site by upgrading to Business/Ecommerce

Check that the WordPress.com Editing Toolkit plugin is installed and activated at {your_site}/wp-admin/plugins.php?plugin_status=all&paged=1&s

Head over to http://calypso.localhost:3000/settings/general/{your_site}?flags=coming-soon-v2

You should be able to toggle Coming Soon mode

The badge should be visible when Coming Soon mode is on

Now deactivate the plugin

Return to the settings page: http://calypso.localhost:3000/settings/general/{your_site}?flags=coming-soon-v2

You should see the following

And the badge should be gone

Switch to a simple site

Ensure that Coming Soon mode is working and you can toggle it on and off.

matticbot · 2020-11-19T04:19:15Z

Test live: https://calypso.live/?branch=add/jetpack-query-plugins-check-for-etk

matticbot · 2020-11-19T04:28:09Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~10689 bytes added 📈 [gzipped])

name                    parsed_size           gzip_size
settings                    +6253 B  (+1.2%)    +1504 B  (+1.1%)
zoninator                    +626 B  (+0.2%)     +167 B  (+0.2%)
wp-super-cache               +626 B  (+0.3%)     +167 B  (+0.3%)
woocommerce                  +626 B  (+0.0%)     +167 B  (+0.0%)
themes                       +626 B  (+0.2%)     +167 B  (+0.2%)
theme                        +626 B  (+0.2%)     +167 B  (+0.2%)
stats                        +626 B  (+0.1%)     +167 B  (+0.1%)
sites                        +626 B  (+0.7%)     +167 B  (+0.6%)
site-purchases               +626 B  (+0.1%)     +167 B  (+0.1%)
site-blocks                  +626 B  (+0.2%)     +167 B  (+0.2%)
settings-writing             +626 B  (+0.1%)     +167 B  (+0.1%)
settings-security            +626 B  (+0.2%)     +167 B  (+0.2%)
settings-performance         +626 B  (+0.1%)     +167 B  (+0.1%)
settings-jetpack             +626 B  (+0.3%)     +167 B  (+0.2%)
settings-discussion          +626 B  (+0.2%)     +167 B  (+0.2%)
sensei                       +626 B  (+0.6%)     +167 B  (+0.5%)
security                     +626 B  (+0.1%)     +167 B  (+0.1%)
scan                         +626 B  (+0.2%)     +167 B  (+0.2%)
reader                       +626 B  (+0.1%)     +167 B  (+0.1%)
purchases                    +626 B  (+0.1%)     +167 B  (+0.1%)
privacy                      +626 B  (+0.2%)     +167 B  (+0.2%)
preview                      +626 B  (+0.4%)     +167 B  (+0.4%)
posts-custom                 +626 B  (+0.2%)     +167 B  (+0.2%)
posts                        +626 B  (+0.2%)     +167 B  (+0.2%)
plugins                      +626 B  (+0.1%)     +167 B  (+0.1%)
plans                        +626 B  (+0.1%)     +167 B  (+0.1%)
people                       +626 B  (+0.2%)     +167 B  (+0.2%)
pages                        +626 B  (+0.2%)     +167 B  (+0.2%)
notification-settings        +626 B  (+0.2%)     +167 B  (+0.2%)
migrate                      +626 B  (+0.4%)     +167 B  (+0.4%)
media                        +626 B  (+0.1%)     +167 B  (+0.1%)
me                           +626 B  (+0.2%)     +167 B  (+0.2%)
marketing                    +626 B  (+0.1%)     +167 B  (+0.1%)
jetpack-connect              +626 B  (+0.1%)     +167 B  (+0.1%)
jetpack-cloud-settings       +626 B  (+0.3%)     +167 B  (+0.3%)
jetpack-cloud                +626 B  (+0.7%)     +167 B  (+0.6%)
import                       +626 B  (+0.3%)     +167 B  (+0.2%)
hosting                      +626 B  (+0.2%)     +167 B  (+0.2%)
home                         +626 B  (+0.1%)     +167 B  (+0.1%)
help                         +626 B  (+0.1%)     +167 B  (+0.1%)
hello-dolly                  +626 B  (+0.6%)     +167 B  (+0.5%)
happychat                    +626 B  (+0.2%)     +167 B  (+0.2%)
gutenberg-editor             +626 B  (+0.1%)     +167 B  (+0.1%)
google-my-business           +626 B  (+0.2%)     +167 B  (+0.2%)
export                       +626 B  (+0.3%)     +167 B  (+0.3%)
email                        +626 B  (+0.2%)     +167 B  (+0.2%)
earn                         +626 B  (+0.2%)     +167 B  (+0.2%)
domains                      +626 B  (+0.1%)     +167 B  (+0.1%)
customize                    +626 B  (+0.3%)     +167 B  (+0.3%)
concierge                    +626 B  (+0.2%)     +167 B  (+0.2%)
comments                     +626 B  (+0.1%)     +167 B  (+0.1%)
checkout                     +626 B  (+0.0%)     +167 B  (+0.0%)
backup                       +626 B  (+0.1%)     +167 B  (+0.1%)
activity                     +626 B  (+0.1%)     +167 B  (+0.1%)
account-close                +626 B  (+0.2%)     +167 B  (+0.2%)
account                      +626 B  (+0.2%)     +167 B  (+0.2%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~2296 bytes added 📈 [gzipped])

name                                                 parsed_size           gzip_size
async-load-calypso-blocks-inline-help-popover            +9984 B  (+2.6%)    +1628 B  (+1.6%)
async-load-signup-steps-site-picker                       +626 B  (+1.0%)     +167 B  (+0.9%)
async-load-design-blocks                                  +626 B  (+0.0%)     +167 B  (+0.0%)
async-load-calypso-components-web-preview-component       +626 B  (+0.1%)     +167 B  (+0.1%)
async-load-calypso-components-sites-popover               +626 B  (+1.0%)     +167 B  (+0.9%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

andrewserong · 2020-11-20T00:35:15Z

The UI behaviour in this PR is testing nicely for me, thanks for putting it together!

There's a bit of behaviour that I'm finding confusing, though when activating / deactivating the plugin. If my test site https://andysfakeatomicsite.wpcomstaging.com/ is set to Coming Soon mode, and then I deactivate the plugin, when I request https://andysfakeatomicsite.wpcomstaging.com/ I still get the Coming Soon page. So it looks like Coming Soon is still working somehow? Is this it falling back to Coming Soon v1?

andrewserong · 2020-11-20T00:39:29Z

Screenshot of the UI for me after de-activating the plugin while it's set to Coming Soon mode. Note that it looks like it displays as "Private" but still has the Coming Soon badge... (The front end of the site displays the Coming Soon screen)

ramonjd · 2020-11-20T01:17:06Z

then I deactivate the plugin, when I request https://andysfakeatomicsite.wpcomstaging.com/ I still get the Coming Soon page. So it looks like Coming Soon is still working somehow? Is this it falling back to Coming Soon v1?

Firstly, thanks for testing this one @andrewserong 🙇

I didn't see that in my testing, which doesn't mean that it's not true :)

So long as the flag exists on the setting page ?flags=coming-soon-v2 we should only be toggling Coming Soon mode 2

If that flag isn't there, even once, it will use v1.

Just guessing, but maybe it's the reason behind what you're seeing here too? #47569 (comment)

andrewserong

These UI changes are testing well for me with both a simple site, and an Atomic site with the ETK plugin active, and then with it deactivated. The concerns about unusual behaviour appear to be due to known and unrelated issues 👍

Just left a comment about forceRefresh because I couldn't see how it was changing anything, but otherwise LGTM!

andrewserong · 2020-11-20T01:24:24Z

client/components/data/query-jetpack-plugins/index.jsx

-		if ( isEqual( nextProps.siteIds, this.props.siteIds ) ) {
+		if (
+			isEqual( nextProps.siteIds, this.props.siteIds ) ||
+			nextProps.forceRefresh === this.props.forceRefresh


This change looks fine to me, but I can't see forceRefresh referenced elsewhere in the PR. What does this change do? Are we passing in forceRefresh to QueryJetpackPlugins anywhere? (I might have missed something obvious!)

What does this change do?

It's a hacky way of getting around isEqual( nextProps.siteIds, this.props.siteIds ). The siteIds barely ever change in a single Calypso session and therefore the query component will never perform another fetch to get the plugin list.

Are we passing in forceRefresh to QueryJetpackPlugins anywhere?

No, but we could! :P

Once I got it working I stopped asking questions.

@ramonjd just thinking, could this have an unintended side-effect that if the list of siteIds changes but forceRefresh remains unchanged, then it won't refresh, when it usually should? From taking a second look at this change, it seems like it'll result in fewer refreshes for fetching plugins, rather than more 🤔

From taking a second look at this change, it seems like it'll result in fewer refreshes for fetching plugins, rather than more

Hmmm. What do you think we could better here?

I was thinking that forceRefresh could work like:

const forceRefresh = needToRefresh ? true : false; ... < QueryJetpackPlugins forceRefresh={ forceRefresh } />

Should the logic be something like:

if ( isEqual( nextProps.siteIds, this.props.siteIds ) && ! nextProps.forceRefresh ) {

That way, if forceRefresh is set, then it won't skip refreshing just because the siteIds match? The only problem there is you then need to know when to switch forceRefresh to false or it'll just keep refreshing when the props change (which mightn't be all that frequent, so maybe it's fine?)

I see in #46070 for notifications, they went with the approach of storing in state e.g. didForceRefresh and setForceRefresh so that it isn't constantly switched on to true. This might be a bit overkill for us, here.

Alternately, do we need this change to <QueryJetpackPlugins in this PR? I don't think it's doing anything currently because we're not passing forceRefresh to it from the settings page, so perhaps it could be skipped? Or is this needed for a follow-up change? If we do need the change, then we might want to do a quick smoke test of the other places <QueryJetpackPlugins> is used, just to be sure.

Apologies if I'm being overly cautious! I just realised that I didn't quite know as much about how this component is used as I thought I did when I started reviewing 😅

You're right. It's best to be sure.

I don't think it's doing anything currently because we're not passing forceRefresh to it from the settings page, so perhaps it could be skipped? Or is this needed for a follow-up change?

I wasn't planning on doing anything really. I did smoke test the other pages, e.g., activity-log and it still runs once over there.

Alternately, do we need this change to <QueryJetpackPlugins in this PR?

We need it for the functionality in this PR to work at all. We could extract it into another PR, which would have to be merged first.

I see in #46070 for notifications, they went with the approach of storing in state e.g. didForceRefresh and setForceRefresh so that it isn't constantly switched on to true. This might be a bit overkill for us, here.

Thanks for looking into that. It would be nice to have. Maybe in a follow up? Or do you think we should defend ourselves first?

We need it for the functionality in this PR to work at all.

Ah, gotcha. Because I couldn't see forceRefresh passed in anywhere, I wasn't sure if it was needed. If it gets it working, then I say go for it!

I think if it's enabled the PR to work the way we expect, and if it isn't causing any trouble for the other uses of QueryJetpackPlugins, then I'd just double-check that it's firing when you expect it to and merge it in. Any of the other improvements we can totally do as follow-ups if we run into any issues 👍

Thanks for your patience with this review!

Because I couldn't see forceRefresh passed in anywhere, I wasn't sure if it was needed.

Andy, your reason strikes again... for the good of the world!

I don't need it. I was hallucinating. Thanks for such a thorough review and for discussing this.

Reverting, and merging without. 🙇

matticbot · 2020-11-20T10:09:25Z

Caution: This PR affects files in the Editing Toolkit Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D53081-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing the Editing Toolkit Plugin for more info: PCYsg-ly5-p2

wp-desktop

WordPress Desktop CI Failure for job "wp-desktop-mac".

@ramonjd please inspect this job's build steps for breaking changes at this link. For temporal failures, you may try to "Rerun Workflow from Failed".

Please also ensure this branch is rebased off latest Calypso.

rerunning

…omic/jetpack sites updated setting form to display warning that etk is disabled Ensuring that we run the plugins query refresh every time the component is called, not just when the siteid list changes

Also remove the site badge

repurposing the selector so that it tells us if a site is atomic and the ETK is not available

it's not needed

… coming soon mode

ramonjd requested a review from a team November 19, 2020 04:19

ramonjd self-assigned this Nov 19, 2020

ramonjd requested a review from roo2 November 19, 2020 04:21

andrewserong approved these changes Nov 20, 2020

View reviewed changes

ramonjd added the [Pri] Normal label Nov 20, 2020

ramonjd force-pushed the add/jetpack-query-plugins-check-for-etk branch from 1059554 to 45d0844 Compare November 20, 2020 10:00

wp-desktop previously requested changes Nov 20, 2020

View reviewed changes

ramonjd force-pushed the add/jetpack-query-plugins-check-for-etk branch from 45d0844 to 0caccaf Compare November 20, 2020 10:18

sarayourfriend changed the base branch from master to trunk November 20, 2020 16:10

ramonjd force-pushed the add/jetpack-query-plugins-check-for-etk branch from 655adbb to a6a633c Compare November 21, 2020 08:23

ramonjd added 5 commits November 23, 2020 09:51

Adding check to see if the editing toolkit plugin is activated for at…

d509830

…omic/jetpack sites updated setting form to display warning that etk is disabled Ensuring that we run the plugins query refresh every time the component is called, not just when the siteid list changes

Updating the selector to check if the site is atomic as well

f0e8947

Also remove the site badge

hiding the coming soon option all together

897e891

repurposing the selector so that it tells us if a site is atomic and the ETK is not available

roll back forceRefresh

0898a5b

it's not needed

Ensure the badge says Private if the user has deactivated while in v1…

5d3b041

… coming soon mode

ramonjd force-pushed the add/jetpack-query-plugins-check-for-etk branch from a6a633c to 5d3b041 Compare November 22, 2020 22:52

ramonjd added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 22, 2020

ramonjd merged commit 3b5178d into trunk Nov 23, 2020

ramonjd deleted the add/jetpack-query-plugins-check-for-etk branch November 23, 2020 00:19

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 23, 2020

jsnajdr mentioned this pull request Aug 24, 2021

Check if ETK is disabled using a site option #55681

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Coming Soon: check to see if the ETK plugin is installed and hide the setting if so #47569

Coming Soon: check to see if the ETK plugin is installed and hide the setting if so #47569

ramonjd commented Nov 19, 2020 •

edited

Loading

matticbot commented Nov 19, 2020

matticbot commented Nov 19, 2020 •

edited

Loading

andrewserong commented Nov 20, 2020

andrewserong commented Nov 20, 2020 •

edited

Loading

ramonjd commented Nov 20, 2020 •

edited

Loading

andrewserong left a comment

andrewserong Nov 20, 2020

ramonjd Nov 20, 2020

andrewserong Nov 20, 2020

ramonjd Nov 20, 2020

andrewserong Nov 20, 2020

ramonjd Nov 20, 2020

andrewserong Nov 20, 2020

ramonjd Nov 20, 2020

matticbot commented Nov 20, 2020

wp-desktop left a comment

Coming Soon: check to see if the ETK plugin is installed and hide the setting if so #47569

Coming Soon: check to see if the ETK plugin is installed and hide the setting if so #47569

Conversation

ramonjd commented Nov 19, 2020 • edited Loading

Changes proposed in this Pull Request

Testing instructions

matticbot commented Nov 19, 2020

matticbot commented Nov 19, 2020 • edited Loading

andrewserong commented Nov 20, 2020

andrewserong commented Nov 20, 2020 • edited Loading

ramonjd commented Nov 20, 2020 • edited Loading

andrewserong left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matticbot commented Nov 20, 2020

wp-desktop left a comment

Choose a reason for hiding this comment

ramonjd commented Nov 19, 2020 •

edited

Loading

matticbot commented Nov 19, 2020 •

edited

Loading

andrewserong commented Nov 20, 2020 •

edited

Loading

ramonjd commented Nov 20, 2020 •

edited

Loading