From bd31ff3ee397235aaf66376e9795520973210a8e Mon Sep 17 00:00:00 2001
From: Raphael <raphael@joindiaspora.com>
Date: Wed, 15 Dec 2010 10:48:07 -0800
Subject: [PATCH] Fix request validations, build.

---
 app/models/request.rb                       | 35 +++--------
 app/views/people/_profile_sidebar.html.haml |  2 +-
 config/locales/diaspora/en.yml              | 14 +++--
 spec/models/request_spec.rb                 | 68 +++++++++++----------
 spec/models/user/connecting_spec.rb         | 11 ++--
 5 files changed, 59 insertions(+), 71 deletions(-)

diff --git a/app/models/request.rb b/app/models/request.rb
index 1456785ae..b01680173 100644
--- a/app/models/request.rb
+++ b/app/models/request.rb
@@ -17,17 +17,16 @@ class Request
   belongs_to :to,   :class => Person
 
   validates_presence_of :from, :to
-  validate :not_already_connected_if_sent
-  validate :not_already_connected_if_not_sent
-  validate :no_pending_request, :if => :sent
+  validates_uniqueness_of :from_id, :scope => :to_id
+  validate :not_already_connected
   validate :not_friending_yourself
 
-  scope :from, lambda { |person| 
+  scope :from, lambda { |person|
     target = (person.is_a?(User) ? person.person : person)
     where(:from_id => target.id)
   }
 
-  scope :to, lambda { |person| 
+  scope :to, lambda { |person|
     target = (person.is_a?(User) ? person.person : person)
     where(:to_id => target.id)
   }
@@ -36,8 +35,7 @@ class Request
   def self.instantiate(opts = {})
     self.new(:from => opts[:from],
              :to   => opts[:to],
-             :into => opts[:into],
-             :sent => true)
+             :into => opts[:into])
   end
 
   def reverse_for accepting_user
@@ -75,30 +73,15 @@ class Request
     requests.map{|r| {:request => r, :sender => senders_hash[r.from_id]}}
   end
   private
-  def no_pending_request
-    if Request.first(:from_id => from_id, :to_id => to_id)
-      errors[:base] << 'You have already sent a request to that person'
-    end
-  end
 
-  def not_already_connected_if_sent
-    if self.sent
-      if Contact.first(:user_id => self.from.owner_id, :person_id => self.to.id)
-        errors[:base] << 'You have already connected to this person'
-      end
-    end
-  end
-
-  def not_already_connected_if_not_sent
-    unless self.sent
-      if Contact.first(:user_id => self.to.owner_id, :person_id => self.from.id)
-        errors[:base] << 'You have already connected to this person'
-      end
+  def not_already_connected
+    if Contact.first(:user_id => self.to.owner_id, :person_id => self.from.id)
+      errors[:base] << 'You have already connected to this person'
     end
   end
 
   def not_friending_yourself
-    if self.to == self.from 
+    if self.to == self.from
       errors[:base] << 'You can not friend yourself'
     end
   end
diff --git a/app/views/people/_profile_sidebar.html.haml b/app/views/people/_profile_sidebar.html.haml
index 95b416d2f..2f9e0ed6c 100644
--- a/app/views/people/_profile_sidebar.html.haml
+++ b/app/views/people/_profile_sidebar.html.haml
@@ -35,7 +35,7 @@
       %br
       %hr{:style=>"width:300px;"}
 
-  -if contact || person == current_user.person
+  -if (contact && !contact.pending?) || person == current_user.person
     %ul#profile_information
       %li
         %h3 #{t('.bio')}
diff --git a/config/locales/diaspora/en.yml b/config/locales/diaspora/en.yml
index 5b0ee8872..b3c9d0125 100644
--- a/config/locales/diaspora/en.yml
+++ b/config/locales/diaspora/en.yml
@@ -44,19 +44,23 @@ en:
               user:
                   attributes:
                       person:
-                          invalid: "is invalid"
+                          invalid: "is invalid."
                       username:
-                          taken: "is already taken"
+                          taken: "is already taken."
                       email:
-                          taken: "is already taken"
+                          taken: "is already taken."
               person:
                   attributes:
                       diaspora_handle:
-                          taken: "is already taken"
+                          taken: "is already taken."
               contact:
                   attributes:
                       person_id:
-                          taken: "must be unique among this user's contacts"
+                          taken: "must be unique among this user's contacts."
+              request:
+                  attributes:
+                      from_id:
+                          taken: "is a duplicate of a pre-existing request."
   application:
       helper:
           unknown_person: "unknown person"
diff --git a/spec/models/request_spec.rb b/spec/models/request_spec.rb
index ca3ff5edf..0786a8286 100644
--- a/spec/models/request_spec.rb
+++ b/spec/models/request_spec.rb
@@ -5,21 +5,23 @@
 require 'spec_helper'
 
 describe Request do
-  let(:user)   { make_user }
-  let(:user2)  { make_user }
-  let(:person) { Factory :person }
-  let(:aspect) { user.aspects.create(:name => "dudes") }
-  let(:request){ user.send_contact_request_to user2.person, aspect }
+  before do
+    @user    = make_user
+    @user2   = make_user
+    @person  = Factory :person
+    @aspect  = @user.aspects.create(:name => "dudes")
+    @aspect2 = @user2.aspects.create(:name => "Snoozers")
+  end
 
   describe 'validations' do
     before do
-      @request = Request.instantiate(:from => user.person, :to => user2.person, :into => aspect)
+      @request = Request.instantiate(:from => @user.person, :to => @user2.person, :into => @aspect)
     end
     it 'is valid' do
       @request.should be_valid
-      @request.from.should == user.person
-      @request.to.should   == user2.person
-      @request.into.should == aspect
+      @request.from.should == @user.person
+      @request.to.should   == @user2.person
+      @request.into.should == @aspect
     end
     it 'is from a person' do
       @request.from = nil
@@ -33,49 +35,49 @@ describe Request do
       @request.into = nil
       @request.should be_valid
     end
+    it 'is not from an existing friend' do
+      Contact.create(:user => @user2, :person => @user.person, :aspects => [@aspect2])
+      @request.should_not be_valid
+    end
     it 'is not a duplicate of an existing pending request' do
-      request
-      @request.should_not be_valid
+      @request.save
+      duplicate_request = Request.instantiate(:from => @user.person, :to => @user2.person, :into => @aspect)
+      duplicate_request.should_not be_valid
     end
-    it 'is not to an existing friend' do
-      connect_users(user, aspect, user2, user2.aspects.create(:name => 'new aspect'))
-      @request.should_not be_valid
-    end
-
     it 'is not to yourself' do
-      @request = Request.instantiate(:from => user.person, :to => user.person, :into => aspect)
+      @request = Request.instantiate(:from => @user.person, :to => @user.person, :into => @aspect)
       @request.should_not be_valid
     end
   end
 
   describe 'scopes' do
     before do
-      @request = Request.instantiate(:from => user.person, :to => user2.person, :into => aspect)
+      @request = Request.instantiate(:from => @user.person, :to => @user2.person, :into => @aspect)
       @request.save
     end
     describe '.from' do
       it 'returns requests from a person' do
-        query = Request.from(user.person)
+        query = Request.from(@user.person)
         query.first.should == @request
       end
 
       it 'returns requests from a user' do
-        query = Request.from(user)
+        query = Request.from(@user)
         query.first.should == @request
       end
     end
     describe '.to' do
       it 'returns requests to a person' do
-        query = Request.to(user2.person)
+        query = Request.to(@user2.person)
         query.first.should == @request
       end
       it 'returns requests to a user' do
-        query = Request.to(user2)
+        query = Request.to(@user2)
         query.first.should == @request
       end
     end
     it 'chains' do
-      Request.from(user).to(user2.person).first.should == @request
+      Request.from(@user).to(@user2.person).first.should == @request
     end
   end
 
@@ -90,7 +92,7 @@ describe Request do
       @hash = @hashes.first
     end
     it 'gives back requests' do
-      @hash[:request].should == Request.from(@user2).to(@user).first(:sent => false)
+      @hash[:request].should == Request.from(@user2).to(@user).first
     end
     it 'gives back people' do
       @hash[:sender].should == @user2.person
@@ -101,24 +103,24 @@ describe Request do
   end
   describe 'xml' do
     before do
-      @request = Request.new(:from => user.person, :to => user2.person, :into => aspect)
+      @request = Request.new(:from => @user.person, :to => @user2.person, :into => @aspect)
       @xml = @request.to_xml.to_s
     end
     describe 'serialization' do
       it 'should not generate xml for the User as a Person' do
-        @xml.should_not include user.person.profile.first_name
+        @xml.should_not include @user.person.profile.first_name
       end
 
       it 'should serialize the handle and not the sender' do
-        @xml.should include user.person.diaspora_handle
+        @xml.should include @user.person.diaspora_handle
       end
 
       it 'serializes the intended recipient handle' do
-        @xml.should include user2.person.diaspora_handle
+        @xml.should include @user2.person.diaspora_handle
       end
 
       it 'should not serialize the exported key' do
-        @xml.should_not include user.person.exported_key
+        @xml.should_not include @user.person.exported_key
       end
 
       it 'does not serialize the id' do
@@ -131,10 +133,10 @@ describe Request do
         @marshalled = Request.from_xml @xml
       end
       it 'marshals the sender' do
-        @marshalled.from.should == user.person
+        @marshalled.from.should == @user.person
       end
       it 'marshals the recipient' do
-        @marshalled.to.should == user2.person
+        @marshalled.to.should == @user2.person
       end
       it 'knows nothing about the aspect' do
         @marshalled.into.should be_nil
@@ -146,10 +148,10 @@ describe Request do
         @marshalled = Diaspora::Parser.from_xml @d_xml
       end
       it 'marshals the sender' do
-        @marshalled.from.should == user.person
+        @marshalled.from.should == @user.person
       end
       it 'marshals the recipient' do
-        @marshalled.to.should == user2.person
+        @marshalled.to.should == @user2.person
       end
       it 'knows nothing about the aspect' do
         @marshalled.into.should be_nil
diff --git a/spec/models/user/connecting_spec.rb b/spec/models/user/connecting_spec.rb
index d34ce3a30..2d63fc408 100644
--- a/spec/models/user/connecting_spec.rb
+++ b/spec/models/user/connecting_spec.rb
@@ -28,7 +28,7 @@ describe Diaspora::UserModules::Connecting do
 
     it 'should not be able to contact request no-one' do
       proc {
-        user.send_contact_request_to(nil, aspect) 
+        user.send_contact_request_to(nil, aspect)
       }.should raise_error(MongoMapper::DocumentNotValid)
     end
     it 'creates a pending contact' do
@@ -93,11 +93,10 @@ describe Diaspora::UserModules::Connecting do
       let(:request2_for_user) {Request.instantiate(:to => user.person, :from => person_one)}
       let(:request_from_myself) {Request.instantiate(:to => user.person, :from => user.person)}
       before do
-        request_for_user.save
         user.receive(request_for_user.to_diaspora_xml, person)
-        @received_request = Request.from(person).to(user.person).first(:sent => false)
+        @received_request = Request.from(person).to(user.person).first
         user.receive(request2_for_user.to_diaspora_xml, person_one)
-        @received_request2 = Request.from(person_one).to(user.person).first(:sent => false)
+        @received_request2 = Request.from(person_one).to(user.person).first
         user.reload
       end
 
@@ -107,7 +106,7 @@ describe Diaspora::UserModules::Connecting do
         }.should change(Request, :count ).by(-1)
       end
       it 'should be able to ignore a pending contact request' do
-        proc { user.ignore_contact_request(@received_request.id) 
+        proc { user.ignore_contact_request(@received_request.id)
         }.should change(Request, :count ).by(-1)
       end
 
@@ -230,7 +229,7 @@ describe Diaspora::UserModules::Connecting do
       end
 
       it 'should disconnect the other user on the same seed' do
-        lambda { 
+        lambda {
           user2.disconnect user.person }.should change {
           user2.reload.contacts.count }.by(-1)
         aspect2.reload.contacts.count.should == 0