Fix invite flow depending on permissions #1182
Conversation
Goes into more detail on what should happen on the org page and the space page.
entity is either space or org also make the org roles load on the space page
|
@brittag this addresses number one of your comments here https://favro.com/card/1e11108a2da81e3bd7153a7a/18F-6974 |
|
This is really great. |
static_src/components/users.jsx
Outdated
| // existing org users but not new ones. | ||
| if (this.currentUserIsSpaceManager) { | ||
| return ( | ||
| <PanelDocumentation> |
There was a problem hiding this comment.
In the future, I can imagine there only needing to be one return statement. All the other content can be compiled as a raw variable.
There was a problem hiding this comment.
i agree but we need to first clean up this decision tree of branches otherwise it could get confusing and inefficient assigning / reassigning variables.
|
Cool! We should be consistent about capitalizing Org Manager and Space Manager throughout the copy (see https://github.com/18F/cg-product/blob/master/ContentGuide.md for list of consistent capitalization/spacing/etc. to use for this kind of thing). |
jcscottiii
dismissed stale reviews from el-mapache and rememberlenny
via
4539f75
static_src/components/users.jsx
Outdated
| @@ -24,6 +24,14 @@ const ORG_MANAGER = 'org_manager'; | |||
| const SPACE_MANAGER = 'space_manager'; | |||
| const ORG_ENTITY = 'organization'; | |||
| const SPACE_ENTITY = 'space'; | |||
| const ORG_INVITE_HELP = 'Only an Org Manager can new invite users to this ' + | |||
There was a problem hiding this comment.
Good move. We might also want to consider moving all strings into a separate file that gets imported.
There was a problem hiding this comment.
agreed. there's a constants.js file too we should probably move things there eventually but should make we group them accordingly.
There was a problem hiding this comment.
@rememberlenny I actually have a ticket somewhere in the backlog to start rounding up strings and splitting them into constant files! 😄
| @@ -24,7 +24,14 @@ const ORG_MANAGER = 'org_manager'; | |||
| const SPACE_MANAGER = 'space_manager'; | |||
| const ORG_ENTITY = 'organization'; | |||
| const SPACE_ENTITY = 'space'; | |||
| const cfCliLink = 'https://docs.cloudfoundry.org/adminguide/cli-user-management.html#space-roles'; | |||
| const ORG_INVITE_HELP = 'Only an Org Manager can new invite users to this ' + | |||
There was a problem hiding this comment.
Is there a reason these long strings aren't being done with dedent and template strings? I can't remember it if there is and wanted to learn for other projects.
There was a problem hiding this comment.
there was this commit that scarred me after it took a while finding the problem lol 77a78f3
There was a problem hiding this comment.
In some cases template strings introduce unnecessary spaces, and will fail tests if the strings aren't formatted identically.
If the strings are all moved into constant files, the issue should go away?
Before this PR:
After this PR
Help Message for a user has been split into three cases
When on the org page without org manager permissions
When on the space page as an space manager
When on the space space not as a space manager
(I didn't bother to go into the difference between existing org users vs new org users because that might be confusing to a non-space manager/ non-org-manager user.)
(The cases when the user is an org manager on either space or org page, there's no help text to ask someone for help or to do things via the CLI)
Org Managers can invite existing org users to space
(This was because the space page didn't load the org roles)
The first commit is the tests adds coverage for the desired behavior mentioned above (CI will fail)
The second commit fixes the code to match the behavior.
(A third commit may be required to fix any functional tests + comments from reviewers)