-
Notifications
You must be signed in to change notification settings - Fork 10.6k
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
Use real name instead of username for messages and direct messages list #3851
Use real name instead of username for messages and direct messages list #3851
Conversation
I can not speak for the rest of the team, but I believe the direct message icon should stay an ampersand sign |
@graywolf336: I think I actually agree with you about the I think I will add an option to turn this on and off, as it is quite a big change, and as you say, not everyone will want it. In terms of performance, there is no impact on speed, there is a small storage impact, as the username as well as the name is stored within the user object for messages. I feel that this impact is relatively small, and I opted for this rather than a speed impact (i.e. making more mongo queries). This also appears to be the technique recommended by mongodb, which is to store the data as you are going to use it, rather than make more requests. |
@alexbrazier if you move back to |
@rodrigok I will try to make those changes in the next few days. One of the tasks I set was to create a database migration to add the name to the database for old messages ... should I still do that, or do you think it is ok for old messages to show the username and the new ones to show the full name? |
@alexbrazier I prefer to have the migration. |
@rodrigok I might need some help with the migration. I need to update all messages to add the name field to the user object, and I need to update the subscriptions for direct messages and set the other users name as the To do this we need to request all users and make the updates. I imagine this would be quite a lot of info to request all at once so it would have to be limited. |
I think this is done, except for the migration. @rodrigok Could I get some input on the database migration as for a large server it will have to get all the user full names and usernames and add them to the messages. Is there a good way of doing this? |
@@ -95,6 +94,10 @@ const CROWD = class CROWD { | |||
active: crowdUser.active | |||
}; | |||
|
|||
if (crowdUser.displayname) { | |||
RocketChat._setRealName(user._id, crowdUser.displayname); |
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.
user._id
is not defined right?
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.
Changed to id
which is defined in the function params
@@ -0,0 +1,24 @@ | |||
RocketChat._setRealName = (userId, 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.
Can we convert to JS?
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.
And, why not inside the user's model?
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.
It's how setUsername
is done, so I just copied that. I think its because it has to make more requests to other models, so it's not self contained within one model.
@@ -64,8 +64,7 @@ RocketChat.roomTypes.add('d', 20, { | |||
}, | |||
|
|||
roomName(roomData) { | |||
const room = ChatSubscription.findOne({ rid: roomData._id }, { fields: { name: 1 } }); | |||
return room && room.name; | |||
return ChatSubscription.findOne({ rid: roomData._id }, { fields: { name: 1, fname: 1 } }); |
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.
What happens when room is undefined?
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.
roomData
or the returned result of ChatSubscription
? Is there a case where it might be undefined?
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.
Probably will not happen
@@ -0,0 +1,170 @@ | |||
Template.membersList.helpers |
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.
This file was converted to JS, can you delete?
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.
Deleted
@@ -23,7 +23,10 @@ Meteor.methods({ | |||
}; | |||
|
|||
const map = (record) => { | |||
return record._user.username; | |||
return { |
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.
This is an API change, can you describe on HISTORY.md that will change from an array of strings to an array of objects?
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.
Done
@@ -0,0 +1,72 @@ | |||
Template.accountBox.helpers |
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.
This file was converted, can you delete?
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.
Done
@@ -0,0 +1,125 @@ | |||
Template.chatRoomItem.helpers |
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.
This file was converted, can you delete?
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.
Done
What about users without name? We have more than 500 users without name in our demo. Almost all came from linkedin OAuth, we need to fix that |
}, | ||
displayName() { | ||
if (RocketChat.settings.get('UI_Use_Real_Name')) { | ||
return this.user.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.
Fallback to username if name doesn't exist
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.
Done
@@ -11,8 +11,8 @@ | |||
{{> avatar username=username}} | |||
</div> | |||
<div class="info"> | |||
<h3 title="{{username}}"><i class="status-{{status}}"></i> {{username}}</h3> | |||
<p class="secondary-font-color">{{name}}</p> | |||
<h3 title="{{name}}"><i class="status-{{status}}"></i> {{name}}</h3> |
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.
Handle case where no 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.
Handled in js (if no name then returns Unnamed
)
@@ -7,7 +7,7 @@ | |||
{{> avatar username=username}} | |||
</div> | |||
<div class="data"> | |||
<h4>{{username}}</h4> | |||
<h4 data-username="{{username}}">{{fname}}</h4> |
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.
Handle case when no name set
@rodrigok Thanks for reviewing. I didn't realise that you could register without a name. As it is currently I think it would just set the name field in the messages to |
@alexbrazier In this particular case it's a bug, but you can turn off the name requirement for registration. |
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.
@rodrigok I have made the changes requested. Is there anything else that needs to be done?
@@ -0,0 +1,24 @@ | |||
RocketChat._setRealName = (userId, 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.
It's how setUsername
is done, so I just copied that. I think its because it has to make more requests to other models, so it's not self contained within one model.
@@ -95,6 +94,10 @@ const CROWD = class CROWD { | |||
active: crowdUser.active | |||
}; | |||
|
|||
if (crowdUser.displayname) { | |||
RocketChat._setRealName(user._id, crowdUser.displayname); |
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.
Changed to id
which is defined in the function params
@@ -0,0 +1,170 @@ | |||
Template.membersList.helpers |
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.
Deleted
}, | ||
displayName() { | ||
if (RocketChat.settings.get('UI_Use_Real_Name')) { | ||
return this.user.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.
Done
@@ -11,8 +11,8 @@ | |||
{{> avatar username=username}} | |||
</div> | |||
<div class="info"> | |||
<h3 title="{{username}}"><i class="status-{{status}}"></i> {{username}}</h3> | |||
<p class="secondary-font-color">{{name}}</p> | |||
<h3 title="{{name}}"><i class="status-{{status}}"></i> {{name}}</h3> |
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.
Handled in js (if no name then returns Unnamed
)
@@ -0,0 +1,72 @@ | |||
Template.accountBox.helpers |
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.
Done
@@ -0,0 +1,125 @@ | |||
Template.chatRoomItem.helpers |
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.
Done
@@ -23,7 +23,10 @@ Meteor.methods({ | |||
}; | |||
|
|||
const map = (record) => { | |||
return record._user.username; | |||
return { |
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.
Done
@@ -64,8 +64,7 @@ RocketChat.roomTypes.add('d', 20, { | |||
}, | |||
|
|||
roomName(roomData) { | |||
const room = ChatSubscription.findOne({ rid: roomData._id }, { fields: { name: 1 } }); | |||
return room && room.name; | |||
return ChatSubscription.findOne({ rid: roomData._id }, { fields: { name: 1, fname: 1 } }); |
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.
roomData
or the returned result of ChatSubscription
? Is there a case where it might be undefined?
Awesome, thanks a lot! Looking very much forward for this feature to be shipped :) @alexbrazier, great job! |
Thanks guys!!! |
Huge thank you. Awaiting this for a long time. |
Seriously! Massive thanks!!!!! |
Hi, I'm new to the Github interface but I can see that the last action on this PR was a year ago, yet it is still marked as "ready to be merged" in the labels and I still don't see names displayed in the conversations and lists on Rocket Chat (only usernames). Anyone cares to tell me what's going on? |
@pH142857 Hi. Usernames shows in conversation. That pull request was already merged. |
@pH142857 Have you set "Use Real Name" to "True" in the "User Interface" section of the Layout Admin settings? |
Oh I thought it was supposed to be the default behaviour, and I also thought there would be another label than "ready to be merge" to signify the merge. I checked the option and it now works, thanks to both of you, that was quick ! :) |
@RocketChat/core
Closes #1209
Closes #3453
Closes #3649
Use real name in most places, instead of username
@
iconChanges