SIS import: ensure imported users have a name

also, don't remove an existing name if the import doesn't
specify one

test plan:
 - if a SIS import creates a new user and all of the
   following columns are missing or blank, the login_id
   should be used as the name.
   * full_name
   * first_name
   * last_name
   * sortable_name
   * short_name
 - if a SIS import updates an existing user and does not
   specify any of the above columns, the user's name
   should remain unchanged (not blanked out!)

fixes ADMIN-579

Change-Id: Id6297bdac3cf13c7561c713b3cd983af3d1ed3c3
Reviewed-on: https://gerrit.instructure.com/136307
Reviewed-by: Jon Willesen <[email protected]>
QA-Review: Deepeeca Soundarrajan <[email protected]>
Tested-by: Jenkins
Product-Review: Jeremy Stanley <[email protected]>

doc/api/sis_csv.md

@@ -277,8 +277,8 @@ still be provided.</td>
 </tr>
 </table>
 
-<p>The user's name (either first_name and last_name, or full_name) should always
-be provided. Otherwise, the name will be blanked out.</p>
+<p>At least one form of name should be supplied. If a user is being created and no name is given,
+the login_id will be used as the name.</p>
 
 
 <p>When a user is 'deleted' it will delete the login tied to the sis_id.

lib/sis/user_importer.rb

@@ -79,6 +79,36 @@ def any_left_to_process?
         return @batched_users.size > 0
       end
 
+      def infer_user_name(user_row, prior_name = nil)
+        if user_row.full_name.present?
+          user_row.full_name
+        elsif user_row.first_name.present? || user_row.last_name.present?
+          [user_row.first_name, user_row.last_name].join(' ')
+        elsif prior_name.present?
+          prior_name
+        elsif user_row.sortable_name.present?
+          user_row.sortable_name
+        elsif user_row.short_name.present?
+          user_row.short_name
+        elsif user_row.login_id.present?
+          user_row.login_id
+        else
+          raise ImportError, "No name given for user"
+        end
+      end
+
+      def infer_sortable_name(user_row, prior_sortable_name = nil)
+        if user_row.full_name.present?
+          nil # force User model to infer sortable name from the full name
+        elsif user_row.sortable_name.present?
+          user_row.sortable_name
+        elsif user_row.last_name.present? || user_row.first_name.present?
+          [user_row.last_name, user_row.first_name].join(', ')
+        else
+          prior_sortable_name
+        end
+      end
+
       def process_batch
         return unless any_left_to_process?
         transaction_timeout = Setting.get('sis_transaction_seconds', '1').to_i.seconds
@@ -110,24 +140,18 @@ def process_batch
 
               user = pseudo.user
               unless user.stuck_sis_fields.include?(:name)
-                user.name = "#{user_row.first_name} #{user_row.last_name}"
-                user.name = user_row.full_name if user_row.full_name.present?
+                user.name = infer_user_name(user_row, user.name)
               end
               unless user.stuck_sis_fields.include?(:sortable_name)
-                user.sortable_name = user_row.last_name.present? && user_row.first_name.present? ? "#{user_row.last_name}, #{user_row.first_name}" : "#{user_row.first_name}#{user_row.last_name}"
-                user.sortable_name = nil if user_row.full_name.present? # force User model to infer sortable name from the full name
-                user.sortable_name = user_row.sortable_name if user_row.sortable_name.present?
+                user.sortable_name = infer_sortable_name(user_row, user.sortable_name)
               end
               unless user.stuck_sis_fields.include?(:short_name)
                 user.short_name = user_row.short_name if user_row.short_name.present?
               end
             else
               user = User.new
-              user.name = "#{user_row.first_name} #{user_row.last_name}"
-              user.name = user_row.full_name if user_row.full_name.present?
-              user.sortable_name = user_row.last_name.present? && user_row.first_name.present? ? "#{user_row.last_name}, #{user_row.first_name}" : "#{user_row.first_name}#{user_row.last_name}"
-              user.sortable_name = nil if user_row.full_name.present? # force User model to infer sortable name from the full name
-              user.sortable_name = user_row.sortable_name if user_row.sortable_name.present?
+              user.name = infer_user_name(user_row)
+              user.sortable_name = infer_sortable_name(user_row)
               user.short_name = user_row.short_name if user_row.short_name.present?
             end
 

spec/lib/sis/csv/user_importer_spec.rb

@@ -194,20 +194,41 @@ def gen_ssha_password(password)
     expect(user.last_name).to eq 'St. Clair'
   end
 
-  it "should tolerate blank first and last names" do
+  it "uses sortable_name if none of first_name/last_name/full_name is given" do
     process_csv_data_cleanly(
-        "user_id,login_id,first_name,last_name,email,status",
-        "user_1,user1,,,[email protected],active"
+        "user_id,login_id,sortable_name,short_name,email,status",
+        "user_1,user1,blah,bleh,[email protected],active"
     )
-    user = CommunicationChannel.by_path('[email protected]').first.user
-    expect(user.name).to eql(" ")
+    user = Pseudonym.by_unique_id('user1').first.user
+    expect(user.name).to eq 'blah'
+  end
 
+  it "uses short_name is none of first_name/last_name/full_name/sortable_name is given" do
     process_csv_data_cleanly(
-        "user_id,login_id,email,status",
-        "user_2,user2,user2@example.com,active"
+        "user_id,login_id,short_name,email,status",
+        "user_1,user1,bleh,user@example.com,active"
     )
-    user = CommunicationChannel.by_path('[email protected]').first.user
-    expect(user.name).to eql(" ")
+    user = Pseudonym.by_unique_id('user1').first.user
+    expect(user.name).to eq 'bleh'
+  end
+
+  it "uses login_id as a name if no form of name is given" do
+    process_csv_data_cleanly(
+      "user_id,login_id,status",
+      "user_1,user1,active"
+    )
+    user = Pseudonym.by_unique_id('user1').first.user
+    expect(user.name).to eq 'user1'
+  end
+
+  it "should leave the name alone if no name is supplied for an existing user" do
+    user = User.create!(:name => 'Greeble')
+    user.pseudonyms.create!(:account => @account, :sis_user_id => 'greeble', :unique_id => '[email protected]')
+    process_csv_data_cleanly(
+      "user_id,login_id,status",
+      "greeble,[email protected],active"
+    )
+    expect(user.reload.name).to eq 'Greeble'
   end
 
   it "should ignore first and last names if full name is provided" do