[WX-1734] Workflow permissions modal #5096
Conversation
|
|
||
| return ( | ||
| <Modal | ||
| title={`Permissions for ${workflowOrNamespace} ${namespace}/${name}`} |
There was a problem hiding this comment.
A few notes here:
- It is easy for this title to cut off at the end. With the export modal, we dealt with this by deciding to remove the name of the specific method being exported from the title.
- We should make it clear for the "workflow" case that viewing and editing permissions happens per-snapshot, not per-method.
- If we still want "workflow" in the title, can we use "method" instead for display?
There was a problem hiding this comment.
- I can definitely update the title to un-include the actual name of the workflow, but I since the
namespace/nameis at the end of the title, I could also potentially add ellipses to deal with any overflow. What do you think? - What do you mean by "make it clear"? Did you have an idea of what this should look like?
- Yes, now that we've decided to use the word
methodas opposed toworkflow, I'll update this naming as well.
There was a problem hiding this comment.
The ellipses work automatically already, and often are needed even for short method names. Firecloud UI has the title as Permissions for Workflow namespace/name/snapshotId, which I do think is a bit confusing, but in Firecloud the title is very unlikely to be cut off. I think something like Edit snapshot permissions or Permissions for snapshot namespace/name/snapshotId could work, depending on whether you want to keep the method name in the title — even if we had the snapshot ID at the end, I wouldn't want the user to see Permissions for method namespace/na... with the snapshot ID cut off and assume that they were editing permissions for all snapshots of the method, when their changes would only actually affect the current snapshot.
| <FormLabel id={id} style={{ ...Style.elements.sectionHeader, margin: '1rem 0 0.5rem 0' }}> | ||
| User | ||
| </FormLabel> | ||
| <TextInput id={id} placeholder='Add a user' value={searchValue} onChange={setSearchValue} /> |
There was a problem hiding this comment.
The Firecloud UI version of this modal validates that your input is a valid email address. We need this here to avoid:
- having an error appear outside the modal and losing your progress if you add a non-valid email address and then attempt to save.
- being able to enter "public" into the box, press Add, and then save to make your method snapshot public in an incredibly confusing way.
There was a problem hiding this comment.
I can definitely add some email validation, similar to what we have in other places. And thank you for catching the 'public' option. I'll make sure to add that in so users can mark their methods as public!
| setPermissions(append({ user: userEmail, role: 'READER' } as RawWorkflowsPermissions)); | ||
| }; | ||
|
|
||
| const save = withBusyState(setWorking, async () => { |
There was a problem hiding this comment.
Can we refresh the page or reload the necessary data after a successful save so that the information on the right side of the page will be up-to-date without having to manually reload?
| }} | ||
| /> | ||
| </div> | ||
| <Clickable |
There was a problem hiding this comment.
We should also disable the remove button for the current user. (I appreciate having this button instead of having to choose "NO ACCESS" in the dropdown, though!)
There was a problem hiding this comment.
Agreed. As an additional note, once I can utilize the kebab menu, a Reader will also not be able to edit permissions like in FCUI.
| ); | ||
| }; | ||
|
|
||
| export const PermissionsModal = (props: WorkflowPermissionsModalProps) => { |
There was a problem hiding this comment.
Should we also have the "publicly readable" checkbox to edit public permissions?
There was a problem hiding this comment.
Yes. Thanks for catching!
…al-kp' into wx-1734-workflow-permissions-modal-kp
There was a problem hiding this comment.
Is there a reason for naming this file differently?
There was a problem hiding this comment.
I'm not sure what you mean — the name is different in reference to what?
| @@ -0,0 +1,16 @@ | |||
| export interface RawWorkflowsPermissions { | |||
There was a problem hiding this comment.
I find Raw in the name a little confusing. What do you think about WorkflowUserPermissions or WorkflowPermissionDetails?
There was a problem hiding this comment.
I also found the name confusing at first, but I was not sure if there was a reason for it related to the permissions structure in Agora. I can definitely change it to WorkflowUserPermissions.
src/workflows/SnapshotActionMenu.tsx
Outdated
| onClick={onEditPermissions} | ||
| > | ||
| {makeMenuIcon('cog')} | ||
| Edit permissions |
There was a problem hiding this comment.
Similar to delete option, can this be renamed to Edit snapshot permissions to make it more clear that this only updates permissions for selected snapshot?
|
When testing, I was able to "add" a user even though it was already on the Current Users list. Do you think it is possible to be able to add some form of validation and disable the
|
|
Similarly, if I am logged in as Rocket Raccoon and I am an
|
|
Re: the two comments above by @salonishah11 — I also noticed when testing the original version of this modal that you could add the same user multiple times. Following the precedent from our decision regarding the "delete snapshot" button's being enabled even when a user does not have sufficient namespace permissions, I did not fix this behavior because it is also present in the original Firecloud UI modal. However, your second comment highlighted an especially undesirable case that I had not found before, and it should not be very time-consuming to implement a fix, so I will add validation to avoid these issues. |
sam-schu
dismissed
their stale review
This PR is now a shared effort between Katrina and I, so I am not considering my own review to count as an approval.
|
Jira Ticket: https://broadworkbench.atlassian.net/browse/WX-1734
Summary of changes:
What
Why
Testing strategy
Screen.Recording.2024-10-03.at.4.03.25.PM.mov