feat: add name field to setup screen + CLI #13491

johnstcn · 2024-06-06T13:47:19Z

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.

dannykopping

Minor nit/question, otherwise LGTM

cli/login.go

BrunoQuaresma · 2024-06-14T12:40:44Z

Design considerations:

Instead of "Full name" just "Name" is ok and most used in other apps.
Move the name field to be the first. It is also standard in other apps.
Remove autofocus from the username and add it to the name field.

BrunoQuaresma · 2024-06-14T12:42:38Z

cli/testdata/coder_login_--help.golden

@@ -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


Nit: I think we should use "name" instead of "full name" since it is uncommon.

I originally used name but @dannykopping suggested full_name instead to avoid confusion between name and username.

I see.... 👍

BrunoQuaresma · 2024-06-14T12:44:25Z

coderd/users.go

@@ -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{


Why do we need to run a separate query to update the name? In my head, it would be done in the CreateUserProfile.

I'm not sure what you mean; there is no such query CreateUserProfile. Do you mean InsertUser?

Here: https://github.com/coder/coder/blob/3eadba7c95f92fe5011e35c9eb98df8709816b70/coderd/users.go#L188C2-L196C4

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.

BrunoQuaresma · 2024-06-14T12:46:21Z

codersdk/users.go

@@ -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"`


Why do we need a special validation for this? IMO we can use something likevalidate:min=3,max=50

I'm not sure why the original validation was added, but I'm using it here for consistency with UpdateUserProfile

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 😅

BrunoQuaresma · 2024-06-14T12:47:37Z

site/src/pages/SetupPage/SetupPageView.tsx

+            onBlur={(e) => {
+              e.target.value = e.target.value.trim();
+              form.handleChange(e);
+            }}


Why do we need this?

I want to ensure that any leading/trailing whitespace is removed before submission.

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.

We don't do that in any other form in the UI

I saw that it was done AccountForm 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 to NormalizeRealUsername for consistency with OIDC logins.

BrunoQuaresma

I left a few considerations but another question. Is the "full name" field optional or required?

johnstcn · 2024-06-14T13:02:55Z

I left a few considerations but another question. Is the "full name" field optional or required?

It's entirely optional.

johnstcn · 2024-06-14T13:11:20Z

Move the name field to be the first. It is also standard in other apps.

As this is an optional field, doesn't it make more sense to leave it till last?

Remove autofocus from the username and add it to the name field.

Is this normal for an optional field?

BrunoQuaresma · 2024-06-14T16:37:00Z

As this is an optional field, doesn't it make more sense to leave it till last?

In the UI, we usually sort fields by relevance and "standards". In most apps, you will see the name is the first one.

Is this normal for an optional field?

Usually, the autofocus is applied to the first field in the form.

It's entirely optional.

Ideally, we would have styling for fields marked as optional with an extra label like (optional) or with red asterisks to say it is required, but unfortunately, we don't follow any convention. 😓

If you have a strong opinion about left the name as the last field, I'm ok with that 👍

mafredri

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.

mafredri · 2024-06-20T12:35:10Z

cli/login.go

+	if errors.Is(err, cliui.Canceled) {
+		return "", nil
+	}
+	if err != nil {
+		return "", err
+	}


Suggested change

if errors.Is(err, cliui.Canceled) {

return "", nil

}

if err != nil {

return "", err

}

if err != nil {

if errors.Is(err, cliui.Canceled) {

return "", nil

}

return "", err

}

Minor nit: one error handling block has less chance of being disconnected in future.

mafredri · 2024-06-20T15:16:26Z

codersdk/users.go

@@ -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"`


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 😅

johnstcn · 2024-06-25T12:47:52Z

I'm going to close this out in favour of a new PR, as this one has gotten fairly cluttered.

feat: add name field to setup screen

22335c7

johnstcn self-assigned this Jun 6, 2024

johnstcn added 8 commits June 6, 2024 14:55

remove autofocus from optional name field

64af358

make fmt

31313bc

fix typo in login args

2805571

fix ConvertUserRows

a62a8ee

reuse FirstUserParams in login_test

ec1befd

fix password reprompt test

e80f4c8

add test to ensure name is optional

c3849be

update golden files

7cf406d

johnstcn requested review from mafredri, dannykopping and BrunoQuaresma June 10, 2024 08:44

dannykopping approved these changes Jun 10, 2024

View reviewed changes

cli/login.go Outdated Show resolved Hide resolved

johnstcn added 2 commits June 14, 2024 09:28

s/name/full_name

ca2bafa

ignore nestif linter complaint

3eadba7

BrunoQuaresma reviewed Jun 14, 2024

View reviewed changes

mafredri approved these changes Jun 20, 2024

View reviewed changes

johnstcn closed this Jun 25, 2024

github-actions bot locked and limited conversation to collaborators Jun 25, 2024

github-actions bot deleted the cj/fullname-ui-setup branch December 15, 2024 00:07

feat: add name field to setup screen + CLI #13491

feat: add name field to setup screen + CLI #13491

Uh oh!

Conversation

johnstcn commented Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dannykopping left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BrunoQuaresma commented Jun 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrunoQuaresma left a comment

Choose a reason for hiding this comment

Uh oh!

johnstcn commented Jun 14, 2024

Uh oh!

johnstcn commented Jun 14, 2024

Uh oh!

BrunoQuaresma commented Jun 14, 2024

Uh oh!

mafredri left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnstcn commented Jun 25, 2024

Uh oh!

Uh oh!

johnstcn commented Jun 6, 2024 •

edited

Loading

mafredri left a comment •

edited

Loading