new PostController#mentionable_in_comment action

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|

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

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
+      mentions.joins(:person).merge(Person.allowed_to_be_mentioned_in_a_comment_to(mentionable.parent))
-      if people == :all
-        mentions
-      else
-        mentions.where(person_id: people)
-      end
     end
 
     def mail_job

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!
+    query = find_by_substring(search_str)
-    return none if search_str.blank? || search_str.size < 2
+    return query if query.is_a?(ActiveRecord::NullRelation)
-
-    sql, tokens = search_query_string(search_str)
 
     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
 

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

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

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)

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

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|

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) {

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

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