- Notifications
You must be signed in to change notification settings - Fork921
feat: add name field to setup screen + CLI#13491
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Minor nit/question, otherwise LGTM
Uh oh!
There was an error while loading.Please reload this page.
Design considerations:
|
| @@ -10,6 +10,9 @@ OPTIONS: | |||
| Specifies an email address to use if creating the first user for the | |||
| deployment. | |||
| --first-user-full-name string, $CODER_FIRST_USER_FULL_NAME | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nit: I think we should use "name" instead of "full name" since it is uncommon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I originally usedname but@dannykopping suggestedfull_name instead to avoid confusion betweenname andusername.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I see.... 👍
| @@ -200,6 +202,19 @@ func (api *API) postFirstUser(rw http.ResponseWriter, r *http.Request) { | |||
| return | |||
| } | |||
| //nolint:gocritic // Needed to create first user. | |||
| if _, err := api.Database.UpdateUserProfile(dbauthz.AsSystemRestricted(ctx), database.UpdateUserProfileParams{ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why do we need to run a separate query to update the name? In my head, it would be done in theCreateUserProfile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure what you mean; there is no such queryCreateUserProfile. Do you meanInsertUser?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I can't see a good reason to create a user without the name and after, update it. IMO, it should be a single operation.
| @@ -90,6 +90,7 @@ type LicensorTrialRequest struct { | |||
| type CreateFirstUserRequest struct { | |||
| Email string `json:"email" validate:"required,email"` | |||
| Username string `json:"username" validate:"required,username"` | |||
| FullName string `json:"name" validate:"user_real_name"` | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why do we need a special validation for this? IMO we can use something likevalidate:min=3,max=50
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure why the original validation was added, but I'm using it here for consistency withUpdateUserProfile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I feel like we're ending up with a bit of inconsistency in the API and API responses. There's a disconnect between full name and name since we're calling it one thing in some places and something else in other places. Here we're essentially:
- Reading in "full name" (cli)
- Serializing as "name" over the wire (JSON)
- Deserializing as "full name"
- Writing "name" to database 😅
| onBlur={(e) => { | ||
| e.target.value = e.target.value.trim(); | ||
| form.handleChange(e); | ||
| }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I want to ensure that any leading/trailing whitespace is removed before submission.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I feel this should be done in the BE before saving the data on DB - so it is consistent between clients. We don't do that in any other form in the UI so, IMO, we can remove this for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We don't do that in any other form in the UI
I saw that it was doneAccountForm so followed suit here.
IMO we are better off trimming in both places.
I feel this should be done in the BE before saving the data on DB
httpapi.UserRealNameValid should handle this, but added an explicit call toNormalizeRealUsername for consistency with OIDC logins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I left a few considerations but another question. Is the "full name" field optional or required?
It's entirely optional. |
As this is an optional field, doesn't it make more sense to leave it till last?
Is this normal for an optional field? |
In the UI, we usually sort fields by relevance and "standards". In most apps, you will see the name is the first one.
Usually, the autofocus is applied to the first field in the form.
Ideally, we would have styling for fields marked as optional with an extra label like If you have a strong opinion about left the name as the last field, I'm ok with that 👍 |
mafredri left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I feel like perhaps we should only keep "full name" as externally visible naming in CLI, and all of the code/API uses plain "name". Otherwise LGTM.
| if errors.Is(err, cliui.Canceled) { | ||
| return "", nil | ||
| } | ||
| if err != nil { | ||
| return "", err | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| iferrors.Is(err,cliui.Canceled) { | |
| return"",nil | |
| } | |
| iferr!=nil { | |
| return"",err | |
| } | |
| iferr!=nil { | |
| iferrors.Is(err,cliui.Canceled) { | |
| return"",nil | |
| } | |
| return"",err | |
| } |
Minor nit: one error handling block has less chance of being disconnected in future.
| @@ -90,6 +90,7 @@ type LicensorTrialRequest struct { | |||
| type CreateFirstUserRequest struct { | |||
| Email string `json:"email" validate:"required,email"` | |||
| Username string `json:"username" validate:"required,username"` | |||
| FullName string `json:"name" validate:"user_real_name"` | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I feel like we're ending up with a bit of inconsistency in the API and API responses. There's a disconnect between full name and name since we're calling it one thing in some places and something else in other places. Here we're essentially:
- Reading in "full name" (cli)
- Serializing as "name" over the wire (JSON)
- Deserializing as "full name"
- Writing "name" to database 😅
I'm going to close this out in favour of a new PR, as this one has gotten fairly cluttered. |
Uh oh!
There was an error while loading.Please reload this page.
Part of#13490
This takes the simpler approach of simply adding Name to the CreateFirstUser payload.
Still need to add the "name" field to the new user creation flow (UI/CLI), but that will be a separate PR.