Editor: Hide SEO description for private/hidden sites #11579

jeremeylduvall · 2017-02-23T22:04:51Z

We disable the SEO settings for any site that is private. This commit disables the SEO description drawer in the Editor when a site is private as well. This is the second attempt after #11476 broke production (see p4TIVU-5UZ-p2).

The previous attempt broke production because it assumed this.props.siteSettings would exist when someone tried to load the editor. With a clear cache, this isn't necessarily true. We fetch site settings in specific instances like when someone tries to visit the settings page directly. When users tried to load the editor with an empty initial state, they received an error because blog_public wasn't defined yet leading to a broken experience.

In the revised approach:

We use the specific getSiteSetting selector under state/selectors/.
There are two safety valves. First, we check the truthiness of this.props.privacySetting. Then, we set a default value of 1 as a fallback.

I did test this on a cleared cache in my regular browser and an incognito window. I didn't experience any of the errors from the previous attempt.

To test

Open up your site settings for a site on the Business plan: http://calypso.localhost:3000/settings/general/
Change the site to public
Click "Add" next to posts to start a new post. Verify that you see the SEO description panel in the editor drawer.
Click "My Sites" and go back to your site settings. Set the site to either hidden or private.
Start a new post again and verify that you do not see the SEO description panel in the drawer.

Resolves: #11406

matticbot · 2017-02-23T22:04:54Z

Test live: https://calypso.live/?branch=fix/11406-hide-seo-panel

aduth · 2017-02-24T02:21:22Z

client/post-editor/editor-drawer/index.jsx

@@ -246,7 +247,10 @@ const EditorDrawer = React.createClass( {

 		const { plan } = this.props.site;
 		const hasBusinessPlan = isBusiness( plan ) || isEnterprise( plan );
-		if ( ! hasBusinessPlan ) {
+		const { privacySetting = 1 } = this.props;


It doesn't appear that the default value here would ever be used; partly because the condition above ! this.props.privacySetting would abort on a falsey value, but also because mapStateToProps always provides at least a null value based on the behavior of getSiteSetting. This would not trigger the default (see example).

aduth

If you're expecting to use site settings, you should render a <QuerySiteSettings /> in the component's render so you're not relying on circumstances of other components on the page.

Also, would the isPrivateSite selector be an appropriate alternative? It uses information from the site object, which is more readily available.

jeremeylduvall · 2017-03-01T13:47:02Z

@aduth Thanks for the review. I'm now rendering in the QuerySiteSettings data component. In my testing, selectors that depend on the site settings object specifically versus the site object are more responsive to changes. With the site object, if I update the privacy options, there have been instances where isPrivateSite returns the incorrect value. I'm assuming that's because they're from different endpoints?

aduth · 2017-03-01T15:18:50Z

With the site object, if I update the privacy options, there have been instances where isPrivateSite returns the incorrect value. I'm assuming that's because they're from different endpoints?

Yeah, based on my understanding, this is expected but could be improved. I ran into a similar case with site icon, in that it's duplicated between both the settings endpoint and the sites endpoint. In my case, I deferred to having the sites state be the preferred (canonical) source, tracking updates to site settings in the sites reducer. We should probably do something similar for privacy settings.

jeremeylduvall · 2017-03-02T13:53:45Z

Good call. I like that approach.

I modified the sites reducer to monitor changes in blog privacy as well. I left comments to this effect in the code for clarity, but since the settings endpoint returns a numerical value for blog_public, we need to use the parseInt expression to convert that to true/false. The isPrivateSite selector now first looks at the site state and has a fallback to site settings.

Extra sanity check since it broke prod last time, with the updates to isPrivateSite, a check in editor-drawer/index.jsx is no longer necessary to prevent errors on a clear cache. I tested on a cleared cache in an incognito window. Worked fine on my side.

Tests for isPrivateSite are now failing. Will update now.

jeremeylduvall · 2017-03-02T14:13:10Z

Tests for isPrivateSite are now updated to match the selector behavior. You can run the specific test for isPrivateSite using the following command:

npm run test-client client/state/selectors/test/is-private-site.js

CircleCI still doesn't like me. Investigating.

jeremeylduvall · 2017-03-02T15:45:39Z

Tests should all be passing now. Long story (hopefully) short: With the updated behavior in isSitePrivate, we now fall back on site settings if site doesn't exist. This calls getSiteSettings.

We use isPrivateSite in isSiteSupportingImageEditor. In the test file, when you provide an empty sites object, you also need to provide an empty siteSettings object. Otherwise, getSiteSettings will fail on the fall back. I've added a comment to this effect for clarity, but I can remove if it's not necessary.

aduth · 2017-03-03T14:01:46Z

client/post-editor/editor-drawer/index.jsx

@@ -328,7 +331,8 @@ export default connect(
 		return {
 			canJetpackUseTaxonomies: isJetpackMinimumVersion( state, siteId, '4.1' ),
 			jetpackVersionSupportsSeo: isJetpackMinimumVersion( state, siteId, '4.4-beta1' ),
-			typeObject: getPostType( state, siteId, type )
+			typeObject: getPostType( state, siteId, type ),
+			isSitePrivate: isPrivateSite( state, siteId ),


Doing cartwheels to avoid no-shadow ? 😛 Seems fine as isPrivate too if that would be clearer than the word shuffling.

Ha I take ESLint rules very seriously :)

aduth · 2017-03-03T14:04:32Z

client/state/selectors/is-private-site.js

+
+	if ( settings ) {
+		// Site settings returns a numerical value for blog_public.
+		return parseInt( settings.blog_public, 10 ) !== 1;


Can we rely on settings.blog_public to always be a string? My hunch is maybe not (via optimistic state updates), in which case what you have here is probably good for safety. If we could be confident, we might be able to simply return '1' !== settings.blog_public.

I'd prefer to keep the current behavior. I feel like I've seen multiple behaviors while testing this out.

aduth · 2017-03-03T14:07:12Z

client/state/sites/reducer.js

 				};
+			} else if ( settings.hasOwnProperty( 'site_icon' ) ) {


Couldn't a settings payload have both blog_public and site_icon? In which case we'd only update one or the other because of else if. Probably better as a separate if.

Couldn't a settings payload have both blog_public and site_icon? In which case we'd only update one or the other because of else if. Probably better as a separate if.

To this point, we should probably add a test for this case.

Couldn't a settings payload have both blog_public and site_icon? In which case we'd only update one or the other because of else if. Probably better as a separate if.

I ran through some scenarios, and I don't think it's likely that the settings payload will actually ever contain both a blog_public and site_icon update. When the site icon is updated, we call saveSiteSettings immediately which automatically saves the settings with only the icon property. You can see this if you change the blog privacy (but don't save), update the icon (see the "Settings Saved" dialogue), and then try to leave the page. You'll see an error because the blog privacy setting is still queued.

I'm going to update it either way just in case this behavior is changed in the future.

I think the more likely scenario is that we receive an entire fresh set of site settings via SITE_SETTINGS_RECEIVE, which would always include both keys, one or the other or both potentially having been updated.

aduth · 2017-03-03T14:07:52Z

client/state/sites/reducer.js

+			if ( settings.hasOwnProperty( 'blog_public' ) ) {
+				// Site settings returns a numerical value for blog_public.
+				// Site state expects a true/false value for is_private.
+				const sitePrivacy = parseInt( settings.blog_public, 10 ) !== 1;


Minor: Might be nice to indicate boolean value by prefixing with is: const isPrivate = ...

aduth · 2017-03-03T14:23:15Z

client/state/sites/reducer.js

@@ -138,34 +138,49 @@ export function items( state = {}, action ) {

 			// A site settings update may or may not include the icon property.
 			// If not, we should simply return state unchanged.
-			if ( ! settings.hasOwnProperty( 'site_icon' ) ) {
+			if ( ! settings.hasOwnProperty( 'site_icon' ) && ! settings.hasOwnProperty( 'blog_public' ) ) {


Not sure what gotchas you might encounter, but doing a reduce on the keys we're monitoring might be a nicer way to encapsulate the separate logic.

Something like:

return reduce( [ 'blog_public', 'site_icon' ], ( memo, key ) => { if ( ! settings.hasOwnProperty( key ) ) { return memo; } switch ( key ) { case 'blog_public': // ... case 'site_icon': // ... } return memo; }, state );

(SITE_RECEIVE reducer above is similar in how it reduces using state as initial value while avoiding direct mutations)

jeremeylduvall · 2017-03-08T13:59:11Z

client/state/sites/test/reducer.js

@@ -412,7 +412,7 @@ describe( 'reducer', () => {
 				}
 			} );

-			expect( state ).to.equal( original );
+			expect( state ).to.eql( original );


Since we're now returning memo, which is a copy of state, it was necessary to change these to eql.

That we needed to change the tests here implies that we're returning a new state reference in the reducer where we shouldn't be (no changes occurred). I think in your reducer's switch where you're breaking upon finding that there are no differences, we should return memo; instead to bypass the rest of the reduce and ensure that we only create a copy of the state object (return a new reference) if we're certain that a change has actually occurred.

Good call. Updated.

aduth · 2017-03-08T16:28:32Z

client/state/sites/reducer.js

+						if ( null === mediaId ) {
+							// Unset icon
+							nextSite = omit( nextSite, 'icon' );
+							break;


This break; is redundant since there's no other logic in the case to be run except the final break; anyways.

aduth · 2017-03-08T16:29:27Z

client/state/sites/reducer.js

+				switch ( key ) {
+					case 'blog_public':
+						const isPrivate = parseInt( settings.blog_public, 10 ) !== 1;
+						nextSite.is_private = isPrivate;


I think we'll want to check (and add test case for) whether the value has actually changed. In this case we're always updating the site state even if the value we've received is the same as what we're currently tracking.

aduth · 2017-03-08T16:34:31Z

client/state/sites/reducer.js

 				return state;
 			}

-			const mediaId = settings.site_icon;
+			let nextSite = { ...site };


Minor-ish: We're creating a shallow clone of the site even if we never end up making any changes. We could do this at the point we need it instead:

let nextSite = site; // ... case 'blog_public': const isPrivate = parseInt( settings.blog_public, 10 ) !== 1; if ( site.is_private === isPrivate ) { return memo; } nextSite = { ...nextSite, is_private: isPrivate }; break;

Only downside of this is that if multiple keys have new values, we'd clone the site multiple times. I think it's the much more likely scenario that nothing will have changed (thus zero cloning) than multiple changed values though.

jeremeylduvall · 2017-03-09T02:16:33Z

I went back and forth on whether or not to clone site outside of the switch block so I could avoid doing the repeated nextSite gymnastics.

nextSite = { ...nextSite, // ... }

You make a good point about frequency. I updated the behavior and added a test case for:

should return same state if received privacy setting and matches current value

aduth

This looks good and works well for me 👍

I did notice that, after changing my business site to private, the next SITE_RECEIVE caused a couple other changes to state. Of them, options.default_sharing_status is probably one that makes sense as being affected by the change in blog_public. Not something that we necessarily need to change here, but maybe in a subsequent pull request.

aduth

Oh, actually, I just noticed that the SEO Description field shows for a hidden business site. Is that expected? I mean, it makes sense given what we're testing (a hidden site does not mean site.is_private). Which means our reducer logic for testing whether blog_public is equal to 1 is not actually accurate.

jeremeylduvall · 2017-03-09T15:12:29Z

I'm not seeing the SEO description field on my side when a site is changed to hidden. site.is_private should return true if the blog_public setting is set to -1 (private) or 0 (hidden). It only returns false if blog_public is set to 1 (public).

Here's a GIF of what I'm experiencing with a hidden site:

aduth · 2017-03-09T15:16:41Z

The problem is that the reducer logic for is_private is not accurate.

In this pull request, we test:

const isPrivate = parseInt( settings.blog_public, 10 ) !== 1;

Server-side, we test:

function is_private() {
	return ( -1 == get_option( 'blog_public' ) );
}

Key difference being that our reducer is treating blog_public of 0 as a private site, but this is not true as considered server-side.

jeremeylduvall · 2017-03-13T14:16:39Z

Per a Slack convo with @aduth, I updated this PR to treat -1 as private and 0 (hidden) as public to match the logic we use on the server. This means that the SEO description will still display for hidden sites at the moment. I'll fix this by introducing a new selector for isHiddenSite. The logic for the SEO description would then be:

if ( ! hasBusinessPlan || isPrivate || isHidden ) {
  	return;
}

~~CircleCI tests are failing.~~ Fixed after rebasing with #12046 and rerunning the tests.

This PR introduces a new selector to test whether or not a site is hidden. If the site is hidden, the SEO description is hidden in the post/page editor as well. Related: #11579 Resolves: #11406

aduth

LGTM 👍

We disable the SEO settings for any site that is private. This commit disables the SEO description drawer in the Editor when a site is private as well. This is the second attempt after #11476 broke production. Both the site and site settings endpoints return a site privacy setting. This PR establishes the site state as the preferred source of truth and updates the isPrivateSite selector as a result. The SEO description will still appear for hidden sites since .is_private will return false. I'll fix that up in another PR with an addition selector for isHiddenSite(). Resolves: #11406

lancewillett · 2017-03-21T01:05:53Z

Really happy to see this one. Nice work! 🎉

jeremeylduvall added [Feature] Post/Page Editor The editor for editing posts and pages. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 23, 2017

jeremeylduvall self-assigned this Feb 23, 2017

jeremeylduvall requested a review from aduth February 23, 2017 22:04

matticbot added the [Size] S Small sized issue label Feb 23, 2017

aduth reviewed Feb 24, 2017

View reviewed changes

yurynix mentioned this pull request Mar 1, 2017

Hide SEO button for domain only sites #11666

Merged

2 tasks

jeremeylduvall force-pushed the fix/11406-hide-seo-panel branch from e4ed244 to 7d5ef86 Compare March 1, 2017 13:39

matticbot added the [Size] S Small sized issue label Mar 1, 2017

jeremeylduvall force-pushed the fix/11406-hide-seo-panel branch from 7d5ef86 to 93af5bb Compare March 2, 2017 13:43

matticbot added [Size] M Medium sized issue and removed [Size] S Small sized issue labels Mar 2, 2017

matticbot added [Size] L Large sized issue and removed [Size] M Medium sized issue labels Mar 2, 2017

jeremeylduvall force-pushed the fix/11406-hide-seo-panel branch from 529176d to 7d08e9c Compare March 2, 2017 15:39

matticbot added the [Size] L Large sized issue label Mar 2, 2017

aduth reviewed Mar 3, 2017

View reviewed changes

jeremeylduvall force-pushed the fix/11406-hide-seo-panel branch from 7d08e9c to 3db5409 Compare March 8, 2017 13:52

matticbot added [Size] XL Probably needs to be broken down into multiple smaller issues and removed [Size] L Large sized issue labels Mar 8, 2017

jeremeylduvall force-pushed the fix/11406-hide-seo-panel branch from 3db5409 to 148ef73 Compare March 8, 2017 13:57

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 8, 2017

jeremeylduvall commented Mar 8, 2017

View reviewed changes

aduth reviewed Mar 8, 2017

View reviewed changes

jeremeylduvall force-pushed the fix/11406-hide-seo-panel branch from e3ce71e to 87946cc Compare March 9, 2017 02:12

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 9, 2017

aduth approved these changes Mar 9, 2017

View reviewed changes

aduth suggested changes Mar 9, 2017

View reviewed changes

aduth added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 9, 2017

jeremeylduvall force-pushed the fix/11406-hide-seo-panel branch from 87946cc to 0b6cb5d Compare March 13, 2017 14:06

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 13, 2017

jeremeylduvall 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 Author Reply labels Mar 13, 2017

jeremeylduvall force-pushed the fix/11406-hide-seo-panel branch from 0b6cb5d to 6ca148d Compare March 13, 2017 14:19

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 13, 2017

jeremeylduvall mentioned this pull request Mar 15, 2017

Editor: Hide SEO description for hidden sites #12164

Merged

aduth approved these changes Mar 17, 2017

View reviewed changes

jeremeylduvall force-pushed the fix/11406-hide-seo-panel branch from 6ca148d to 3d24435 Compare March 18, 2017 20:17

jeremeylduvall added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 18, 2017

jeremeylduvall merged commit 6e96d19 into master Mar 21, 2017

jeremeylduvall deleted the fix/11406-hide-seo-panel branch March 21, 2017 16:53

matticbot added [Size] XL Probably needs to be broken down into multiple smaller issues and removed [Status] Ready to Merge labels Mar 21, 2017

jeremeylduvall mentioned this pull request Apr 6, 2017

SEO settings: hide editor sidebar SEO Description menu for non-indexed sites #11406

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editor: Hide SEO description for private/hidden sites #11579

Editor: Hide SEO description for private/hidden sites #11579

jeremeylduvall commented Feb 23, 2017 •

edited

Loading

matticbot commented Feb 23, 2017

aduth Feb 24, 2017

aduth left a comment

jeremeylduvall commented Mar 1, 2017

aduth commented Mar 1, 2017

jeremeylduvall commented Mar 2, 2017

jeremeylduvall commented Mar 2, 2017 •

edited

Loading

jeremeylduvall commented Mar 2, 2017

aduth Mar 3, 2017

jeremeylduvall Mar 4, 2017 •

edited

Loading

aduth Mar 3, 2017

jeremeylduvall Mar 8, 2017

aduth Mar 3, 2017

aduth Mar 3, 2017

jeremeylduvall Mar 5, 2017 •

edited

Loading

aduth Mar 6, 2017

aduth Mar 3, 2017

aduth Mar 3, 2017

jeremeylduvall Mar 8, 2017

aduth Mar 8, 2017

jeremeylduvall Mar 8, 2017

aduth Mar 8, 2017

aduth Mar 8, 2017

aduth Mar 8, 2017

jeremeylduvall commented Mar 9, 2017

aduth left a comment

aduth left a comment

jeremeylduvall commented Mar 9, 2017

aduth commented Mar 9, 2017 •

edited

Loading

jeremeylduvall commented Mar 13, 2017 •

edited

Loading

aduth left a comment

lancewillett commented Mar 21, 2017

Editor: Hide SEO description for private/hidden sites #11579

Editor: Hide SEO description for private/hidden sites #11579

Conversation

jeremeylduvall commented Feb 23, 2017 • edited Loading

To test

matticbot commented Feb 23, 2017

Choose a reason for hiding this comment

aduth left a comment

Choose a reason for hiding this comment

jeremeylduvall commented Mar 1, 2017

aduth commented Mar 1, 2017

jeremeylduvall commented Mar 2, 2017

jeremeylduvall commented Mar 2, 2017 • edited Loading

jeremeylduvall commented Mar 2, 2017

Choose a reason for hiding this comment

jeremeylduvall Mar 4, 2017 • edited Loading

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

jeremeylduvall Mar 5, 2017 • edited Loading

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

Choose a reason for hiding this comment

jeremeylduvall commented Mar 9, 2017

aduth left a comment

Choose a reason for hiding this comment

aduth left a comment

Choose a reason for hiding this comment

jeremeylduvall commented Mar 9, 2017

aduth commented Mar 9, 2017 • edited Loading

jeremeylduvall commented Mar 13, 2017 • edited Loading

aduth left a comment

Choose a reason for hiding this comment

lancewillett commented Mar 21, 2017

jeremeylduvall commented Feb 23, 2017 •

edited

Loading

jeremeylduvall commented Mar 2, 2017 •

edited

Loading

jeremeylduvall Mar 4, 2017 •

edited

Loading

jeremeylduvall Mar 5, 2017 •

edited

Loading

aduth commented Mar 9, 2017 •

edited

Loading

jeremeylduvall commented Mar 13, 2017 •

edited

Loading