[Remove SitesList] Add isSiteCustomizable selector #13160

Tug · 2017-04-17T23:11:18Z

This PR creates the isSiteCustomizable() selector and update the getSite selector to use it instead of using getComputedAttribute. It also updates usages of isCustomizable() in components so we can remove Sites#isCustomizable() as well as the computed attribute site.is_customizable.

Testing Instructions

Checkout the branch locally
Run the tests (make test)
Run the app and check that you don't have access to Themes in the left sidebar when you don't have the permissions.

matticbot · 2017-04-17T23:11:19Z

Test live: https://calypso.live/?branch=update/site-is-customizable

dmsnell · 2017-04-18T05:24:44Z

client/state/sites/selectors.js

@@ -14,7 +14,7 @@ import {
 	some,
 	split,
 	includes,
-	startsWith,
+	startsWith


is this unintentional? seems like an unnecessary change…

dmsnell · 2017-04-18T05:26:29Z

client/state/sites/selectors.js

+	let defaultPostFormat = getSiteOption( state, siteId, 'default_post_format' );
+	if ( ! defaultPostFormat || defaultPostFormat === '0' ) {
+		defaultPostFormat = 'standard';
+	}


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 😉

dmsnell · 2017-04-18T05:34:38Z

client/state/sites/selectors.js

+		default_post_format: defaultPostFormat
+	};
+}
+


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?

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

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?

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.

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

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).

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

mtias · 2017-04-18T18:45:37Z

client/state/sites/selectors.js

 		};
 	},
 	( state ) => state.sites.items
 );

+export function getSiteOptions( state, siteId ) {


Mmm, I actually don't think we should make this an exportable selector. It should only be used in getSite. Also probably the name of the function should be changed to reflect we are computing values and not just returning site.options.

gwwar · 2017-04-18T20:32:41Z

Changes here look pretty reasonable 👍 Looks like this needs a rebase, ping me when it's ready and I can help smoke test.

matticbot · 2017-04-19T07:27:43Z

@Tug This PR needs a rebase

…gainst incomplete states

… not a selector

Tug · 2017-04-21T10:35:01Z

Rebased and renamed getSiteOptions() into computeSiteOptions().

gziolo

LGTM 👍 I haven't tested it, because I'm not sure when I shouldn't have access to themes.

Tug · 2017-04-21T11:37:35Z

I think you don't have access when you have any other role than "Administrator" on a site.

…omizable" This reverts commit d180acc, reversing changes made to 78d8f29.

) * WIP Add QuerySites in Layout and shift from sitesFactory to Redux sites * WIP more seek & destroy sites-list (Navigation, Picker, Sidebar) * Tackle SitesList in my-sites/controller * Use post-rebase selector areAllSitesSingleUser * Selectors: getSite: support non-ID-based retrieval In the process of removing SitesList, some instances are found of: sites.getSite( siteFragment ) where `siteFragment` is of mixed format. See lib/route/test. * my-sites: controller: SITES_ONCE_CHANGED; no more sites.once * Post-rebase fix * Update CurrentSite and AllSites to use getSites. * ES6ify SiteSelector * AllSites: Use ES6-style defaultProps * getSite: don't return false * CurrentSite: Use ES6-style state initializer * SiteSelector: Fix passing of `this` to `map` Lodash's `map` doesn't support passing of `this` (anymore?). Fixes TypeError. * Appease linter * Selectors: canManageSelectedOrAnySite, getSelectedOrAllSites * Use getSelectedOrAllSites selector in current site block. * Fix exports. * Fix switch sites. * Add isRequestingMissingSites selector. * Resetting attributes creates a circular call with getRawSite That means we end up passing a slug to getRawSite, which fails, meaning computted attributes are not set. * getSites: wrap with createSelector * Add siteSearch HoC to replace sites.search in SiteSelector * Remove sites-list from sites dropdown. * Use getPrimarySiteId() selector * CurrentSite: connect to redux; bind selector :( * siteSelection: ensure siteID is numeric before firing setting actions Adds `getSiteId` selector. * Sites: use connect to provide selected site * [DELETE AFTER 13121] PostsNavigation: connect to Redux * rm selector canManageSelectedOrAnySite * Revert "Merge pull request #13160 from Automattic/update/site-is-customizable" This reverts commit d180acc, reversing changes made to 78d8f29. * getSite: re-remove cyclical selector calls * Stats: Controllers: Don't depend on sites-list * Import was kept, as `sites` is passed as props in one controller. To be fixed in a separate PR. * SitesDropdown: "fix" tests * Avoid duplicate selector call, use `get` * state/lib/middleware: better naming, comments * siteSelection: fix redirs, wait for selected site's data * Keep SitesList's `selected` in sync with Redux - :-( - Aims to fix regressions found, notably around flows in `/domains` * siteSelect: immediately redirectToPrimary if data ready This mimics the behavior that used to be in place with `sites.initialized`. Without it, render jumps may happen as we default to a site-less view then correct to a site-specific one. * NavItem: fix propTypes (`count`) Component logic expects non-number `count`, e.g. when no site is selected. This eliminates React type warnings. * Signup: fetchSitesAndUser: wait & load new site into Redux store One of the async jobs of fetchSitesAndUser is to trigger a `fetch` on `lib/sites-list`, wait for changes to that structure, check for the local availability of data for the new site, then either continue or retrigger a fetch. This commit ports that behavior to our Redux data layer. * Invites: Pull sites from server upon accepting Before #13094, this behavior was ensured by the `acceptInvite` action creator, which dispatches old Flux actions that `lib/sites-list` listens to in order to call a `sync` on itself.

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

Tug self-assigned this Apr 17, 2017

Tug added Framework [Status] In Progress labels Apr 17, 2017

Tug force-pushed the update/site-is-customizable branch from b3b731f to 1bac497 Compare April 17, 2017 23:16

Tug mentioned this pull request Apr 17, 2017

[Remove SitesList] Move lib/site/computed-attributes to appropriate state folder #11310

Closed

2 tasks

Tug added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Apr 17, 2017

Tug requested review from gwwar and dmsnell April 17, 2017 23:29

dmsnell reviewed Apr 18, 2017

View reviewed changes

Tug force-pushed the update/site-is-customizable branch from 1bac497 to 08da6a7 Compare April 18, 2017 07:43

mtias reviewed Apr 18, 2017

View reviewed changes

matticbot added the [Status] Needs Rebase label Apr 19, 2017

Tug added 9 commits April 21, 2017 12:19

Move site computed attributes to site/selectors

b383007

Remove isCustomizable() function from Site

d86591f

Create isSiteCustomizable selector and harden getComputedAttributes a…

19debab

…gainst incomplete states

Remove duplicate computed attributes

accd0a8

Add tests for getSiteOptions

dc4e889

Add tests for isSiteCustomizable

4933c75

Update is_customizable prop in getVisibleSites tests

2f66f23

Move isSiteCustomizable to state/selectors

aa56e2a

Remove is_customizable attribute from site

4489fdf

Rename getSiteOptions to computeSiteOptions to make it clear that its…

158c8f0

… not a selector

Tug force-pushed the update/site-is-customizable branch from dc57fe6 to 158c8f0 Compare April 21, 2017 10:25

Tug changed the title ~~[Remove SitesList] Add isSiteCustomizable and getSiteOptions selectors~~ [Remove SitesList] Add isSiteCustomizable selector Apr 21, 2017

gziolo added [Status] Needs Author Reply [Status] Ready to Merge and removed [Status] Needs Rebase [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs Author Reply labels Apr 21, 2017

gziolo approved these changes Apr 21, 2017

View reviewed changes

Tug merged commit d180acc into master Apr 21, 2017

Tug deleted the update/site-is-customizable branch April 21, 2017 11:39

matticbot removed the [Status] Ready to Merge label Apr 21, 2017

mcsf added a commit that referenced this pull request Apr 26, 2017

Revert "Merge pull request #13160 from Automattic/update/site-is-cust…

d8434b5

…omizable" This reverts commit d180acc, reversing changes made to 78d8f29.

mcsf added a commit that referenced this pull request May 2, 2017

Revert "Merge pull request #13160 from Automattic/update/site-is-cust…

d8b59ce

…omizable" This reverts commit d180acc, reversing changes made to 78d8f29.

mcsf added a commit that referenced this pull request May 8, 2017

Revert "Merge pull request #13160 from Automattic/update/site-is-cust…

4ba38a1

…omizable" This reverts commit d180acc, reversing changes made to 78d8f29.

mcsf added a commit that referenced this pull request May 9, 2017

Revert "Merge pull request #13160 from Automattic/update/site-is-cust…

0067812

…omizable" This reverts commit d180acc, reversing changes made to 78d8f29.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Remove SitesList] Add isSiteCustomizable selector #13160

[Remove SitesList] Add isSiteCustomizable selector #13160

Tug commented Apr 17, 2017 •

edited

Loading

matticbot commented Apr 17, 2017

dmsnell Apr 18, 2017

dmsnell Apr 18, 2017

dmsnell Apr 18, 2017

Tug Apr 18, 2017

dmsnell Apr 18, 2017

Tug Apr 18, 2017 •

edited

Loading

dmsnell Apr 18, 2017

aduth Apr 18, 2017

mtias Apr 18, 2017

mtias Apr 18, 2017

gwwar commented Apr 18, 2017

matticbot commented Apr 19, 2017

Tug commented Apr 21, 2017 •

edited

Loading

gziolo left a comment

Tug commented Apr 21, 2017

[Remove SitesList] Add isSiteCustomizable selector #13160

[Remove SitesList] Add isSiteCustomizable selector #13160

Conversation

Tug commented Apr 17, 2017 • edited Loading

Testing Instructions

matticbot commented Apr 17, 2017

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

Tug Apr 18, 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

gwwar commented Apr 18, 2017

matticbot commented Apr 19, 2017

Tug commented Apr 21, 2017 • edited Loading

gziolo left a comment

Choose a reason for hiding this comment

Tug commented Apr 21, 2017

Tug commented Apr 17, 2017 •

edited

Loading

Tug Apr 18, 2017 •

edited

Loading

Tug commented Apr 21, 2017 •

edited

Loading