Invite users to spaces #1162

el-mapache · 2017-07-19T16:42:40Z

Org managers can now associate users with spaces.

Screenshot:

In the case of a space manager who is not an org manager, they are directed to use the cloud foundry api (in the case of associating an existing user) or to ask their org manager to do it for them.

Screenshot:

jcscottiii · 2017-07-19T17:49:41Z

i'm surprised you have to associate the user to the org first. But you're right!
(i thought they were isolated enough.)
I ran this and it doesn't work unless you first associate the user first

# via docker run -it governmentpaas/cf-cli /bin/sh

  cf login -a https://api.local.pcfdev.io --skip-ssl-validation -u admin -p admin -o system && \
  cf create-user org-manager org-managerpw && \
  cf create-user space-manager space-managerpw && \
  cf create-org sample-org && \
  cf create-space sample-space -o sample-org && \
  cf set-org-role org-manager sample-org OrgManager && \
  cf set-space-role space-manager sample-org sample-space SpaceManager && \
  cf create-user org-manager org-managerpw && \
  cf create-user test-user test-pw && \
  export TEST_USER_GUID=$(cf curl /v2/users | jq -r '.resources[] | select(.entity.username == "test-user") | .metadata.guid') && \
  cf login -a https://api.local.pcfdev.io --skip-ssl-validation -u org-manager -p org-managerpw && \
  export ORG_GUID=$(cf org sample-org --guid) && \
  export SPACE_GUID=$(cf space sample-space --guid) && \
  cf curl "/v2/spaces/$SPACE_GUID/auditors/$TEST_USER_GUID" -X PUT

But if it do a curl put beforehand to the org users, it works

# via docker run -it governmentpaas/cf-cli /bin/sh

  cf login -a https://api.local.pcfdev.io --skip-ssl-validation -u admin -p admin -o system && \
  cf create-user org-manager org-managerpw && \
  cf create-user space-manager space-managerpw && \
  cf create-org sample-org && \
  cf create-space sample-space -o sample-org && \
  cf set-org-role org-manager sample-org OrgManager && \
  cf set-space-role space-manager sample-org sample-space SpaceManager && \
  cf create-user org-manager org-managerpw && \
  cf create-user test-user test-pw && \
  export TEST_USER_GUID=$(cf curl /v2/users | jq -r '.resources[] | select(.entity.username == "test-user") | .metadata.guid') && \
  cf login -a https://api.local.pcfdev.io --skip-ssl-validation -u org-manager -p org-managerpw && \
  export ORG_GUID=$(cf org sample-org --guid) && \
  export SPACE_GUID=$(cf space sample-space --guid) && \
  cf curl "/v2/organizations/$ORG_GUID/users/$TEST_USER_GUID" -X PUT && \ 
  cf curl "/v2/spaces/$SPACE_GUID/auditors/$TEST_USER_GUID" -X PUT

However, there's another problem. If I log in as the space-manager instead. It won't work because the space-manager can't associate someone to the org.

There might be a separate UI for the space-managers to only pick people who are already associated to the org to choose from.

el-mapache · 2017-07-19T18:28:15Z

static_src/components/users_invite.jsx

    return message;
  }

+  get entityType() {
+    return /space/.test(this.props.inviteEntityType) ? 'space' : 'organization';


I think, in a future PR, we will want to have some kind of service class or utility code that takes the raw name of the entity and gives us the human-friendly version.

el-mapache · 2017-07-19T18:28:45Z

@jcscottiii So given that, will we need to figure out what that UI looks like before this PR will be valuable?

jcscottiii · 2017-07-19T19:01:10Z

@el-mapache in the meantime, you can do a check and show error.
There's four cases.
1
Given I'm a space manager and not the org manager, if I try to invite a user to my space who currently does not exist in the org, show an error asking for an org manager to first invite them to the org.

2
Given I'm a space manager and not the org manager, if i try to invite a user to my space who current does exist in my org, the user is added to my space.

3
Given I'm the org manager (and not the space manager, even though it doesn't matter), if i try to invite a user to a space who current does exist in my org, the user is added to my space.

4
Given I'm the org manager (and not the space manager, even though it doesn't matter), if i try to invite a user to a space who current does not exist in my org, the user is added to my space and my org.

el-mapache · 2017-07-19T21:06:47Z

@jcscottiii Just so I am totally clear: we want to prevent the invite from ever hitting the UAA API, correct?

rememberlenny · 2017-07-20T15:02:01Z

Jumping on this

jcscottiii · 2017-07-24T16:24:10Z

static_src/components/users.jsx

+    const { currentUser } = this.state;
+    const { currentOrgGuid } = OrgStore;
+
+    // TODO this is not 100% robust.


why isn't it robust?

left this in by mistake!

jcscottiii · 2017-07-24T16:31:52Z

static_src/actions/user_actions.js

-      entityUsers = cfApi.fetchSpaceUserRoles(entityGuid);
-    }
-    return Promise.resolve(entityUsers);
+    return cfApi[(entityType === ORG_NAME) ? 'fetchOrgUsers' : 'fetchSpaceUserRoles'](entityGuid);


while this makes the code more concise, i wonder if this takes away from the clarify and future expansion of the code (stuff to do in the branches)

If we were doing more things in the branch, I would advocate to move the code into separate methods to maintain readability. I think this is ok as is, for now.

jcscottiii · 2017-07-24T16:33:28Z

static_src/components/user_list.jsx

@@ -134,8 +134,6 @@ export default class UserList extends React.Component {

    if (this.props.empty) {
      content = this.emptyState;
-    } else if (this.props.users.length === 1) {


why did this get removed?

I didn't think the onlyOneState makes sense anymore. If the user is an org manager, we will want to show the invite component, and if they are a space manager, we want to show the message telling them to ask their org manager to invite users.

@jcscottiii Maybe we need to show it regardless?

leave it in for now. we can revisit this when we develop the UI for the space managers to move exiting org users into the space.

if it's a hassle to maintain that separate onlyOneState state, let's talk about it then though.

jcscottiii · 2017-07-24T16:37:19Z

static_src/components/users.jsx

+    );
+  }
+
+  get userInvite() {


(at first glance, i could be wrong), will this come up on the org page for a non admin, non org manager user? (this text is catered to the space page)

Not you are correct! I will need to add the entityType interpolation we will in the UserInvite component. I might just move this to a separate component so there isn't a ton of logic in the method.

jcscottiii · 2017-07-24T16:38:53Z

static_src/components/users_invite.jsx

    return message;
  }

+  get entityType() {
+    return this.isOrganization ? 'organization' : 'space';


const values for organization and space

jcscottiii · 2017-07-24T16:43:22Z

static_src/components/users_invite.jsx

+  }
+
+  get isOrganization() {
+    return /org/.test(this.props.inviteEntityType);


is there a comparison we can do without a regex?

i only ask because, we should know for sure what values inviteEntityType could be

We could compare against the exact string that inviteEntityType is. I didn't use a const because we don't currently (I dont think?) have a consistent way that we are handling those in the app.

I'll just add the constant for now, I'll make a ticket to go through an audit our use of strings/constants, and where we put the constants e.g. all in the constants.js file, or in additional domain specific files, etc.

I'll make a ticket to go through an audit our use of strings/constants, and where we put the constants e.g. all in the constants.js file

That's a great idea!

el-mapache · 2017-07-24T19:41:03Z

@jcscottiii Updated based on your feedback + rebased with master

jcscottiii · 2017-07-24T20:35:11Z

@el-mapache whoops you were right about that one comment. i meant to say if you think users[0] makes it more clear that you need that element.

el-mapache · 2017-07-24T20:40:01Z

@jcscottiii Ahh ok. I think maybe calling it users is misleading, we are filtering all the users down to just the current user. What do you think about grabbing the first element immediately and just checking if (!user)? I think that is probably clearer.

jcscottiii · 2017-07-24T20:44:59Z

static_src/test/unit/components/users.spec.jsx

-    describe('when at org level', function () {
-      beforeEach(function () {
+    describe('when at org level', () => {
+      it('doesnt have permissions to edit users', () => {


i know this was here before, but let's change this statement (or change the test, but probably the former). it doesn't test the permissions of the users.

jcscottiii · 2017-07-24T20:45:10Z

static_src/test/unit/components/users.spec.jsx


-      it('doesnt have permissions to edit users', function () {
+    describe('when at space level', () => {
+      it('doesnt have permissions to edit users', () => {


same as https://github.com/18F/cg-dashboard/pull/1162/files#r129148710

jcscottiii · 2017-07-24T20:49:45Z

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

@@ -134,22 +135,25 @@ describe('UserStore', function () {
      expectedUsers = [
        {
          guid: userGuidA,
-          roles: { [spaceGuid]: ['space_developer'] }
+          roles: { [spaceGuid]: ['space_developer'] },
+          space_roles: [ 'space_developer' ]


is there a reason we need this new attribute space_roles when we already have a mapping between the space guid and the roles that person has for the guid?

Its not a new attribute, its an old one. space_roles is returned when we fetch the user from the API. We do a transformation that adds the roles key, but in order to pass a deep equality test, the object needs to retain the space_roles key.

We could edit the code to the delete space_roles from the underlying object, but I don't think it hurts to have the extra field.

For context, the test originally passed without it because the code wasn't set up to handle the case of a new user being created from the spaces view. In the old code, we would get the new user object from the database, and then overwrite it with a new object that just contained a guid and roles.

ahh didn't know it was already there. in that case, we can leave it. i think we need to revisit the whole topic of the post-processing of data (like this transformation). but another day!

jcscottiii · 2017-07-24T20:52:01Z

sorry i didn't go through everything last time. just finished now. left one main question.

jcscottiii · 2017-07-24T20:52:45Z

@el-mapache re: #1162 (comment) i like that much better

el-mapache · 2017-07-24T21:10:31Z

@jcscottiii alright, updated based on current round of feedback!

jcscottiii · 2017-07-24T21:13:49Z

once the tests pass

Pass in entitytype to UserInvite component Fix lint errors, only show invites when org manager is current user Fixup unit tests Add specs, remove unused sandbox refs * Use <PanelDocumentation /> component for invite fallback message Store users before merging roles Linting an spec fixes Address PR feedback

jcscottiii · 2017-07-25T13:07:48Z

@el-mapache thanks for squashing the commits!

rememberlenny · 2017-07-25T14:04:17Z

Thanks for this! It looks great!

el-mapache force-pushed the ab-lb-user-space-associate branch from 049039f to ec3098a Compare July 19, 2017 17:57

el-mapache requested review from jcscottiii and fureigh July 19, 2017 18:23

el-mapache commented Jul 19, 2017

View reviewed changes

el-mapache changed the title ~~AB+LB: Spike code to associate user w/ spaces~~ Invite users to spaces Jul 19, 2017

el-mapache force-pushed the ab-lb-user-space-associate branch 4 times, most recently from 5ebac53 to 1f4aea0 Compare July 24, 2017 15:46

el-mapache self-assigned this Jul 24, 2017

el-mapache added the review-ready label Jul 24, 2017

el-mapache force-pushed the ab-lb-user-space-associate branch from 1f4aea0 to cd6cf88 Compare July 24, 2017 15:53

jcscottiii reviewed Jul 24, 2017

View reviewed changes

el-mapache force-pushed the ab-lb-user-space-associate branch from cd6cf88 to c6be7ac Compare July 24, 2017 19:40

cloud-gov deleted a comment from el-mapache Jul 24, 2017

jcscottiii reviewed Jul 24, 2017

View reviewed changes

jcscottiii approved these changes Jul 24, 2017

View reviewed changes

el-mapache force-pushed the ab-lb-user-space-associate branch from b7e5de7 to f6f1abd Compare July 24, 2017 23:36

jcscottiii merged commit a0bec59 into master Jul 25, 2017

rememberlenny deleted the ab-lb-user-space-associate branch July 25, 2017 14:04

Invite users to spaces #1162

Invite users to spaces #1162

Conversation

el-mapache commented Jul 19, 2017 • edited Loading

jcscottiii commented Jul 19, 2017

Choose a reason for hiding this comment

el-mapache commented Jul 19, 2017

jcscottiii commented Jul 19, 2017 • edited Loading

el-mapache commented Jul 19, 2017

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

el-mapache Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

el-mapache Jul 24, 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

el-mapache commented Jul 24, 2017 • edited Loading

jcscottiii commented Jul 24, 2017

el-mapache commented Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

el-mapache Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcscottiii commented Jul 24, 2017

jcscottiii commented Jul 24, 2017

el-mapache commented Jul 24, 2017

jcscottiii commented Jul 24, 2017

jcscottiii commented Jul 25, 2017

rememberlenny commented Jul 25, 2017

el-mapache commented Jul 19, 2017 •

edited

Loading

jcscottiii commented Jul 19, 2017 •

edited

Loading

el-mapache Jul 24, 2017 •

edited

Loading

el-mapache Jul 24, 2017 •

edited

Loading

el-mapache commented Jul 24, 2017 •

edited

Loading

el-mapache commented Jul 24, 2017 •

edited

Loading

el-mapache Jul 24, 2017 •

edited

Loading