State: Migrate lib/site-roles to Redux state #9249

tyxla · 2016-11-09T16:12:31Z

This PR migrates from the legacy lib/site-roles Flux store to Redux - fixes #7921. The PR takes care of the following:

Creates Redux actions, reducer, schema, selectors and README for site roles.
Adds tests for the above.
Introduces a QuerySiteRoles query component.
Updates the RoleSelect component to use Redux and converts it to a stateless functional component.
Removes the legacy lib/site-roles Flux store.

To test:

Checkout this branch.
Use one of your public WordPress.com sites for the next steps.
Go to People, click on a user and verify the roles load properly in the Role field.
Click on the button to add a new user and verify the roles load properly in the Role field.
Verify step 3 and 4 work properly with a clean Redux state.
Verify the last role in the role select field is called Follower.
Use one of your private WordPress.com sites, go through steps 3 to 6, but this time verify the last role is called "Viewer".
Use one of your Jetpack sites and go through step 3 to 5, verify all works as expected.
Verify all tests pass: npm run test-client client/state/site-roles/

matticbot · 2016-11-09T16:12:33Z

Test live: https://calypso.live/?branch=update/redixify-site-roles

tyxla · 2016-11-10T08:42:57Z

/cc @aduth @ockham

aduth · 2016-11-11T15:17:42Z

client/lib/wpcom-undocumented/lib/site.js

@@ -118,7 +118,7 @@ UndocumentedSite.prototype.shortcodes = function( attributes, callback ) {
 };

 UndocumentedSite.prototype.getRoles = function( callback ) {
-	this.wpcom.withLocale().req.get( '/sites/' + this._id + '/roles', {}, callback );
+	return this.wpcom.withLocale().req.get( '/sites/' + this._id + '/roles', {}, callback );


If you rebase, this change will be omitted now (#9116), though merge will figure it out too in any case.

👍 true - I rebased, and it did go away. Great job with #9116 btw.

aduth · 2016-11-11T15:21:56Z

client/my-sites/people/role-select/index.jsx

+
+const RoleSelect = ( props ) => {
+	const { siteRoles, site, includeFollower, siteId, id, explanation, translate } = props;
+	const roles = siteRoles && siteRoles.slice( 0 );


Instead of Array#slice to copy the array because we use Array#push below, we could instead use Array#concat:

let { siteRoles } = props; if ( siteRoles && includeFollower ) { siteRoles = siteRoles.concat( getWPCOMFollowerRole( props ) ); }

aduth · 2016-11-11T15:23:28Z

client/my-sites/people/role-select/index.jsx

+import { getSite } from 'state/sites/selectors';
+import { getSiteRoles } from 'state/site-roles/selectors';
+
+const getWPCOMFollowerRole = ( { site, translate } ) => {


Per naming conventions on abbreviations this should probably be getWpcomFollowerRole

aduth · 2016-11-11T15:25:48Z

client/my-sites/people/role-select/index.jsx

-				return;
+	return (
+		<FormFieldset key={ siteId } disabled={ ! roles }>
+			{ siteId &&


This won't work the way you think it does. The logic is such that it'll check the truthiness of each and stop at whichever is falsey or the final value, so it'd either render null for an empty site ID or the <QuerySiteRoles /> component, but never the <QuerySites /> component.

Instead, you might want:

{ siteId && <QuerySites siteId={ siteId } /> } { siteId && <QuerySiteRoles siteId={ siteId } /> }

Or:

{ siteId && [ <QuerySites key="query-sites" siteId={ siteId } />, <QuerySiteRoles key="query-site-roles" siteId={ siteId } /> ] }

aduth · 2016-11-11T15:27:01Z

client/my-sites/people/role-select/index.jsx

-					</FormSettingExplanation>
+			<FormLabel htmlFor={ id }>
+				{ translate( 'Role', {
+					context: 'Text that is displayed in a label of a form.'


Acknowledging it was there previously, I find this to be a strange context for the string. Why do we need context in this case? Role doesn't seem specific to a form.

Makes sense - I don't think this would help the translators in any way, so removing that.

aduth · 2016-11-11T15:30:09Z

client/my-sites/people/role-select/index.jsx

+			</FormLabel>
+			<FormSelect { ...omit( props, omitProps ) }>
+				{
+					roles && roles.map( ( role ) => {


The nice thing about Lodash's map is that it would handle the empty roles for you automatically.

It's also generally faster.

I don't feel particularly strongly about changing this.

Oh, I was left with the impression that the native Array.map would be faster. Good call, thanks!

With latest changes, you don't need the siteRoles && check anymore. map will return an empty array if passed a falsey value.

Oh, I was left with the impression that the native Array.map would be faster.

Lodash "cheats" and sometimes forgoes full spec compliancy vs. native counterparts in favor of performance. In the case of array iteration ignoring sparse array checks.

See also:

Performance you-dont-need/You-Dont-Need-Lodash-Underscore#17

https://www.youtube.com/watch?v=OP8jdbcDfaA

aduth · 2016-11-11T15:32:03Z

client/state/site-roles/actions.js

+			dispatch( {
+				type: SITE_ROLES_REQUEST_FAILURE,
+				siteId,
+				error: pick( error, [ 'error', 'status', 'message' ] )


That's a lot of work to shape an object that we don't use 😄

👍 Removing this, since we don't use the error at all.

tyxla · 2016-11-11T16:13:26Z

Thanks for the review, @aduth, the PR is ready for a new one.

aduth

This works well in my testing. A few thoughts that may or may not make sense to address here vs. a subsequent pull request, but otherwise LGTM 👍

aduth · 2016-11-11T20:55:24Z

client/my-sites/people/role-select/index.jsx

+	];
+
+	if ( site && siteRoles && includeFollower ) {
+		siteRoles = siteRoles.concat( getWpcomFollowerRole( props ) );


Do you think this is something that belongs in a state selector? In other words, should getSiteRoles include the follower role automatically when the site is not private?

Sure, that would make sense. I didn't want to overcomplicate this PR, so let's keep this for another one.

aduth · 2016-11-11T20:56:49Z

client/my-sites/people/role-select/index.jsx

+			<FormLabel htmlFor={ id }>
+				{ translate( 'Role' ) }
+			</FormLabel>
+			<FormSelect { ...omit( props, omitProps ) }>


If we could figure out based on usage what's expected for this component to pass through (e.g. onChange), would be nice to avoid the omit. I see in a few cases there's an id and name passed, though I'm doubtful those are really necessary.

Good call. The list of omitProps has grown. Let's keep this for another PR.

tyxla added People Management State [Status] In Progress labels Nov 9, 2016

tyxla added this to the GM Course - Redux milestone Nov 9, 2016

tyxla self-assigned this Nov 9, 2016

mtias mentioned this pull request Nov 9, 2016

Framework: Redux single data tree and query components #5046

Closed

tyxla force-pushed the update/redixify-site-roles branch 2 times, most recently from 037c50a to 793ebe4 Compare November 10, 2016 08:22

tyxla 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 Nov 10, 2016

aduth reviewed Nov 11, 2016

View reviewed changes

tyxla force-pushed the update/redixify-site-roles branch from 793ebe4 to 09c8fd5 Compare November 11, 2016 15:36

aduth approved these changes Nov 11, 2016

View reviewed changes

aduth 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 Nov 11, 2016

tyxla added 6 commits November 14, 2016 09:26

State: Add actions, reducer, schema and selectors for site roles

aa1ebd6

State: Add tests for site roles actions, reducer and selectors

0302cbe

Components: Add QuerySiteRoles query component

3945534

People: Reduxify the RoleSelect component

cce1739

Remove lib/site-roles module

90c9f5d

Components: Clean up RoleSelect component

5e32301

tyxla force-pushed the update/redixify-site-roles branch from d331391 to 5e32301 Compare November 14, 2016 07:36

tyxla merged commit 324337f into master Nov 14, 2016

matticbot removed the [Status] Ready to Merge label Nov 14, 2016

tyxla deleted the update/redixify-site-roles branch November 14, 2016 07:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

State: Migrate lib/site-roles to Redux state #9249

State: Migrate lib/site-roles to Redux state #9249

tyxla commented Nov 9, 2016 •

edited

Loading

matticbot commented Nov 9, 2016

tyxla commented Nov 10, 2016

aduth Nov 11, 2016

tyxla Nov 11, 2016

aduth Nov 11, 2016

aduth Nov 11, 2016

aduth Nov 11, 2016

aduth Nov 11, 2016

tyxla Nov 11, 2016

aduth Nov 11, 2016

tyxla Nov 11, 2016

aduth Nov 11, 2016

aduth Nov 11, 2016

tyxla Nov 11, 2016

tyxla commented Nov 11, 2016

aduth left a comment

aduth Nov 11, 2016

tyxla Nov 14, 2016

aduth Nov 11, 2016

tyxla Nov 14, 2016

State: Migrate lib/site-roles to Redux state #9249

State: Migrate lib/site-roles to Redux state #9249

Conversation

tyxla commented Nov 9, 2016 • edited Loading

matticbot commented Nov 9, 2016

tyxla commented Nov 10, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyxla commented Nov 11, 2016

aduth 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

tyxla commented Nov 9, 2016 •

edited

Loading