From da532b4d0a1939bbfa063beaffc53582aa3907d6 Mon Sep 17 00:00:00 2001
From: Lucas Di Pentima <lucas.dipentima@curii.com>
Date: Wed, 6 Jul 2022 18:02:50 -0300
Subject: [PATCH] 18858: Improves test & fixes discovered bug by said improved
 test.

Also, fixes update decision function to avoid unnecessary calls on
users that are already inactive but the CSV file list them with admin=true.

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas.dipentima@curii.com>
---
 tools/sync-users/sync-users.go      |  3 +--
 tools/sync-users/sync-users_test.go | 38 +++++++++++++++++++++++------
 2 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/tools/sync-users/sync-users.go b/tools/sync-users/sync-users.go
index 45b6e96b87..4b679ffe18 100644
--- a/tools/sync-users/sync-users.go
+++ b/tools/sync-users/sync-users.go
@@ -372,7 +372,7 @@ type userRecord struct {
 
 func needsUpdating(user arvados.User, record userRecord) bool {
 	userData := userRecord{"", user.FirstName, user.LastName, user.IsActive, user.IsAdmin}
-	recordData := userRecord{"", record.FirstName, record.LastName, record.Active, record.Admin}
+	recordData := userRecord{"", record.FirstName, record.LastName, record.Active, record.Active && record.Admin}
 	return userData != recordData
 }
 
@@ -444,7 +444,6 @@ func ProcessRecord(cfg *ConfigParams, record userRecord, userIDToUUID map[string
 			}
 		}
 	}
-	allUsers[record.UserID] = user
 	if createRequired {
 		log.Printf("Created user %q", record.UserID)
 	}
diff --git a/tools/sync-users/sync-users_test.go b/tools/sync-users/sync-users_test.go
index e5cbb77d16..976d625996 100644
--- a/tools/sync-users/sync-users_test.go
+++ b/tools/sync-users/sync-users_test.go
@@ -349,6 +349,24 @@ func (s *TestSuite) TestUserCreationAndUpdate(c *C) {
 
 func (s *TestSuite) TestDeactivateUnlisted(c *C) {
 	localUserUuidRegex := regexp.MustCompile(fmt.Sprintf("^%s-tpzed-[0-9a-z]{15}$", s.cfg.ClusterID))
+
+	var user1 arvados.User
+	for _, nr := range []int{1, 2} {
+		var newUser arvados.User
+		err := CreateUser(s.cfg.Client, &newUser, map[string]string{
+			"email":      fmt.Sprintf("user%d@example.com", nr),
+			"first_name": "Example",
+			"last_name":  fmt.Sprintf("User%d", nr),
+			"is_active":  "true",
+			"is_admin":   "false",
+		})
+		c.Assert(err, IsNil)
+		c.Assert(newUser.IsActive, Equals, true)
+		if nr == 1 {
+			user1 = newUser // for later confirmation
+		}
+	}
+
 	users, err := ListUsers(s.cfg.Client)
 	c.Assert(err, IsNil)
 	previouslyActiveUsers := 0
@@ -364,19 +382,21 @@ func (s *TestSuite) TestDeactivateUnlisted(c *C) {
 			previouslyActiveUsers++
 		}
 	}
-	// At least 3 active users: System root, Anonymous and the current user.
-	// Other active users should exist from fixture.
+	// Active users: System root, Anonymous, current user and the
+	// ones just created (other active users may exist from fixture).
 	c.Logf("Initial active users count: %d", previouslyActiveUsers)
-	c.Assert(previouslyActiveUsers > 3, Equals, true)
+	c.Assert(previouslyActiveUsers > 5, Equals, true)
 
-	s.cfg.DeactivateUnlisted = true
-	s.cfg.Verbose = true
+	// Here we omit user2@example.com from the CSV file.
 	data := [][]string{
-		{"user1@example.com", "Example", "User1", "0", "0"},
+		{"user1@example.com", "Example", "User1", "1", "0"},
 	}
 	tmpfile, err := MakeTempCSVFile(data)
 	c.Assert(err, IsNil)
 	defer os.Remove(tmpfile.Name())
+
+	s.cfg.DeactivateUnlisted = true
+	s.cfg.Verbose = true
 	s.cfg.Path = tmpfile.Name()
 	err = doMain(s.cfg)
 	c.Assert(err, IsNil)
@@ -388,6 +408,7 @@ func (s *TestSuite) TestDeactivateUnlisted(c *C) {
 		fmt.Sprintf("%s-tpzed-000000000000000", s.cfg.ClusterID): true,
 		fmt.Sprintf("%s-tpzed-anonymouspublic", s.cfg.ClusterID): true,
 		s.cfg.CurrentUser.UUID: true,
+		user1.UUID:             true,
 	}
 	remainingActiveUUIDs := map[string]bool{}
 	seenUserEmails := map[string]bool{}
@@ -404,9 +425,10 @@ func (s *TestSuite) TestDeactivateUnlisted(c *C) {
 			currentlyActiveUsers++
 		}
 	}
-	// 3 active users remaining: System root, Anonymous and the current user.
+	// 4 active users remaining: System root, Anonymous, the current user
+	// and user1@example.com
 	c.Logf("Active local users remaining: %v", remainingActiveUUIDs)
-	c.Assert(currentlyActiveUsers, Equals, 3)
+	c.Assert(currentlyActiveUsers, Equals, 4)
 }
 
 func (s *TestSuite) TestFailOnDuplicatedEmails(c *C) {
-- 
2.30.2