Skip to content

Commit

Permalink
Add Team Reviewers to Pull Request (#4512)
Browse files Browse the repository at this point in the history 
* Add Team Reviewers to Pull Request
Fixes #1126

* Fix conflicts

* Delay new scopes until teams are requested

* Feedback
  • Loading branch information
@alexr00
alexr00 authored Apr 14, 2023
1 parent 09c5230 commit ec75439
Show file tree
Hide file tree
Showing 24 changed files with 646 additions and 102 deletions.
2 changes: 1 addition & 1 deletion src/common/githubRef.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
2 changes: 1 addition & 1 deletion src/env/browser/ssh.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export const sshParse = (url: string): Config | undefined => {
* @param {ConfigResolver?} resolveConfig ssh config resolver (default: from ~/.ssh/config)
* @returns {Config}
*/
export const resolve = (url: string, resolveConfig = Resolvers.current) => {
export const resolve = (url: string, resolveConfig = Resolvers.current) => {
const config = sshParse(url);
return config && resolveConfig(config);
};
Expand Down
15 changes: 11 additions & 4 deletions src/github/activityBarViewProvider.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, isTeam, reviewerId, ReviewEvent, ReviewState } from './interface';
import { PullRequestModel } from './pullRequestModel';
import { getDefaultMergeMethod } from './pullRequestOverview';
import { PullRequestView } from './pullRequestOverviewCommon';
Expand Down Expand Up @@ -116,8 +116,15 @@ export class PullRequestViewProvider extends WebviewViewBase implements vscode.W
}

private reRequestReview(message: IRequestMessage<string>): 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';
}
Expand Down Expand Up @@ -266,7 +273,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;
Expand Down
88 changes: 63 additions & 25 deletions src/github/credentials.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'];
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;
Expand All @@ -44,11 +48,15 @@ export class CredentialStore implements vscode.Disposable {
private _disposables: vscode.Disposable[];
private _onDidInitialize: vscode.EventEmitter<void> = new vscode.EventEmitter();
public readonly onDidInitialize: vscode.Event<void> = this._onDidInitialize.event;
private _scopes: string[];
private _scopesEnterprise: string[];

private _onDidGetSession: vscode.EventEmitter<void> = 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 () => {
Expand All @@ -69,7 +77,17 @@ export class CredentialStore implements vscode.Disposable {
);
}

private async initialize(authProviderId: AuthProvider, getAuthSessionOptions: vscode.AuthenticationGetSessionOptions = {}): Promise<void> {
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<void> {
Logger.debug(`Initializing GitHub${getGitHubSuffix(authProviderId)} authentication provider.`, 'Authentication');
if (authProviderId === AuthProvider['github-enterprise']) {
if (!hasEnterpriseUri()) {
Expand All @@ -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) {
Expand Down Expand Up @@ -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();
Expand All @@ -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) {
Expand All @@ -159,13 +188,26 @@ 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;
}
return this._githubEnterpriseAPI;
}

public async getHubEnsureAdditionalScopes(authProviderId: AuthProvider): Promise<GitHub | undefined> {
const hasScopesAlready = this.isAuthenticatedWithAdditionalScopes(authProviderId);
await this.initialize(authProviderId, { createIfNone: !hasScopesAlready }, SCOPES_WITH_ADDITIONAL);
return this.getHub(authProviderId);
}

public async getHubOrLogin(authProviderId: AuthProvider): Promise<GitHub | undefined> {
if (authProviderId === AuthProvider.github) {
return this._githubAPI ?? (await this.login(authProviderId));
Expand Down Expand Up @@ -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<string> {
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<GitHub> {
Expand Down
93 changes: 90 additions & 3 deletions src/github/folderRepositoryManager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ 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, 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';
Expand All @@ -39,6 +39,7 @@ import {
getRelatedUsersFromTimelineEvents,
loginComparator,
parseGraphQLUser,
teamComparator,
variableSubstitution,
} from './utils';

Expand Down Expand Up @@ -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<string, PageInformation> = new Map<string, PageInformation>();
Expand Down Expand Up @@ -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);
Expand All @@ -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 => {
Expand Down Expand Up @@ -821,6 +862,52 @@ export class FolderRepositoryManager implements vscode.Disposable {
return this._fetchAssignableUsersPromise;
}

async getTeamReviewers(refreshKind: TeamReviewerRefreshKind): Promise<{ [key: string]: ITeam[] }> {
if (refreshKind === TeamReviewerRefreshKind.Force) {
delete this._teamReviewers;
}

if (this._teamReviewers) {
return this._teamReviewers;
}

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(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
}
}

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<number> {
return repository.getOrgTeamsCount();
}

async getPullRequestParticipants(githubRepository: GitHubRepository, pullRequestNumber: number): Promise<{ participants: IAccount[], viewer: IAccount }> {
return {
participants: await githubRepository.getPullRequestParticipants(pullRequestNumber),
Expand Down
Loading

0 comments on commit ec75439

Please sign in to comment.