Create space user's dropdown form for adding other org users #1172

rememberlenny · 2017-07-28T08:49:07Z

jcscottiii · 2017-07-28T14:31:34Z

static_src/actions/user_actions.js

 const ORG_NAME = OrgStore.cfName;
+// const SPACE_NAME = SpaceStore.cfName;


why was this commented out?

It has a linking error because it's not used.

lets delete it!

jcscottiii · 2017-07-28T14:33:11Z

static_src/components/users_parent_entity_user_selector.jsx

+  };
+}
+
+export default class UsersParentEntityUserSelector extends React.Component {


we might want to change the name of this. at first glance it's not that clear.

I totally agree. Originally it was supposed to be a module. Not it only does one thing, check org users, if user is in a space.

jcscottiii · 2017-07-28T14:41:18Z

static_src/components/users.jsx

+    if (!this.currentUserIsSpaceManager) {
+      return (
+        <PanelDocumentation>
+          ORG AND SPACE MANAGERS CAN DO THIS.


Not sure you meant to have this text here.

Hahaha. No it should be :)

jcscottiii · 2017-07-28T14:43:39Z

static_src/components/users_parent_entity_user_selector.jsx

+    const roles = 'space_auditor';
+    if (values.userGuid) {
+      const userGuid = values.userGuid.value;
+      userActions.addUserRoles(roles, apiKey, userGuid, entityGuid, entityType);


i think in order to prevent confusion, we should only show org users currently without space roles. then here, upon successful submission, we should remove the user from the list so we don't see them here.

I was working on last night, but didn't know quite how the EventEmitter on the UserStore worked. I need to dig in a bit.

Basically, the logic to do this works.

jcscottiii · 2017-07-28T14:47:47Z

static_src/components/users_parent_entity_user_selector.jsx

+    }
+  }
+
+  get errorMessage() {


test for this

It looks like this is copied from the UserInvite component, seems like a good time to extract a out module or higher-order component.

jcscottiii · 2017-07-28T14:47:54Z

static_src/components/users_parent_entity_user_selector.jsx

+    return message;
+  }
+
+  get invitationMessage() {


test for this

See above ^^

jcscottiii · 2017-07-28T14:48:04Z

static_src/components/users_parent_entity_user_selector.jsx

+      ` to this ${currentEntity}.`;
+  }
+
+  get userSelector() {


test for this

jcscottiii

some feedback

el-mapache

Looks good thus far! The two things I noticed most were:

new component name is fairly verbose and maybe not as clear as it could be
The new component borrows a lot from the UsersInvite component. There is probably a way to extract the common functionality and share it between the two components

el-mapache · 2017-07-28T14:56:54Z

static_src/components/users_parent_entity_user_selector.jsx

+export default class UsersParentEntityUserSelector extends React.Component {
+  constructor(props) {
+    super(props);
+    FormStore.create(USERS_PARENT_ENTITY_USER_FORM_GUID);


Creating a form here could produce unwanted side effects e.g. if this component subscribed to the FormStore's change event. It should be called in componentDidMount

el-mapache · 2017-07-28T14:57:57Z

static_src/components/users_parent_entity_user_selector.jsx

+  _onSubmitForm(errs, values) {
+    const entityType = this.state.currentEntity;
+    const entityGuid = this.state.currentEntityGuid;
+    const apiKey = 'auditors';


Lets make these constants

el-mapache · 2017-07-28T14:59:42Z

static_src/components/users_parent_entity_user_selector.jsx

+    }
+  }
+
+  get errorMessage() {


It looks like this is copied from the UserInvite component, seems like a good time to extract a out module or higher-order component.

el-mapache · 2017-07-28T14:59:55Z

static_src/components/users_parent_entity_user_selector.jsx

+    return message;
+  }
+
+  get invitationMessage() {


See above ^^

el-mapache · 2017-07-28T15:04:15Z

static_src/test/unit/stores/user_store.spec.js

@@ -682,6 +682,19 @@ describe('UserStore', function () {
    });
  });

+  describe('getAllInOrgAndNotSpace()', function() {
+    it('should find all users that are in an org, without the space users', function() {


I am probably misreading this spec, but from the description, shouldn't the testUser be excluded here? They don't have a space_roles key on their object, they do have a space role in their roles.

Should the corresponding method in UserStore also check for the existence of space roles within the roles object?

msecret · 2017-07-28T19:31:18Z

static_src/components/users_parent_entity_user_selector.jsx

+
+
+function stateSetter(props) {
+  return {


Why is state needed in this component if everything is passed down from props? https://github.com/18F/cg-dashboard/blob/master/CONTRIBUTING.md#components

Its not. This is a great document. Thanks.

…n-space-org-users

el-mapache · 2017-07-31T13:38:39Z

static_src/components/users_org_user_selector.jsx

+  _onSubmitForm(errs, values) {
+    const { currentEntity } = this.props;
+    const { currentEntityGuid } = this.props;
+    const apiKey = AUDITOR_NAME;


Should this be renamed to endpoint? When I read api key, I think of an access key, or a secret. I know you didn't introduce this pattern, but maybe we can start renaming now!

i like the idea of renaming it! let's move it to resource (i think of endpoint as being the full path).

(coming from me, the person who introduced the pattern lol)

there's another refactor while we are talking about it, (but outside the scope of this PR)
addUserRoles currently only takes one role but it and the argument are plural roles.

This needs to be made singular. Also, apiKey / resource/ endpoint are the actual role in the uri. it could be confusing between whichever variable name is used vs the roles which is a map to the roles used in the frontend for the check boxes.

would like to rename roles but not sure to what.

jcscottiii · 2017-07-31T20:01:46Z

static_src/components/users.jsx

+    }
+
+    return (
+      <OrgUsersSelector


this component has a lot of attributes. for a lot of attributes, you can make this generic like UserSelector.
For OrgUserSelector, you probably wouldn't need all this. I'm leaning to keeping all of these and just rename to UserSelector and we can refactor for more generic cases later on.

jcscottiii · 2017-08-01T14:59:13Z

static_src/actions/user_actions.js

@@ -146,7 +148,7 @@ const userActions = {
    });
  },

-  addUserRoles(roles, apiKey, userGuid, entityGuid, entityType) {
+  addUserRoles(resource_role_name, user_role, userGuid, entityGuid, entityType) {


ahh sorry for the confusing conversation.
we were saying apiKey -> resource
and you can revert resource_role_name to roles and leave roles as-is for now.
we will address the naming when we address addUserRoles and addedUserRoles and the mapping to the frontend roles

We should stick to camel case as well

jcscottiii · 2017-08-01T15:18:12Z

static_src/components/users_selector.jsx

+const USERS_PARENT_ENTITY_USER_FORM_GUID = 'users-parent-entity-users-form';
+
+const propTypes = {
+  orgUsersSelectorDisabled: PropTypes.bool,


this can be renamed to selectorDisabled or usersSelectorDisabled

jcscottiii

https://github.com/18F/cg-dashboard/pull/1172/files/38ed848f8e82f42a98ffb69df16e4a4b2a247d44#r130637901 and
https://github.com/18F/cg-dashboard/pull/1172/files/38ed848f8e82f42a98ffb69df16e4a4b2a247d44#r130632410 were not addressed

fureigh · 2017-08-01T17:45:19Z

@rememberlenny: Is the class in line 78 of https://github.com/18F/cg-dashboard/blob/339846c962783db8afa63f054be202af00b1a0e0/static_src/components/users_selector.jsx meant to be "test-users"? If so, LGTM.

rememberlenny · 2017-08-01T17:51:18Z

@fureigh It was a copy over. Ill change for specificity.

rememberlenny · 2017-08-01T18:29:21Z

@fureigh Its a react syntax. If you take a look at the other components, you will see that they all have a test- prefix class. This allows us to target the elements in a test.

rememberlenny added 12 commits July 28, 2017 03:55

Add new constants to check for user role by username

e56bc8c

Create ability to get all users in org that aren't in space

4ac8ee1

Create functions used to check space org role status

503e7f4

Create the component to manage the org parent user selector

0bba2f7

Add the selector to the user component

931eef7

Get the selector based user adding working

9fa5cbc

Add a component test

2461e0c

Remove unused store cases

651e522

Create check again space users

9c2815e

Take care of linting

fa73119

Lint tests

04a8972

Fix linting

1d3f87f

jcscottiii reviewed Jul 28, 2017

View reviewed changes

jcscottiii suggested changes Jul 28, 2017

View reviewed changes

jcscottiii requested review from fureigh and el-mapache July 28, 2017 14:49

el-mapache suggested changes Jul 28, 2017

View reviewed changes

rememberlenny added 3 commits July 28, 2017 12:06

Remove unused comment

55defcd

Rename component to be easily grokable

33a9778

Pull out constants

4947863

msecret reviewed Jul 28, 2017

View reviewed changes

rememberlenny added 3 commits July 28, 2017 17:07

Rework file names and component reference

42cb434

Get the error working on the component

a94ab91

Get the working selector check

06ad595

rememberlenny added 2 commits July 28, 2017 18:12

Merge branch 'master' of github.com:18F/cg-dashboard into lkb-dropdow…

02106fb

…n-space-org-users

Save the changes on the user store check

b98bf11

rememberlenny force-pushed the lkb-dropdown-space-org-users branch from 01d461d to b98bf11 Compare July 28, 2017 22:15

Fix linting

a7813cf

el-mapache reviewed Jul 31, 2017

View reviewed changes

jcscottiii reviewed Jul 31, 2017

View reviewed changes

rememberlenny force-pushed the lkb-dropdown-space-org-users branch 4 times, most recently from 93793bc to 0a2aa6d Compare August 1, 2017 14:33

Add the selector detection

80ca7d6

rememberlenny force-pushed the lkb-dropdown-space-org-users branch from 0a2aa6d to 80ca7d6 Compare August 1, 2017 14:35

rememberlenny added 2 commits August 1, 2017 10:38

Rename the component

fe47005

Change the function argument names

38ed848

jcscottiii reviewed Aug 1, 2017

View reviewed changes

rememberlenny added 2 commits August 1, 2017 11:55

Rename usersSelectorDisabled

ff12ede

Linting

5d8b81d

jcscottiii suggested changes Aug 1, 2017

View reviewed changes

rememberlenny added 2 commits August 1, 2017 12:33

Re-update user action names

57ac1d0

Fix new var name on params around addUserRoles

339846c

jcscottiii approved these changes Aug 1, 2017

View reviewed changes

el-mapache approved these changes Aug 1, 2017

View reviewed changes

Rename classes for specificity

f655f6e

rememberlenny merged commit 4317913 into master Aug 1, 2017

rememberlenny deleted the lkb-dropdown-space-org-users branch August 1, 2017 18:29

		const ORG_NAME = OrgStore.cfName;
		// const SPACE_NAME = SpaceStore.cfName;

Create space user's dropdown form for adding other org users #1172

Create space user's dropdown form for adding other org users #1172

Conversation

rememberlenny commented Jul 28, 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

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

jcscottiii left a comment

Choose a reason for hiding this comment

el-mapache 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

jcscottiii Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

jcscottiii Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcscottiii Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcscottiii left a comment

Choose a reason for hiding this comment

fureigh commented Aug 1, 2017

rememberlenny commented Aug 1, 2017

rememberlenny commented Aug 1, 2017

jcscottiii Jul 31, 2017 •

edited

Loading

jcscottiii Jul 31, 2017 •

edited

Loading

jcscottiii Aug 1, 2017 •

edited

Loading