From ec8a785df93ea444808bd998266db1402e3871a2 Mon Sep 17 00:00:00 2001 From: benj <25828824+brrusselburg@users.noreply.github.com> Date: Fri, 14 Mar 2025 18:08:28 -0500 Subject: [PATCH] FIX: Ensure assign plugin respects `prioritize_full_name_in_ux` site setting (#632) * WIP * more messing around * undos, notes to self * WIP notes * completed logic and added tests for existing code and implemented code * fixed failing tests * fixed failing tests * added suggested changes * added suggested changes * fix merge * fix linting issues * possible flaky test * removed comment * further user name display changes * implemented suggested change * fixed bug in small action post when assigning or unassigning * fixed breaking tests * refactored test --------- Co-authored-by: Juan David Martinez --- .../discourse/components/assigned-to-post.gjs | 11 +- .../components/topic-level-assign-menu.js | 16 +- .../initializers/extend-for-assigns.js | 24 ++- .../discourse/models/assignment.js | 2 + config/locales/client.en.yml | 2 +- lib/assigner.rb | 20 +- .../serializers/topic_view_serializer_spec.rb | 1 + spec/system/assign_post_spec.rb | 188 ++++++++++++++++++ spec/system/assign_topic_spec.rb | 26 +++ spec/system/page_objects/pages/topic.rb | 40 +++- 10 files changed, 313 insertions(+), 17 deletions(-) create mode 100644 spec/system/assign_post_spec.rb diff --git a/assets/javascripts/discourse/components/assigned-to-post.gjs b/assets/javascripts/discourse/components/assigned-to-post.gjs index 6af28c9..6c17144 100644 --- a/assets/javascripts/discourse/components/assigned-to-post.gjs +++ b/assets/javascripts/discourse/components/assigned-to-post.gjs @@ -9,6 +9,15 @@ import DMenu from "float-kit/components/d-menu"; export default class AssignedToPost extends Component { @service taskActions; + @service siteSettings; + + get nameOrUsername() { + if (this.siteSettings.prioritize_full_name_in_ux) { + return this.args.assignedToUser.name || this.args.assignedToUser.username; + } else { + return this.args.assignedToUser.username; + } + } @action unassign() { @@ -33,7 +42,7 @@ export default class AssignedToPost extends Component { {{#if @assignedToUser}} - {{@assignedToUser.username}} + {{this.nameOrUsername}} {{else}} {{@assignedToGroup.name}} {{/if}} diff --git a/assets/javascripts/discourse/components/topic-level-assign-menu.js b/assets/javascripts/discourse/components/topic-level-assign-menu.js index 4425c42..247b1e7 100644 --- a/assets/javascripts/discourse/components/topic-level-assign-menu.js +++ b/assets/javascripts/discourse/components/topic-level-assign-menu.js @@ -72,7 +72,12 @@ export default { const content = []; if (this.topic.isAssigned()) { - content.push(unassignFromTopicButton(this.topic)); + content.push( + unassignFromTopicButton( + this.topic, + this.siteSettings.prioritize_full_name_in_ux + ) + ); } if (this.topic.hasAssignedPosts()) { @@ -131,9 +136,14 @@ function reassignToSelfButton() { }; } -function unassignFromTopicButton(topic) { - const username = +function unassignFromTopicButton(topic, prioritize_full_name_in_ux) { + let username = topic.assigned_to_user?.username || topic.assigned_to_group?.name; + + if (topic.assigned_to_user && prioritize_full_name_in_ux) { + username = topic.assigned_to_user?.name || topic.assigned_to_user?.username; + } + const icon = topic.assigned_to_user ? avatarHtml(topic.assigned_to_user, "small") : iconHTML("user-xmark"); diff --git a/assets/javascripts/discourse/initializers/extend-for-assigns.js b/assets/javascripts/discourse/initializers/extend-for-assigns.js index 4715d23..9c9b23a 100644 --- a/assets/javascripts/discourse/initializers/extend-for-assigns.js +++ b/assets/javascripts/discourse/initializers/extend-for-assigns.js @@ -30,6 +30,7 @@ const DEPENDENT_KEYS = [ "topic.assigned_to_group", "currentUser.can_assign", "topic.assigned_to_user.username", + "topic.assigned_to_user.name", ]; function defaultTitle(topic) { @@ -481,7 +482,11 @@ function initialize(api) { } const icon = iconHTML(assignee.username ? "user-plus" : "group-plus"); - const name = assignee.username || assignee.name; + const name = + siteSettings.prioritize_full_name_in_ux || !assignee.username + ? assignee.name || assignee.username + : assignee.username; + const tagName = params.tagName || "a"; const href = tagName === "a" @@ -553,13 +558,18 @@ function initialize(api) { )}`; }; + let displayedName = ""; if (assignedToUser) { + displayedName = this.siteSettings.prioritize_full_name_in_ux + ? assignedToUser.name || assignedToUser.username + : assignedToUser.username; + assigneeElements.push( h( "span.assignee", new RawHtml({ html: assignedHtml( - assignedToUser.username, + displayedName, assignedToUserPath(assignedToUser), "user" ), @@ -581,10 +591,17 @@ function initialize(api) { ) ); } + if (indirectlyAssignedTo) { Object.keys(indirectlyAssignedTo).map((postId) => { const assignee = indirectlyAssignedTo[postId].assigned_to; const postNumber = indirectlyAssignedTo[postId].post_number; + + displayedName = + this.siteSettings.prioritize_full_name_in_ux || !assignee.username + ? assignee.name || assignee.username + : assignee.username; + assigneeElements.push( h("span.assignee", [ h( @@ -597,13 +614,14 @@ function initialize(api) { }, i18n("discourse_assign.assign_post_to_multiple", { post_number: postNumber, - username: assignee.username || assignee.name, + username: displayedName, }) ), ]) ); }); } + if (!isEmpty(assigneeElements)) { return h("p.assigned-to", [ assignedToUser ? iconNode("user-plus") : iconNode("group-plus"), diff --git a/assets/javascripts/discourse/models/assignment.js b/assets/javascripts/discourse/models/assignment.js index eb79234..67b0086 100644 --- a/assets/javascripts/discourse/models/assignment.js +++ b/assets/javascripts/discourse/models/assignment.js @@ -17,6 +17,7 @@ export class Assignment extends EmberObject { static fromPost(post) { const assignment = new Assignment(); assignment.username = post.assigned_to.username; + assignment.name = post.assigned_to.name; assignment.groupName = post.assigned_to.name; assignment.status = post.assignment_status; assignment.note = post.assignment_note; @@ -31,6 +32,7 @@ export class Assignment extends EmberObject { // and models from server, that's why we have to call it "group_name" now @tracked group_name; @tracked username; + @tracked name; @tracked status; @tracked note; targetId; diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 9f82fa2..2a9bfc8 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -100,7 +100,7 @@ en: assignable_levels: title: "Who can assign this group" user: - notification_level_when_assigned: + notification_level_when_assigned: label: "When assigned" watch_topic: "Watch topic" track_topic: "Track topic" diff --git a/lib/assigner.rb b/lib/assigner.rb index 7b2460a..71c05d8 100644 --- a/lib/assigner.rb +++ b/lib/assigner.rb @@ -405,9 +405,7 @@ class ::Assigner if SiteSetting.unassign_creates_tracking_post && !silent post_type = SiteSetting.assigns_public ? Post.types[:small_action] : Post.types[:whisper] - custom_fields = { - "action_code_who" => assigned_to.is_a?(User) ? assigned_to.username : assigned_to.name, - } + custom_fields = small_action_username_or_name(assigned_to) if post_target? custom_fields.merge!("action_code_path" => "/p/#{@target.id}") @@ -493,10 +491,20 @@ class ::Assigner Jobs.enqueue(:assign_notification, assignment_id: assignment.id) end + def small_action_username_or_name(assign_to) + if (assign_to.is_a?(User) && SiteSetting.prioritize_full_name_in_ux) || + !assign_to.try(:username) + custom_fields = { "action_code_who" => assign_to.name || assign_to.username } + else + custom_fields = { + "action_code_who" => assign_to.is_a?(User) ? assign_to.username : assign_to.name, + } + end + custom_fields + end + def add_small_action_post(action_code, assign_to, text) - custom_fields = { - "action_code_who" => assign_to.is_a?(User) ? assign_to.username : assign_to.name, - } + custom_fields = small_action_username_or_name(assign_to) if post_target? custom_fields.merge!( diff --git a/spec/serializers/topic_view_serializer_spec.rb b/spec/serializers/topic_view_serializer_spec.rb index 6669e12..cd8acf9 100644 --- a/spec/serializers/topic_view_serializer_spec.rb +++ b/spec/serializers/topic_view_serializer_spec.rb @@ -19,6 +19,7 @@ RSpec.describe TopicViewSerializer do it "includes assigned user in serializer" do Assigner.new(topic, user).assign(user) serializer = TopicViewSerializer.new(TopicView.new(topic), scope: guardian) + expect(serializer.as_json[:topic_view][:assigned_to_user][:name]).to eq(user.name) expect(serializer.as_json[:topic_view][:assigned_to_user][:username]).to eq(user.username) expect(serializer.as_json[:topic_view][:assigned_to_group]).to be nil end diff --git a/spec/system/assign_post_spec.rb b/spec/system/assign_post_spec.rb new file mode 100644 index 0000000..60c27eb --- /dev/null +++ b/spec/system/assign_post_spec.rb @@ -0,0 +1,188 @@ +# frozen_string_literal: true + +describe "Assign | Assigning posts", type: :system do + let(:topic_page) { PageObjects::Pages::Topic.new } + let(:assign_modal) { PageObjects::Modals::Assign.new } + fab!(:staff_user) { Fabricate(:user, groups: [Group[:staff]]) } + fab!(:admin) + fab!(:topic) + fab!(:post1) { Fabricate(:post, topic: topic) } + fab!(:post2) { Fabricate(:post, topic: topic) } + + before do + SiteSetting.assign_enabled = true + SiteSetting.prioritize_full_name_in_ux = false + + # The system tests in this file are flaky and auth token related so turning this on + SiteSetting.verbose_auth_token_logging = true + + sign_in(admin) + end + + def assign_post(post, assignee) + topic_page.click_assign_post(post) + assign_modal.assignee = assignee + assign_modal.confirm + end + + describe "with open topic" do + before { SiteSetting.prioritize_full_name_in_ux = false } + + it "can assign and unassign" do + visit "/t/#{topic.id}" + assign_post(post2, staff_user) + + expect(assign_modal).to be_closed + + expect(topic_page).to have_assigned_post(user: staff_user, at_post: 3) + + expect(topic_page.find_post_assign(post1.post_number)).to have_content(staff_user.username) + expect(topic_page.find_post_assign(post2.post_number)).to have_content(staff_user.username) + + visit "/t/#{topic.id}" + + topic_page.click_unassign_post(post2) + + expect(topic_page).to have_unassigned_from_post(user: staff_user, at_post: 4) + expect(page).to have_no_css("#topic .assigned-to") + end + + it "can submit modal form with shortcuts" do + visit "/t/#{topic.id}" + + topic_page.click_assign_post(post2) + assign_modal.assignee = staff_user + + find("body").send_keys(:tab) + find("body").send_keys(:control, :enter) + + expect(assign_modal).to be_closed + expect(topic_page).to have_assigned_post(user: staff_user, at_post: 3) + expect(topic_page.find_post_assign(post1.post_number)).to have_content(staff_user.username) + expect(topic_page.find_post_assign(post2.post_number)).to have_content(staff_user.username) + end + + context "when prioritize_full_name_in_ux setting is enabled" do + before { SiteSetting.prioritize_full_name_in_ux = true } + + it "shows the user's name after assign" do + visit "/t/#{topic.id}" + + assign_post(post2, staff_user) + + expect(topic_page.find_post_assign(post1.post_number)).to have_content(staff_user.name) + expect(topic_page.find_post_assign(post2.post_number)).to have_content(staff_user.name) + end + + it "show the user's username if there is no name" do + visit "/t/#{topic.id}" + staff_user.name = nil + staff_user.save + staff_user.reload + + assign_post(post2, staff_user) + + expect(topic_page.find_post_assign(post1.post_number)).to have_content(staff_user.username) + expect(topic_page.find_post_assign(post2.post_number)).to have_content(staff_user.username) + end + end + + context "when assigns are not public" do + before { SiteSetting.assigns_public = false } + + it "assigned small action post has 'private-assign' in class attribute" do + visit "/t/#{topic.id}" + + assign_post(post2, staff_user) + + expect(assign_modal).to be_closed + expect(topic_page).to have_assigned_post( + user: staff_user, + at_post: 3, + class_attribute: ".private-assign", + ) + end + end + + context "when unassign_on_close is set to true" do + before { SiteSetting.unassign_on_close = true } + + it "unassigns the topic on close" do + visit "/t/#{topic.id}" + + assign_post(post2, staff_user) + + expect(assign_modal).to be_closed + expect(topic_page).to have_assigned_post(user: staff_user, at_post: 3) + + find(".timeline-controls .toggle-admin-menu").click + find(".topic-admin-close").click + + expect(find("#post_4")).to have_content( + I18n.t("js.action_codes.closed.enabled", when: "just now"), + ) + expect(page).to have_no_css("#post_5") + expect(page).to have_no_css("#topic .assigned-to") + end + + it "can assign the previous assignee" do + visit "/t/#{topic.id}" + + assign_post(post2, staff_user) + + expect(assign_modal).to be_closed + expect(topic_page).to have_assigned_post(user: staff_user, at_post: 3) + + find(".timeline-controls .toggle-admin-menu").click + find(".topic-admin-close").click + + expect(find("#post_4")).to have_content( + I18n.t("js.action_codes.closed.enabled", when: "just now"), + ) + expect(page).to have_no_css("#post_5") + expect(page).to have_no_css("#topic .assigned-to") + + assign_post(post2, staff_user) + + expect(page).to have_no_css("#post_5") + expect(topic_page.find_post_assign(post1.post_number)).to have_content(staff_user.username) + expect(topic_page.find_post_assign(post2.post_number)).to have_content(staff_user.username) + end + + context "when reassign_on_open is set to true" do + before { SiteSetting.reassign_on_open = true } + + it "reassigns the topic on open" do + visit "/t/#{topic.id}" + + assign_post(post2, staff_user) + expect(assign_modal).to be_closed + expect(topic_page).to have_assigned_post(user: staff_user, at_post: 3) + + find(".timeline-controls .toggle-admin-menu").click + find(".topic-admin-close").click + + expect(find("#post_4")).to have_content( + I18n.t("js.action_codes.closed.enabled", when: "just now"), + ) + expect(page).to have_no_css("#post_5") + expect(page).to have_no_css("#topic .assigned-to") + + find(".timeline-controls .toggle-admin-menu").click + find(".topic-admin-open").click + + expect(find("#post_5")).to have_content( + I18n.t("js.action_codes.closed.disabled", when: "just now"), + ) + expect(page).to have_no_css("#post_6") + expect(topic_page.find_post_assign(post1.post_number)).to have_content( + staff_user.username, + ) + expect(topic_page.find_post_assign(post2.post_number)).to have_content( + staff_user.username, + ) + end + end + end + end +end diff --git a/spec/system/assign_topic_spec.rb b/spec/system/assign_topic_spec.rb index b30a5b7..b2fa6f0 100644 --- a/spec/system/assign_topic_spec.rb +++ b/spec/system/assign_topic_spec.rb @@ -10,6 +10,7 @@ describe "Assign | Assigning topics", type: :system do before do SiteSetting.assign_enabled = true + SiteSetting.prioritize_full_name_in_ux = false # The system tests in this file are flaky and auth token related so turning this on SiteSetting.verbose_auth_token_logging = true @@ -49,6 +50,31 @@ describe "Assign | Assigning topics", type: :system do expect(find("#topic .assigned-to")).to have_content(staff_user.username) end + context "when prioritize_full_name_in_ux setting is enabled" do + before { SiteSetting.prioritize_full_name_in_ux = true } + + it "shows the user's name after assign" do + visit "/t/#{topic.id}" + + topic_page.click_assign_topic + assign_modal.assignee = staff_user + assign_modal.confirm + expect(find("#topic .assigned-to")).to have_content(staff_user.name) + end + + it "show the user's username if there is no name" do + visit "/t/#{topic.id}" + staff_user.name = nil + staff_user.save + staff_user.reload + + topic_page.click_assign_topic + assign_modal.assignee = staff_user + assign_modal.confirm + expect(find("#topic .assigned-to")).to have_content(staff_user.name) + end + end + context "when assigns are not public" do before { SiteSetting.assigns_public = false } diff --git a/spec/system/page_objects/pages/topic.rb b/spec/system/page_objects/pages/topic.rb index fe70463..81f0a0c 100644 --- a/spec/system/page_objects/pages/topic.rb +++ b/spec/system/page_objects/pages/topic.rb @@ -12,28 +12,62 @@ module PageObjects find("[data-value='unassign']").click end + def click_unassign_post(post) + find("#topic-footer-dropdown-reassign").click + data_value = "unassign-from-post-#{post.id}" + find("[data-value=\"#{data_value}\"]").click + end + + def click_assign_post(post) + find_post_action_button(post, :show_more).click + assign_post = within_post(post) { find(".post-action-menu__assign-post") } + assign_post.click + end + def click_edit_topic_assignment find("#topic-footer-dropdown-reassign").click find("[data-value='reassign']").click end + def find_post_assign(post_number) + within("#post_#{post_number}") { find(".assigned-to") } + end + def has_assigned?(args) has_assignment_action?(action: "assigned", **args) end + def has_assigned_post?(args) + has_assignment_action?(action: "assigned_to_post", **args) + end + def has_unassigned?(args) has_assignment_action?(action: "unassigned", **args) end + def has_unassigned_from_post?(args) + has_assignment_action?(action: "unassigned_from_post", **args) + end + def has_assignment_action?(args) assignee = args[:group]&.name || args[:user]&.username container = args[:at_post] ? find("#post_#{args[:at_post]}#{args[:class_attribute] || ""}") : page - container.has_content?( - I18n.t("js.action_codes.#{args[:action]}", who: "@#{assignee}", when: "just now"), - ) + post_content = + I18n.t( + "js.action_codes.#{args[:action]}", + path: "", + who: "@#{assignee}", + when: "just now", + ) + + if args[:action] == "assigned_to_post" || args[:action] == "unassigned_from_post" + post_content.gsub!(%r{]*>(.*?)}, '\1') + end + + container.has_content?(post_content) end end end