[pat] Token UI polish #15015

jeanp413 · 2022-11-28T19:22:52Z

Description

Validate PAT token name

Related Issue(s)

How to test

Release Notes

NONE

Documentation

Werft options:

/werft with-local-preview
If enabled this will build install/preview
/werft with-preview
/werft with-large-vm
/werft with-integration-tests=all
Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

werft-gitpod-dev-com · 2022-11-28T19:34:46Z

started the job as gitpod-build-jp-pat-validation.1 because the annotations in the pull request description changed
(with .werft/ from main)

jeanp413 · 2022-11-28T19:55:08Z

/werft run recreate-preview=true

👍 started the job as gitpod-build-jp-pat-validation.3
(with .werft/ from main)

jeanp413 · 2022-11-30T04:57:30Z

/werft run

👍 started the job as gitpod-build-jp-pat-validation.6
(with .werft/ from main)

mustard-mh · 2022-11-30T07:25:29Z

/werft run recreate-preview=true

👍 started the job as gitpod-build-jp-pat-validation.8
(with .werft/ from main)

components/dashboard/src/settings/PersonalAccessTokens.tsx

gtsiolis

Thanks for tackling most of the issues in #15002, @jeanp413 & @mustard-mh!

I was in the neighborhood and left some UX comments below. 🏀

gtsiolis · 2022-11-30T11:38:38Z

components/dashboard/src/components/PillLabel.tsx

+const PillClsMap: Record<PillType, string> = {
+    info: "bg-blue-50 text-blue-500 dark:bg-blue-500 dark:text-blue-100",
+    warn: "bg-orange-100 text-orange-700 dark:bg-orange-600 dark:text-orange-100",
+    success: "bg-green-100 text-green-700 dark:bg-green-600 dark:text-green-100",
+};


praise: Thank you for extending this component, @jeanp413 & @mustard-mh!

Actually this is @mustard-mh change 😄

I realized this a few hours after posting the comment above.

Thanks for giving credit where credit is due, @jeanp413! 🧡

components/dashboard/src/components/Loader.tsx

components/dashboard/src/settings/PersonalAccessTokens.tsx

gtsiolis · 2022-11-30T12:22:15Z

components/dashboard/src/settings/PersonalAccessTokens.tsx

+                                    token={t}
+                                    menuEntries={[
+                                        {
+                                            title: "Edit",


issue: Not the best line to comment this, but when you Edit a token and add or remove permissions, the expiration date changes back to today. ❗

It can be reproduced.

But it's weird that from the code, we don't send expirationTime, but devTools shows we have send it (maybe a default value of protobuf Timestamp)

And server should not update expirationDate too.

cc @easyCZ

For the fine grained update, there's a second field which you can supply - updateMask. It's a list of of the token fields which should be updated.

It may make sense to link the update form "changes" to those fields and send them to ensure its updated. I'll investigate and can look into fixing the backend, it should never send back internal error code anyway.

updateMask has added in dashboard code, but not worked

Test failed

I can fix it later @easyCZ

I've added a couple of fixes to API validation and Not Found handling in the following:

[pat] Fix NotFound error handling #15075

[pat] Harden retrieval of PATs and check for nil UUIDs #15072

That should at least give us more info about the error.

Looks like the original migration put the on update on the wrong column. The on-update should be on set on _lastModified

Great find! I'll add a migration tomorrow.

Migration PR: #15093

The update issue, where the expiration would be reset is now fixed in staging.

The remaining issue is that a no-update returns a Not Found from the API. Tracked here #15105

components/dashboard/src/settings/PersonalAccessTokens.tsx

components/dashboard/src/components/PillLabel.tsx

components/dashboard/src/settings/PersonalAccessTokens.tsx

Co-authored-by: Huiwen <[email protected]>

easyCZ · 2022-12-01T10:02:26Z

Preview is broken for me, rerunning.

/werft run with-clean-slate-deployment=true

👍 started the job as gitpod-build-jp-pat-validation.18
(with .werft/ from main)

easyCZ · 2022-12-01T12:31:10Z

Looking good. There's a couple of minor aspects which we can follow-up on.

Here we should drop the `Personal` keywords, to be consistent with other places.

here also

The expiration icon should have a tooltip to explain why it's there

Validation works great
We still have the update issue which changes the expiration, but that will be fixed once we roll out [pat] Fix expiration time auto-udpate #15097

easyCZ · 2022-12-01T12:32:55Z

Scopes updating also works as expected

easyCZ · 2022-12-01T12:34:11Z

I'm going to approve the PR, and I'll track/put up PRs for the small bits from this PR. Thanks a lot for the improvements, this is awesome to see materialize.

easyCZ · 2022-12-01T13:11:56Z

The follow-up to improve the expiration time UX (tooltip) is tracked here #15103

laushinka · 2022-12-05T11:20:14Z

Hi @jeanp413, sorry for the late question, but was there a reason why TokenEntry.tsx was moved into PersonalAccessTokens.tsx? Asking because the component now has quite a lot to navigate reading through, and I think we can benefit from extracting things out for readability's sake.

gtsiolis · 2022-12-05T11:24:00Z

@laushinka I had a similar thought back in #15015 (comment).

jeanp413 · 2022-12-05T13:03:49Z

@laushinka sorry, I had it in my mind to ask you about it here in the review but then forgot about it 🤦, I moved because I created another small component only for tokens in PersonalAccessTokens.tsx that I needed to reuse in the TokenEntry.tsx too and wanted to avoid cyclic dependencies, I saw other files that coupled all related components in one file so I just followed that.

If it's preferred to have split components, I can do a follow up PR today 👍 , and if that's the case would it makes sense to group multiple files related to PAT inside a subfolder or is it Ok to add them all in settings folder?

laushinka · 2022-12-05T14:25:46Z

@laushinka sorry, I had it in my mind to ask you about it here in the review but then forgot about it 🤦

@jeanp413 All good! I just did this PR. Hope that looks reasonable to you too :)

would it makes sense to group multiple files related to PAT inside a subfolder or is it Ok to add them all in settings folder?

I thought about it too, and I think it would make sense to group them all inside a subfolder as you suggest 👍🏼 I didn't do that in the PR above, but I can do a follow-up.

roboquat added do-not-merge/work-in-progress release-note-none size/XS labels Nov 28, 2022

roboquat added size/S and removed size/XS labels Nov 28, 2022

jeanp413 force-pushed the jp/pat-validation branch from ed24884 to a4bb5af Compare November 29, 2022 18:10

roboquat added size/XXL and removed size/S labels Nov 30, 2022

mustard-mh force-pushed the jp/pat-validation branch from 5f034ce to 8d3a833 Compare November 30, 2022 07:46

jeanp413 commented Nov 30, 2022

View reviewed changes

components/dashboard/src/settings/PersonalAccessTokens.tsx Outdated Show resolved Hide resolved

jeanp413 commented Nov 30, 2022

View reviewed changes

components/dashboard/src/settings/PersonalAccessTokens.tsx Outdated Show resolved Hide resolved

gtsiolis reviewed Nov 30, 2022

View reviewed changes

mustard-mh force-pushed the jp/pat-validation branch from d6f77f0 to 7ffe8cc Compare November 30, 2022 13:11

[pat] Token UI polish

d660036

Co-authored-by: Huiwen <[email protected]>

jeanp413 force-pushed the jp/pat-validation branch from 7ffe8cc to d660036 Compare November 30, 2022 20:01

jeanp413 marked this pull request as ready for review November 30, 2022 20:02

jeanp413 requested a review from a team November 30, 2022 20:02

roboquat removed the do-not-merge/work-in-progress label Nov 30, 2022

github-actions bot added the team: webapp Issue belongs to the WebApp team label Nov 30, 2022

mustard-mh changed the title ~~[pat] Validate name~~ [pat] Token UI polish Nov 30, 2022

easyCZ mentioned this pull request Dec 1, 2022

[pat] Apply on-update property to _lastModified column, remove from expirationTime #15093

Merged

4 tasks

easyCZ approved these changes Dec 1, 2022

View reviewed changes

roboquat merged commit 77c68c7 into main Dec 1, 2022

roboquat deleted the jp/pat-validation branch December 1, 2022 12:35

easyCZ mentioned this pull request Dec 1, 2022

[pat] Remove Personal from Access Tokens UI #15099

Closed

4 tasks

roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Dec 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pat] Token UI polish #15015

[pat] Token UI polish #15015

jeanp413 commented Nov 28, 2022 •

edited by mustard-mh

Loading

werft-gitpod-dev-com bot commented Nov 28, 2022

jeanp413 commented Nov 28, 2022 •

edited by werft-gitpod-dev-com bot

Loading

jeanp413 commented Nov 30, 2022 •

edited by werft-gitpod-dev-com bot

Loading

mustard-mh commented Nov 30, 2022 •

edited by werft-gitpod-dev-com bot

Loading

gtsiolis left a comment •

edited

Loading

gtsiolis Nov 30, 2022 •

edited

Loading

jeanp413 Nov 30, 2022

gtsiolis Nov 30, 2022 •

edited

Loading

gtsiolis Nov 30, 2022

mustard-mh Nov 30, 2022

easyCZ Nov 30, 2022

mustard-mh Nov 30, 2022 •

edited

Loading

easyCZ Nov 30, 2022

easyCZ Nov 30, 2022

easyCZ Nov 30, 2022

easyCZ Dec 1, 2022

easyCZ Dec 1, 2022

easyCZ Dec 1, 2022

easyCZ commented Dec 1, 2022 •

edited by werft-gitpod-dev-com bot

Loading

easyCZ commented Dec 1, 2022

easyCZ commented Dec 1, 2022

easyCZ commented Dec 1, 2022

easyCZ commented Dec 1, 2022

laushinka commented Dec 5, 2022 •

edited

Loading

gtsiolis commented Dec 5, 2022 •

edited

Loading

jeanp413 commented Dec 5, 2022

laushinka commented Dec 5, 2022 •

edited

Loading

[pat] Token UI polish #15015

[pat] Token UI polish #15015

Conversation

jeanp413 commented Nov 28, 2022 • edited by mustard-mh Loading

Description

Related Issue(s)

How to test

Release Notes

Documentation

Werft options:

werft-gitpod-dev-com bot commented Nov 28, 2022

jeanp413 commented Nov 28, 2022 • edited by werft-gitpod-dev-com bot Loading

jeanp413 commented Nov 30, 2022 • edited by werft-gitpod-dev-com bot Loading

mustard-mh commented Nov 30, 2022 • edited by werft-gitpod-dev-com bot Loading

gtsiolis left a comment • edited Loading

Choose a reason for hiding this comment

gtsiolis Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gtsiolis Nov 30, 2022 • 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

mustard-mh Nov 30, 2022 • 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

easyCZ commented Dec 1, 2022 • edited by werft-gitpod-dev-com bot Loading

easyCZ commented Dec 1, 2022

easyCZ commented Dec 1, 2022

easyCZ commented Dec 1, 2022

easyCZ commented Dec 1, 2022

laushinka commented Dec 5, 2022 • edited Loading

gtsiolis commented Dec 5, 2022 • edited Loading

jeanp413 commented Dec 5, 2022

laushinka commented Dec 5, 2022 • edited Loading

jeanp413 commented Nov 28, 2022 •

edited by mustard-mh

Loading

jeanp413 commented Nov 28, 2022 •

edited by werft-gitpod-dev-com bot

Loading

jeanp413 commented Nov 30, 2022 •

edited by werft-gitpod-dev-com bot

Loading

mustard-mh commented Nov 30, 2022 •

edited by werft-gitpod-dev-com bot

Loading

gtsiolis left a comment •

edited

Loading

gtsiolis Nov 30, 2022 •

edited

Loading

gtsiolis Nov 30, 2022 •

edited

Loading

mustard-mh Nov 30, 2022 •

edited

Loading

easyCZ commented Dec 1, 2022 •

edited by werft-gitpod-dev-com bot

Loading

laushinka commented Dec 5, 2022 •

edited

Loading

gtsiolis commented Dec 5, 2022 •

edited

Loading

laushinka commented Dec 5, 2022 •

edited

Loading