Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

Closed
johnstcn wants to merge11 commits intomainfromcj/fullname-ui-setup

Conversation

@johnstcn
Copy link
Member

@johnstcnjohnstcn commentedJun 6, 2024•
edited
Loading

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.

@johnstcnjohnstcn self-assigned thisJun 6, 2024
Copy link
Contributor

@dannykoppingdannykopping left a 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

@BrunoQuaresma
Copy link
Contributor

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.

Specifies an email address to use if creating the first user for the
deployment.

--first-user-full-name string, $CODER_FIRST_USER_FULL_NAME
Copy link
Contributor

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.

Copy link
MemberAuthor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see.... 👍

}

//nolint:gocritic // Needed to create first user.
if_,err:=api.Database.UpdateUserProfile(dbauthz.AsSystemRestricted(ctx), database.UpdateUserProfileParams{
Copy link
Contributor

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.

Copy link
MemberAuthor

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

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.

typeCreateFirstUserRequeststruct {
Emailstring`json:"email" validate:"required,email"`
Usernamestring`json:"username" validate:"required,username"`
FullNamestring`json:"name" validate:"user_real_name"`
Copy link
Contributor

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

Copy link
MemberAuthor

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

BrunoQuaresma reacted with thumbs up emoji
Copy link
Member

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:

  1. Reading in "full name" (cli)
  2. Serializing as "name" over the wire (JSON)
  3. Deserializing as "full name"
  4. Writing "name" to database 😅

onBlur={(e)=>{
e.target.value=e.target.value.trim();
form.handleChange(e);
}}
Copy link
Contributor

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?

Copy link
MemberAuthor

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.

Copy link
Contributor

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.

Copy link
MemberAuthor

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.

Copy link
Contributor

@BrunoQuaresmaBrunoQuaresma left a 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?

@johnstcn
Copy link
MemberAuthor

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

It's entirely optional.

@johnstcn
Copy link
MemberAuthor

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
Copy link
Contributor

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 👍

Copy link
Member

@mafredrimafredri left a comment•
edited
Loading

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.

Comment on lines +66 to +71
iferrors.Is(err,cliui.Canceled) {
return"",nil
}
iferr!=nil {
return"",err
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.

typeCreateFirstUserRequeststruct {
Emailstring`json:"email" validate:"required,email"`
Usernamestring`json:"username" validate:"required,username"`
FullNamestring`json:"name" validate:"user_real_name"`
Copy link
Member

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:

  1. Reading in "full name" (cli)
  2. Serializing as "name" over the wire (JSON)
  3. Deserializing as "full name"
  4. Writing "name" to database 😅

@johnstcn
Copy link
MemberAuthor

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

@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJun 25, 2024
@github-actionsgithub-actionsbot deleted the cj/fullname-ui-setup branchDecember 15, 2024 00:07
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@mafredrimafredrimafredri approved these changes

@dannykoppingdannykoppingdannykopping approved these changes

+1 more reviewer

@BrunoQuaresmaBrunoQuaresmaBrunoQuaresma left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@johnstcnjohnstcn

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@johnstcn@BrunoQuaresma@mafredri@dannykopping
[8]ページ先頭
©2009-2025 Movatter.jp