From 35525bd8a79b6b2fbb520d3dee75f1c707206b98 Mon Sep 17 00:00:00 2001 From: Alex Ross Date: Fri, 10 Feb 2023 11:32:28 +0100 Subject: [PATCH 1/4] Add Team Reviewers to Pull Request Fixes #1126 --- src/common/githubRef.ts | 2 +- src/github/activityBarViewProvider.ts | 4 +- src/github/credentials.ts | 2 +- src/github/folderRepositoryManager.ts | 89 +++++++++++++++++- src/github/githubRepository.ts | 83 ++++++++++++++++- src/github/graphql.ts | 59 ++++++++++++ src/github/interface.ts | 24 ++++- src/github/pullRequestModel.ts | 68 ++++++++++---- src/github/pullRequestOverview.ts | 93 ++++++++++++++----- src/github/queries.gql | 69 ++++++++++++++ src/github/utils.ts | 22 ++++- .../builders/graphql/pullRequestBuilder.ts | 1 + webviews/components/merge.tsx | 4 +- webviews/components/reviewer.tsx | 4 +- webviews/components/sidebar.tsx | 4 +- webviews/components/user.tsx | 6 +- 16 files changed, 470 insertions(+), 64 deletions(-) diff --git a/src/common/githubRef.ts b/src/common/githubRef.ts index 89ad60b069..eb6847443b 100644 --- a/src/common/githubRef.ts +++ b/src/common/githubRef.ts @@ -8,7 +8,7 @@ import { Protocol } from './protocol'; export class GitHubRef { public repositoryCloneUrl: Protocol; constructor(public ref: string, public label: string, public sha: string, repositoryCloneUrl: string, - public readonly owner: string, public readonly name: string) { + public readonly owner: string, public readonly name: string, public readonly isInOrganization: boolean) { this.repositoryCloneUrl = new Protocol(repositoryCloneUrl); } } diff --git a/src/github/activityBarViewProvider.ts b/src/github/activityBarViewProvider.ts index 4a78f07e22..bffdcf55e8 100644 --- a/src/github/activityBarViewProvider.ts +++ b/src/github/activityBarViewProvider.ts @@ -11,7 +11,7 @@ import { dispose, formatError } from '../common/utils'; import { getNonce, IRequestMessage, WebviewViewBase } from '../common/webview'; import { ReviewManager } from '../view/reviewManager'; import { FolderRepositoryManager } from './folderRepositoryManager'; -import { GithubItemStateEnum, ReviewEvent, ReviewState } from './interface'; +import { GithubItemStateEnum, reviewerId, ReviewEvent, ReviewState } from './interface'; import { PullRequestModel } from './pullRequestModel'; import { getDefaultMergeMethod } from './pullRequestOverview'; import { PullRequestView } from './pullRequestOverviewCommon'; @@ -266,7 +266,7 @@ export class PullRequestViewProvider extends WebviewViewBase implements vscode.W private updateReviewers(review?: CommonReviewEvent): void { if (review) { const existingReviewer = this._existingReviewers.find( - reviewer => review.user.login === reviewer.reviewer.login, + reviewer => review.user.login === reviewerId(reviewer.reviewer), ); if (existingReviewer) { existingReviewer.state = review.state; diff --git a/src/github/credentials.ts b/src/github/credentials.ts index 43b7cc933d..66836e1aa9 100644 --- a/src/github/credentials.ts +++ b/src/github/credentials.ts @@ -28,7 +28,7 @@ const PROMPT_FOR_SIGN_IN_STORAGE_KEY = 'login'; // If the scopes are changed, make sure to notify all interested parties to make sure this won't cause problems. const SCOPES_OLD = ['read:user', 'user:email', 'repo']; -export const SCOPES = ['read:user', 'user:email', 'repo', 'workflow']; +export const SCOPES = ['read:user', 'user:email', 'repo', 'workflow', 'read:org']; export interface GitHub { octokit: LoggingOctokit; diff --git a/src/github/folderRepositoryManager.ts b/src/github/folderRepositoryManager.ts index 764f8860d6..475c2226d2 100644 --- a/src/github/folderRepositoryManager.ts +++ b/src/github/folderRepositoryManager.ts @@ -27,7 +27,7 @@ import { OctokitCommon } from './common'; import { CredentialStore } from './credentials'; import { GitHubRepository, ItemsData, PullRequestData, ViewerPermission } from './githubRepository'; import { PullRequestState, UserResponse } from './graphql'; -import { IAccount, ILabel, IMilestone, IPullRequestsPagingOptions, PRType, RepoAccessAndMergeMethods, User } from './interface'; +import { IAccount, ILabel, IMilestone, IPullRequestsPagingOptions, ITeam, PRType, RepoAccessAndMergeMethods, User } from './interface'; import { IssueModel } from './issueModel'; import { MilestoneModel } from './milestoneModel'; import { PullRequestGitHelper, PullRequestMetadata } from './pullRequestGitHelper'; @@ -39,6 +39,7 @@ import { getRelatedUsersFromTimelineEvents, loginComparator, parseGraphQLUser, + teamComparator, variableSubstitution, } from './utils'; @@ -125,7 +126,9 @@ export class FolderRepositoryManager implements vscode.Disposable { private _mentionableUsers?: { [key: string]: IAccount[] }; private _fetchMentionableUsersPromise?: Promise<{ [key: string]: IAccount[] }>; private _assignableUsers?: { [key: string]: IAccount[] }; + private _teamReviewers?: { [key: string]: ITeam[] }; private _fetchAssignableUsersPromise?: Promise<{ [key: string]: IAccount[] }>; + private _fetchTeamReviewersPromise?: Promise<{ [key: string]: ITeam[] }>; private _gitBlameCache: { [key: string]: string } = {}; private _githubManager: GitHubManager; private _repositoryPageInformation: Map = new Map(); @@ -734,7 +737,7 @@ export class FolderRepositoryManager implements vscode.Disposable { // file doesn't exist or json is unexpectedly invalid } if (repoSpecificCache) { - cache[repo.remote.repositoryName] = repoSpecificCache; + cache[repo.remote.remoteName] = repoSpecificCache; return true; } }))).every(value => value); @@ -747,6 +750,44 @@ export class FolderRepositoryManager implements vscode.Disposable { return undefined; } + private async getTeamReviewersFromGlobalState(): Promise<{ [key: string]: ITeam[] } | undefined> { + Logger.appendLine('Trying to use globalState for team reviewers.'); + + const teamReviewersCacheLocation = vscode.Uri.joinPath(this.context.globalStorageUri, 'teamReviewers'); + let teamReviewersCacheExists; + try { + teamReviewersCacheExists = await vscode.workspace.fs.stat(teamReviewersCacheLocation); + } catch (e) { + // file doesn't exit + } + if (!teamReviewersCacheExists) { + return undefined; + } + + const cache: { [key: string]: ITeam[] } = {}; + const hasAllRepos = (await Promise.all(this._githubRepositories.map(async (repo) => { + const key = `${repo.remote.owner}/${repo.remote.repositoryName}.json`; + const repoSpecificFile = vscode.Uri.joinPath(teamReviewersCacheLocation, key); + let repoSpecificCache; + try { + repoSpecificCache = await vscode.workspace.fs.readFile(repoSpecificFile); + } catch (e) { + // file doesn't exist + } + if (repoSpecificCache && repoSpecificCache.toString()) { + cache[repo.remote.remoteName] = JSON.parse(repoSpecificCache.toString()) ?? []; + return true; + } + }))).every(value => value); + if (hasAllRepos) { + Logger.appendLine(`Using globalState team reviewers for ${Object.keys(cache).length}.`); + return cache; + } + + Logger.appendLine(`No globalState for team reviewers.`); + return undefined; + } + private createFetchMentionableUsersPromise(): Promise<{ [key: string]: IAccount[] }> { const cache: { [key: string]: IAccount[] } = {}; return new Promise<{ [key: string]: IAccount[] }>(resolve => { @@ -821,6 +862,50 @@ export class FolderRepositoryManager implements vscode.Disposable { return this._fetchAssignableUsersPromise; } + async getTeamReviewers(clearCache?: boolean): Promise<{ [key: string]: ITeam[] }> { + if (clearCache) { + delete this._teamReviewers; + } + + if (this._teamReviewers) { + return this._teamReviewers; + } + + const globalStateTeamReviewers = clearCache ? undefined : await this.getTeamReviewersFromGlobalState(); + if (globalStateTeamReviewers) { + return globalStateTeamReviewers || {}; + } + + if (!this._fetchTeamReviewersPromise) { + const cache: { [key: string]: ITeam[] } = {}; + return (this._fetchTeamReviewersPromise = new Promise(resolve => { + const promises = this._githubRepositories.map(async githubRepository => { + const data = await githubRepository.getTeams(); + cache[githubRepository.remote.remoteName] = data.sort(teamComparator); + return; + }); + + Promise.all(promises).then(() => { + this._teamReviewers = cache; + this._fetchTeamReviewersPromise = undefined; + const teamReviewersCacheLocation = vscode.Uri.joinPath(this.context.globalStorageUri, 'teamReviewers'); + Promise.all(this._githubRepositories.map(async (repo) => { + const key = `${repo.remote.owner}/${repo.remote.repositoryName}.json`; + const repoSpecificFile = vscode.Uri.joinPath(teamReviewersCacheLocation, key); + await vscode.workspace.fs.writeFile(repoSpecificFile, new TextEncoder().encode(JSON.stringify(cache[repo.remote.remoteName]))); + })); + resolve(cache); + }); + })); + } + + return this._fetchTeamReviewersPromise; + } + + async getOrgTeamsCount(repository: GitHubRepository): Promise { + return repository.getOrgTeamsCount(); + } + async getPullRequestParticipants(githubRepository: GitHubRepository, pullRequestNumber: number): Promise<{ participants: IAccount[], viewer: IAccount }> { return { participants: await githubRepository.getPullRequestParticipants(pullRequestNumber), diff --git a/src/github/githubRepository.ts b/src/github/githubRepository.ts index 3572d715ca..18f13a7624 100644 --- a/src/github/githubRepository.ts +++ b/src/github/githubRepository.ts @@ -26,12 +26,14 @@ import { MaxIssueResponse, MentionableUsersResponse, MilestoneIssuesResponse, + OrganizationTeamsCountResponse, + OrganizationTeamsResponse, PullRequestParticipantsResponse, PullRequestResponse, PullRequestsResponse, ViewerPermissionResponse, } from './graphql'; -import { CheckState, IAccount, IMilestone, Issue, PullRequest, PullRequestChecks, RepoAccessAndMergeMethods } from './interface'; +import { CheckState, IAccount, IMilestone, Issue, ITeam, PullRequest, PullRequestChecks, RepoAccessAndMergeMethods } from './interface'; import { IssueModel } from './issueModel'; import { LoggingOctokit } from './loggingOctokit'; import { PullRequestModel } from './pullRequestModel'; @@ -985,6 +987,85 @@ export class GitHubRepository implements vscode.Disposable { return ret; } + async getOrgTeamsCount(): Promise { + Logger.debug(`Fetch Teams Count - enter`, GitHubRepository.ID); + const { query, remote, schema } = await this.ensure(); + + try { + const result: { data: OrganizationTeamsCountResponse } = await query({ + query: schema.GetOrganizationTeamsCount, + variables: { + login: remote.owner + }, + }); + return result.data.organization.teams.totalCount; + } catch (e) { + Logger.debug(`Unable to fetch teams Count: ${e}`, GitHubRepository.ID); + if ( + e.graphQLErrors && + e.graphQLErrors.length > 0 && + e.graphQLErrors[0].type === 'INSUFFICIENT_SCOPES' + ) { + vscode.window.showWarningMessage( + `GitHub teams features will not work. ${e.graphQLErrors[0].message}`, + ); + } + return 0; + } + } + + async getTeams(): Promise { + Logger.debug(`Fetch Teams - enter`, GitHubRepository.ID); + const { query, remote, schema } = await this.ensure(); + + let after: string | null = null; + let hasNextPage = false; + const ret: ITeam[] = []; + + do { + try { + const result: { data: OrganizationTeamsResponse } = await query({ + query: schema.GetOrganizationTeams, + variables: { + login: remote.owner, + after: after, + repoName: remote.repositoryName, + }, + }); + + result.data.organization.teams.nodes.forEach(node => { + if (node.repositories.nodes.find(repo => repo.name === remote.repositoryName)) { + ret.push({ + avatarUrl: getAvatarWithEnterpriseFallback(node.avatarUrl, undefined, this.remote.authProviderId), + name: node.name, + url: node.url, + slug: node.slug, + id: node.id, + org: remote.owner + }); + } + }); + + hasNextPage = result.data.organization.teams.pageInfo.hasNextPage; + after = result.data.organization.teams.pageInfo.endCursor; + } catch (e) { + Logger.debug(`Unable to fetch teams: ${e}`, GitHubRepository.ID); + if ( + e.graphQLErrors && + e.graphQLErrors.length > 0 && + e.graphQLErrors[0].type === 'INSUFFICIENT_SCOPES' + ) { + vscode.window.showWarningMessage( + `GitHub teams features will not work. ${e.graphQLErrors[0].message}`, + ); + } + return ret; + } + } while (hasNextPage); + + return ret; + } + async getPullRequestParticipants(pullRequestNumber: number): Promise { Logger.debug(`Fetch participants from a Pull Request`, GitHubRepository.ID); const { query, remote, schema } = await this.ensure(); diff --git a/src/github/graphql.ts b/src/github/graphql.ts index 9bb0eb9f7b..5fd2bc0721 100644 --- a/src/github/graphql.ts +++ b/src/github/graphql.ts @@ -77,6 +77,19 @@ export interface Account { email: string; } +interface Team { + avatarUrl: string; + name: string; + url: string; + repositories: { + nodes: { + name: string + }[]; + }; + slug: string; + id: string; +} + export interface ReviewComment { __typename: string; id: string; @@ -225,6 +238,29 @@ export interface PendingReviewIdResponse { rateLimit: RateLimit; } +export interface GetReviewRequestsResponse { + repository: { + pullRequest: { + reviewRequests: { + nodes: { + requestedReviewer: { + // Shared properties between accounts and teams + avatarUrl: string; + url: string; + name: string; + // Account properties + login?: string; + email?: string; + // Team properties + slug?: string; + id?: string; + }; + }[]; + }; + }; + }; +}; + export interface PullRequestState { repository: { pullRequest: { @@ -271,6 +307,28 @@ export interface AssignableUsersResponse { rateLimit: RateLimit; } +export interface OrganizationTeamsCountResponse { + organization: { + teams: { + totalCount: number; + }; + }; +} + +export interface OrganizationTeamsResponse { + organization: { + teams: { + nodes: Team[]; + totalCount: number; + pageInfo: { + hasNextPage: boolean; + endCursor: string; + }; + }; + }; + rateLimit: RateLimit; +} + export interface PullRequestParticipantsResponse { repository: { pullRequest: { @@ -398,6 +456,7 @@ export interface ListBranchesResponse { } export interface RefRepository { + isInOrganization: boolean; owner: { login: string; }; diff --git a/src/github/interface.ts b/src/github/interface.ts index 07ae798906..0526a622b1 100644 --- a/src/github/interface.ts +++ b/src/github/interface.ts @@ -29,7 +29,7 @@ export enum PullRequestMergeability { } export interface ReviewState { - reviewer: IAccount; + reviewer: IAccount | ITeam; state: string; } @@ -41,6 +41,27 @@ export interface IAccount { email?: string; } +export interface ITeam { + name: string; + avatarUrl?: string; + url: string; + slug: string; + org: string; + id: string; +} + +export function reviewerId(reviewer: ITeam | IAccount): string { + return (reviewer as ITeam).id || (reviewer as IAccount).login; +} + +export function reviewerLabel(reviewer: ITeam | IAccount): string { + return (reviewer as ITeam).name || (reviewer as IAccount).login; +} + +export function isTeam(reviewer: ITeam | IAccount): reviewer is ITeam { + return (reviewer as ITeam).id !== undefined; +} + export interface ISuggestedReviewer extends IAccount { isAuthor: boolean; isCommenter: boolean; @@ -62,6 +83,7 @@ export interface MergePullRequest { export interface IRepository { cloneUrl: string; + isInOrganization: boolean; owner: string; name: string; } diff --git a/src/github/pullRequestModel.ts b/src/github/pullRequestModel.ts index f28dd84869..bbd813990b 100644 --- a/src/github/pullRequestModel.ts +++ b/src/github/pullRequestModel.ts @@ -27,6 +27,7 @@ import { DeleteReactionResponse, DeleteReviewResponse, EditCommentResponse, + GetReviewRequestsResponse, LatestReviewCommitResponse, LatestReviewsResponse, MarkPullRequestReadyForReviewResponse, @@ -48,6 +49,7 @@ import { IAccount, IRawFileChange, ISuggestedReviewer, + ITeam, MergeMethod, PullRequest, PullRequestChecks, @@ -58,7 +60,6 @@ import { IssueModel } from './issueModel'; import { convertRESTPullRequestToRawPullRequest, convertRESTReviewEvent, - convertRESTUserToAccount, getReactionGroup, insertNewCommitsSinceReview, parseGraphQLComment, @@ -232,14 +233,14 @@ export class PullRequestModel extends IssueModel implements IPullRe this.isRemoteHeadDeleted = item.isRemoteHeadDeleted; } if (item.head) { - this.head = new GitHubRef(item.head.ref, item.head.label, item.head.sha, item.head.repo.cloneUrl, item.head.repo.owner, item.head.repo.name); + this.head = new GitHubRef(item.head.ref, item.head.label, item.head.sha, item.head.repo.cloneUrl, item.head.repo.owner, item.head.repo.name, item.head.repo.isInOrganization); } if (item.isRemoteBaseDeleted != null) { this.isRemoteBaseDeleted = item.isRemoteBaseDeleted; } if (item.base) { - this.base = new GitHubRef(item.base.ref, item.base!.label, item.base!.sha, item.base!.repo.cloneUrl, item.base.repo.owner, item.base.repo.name); + this.base = new GitHubRef(item.base.ref, item.base!.label, item.base!.sha, item.base!.repo.cloneUrl, item.base.repo.owner, item.base.repo.name, item.base.repo.isInOrganization); } } @@ -744,29 +745,57 @@ export class PullRequestModel extends IssueModel implements IPullRe /** * Get existing requests to review. */ - async getReviewRequests(): Promise { + async getReviewRequests(): Promise<(IAccount | ITeam)[]> { const githubRepository = this.githubRepository; - const { remote, octokit } = await githubRepository.ensure(); - const result = await octokit.call(octokit.api.pulls.listRequestedReviewers, { - owner: remote.owner, - repo: remote.repositoryName, - pull_number: this.number, - }); + const { remote, query, schema } = await githubRepository.ensure(); - return result.data.users.map((user: any) => convertRESTUserToAccount(user, githubRepository)); + const { data } = await query({ + query: schema.GetReviewRequests, + variables: { + number: this.number, + owner: remote.owner, + name: remote.repositoryName + }, + }); + return data.repository.pullRequest.reviewRequests.nodes.map(reviewer => { + if (reviewer.requestedReviewer.login) { + const account: IAccount = { + login: reviewer.requestedReviewer.login, + url: reviewer.requestedReviewer.url, + avatarUrl: reviewer.requestedReviewer.avatarUrl, + email: reviewer.requestedReviewer.email, + name: reviewer.requestedReviewer.name + }; + return account; + } else { + const team: ITeam = { + name: reviewer.requestedReviewer.name, + url: reviewer.requestedReviewer.url, + avatarUrl: reviewer.requestedReviewer.avatarUrl, + id: reviewer.requestedReviewer.id!, + org: remote.owner, + slug: reviewer.requestedReviewer.slug! + }; + return team; + } + }); } /** * Add reviewers to a pull request * @param reviewers A list of GitHub logins */ - async requestReview(reviewers: string[]): Promise { - const { octokit, remote } = await this.githubRepository.ensure(); - await octokit.call(octokit.api.pulls.requestReviewers, { - owner: remote.owner, - repo: remote.repositoryName, - pull_number: this.number, - reviewers, + async requestReview(reviewers: string[], teamReviewers: string[]): Promise { + const { mutate, schema } = await this.githubRepository.ensure(); + await mutate({ + mutation: schema.AddReviewers, + variables: { + input: { + pullRequestId: this.graphNodeId, + teamIds: teamReviewers, + userIds: reviewers + }, + }, }); } @@ -774,13 +803,14 @@ export class PullRequestModel extends IssueModel implements IPullRe * Remove a review request that has not yet been completed * @param reviewer A GitHub Login */ - async deleteReviewRequest(reviewers: string[]): Promise { + async deleteReviewRequest(reviewers: string[], teamReviewers: string[]): Promise { const { octokit, remote } = await this.githubRepository.ensure(); await octokit.call(octokit.api.pulls.removeRequestedReviewers, { owner: remote.owner, repo: remote.repositoryName, pull_number: this.number, reviewers, + team_reviewers: teamReviewers }); } diff --git a/src/github/pullRequestOverview.ts b/src/github/pullRequestOverview.ts index 730c7653fc..690c168a08 100644 --- a/src/github/pullRequestOverview.ts +++ b/src/github/pullRequestOverview.ts @@ -16,9 +16,12 @@ import { GithubItemStateEnum, IAccount, IMilestone, + isTeam, ISuggestedReviewer, + ITeam, MergeMethod, MergeMethodsAvailability, + reviewerId, ReviewEvent, ReviewState, } from './interface'; @@ -42,6 +45,7 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel r.reviewer.login === currentUser.login); + const review = reviewers.find(r => reviewerId(r.reviewer) === currentUser.login); // There will always be a review. If not then the PR shouldn't have been or fetched/shown for the current user return review?.state; } @@ -169,7 +173,8 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel { const [ @@ -181,7 +186,8 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel { + suggestedReviewers: ISuggestedReviewer[] | undefined, refreshTeamReviewers: boolean + ): Promise<(vscode.QuickPickItem & { reviewer?: IAccount | ITeam })[]> { if (!suggestedReviewers) { return []; } const allAssignableUsers = await this._folderRepositoryManager.getAssignableUsers(); - const assignableUsers = allAssignableUsers[this._item.remote.remoteName] ?? []; + const teamReviewers = this._item.base.isInOrganization ? await this._folderRepositoryManager.getTeamReviewers(refreshTeamReviewers) : []; + const assignableUsers: (IAccount | ITeam)[] = teamReviewers[this._item.remote.remoteName] ?? []; + assignableUsers.push(...allAssignableUsers[this._item.remote.remoteName]); + // used to track logins that shouldn't be added to pick list // e.g. author, existing and already added reviewers const skipList: Set = new Set([ this._item.author.login, - ...this._existingReviewers.map(reviewer => reviewer.reviewer.login), + ...this._existingReviewers.map(reviewer => reviewerId(reviewer.reviewer)), ]); - const reviewers: (vscode.QuickPickItem & { reviewer?: IAccount })[] = []; + const reviewers: (vscode.QuickPickItem & { reviewer?: IAccount | ITeam })[] = []; // Start will all existing reviewers so they show at the top for (const reviewer of this._existingReviewers) { reviewers.push({ - label: reviewer.reviewer.login, + label: (reviewer.reviewer as IAccount).login ?? `${(reviewer.reviewer as ITeam).org}/${(reviewer.reviewer as ITeam).slug}`, description: reviewer.reviewer.name, reviewer: reviewer.reviewer, picked: true @@ -400,12 +410,12 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel): Promise { - const quickPick = vscode.window.createQuickPick(); + const quickPick = vscode.window.createQuickPick(); + // The quick-max is used to show the "update reviewers" button. If the number of teams is less than the quick-max, then they'll be automatically updated when the quick pick is opened. + const quickMaxTeamReviewers = 100; try { quickPick.busy = true; quickPick.canSelectMany = true; quickPick.matchOnDescription = true; quickPick.show(); - quickPick.items = await this.getReviewersQuickPickItems(this._item.suggestedReviewers); - quickPick.selectedItems = quickPick.items.filter(item => item.picked); + const updateItems = async (forceRefreshTeamReviewers: boolean) => { + quickPick.items = await this.getReviewersQuickPickItems(this._item.suggestedReviewers, forceRefreshTeamReviewers); + quickPick.selectedItems = quickPick.items.filter(item => item.picked); + }; + + updateItems(this._teamsCount <= quickMaxTeamReviewers); + if (this._item.base.isInOrganization && (this._teamsCount > quickMaxTeamReviewers)) { + quickPick.buttons = [{ iconPath: new vscode.ThemeIcon('organization'), tooltip: vscode.l10n.t('Show or refresh team reviewers') }]; + } + quickPick.onDidTriggerButton(async () => { + quickPick.busy = true; + await updateItems(true); + quickPick.busy = false; + }); quickPick.busy = false; const acceptPromise = asPromise(quickPick.onDidAccept).then(() => { - return quickPick.selectedItems.filter(item => item.reviewer) as (vscode.QuickPickItem & { reviewer: IAccount })[] | undefined; + return quickPick.selectedItems.filter(item => item.reviewer) as (vscode.QuickPickItem & { reviewer: IAccount | ITeam })[] | undefined; }); const hidePromise = asPromise(quickPick.onDidHide); - const allReviewers = await Promise.race<(vscode.QuickPickItem & { reviewer: IAccount })[] | void>([acceptPromise, hidePromise]); + const allReviewers = await Promise.race<(vscode.QuickPickItem & { reviewer: IAccount | ITeam })[] | void>([acceptPromise, hidePromise]); quickPick.busy = true; + if (allReviewers) { - const newReviewers = allReviewers.map(r => r.label); - const removedReviewers = this._existingReviewers.filter(existing => !newReviewers.find(newReviewer => newReviewer === existing.reviewer.login)); - await this._item.requestReview(newReviewers); - await this._item.deleteReviewRequest(removedReviewers.map(reviewer => reviewer.reviewer.login)); + const newUserReviewers: string[] = []; + const newTeamReviewers: string[] = []; + allReviewers.forEach(reviewer => { + const newReviewers = isTeam(reviewer.reviewer) ? newTeamReviewers : newUserReviewers; + newReviewers.push(reviewerId(reviewer.reviewer)); + }); + + const removedUserReviewers: string[] = []; + const removedTeamReviewers: string[] = []; + this._existingReviewers.forEach(existing => { + let newReviewers: string[] = isTeam(existing.reviewer) ? newTeamReviewers : newUserReviewers; + let removedReviewers: string[] = isTeam(existing.reviewer) ? removedTeamReviewers : removedUserReviewers; + if (!newReviewers.find(newTeamReviewer => newTeamReviewer === reviewerId(existing.reviewer))) { + removedReviewers.push(reviewerId(existing.reviewer)); + } + }); + + await this._item.requestReview(newUserReviewers, newTeamReviewers); + await this._item.deleteReviewRequest(removedUserReviewers, removedTeamReviewers); const addedReviewers: ReviewState[] = allReviewers.map(selected => { return { reviewer: selected.reviewer, @@ -824,7 +864,7 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel review.user.login === reviewer.reviewer.login, + reviewer => review.user.login === (reviewer.reviewer as IAccount).login, ); if (existingReviewer) { existingReviewer.state = review.state; @@ -889,8 +929,15 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel): void { - this._item.requestReview([message.args]).then(() => { - const reviewer = this._existingReviewers.find(reviewer => reviewer.reviewer.login === message.args); + const reviewer = this._existingReviewers.find(reviewer => reviewerId(reviewer.reviewer) === message.args); + const userReviewers: string[] = []; + const teamReviewers: string[] = []; + if (reviewer && isTeam(reviewer.reviewer)) { + teamReviewers.push(reviewer.reviewer.id); + } else if (reviewer && !isTeam(reviewer.reviewer)) { + userReviewers.push(reviewer.reviewer.login); + } + this._item.requestReview(userReviewers, teamReviewers).then(() => { if (reviewer) { reviewer.state = 'REQUESTED'; } diff --git a/src/github/queries.gql b/src/github/queries.gql index a4dfff92e9..e595802457 100644 --- a/src/github/queries.gql +++ b/src/github/queries.gql @@ -186,6 +186,7 @@ fragment PullRequestFragment on PullRequest { headRefName headRefOid headRepository { + isInOrganization owner { login } @@ -197,6 +198,7 @@ fragment PullRequestFragment on PullRequest { baseRefName baseRefOid baseRepository { + isInOrganization owner { login } @@ -326,6 +328,65 @@ query LatestReviews($owner: String!, $name: String!, $number: Int!) { } } +query GetOrganizationTeamsCount($login: String!) { + organization(login: $login) { + teams(first: 0, privacy: VISIBLE) { + totalCount + } + } +} + +query GetOrganizationTeams($login: String!, $after: String, $repoName: String!) { + organization(login: $login) { + teams(first: 100, after: $after, privacy: VISIBLE) { + nodes { + name + avatarUrl + url + repositories(first: 5, query: $repoName) { + nodes { + name + } + } + slug + id + } + totalCount + pageInfo { + hasNextPage + endCursor + } + } + } +} + +query GetReviewRequests($owner: String!, $name: String!, $number: Int!) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + reviewRequests(first: 100) { + nodes { + requestedReviewer { + ... on User { + login + avatarUrl + url + email + name + } + ... on Team { + name + avatarUrl + url + slug + id + } + } + } + } + } + } +} + fragment ReviewComment on PullRequestReviewComment { id databaseId @@ -703,6 +764,14 @@ mutation AddReviewThread($input: AddPullRequestReviewThreadInput!) { } } +mutation AddReviewers($input: RequestReviewsInput!) { + requestReviews(input: $input) { + pullRequest { + id + } + } +} + mutation EditComment($input: UpdatePullRequestReviewCommentInput!) { updatePullRequestReviewComment(input: $input) { pullRequestReviewComment { diff --git a/src/github/utils.ts b/src/github/utils.ts index 8f8ac579b5..cd62af26c0 100644 --- a/src/github/utils.ts +++ b/src/github/utils.ts @@ -30,9 +30,12 @@ import { IMilestone, Issue, ISuggestedReviewer, + ITeam, MergeMethod, PullRequest, PullRequestMergeability, + reviewerId, + reviewerLabel, ReviewState, User, } from './interface'; @@ -241,13 +244,14 @@ export function convertRESTUserToAccount( }; } -export function convertRESTHeadToIGitHubRef(head: OctokitCommon.PullsListResponseItemHead) { +export function convertRESTHeadToIGitHubRef(head: OctokitCommon.PullsListResponseItemHead): IGitHubRef { return { label: head.label, ref: head.ref, sha: head.sha, repo: { cloneUrl: head.repo.clone_url, + isInOrganization: !!head.repo.organization, owner: head.repo.owner!.login, name: head.repo.name }, @@ -533,6 +537,7 @@ function parseRef(refName: string, oid: string, repository?: GraphQL.RefReposito sha: oid, repo: { cloneUrl: repository.url, + isInOrganization: repository.isInOrganization, owner: repository.owner.login, name: refName }, @@ -704,6 +709,13 @@ export function loginComparator(a: IAccount, b: IAccount) { // sensitivity: 'accent' allows case insensitive comparison return a.login.localeCompare(b.login, 'en', { sensitivity: 'accent' }); } +/** + * Used for case insensitive sort by team name + */ +export function teamComparator(a: ITeam, b: ITeam) { + // sensitivity: 'accent' allows case insensitive comparison + return a.name.localeCompare(b.name, 'en', { sensitivity: 'accent' }); +} export function parseGraphQLReviewEvent( review: GraphQL.SubmittedReview, @@ -1004,7 +1016,7 @@ export function getRepositoryForFile(gitAPI: GitApiImpl, file: vscode.Uri): Repo * @param author The author of the pull request */ export function parseReviewers( - requestedReviewers: IAccount[], + requestedReviewers: (IAccount | ITeam)[], timelineEvents: Common.TimelineEvent[], author: IAccount, ): ReviewState[] { @@ -1027,13 +1039,13 @@ export function parseReviewers( } requestedReviewers.forEach(request => { - if (!seen.get(request.login)) { + if (!seen.get(reviewerId(request))) { reviewers.push({ reviewer: request, state: 'REQUESTED', }); } else { - const reviewer = reviewers.find(r => r.reviewer.login === request.login); + const reviewer = reviewers.find(r => reviewerId(r.reviewer) === reviewerId(request)); reviewer!.state = 'REQUESTED'; } }); @@ -1048,7 +1060,7 @@ export function parseReviewers( return -1; } - return a.reviewer.login.toLowerCase() < b.reviewer.login.toLowerCase() ? -1 : 1; + return reviewerLabel(a.reviewer).toLowerCase() < reviewerLabel(b.reviewer).toLowerCase() ? -1 : 1; }); return reviewers; diff --git a/src/test/builders/graphql/pullRequestBuilder.ts b/src/test/builders/graphql/pullRequestBuilder.ts index 46830523dc..49522f9d63 100644 --- a/src/test/builders/graphql/pullRequestBuilder.ts +++ b/src/test/builders/graphql/pullRequestBuilder.ts @@ -9,6 +9,7 @@ import { PullRequestResponse, Ref, RefRepository } from '../../../github/graphql import { RateLimitBuilder } from './rateLimitBuilder'; const RefRepositoryBuilder = createBuilderClass()({ + isInOrganization: { default: false }, owner: createLink()({ login: { default: 'me' }, }), diff --git a/webviews/components/merge.tsx b/webviews/components/merge.tsx index 7d5cb9d5ec..51c0340a6f 100644 --- a/webviews/components/merge.tsx +++ b/webviews/components/merge.tsx @@ -14,7 +14,7 @@ import React, { useState, } from 'react'; import { groupBy } from '../../src/common/utils'; -import { GithubItemStateEnum, MergeMethod, PullRequestMergeability } from '../../src/github/interface'; +import { GithubItemStateEnum, MergeMethod, PullRequestMergeability, reviewerId } from '../../src/github/interface'; import { PullRequest } from '../common/cache'; import PullRequestContext, { PRContext } from '../common/context'; import { Reviewer } from '../components/reviewer'; @@ -84,7 +84,7 @@ const InlineReviewers = ({ pr, isSimple }: { pr: PullRequest; isSimple: boolean
{' '} {pr.reviewers.map(state => ( - + ))}
) : null diff --git a/webviews/components/reviewer.tsx b/webviews/components/reviewer.tsx index ce8d1df75f..6f5b460425 100644 --- a/webviews/components/reviewer.tsx +++ b/webviews/components/reviewer.tsx @@ -3,7 +3,7 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ import React, { cloneElement, useContext } from 'react'; -import { ReviewState } from '../../src/github/interface'; +import { reviewerId, ReviewState } from '../../src/github/interface'; import PullRequestContext from '../common/context'; import { checkIcon, commentIcon, pendingIcon, requestChanges, syncIcon } from './icon'; import { AuthorLink, Avatar } from './user'; @@ -21,7 +21,7 @@ export function Reviewer(reviewState: ReviewState) {
{ state !== 'REQUESTED' ? - () : null } diff --git a/webviews/components/sidebar.tsx b/webviews/components/sidebar.tsx index 7ce2bcc5ae..9f05c0c5c8 100644 --- a/webviews/components/sidebar.tsx +++ b/webviews/components/sidebar.tsx @@ -5,7 +5,7 @@ import React, { useContext } from 'react'; import { gitHubLabelColor } from '../../src/common/utils'; -import { IMilestone } from '../../src/github/interface'; +import { IMilestone, reviewerId } from '../../src/github/interface'; import { PullRequest } from '../common/cache'; import PullRequestContext from '../common/context'; import { Label } from '../common/label'; @@ -43,7 +43,7 @@ export default function Sidebar({ reviewers, labels, hasWritePermission, isIssue
{reviewers && reviewers.length ? ( reviewers.map(state => ( - + )) ) : (
None yet
diff --git a/webviews/components/user.tsx b/webviews/components/user.tsx index 1ac5a516f5..98b6ab7156 100644 --- a/webviews/components/user.tsx +++ b/webviews/components/user.tsx @@ -4,10 +4,10 @@ *--------------------------------------------------------------------------------------------*/ import * as React from 'react'; -import { PullRequest } from '../common/cache'; +import { IAccount, ITeam, reviewerLabel } from '../../src/github/interface'; import { Icon } from './icon'; -export const Avatar = ({ for: author }: { for: Partial }) => ( +export const Avatar = ({ for: author }: { for: Partial }) => ( {author.avatarUrl ? ( @@ -17,7 +17,7 @@ export const Avatar = ({ for: author }: { for: Partial }) ); -export const AuthorLink = ({ for: author, text = author.login }: { for: PullRequest['author']; text?: string }) => ( +export const AuthorLink = ({ for: author, text = reviewerLabel(author) }: { for: IAccount | ITeam; text?: string }) => ( {text} From 649a2b76c61c3b8a4d7b3f602f8bae48f849a56f Mon Sep 17 00:00:00 2001 From: Alex Ross Date: Thu, 30 Mar 2023 11:57:43 +0200 Subject: [PATCH 2/4] Fix conflicts --- src/github/activityBarViewProvider.ts | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/github/activityBarViewProvider.ts b/src/github/activityBarViewProvider.ts index bffdcf55e8..30edbc1d9b 100644 --- a/src/github/activityBarViewProvider.ts +++ b/src/github/activityBarViewProvider.ts @@ -11,7 +11,7 @@ import { dispose, formatError } from '../common/utils'; import { getNonce, IRequestMessage, WebviewViewBase } from '../common/webview'; import { ReviewManager } from '../view/reviewManager'; import { FolderRepositoryManager } from './folderRepositoryManager'; -import { GithubItemStateEnum, reviewerId, ReviewEvent, ReviewState } from './interface'; +import { GithubItemStateEnum, isTeam, reviewerId, ReviewEvent, ReviewState } from './interface'; import { PullRequestModel } from './pullRequestModel'; import { getDefaultMergeMethod } from './pullRequestOverview'; import { PullRequestView } from './pullRequestOverviewCommon'; @@ -116,8 +116,15 @@ export class PullRequestViewProvider extends WebviewViewBase implements vscode.W } private reRequestReview(message: IRequestMessage): void { - this._item.requestReview([message.args]).then(() => { - const reviewer = this._existingReviewers.find(reviewer => reviewer.reviewer.login === message.args); + const reviewer = this._existingReviewers.find(reviewer => reviewerId(reviewer.reviewer) === message.args); + const userReviewers: string[] = []; + const teamReviewers: string[] = []; + if (reviewer && isTeam(reviewer.reviewer)) { + teamReviewers.push(reviewer.reviewer.id); + } else if (reviewer && !isTeam(reviewer.reviewer)) { + userReviewers.push(reviewer.reviewer.login); + } + this._item.requestReview(userReviewers, teamReviewers).then(() => { if (reviewer) { reviewer.state = 'REQUESTED'; } From a75ddac16b29a1178d5a86dc95fc73ebce01548d Mon Sep 17 00:00:00 2001 From: Alex Ross Date: Fri, 14 Apr 2023 14:38:53 +0200 Subject: [PATCH 3/4] Delay new scopes until teams are requested --- src/github/credentials.ts | 88 +++++++++++++------ src/github/folderRepositoryManager.ts | 44 +++++----- src/github/githubRepository.ts | 41 ++++++++- src/github/graphql.ts | 2 +- src/github/pullRequestModel.ts | 19 ++-- src/github/pullRequestOverview.ts | 22 +++-- src/github/queries.gql | 28 +++++- .../github/folderRepositoryManager.test.ts | 2 +- src/test/github/pullRequestGitHelper.test.ts | 2 +- src/test/github/pullRequestModel.test.ts | 7 +- src/test/github/pullRequestOverview.test.ts | 9 +- src/test/view/prsTree.test.ts | 4 +- src/test/view/reviewCommentController.test.ts | 1 + 13 files changed, 190 insertions(+), 79 deletions(-) diff --git a/src/github/credentials.ts b/src/github/credentials.ts index 66836e1aa9..23ecd1d376 100644 --- a/src/github/credentials.ts +++ b/src/github/credentials.ts @@ -28,7 +28,11 @@ const PROMPT_FOR_SIGN_IN_STORAGE_KEY = 'login'; // If the scopes are changed, make sure to notify all interested parties to make sure this won't cause problems. const SCOPES_OLD = ['read:user', 'user:email', 'repo']; -export const SCOPES = ['read:user', 'user:email', 'repo', 'workflow', 'read:org']; +const SCOPES = ['read:user', 'user:email', 'repo', 'workflow']; +const SCOPES_WITH_ADDITIONAL = ['read:user', 'user:email', 'repo', 'workflow', 'read:org']; + +const LAST_USED_SCOPES_GITHUB_KEY = 'githubPullRequest.lastUsedScopes'; +const LAST_USED_SCOPES_ENTERPRISE_KEY = 'githubPullRequest.lastUsedScopesEnterprise'; export interface GitHub { octokit: LoggingOctokit; @@ -44,11 +48,15 @@ export class CredentialStore implements vscode.Disposable { private _disposables: vscode.Disposable[]; private _onDidInitialize: vscode.EventEmitter = new vscode.EventEmitter(); public readonly onDidInitialize: vscode.Event = this._onDidInitialize.event; + private _scopes: string[]; + private _scopesEnterprise: string[]; private _onDidGetSession: vscode.EventEmitter = new vscode.EventEmitter(); public readonly onDidGetSession = this._onDidGetSession.event; - constructor(private readonly _telemetry: ITelemetry, private readonly _context: vscode.ExtensionContext) { + constructor(private readonly _telemetry: ITelemetry, private readonly context: vscode.ExtensionContext) { + this.setScopesFromState(); + this._disposables = []; this._disposables.push( vscode.authentication.onDidChangeSessions(async () => { @@ -69,7 +77,17 @@ export class CredentialStore implements vscode.Disposable { ); } - private async initialize(authProviderId: AuthProvider, getAuthSessionOptions: vscode.AuthenticationGetSessionOptions = {}): Promise { + private setScopesFromState() { + this._scopes = this.context.globalState.get(LAST_USED_SCOPES_GITHUB_KEY, SCOPES); + this._scopesEnterprise = this.context.globalState.get(LAST_USED_SCOPES_ENTERPRISE_KEY, SCOPES); + } + + private async saveScopesInState() { + await this.context.globalState.update(LAST_USED_SCOPES_GITHUB_KEY, this._scopes); + await this.context.globalState.update(LAST_USED_SCOPES_ENTERPRISE_KEY, this._scopesEnterprise); + } + + private async initialize(authProviderId: AuthProvider, getAuthSessionOptions: vscode.AuthenticationGetSessionOptions = {}, scopes: string[] = authProviderId === AuthProvider.github ? this._scopes : this._scopesEnterprise): Promise { Logger.debug(`Initializing GitHub${getGitHubSuffix(authProviderId)} authentication provider.`, 'Authentication'); if (authProviderId === AuthProvider['github-enterprise']) { if (!hasEnterpriseUri()) { @@ -84,8 +102,16 @@ export class CredentialStore implements vscode.Disposable { let session: vscode.AuthenticationSession | undefined = undefined; let isNew: boolean = false; + let usedScopes: string[] | undefined = SCOPES; try { - const result = await this.getSession(authProviderId, getAuthSessionOptions); + // Set scopes before getting the session to prevent new session events from using the old scopes. + if (authProviderId === AuthProvider.github) { + this._scopes = scopes; + } else { + this._scopesEnterprise = scopes; + } + const result = await this.getSession(authProviderId, getAuthSessionOptions, scopes); + usedScopes = result.scopes; session = result.session; isNew = result.isNew; } catch (e) { @@ -115,9 +141,12 @@ export class CredentialStore implements vscode.Disposable { } if (authProviderId === AuthProvider.github) { this._githubAPI = github; + this._scopes = usedScopes; } else { this._githubEnterpriseAPI = github; + this._scopesEnterprise = usedScopes; } + await this.saveScopesInState(); if (!(getAuthSessionOptions.createIfNone || getAuthSessionOptions.forceNewSession) || isNew) { this._onDidInitialize.fire(); @@ -127,15 +156,15 @@ export class CredentialStore implements vscode.Disposable { } } - private async doCreate(options: vscode.AuthenticationGetSessionOptions) { - await this.initialize(AuthProvider.github, options); + private async doCreate(options: vscode.AuthenticationGetSessionOptions, additionalScopes: boolean = false) { + await this.initialize(AuthProvider.github, options, additionalScopes ? SCOPES_WITH_ADDITIONAL : undefined); if (hasEnterpriseUri()) { - await this.initialize(AuthProvider['github-enterprise'], options); + await this.initialize(AuthProvider['github-enterprise'], options, additionalScopes ? SCOPES_WITH_ADDITIONAL : undefined); } } - public async create(options: vscode.AuthenticationGetSessionOptions = {}) { - return this.doCreate(options); + public async create(options: vscode.AuthenticationGetSessionOptions = {}, additionalScopes: boolean = false) { + return this.doCreate(options, additionalScopes); } public async recreate(reason?: string) { @@ -159,6 +188,13 @@ export class CredentialStore implements vscode.Disposable { return !!this._githubEnterpriseAPI; } + public isAuthenticatedWithAdditionalScopes(authProviderId: AuthProvider): boolean { + if (authProviderId === AuthProvider.github) { + return !!this._githubAPI && this._scopes.length == SCOPES_WITH_ADDITIONAL.length; + } + return !!this._githubEnterpriseAPI && this._scopes.length == SCOPES_WITH_ADDITIONAL.length; + } + public getHub(authProviderId: AuthProvider): GitHub | undefined { if (authProviderId === AuthProvider.github) { return this._githubAPI; @@ -166,6 +202,12 @@ export class CredentialStore implements vscode.Disposable { return this._githubEnterpriseAPI; } + public async getHubEnsureAdditionalScopes(authProviderId: AuthProvider): Promise { + const hasScopesAlready = this.isAuthenticatedWithAdditionalScopes(authProviderId); + await this.initialize(authProviderId, { createIfNone: !hasScopesAlready }, SCOPES_WITH_ADDITIONAL); + return this.getHub(authProviderId); + } + public async getHubOrLogin(authProviderId: AuthProvider): Promise { if (authProviderId === AuthProvider.github) { return this._githubAPI ?? (await this.login(authProviderId)); @@ -265,39 +307,35 @@ export class CredentialStore implements vscode.Disposable { }); } - private async getSession(authProviderId: AuthProvider, getAuthSessionOptions: vscode.AuthenticationGetSessionOptions): Promise<{ session: vscode.AuthenticationSession | undefined, isNew: boolean }> { - let session: vscode.AuthenticationSession | undefined = getAuthSessionOptions.forceNewSession ? undefined : await vscode.authentication.getSession(authProviderId, SCOPES, { silent: true }); + private async getSession(authProviderId: AuthProvider, getAuthSessionOptions: vscode.AuthenticationGetSessionOptions, scopes: string[]): Promise<{ session: vscode.AuthenticationSession | undefined, isNew: boolean, scopes: string[] }> { + let session: vscode.AuthenticationSession | undefined = getAuthSessionOptions.forceNewSession ? undefined : await vscode.authentication.getSession(authProviderId, scopes, { silent: true }); if (session) { - return { session, isNew: false }; + return { session, isNew: false, scopes }; } + let usedScopes: string[]; + if (getAuthSessionOptions.createIfNone && !getAuthSessionOptions.forceNewSession) { const silent = getAuthSessionOptions.silent; getAuthSessionOptions.createIfNone = false; getAuthSessionOptions.silent = true; session = await vscode.authentication.getSession(authProviderId, SCOPES_OLD, getAuthSessionOptions); + usedScopes = SCOPES_OLD; if (!session) { getAuthSessionOptions.createIfNone = true; getAuthSessionOptions.silent = silent; - session = await vscode.authentication.getSession(authProviderId, SCOPES, getAuthSessionOptions); + session = await vscode.authentication.getSession(authProviderId, scopes, getAuthSessionOptions); + usedScopes = scopes; } } else if (getAuthSessionOptions.forceNewSession) { - session = await vscode.authentication.getSession(authProviderId, SCOPES, getAuthSessionOptions); + session = await vscode.authentication.getSession(authProviderId, scopes, getAuthSessionOptions); + usedScopes = scopes; } else { session = await vscode.authentication.getSession(authProviderId, SCOPES_OLD, getAuthSessionOptions); + usedScopes = SCOPES_OLD; } - return { session, isNew: !!session }; - } - - private async getSessionOrLogin(authProviderId: AuthProvider): Promise { - const session = (await this.getSession(authProviderId, { createIfNone: true })).session!; - if (authProviderId === AuthProvider.github) { - this._sessionId = session.id; - } else { - this._enterpriseSessionId = session.id; - } - return session.accessToken; + return { session, isNew: !!session, scopes: usedScopes }; } private async createHub(token: string, authProviderId: AuthProvider): Promise { diff --git a/src/github/folderRepositoryManager.ts b/src/github/folderRepositoryManager.ts index 475c2226d2..c3c6be3c3c 100644 --- a/src/github/folderRepositoryManager.ts +++ b/src/github/folderRepositoryManager.ts @@ -25,7 +25,7 @@ import { git } from '../gitProviders/gitCommands'; import { UserCompletion, userMarkdown } from '../issues/util'; import { OctokitCommon } from './common'; import { CredentialStore } from './credentials'; -import { GitHubRepository, ItemsData, PullRequestData, ViewerPermission } from './githubRepository'; +import { GitHubRepository, ItemsData, PullRequestData, TeamReviewerRefreshKind, ViewerPermission } from './githubRepository'; import { PullRequestState, UserResponse } from './graphql'; import { IAccount, ILabel, IMilestone, IPullRequestsPagingOptions, ITeam, PRType, RepoAccessAndMergeMethods, User } from './interface'; import { IssueModel } from './issueModel'; @@ -862,8 +862,8 @@ export class FolderRepositoryManager implements vscode.Disposable { return this._fetchAssignableUsersPromise; } - async getTeamReviewers(clearCache?: boolean): Promise<{ [key: string]: ITeam[] }> { - if (clearCache) { + async getTeamReviewers(refreshKind: TeamReviewerRefreshKind): Promise<{ [key: string]: ITeam[] }> { + if (refreshKind === TeamReviewerRefreshKind.Force) { delete this._teamReviewers; } @@ -871,31 +871,33 @@ export class FolderRepositoryManager implements vscode.Disposable { return this._teamReviewers; } - const globalStateTeamReviewers = clearCache ? undefined : await this.getTeamReviewersFromGlobalState(); + const globalStateTeamReviewers = (refreshKind === TeamReviewerRefreshKind.Force) ? undefined : await this.getTeamReviewersFromGlobalState(); if (globalStateTeamReviewers) { return globalStateTeamReviewers || {}; } if (!this._fetchTeamReviewersPromise) { const cache: { [key: string]: ITeam[] } = {}; - return (this._fetchTeamReviewersPromise = new Promise(resolve => { - const promises = this._githubRepositories.map(async githubRepository => { - const data = await githubRepository.getTeams(); - cache[githubRepository.remote.remoteName] = data.sort(teamComparator); - return; - }); + return (this._fetchTeamReviewersPromise = new Promise(async (resolve) => { + // Go through one github repo at a time so that we don't make overlapping auth calls + for (const githubRepository of this._githubRepositories) { + try { + const data = await githubRepository.getTeams(refreshKind); + cache[githubRepository.remote.remoteName] = data.sort(teamComparator); + } catch (e) { + // ignore errors from getTeams + } + } - Promise.all(promises).then(() => { - this._teamReviewers = cache; - this._fetchTeamReviewersPromise = undefined; - const teamReviewersCacheLocation = vscode.Uri.joinPath(this.context.globalStorageUri, 'teamReviewers'); - Promise.all(this._githubRepositories.map(async (repo) => { - const key = `${repo.remote.owner}/${repo.remote.repositoryName}.json`; - const repoSpecificFile = vscode.Uri.joinPath(teamReviewersCacheLocation, key); - await vscode.workspace.fs.writeFile(repoSpecificFile, new TextEncoder().encode(JSON.stringify(cache[repo.remote.remoteName]))); - })); - resolve(cache); - }); + this._teamReviewers = cache; + this._fetchTeamReviewersPromise = undefined; + const teamReviewersCacheLocation = vscode.Uri.joinPath(this.context.globalStorageUri, 'teamReviewers'); + Promise.all(this._githubRepositories.map(async (repo) => { + const key = `${repo.remote.owner}/${repo.remote.repositoryName}.json`; + const repoSpecificFile = vscode.Uri.joinPath(teamReviewersCacheLocation, key); + await vscode.workspace.fs.writeFile(repoSpecificFile, new TextEncoder().encode(JSON.stringify(cache[repo.remote.remoteName]))); + })); + resolve(cache); })); } diff --git a/src/github/githubRepository.ts b/src/github/githubRepository.ts index 18f13a7624..b9a26d244d 100644 --- a/src/github/githubRepository.ts +++ b/src/github/githubRepository.ts @@ -81,6 +81,12 @@ export enum ViewerPermission { Write = 'WRITE', } +export enum TeamReviewerRefreshKind { + None, + Try, + Force +} + export interface ForkDetails { isFork: boolean; parent: { @@ -282,6 +288,23 @@ export class GitHubRepository implements vscode.Disposable { return this; } + async ensureAdditionalScopes(): Promise { + this._initialized = true; + + if (!this._credentialStore.isAuthenticated(this.remote.authProviderId)) { + // We need auth now. (ex., a PR is already checked out) + // We can no longer wait until later for login to be done + await this._credentialStore.create(undefined, true); + if (!this._credentialStore.isAuthenticated(this.remote.authProviderId)) { + this._hub = await this._credentialStore.showSignInNotification(this.remote.authProviderId); + } + } else { + this._hub = await this._credentialStore.getHubEnsureAdditionalScopes(this.remote.authProviderId); + } + + return this; + } + async getDefaultBranch(): Promise { const overrideSetting = getOverrideBranch(); if (overrideSetting) { @@ -753,7 +776,7 @@ export class GitHubRepository implements vscode.Disposable { if (model) { model.update(pullRequest); } else { - model = new PullRequestModel(this._telemetry, this, this.remote, pullRequest); + model = new PullRequestModel(this._credentialStore, this._telemetry, this, this.remote, pullRequest); model.onDidInvalidate(() => this.getPullRequest(pullRequest.number)); this._pullRequestModels.set(pullRequest.number, model); this._onDidAddPullRequest.fire(model); @@ -989,7 +1012,11 @@ export class GitHubRepository implements vscode.Disposable { async getOrgTeamsCount(): Promise { Logger.debug(`Fetch Teams Count - enter`, GitHubRepository.ID); - const { query, remote, schema } = await this.ensure(); + if (!this._credentialStore.isAuthenticatedWithAdditionalScopes(this.remote.authProviderId)) { + return 0; + } + + const { query, remote, schema } = await this.ensureAdditionalScopes(); try { const result: { data: OrganizationTeamsCountResponse } = await query({ @@ -1014,9 +1041,14 @@ export class GitHubRepository implements vscode.Disposable { } } - async getTeams(): Promise { + async getTeams(refreshKind: TeamReviewerRefreshKind): Promise { Logger.debug(`Fetch Teams - enter`, GitHubRepository.ID); - const { query, remote, schema } = await this.ensure(); + if ((refreshKind === TeamReviewerRefreshKind.None) || (refreshKind === TeamReviewerRefreshKind.Try && !this._credentialStore.isAuthenticatedWithAdditionalScopes(this.remote.authProviderId))) { + Logger.debug(`Fetch Teams - exit without fetching teams`, GitHubRepository.ID); + return []; + } + + const { query, remote, schema } = await this.ensureAdditionalScopes(); let after: string | null = null; let hasNextPage = false; @@ -1063,6 +1095,7 @@ export class GitHubRepository implements vscode.Disposable { } } while (hasNextPage); + Logger.debug(`Fetch Teams - exit`, GitHubRepository.ID); return ret; } diff --git a/src/github/graphql.ts b/src/github/graphql.ts index 5fd2bc0721..ee13fe0a6a 100644 --- a/src/github/graphql.ts +++ b/src/github/graphql.ts @@ -254,7 +254,7 @@ export interface GetReviewRequestsResponse { // Team properties slug?: string; id?: string; - }; + } | null; }[]; }; }; diff --git a/src/github/pullRequestModel.ts b/src/github/pullRequestModel.ts index bbd813990b..6e4c18bb83 100644 --- a/src/github/pullRequestModel.ts +++ b/src/github/pullRequestModel.ts @@ -18,6 +18,7 @@ import { ReviewEvent as CommonReviewEvent, EventType, TimelineEvent } from '../c import { resolvePath, toPRUri, toReviewUri } from '../common/uri'; import { formatError } from '../common/utils'; import { OctokitCommon } from './common'; +import { CredentialStore } from './credentials'; import { FolderRepositoryManager } from './folderRepositoryManager'; import { GitHubRepository } from './githubRepository'; import { @@ -139,6 +140,7 @@ export class PullRequestModel extends IssueModel implements IPullRe _telemetry: ITelemetry; constructor( + private readonly credentialStore: CredentialStore, telemetry: ITelemetry, githubRepository: GitHubRepository, remote: Remote, @@ -750,15 +752,17 @@ export class PullRequestModel extends IssueModel implements IPullRe const { remote, query, schema } = await githubRepository.ensure(); const { data } = await query({ - query: schema.GetReviewRequests, + query: this.credentialStore.isAuthenticatedWithAdditionalScopes(githubRepository.remote.authProviderId) ? schema.GetReviewRequestsAdditionalScopes : schema.GetReviewRequests, variables: { number: this.number, owner: remote.owner, name: remote.repositoryName }, }); - return data.repository.pullRequest.reviewRequests.nodes.map(reviewer => { - if (reviewer.requestedReviewer.login) { + + const reviewers: (IAccount | ITeam)[] = []; + for (const reviewer of data.repository.pullRequest.reviewRequests.nodes) { + if (reviewer.requestedReviewer?.login) { const account: IAccount = { login: reviewer.requestedReviewer.login, url: reviewer.requestedReviewer.url, @@ -766,8 +770,8 @@ export class PullRequestModel extends IssueModel implements IPullRe email: reviewer.requestedReviewer.email, name: reviewer.requestedReviewer.name }; - return account; - } else { + reviewers.push(account); + } else if (reviewer.requestedReviewer) { const team: ITeam = { name: reviewer.requestedReviewer.name, url: reviewer.requestedReviewer.url, @@ -776,9 +780,10 @@ export class PullRequestModel extends IssueModel implements IPullRe org: remote.owner, slug: reviewer.requestedReviewer.slug! }; - return team; + reviewers.push(team); } - }); + } + return reviewers; } /** diff --git a/src/github/pullRequestOverview.ts b/src/github/pullRequestOverview.ts index 690c168a08..30c44a735e 100644 --- a/src/github/pullRequestOverview.ts +++ b/src/github/pullRequestOverview.ts @@ -12,6 +12,7 @@ import { ReviewEvent as CommonReviewEvent } from '../common/timelineEvent'; import { asPromise, dispose, formatError } from '../common/utils'; import { IRequestMessage, PULL_REQUEST_OVERVIEW_VIEW_TYPE } from '../common/webview'; import { FolderRepositoryManager } from './folderRepositoryManager'; +import { TeamReviewerRefreshKind } from './githubRepository'; import { GithubItemStateEnum, IAccount, @@ -353,14 +354,14 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel { if (!suggestedReviewers) { return []; } const allAssignableUsers = await this._folderRepositoryManager.getAssignableUsers(); - const teamReviewers = this._item.base.isInOrganization ? await this._folderRepositoryManager.getTeamReviewers(refreshTeamReviewers) : []; + const teamReviewers = this._item.base.isInOrganization ? await this._folderRepositoryManager.getTeamReviewers(refreshKind) : []; const assignableUsers: (IAccount | ITeam)[] = teamReviewers[this._item.remote.remoteName] ?? []; assignableUsers.push(...allAssignableUsers[this._item.remote.remoteName]); @@ -522,19 +523,22 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel { - quickPick.items = await this.getReviewersQuickPickItems(this._item.suggestedReviewers, forceRefreshTeamReviewers); + const updateItems = async (refreshKind: TeamReviewerRefreshKind) => { + quickPick.items = await this.getReviewersQuickPickItems(this._item.suggestedReviewers, refreshKind); quickPick.selectedItems = quickPick.items.filter(item => item.picked); }; - updateItems(this._teamsCount <= quickMaxTeamReviewers); - if (this._item.base.isInOrganization && (this._teamsCount > quickMaxTeamReviewers)) { + await updateItems((this._teamsCount !== 0 && this._teamsCount <= quickMaxTeamReviewers) ? TeamReviewerRefreshKind.Try : TeamReviewerRefreshKind.None); + if (this._item.base.isInOrganization) { quickPick.buttons = [{ iconPath: new vscode.ThemeIcon('organization'), tooltip: vscode.l10n.t('Show or refresh team reviewers') }]; } - quickPick.onDidTriggerButton(async () => { + quickPick.onDidTriggerButton(() => { quickPick.busy = true; - await updateItems(true); - quickPick.busy = false; + quickPick.ignoreFocusOut = true; + updateItems(TeamReviewerRefreshKind.Force).then(() => { + quickPick.ignoreFocusOut = false; + quickPick.busy = false; + }); }); quickPick.busy = false; const acceptPromise = asPromise(quickPick.onDidAccept).then(() => { diff --git a/src/github/queries.gql b/src/github/queries.gql index e595802457..00f70ba4cb 100644 --- a/src/github/queries.gql +++ b/src/github/queries.gql @@ -358,9 +358,15 @@ query GetOrganizationTeams($login: String!, $after: String, $repoName: String!) } } } + rateLimit { + limit + cost + remaining + resetAt + } } -query GetReviewRequests($owner: String!, $name: String!, $number: Int!) { +query GetReviewRequestsAdditionalScopes($owner: String!, $name: String!, $number: Int!) { repository(owner: $owner, name: $name) { pullRequest(number: $number) { reviewRequests(first: 100) { @@ -387,6 +393,26 @@ query GetReviewRequests($owner: String!, $name: String!, $number: Int!) { } } +query GetReviewRequests($owner: String!, $name: String!, $number: Int!) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + reviewRequests(first: 100) { + nodes { + requestedReviewer { + ... on User { + login + avatarUrl + url + email + name + } + } + } + } + } + } +} + fragment ReviewComment on PullRequestReviewComment { id databaseId diff --git a/src/test/github/folderRepositoryManager.test.ts b/src/test/github/folderRepositoryManager.test.ts index 5a4e4e007d..244489e72e 100644 --- a/src/test/github/folderRepositoryManager.test.ts +++ b/src/test/github/folderRepositoryManager.test.ts @@ -55,7 +55,7 @@ describe('PullRequestManager', function () { const rootUri = Uri.file('C:\\users\\test\\repo'); const repository = new GitHubRepository(remote, rootUri, manager.credentialStore, telemetry); const prItem = convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().build(), repository); - const pr = new PullRequestModel(telemetry, repository, remote, prItem); + const pr = new PullRequestModel(manager.credentialStore, telemetry, repository, remote, prItem); manager.activePullRequest = pr; assert(changeFired.called); diff --git a/src/test/github/pullRequestGitHelper.test.ts b/src/test/github/pullRequestGitHelper.test.ts index 193da32737..8779e56c9e 100644 --- a/src/test/github/pullRequestGitHelper.test.ts +++ b/src/test/github/pullRequestGitHelper.test.ts @@ -68,7 +68,7 @@ describe('PullRequestGitHelper', function () { repository.expectFetch('you', 'my-branch:pr/me/100', 1); repository.expectPull(true); - const pullRequest = new PullRequestModel(telemetry, gitHubRepository, remote, prItem); + const pullRequest = new PullRequestModel(credentialStore, telemetry, gitHubRepository, remote, prItem); if (!pullRequest.isResolved()) { assert(false, 'pull request head not resolved successfully'); diff --git a/src/test/github/pullRequestModel.test.ts b/src/test/github/pullRequestModel.test.ts index b5cea79c9d..5b93ac74a4 100644 --- a/src/test/github/pullRequestModel.test.ts +++ b/src/test/github/pullRequestModel.test.ts @@ -77,21 +77,21 @@ describe('PullRequestModel', function () { it('should return `state` properly as `open`', function () { const pr = new PullRequestBuilder().state('open').build(); - const open = new PullRequestModel(telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr, repo)); + const open = new PullRequestModel(credentials, telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr, repo)); assert.strictEqual(open.state, GithubItemStateEnum.Open); }); it('should return `state` properly as `closed`', function () { const pr = new PullRequestBuilder().state('closed').build(); - const open = new PullRequestModel(telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr, repo)); + const open = new PullRequestModel(credentials, telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr, repo)); assert.strictEqual(open.state, GithubItemStateEnum.Closed); }); it('should return `state` properly as `merged`', function () { const pr = new PullRequestBuilder().merged(true).state('closed').build(); - const open = new PullRequestModel(telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr, repo)); + const open = new PullRequestModel(credentials, telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr, repo)); assert.strictEqual(open.state, GithubItemStateEnum.Merged); }); @@ -100,6 +100,7 @@ describe('PullRequestModel', function () { it('should update the cache when then cache is initialized', async function () { const pr = new PullRequestBuilder().build(); const model = new PullRequestModel( + credentials, telemetry, repo, remote, diff --git a/src/test/github/pullRequestOverview.test.ts b/src/test/github/pullRequestOverview.test.ts index 83c98ed3a5..5337bbf1f7 100644 --- a/src/test/github/pullRequestOverview.test.ts +++ b/src/test/github/pullRequestOverview.test.ts @@ -33,6 +33,7 @@ describe('PullRequestOverview', function () { let remote: GitHubRemote; let repo: MockGitHubRepository; let telemetry: MockTelemetry; + let credentialStore: CredentialStore; beforeEach(async function () { sinon = createSandbox(); @@ -41,7 +42,7 @@ describe('PullRequestOverview', function () { const repository = new MockRepository(); telemetry = new MockTelemetry(); - const credentialStore = new CredentialStore(telemetry, context); + credentialStore = new CredentialStore(telemetry, context); pullRequestManager = new FolderRepositoryManager(context, repository, telemetry, new GitApiImpl(), credentialStore); const url = 'https://github.com/aaa/bbb'; @@ -73,7 +74,7 @@ describe('PullRequestOverview', function () { }); const prItem = convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(1000).build(), repo); - const prModel = new PullRequestModel(telemetry, repo, remote, prItem); + const prModel = new PullRequestModel(credentialStore, telemetry, repo, remote, prItem); await PullRequestOverviewPanel.createOrShow(EXTENSION_URI, pullRequestManager, prModel); @@ -106,7 +107,7 @@ describe('PullRequestOverview', function () { }); const prItem0 = convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(1000).build(), repo); - const prModel0 = new PullRequestModel(telemetry, repo, remote, prItem0); + const prModel0 = new PullRequestModel(credentialStore, telemetry, repo, remote, prItem0); const resolveStub = sinon.stub(pullRequestManager, 'resolvePullRequest').resolves(prModel0); sinon.stub(prModel0, 'getReviewRequests').resolves([]); sinon.stub(prModel0, 'getTimelineEvents').resolves([]); @@ -119,7 +120,7 @@ describe('PullRequestOverview', function () { assert.strictEqual(panel0!.getCurrentTitle(), 'Pull Request #1000'); const prItem1 = convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(2000).build(), repo); - const prModel1 = new PullRequestModel(telemetry, repo, remote, prItem1); + const prModel1 = new PullRequestModel(credentialStore, telemetry, repo, remote, prItem1); resolveStub.resolves(prModel1); sinon.stub(prModel1, 'getReviewRequests').resolves([]); sinon.stub(prModel1, 'getTimelineEvents').resolves([]); diff --git a/src/test/view/prsTree.test.ts b/src/test/view/prsTree.test.ts index 8f1f124db3..898fe1611a 100644 --- a/src/test/view/prsTree.test.ts +++ b/src/test/view/prsTree.test.ts @@ -148,7 +148,7 @@ describe('GitHub Pull Requests view', function () { }); }).pullRequest; const prItem0 = parseGraphQLPullRequest(pr0.repository.pullRequest, gitHubRepository); - const pullRequest0 = new PullRequestModel(telemetry, gitHubRepository, remote, prItem0); + const pullRequest0 = new PullRequestModel(credentialStore, telemetry, gitHubRepository, remote, prItem0); const pr1 = gitHubRepository.addGraphQLPullRequest(builder => { builder.pullRequest(pr => { @@ -164,7 +164,7 @@ describe('GitHub Pull Requests view', function () { }); }).pullRequest; const prItem1 = parseGraphQLPullRequest(pr1.repository.pullRequest, gitHubRepository); - const pullRequest1 = new PullRequestModel(telemetry, gitHubRepository, remote, prItem1); + const pullRequest1 = new PullRequestModel(credentialStore, telemetry, gitHubRepository, remote, prItem1); const repository = new MockRepository(); await repository.addRemote(remote.remoteName, remote.url); diff --git a/src/test/view/reviewCommentController.test.ts b/src/test/view/reviewCommentController.test.ts index 5d77f3b9f7..0ee631e466 100644 --- a/src/test/view/reviewCommentController.test.ts +++ b/src/test/view/reviewCommentController.test.ts @@ -84,6 +84,7 @@ describe('ReviewCommentController', function () { const pr = new PullRequestBuilder().build(); githubRepo = new MockGitHubRepository(remote, credentialStore, telemetry, sinon); activePullRequest = new PullRequestModel( + credentialStore, telemetry, githubRepo, remote, From 22e02ec0f697999928dab334588f18453c71851e Mon Sep 17 00:00:00 2001 From: Alex Ross Date: Fri, 14 Apr 2023 14:43:16 +0200 Subject: [PATCH 4/4] Feedback --- src/github/interface.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/github/interface.ts b/src/github/interface.ts index 0526a622b1..afaba0b185 100644 --- a/src/github/interface.ts +++ b/src/github/interface.ts @@ -51,15 +51,15 @@ export interface ITeam { } export function reviewerId(reviewer: ITeam | IAccount): string { - return (reviewer as ITeam).id || (reviewer as IAccount).login; + return isTeam(reviewer) ? reviewer.id : reviewer.login; } export function reviewerLabel(reviewer: ITeam | IAccount): string { - return (reviewer as ITeam).name || (reviewer as IAccount).login; + return isTeam(reviewer) ? reviewer.name : reviewer.login; } export function isTeam(reviewer: ITeam | IAccount): reviewer is ITeam { - return (reviewer as ITeam).id !== undefined; + return 'id' in reviewer; } export interface ISuggestedReviewer extends IAccount {