Skip to content

Add Team Reviewers to Pull Request #4512

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alexr00 merged 5 commits into from
Apr 14, 2023
Merged

Add Team Reviewers to Pull Request #4512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
35525bd
Add Team Reviewers to Pull Request
alexr00 Feb 10, 2023
649a2b7
Fix conflicts
alexr00 Mar 30, 2023
a75ddac
Delay new scopes until teams are requested
alexr00 Apr 14, 2023
22e02ec
Feedback
alexr00 Apr 14, 2023
bf0a0bc
Merge branch 'main' into alexr00/issue1126
alexr00 Apr 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/common/githubRef.ts
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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`;
Copy link
Contributor

@kabel kabel Feb 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There could be name collisions with enterprise instances.

Copy link
Member Author

@alexr00 alexr00 Apr 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could only get a collision if there are two unrelated enterprises with the same owner + repo name combination. Is that correct?

Copy link
Contributor

@kabel kabel Apr 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the cache key makes no distinction between repo origins, yes, two unrelated enterprises might collide. But more importantly and likely would be a collision between github.com repos and any enterprise.

Example:

  • github.com/microsoft/vscode
  • github.contoso.net/microsoft/vscode

While it may be unlikely that these examples exist and represent distinct teams, they are indeed two different sources of truth for information when querying/caching on the context microsoft/vscode

Copy link
Member Author

@alexr00 alexr00 Apr 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks. I'll update the key in another PR.

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