Skip to content
This repository has been archived by the owner on May 19, 2020. It is now read-only.

fix space user from automatically being populated upon invite #1194

Merged
merged 7 commits into from
Aug 14, 2017

Conversation

jcscottiii
Copy link
Contributor

in prod, if you try to invite a user to the space by e-mail, they will
not be populated in the user list automatically. you need to refresh to
see them.

James C. Scott added 2 commits August 11, 2017 15:08
in prod, if you try to invite a user to the space by e-mail, they will
not be populated in the user list automatically. you need to refresh to
see them.
Copy link
Contributor

@el-mapache el-mapache left a comment

Choose a reason for hiding this comment

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

Thanks for getting this working again!

I just have a few additional comments:

  • on line 42 of user_actions we have a method fetchSpaceUserRoles. This isn't called anywhere in the application and should probably be removed. @jcscottiii pointed out that this is called, rescinding.

  • This PR adds some new methods to associate users to spaces, but what about the existing store code that handles the SPACE_USER_ROLES_RECEIVED action type? Do we need both ways to associate the user to the space?

  • The comment on line 67 in user_actions doesn't apply anymore, since the method call it was commenting has been removed.

USER_ORG_ASSOCIATED: null,
// Action to trigger email sent to user with cloud.gov invite url
// and associate user to space.
USER_SPACE_ASSOCIATE: null,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment is slightly misleading, since this action type doesn't trigger the email invite as a result of it being dispatched.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right. i just copied the one above. will change both of them.

@jcscottiii
Copy link
Contributor Author

userAction. fetchSpaceUserRoles is called in routes.js @el-mapache

@el-mapache
Copy link
Contributor

el-mapache commented Aug 14, 2017

I think we should still be able to combine the calls somehow? fetchSpaceUserRoles doesn't actually get roles, it gets all the users in a given space. You can ignore this too, I thought that this call and the action I commented on above were related, they aren't.

@jcscottiii
Copy link
Contributor Author

that one call gets both the users and their roles. userAction.fetchSpaceUserRoles calls cfApi.fetchSpaceUserRoles (by listing the roles, you get all the users as well. as a side note: we should probably do this one call for the org stuff, currently it gets the org users in one call then the roles in another call, then merges)

image

https://apidocs.cloudfoundry.org/268/spaces/retrieving_the_roles_of_all_users_in_the_space.html

@el-mapache
Copy link
Contributor

el-mapache commented Aug 14, 2017

Right, I understand that, I guess I was just saying that the function is not well named. The spaces API specifically doesn't let you get all the users (without their roles, like the orgs API), it only has a function to get the users and their roles within the space. This isn't necessarily calling out this code specifically, the method is called the same thing in the cloud foundry api and it seems confusing.

@el-mapache
Copy link
Contributor

Like you said, Orgs also exposes the same call to get users and their roles for orgs, so we should for sure use that going forward.

@jcscottiii
Copy link
Contributor Author

re: #1194 (comment)
maybe rename the userAction to fetchSpaceUsersAndRoles?

This PR adds some new methods to associate users to spaces, but what about the existing store code that handles the SPACE_USER_ROLES_RECEIVED action type? Do we need both ways to associate the user to the space?

We need both. The associated action toggles _inviteInputActive (even though it's not being used which is another story for another PR). These types of indication of states depending on if you're fetching or inviting someone new are exclusive (when we use them). BUT the merge logic that they both use, that could be consolidated to a new shared function.

@jcscottiii
Copy link
Contributor Author

@el-mapache i don't see a comment there?

The comment on line 67 in user_actions doesn't apply anymore, since the method call it was commenting has been removed.

@el-mapache
Copy link
Contributor

@jcscottiii Whoops! I typed user_actions but it should be user_store

@el-mapache
Copy link
Contributor

@jcscottiii So you are saying that we were previously overloading SPACE_USER_ROLES_RECEIVED and we should have had a discrete action type just for associating users to spaces all along? I can get behind that.

@jcscottiii
Copy link
Contributor Author

@el-mapache should be ready to go!

Copy link
Contributor

@el-mapache el-mapache left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for walking me through the changes!

@el-mapache el-mapache merged commit 36c92b2 into master Aug 14, 2017
@el-mapache el-mapache deleted the js-fix-invite-space-user-list branch August 14, 2017 15:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants