Merge pull request #6805 from cmrd-senya/mention-name-fixup

Fix up the meaning of the name parameter in mention

app/helpers/people_helper.rb

@@ -28,7 +28,9 @@ module PeopleHelper
     opts[:class] << " self" if defined?(user_signed_in?) && user_signed_in? && current_user.person == person
     opts[:class] << " hovercardable" if defined?(user_signed_in?) && user_signed_in? && current_user.person != person
     remote_or_hovercard_link = Rails.application.routes.url_helpers.person_path(person).html_safe
-    "<a data-hovercard='#{remote_or_hovercard_link}' href='#{remote_or_hovercard_link}' class='#{opts[:class]}' #{ ("target=" + opts[:target]) if opts[:target]}>#{h(person.name)}</a>".html_safe
+    "<a data-hovercard='#{remote_or_hovercard_link}' href='#{remote_or_hovercard_link}' class='#{opts[:class]}'>"\
+      "#{html_escape_once(opts[:display_name] || person.name)}</a>"\
+      .html_safe
   end
 
   def person_image_tag(person, size = :thumb_small)
@@ -58,7 +60,7 @@ module PeopleHelper
     absolute = opts.delete(:absolute)
 
     if person.local?
-      username = person.diaspora_handle.split('@')[0]
+      username = person.username
       unless username.include?('.')
         opts.merge!(:username => username)
         if absolute

lib/diaspora/mentionable.rb

@@ -14,8 +14,8 @@ module Diaspora::Mentionable
     mention = mention_str.match(REGEX)[2]
     del_pos = mention.rindex(/;/)
 
-    name   = mention[0..(del_pos-1)].strip
-    handle = mention[(del_pos+1)..-1].strip
+    name = mention[0..(del_pos - 1)].strip
+    handle = mention[(del_pos + 1)..-1].strip
 
     [name, handle]
   end
@@ -85,33 +85,33 @@ module Diaspora::Mentionable
   module MentionsInternal
     extend ::PeopleHelper
 
-    # output a formatted mention link as defined by the given options,
-    # use the fallback name if the person is unavailable
+    # output a formatted mention link as defined by the given arguments.
+    # if the display name is blank, falls back to the person's name.
     # @see Diaspora::Mentions#format
     #
     # @param [Person] AR Person
-    # @param [String] fallback name
+    # @param [String] display name
     # @param [Hash] formatting options
-    def self.mention_link(person, fallback_name, opts)
-      return fallback_name unless person.present?
+    def self.mention_link(person, display_name, opts)
+      return display_name unless person.present?
 
       if opts[:plain_text]
-        person.name
+        display_name.presence || person.name
       else
-        person_link(person, class: PERSON_HREF_CLASS)
+        person_link(person, class: PERSON_HREF_CLASS, display_name: display_name)
       end
     end
 
-    # output a markdown formatted link to the given person or the given fallback
-    # string, in case the person is not present
+    # output a markdown formatted link to the given person with the display name as the link text.
+    # if the display name is blank, falls back to the person's name.
     #
     # @param [Person] AR Person
-    # @param [String] fallback name
+    # @param [String] display name
     # @return [String] markdown person link
-    def self.profile_link(person, fallback_name)
-      return fallback_name unless person.present?
+    def self.profile_link(person, display_name)
+      return display_name unless person.present?
 
-      "[#{person.name}](#{local_or_remote_person_path(person)})"
+      "[#{display_name.presence || person.name}](#{local_or_remote_person_path(person)})"
     end
 
     # takes a user and an array of aspect ids or an array containing "all" as

spec/helpers/people_helper_spec.rb

@@ -77,6 +77,11 @@ describe PeopleHelper, :type => :helper do
     it 'links by id for a local user' do
       expect(person_link(@user.person)).to include "href='#{person_path(@user.person)}'"
     end
+
+    it "recognizes the 'display_name' option" do
+      display_name = "string used as a name"
+      expect(person_link(@person, display_name: display_name)).to match(%r{<a [^>]+>#{display_name}</a>})
+    end
   end
 
   describe '#local_or_remote_person_path' do

spec/integration/mentioning_spec.rb

@@ -51,6 +51,7 @@ describe "mentioning", type: :request do
     expect(status_msg.public?).to be false
     expect(status_msg.text).to include(@user3.name)
     expect(status_msg.text).not_to include(@user3.diaspora_handle)
+    expect(status_msg.text).to include(user_profile_path(username: @user3.username))
 
     expect(stream_for(@user3).map(&:id)).not_to include(status_msg.id)
     expect(mention_stream_for(@user3).map(&:id)).not_to include(status_msg.id)

spec/lib/diaspora/federation/receive_spec.rb

@@ -425,7 +425,7 @@ describe Diaspora::Federation::Receive do
       expect(profile.location).to eq(profile_entity.location)
       expect(profile.searchable).to eq(profile_entity.searchable)
       expect(profile.nsfw).to eq(profile_entity.nsfw)
-      expect(profile.tag_string).to eq(profile_entity.tag_string)
+      expect(profile.tag_string.split(" ")).to match_array(profile_entity.tag_string.split(" "))
     end
   end
 

spec/lib/diaspora/mentionable_spec.rb

@@ -6,11 +6,12 @@ describe Diaspora::Mentionable do
 
   before do
     @people = [alice, bob, eve].map(&:person)
+    @names = %w(Alice\ A Bob\ B "Eve>\ E)
     @test_txt = <<-STR
 This post contains a lot of mentions
-one @{Alice A; #{@people[0].diaspora_handle}},
-two @{Bob B; #{@people[1].diaspora_handle}} and finally
-three @{"Eve> E; #{@people[2].diaspora_handle}}.
+one @{#{@names[0]}; #{@people[0].diaspora_handle}},
+two @{#{@names[1]}; #{@people[1].diaspora_handle}} and finally
+three @{#{@names[2]}; #{@people[2].diaspora_handle}}.
 STR
     @test_txt_plain = <<-STR
 This post contains a lot of mentions
@@ -18,53 +19,50 @@ one Alice A,
 two Bob B and finally
 three &quot;Eve&gt; E.
 STR
-    @status_msg = FactoryGirl.build(:status_message, text: @test_txt)
   end
 
   describe "#format" do
     context "html output" do
       it "adds the links to the formatted message" do
-        fmt_msg = Diaspora::Mentionable.format(@status_msg.text, @people)
+        fmt_msg = Diaspora::Mentionable.format(@test_txt, @people)
 
-        @people.each do |person|
-          expect(fmt_msg).to include person_link(person, class: "mention hovercardable")
+        [@people, @names].transpose.each do |person, name|
+          expect(fmt_msg).to include person_link(person, class: "mention hovercardable", display_name: name)
         end
       end
 
       it "should work correct when message is escaped html" do
-        raw_msg = @status_msg.text
-        fmt_msg = Diaspora::Mentionable.format(CGI.escapeHTML(raw_msg), @people)
+        fmt_msg = Diaspora::Mentionable.format(CGI.escapeHTML(@test_txt), @people)
 
-        @people.each do |person|
-          expect(fmt_msg).to include person_link(person, class: "mention hovercardable")
+        [@people, @names].transpose.each do |person, name|
+          expect(fmt_msg).to include person_link(person, class: "mention hovercardable", display_name: name)
         end
       end
 
       it "escapes the link title (name)" do
-        p = @people[0].profile
-        p.first_name = "</a><script>alert('h')</script>"
-        p.save!
+        name = "</a><script>alert('h')</script>"
+        test_txt = "two @{#{name}; #{@people[0].diaspora_handle}} and finally"
 
-        fmt_msg = Diaspora::Mentionable.format(@status_msg.text, @people)
+        fmt_msg = Diaspora::Mentionable.format(test_txt, @people)
 
-        expect(fmt_msg).not_to include(p.first_name)
+        expect(fmt_msg).not_to include(name)
         expect(fmt_msg).to include("&gt;", "&lt;", "&#39;") # ">", "<", "'"
       end
     end
 
     context "plain text output" do
       it "removes mention markup and displays unformatted name" do
-        fmt_msg = Diaspora::Mentionable.format(@status_msg.text, @people, plain_text: true)
+        fmt_msg = Diaspora::Mentionable.format(@test_txt, @people, plain_text: true)
 
-        @people.each do |person|
-          expect(fmt_msg).to include person.first_name
+        @names.each do |name|
+          expect(fmt_msg).to include CGI.escapeHTML(name)
         end
         expect(fmt_msg).not_to include "<a", "</a>", "hovercardable"
       end
     end
 
-    it "leaves the name of people that cannot be found" do
-      fmt_msg = Diaspora::Mentionable.format(@status_msg.text, [])
+    it "leaves the names of people that cannot be found" do
+      fmt_msg = Diaspora::Mentionable.format(@test_txt, [])
       expect(fmt_msg).to eql @test_txt_plain
     end
   end
@@ -111,7 +109,7 @@ STR
       aspect_id = @user_a.aspects.where(name: "generic").first.id
       txt = Diaspora::Mentionable.filter_for_aspects(@test_txt_c, @user_a, aspect_id)
 
-      expect(txt).to include(@user_c.person.name)
+      expect(txt).to include("user C")
       expect(txt).to include(local_or_remote_person_path(@user_c.person))
       expect(txt).not_to include("href")
       expect(txt).not_to include(@mention_c)