Merge pull request #438 from sfu/feature-sis-user-short_name

Add display name field to SIS User Import

doc/api/sis_csv.md

@@ -79,6 +79,11 @@ otherwise) it will <em>not</em> be overwritten</p></td>
 <td>Last name of the user.</td>
 </tr>
 <tr>
+<td>short_name</td>
+<td>text</td>
+<td>Display name of the user.</td>
+</tr>
+<tr>
 <td>email</td>
 <td>text</td>
 <td>The email address of the user. This might be the same as login_id, but should
@@ -99,10 +104,10 @@ student to be able to log in but just not participate, leave the student
 Sample:
 
 <pre>
-user_id,login_id,password,first_name,last_name,email,status
-01103,bsmith01,,Bob,Smith,bob.smith@myschool.edu,active
-13834,jdoe03,,John,Doe,john.doe@myschool.edu,active
-13aa3,psue01,,Peggy,Sue,peggy.sue@myschool.edu,active
+user_id,login_id,password,first_name,last_name,short_name,email,status
+01103,bsmith01,,Bob,Smith,Bobby Smith,bob.smith@myschool.edu,active
+13834,jdoe03,,John,Doe,,john.doe@myschool.edu,active
+13aa3,psue01,,Peggy,Sue,,peggy.sue@myschool.edu,active
 </pre>
 
 accounts.csv

lib/sis/csv/user_importer.rb

@@ -33,7 +33,7 @@ module SIS
             update_progress
 
             begin
-              importer.add_user(row['user_id'], row['login_id'], row['status'], row['first_name'], row['last_name'], row['email'], row['password'], row['ssha_password'])
+              importer.add_user(row['user_id'], row['login_id'], row['status'], row['first_name'], row['last_name'], row['email'], row['password'], row['ssha_password'], nil, row['short_name'])
             rescue ImportError => e
               messages << "#{e}"
             end

lib/sis/user_importer.rb

@@ -62,14 +62,14 @@ module SIS
         @users_to_update_account_associations = []
       end
 
-      def add_user(user_id, login_id, status, first_name, last_name, email=nil, password=nil, ssha_password=nil, integration_id=nil)
-        @logger.debug("Processing User #{[user_id, login_id, status, first_name, last_name, email, password, ssha_password, integration_id].inspect}")
+      def add_user(user_id, login_id, status, first_name, last_name, email=nil, password=nil, ssha_password=nil, integration_id=nil, short_name=nil)
+        @logger.debug("Processing User #{[user_id, login_id, status, first_name, last_name, email, password, ssha_password, integration_id, short_name].inspect}")
 
         raise ImportError, "No user_id given for a user" if user_id.blank?
         raise ImportError, "No login_id given for user #{user_id}" if login_id.blank?
         raise ImportError, "Improper status for user #{user_id}" unless status =~ /\A(active|deleted)/i
 
-        @batched_users << [user_id.to_s, login_id, status, first_name, last_name, email, password, ssha_password, integration_id]
+        @batched_users << [user_id.to_s, login_id, status, first_name, last_name, email, password, ssha_password, integration_id, short_name]
         process_batch if @batched_users.size >= @updates_every
       end
 
@@ -86,7 +86,7 @@ module SIS
           while !@batched_users.empty? && tx_end_time > Time.now
             user_row = @batched_users.shift
             @logger.debug("Processing User #{user_row.inspect}")
-            user_id, login_id, status, first_name, last_name, email, password, ssha_password, integration_id = user_row
+            user_id, login_id, status, first_name, last_name, email, password, ssha_password, integration_id, short_name = user_row
 
             pseudo = @root_account.pseudonyms.find_by_sis_user_id(user_id.to_s)
             pseudo_by_login = @root_account.pseudonyms.active.by_unique_id(login_id).first
@@ -110,10 +110,14 @@ module SIS
               unless user.stuck_sis_fields.include?(:sortable_name)
                 user.sortable_name = last_name.present? && first_name.present? ? "#{last_name}, #{first_name}" : "#{first_name}#{last_name}"
               end
+              unless user.stuck_sis_fields.include?(:short_name)
+                user.short_name = short_name if short_name.present?
+              end
             else
               user = User.new
               user.name = "#{first_name} #{last_name}"
               user.sortable_name = last_name.present? && first_name.present? ? "#{last_name}, #{first_name}" : "#{first_name}#{last_name}"
+              user.short_name = short_name if short_name.present?
             end
 
             # we just leave all users registered now

spec/lib/sis/csv/user_importer_spec.rb

@@ -35,6 +35,7 @@ describe SIS::CSV::UserImporter do
     user = CommunicationChannel.find_by_path('user@example.com').user
     user.account.should eql(@account)
     user.name.should eql("User Uno")
+    user.short_name.should eql("User Uno")
 
     user.pseudonyms.count.should eql(1)
     pseudonym = user.pseudonyms.first
@@ -51,6 +52,7 @@ describe SIS::CSV::UserImporter do
     user = CommunicationChannel.find_by_path('user@example.com').user
     user.account.should eql(@account)
     user.name.should eql("User Uno 2")
+    user.short_name.should eql("User Uno 2")
 
     user.pseudonyms.count.should eql(1)
     pseudonym = user.pseudonyms.first
@@ -68,6 +70,44 @@ describe SIS::CSV::UserImporter do
     user = CommunicationChannel.find_by_path('user@example.com').user
     user.account.should eql(@account)
     user.name.should eql("My Awesome Name")
+    user.short_name.should eql("My Awesome Name")
+  end
+
+  it "should create new users with display name" do
+    process_csv_data_cleanly(
+        "user_id,login_id,first_name,last_name,short_name,email,status",
+        "user_1,user1,User,Uno,The Uno,user@example.com,active"
+    )
+    user = CommunicationChannel.find_by_path('user@example.com').user
+    user.account.should eql(@account)
+    user.name.should eql("User Uno")
+    user.short_name.should eql("The Uno")
+
+    user.pseudonyms.count.should eql(1)
+    pseudonym = user.pseudonyms.first
+    pseudonym.unique_id.should eql('user1')
+
+    user.communication_channels.count.should eql(1)
+    cc = user.communication_channels.first
+    cc.path.should eql("user@example.com")
+
+    # Field order shouldn't matter
+    process_csv_data_cleanly(
+        "user_id,login_id,first_name,last_name,email,status,short_name",
+        "user_2,user2,User,Dos,user2@example.com,active,The Dos"
+    )
+    user = CommunicationChannel.find_by_path('user2@example.com').user
+    user.account.should eql(@account)
+    user.name.should eql("User Dos")
+    user.short_name.should eql("The Dos")
+
+    user.pseudonyms.count.should eql(1)
+    pseudonym = user.pseudonyms.first
+    pseudonym.unique_id.should eql('user2')
+
+    user.communication_channels.count.should eql(1)
+    cc = user.communication_channels.first
+    cc.path.should eql("user2@example.com")
   end
 
   it "should preserve first name/last name split" do
@@ -82,6 +122,22 @@ describe SIS::CSV::UserImporter do
     user.last_name.should == 'St. Clair'
   end
 
+  it "should tolerate blank first and last names" do
+    process_csv_data_cleanly(
+        "user_id,login_id,first_name,last_name,email,status",
+        "user_1,user1,,,user@example.com,active"
+    )
+    user = CommunicationChannel.find_by_path('user@example.com').user
+    user.name.should eql(" ")
+
+    process_csv_data_cleanly(
+        "user_id,login_id,email,status",
+        "user_2,user2,user2@example.com,active"
+    )
+    user = CommunicationChannel.find_by_path('user2@example.com').user
+    user.name.should eql(" ")
+  end
+
   it "should set passwords and not overwrite current passwords" do
     process_csv_data_cleanly(
       "user_id,login_id,password,first_name,last_name,email,status,ssha_password",
@@ -320,6 +376,20 @@ describe SIS::CSV::UserImporter do
     Pseudonym.find_by_account_id_and_sis_user_id(@account.id, "user_1").user.last_name.should == "Uno-Dos"
   end
 
+  it "should allow a user to update display name specifically" do
+    process_csv_data_cleanly(
+        "user_id,login_id,first_name,last_name,short_name,email,status",
+        "user_1,user1,User,Uno,The Uno,user1@example.com,active"
+    )
+
+    process_csv_data_cleanly(
+        "user_id,login_id,first_name,last_name,short_name,email,status",
+        "user_1,user1,User,Uno,The Uno-Dos,user1@example.com,active"
+    )
+
+    Pseudonym.find_by_account_id_and_sis_user_id(@account.id, "user_1").user.short_name.should == "The Uno-Dos"
+  end
+
   it "should allow a user to update emails specifically" do
     enable_cache do
       now = Time.now
@@ -707,6 +777,29 @@ describe SIS::CSV::UserImporter do
     Pseudonym.find_by_unique_id('user5').should be_nil
   end
 
+  it "should handle display name stickiness" do
+    process_csv_data_cleanly(
+        "user_id,login_id,first_name,last_name,short_name,email,status",
+        "user_1,user1,User,Uno,The Uno,,active"
+    )
+    user = Pseudonym.find_by_unique_id('user1').user
+    user.short_name = 'The Amazing Uno'
+    user.save!
+    process_csv_data_cleanly(
+        "user_id,login_id,first_name,last_name,short_name,email,status",
+        "user_1,user1,User,Uno,The Uno-Dos,,active"
+    )
+    user.reload
+    user.short_name.should == 'The Amazing Uno'
+    process_csv_data_cleanly(
+        "user_id,login_id,first_name,last_name,short_name,email,status",
+        "user_1,user1,User,Uno,The Uno-Dos,,active",
+        {:override_sis_stickiness => true}
+    )
+    user.reload
+    user.short_name.should == 'The Uno-Dos'
+  end
+
   it 'should leave users around always' do
     process_csv_data_cleanly(
       "user_id,login_id,first_name,last_name,email,status",