Implement cascading quota for reviewer suggestions
Each source (suggestions, collaborators, teams) has base quota of 5. Unfilled slots cascade to later sources, allowing up to 15 total. Adds unit tests with HTTP mocks.
This commit is contained in:
Kynan Ware
parent
d643d5386e
commit
2d191e5ba0
2 changed files with 247 additions and 26 deletions
|
|
@ -958,13 +958,16 @@ func (r ReviewerTeam) Slug() string {
|
|||
func (r ReviewerTeam) sealedReviewerCandidate() {}
|
||||
|
||||
// SuggestedReviewerActors fetches suggested reviewers for a pull request.
|
||||
// It combines results from:
|
||||
// - suggestedReviewerActors (10 max) - suggested based on activity
|
||||
// - repository collaborators (10 max) - all collaborators
|
||||
// - organization teams (10 max for org repos) - all teams (if owner is an org)
|
||||
// Results are returned in that order with duplicates removed.
|
||||
// It combines results from three sources using a cascading quota system:
|
||||
// - suggestedReviewerActors - suggested based on PR activity (base quota: 5)
|
||||
// - repository collaborators - all collaborators (base quota: 5 + unfilled from suggestions)
|
||||
// - organization teams - all teams for org repos (base quota: 5 + unfilled from collaborators)
|
||||
//
|
||||
// This ensures we show up to 15 total candidates, with each source filling any
|
||||
// unfilled quota from the previous source. Results are deduplicated.
|
||||
// Returns the candidates, a MoreResults count, and an error.
|
||||
func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, query string) ([]ReviewerCandidate, int, error) {
|
||||
// Fetch 10 from each source to allow cascading quota to fill from available results.
|
||||
// Use a single query that includes organization.teams - if the owner is not an org,
|
||||
// we'll get a "Could not resolve to an Organization" error which we handle gracefully.
|
||||
type responseData struct {
|
||||
|
|
@ -985,7 +988,7 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string,
|
|||
} `graphql:"... on Bot"`
|
||||
}
|
||||
}
|
||||
} `graphql:"suggestedReviewerActors(first: 5, query: $query)"`
|
||||
} `graphql:"suggestedReviewerActors(first: 10, query: $query)"`
|
||||
} `graphql:"... on PullRequest"`
|
||||
} `graphql:"node(id: $id)"`
|
||||
Repository struct {
|
||||
|
|
@ -995,7 +998,7 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string,
|
|||
Login string
|
||||
Name string
|
||||
}
|
||||
} `graphql:"collaborators(first: 5, query: $query)"`
|
||||
} `graphql:"collaborators(first: 10, query: $query)"`
|
||||
} `graphql:"repository(owner: $owner, name: $name)"`
|
||||
Organization struct {
|
||||
Teams struct {
|
||||
|
|
@ -1003,7 +1006,7 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string,
|
|||
Nodes []struct {
|
||||
Slug string
|
||||
}
|
||||
} `graphql:"teams(first: 5, query: $query)"`
|
||||
} `graphql:"teams(first: 10, query: $query)"`
|
||||
} `graphql:"organization(login: $owner)"`
|
||||
}
|
||||
|
||||
|
|
@ -1022,54 +1025,73 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string,
|
|||
return nil, 0, err
|
||||
}
|
||||
|
||||
ownerName := repo.RepoOwner()
|
||||
// Build candidates using cascading quota logic:
|
||||
// Each source has a base quota of 5, plus any unfilled quota from previous sources.
|
||||
// This ensures we show up to 15 total candidates, filling gaps when earlier sources have fewer.
|
||||
seen := make(map[string]bool)
|
||||
var candidates []ReviewerCandidate
|
||||
const baseQuota = 5
|
||||
|
||||
// Add suggested reviewers first (excluding author)
|
||||
// Suggested reviewers (excluding author)
|
||||
suggestionsAdded := 0
|
||||
for _, n := range result.Node.PullRequest.SuggestedActors.Nodes {
|
||||
if suggestionsAdded >= baseQuota {
|
||||
break
|
||||
}
|
||||
if n.IsAuthor {
|
||||
continue
|
||||
}
|
||||
var candidate ReviewerCandidate
|
||||
var login string
|
||||
if n.Reviewer.TypeName == "User" && n.Reviewer.User.Login != "" {
|
||||
candidate = NewReviewerUser(n.Reviewer.User.Login, n.Reviewer.User.Name)
|
||||
login = n.Reviewer.User.Login
|
||||
candidate = NewReviewerUser(login, n.Reviewer.User.Name)
|
||||
} else if n.Reviewer.TypeName == "Bot" && n.Reviewer.Bot.Login != "" {
|
||||
candidate = NewReviewerBot(n.Reviewer.Bot.Login)
|
||||
login = n.Reviewer.Bot.Login
|
||||
candidate = NewReviewerBot(login)
|
||||
} else {
|
||||
continue
|
||||
}
|
||||
|
||||
login := candidate.Login()
|
||||
if !seen[login] {
|
||||
seen[login] = true
|
||||
candidates = append(candidates, candidate)
|
||||
suggestionsAdded++
|
||||
}
|
||||
}
|
||||
|
||||
// Add collaborators (deduped against suggested)
|
||||
// Collaborators: quota = base + unfilled from suggestions
|
||||
collaboratorsQuota := baseQuota + (baseQuota - suggestionsAdded)
|
||||
collaboratorsAdded := 0
|
||||
for _, c := range result.Repository.Collaborators.Nodes {
|
||||
if collaboratorsAdded >= collaboratorsQuota {
|
||||
break
|
||||
}
|
||||
if c.Login == "" {
|
||||
continue
|
||||
}
|
||||
candidate := NewReviewerUser(c.Login, c.Name)
|
||||
login := candidate.Login()
|
||||
if !seen[login] {
|
||||
seen[login] = true
|
||||
candidates = append(candidates, candidate)
|
||||
if !seen[c.Login] {
|
||||
seen[c.Login] = true
|
||||
candidates = append(candidates, NewReviewerUser(c.Login, c.Name))
|
||||
collaboratorsAdded++
|
||||
}
|
||||
}
|
||||
|
||||
// Add teams (will be empty if owner is not an org)
|
||||
// Teams: quota = base + unfilled from collaborators
|
||||
teamsQuota := baseQuota + (collaboratorsQuota - collaboratorsAdded)
|
||||
teamsAdded := 0
|
||||
ownerName := repo.RepoOwner()
|
||||
for _, t := range result.Organization.Teams.Nodes {
|
||||
if teamsAdded >= teamsQuota {
|
||||
break
|
||||
}
|
||||
if t.Slug == "" {
|
||||
continue
|
||||
}
|
||||
candidate := NewReviewerTeam(ownerName, t.Slug)
|
||||
login := candidate.Login()
|
||||
if !seen[login] {
|
||||
seen[login] = true
|
||||
candidates = append(candidates, candidate)
|
||||
teamLogin := fmt.Sprintf("%s/%s", ownerName, t.Slug)
|
||||
if !seen[teamLogin] {
|
||||
seen[teamLogin] = true
|
||||
candidates = append(candidates, NewReviewerTeam(ownerName, t.Slug))
|
||||
teamsAdded++
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -2,6 +2,8 @@ package api
|
|||
|
||||
import (
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/cli/cli/v2/internal/ghrepo"
|
||||
|
|
@ -136,3 +138,200 @@ func Test_Logins(t *testing.T) {
|
|||
})
|
||||
}
|
||||
}
|
||||
|
||||
// mockReviewerResponse generates a GraphQL response for SuggestedReviewerActors tests.
|
||||
// It creates suggestions (s1, s2...), collaborators (c1, c2...), and teams (team1, team2...).
|
||||
// totalCollabs and totalTeams set the TotalCount fields (for "more results" calculation).
|
||||
func mockReviewerResponse(suggestions, collabs, teams, totalCollabs, totalTeams int) string {
|
||||
var suggestionNodes, collabNodes, teamNodes []string
|
||||
|
||||
for i := 1; i <= suggestions; i++ {
|
||||
suggestionNodes = append(suggestionNodes,
|
||||
fmt.Sprintf(`{"isAuthor": false, "reviewer": {"__typename": "User", "login": "s%d", "name": "S%d"}}`, i, i))
|
||||
}
|
||||
for i := 1; i <= collabs; i++ {
|
||||
collabNodes = append(collabNodes,
|
||||
fmt.Sprintf(`{"login": "c%d", "name": "C%d"}`, i, i))
|
||||
}
|
||||
for i := 1; i <= teams; i++ {
|
||||
teamNodes = append(teamNodes,
|
||||
fmt.Sprintf(`{"slug": "team%d"}`, i))
|
||||
}
|
||||
|
||||
return fmt.Sprintf(`{
|
||||
"data": {
|
||||
"node": {"suggestedReviewerActors": {"nodes": [%s]}},
|
||||
"repository": {"collaborators": {"totalCount": %d, "nodes": [%s]}},
|
||||
"organization": {"teams": {"totalCount": %d, "nodes": [%s]}}
|
||||
}
|
||||
}`, strings.Join(suggestionNodes, ","), totalCollabs, strings.Join(collabNodes, ","),
|
||||
totalTeams, strings.Join(teamNodes, ","))
|
||||
}
|
||||
|
||||
func TestSuggestedReviewerActors(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
httpStubs func(*httpmock.Registry)
|
||||
expectedCount int
|
||||
expectedLogins []string
|
||||
expectedMore int
|
||||
expectError bool
|
||||
}{
|
||||
{
|
||||
name: "all sources plentiful - 5 each from cascading quota",
|
||||
httpStubs: func(reg *httpmock.Registry) {
|
||||
reg.Register(
|
||||
httpmock.GraphQL(`query SuggestedReviewerActors\b`),
|
||||
httpmock.StringResponse(mockReviewerResponse(6, 6, 6, 20, 10)))
|
||||
},
|
||||
expectedCount: 15,
|
||||
expectedLogins: []string{"s1", "s2", "s3", "s4", "s5", "c1", "c2", "c3", "c4", "c5", "OWNER/team1", "OWNER/team2", "OWNER/team3", "OWNER/team4", "OWNER/team5"},
|
||||
expectedMore: 30,
|
||||
},
|
||||
{
|
||||
name: "few suggestions - collaborators fill gap",
|
||||
httpStubs: func(reg *httpmock.Registry) {
|
||||
reg.Register(
|
||||
httpmock.GraphQL(`query SuggestedReviewerActors\b`),
|
||||
httpmock.StringResponse(mockReviewerResponse(2, 10, 6, 50, 10)))
|
||||
},
|
||||
expectedCount: 15,
|
||||
expectedLogins: []string{"s1", "s2", "c1", "c2", "c3", "c4", "c5", "c6", "c7", "c8", "OWNER/team1", "OWNER/team2", "OWNER/team3", "OWNER/team4", "OWNER/team5"},
|
||||
expectedMore: 60,
|
||||
},
|
||||
{
|
||||
name: "few suggestions and collaborators - teams fill gap",
|
||||
httpStubs: func(reg *httpmock.Registry) {
|
||||
reg.Register(
|
||||
httpmock.GraphQL(`query SuggestedReviewerActors\b`),
|
||||
httpmock.StringResponse(mockReviewerResponse(2, 3, 10, 3, 10)))
|
||||
},
|
||||
expectedCount: 15,
|
||||
expectedLogins: []string{"s1", "s2", "c1", "c2", "c3", "OWNER/team1", "OWNER/team2", "OWNER/team3", "OWNER/team4", "OWNER/team5", "OWNER/team6", "OWNER/team7", "OWNER/team8", "OWNER/team9", "OWNER/team10"},
|
||||
expectedMore: 13,
|
||||
},
|
||||
{
|
||||
name: "no suggestions or collaborators - teams only",
|
||||
httpStubs: func(reg *httpmock.Registry) {
|
||||
reg.Register(
|
||||
httpmock.GraphQL(`query SuggestedReviewerActors\b`),
|
||||
httpmock.StringResponse(mockReviewerResponse(0, 0, 10, 0, 10)))
|
||||
},
|
||||
expectedCount: 10, // max 15, but only 10 teams available
|
||||
expectedLogins: []string{"OWNER/team1", "OWNER/team2", "OWNER/team3", "OWNER/team4", "OWNER/team5", "OWNER/team6", "OWNER/team7", "OWNER/team8", "OWNER/team9", "OWNER/team10"},
|
||||
expectedMore: 10,
|
||||
},
|
||||
{
|
||||
name: "author excluded from suggestions",
|
||||
httpStubs: func(reg *httpmock.Registry) {
|
||||
// Custom response with author flag
|
||||
reg.Register(
|
||||
httpmock.GraphQL(`query SuggestedReviewerActors\b`),
|
||||
httpmock.StringResponse(`{
|
||||
"data": {
|
||||
"node": {"suggestedReviewerActors": {"nodes": [
|
||||
{"isAuthor": true, "reviewer": {"__typename": "User", "login": "author", "name": "Author"}},
|
||||
{"isAuthor": false, "reviewer": {"__typename": "User", "login": "s1", "name": "S1"}},
|
||||
{"isAuthor": false, "reviewer": {"__typename": "User", "login": "s2", "name": "S2"}}
|
||||
]}},
|
||||
"repository": {"collaborators": {"totalCount": 5, "nodes": [{"login": "c1", "name": "C1"}]}},
|
||||
"organization": {"teams": {"totalCount": 3, "nodes": [{"slug": "team1"}]}}
|
||||
}
|
||||
}`))
|
||||
},
|
||||
expectedCount: 4,
|
||||
expectedLogins: []string{"s1", "s2", "c1", "OWNER/team1"},
|
||||
expectedMore: 8,
|
||||
},
|
||||
{
|
||||
name: "deduplication across sources",
|
||||
httpStubs: func(reg *httpmock.Registry) {
|
||||
// Custom response with duplicate user
|
||||
reg.Register(
|
||||
httpmock.GraphQL(`query SuggestedReviewerActors\b`),
|
||||
httpmock.StringResponse(`{
|
||||
"data": {
|
||||
"node": {"suggestedReviewerActors": {"nodes": [
|
||||
{"isAuthor": false, "reviewer": {"__typename": "User", "login": "shareduser", "name": "Shared"}}
|
||||
]}},
|
||||
"repository": {"collaborators": {"totalCount": 10, "nodes": [
|
||||
{"login": "shareduser", "name": "Shared"},
|
||||
{"login": "c1", "name": "C1"}
|
||||
]}},
|
||||
"organization": {"teams": {"totalCount": 5, "nodes": [{"slug": "team1"}]}}
|
||||
}
|
||||
}`))
|
||||
},
|
||||
expectedCount: 3,
|
||||
expectedLogins: []string{"shareduser", "c1", "OWNER/team1"},
|
||||
expectedMore: 15,
|
||||
},
|
||||
{
|
||||
name: "personal repo - no organization teams",
|
||||
httpStubs: func(reg *httpmock.Registry) {
|
||||
reg.Register(
|
||||
httpmock.GraphQL(`query SuggestedReviewerActors\b`),
|
||||
httpmock.StringResponse(`{
|
||||
"data": {
|
||||
"node": {"suggestedReviewerActors": {"nodes": [
|
||||
{"isAuthor": false, "reviewer": {"__typename": "User", "login": "s1", "name": "S1"}}
|
||||
]}},
|
||||
"repository": {"collaborators": {"totalCount": 3, "nodes": [{"login": "c1", "name": "C1"}]}},
|
||||
"organization": null
|
||||
},
|
||||
"errors": [{"message": "Could not resolve to an Organization with the login of 'OWNER'."}]
|
||||
}`))
|
||||
},
|
||||
expectedCount: 2,
|
||||
expectedLogins: []string{"s1", "c1"},
|
||||
expectedMore: 3,
|
||||
},
|
||||
{
|
||||
name: "bot reviewer included",
|
||||
httpStubs: func(reg *httpmock.Registry) {
|
||||
reg.Register(
|
||||
httpmock.GraphQL(`query SuggestedReviewerActors\b`),
|
||||
httpmock.StringResponse(`{
|
||||
"data": {
|
||||
"node": {"suggestedReviewerActors": {"nodes": [
|
||||
{"isAuthor": false, "reviewer": {"__typename": "Bot", "login": "copilot-pull-request-reviewer"}},
|
||||
{"isAuthor": false, "reviewer": {"__typename": "User", "login": "s1", "name": "S1"}}
|
||||
]}},
|
||||
"repository": {"collaborators": {"totalCount": 5, "nodes": []}},
|
||||
"organization": {"teams": {"totalCount": 0, "nodes": []}}
|
||||
}
|
||||
}`))
|
||||
},
|
||||
expectedCount: 2,
|
||||
expectedLogins: []string{"copilot-pull-request-reviewer", "s1"},
|
||||
expectedMore: 5,
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
reg := &httpmock.Registry{}
|
||||
if tt.httpStubs != nil {
|
||||
tt.httpStubs(reg)
|
||||
}
|
||||
|
||||
client := newTestClient(reg)
|
||||
repo, _ := ghrepo.FromFullName("OWNER/REPO")
|
||||
|
||||
candidates, moreResults, err := SuggestedReviewerActors(client, repo, "PR_123", "")
|
||||
if tt.expectError {
|
||||
assert.Error(t, err)
|
||||
return
|
||||
}
|
||||
assert.NoError(t, err)
|
||||
assert.Equal(t, tt.expectedCount, len(candidates), "candidate count mismatch")
|
||||
assert.Equal(t, tt.expectedMore, moreResults, "moreResults mismatch")
|
||||
|
||||
logins := make([]string, len(candidates))
|
||||
for i, c := range candidates {
|
||||
logins[i] = c.Login()
|
||||
}
|
||||
assert.Equal(t, tt.expectedLogins, logins)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue