Merge pull request #6951 from svbergerem/contacts-search

Modify search to include contacts

Changelog.md

@@ -169,6 +169,7 @@ before.
 * Add 'Be excellent to each other!' to the sidebar [#6914](https://github.com/diaspora/diaspora/pull/6910)
 * Expose Sidekiq dead queue configuration options
 * Properly support pluralization in timeago strings [#6926](https://github.com/diaspora/diaspora/pull/6926)
+* Return all contacts in people search [#6951](https://github.com/diaspora/diaspora/pull/6951)
 
 # 0.5.11.0
 

app/assets/javascripts/app/views/publisher/mention_view.js

@@ -31,7 +31,8 @@ app.views.PublisherMention = app.views.SearchBase.extend({
     app.views.SearchBase.prototype.initialize.call(this, {
       typeaheadInput: this.typeaheadInput,
       customSearch: true,
-      autoselect: true
+      autoselect: true,
+      remoteRoute: "/contacts"
     });
   },
 

app/assets/javascripts/app/views/search_base_view.js

@@ -25,11 +25,6 @@ app.views.SearchBase = app.views.Base.extend({
         return this.bloodhoundTokenizer(datum.name).concat(datum.handle);
       }.bind(this),
       queryTokenizer: Bloodhound.tokenizers.whitespace,
-      prefetch: {
-        url: "/contacts.json",
-        transform: this.transformBloodhoundResponse,
-        cache: false
-      },
       sufficient: 5
     };
 

app/controllers/contacts_controller.rb

@@ -14,11 +14,10 @@ class ContactsController < ApplicationController
       # Used by the mobile site
       format.mobile { set_up_contacts_mobile }
 
-      # Used to populate mentions in the publisher
+      # Used for mentions in the publisher
       format.json {
-        aspect_ids = params[:aspect_ids] || current_user.aspects.map(&:id)
-        @people = Person.all_from_aspects(aspect_ids, current_user).for_json
-        render :json => @people.to_json
+        @people = Person.search(params[:q], current_user, only_contacts: true).limit(15)
+        render json: @people
       }
     end
   end

app/models/person.rb

@@ -56,7 +56,9 @@ class Person < ActiveRecord::Base
   validates :serialized_public_key, :presence => true
   validates :diaspora_handle, :uniqueness => true
 
-  scope :searchable, -> { joins(:profile).where(:profiles => {:searchable => true}) }
+  scope :searchable, -> (user) {
+    joins(:profile).where("profiles.searchable = true OR contacts.user_id = ?", user.id)
+  }
   scope :remote, -> { where('people.owner_id IS NULL') }
   scope :local, -> { where('people.owner_id IS NOT NULL') }
   scope :for_json, -> {
@@ -143,27 +145,23 @@ class Person < ActiveRecord::Base
     [where_clause, q_tokens]
   end
 
-  def self.search(query, user)
-    return self.where("1 = 0") if query.to_s.blank? || query.to_s.length < 2
+  def self.search(search_str, user, only_contacts: false)
+    search_str.strip!
+    return none if search_str.blank? || search_str.size < 2
 
-    sql, tokens = self.search_query_string(query)
+    sql, tokens = search_query_string(search_str)
 
-    Person.searchable.where(sql, *tokens).joins(
-      "LEFT OUTER JOIN contacts ON contacts.user_id = #{user.id} AND contacts.person_id = people.id"
-    ).includes(:profile
-    ).order(search_order)
-  end
+    query = if only_contacts
+              joins(:contacts).where(contacts: {user_id: user.id})
+            else
+              joins(
+                "LEFT OUTER JOIN contacts ON contacts.user_id = #{user.id} AND contacts.person_id = people.id"
+              ).searchable(user)
+            end
 
-  # @return [Array<String>] postgreSQL and mysql deal with null values in orders differently, it seems.
-  def self.search_order
-    @search_order ||= Proc.new {
-      order = if AppConfig.postgres?
-        "ASC"
-      else
-        "DESC"
-      end
-      ["contacts.user_id #{order}", "profiles.last_name ASC", "profiles.first_name ASC"]
-    }.call
+    query.where(sql, *tokens)
+         .includes(:profile)
+         .order(["contacts.user_id IS NULL", "profiles.last_name ASC", "profiles.first_name ASC"])
   end
 
   def name(opts = {})

spec/controllers/contacts_controller_spec.rb

@@ -49,25 +49,26 @@ describe ContactsController, :type => :controller do
       end
     end
 
-    context 'format json' do
-      it 'assumes all aspects if none are specified' do
-        get :index, :format => 'json'
-        expect(assigns[:people].map(&:id)).to match_array(bob.contacts.map { |c| c.person.id })
-        expect(response).to be_success
+    context "format json" do
+      before do
+        @person1 = FactoryGirl.create(:person)
+        bob.share_with(@person1, bob.aspects.first)
+        @person2 = FactoryGirl.create(:person)
       end
 
-      it 'returns the contacts for multiple aspects' do
-        get :index, :aspect_ids => bob.aspect_ids, :format => 'json'
-        expect(assigns[:people].map(&:id)).to match_array(bob.contacts.map { |c| c.person.id })
+      it "succeeds" do
+        get :index, q: @person1.first_name, format: "json"
         expect(response).to be_success
       end
 
-      it 'does not return duplicate contacts' do
-        aspect = bob.aspects.create(:name => 'hilarious people')
-        aspect.contacts << bob.contact_for(eve.person)
-        get :index, :format => 'json', :aspect_ids => bob.aspect_ids
-        expect(assigns[:people].map { |p| p.id }.uniq).to eq(assigns[:people].map { |p| p.id })
-        expect(assigns[:people].map(&:id)).to match_array(bob.contacts.map { |c| c.person.id })
+      it "responds with json" do
+        get :index, q: @person1.first_name, format: "json"
+        expect(response.body).to eq([@person1].to_json)
+      end
+
+      it "only returns contacts" do
+        get :index, q: @person2.first_name, format: "json"
+        expect(response.body).to eq([].to_json)
       end
     end
   end

spec/factories.rb

@@ -52,12 +52,6 @@ FactoryGirl.define do
     end
   end
 
-  factory :searchable_person, :parent => :person do
-    after(:build) do |person|
-      person.profile = FactoryGirl.build(:profile, :person => person, :searchable => true)
-    end
-  end
-
   factory :like do
     association :author, :factory => :person
     association :target, :factory => :status_message

spec/javascripts/app/views/publisher_mention_view_spec.js

@@ -19,6 +19,7 @@ describe("app.views.PublisherMention", function() {
       expect(call.args[0].typeaheadInput.selector).toBe("#publisher .typeahead-mention-box");
       expect(call.args[0].customSearch).toBeTruthy();
       expect(call.args[0].autoselect).toBeTruthy();
+      expect(call.args[0].remoteRoute).toBe("/contacts");
     });
 
     it("calls bindTypeaheadEvents", function() {

spec/models/person_spec.rb

@@ -294,10 +294,11 @@ describe Person, :type => :model do
       user_profile.last_name = "asdji"
       user_profile.save
 
-      @robert_grimm = FactoryGirl.build(:searchable_person)
-      @eugene_weinstein = FactoryGirl.build(:searchable_person)
-      @yevgeniy_dodis = FactoryGirl.build(:searchable_person)
-      @casey_grippi = FactoryGirl.build(:searchable_person)
+      @robert_grimm = FactoryGirl.build(:person)
+      @eugene_weinstein = FactoryGirl.build(:person)
+      @yevgeniy_dodis = FactoryGirl.build(:person)
+      @casey_grippi = FactoryGirl.build(:person)
+      @invisible_person = FactoryGirl.build(:person)
 
       @robert_grimm.profile.first_name = "Robert"
       @robert_grimm.profile.last_name = "Grimm"
@@ -318,7 +319,14 @@ describe Person, :type => :model do
       @casey_grippi.profile.last_name = "Grippi"
       @casey_grippi.profile.save
       @casey_grippi.reload
+
+      @invisible_person.profile.first_name = "Johnson"
+      @invisible_person.profile.last_name = "Invisible"
+      @invisible_person.profile.searchable = false
+      @invisible_person.profile.save
+      @invisible_person.reload
     end
+
     it 'orders results by last name' do
       @robert_grimm.profile.first_name = "AAA"
       @robert_grimm.profile.save!
@@ -367,10 +375,15 @@ describe Person, :type => :model do
       expect(people.first).to eq(@casey_grippi)
     end
 
-    it 'only displays searchable people' do
-      invisible_person = FactoryGirl.build(:person, :profile => FactoryGirl.build(:profile, :searchable => false, :first_name => "johnson"))
-      expect(Person.search("johnson", @user)).not_to include invisible_person
-      expect(Person.search("", @user)).not_to include invisible_person
+    it "doesn't display people that are neither searchable nor contacts" do
+      expect(Person.search("Johnson", @user)).to be_empty
+    end
+
+    it "displays contacts that are not searchable" do
+      @user.contacts.create(person: @invisible_person, aspects: [@user.aspects.first])
+      people = Person.search("Johnson", @user)
+      expect(people.count).to eq(1)
+      expect(people.first).to eq(@invisible_person)
     end
 
     it 'returns results for Diaspora handles' do
@@ -396,6 +409,65 @@ describe Person, :type => :model do
       people = Person.search("AAA", @user)
       expect(people.map { |p| p.name }).to eq([@casey_grippi, @yevgeniy_dodis, @robert_grimm, @eugene_weinstein].map { |p| p.name })
     end
+
+    context "only contacts" do
+      before do
+        @robert_contact = @user.contacts.create(person: @robert_grimm, aspects: [@user.aspects.first])
+        @eugene_contact = @user.contacts.create(person: @eugene_weinstein, aspects: [@user.aspects.first])
+        @invisible_contact = @user.contacts.create(person: @invisible_person, aspects: [@user.aspects.first])
+      end
+
+      it "orders results by last name" do
+        @robert_grimm.profile.first_name = "AAA"
+        @robert_grimm.profile.save!
+
+        @eugene_weinstein.profile.first_name = "AAA"
+        @eugene_weinstein.profile.save!
+
+        @casey_grippi.profile.first_name = "AAA"
+        @casey_grippi.profile.save!
+
+        people = Person.search("AAA", @user, only_contacts: true)
+        expect(people.map(&:name)).to eq([@robert_grimm, @eugene_weinstein].map(&:name))
+      end
+
+      it "returns nothing on an empty query" do
+        people = Person.search("", @user, only_contacts: true)
+        expect(people).to be_empty
+      end
+
+      it "returns nothing on a one-character query" do
+        people = Person.search("i", @user, only_contacts: true)
+        expect(people).to be_empty
+      end
+
+      it "returns results for partial names" do
+        people = Person.search("Eug", @user, only_contacts: true)
+        expect(people.count).to eq(1)
+        expect(people.first).to eq(@eugene_weinstein)
+
+        people = Person.search("wEi", @user, only_contacts: true)
+        expect(people.count).to eq(1)
+        expect(people.first).to eq(@eugene_weinstein)
+
+        @user.contacts.create(person: @casey_grippi, aspects: [@user.aspects.first])
+        people = Person.search("gri", @user, only_contacts: true)
+        expect(people.count).to eq(2)
+        expect(people.first).to eq(@robert_grimm)
+        expect(people.second).to eq(@casey_grippi)
+      end
+
+      it "returns results for full names" do
+        people = Person.search("Robert Grimm", @user, only_contacts: true)
+        expect(people.count).to eq(1)
+        expect(people.first).to eq(@robert_grimm)
+      end
+
+      it "returns results for Diaspora handles" do
+        people = Person.search(@robert_grimm.diaspora_handle, @user, only_contacts: true)
+        expect(people).to eq([@robert_grimm])
+      end
+    end
   end
 
   context 'people finders for webfinger' do