Filter mentions on status message creation

app/models/status_message.rb

@@ -28,7 +28,6 @@ class StatusMessage < Post
   attr_accessor :oembed_url
   attr_accessor :open_graph_url
 
-  before_create :filter_mentions
   after_create :create_mentions
   after_commit :queue_gather_oembed_data, :on => :create, :if => :contains_oembed_url_in_text?
   after_commit :queue_gather_open_graph_data, :on => :create, :if => :contains_open_graph_url_in_text?
@@ -95,10 +94,6 @@ class StatusMessage < Post
     mentioned_people.include? person
   end
 
-  def notify_person(person)
-    self.mentions.where(:person_id => person.id).first.try(:notify_recipient)
-  end
-
   def comment_email_subject
     message.title
   end
@@ -150,15 +145,6 @@ 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)

app/services/status_message_creation_service.rb

@@ -19,9 +19,20 @@ class StatusMessageCreationService
 
   def build_status_message(params)
     public = params[:public] || false
+    filter_mentions params
     user.build_post(:status_message, params[:status_message].merge(public: public))
   end
 
+  def filter_mentions(params)
+    unless params[:public]
+      params[:status_message][:text] = Diaspora::Mentionable.filter_for_aspects(
+        params[:status_message][:text],
+        user,
+        *params[:aspect_ids]
+      )
+    end
+  end
+
   def add_attachments(status_message, params)
     add_location(status_message, params[:location_address], params[:location_coords])
     add_poll(status_message, params)

spec/integration/mentioning_spec.rb

@@ -1,18 +1,13 @@
-
+require "spec_helper"
-require 'spec_helper'
+require "requests_helper"
 
 module MentioningSpecHelpers
   def default_aspect
-    @user1.aspects.where(name: 'generic').first
+    @user1.aspects.where(name: "generic").first
   end
 
   def text_mentioning(user)
-    handle = user.diaspora_handle
+    "this is a text mentioning @{#{user.name}; #{user.diaspora_handle}} ... have fun testing!"
-    "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)
@@ -20,13 +15,18 @@ module MentioningSpecHelpers
     stream.posts
   end
 
+  def mention_stream_for(user)
+    stream = Stream::Mention.new(user)
+    stream.posts
+  end
+
   def users_connected?(user1, user2)
     user1.contacts.where(person_id: user2.person).count > 0
   end
 end
 
 
-describe 'mentioning', :type => :request do
+describe "mentioning", type: :request do
   include MentioningSpecHelpers
 
   before do
@@ -35,24 +35,61 @@ describe 'mentioning', :type => :request do
     @user3 = FactoryGirl.create :user
 
     @user1.share_with(@user2.person, default_aspect)
+    login @user1
   end
 
   # see: https://github.com/diaspora/diaspora/issues/4160
-  it 'only mentions people that are in the target aspect' do
+  it "doesn't mention people that aren't in the target aspect" do
-    expect(users_connected?(@user1, @user2)).to be true
     expect(users_connected?(@user1, @user3)).to be false
 
     status_msg = nil
     expect {
-      status_msg = @user1.post(:status_message, {text: text_mentioning(@user3), to: default_aspect})
+      post "/status_messages.json", status_message: {text: text_mentioning(@user3)}, aspect_ids: default_aspect.id.to_s
+      status_msg = StatusMessage.find(JSON.parse(response.body)["id"])
     }.to change(Post, :count).by(1).and change(AspectVisibility, :count).by(1)
 
     expect(status_msg).not_to be_nil
     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(notifications_about_mentioning(@user3)).to be_empty
+    expect(stream_for(@user3).map(&:id)).not_to include(status_msg.id)
-    expect(stream_for(@user3).map { |item| item.id }).not_to include(status_msg.id)
+    expect(mention_stream_for(@user3).map(&:id)).not_to include(status_msg.id)
   end
 
+  it "mentions people in public posts" do
+    expect(users_connected?(@user1, @user3)).to be false
+
+    status_msg = nil
+    expect {
+      post "/status_messages.json", status_message: {text: text_mentioning(@user3)}, aspect_ids: "public"
+      status_msg = StatusMessage.find(JSON.parse(response.body)["id"])
+    }.to change(Post, :count).by(1)
+
+    expect(status_msg).not_to be_nil
+    expect(status_msg.public?).to be true
+    expect(status_msg.text).to include(@user3.name)
+    expect(status_msg.text).to include(@user3.diaspora_handle)
+
+    expect(stream_for(@user3).map(&:id)).to include(status_msg.id)
+    expect(mention_stream_for(@user3).map(&:id)).to include(status_msg.id)
+  end
+
+  it "mentions people that are in the target aspect" do
+    expect(users_connected?(@user1, @user2)).to be true
+
+    status_msg = nil
+    expect {
+      post "/status_messages.json", status_message: {text: text_mentioning(@user2)}, aspect_ids: default_aspect.id.to_s
+      status_msg = StatusMessage.find(JSON.parse(response.body)["id"])
+    }.to change(Post, :count).by(1).and change(AspectVisibility, :count).by(1)
+
+    expect(status_msg).not_to be_nil
+    expect(status_msg.public?).to be false
+    expect(status_msg.text).to include(@user2.name)
+    expect(status_msg.text).to include(@user2.diaspora_handle)
+
+    expect(stream_for(@user2).map(&:id)).to include(status_msg.id)
+    expect(mention_stream_for(@user2).map(&:id)).to include(status_msg.id)
+  end
 end

spec/models/status_message_spec.rb

@@ -81,11 +81,6 @@ describe StatusMessage, type: :model do
       expect(status).to receive(:build_tags)
       status.save
     end
-
-    it "calls filter_mentions" do
-      expect(status).to receive(:filter_mentions)
-      status.save
-    end
   end
 
   describe ".after_create" do
@@ -189,28 +184,6 @@ describe StatusMessage, type: :model do
         expect(status_message.mentions?(FactoryGirl.build(:person))).to be false
       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
-
-        expect(Diaspora::Mentionable).to 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)
-        expect(Diaspora::Mentionable).not_to receive(:filter_for_aspects)
-
-        msg.send(:filter_mentions)
-      end
-    end
   end
 
   describe "#nsfw" do

spec/services/status_message_creation_service_spec.rb

@@ -153,6 +153,44 @@ describe StatusMessageCreationService do
       end
     end
 
+    context "with mentions" do
+      let(:mentioned_user) { FactoryGirl.create(:user) }
+      let(:text) {
+        "Hey @{#{bob.name}; #{bob.diaspora_handle}} and @{#{mentioned_user.name}; #{mentioned_user.diaspora_handle}}!"
+      }
+
+      it "calls Diaspora::Mentionable#filter_for_aspects for private posts" do
+        expect(Diaspora::Mentionable).to receive(:filter_for_aspects).with(text, alice, aspect.id.to_s)
+          .and_call_original
+        StatusMessageCreationService.new(alice).create(params)
+      end
+
+      it "keeps mentions for users in one of the aspects" do
+        status_message = StatusMessageCreationService.new(alice).create(params)
+        expect(status_message.text).to include bob.name
+        expect(status_message.text).to include bob.diaspora_handle
+      end
+
+      it "removes mentions for users in other aspects" do
+        status_message = StatusMessageCreationService.new(alice).create(params)
+        expect(status_message.text).to include mentioned_user.name
+        expect(status_message.text).not_to include mentioned_user.diaspora_handle
+      end
+
+      it "doesn't call Diaspora::Mentionable#filter_for_aspects for public posts" do
+        expect(Diaspora::Mentionable).not_to receive(:filter_for_aspects)
+        StatusMessageCreationService.new(alice).create(params.merge(public: true))
+      end
+
+      it "keeps all mentions in public posts" do
+        status_message = StatusMessageCreationService.new(alice).create(params.merge(public: true))
+        expect(status_message.text).to include bob.name
+        expect(status_message.text).to include bob.diaspora_handle
+        expect(status_message.text).to include mentioned_user.name
+        expect(status_message.text).to include mentioned_user.diaspora_handle
+      end
+    end
+
     context "dispatch" do
       it "dispatches the StatusMessage" do
         expect(alice).to receive(:dispatch_post).with(instance_of(StatusMessage), hash_including(service_types: []))