add_person_to_aspect is now add_contact_to_aspect, some methods now take models rather than ids

app/controllers/aspects_controller.rb

@@ -91,7 +91,11 @@ class AspectsController < ApplicationController
   end
 
   def move_contact
-    unless current_user.move_contact( :person_id => params[:person_id], :from => params[:from], :to => params[:to][:to])
+    @person = Person.find(params[:person_id])
+    @from_aspect = current_user.aspects.find(params[:from])
+    @to_aspect = current_user.aspects.find(params[:to][:to])
+
+    unless current_user.move_contact( @person, @from_aspect, @to_aspect)
       flash[:error] = I18n.t 'aspects.move_contact.error',:inspect => params.inspect
     end
     if aspect = current_user.aspect_by_id(params[:to][:to])
@@ -104,30 +108,22 @@ class AspectsController < ApplicationController
   end
 
   def add_to_aspect
-    begin current_user.add_person_to_aspect( params[:person_id], params[:aspect_id])
+    @person = Person.find(params[:person_id])
-
     @aspect = current_user.aspects.find(params[:aspect_id])
-      @aspect_id = @aspect.id
+    @contact = current_user.contact_for(@person)
-      @person_id = params[:person_id]
 
+    current_user.add_contact_to_aspect(@contact, @aspect)
     flash.now[:notice] =  I18n.t 'aspects.add_to_aspect.success'
 
     respond_to do |format|
       format.js { render :json => {
         :button_html => render_to_string(:partial => 'aspects/add_to_aspect',
-                           :locals => {:aspect_id => @aspect_id,
+                         :locals => {:aspect_id => @aspect.id,
-                                       :person_id => @person_id}),
+                                     :person_id => @person.id}),
         :badge_html =>  render_to_string(:partial => 'aspects/aspect_badge',
                             :locals => {:aspect => @aspect})
         }}
-        format.html{ redirect_to aspect_path(@aspect_id)}
+      format.html{ redirect_to aspect_path(@aspect.id)}
-      end
-    rescue Exception => e
-      flash.now[:error] =  I18n.t 'aspects.add_to_aspect.failure'
-      respond_to do |format|
-        format.js  { render :text => e, :status => 403 }
-        format.html{ redirect_to aspect_path(@aspect_id)}
-      end
     end
   end
 

app/controllers/people_controller.rb

@@ -51,6 +51,7 @@ class PeopleController < ApplicationController
 
       @profile = @person.profile
       @contact = current_user.contact_for(@person)
+      @aspects_with_person = []
 
       if @contact
         @aspects_with_person = @contact.aspects

app/controllers/photos_controller.rb

@@ -13,9 +13,13 @@ class PhotosController < ApplicationController
     @person = Person.find_by_id(params[:person_id])
 
     if @person
+      @incoming_request = Request.to(current_user).from(@person).first
+      @outgoing_request = Request.from(current_user).to(@person).first
+
       @profile = @person.profile
       @contact = current_user.contact_for(@person)
       @is_contact = @person != current_user.person && @contact
+      @aspects_with_person = []
 
       if @contact
         @aspects_with_person = @contact.aspects

app/helpers/aspects_helper.rb

@@ -23,11 +23,11 @@ module AspectsHelper
     link_to image_tag('icons/monotone_check_yes.png'), {:controller => "aspects", :action => 'remove_from_aspect', :aspect_id => aspect_id, :person_id => person_id}, :remote => true, :class => 'added button'
   end
 
-  def aspect_membership_button(aspect_id, contact)
+  def aspect_membership_button(aspect_id, contact, person)
-    unless contact.aspect_ids.include?(aspect_id)
+    if contact.nil? || !contact.aspect_ids.include?(aspect_id)
-      add_to_aspect_button(aspect_id, contact.person.id)
+      add_to_aspect_button(aspect_id, person.id)
     else
-      remove_from_aspect_button(aspect_id, contact.person.id)
+      remove_from_aspect_button(aspect_id, person.id)
     end
   end
 end

app/models/user.rb

@@ -96,26 +96,19 @@ class User
     end
   end
 
-  def move_contact(opts = {})
+  def move_contact(person, to_aspect, from_aspect)
-    if opts[:to] == opts[:from]
+    contact = contact_for(person)
+    if to_aspect == from_aspect
       true
-    elsif opts[:person_id] && opts[:to] && opts[:from]
+    elsif add_contact_to_aspect(contact, to_aspect)
-      from_aspect = self.aspects.find_by_id(opts[:from])
+      delete_person_from_aspect(person.id, from_aspect.id)
-
-      if add_person_to_aspect(opts[:person_id], opts[:to])
-        delete_person_from_aspect(opts[:person_id], opts[:from])
-      end
     end
   end
 
-  def add_person_to_aspect(person_id, aspect_id)
+  def add_contact_to_aspect(contact, aspect)
-    contact = contact_for(Person.find(person_id))
+    return true if contact.aspect_ids.include?(aspect.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 connected to" unless contact
-    raise 'Can not add person who is already in the aspect' if aspect.contacts.include?(contact)
     contact.aspects << aspect
     contact.save!
-    aspect.save!
   end
 
   def delete_person_from_aspect(person_id, aspect_id, opts = {})

app/views/people/_aspect_list.haml

@@ -23,13 +23,13 @@
     });
 
 .aspects
-  .badges
+  .badges{:class => ("hidden" if !contact)}
     - for aspect in aspects_with_person
       = render :partial => 'aspects/aspect_badge', :locals => {:aspect => aspect}
     .right
       = link_to "edit aspect membership", "#", :id=> "edit_contact_aspects"
 
-  .edit
+  .edit{:class => ("hidden" if contact)}
     .contact_list
       %ul
         - for aspect in aspects_with_person
@@ -37,14 +37,14 @@
             %span.name
               = link_to aspect.name, aspect
             .right
-              = aspect_membership_button(aspect.id, contact)
+              = aspect_membership_button(aspect.id, contact, person)
 
         - for aspect in aspects_without_person
           %li
             %span.name
               = link_to aspect.name, aspect
             .right
-              = aspect_membership_button(aspect.id, contact)
+              = aspect_membership_button(aspect.id, contact, person)
 
     .right
       = link_to "done editing", "#", :id => "done_contact_aspects"

app/views/shared/_contact_list.html.haml

@@ -11,5 +11,5 @@
         %span.name
           = link_to hash[:person].name, hash[:person]
         .right
-          = aspect_membership_button(aspect_id, hash[:contact])
+          = aspect_membership_button(aspect_id, hash[:contact], hash[:person])
 

public/stylesheets/sass/application.sass

@@ -2016,9 +2016,6 @@ h3,h4
     :max-height auto
     :width 298px
 
-  .edit
-    :display none
-
 .aspects .aspect_badge
   :font
     :size 1em

spec/controllers/aspects_controller_spec.rb

@@ -146,7 +146,12 @@ describe AspectsController do
   end
 
   describe "#move_contact" do
-    let(:opts) { {:person_id => "person_id", :from => "from_aspect_id", :to => {:to => "to_aspect_id"}} }
+    let(:opts) { {
+      :person_id => "person_id",
+      :from => "from_aspect_id",
+      :to =>
+        {:to => "to_aspect_id"}
+    } }
     it 'calls the move_contact_method' do
       pending "need to figure out what is the deal with remote requests"
       @controller.stub!(:current_user).and_return(@user)
@@ -222,8 +227,11 @@ describe AspectsController do
 
   describe "#add_to_aspect" do
     context 'with a non-contact' do
-      it 'creates a pending contact' do
+      before do
-        pending
+        @person = Factory(:person)
+      end
+      it 'calls send_contact_request_to' do
+
       end
     end
     it 'adds the users to the aspect' do
@@ -238,7 +246,7 @@ describe AspectsController do
 
   describe "#remove_from_aspect" do
     it 'removes contacts from an aspect' do
-      @user.add_person_to_aspect( @user2.person.id, @aspect1.id)
+      @user.add_contact_to_aspect(@contact, @aspect1)
       @aspect.reload
       @aspect.contacts.include?(@contact).should be true
       post 'remove_from_aspect', :format => 'js', :person_id => @user2.person.id, :aspect_id => @aspect.id

spec/lib/diaspora/exporter_spec.rb

@@ -51,7 +51,7 @@ describe Diaspora::Exporter do
 
     before do
       connect_users(@user1, @aspect1, @user3, @aspect3)
-      @user1.add_person_to_aspect(@user3.person.id, @aspect.id)
+      @user1.add_contact_to_aspect(@user1.contact_for(@user3.person), @aspect)
       @user1.reload
     end
 

spec/models/aspect_spec.rb

@@ -13,7 +13,6 @@ describe Aspect do
   let(:aspect) {user.aspects.create(:name => 'losers')}
   let(:aspect2) {user2.aspects.create(:name => 'failures')}
   let(:aspect1) {user.aspects.create(:name => 'cats')}
-  let(:not_contact) { Factory(:person, :diaspora_handle => "not@person.com")}
   let(:user3) {make_user}
   let(:aspect3) {user3.aspects.create(:name => "lala")}
 
@@ -96,10 +95,10 @@ describe Aspect do
 
       it 'returns multiple aspects if the person is there' do
         user.reload
-        user.add_person_to_aspect(connected_person.id, aspect1.id)
+        contact = user.contact_for(connected_person)
+        user.add_contact_to_aspect(contact, aspect1)
         aspects = user.aspects_with_person(connected_person)
         aspects.count.should == 2
-        contact = user.contact_for(connected_person)
         aspects.each{ |asp| asp.contacts.include?(contact).should be_true }
         aspects.include?(aspect_without_contact).should be_false
       end
@@ -147,42 +146,34 @@ describe Aspect do
   end
 
   context "aspect management" do
-    let(:contact){user.contact_for(user2.person)}
     before do
       connect_users(user, aspect, user2, aspect2)
       aspect.reload
       user.reload
+      @contact = user.contact_for(user2.person)
     end
 
 
-    describe "#add_person_to_aspect" do
+    describe "#add_contact_to_aspect" do
-      it 'adds the user to the aspect' do
+      it 'adds the contact to the aspect' do
-        aspect1.contacts.include?(contact).should be_false 
+        aspect1.contacts.include?(@contact).should be_false
-        user.add_person_to_aspect(user2.person.id, aspect1.id)
+        user.add_contact_to_aspect(@contact, aspect1)
         aspect1.reload
-        aspect1.contacts.include?(contact).should be_true
+        aspect1.contacts.include?(@contact).should be_true
       end
 
-      it 'raises if its an aspect that the user does not own'do
+      it 'returns true if they are already in the aspect' do
-        proc{user.add_person_to_aspect(user2.person.id, aspect2.id) }.should raise_error /Can not add person to an aspect you do not own/
+        user.add_contact_to_aspect(@contact, aspect).should == true
-      end
-
-      it 'does not allow to have duplicate contacts in an aspect' do
-        proc{user.add_person_to_aspect(not_contact.id, aspect1.id) }.should raise_error /Can not add person you are not connected to/
-      end
-
-      it 'does not allow you to add a person if they are already in the aspect' do
-        proc{user.add_person_to_aspect(user2.person.id, aspect.id) }.should raise_error /Can not add person who is already in the aspect/
       end
     end
 
     describe '#delete_person_from_aspect' do
       it 'deletes a user from the aspect' do
-        user.add_person_to_aspect(user2.person.id, aspect1.id)
+        user.add_contact_to_aspect(@contact, aspect1)
         user.reload
         user.delete_person_from_aspect(user2.person.id, aspect1.id)
         user.reload
-        aspect1.reload.contacts.include?(contact).should be false
+        aspect1.reload.contacts.include?(@contact).should be false
       end
 
       it 'should check to make sure you have the aspect ' do
@@ -190,7 +181,7 @@ describe Aspect do
       end
 
       it 'deletes no posts' do
-         user.add_person_to_aspect(user2.person.id, aspect1.id)
+         user.add_contact_to_aspect(@contact, aspect1)
          user.reload
          user2.post(:status_message, :message => "Hey Dude", :to => aspect2.id)
          lambda{
@@ -203,9 +194,9 @@ describe Aspect do
       end
 
       it 'should allow a force removal of a contact from an aspect' do
-        contact.aspect_ids.should_receive(:count).exactly(0).times
+        @contact.aspect_ids.should_receive(:count).exactly(0).times
 
-        user.add_person_to_aspect(user2.person.id, aspect1.id)
+        user.add_contact_to_aspect(@contact, aspect1)
         user.delete_person_from_aspect(user2.person.id, aspect.id, :force => true)
       end
 
@@ -235,34 +226,29 @@ describe Aspect do
 
       describe '#move_contact' do
         it 'should be able to move a contact from one of users existing aspects to another' do
-          user.move_contact(:person_id => user2.person.id, :from => aspect.id, :to => aspect1.id)
+          user.move_contact(user2.person, aspect1, aspect)
           aspect.reload
           aspect1.reload
 
-          aspect.contacts.include?(contact).should be_false
+          aspect.contacts.include?(@contact).should be_false
-          aspect1.contacts.include?(contact).should be_true
+          aspect1.contacts.include?(@contact).should be_true
         end
 
         it "should not move a person who is not a contact" do
-          proc{ user.move_contact(:person_id => connected_person.id, :from => aspect.id, :to => aspect1.id) }.should raise_error /Can not add person you are not connected to/
+          proc{
+            user.move_contact(connected_person, aspect1, aspect)
+          }.should raise_error
+
           aspect.reload
           aspect1.reload
           aspect.contacts.first(:person_id => connected_person.id).should be_nil
           aspect1.contacts.first(:person_id => connected_person.id).should be_nil
         end
 
-        it "should not move a person to a aspect that's not his" do
-          proc {user.move_contact(:person_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.contacts.include?(contact).should be true
-          aspect2.contacts.include?(contact).should be false
-        end
-
         it 'does not try to delete if add person did not go through' do
-          user.should_receive(:add_person_to_aspect).and_return(false)
+          user.should_receive(:add_contact_to_aspect).and_return(false)
           user.should_not_receive(:delete_person_from_aspect)
-          user.move_contact(:person_id => user2.person.id, :from => aspect.id, :to => aspect1.id)
+          user.move_contact(user2.person, aspect1, aspect)
         end
       end
     end

spec/models/user/connecting_spec.rb

@@ -242,7 +242,9 @@ describe Diaspora::UserModules::Connecting do
       end
 
       it 'should remove the contact from all aspects they are in' do
-        user.add_person_to_aspect(user2.person.id, aspect1.id)
+        user.add_contact_to_aspect(
+          user.contact_for(user2.person),
+          aspect1)
         aspect.reload.contacts.count.should == 1
         aspect1.reload.contacts.count.should == 1
         lambda { user.disconnected_by user2.person }.should change {

spec/models/user/posting_spec.rb

@@ -134,7 +134,8 @@ describe User do
       connect_users(user, aspect, user2, aspect2)
       connect_users(user, aspect, user3, aspect3)
       connect_users(user, aspect1, user4, aspect4)
-      user.add_person_to_aspect(user2.person.id, aspect1.id)
+      contact = user.contact_for(user2.person)
+      user.add_contact_to_aspect(contact, aspect1)
       user.reload
     end
 

spec/models/user/receive_spec.rb

@@ -20,7 +20,8 @@ describe User do
   end
 
   it 'should stream only one message to the everyone aspect when a multi-aspected contacts posts' do
-    user.add_person_to_aspect(user2.person.id, user.aspects.create(:name => "villains").id)
+    contact = user.contact_for(user2.person)
+    user.add_contact_to_aspect(contact, user.aspects.create(:name => "villains"))
     status = user2.post(:status_message, :message => "Users do things", :to => aspect2.id)
     xml = status.to_diaspora_xml
     Diaspora::WebSocket.should_receive(:queue_to_user).exactly(:once)