diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts index 5a5c46f28d99a..1b1753aa821eb 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts @@ -3770,42 +3770,74 @@ layer("GitHubPullRequestApi.layer", (it) => { }), ); - it.effect("reads who may review and who already has in one request", () => + const reviewerTeam = { + slug: "web", + combinedSlug: "acme/web", + name: "Web", + avatarUrl: null, + repositories: { nodes: [{ nameWithOwner: "acme/web" }] }, + }; + it.effect.each([ + { + name: "lists teams with access beside the people", + teams: { + data: { + organization: { teams: { pageInfo: { hasNextPage: false }, nodes: [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* () { - 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", - }, + route( + [ + "ReviewerCandidates", + { + 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", }, - ], - }, + }, + ], }, }, }, - }), - ), - ), + }, + }, + ], + ["ReviewerTeams", teams], ); const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; @@ -3816,12 +3848,13 @@ 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")).toEqual([ + { owner: "acme", name: "web", number: 7 }, ]); + expect(variablesOf("ReviewerTeams")).toEqual([{ owner: "acme", name: "web" }]); + expect(list.candidates.map((candidate) => [candidate.login, candidate.isRequested])).toEqual( + expected, + ); }), ); diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.ts index 2defbb6684919..8fc3b23904f17 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.ts @@ -86,6 +86,7 @@ import { toPullRequestWatchFingerprint, toReviewDismissalsPage, toReviewerCandidates, + toReviewerTeams, toReviewThreadCommentsPage, toReviewThreadPage, toStackMembership, @@ -2279,13 +2280,22 @@ 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 }) - .pipe( - graphql({ ...input, operation, allowReserve: true }), - Effect.map(toReviewerCandidates), - Effect.flatMap(found({ ...input, operation })), - ); + const target = { ...input, operation, allowReserve: true }; + return Effect.all( + [ + client.ReviewerCandidates({ owner, name, number: input.number }).pipe(graphql(target)), + client.ReviewerTeams({ owner, name }).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, 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 c816f76a7801d..d63bdaf1ee577 100644 --- a/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts +++ b/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts @@ -17,6 +17,7 @@ import { toPullRequestFilesViewedPage, toPullRequestRows, toReviewerCandidates, + toReviewerTeams, toReviewThreadCommentsPage, toCanWrite, toReviewThreadPage, @@ -29,6 +30,7 @@ import { PullRequestCore, PullRequestFilesViewed, ReviewerCandidates, + ReviewerTeams, ViewerPermissions, } from "./pullRequestReads.graphql.ts"; import { ReviewThreadComments, ReviewThreads } from "./pullRequestReviews.graphql.ts"; @@ -207,6 +209,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, @@ -1335,6 +1339,48 @@ 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), + 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 7b8851f7bed2d..4fa0f8b49c4f6 100644 --- a/packages/source-control-github/src/server/gitHubPullRequestJson.ts +++ b/packages/source-control-github/src/server/gitHubPullRequestJson.ts @@ -64,6 +64,7 @@ import type { RepositoryAuthors, RepositoryLabels, ReviewerCandidates, + ReviewerTeams, ViewerPermissions, } from "./pullRequestReads.graphql.ts"; import type { @@ -1431,9 +1432,40 @@ 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, + teams: PullRequestReviewerCandidateList | null = null, ): PullRequestReviewerCandidateList | null { const repository = result.repository; if (repository == null) return null; @@ -1457,6 +1489,9 @@ export function toReviewerCandidates( isRequested: true, }); } + for (const team of teams?.candidates ?? []) { + 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 || login === author || candidates.has(`user ${login}`)) continue; @@ -1471,7 +1506,7 @@ export function toReviewerCandidates( } return { candidates: [...candidates.values()], - 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 78e5efe0e4558..bc6e977044442 100644 --- a/packages/source-control-github/src/server/pullRequestReads.graphql +++ b/packages/source-control-github/src/server/pullRequestReads.graphql @@ -404,10 +404,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!) { repository(owner: $owner, name: $name) { assignableUsers(first: 100) { @@ -450,6 +450,30 @@ query ReviewerCandidates($owner: String!, $name: String!, $number: Int!) { } } +# 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!) { + organization(login: $owner) { + teams(first: 100) { + 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 5bc09fa4e2ce4..701d5ce01753e 100644 --- a/packages/source-control-github/src/server/pullRequestReads.graphql.ts +++ b/packages/source-control-github/src/server/pullRequestReads.graphql.ts @@ -255,7 +255,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, @@ -266,7 +266,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({ @@ -280,7 +280,7 @@ export const PullRequestCoreRepositoryPullRequestReviewRequestsNodesReviewReques requestedReviewer: Schema.NullOr(Schema.Union([ RequestedReviewerUser, RequestedReviewerBot, - Team, + RequestedReviewerTeam, RequestedReviewerOther ])) }) @@ -1156,7 +1156,7 @@ export const ReviewerCandidatesRepositoryPullRequestReviewRequestsNodesReviewReq /** The reviewer that is requested. */ requestedReviewer: Schema.NullOr(Schema.Union([ PullRequestPreviewRepositoryPullRequestAuthorUser, - Team, + RequestedReviewerTeam, NodesBot, RequestedReviewerOther ])) @@ -1216,6 +1216,77 @@ 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: PullRequestCoreRepositoryPullRequestCommitsNodesCommitStatusCheckRollupContextsPageInfoPageInfo, + /** 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!" } + ], + selections: [ + { name: "organization", body: ["(login:$", "owner", "){teams(first:100){pageInfo{hasNextPage}nodes{slug combinedSlug name avatarUrl repositories(query:$", "name", " first:100){nodes{nameWithOwner}}}}}"] } + ], + fragments: [] + }, + variables: { + owner: Schema.String, + name: 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 +} + /** A label for categorizing Issues, Pull Requests, Milestones, or Discussions with a given Repository. */ export const RepositoryLabelsNodesLabel = Schema.Struct({ /** Identifies the label name. */ @@ -1724,4 +1795,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, 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, RepositoryAuthors, PullRequestFilesViewed, PullRequestFileStats, PullRequestMergeMessage, PullRequestActionState, PullRequestBranchToDelete, PullRequestSourceBranch)