Merge pull request #7001 from svbergerem/fix-conversation-recipient-suggestions

Fix conversations autoSuggest showing non-mutual contacts

Changelog.md

@@ -167,6 +167,7 @@ The command will report queues that still have jobs and launch sidekiq process f
 * Add aspects to the aspect membership dropdown when creating them on the getting started page [#6864](https://github.com/diaspora/diaspora/pull/6864)
 * Strip markdown from message preview in conversations list [#6923](https://github.com/diaspora/diaspora/pull/6923)
 * Improve tag stream performance [#6903](https://github.com/diaspora/diaspora/pull/6903)
+* Only show mutual contacts in conversations auto suggestions [#7001](https://github.com/diaspora/diaspora/pull/7001)
 
 ## Features
 * Support color themes [#6033](https://github.com/diaspora/diaspora/pull/6033)

app/controllers/conversations_controller.rb

@@ -104,7 +104,7 @@ class ConversationsController < ApplicationController
   private
 
   def contacts_data
-    current_user.contacts.sharing.joins(person: :profile)
+    current_user.contacts.mutual.joins(person: :profile)
       .pluck(*%w(contacts.id profiles.first_name profiles.last_name people.diaspora_handle))
       .map {|contact_id, *name_attrs|
         {value: contact_id, name: ERB::Util.h(Person.name_from_attrs(*name_attrs)) }

app/models/contact.rb

@@ -33,6 +33,8 @@ class Contact < ActiveRecord::Base
   # contact.receiving is true when contact.user is sharing with contact.person
   scope :receiving, -> { where(receiving: true) }
 
+  scope :mutual, -> { sharing.receiving }
+
   scope :for_a_stream, -> { includes(:aspects, person: :profile).order("profiles.last_name ASC") }
 
   scope :only_sharing, -> { sharing.where(receiving: false) }

spec/controllers/conversations_controller_spec.rb

@@ -23,10 +23,13 @@ describe ConversationsController, :type => :controller do
     end
 
     it "assigns a json list of contacts that are sharing with the person" do
+      sharing_user = FactoryGirl.create(:user_with_aspect)
+      sharing_user.share_with(alice.person, sharing_user.aspects.first)
       get :new, :modal => true
-      expect(assigns(:contacts_json)).to include(alice.contacts.where(:sharing => true).first.person.name)
+      expect(assigns(:contacts_json)).to include(alice.contacts.where(sharing: true, receiving: true).first.person.name)
       alice.contacts << Contact.new(:person_id => eve.person.id, :user_id => alice.id, :sharing => false, :receiving => true)
-      expect(assigns(:contacts_json)).not_to include(alice.contacts.where(:sharing => false).first.person.name)
+      expect(assigns(:contacts_json)).not_to include(alice.contacts.where(sharing: false).first.person.name)
+      expect(assigns(:contacts_json)).not_to include(alice.contacts.where(receiving: false).first.person.name)
     end
 
     it "assigns a contact if passed a contact id" do

spec/models/contact_spec.rb

@@ -85,10 +85,15 @@ describe Contact, type: :model do
       it "returns contacts with sharing true" do
         expect {
           alice.contacts.create!(sharing: true, person: FactoryGirl.create(:person))
-          alice.contacts.create!(sharing: false, person: FactoryGirl.create(:person))
         }.to change {
           Contact.sharing.count
         }.by(1)
+
+        expect {
+          alice.contacts.create!(sharing: false, person: FactoryGirl.create(:person))
+        }.to change {
+          Contact.sharing.count
+        }.by(0)
       end
     end
 
@@ -96,23 +101,50 @@ describe Contact, type: :model do
       it "returns contacts with sharing true" do
         expect {
           alice.contacts.create!(receiving: true, person: FactoryGirl.build(:person))
+        }.to change {
+          Contact.receiving.count
+        }.by(1)
+
+        expect {
           alice.contacts.create!(receiving: false, person: FactoryGirl.build(:person))
         }.to change {
           Contact.receiving.count
+        }.by(0)
+      end
+    end
+
+    describe "mutual" do
+      it "returns contacts with sharing true and receiving true" do
+        expect {
+          alice.contacts.create!(receiving: true, sharing: true, person: FactoryGirl.build(:person))
+        }.to change {
+          Contact.mutual.count
         }.by(1)
+
+        expect {
+          alice.contacts.create!(receiving: false, sharing: true, person: FactoryGirl.build(:person))
+          alice.contacts.create!(receiving: true, sharing: false, person: FactoryGirl.build(:person))
+        }.to change {
+          Contact.mutual.count
+        }.by(0)
       end
     end
 
     describe "only_sharing" do
       it "returns contacts with sharing true and receiving false" do
         expect {
-          alice.contacts.create!(receiving: true, sharing: true, person: FactoryGirl.build(:person))
           alice.contacts.create!(receiving: false, sharing: true, person: FactoryGirl.build(:person))
           alice.contacts.create!(receiving: false, sharing: true, person: FactoryGirl.build(:person))
-          alice.contacts.create!(receiving: true, sharing: false, person: FactoryGirl.build(:person))
         }.to change {
-          Contact.receiving.count
+          Contact.only_sharing.count
         }.by(2)
+
+        expect {
+          alice.contacts.create!(receiving: true, sharing: true, person: FactoryGirl.build(:person))
+          alice.contacts.create!(receiving: true, sharing: false, person: FactoryGirl.build(:person))
+        }.to change {
+          Contact.only_sharing.count
+        }.by(0)
       end
     end