[WX-1734] Workflow permissions modal #5096

kpierre13 · 2024-09-20T16:13:12Z

Jira Ticket: https://broadworkbench.atlassian.net/browse/WX-1734

Summary of changes:

What

Adding a permissions modal to the workflows pages

Why

Firecloud UI to Terra UI migration

Testing strategy

Manual testing
Unit tests

Screen.Recording.2024-10-03.at.4.03.25.PM.mov

Since the video was recorded, the modal was updated with a warning as shown below:

src/pages/workflows/workflow/common/PermissionsModal.tsx

sam-schu · 2024-09-23T20:33:04Z

src/pages/workflows/workflow/common/PermissionsModal.tsx

+
+  return (
+    <Modal
+      title={`Permissions for ${workflowOrNamespace} ${namespace}/${name}`}


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?

I can definitely update the title to un-include the actual name of the workflow, but I since the namespace/name is 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 method as opposed to workflow, I'll update this naming as well.

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.

sam-schu · 2024-09-23T20:43:23Z

src/pages/workflows/workflow/common/PermissionsModal.tsx

+              <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} />


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.

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!

sam-schu · 2024-09-23T20:49:45Z

src/pages/workflows/workflow/common/PermissionsModal.tsx

+    setPermissions(append({ user: userEmail, role: 'READER' } as RawWorkflowsPermissions));
+  };
+
+  const save = withBusyState(setWorking, async () => {


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?

sam-schu · 2024-09-23T20:55:10Z

src/pages/workflows/workflow/common/PermissionsModal.tsx

+          }}
+        />
+      </div>
+      <Clickable


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!)

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.

sam-schu · 2024-09-23T20:59:03Z

src/pages/workflows/workflow/common/PermissionsModal.tsx

+  );
+};
+
+export const PermissionsModal = (props: WorkflowPermissionsModalProps) => {


Should we also have the "publicly readable" checkbox to edit public permissions?

Yes. Thanks for catching!

…al-kp' into wx-1734-workflow-permissions-modal-kp

salonishah11 · 2024-10-03T20:53:55Z

src/pages/workflows/workflow/workflows-acl-utils.ts

Is there a reason for naming this file differently?

I'm not sure what you mean — the name is different in reference to what?

It seems files are usually named in camel case pattern and this one is named with all lower case letters and with - in it. Maybe it can be named as WorkflowsAclUtils?

I assume that this file name is modeled off of src/pages/workflows/workflow-utils.ts, whose name in turn likely follows the convention of all the utils files in packages/core-utils/src. If you think we should not be following the pattern from that directory, I can rename this file now, and then we can rename workflow-utils.ts as part of this ticket.

Ah I didn't know about that existing pattern! In this case ignore my suggestion.

salonishah11 · 2024-10-03T21:16:39Z

src/pages/workflows/workflow/workflows-acl-utils.ts

@@ -0,0 +1,16 @@
+export interface RawWorkflowsPermissions {


I find Raw in the name a little confusing. What do you think about WorkflowUserPermissions or WorkflowPermissionDetails?

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.

salonishah11 · 2024-10-03T21:24:10Z

src/workflows/SnapshotActionMenu.tsx

+        onClick={onEditPermissions}
+      >
+        {makeMenuIcon('cog')}
+        Edit permissions


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?

salonishah11 · 2024-10-03T21:34:59Z

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 Add button when an email id entered is is already on the list? Also, in the below screenshot, Rocket was already an Owner on the snapshot. When I added the same user again it was defaulted to Reader. Clicking on Save changed the permission for Rocket to Reader.

salonishah11 · 2024-10-03T21:53:43Z

Similarly, if I am logged in as Rocket Raccoon and I am an Owner of a snapshot, I am able to "add" myself again in the Current Users list with Reader permission. I can't edit my own permission in the modal and hence clicking on Save throws an error correctly from Agora stating that I can't edit my own permissions. It would be good to add validation around not being able to add pre-existing user to avoid this error from Agora.

sam-schu · 2024-10-04T16:51:15Z

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.

This PR is now a shared effort between Katrina and I, so I am not considering my own review to count as an approval.

salonishah11

One small nit regarding file naming otherwise LGTM

sonarcloud · 2024-10-10T19:46:10Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
95.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

kpierre13 added 9 commits September 10, 2024 10:22

wip

7070133

wip

6af9ea5

add new endpoints

3e4b075

add temp button for modal

fb32943

PermissionsModal

14c4872

PermissionsModal tests

4202482

wrap in act() errors

3fbe881

Add key to User component

340d3a0

update aria-label

24a173d

broadbot requested a deployment to pr-3 September 23, 2024 14:27 Abandoned

broadbot temporarily deployed to pr-3 September 23, 2024 14:40 Inactive

sam-schu self-requested a review September 23, 2024 15:28

sam-schu previously requested changes Sep 23, 2024

View reviewed changes

kpierre13 added 3 commits September 25, 2024 11:02

wip

c7921bc

add email validation

3b95dce

Update modal title; add publicly readable checkbox; update tests

f3e3b8a

kpierre13 requested a review from sam-schu September 27, 2024 13:56

kpierre13 added 13 commits September 27, 2024 09:57

wip

ff42081

wip

54de947

add new endpoints

f90b83b

add temp button for modal

6404e8b

PermissionsModal

46f7767

PermissionsModal tests

5297368

wrap in act() errors

cdbc533

Add key to User component

754cb66

update aria-label

75d186d

wip

51daaf8

add email validation

c913e02

Update modal title; add publicly readable checkbox; update tests

2e90fe0

Merge remote-tracking branch 'origin/wx-1734-workflow-permissions-mod…

ca022f0

…al-kp' into wx-1734-workflow-permissions-modal-kp

salonishah11 reviewed Oct 3, 2024

View reviewed changes

sam-schu added 2 commits October 4, 2024 10:32

Update naming

3e0f5ac

Rename snapshot action menu button

64713ea

sam-schu added 2 commits October 4, 2024 13:08

Do not allow existing users to be added again

3b8a585

Merge branch 'dev' into wx-1734-workflow-permissions-modal-kp

bc7987d

broadbot temporarily deployed to pr-3 October 4, 2024 17:22 Inactive

salonishah11 approved these changes Oct 7, 2024

View reviewed changes

aednichols approved these changes Oct 7, 2024

View reviewed changes

sam-schu added 2 commits October 9, 2024 12:52

Switch lodash import to lodash/fp

55af3ed

Improve loading spinner

3bce5f5

broadbot temporarily deployed to pr-3 October 9, 2024 19:42 Inactive

sam-schu added 2 commits October 9, 2024 16:05

Improve test typing

979f41c

Merge branch 'dev' into wx-1734-workflow-permissions-modal-kp

948ecd2

broadbot temporarily deployed to pr-3 October 9, 2024 20:20 Inactive

Improve test

db0b5b1

broadbot temporarily deployed to pr-3 October 10, 2024 15:37 Inactive

sam-schu added 2 commits October 10, 2024 15:30

Add user groups warning

5cdd35c

Merge branch 'dev' into wx-1734-workflow-permissions-modal-kp

6834050

broadbot temporarily deployed to pr-3 October 10, 2024 19:46 Inactive

sam-schu added this pull request to the merge queue Oct 10, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 10, 2024

sam-schu added this pull request to the merge queue Oct 10, 2024

Merged via the queue into dev with commit adbf363 Oct 10, 2024
10 checks passed

sam-schu deleted the wx-1734-workflow-permissions-modal-kp branch October 10, 2024 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WX-1734] Workflow permissions modal #5096

[WX-1734] Workflow permissions modal #5096

kpierre13 commented Sep 20, 2024 •

edited by sam-schu

Loading

sam-schu Sep 23, 2024

kpierre13 Sep 23, 2024 •

edited

Loading

sam-schu Sep 24, 2024

sam-schu Sep 23, 2024

kpierre13 Sep 23, 2024

sam-schu Sep 23, 2024

sam-schu Sep 23, 2024 •

edited

Loading

kpierre13 Sep 23, 2024 •

edited

Loading

sam-schu Sep 23, 2024

kpierre13 Sep 23, 2024

salonishah11 Oct 3, 2024

sam-schu Oct 4, 2024

salonishah11 Oct 7, 2024

sam-schu Oct 7, 2024

salonishah11 Oct 8, 2024

salonishah11 Oct 3, 2024

sam-schu Oct 4, 2024

salonishah11 Oct 3, 2024

salonishah11 commented Oct 3, 2024 •

edited

Loading

salonishah11 commented Oct 3, 2024

sam-schu commented Oct 4, 2024

salonishah11 left a comment

sonarcloud bot commented Oct 10, 2024

[WX-1734] Workflow permissions modal #5096

[WX-1734] Workflow permissions modal #5096

Conversation

kpierre13 commented Sep 20, 2024 • edited by sam-schu Loading

Jira Ticket: https://broadworkbench.atlassian.net/browse/WX-1734

Summary of changes:

What

Why

Testing strategy

Choose a reason for hiding this comment

kpierre13 Sep 23, 2024 • 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

sam-schu Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

kpierre13 Sep 23, 2024 • 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

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

salonishah11 commented Oct 3, 2024 • edited Loading

salonishah11 commented Oct 3, 2024

sam-schu commented Oct 4, 2024

salonishah11 left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Oct 10, 2024

Quality Gate passed

kpierre13 commented Sep 20, 2024 •

edited by sam-schu

Loading

kpierre13 Sep 23, 2024 •

edited

Loading

sam-schu Sep 23, 2024 •

edited

Loading

kpierre13 Sep 23, 2024 •

edited

Loading

salonishah11 commented Oct 3, 2024 •

edited

Loading