From 46cbc6e52a9d4a03dff8228ce11ac4876d247a15 Mon Sep 17 00:00:00 2001 From: Steffen van Bergerem Date: Mon, 4 Jul 2016 01:59:52 +0200 Subject: [PATCH] Filter mentions on status message creation --- app/models/status_message.rb | 14 ---- .../status_message_creation_service.rb | 11 +++ spec/integration/mentioning_spec.rb | 67 ++++++++++++++----- spec/models/status_message_spec.rb | 27 -------- .../status_message_creation_service_spec.rb | 38 +++++++++++ 5 files changed, 101 insertions(+), 56 deletions(-) diff --git a/app/models/status_message.rb b/app/models/status_message.rb index 28eab528d..78d8a4b09 100644 --- a/app/models/status_message.rb +++ b/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) diff --git a/app/services/status_message_creation_service.rb b/app/services/status_message_creation_service.rb index f9fcdea1a..b541d194d 100644 --- a/app/services/status_message_creation_service.rb +++ b/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) diff --git a/spec/integration/mentioning_spec.rb b/spec/integration/mentioning_spec.rb index 1b127bea7..8d93ebd81 100644 --- a/spec/integration/mentioning_spec.rb +++ b/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 @{Mention User ; #{handle}} ... have fun testing!" - end - - def notifications_about_mentioning(user) - Notifications::Mentioned.where(recipient_id: user.id) + "this is a text mentioning @{#{user.name}; #{user.diaspora_handle}} ... have fun testing!" 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 - expect(users_connected?(@user1, @user2)).to be true + it "doesn't mention people that aren't in the target aspect" do 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 { |item| item.id }).not_to include(status_msg.id) + 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) 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 diff --git a/spec/models/status_message_spec.rb b/spec/models/status_message_spec.rb index dac72238c..88d15b923 100644 --- a/spec/models/status_message_spec.rb +++ b/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 diff --git a/spec/services/status_message_creation_service_spec.rb b/spec/services/status_message_creation_service_spec.rb index 1fa3f035a..617034e09 100644 --- a/spec/services/status_message_creation_service_spec.rb +++ b/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: []))