diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts index 5668eedceb69f..f8076a29e9c4a 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts @@ -4270,42 +4270,70 @@ layer("GitHubPullRequestApi.layer", (it) => { }), ); - it.effect("reads who may review and who already has in one request", () => - Effect.gen(function* () { - mockedExecute.mockReturnValue( - Effect.succeed( - output( - encodeJson({ - data: { - repository: { - assignableUsers: { - pageInfo: { hasNextPage: false }, - nodes: ["bilal", "octocat", "hubot"].map((login) => ({ - login, - name: null, - avatarUrl: `https://avatars/${login}`, - })), - }, - pullRequest: { - author: { __typename: "User", login: "bilal" }, - reviewRequests: { - nodes: [ - { - requestedReviewer: { - __typename: "User", - login: "octocat", - name: null, - avatarUrl: "https://avatars/octocat", - }, - }, - ], - }, - }, - }, + const reviewerTeam = { + slug: "web", + combinedSlug: "acme/web", + name: "Web", + avatarUrl: null, + repositories: { nodes: [{ nameWithOwner: "acme/web" }] }, + }; + const reviewerTeams = (teams: ReadonlyArray) => ({ + data: { organization: { teams: { pageInfo: { hasNextPage: false }, nodes: teams } } }, + }); + const reviewerPeople = (assignable: ReadonlyArray, requested: ReadonlyArray) => ({ + data: { + repository: { + assignableUsers: { + pageInfo: { hasNextPage: false }, + nodes: assignable.map((login) => ({ + login, + name: null, + avatarUrl: `https://avatars/${login}`, + })), + }, + pullRequest: { + author: { __typename: "User", login: "bilal" }, + reviewRequests: { + nodes: requested.map((login) => ({ + requestedReviewer: { + __typename: "User", + login, + name: null, + avatarUrl: `https://avatars/${login}`, }, - }), - ), - ), + })), + }, + }, + }, + }, + }); + + it.effect.each([ + { + name: "lists teams with access beside the people", + teams: reviewerTeams([reviewerTeam]), + expected: [ + ["octocat", true], + ["acme/web", false], + ["hubot", false], + ], + }, + { + name: "still lists the people when the token cannot read the organization's teams", + teams: { + data: null, + errors: [{ type: "INSUFFICIENT_SCOPES", message: "Your token needs read:org." }], + }, + expected: [ + ["octocat", true], + ["hubot", false], + ], + }, + ])("$name", ({ teams, expected }) => + Effect.gen(function* () { + route( + ["ReviewerCandidates", reviewerPeople(["bilal", "octocat", "hubot"], ["octocat"])], + ["ReviewerTeams", teams], ); const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; @@ -4316,45 +4344,31 @@ layer("GitHubPullRequestApi.layer", (it) => { number: 7, }); - assert.strictEqual(mockedExecute.mock.calls.length, 1); - expect(varsAt(0)).toMatchObject({ owner: "acme", name: "web", number: 7 }); - expect(list.candidates.map((candidate) => [candidate.login, candidate.isRequested])).toEqual([ - ["octocat", true], - ["hubot", false], + expect(variablesOf("ReviewerCandidates")).toMatchObject([ + { owner: "acme", name: "web", number: 7 }, ]); + expect(variablesOf("ReviewerTeams")).toMatchObject([{ owner: "acme", name: "web" }]); + expect(list.candidates.map((candidate) => [candidate.login, candidate.isRequested])).toEqual( + expected, + ); }), ); it.effect("asks GitHub to search everyone assignable when the reviewer menu has a query", () => Effect.gen(function* () { - mockedExecute.mockReturnValue( - Effect.succeed( - output( - encodeJson({ - data: { - repository: { - assignableUsers: { - pageInfo: { hasNextPage: false }, - nodes: [{ login: "octocat", name: null, avatarUrl: "https://avatars/octocat" }], - }, - pullRequest: { - author: { __typename: "User", login: "bilal" }, - reviewRequests: { - nodes: ["octocat", "unrelated"].map((login) => ({ - requestedReviewer: { - __typename: "User", - login, - name: null, - avatarUrl: "https://avatars/user", - }, - })), - }, - }, - }, - }, - }), - ), - ), + route( + ["ReviewerCandidates", reviewerPeople(["octocat"], ["octocat", "unrelated"])], + [ + "ReviewerTeams", + reviewerTeams([ + { + ...reviewerTeam, + slug: "octonauts", + combinedSlug: "acme/octonauts", + name: "Octonauts", + }, + ]), + ], ); const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; @@ -4366,9 +4380,15 @@ layer("GitHubPullRequestApi.layer", (it) => { query: "octo", }); - expect(varsAt(0)).toMatchObject({ owner: "acme", name: "web", number: 7, query: "octo" }); + expect(variablesOf("ReviewerCandidates")).toMatchObject([ + { owner: "acme", name: "web", number: 7, query: "octo" }, + ]); + expect(variablesOf("ReviewerTeams")).toMatchObject([ + { owner: "acme", name: "web", query: "octo" }, + ]); expect(list.candidates.map((candidate) => [candidate.login, candidate.isRequested])).toEqual([ ["octocat", true], + ["acme/octonauts", false], ]); }), ); diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.ts index 5454926281fcc..ff6685896ccba 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.ts @@ -103,6 +103,7 @@ import { toPullRequestWatchFingerprint, toReviewDismissalsPage, toReviewerCandidates, + toReviewerTeams, toReviewThreadCommentsPage, toLaterReviewThreadPage, toReviewThreadPage, @@ -2463,13 +2464,25 @@ export const make = Effect.gen(function* () { function (input) { const { owner, name } = parseRepositorySelector(input.repository); const operation = "listReviewerCandidates"; - return client - .ReviewerCandidates({ owner, name, number: input.number, query: input.query ?? null }) - .pipe( - graphql({ ...input, operation, allowReserve: true }), - Effect.map((result) => toReviewerCandidates(result, input.query)), - Effect.flatMap(found({ ...input, operation })), - ); + const target = { ...input, operation, allowReserve: true }; + const query = input.query ?? null; + return Effect.all( + [ + client + .ReviewerCandidates({ owner, name, number: input.number, query }) + .pipe(graphql(target)), + client.ReviewerTeams({ owner, name, query }).pipe( + graphql(target), + Effect.map((result) => toReviewerTeams(result, `${owner}/${name}`)), + // Teams need `read:org` and an organization owner; people are still worth listing. + Effect.orElseSucceed(() => null), + ), + ], + { concurrency: "unbounded" }, + ).pipe( + Effect.map(([people, teams]) => toReviewerCandidates(people, input.query, teams)), + Effect.flatMap(found({ ...input, operation })), + ); }, ), diff --git a/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts b/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts index 5f084ac203fb6..0061e95764a2c 100644 --- a/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts +++ b/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts @@ -19,6 +19,7 @@ import { toPullRequestFilesViewedPage, toPullRequestRows, toReviewerCandidates, + toReviewerTeams, toReviewThreadCommentsPage, toCanWrite, toReviewThreadPage, @@ -31,6 +32,7 @@ import { PullRequestCore, PullRequestFilesViewed, ReviewerCandidates, + ReviewerTeams, ViewerPermissions, } from "./pullRequestReads.graphql.ts"; import { ReviewThreadComments, ReviewThreads } from "./pullRequestReviews.graphql.ts"; @@ -212,6 +214,8 @@ const decodeReviewThreadCommentsJson = (raw: string) => const decodeViewerPermissionsJson = through(ViewerPermissions.result, toViewerPermissions); const decodeLabelCandidatesJson = through(LabelCandidates.result, toLabelCandidates); const decodeReviewerCandidatesJson = through(ReviewerCandidates.result, toReviewerCandidates); +const decodeReviewerCandidates = Schema.decodeUnknownSync(ReviewerCandidates.result); +const decodeReviewerTeams = Schema.decodeSync(ReviewerTeams.result); const decodePullRequestFilesViewedJson = through( PullRequestFilesViewed.result, toPullRequestFilesViewedPage, @@ -1462,6 +1466,49 @@ describe("reviewer candidate decoding", () => { }, ]); }); + + it("offers the organization's teams that can reach this repository, each once", () => { + const team = (slug: string, repositories: ReadonlyArray) => ({ + slug, + combinedSlug: `acme/${slug}`, + name: slug, + avatarUrl: null, + repositories: { nodes: repositories.map((nameWithOwner) => ({ nameWithOwner })) }, + }); + const teams = decodeReviewerTeams({ + organization: { + teams: { + pageInfo: { hasNextPage: true }, + nodes: [ + team("reviewers", ["acme/web"]), + team("frontend", ["ACME/Web"]), + team("legacy", ["acme/web-legacy"]), + ], + }, + }, + }); + const people = JSON.parse( + candidatesJson({ + assignable: [{ login: "octocat" }], + requested: [{ slug: "reviewers", combinedSlug: "acme/reviewers", teamName: "Reviewers" }], + }), + ) as { readonly data: unknown }; + + const list = toReviewerCandidates( + decodeReviewerCandidates(people.data), + undefined, + toReviewerTeams(teams, "acme/web"), + ); + + expect( + list?.candidates.map(({ kind, login, isRequested }) => [kind, login, isRequested]), + ).toEqual([ + ["team", "acme/reviewers", true], + ["team", "acme/frontend", false], + ["user", "octocat", false], + ]); + expect(list?.truncated).toBe(true); + }); }); describe("reviewer request payload", () => { diff --git a/packages/source-control-github/src/server/gitHubPullRequestJson.ts b/packages/source-control-github/src/server/gitHubPullRequestJson.ts index a2b3f3c2c3edd..63f99b2a7a3c9 100644 --- a/packages/source-control-github/src/server/gitHubPullRequestJson.ts +++ b/packages/source-control-github/src/server/gitHubPullRequestJson.ts @@ -73,6 +73,7 @@ import type { RepositoryReferences, ReferenceCandidate, ReviewerCandidates, + ReviewerTeams, ViewerPermissions, } from "./pullRequestReads.graphql.ts"; import type { @@ -1555,10 +1556,41 @@ export interface GitHubBaseComparison { readonly viewerCanUpdate: boolean; } +/** Teams with access to `repository` (owner/name). Null when the owner is not an organization. */ +export function toReviewerTeams( + result: ReviewerTeams.Result, + repository: string, +): PullRequestReviewerCandidateList | null { + const teams = result.organization?.teams; + if (teams == null) return null; + const wanted = repository.toLowerCase(); + return { + candidates: nodesOf(teams).flatMap((team) => { + const slug = trimmed(team.slug); + const hasAccess = nodesOf(team.repositories).some( + (node) => node.nameWithOwner.toLowerCase() === wanted, + ); + if (slug === null || !hasAccess) return []; + return [ + { + id: slug, + kind: "team" as const, + login: trimmed(team.combinedSlug) ?? slug, + name: trimmed(team.name), + avatarUrl: trimmed(team.avatarUrl), + isRequested: false, + }, + ]; + }), + truncated: teams.pageInfo.hasNextPage, + }; +} + /** assignableUsers avoids push/read:org requirements. Keep requested outsiders/apps removable. */ export function toReviewerCandidates( result: ReviewerCandidates.Result, query?: string, + teams: PullRequestReviewerCandidateList | null = null, ): PullRequestReviewerCandidateList | null { const repository = result.repository; if (repository == null) return null; @@ -1583,9 +1615,13 @@ export function toReviewerCandidates( }); } const hostMatches = new Set(); + for (const team of teams?.candidates ?? []) { + hostMatches.add(`team ${team.id}`); + if (!candidates.has(`team ${team.id}`)) candidates.set(`team ${team.id}`, team); + } for (const node of nodesOf(repository.assignableUsers)) { const login = trimmed(node.login); - if (login !== null) hostMatches.add(login); + if (login !== null) hostMatches.add(`user ${login}`); if (login === null || login === author || candidates.has(`user ${login}`)) continue; candidates.set(`user ${login}`, { id: login, @@ -1598,9 +1634,9 @@ export function toReviewerCandidates( } return { candidates: [...candidates.values()].filter( - (candidate) => !query?.trim() || (candidate.kind === "user" && hostMatches.has(candidate.id)), + (candidate) => !query?.trim() || hostMatches.has(`${candidate.kind} ${candidate.id}`), ), - truncated: repository.assignableUsers.pageInfo.hasNextPage, + truncated: repository.assignableUsers.pageInfo.hasNextPage || teams?.truncated === true, }; } diff --git a/packages/source-control-github/src/server/pullRequestReads.graphql b/packages/source-control-github/src/server/pullRequestReads.graphql index 88fbe80828e1e..862461f8a79f2 100644 --- a/packages/source-control-github/src/server/pullRequestReads.graphql +++ b/packages/source-control-github/src/server/pullRequestReads.graphql @@ -421,10 +421,10 @@ query ViewerPermissions($owner: String!, $name: String!, $number: Int!) { # the repository) rather than `collaborators`, which the REST API refuses to anyone without push # access and which would therefore be empty for exactly the reader most likely to be looking. # -# Teams are asked for only where one has already been requested, so a request to a team can be -# taken back. The teams a repository could newly be sent to live on the owning organization and -# need `read:org`, which a repository-scoped token need not carry, and a query GitHub refuses -# fails whole, taking the people down with the teams. +# Teams already requested are read here so those requests can be taken back. The teams a review +# could newly be sent to come from ReviewerTeams, a separate read: they need `read:org`, which a +# repository-scoped token need not carry, and a query GitHub refuses fails whole, taking the +# people down with the teams. query ReviewerCandidates($owner: String!, $name: String!, $number: Int!, $query: String) { repository(owner: $owner, name: $name) { assignableUsers(first: 100, query: $query) { @@ -467,6 +467,30 @@ query ReviewerCandidates($owner: String!, $name: String!, $number: Int!, $query: } } +# GitHub refuses a review request to a team without access to the repository, and the REST list of +# a repository's teams needs admin, so each team's repositories are searched for this one by name. +# `query` matches loosely; the exact repository is picked out from the results. +query ReviewerTeams($owner: String!, $name: String!, $query: String) { + organization(login: $owner) { + teams(first: 100, query: $query) { + pageInfo { + hasNextPage + } + nodes { + slug + combinedSlug + name + avatarUrl + repositories(query: $name, first: 100) { + nodes { + nameWithOwner + } + } + } + } + } +} + query LabelCandidates($owner: String!, $name: String!, $number: Int!) { repository(owner: $owner, name: $name) { labels(first: 100, orderBy: { field: NAME, direction: ASC }) { diff --git a/packages/source-control-github/src/server/pullRequestReads.graphql.ts b/packages/source-control-github/src/server/pullRequestReads.graphql.ts index a12334d8f98c3..9fb87315ccebd 100644 --- a/packages/source-control-github/src/server/pullRequestReads.graphql.ts +++ b/packages/source-control-github/src/server/pullRequestReads.graphql.ts @@ -325,7 +325,7 @@ export const RequestedReviewerBot = Schema.Struct({ export interface RequestedReviewerBot extends Schema.Struct.Type {} /** A team of users in an organization. */ -export const Team = Schema.Struct({ +export const RequestedReviewerTeam = Schema.Struct({ __typename: Schema.Literal("Team"), /** The slug corresponding to the team. */ slug: Schema.String, @@ -336,7 +336,7 @@ export const Team = Schema.Struct({ /** A URL pointing to the team's avatar. */ teamAvatarUrl: Schema.NullOr(Shared.URI) }) -export interface Team extends Schema.Struct.Type {} +export interface RequestedReviewerTeam extends Schema.Struct.Type {} /** Types that can be requested reviewers. */ export const RequestedReviewerOther = Schema.Struct({ @@ -350,7 +350,7 @@ export const PullRequestCoreRepositoryPullRequestReviewRequestsNodesReviewReques requestedReviewer: Schema.NullOr(Schema.Union([ RequestedReviewerUser, RequestedReviewerBot, - Team, + RequestedReviewerTeam, RequestedReviewerOther ])) }) @@ -1199,7 +1199,7 @@ export const ReviewerCandidatesRepositoryPullRequestReviewRequestsNodesReviewReq /** The reviewer that is requested. */ requestedReviewer: Schema.NullOr(Schema.Union([ PullRequestPreviewRepositoryPullRequestAuthorUser, - Team, + RequestedReviewerTeam, NodesBot, RequestedReviewerOther ])) @@ -1261,6 +1261,79 @@ export declare namespace ReviewerCandidates { export type Result = typeof ReviewerCandidates.result.Type } +/** A repository contains the content for a project. */ +export const NodesRepository = Schema.Struct({ + /** The repository's name with owner. */ + nameWithOwner: Schema.String +}) +export interface NodesRepository extends Schema.Struct.Type {} + +/** The connection type for Repository. */ +export const TeamRepositoryConnection = Schema.Struct({ + /** A list of nodes. */ + nodes: Schema.NullOr(Schema.Array(Schema.NullOr(NodesRepository))) +}) +export interface TeamRepositoryConnection extends Schema.Struct.Type {} + +/** A team of users in an organization. */ +export const NodesTeam = Schema.Struct({ + /** The slug corresponding to the team. */ + slug: Schema.String, + /** The slug corresponding to the organization and team. */ + combinedSlug: Schema.String, + /** The name of the team. */ + name: Schema.String, + /** A URL pointing to the team's avatar. */ + avatarUrl: Schema.NullOr(Shared.URI), + /** A list of repositories this team has access to. */ + repositories: TeamRepositoryConnection +}) +export interface NodesTeam extends Schema.Struct.Type {} + +/** The connection type for Team. */ +export const TeamConnection = Schema.Struct({ + /** Information to aid in pagination. */ + pageInfo: AssignableUsersPageInfoPageInfo, + /** A list of nodes. */ + nodes: Schema.NullOr(Schema.Array(Schema.NullOr(NodesTeam))) +}) +export interface TeamConnection extends Schema.Struct.Type {} + +/** An account on GitHub, with one or more owners, that has repositories, members and teams. */ +export const Organization = Schema.Struct({ + /** A list of teams in this organization. */ + teams: TeamConnection +}) +export interface Organization extends Schema.Struct.Type {} + +/** `query ReviewerTeams` */ +export const ReviewerTeams = GraphQL.query("ReviewerTeams", { + document: { + variables: [ + { name: "owner", body: "String!" }, + { name: "name", body: "String!" }, + { name: "query", body: "String" } + ], + selections: [ + { name: "organization", body: ["(login:$", "owner", "){teams(first:100 query:$", "query", "){pageInfo{hasNextPage}nodes{slug combinedSlug name avatarUrl repositories(query:$", "name", " first:100){nodes{nameWithOwner}}}}}"] } + ], + fragments: [] + }, + variables: { + owner: Schema.String, + name: Schema.String, + query: Schema.optional(Schema.NullOr(Schema.String)) + }, + result: Schema.Struct({ + /** Lookup a organization by login. */ + organization: Schema.NullOr(Organization) + }) +}) +export declare namespace ReviewerTeams { + export type Variables = typeof ReviewerTeams.variables.Type + export type Result = typeof ReviewerTeams.result.Type +} + /** The connection type for Label. */ export const LabelCandidatesRepositoryLabelsLabelConnection = Schema.Struct({ /** Information to aid in pagination. */ @@ -1834,4 +1907,4 @@ export declare namespace PullRequestSourceBranch { } /** Every operation in pullRequestReads.graphql. */ -export const PullRequestReadsGraphQLGroup = GraphQLGroup.make(RoutingIdentityViewer, ActorAvatars, PullRequestCore, PullRequestCheckContexts, PullRequestPreview, PullRequestActivity, PullRequestCommitStats, PullRequestHeads, PullRequestNodeId, PullRequestSubject, ViewerPermissions, ReviewerCandidates, LabelCandidates, RepositoryLabels, RepositoryReferences, RepositoryAuthors, PullRequestFilesViewed, PullRequestFileStats, PullRequestMergeMessage, PullRequestActionState, PullRequestBranchToDelete, PullRequestSourceBranch) +export const PullRequestReadsGraphQLGroup = GraphQLGroup.make(RoutingIdentityViewer, ActorAvatars, PullRequestCore, PullRequestCheckContexts, PullRequestPreview, PullRequestActivity, PullRequestCommitStats, PullRequestHeads, PullRequestNodeId, PullRequestSubject, ViewerPermissions, ReviewerCandidates, ReviewerTeams, LabelCandidates, RepositoryLabels, RepositoryReferences, RepositoryAuthors, PullRequestFilesViewed, PullRequestFileStats, PullRequestMergeMessage, PullRequestActionState, PullRequestBranchToDelete, PullRequestSourceBranch)