fixed: Prefer display name of user on table UI#18408
fixed: Prefer display name of user on table UI#18408kenchan0130 wants to merge 1 commit intogrokability:developfrom
Conversation
… to use displayname
| return $asset->assigned ? [ | ||
| 'id' => (int) $asset->assigned->id, | ||
| 'username' => e($asset->assigned->username), | ||
| 'display_name' => e($asset->assigned->display_name), |
There was a problem hiding this comment.
Although the polymorphicItemFormatter method of JavaScript is also used in AccessoriesTransformer.php, the transformUserCompact function already passes display_name. Therefore, I determine that the change is only necessary in AssetsTransformer.php.
|
@kenchan0130 @snipe could it be that #18408 breaks names shown in 'recent activity' and 'Activity report'? Suddenly, on my test install, upgraded to the latest pull (v8.3.7-95-g542cdef0b), names are empty in dashboard and report, but only for LDAP Sync and only when users present in DB are updated. Newly created users are displayed correctly. (I don't sync Display Names with AD, it always was handled by Snipe-IT.) My other install (v8.3.7-84-g5ddb0b4a55) is not affected by this. EDIT: If 'Display Name' is manually filled in user profile, only then it is shown in the 'Activity report'. IMHO, taking 'Name' information instead of 'Display name' makes way more sense, because 'Name' format is controlled via 'Admin Settings > Localization', not entered manually or on LDAP Sync. |
|
@swift2512 I'm not seeing what you're seeing, unless I'm misunderstanding. 
|
|
@snipe this only applies to LDAP Sync. |
I understand your point, but my expectation is slightly different: https://github.com/grokability/snipe-it/blob/master/app/Models/User.php#L211-L216 to reflect this expected behavior.
When using LDAP Sync, does the display name field in the Snipe-IT database always have a value?
|
|
@kenchan0130 I agree that if Display Name is filled, then it should be used. But this shouldn't break other stuff. EDIT: Honestly, I don't understand what was the reason behind this fix. I've tested on my main system that wasn't updated to the latest. I've manually set 'Display name' to some value and this value is displayed in:
|
I’ve checked the latest build running on Docker locally, and everything seems to be working fine. |
|
Sorry, I've tried different backups, did some testing and find out that my problem is not related to this fix. It's just a bug (or a feature). |
| // display the username if it's checked out to a user, but don't do it if the username's there already | ||
| if (value.username && !value.name.match('\\(') && !value.name.match('\\)')) { | ||
| value.name = value.name + ' (' + value.username + ')'; | ||
| value.name = value.display_name + ' (' + value.username + ')'; |
There was a problem hiding this comment.
I think we'd need to check if display_name is set here. That formatter touches a lot of things.
There was a problem hiding this comment.
I understand that the ORM function deriving display_name is designed to return full name when display_name is empty.
As I mentioned in #18408 (comment), my understanding is that the recent change ensures the display_name attribute in the JSON will not be empty.
Is your concern that this formatter is (or will be) used outside of AccessoriesTransformer and AssetsTransformer as well?
Also, if a change is needed, are you expecting something like this?
if (value.username && !value.name.match('\\(') && !value.name.match('\\)')) {
const name = value.display_name ? value.display_name : value.username;
value.name = name + ' (' + value.username + ')';
}If this works for you, I can apply the change right away.
3 participants
In #18404, I've updated the name displayed in the UI, but I overlooked the parts rendered by JavaScript.
Therefore, in this change, I’ve ensured that elements rendered by JavaScript also prioritize the display name.