diff --git a/Changelog.md b/Changelog.md index 148764dd2..f7f4cd59b 100644 --- a/Changelog.md +++ b/Changelog.md @@ -18,6 +18,7 @@ * Show medium avatar in hovercard [#4203](https://github.com/diaspora/diaspora/pull/4203) * Fix posting to Twitter [#2758](https://github.com/diaspora/diaspora/issues/2758) * Don't show hovercards for current user in comments [#3999](https://github.com/diaspora/diaspora/issues/3999) +* Replace mentions of out-of-aspect people with markdown links [#4161](https://github.com/diaspora/diaspora/pull/4161) ## Features @@ -45,13 +46,13 @@ To update do the following: 1. Before updating (even before the `git pull`!) stop your application server (Unicorn by default, started through Foreman). 2. In case you did already run `git pull` checkout v0.0.3.4: - + ``` git fetch origin git checkout v0.0.3.4 bundle ``` - + 3. Start Resque web (you'll need temporary access to port 5678, check your Firewall if needed!): @@ -76,7 +77,7 @@ To update do the following: Don't forget to close the port on the Firewall again, if you had to open it. 6. In case you needed to do step 2., run: - + ``` git checkout master bundle diff --git a/app/helpers/markdownify_helper.rb b/app/helpers/markdownify_helper.rb index c8c79756d..95f445c41 100644 --- a/app/helpers/markdownify_helper.rb +++ b/app/helpers/markdownify_helper.rb @@ -35,15 +35,15 @@ module MarkdownifyHelper message = markdown.render(message).html_safe - if target.respond_to?(:format_mentions) - message = target.format_mentions(message) + if target.respond_to?(:mentioned_people) + message = Diaspora::Mentionable.format(message, target.mentioned_people) end message = Diaspora::Taggable.format_tags(message, :no_escape => true) return message.html_safe end - + def strip_markdown(text) renderer = Redcarpet::Markdown.new(Redcarpet::Render::StripDown, :autolink => true) renderer.render(text).strip diff --git a/app/models/status_message.rb b/app/models/status_message.rb index 0c95ebc01..13821c4bd 100644 --- a/app/models/status_message.rb +++ b/app/models/status_message.rb @@ -28,6 +28,7 @@ class StatusMessage < Post attr_accessible :text, :provider_display_name, :frame_name attr_accessor :oembed_url + before_create :filter_mentions after_create :create_mentions after_create :queue_gather_oembed_data, :if => :contains_oembed_url_in_text? @@ -75,41 +76,29 @@ class StatusMessage < Post return self.raw_message unless self.raw_message escaped_message = opts[:plain_text] ? self.raw_message : ERB::Util.h(self.raw_message) - mentioned_message = self.format_mentions(escaped_message, opts) + mentioned_message = Diaspora::Mentionable.format(escaped_message, self.mentioned_people, opts) Diaspora::Taggable.format_tags(mentioned_message, opts.merge(:no_escape => true)) end - def format_mentions(text, opts = {}) - form_message = text.to_str.gsub(Mention::REGEX) do |matched_string| - people = self.mentioned_people - person = people.detect{ |p| - p.diaspora_handle == $~[2] unless p.nil? - } - - if opts[:plain_text] - person ? ERB::Util.h(person.name) : ERB::Util.h($~[1]) - else - person ? person_link(person, :class => 'mention hovercardable') : ERB::Util.h($~[1]) - end - end - form_message - end - def mentioned_people if self.persisted? create_mentions if self.mentions.empty? self.mentions.includes(:person => :profile).map{ |mention| mention.person } else - mentioned_people_from_string + Diaspora::Mentionable.people_from_string(self.raw_message) end end + ## TODO ---- + # don't put presentation logic in the model! def mentioned_people_names self.mentioned_people.map(&:name).join(', ') end + ## ---- ---- def create_mentions - mentioned_people_from_string.each do |person| + ppl = Diaspora::Mentionable.people_from_string(self.raw_message) + ppl.each do |person| self.mentions.find_or_create_by_person_id(person.id) end end @@ -122,13 +111,6 @@ class StatusMessage < Post self.mentions.where(:person_id => person.id).first.try(:notify_recipient) end - def mentioned_people_from_string - identifiers = self.raw_message.scan(Mention::REGEX).map do |match| - match.last - end - identifiers.empty? ? [] : Person.where(:diaspora_handle => identifiers) - end - def after_dispatch(sender) self.update_and_dispatch_attached_photos(sender) end @@ -178,6 +160,15 @@ class StatusMessage < Post end end + def filter_mentions + return if self.public? || self.aspects.empty? + + author_usr = self.author.try(:owner) + aspect_ids = self.aspects.map(&:id) + + self.raw_message = Diaspora::Mentionable.filter_for_aspects(self.raw_message, author_usr, *aspect_ids) + end + private def self.tag_stream(tag_ids) joins(:taggings).where('taggings.tag_id IN (?)', tag_ids) diff --git a/lib/diaspora/mentionable.rb b/lib/diaspora/mentionable.rb new file mode 100644 index 000000000..b91ad0af5 --- /dev/null +++ b/lib/diaspora/mentionable.rb @@ -0,0 +1,134 @@ + +module Diaspora::Mentionable + + # regex for finding mention markup in plain text + # ex. + # "message @{User Name; user@pod.net} text" + # will yield "User Name" and "user@pod.net" + REGEX = /@\{([^;]+); ([^\}]+)\}/ + + # class attribute that will be added to all mention html links + PERSON_HREF_CLASS = "mention hovercardable" + + # takes a message text and returns the text with mentions in (html escaped) + # plain text or formatted with html markup linking to user profiles. + # default is html output. + # + # @param [String] text containing mentions + # @param [Array] list of mentioned people + # @param [Hash] formatting options + # @return [String] formatted message + def self.format(msg_text, people, *opts) + people = [*people] + fmt_msg = msg_text.to_s.gsub(REGEX) do |match_str| + # for some reason gsub doesn't always produce MatchData... + m = REGEX.match(match_str) + person = people.detect{ |p| p.diaspora_handle == m[2] } + + ERB::Util.h(MentionsInternal.mention_link(person, m[1], *opts)) + end + + fmt_msg + end + + # takes a message text and returns an array of people constructed from the + # contained mentions + # + # @param [String] text containing mentions + # @return [Array] array of people + def self.people_from_string(msg_text) + identifiers = msg_text.to_s.scan(REGEX).map do |match| + match.last + end + + return [] if identifiers.empty? + Person.where(diaspora_handle: identifiers) + end + + # takes a message text and converts mentions for people that are not in the + # given aspects to simple markdown links, leaving only mentions for people who + # will actually be able to receive notifications for being mentioned. + # + # @param [String] message text + # @param [User] aspect owner + # @param [Mixed] array containing aspect ids or "all" + # @return [String] message text with filtered mentions + def self.filter_for_aspects(msg_text, user, *aspects) + aspect_ids = MentionsInternal.get_aspect_ids(user, *aspects) + + mentioned_ppl = people_from_string(msg_text) + aspects_ppl = AspectMembership.where(aspect_id: aspect_ids) + .includes(:contact => :person) + .map(&:person) + + filtered_msg = msg_text.to_s.gsub(REGEX) do |match_str| + # for some reason gsub doesn't always produce MatchData... + m = REGEX.match(match_str) + person = mentioned_ppl.detect{ |p| p.diaspora_handle == m[2] } + + mention = match_str + mention = MentionsInternal.profile_link(person, m[1]) unless aspects_ppl.include?(person) + + mention + end + + filtered_msg + end + + private + + # inline module for namespacing + module MentionsInternal + extend ::PeopleHelper + + # output a formatted mention link as defined by the given options, + # use the fallback name if the person is unavailable + # @see Diaspora::Mentions#format + # + # @param [Person] AR Person + # @param [String] fallback name + # @param [Hash] formatting options + def self.mention_link(person, fallback_name, *opts) + return fallback_name unless person.present? + + if opts.include?(:plain_text) + person.name + else + person_link(person, class: PERSON_HREF_CLASS) + end + end + + # output a markdown formatted link to the given person or the given fallback + # string, in case the person is not present + # + # @param [Person] AR Person + # @param [String] fallback name + # @return [String] markdown person link + def self.profile_link(person, fallback_name) + return fallback_name unless person.present? + + "[#{person.name}](#{local_or_remote_person_path(person)})" + end + + # takes a user and an array of aspect ids or an array containing "all" as + # the first element. will do some checking on ids and return them in an array + # in case of "all", returns an array with all the users aspect ids + # + # @param [User] owner of the aspects + # @param [Array] aspect ids or "all" + # @return [Array] aspect ids + def self.get_aspect_ids(user, *aspects) + return [] if aspects.empty? + + if (!aspects.first.kind_of?(Integer)) && aspects.first.to_sym == :all + return user.aspects.pluck(:id) + end + + ids = aspects.select { |id| Integer(id) != nil } # only numeric + + #make sure they really belong to the user + user.aspects.where(id: ids).pluck(:id) + end + end + +end diff --git a/lib/publisher.rb b/lib/publisher.rb index 2746935f0..3de0d0aed 100644 --- a/lib/publisher.rb +++ b/lib/publisher.rb @@ -28,8 +28,8 @@ class Publisher private def formatted_message if self.prefill.present? - StatusMessage.new(:text => self.prefill). - format_mentions(self.prefill, :plain_text => true) + sm = StatusMessage.new(:text => self.prefill) + Diaspora::Mentionable.format(sm.raw_message, sm.mentioned_people, plain_text: true) end end end diff --git a/spec/factories.rb b/spec/factories.rb index d814ae7aa..55620acdd 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -99,6 +99,14 @@ FactoryGirl.define do end end + factory(:status_message_in_aspect, parent: :status_message) do + self.public false + after :build do |sm| + sm.author = FactoryGirl.create(:user_with_aspect).person + sm.aspects << sm.author.owner.aspects.first + end + end + factory(:photo) do sequence(:random_string) {|n| SecureRandom.hex(10) } association :author, :factory => :person diff --git a/spec/integration/mentioning_spec.rb b/spec/integration/mentioning_spec.rb new file mode 100644 index 000000000..dac2bcc0e --- /dev/null +++ b/spec/integration/mentioning_spec.rb @@ -0,0 +1,58 @@ + +require 'spec_helper' + +module MentioningSpecHelpers + def default_aspect + @user1.aspects.where(name: 'generic') + end + + def text_mentioning(user) + handle = user.diaspora_handle + "this is a text mentioning @{Mention User ; #{handle}} ... have fun testing!" + end + + def notifications_about_mentioning(user) + Notifications::Mentioned.where(recipient_id: user.id) + end + + def stream_for(user) + stream = Stream::Multi.new(user) + stream.posts + end + + def users_connected?(user1, user2) + user1.contacts.where(person_id: user2.person).count > 0 + end +end + + +describe 'mentioning' do + include MentioningSpecHelpers + + before do + @user1 = FactoryGirl.create :user_with_aspect + @user2 = FactoryGirl.create :user + @user3 = FactoryGirl.create :user + + @user1.share_with(@user2.person, default_aspect) + end + + # see: https://github.com/diaspora/diaspora/issues/4160 + it 'only mentions people that are in the target aspect' do + users_connected?(@user1, @user2).should be_true + users_connected?(@user1, @user3).should be_false + + status_msg = nil + lambda do + status_msg = @user1.post(:status_message, {text: text_mentioning(@user3), to: default_aspect}) + end.should change(Post, :count).by(1) + + status_msg.should_not be_nil + status_msg.public?.should be_false + status_msg.text.should include(@user3.name) + + notifications_about_mentioning(@user3).should be_empty + stream_for(@user3).map { |item| item.id }.should_not include(status_msg.id) + end + +end diff --git a/spec/lib/diaspora/mentionable_spec.rb b/spec/lib/diaspora/mentionable_spec.rb new file mode 100644 index 000000000..8e7819d20 --- /dev/null +++ b/spec/lib/diaspora/mentionable_spec.rb @@ -0,0 +1,127 @@ + +require 'spec_helper' + +describe Diaspora::Mentionable do + include PeopleHelper + + before do + @people = [alice, bob, eve].map(&:person) + @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}}. +STR + @test_txt_plain = <<-STR +This post contains a lot of mentions +one Alice A, +two Bob B and finally +three Eve E. +STR + @short_txt = "@{M1; m1@a.at} text @{M2 ; m2@b.be}text @{M3; m3@c.ca}" + @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.raw_message, @people) + + fmt_msg.should include(person_link(@people[0], class: 'mention hovercardable')) + fmt_msg.should include(person_link(@people[1], class: 'mention hovercardable')) + fmt_msg.should include(person_link(@people[2], class: 'mention hovercardable')) + end + + it 'escapes the link title (name)' do + p = @people[0].profile + p.first_name = "" + p.save! + + fmt_msg = Diaspora::Mentionable.format(@status_msg.raw_message, @people) + + fmt_msg.should_not include(p.first_name) + fmt_msg.should include(">", "<", "'") # ">", "<", "'" + end + end + + context 'plain text output' do + it 'removes mention markup and displays unformatted name' do + s_msg = FactoryGirl.build(:status_message, text: @short_txt) + fmt_msg = Diaspora::Mentionable.format(s_msg.raw_message, @people, plain_text: true) + + fmt_msg.should eql "M1 text M2 text M3" + end + end + + it 'leaves the name of people that cannot be found' do + fmt_msg = Diaspora::Mentionable.format(@status_msg.raw_message, []) + fmt_msg.should eql @test_txt_plain + end + end + + describe '#people_from_string' do + it 'extracts the mentioned people from the text' do + ppl = Diaspora::Mentionable.people_from_string(@test_txt) + ppl.should include(*@people) + end + + describe 'returns an empty array if nobody was found' do + it 'gets a post without mentions' do + ppl = Diaspora::Mentionable.people_from_string("post w/o mentions") + ppl.should be_empty + end + + it 'gets a post with invalid handles' do + ppl = Diaspora::Mentionable.people_from_string("@{a; xxx@xxx.xx} @{b; yyy@yyyy.yyy}") + ppl.should be_empty + end + end + end + + describe '#filter_for_aspects' do + before do + @user_A = FactoryGirl.create(:user_with_aspect, :username => "user_a") + @user_B = FactoryGirl.create(:user, :username => "user_b") + @user_C = FactoryGirl.create(:user, :username => "user_c") + + @user_A.aspects.create!(name: 'second') + + @mention_B = "@{user B; #{@user_B.diaspora_handle}}" + @mention_C = "@{user C; #{@user_C.diaspora_handle}}" + + @user_A.share_with(@user_B.person, @user_A.aspects.where(name: 'generic')) + @user_A.share_with(@user_C.person, @user_A.aspects.where(name: 'second')) + + @test_txt_B = "mentioning #{@mention_B}" + @test_txt_C = "mentioning #{@mention_C}" + @test_txt_BC = "mentioning #{@mention_B}} and #{@mention_C}" + + Diaspora::Mentionable.stub!(:current_user).and_return(@user_A) + end + + it 'filters mention, if contact is not in a given aspect' do + aspect_id = @user_A.aspects.where(name: 'generic').first.id + txt = Diaspora::Mentionable.filter_for_aspects(@test_txt_C, @user_A, aspect_id) + + txt.should include(@user_C.person.name) + txt.should include(local_or_remote_person_path(@user_C.person)) + txt.should_not include("href") + txt.should_not include(@mention_C) + end + + it 'leaves mention, if contact is in a given aspect' do + aspect_id = @user_A.aspects.where(name: 'generic').first.id + txt = Diaspora::Mentionable.filter_for_aspects(@test_txt_B, @user_A, aspect_id) + + txt.should include("user B") + txt.should include(@mention_B) + end + + it 'recognizes "all" as keyword for aspects' do + txt = Diaspora::Mentionable.filter_for_aspects(@test_txt_BC, @user_A, "all") + + txt.should include(@mention_B) + txt.should include(@mention_C) + end + end +end diff --git a/spec/models/status_message_spec.rb b/spec/models/status_message_spec.rb index 3e9bef962..e80c23096 100644 --- a/spec/models/status_message_spec.rb +++ b/spec/models/status_message_spec.rb @@ -76,12 +76,18 @@ describe StatusMessage do status.should_receive(:build_tags) status.save end + + it 'calls filter_mentions' do + status = FactoryGirl.build(:status_message) + status.should_receive(:filter_mentions) + status.save + end end describe '.after_create' do it 'calls create_mentions' do - status = FactoryGirl.build(:status_message) - status.should_receive(:create_mentions) + status = FactoryGirl.build(:status_message, text: "text @{Test; #{alice.diaspora_handle}}") + status.should_receive(:create_mentions).and_call_original status.save end end @@ -94,6 +100,7 @@ describe StatusMessage do post.author.should == person end end + it "should have either a message or at least one photo" do n = FactoryGirl.build(:status_message, :text => nil) # n.valid?.should be_false @@ -136,36 +143,6 @@ STR @sm = FactoryGirl.create(:status_message, :text => @test_string ) end - describe '#format_mentions' do - it 'adds the links in the formated message text' do - message = @sm.format_mentions(@sm.raw_message) - message.should include(person_link(@people[0], :class => 'mention hovercardable')) - message.should include(person_link(@people[1], :class => 'mention hovercardable')) - message.should include(person_link(@people[2], :class => 'mention hovercardable')) - end - - context 'with :plain_text option' do - it 'removes the mention syntax and displays the unformatted name' do - status = FactoryGirl.build(:status_message, :text => "@{Barack Obama; barak@joindiaspora.com } is so cool @{Barack Obama; barak@joindiaspora.com } ") - status.format_mentions(status.raw_message, :plain_text => true).should == 'Barack Obama is so cool Barack Obama ' - end - end - - it 'leaves the name of people that cannot be found' do - @sm.stub(:mentioned_people).and_return([]) - @sm.format_mentions(@sm.raw_message).should == <<-STR -Raphael can mention people like Raphael Ilya -can mention people like Raphaellike Raphael Daniel can mention people like Raph -STR - end - it 'escapes the link title' do - p = @people[0].profile - p.first_name="" - p.save! - - @sm.format_mentions(@sm.raw_message).should_not include(@people[0].profile.first_name) - end - end describe '#formatted_message' do it 'escapes the message' do xss = " " @@ -178,15 +155,9 @@ STR end end - describe '#mentioned_people_from_string' do - it 'extracts the mentioned people from the message' do - @sm.mentioned_people_from_string.to_set.should == @people.to_set - end - end describe '#create_mentions' do - it 'creates a mention for everyone mentioned in the message' do - @sm.should_receive(:mentioned_people_from_string).and_return(@people) + Diaspora::Mentionable.should_receive(:people_from_string).and_return(@people) @sm.mentions.delete_all @sm.create_mentions @sm.mentions(true).map{|m| m.person}.to_set.should == @people.to_set @@ -200,6 +171,7 @@ STR }.to_not raise_error end end + describe '#mentioned_people' do it 'calls create_mentions if there are no mentions in the db' do @sm.mentions.delete_all @@ -226,24 +198,46 @@ STR end end - describe "#nsfw" do - it 'returns MatchObject (true) if the post contains #nsfw (however capitalised)' do - status = FactoryGirl.build(:status_message, :text => "This message is #nSFw") - status.nsfw.should be_true - end - - it 'returns nil (false) if the post does not contain #nsfw' do - status = FactoryGirl.build(:status_message, :text => "This message is #sFW") - status.nsfw.should be_false - end - end - describe "#notify_person" do it 'notifies the person mentioned' do Notification.should_receive(:notify).with(alice, anything, anything) @sm.notify_person(alice.person) end end + + describe "#filter_mentions" do + it 'calls Diaspora::Mentionable#filter_for_aspects' do + msg = FactoryGirl.build(:status_message_in_aspect) + + msg_txt = msg.raw_message + author_usr = msg.author.owner + aspect_id = author_usr.aspects.first.id + + Diaspora::Mentionable.should_receive(:filter_for_aspects) + .with(msg_txt, author_usr, aspect_id) + + msg.send(:filter_mentions) + end + + it "doesn't do anything when public" do + msg = FactoryGirl.build(:status_message, public: true) + Diaspora::Mentionable.should_not_receive(:filter_for_aspects) + + msg.send(:filter_mentions) + end + end + end + + describe "#nsfw" do + it 'returns MatchObject (true) if the post contains #nsfw (however capitalised)' do + status = FactoryGirl.build(:status_message, :text => "This message is #nSFw") + status.nsfw.should be_true + end + + it 'returns nil (false) if the post does not contain #nsfw' do + status = FactoryGirl.build(:status_message, :text => "This message is #sFW") + status.nsfw.should be_false + end end describe 'tags' do diff --git a/spec/support/user_methods.rb b/spec/support/user_methods.rb index 2e7b04089..2f2c4ea46 100644 --- a/spec/support/user_methods.rb +++ b/spec/support/user_methods.rb @@ -14,11 +14,14 @@ class User def post(class_name, opts = {}) inlined_jobs do + aspects = self.aspects_from_ids(opts[:to]) + p = build_post(class_name, opts) + p.aspects = aspects + if p.save! self.aspects.reload - aspects = self.aspects_from_ids(opts[:to]) add_to_streams(p, aspects) dispatch_opts = {:url => post_url(p), :to => opts[:to]} dispatch_opts.merge!(:additional_subscribers => p.root.author) if class_name == :reshare