20831: Fix tests

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz@curii.com>

diff --git a/services/api/app/controllers/arvados/v1/users_controller.rb b/services/api/app/controllers/arvados/v1/users_controller.rb
index afe2f96cca6237679c8f4cf2ecab7ad9021fe369..8f69c6cf7777e0d4f464b7d4508f6735ac2f279d 100644 (file)
--- a/services/api/app/controllers/arvados/v1/users_controller.rb
+++ b/services/api/app/controllers/arvados/v1/users_controller.rb
@@ -16,7 +16,12 @@ class Arvados::V1::UsersController < ApplicationController
   # records from LoginCluster.
   def batch_update
     @objects = []
-    params[:updates].andand.each do |uuid, attrs|
+    # update_remote_user takes a row lock on the User record, so sort
+    # the keys so we always lock them in the same order.
+    sorted = params[:updates].keys.sort
+    sorted.each do |uuid|
+      attrs = params[:updates][uuid]
+      attrs[:uuid] = uuid
       u = User.update_remote_user nullify_attrs(attrs)
       @objects << u
     end

diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index 3b398100fa7ba54a6696d3ec7b872c7fb136a553..d9040387e9e53de4b9b09e0cb7e04b0bc58e62b1 100644 (file)
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -34,6 +34,7 @@ class User < ArvadosModel
   before_create :set_initial_username, :if => Proc.new {
     username.nil? and email
   }
+  before_create :active_is_not_nil
   after_create :after_ownership_change
   after_create :setup_on_activate
   after_create :add_system_group_permission_link
@@ -382,7 +383,7 @@ SELECT target_uuid, perm_level
   end
 
   def set_initial_username(requested: false)
-    if !requested.is_a?(String) || requested.empty?
+    if (!requested.is_a?(String) || requested.empty?) and email
       email_parts = email.partition("@")
       local_parts = email_parts.first.partition("+")
       if email_parts.any?(&:empty?)
@@ -393,13 +394,20 @@ SELECT target_uuid, perm_level
         requested = email_parts.first
       end
     end
-    requested.sub!(/^[^A-Za-z]+/, "")
-    requested.gsub!(/[^A-Za-z0-9]/, "")
-    unless requested.empty?
+    if requested
+      requested.sub!(/^[^A-Za-z]+/, "")
+      requested.gsub!(/[^A-Za-z0-9]/, "")
+    end
+    unless !requested || requested.empty?
       self.username = find_usable_username_from(requested)
     end
   end
 
+  def active_is_not_nil
+    self.is_active = false if self.is_active.nil?
+    self.is_admin = false if self.is_admin.nil?
+  end
+
   # Move this user's (i.e., self's) owned items to new_owner_uuid and
   # new_user_uuid (for things normally owned directly by the user).
   #
@@ -610,20 +618,18 @@ SELECT target_uuid, perm_level
         end
       end
 
-      if user.username.nil? || user.username == ""
-        # Don't have a username yet, set one
-        user.set_initial_username(requested: remote_user[:username])
-      end
+      user.email = needupdate[:email] if needupdate[:email]
 
       loginCluster = Rails.configuration.Login.LoginCluster
-      if remote_user_prefix != loginCluster
-        # Will only try to change username if upstream is login cluster
+      if user.username.nil? || user.username == ""
+        # Don't have a username yet, set one
+        needupdate[:username] = user.set_initial_username(requested: remote_user[:username])
+      elsif remote_user_prefix != loginCluster
+        # Upstream is not login cluster, don't try to change the
+        # username once set.
         needupdate.delete :username
       end
 
-      needupdate[:is_admin] = false if user.is_admin.nil?
-      needupdate[:is_active] = false if user.is_active.nil?
-
       if needupdate.length > 0
         begin
           user.update!(needupdate)

diff --git a/services/api/test/functional/arvados/v1/users_controller_test.rb b/services/api/test/functional/arvados/v1/users_controller_test.rb
index 16271c9e8f2db71758137850b479633e54136808..fe24e441ce46d7f38bc74e6022a3a671cfe60742 100644 (file)
--- a/services/api/test/functional/arvados/v1/users_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/users_controller_test.rb
@@ -1063,23 +1063,28 @@ The Arvados team.
                 'is_active' => true,
                 'is_admin' => true,
                 'prefs' => {'foo' => 'bar'},
+                'is_invited' => true
               },
               newuuid => {
                 'first_name' => 'noot',
                 'email' => 'root@remot.example.com',
                 'username' => '',
+                'is_invited' => true
               },
               unchanginguuid => {
                 'email' => 'root@unchanging.example.com',
                 'prefs' => {'foo' => {'bar' => 'baz'}},
+                'is_invited' => true
               },
               conflictinguuid1 => {
                 'email' => 'root@conflictingname1.example.com',
-                'username' => 'active'
+                'username' => 'active',
+                'is_invited' => true
               },
               conflictinguuid2 => {
                 'email' => 'root@conflictingname2.example.com',
-                'username' => 'federatedactive'
+                'username' => 'federatedactive',
+                'is_invited' => true
               },
             }})
     assert_response(:success)

diff --git a/services/api/test/integration/users_test.rb b/services/api/test/integration/users_test.rb
index ca143363892cad7065e65d704d1c76bbd7551c83..f8956b21e24a0772899b5c796dd2b2e650fc1e6e 100644 (file)
--- a/services/api/test/integration/users_test.rb
+++ b/services/api/test/integration/users_test.rb
@@ -303,15 +303,15 @@ class UsersTest < ActionDispatch::IntegrationTest
     assert_response :success
     rp = json_response
     assert_not_nil rp["uuid"]
-    assert_not_nil rp["is_active"]
-    assert_nil rp["is_admin"]
+    assert_equal true, rp["is_active"]
+    assert_equal false, rp["is_admin"]
 
     get "/arvados/v1/users/#{rp['uuid']}",
       params: {format: 'json'},
       headers: auth(:admin)
     assert_response :success
     assert_equal rp["uuid"], json_response['uuid']
-    assert_nil json_response['is_admin']
+    assert_equal false, json_response['is_admin']
     assert_equal true, json_response['is_active']
     assert_equal 'foo@example.com', json_response['email']
     assert_equal 'barney', json_response['username']

diff --git a/services/api/test/unit/user_test.rb b/services/api/test/unit/user_test.rb
index 35e6ec9e07afdb153de88f3f8bfe86fd6ddff6a7..97f096d29e65eba59ef12e295547490ff27362aa 100644 (file)
--- a/services/api/test/unit/user_test.rb
+++ b/services/api/test/unit/user_test.rb
@@ -153,12 +153,12 @@ class UserTest < ActiveSupport::TestCase
     assert_equal("active/foo", repositories(:foo).name)
   end
 
-  [[false, 'foo@example.com', true, nil],
-   [false, 'bar@example.com', nil, true],
-   [true, 'foo@example.com', true, nil],
+  [[false, 'foo@example.com', true, false],
+   [false, 'bar@example.com', false, true],
+   [true, 'foo@example.com', true, false],
    [true, 'bar@example.com', true, true],
-   [false, '', nil, nil],
-   [true, '', true, nil]
+   [false, '', false, false],
+   [true, '', true, false]
   ].each do |auto_admin_first_user_config, auto_admin_user_config, foo_should_be_admin, bar_should_be_admin|
     # In each case, 'foo' is created first, then 'bar', then 'bar2', then 'baz'.
     test "auto admin with auto_admin_first=#{auto_admin_first_user_config} auto_admin=#{auto_admin_user_config}" do