Skip to content

Commit a2e23a2

Browse files
andyfellerandyfeller
authored
Merge pull request cli#11407 from cli/andyfeller/11403-pr-team-reviewers
Include org teams for PR reviewers
2 parents 7d70980 + bbc3d02 commit a2e23a2

4 files changed

Lines changed: 251 additions & 3 deletions

File tree

‎pkg/cmd/pr/create/create_test.go‎

Lines changed: 126 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1655,7 +1655,7 @@ func Test_createRun(t *testing.T) {
16551655
expectedOut: "https://github.com/OWNER/REPO/pull/12\n",
16561656
},
16571657
{
1658-
name: "if reviewer contains any team, fetch teams",
1658+
name: "fetch org teams non-interactively if reviewer contains any team",
16591659
setup: func(opts *CreateOptions, t *testing.T) func() {
16601660
opts.TitleProvided = true
16611661
opts.BodyProvided = true
@@ -1718,7 +1718,7 @@ func Test_createRun(t *testing.T) {
17181718
expectedErrOut: "",
17191719
},
17201720
{
1721-
name: "if reviewer does NOT contain any team, do NOT fetch teams",
1721+
name: "do not fetch org teams non-interactively if reviewer does not contain any team",
17221722
setup: func(opts *CreateOptions, t *testing.T) func() {
17231723
opts.TitleProvided = true
17241724
opts.BodyProvided = true
@@ -1773,6 +1773,130 @@ func Test_createRun(t *testing.T) {
17731773
expectedOut: "https://github.com/OWNER/REPO/pull/12\n",
17741774
expectedErrOut: "",
17751775
},
1776+
{
1777+
name: "fetch org teams interactively if reviewer metadata selected",
1778+
tty: true,
1779+
setup: func(opts *CreateOptions, t *testing.T) func() {
1780+
// In order to test additional metadata, title and body cannot be provided here.
1781+
opts.HeadBranch = "feature"
1782+
return func() {}
1783+
},
1784+
cmdStubs: func(cs *run.CommandStubber) {
1785+
// Stub git commits for `initDefaultTitleBody` when initializing PR state.
1786+
cs.Register(
1787+
"git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature",
1788+
0,
1789+
"3a9b48085046d156c5acce8f3b3a0532cd706a4a\u0000first commit of pr\u0000first commit description\u0000\n",
1790+
)
1791+
cs.Register(`git rev-parse --show-toplevel`, 0, "")
1792+
},
1793+
promptStubs: func(pm *prompter.PrompterMock) {
1794+
firstConfirmSubmission := true
1795+
pm.InputFunc = func(message, defaultValue string) (string, error) {
1796+
switch message {
1797+
case "Title (required)":
1798+
return "TITLE", nil
1799+
default:
1800+
return "", fmt.Errorf("unexpected input prompt: %s", message)
1801+
}
1802+
}
1803+
pm.MarkdownEditorFunc = func(message, defaultValue string, allowEmpty bool) (string, error) {
1804+
switch message {
1805+
case "Body":
1806+
return "BODY", nil
1807+
default:
1808+
return "", fmt.Errorf("unexpected markdown editor prompt: %s", message)
1809+
}
1810+
}
1811+
pm.MultiSelectFunc = func(message string, defaults []string, options []string) ([]int, error) {
1812+
switch message {
1813+
case "What would you like to add?":
1814+
return prompter.IndexesFor(options, "Reviewers")
1815+
case "Reviewers":
1816+
return prompter.IndexesFor(options, "MonaLisa (Mona Display Name)", "OWNER/core")
1817+
default:
1818+
return nil, fmt.Errorf("unexpected multi-select prompt: %s", message)
1819+
}
1820+
}
1821+
pm.SelectFunc = func(message, defaultValue string, options []string) (int, error) {
1822+
switch message {
1823+
case "Where should we push the 'feature' branch?":
1824+
return 0, nil
1825+
case "What's next?":
1826+
if firstConfirmSubmission {
1827+
firstConfirmSubmission = false
1828+
return prompter.IndexFor(options, "Add metadata")
1829+
}
1830+
return prompter.IndexFor(options, "Submit")
1831+
default:
1832+
return 0, fmt.Errorf("unexpected select prompt: %s", message)
1833+
}
1834+
}
1835+
},
1836+
httpStubs: func(reg *httpmock.Registry, t *testing.T) {
1837+
reg.Register(
1838+
httpmock.GraphQL(`query UserCurrent\b`),
1839+
httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`))
1840+
reg.Register(
1841+
httpmock.GraphQL(`query PullRequestTemplates\b`),
1842+
httpmock.StringResponse(`{ "data": { "repository": { "pullRequestTemplates": [] } } }`),
1843+
)
1844+
reg.Register(
1845+
httpmock.GraphQL(`query RepositoryAssignableUsers\b`),
1846+
httpmock.StringResponse(`
1847+
{ "data": { "repository": { "assignableUsers": {
1848+
"nodes": [
1849+
{ "login": "hubot", "id": "HUBOTID", "name": "" },
1850+
{ "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name" }
1851+
],
1852+
"pageInfo": { "hasNextPage": false }
1853+
} } } }
1854+
`))
1855+
reg.Register(
1856+
httpmock.GraphQL(`query OrganizationTeamList\b`),
1857+
httpmock.StringResponse(`
1858+
{ "data": { "organization": { "teams": {
1859+
"nodes": [
1860+
{ "slug": "core", "id": "COREID" },
1861+
{ "slug": "robots", "id": "ROBOTID" }
1862+
],
1863+
"pageInfo": { "hasNextPage": false }
1864+
} } } }
1865+
`))
1866+
reg.Register(
1867+
httpmock.GraphQL(`mutation PullRequestCreate\b`),
1868+
httpmock.GraphQLMutation(`
1869+
{ "data": { "createPullRequest": { "pullRequest": {
1870+
"id": "NEWPULLID",
1871+
"URL": "https://github.com/OWNER/REPO/pull/12"
1872+
} } } }
1873+
`,
1874+
func(inputs map[string]interface{}) {
1875+
assert.Equal(t, "TITLE", inputs["title"])
1876+
assert.Equal(t, "BODY", inputs["body"])
1877+
if v, ok := inputs["assigneeIds"]; ok {
1878+
t.Errorf("did not expect assigneeIds: %v", v)
1879+
}
1880+
if v, ok := inputs["userIds"]; ok {
1881+
t.Errorf("did not expect userIds: %v", v)
1882+
}
1883+
}))
1884+
reg.Register(
1885+
httpmock.GraphQL(`mutation PullRequestCreateRequestReviews\b`),
1886+
httpmock.GraphQLMutation(`
1887+
{ "data": { "requestReviews": {
1888+
"clientMutationId": ""
1889+
} } }
1890+
`, func(inputs map[string]interface{}) {
1891+
assert.Equal(t, "NEWPULLID", inputs["pullRequestId"])
1892+
assert.Equal(t, []interface{}{"COREID"}, inputs["teamIds"])
1893+
assert.Equal(t, []interface{}{"MONAID"}, inputs["userIds"])
1894+
assert.Equal(t, true, inputs["union"])
1895+
}))
1896+
},
1897+
expectedOut: "https://github.com/OWNER/REPO/pull/12\n",
1898+
expectedErrOut: "\nCreating pull request for feature into master in OWNER/REPO\n\n",
1899+
},
17761900
}
17771901
for _, tt := range tests {
17781902
t.Run(tt.name, func(t *testing.T) {

‎pkg/cmd/pr/shared/completion.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,10 @@ import (
1414
func RequestableReviewersForCompletion(httpClient *http.Client, repo ghrepo.Interface) ([]string, error) {
1515
client := api.NewClientFromHTTP(api.NewCachedHTTPClient(httpClient, time.Minute*2))
1616

17-
metadata, err := api.RepoMetadata(client, repo, api.RepoMetadataInput{Reviewers: true})
17+
metadata, err := api.RepoMetadata(client, repo, api.RepoMetadataInput{
18+
Reviewers: true,
19+
TeamReviewers: true,
20+
})
1821
if err != nil {
1922
return nil, err
2023
}

‎pkg/cmd/pr/shared/completion_test.go‎

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
package shared
2+
3+
import (
4+
"net/http"
5+
"testing"
6+
7+
"github.com/cli/cli/v2/internal/ghrepo"
8+
"github.com/cli/cli/v2/pkg/httpmock"
9+
"github.com/stretchr/testify/require"
10+
)
11+
12+
func TestRequestableReviewersForCompletion(t *testing.T) {
13+
tests := []struct {
14+
name string
15+
expectedReviewers []string
16+
httpStubs func(*httpmock.Registry, *testing.T)
17+
}{
18+
{
19+
name: "when users and teams are both available, both are listed",
20+
expectedReviewers: []string{"MonaLisa\tMona Display Name", "OWNER/core", "OWNER/robots", "hubot"},
21+
httpStubs: func(reg *httpmock.Registry, t *testing.T) {
22+
reg.Register(
23+
httpmock.GraphQL(`query UserCurrent\b`),
24+
httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`))
25+
reg.Register(
26+
httpmock.GraphQL(`query RepositoryAssignableUsers\b`),
27+
httpmock.StringResponse(`
28+
{ "data": { "repository": { "assignableUsers": {
29+
"nodes": [
30+
{ "login": "hubot", "id": "HUBOTID", "name": "" },
31+
{ "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name" }
32+
],
33+
"pageInfo": { "hasNextPage": false }
34+
} } } }
35+
`))
36+
reg.Register(
37+
httpmock.GraphQL(`query OrganizationTeamList\b`),
38+
httpmock.StringResponse(`
39+
{ "data": { "organization": { "teams": {
40+
"nodes": [
41+
{ "slug": "core", "id": "COREID" },
42+
{ "slug": "robots", "id": "ROBOTID" }
43+
],
44+
"pageInfo": { "hasNextPage": false }
45+
} } } }
46+
`))
47+
},
48+
},
49+
{
50+
name: "when users are available but teams aren't, users are listed",
51+
expectedReviewers: []string{"MonaLisa\tMona Display Name", "hubot"},
52+
httpStubs: func(reg *httpmock.Registry, t *testing.T) {
53+
reg.Register(
54+
httpmock.GraphQL(`query UserCurrent\b`),
55+
httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`))
56+
reg.Register(
57+
httpmock.GraphQL(`query RepositoryAssignableUsers\b`),
58+
httpmock.StringResponse(`
59+
{ "data": { "repository": { "assignableUsers": {
60+
"nodes": [
61+
{ "login": "hubot", "id": "HUBOTID", "name": "" },
62+
{ "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name" }
63+
],
64+
"pageInfo": { "hasNextPage": false }
65+
} } } }
66+
`))
67+
reg.Register(
68+
httpmock.GraphQL(`query OrganizationTeamList\b`),
69+
httpmock.StringResponse(`
70+
{ "data": { "organization": { "teams": {
71+
"nodes": [],
72+
"pageInfo": { "hasNextPage": false }
73+
} } } }
74+
`))
75+
},
76+
},
77+
{
78+
name: "when teams are available but users aren't, teams are listed",
79+
expectedReviewers: []string{"OWNER/core", "OWNER/robots"},
80+
httpStubs: func(reg *httpmock.Registry, t *testing.T) {
81+
reg.Register(
82+
httpmock.GraphQL(`query UserCurrent\b`),
83+
httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`))
84+
reg.Register(
85+
httpmock.GraphQL(`query RepositoryAssignableUsers\b`),
86+
httpmock.StringResponse(`
87+
{ "data": { "repository": { "assignableUsers": {
88+
"nodes": [],
89+
"pageInfo": { "hasNextPage": false }
90+
} } } }
91+
`))
92+
reg.Register(
93+
httpmock.GraphQL(`query OrganizationTeamList\b`),
94+
httpmock.StringResponse(`
95+
{ "data": { "organization": { "teams": {
96+
"nodes": [
97+
{ "slug": "core", "id": "COREID" },
98+
{ "slug": "robots", "id": "ROBOTID" }
99+
],
100+
"pageInfo": { "hasNextPage": false }
101+
} } } }
102+
`))
103+
},
104+
},
105+
}
106+
107+
for _, tt := range tests {
108+
t.Run(tt.name, func(t *testing.T) {
109+
reg := &httpmock.Registry{}
110+
defer reg.Verify(t)
111+
if tt.httpStubs != nil {
112+
tt.httpStubs(reg, t)
113+
}
114+
115+
reviewers, err := RequestableReviewersForCompletion(&http.Client{Transport: reg}, ghrepo.New("OWNER", "REPO"))
116+
require.NoError(t, err)
117+
require.Equal(t, tt.expectedReviewers, reviewers)
118+
})
119+
}
120+
}

‎pkg/cmd/pr/shared/survey.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,7 @@ func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface
181181
// Retrieve and process data for survey prompts based on the extra fields selected
182182
metadataInput := api.RepoMetadataInput{
183183
Reviewers: isChosen("Reviewers"),
184+
TeamReviewers: isChosen("Reviewers"),
184185
Assignees: isChosen("Assignees"),
185186
ActorAssignees: isChosen("Assignees") && state.ActorAssignees,
186187
Labels: isChosen("Labels"),

0 commit comments

Comments
 (0)