Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

client/web: Add UI elements for Azure DevOps auth provider#47865

Merged
indradhanush merged 12 commits into
mainfrom
ig/ado-ui
Feb 21, 2023
Merged

client/web: Add UI elements for Azure DevOps auth provider#47865
indradhanush merged 12 commits into
mainfrom
ig/ado-ui

Conversation

@indradhanush

@indradhanush indradhanush commented Feb 20, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR does a few related things:

  1. Add an icon on the signin page for Azure DevOps
  2. Implement external data functions and add code to show the connected account data for Azure DevOps
  3. Changes the code to display connected account information to use the displayName instead of the url property
  • This change was required because the closest thing to an account page (equivalent of github.com/indradhanush for example) for ADO that I found was: https://aex.dev.azure.com/me. Which shows the currently logged in user, but I couldn't find a permanent URI for a user. As a result the connected account information could not be displayed if we continued to display it against the check for a non-null url in the frontend code
  • Additionally, presence of a display name as the primary check makes more sense
  • On code review, I've reverted this change and instead added a special case handling for azuredevops

👉 Finally, it fixes a bug (discovered as a result of implementing this) with the parseProvider where I was incorrectly using the Kind instead of Type.

Sidenote: Since both are strings and the only difference is that the former is in UPPERCASE while the latter is in lowercase, this is an easy mistake to make. The real fix should be that these should be custom types so that methods that accept these as an argument get the powers of type checking. Expect a follow up PR.

Test plan

  • Builds should pass
  • Tested locally

Sign in page (notice the cute icon)

image

Accounts security page, not connected account

image

Accounts security page, connected account

image

Account disconnection prompt

image

App preview:

Check out the client app preview documentation to learn more.

@cla-bot cla-bot Bot added the cla-signed label Feb 20, 2023
@indradhanush indradhanush mentioned this pull request Feb 20, 2023
Closed
10 tasks
@indradhanush indradhanush marked this pull request as draft February 20, 2023 07:31
@sg-e2e-regression-test-bob

sg-e2e-regression-test-bob commented Feb 20, 2023 •
edited
Loading

Copy link
Copy Markdown

Bundle size report 📦

Initial size Total size Async size Modules
0.01% (+0.16 kb) 0.01% (+1.23 kb) 0.01% (+1.07 kb) 0.00% (0)

Look at the Statoscope report for a full comparison between the commits 47669b0 and 8e24306 or learn more.

Open explanation
  • Initial size is the size of the initial bundle (the one that is loaded when you open the page)
  • Total size is the size of the initial bundle + all the async loaded chunks
  • Async size is the size of all the async loaded chunks
  • Modules is the number of modules in the initial bundle

@indradhanush indradhanush changed the title client/web: Add icon for Azure DevOps auth provider client/web: Add UI elements for Azure DevOps auth provider Feb 20, 2023
@indradhanush indradhanush marked this pull request as ready for review February 20, 2023 15:10
@indradhanush indradhanush requested review from a team February 20, 2023 15:10
@indradhanush indradhanush self-assigned this Feb 20, 2023
@indradhanush indradhanush added the ADO Tier 1 support for Azure DevOps label Feb 20, 2023
sashaostrikov
sashaostrikov reviewed Feb 20, 2023
View reviewed changes

@sashaostrikov sashaostrikov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, left a comment about changing default behaviour for external account URL/display name

Comment thread client/web/src/user/settings/auth/ExternalAccount.tsx Outdated
indradhanush
indradhanush commented Feb 20, 2023
View reviewed changes
Comment thread client/web/src/user/settings/auth/ExternalAccount.tsx Outdated

@indradhanush indradhanush Feb 20, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sg lint -fix format wanted to make this change.

@indradhanush indradhanush Feb 20, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I know why. Will fix.

@mrnugget mrnugget Feb 20, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, that seems like a bug though

mrnugget
mrnugget approved these changes Feb 20, 2023
View reviewed changes

@mrnugget mrnugget left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good PR description!

Comment thread client/web/src/user/settings/auth/ExternalAccount.tsx Outdated

@mrnugget mrnugget Feb 20, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, that seems like a bug though

@indradhanush

indradhanush commented Feb 20, 2023

Copy link
Copy Markdown
Contributor Author

Hmmm, that seems like a bug though

Fixed now. Was some leftover of trying to restore it to its original form while working on Alex's suggestion.

@indradhanush
client/web: Fix regression with spacing
35fb1e8 
Snapshot testing for the win!
@sourcegraph-bot

sourcegraph-bot commented Feb 20, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff 8e24306...47669b0.

Notify File(s)
@eseliger internal/extsvc/azuredevops/users.go
@unknwon enterprise/cmd/frontend/internal/auth/azureoauth/provider.go
enterprise/cmd/frontend/internal/auth/oauth/provider.go

varsanojidan
varsanojidan approved these changes Feb 20, 2023
View reviewed changes

@varsanojidan varsanojidan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@indradhanush indradhanush merged commit f3a35b2 into main Feb 21, 2023
@indradhanush indradhanush deleted the ig/ado-ui branch February 21, 2023 07:10
@indradhanush indradhanush mentioned this pull request Feb 24, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@sashaostrikov sashaostrikov sashaostrikov left review comments

@mrnugget mrnugget mrnugget approved these changes

@varsanojidan varsanojidan varsanojidan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@indradhanush indradhanush

Labels

ADO Tier 1 support for Azure DevOps cla-signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@indradhanush @sg-e2e-regression-test-bob @sourcegraph-bot @mrnugget @varsanojidan @sashaostrikov