diff --git a/contribs/github-bot/internal/requirements/reviewer.go b/contribs/github-bot/internal/requirements/reviewer.go index 3e91c394fb7..36216b0b75c 100644 --- a/contribs/github-bot/internal/requirements/reviewer.go +++ b/contribs/github-bot/internal/requirements/reviewer.go @@ -11,6 +11,38 @@ import ( "github.com/xlab/treeprint" ) +// deduplicateReviews returns a list of reviews with at most 1 review per +// author, where approval/changes requested reviews are preferred over comments +// and later reviews are preferred over earlier ones. +func deduplicateReviews(reviews []*github.PullRequestReview) []*github.PullRequestReview { + added := make(map[string]int) + result := make([]*github.PullRequestReview, 0, len(reviews)) + for _, rev := range reviews { + idx, ok := added[rev.User.GetLogin()] + switch utils.ReviewState(rev.GetState()) { + case utils.ReviewStateApproved, utils.ReviewStateChangesRequested: + // this review changes the "approval state", and is more relevant, + // so substitute it with the previous one if it exists. + if ok { + result[idx] = rev + } else { + result = append(result, rev) + added[rev.User.GetLogin()] = len(result) - 1 + } + case utils.ReviewStateCommented: + // this review does not change the "approval state", so only append + // it if a previous review doesn't exist. + if !ok { + result = append(result, rev) + added[rev.User.GetLogin()] = len(result) - 1 + } + default: + panic(fmt.Sprintf("invalid review state %q", rev.GetState())) + } + } + return result +} + // ReviewByUserRequirement asserts that there is a review by the given user, // and if given that the review matches the desiredState. type ReviewByUserRequirement struct { @@ -28,6 +60,23 @@ func (r *ReviewByUserRequirement) IsSatisfied(pr *github.PullRequest, details tr detail += fmt.Sprintf(" (with state %q)", r.desiredState) } + // Check if user already approved this PR. + reviews, err := r.gh.ListPRReviews(pr.GetNumber()) + if err != nil { + r.gh.Logger.Errorf("unable to check if user %s already approved this PR: %v", r.user, err) + return utils.AddStatusNode(false, detail, details) + } + reviews = deduplicateReviews(reviews) + + for _, review := range reviews { + if review.GetUser().GetLogin() == r.user { + r.gh.Logger.Debugf("User %s already reviewed PR %d with state %s", r.user, pr.GetNumber(), review.GetState()) + result := r.desiredState == "" || review.GetState() == r.desiredState + return utils.AddStatusNode(result, detail, details) + } + } + r.gh.Logger.Debugf("User %s has not reviewed PR %d yet", r.user, pr.GetNumber()) + // If not a dry run, make the user a reviewer if he's not already. if !r.gh.DryRun { requested := false @@ -62,22 +111,6 @@ func (r *ReviewByUserRequirement) IsSatisfied(pr *github.PullRequest, details tr } } - // Check if user already approved this PR. - reviews, err := r.gh.ListPRReviews(pr.GetNumber()) - if err != nil { - r.gh.Logger.Errorf("unable to check if user %s already approved this PR: %v", r.user, err) - return utils.AddStatusNode(false, detail, details) - } - - for _, review := range reviews { - if review.GetUser().GetLogin() == r.user { - r.gh.Logger.Debugf("User %s already reviewed PR %d with state %s", r.user, pr.GetNumber(), review.GetState()) - result := r.desiredState == "" || review.GetState() == r.desiredState - return utils.AddStatusNode(result, detail, details) - } - } - r.gh.Logger.Debugf("User %s has not reviewed PR %d yet", r.user, pr.GetNumber()) - return utils.AddStatusNode(false, detail, details) } @@ -123,6 +156,14 @@ func (r *ReviewByTeamMembersRequirement) IsSatisfied(pr *github.PullRequest, det return utils.AddStatusNode(false, detail, details) } + reviews, err := r.gh.ListPRReviews(pr.GetNumber()) + if err != nil { + r.gh.Logger.Errorf("unable to fetch existing reviews of pr %d: %v", pr.GetNumber(), err) + return utils.AddStatusNode(false, detail, details) + } + + reviews = deduplicateReviews(reviews) + // If not a dry run, request a team review if no member has reviewed yet, // and the team review has not been requested. if !r.gh.DryRun { @@ -144,12 +185,18 @@ func (r *ReviewByTeamMembersRequirement) IsSatisfied(pr *github.PullRequest, det if !teamRequested { for _, user := range reviewers.Users { - if slices.ContainsFunc(teamMembers, func(memb *github.User) bool { - return memb.GetID() == user.GetID() - }) { + if containsUserWithLogin(teamMembers, user.GetLogin()) { usersRequested = append(usersRequested, user.GetLogin()) } } + + for _, rev := range reviews { + // if not already requested and user is a team member... + if !slices.Contains(usersRequested, rev.User.GetLogin()) && + containsUserWithLogin(teamMembers, rev.User.GetLogin()) { + usersRequested = append(usersRequested, rev.User.GetLogin()) + } + } } switch { @@ -176,33 +223,29 @@ func (r *ReviewByTeamMembersRequirement) IsSatisfied(pr *github.PullRequest, det // Check how many members of this team already reviewed this PR. reviewCount := uint(0) - reviews, err := r.gh.ListPRReviews(pr.GetNumber()) - if err != nil { - r.gh.Logger.Errorf("unable to check if a member of team %s already reviewed this PR: %v", r.team, err) - return utils.AddStatusNode(false, detail, details) - } - stateStr := "" - if r.desiredState != "" { - stateStr = fmt.Sprintf("%q ", r.desiredState) - } for _, review := range reviews { - for _, member := range teamMembers { - if review.GetUser().GetLogin() == member.GetLogin() { - if desired := r.desiredState; desired == "" || desired == review.GetState() { - reviewCount += 1 - } - r.gh.Logger.Debugf( - "Member %s from team %s already reviewed PR %d with state %s (%d/%d required %sreview(s))", - member.GetLogin(), r.team, pr.GetNumber(), review.GetState(), reviewCount, r.count, stateStr, - ) + login := review.GetUser().GetLogin() + if containsUserWithLogin(teamMembers, login) { + if desired := r.desiredState; desired == "" || desired == review.GetState() { + reviewCount += 1 } + r.gh.Logger.Debugf( + "Member %s from team %s already reviewed PR %d with state %s (%d/%d required review(s) with state %q)", + login, r.team, pr.GetNumber(), review.GetState(), reviewCount, r.count, r.desiredState, + ) } } return utils.AddStatusNode(reviewCount >= r.count, detail, details) } +func containsUserWithLogin(users []*github.User, login string) bool { + return slices.ContainsFunc(users, func(u *github.User) bool { + return u.GetLogin() == login + }) +} + // WithCount specifies the number of required reviews. // By default, this is 1. func (r *ReviewByTeamMembersRequirement) WithCount(n uint) *ReviewByTeamMembersRequirement { @@ -262,20 +305,17 @@ func (r *ReviewByOrgMembersRequirement) IsSatisfied(pr *github.PullRequest, deta r.gh.Logger.Errorf("unable to check number of reviews on this PR: %v", err) return utils.AddStatusNode(false, detail, details) } + reviews = deduplicateReviews(reviews) - stateStr := "" - if r.desiredState != "" { - stateStr = fmt.Sprintf("%q ", r.desiredState) - } for _, review := range reviews { if review.GetAuthorAssociation() == "MEMBER" { if r.desiredState == "" || review.GetState() == r.desiredState { reviewed++ } r.gh.Logger.Debugf( - "Member %s already reviewed PR %d with state %s (%d/%d required %sreviews)", + "Member %s already reviewed PR %d with state %s (%d/%d required reviews with state %q)", review.GetUser().GetLogin(), pr.GetNumber(), review.GetState(), - reviewed, r.count, stateStr, + reviewed, r.count, r.desiredState, ) } } diff --git a/contribs/github-bot/internal/requirements/reviewer_test.go b/contribs/github-bot/internal/requirements/reviewer_test.go index 235dca14034..b952294a338 100644 --- a/contribs/github-bot/internal/requirements/reviewer_test.go +++ b/contribs/github-bot/internal/requirements/reviewer_test.go @@ -16,6 +16,68 @@ import ( "github.com/xlab/treeprint" ) +func Test_deduplicateReviews(t *testing.T) { + tests := []struct { + name string + reviews []*github.PullRequestReview + expected []*github.PullRequestReview + }{ + { + name: "three different authors", + reviews: []*github.PullRequestReview{ + {User: &github.User{Login: github.String("user1")}, State: github.String("APPROVED")}, + {User: &github.User{Login: github.String("user2")}, State: github.String("CHANGES_REQUESTED")}, + {User: &github.User{Login: github.String("user3")}, State: github.String("COMMENTED")}, + }, + expected: []*github.PullRequestReview{ + {User: &github.User{Login: github.String("user1")}, State: github.String("APPROVED")}, + {User: &github.User{Login: github.String("user2")}, State: github.String("CHANGES_REQUESTED")}, + {User: &github.User{Login: github.String("user3")}, State: github.String("COMMENTED")}, + }, + }, + { + name: "single author - approval then comment", + reviews: []*github.PullRequestReview{ + {User: &github.User{Login: github.String("user1")}, State: github.String("APPROVED")}, + {User: &github.User{Login: github.String("user1")}, State: github.String("COMMENTED")}, + }, + expected: []*github.PullRequestReview{ + {User: &github.User{Login: github.String("user1")}, State: github.String("APPROVED")}, + }, + }, + { + name: "single author - approval then changes requested", + reviews: []*github.PullRequestReview{ + {User: &github.User{Login: github.String("user1")}, State: github.String("APPROVED")}, + {User: &github.User{Login: github.String("user1")}, State: github.String("CHANGES_REQUESTED")}, + }, + expected: []*github.PullRequestReview{ + {User: &github.User{Login: github.String("user1")}, State: github.String("CHANGES_REQUESTED")}, + }, + }, + { + name: "two authors - mixed reviews", + reviews: []*github.PullRequestReview{ + {User: &github.User{Login: github.String("userA")}, State: github.String("APPROVED")}, + {User: &github.User{Login: github.String("userB")}, State: github.String("CHANGES_REQUESTED")}, + {User: &github.User{Login: github.String("userA")}, State: github.String("CHANGES_REQUESTED")}, + {User: &github.User{Login: github.String("userB")}, State: github.String("COMMENTED")}, + }, + expected: []*github.PullRequestReview{ + {User: &github.User{Login: github.String("userA")}, State: github.String("CHANGES_REQUESTED")}, + {User: &github.User{Login: github.String("userB")}, State: github.String("CHANGES_REQUESTED")}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := deduplicateReviews(tt.reviews) + assert.Equal(t, tt.expected, result) + }) + } +} + func TestReviewByUser(t *testing.T) { t.Parallel() @@ -34,6 +96,10 @@ func TestReviewByUser(t *testing.T) { }, { User: &github.User{Login: github.String("user")}, State: github.String("APPROVED"), + }, { + // Should be ignored in favour of the following one + User: &github.User{Login: github.String("anotherOne")}, + State: github.String("APPROVED"), }, { User: &github.User{Login: github.String("anotherOne")}, State: github.String("CHANGES_REQUESTED"), @@ -138,6 +204,10 @@ func TestReviewByTeamMembers(t *testing.T) { reviews := []*github.PullRequestReview{ { + // only later review should be counted. + User: &github.User{Login: github.String("user1")}, + State: github.String("CHANGES_REQUESTED"), + }, { User: &github.User{Login: github.String("user1")}, State: github.String("APPROVED"), }, { @@ -149,6 +219,10 @@ func TestReviewByTeamMembers(t *testing.T) { }, { User: &github.User{Login: github.String("user4")}, State: github.String("CHANGES_REQUESTED"), + }, { + // only later review should be counted. + User: &github.User{Login: github.String("user5")}, + State: github.String("APPROVED"), }, { User: &github.User{Login: github.String("user5")}, State: github.String("CHANGES_REQUESTED"), @@ -163,22 +237,113 @@ func TestReviewByTeamMembers(t *testing.T) { for _, testCase := range []struct { name string - team string - count uint - state utils.ReviewState + req *ReviewByTeamMembersRequirement + reviews []*github.PullRequestReview reviewers github.Reviewers expectedResult byte }{ - {"3/3 team members approved", "team1", 3, utils.ReviewStateApproved, reviewers, satisfied}, - {"3/3 team members approved (with user reviewers)", "team1", 3, utils.ReviewStateApproved, userReviewers, satisfied}, - {"1/1 team member approved", "team2", 1, utils.ReviewStateApproved, reviewers, satisfied}, - {"1/2 team member approved", "team2", 2, utils.ReviewStateApproved, reviewers, notSatisfied}, - {"0/1 team member approved", "team3", 1, utils.ReviewStateApproved, reviewers, notSatisfied}, - {"0/1 team member approved with request", "team3", 1, utils.ReviewStateApproved, noReviewers, notSatisfied | withRequest}, - {"team doesn't exist with request", "team4", 1, utils.ReviewStateApproved, noReviewers, notSatisfied | withRequest}, - {"3/3 team members reviewed", "team2", 3, "", reviewers, satisfied}, - {"2/2 team members rejected", "team2", 2, utils.ReviewStateChangesRequested, reviewers, satisfied}, - {"1/3 team members approved", "team2", 3, utils.ReviewStateApproved, reviewers, notSatisfied}, + { + name: "3/3 team members approved", + req: ReviewByTeamMembers(nil, "team1"). + WithCount(3). + WithDesiredState(utils.ReviewStateApproved), + reviews: reviews, + reviewers: reviewers, + expectedResult: satisfied, + }, + { + name: "3/3 team members approved (with user reviewers)", + req: ReviewByTeamMembers(nil, "team1"). + WithCount(3). + WithDesiredState(utils.ReviewStateApproved), + reviews: reviews, + reviewers: userReviewers, + expectedResult: satisfied, + }, + { + name: "1/1 team member approved", + req: ReviewByTeamMembers(nil, "team2"). + WithDesiredState(utils.ReviewStateApproved), + reviews: reviews, + reviewers: reviewers, + expectedResult: satisfied, + }, + { + name: "1/2 team member approved", + req: ReviewByTeamMembers(nil, "team2"). + WithCount(2). + WithDesiredState(utils.ReviewStateApproved), + reviews: reviews, + reviewers: reviewers, + expectedResult: notSatisfied, + }, + { + name: "0/1 team member approved", + req: ReviewByTeamMembers(nil, "team3"). + WithDesiredState(utils.ReviewStateApproved), + reviews: reviews, + reviewers: reviewers, + expectedResult: notSatisfied, + }, + { + name: "0/1 team member reviewed with request", + req: ReviewByTeamMembers(nil, "team3"), + // Show there are no current reviews, so we actually perform the request. + reviewers: noReviewers, + expectedResult: notSatisfied | withRequest, + }, + { + name: "3/3 team member approved from review list", + req: ReviewByTeamMembers(nil, "team1"). + WithDesiredState(utils.ReviewStateApproved). + WithCount(3), + reviews: reviews, + reviewers: noReviewers, + expectedResult: satisfied, + }, + { + name: "1/2 team member approved from review list", + req: ReviewByTeamMembers(nil, "team3"). + WithDesiredState(utils.ReviewStateApproved). + WithCount(2), + reviews: reviews, + reviewers: noReviewers, + expectedResult: notSatisfied, + }, + { + name: "team doesn't exist with request", + req: ReviewByTeamMembers(nil, "team4"). + WithDesiredState(utils.ReviewStateApproved), + reviews: reviews, + reviewers: noReviewers, + expectedResult: notSatisfied | withRequest, + }, + { + name: "3/3 team members reviewed", + req: ReviewByTeamMembers(nil, "team2"). + WithCount(3), + reviews: reviews, + reviewers: reviewers, + expectedResult: satisfied, + }, + { + name: "2/2 team members rejected", + req: ReviewByTeamMembers(nil, "team2"). + WithCount(2). + WithDesiredState(utils.ReviewStateChangesRequested), + reviews: reviews, + reviewers: reviewers, + expectedResult: satisfied, + }, + { + name: "1/3 team members approved", + req: ReviewByTeamMembers(nil, "team2"). + WithCount(3). + WithDesiredState(utils.ReviewStateApproved), + reviews: reviews, + reviewers: reviewers, + expectedResult: notSatisfied, + }, } { t.Run(testCase.name, func(t *testing.T) { t.Parallel() @@ -202,17 +367,17 @@ func TestReviewByTeamMembers(t *testing.T) { ), mock.WithRequestMatchPages( mock.EndpointPattern{ - Pattern: fmt.Sprintf("/orgs/teams/%s/members", testCase.team), + Pattern: fmt.Sprintf("/orgs/teams/%s/members", testCase.req.team), Method: "GET", }, - members[testCase.team], + members[testCase.req.team], ), mock.WithRequestMatchPages( mock.EndpointPattern{ Pattern: "/repos/pulls/0/reviews", Method: "GET", }, - reviews, + testCase.reviews, ), ) @@ -224,13 +389,13 @@ func TestReviewByTeamMembers(t *testing.T) { pr := &github.PullRequest{} details := treeprint.New() - requirement := ReviewByTeamMembers(gh, testCase.team). - WithCount(testCase.count). - WithDesiredState(testCase.state) + req := new(ReviewByTeamMembersRequirement) + *req = *testCase.req + req.gh = gh - expSatisfied := testCase.expectedResult&satisfied != 0 + expSatisfied := testCase.expectedResult&satisfied > 0 expRequested := testCase.expectedResult&withRequest > 0 - assert.Equal(t, expSatisfied, requirement.IsSatisfied(pr, details), + assert.Equal(t, expSatisfied, req.IsSatisfied(pr, details), "requirement should have a satisfied status: %t", expSatisfied) assert.True(t, utils.TestLastNodeStatus(t, expSatisfied, details), "requirement details should have a status: %t", expSatisfied) @@ -248,6 +413,11 @@ func TestReviewByOrgMembers(t *testing.T) { User: &github.User{Login: github.String("user1")}, State: github.String("APPROVED"), AuthorAssociation: github.String("MEMBER"), + }, { + // should be ignored in favour of the following one. + User: &github.User{Login: github.String("user2")}, + State: github.String("CHANGES_REQUESTED"), + AuthorAssociation: github.String("COLLABORATOR"), }, { User: &github.User{Login: github.String("user2")}, State: github.String("APPROVED"),