From 7083d2aa03a78dd4726c1e6c769fa7c3835145c0 Mon Sep 17 00:00:00 2001 From: ilya <ilya@laptop.(none)> Date: Fri, 22 Oct 2010 16:16:39 -0700 Subject: [PATCH] Moving friendships into a 'Contact' proxy object. --- app/models/aspect.rb | 3 +- app/models/contact.rb | 16 ++++++ app/models/user.rb | 37 +++++++------ lib/diaspora/user/friending.rb | 29 +++++++---- lib/diaspora/user/querying.rb | 15 ++---- lib/diaspora/user/receiving.rb | 33 ++++++------ spec/lib/diaspora/parser_spec.rb | 5 +- spec/misc_spec.rb | 14 ++--- spec/models/album_spec.rb | 2 +- spec/models/aspect_spec.rb | 44 +++++++++------- spec/models/contact_spec.rb | 31 +++++++++++ spec/models/user/receive_spec.rb | 3 +- spec/models/user/user_friending_spec.rb | 69 +++++++++++++++++++------ spec/models/user/visible_posts_spec.rb | 11 ---- 14 files changed, 198 insertions(+), 114 deletions(-) create mode 100644 app/models/contact.rb create mode 100644 spec/models/contact_spec.rb diff --git a/app/models/aspect.rb b/app/models/aspect.rb index 0fceedeadd..782e9ea7ca 100644 --- a/app/models/aspect.rb +++ b/app/models/aspect.rb @@ -6,11 +6,10 @@ class Aspect include MongoMapper::Document key :name, String - key :person_ids, Array key :request_ids, Array key :post_ids, Array - many :people, :in => :person_ids, :class_name => 'Person' + many :people, :foreign_key => 'aspect_ids', :class_name => 'Contact' many :requests, :in => :request_ids, :class_name => 'Request' many :posts, :in => :post_ids, :class_name => 'Post' diff --git a/app/models/contact.rb b/app/models/contact.rb new file mode 100644 index 0000000000..1b5a4b050d --- /dev/null +++ b/app/models/contact.rb @@ -0,0 +1,16 @@ +# Copyright (c) 2010, Diaspora Inc. This file is +# licensed under the Affero General Public License version 3 or later. See +# the COPYRIGHT file. + +class Contact + include MongoMapper::Document + belongs_to :user + validates_presence_of :user + + belongs_to :person + validates_presence_of :person + + key :aspect_ids, Array, :typecast => 'ObjectId' + many :aspects, :in => :aspect_ids, :class_name => 'Aspect' + validates_presence_of :aspects +end diff --git a/app/models/user.rb b/app/models/user.rb index 343cc2d76d..697c38263f 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -55,7 +55,7 @@ class User end many :inviters, :in => :inviter_ids, :class_name => 'User' - many :friends, :in => :friend_ids, :class_name => 'Person' + many :friends, :in => :friend_ids, :class_name => 'Contact' many :visible_people, :in => :visible_person_ids, :class_name => 'Person' # One of these needs to go many :pending_requests, :in => :pending_request_ids, :class_name => 'Request' many :raw_visible_posts, :in => :visible_post_ids, :class_name => 'Post' @@ -119,22 +119,27 @@ class User end def add_person_to_aspect(person_id, aspect_id, opts = {}) - raise "Can not add person to an aspect you do not own" unless aspect = self.aspects.find_by_id(aspect_id) - raise "Can not add person you are not friends with" unless person = self.find_friend_by_id(person_id) - raise 'Can not add person who is already in the aspect' if aspect.person_ids.include?(person_id) - aspect.people << person + contact = contact_for(Person.find(person_id)) + raise "Can not add person to an aspect you do not own" unless aspect = self.aspects.find_by_id(aspect_id) + raise "Can not add person you are not friends with" unless contact + raise 'Can not add person who is already in the aspect' if aspect.people.include?(contact) + contact.aspects << aspect opts[:posts] ||= self.raw_visible_posts.all(:person_id => person_id) aspect.posts += opts[:posts] + contact.save aspect.save end def delete_person_from_aspect(person_id, aspect_id, opts = {}) - raise "Can not delete a person from an aspect you do not own" unless aspect = self.aspects.find_by_id(aspect_id) - aspect.person_ids.delete(person_id.to_id) + aspect = Aspect.find(aspect_id) + raise "Can not delete a person from an aspect you do not own" unless aspect.user == self + contact = contact_for Person.find(person_id) + contact.aspects.delete aspect opts[:posts] ||= aspect.posts.all(:person_id => person_id) aspect.posts -= opts[:posts] aspect.save + contact.save end ######## Posting ######## @@ -211,17 +216,17 @@ class User aspects = self.aspects.find_all_by_id(aspect_ids) end #send to the aspects - target_people = [] + target_contacts = [] aspects.each { |aspect| aspect.posts << post aspect.save - target_people = target_people | aspect.people + target_contacts = target_contacts | aspect.people } push_to_hub(post) if post.respond_to?(:public) && post.public - push_to_people(post, target_people) + push_to_people(post, target_contacts.map{|c|c.person}) end def push_to_people(post, people) @@ -274,7 +279,7 @@ class User if owns? comment.post comment.post_creator_signature = comment.sign_with_key(encryption_key) comment.save - push_to_people comment, people_in_aspects(aspects_with_post(comment.post.id)) + push_to_people comment, contacts_in_aspects(aspects_with_post(comment.post.id)).map{|c|c.person} elsif owns? comment comment.save push_to_people comment, [comment.post.person] @@ -288,7 +293,7 @@ class User post.unsocket_from_uid(self.id, :aspect_ids => aspect_ids) if post.respond_to? :unsocket_from_uid retraction = Retraction.for(post) - push_to_people retraction, people_in_aspects(aspects_with_post(post.id)) + push_to_people retraction, contacts_in_aspects(aspects_with_post(post.id)).map{|c| c.person} retraction end @@ -439,11 +444,11 @@ class User end def unfriend_everyone - friends.each { |friend| - if friend.owner? - friend.owner.unfriended_by self.person + friends.each { |contact| + if contact.person.owner? + contact.person.owner.unfriended_by self.person else - self.unfriend friend + self.unfriend contact end } end diff --git a/lib/diaspora/user/friending.rb b/lib/diaspora/user/friending.rb index 4a19ffd688..0eef9bebab 100644 --- a/lib/diaspora/user/friending.rb +++ b/lib/diaspora/user/friending.rb @@ -11,7 +11,7 @@ module Diaspora raise "You have already sent a friend request to that person!" if self.pending_requests.detect{ |x| x.destination_url == desired_friend.receive_url } raise "You are already friends with that person!" if self.friends.detect{ - |x| x.receive_url == desired_friend.receive_url} + |x| x.person.receive_url == desired_friend.receive_url} request = Request.instantiate( :to => desired_friend.receive_url, :from => self.person, @@ -91,13 +91,15 @@ module Diaspora end def remove_friend(bad_friend) - raise "Friend not deleted" unless self.friend_ids.delete( bad_friend.id ) - aspects.each{|aspect| - if aspect.person_ids.delete( bad_friend.id ) - aspect.posts.delete_if { |post| - post.person_id == bad_friend.id} - end} - self.save + contact = contact_for(bad_friend) + raise "Friend not deleted" unless self.friend_ids.delete(contact.id) + contact.aspects.each{|aspect| + contact.aspects.delete(aspect) + aspect.posts.delete_if { |post| + post.person_id == bad_friend.id + } + aspect.save + } self.raw_visible_posts.find_all_by_person_id( bad_friend.id ).each{|post| self.visible_post_ids.delete( post.id ) @@ -105,7 +107,7 @@ module Diaspora (post.user_refs > 0 || post.person.owner.nil? == false) ? post.save : post.destroy } self.save - + contact.destroy bad_friend.save end @@ -115,12 +117,17 @@ module Diaspora end def activate_friend(person, aspect) - aspect.people << person - friends << person + new_contact = Contact.create(:user => self, :person => person, :aspects => [aspect]) + new_contact.aspects << aspect + friends << new_contact save aspect.save end + def contact_for(person) + friends.first(:person_id => person.id) + end + def request_from_me?(request) (pending_request_ids.include?(request.id.to_id)) && (request.callback_url == person.receive_url) end diff --git a/lib/diaspora/user/querying.rb b/lib/diaspora/user/querying.rb index b22c8b73ed..f1fbc7704d 100644 --- a/lib/diaspora/user/querying.rb +++ b/lib/diaspora/user/querying.rb @@ -24,7 +24,7 @@ module Diaspora def visible_person_by_id( id ) id = id.to_id return self.person if id == self.person.id - result = friends.detect{|x| x.id == id } + result = friends.first(:person_id => id).person result = visible_people.detect{|x| x.id == id } unless result result end @@ -38,22 +38,17 @@ module Diaspora aspects.detect{|x| x.id == id } end - def find_friend_by_id(id) - id = id.to_id - friends.detect{|x| x.id == id } - end - def aspects_with_post( id ) self.aspects.find_all_by_post_ids( id.to_id ) end def aspects_with_person person - aspects.all(:person_ids => person.id) + contact_for(person).aspects end - def people_in_aspects aspects - aspects.inject([]) do |found_people,aspect| - found_people | aspect.people + def contacts_in_aspects aspects + aspects.inject([]) do |contacts,aspect| + contacts | aspect.people end end diff --git a/lib/diaspora/user/receiving.rb b/lib/diaspora/user/receiving.rb index f459862ba1..985346703f 100644 --- a/lib/diaspora/user/receiving.rb +++ b/lib/diaspora/user/receiving.rb @@ -16,26 +16,23 @@ module Diaspora sender_in_xml = sender(object, xml) - if (salmon_author == sender_in_xml) - - if object.is_a? Request - receive_request object, sender_in_xml - elsif self.friend_ids.include? salmon_author.id - if object.is_a? Retraction - receive_retraction object, xml - elsif object.is_a? Profile - receive_profile object, xml - elsif object.is_a?(Comment) - receive_comment object, xml - else - receive_post object, xml - end - else - raise "Not friends with that person" - end + if (salmon_author != sender_in_xml) + raise "Malicious Post, #{salmon_author.real_name} with id #{salmon_author.id} is sending a #{object.class} as #{sender_in_xml.real_name} with id #{sender_in_xml.id} " + end + + if object.is_a? Request + return receive_request object, sender_in_xml + end + raise "Not friends with that person" unless self.contact_for(salmon_author) + if object.is_a? Retraction + receive_retraction object, xml + elsif object.is_a? Profile + receive_profile object, xml + elsif object.is_a?(Comment) + receive_comment object, xml else - raise "Malicious Post, #{salmon_author.real_name} with id #{salmon_author.id} is sending a #{object.class} as #{sender_in_xml.real_name} with id #{sender_in_xml.id} " + receive_post object, xml end end diff --git a/spec/lib/diaspora/parser_spec.rb b/spec/lib/diaspora/parser_spec.rb index f97a514c37..ea71f1621c 100644 --- a/spec/lib/diaspora/parser_spec.rb +++ b/spec/lib/diaspora/parser_spec.rb @@ -98,8 +98,9 @@ describe Diaspora::Parser do user.reload aspect.reload - aspect.people.include?(new_person).should be true - user.friends.include?(new_person).should be true + new_contact = user.contact_for(new_person) + aspect.people.include?(new_contact).should be true + user.friends.include?(new_contact).should be true end it 'should process retraction for a person' do diff --git a/spec/misc_spec.rb b/spec/misc_spec.rb index 12f7837c14..75580af7bf 100644 --- a/spec/misc_spec.rb +++ b/spec/misc_spec.rb @@ -18,18 +18,20 @@ describe 'making sure the spec runner works' do @user2 = Factory.create(:user) @aspect2 = @user2.aspect(:name => "bruisers") friend_users(@user1, @aspect1, @user2, @aspect2) - @user1.reload - @aspect1.reload - @user2.reload - @aspect2.reload end it 'makes the first user friends with the second' do - @aspect1.people.include?(@user2.person).should be_true + contact = @user1.contact_for @user2.person + @user1.friends.should include contact + @aspect1.people.should include contact + contact.aspects.include?( @aspect1 ).should be true end it 'makes the second user friends with the first' do - @aspect2.people.include?(@user1.person).should be_true + contact = @user2.contact_for @user1.person + @user2.friends.should include contact + @aspect2.people.should include contact + contact.aspects.include?( @aspect2 ).should be true end end end diff --git a/spec/models/album_spec.rb b/spec/models/album_spec.rb index a47c6026de..e5b93593a4 100644 --- a/spec/models/album_spec.rb +++ b/spec/models/album_spec.rb @@ -20,7 +20,7 @@ describe Album do end it 'has many photos' do - album.associations[:photos].type == :many + album.associations[:photos].type.should == :many end context 'when an album has two attached images' do diff --git a/spec/models/aspect_spec.rb b/spec/models/aspect_spec.rb index 336d22cb32..e99856df24 100644 --- a/spec/models/aspect_spec.rb +++ b/spec/models/aspect_spec.rb @@ -29,15 +29,17 @@ describe Aspect do end it 'should be able to have other users' do - aspect.people << user2.person - aspect.people.include?(user.person).should be false - aspect.people.include?(user2.person).should be true + Contact.create(:user => user, :person => user2.person, :aspects => [aspect]) + aspect.people.first(:person_id => user.person.id).should be_nil + aspect.people.first(:person_id => user2.person.id).should_not be_nil aspect.people.size.should == 1 end it 'should be able to have users and people' do - aspect.people << user2.person - aspect.people << friend_2 + contact1 = Contact.create(:user => user, :person => user2.person, :aspects => [aspect]) + contact2 = Contact.create(:user => user, :person => friend_2, :aspects => [aspect]) + aspect.people.include?(contact1).should be_true + aspect.people.include?(contact2).should be_true aspect.save.should be_true end end @@ -69,7 +71,7 @@ describe Aspect do end it 'should have people' do - aspect.people.all.include?(friend).should be true + aspect.people.first(:person_id => friend.id).should be_true aspect.people.size.should == 1 end @@ -87,8 +89,9 @@ describe Aspect do user.add_person_to_aspect(friend.id, aspect1.id) aspects = user.aspects_with_person(friend) aspects.count.should == 2 - aspects.each{ |asp| asp.people.include?(friend) } - aspects.should_not include aspect_without_friend + contact = user.contact_for(friend) + aspects.each{ |asp| asp.people.include?(contact).should be_true } + aspects.include?(aspect_without_friend).should be_false end end end @@ -140,6 +143,7 @@ describe Aspect do end context "aspect management" do + let(:contact){user.contact_for(user2.person)} before do friend_users(user, aspect, user2, aspect2) aspect.reload @@ -149,10 +153,10 @@ describe Aspect do describe "#add_person_to_aspect" do it 'adds the user to the aspect' do - aspect1.people.should_not include user2.person + aspect1.people.should_not include contact user.add_person_to_aspect(user2.person.id, aspect1.id) aspect1.reload - aspect1.people.should include user2.person + aspect1.people.should include contact end it 'raises if its an aspect that the user does not own'do @@ -172,10 +176,10 @@ describe Aspect do it 'deletes a user from the aspect' do user.add_person_to_aspect(user2.person.id, aspect1.id) user.reload - user.aspects.find_by_id(aspect1.id).people.include?(user2.person).should be true + aspect1.reload.people.include?(contact).should be true user.delete_person_from_aspect(user2.person.id, aspect1.id) user.reload - user.aspects.find_by_id(aspect1.id).people.include?(user2.person).should be false + aspect1.reload.people.include?(contact).should be false end it 'should check to make sure you have the aspect ' do @@ -189,9 +193,7 @@ describe Aspect do let(:message2){user3.post(:status_message, :message => "other post", :to => aspect3.id)} before do - friend_users(user, aspect, user3, aspect3) user.receive message.to_diaspora_xml, user2.person - user.receive message2.to_diaspora_xml, user3.person aspect.reload @post_count = aspect.posts.count @post_count1 = aspect1.posts.count @@ -213,6 +215,8 @@ describe Aspect do end it 'should not delete other peoples posts' do + friend_users(user, aspect, user3, aspect3) + user.receive message2.to_diaspora_xml, user3.person user.delete_person_from_aspect(user2.person.id, aspect.id) aspect.reload aspect.posts.should == [message2] @@ -224,24 +228,24 @@ describe Aspect do aspect.reload aspect1.reload - aspect.person_ids.include?(user2.person.id).should be false - aspect1.people.include?(user2.person).should be true + aspect.people.include?(contact).should be false + aspect1.people.include?(contact).should be true end it "should not move a person who is not a friend" do proc{ user.move_friend(:friend_id => friend.id, :from => aspect.id, :to => aspect1.id) }.should raise_error /Can not add person you are not friends with/ aspect.reload aspect1.reload - aspect.people.include?(friend).should be false - aspect1.people.include?(friend).should be false + aspect.people.first(:person_id => friend.id).should be_nil + aspect1.people.first(:person_id => friend.id).should be_nil end it "should not move a person to a aspect that's not his" do proc {user.move_friend(:friend_id => user2.person.id, :from => aspect.id, :to => aspect2.id )}.should raise_error /Can not add person to an aspect you do not own/ aspect.reload aspect2.reload - aspect.people.include?(user2.person).should be true - aspect2.people.include?(user2.person).should be false + aspect.people.include?(contact).should be true + aspect2.people.include?(contact).should be false end it 'should move all posts by that user to the new aspect' do diff --git a/spec/models/contact_spec.rb b/spec/models/contact_spec.rb new file mode 100644 index 0000000000..5bc480289c --- /dev/null +++ b/spec/models/contact_spec.rb @@ -0,0 +1,31 @@ +# Copyright (c) 2010, Diaspora Inc. This file is +# licensed under the Affero General Public License version 3 or later. See +# the COPYRIGHT file. + +require 'spec_helper' + +describe Contact do + describe 'validations' do + let(:contact){Contact.new} + + it 'requires a user' do + contact.valid? + contact.errors.full_messages.should include "User can't be blank" + end + + it 'requires a person' do + contact.valid? + contact.errors.full_messages.should include "Person can't be blank" + end + + it 'has many aspects' do + contact.associations[:aspects].type.should == :many + end + + it 'has at least one aspect' do + contact.valid? + contact.errors.full_messages.should include "Aspects can't be blank" + end + + end +end diff --git a/spec/models/user/receive_spec.rb b/spec/models/user/receive_spec.rb index ab83b8c5db..e339c56fc8 100644 --- a/spec/models/user/receive_spec.rb +++ b/spec/models/user/receive_spec.rb @@ -23,10 +23,9 @@ describe User do status_message = user2.post :status_message, :message => 'store this!', :to => aspect2.id xml = status_message.to_diaspora_xml - user2.destroy + user2.delete status_message.destroy - user lambda {user.receive xml , user2.person}.should change(Post,:count).by(1) end diff --git a/spec/models/user/user_friending_spec.rb b/spec/models/user/user_friending_spec.rb index 63a472031c..cda6104e1c 100644 --- a/spec/models/user/user_friending_spec.rb +++ b/spec/models/user/user_friending_spec.rb @@ -13,6 +13,7 @@ describe Diaspora::UserModules::Friending do let(:person_one) { Factory.create :person } let(:person_two) { Factory.create :person } + let(:person_three) { Factory.create :person } let(:user2) { Factory.create :user } let(:aspect2) { user2.aspect(:name => "aspect two") } @@ -24,6 +25,41 @@ describe Diaspora::UserModules::Friending do Notifier.stub!(:request_accepted).and_return(deliverable) end + + describe '#contact_for' do + + it 'returns a contact' do + contact = Contact.create(:user => user, :person => person_one, :aspects => [aspect]) + user.friends << contact + user.contact_for(person_one).should be_true + end + + it 'returns the correct contact' do + contact = Contact.create(:user => user, :person => person_one, :aspects => [aspect]) + user.friends << contact + + contact2 = Contact.create(:user => user, :person => person_two, :aspects => [aspect]) + user.friends << contact2 + + contact3 = Contact.create(:user => user, :person => person_three, :aspects => [aspect]) + user.friends << contact3 + + user.contact_for(person_two).person.should == person_two + end + + it 'returns nil for a non-contact' do + user.contact_for(person_one).should be_nil + end + + it 'returns nil when someone else has contact with the target' do + contact = Contact.create(:user => user, :person => person_one, :aspects => [aspect]) + user.friends << contact + user2.contact_for(person_one).should be_nil + end + + end + + context 'friend requesting' do it "should assign a request to a aspect" do aspect.requests.size.should == 0 @@ -52,10 +88,9 @@ describe Diaspora::UserModules::Friending do end it 'should not be able to friend request an existing friend' do - user.friends << friend - user.save + friend_users(user, aspect, user2, aspect2) - proc { user.send_friend_request_to(friend, aspect) }.should raise_error + proc { user.send_friend_request_to(user2.person, aspect1) }.should raise_error end it 'should not be able to friend request yourself' do @@ -99,14 +134,14 @@ describe Diaspora::UserModules::Friending do proc { user2.accept_friend_request @request_three.id, aspect2.id }.should_not change(Person, :count) - user2.friends.include?(user.person).should be true + user2.contact_for(user.person).should_not be_nil end it 'should not delete the ignored user on the same pod' do proc { user2.ignore_friend_request @request_three.id }.should_not change(Person, :count) - user2.friends.include?(user.person).should be false + user2.contact_for(user.person).should be_nil end it 'sends an email to the receiving user' do @@ -118,6 +153,7 @@ describe Diaspora::UserModules::Friending do end + context 'Two users receiving requests from one person' do before do user.receive @req_xml, person_one @@ -127,28 +163,28 @@ describe Diaspora::UserModules::Friending do describe '#accept_friend_request' do it 'should both users should befriend the same person' do user.accept_friend_request @request.id, aspect.id - user.friends.include?(person_one).should be true + user.contact_for(person_one).should_not be_nil user2.accept_friend_request @request_two.id, aspect2.id - user2.friends.include?(person_one).should be true + user2.contact_for(person_one).should_not be_nil end it 'should keep the person around if one of the users rejects him' do user.accept_friend_request @request.id, aspect.id - user.friends.include?(person_one).should be true + user.contact_for(person_one).should_not be_nil user2.ignore_friend_request @request_two.id - user2.friends.include?(person_one).should be false + user2.contact_for(person_one).should be_nil end end it 'should keep the person around if the users ignores them' do user.ignore_friend_request user.pending_requests.first.id - user.friends.include?(person_one).should be false + user.contact_for(person_one).should be_nil user2.ignore_friend_request user2.pending_requests.first.id #@request_two.id - user2.friends.include?(person_one).should be false + user2.contact_for(person_one).should be_nil end end @@ -178,12 +214,12 @@ describe Diaspora::UserModules::Friending do user.accept_friend_request @request.id, aspect.id user.pending_requests.size.should be 1 user.friends.size.should be 1 - user.friends.include?(person_one).should be true + user.contact_for(person_one).should_not be_nil user.ignore_friend_request @request_two.id user.pending_requests.size.should be 0 user.friends.size.should be 1 - user.friends.include?(person_two).should be false + user.contact_for(person_two).should be_nil end end @@ -193,8 +229,9 @@ describe Diaspora::UserModules::Friending do end it 'should unfriend the other user on the same seed' do - lambda { user2.unfriend user.person }.should change { - user2.friends.count }.by(-1) + lambda { + user2.unfriend user.person }.should change { + user2.reload.friends.count }.by(-1) aspect2.reload.people.count.should == 0 end @@ -206,6 +243,8 @@ describe Diaspora::UserModules::Friending do it 'should remove the friend from all aspects they are in' do user.add_person_to_aspect(user2.person.id, aspect1.id) + aspect.reload.people.count.should == 1 + aspect1.reload.people.count.should == 1 lambda { user.unfriended_by user2.person }.should change { user.friends.count }.by(-1) aspect.reload.people.count.should == 0 diff --git a/spec/models/user/visible_posts_spec.rb b/spec/models/user/visible_posts_spec.rb index c14927a4bb..77490681ab 100644 --- a/spec/models/user/visible_posts_spec.rb +++ b/spec/models/user/visible_posts_spec.rb @@ -73,17 +73,6 @@ describe User do end end - describe '#find_friend_by_id' do - it 'should find a friend' do - friend_users(user, first_aspect, user2, user2.aspects.first) - user.find_friend_by_id(user2.person.id).should == user2.person - end - - it 'should not find a non-friend' do - user = Factory :user - user.find_friend_by_id(user2.person.id).should be nil - end - end end describe '#albums_by_aspect' do let!(:first_aspect) {user2.aspect(:name => 'bruisers')} -- GitLab