replace mentions of out-of-aspect people in limited posts with just a

markdown link to their profile (fixes #2516)

add failing spec for #4160 / #2516

extend the spec a bit more

refactor mention handling in a status message

add method for filtering mentions by aspects

wire mention filtering into the status message model, adapt a few tests to
work properly

cosmetic changes

shorten helper methods

add changelog entry

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

app/helpers/markdownify_helper.rb

@@ -35,15 +35,15 @@ module MarkdownifyHelper
 
     message = markdown.render(message).html_safe
 
-    if target.respond_to?(:format_mentions)
+    if target.respond_to?(:mentioned_people)
-      message = target.format_mentions(message)
+      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

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)

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<Person>] 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<Person>] 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

lib/publisher.rb

@@ -28,8 +28,8 @@ class Publisher
   private
   def formatted_message
     if self.prefill.present?
-      StatusMessage.new(:text => self.prefill).
+      sm = StatusMessage.new(:text => self.prefill)
-        format_mentions(self.prefill, :plain_text => true)
+      Diaspora::Mentionable.format(sm.raw_message, sm.mentioned_people, plain_text: true)
     end
   end
 end

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

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

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 = "</a><script>alert('h')</script>"
+        p.save!
+
+        fmt_msg = Diaspora::Mentionable.format(@status_msg.raw_message, @people)
+
+        fmt_msg.should_not include(p.first_name)
+        fmt_msg.should include("&gt;", "&lt;", "&#x27;") # ">", "<", "'"
+      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

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 = FactoryGirl.build(:status_message, text: "text @{Test; #{alice.diaspora_handle}}")
-      status.should_receive(:create_mentions)
+      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="</a><script>alert('h')</script>"
-        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 = "</a> <script> alert('hey'); </script>"
@@ -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

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