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

[Remove SitesList] Add isSiteCustomizable selector #13160

Merged
merged 10 commits into from
Apr 21, 2017
6 changes: 0 additions & 6 deletions client/lib/site/computed-attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,5 @@ export default function( site ) {
isHttps( attributes.options.unmapped_url )
);

//TODO:(ehg) Replace instances with canCurrentUser selector when my-sites/sidebar is connected
attributes.is_customizable = !! (
site.capabilities &&
site.capabilities.edit_theme_options
);

return attributes;
}
4 changes: 0 additions & 4 deletions client/lib/site/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -218,8 +218,4 @@ Site.prototype.isUpgradeable = function() {
return this.capabilities && this.capabilities.manage_options;
};

Site.prototype.isCustomizable = function() {
return !! ( this.capabilities && this.capabilities.edit_theme_options );
};

module.exports = Site;
1 change: 1 addition & 0 deletions client/state/selectors/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ export isSendingBillingReceiptEmail from './is-sending-billing-receipt-email';
export isSharingButtonsSaveSuccessful from './is-sharing-buttons-save-successful';
export isSiteAutomatedTransfer from './is-site-automated-transfer';
export isSiteBlocked from './is-site-blocked';
export isSiteCustomizable from './is-site-customizable';
export isSiteOnFreePlan from './is-site-on-free-plan';
export isSiteSupportingImageEditor from './is-site-supporting-image-editor';
export isSiteUpgradeable from './is-site-upgradeable';
Expand Down
24 changes: 24 additions & 0 deletions client/state/selectors/is-site-customizable.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/**
* Internal dependencies
*/
import { canCurrentUser } from 'state/selectors';
import { getCurrentUserId } from 'state/current-user/selectors';
import { getRawSite } from 'state/sites/selectors';

/**
* Returns true if the site can be customized by the user, false if the
* site cannot be customized, or null if customizing ability cannot be
* determined.
*
* @param {Object} state Global state tree
* @param {Number} siteId Site ID
* @return {?Boolean} Whether site is customizable
*/
export default function isSiteCustomizable( state, siteId ) {
// Cannot determine site customizing ability if there is no current user
if ( ! getCurrentUserId( state ) || ! getRawSite( state, siteId ) ) {
return null;
}

return canCurrentUser( state, siteId, 'edit_theme_options' );
}
1 change: 0 additions & 1 deletion client/state/selectors/test/get-visible-sites.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ describe( 'getVisibleSites()', () => {
domain: 'example.com',
slug: 'example.com',
hasConflict: false,
is_customizable: false,
is_previewable: false,
options: {
default_post_format: 'standard',
Expand Down
58 changes: 58 additions & 0 deletions client/state/selectors/test/is-site-customizable.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/**
* External dependencies
*/
import { expect } from 'chai';

/**
* Internal dependencies
*/
import { isSiteCustomizable } from '../';

describe( 'isSiteCustomizable()', () => {
it( 'should return null if the capability is not set for the current user', () => {
const isCustomizable = isSiteCustomizable( {
sites: {
items: {
77203199: {
ID: 77203199,
URL: 'https://example.com'
}
}
},
currentUser: {
id: 12345678,
capabilities: {
77203199: {}
}
}
}, 77203199 );

expect( isCustomizable ).to.be.null;
} );

it( 'should return true is the corresponding user capability is true for this site', () => {
const isCustomizable = isSiteCustomizable( {
sites: {
items: {
77203199: {
ID: 77203199,
URL: 'http://example.com',
options: {
unmapped_url: 'http://example.com'
}
}
}
},
currentUser: {
id: 12345678,
capabilities: {
77203199: {
edit_theme_options: true
}
}
}
}, 77203199 );

expect( isCustomizable ).to.be.true;
} );
} );
27 changes: 24 additions & 3 deletions client/state/sites/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import { isHttps, withoutHttp, addQueryArgs, urlToSlug } from 'lib/url';
import createSelector from 'lib/create-selector';
import { fromApi as seoTitleFromApi } from 'components/seo/meta-title-editor/mappings';
import versionCompare from 'lib/version-compare';
import getComputedAttributes from 'lib/site/computed-attributes';
import { getCustomizerFocus } from 'my-sites/customize/panels';

/**
Expand Down Expand Up @@ -63,18 +62,40 @@ export const getSite = createSelector(

return {
...site,
...getComputedAttributes( site ),
...getJetpackComputedAttributes( state, siteId ),
hasConflict: isSiteConflicting( state, siteId ),
title: getSiteTitle( state, siteId ),
slug: getSiteSlug( state, siteId ),
domain: getSiteDomain( state, siteId ),
is_previewable: isSitePreviewable( state, siteId )
is_previewable: isSitePreviewable( state, siteId ),
options: computeSiteOptions( state, siteId ),
};
},
( state ) => state.sites.items
);

export function computeSiteOptions( state, siteId ) {
const site = getRawSite( state, siteId );
if ( ! site ) {
return null;
}

const isWpcomMappedDomain = getSiteOption( state, siteId, 'is_mapped_domain' ) && ! isJetpackSite( state, siteId );
const wpcomUrl = withoutHttp( getSiteOption( state, siteId, 'unmapped_url' ) );

// The 'standard' post format is saved as an option of '0'
let defaultPostFormat = getSiteOption( state, siteId, 'default_post_format' );
if ( ! defaultPostFormat || defaultPostFormat === '0' ) {
defaultPostFormat = 'standard';
}
Copy link
Member

Choose a reason for hiding this comment

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

tests like this are good reasons for normalizing data form the API on entry into Calypso. instead of scattering around the checks and converting everywhere the data is used it would be helpful if it never came into the system in the odd formats. that's a side-comment 😉


return {
...site.options,
...isWpcomMappedDomain && { wpcom_url: wpcomUrl },
default_post_format: defaultPostFormat
};
}

Copy link
Member

Choose a reason for hiding this comment

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

I see that the other new selector is in the state/selectors directory but this one is being added to the existing file. do we need to move it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about that. isSiteCustomizable() can live on its own now, it's not a computed attribute, whereas options still is. We might want to change it later but I think it makes sense to have it alongside getSite for now

Copy link
Member

Choose a reason for hiding this comment

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

well we have been adding all new selectors to the selector directory. I should probably rephrase the question: is there a good reason why we aren't adding it to the selector directory instead?

Copy link
Contributor Author

@Tug Tug Apr 18, 2017

Choose a reason for hiding this comment

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

First, I don't feel strongly on this so if you really think this is a blocker I can simply move it to state/selectors.

My logic is that all the selectors used to assemble the site in the getSite selector currently lives in that file and I'm not sure we plan to even use this selector at all or if we will always just access options through the property of the site.
Whether we keep it as a computed property or not is probably a subject for another PR since I haven't started to update usage of site.options anywhere (and I'm not sure we should).

In the case of is_customizable, I was able to update all usage to use the new selector instead and removed it from the site computed properties, so it's definitely a valid/useful selector and this is why it was moved to state/selectors.

Copy link
Member

Choose a reason for hiding this comment

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

let's see if @aduth has an opinion on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Having authored the yet-unmerged #10234, I tend to sway more on the extreme of forbidding new selectors outside state/selectors directory, regardless of any inconsistencies it might introduce (an expected and necessary pain of adopting a new recommendation).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, let's put everything new on state/selectors.

export function getJetpackComputedAttributes( state, siteId ) {
if ( ! isJetpackSite( state, siteId ) ) {
return {};
Expand Down
60 changes: 59 additions & 1 deletion client/state/sites/test/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import config from 'config';
import { useSandbox } from 'test/helpers/use-sinon';
import {
getSite,
computeSiteOptions,
getSiteCollisions,
isSiteConflicting,
isSingleUserSite,
Expand Down Expand Up @@ -104,7 +105,6 @@ describe( 'selectors', () => {
domain: 'example.com',
slug: 'example.com',
hasConflict: false,
is_customizable: false,
is_previewable: true,
options: {
default_post_format: 'standard',
Expand All @@ -114,6 +114,64 @@ describe( 'selectors', () => {
} );
} );

describe( '#computeSiteOptions()', () => {
it( 'should return null if the site is not known', () => {
const siteOptions = computeSiteOptions( {
sites: {
items: {}
}
}, 2916284 );

expect( siteOptions ).to.be.null;
} );

it( 'should return a the site options along with the computed option wpcom_url', () => {
const siteOptions = computeSiteOptions( {
sites: {
items: {
2916284: {
ID: 2916284,
URL: 'https://example.com',
options: {
unmapped_url: 'https://example.wordpress.com',
is_mapped_domain: true
}
}
}
}
}, 2916284 );

expect( siteOptions ).to.eql( {
default_post_format: 'standard',
unmapped_url: 'https://example.wordpress.com',
is_mapped_domain: true,
wpcom_url: 'example.wordpress.com'
} );
} );

it( 'should fix `default_post_format` if it is equal to \'0\'', () => {
const siteOptions = computeSiteOptions( {
sites: {
items: {
2916284: {
ID: 2916284,
URL: 'https://example.com',
options: {
default_post_format: '0',
unmapped_url: 'https://example.wordpress.com'
}
}
}
}
}, 2916284 );

expect( siteOptions ).to.eql( {
default_post_format: 'standard',
unmapped_url: 'https://example.wordpress.com'
} );
} );
} );

describe( '#getSiteCollisions', () => {
it( 'should not consider distinct URLs as conflicting', () => {
const collisions = getSiteCollisions( {
Expand Down
4 changes: 2 additions & 2 deletions client/state/ui/guided-tours/contexts.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
} from 'state/ui/selectors';
import { getLastAction } from 'state/ui/action-log/selectors';
import { getCurrentUser } from 'state/current-user/selectors';
import { canCurrentUser } from 'state/selectors';
import { canCurrentUser, isSiteCustomizable } from 'state/selectors';
import {
hasDefaultSiteTitle,
isCurrentPlanPaid,
Expand Down Expand Up @@ -122,7 +122,7 @@ export const isSelectedSitePreviewable = state =>
* @return {Boolean} True if user can run customizer, false otherwise.
*/
export const isSelectedSiteCustomizable = state =>
getSelectedSite( state ) && getSelectedSite( state ).is_customizable;
isSiteCustomizable( state, getSelectedSiteId( state ) );

/**
* Returns a selector that tests whether an A/B test is in a given variant.
Expand Down
1 change: 0 additions & 1 deletion client/state/ui/test/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ describe( 'selectors', () => {
URL: 'https://example.com',
domain: 'example.com',
hasConflict: false,
is_customizable: false,
is_previewable: false,
options: {
default_post_format: 'standard',
Expand Down