Made pull request reviewer assignment via command always exclude PR a…

…uthor (#12758)

.ci/magician/cmd/membership_checker_test.go

@@ -57,7 +57,7 @@ func TestExecMembershipChecker_GooglerFlow(t *testing.T) {
 		userType:           github.GooglerUserType,
 		calledMethods:      make(map[string][][]any),
 		requestedReviewers: []github.User{github.User{Login: "reviewer1"}},
-		previousReviewers:  []github.User{github.User{Login: github.GetRandomReviewer()}, github.User{Login: "reviewer3"}},
+		previousReviewers:  []github.User{github.User{Login: github.GetRandomReviewer(nil)}, github.User{Login: "reviewer3"}},
 	}
 	cb := &mockCloudBuild{
 		calledMethods: make(map[string][][]any),
@@ -83,8 +83,8 @@ func TestExecMembershipChecker_AmbiguousUserFlow(t *testing.T) {
 		},
 		userType:           github.CommunityUserType,
 		calledMethods:      make(map[string][][]any),
-		requestedReviewers: []github.User{github.User{Login: github.GetRandomReviewer()}},
-		previousReviewers:  []github.User{github.User{Login: github.GetRandomReviewer()}, github.User{Login: "reviewer3"}},
+		requestedReviewers: []github.User{github.User{Login: github.GetRandomReviewer(nil)}},
+		previousReviewers:  []github.User{github.User{Login: github.GetRandomReviewer(nil)}, github.User{Login: "reviewer3"}},
 	}
 	cb := &mockCloudBuild{
 		calledMethods: make(map[string][][]any),

.ci/magician/cmd/reassign_reviewer.go

@@ -58,29 +58,44 @@ var reassignReviewerCmd = &cobra.Command{
 }
 
 func execReassignReviewer(prNumber, newPrimaryReviewer string, gh GithubClient) error {
+	pullRequest, err := gh.GetPullRequest(prNumber)
+	if err != nil {
+		return err
+	}
 	comments, err := gh.GetPullRequestComments(prNumber)
 	if err != nil {
 		return err
 	}
 
 	reviewerComment, currentReviewer := github.FindReviewerComment(comments)
+	if newPrimaryReviewer == "" {
+		newPrimaryReviewer = github.GetRandomReviewer([]string{currentReviewer, pullRequest.User.Login})
+	}
+
+	if newPrimaryReviewer == "" {
+		return errors.New("no primary reviewer found")
+	}
+	if newPrimaryReviewer == currentReviewer {
+		return fmt.Errorf("primary reviewer is already %s", newPrimaryReviewer)
+	}
+
+	fmt.Println("New primary reviewer is ", newPrimaryReviewer)
+	comment := github.FormatReviewerComment(newPrimaryReviewer)
 
 	if currentReviewer == "" {
 		fmt.Println("No reviewer comment found, creating one")
-		newPrimaryReviewer, err = createReviewComment(prNumber, newPrimaryReviewer, gh)
+		err := gh.PostComment(prNumber, comment)
 		if err != nil {
 			return err
 		}
 	} else {
-		fmt.Println("Reassigning to random reviewer")
-		newPrimaryReviewer, err = updateReviewComment(prNumber, currentReviewer, newPrimaryReviewer, reviewerComment.ID, gh)
+		fmt.Println("Updating reviewer comment")
+		err := gh.UpdateComment(prNumber, comment, reviewerComment.ID)
 		if err != nil {
 			return err
 		}
 	}
 
-	fmt.Println("New primary reviewer is ", newPrimaryReviewer)
-
 	err = gh.RequestPullRequestReviewers(prNumber, []string{newPrimaryReviewer})
 	if err != nil {
 		return err
@@ -89,38 +104,6 @@ func execReassignReviewer(prNumber, newPrimaryReviewer string, gh GithubClient)
 	return nil
 }
 
-func createReviewComment(prNumber, newPrimaryReviewer string, gh GithubClient) (string, error) {
-	if newPrimaryReviewer == "" {
-		newPrimaryReviewer = github.GetRandomReviewer()
-	}
-
-	if newPrimaryReviewer == "" {
-		return "", errors.New("no primary reviewer found")
-	}
-
-	err := gh.PostComment(prNumber, github.FormatReviewerComment(newPrimaryReviewer))
-	if err != nil {
-		return "", err
-	}
-	return newPrimaryReviewer, nil
-}
-
-func updateReviewComment(prNumber, currentReviewer, newPrimaryReviewer string, reviewerCommentID int, gh GithubClient) (string, error) {
-	if newPrimaryReviewer == "" {
-		newPrimaryReviewer = github.GetNewRandomReviewer(currentReviewer)
-	}
-
-	if currentReviewer == newPrimaryReviewer {
-		return newPrimaryReviewer, fmt.Errorf("primary reviewer is already %s", newPrimaryReviewer)
-	}
-
-	err := gh.UpdateComment(prNumber, github.FormatReviewerComment(newPrimaryReviewer), reviewerCommentID)
-	if err != nil {
-		return "", err
-	}
-	return newPrimaryReviewer, nil
-}
-
 func init() {
 	rootCmd.AddCommand(reassignReviewerCmd)
 }

.ci/magician/cmd/request_reviewer_test.go

@@ -24,7 +24,7 @@ import (
 )
 
 func TestExecRequestReviewer(t *testing.T) {
-	availableReviewers := github.AvailableReviewers()
+	availableReviewers := github.AvailableReviewers(nil)
 	if len(availableReviewers) < 3 {
 		t.Fatalf("not enough available reviewers (%v) to run TestExecRequestReviewer (need at least 3)", availableReviewers)
 	}

.ci/magician/cmd/scheduled_pr_reminders_test.go

@@ -11,7 +11,7 @@ import (
 )
 
 func TestNotificationState(t *testing.T) {
-	availableReviewers := membership.AvailableReviewers()
+	availableReviewers := membership.AvailableReviewers(nil)
 	firstCoreReviewer := availableReviewers[0]
 	secondCoreReviewer := availableReviewers[1]
 	cases := map[string]struct {
@@ -964,7 +964,7 @@ func TestShouldNotify(t *testing.T) {
 }
 
 func TestFormatReminderComment(t *testing.T) {
-	availableReviewers := membership.AvailableReviewers()
+	availableReviewers := membership.AvailableReviewers(nil)
 	firstCoreReviewer := availableReviewers[0]
 	secondCoreReviewer := availableReviewers[1]
 	cases := map[string]struct {

.ci/magician/github/membership.go

@@ -102,27 +102,20 @@ func isOrgMember(author, org, githubToken string) bool {
 	return err == nil
 }
 
-func GetRandomReviewer() string {
-	availableReviewers := AvailableReviewers()
+// GetRandomReviewer returns a random available reviewer (optionally excluding some people from the reviewer pool)
+func GetRandomReviewer(excludedReviewers []string) string {
+	availableReviewers := AvailableReviewers(excludedReviewers)
 	reviewer := availableReviewers[rand.Intn(len(availableReviewers))]
 	return reviewer
 }
 
-// Return a random reviewer other than the old reviewer
-func GetNewRandomReviewer(oldReviewer string) string {
-	availableReviewers := AvailableReviewers()
-	availableReviewers = utils.Removes(availableReviewers, []string{oldReviewer})
-	reviewer := availableReviewers[rand.Intn(len(availableReviewers))]
-	return reviewer
-}
-
-func AvailableReviewers() []string {
-	return available(time.Now(), maps.Keys(reviewerRotation), onVacationReviewers)
+func AvailableReviewers(excludedReviewers []string) []string {
+	return available(time.Now(), maps.Keys(reviewerRotation), onVacationReviewers, excludedReviewers)
 }
 
-func available(nowTime time.Time, allReviewers []string, vacationList []onVacationReviewer) []string {
-	onVacationList := onVacation(nowTime, vacationList)
-	return utils.Removes(allReviewers, onVacationList)
+func available(nowTime time.Time, allReviewers []string, vacationList []onVacationReviewer, excludedReviewers []string) []string {
+	excludedReviewers = append(excludedReviewers, onVacation(nowTime, vacationList)...)
+	return utils.Removes(allReviewers, excludedReviewers)
 }
 
 func onVacation(nowTime time.Time, vacationList []onVacationReviewer) []string {

.ci/magician/github/membership_test.go

@@ -49,11 +49,12 @@ func TestAvailable(t *testing.T) {
 	}
 
 	tests := []struct {
-		name       string
-		rotation   []string
-		onVacation []onVacationReviewer
-		timeNow    time.Time
-		want       []string
+		name              string
+		rotation          []string
+		onVacation        []onVacationReviewer
+		timeNow           time.Time
+		excludedReviewers []string
+		want              []string
 	}{
 		{
 			name:     "reviewers on vacation start date are excluded",
@@ -121,10 +122,16 @@ func TestAvailable(t *testing.T) {
 			timeNow: time.Date(2024, 4, 3, 0, 0, 0, 0, newYork),
 			want:    []string{"id1"},
 		},
+		{
+			name:              "explicitly excluded reviewers",
+			rotation:          []string{"id1", "id2"},
+			excludedReviewers: []string{"id2"},
+			want:              []string{"id1"},
+		},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			got := available(test.timeNow, test.rotation, test.onVacation)
+			got := available(test.timeNow, test.rotation, test.onVacation, test.excludedReviewers)
 			if diff := cmp.Diff(test.want, got); diff != "" {
 				t.Errorf("available(%v, %v, %v) got diff: %s", test.timeNow, test.rotation, test.onVacation, diff)
 			}

.ci/magician/github/reviewer_assignment.go

@@ -49,7 +49,7 @@ func ChooseCoreReviewers(requestedReviewers, previousReviewers []User) (reviewer
 	}
 
 	if !hasPrimaryReviewer {
-		newPrimaryReviewer = GetRandomReviewer()
+		newPrimaryReviewer = GetRandomReviewer(nil)
 		reviewersToRequest = append(reviewersToRequest, newPrimaryReviewer)
 	}
 

.ci/magician/github/reviewer_assignment_test.go

@@ -25,11 +25,11 @@ import (
 )
 
 func TestChooseCoreReviewers(t *testing.T) {
-	if len(AvailableReviewers()) < 2 {
-		t.Fatalf("not enough available reviewers (%v) to test (need at least 2)", AvailableReviewers())
+	if len(AvailableReviewers(nil)) < 2 {
+		t.Fatalf("not enough available reviewers (%v) to test (need at least 2)", AvailableReviewers(nil))
 	}
-	firstCoreReviewer := AvailableReviewers()[0]
-	secondCoreReviewer := AvailableReviewers()[1]
+	firstCoreReviewer := AvailableReviewers(nil)[0]
+	secondCoreReviewer := AvailableReviewers(nil)[1]
 	cases := map[string]struct {
 		RequestedReviewers                               []User
 		PreviousReviewers                                []User
@@ -39,7 +39,7 @@ func TestChooseCoreReviewers(t *testing.T) {
 		"no previous review requests assigns new reviewer from team": {
 			RequestedReviewers:      []User{},
 			PreviousReviewers:       []User{},
-			ExpectReviewersFromList: AvailableReviewers(),
+			ExpectReviewersFromList: AvailableReviewers(nil),
 			ExpectPrimaryReviewer:   true,
 		},
 		"requested reviewer from team means that primary reviewer was already selected": {
@@ -61,7 +61,7 @@ func TestChooseCoreReviewers(t *testing.T) {
 		"previously involved reviewers that are not team members are ignored": {
 			RequestedReviewers:      []User{},
 			PreviousReviewers:       []User{User{Login: "foobar"}},
-			ExpectReviewersFromList: AvailableReviewers(),
+			ExpectReviewersFromList: AvailableReviewers(nil),
 			ExpectPrimaryReviewer:   true,
 		},
 		"only previously involved team member reviewers will have review requested": {