fix uniqueness validation of login ids

fixes CNVS-7266

test plan:
 * create an SIS user with a login id, then delete them
 * create a non-SIS user with the same login id
 * use an SIS import to resurrect the original user
 * the SIS import should succeed, but warn that it skipped the user
   cause someone else had his login information

Change-Id: I2102107fefb1d20fae6046ebe3a7e0c03416dcc9
Reviewed-on: https://gerrit.instructure.com/22867
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Cody Cutrer <cody@instructure.com>
QA-Review: Jeremy Putnam <jeremyp@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>

Author: ccutrer

app/models/pseudonym.rb

@@ -56,7 +56,7 @@ class Pseudonym < ActiveRecord::Base
     config.validations_scope = [:account_id, :workflow_state]
     config.perishable_token_valid_for = 30.minutes
     config.validates_length_of_login_field_options = {:within => 1..MAX_UNIQUE_ID_LENGTH}
-    config.validates_uniqueness_of_login_field_options = { :case_sensitive => false, :scope => [:account_id, :workflow_state], :if => lambda { |p| p.unique_id_changed? && p.active? } }
+    config.validates_uniqueness_of_login_field_options = { :case_sensitive => false, :scope => [:account_id, :workflow_state], :if => lambda { |p| (p.unique_id_changed? || p.workflow_state_changed?) && p.active? } }
   end
 
   attr_writer :require_password

lib/sis/user_importer.rb

@@ -91,13 +91,15 @@ def process_batch
             pseudo ||= pseudo_by_login
             pseudo ||= @root_account.pseudonyms.active.by_unique_id(email).first if email.present?
 
+            status_is_active = !(status =~ /\Adeleted/i)
+
             if pseudo
-              if pseudo.sis_user_id.present? && pseudo.sis_user_id != user_id
+              if pseudo.sis_user_id && pseudo.sis_user_id != user_id
                 @messages << "user #{pseudo.sis_user_id} has already claimed #{user_id}'s requested login information, skipping"
                 next
               end
-              if !pseudo_by_login.nil? && pseudo.unique_id != login_id
-                @messages << "user #{pseudo_by_login.sis_user_id} has already claimed #{user_id}'s requested login information, skipping"
+              if pseudo_by_login && (pseudo.unique_id != login_id || pseudo != pseudo_by_login && status_is_active)
+                @messages << "user #{pseudo_by_login.sis_user_id || pseudo_by_login.user_id} has already claimed #{user_id}'s requested login information, skipping"
                 next
               end
 
@@ -120,8 +122,6 @@ def process_batch
             should_add_account_associations = false
             should_update_account_associations = false
 
-            status_is_active = !(status =~ /\Adeleted/i)
-
             if !status_is_active && !user.new_record?
               # if this user is deleted, we're just going to make sure the user isn't enrolled in anything in this root account and
               # delete the pseudonym.

spec/lib/sis/csv/user_importer_spec.rb

@@ -955,4 +955,19 @@ def gen_ssha_password(password)
     user_1.pseudonym.should_not == @pseudonym
   end
 
+  it "should error nicely when resurrecting an SIS user that conflicts with an active user" do
+    process_csv_data_cleanly(
+        "user_id,login_id,first_name,last_name,email,status",
+        "user_1,user1,User,Uno,user1@example.com,deleted"
+    )
+    @non_sis_user = user_with_pseudonym(:active_all => 1)
+    @pseudonym = @non_sis_user.pseudonyms.create!(:unique_id => 'user1', :account => @account)
+    importer = process_csv_data(
+        "user_id,login_id,first_name,last_name,email,status",
+        "user_1,user1,User,Uno,user1@example.com,active"
+    )
+    importer.errors.should == []
+    importer.warnings.length.should == 1
+    importer.warnings.last.last.should == "user #{@non_sis_user.id} has already claimed user_1's requested login information, skipping"
+  end
 end