cleaned up and documented Invitation. moved an action into a User before_create callback

app/models/invitation.rb

@@ -1,62 +1,78 @@
 #   Copyright (c) 2010, Diaspora Inc.  This file is
 #   licensed under the Affero General Public License version 3 or later.  See
 #   the COPYRIGHT file.
-#
+
 class Invitation < ActiveRecord::Base
 
   belongs_to :sender, :class_name => 'User'
   belongs_to :recipient, :class_name => 'User'
   belongs_to :aspect
 
-  validates_presence_of :sender, :recipient, :aspect
+  validates_presence_of :sender,
+                        :recipient,
+                        :aspect
 
-  def self.invite(opts = {})
+  # @param opts [Hash] Takes :identifier, :service, :idenfitier, :from, :message
+  # @return [User]
+  def self.invite(opts={})
     opts[:identifier].downcase! if opts[:identifier]
     # return if the current user is trying to invite himself via email
     return false if opts[:identifier] == opts[:from].email
 
-    existing_user = self.find_existing_user(opts[:service], opts[:identifier])
-
-    if existing_user
+    if existing_user = self.find_existing_user(opts[:service], opts[:identifier])
+      # If the sender of the invitation is already connected to the person
+      # he is inviting, raise an error.
       if opts[:from].contact_for(opts[:from].person)
         raise "You are already connceted to this person"
+
+      # Check whether or not the existing User has already been invited;
+      # and if so, start sharing with the Person.
       elsif not existing_user.invited?
         opts[:from].share_with(existing_user.person, opts[:into])
         return
+
+      # If the sender has already invited the recipient, raise an error.
       elsif Invitation.where(:sender_id => opts[:from].id, :recipient_id => existing_user.id).first
         raise "You already invited this person"
+
+      # When everything checks out, we merge the existing user into the
+      # options hash to pass on to self.create_invitee.
+      else
+        opts.merge(:existing_user => existing_user)
       end
     end
-    opts[:existing_user] = existing_user
+
     create_invitee(opts)
   end
 
+  # @param service [String] String representation of the service invitation provider (i.e. facebook, email)
+  # @param identifier [String] String representation of the reciepients identity on the provider (i.e. 'bob.smith', bob@aol.com)
+  # @return [User]
   def self.find_existing_user(service, identifier)
-    existing_user = User.where(:invitation_service => service,
-                               :invitation_identifier => identifier).first
-    if service == 'email'
-      existing_user ||= User.where(:email => identifier).first
-    else
-      existing_user ||= User.joins(:services).where(:services => {:type => "Services::#{service.titleize}", :uid => identifier}).first
+    unless existing_user = User.where(:invitation_service => service,
+                                      :invitation_identifier => identifier).first
+      if service == 'email'
+        existing_user ||= User.where(:email => identifier).first
+      else
+        existing_user ||= User.joins(:services).where(:services => {:type => "Services::#{service.titleize}", :uid => identifier}).first
+      end
     end
 
     existing_user
   end
 
-  def self.new_user_by_service_and_identifier(service, identifier)
-    result = User.new()
-    result.invitation_service = service
-    result.invitation_identifier = identifier
-    result.email = identifier if service == 'email'
-    result.valid?
-    result
-  end
-
   # @params opts [Hash] Takes :from, :existing_user, :service, :identifier, :message
   # @return [User]
   def self.create_invitee(opts={})
-    invitee = opts[:existing_user] || new_user_by_service_and_identifier(opts[:service], opts[:identifier])
+    invitee = opts[:existing_user]
+    invitee ||= User.new(:invitation_service => opts[:service], :invitation_identifier => opts[:identifier])
+
+    # (dan) I'm not sure why, but we need to call .valid? on our User.
+    invitee.valid?
+
+    # Return a User immediately if an invalid email is passed in
     return invitee if opts[:service] == 'email' && !opts[:identifier].match(Devise.email_regexp)
+
     if invitee.new_record?
       invitee.errors.clear
       invitee.serialized_private_key = User.generate_key if invitee.serialized_private_key.blank?
@@ -65,6 +81,7 @@ class Invitation < ActiveRecord::Base
       return invitee
     end
 
+    # Logic if there is an explicit sender
     if opts[:from]
       invitee.save(:validate => false)
       Invitation.create!(:sender => opts[:from],
@@ -75,9 +92,12 @@ class Invitation < ActiveRecord::Base
     end
     invitee.skip_invitation = (opts[:service] != 'email')
     invitee.invite!
-    log_string = "event=invitation_sent to=#{opts[:identifier]} service=#{opts[:service]} "
-    log_string << "inviter=#{opts[:from].diaspora_handle} inviter_uid=#{opts[:from].id} inviter_created_at_unix=#{opts[:from].created_at.to_i}" if opts[:from]
-    Rails.logger.info(log_string)
+
+    # Logging the invitation action
+    log_hash = {:event => :invitation_sent, :to => opts[:identifier], :service => opts[:service]}
+    log_hash.merge({:inviter => opts[:from].diaspora_handle, :invitier_uid => opts[:from].id, :inviter_created_at_unix => opts[:from].created_at.to_i}) if opts[:from]
+    Rails.logger.info(log_hash)
+
     invitee
   end
 
@@ -85,12 +105,15 @@ class Invitation < ActiveRecord::Base
     recipient.invite!
   end
 
+  # @return Contact
   def share_with!
-    contact = sender.share_with(recipient.person, aspect)
-    destroy if contact
+    if contact = sender.share_with(recipient.person, aspect)
+      self.destroy
+    end
     contact
   end
 
+  # @return [String]
   def recipient_identifier
     if recipient.invitation_service == 'email'
       recipient.invitation_identifier

app/models/user.rb

@@ -46,12 +46,36 @@ class User < ActiveRecord::Base
   has_many :authorizations, :class_name => 'OAuth2::Provider::Models::ActiveRecord::Authorization', :foreign_key => :resource_owner_id
   has_many :applications, :through => :authorizations, :source => :client
 
-  before_save do
-    person.save if person && person.changed?
-  end
-  before_save :guard_unconfirmed_email
+  before_save :guard_unconfirmed_email,
+              :save_person!
 
-  attr_accessible :getting_started, :password, :password_confirmation, :language, :disable_mail
+  before_create :infer_email_from_invitation_provider
+
+  attr_accessible :getting_started,
+                  :password,
+                  :password_confirmation,
+                  :language,
+                  :disable_mail,
+                  :invitation_service,
+                  :invitation_identifier
+
+  # Sometimes we access the person in a strange way and need to do this
+  # @note we should make this method depricated.
+  #
+  # @return [Person]
+  def save_person!
+    self.person.save if self.person && self.person.changed?
+    self.person
+  end
+
+  # Set the User's email to the one they've been invited at, if the user
+  # is being created via an invitation.
+  #
+  # @return [User]
+  def infer_email_from_invitation_provider
+    self.email = self.invitation_identifier if self.invitation_service == 'email'
+    self
+  end
 
   def update_user_preferences(pref_hash)
     if self.disable_mail

spec/models/invitation_spec.rb

@@ -43,32 +43,6 @@ describe Invitation do
     @invitation.message.should == "!"
   end
 
-  describe '.new_user_by_service_and_identifier' do
-    let(:inv) { Invitation.new_user_by_service_and_identifier(@type, @identifier) }
-
-    it 'returns User.new for a non-existent user for email' do
-      @type = "email"
-      @identifier = "maggie@example.org"
-      inv.invitation_identifier.should == @identifier
-      inv.invitation_service.should == 'email'
-      inv.should_not be_persisted
-      lambda {
-        inv.reload
-      }.should raise_error ActiveRecord::RecordNotFound
-    end
-
-    it 'returns User.new for a non-existent user' do
-      @type = "facebook"
-      @identifier = "1234892323"
-      inv.invitation_identifier.should == @identifier
-      inv.invitation_service.should == @type
-      inv.persisted?.should be_false
-      lambda {
-        inv.reload
-      }.should raise_error ActiveRecord::RecordNotFound
-    end
-  end
-
   describe '.find_existing_user' do
     let(:inv) { Invitation.find_existing_user(@type, @identifier) }
 

spec/models/user_spec.rb

@@ -20,6 +20,42 @@ describe User do
 
   end
 
+  context 'callbacks' do
+    describe '#save_person!' do
+      it 'saves the corresponding user if it has changed' do
+        alice.person.url = "http://stuff.com"
+        Person.any_instance.should_receive(:save)
+        alice.save
+      end
+
+      it 'does not save the corresponding user if it has not changed' do
+        Person.any_instance.should_not_receive(:save)
+        alice.save
+      end
+    end
+
+    describe '#infer_email_from_invitation_provider' do
+      it 'sets corresponding email if invitation_service is email' do
+        addr = '12345@alice.com'
+        alice.invitation_service = 'email'
+        alice.invitation_identifier = addr
+        
+        lambda {
+          alice.infer_email_from_invitation_provider
+        }.should change(alice, :email)
+      end
+
+      it 'does not set an email if invitation_service is not email' do
+        addr = '1233123'
+        alice.invitation_service = 'facebook'
+        alice.invitation_identifier = addr
+        
+        lambda {
+          alice.infer_email_from_invitation_provider
+        }.should_not change(alice, :email)
+      end
+    end
+  end
 
   describe 'overwriting people' do
     it 'does not overwrite old users with factory' do