Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ee): add name field for user model, add api to update it #2100

Merged
merged 10 commits into from
May 16, 2024

Conversation

darknight
Copy link
Contributor

For closing #2054

  • add new migration to alter users table

    • name could be NULL for back compatibility
  • update UserDAO to add name field, add update_user_name to execute SQL UPDATE

  • implement new endpoint update_user_name for AuthenticationService

  • implement new mutation endpoint update_user_name

  • run make update-db-schema to update ee/tabby-db/schema/schema.sql & ee/tabby-db/schema/schema.svg

  • run tabby-schema/examples/update-schema.rs to update schema.graphql file

  • udpate README

@darknight
Copy link
Contributor Author

Manual test result:

Screenshot 2024-05-12 161217 Screenshot 2024-05-12 161235

@darknight
Copy link
Contributor Author

autofix failed due to

error[E0601]: `main` function not found in crate `tabby`

cargo_bloat failed due to

Resource not accessible by integration

Test coverage failed due to:

There was an error running the uploader: Error uploading to [https://codecov.io:](https://codecov.io/) Error: There was an error fetching the storage URL during POST: 429 - {'detail': ErrorDetail(string='Rate limit reached. Please upload with the Codecov repository upload token to resolve issue. Expected time to availability: 678s.', code='throttled')}

All of errors seem irrelevant to my change

@darknight
Copy link
Contributor Author

code coverage report Rate limit reached error, and cargo bloat report Resource not accessible by integration. I suspect this is due to external pull request from forked repository, but not very sure about it.

Copy link
Member

@wsxiaoys wsxiaoys left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM.

As a followup, please fill user.name field when user registered from oauth identity providers.

@wsxiaoys wsxiaoys marked this pull request as draft May 14, 2024 02:51
@darknight
Copy link
Contributor Author

Otherwise LGTM.

As a followup, please fill user.name field when user registered from oauth identity providers.

Correct me if I'm wrong, currently when user register from oauth, we only fetch user email info then create user accordingly.
Now we also want to fetch username info, and call update_user_name during the register process?

One more concern is, in this PR I didn't put UNIQUE constraint on the new name field, just like email or auth_token, this is more like product decision, may need your suggestion. cc: @wsxiaoys

@wsxiaoys wsxiaoys marked this pull request as ready for review May 15, 2024 16:56
Copy link
Member

@wsxiaoys wsxiaoys left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

ee/tabby-schema/src/schema/auth.rs Outdated Show resolved Hide resolved
ee/tabby-schema/src/schema/auth.rs Outdated Show resolved Hide resolved
#[validate(regex(
code = "username",
path = "crate::schema::constants::USERNAME_REGEX",
message = "Invalid username, only alphabet and space character are allowed, and it must start and end with alphabet"
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
message = "Invalid username, only alphabet and space character are allowed, and it must start and end with alphabet"
message = "Invalid name, only alphabet and space character are allowed, and it must start and end with alphabet"

Is non-english characters, e.j german / chinese valid name with this regex?

ee/tabby-schema/src/schema/auth.rs Outdated Show resolved Hide resolved
ee/tabby-schema/src/schema/auth.rs Outdated Show resolved Hide resolved
ee/tabby-schema/src/schema/auth.rs Outdated Show resolved Hide resolved
@wsxiaoys
Copy link
Member

Please also rebase against main

@wsxiaoys wsxiaoys enabled auto-merge (squash) May 16, 2024 16:11
@wsxiaoys wsxiaoys disabled auto-merge May 16, 2024 16:38
@wsxiaoys wsxiaoys merged commit b040d22 into TabbyML:main May 16, 2024
3 of 5 checks passed
@darknight darknight deleted the tab-599-user-name branch May 18, 2024 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants