feat: add ordering to auth providers in login screen#50434
Conversation
erzhtor
left a comment
There was a problem hiding this comment.
Thanks for handling this! Besides left comments, looks good!
| "type": "string", | ||
| "description": "URL of the Gerrit instance, such as https://gerrit-review.googlesource.com or https://gerrit.example.com.", | ||
| "default": "https://gerrit-review.googlesource.com/" | ||
| }, |
There was a problem hiding this comment.
Don't we have displayName and displayPrefix for Gerrit?
There was a problem hiding this comment.
Nope, because we are hiding Gerrit by default in this code: https://sourcegraph.sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/client/web/src/auth/SignInPage.tsx?L66
| "description": "The name to use when displaying this authentication provider in the UI. Defaults to an auto-generated name with the type of authentication provider and other relevant identifiers (such as a hostname).", | ||
| "type": "string" | ||
| }, | ||
| "displayPrefix": { |
There was a problem hiding this comment.
Was this a requested somewhere? I'm not against this but I just thought that 1) it would be better having a single way to present providers on the page 2) avoid unnecessary configuration options, thus keeping it simple
There was a problem hiding this comment.
This was not requested, but I think the current way is rather inflexible and incompatible, depending on what you put into the DisplayName:
Even having a single option for all of the providers might not work, since depending on the intent, you might need something different. Do you really think it complicates the configuration that much? It's optional in the end...
…" [no-bazel] This reverts commit 2229942.
Reverts sourcegraph/sourcegraph#50434 Test Plan: CI tests
## Description This PR makes several changes to how we handle auth providers in the code, which in the end allows us to change ordering of auth providers on the login screen. 1. We do not sort auth providers by default (this is an optimization). We call the `providers.Providers()` method in a bunch of places, but really only need it sorted for the UI 2. Added default fields to all auth providers (where it made sense), since they are shared amongst all of them ``` DisplayName string `json:"displayName,omitempty"` DisplayPrefix *string `json:"displayPrefix,omitempty"` Hidden bool `json:"hidden,omitempty"` Order int `json:"order,omitempty"` ``` 3. Allows to hide each auth provider from the user completely by adding the `"hidden"` property to the auth provider config 4. Allows to change the prefix of the auth provider on the screen from `"Continue with "` to whatever the admin likes 5. Allows to change the order of auth providers on the login screen by adding optional "order" attribute, which is a positive int (greater than 0). Providers with order will be sorted first, followed by ordering we had up until now. ## Related - https://github.com/sourcegraph/sourcegraph/issues/49831 - #50284 ## Possible improvements Remove the usage of `reflect`, since I don't like it that much. However our [current json schema library for generating Go code](https://github.com/sourcegraph/go-jsonschema) does not support struct embedding with `$ref` attribute, which would fix the problem more elegantly. I was not smart enough to figure out how to change it to support that. ## Test plan Tested locally + unit tests.
Reverts sourcegraph/sourcegraph#50434 Test Plan: CI tests
Description
This PR makes several changes to how we handle auth providers in the code, which in the end allows us to change ordering of auth providers on the login screen.
providers.Providers()method in a bunch of places, but really only need it sorted for the UI"hidden"property to the auth provider config"Continue with "to whatever the admin likesRelated
Possible improvements
Remove the usage of
reflect, since I don't like it that much. However our current json schema library for generating Go code does not support struct embedding with$refattribute, which would fix the problem more elegantly. I was not smart enough to figure out how to change it to support that.Test plan
Tested locally + unit tests.