diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index e88af7a77..2441173bc 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -3,7 +3,7 @@ # the COPYRIGHT file. class PostsController < ApplicationController - before_action :authenticate_user!, only: :destroy + before_action :authenticate_user!, only: %i(destroy mentionable) before_action :set_format_if_malformed_from_status_net, only: :show respond_to :html, :mobile, :json, :xml @@ -50,6 +50,21 @@ class PostsController < ApplicationController end end + def mentionable + respond_to do |format| + format.json { + if params[:id].present? && params[:q].present? + render json: post_service.mentionable_in_comment(params[:id], params[:q]) + else + render nothing: true, status: 204 + end + } + format.any { render nothing: true, status: 406 } + end + rescue ActiveRecord::RecordNotFound + render nothing: true, status: 404 + end + def destroy post_service.destroy(params[:id]) respond_to do |format| diff --git a/app/models/comment.rb b/app/models/comment.rb index 483215f03..fb5446173 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -54,14 +54,6 @@ class Comment < ActiveRecord::Base self[:text] = text.to_s.strip #to_s if for nil, for whatever reason end - def people_allowed_to_be_mentioned - if parent.public? - :all - else - [*parent.comments.pluck(:author_id), *parent.likes.pluck(:author_id), parent.author_id].uniq - end - end - def add_mention_subscribers? super && parent.author.local? end diff --git a/app/models/notifications/mentioned_in_comment.rb b/app/models/notifications/mentioned_in_comment.rb index 77038864a..5e5466b68 100644 --- a/app/models/notifications/mentioned_in_comment.rb +++ b/app/models/notifications/mentioned_in_comment.rb @@ -11,12 +11,7 @@ module Notifications end def self.filter_mentions(mentions, mentionable, _recipient_user_ids) - people = mentionable.people_allowed_to_be_mentioned - if people == :all - mentions - else - mentions.where(person_id: people) - end + mentions.joins(:person).merge(Person.allowed_to_be_mentioned_in_a_comment_to(mentionable.parent)) end def mail_job diff --git a/app/models/person.rb b/app/models/person.rb index 8e0d5b6e2..e9eefc6ec 100644 --- a/app/models/person.rb +++ b/app/models/person.rb @@ -62,7 +62,8 @@ class Person < ActiveRecord::Base scope :remote, -> { where('people.owner_id IS NULL') } scope :local, -> { where('people.owner_id IS NOT NULL') } scope :for_json, -> { - select('DISTINCT people.id, people.guid, people.diaspora_handle') + select("people.id, people.guid, people.diaspora_handle") + .distinct .includes(:profile) } @@ -94,6 +95,78 @@ class Person < ActiveRecord::Base .where(:posts => {:root_guid => StatusMessage.guids_for_author(user.person), :type => 'Reshare'} ) } + # This scope selects people where the full name contains the search_str or diaspora ID + # starts with the search_str. + # However, if the search_str doesn't have more than 1 non-whitespace character, it'll return an empty set. + # @param [String] search substring + # @return [Person::ActiveRecord_Relation] + scope :find_by_substring, ->(search_str) { + search_str.strip! + return none if search_str.blank? || search_str.size < 2 + + sql, tokens = search_query_string(search_str) + joins(:profile).where(sql, *tokens) + } + + # Left joins likes and comments to a specific post where people are authors of these comments and likes + # @param [String, Integer] post ID for which comments and likes should be joined + # @return [Person::ActiveRecord_Relation] + scope :left_join_visible_post_interactions_on_authorship, ->(post_id) { + comments_sql = <<-SQL + LEFT OUTER JOIN comments ON + comments.author_id = people.id AND comments.commentable_type = 'Post' AND comments.commentable_id = #{post_id} + SQL + + likes_sql = <<-SQL + LEFT OUTER JOIN likes ON + likes.author_id = people.id AND likes.target_type = 'Post' AND likes.target_id = #{post_id} + SQL + + joins(comments_sql).joins(likes_sql) + } + + # Selects people who can be mentioned in a comment to a specific post. For public posts all people + # are allowed, so no additional constraints are added. For private posts selection is limited to + # people who have posted comments or likes for this post. + # @param [Post] the post for which we query mentionable in comments people + # @return [Person::ActiveRecord_Relation] + scope :allowed_to_be_mentioned_in_a_comment_to, ->(post) { + if post.public? + all + else + left_join_visible_post_interactions_on_authorship(post.id) + .where("comments.id IS NOT NULL OR likes.id IS NOT NULL OR people.id = #{post.author_id}") + end + } + + # This scope adds sorting of people in the order, appropriate for suggesting to a user (current user) who + # has requested a list of the people mentionable in a comment for a specific post. + # Sorts people in the following priority: post author > commenters > likers > contacts > non-contacts + # @param [Post] post for which the mentionable in comment people list is requested + # @param [User] user who requests the people list + # @return [Person::ActiveRecord_Relation] + scope :sort_for_mention_suggestion, ->(post, user) { + left_join_visible_post_interactions_on_authorship(post.id) + .joins("LEFT OUTER JOIN contacts ON people.id = contacts.person_id AND contacts.user_id = #{user.id}") + .joins(:profile) + .select(<<-SQL + people.id = #{unscoped { post.author_id }} AS is_author, + comments.id IS NOT NULL AS is_commenter, + likes.id IS NOT NULL AS is_liker, + contacts.id IS NOT NULL AS is_contact + SQL + ) + .order(<<-SQL + is_author DESC, + is_commenter DESC, + is_liker DESC, + is_contact DESC, + profiles.full_name, + people.diaspora_handle + SQL + ) + } + def self.community_spotlight Person.joins(:roles).where(:roles => {:name => 'spotlight'}) end @@ -128,7 +201,7 @@ class Person < ActiveRecord::Base self.guid end - def self.search_query_string(query) + private_class_method def self.search_query_string(query) query = query.downcase like_operator = AppConfig.postgres? ? "ILIKE" : "LIKE" @@ -146,15 +219,13 @@ class Person < ActiveRecord::Base end def self.search(search_str, user, only_contacts: false, mutual: false) - search_str.strip! - return none if search_str.blank? || search_str.size < 2 - - sql, tokens = search_query_string(search_str) + query = find_by_substring(search_str) + return query if query.is_a?(ActiveRecord::NullRelation) query = if only_contacts - joins(:contacts).where(contacts: {user_id: user.id}) + query.joins(:contacts).where(contacts: {user_id: user.id}) else - joins( + query.joins( "LEFT OUTER JOIN contacts ON contacts.user_id = #{user.id} AND contacts.person_id = people.id" ).searchable(user) end @@ -162,8 +233,6 @@ class Person < ActiveRecord::Base query = query.where(contacts: {sharing: true, receiving: true}) if mutual query.where(closed_account: false) - .where(sql, *tokens) - .includes(:profile) .order(["contacts.user_id IS NULL", "profiles.last_name ASC", "profiles.first_name ASC"]) end diff --git a/app/models/status_message.rb b/app/models/status_message.rb index 28d838536..423ea01fc 100644 --- a/app/models/status_message.rb +++ b/app/models/status_message.rb @@ -95,6 +95,8 @@ class StatusMessage < Post photos.each {|photo| photo.receive(recipient_user_ids) } end + # Note: the next two methods can be safely removed once changes from #6818 are deployed on every pod + # see StatusMessageCreationService#dispatch # Only includes those people, to whom we're going to send a federation entity # (and doesn't define exhaustive list of people who can receive it) def people_allowed_to_be_mentioned diff --git a/app/services/post_service.rb b/app/services/post_service.rb index 99ce0d28e..6d35a385d 100644 --- a/app/services/post_service.rb +++ b/app/services/post_service.rb @@ -31,6 +31,17 @@ class PostService user.retract(post) end + def mentionable_in_comment(post_id, query) + post = find!(post_id) + Person + .allowed_to_be_mentioned_in_a_comment_to(post) + .where.not(id: user.person_id) + .find_by_substring(query) + .sort_for_mention_suggestion(post, user) + .for_json + .limit(15) + end + private attr_reader :user diff --git a/config/routes.rb b/config/routes.rb index 288733244..f71fe120a 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -32,6 +32,7 @@ Diaspora::Application.routes.draw do resources :posts, only: %i(show destroy) do member do get :interactions + get :mentionable end resource :participation, only: %i(create destroy) diff --git a/spec/controllers/posts_controller_spec.rb b/spec/controllers/posts_controller_spec.rb index 7f2d9c9fb..2ce6b677c 100644 --- a/spec/controllers/posts_controller_spec.rb +++ b/spec/controllers/posts_controller_spec.rb @@ -4,6 +4,7 @@ describe PostsController, type: :controller do let(:post) { alice.post(:status_message, text: "ohai", to: alice.aspects.first) } + let(:post_service) { controller.send(:post_service) } describe "#show" do context "user signed in" do @@ -166,6 +167,48 @@ describe PostsController, type: :controller do end end + describe "#mentionable" do + context "with a user signed in" do + before do + sign_in alice + end + + it "returns status 204 without a :q parameter" do + get :mentionable, id: post.id, format: :json + expect(response.status).to eq(204) + end + + it "responses status 406 (not acceptable) on html request" do + get :mentionable, id: post.id, q: "whatever", format: :html + expect(response.status).to eq(406) + end + + it "responses status 404 when the post can't be found" do + expect(post_service).to receive(:find!) do + raise ActiveRecord::RecordNotFound + end + get :mentionable, id: post.id, q: "whatever", format: :json + expect(response.status).to eq(404) + end + + it "calls PostService#mentionable_in_comment and passes the result as a response" do + expect(post_service).to receive(:mentionable_in_comment).with(post.id.to_s, "whatever").and_return([bob.person]) + get :mentionable, id: post.id, q: "whatever", format: :json + expect(response.status).to eq(200) + expect(response.body).to eq([bob.person].to_json) + end + end + + context "without a user signed in" do + it "returns 401" do + allow(post_service).to receive(:mentionable_in_comment).and_return([]) + get :mentionable, id: post.id, q: "whatever", format: :json + expect(response.status).to eq(401) + expect(JSON.parse(response.body)["error"]).to eq(I18n.t("devise.failure.unauthenticated")) + end + end + end + describe "#destroy" do context "own post" do before do diff --git a/spec/factories.rb b/spec/factories.rb index ec8cec960..d89eb7d95 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -32,12 +32,17 @@ FactoryGirl.define do end factory(:person, aliases: %i(author)) do + transient do + first_name nil + end + sequence(:diaspora_handle) {|n| "bob-person-#{n}#{r_str}@example.net" } pod { Pod.find_or_create_by(url: "http://example.net") } serialized_public_key OpenSSL::PKey::RSA.generate(1024).public_key.export - after(:build) do |person| + after(:build) do |person, evaluator| unless person.profile.first_name.present? person.profile = FactoryGirl.build(:profile, person: person) + person.profile.first_name = evaluator.first_name if evaluator.first_name end end after(:create) do |person| diff --git a/spec/models/comment_spec.rb b/spec/models/comment_spec.rb index 05499dc39..82d46566c 100644 --- a/spec/models/comment_spec.rb +++ b/spec/models/comment_spec.rb @@ -23,25 +23,6 @@ describe Comment, type: :model do end end - describe "#people_allowed_to_be_mentioned" do - let(:kate) { FactoryGirl.create(:user_with_aspect, friends: [bob]) } - let(:olga) { FactoryGirl.create(:user_with_aspect, friends: [bob]) } - - it "returns the author and people who have commented or liked the private post" do - eve.comment!(status_bob, "comment text") - kate.like!(status_bob) - olga.participate!(status_bob) - status_bob.reload - expect(comment_alice.people_allowed_to_be_mentioned).to match_array([alice, bob, eve, kate].map(&:person_id)) - end - - it "returns :all for public posts" do - # set parent public - status_bob.update(public: true) - expect(comment_alice.people_allowed_to_be_mentioned).to eq(:all) - end - end - describe "#subscribers" do let(:status_bob) { FactoryGirl.create(:status_message, public: true, author: bob.person) } let(:comment_alice) { diff --git a/spec/models/person_spec.rb b/spec/models/person_spec.rb index bb2bb7d6b..4e65c6ccc 100644 --- a/spec/models/person_spec.rb +++ b/spec/models/person_spec.rb @@ -94,6 +94,89 @@ describe Person, :type => :model do expect(Person.who_have_reshared_a_users_posts(alice)).to eq([reshare.author]) end end + + describe ".find_by_substring" do + it "returns \"none\" when the substring is less than 1 non-space character" do + expect(Person.find_by_substring("R")).to eq(Person.none) + expect(Person.find_by_substring("R ")).to eq(Person.none) + expect(Person.find_by_substring("")).to eq(Person.none) + expect(Person.find_by_substring(" ")).to eq(Person.none) + end + + it "finds a person with a profile name containing the substring" do + substring = r_str + person = FactoryGirl.create(:person, first_name: "A#{substring}A") + expect(Person.find_by_substring(substring)).to include(person) + end + + it "finds a person with a diaspora ID starting with the substring" do + substring = r_str + person = FactoryGirl.create(:person, diaspora_handle: "#{substring}A@pod.tld") + expect(Person.find_by_substring(substring)).to include(person) + end + end + + describe ".allowed_to_be_mentioned_in_a_comment_to" do + let(:status_bob) { bob.post(:status_message, text: "hello", to: bob.aspects.first.id) } + + it "returns the author and people who have commented or liked the private post" do + kate = FactoryGirl.create(:user_with_aspect, friends: [bob]) + olga = FactoryGirl.create(:user_with_aspect, friends: [bob]) + alice.comment!(status_bob, "why so formal?") + eve.comment!(status_bob, "comment text") + kate.like!(status_bob) + olga.participate!(status_bob) + expect( + Person.allowed_to_be_mentioned_in_a_comment_to(status_bob).ids + ).to match_array([alice, bob, eve, kate].map(&:person_id)) + end + + it "returns all for public posts" do + status_bob.update(public: true) # set parent public + expect(Person.allowed_to_be_mentioned_in_a_comment_to(status_bob)).to eq(Person.all) + end + end + + describe ".sort_for_mention_suggestion" do + let(:status_message) { FactoryGirl.create(:status_message) } + + it "returns people sorted in the order: post author > commenters > likers > contacts" do + like = FactoryGirl.create(:like, target: status_message) + comment = FactoryGirl.create(:comment, post: status_message) + current_user = FactoryGirl.create(:user_with_aspect, friends: [alice]) + result = Person.select(:id, :guid).sort_for_mention_suggestion(status_message, current_user) + expect(result[0]).to eq(status_message.author) + expect(result[1]).to eq(comment.author) + expect(result[2]).to eq(like.author) + expect(result[3]).to eq(alice.person) # a contact of the current user + end + + it "sorts people of the same priority by profile name" do + current_user = FactoryGirl.create(:user_with_aspect) + person1 = FactoryGirl.create(:person, first_name: "x2") + person2 = FactoryGirl.create(:person, first_name: "x1") + result = Person + .select(:id, :guid) + .where(id: [person1.id, person2.id]) + .sort_for_mention_suggestion(status_message, current_user) + expect(result[0].id).to eq(person2.id) + expect(result[1].id).to eq(person1.id) + end + + it "sorts people of the same priority and same names by diaspora ID" do + current_user = FactoryGirl.create(:user_with_aspect) + person1 = FactoryGirl.create(:person, diaspora_handle: "x2@pod.tld") + person1.profile.update(first_name: "John", last_name: "Doe") + person2 = FactoryGirl.create(:person, diaspora_handle: "x1@pod.tld") + person2.profile.update(first_name: "John", last_name: "Doe") + result = Person + .select(:id, :guid) + .where(id: [person1.id, person2.id]) + .sort_for_mention_suggestion(status_message, current_user) + expect(result[0].id).to eq(person2.id) + expect(result[1].id).to eq(person1.id) + end + end end describe "delegating" do diff --git a/spec/services/post_service_spec.rb b/spec/services/post_service_spec.rb index b19a1638d..28dfae6c1 100644 --- a/spec/services/post_service_spec.rb +++ b/spec/services/post_service_spec.rb @@ -193,4 +193,220 @@ describe PostService do expect(StatusMessage.find_by_id(post.id)).not_to be_nil end end + + describe "#mentionable_in_comment" do + describe "semi-integration test" do + let(:post_author_attributes) { {first_name: "Ro#{r_str}"} } + let(:post_author) { FactoryGirl.create(:person, post_author_attributes) } + let(:current_user) { FactoryGirl.create(:user_with_aspect) } + let(:post_service) { PostService.new(current_user) } + + shared_context "with commenters and likers" do + # randomize ids of the created people so that the test doesn't pass just because of + # the id sequence matched against the expected ordering + let(:ids) { (1..4).map {|i| Person.maximum(:id) + i }.shuffle } + + before do + # in case when post_author has't been instantiated before this context, specify id + # in order to avoid id conflict with the people generated here + post_author_attributes.merge!(id: ids.max + 1) + end + + let!(:commenter1) { + FactoryGirl.create(:person, id: ids.shift, first_name: "Ro1#{r_str}").tap {|person| + FactoryGirl.create(:comment, author: person, post: post) + } + } + + let!(:commenter2) { + FactoryGirl.create(:person, id: ids.shift, first_name: "Ro2#{r_str}").tap {|person| + FactoryGirl.create(:comment, author: person, post: post) + } + } + + let!(:liker1) { + FactoryGirl.create(:person, id: ids.shift, first_name: "Ro1#{r_str}").tap {|person| + FactoryGirl.create(:like, author: person, target: post) + } + } + + let!(:liker2) { + FactoryGirl.create(:person, id: ids.shift, first_name: "Ro2#{r_str}").tap {|person| + FactoryGirl.create(:like, author: person, target: post) + } + } + end + + shared_context "with a current user's friend" do + let!(:current_users_friend) { + FactoryGirl.create(:person).tap {|friend| + current_user.contacts.create!( + person: friend, + aspects: [current_user.aspects.first], + sharing: true, + receiving: true + ) + } + } + end + + context "with private post" do + let(:post) { FactoryGirl.create(:status_message, text: "ohai", author: post_author) } + + context "when the post doesn't have a visibility for the current user" do + it "doesn't find a post and raises an exception" do + expect { + post_service.mentionable_in_comment(post.id, "Ro") + }.to raise_error(ActiveRecord::RecordNotFound) + end + end + + context "when the post has a visibility for the current user" do + before do + ShareVisibility.batch_import([current_user.id], post) + end + + context "with commenters and likers" do + include_context "with commenters and likers" + + it "returns mention suggestions in the correct order" do + expected_suggestions = [ + post_author, commenter1, commenter2, liker1, liker2 + ] + expect(post_service.mentionable_in_comment(post.id, "Ro")).to eq(expected_suggestions) + end + end + + context "with a current user's friend" do + include_context "with a current user's friend" + + it "doesn't include a contact" do + expect(post_service.mentionable_in_comment(post.id, current_users_friend.first_name)).to be_empty + end + end + + it "doesn't include a non contact" do + expect(post_service.mentionable_in_comment(post.id, eve.person.first_name)).to be_empty + end + end + end + + context "with public post" do + let(:post) { FactoryGirl.create(:status_message, text: "ohai", public: true, author: post_author) } + + context "with commenters and likers and with a current user's friend" do + include_context "with commenters and likers" + include_context "with a current user's friend" + + it "returns mention suggestions in the correct order" do + result = post_service.mentionable_in_comment(post.id, "Ro") + expect(result.size).to be > 7 + # participants: post author, comments, likers + expect(result[0..4]).to eq([post_author, commenter1, commenter2, liker1, liker2]) + # contacts + expect(result[5]).to eq(current_users_friend) + # non-contacts + result[6..-1].each {|person| + expect(person.contacts.where(user_id: current_user.id)).to be_empty + expect(person.profile.first_name).to include("Ro") + } + end + + it "doesn't include people with non-matching names" do + commenter = FactoryGirl.create(:person, first_name: "RRR#{r_str}") + FactoryGirl.create(:comment, author: commenter) + liker = FactoryGirl.create(:person, first_name: "RRR#{r_str}") + FactoryGirl.create(:like, author: liker) + friend = FactoryGirl.create(:person, first_name: "RRR#{r_str}") + current_user.contacts.create!( + person: friend, + aspects: [current_user.aspects.first], + sharing: true, + receiving: true + ) + + result = post_service.mentionable_in_comment(post.id, "Ro") + expect(result).not_to include(commenter) + expect(result).not_to include(liker) + expect(result).not_to include(friend) + end + end + + shared_examples "current user can't mention himself" do + before do + current_user.profile.update(first_name: "Ro#{r_str}") + end + + it "doesn't include current user" do + expect(post_service.mentionable_in_comment(post.id, "Ro")).not_to include(current_user.person) + end + end + + context "when current user is a post author" do + let(:post_author) { current_user.person } + + include_examples "current user can't mention himself" + end + + context "current user is a participant" do + before do + current_user.like!(post) + current_user.comment!(post, "hello") + end + + include_examples "current user can't mention himself" + end + + context "current user is a stranger matching a search pattern" do + include_examples "current user can't mention himself" + end + + it "doesn't fail when the post author doesn't match the requested pattern" do + expect(post_service.mentionable_in_comment(post.id, "#{r_str}#{r_str}#{r_str}")).to be_empty + end + + it "renders a commenter with multiple comments only once" do + person = FactoryGirl.create(:person, first_name: "Ro2#{r_str}") + 2.times { FactoryGirl.create(:comment, author: person, post: post) } + expect(post_service.mentionable_in_comment(post.id, person.first_name).length).to eq(1) + end + end + end + + describe "unit test" do + let(:post_service) { PostService.new(alice) } + + before do + expect(post_service).to receive(:find!).and_return(post) + end + + it "calls Person.allowed_to_be_mentioned_in_a_comment_to" do + expect(Person).to receive(:allowed_to_be_mentioned_in_a_comment_to).with(post).and_call_original + post_service.mentionable_in_comment(post.id, "whatever") + end + + it "calls Person.find_by_substring" do + expect(Person).to receive(:find_by_substring).with("whatever").and_call_original + post_service.mentionable_in_comment(post.id, "whatever") + end + + it "calls Person.sort_for_mention_suggestion" do + expect(Person).to receive(:sort_for_mention_suggestion).with(post, alice).and_call_original + post_service.mentionable_in_comment(post.id, "whatever") + end + + it "calls Person.limit" do + expect_any_instance_of(Person::ActiveRecord_Relation).to receive(:limit).with(15).and_call_original + post_service.mentionable_in_comment(post.id, "whatever") + end + + it "contains a constraint on a current user" do + expect(Person).to receive(:allowed_to_be_mentioned_in_a_comment_to) { Person.all } + expect(Person).to receive(:find_by_substring) { Person.all } + expect(Person).to receive(:sort_for_mention_suggestion) { Person.all } + expect(post_service.mentionable_in_comment(post.id, alice.person.first_name)) + .not_to include(alice.person) + end + end + end end