From 2663cb86ae09b423195d4f11c8a8ec18ed4857e2 Mon Sep 17 00:00:00 2001 From: marstall Date: Thu, 23 Jan 2025 12:26:59 -0500 Subject: [PATCH] FEATURE: add new hidden site setting to show full names in user card adds a hidden site setting, "prioritize_full_names_in_ux", whose effect is to prefer full names in user-menu notifications Co-authored-by: Mark VanLandingham Co-authored-by: Alan Guo Xiang Tan --- .../app/components/user-menu/messages-list.js | 11 ++- .../app/lib/notification-types/base.js | 6 +- .../app/lib/user-menu/bookmark-item.js | 6 +- .../app/lib/user-menu/message-item.js | 6 +- .../tests/fixtures/notification-fixtures.js | 1 + .../discourse/tests/fixtures/user-menu.js | 3 +- .../unit/lib/notification-types/liked-test.js | 3 +- app/controllers/notifications_controller.rb | 8 +- app/controllers/users_controller.rb | 10 +- app/models/notification.rb | 3 + app/serializers/notification_serializer.rb | 11 ++- config/site_settings.yml | 4 + spec/models/notification_spec.rb | 1 + .../page_objects/components/user_menu.rb | 24 +++++ spec/system/viewing_user_menu_spec.rb | 95 +++++++++++++++++++ 15 files changed, 172 insertions(+), 20 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/user-menu/messages-list.js b/app/assets/javascripts/discourse/app/components/user-menu/messages-list.js index dd62b95cdea..a64c74d0a69 100644 --- a/app/assets/javascripts/discourse/app/components/user-menu/messages-list.js +++ b/app/assets/javascripts/discourse/app/components/user-menu/messages-list.js @@ -77,13 +77,16 @@ export default class UserMenuMessagesList extends UserMenuNotificationsList { const topics = data.topics.map((t) => this.store.createRecord("topic", t)); await Topic.applyTransformations(topics); - if (this.siteSettings.show_user_menu_avatars) { + if ( + this.siteSettings.show_user_menu_avatars || + this.siteSettings.prioritize_full_name_in_ux + ) { // Populate avatar_template for lastPoster const usersById = new Map(data.users.map((u) => [u.id, u])); topics.forEach((t) => { - t.last_poster_avatar_template = usersById.get( - t.lastPoster.user_id - )?.avatar_template; + const lastPoster = usersById.get(t.lastPoster.user_id); + t.last_poster_avatar_template = lastPoster?.avatar_template; + t.last_poster_name = lastPoster?.name; }); } diff --git a/app/assets/javascripts/discourse/app/lib/notification-types/base.js b/app/assets/javascripts/discourse/app/lib/notification-types/base.js index a4e676ab88d..92b4c5145c9 100644 --- a/app/assets/javascripts/discourse/app/lib/notification-types/base.js +++ b/app/assets/javascripts/discourse/app/lib/notification-types/base.js @@ -73,7 +73,11 @@ export default class NotificationTypeBase { * @returns {string} The label is the first part of the text content displayed in the notification. For example, in a like notification, the username of the user who liked the post is the label. If a falsey value is returned, the label is omitted. */ get label() { - return this.username; + if (!this.siteSettings.prioritize_full_name_in_ux) { + return this.username; + } + + return this.notification.acting_user_name || this.username; } /** diff --git a/app/assets/javascripts/discourse/app/lib/user-menu/bookmark-item.js b/app/assets/javascripts/discourse/app/lib/user-menu/bookmark-item.js index bb0401f38b5..5a69a4f7b17 100644 --- a/app/assets/javascripts/discourse/app/lib/user-menu/bookmark-item.js +++ b/app/assets/javascripts/discourse/app/lib/user-menu/bookmark-item.js @@ -24,7 +24,11 @@ export default class UserMenuBookmarkItem extends UserMenuBaseItem { } get label() { - return this.bookmark.user?.username; + if (!this.siteSettings.prioritize_full_name_in_ux) { + return this.bookmark.user?.username; + } + + return this.bookmark.user?.name || this.bookmark.user?.username; } get description() { diff --git a/app/assets/javascripts/discourse/app/lib/user-menu/message-item.js b/app/assets/javascripts/discourse/app/lib/user-menu/message-item.js index 1ce55fca75f..1bc39521797 100644 --- a/app/assets/javascripts/discourse/app/lib/user-menu/message-item.js +++ b/app/assets/javascripts/discourse/app/lib/user-menu/message-item.js @@ -31,7 +31,11 @@ export default class UserMenuMessageItem extends UserMenuBaseItem { } get label() { - return this.message.last_poster_username; + if (!this.siteSettings.prioritize_full_name_in_ux) { + return this.message.last_poster_username; + } + + return this.message.last_poster_name || this.message.last_poster_username; } get description() { diff --git a/app/assets/javascripts/discourse/tests/fixtures/notification-fixtures.js b/app/assets/javascripts/discourse/tests/fixtures/notification-fixtures.js index 9122200e311..e8f50ff6d3f 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/notification-fixtures.js +++ b/app/assets/javascripts/discourse/tests/fixtures/notification-fixtures.js @@ -7,6 +7,7 @@ export default { id: 5340, notification_type: NOTIFICATION_TYPES.edited, acting_user_avatar_template: "/letter_avatar_proxy/v4/letter/o/f05b48/{size}.png", + acting_user_name: "Veles In", read: false, post_number: 1, topic_id: 130, diff --git a/app/assets/javascripts/discourse/tests/fixtures/user-menu.js b/app/assets/javascripts/discourse/tests/fixtures/user-menu.js index afe75d0914b..d5f0f97544c 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/user-menu.js +++ b/app/assets/javascripts/discourse/tests/fixtures/user-menu.js @@ -57,7 +57,7 @@ export default { user: { id: 1, username: "osama", - name: "Osama", + name: "Osama S", avatar_template: "/letter_avatar_proxy/v4/letter/o/f05b48/{size}.png", }, @@ -143,6 +143,7 @@ export default { read_notifications: [], users: [{id: 13, username: "mixtape", + name: "Mix Tape", avatar_template: "/letter_avatar_proxy/v4/letter/o/f05b48/{size}.png" }] } diff --git a/app/assets/javascripts/discourse/tests/unit/lib/notification-types/liked-test.js b/app/assets/javascripts/discourse/tests/unit/lib/notification-types/liked-test.js index eef0ab5ecd0..fb9d73d18ae 100644 --- a/app/assets/javascripts/discourse/tests/unit/lib/notification-types/liked-test.js +++ b/app/assets/javascripts/discourse/tests/unit/lib/notification-types/liked-test.js @@ -1,3 +1,4 @@ +import { getOwner } from "@ember/application"; import { setupTest } from "ember-qunit"; import { module, test } from "qunit"; import { deepMerge } from "discourse/lib/object"; @@ -41,7 +42,7 @@ module("Unit | Notification Types | liked", function (hooks) { const director = createRenderDirector( notification, "liked", - this.siteSettings + getOwner(this).lookup("service:site-settings") ); notification.data.count = 2; assert.strictEqual( diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index e8e0d111699..ac32ac61c73 100644 --- a/app/controllers/notifications_controller.rb +++ b/app/controllers/notifications_controller.rb @@ -57,8 +57,8 @@ class NotificationsController < ApplicationController notifications = Notification.filter_inaccessible_topic_notifications(current_user.guardian, notifications) - notifications = - Notification.populate_acting_user(notifications) if SiteSetting.show_user_menu_avatars + + notifications = Notification.populate_acting_user(notifications) json = { notifications: serialize_data(notifications, NotificationSerializer), @@ -88,8 +88,8 @@ class NotificationsController < ApplicationController notifications = notifications.offset(offset).limit(limit) notifications = Notification.filter_inaccessible_topic_notifications(current_user.guardian, notifications) - notifications = - Notification.populate_acting_user(notifications) if SiteSetting.show_user_menu_avatars + + notifications = Notification.populate_acting_user(notifications) render_json_dump( notifications: serialize_data(notifications, NotificationSerializer), total_rows_notifications: total_rows, diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 2c970da1dc4..407b95b8dd5 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1927,9 +1927,7 @@ class UsersController < ApplicationController end if reminder_notifications.present? - if SiteSetting.show_user_menu_avatars - Notification.populate_acting_user(reminder_notifications) - end + Notification.populate_acting_user(reminder_notifications) serialized_notifications = ActiveModel::ArraySerializer.new( reminder_notifications, @@ -2000,7 +1998,7 @@ class UsersController < ApplicationController end if unread_notifications.present? - Notification.populate_acting_user(unread_notifications) if SiteSetting.show_user_menu_avatars + Notification.populate_acting_user(unread_notifications) serialized_unread_notifications = ActiveModel::ArraySerializer.new( unread_notifications, @@ -2013,7 +2011,7 @@ class UsersController < ApplicationController serialized_messages = serialize_data(messages_list, TopicListSerializer, scope: guardian, root: false)[:topics] serialized_users = - if SiteSetting.show_user_menu_avatars + if SiteSetting.show_user_menu_avatars || !SiteSetting.prioritize_username_in_ux users = messages_list.topics.map { |t| t.posters.last.user }.flatten.compact.uniq(&:id) serialize_data(users, BasicUserSerializer, scope: guardian, root: false) else @@ -2022,7 +2020,7 @@ class UsersController < ApplicationController end if read_notifications.present? - Notification.populate_acting_user(read_notifications) if SiteSetting.show_user_menu_avatars + Notification.populate_acting_user(read_notifications) serialized_read_notifications = ActiveModel::ArraySerializer.new( read_notifications, diff --git a/app/models/notification.rb b/app/models/notification.rb index 15b45662eb7..a7494e8bacc 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -365,6 +365,9 @@ class Notification < ActiveRecord::Base end def self.populate_acting_user(notifications) + if !(SiteSetting.show_user_menu_avatars || SiteSetting.prioritize_full_name_in_ux) + return notifications + end usernames = notifications.map do |notification| notification.acting_username = diff --git a/app/serializers/notification_serializer.rb b/app/serializers/notification_serializer.rb index 486ccca078b..546cbe4b63b 100644 --- a/app/serializers/notification_serializer.rb +++ b/app/serializers/notification_serializer.rb @@ -14,7 +14,8 @@ class NotificationSerializer < ApplicationSerializer :slug, :data, :is_warning, - :acting_user_avatar_template + :acting_user_avatar_template, + :acting_user_name def slug Slug.for(object.topic.title) if object.topic.present? @@ -55,4 +56,12 @@ class NotificationSerializer < ApplicationSerializer def include_acting_user_avatar_template? object.acting_user.present? end + + def acting_user_name + object.acting_user.name + end + + def include_acting_user_name? + object.acting_user.present? + end end diff --git a/config/site_settings.yml b/config/site_settings.yml index a8904196ebe..95a25e390af 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -759,6 +759,10 @@ users: prioritize_username_in_ux: client: true default: true + prioritize_full_name_in_ux: + client: true + default: false + hidden: true email_token_valid_hours: default: 48 min: 1 diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index 06fe1837f9d..d84798c76dc 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -837,6 +837,7 @@ RSpec.describe Notification do end it "Sets the acting_user correctly for each notification" do + SiteSetting.prioritize_full_name_in_ux = true Notification.populate_acting_user( [notification1, notification2, notification3, notification4, notification5], ) diff --git a/spec/system/page_objects/components/user_menu.rb b/spec/system/page_objects/components/user_menu.rb index 0591bb12877..3fe29c73448 100644 --- a/spec/system/page_objects/components/user_menu.rb +++ b/spec/system/page_objects/components/user_menu.rb @@ -51,6 +51,30 @@ module PageObjects ) end + def has_user_full_name_mentioned_notification?(topic, user_that_mentioned) + expect(find("#quick-access-replies .mentioned").text).to eq( + "#{user_that_mentioned.name} #{topic.title}", + ) + end + + def has_user_full_name_messaged_notification?(post, user) + expect(find("#quick-access-all-notifications .private-message").text).to eq( + "#{user.name} #{post.topic.title}", + ) + end + + def has_user_full_name_bookmarked_notification?(post, user) + expect(find("#quick-access-bookmarks .bookmark").text).to eq( + "#{user.name} #{post.topic.title}", + ) + end + + def has_user_username_mentioned_notification?(topic, user_that_mentioned) + expect(find("#quick-access-replies .mentioned").text).to eq( + "#{user_that_mentioned.username} #{topic.title}", + ) + end + def has_right_replies_button_count?(count) expect(find("#user-menu-button-replies").text).to eq(count.to_s) end diff --git a/spec/system/viewing_user_menu_spec.rb b/spec/system/viewing_user_menu_spec.rb index 0b6b63a85eb..e69f599b030 100644 --- a/spec/system/viewing_user_menu_spec.rb +++ b/spec/system/viewing_user_menu_spec.rb @@ -48,5 +48,100 @@ RSpec.describe "Viewing User Menu", system: true do expect(user_menu).to have_group_mentioned_notification(topic, user, mentionable_group) end + + context "with SiteSetting.prioritize_full_name_in_ux=true" do + before { SiteSetting.prioritize_full_name_in_ux = true } + + it "should display user full name in mention notifications" do + Jobs.run_immediately! + + user = Fabricate(:user) + user2 = Fabricate(:user, name: "John Doe") + PostCreator.create!(user, topic_id: topic.id, raw: "Hello @#{user2.username}") + + sign_in(user2) + + visit("/latest") + + user_menu.open + + expect(user_menu).to have_right_replies_button_count(1) + + user_menu.click_replies_notifications_tab + + expect(user_menu).to have_user_full_name_mentioned_notification(topic, user) + end + + it "should display user full name in message notification" do + Jobs.run_immediately! + + user = Fabricate(:moderator) + user2 = Fabricate(:user, name: "John Doe") + post = + PostCreator.create!( + user, + title: "message", + raw: "private message", + archetype: Archetype.private_message, + target_usernames: [user2.username], + ) + + sign_in(user2) + + visit("/latest") + user_menu.open + + expect(user_menu).to have_user_full_name_messaged_notification(post, user) + end + + it "should display user full name in bookmark notification" do + Jobs.run_immediately! + + user = Fabricate(:moderator) + user2 = Fabricate(:user, name: "John Doe") + post = + PostCreator.create!( + user, + title: "message in a bottle", + raw: "private message", + archetype: Archetype.private_message, + target_usernames: [user2.username], + ) + Bookmark.create!(bookmarkable: post, user: user2) + sign_in(user2) + + visit("/latest") + user_menu.open + user_menu.click_bookmarks_tab + expect(user_menu).to have_user_full_name_bookmarked_notification(post, user) + end + end + + context "with SiteSetting.prioritize_full_name_in_ux=false" do + before { SiteSetting.prioritize_full_name_in_ux = false } + + it "should display only username in mention notifications" do + Jobs.run_immediately! + + SiteSetting.prioritize_username_in_ux = true + + user = Fabricate(:user) + user2 = Fabricate(:user, name: "John Doe") + + PostCreator.create!(user, topic_id: topic.id, raw: "Hello @#{user2.username}") + + sign_in(user2) + + visit("/latest") + + user_menu.open + + expect(user_menu).to have_right_replies_button_count(1) + + user_menu.click_replies_notifications_tab + + expect(user_menu).to have_user_username_mentioned_notification(topic, user) + end + end end end