feat: [M3-7529] – Support VPC in Personal Access Token drawer #10024

dwiley-akamai · 2024-01-02T22:08:30Z

Description 📝

If VPCs should be displayed in the app (based on customer tags or feature flag), display a scope for VPCs in the Access Token drawers (My Profile > API Tokens > the Create & View drawers) also.

To-Do

display only if VPC is enabled
confirm tooltip copy
Unit tests

Changes 🔄

Allow AccessCell to take an optional tooltipText prop so we can display tooltip for disabled "Read Only" cell for VPC scope
Add logic in CreateAPITokenDrawer.tsx and ViewAPITokenDrawer.tsx so there's a VPC line item only if VPCs should be displayed in the app based on customer tag or feature flag

Preview 📷

Screenshots

Add Personal Access Token drawer:

View Scopes drawer:

How to test 🧪

Prerequisites

an account with proper VPC tags

Verification steps

These changes should be confirmed both with and without the proper VPC tag. In the latter case, we want to test by toggling the VPC feature flag in the DevTools on and off to ensure the expected behavior is observed.

VPC tag on account --> "VPCs" line item in Create Token & "View Scopes" drawers
VPC tag not on account, VPC feature flag off --> no line item for "VPCs" in either drawer
VPC tag not on account, VPC feature flag on --> "VPCs" line item in Create Token & "View Scopes" drawers

Toggling the feature flag on and off in the middle of testing should result in immediately observing the expected behavior once the drawer is re-opened

As an Author I have considered 🤔

…adio button disabled

dwiley-akamai · 2024-01-02T22:10:37Z

packages/manager/src/features/ObjectStorage/AccessKeyLanding/AccessCell.tsx

+
+  return tooltipText ? (
+    <Tooltip placement="top" title={tooltipText}>
+      <span>{radioBtn}</span>


By default, tooltips do not activate on disabled elements. The workaround for this is to add a wrapper such as a <span>. More details here.

… in CreateAPITokenDrawer & ViewAPITokenDrawer

packages/manager/src/features/Profile/APITokens/CreateAPITokenDrawer.tsx

github-actions · 2024-01-04T22:08:11Z

Coverage Report: ✅
Base Coverage: 79.83%
Current Coverage: 79.83%

bnussman-akamai · 2024-01-05T15:05:07Z

packages/manager/src/features/Profile/APITokens/CreateAPITokenDrawer.tsx

+  const [basePermNameMap, setBasePermNameMap] = React.useState(
+    _basePermNameMap
+  );


Rather than having state for this, could we derive the value?

Would it work if we had something like getPermsMap that would take basePermNameMap and omit values as needed?

const basePermNameMap = getPermsMap(flags.vpc ? [] : ['vpc']);

It can be derived. Avoiding the edge case I referenced previously would require having useRef in the mix, but I don't think that edge case is valid at this time: with open beta, API is returning VPCs in the account.capabilities array for all users. That actually makes all of the conditional logic for the PAT unnecessary, but having some conditional logic just to be on the safe side until VPC enters GA wouldn't be bad.

So we could do a simplified approach like

// @TODO VPC: once VPC enters GA, remove _basePermNameMap logic and references. // Just use the basePermNameMap import directly w/o any manipulation. const basePermNameMap = getPermsNameMap(_basePermNameMap, { name: 'vpc', shouldBeIncluded: showVPCs, });

where the VPC perm would get removed if showVPCs was false. This would be vulnerable to the edge case, but that edge case would only come into play now if open beta got rolled back and we were testing with our DevTool (or the LD feature flag was toggled at the exact moment a user was interacting with this flow).

jaalah-akamai · 2024-01-11T15:56:10Z

packages/manager/src/features/Profile/APITokens/ViewAPITokenDrawer.tsx

+    return () => {
+      setBasePermNameMap(basePermNameMap);
+    };
+  }, [open, showVPCs]);


We actually took a slightly different approach which I'm interested in hearing your thoughts. We actually removed them from the list of perms until it no longer needs to be feature flagged: https://github.com/linode/manager/pull/10040/files

I noticed that and gave that approach a try earlier in the week, commenting out 'vpc' in basePerms and vpc: 'VPCs' in basePermNameMap in utils.ts and adding the below (+ commenting out the current approach) in CreateAPITokenDrawer.tsx:

if (showVPCs) { const vpcScopeIndex = allPermissions.findIndex( ([scope]) => scope === 'vpc' ); if (vpcScopeIndex === -1) { allPermissions.push(['vpc', 0]); } basePermNameMap.vpc = 'VPCs'; }

It resulted in some weird behavior though where the scope for VPC doesn't appear until another scope is clicked on:

Screen.Recording.2024-01-11.at.1.05.10.PM.mov

Since GA isn't far off and because API is returning VPCs in account.capabilities for all users, I'm leaning towards the simplified approach I described in my previous comment

dwiley-akamai · 2024-01-12T23:18:57Z

Looking into the unit test failures for CreateAPITokenDrawer & ViewAPITokenDrawer

dwiley-akamai · 2024-01-16T19:00:58Z

Looking into the unit test failures for CreateAPITokenDrawer & ViewAPITokenDrawer

Spent some time looking into these and haven't reached a solution -- the failed assertions basically say the VPC line/element cannot be found, but based on the flags/mock data and logic in the underlying components, that shouldn't be the case. Could I get a double-check on those in case I happened to overlook something? @bnussman-akamai @jaalah-akamai

jdamore-linode · 2024-01-16T20:05:23Z

packages/manager/src/features/Profile/APITokens/utils.ts

+  if (basePermNameMap[perm.name] && !perm.shouldBeIncluded) {
+    delete basePermNameMap[perm.name];
+  }
+
+  return basePermNameMap;


This should fix the unit test failures:

Suggested change

if (basePermNameMap[perm.name] && !perm.shouldBeIncluded) {

delete basePermNameMap[perm.name];

}

return basePermNameMap;

const modifiedBasePerms = {...basePermNameMap};

if (modifiedBasePerms[perm.name] && !perm.shouldBeIncluded) {

delete modifiedBasePerms[perm.name];

}

return modifiedBasePerms;

When the ViewAPITokenDrawer and CreateAPITokenDrawer components call this function, they both pass a reference to the basePermNameMap constant that's defined and exported at the top of this module.

Because the first several tests in CreateAPITokenDrawer.test.tsx and ViewAPITokenDrawer.test.tsx do not have VPC enabled, when getPermsNameMap gets called it's mutating the basePermNameMap object by deleting the vpc property. When subsequent tests import basePermNameMap, the vpc property has already been removed and as a result is excluded from the getPermsNameMap function's output and so the VPC permission is not rendered. You can test this by moving your tests to the top of their .test file; as long as they run before other tests where the vpc flag would be disabled, they'll pass.

I think you could fix this a couple ways but the suggestion above does it by ensuring that getPermsNameMap returns a new perm map rather than mutating the given map.

👀 That makes sense, thanks for taking a look and for the explanation!

bnussman-akamai · 2024-01-17T14:43:46Z

Changes are looking good. Should we fix Child Account Access to use a similar pattern? Maybe getPermsNameMap could take an array of things to conditionally hide? I'm seeing the bug where if I make a selection, Child Account Access pops up randomly.

jaalah-akamai · 2024-01-17T15:38:43Z

Changes are looking good. Should we fix Child Account Access to use a similar pattern? Maybe getPermsNameMap could take an array of things to conditionally hide? I'm seeing the bug where if I make a selection, Child Account Access pops up randomly.

I'll create a ticket and update this separately 👍

DevDW added 3 commits January 2, 2024 15:56

VPCs option in PAT drawer, successful submits possible, 'Read Only' r…

1ce7027

…adio button disabled

working tooltip

f6a06b6

rename variable

9d42a01

dwiley-akamai added the VPC Relating to VPC project label Jan 2, 2024

dwiley-akamai self-assigned this Jan 2, 2024

dwiley-akamai commented Jan 2, 2024

View reviewed changes

DevDW added 3 commits January 3, 2024 16:10

Display scope for VPC conditionally based on isFeatureEnabled() logic…

7d4a5bb

… in CreateAPITokenDrawer & ViewAPITokenDrawer

Added changeset: Support VPC in Access Token drawers

1fec282

Unit tests

4c46984

dwiley-akamai commented Jan 4, 2024

View reviewed changes

packages/manager/src/features/Profile/APITokens/CreateAPITokenDrawer.tsx Outdated Show resolved Hide resolved

dwiley-akamai marked this pull request as ready for review January 4, 2024 21:26

dwiley-akamai requested a review from a team as a code owner January 4, 2024 21:26

dwiley-akamai requested review from bnussman-akamai and jaalah-akamai and removed request for a team January 4, 2024 21:26

Passing utils.test.ts

e3eaa66

bnussman-akamai reviewed Jan 5, 2024

View reviewed changes

Merge in latest develop

cf80e18

jaalah-akamai reviewed Jan 11, 2024

View reviewed changes

DevDW added 2 commits January 12, 2024 16:42

Merge in latest develop and resolve conflict

2f09ba1

Take the simplified approach instead

576cd97

Update logic in ViewAPITokenDrawer.tsx as well

286c6d9

jdamore-linode reviewed Jan 16, 2024

View reviewed changes

Adjust getPermsNameMap() and get unit tests passing

40c81d0

jaalah-akamai approved these changes Jan 17, 2024

View reviewed changes

bnussman-akamai approved these changes Jan 17, 2024

View reviewed changes

bnussman-akamai added the Approved Multiple approvals and ready to merge! label Jan 17, 2024

dwiley-akamai merged commit b9ce55d into linode:develop Jan 17, 2024
18 checks passed

dwiley-akamai deleted the M3-7529-vpc-personal-access-token-drawer branch January 17, 2024 16:41

This was referenced Jan 18, 2024

Release v1.110.0 - release → staging #10081

Closed

Release v1.110.0 - release → staging #10082

Merged

upcoming: [M3-7647] - Update logic for child_account PAT #10083

Merged

Release v1.110.0 - staging → master #10092

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [M3-7529] – Support VPC in Personal Access Token drawer #10024

feat: [M3-7529] – Support VPC in Personal Access Token drawer #10024

dwiley-akamai commented Jan 2, 2024 •

edited

Loading

dwiley-akamai Jan 2, 2024

github-actions bot commented Jan 4, 2024 •

edited

Loading

bnussman-akamai Jan 5, 2024

dwiley-akamai Jan 9, 2024

jaalah-akamai Jan 11, 2024

dwiley-akamai Jan 11, 2024

dwiley-akamai commented Jan 12, 2024

dwiley-akamai commented Jan 16, 2024

jdamore-linode Jan 16, 2024

dwiley-akamai Jan 16, 2024

bnussman-akamai commented Jan 17, 2024 •

edited by jaalah-akamai

Loading

jaalah-akamai commented Jan 17, 2024

-  if (basePermNameMap[perm.name] && !perm.shouldBeIncluded) {
-    delete basePermNameMap[perm.name];
-  }
-  return basePermNameMap;
+  const modifiedBasePerms = {...basePermNameMap};
+  if (modifiedBasePerms[perm.name] && !perm.shouldBeIncluded) {
+    delete modifiedBasePerms[perm.name];
+  }
+  return modifiedBasePerms;

feat: [M3-7529] – Support VPC in Personal Access Token drawer #10024

feat: [M3-7529] – Support VPC in Personal Access Token drawer #10024

Conversation

dwiley-akamai commented Jan 2, 2024 • edited Loading

Description 📝

To-Do

Changes 🔄

Preview 📷

How to test 🧪

Prerequisites

Verification steps

As an Author I have considered 🤔

dwiley-akamai Jan 2, 2024

Choose a reason for hiding this comment

github-actions bot commented Jan 4, 2024 • edited Loading

bnussman-akamai Jan 5, 2024

Choose a reason for hiding this comment

dwiley-akamai Jan 9, 2024

Choose a reason for hiding this comment

jaalah-akamai Jan 11, 2024

Choose a reason for hiding this comment

dwiley-akamai Jan 11, 2024

Choose a reason for hiding this comment

dwiley-akamai commented Jan 12, 2024

dwiley-akamai commented Jan 16, 2024

jdamore-linode Jan 16, 2024

Choose a reason for hiding this comment

dwiley-akamai Jan 16, 2024

Choose a reason for hiding this comment

bnussman-akamai commented Jan 17, 2024 • edited by jaalah-akamai Loading

jaalah-akamai commented Jan 17, 2024

dwiley-akamai commented Jan 2, 2024 •

edited

Loading

github-actions bot commented Jan 4, 2024 •

edited

Loading

bnussman-akamai commented Jan 17, 2024 •

edited by jaalah-akamai

Loading