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 <juan@discourse.org>

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 {
 
     <a href={{@href}} class="assigned-to-username">
       {{#if @assignedToUser}}
-        {{@assignedToUser.username}}
+        {{this.nameOrUsername}}
       {{else}}
         {{@assignedToGroup.name}}
       {{/if}}

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) {
+function unassignFromTopicButton(topic, prioritize_full_name_in_ux) {
-  const username =
+  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");

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) {
         )}</span>`;
       };
 
+      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"),

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;

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 = {
+        custom_fields = small_action_username_or_name(assigned_to)
-          "action_code_who" => assigned_to.is_a?(User) ? assigned_to.username : assigned_to.name,
-        }
 
         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 = {
+    custom_fields = small_action_username_or_name(assign_to)
-      "action_code_who" => assign_to.is_a?(User) ? assign_to.username : assign_to.name,
-    }
 
     if post_target?
       custom_fields.merge!(

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

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

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 }
 

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?(
+        post_content =
-          I18n.t("js.action_codes.#{args[:action]}", who: "@#{assignee}", when: "just now"),
+          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{<a[^>]*>(.*?)</a>}, '\1')
+        end
+
+        container.has_content?(post_content)
       end
     end
   end