From d200e92aebcc309445c524a76ea45ec2425fa6d3 Mon Sep 17 00:00:00 2001
From: cmrd Senya <senya@riseup.net>
Date: Tue, 19 Apr 2016 08:34:09 +0300
Subject: [PATCH] Fix up the meaning of the name parameter in mentions
The desktop frontend now treats the "name" parameter of mention as
a string to display unconditionally. But the Diaspora::Mentionable
renders mentions the different way: "name" is treated as a fallback
string which is rendered only if the person's name is unavailable.
This reflects on the mobile version ATM. This patch makes it behave
the same way as the current desktop version does.
---
app/helpers/people_helper.rb | 6 ++--
lib/diaspora/mentionable.rb | 30 +++++++++----------
spec/helpers/people_helper_spec.rb | 5 ++++
spec/integration/mentioning_spec.rb | 1 +
spec/lib/diaspora/mentionable_spec.rb | 42 +++++++++++++--------------
5 files changed, 45 insertions(+), 39 deletions(-)
diff --git a/app/helpers/people_helper.rb b/app/helpers/people_helper.rb
index 8d5e14c117..18461351e5 100644
--- a/app/helpers/people_helper.rb
+++ b/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
diff --git a/lib/diaspora/mentionable.rb b/lib/diaspora/mentionable.rb
index f64857f771..147364cb3e 100644
--- a/lib/diaspora/mentionable.rb
+++ b/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
diff --git a/spec/helpers/people_helper_spec.rb b/spec/helpers/people_helper_spec.rb
index 24f0bf4455..58b46b73a6 100644
--- a/spec/helpers/people_helper_spec.rb
+++ b/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
diff --git a/spec/integration/mentioning_spec.rb b/spec/integration/mentioning_spec.rb
index 8d93ebd819..360c7465fd 100644
--- a/spec/integration/mentioning_spec.rb
+++ b/spec/integration/mentioning_spec.rb
@@ -52,6 +52,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)
diff --git a/spec/lib/diaspora/mentionable_spec.rb b/spec/lib/diaspora/mentionable_spec.rb
index efc0860b02..280fcbac12 100644
--- a/spec/lib/diaspora/mentionable_spec.rb
+++ b/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 "Eve> 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(">", "<", "'") # ">", "<", "'"
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)
--
GitLab