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

fix: persisted user display name #1425

Merged
merged 2 commits into from
Oct 17, 2024
Merged

Conversation

willydouhard
Copy link
Collaborator

No description provided.

@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. auth Pertaining to authentication. backend Pertains to the Python backend. labels Oct 10, 2024
Copy link
Collaborator

@dokterbob dokterbob left a comment

Choose a reason for hiding this comment

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

Isn't creating a persisted user from a user the data layer's job?

In that case, I would expect this code to live in the data layer, e.g. in

return PersistedUser(
and the likes.

Similarly, it seems to be we should want to persist it create_user().

Overall, I don't think this is part of authenticating a user.

Or do I not understand this well?

backend/chainlit/auth.py Show resolved Hide resolved
@willydouhard
Copy link
Collaborator Author

Isn't creating a persisted user from a user the data layer's job?

In that case, I would expect this code to live in the data layer, e.g. in

return PersistedUser(

and the likes.
Similarly, it seems to be we should want to persist it create_user().

Overall, I don't think this is part of authenticating a user.

Or do I not understand this well?

The display_name should never be sent to the backend. The use case is to have unidentifiable users in the backend but on the UI the real name is displayed

@dokterbob
Copy link
Collaborator

The display_name should never be sent to the backend. The use case is to have unidentifiable users in the backend but on the UI the real name is displayed

This seems to me a very uncommon use case for a field called display_name. The field name is very commonly used and it usually is the username as it is rendered, like a nickname.

Literally all I could find out in the code about it is "The User class now has a display_name field. It will not be persisted by the data layer."
Which makes adding it explicitly to a PersistedUser class quite confusing (at least to me!)!

My point is that if this feature is already confusing to a fellow maintainer, devs will struggle even more using it. Getting clarity on the naming on the feature and documenting it's use and use case would resolve it.

Could you please:

  1. Explain where and how this is used.
  2. Refer to a PR or page in the docs where I can read about it?
  3. Change the name into something with less overlap with pre-existing expectations?

@dokterbob dokterbob merged commit 1777f49 into main Oct 17, 2024
16 checks passed
@dokterbob dokterbob deleted the willy/fix-persisted-user-display-name branch October 17, 2024 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth Pertaining to authentication. backend Pertains to the Python backend. size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants