clearify identity validations

Identity.new.valid? should not crash. So validate presence where needed and
skip the other validations if the value is absent.

3 files changed, 33 insertions, 8 deletions

diff --git a/app/models/identity.rb b/app/models/identity.rb
index f2727c8..2f6241c 100644
--- a/app/models/identity.rb
+++ b/app/models/identity.rb
@@ -10,7 +10,9 @@ class Identity < CouchRest::Model::Base
   property :keys, HashWithIndifferentAccess
   property :cert_fingerprints, Hash
 
-  validate :alias_available
+  validates :address, presence: true
+  validate :address_available
+  validates :destination, presence: true, if: :enabled?
   validates :destination, uniqueness: {scope: :address}
   validate :address_local_email
   validate :destination_email
@@ -94,7 +96,11 @@ class Identity < CouchRest::Model::Base
   end
 
   def enabled?
-    self.destination && self.user_id
+    self.user_id
+  end
+
+  def disabled?
+    !enabled?
   end
 
   def disable
@@ -123,12 +129,12 @@ class Identity < CouchRest::Model::Base
 
   # for LoginFormatValidation
   def login
-    self.address.handle
+    address.handle if address.present?
   end
 
   protected
 
-  def alias_available
+  def address_available
     blocking_identities = Identity.by_address.key(address).all
     blocking_identities.delete self
     if self.user
@@ -140,15 +146,18 @@ class Identity < CouchRest::Model::Base
   end
 
   def address_local_email
-    return if address.valid? #this ensures it is a valid local email address
+    # caught by presence validation
+    return if address.blank?
+    return if address.valid?
     address.errors.each do |attribute, error|
       self.errors.add(:address, error)
     end
   end
 
   def destination_email
-    return if destination.nil?   # this identity is disabled
-    return if destination.valid? # this ensures it is Email
+    # caught by presence validation or this identity is disabled
+    return if destination.blank?
+    return if destination.valid?
     destination.errors.each do |attribute, error|
       self.errors.add(:destination, error)
     end
diff --git a/test/integration/api/smtp_cert_test.rb b/test/integration/api/smtp_cert_test.rb
index 04e6f31..f72362d 100644
--- a/test/integration/api/smtp_cert_test.rb
+++ b/test/integration/api/smtp_cert_test.rb
@@ -34,7 +34,7 @@ class SmtpCertTest < ApiIntegrationTest
     cert = OpenSSL::X509::Certificate.new(get_response.body)
     fingerprint = OpenSSL::Digest::SHA1.hexdigest(cert.to_der).scan(/../).join(':')
     today = DateTime.now.to_date.to_s
-    assert_equal({fingerprint => today}, @user.identity.cert_fingerprints)
+    assert_equal({fingerprint => today}, @user.reload.identity.cert_fingerprints)
   end
 
   test "fetching smtp certs requires email account" do
diff --git a/test/unit/identity_test.rb b/test/unit/identity_test.rb
index 54c0657..49b2075 100644
--- a/test/unit/identity_test.rb
+++ b/test/unit/identity_test.rb
@@ -7,6 +7,22 @@ class IdentityTest < ActiveSupport::TestCase
     @user = find_record :user
   end
 
+  test "blank identity does not crash on valid?" do
+    id = Identity.new
+    assert !id.valid?
+  end
+
+  test "enabled identity requires destination" do
+    id = Identity.new user: @user, address: @user.email_address
+    assert !id.valid?
+    assert_equal ["can't be blank"], id.errors[:destination]
+  end
+
+  test "disabled identity requires no destination" do
+    id = Identity.new address: @user.email_address
+    assert id.valid?
+  end
+
   test "initial identity for a user" do
     id = Identity.for(@user)
     assert_equal @user.email_address, id.address