From db65e9c893eba8e1daa96fa76afccbf446d2acf3 Mon Sep 17 00:00:00 2001 From: Kevin Abel Date: Fri, 3 Feb 2023 10:00:47 -0600 Subject: [PATCH 1/4] Add configurable avatar fallback for GitHub Enterprise In GitHub Enterprise, the avatarUrl endpoints that the APIs return require authentication in standard configuration. There is currently no way to do token authentication to these endpoints, so a fallback is necessary to render an image. The interface was previously coded to handle fallback to the Gravatar service (just like GitHub Desktop [did](https://github.com/desktop/desktop/pull/3513)), but this behavior was disabled in 13ab0a7653008e8433cbac0136d4d8fb97e1328c and 6769dd4cba8b3b6bfef20c94ef28918103634680 because of end-user preference for making this feature configurable. A new setting has been added * `githubPullRequests.defaultGravatarsStyle` is an `enum` of the Gravatar supported fallback styles plus an internal value of `none` to disable the integration These settings provide a richer user experience for GitHub Enterprise pull requests, allowing users to opt-in/out on avatar rendering via a third-party service with multiple options for the default fallback image style. For consistency in multiple interface areas where `` is rendered, the graphQL queryies (and matching typescript interfaces) have been updated to include the necessary `email` property for integration with Gravatar. REST provided actors are missing email. If missing email and using GitHub Enterprise, make another REST API call to the user endpoiint to get email. This allows a fallback avatar to be generated. This fixes GitHub Enterprise avatars not being rendered in the tree views. --- package.json | 24 +++++ package.nls.json | 8 ++ src/github/folderRepositoryManager.ts | 4 +- src/github/githubRepository.ts | 34 +++--- src/github/graphql.ts | 35 ++++-- src/github/issueModel.ts | 4 +- src/github/issueOverview.ts | 1 + src/github/pullRequestModel.ts | 7 +- src/github/pullRequestOverview.ts | 1 + src/github/queries.gql | 39 +++++++ src/github/utils.ts | 102 +++++++++++------- .../github/folderRepositoryManager.test.ts | 4 +- src/test/github/pullRequestGitHelper.test.ts | 2 +- src/test/github/pullRequestModel.test.ts | 16 +-- src/test/github/pullRequestOverview.test.ts | 6 +- src/test/view/reviewCommentController.test.ts | 2 +- 16 files changed, 209 insertions(+), 80 deletions(-) diff --git a/package.json b/package.json index 875ef36e64..f69095f71a 100644 --- a/package.json +++ b/package.json @@ -514,6 +514,30 @@ "type": "boolean", "default": false, "description": "%githubPullRequests.showPullRequestNumberInTree.description%" + }, + "githubPullRequests.defaultGravatarsStyle": { + "type": "string", + "default": "invertocat", + "enum": [ + "none", + "identicon", + "mp", + "monsterid", + "retro", + "robohash", + "wavatar" + ], + "enumDescriptions": [ + "%githubPullRequests.defaultGravatarsStyle.none%", + "%githubPullRequests.defaultGravatarsStyle.identicon%", + "%githubPullRequests.defaultGravatarsStyle.mp%", + "%githubPullRequests.defaultGravatarsStyle.monsterid%", + "%githubPullRequests.defaultGravatarsStyle.retro%", + "%githubPullRequests.defaultGravatarsStyle.robohash%", + "%githubPullRequests.defaultGravatarsStyle.wavatar%" + ], + "description": "%githubPullRequests.defaultGravatarsStyle.description%", + "scope": "window" } } }, diff --git a/package.nls.json b/package.nls.json index b1359b0989..5fa4c41934 100644 --- a/package.nls.json +++ b/package.nls.json @@ -62,6 +62,14 @@ "githubPullRequests.defaultCommentType.description": "The default comment type to use when submitting a comment and there is no active review", "githubPullRequests.defaultCommentType.single": "Submits the comment as a single comment that will be immediately visible to other users", "githubPullRequests.defaultCommentType.review": "Submits the comment as a review comment that will be visible to other users once the review is submitted", + "githubPullRequests.defaultGravatarsStyle.description":"Specifies the style of the gravatar default (fallback) images", + "githubPullRequests.defaultGravatarsStyle.none":"Disables Gravatar integration for GitHub Enterprise repositories", + "githubPullRequests.defaultGravatarsStyle.identicon":"A geometric pattern", + "githubPullRequests.defaultGravatarsStyle.mp":"A simple, cartoon-style silhouetted outline of a person (does not vary by email hash)", + "githubPullRequests.defaultGravatarsStyle.monsterid":"A monster with different colors, faces, etc", + "githubPullRequests.defaultGravatarsStyle.retro":"8-bit arcade-style pixelated faces", + "githubPullRequests.defaultGravatarsStyle.robohash":"A robot with different colors, faces, etc", + "githubPullRequests.defaultGravatarsStyle.wavatar":"A face with differing features and backgrounds", "githubPullRequests.setAutoMerge.description": "Checks the \"Auto-merge\" checkbox in the \"Create Pull Request\" view.", "githubIssues.ignoreMilestones.description": "An array of milestones titles to never show issues from.", "githubIssues.createIssueTriggers.description": "Strings that will cause the 'Create issue from comment' code action to show.", diff --git a/src/github/folderRepositoryManager.ts b/src/github/folderRepositoryManager.ts index 5b0632d61b..6d7b7c3b88 100644 --- a/src/github/folderRepositoryManager.ts +++ b/src/github/folderRepositoryManager.ts @@ -1563,7 +1563,7 @@ export class FolderRepositoryManager implements vscode.Disposable { // Create PR const { data } = await repo.octokit.call(repo.octokit.api.issues.create, params); - const item = convertRESTIssueToRawPullRequest(data, repo); + const item = await convertRESTIssueToRawPullRequest(data, repo); const issueModel = new IssueModel(repo, repo.remote, item); /* __GDPR__ @@ -2018,7 +2018,7 @@ export class FolderRepositoryManager implements vscode.Disposable { repo: remote.repositoryName, pull_number: pullRequest.number, }); - pullRequest.update(convertRESTPullRequestToRawPullRequest(data, githubRepository)); + pullRequest.update(await convertRESTPullRequestToRawPullRequest(data, githubRepository)); } if (!pullRequest.mergeBase) { diff --git a/src/github/githubRepository.ts b/src/github/githubRepository.ts index 9efffd2b56..dd18a46b1a 100644 --- a/src/github/githubRepository.ts +++ b/src/github/githubRepository.ts @@ -397,18 +397,20 @@ export class GitHubRepository implements vscode.Disposable { }; } - const pullRequests = result.data - .map(pullRequest => { - if (!pullRequest.head.repo) { - Logger.appendLine('The remote branch for this PR was already deleted.', GitHubRepository.ID); - return null; - } - - return this.createOrUpdatePullRequestModel( - convertRESTPullRequestToRawPullRequest(pullRequest, this), - ); - }) - .filter(item => item !== null) as PullRequestModel[]; + const pullRequests = ( + await Promise.all( + result.data.map(async pullRequest => { + if (!pullRequest.head.repo) { + Logger.appendLine('The remote branch for this PR was already deleted.', GitHubRepository.ID); + return null; + } + + return this.createOrUpdatePullRequestModel( + await convertRESTPullRequestToRawPullRequest(pullRequest, this), + ); + }), + ) + ).filter(item => item !== null) as PullRequestModel[]; Logger.debug(`Fetch all pull requests - done`, GitHubRepository.ID); return { @@ -926,7 +928,7 @@ export class GitHubRepository implements vscode.Disposable { ...result.data.repository.mentionableUsers.nodes.map(node => { return { login: node.login, - avatarUrl: getAvatarWithEnterpriseFallback(node.avatarUrl, undefined, this.remote.authProviderId), + avatarUrl: getAvatarWithEnterpriseFallback(node.avatarUrl, node.email, this.remote.authProviderId), name: node.name, url: node.url, email: node.email, @@ -969,7 +971,7 @@ export class GitHubRepository implements vscode.Disposable { ...result.data.repository.assignableUsers.nodes.map(node => { return { login: node.login, - avatarUrl: getAvatarWithEnterpriseFallback(node.avatarUrl, undefined, this.remote.authProviderId), + avatarUrl: getAvatarWithEnterpriseFallback(node.avatarUrl, node.email, this.remote.authProviderId), name: node.name, url: node.url, email: node.email, @@ -1106,7 +1108,7 @@ export class GitHubRepository implements vscode.Disposable { ...result.data.repository.pullRequest.participants.nodes.map(node => { return { login: node.login, - avatarUrl: getAvatarWithEnterpriseFallback(node.avatarUrl, undefined, this.remote.authProviderId), + avatarUrl: getAvatarWithEnterpriseFallback(node.avatarUrl, node.email, this.remote.authProviderId), name: node.name, url: node.url, email: node.email, @@ -1218,7 +1220,7 @@ export class GitHubRepository implements vscode.Disposable { id: context.id, url: context.targetUrl ?? undefined, avatarUrl: context.avatarUrl - ? getAvatarWithEnterpriseFallback(context.avatarUrl, undefined, this.remote.authProviderId) + ? getAvatarWithEnterpriseFallback(context.avatarUrl, context.creator?.email, this.remote.authProviderId) : undefined, state: this.mapStateAsCheckState(context.state), description: context.description, diff --git a/src/github/graphql.ts b/src/github/graphql.ts index 47b8e0e33d..43a528a437 100644 --- a/src/github/graphql.ts +++ b/src/github/graphql.ts @@ -13,6 +13,7 @@ export interface MergedEvent { login: string; avatarUrl: string; url: string; + email?: string; }; createdAt: string; mergeRef: { @@ -32,6 +33,7 @@ export interface HeadRefDeletedEvent { login: string; avatarUrl: string; url: string; + email?: string; }; createdAt: string; headRefName: string; @@ -99,6 +101,7 @@ export interface ReviewComment { login: string; avatarUrl: string; url: string; + email?: string; }; path: string; originalPosition: number; @@ -130,15 +133,26 @@ export interface Commit { id: string; commit: { author: { + avatarUrl: string + name: string + email: string user: { login: string; avatarUrl: string; url: string; + email: string; }; }; committer: { - avatarUrl: string; - name: string; + avatarUrl: string + name: string + email: string + user: { + login: string; + avatarUrl: string; + url: string; + email: string; + }; }; oid: string; message: string; @@ -155,11 +169,13 @@ export interface AssignedEvent { login: string; avatarUrl: string; url: string; + email?: string; }; user: { login: string; avatarUrl: string; url: string; + email: string; }; } @@ -173,6 +189,7 @@ export interface Review { login: string; avatarUrl: string; url: string; + email?: string; }; state: 'COMMENTED' | 'APPROVED' | 'CHANGES_REQUESTED' | 'PENDING'; body: string; @@ -479,6 +496,7 @@ export interface SuggestedReviewerResponse { avatarUrl: string; name: string; url: string; + email: string; }; } @@ -504,6 +522,7 @@ export interface PullRequest { login: string; url: string; avatarUrl: string; + email?: string }; comments?: { nodes: AbbreviatedIssueComment[]; @@ -672,10 +691,11 @@ export interface ContributionsCollection { export interface UserResponse { user: { login: string; - avatarUrl?: string; - bio?: string; - company?: string; - location?: string; + avatarUrl: string; + email: string; + bio: string; + company: string; + location: string; name: string; contributionsCollection: ContributionsCollection; url: string; @@ -707,6 +727,9 @@ export interface StatusContext { targetUrl: string | null; avatarUrl: string | null; isRequired: boolean; + creator?: { + email?: string + } } export interface CheckRun { diff --git a/src/github/issueModel.ts b/src/github/issueModel.ts index 14eea6eec3..54705027f8 100644 --- a/src/github/issueModel.ts +++ b/src/github/issueModel.ts @@ -18,7 +18,7 @@ import { UpdatePullRequestResponse, } from './graphql'; import { GithubItemStateEnum, IAccount, IMilestone, IPullRequestEditData, Issue } from './interface'; -import { parseGraphQlIssueComment, parseGraphQLTimelineEvents } from './utils'; +import { getAvatarWithEnterpriseFallback, parseGraphQlIssueComment, parseGraphQLTimelineEvents } from './utils'; export class IssueModel { static ID = 'IssueModel'; @@ -71,7 +71,7 @@ export class IssueModel { public get userAvatar(): string | undefined { if (this.item) { - return this.item.user.avatarUrl; + return getAvatarWithEnterpriseFallback(this.item.user.avatarUrl, this.item.user.email, this.githubRepository.remote.authProviderId); } return undefined; diff --git a/src/github/issueOverview.ts b/src/github/issueOverview.ts index f278368648..ad2fc573eb 100644 --- a/src/github/issueOverview.ts +++ b/src/github/issueOverview.ts @@ -151,6 +151,7 @@ export class IssueOverviewPanel extends W name: this._item.author.name, avatarUrl: this._item.userAvatar, url: this._item.author.url, + email: this._item.author.email, }, state: this._item.state, events: timelineEvents, diff --git a/src/github/pullRequestModel.ts b/src/github/pullRequestModel.ts index 48c3a557f8..621b0726e9 100644 --- a/src/github/pullRequestModel.ts +++ b/src/github/pullRequestModel.ts @@ -61,6 +61,7 @@ import { IssueModel } from './issueModel'; import { convertRESTPullRequestToRawPullRequest, convertRESTReviewEvent, + getAvatarWithEnterpriseFallback, getReactionGroup, insertNewCommitsSinceReview, parseGraphQLComment, @@ -785,7 +786,7 @@ export class PullRequestModel extends IssueModel implements IPullRe const account: IAccount = { login: reviewer.requestedReviewer.login, url: reviewer.requestedReviewer.url, - avatarUrl: reviewer.requestedReviewer.avatarUrl, + avatarUrl: getAvatarWithEnterpriseFallback(reviewer.requestedReviewer.avatarUrl, reviewer.requestedReviewer.email, remote.authProviderId), email: reviewer.requestedReviewer.email, name: reviewer.requestedReviewer.name }; @@ -794,7 +795,7 @@ export class PullRequestModel extends IssueModel implements IPullRe const team: ITeam = { name: reviewer.requestedReviewer.name, url: reviewer.requestedReviewer.url, - avatarUrl: reviewer.requestedReviewer.avatarUrl, + avatarUrl: getAvatarWithEnterpriseFallback(reviewer.requestedReviewer.avatarUrl, undefined, remote.authProviderId), id: reviewer.requestedReviewer.id!, org: remote.owner, slug: reviewer.requestedReviewer.slug! @@ -1244,7 +1245,7 @@ export class PullRequestModel extends IssueModel implements IPullRe repo: remote.repositoryName, pull_number: this.number, }); - this.update(convertRESTPullRequestToRawPullRequest(info.data, githubRepository)); + this.update(await convertRESTPullRequestToRawPullRequest(info.data, githubRepository)); } let compareWithBaseRef = this.base.sha; diff --git a/src/github/pullRequestOverview.ts b/src/github/pullRequestOverview.ts index a0a9f0aa5a..d8f5ab8edd 100644 --- a/src/github/pullRequestOverview.ts +++ b/src/github/pullRequestOverview.ts @@ -238,6 +238,7 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel { + let email = user.email; + if (githubRepository.remote.authProviderId === AuthProvider['github-enterprise'] && email === undefined) { + const { data } = await githubRepository.octokit.call(githubRepository.octokit.api.users.getByUsername, { + username: user.login + }); + + email = data.email; + } + + email ||= undefined; + return { login: user.login, url: user.html_url, - avatarUrl: githubRepository ? getAvatarWithEnterpriseFallback(user.avatar_url, user.gravatar_id ?? undefined, githubRepository.remote.authProviderId) : undefined, + avatarUrl: getAvatarWithEnterpriseFallback(user.avatar_url, email, githubRepository.remote.authProviderId), + email: email, }; } @@ -261,12 +273,12 @@ export function convertRESTHeadToIGitHubRef(head: OctokitCommon.PullsListRespons }; } -export function convertRESTPullRequestToRawPullRequest( +export async function convertRESTPullRequestToRawPullRequest( pullRequest: | OctokitCommon.PullsGetResponseData | OctokitCommon.PullsListResponseItem, githubRepository: GitHubRepository, -): PullRequest { +): Promise { const { number, body, @@ -293,11 +305,11 @@ export function convertRESTPullRequestToRawPullRequest( title, titleHTML: title, url: html_url, - user: convertRESTUserToAccount(user!, githubRepository), + user: await convertRESTUserToAccount(user!, githubRepository), state, merged: (pullRequest as OctokitCommon.PullsGetResponseData).merged || false, assignees: assignees - ? assignees.map(assignee => convertRESTUserToAccount(assignee!, githubRepository)) + ? await Promise.all(assignees.map(assignee => convertRESTUserToAccount(assignee!, githubRepository))) : undefined, createdAt: created_at, updatedAt: updated_at, @@ -318,10 +330,10 @@ export function convertRESTPullRequestToRawPullRequest( return item; } -export function convertRESTIssueToRawPullRequest( +export async function convertRESTIssueToRawPullRequest( pullRequest: OctokitCommon.IssuesCreateResponseData, githubRepository: GitHubRepository, -): PullRequest { +): Promise { const { number, body, @@ -345,10 +357,10 @@ export function convertRESTIssueToRawPullRequest( title, titleHTML: title, url: html_url, - user: convertRESTUserToAccount(user!, githubRepository), + user: await convertRESTUserToAccount(user!, githubRepository), state, assignees: assignees - ? assignees.map(assignee => convertRESTUserToAccount(assignee!, githubRepository)) + ? await Promise.all(assignees.map(assignee => convertRESTUserToAccount(assignee!, githubRepository))) : undefined, createdAt: created_at, updatedAt: updated_at, @@ -361,10 +373,10 @@ export function convertRESTIssueToRawPullRequest( return item; } -export function convertRESTReviewEvent( +export async function convertRESTReviewEvent( review: OctokitCommon.PullsCreateReviewResponseData, githubRepository: GitHubRepository, -): Common.ReviewEvent { +): Promise { return { event: Common.EventType.Reviewed, comments: [], @@ -372,7 +384,7 @@ export function convertRESTReviewEvent( body: review.body, bodyHTML: review.body, htmlUrl: review.html_url, - user: convertRESTUserToAccount(review.user!, githubRepository), + user: await convertRESTUserToAccount(review.user!, githubRepository), authorAssociation: review.user!.type, state: review.state as 'COMMENTED' | 'APPROVED' | 'CHANGES_REQUESTED' | 'PENDING', id: review.id, @@ -533,7 +545,7 @@ function parseAuthor( return { login: author.login, url: author.url, - avatarUrl: getAvatarWithEnterpriseFallback(author.avatarUrl, undefined, githubRepository.remote.authProviderId), + avatarUrl: getAvatarWithEnterpriseFallback(author.avatarUrl, author.email, githubRepository.remote.authProviderId), email: author.email }; } else { @@ -618,7 +630,7 @@ export function parseGraphQLPullRequest( allowAutoMerge: graphQLPullRequest.viewerCanEnableAutoMerge || graphQLPullRequest.viewerCanDisableAutoMerge, labels: graphQLPullRequest.labels.nodes, isDraft: graphQLPullRequest.isDraft, - suggestedReviewers: parseSuggestedReviewers(graphQLPullRequest.suggestedReviewers), + suggestedReviewers: parseSuggestedReviewers(graphQLPullRequest.suggestedReviewers, githubRepository), comments: parseComments(graphQLPullRequest.comments?.nodes, githubRepository), milestone: parseMilestone(graphQLPullRequest.milestone), assignees: graphQLPullRequest.assignees?.nodes.map(assignee => parseAuthor(assignee, githubRepository)), @@ -669,16 +681,16 @@ export function parseGraphQLIssue(issue: GraphQL.PullRequest, githubRepository: function parseSuggestedReviewers( suggestedReviewers: GraphQL.SuggestedReviewerResponse[] | undefined, + githubRepository: GitHubRepository ): ISuggestedReviewer[] { if (!suggestedReviewers) { return []; } const ret: ISuggestedReviewer[] = suggestedReviewers.map(suggestedReviewer => { + const user: IAccount = parseAuthor(suggestedReviewer.reviewer, githubRepository); + return { - login: suggestedReviewer.reviewer.login, - avatarUrl: suggestedReviewer.reviewer.avatarUrl, - name: suggestedReviewer.reviewer.name, - url: suggestedReviewer.reviewer.url, + ...user, isAuthor: suggestedReviewer.isAuthor, isCommenter: suggestedReviewer.isCommenter, }; @@ -774,7 +786,7 @@ export function parseGraphQLTimelineEvents( sha: commitEv.commit.oid, author: commitEv.commit.author.user ? parseAuthor(commitEv.commit.author.user, githubRepository) - : { login: commitEv.commit.committer.name }, + : { login: commitEv.commit.author.name, avatarUrl: commitEv.commit.author.avatarUrl, email: commitEv.commit.author.email }, htmlUrl: commitEv.url, message: commitEv.commit.message, authoredDate: new Date(commitEv.commit.authoredDate), @@ -802,7 +814,7 @@ export function parseGraphQLTimelineEvents( id: assignEv.id, event: type, user: parseAuthor(assignEv.user, githubRepository), - actor: assignEv.actor, + actor: parseAuthor(assignEv.actor, githubRepository), }); return; case Common.EventType.HeadRefDeleted: @@ -824,16 +836,17 @@ export function parseGraphQLTimelineEvents( return normalizedEvents; } -export function parseGraphQLUser(user: GraphQL.UserResponse, githubRepository: GitHubRepository): User { +export function parseGraphQLUser(resp: GraphQL.UserResponse, githubRepository: GitHubRepository): User { + const user = resp.user; return { - login: user.user.login, - name: user.user.name, - avatarUrl: getAvatarWithEnterpriseFallback(user.user.avatarUrl ?? '', undefined, githubRepository.remote.authProviderId), - url: user.user.url, - bio: user.user.bio, - company: user.user.company, - location: user.user.location, - commitContributions: parseGraphQLCommitContributions(user.user.contributionsCollection), + login: user.login, + name: user.name, + avatarUrl: getAvatarWithEnterpriseFallback(user.avatarUrl, user.email, githubRepository.remote.authProviderId), + url: user.url, + bio: user.bio, + company: user.company, + location: user.location, + commitContributions: parseGraphQLCommitContributions(user.contributionsCollection), }; } @@ -1119,12 +1132,29 @@ export function hasEnterpriseUri(): boolean { } export function generateGravatarUrl(gravatarId: string | undefined, size: number = 200): string | undefined { - return !!gravatarId ? `https://www.gravatar.com/avatar/${gravatarId}?s=${size}&d=retro` : undefined; + if (!gravatarId) { + return undefined; + } + const none = 'none'; + + let style = vscode.workspace.getConfiguration('githubPullRequests').get('defaultGravatarsStyle', none); + if (style === none) { + return undefined; + } + + return `https://www.gravatar.com/avatar/${gravatarId}?s=${size}&d=${style}`; } -export function getAvatarWithEnterpriseFallback(avatarUrl: string, email: string | undefined, authProviderId: AuthProvider): string | undefined { - return authProviderId === AuthProvider.github ? avatarUrl : (email ? generateGravatarUrl( - crypto.createHash('md5').update(email?.trim()?.toLowerCase()).digest('hex')) : undefined); +export function getAvatarWithEnterpriseFallback(avatarUrl: string | undefined, email: string | undefined, authProviderId: AuthProvider): string | undefined { + if (authProviderId === AuthProvider.github) { + return avatarUrl; + } + + if (!email) { + return undefined; + } + + return generateGravatarUrl(crypto.createHash('md5').update(email.trim().toLowerCase()).digest('hex')); } export function getPullsUrl(repo: GitHubRepository) { diff --git a/src/test/github/folderRepositoryManager.test.ts b/src/test/github/folderRepositoryManager.test.ts index 244489e72e..c8b8bd3cc0 100644 --- a/src/test/github/folderRepositoryManager.test.ts +++ b/src/test/github/folderRepositoryManager.test.ts @@ -43,7 +43,7 @@ describe('PullRequestManager', function () { }); describe('activePullRequest', function () { - it('gets and sets the active pull request', function () { + it('gets and sets the active pull request', async function () { assert.strictEqual(manager.activePullRequest, undefined); const changeFired = sinon.spy(); @@ -54,7 +54,7 @@ describe('PullRequestManager', function () { const remote = new GitHubRemote('origin', url, protocol, GitHubServerType.GitHubDotCom); 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 prItem = await convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().build(), repository); const pr = new PullRequestModel(manager.credentialStore, telemetry, repository, remote, prItem); manager.activePullRequest = pr; diff --git a/src/test/github/pullRequestGitHelper.test.ts b/src/test/github/pullRequestGitHelper.test.ts index 8779e56c9e..f1f1f3968e 100644 --- a/src/test/github/pullRequestGitHelper.test.ts +++ b/src/test/github/pullRequestGitHelper.test.ts @@ -50,7 +50,7 @@ describe('PullRequestGitHelper', function () { const remote = new GitHubRemote('elsewhere', url, new Protocol(url), GitHubServerType.GitHubDotCom); const gitHubRepository = new MockGitHubRepository(remote, credentialStore, telemetry, sinon); - const prItem = convertRESTPullRequestToRawPullRequest( + const prItem = await convertRESTPullRequestToRawPullRequest( new PullRequestBuilder() .number(100) .user(u => u.login('me')) diff --git a/src/test/github/pullRequestModel.test.ts b/src/test/github/pullRequestModel.test.ts index 5b93ac74a4..3f6c0bfe2b 100644 --- a/src/test/github/pullRequestModel.test.ts +++ b/src/test/github/pullRequestModel.test.ts @@ -75,28 +75,28 @@ describe('PullRequestModel', function () { sinon.restore(); }); - it('should return `state` properly as `open`', function () { + it('should return `state` properly as `open`', async function () { const pr = new PullRequestBuilder().state('open').build(); - const open = new PullRequestModel(credentials, telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr, repo)); + const open = new PullRequestModel(credentials, telemetry, repo, remote, await convertRESTPullRequestToRawPullRequest(pr, repo)); assert.strictEqual(open.state, GithubItemStateEnum.Open); }); - it('should return `state` properly as `closed`', function () { + it('should return `state` properly as `closed`', async function () { const pr = new PullRequestBuilder().state('closed').build(); - const open = new PullRequestModel(credentials, telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr, repo)); + const open = new PullRequestModel(credentials, telemetry, repo, remote, await convertRESTPullRequestToRawPullRequest(pr, repo)); assert.strictEqual(open.state, GithubItemStateEnum.Closed); }); - it('should return `state` properly as `merged`', function () { + it('should return `state` properly as `merged`', async function () { const pr = new PullRequestBuilder().merged(true).state('closed').build(); - const open = new PullRequestModel(credentials, telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr, repo)); + const open = new PullRequestModel(credentials, telemetry, repo, remote, await convertRESTPullRequestToRawPullRequest(pr, repo)); assert.strictEqual(open.state, GithubItemStateEnum.Merged); }); - describe('reviewThreadCache', function () { + describe('reviewThreadCache', async function () { it('should update the cache when then cache is initialized', async function () { const pr = new PullRequestBuilder().build(); const model = new PullRequestModel( @@ -104,7 +104,7 @@ describe('PullRequestModel', function () { telemetry, repo, remote, - convertRESTPullRequestToRawPullRequest(pr, repo), + await convertRESTPullRequestToRawPullRequest(pr, repo), ); repo.queryProvider.expectGraphQLQuery( diff --git a/src/test/github/pullRequestOverview.test.ts b/src/test/github/pullRequestOverview.test.ts index bfc401eb8c..8f9be6806a 100644 --- a/src/test/github/pullRequestOverview.test.ts +++ b/src/test/github/pullRequestOverview.test.ts @@ -73,7 +73,7 @@ describe('PullRequestOverview', function () { }); }); - const prItem = convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(1000).build(), repo); + const prItem = await convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(1000).build(), repo); const prModel = new PullRequestModel(credentialStore, telemetry, repo, remote, prItem); await PullRequestOverviewPanel.createOrShow(EXTENSION_URI, pullRequestManager, prModel); @@ -106,7 +106,7 @@ describe('PullRequestOverview', function () { }); }); - const prItem0 = convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(1000).build(), repo); + const prItem0 = await convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(1000).build(), repo); const prModel0 = new PullRequestModel(credentialStore, telemetry, repo, remote, prItem0); const resolveStub = sinon.stub(pullRequestManager, 'resolvePullRequest').resolves(prModel0); sinon.stub(prModel0, 'getReviewRequests').resolves([]); @@ -119,7 +119,7 @@ describe('PullRequestOverview', function () { assert.strictEqual(createWebviewPanel.callCount, 1); assert.strictEqual(panel0!.getCurrentTitle(), 'Pull Request #1000'); - const prItem1 = convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(2000).build(), repo); + const prItem1 = await convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(2000).build(), repo); const prModel1 = new PullRequestModel(credentialStore, telemetry, repo, remote, prItem1); resolveStub.resolves(prModel1); sinon.stub(prModel1, 'getReviewRequests').resolves([]); diff --git a/src/test/view/reviewCommentController.test.ts b/src/test/view/reviewCommentController.test.ts index 1975b2d2e3..49b9307dc1 100644 --- a/src/test/view/reviewCommentController.test.ts +++ b/src/test/view/reviewCommentController.test.ts @@ -90,7 +90,7 @@ describe('ReviewCommentController', function () { telemetry, githubRepo, remote, - convertRESTPullRequestToRawPullRequest(pr, githubRepo), + await convertRESTPullRequestToRawPullRequest(pr, githubRepo), ); manager.activePullRequest = activePullRequest; From 8c2b8d90dab9c7c9c03e165688458a23624b934b Mon Sep 17 00:00:00 2001 From: Kevin Abel Date: Wed, 8 Feb 2023 12:00:51 -0600 Subject: [PATCH 2/4] Implement Enterprise Avatar REST service Uses the undocumented enterprise avatar REST service to construct a data uri for the webviews. The REST service is protected from too many concurrent requests by the `p-queue` package that should be compatible with all runtimes. Undoes some async APIs by making the enterprise decorations (avatarUrl replacement and/or email fetching) their own utility functions, run only when necessary. Consolidates the test for Enterprise auth. --- package.json | 3 +- src/common/remote.ts | 6 +- src/gitExtensionIntegration.ts | 3 +- src/github/activityBarViewProvider.ts | 2 +- src/github/createPRViewProvider.ts | 2 +- src/github/credentials.ts | 18 +- src/github/folderRepositoryManager.ts | 26 +- src/github/githubRepository.ts | 238 ++++++++---- src/github/graphql.ts | 6 +- src/github/interface.ts | 9 +- src/github/issueModel.ts | 36 +- src/github/issueOverview.ts | 4 +- src/github/pullRequestModel.ts | 150 ++++++-- src/github/pullRequestOverview.ts | 2 +- src/github/utils.ts | 353 ++++++++++++------ src/issues/stateManager.ts | 2 +- .../github/folderRepositoryManager.test.ts | 4 +- src/test/github/pullRequestGitHelper.test.ts | 5 +- src/test/github/pullRequestModel.test.ts | 16 +- src/test/github/pullRequestOverview.test.ts | 6 +- src/test/view/prsTree.test.ts | 8 +- src/test/view/reviewCommentController.test.ts | 2 +- webpack.config.js | 7 + .../editorWebview/test/builder/pullRequest.ts | 1 - yarn.lock | 25 ++ 25 files changed, 646 insertions(+), 288 deletions(-) diff --git a/package.json b/package.json index f69095f71a..9361670f52 100644 --- a/package.json +++ b/package.json @@ -517,7 +517,7 @@ }, "githubPullRequests.defaultGravatarsStyle": { "type": "string", - "default": "invertocat", + "default": "none", "enum": [ "none", "identicon", @@ -2379,6 +2379,7 @@ "fast-deep-equal": "^3.1.3", "lru-cache": "6.0.0", "marked": "^4.0.10", + "p-queue": "^6.6.2", "react": "^16.12.0", "react-dom": "^16.12.0", "ssh-config": "4.1.1", diff --git a/src/common/remote.ts b/src/common/remote.ts index 4f3692fc3b..4f76fe35ed 100644 --- a/src/common/remote.ts +++ b/src/common/remote.ts @@ -4,7 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import { Repository } from '../api/api'; -import { getEnterpriseUri } from '../github/utils'; +import { getEnterpriseUri, isEnterprise } from '../github/utils'; import { AuthProvider, GitHubServerType } from './authentication'; import { Protocol } from './protocol'; @@ -28,6 +28,10 @@ export class Remote { return this.host === getEnterpriseUri()?.authority ? AuthProvider['github-enterprise'] : AuthProvider.github; } + public get isEnterprise(): boolean { + return isEnterprise(this.authProviderId); + } + constructor( public readonly remoteName: string, public readonly url: string, diff --git a/src/gitExtensionIntegration.ts b/src/gitExtensionIntegration.ts index 5cac393313..88ee9cc940 100644 --- a/src/gitExtensionIntegration.ts +++ b/src/gitExtensionIntegration.ts @@ -7,6 +7,7 @@ import { RemoteSource, RemoteSourceProvider } from './@types/git'; import { AuthProvider } from './common/authentication'; import { OctokitCommon } from './github/common'; import { CredentialStore, GitHub } from './github/credentials'; +import { isEnterprise } from './github/utils'; interface Repository { readonly full_name: string; @@ -39,7 +40,7 @@ export class GithubRemoteSourceProvider implements RemoteSourceProvider { private userReposCache: RemoteSource[] = []; constructor(private readonly credentialStore: CredentialStore, private readonly authProviderId: AuthProvider = AuthProvider.github) { - if (authProviderId === AuthProvider['github-enterprise']) { + if (isEnterprise(authProviderId)) { this.name = 'GitHub Enterprise'; } } diff --git a/src/github/activityBarViewProvider.ts b/src/github/activityBarViewProvider.ts index 15614a3088..7e2c94f791 100644 --- a/src/github/activityBarViewProvider.ts +++ b/src/github/activityBarViewProvider.ts @@ -408,7 +408,7 @@ export class PullRequestViewProvider extends WebviewViewBase implements vscode.W - + Active Pull Request diff --git a/src/github/createPRViewProvider.ts b/src/github/createPRViewProvider.ts index 3f444c08d8..f8933f1dec 100644 --- a/src/github/createPRViewProvider.ts +++ b/src/github/createPRViewProvider.ts @@ -671,7 +671,7 @@ export class CreatePullRequestViewProvider extends WebviewViewBase implements vs - + Create Pull Request diff --git a/src/github/credentials.ts b/src/github/credentials.ts index f6ef11018a..0d45c6e2e9 100644 --- a/src/github/credentials.ts +++ b/src/github/credentials.ts @@ -16,7 +16,7 @@ import { ITelemetry } from '../common/telemetry'; import { agent } from '../env/node/net'; import { IAccount } from './interface'; import { LoggingApolloClient, LoggingOctokit, RateLogger } from './loggingOctokit'; -import { convertRESTUserToAccount, getEnterpriseUri, hasEnterpriseUri } from './utils'; +import { convertRESTUserToAccount, getEnterpriseUri, hasEnterpriseUri, isEnterprise } from './utils'; const TRY_AGAIN = vscode.l10n.t('Try again?'); const CANCEL = vscode.l10n.t('Cancel'); @@ -89,7 +89,7 @@ export class CredentialStore implements vscode.Disposable { 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 (isEnterprise(authProviderId)) { if (!hasEnterpriseUri()) { Logger.debug(`GitHub Enterprise provider selected without URI.`, 'Authentication'); return; @@ -124,7 +124,7 @@ export class CredentialStore implements vscode.Disposable { } if (session) { - if (authProviderId === AuthProvider.github) { + if (!isEnterprise(authProviderId)) { this._sessionId = session.id; } else { this._enterpriseSessionId = session.id; @@ -139,7 +139,7 @@ export class CredentialStore implements vscode.Disposable { return this.initialize(authProviderId, getAuthSessionOptions); } } - if (authProviderId === AuthProvider.github) { + if (!isEnterprise(authProviderId)) { this._githubAPI = github; this._scopes = usedScopes; } else { @@ -182,7 +182,7 @@ export class CredentialStore implements vscode.Disposable { } public isAuthenticated(authProviderId: AuthProvider): boolean { - if (authProviderId === AuthProvider.github) { + if (!isEnterprise(authProviderId)) { return !!this._githubAPI; } return !!this._githubEnterpriseAPI; @@ -196,7 +196,7 @@ export class CredentialStore implements vscode.Disposable { } public getHub(authProviderId: AuthProvider): GitHub | undefined { - if (authProviderId === AuthProvider.github) { + if (!isEnterprise(authProviderId)) { return this._githubAPI; } return this._githubEnterpriseAPI; @@ -209,7 +209,7 @@ export class CredentialStore implements vscode.Disposable { } public async getHubOrLogin(authProviderId: AuthProvider): Promise { - if (authProviderId === AuthProvider.github) { + if (!isEnterprise(authProviderId)) { return this._githubAPI ?? (await this.login(authProviderId)); } return this._githubEnterpriseAPI ?? (await this.login(authProviderId)); @@ -341,7 +341,7 @@ export class CredentialStore implements vscode.Disposable { private async createHub(token: string, authProviderId: AuthProvider): Promise { let baseUrl = 'https://api.github.com'; let enterpriseServerUri: vscode.Uri | undefined; - if (authProviderId === AuthProvider['github-enterprise']) { + if (isEnterprise(authProviderId)) { enterpriseServerUri = getEnterpriseUri(); } @@ -415,5 +415,5 @@ const link = (url: string, token: string) => ); function getGitHubSuffix(authProviderId: AuthProvider) { - return authProviderId === AuthProvider.github ? '' : ' Enterprise'; + return isEnterprise(authProviderId) ? ' Enterprise' : ''; } diff --git a/src/github/folderRepositoryManager.ts b/src/github/folderRepositoryManager.ts index 6d7b7c3b88..eabbb2cdc5 100644 --- a/src/github/folderRepositoryManager.ts +++ b/src/github/folderRepositoryManager.ts @@ -48,6 +48,8 @@ import { getRelatedUsersFromTimelineEvents, loginComparator, parseGraphQLUser, + replaceAccountAvatarUrls, + replaceAvatarUrl, teamComparator, variableSubstitution, } from './utils'; @@ -1563,7 +1565,12 @@ export class FolderRepositoryManager implements vscode.Disposable { // Create PR const { data } = await repo.octokit.call(repo.octokit.api.issues.create, params); - const item = await convertRESTIssueToRawPullRequest(data, repo); + const item = convertRESTIssueToRawPullRequest(data); + + if (repo.remote.isEnterprise) { + await replaceAccountAvatarUrls(item, repo.octokit); + } + const issueModel = new IssueModel(repo, repo.remote, item); /* __GDPR__ @@ -2018,7 +2025,13 @@ export class FolderRepositoryManager implements vscode.Disposable { repo: remote.repositoryName, pull_number: pullRequest.number, }); - pullRequest.update(await convertRESTPullRequestToRawPullRequest(data, githubRepository)); + const pr = convertRESTPullRequestToRawPullRequest(data); + + if (remote.isEnterprise) { + await replaceAccountAvatarUrls(pr, octokit); + } + + pullRequest.update(pr); } if (!pullRequest.mergeBase) { @@ -2082,7 +2095,7 @@ export class FolderRepositoryManager implements vscode.Disposable { async resolveUser(owner: string, repositoryName: string, login: string): Promise { Logger.debug(`Fetch user ${login}`, FolderRepositoryManager.ID); const githubRepository = await this.createGitHubRepositoryFromOwnerName(owner, repositoryName); - const { query, schema } = await githubRepository.ensure(); + const { query, schema, remote, octokit } = await githubRepository.ensure(); try { const { data } = await query({ @@ -2091,7 +2104,12 @@ export class FolderRepositoryManager implements vscode.Disposable { login, }, }); - return parseGraphQLUser(data, githubRepository); + + if (remote.isEnterprise) { + await replaceAvatarUrl(data.user, octokit); + } + + return parseGraphQLUser(data); } catch (e) { // Ignore cases where the user doesn't exist if (!(e.message as (string | undefined))?.startsWith('GraphQL error: Could not resolve to a User with the login of')) { diff --git a/src/github/githubRepository.ts b/src/github/githubRepository.ts index dd18a46b1a..9ab4f18245 100644 --- a/src/github/githubRepository.ts +++ b/src/github/githubRepository.ts @@ -50,13 +50,15 @@ import { PullRequestModel } from './pullRequestModel'; import defaultSchema from './queries.gql'; import { convertRESTPullRequestToRawPullRequest, - getAvatarWithEnterpriseFallback, getOverrideBranch, getPRFetchQuery, parseGraphQLIssue, parseGraphQLPullRequest, parseGraphQLViewerPermission, parseMilestone, + replaceAccountAvatarUrls, + replaceAvatarUrl, + replaceIssuesAvatarUrls, } from './utils'; export const PULL_REQUEST_PAGE_SIZE = 20; @@ -397,20 +399,21 @@ export class GitHubRepository implements vscode.Disposable { }; } - const pullRequests = ( - await Promise.all( - result.data.map(async pullRequest => { - if (!pullRequest.head.repo) { - Logger.appendLine('The remote branch for this PR was already deleted.', GitHubRepository.ID); - return null; - } - - return this.createOrUpdatePullRequestModel( - await convertRESTPullRequestToRawPullRequest(pullRequest, this), - ); - }), - ) - ).filter(item => item !== null) as PullRequestModel[]; + const pullRequests: PullRequestModel[] = []; + for (const pullRequest of result.data) { + if (!pullRequest.head.repo) { + Logger.appendLine('The remote branch for this PR was already deleted.', GitHubRepository.ID); + continue; + } + + const pr = convertRESTPullRequestToRawPullRequest(pullRequest); + + if (remote.isEnterprise) { + await replaceAccountAvatarUrls(pr, octokit); + } + + pullRequests.push(this.createOrUpdatePullRequestModel(pr)); + } Logger.debug(`Fetch all pull requests - done`, GitHubRepository.ID); return { @@ -434,7 +437,7 @@ export class GitHubRepository implements vscode.Disposable { async getPullRequestForBranch(branch: string): Promise { try { Logger.debug(`Fetch pull requests for branch - enter`, GitHubRepository.ID); - const { query, remote, schema } = await this.ensure(); + const { query, remote, schema, octokit } = await this.ensure(); const { data } = await query({ query: schema.PullRequestForHead, variables: { @@ -446,8 +449,13 @@ export class GitHubRepository implements vscode.Disposable { Logger.debug(`Fetch pull requests for branch - done`, GitHubRepository.ID); if (data?.repository.pullRequests.nodes.length > 0) { - const prs = data.repository.pullRequests.nodes.map(node => parseGraphQLPullRequest(node, this)); + const prs = data.repository.pullRequests.nodes.map(node => parseGraphQLPullRequest(node)); const mostRecentOrOpenPr = prs.find(pr => pr.state.toLowerCase() === 'open') ?? prs[0]; + + if (remote.isEnterprise) { + await replaceAccountAvatarUrls(mostRecentOrOpenPr, octokit); + } + return this.createOrUpdatePullRequestModel(mostRecentOrOpenPr); } } catch (e) { @@ -521,7 +529,7 @@ export class GitHubRepository implements vscode.Disposable { async getIssuesForUserByMilestone(_page?: number): Promise { try { Logger.debug(`Fetch all issues - enter`, GitHubRepository.ID); - const { query, remote, schema } = await this.ensure(); + const { query, remote, schema, octokit } = await this.ensure(); const { data } = await query({ query: schema.GetMilestonesWithIssues, variables: { @@ -534,16 +542,20 @@ export class GitHubRepository implements vscode.Disposable { const milestones: { milestone: IMilestone; issues: IssueModel[] }[] = []; if (data && data.repository.milestones && data.repository.milestones.nodes) { - data.repository.milestones.nodes.forEach(raw => { + for (const raw of data.repository.milestones.nodes) { const milestone = parseMilestone(raw); - if (milestone) { - const issues: IssueModel[] = []; - raw.issues.edges.forEach(issue => { - issues.push(new IssueModel(this, this.remote, parseGraphQLIssue(issue.node, this))); - }); - milestones.push({ milestone, issues }); + if (!milestone) { + continue; } - }); + + const issues: Issue[] = raw.issues.edges.map(edge => parseGraphQLIssue(edge.node)); + + if (remote.isEnterprise) { + await replaceIssuesAvatarUrls(issues, octokit); + } + + milestones.push({ milestone, issues: issues.map(issue => new IssueModel(this, this.remote, issue)) }); + } } return { items: milestones, @@ -558,7 +570,7 @@ export class GitHubRepository implements vscode.Disposable { async getIssuesWithoutMilestone(_page?: number): Promise { try { Logger.debug(`Fetch issues without milestone- enter`, GitHubRepository.ID); - const { query, remote, schema } = await this.ensure(); + const { query, remote, schema, octokit } = await this.ensure(); const { data } = await query({ query: schema.IssuesWithoutMilestone, variables: { @@ -571,12 +583,18 @@ export class GitHubRepository implements vscode.Disposable { const issues: Issue[] = []; if (data && data.repository.issues.edges) { - data.repository.issues.edges.forEach(raw => { - if (raw.node.id) { - issues.push(parseGraphQLIssue(raw.node, this)); + for (const raw of data.repository.issues.edges) { + if (!raw.node.id) { + continue; } - }); + issues.push(parseGraphQLIssue(raw.node)); + } } + + if (remote.isEnterprise) { + await replaceIssuesAvatarUrls(issues, octokit); + } + return { items: issues, hasMorePages: data.repository.issues.pageInfo.hasNextPage, @@ -590,7 +608,7 @@ export class GitHubRepository implements vscode.Disposable { async getIssues(page?: number, queryString?: string): Promise { try { Logger.debug(`Fetch issues with query - enter`, GitHubRepository.ID); - const { query, schema } = await this.ensure(); + const { query, schema, remote, octokit } = await this.ensure(); const { data } = await query({ query: schema.Issues, variables: { @@ -601,12 +619,18 @@ export class GitHubRepository implements vscode.Disposable { const issues: Issue[] = []; if (data && data.search.edges) { - data.search.edges.forEach(raw => { - if (raw.node.id) { - issues.push(parseGraphQLIssue(raw.node, this)); + for (const raw of data.search.edges) { + if (!raw.node.id) { + continue; } - }); + issues.push(parseGraphQLIssue(raw.node)); + } + } + + if (remote.isEnterprise) { + await replaceIssuesAvatarUrls(issues, octokit); } + return { items: issues, hasMorePages: data.search.pageInfo.hasNextPage, @@ -700,7 +724,7 @@ export class GitHubRepository implements vscode.Disposable { async getPullRequestsForCategory(categoryQuery: string, page?: number): Promise { try { Logger.debug(`Fetch pull request category ${categoryQuery} - enter`, GitHubRepository.ID); - const { octokit, query, schema } = await this.ensure(); + const { octokit, query, schema, remote } = await this.ensure(); const user = await this.getAuthenticatedUser(); // Search api will not try to resolve repo that redirects, so get full name first @@ -727,18 +751,21 @@ export class GitHubRepository implements vscode.Disposable { const hasMorePages = !!headers.link && headers.link.indexOf('rel="next"') > -1; const pullRequestResponses = await Promise.all(promises); - const pullRequests = pullRequestResponses - .map(response => { - if (!response.repository.pullRequest.headRef) { - Logger.appendLine('The remote branch for this PR was already deleted.', GitHubRepository.ID); - return null; - } + const pullRequests: PullRequestModel[] = []; + for (const response of pullRequestResponses) { + if (!response.repository.pullRequest.headRef) { + Logger.appendLine('The remote branch for this PR was already deleted.', GitHubRepository.ID); + continue; + } - return this.createOrUpdatePullRequestModel( - parseGraphQLPullRequest(response.repository.pullRequest, this), - ); - }) - .filter(item => item !== null) as PullRequestModel[]; + const pr = parseGraphQLPullRequest(response.repository.pullRequest); + + if (remote.isEnterprise) { + await replaceAccountAvatarUrls(pr, octokit); + } + + pullRequests.push(this.createOrUpdatePullRequestModel(pr)); + } Logger.debug(`Fetch pull request category ${categoryQuery} - done`, GitHubRepository.ID); @@ -778,7 +805,7 @@ export class GitHubRepository implements vscode.Disposable { try { Logger.debug(`Create pull request - enter`, GitHubRepository.ID); const metadata = await this.getMetadata(); - const { mutate, schema } = await this.ensure(); + const { mutate, schema, remote, octokit } = await this.ensure(); const { data } = await mutate({ mutation: schema.CreatePullRequest, @@ -797,7 +824,13 @@ export class GitHubRepository implements vscode.Disposable { if (!data) { throw new Error('Failed to create pull request.'); } - return this.createOrUpdatePullRequestModel(parseGraphQLPullRequest(data.createPullRequest.pullRequest, this)); + const pr = parseGraphQLPullRequest(data.createPullRequest.pullRequest); + + if (remote.isEnterprise) { + await replaceAccountAvatarUrls(pr, octokit); + } + + return this.createOrUpdatePullRequestModel(pr); } catch (e) { Logger.error(`Unable to create PR: ${e}`, GitHubRepository.ID); throw e; @@ -807,7 +840,7 @@ export class GitHubRepository implements vscode.Disposable { async getPullRequest(id: number): Promise { try { Logger.debug(`Fetch pull request ${id} - enter`, GitHubRepository.ID); - const { query, remote, schema } = await this.ensure(); + const { query, remote, schema, octokit } = await this.ensure(); const { data } = await query({ query: schema.PullRequest, @@ -818,7 +851,13 @@ export class GitHubRepository implements vscode.Disposable { }, }); Logger.debug(`Fetch pull request ${id} - done`, GitHubRepository.ID); - return this.createOrUpdatePullRequestModel(parseGraphQLPullRequest(data.repository.pullRequest, this)); + const pr = parseGraphQLPullRequest(data.repository.pullRequest); + + if (remote.isEnterprise) { + await replaceAccountAvatarUrls(pr, octokit); + } + + return this.createOrUpdatePullRequestModel(pr); } catch (e) { Logger.error(`Unable to fetch PR: ${e}`, GitHubRepository.ID); return; @@ -828,7 +867,7 @@ export class GitHubRepository implements vscode.Disposable { async getIssue(id: number, withComments: boolean = false): Promise { try { Logger.debug(`Fetch issue ${id} - enter`, GitHubRepository.ID); - const { query, remote, schema } = await this.ensure(); + const { query, remote, schema, octokit } = await this.ensure(); const { data } = await query({ query: withComments ? schema.IssueWithComments : schema.Issue, @@ -840,7 +879,13 @@ export class GitHubRepository implements vscode.Disposable { }, true); // Don't retry on SAML errors as it's too distruptive for this query. Logger.debug(`Fetch issue ${id} - done`, GitHubRepository.ID); - return new IssueModel(this, remote, parseGraphQLIssue(data.repository.pullRequest, this)); + const issue = parseGraphQLIssue(data.repository.pullRequest); + + if (remote.isEnterprise) { + await replaceIssuesAvatarUrls([issue], octokit); + } + + return new IssueModel(this, remote, issue); } catch (e) { Logger.error(`Unable to fetch PR: ${e}`, GitHubRepository.ID); return; @@ -906,7 +951,7 @@ export class GitHubRepository implements vscode.Disposable { async getMentionableUsers(): Promise { Logger.debug(`Fetch mentionable users - enter`, GitHubRepository.ID); - const { query, remote, schema } = await this.ensure(); + const { query, remote, schema, octokit } = await this.ensure(); let after: string | null = null; let hasNextPage = false; @@ -924,11 +969,18 @@ export class GitHubRepository implements vscode.Disposable { }, }); + + if (remote.isEnterprise) { + await Promise.all( + result.data.repository.mentionableUsers.nodes.map(node => replaceAvatarUrl(node, octokit)), + ); + } + ret.push( ...result.data.repository.mentionableUsers.nodes.map(node => { return { login: node.login, - avatarUrl: getAvatarWithEnterpriseFallback(node.avatarUrl, node.email, this.remote.authProviderId), + avatarUrl: node.avatarUrl, name: node.name, url: node.url, email: node.email, @@ -949,7 +1001,7 @@ export class GitHubRepository implements vscode.Disposable { async getAssignableUsers(): Promise { Logger.debug(`Fetch assignable users - enter`, GitHubRepository.ID); - const { query, remote, schema } = await this.ensure(); + const { query, remote, schema, octokit } = await this.ensure(); let after: string | null = null; let hasNextPage = false; @@ -967,11 +1019,17 @@ export class GitHubRepository implements vscode.Disposable { }, }); + if (remote.isEnterprise) { + await Promise.all( + result.data.repository.assignableUsers.nodes.map(node => replaceAvatarUrl(node, octokit)), + ); + } + ret.push( ...result.data.repository.assignableUsers.nodes.map(node => { return { login: node.login, - avatarUrl: getAvatarWithEnterpriseFallback(node.avatarUrl, node.email, this.remote.authProviderId), + avatarUrl: node.avatarUrl, name: node.name, url: node.url, email: node.email, @@ -1037,7 +1095,7 @@ export class GitHubRepository implements vscode.Disposable { return []; } - const { query, remote, schema } = await this.ensureAdditionalScopes(); + const { query, remote, schema, octokit } = await this.ensureAdditionalScopes(); let after: string | null = null; let hasNextPage = false; @@ -1054,18 +1112,21 @@ export class GitHubRepository implements vscode.Disposable { }, }); - result.data.organization.teams.nodes.forEach(node => { - const team: ITeam = { - avatarUrl: getAvatarWithEnterpriseFallback(node.avatarUrl, undefined, this.remote.authProviderId), - name: node.name, - url: node.url, - slug: node.slug, - id: node.id, - org: remote.owner - }; - orgTeams.push({ ...team, repositoryNames: node.repositories.nodes.map(repo => repo.name) }); - }); + const teamsWithRepositoryNames = result.data.organization.teams.nodes.map(node => ({ + avatarUrl: node.avatarUrl, + name: node.name, + url: node.url, + slug: node.slug, + id: node.id, + org: remote.owner, + repositoryNames: node.repositories.nodes.map(repo => repo.name), + })); + + if (remote.isEnterprise) { + await Promise.all(teamsWithRepositoryNames.map(team => replaceAvatarUrl(team, octokit))); + } + orgTeams.push(...teamsWithRepositoryNames); hasNextPage = result.data.organization.teams.pageInfo.hasNextPage; after = result.data.organization.teams.pageInfo.endCursor; } catch (e) { @@ -1089,7 +1150,7 @@ export class GitHubRepository implements vscode.Disposable { async getPullRequestParticipants(pullRequestNumber: number): Promise { Logger.debug(`Fetch participants from a Pull Request`, GitHubRepository.ID); - const { query, remote, schema } = await this.ensure(); + const { query, remote, schema, octokit } = await this.ensure(); const ret: IAccount[] = []; @@ -1104,11 +1165,17 @@ export class GitHubRepository implements vscode.Disposable { }, }); + if (remote.isEnterprise) { + await Promise.all( + result.data.repository.pullRequest.participants.nodes.map(node => replaceAvatarUrl(node, octokit)), + ); + } + ret.push( ...result.data.repository.pullRequest.participants.nodes.map(node => { return { login: node.login, - avatarUrl: getAvatarWithEnterpriseFallback(node.avatarUrl, node.email, this.remote.authProviderId), + avatarUrl: node.avatarUrl, name: node.name, url: node.url, email: node.email, @@ -1163,7 +1230,7 @@ export class GitHubRepository implements vscode.Disposable { */ private _useFallbackChecks: boolean = false; async getStatusChecks(number: number): Promise<[PullRequestChecks | null, PullRequestReviewRequirement | null]> { - const { query, remote, schema } = await this.ensure(); + const { query, remote, schema, octokit } = await this.ensure(); const captureUseFallbackChecks = this._useFallbackChecks; let result: ApolloQueryResult; try { @@ -1202,13 +1269,7 @@ export class GitHubRepository implements vscode.Disposable { return { id: context.id, url: context.checkSuite?.app?.url, - avatarUrl: - context.checkSuite?.app?.logoUrl && - getAvatarWithEnterpriseFallback( - context.checkSuite.app.logoUrl, - undefined, - this.remote.authProviderId, - ), + avatarUrl: context.checkSuite?.app?.logoUrl, state: this.mapStateAsCheckState(context.conclusion), description: context.title, context: context.name, @@ -1219,9 +1280,7 @@ export class GitHubRepository implements vscode.Disposable { return { id: context.id, url: context.targetUrl ?? undefined, - avatarUrl: context.avatarUrl - ? getAvatarWithEnterpriseFallback(context.avatarUrl, context.creator?.email, this.remote.authProviderId) - : undefined, + avatarUrl: context.avatarUrl ?? undefined, state: this.mapStateAsCheckState(context.state), description: context.description, context: context.context, @@ -1278,6 +1337,19 @@ export class GitHubRepository implements vscode.Disposable { } } + if (remote.isEnterprise) { + await Promise.all(checks.statuses.map(async status => { + const user: IAccount = { + login: '', + url: status.url || '', + email: status.creator?.email, + avatarUrl: status.avatarUrl, + }; + await replaceAvatarUrl(user, octokit); + status.avatarUrl = user.avatarUrl; + })); + } + return [checks.statuses.length ? checks : null, reviewRequirement]; } diff --git a/src/github/graphql.ts b/src/github/graphql.ts index 43a528a437..caf5b6c8ad 100644 --- a/src/github/graphql.ts +++ b/src/github/graphql.ts @@ -693,9 +693,9 @@ export interface UserResponse { login: string; avatarUrl: string; email: string; - bio: string; - company: string; - location: string; + bio: string | null; + company: string | null; + location: string | null; name: string; contributionsCollection: ContributionsCollection; url: string; diff --git a/src/github/interface.ts b/src/github/interface.ts index 90633d9fb8..f0388c41f7 100644 --- a/src/github/interface.ts +++ b/src/github/interface.ts @@ -176,9 +176,9 @@ export type RepoAccessAndMergeMethods = { }; export interface User extends IAccount { - company?: string; - location?: string; - bio?: string; + company: string | null; + location: string | null; + bio: string | null; commitContributions: { createdAt: Date; repoNameWithOwner: string; @@ -202,6 +202,9 @@ export interface PullRequestCheckStatus { targetUrl: string | null; context: string; isRequired: boolean; + creator?: { + email?: string; + }; } export interface PullRequestChecks { diff --git a/src/github/issueModel.ts b/src/github/issueModel.ts index 54705027f8..b8ab96aa47 100644 --- a/src/github/issueModel.ts +++ b/src/github/issueModel.ts @@ -18,7 +18,7 @@ import { UpdatePullRequestResponse, } from './graphql'; import { GithubItemStateEnum, IAccount, IMilestone, IPullRequestEditData, Issue } from './interface'; -import { getAvatarWithEnterpriseFallback, parseGraphQlIssueComment, parseGraphQLTimelineEvents } from './utils'; +import { parseGraphQlIssueComment, parseGraphQLTimelineEvents, replaceAvatarUrl, replaceTimelineEventAvatarUrls } from './utils'; export class IssueModel { static ID = 'IssueModel'; @@ -71,7 +71,7 @@ export class IssueModel { public get userAvatar(): string | undefined { if (this.item) { - return getAvatarWithEnterpriseFallback(this.item.user.avatarUrl, this.item.user.email, this.githubRepository.remote.authProviderId); + return this.item.user.avatarUrl; } return undefined; @@ -81,7 +81,7 @@ export class IssueModel { if (this.item) { const key = this.userAvatar; if (key) { - const uri = vscode.Uri.parse(`${key}&s=${64}`); + const uri = vscode.Uri.parse(key); // hack, to ensure queries are not wrongly encoded. const originalToStringFn = uri.toString; @@ -201,7 +201,7 @@ export class IssueModel { } async createIssueComment(text: string): Promise { - const { mutate, schema } = await this.githubRepository.ensure(); + const { mutate, schema, remote, octokit } = await this.githubRepository.ensure(); const { data } = await mutate({ mutation: schema.AddIssueComment, variables: { @@ -212,12 +212,18 @@ export class IssueModel { }, }); - return parseGraphQlIssueComment(data!.addComment.commentEdge.node, this.githubRepository); + const comment = parseGraphQlIssueComment(data!.addComment.commentEdge.node); + + if (remote.isEnterprise && comment.user) { + await replaceAvatarUrl(comment.user, octokit); + } + + return comment; } async editIssueComment(comment: IComment, text: string): Promise { try { - const { mutate, schema } = await this.githubRepository.ensure(); + const { mutate, schema, remote, octokit } = await this.githubRepository.ensure(); const { data } = await mutate({ mutation: schema.EditIssueComment, @@ -229,7 +235,13 @@ export class IssueModel { }, }); - return parseGraphQlIssueComment(data!.updateIssueComment.issueComment, this.githubRepository); + const newComment = parseGraphQlIssueComment(data!.updateIssueComment.issueComment); + + if (remote.isEnterprise && newComment.user) { + await replaceAvatarUrl(newComment.user, octokit); + } + + return newComment; } catch (e) { throw new Error(formatError(e)); } @@ -279,7 +291,7 @@ export class IssueModel { async getIssueTimelineEvents(): Promise { Logger.debug(`Fetch timeline events of issue #${this.number} - enter`, IssueModel.ID); const githubRepository = this.githubRepository; - const { query, remote, schema } = await githubRepository.ensure(); + const { query, remote, schema, octokit } = await githubRepository.ensure(); try { const { data } = await query({ @@ -291,7 +303,11 @@ export class IssueModel { }, }); const ret = data.repository.pullRequest.timelineItems.nodes; - const events = parseGraphQLTimelineEvents(ret, githubRepository); + const events = parseGraphQLTimelineEvents(ret); + + if (remote.isEnterprise) { + await replaceTimelineEventAvatarUrls(events, octokit); + } return events; } catch (e) { @@ -299,6 +315,4 @@ export class IssueModel { return []; } } - - } diff --git a/src/github/issueOverview.ts b/src/github/issueOverview.ts index ad2fc573eb..bf0d2c8692 100644 --- a/src/github/issueOverview.ts +++ b/src/github/issueOverview.ts @@ -149,7 +149,7 @@ export class IssueOverviewPanel extends W author: { login: this._item.author.login, name: this._item.author.name, - avatarUrl: this._item.userAvatar, + avatarUrl: this._item.author.avatarUrl, url: this._item.author.url, email: this._item.author.email, }, @@ -398,7 +398,7 @@ export class IssueOverviewPanel extends W - + Pull Request #${number} diff --git a/src/github/pullRequestModel.ts b/src/github/pullRequestModel.ts index 621b0726e9..6fef82ed3a 100644 --- a/src/github/pullRequestModel.ts +++ b/src/github/pullRequestModel.ts @@ -61,7 +61,6 @@ import { IssueModel } from './issueModel'; import { convertRESTPullRequestToRawPullRequest, convertRESTReviewEvent, - getAvatarWithEnterpriseFallback, getReactionGroup, insertNewCommitsSinceReview, parseGraphQLComment, @@ -70,6 +69,9 @@ import { parseGraphQLReviewThread, parseGraphQLTimelineEvents, parseMergeability, + replaceAccountAvatarUrls, + replaceAvatarUrl, + replaceTimelineEventAvatarUrls, restPaginate, } from './utils'; @@ -355,7 +357,13 @@ export class PullRequestModel extends IssueModel implements IPullRe */ this._telemetry.sendTelemetryEvent('pr.close'); - return convertRESTPullRequestToRawPullRequest(ret.data, this.githubRepository); + const pr = convertRESTPullRequestToRawPullRequest(ret.data); + + if (remote.isEnterprise) { + await replaceAccountAvatarUrls(pr, octokit); + } + + return pr; } /** @@ -374,7 +382,13 @@ export class PullRequestModel extends IssueModel implements IPullRe body: message, }); - return convertRESTReviewEvent(data, this.githubRepository); + const reviewEvent = convertRESTReviewEvent(data); + + if (remote.isEnterprise) { + await replaceAvatarUrl(reviewEvent.user, octokit); + } + + return reviewEvent; } /** @@ -384,7 +398,7 @@ export class PullRequestModel extends IssueModel implements IPullRe */ async submitReview(event?: ReviewEvent, body?: string): Promise { let pendingReviewId = await this.getPendingReviewId(); - const { mutate, schema } = await this.githubRepository.ensure(); + const { mutate, schema, remote, octokit } = await this.githubRepository.ensure(); if (!pendingReviewId && (event === ReviewEvent.Comment)) { // Create a new review so that we can comment on it. @@ -403,7 +417,16 @@ export class PullRequestModel extends IssueModel implements IPullRe this.hasPendingReview = false; await this.updateDraftModeContext(); - const reviewEvent = parseGraphQLReviewEvent(data!.submitPullRequestReview.pullRequestReview, this.githubRepository); + const reviewEvent = parseGraphQLReviewEvent(data!.submitPullRequestReview.pullRequestReview); + + if (remote.isEnterprise) { + await Promise.all([ + replaceAvatarUrl(reviewEvent.user, octokit), + ...reviewEvent.comments.map(comment => + Promise.resolve(comment.user && replaceAvatarUrl(comment.user, octokit)), + ), + ]); + } const threadWithComment = this._reviewThreadsCache.find(thread => thread.comments.length ? (thread.comments[0].pullRequestReviewId === reviewEvent.id) : undefined, @@ -496,7 +519,7 @@ export class PullRequestModel extends IssueModel implements IPullRe */ async deleteReview(): Promise<{ deletedReviewId: number; deletedReviewComments: IComment[] }> { const pendingReviewId = await this.getPendingReviewId(); - const { mutate, schema } = await this.githubRepository.ensure(); + const { mutate, schema, remote, octokit } = await this.githubRepository.ensure(); const { data } = await mutate({ mutation: schema.DeleteReview, variables: { @@ -511,9 +534,15 @@ export class PullRequestModel extends IssueModel implements IPullRe this.getReviewThreads(); + const reviewComments = comments.nodes.map(comment => parseGraphQLComment(comment, false)); + + if (remote.isEnterprise) { + await Promise.all(reviewComments.map(comment => Promise.resolve(comment.user && replaceAvatarUrl(comment.user, octokit)))); + } + return { deletedReviewId: databaseId, - deletedReviewComments: comments.nodes.map(comment => parseGraphQLComment(comment, false, this.githubRepository)), + deletedReviewComments: reviewComments, }; } @@ -569,7 +598,7 @@ export class PullRequestModel extends IssueModel implements IPullRe } const pendingReviewId = await this.getPendingReviewId(); - const { mutate, schema } = await this.githubRepository.ensure(); + const { mutate, schema, remote, octokit } = await this.githubRepository.ensure(); const { data } = await mutate({ mutation: schema.AddReviewThread, variables: { @@ -600,7 +629,12 @@ export class PullRequestModel extends IssueModel implements IPullRe } const thread = data.addPullRequestReviewThread.thread; - const newThread = parseGraphQLReviewThread(thread, this.githubRepository); + const newThread = parseGraphQLReviewThread(thread); + + if (remote.isEnterprise) { + await Promise.all(newThread.comments.map(comment => Promise.resolve(comment.user && replaceAvatarUrl(comment.user, octokit)))); + } + this._reviewThreadsCache.push(newThread); this._onDidChangeReviewThreads.fire({ added: [newThread], changed: [], removed: [] }); return newThread; @@ -630,7 +664,7 @@ export class PullRequestModel extends IssueModel implements IPullRe pendingReviewId = await this.startReview(commitId); } - const { mutate, schema } = await this.githubRepository.ensure(); + const { mutate, schema, remote, octokit } = await this.githubRepository.ensure(); const { data } = await mutate({ mutation: schema.AddComment, variables: { @@ -648,7 +682,11 @@ export class PullRequestModel extends IssueModel implements IPullRe } const { comment } = data.addPullRequestReviewComment; - const newComment = parseGraphQLComment(comment, false, this.githubRepository); + const newComment = parseGraphQLComment(comment, false); + + if (remote.isEnterprise && newComment.user) { + await replaceAvatarUrl(newComment.user, octokit); + } if (isSingleComment) { newComment.isDraft = false; @@ -691,7 +729,7 @@ export class PullRequestModel extends IssueModel implements IPullRe * @param text The new comment text */ async editReviewComment(comment: IComment, text: string): Promise { - const { mutate, schema } = await this.githubRepository.ensure(); + const { mutate, schema, remote, octokit } = await this.githubRepository.ensure(); let threadWithComment = this._reviewThreadsCache.find(thread => thread.comments.some(c => c.graphNodeId === comment.graphNodeId), ); @@ -717,8 +755,12 @@ export class PullRequestModel extends IssueModel implements IPullRe const newComment = parseGraphQLComment( data.updatePullRequestReviewComment.pullRequestReviewComment, !!comment.isResolved, - this.githubRepository ); + + if (remote.isEnterprise && newComment.user) { + await replaceAvatarUrl(newComment.user, octokit); + } + if (threadWithComment) { const index = threadWithComment.comments.findIndex(c => c.graphNodeId === comment.graphNodeId); threadWithComment.comments.splice(index, 1, newComment); @@ -769,7 +811,7 @@ export class PullRequestModel extends IssueModel implements IPullRe */ async getReviewRequests(): Promise<(IAccount | ITeam)[]> { const githubRepository = this.githubRepository; - const { remote, query, schema } = await githubRepository.ensure(); + const { remote, query, schema, octokit } = await githubRepository.ensure(); const { data } = await query({ query: this.credentialStore.isAuthenticatedWithAdditionalScopes(githubRepository.remote.authProviderId) ? schema.GetReviewRequestsAdditionalScopes : schema.GetReviewRequests, @@ -786,7 +828,7 @@ export class PullRequestModel extends IssueModel implements IPullRe const account: IAccount = { login: reviewer.requestedReviewer.login, url: reviewer.requestedReviewer.url, - avatarUrl: getAvatarWithEnterpriseFallback(reviewer.requestedReviewer.avatarUrl, reviewer.requestedReviewer.email, remote.authProviderId), + avatarUrl: reviewer.requestedReviewer.avatarUrl, email: reviewer.requestedReviewer.email, name: reviewer.requestedReviewer.name }; @@ -795,7 +837,7 @@ export class PullRequestModel extends IssueModel implements IPullRe const team: ITeam = { name: reviewer.requestedReviewer.name, url: reviewer.requestedReviewer.url, - avatarUrl: getAvatarWithEnterpriseFallback(reviewer.requestedReviewer.avatarUrl, undefined, remote.authProviderId), + avatarUrl: reviewer.requestedReviewer.avatarUrl, id: reviewer.requestedReviewer.id!, org: remote.owner, slug: reviewer.requestedReviewer.slug! @@ -803,6 +845,11 @@ export class PullRequestModel extends IssueModel implements IPullRe reviewers.push(team); } } + + if (remote.isEnterprise) { + await Promise.all(reviewers.map(reviewer => replaceAvatarUrl(reviewer, octokit))); + } + return reviewers; } @@ -884,7 +931,7 @@ export class PullRequestModel extends IssueModel implements IPullRe } async getReviewThreads(): Promise { - const { remote, query, schema } = await this.githubRepository.ensure(); + const { remote, query, schema, octokit } = await this.githubRepository.ensure(); try { const { data } = await query({ query: schema.PullRequestComments, @@ -895,9 +942,21 @@ export class PullRequestModel extends IssueModel implements IPullRe }, }); - const reviewThreads = data.repository.pullRequest.reviewThreads.nodes.map(node => { - return parseGraphQLReviewThread(node, this.githubRepository); - }); + const reviewThreads = data.repository.pullRequest.reviewThreads.nodes.map(node => + parseGraphQLReviewThread(node), + ); + + if (remote.isEnterprise) { + await Promise.all( + reviewThreads + .map(thread => + thread.comments.map(comment => + Promise.resolve(comment.user && replaceAvatarUrl(comment.user, octokit)), + ), + ) + .flat(), + ); + } const oldReviewThreads = this._reviewThreadsCache; this._reviewThreadsCache = reviewThreads; @@ -913,7 +972,7 @@ export class PullRequestModel extends IssueModel implements IPullRe * Get all review comments. */ async initializeReviewComments(): Promise { - const { remote, query, schema } = await this.githubRepository.ensure(); + const { remote, query, schema, octokit } = await this.githubRepository.ensure(); try { const { data } = await query({ query: schema.PullRequestComments, @@ -925,12 +984,16 @@ export class PullRequestModel extends IssueModel implements IPullRe }); const comments = data.repository.pullRequest.reviewThreads.nodes - .map(node => node.comments.nodes.map(comment => parseGraphQLComment(comment, node.isResolved, this.githubRepository), remote)) + .map(node => node.comments.nodes.map(comment => parseGraphQLComment(comment, node.isResolved), remote)) .reduce((prev, curr) => prev.concat(curr), []) .sort((a: IComment, b: IComment) => { return a.createdAt > b.createdAt ? 1 : -1; }); + if (remote.isEnterprise) { + await Promise.all(comments.map(comment => comment.user && replaceAvatarUrl(comment.user, octokit))); + } + this.comments = comments; } catch (e) { Logger.error(`Failed to get pull request review comments: ${e}`, PullRequestModel.ID); @@ -1016,7 +1079,7 @@ export class PullRequestModel extends IssueModel implements IPullRe */ async getTimelineEvents(): Promise { Logger.debug(`Fetch timeline events of PR #${this.number} - enter`, PullRequestModel.ID); - const { query, remote, schema } = await this.githubRepository.ensure(); + const { query, remote, schema, octokit } = await this.githubRepository.ensure(); try { const [{ data }, latestReviewCommitInfo, currentUser, reviewThreads] = await Promise.all([ @@ -1034,7 +1097,11 @@ export class PullRequestModel extends IssueModel implements IPullRe ]); const ret = data.repository.pullRequest.timelineItems.nodes; - const events = parseGraphQLTimelineEvents(ret, this.githubRepository); + const events = parseGraphQLTimelineEvents(ret); + + if (remote.isEnterprise) { + await replaceTimelineEventAvatarUrls(events, octokit); + } this.addReviewTimelineEventComments(events, reviewThreads); insertNewCommitsSinceReview(events, latestReviewCommitInfo?.sha, currentUser, this.head); @@ -1245,7 +1312,14 @@ export class PullRequestModel extends IssueModel implements IPullRe repo: remote.repositoryName, pull_number: this.number, }); - this.update(await convertRESTPullRequestToRawPullRequest(info.data, githubRepository)); + + const pr = convertRESTPullRequestToRawPullRequest(info.data); + + if (remote.isEnterprise) { + await replaceAccountAvatarUrls(pr, octokit); + } + + this.update(pr); } let compareWithBaseRef = this.base.sha; @@ -1448,7 +1522,7 @@ export class PullRequestModel extends IssueModel implements IPullRe } async resolveReviewThread(threadId: string): Promise { - const { mutate, schema } = await this.githubRepository.ensure(); + const { mutate, schema, remote, octokit } = await this.githubRepository.ensure(); // optimistically update const oldThread = this._reviewThreadsCache.find(thread => thread.id === threadId); @@ -1481,14 +1555,23 @@ export class PullRequestModel extends IssueModel implements IPullRe const index = this._reviewThreadsCache.findIndex(thread => thread.id === threadId); if (index > -1) { - const thread = parseGraphQLReviewThread(data.resolveReviewThread.thread, this.githubRepository); + const thread = parseGraphQLReviewThread(data.resolveReviewThread.thread); + + if (remote.isEnterprise) { + await Promise.all( + thread.comments.map(comment => + Promise.resolve(comment.user && replaceAvatarUrl(comment.user, octokit)), + ), + ); + } + this._reviewThreadsCache.splice(index, 1, thread); this._onDidChangeReviewThreads.fire({ added: [], changed: [thread], removed: [] }); } } async unresolveReviewThread(threadId: string): Promise { - const { mutate, schema } = await this.githubRepository.ensure(); + const { mutate, schema, remote, octokit } = await this.githubRepository.ensure(); // optimistically update const oldThread = this._reviewThreadsCache.find(thread => thread.id === threadId); @@ -1521,7 +1604,16 @@ export class PullRequestModel extends IssueModel implements IPullRe const index = this._reviewThreadsCache.findIndex(thread => thread.id === threadId); if (index > -1) { - const thread = parseGraphQLReviewThread(data.unresolveReviewThread.thread, this.githubRepository); + const thread = parseGraphQLReviewThread(data.unresolveReviewThread.thread); + + if (remote.isEnterprise) { + await Promise.all( + thread.comments.map(comment => + Promise.resolve(comment.user && replaceAvatarUrl(comment.user, octokit)), + ), + ); + } + this._reviewThreadsCache.splice(index, 1, thread); this._onDidChangeReviewThreads.fire({ added: [], changed: [thread], removed: [] }); } diff --git a/src/github/pullRequestOverview.ts b/src/github/pullRequestOverview.ts index d8f5ab8edd..08f784cd99 100644 --- a/src/github/pullRequestOverview.ts +++ b/src/github/pullRequestOverview.ts @@ -236,7 +236,7 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel { - let email = user.email; - if (githubRepository.remote.authProviderId === AuthProvider['github-enterprise'] && email === undefined) { - const { data } = await githubRepository.octokit.call(githubRepository.octokit.api.users.getByUsername, { - username: user.login - }); - - email = data.email; - } - - email ||= undefined; +export function isEnterprise(provider: AuthProvider): boolean { + return provider === AuthProvider['github-enterprise']; +} +export function convertRESTUserToAccount(user: OctokitCommon.PullsListResponseItemUser): IAccount { return { login: user.login, url: user.html_url, - avatarUrl: getAvatarWithEnterpriseFallback(user.avatar_url, email, githubRepository.remote.authProviderId), - email: email, + avatarUrl: user.avatar_url, + email: user.email === null ? '' : user.email, }; } @@ -273,12 +265,9 @@ export function convertRESTHeadToIGitHubRef(head: OctokitCommon.PullsListRespons }; } -export async function convertRESTPullRequestToRawPullRequest( - pullRequest: - | OctokitCommon.PullsGetResponseData - | OctokitCommon.PullsListResponseItem, - githubRepository: GitHubRepository, -): Promise { +export function convertRESTPullRequestToRawPullRequest( + pullRequest: OctokitCommon.PullsGetResponseData | OctokitCommon.PullsListResponseItem, +): PullRequest { const { number, body, @@ -305,12 +294,10 @@ export async function convertRESTPullRequestToRawPullRequest( title, titleHTML: title, url: html_url, - user: await convertRESTUserToAccount(user!, githubRepository), + user: convertRESTUserToAccount(user!), state, merged: (pullRequest as OctokitCommon.PullsGetResponseData).merged || false, - assignees: assignees - ? await Promise.all(assignees.map(assignee => convertRESTUserToAccount(assignee!, githubRepository))) - : undefined, + assignees: assignees?.map(assignee => convertRESTUserToAccount(assignee!)), createdAt: created_at, updatedAt: updated_at, head: head.repo ? convertRESTHeadToIGitHubRef(head as OctokitCommon.PullsListResponseItemHead) : undefined, @@ -330,10 +317,7 @@ export async function convertRESTPullRequestToRawPullRequest( return item; } -export async function convertRESTIssueToRawPullRequest( - pullRequest: OctokitCommon.IssuesCreateResponseData, - githubRepository: GitHubRepository, -): Promise { +export function convertRESTIssueToRawPullRequest(pullRequest: OctokitCommon.IssuesCreateResponseData): PullRequest { const { number, body, @@ -357,11 +341,9 @@ export async function convertRESTIssueToRawPullRequest( title, titleHTML: title, url: html_url, - user: await convertRESTUserToAccount(user!, githubRepository), + user: convertRESTUserToAccount(user!), state, - assignees: assignees - ? await Promise.all(assignees.map(assignee => convertRESTUserToAccount(assignee!, githubRepository))) - : undefined, + assignees: assignees?.map(assignee => convertRESTUserToAccount(assignee!)), createdAt: created_at, updatedAt: updated_at, labels: labels.map(l => @@ -373,18 +355,15 @@ export async function convertRESTIssueToRawPullRequest( return item; } -export async function convertRESTReviewEvent( - review: OctokitCommon.PullsCreateReviewResponseData, - githubRepository: GitHubRepository, -): Promise { +export function convertRESTReviewEvent(review: OctokitCommon.PullsCreateReviewResponseData): Common.ReviewEvent { return { event: Common.EventType.Reviewed, comments: [], - submittedAt: (review as any).submitted_at, // TODO fix typings upstream + submittedAt: review.submitted_at || '', body: review.body, bodyHTML: review.body, htmlUrl: review.html_url, - user: await convertRESTUserToAccount(review.user!, githubRepository), + user: convertRESTUserToAccount(review.user!), authorAssociation: review.user!.type, state: review.state as 'COMMENTED' | 'APPROVED' | 'CHANGES_REQUESTED' | 'PENDING', id: review.id, @@ -429,7 +408,7 @@ export function convertGraphQLEventType(text: string) { } } -export function parseGraphQLReviewThread(thread: GraphQL.ReviewThread, githubRepository: GitHubRepository): IReviewThread { +export function parseGraphQLReviewThread(thread: GraphQL.ReviewThread): IReviewThread { return { id: thread.id, prReviewDatabaseId: thread.comments.edges && thread.comments.edges.length ? @@ -445,12 +424,12 @@ export function parseGraphQLReviewThread(thread: GraphQL.ReviewThread, githubRep originalEndLine: thread.originalLine, diffSide: thread.diffSide, isOutdated: thread.isOutdated, - comments: thread.comments.nodes.map(comment => parseGraphQLComment(comment, thread.isResolved, githubRepository)), + comments: thread.comments.nodes.map(comment => parseGraphQLComment(comment, thread.isResolved)), subjectType: thread.subjectType }; } -export function parseGraphQLComment(comment: GraphQL.ReviewComment, isResolved: boolean, githubRepository: GitHubRepository): IComment { +export function parseGraphQLComment(comment: GraphQL.ReviewComment, isResolved: boolean): IComment { const c: IComment = { id: comment.databaseId, url: comment.url, @@ -465,7 +444,7 @@ export function parseGraphQLComment(comment: GraphQL.ReviewComment, isResolved: commitId: comment.commit.oid, originalPosition: comment.originalPosition, originalCommitId: comment.originalCommit && comment.originalCommit.oid, - user: comment.author ? parseAuthor(comment.author, githubRepository) : undefined, + user: comment.author ? parseAuthor(comment.author) : undefined, createdAt: comment.createdAt, htmlUrl: comment.url, graphNodeId: comment.id, @@ -481,7 +460,7 @@ export function parseGraphQLComment(comment: GraphQL.ReviewComment, isResolved: return c; } -export function parseGraphQlIssueComment(comment: GraphQL.IssueComment, githubRepository: GitHubRepository): IComment { +export function parseGraphQlIssueComment(comment: GraphQL.IssueComment): IComment { return { id: comment.databaseId, url: comment.url, @@ -489,7 +468,7 @@ export function parseGraphQlIssueComment(comment: GraphQL.IssueComment, githubRe bodyHTML: comment.bodyHTML, canEdit: comment.viewerCanDelete, canDelete: comment.viewerCanDelete, - user: parseAuthor(comment.author, githubRepository), + user: parseAuthor(comment.author), createdAt: comment.createdAt, htmlUrl: comment.url, graphNodeId: comment.id, @@ -537,16 +516,13 @@ function parseRef(refName: string, oid: string, repository?: GraphQL.RefReposito }; } -function parseAuthor( - author: { login: string; url: string; avatarUrl: string; email?: string } | null, - githubRepository: GitHubRepository, -): IAccount { +function parseAuthor(author: IAccount | null): IAccount { if (author) { return { login: author.login, url: author.url, - avatarUrl: getAvatarWithEnterpriseFallback(author.avatarUrl, author.email, githubRepository.remote.authProviderId), - email: author.email + avatarUrl: author.avatarUrl, + email: author.email, }; } else { return { @@ -602,10 +578,7 @@ export function parseMergeability(mergeability: 'UNKNOWN' | 'MERGEABLE' | 'CONFL return parsed; } -export function parseGraphQLPullRequest( - graphQLPullRequest: GraphQL.PullRequest, - githubRepository: GitHubRepository, -): PullRequest { +export function parseGraphQLPullRequest(graphQLPullRequest: GraphQL.PullRequest): PullRequest { return { id: graphQLPullRequest.databaseId, graphNodeId: graphQLPullRequest.id, @@ -619,10 +592,18 @@ export function parseGraphQLPullRequest( createdAt: graphQLPullRequest.createdAt, updatedAt: graphQLPullRequest.updatedAt, isRemoteHeadDeleted: !graphQLPullRequest.headRef, - head: parseRef(graphQLPullRequest.headRef?.name ?? graphQLPullRequest.headRefName, graphQLPullRequest.headRefOid, graphQLPullRequest.headRepository), + head: parseRef( + graphQLPullRequest.headRef?.name ?? graphQLPullRequest.headRefName, + graphQLPullRequest.headRefOid, + graphQLPullRequest.headRepository, + ), isRemoteBaseDeleted: !graphQLPullRequest.baseRef, - base: parseRef(graphQLPullRequest.baseRef?.name ?? graphQLPullRequest.baseRefName, graphQLPullRequest.baseRefOid, graphQLPullRequest.baseRepository), - user: parseAuthor(graphQLPullRequest.author, githubRepository), + base: parseRef( + graphQLPullRequest.baseRef?.name ?? graphQLPullRequest.baseRefName, + graphQLPullRequest.baseRefOid, + graphQLPullRequest.baseRepository, + ), + user: parseAuthor(graphQLPullRequest.author), merged: graphQLPullRequest.merged, mergeable: parseMergeability(graphQLPullRequest.mergeable, graphQLPullRequest.mergeStateStatus), autoMerge: !!graphQLPullRequest.autoMergeRequest, @@ -630,14 +611,14 @@ export function parseGraphQLPullRequest( allowAutoMerge: graphQLPullRequest.viewerCanEnableAutoMerge || graphQLPullRequest.viewerCanDisableAutoMerge, labels: graphQLPullRequest.labels.nodes, isDraft: graphQLPullRequest.isDraft, - suggestedReviewers: parseSuggestedReviewers(graphQLPullRequest.suggestedReviewers, githubRepository), - comments: parseComments(graphQLPullRequest.comments?.nodes, githubRepository), + suggestedReviewers: parseSuggestedReviewers(graphQLPullRequest.suggestedReviewers), + comments: parseComments(graphQLPullRequest.comments?.nodes), milestone: parseMilestone(graphQLPullRequest.milestone), - assignees: graphQLPullRequest.assignees?.nodes.map(assignee => parseAuthor(assignee, githubRepository)), + assignees: graphQLPullRequest.assignees?.nodes.map(assignee => parseAuthor(assignee)), }; } -function parseComments(comments: GraphQL.AbbreviatedIssueComment[] | undefined, githubRepository: GitHubRepository) { +function parseComments(comments: GraphQL.AbbreviatedIssueComment[] | undefined) { if (!comments) { return; } @@ -648,7 +629,7 @@ function parseComments(comments: GraphQL.AbbreviatedIssueComment[] | undefined, }[] = []; for (const comment of comments) { parsedComments.push({ - author: parseAuthor(comment.author, githubRepository), + author: parseAuthor(comment.author), body: comment.body, databaseId: comment.databaseId, }); @@ -657,7 +638,7 @@ function parseComments(comments: GraphQL.AbbreviatedIssueComment[] | undefined, return parsedComments; } -export function parseGraphQLIssue(issue: GraphQL.PullRequest, githubRepository: GitHubRepository): Issue { +export function parseGraphQLIssue(issue: GraphQL.PullRequest): Issue { return { id: issue.databaseId, graphNodeId: issue.id, @@ -670,24 +651,23 @@ export function parseGraphQLIssue(issue: GraphQL.PullRequest, githubRepository: titleHTML: issue.titleHTML, createdAt: issue.createdAt, updatedAt: issue.updatedAt, - assignees: issue.assignees?.nodes.map(assignee => parseAuthor(assignee, githubRepository)), - user: parseAuthor(issue.author, githubRepository), + assignees: issue.assignees?.nodes.map(assignee => parseAuthor(assignee)), + user: parseAuthor(issue.author), labels: issue.labels.nodes, - repositoryName: issue.repository?.name ?? githubRepository.remote.repositoryName, - repositoryOwner: issue.repository?.owner.login ?? githubRepository.remote.owner, - repositoryUrl: issue.repository?.url ?? githubRepository.remote.url, + repositoryName: issue.repository?.name, + repositoryOwner: issue.repository?.owner.login, + repositoryUrl: issue.repository?.url, }; } function parseSuggestedReviewers( suggestedReviewers: GraphQL.SuggestedReviewerResponse[] | undefined, - githubRepository: GitHubRepository ): ISuggestedReviewer[] { if (!suggestedReviewers) { return []; } const ret: ISuggestedReviewer[] = suggestedReviewers.map(suggestedReviewer => { - const user: IAccount = parseAuthor(suggestedReviewer.reviewer, githubRepository); + const user: IAccount = parseAuthor(suggestedReviewer.reviewer); return { ...user, @@ -714,18 +694,15 @@ export function teamComparator(a: ITeam, b: ITeam) { return a.name.localeCompare(b.name, 'en', { sensitivity: 'accent' }); } -export function parseGraphQLReviewEvent( - review: GraphQL.SubmittedReview, - githubRepository: GitHubRepository, -): Common.ReviewEvent { +export function parseGraphQLReviewEvent(review: GraphQL.SubmittedReview): Common.ReviewEvent { return { event: Common.EventType.Reviewed, - comments: review.comments.nodes.map(comment => parseGraphQLComment(comment, false, githubRepository)).filter(c => !c.inReplyToId), + comments: review.comments.nodes.map(comment => parseGraphQLComment(comment, false)).filter(c => !c.inReplyToId), submittedAt: review.submittedAt, body: review.body, bodyHTML: review.bodyHTML, htmlUrl: review.url, - user: parseAuthor(review.author, githubRepository), + user: parseAuthor(review.author), authorAssociation: review.authorAssociation, state: review.state, id: review.databaseId, @@ -741,10 +718,9 @@ export function parseGraphQLTimelineEvents( | GraphQL.AssignedEvent | GraphQL.HeadRefDeletedEvent )[], - githubRepository: GitHubRepository, ): Common.TimelineEvent[] { const normalizedEvents: Common.TimelineEvent[] = []; - events.forEach(event => { + for (const event of events) { const type = convertGraphQLEventType(event.__typename); switch (type) { @@ -754,7 +730,7 @@ export function parseGraphQLTimelineEvents( htmlUrl: commentEvent.url, body: commentEvent.body, bodyHTML: commentEvent.bodyHTML, - user: parseAuthor(commentEvent.author, githubRepository), + user: parseAuthor(commentEvent.author), event: type, canEdit: commentEvent.viewerCanUpdate, canDelete: commentEvent.viewerCanDelete, @@ -762,7 +738,7 @@ export function parseGraphQLTimelineEvents( graphNodeId: commentEvent.id, createdAt: commentEvent.createdAt, }); - return; + break; case Common.EventType.Reviewed: const reviewEvent = event as GraphQL.Review; normalizedEvents.push({ @@ -772,12 +748,12 @@ export function parseGraphQLTimelineEvents( body: reviewEvent.body, bodyHTML: reviewEvent.bodyHTML, htmlUrl: reviewEvent.url, - user: parseAuthor(reviewEvent.author, githubRepository), + user: parseAuthor(reviewEvent.author), authorAssociation: reviewEvent.authorAssociation, state: reviewEvent.state, id: reviewEvent.databaseId, }); - return; + break; case Common.EventType.Committed: const commitEv = event as GraphQL.Commit; normalizedEvents.push({ @@ -785,20 +761,24 @@ export function parseGraphQLTimelineEvents( event: type, sha: commitEv.commit.oid, author: commitEv.commit.author.user - ? parseAuthor(commitEv.commit.author.user, githubRepository) - : { login: commitEv.commit.author.name, avatarUrl: commitEv.commit.author.avatarUrl, email: commitEv.commit.author.email }, + ? parseAuthor(commitEv.commit.author.user) + : parseAuthor({ + login: commitEv.commit.author.name, + avatarUrl: commitEv.commit.author.avatarUrl, + email: commitEv.commit.author.email, + url: '', + }), htmlUrl: commitEv.url, message: commitEv.commit.message, authoredDate: new Date(commitEv.commit.authoredDate), - } as Common.CommitEvent); // TODO remove cast - return; + }); + break; case Common.EventType.Merged: const mergeEv = event as GraphQL.MergedEvent; - normalizedEvents.push({ id: mergeEv.id, event: type, - user: parseAuthor(mergeEv.actor, githubRepository), + user: parseAuthor(mergeEv.actor), createdAt: mergeEv.createdAt, mergeRef: mergeEv.mergeRef.name, sha: mergeEv.commit.oid, @@ -806,42 +786,38 @@ export function parseGraphQLTimelineEvents( url: mergeEv.url, graphNodeId: mergeEv.id, }); - return; + break; case Common.EventType.Assigned: const assignEv = event as GraphQL.AssignedEvent; - normalizedEvents.push({ id: assignEv.id, event: type, - user: parseAuthor(assignEv.user, githubRepository), - actor: parseAuthor(assignEv.actor, githubRepository), + user: parseAuthor(assignEv.user), + actor: parseAuthor(assignEv.actor), }); - return; + break; case Common.EventType.HeadRefDeleted: const deletedEv = event as GraphQL.HeadRefDeletedEvent; - normalizedEvents.push({ id: deletedEv.id, event: type, - actor: parseAuthor(deletedEv.actor, githubRepository), + actor: parseAuthor(deletedEv.actor), createdAt: deletedEv.createdAt, headRef: deletedEv.headRefName, }); - return; - default: break; } - }); + } return normalizedEvents; } -export function parseGraphQLUser(resp: GraphQL.UserResponse, githubRepository: GitHubRepository): User { +export function parseGraphQLUser(resp: GraphQL.UserResponse): User { const user = resp.user; return { login: user.login, name: user.name, - avatarUrl: getAvatarWithEnterpriseFallback(user.avatarUrl, user.email, githubRepository.remote.authProviderId), + avatarUrl: user.avatarUrl, url: user.url, bio: user.bio, company: user.company, @@ -1131,30 +1107,177 @@ export function hasEnterpriseUri(): boolean { return !!getEnterpriseUri(); } -export function generateGravatarUrl(gravatarId: string | undefined, size: number = 200): string | undefined { - if (!gravatarId) { +const GRAVATAR_STYLE_NONE = 'none'; + +function isGravatarEnabled() { + return getGravatarStyle() !== GRAVATAR_STYLE_NONE; +} + +function getGravatarStyle() { + return vscode.workspace.getConfiguration('githubPullRequests').get('defaultGravatarsStyle', GRAVATAR_STYLE_NONE); +} + +function generateGravatarUrl(gravatarId: string | undefined, size: number = 200): string | undefined { + if (!gravatarId || !isGravatarEnabled()) { return undefined; } - const none = 'none'; - let style = vscode.workspace.getConfiguration('githubPullRequests').get('defaultGravatarsStyle', none); - if (style === none) { - return undefined; + return `https://www.gravatar.com/avatar/${gravatarId}?s=${size}&d=${getGravatarStyle()}`; +} + +// This limits the concurrent promises that fetch avatars from the Enterprise REST service +const enterpriseAvatarQueue = new PQueue({concurrency: 3}); + +// This is an in-memory cache of Enterprise avatar data URIs +const enterpriseAvatarCache: {[k: string]: Promise} = {}; + +async function getEnterpriseAvatarUrl(avatarUrl: string | undefined, octokit: LoggingOctokit): Promise { + try { + if (!avatarUrl || !hasEnterpriseUri()) { + return; + } + + const avatarUri = vscode.Uri.parse(avatarUrl, true); + const enterpriseUri = getEnterpriseUri()!; + const enterpriseAvatarRestBase = '/enterprise/avatars'; + + // static asset from enterprise does not need authentication + if (avatarUri.scheme === 'data' || avatarUri.authority === `assets.${enterpriseUri.authority}`) { + return avatarUrl; + } + + // only proxy avatars from the "avatars" sub-domain of Enterprise + if (avatarUri.authority !== `avatars.${enterpriseUri.authority}`) { + return; + } + + const cacheKey = `${avatarUri.path}?${avatarUri.query}`; + const options = {}; + const qs = new URLSearchParams(avatarUri.query); + + qs.forEach((v, k) => { + options[k] = v; + }); + + if (!(cacheKey in enterpriseAvatarCache)) { + enterpriseAvatarCache[cacheKey] = enterpriseAvatarQueue.add(() => + octokit.api.request(`GET ${enterpriseAvatarRestBase}${avatarUri.path}`, options).then( + resp => { + const dataUri = `data:${resp.headers['content-type']};base64,${Buffer.from(resp.data).toString( + 'base64', + )}`; + return dataUri; + }, + () => { + delete enterpriseAvatarCache[cacheKey]; + return; + }, + ), + ); + } + + const avatarDataUri = await enterpriseAvatarCache[cacheKey]; + if (avatarDataUri) { + return avatarDataUri; + } + } catch { + // ignore } +} - return `https://www.gravatar.com/avatar/${gravatarId}?s=${size}&d=${style}`; +export async function replaceAvatarUrl(user: IAccount | ITeam, octokit: LoggingOctokit): Promise { + const origAvatarUrl = user.avatarUrl; + user.avatarUrl = undefined; + + const enterpriseAvatarUrl = await getEnterpriseAvatarUrl(origAvatarUrl, octokit); + if (enterpriseAvatarUrl) { + user.avatarUrl = enterpriseAvatarUrl; + return; + } + + if (!('login' in user)) { + return; + } + + if (user.email === undefined && user.login) { + try { + const { data } = await octokit.call(octokit.api.users.getByUsername, { + username: user.login + }); + + user.email = data.email || undefined; + } catch { + // ignore + } + } + + if (!user.email) { + return; + } + + user.avatarUrl = generateGravatarUrl(crypto.createHash('md5').update(user.email.trim().toLowerCase()).digest('hex')); } -export function getAvatarWithEnterpriseFallback(avatarUrl: string | undefined, email: string | undefined, authProviderId: AuthProvider): string | undefined { - if (authProviderId === AuthProvider.github) { - return avatarUrl; +export function replaceAccountAvatarUrls(pr: PullRequest, octokit: LoggingOctokit): Promise { + const promises: Promise[] = []; + promises.push(replaceAvatarUrl(pr.user, octokit)); + if (pr.assignees) { + promises.push(...pr.assignees.map(user => replaceAvatarUrl(user, octokit))); + } + if (pr.suggestedReviewers) { + promises.push(...pr.suggestedReviewers.map(user => replaceAvatarUrl(user, octokit))); } + return Promise.all(promises); +} - if (!email) { - return undefined; +export function replaceTimelineEventAvatarUrls(events: Common.TimelineEvent[], octokit: LoggingOctokit): Promise { + const promises: Promise[] = []; + + for (const event of events) { + const type = event.event; + switch (type) { + case Common.EventType.Commented: + const commentEvent = event as Common.CommentEvent; + promises.push(replaceAvatarUrl(commentEvent.user, octokit)); + break; + case Common.EventType.Reviewed: + const reviewEvent = event as Common.ReviewEvent; + promises.push(replaceAvatarUrl(reviewEvent.user, octokit)); + break; + case Common.EventType.Committed: + const commitEv = event as Common.CommitEvent; + promises.push(replaceAvatarUrl(commitEv.author, octokit)); + break; + case Common.EventType.Merged: + const mergeEv = event as Common.MergedEvent; + promises.push(replaceAvatarUrl(mergeEv.user, octokit)); + break; + case Common.EventType.Assigned: + const assignEv = event as Common.AssignEvent; + promises.push(replaceAvatarUrl(assignEv.user, octokit)); + promises.push(replaceAvatarUrl(assignEv.actor, octokit)); + break; + case Common.EventType.HeadRefDeleted: + const deletedEv = event as Common.HeadRefDeleteEvent; + promises.push(replaceAvatarUrl(deletedEv.actor, octokit)); + break; + } + } + + return Promise.all(promises); +} + +export function replaceIssuesAvatarUrls(issues: Issue[], octokit: LoggingOctokit): Promise { + const promises: Promise[] = []; + + for (const issue of issues) { + promises.push(replaceAvatarUrl(issue.user, octokit)); + if (issue.assignees) { + promises.push(...issue.assignees.map(user => replaceAvatarUrl(user, octokit))); + } } - return generateGravatarUrl(crypto.createHash('md5').update(email.trim().toLowerCase()).digest('hex')); + return Promise.all(promises); } export function getPullsUrl(repo: GitHubRepository) { diff --git a/src/issues/stateManager.ts b/src/issues/stateManager.ts index f1fe2842bc..a223068d0b 100644 --- a/src/issues/stateManager.ts +++ b/src/issues/stateManager.ts @@ -307,7 +307,7 @@ export class StateManager { } if (!user) { const enterpriseRemotes = parseRepositoryRemotes(folderManager.repository).filter( - remote => remote.authProviderId === AuthProvider['github-enterprise'] + remote => remote.isEnterprise ); user = await this.getCurrentUser(enterpriseRemotes.length ? AuthProvider['github-enterprise'] : AuthProvider.github); } diff --git a/src/test/github/folderRepositoryManager.test.ts b/src/test/github/folderRepositoryManager.test.ts index c8b8bd3cc0..a3919c19a9 100644 --- a/src/test/github/folderRepositoryManager.test.ts +++ b/src/test/github/folderRepositoryManager.test.ts @@ -43,7 +43,7 @@ describe('PullRequestManager', function () { }); describe('activePullRequest', function () { - it('gets and sets the active pull request', async function () { + it('gets and sets the active pull request', function () { assert.strictEqual(manager.activePullRequest, undefined); const changeFired = sinon.spy(); @@ -54,7 +54,7 @@ describe('PullRequestManager', function () { const remote = new GitHubRemote('origin', url, protocol, GitHubServerType.GitHubDotCom); const rootUri = Uri.file('C:\\users\\test\\repo'); const repository = new GitHubRepository(remote, rootUri, manager.credentialStore, telemetry); - const prItem = await convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().build(), repository); + const prItem = convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().build()); const pr = new PullRequestModel(manager.credentialStore, telemetry, repository, remote, prItem); manager.activePullRequest = pr; diff --git a/src/test/github/pullRequestGitHelper.test.ts b/src/test/github/pullRequestGitHelper.test.ts index f1f1f3968e..30a44329a0 100644 --- a/src/test/github/pullRequestGitHelper.test.ts +++ b/src/test/github/pullRequestGitHelper.test.ts @@ -50,7 +50,7 @@ describe('PullRequestGitHelper', function () { const remote = new GitHubRemote('elsewhere', url, new Protocol(url), GitHubServerType.GitHubDotCom); const gitHubRepository = new MockGitHubRepository(remote, credentialStore, telemetry, sinon); - const prItem = await convertRESTPullRequestToRawPullRequest( + const prItem = convertRESTPullRequestToRawPullRequest( new PullRequestBuilder() .number(100) .user(u => u.login('me')) @@ -61,8 +61,7 @@ describe('PullRequestGitHelper', function () { h.repo(r => (r).clone_url('git@github.com:you/name.git')); h.ref('my-branch'); }) - .build(), - gitHubRepository, + .build() ); repository.expectFetch('you', 'my-branch:pr/me/100', 1); diff --git a/src/test/github/pullRequestModel.test.ts b/src/test/github/pullRequestModel.test.ts index 3f6c0bfe2b..2f70f42eb8 100644 --- a/src/test/github/pullRequestModel.test.ts +++ b/src/test/github/pullRequestModel.test.ts @@ -75,28 +75,28 @@ describe('PullRequestModel', function () { sinon.restore(); }); - it('should return `state` properly as `open`', async function () { + it('should return `state` properly as `open`', function () { const pr = new PullRequestBuilder().state('open').build(); - const open = new PullRequestModel(credentials, telemetry, repo, remote, await convertRESTPullRequestToRawPullRequest(pr, repo)); + const open = new PullRequestModel(credentials, telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr)); assert.strictEqual(open.state, GithubItemStateEnum.Open); }); - it('should return `state` properly as `closed`', async function () { + it('should return `state` properly as `closed`', function () { const pr = new PullRequestBuilder().state('closed').build(); - const open = new PullRequestModel(credentials, telemetry, repo, remote, await convertRESTPullRequestToRawPullRequest(pr, repo)); + const open = new PullRequestModel(credentials, telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr)); assert.strictEqual(open.state, GithubItemStateEnum.Closed); }); - it('should return `state` properly as `merged`', async function () { + it('should return `state` properly as `merged`', function () { const pr = new PullRequestBuilder().merged(true).state('closed').build(); - const open = new PullRequestModel(credentials, telemetry, repo, remote, await convertRESTPullRequestToRawPullRequest(pr, repo)); + const open = new PullRequestModel(credentials, telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr)); assert.strictEqual(open.state, GithubItemStateEnum.Merged); }); - describe('reviewThreadCache', async function () { + describe('reviewThreadCache', function () { it('should update the cache when then cache is initialized', async function () { const pr = new PullRequestBuilder().build(); const model = new PullRequestModel( @@ -104,7 +104,7 @@ describe('PullRequestModel', function () { telemetry, repo, remote, - await convertRESTPullRequestToRawPullRequest(pr, repo), + convertRESTPullRequestToRawPullRequest(pr), ); repo.queryProvider.expectGraphQLQuery( diff --git a/src/test/github/pullRequestOverview.test.ts b/src/test/github/pullRequestOverview.test.ts index 8f9be6806a..ccd11c6e52 100644 --- a/src/test/github/pullRequestOverview.test.ts +++ b/src/test/github/pullRequestOverview.test.ts @@ -73,7 +73,7 @@ describe('PullRequestOverview', function () { }); }); - const prItem = await convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(1000).build(), repo); + const prItem = convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(1000).build()); const prModel = new PullRequestModel(credentialStore, telemetry, repo, remote, prItem); await PullRequestOverviewPanel.createOrShow(EXTENSION_URI, pullRequestManager, prModel); @@ -106,7 +106,7 @@ describe('PullRequestOverview', function () { }); }); - const prItem0 = await convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(1000).build(), repo); + const prItem0 = convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(1000).build()); const prModel0 = new PullRequestModel(credentialStore, telemetry, repo, remote, prItem0); const resolveStub = sinon.stub(pullRequestManager, 'resolvePullRequest').resolves(prModel0); sinon.stub(prModel0, 'getReviewRequests').resolves([]); @@ -119,7 +119,7 @@ describe('PullRequestOverview', function () { assert.strictEqual(createWebviewPanel.callCount, 1); assert.strictEqual(panel0!.getCurrentTitle(), 'Pull Request #1000'); - const prItem1 = await convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(2000).build(), repo); + const prItem1 = convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(2000).build()); const prModel1 = new PullRequestModel(credentialStore, telemetry, repo, remote, prItem1); resolveStub.resolves(prModel1); sinon.stub(prModel1, 'getReviewRequests').resolves([]); diff --git a/src/test/view/prsTree.test.ts b/src/test/view/prsTree.test.ts index 898fe1611a..c17641755f 100644 --- a/src/test/view/prsTree.test.ts +++ b/src/test/view/prsTree.test.ts @@ -147,7 +147,7 @@ describe('GitHub Pull Requests view', function () { ); }); }).pullRequest; - const prItem0 = parseGraphQLPullRequest(pr0.repository.pullRequest, gitHubRepository); + const prItem0 = parseGraphQLPullRequest(pr0.repository.pullRequest); const pullRequest0 = new PullRequestModel(credentialStore, telemetry, gitHubRepository, remote, prItem0); const pr1 = gitHubRepository.addGraphQLPullRequest(builder => { @@ -163,7 +163,7 @@ describe('GitHub Pull Requests view', function () { ); }); }).pullRequest; - const prItem1 = parseGraphQLPullRequest(pr1.repository.pullRequest, gitHubRepository); + const prItem1 = parseGraphQLPullRequest(pr1.repository.pullRequest); const pullRequest1 = new PullRequestModel(credentialStore, telemetry, gitHubRepository, remote, prItem1); const repository = new MockRepository(); @@ -201,14 +201,14 @@ describe('GitHub Pull Requests view', function () { assert.strictEqual(localItem0.description, 'by @me'); assert.strictEqual(localItem0.collapsibleState, vscode.TreeItemCollapsibleState.Collapsed); assert.strictEqual(localItem0.contextValue, 'pullrequest:local:nonactive'); - assert.deepStrictEqual(localItem0.iconPath!.toString(), 'https://avatars.com/me.jpg&s=64'); + assert.deepStrictEqual(localItem0.iconPath!.toString(), 'https://avatars.com/me.jpg'); assert.strictEqual(localItem1.label, '✓ one'); assert.strictEqual(localItem1.tooltip, 'Current Branch * one by @you'); assert.strictEqual(localItem1.description, 'by @you'); assert.strictEqual(localItem1.collapsibleState, vscode.TreeItemCollapsibleState.Collapsed); assert.strictEqual(localItem1.contextValue, 'pullrequest:local:active'); - assert.deepStrictEqual(localItem1.iconPath!.toString(), 'https://avatars.com/you.jpg&s=64'); + assert.deepStrictEqual(localItem1.iconPath!.toString(), 'https://avatars.com/you.jpg'); }); }); }); diff --git a/src/test/view/reviewCommentController.test.ts b/src/test/view/reviewCommentController.test.ts index 49b9307dc1..c6d1825de8 100644 --- a/src/test/view/reviewCommentController.test.ts +++ b/src/test/view/reviewCommentController.test.ts @@ -90,7 +90,7 @@ describe('ReviewCommentController', function () { telemetry, githubRepo, remote, - await convertRESTPullRequestToRawPullRequest(pr, githubRepo), + convertRESTPullRequestToRawPullRequest(pr), ); manager.activePullRequest = activePullRequest; diff --git a/webpack.config.js b/webpack.config.js index 41d5a84070..599f17e0eb 100644 --- a/webpack.config.js +++ b/webpack.config.js @@ -304,6 +304,13 @@ async function getExtensionConfig(target, mode, env) { 'dist-web', 'index.js', ), + 'p-queue': path.join( + __dirname, + 'node_modules', + 'p-queue', + 'dist', + 'index.js', + ), 'node-fetch': 'cross-fetch', '../env/node/net': path.resolve(__dirname, 'src', 'env', 'browser', 'net'), '../env/node/ssh': path.resolve(__dirname, 'src', 'env', 'browser', 'ssh'), diff --git a/webviews/editorWebview/test/builder/pullRequest.ts b/webviews/editorWebview/test/builder/pullRequest.ts index fb9e40d409..4ea3980686 100644 --- a/webviews/editorWebview/test/builder/pullRequest.ts +++ b/webviews/editorWebview/test/builder/pullRequest.ts @@ -7,7 +7,6 @@ import { GithubItemStateEnum, PullRequestMergeability } from '../../../../src/gi import { PullRequest } from '../../../../src/github/views'; import { createBuilderClass } from '../../../../src/test/builders/base'; import { CombinedStatusBuilder } from '../../../../src/test/builders/rest/combinedStatusBuilder'; - import { AccountBuilder } from './account'; export const PullRequestBuilder = createBuilderClass()({ diff --git a/yarn.lock b/yarn.lock index f9a4aa9d40..d91e27ac74 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2569,6 +2569,11 @@ event-stream@^4.0.1: stream-combiner "^0.2.2" through "^2.3.8" +eventemitter3@^4.0.4: + version "4.0.7" + resolved "https://registry.yarnpkg.com/eventemitter3/-/eventemitter3-4.0.7.tgz#2de9b68f6528d5644ef5c59526a1b4a07306169f" + integrity sha512-8guHBZCwKnFhYdHr2ysuRWErTwhoN2X8XELRlrRwpmfeY2jjuUN4taQMsULKUVo1K4DvZl+0pgfyoysHxvmvEw== + events@3.2.0: version "3.2.0" resolved "https://registry.yarnpkg.com/events/-/events-3.2.0.tgz#93b87c18f8efcd4202a461aec4dfc0556b639379" @@ -4277,6 +4282,11 @@ p-all@^1.0.0: dependencies: p-map "^1.0.0" +p-finally@^1.0.0: + version "1.0.0" + resolved "https://registry.yarnpkg.com/p-finally/-/p-finally-1.0.0.tgz#3fbcfb15b899a44123b34b6dcc18b724336a2cae" + integrity sha512-LICb2p9CB7FS+0eR1oqWnHhp0FljGLZCWBE9aix0Uye9W8LTQPwMTYVGWQWIw9RdQiDg4+epXQODwIYJtSJaow== + p-limit@^1.1.0: version "1.3.0" resolved "https://registry.yarnpkg.com/p-limit/-/p-limit-1.3.0.tgz#b86bd5f0c25690911c7590fcbfc2010d54b3ccb8" @@ -4324,6 +4334,21 @@ p-map@^1.0.0: resolved "https://registry.yarnpkg.com/p-map/-/p-map-1.2.0.tgz#e4e94f311eabbc8633a1e79908165fca26241b6b" integrity sha512-r6zKACMNhjPJMTl8KcFH4li//gkrXWfbD6feV8l6doRHlzljFWGJ2AP6iKaCJXyZmAUMOPtvbW7EXkbWO/pLEA== +p-queue@^6.6.2: + version "6.6.2" + resolved "https://registry.yarnpkg.com/p-queue/-/p-queue-6.6.2.tgz#2068a9dcf8e67dd0ec3e7a2bcb76810faa85e426" + integrity sha512-RwFpb72c/BhQLEXIZ5K2e+AhgNVmIejGlTgiB9MzZ0e93GRvqZ7uSi0dvRF7/XIXDeNkra2fNHBxTyPDGySpjQ== + dependencies: + eventemitter3 "^4.0.4" + p-timeout "^3.2.0" + +p-timeout@^3.2.0: + version "3.2.0" + resolved "https://registry.yarnpkg.com/p-timeout/-/p-timeout-3.2.0.tgz#c7e17abc971d2a7962ef83626b35d635acf23dfe" + integrity sha512-rhIwUycgwwKcP9yTOOFK/AKsAopjjCakVqLHePO3CC6Mir1Z99xT+R63jZxAT5lFZLa2inS5h+ZS2GvR99/FBg== + dependencies: + p-finally "^1.0.0" + p-try@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/p-try/-/p-try-1.0.0.tgz#cbc79cdbaf8fd4228e13f621f2b1a237c1b207b3" From b1d859a8ca4009c8148d52d89e31b990f946df61 Mon Sep 17 00:00:00 2001 From: Kevin Abel Date: Tue, 14 Feb 2023 11:49:32 -0600 Subject: [PATCH 3/4] Move avatar related utils to own class Also implements HTTP caching client by saving the avatar REST headers and responses to the globalStorage. --- src/github/avatars.ts | 379 ++++++++++++++++++ src/github/folderRepositoryManager.ts | 32 +- src/github/githubRepository.ts | 67 ++-- src/github/issueModel.ts | 33 +- src/github/pullRequestModel.ts | 62 ++- src/github/utils.ts | 176 -------- .../github/folderRepositoryManager.test.ts | 6 +- src/test/github/githubRepository.test.ts | 7 +- src/test/github/pullRequestGitHelper.test.ts | 4 +- src/test/github/pullRequestModel.test.ts | 10 +- src/test/github/pullRequestOverview.test.ts | 9 +- src/test/mocks/mockGitHubRepository.ts | 4 +- src/test/view/prsTree.test.ts | 7 +- src/test/view/reviewCommentController.test.ts | 3 + webpack.config.js | 20 +- 15 files changed, 537 insertions(+), 282 deletions(-) create mode 100644 src/github/avatars.ts diff --git a/src/github/avatars.ts b/src/github/avatars.ts new file mode 100644 index 0000000000..0cc06f0acb --- /dev/null +++ b/src/github/avatars.ts @@ -0,0 +1,379 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ +'use strict'; + +import * as crypto from 'crypto'; +import { RequestError } from '@octokit/request-error'; +import { OctokitResponse, RequestParameters, ResponseHeaders } from '@octokit/types'; +import PQueue from 'p-queue'; +import * as vscode from 'vscode'; +import Logger from '../common/logger'; +import * as Common from '../common/timelineEvent'; +import { IAccount, Issue, ITeam, PullRequest } from './interface'; +import { LoggingOctokit } from './loggingOctokit'; +import { getEnterpriseUri, hasEnterpriseUri } from './utils'; + +const GRAVATAR_STYLE_NONE = 'none'; + +function isGravatarEnabled() { + return getGravatarStyle() !== GRAVATAR_STYLE_NONE; +} + +function getGravatarStyle() { + return vscode.workspace + .getConfiguration('githubPullRequests') + .get('defaultGravatarsStyle', GRAVATAR_STYLE_NONE); +} + +function generateGravatarUrl(gravatarId: string | undefined, size: number = 200): string | undefined { + if (!gravatarId || !isGravatarEnabled()) { + return undefined; + } + + return `https://www.gravatar.com/avatar/${gravatarId}?s=${size}&d=${getGravatarStyle()}`; +} + +function getExtensionFromType(type: string): string { + switch (type) { + case 'image/avif': + return '.avif'; + case 'image/bmp': + return '.bmp'; + case 'image/gif': + return '.gif'; + case 'image/vnd.microsoft.icon': + return '.ico'; + case 'image/jpeg': + return '.jpg'; + case 'image/png': + return '.png'; + case 'image/svg+xml': + return '.svg'; + case 'image/tiff': + return '.tif'; + case 'image/webp': + return '.webp'; + } + + return '.bin'; +} + +interface CacheControl { + 'max-age'?: number; + 's-maxage'?: number; + [directive: string]: string | number | undefined; +} + +interface CacheResult { + content?: Buffer; + contentType?: string; + uri?: vscode.Uri; + isFresh: boolean; + doWrite: boolean; +} + +// This limits the concurrent promises that fetch avatars from the Enterprise REST service +const enterpriseAvatarQueue = new PQueue({ concurrency: 3 }); + +function convertBinaryToDataUri(data: Buffer, contentType: string) { + return `data:${contentType};base64,${data.toString('base64')}`; +} + +const LOGGER_COMPONENT = 'Avatars'; +const ENTERPRISE_AVATAR_REST_BASE = '/enterprise/avatars'; + +export class Avatars { + private baseUri: vscode.Uri; + private rereadCache: boolean = true; + private lastReadAuthority: string = ''; + private headersCache: { [k: string]: ResponseHeaders } = {}; + + constructor(public context: vscode.ExtensionContext) { + this.baseUri = vscode.Uri.joinPath(context.globalStorageUri, 'avatarCache'); + } + + private getCacheBase(authority: string) { + return vscode.Uri.joinPath(this.baseUri, authority); + } + + private getCacheMetaFile(authority: string) { + return vscode.Uri.joinPath(this.getCacheBase(authority), '.meta.json'); + } + + private getCacheFile(authority: string, key: string, type: string) { + return vscode.Uri.joinPath(this.getCacheBase(authority), `${key}${getExtensionFromType(type)}`); + } + + private async _reloadCache(authority: string) { + if (this.lastReadAuthority !== authority) { + this.rereadCache = true; + } + + if (!this.rereadCache) { + return; + } + + await vscode.workspace.fs.createDirectory(this.getCacheBase(authority)); + const cacheMetaFile = this.getCacheMetaFile(authority); + + this.headersCache = {}; + + try { + const loadedCache = await vscode.workspace.fs.readFile(cacheMetaFile); + this.headersCache = JSON.parse(loadedCache.toString()) || {}; + this.lastReadAuthority = authority; + this.rereadCache = false; + } catch (e) { + Logger.debug(e, LOGGER_COMPONENT); + } + } + + private async checkCache( + authority: string, + key: string, + name: string, + options: RequestParameters, + ): Promise { + const result: CacheResult = { + isFresh: false, + doWrite: false, + }; + + if (!(key in this.headersCache)) { + return result; + } + + const headers = this.headersCache[key]; + result.contentType = headers['content-type'] || ''; + result.uri = this.getCacheFile(authority, name, result.contentType); + + try { + result.content = Buffer.from(await vscode.workspace.fs.readFile(result.uri)); + + const cacheControlDirectives: CacheControl = {}; + const cacheControl = (headers['cache-control'] || '').split(','); + for (const directive of cacheControl) { + let [name, param] = directive.split('=', 2); + name = name.trim().toLowerCase(); + if (name === 'max-age' || name == 's-maxage') { + const age = Number.parseInt(param, 10); + cacheControlDirectives[name] = age; + } else { + cacheControlDirectives[name] = param; + } + } + + const serverDate = headers.date ? new Date(headers.date) : new Date(); + const expireAt = serverDate.setSeconds( + serverDate.getSeconds() + + (cacheControlDirectives['s-maxage'] ?? cacheControlDirectives['max-age'] ?? 0), + ); + if (expireAt - Date.now() > 0) { + Logger.appendLine('Cache fresh hit', LOGGER_COMPONENT); + result.isFresh = true; + return result; + } + + Logger.appendLine('Cache stale hit', LOGGER_COMPONENT); + + options.headers = {}; + if (headers['last-modified']) { + options.headers['if-modified-since'] = headers['last-modified']; + } + if (headers.etag) { + options.headers['if-none-match'] = headers.etag; + } + } catch (e) { + Logger.appendLine('Corrupt cache entry removed', LOGGER_COMPONENT); + delete this.headersCache[key]; + result.doWrite = true; + } + + return result; + } + + public async clear() { + await vscode.workspace.fs.delete(this.baseUri, { recursive: true }); + await vscode.workspace.fs.createDirectory(this.baseUri); + this.rereadCache = true; + } + + public async getEnterpriseAvatarUrl( + avatarUrl: string | undefined, + octokit: LoggingOctokit, + ): Promise { + try { + if (!avatarUrl || !hasEnterpriseUri()) { + return; + } + + const avatarUri = vscode.Uri.parse(avatarUrl, true); + const authority = avatarUri.authority; + const enterpriseUri = getEnterpriseUri()!; + + // static asset from enterprise does not need authentication + if (avatarUri.scheme === 'data' || authority === `assets.${enterpriseUri.authority}`) { + return avatarUrl; + } + + // only proxy avatars from the "avatars" sub-domain of Enterprise + if (authority !== `avatars.${enterpriseUri.authority}`) { + return; + } + + const cacheKey = `${avatarUri.path}?${avatarUri.query}`; + const cacheFileName = crypto.createHash('sha256').update(cacheKey).digest('hex'); + const options: RequestParameters = {}; + const qs = new URLSearchParams(avatarUri.query); + qs.forEach((v, k) => { + options[k] = v; + }); + + await this._reloadCache(authority); + + const cacheResult = await this.checkCache(authority, cacheKey, cacheFileName, options); + if (cacheResult.isFresh) { + return convertBinaryToDataUri(cacheResult.content!, cacheResult.contentType!); + } + + const avatarDataUri = await enterpriseAvatarQueue.add(() => + octokit.api.request(`GET ${ENTERPRISE_AVATAR_REST_BASE}${avatarUri.path}`, options).then( + async (resp: OctokitResponse) => { + this.headersCache[cacheKey] = resp.headers; + cacheResult.doWrite = true; + cacheResult.content = Buffer.from(resp.data); + cacheResult.contentType = resp.headers['content-type'] || ''; + cacheResult.uri = this.getCacheFile(authority, cacheFileName, cacheResult.contentType); + await vscode.workspace.fs.writeFile(cacheResult.uri, cacheResult.content); + return convertBinaryToDataUri(cacheResult.content, cacheResult.contentType); + }, + (reason: RequestError) => { + if (reason.status !== 304) { + Logger.warn(`REST request failed: ${reason.message}`, LOGGER_COMPONENT); + return; + } + + Logger.appendLine('Stale cache entry refreshed', LOGGER_COMPONENT); + for (const header of Object.keys(reason.headers)) { + this.headersCache[cacheKey][header] = reason.headers[header]; + } + cacheResult.doWrite = true; + return convertBinaryToDataUri(cacheResult.content!, cacheResult.contentType!); + }, + ), + ); + + if (cacheResult.doWrite) { + await vscode.workspace.fs.writeFile( + this.getCacheMetaFile(authority), + new TextEncoder().encode(JSON.stringify(this.headersCache)), + ); + } + + if (avatarDataUri) { + return avatarDataUri; + } + } catch (e) { + Logger.debug(e, LOGGER_COMPONENT); + } + } + + public async replaceAvatarUrl(user: IAccount | ITeam, octokit: LoggingOctokit): Promise { + const origAvatarUrl = user.avatarUrl; + user.avatarUrl = undefined; + + const enterpriseAvatarUrl = await this.getEnterpriseAvatarUrl(origAvatarUrl, octokit); + if (enterpriseAvatarUrl) { + user.avatarUrl = enterpriseAvatarUrl; + return; + } + + if (!('login' in user)) { + return; + } + + if (user.email === undefined && user.login) { + try { + const { data } = await octokit.call(octokit.api.users.getByUsername, { + username: user.login, + }); + + user.email = data.email || undefined; + } catch { + // ignore + } + } + + if (!user.email) { + return; + } + + user.avatarUrl = generateGravatarUrl( + crypto.createHash('md5').update(user.email.trim().toLowerCase()).digest('hex'), + ); + } + + public replaceAccountAvatarUrls(pr: PullRequest, octokit: LoggingOctokit): Promise { + const promises: Promise[] = []; + promises.push(this.replaceAvatarUrl(pr.user, octokit)); + if (pr.assignees) { + promises.push(...pr.assignees.map(user => this.replaceAvatarUrl(user, octokit))); + } + if (pr.suggestedReviewers) { + promises.push(...pr.suggestedReviewers.map(user => this.replaceAvatarUrl(user, octokit))); + } + return Promise.all(promises); + } + + public replaceTimelineEventAvatarUrls(events: Common.TimelineEvent[], octokit: LoggingOctokit): Promise { + const promises: Promise[] = []; + + for (const event of events) { + const type = event.event; + switch (type) { + case Common.EventType.Commented: + const commentEvent = event as Common.CommentEvent; + promises.push(this.replaceAvatarUrl(commentEvent.user, octokit)); + break; + case Common.EventType.Reviewed: + const reviewEvent = event as Common.ReviewEvent; + promises.push(this.replaceAvatarUrl(reviewEvent.user, octokit)); + break; + case Common.EventType.Committed: + const commitEv = event as Common.CommitEvent; + promises.push(this.replaceAvatarUrl(commitEv.author, octokit)); + break; + case Common.EventType.Merged: + const mergeEv = event as Common.MergedEvent; + promises.push(this.replaceAvatarUrl(mergeEv.user, octokit)); + break; + case Common.EventType.Assigned: + const assignEv = event as Common.AssignEvent; + promises.push(this.replaceAvatarUrl(assignEv.user, octokit)); + promises.push(this.replaceAvatarUrl(assignEv.actor, octokit)); + break; + case Common.EventType.HeadRefDeleted: + const deletedEv = event as Common.HeadRefDeleteEvent; + promises.push(this.replaceAvatarUrl(deletedEv.actor, octokit)); + break; + } + } + + return Promise.all(promises); + } + + public replaceIssuesAvatarUrls(issues: Issue[], octokit: LoggingOctokit): Promise { + const promises: Promise[] = []; + + for (const issue of issues) { + promises.push(this.replaceAvatarUrl(issue.user, octokit)); + if (issue.assignees) { + promises.push(...issue.assignees.map(user => this.replaceAvatarUrl(user, octokit))); + } + } + + return Promise.all(promises); + } +} diff --git a/src/github/folderRepositoryManager.ts b/src/github/folderRepositoryManager.ts index eabbb2cdc5..25cec92f4f 100644 --- a/src/github/folderRepositoryManager.ts +++ b/src/github/folderRepositoryManager.ts @@ -32,6 +32,7 @@ import { EXTENSION_ID } from '../constants'; import { NEVER_SHOW_PULL_NOTIFICATION, REPO_KEYS, ReposState } from '../extensionState'; import { git } from '../gitProviders/gitCommands'; import { UserCompletion, userMarkdown } from '../issues/util'; +import { Avatars } from './avatars'; import { OctokitCommon } from './common'; import { CredentialStore } from './credentials'; import { GitHubRepository, ItemsData, PullRequestData, TeamReviewerRefreshKind, ViewerPermission } from './githubRepository'; @@ -48,8 +49,6 @@ import { getRelatedUsersFromTimelineEvents, loginComparator, parseGraphQLUser, - replaceAccountAvatarUrls, - replaceAvatarUrl, teamComparator, variableSubstitution, } from './utils'; @@ -140,6 +139,7 @@ export class FolderRepositoryManager implements vscode.Disposable { private _gitBlameCache: { [key: string]: string } = {}; private _githubManager: GitHubManager; private _repositoryPageInformation: Map = new Map(); + private readonly _avatars: Avatars; private _onDidMergePullRequest = new vscode.EventEmitter(); readonly onDidMergePullRequest = this._onDidMergePullRequest.event; @@ -182,6 +182,8 @@ export class FolderRepositoryManager implements vscode.Disposable { this._subs.push(_credentialStore.onDidInitialize(() => this.updateRepositories())); + this._avatars = new Avatars(this.context); + this.setUpCompletionItemProvider(); this.cleanStoredRepoState(); @@ -1247,6 +1249,7 @@ export class FolderRepositoryManager implements vscode.Disposable { includeIssuesWithoutMilestone: boolean = false, query?: string, ): Promise> { + const _avatars = this._avatars; const milestones: ItemsResponseResult = await this.fetchPagedData( options, 'issuesKey', @@ -1270,7 +1273,7 @@ export class FolderRepositoryManager implements vscode.Disposable { }, issues: await Promise.all(additionalIssues.items.map(async (issue) => { const githubRepository = await this.getRepoForIssue(issue); - return new IssueModel(githubRepository, githubRepository.remote, issue); + return new IssueModel(githubRepository, githubRepository.remote, issue, _avatars); })), }); } @@ -1313,6 +1316,7 @@ export class FolderRepositoryManager implements vscode.Disposable { async getIssues( query?: string, ): Promise> { + const _avatars = this._avatars; const data = await this.fetchPagedData({ fetchNextPage: false, fetchOnePagePerRepo: false }, 'issuesKey', PagedDataType.IssueSearch, PRType.All, query); const mappedData: ItemsResponseResult = { items: [], @@ -1321,7 +1325,7 @@ export class FolderRepositoryManager implements vscode.Disposable { }; for (const issue of data.items) { const githubRepository = await this.getRepoForIssue(issue); - mappedData.items.push(new IssueModel(githubRepository, githubRepository.remote, issue)); + mappedData.items.push(new IssueModel(githubRepository, githubRepository.remote, issue, _avatars)); } return mappedData; } @@ -1554,6 +1558,7 @@ export class FolderRepositoryManager implements vscode.Disposable { async createIssue(params: OctokitCommon.IssuesCreateParams): Promise { try { + const _avatars = this._avatars; const repo = this._githubRepositories.find( r => r.remote.owner === params.owner && r.remote.repositoryName === params.repo, ); @@ -1568,10 +1573,10 @@ export class FolderRepositoryManager implements vscode.Disposable { const item = convertRESTIssueToRawPullRequest(data); if (repo.remote.isEnterprise) { - await replaceAccountAvatarUrls(item, repo.octokit); + await _avatars.replaceAccountAvatarUrls(item, repo.octokit); } - const issueModel = new IssueModel(repo, repo.remote, item); + const issueModel = new IssueModel(repo, repo.remote, item, _avatars); /* __GDPR__ "issue.create.success" : { @@ -2017,6 +2022,7 @@ export class FolderRepositoryManager implements vscode.Disposable { Logger.debug(`Fulfill pull request missing info - start`, FolderRepositoryManager.ID); const githubRepository = pullRequest.githubRepository; + const _avatars = this._avatars; const { octokit, remote } = await githubRepository.ensure(); if (!pullRequest.base) { @@ -2028,7 +2034,7 @@ export class FolderRepositoryManager implements vscode.Disposable { const pr = convertRESTPullRequestToRawPullRequest(data); if (remote.isEnterprise) { - await replaceAccountAvatarUrls(pr, octokit); + await _avatars.replaceAccountAvatarUrls(pr, octokit); } pullRequest.update(pr); @@ -2094,6 +2100,7 @@ export class FolderRepositoryManager implements vscode.Disposable { async resolveUser(owner: string, repositoryName: string, login: string): Promise { Logger.debug(`Fetch user ${login}`, FolderRepositoryManager.ID); + const _avatars = this._avatars; const githubRepository = await this.createGitHubRepositoryFromOwnerName(owner, repositoryName); const { query, schema, remote, octokit } = await githubRepository.ensure(); @@ -2106,7 +2113,7 @@ export class FolderRepositoryManager implements vscode.Disposable { }); if (remote.isEnterprise) { - await replaceAvatarUrl(data.user, octokit); + await _avatars.replaceAvatarUrl(data.user, octokit); } return parseGraphQLUser(data); @@ -2341,7 +2348,14 @@ export class FolderRepositoryManager implements vscode.Disposable { } private async createAndAddGitHubRepository(remote: Remote, credentialStore: CredentialStore, silent?: boolean) { - const repo = new GitHubRepository(GitHubRemote.remoteAsGitHub(remote, await this._githubManager.isGitHub(remote.gitProtocol.normalizeUri()!)), this.repository.rootUri, credentialStore, this.telemetry, silent); + const repo = new GitHubRepository( + GitHubRemote.remoteAsGitHub(remote, await this._githubManager.isGitHub(remote.gitProtocol.normalizeUri()!)), + this.repository.rootUri, + credentialStore, + this.telemetry, + this._avatars, + silent + ); this._githubRepositories.push(repo); return repo; } diff --git a/src/github/githubRepository.ts b/src/github/githubRepository.ts index 9ab4f18245..2d3a3af7b4 100644 --- a/src/github/githubRepository.ts +++ b/src/github/githubRepository.ts @@ -11,6 +11,7 @@ import { Protocol } from '../common/protocol'; import { GitHubRemote, parseRemote } from '../common/remote'; import { ITelemetry } from '../common/telemetry'; import { PRCommentControllerRegistry } from '../view/pullRequestCommentControllerRegistry'; +import { Avatars } from './avatars'; import { OctokitCommon } from './common'; import { CredentialStore, GitHub } from './credentials'; import { @@ -56,9 +57,6 @@ import { parseGraphQLPullRequest, parseGraphQLViewerPermission, parseMilestone, - replaceAccountAvatarUrls, - replaceAvatarUrl, - replaceIssuesAvatarUrls, } from './utils'; export const PULL_REQUEST_PAGE_SIZE = 20; @@ -180,6 +178,7 @@ export class GitHubRepository implements vscode.Disposable { public readonly rootUri: vscode.Uri, private readonly _credentialStore: CredentialStore, private readonly _telemetry: ITelemetry, + private readonly _avatars: Avatars, silent: boolean = false ) { // kick off the comments controller early so that the Comments view is visible and doesn't pop up later in an way that's jarring @@ -378,7 +377,7 @@ export class GitHubRepository implements vscode.Disposable { async getAllPullRequests(page?: number): Promise { try { Logger.debug(`Fetch all pull requests - enter`, GitHubRepository.ID); - const { octokit, remote } = await this.ensure(); + const { octokit, remote, _avatars } = await this.ensure(); const result = await octokit.call(octokit.api.pulls.list, { owner: remote.owner, repo: remote.repositoryName, @@ -409,7 +408,7 @@ export class GitHubRepository implements vscode.Disposable { const pr = convertRESTPullRequestToRawPullRequest(pullRequest); if (remote.isEnterprise) { - await replaceAccountAvatarUrls(pr, octokit); + await _avatars.replaceAccountAvatarUrls(pr, octokit); } pullRequests.push(this.createOrUpdatePullRequestModel(pr)); @@ -437,7 +436,7 @@ export class GitHubRepository implements vscode.Disposable { async getPullRequestForBranch(branch: string): Promise { try { Logger.debug(`Fetch pull requests for branch - enter`, GitHubRepository.ID); - const { query, remote, schema, octokit } = await this.ensure(); + const { query, remote, schema, octokit, _avatars } = await this.ensure(); const { data } = await query({ query: schema.PullRequestForHead, variables: { @@ -453,7 +452,7 @@ export class GitHubRepository implements vscode.Disposable { const mostRecentOrOpenPr = prs.find(pr => pr.state.toLowerCase() === 'open') ?? prs[0]; if (remote.isEnterprise) { - await replaceAccountAvatarUrls(mostRecentOrOpenPr, octokit); + await _avatars.replaceAccountAvatarUrls(mostRecentOrOpenPr, octokit); } return this.createOrUpdatePullRequestModel(mostRecentOrOpenPr); @@ -529,7 +528,7 @@ export class GitHubRepository implements vscode.Disposable { async getIssuesForUserByMilestone(_page?: number): Promise { try { Logger.debug(`Fetch all issues - enter`, GitHubRepository.ID); - const { query, remote, schema, octokit } = await this.ensure(); + const { query, remote, schema, octokit, _avatars } = await this.ensure(); const { data } = await query({ query: schema.GetMilestonesWithIssues, variables: { @@ -551,10 +550,10 @@ export class GitHubRepository implements vscode.Disposable { const issues: Issue[] = raw.issues.edges.map(edge => parseGraphQLIssue(edge.node)); if (remote.isEnterprise) { - await replaceIssuesAvatarUrls(issues, octokit); + await _avatars.replaceIssuesAvatarUrls(issues, octokit); } - milestones.push({ milestone, issues: issues.map(issue => new IssueModel(this, this.remote, issue)) }); + milestones.push({ milestone, issues: issues.map(issue => new IssueModel(this, this.remote, issue, _avatars)) }); } } return { @@ -570,7 +569,7 @@ export class GitHubRepository implements vscode.Disposable { async getIssuesWithoutMilestone(_page?: number): Promise { try { Logger.debug(`Fetch issues without milestone- enter`, GitHubRepository.ID); - const { query, remote, schema, octokit } = await this.ensure(); + const { query, remote, schema, octokit, _avatars } = await this.ensure(); const { data } = await query({ query: schema.IssuesWithoutMilestone, variables: { @@ -592,7 +591,7 @@ export class GitHubRepository implements vscode.Disposable { } if (remote.isEnterprise) { - await replaceIssuesAvatarUrls(issues, octokit); + await _avatars.replaceIssuesAvatarUrls(issues, octokit); } return { @@ -608,7 +607,7 @@ export class GitHubRepository implements vscode.Disposable { async getIssues(page?: number, queryString?: string): Promise { try { Logger.debug(`Fetch issues with query - enter`, GitHubRepository.ID); - const { query, schema, remote, octokit } = await this.ensure(); + const { query, schema, remote, octokit, _avatars } = await this.ensure(); const { data } = await query({ query: schema.Issues, variables: { @@ -628,7 +627,7 @@ export class GitHubRepository implements vscode.Disposable { } if (remote.isEnterprise) { - await replaceIssuesAvatarUrls(issues, octokit); + await _avatars.replaceIssuesAvatarUrls(issues, octokit); } return { @@ -724,7 +723,7 @@ export class GitHubRepository implements vscode.Disposable { async getPullRequestsForCategory(categoryQuery: string, page?: number): Promise { try { Logger.debug(`Fetch pull request category ${categoryQuery} - enter`, GitHubRepository.ID); - const { octokit, query, schema, remote } = await this.ensure(); + const { octokit, query, schema, remote, _avatars } = await this.ensure(); const user = await this.getAuthenticatedUser(); // Search api will not try to resolve repo that redirects, so get full name first @@ -761,7 +760,7 @@ export class GitHubRepository implements vscode.Disposable { const pr = parseGraphQLPullRequest(response.repository.pullRequest); if (remote.isEnterprise) { - await replaceAccountAvatarUrls(pr, octokit); + await _avatars.replaceAccountAvatarUrls(pr, octokit); } pullRequests.push(this.createOrUpdatePullRequestModel(pr)); @@ -792,7 +791,7 @@ export class GitHubRepository implements vscode.Disposable { if (model) { model.update(pullRequest); } else { - model = new PullRequestModel(this._credentialStore, this._telemetry, this, this.remote, pullRequest); + model = new PullRequestModel(this._credentialStore, this._telemetry, this, this.remote, pullRequest, this._avatars); model.onDidInvalidate(() => this.getPullRequest(pullRequest.number)); this._pullRequestModels.set(pullRequest.number, model); this._onDidAddPullRequest.fire(model); @@ -805,7 +804,7 @@ export class GitHubRepository implements vscode.Disposable { try { Logger.debug(`Create pull request - enter`, GitHubRepository.ID); const metadata = await this.getMetadata(); - const { mutate, schema, remote, octokit } = await this.ensure(); + const { mutate, schema, remote, octokit, _avatars } = await this.ensure(); const { data } = await mutate({ mutation: schema.CreatePullRequest, @@ -827,7 +826,7 @@ export class GitHubRepository implements vscode.Disposable { const pr = parseGraphQLPullRequest(data.createPullRequest.pullRequest); if (remote.isEnterprise) { - await replaceAccountAvatarUrls(pr, octokit); + await _avatars.replaceAccountAvatarUrls(pr, octokit); } return this.createOrUpdatePullRequestModel(pr); @@ -840,7 +839,7 @@ export class GitHubRepository implements vscode.Disposable { async getPullRequest(id: number): Promise { try { Logger.debug(`Fetch pull request ${id} - enter`, GitHubRepository.ID); - const { query, remote, schema, octokit } = await this.ensure(); + const { query, remote, schema, octokit, _avatars } = await this.ensure(); const { data } = await query({ query: schema.PullRequest, @@ -854,7 +853,7 @@ export class GitHubRepository implements vscode.Disposable { const pr = parseGraphQLPullRequest(data.repository.pullRequest); if (remote.isEnterprise) { - await replaceAccountAvatarUrls(pr, octokit); + await _avatars.replaceAccountAvatarUrls(pr, octokit); } return this.createOrUpdatePullRequestModel(pr); @@ -867,7 +866,7 @@ export class GitHubRepository implements vscode.Disposable { async getIssue(id: number, withComments: boolean = false): Promise { try { Logger.debug(`Fetch issue ${id} - enter`, GitHubRepository.ID); - const { query, remote, schema, octokit } = await this.ensure(); + const { query, remote, schema, octokit, _avatars } = await this.ensure(); const { data } = await query({ query: withComments ? schema.IssueWithComments : schema.Issue, @@ -882,10 +881,10 @@ export class GitHubRepository implements vscode.Disposable { const issue = parseGraphQLIssue(data.repository.pullRequest); if (remote.isEnterprise) { - await replaceIssuesAvatarUrls([issue], octokit); + await _avatars.replaceIssuesAvatarUrls([issue], octokit); } - return new IssueModel(this, remote, issue); + return new IssueModel(this, remote, issue, _avatars); } catch (e) { Logger.error(`Unable to fetch PR: ${e}`, GitHubRepository.ID); return; @@ -951,7 +950,7 @@ export class GitHubRepository implements vscode.Disposable { async getMentionableUsers(): Promise { Logger.debug(`Fetch mentionable users - enter`, GitHubRepository.ID); - const { query, remote, schema, octokit } = await this.ensure(); + const { query, remote, schema, octokit, _avatars } = await this.ensure(); let after: string | null = null; let hasNextPage = false; @@ -972,7 +971,7 @@ export class GitHubRepository implements vscode.Disposable { if (remote.isEnterprise) { await Promise.all( - result.data.repository.mentionableUsers.nodes.map(node => replaceAvatarUrl(node, octokit)), + result.data.repository.mentionableUsers.nodes.map(node => _avatars.replaceAvatarUrl(node, octokit)), ); } @@ -1001,7 +1000,7 @@ export class GitHubRepository implements vscode.Disposable { async getAssignableUsers(): Promise { Logger.debug(`Fetch assignable users - enter`, GitHubRepository.ID); - const { query, remote, schema, octokit } = await this.ensure(); + const { query, remote, schema, octokit, _avatars } = await this.ensure(); let after: string | null = null; let hasNextPage = false; @@ -1021,7 +1020,7 @@ export class GitHubRepository implements vscode.Disposable { if (remote.isEnterprise) { await Promise.all( - result.data.repository.assignableUsers.nodes.map(node => replaceAvatarUrl(node, octokit)), + result.data.repository.assignableUsers.nodes.map(node => _avatars.replaceAvatarUrl(node, octokit)), ); } @@ -1095,7 +1094,7 @@ export class GitHubRepository implements vscode.Disposable { return []; } - const { query, remote, schema, octokit } = await this.ensureAdditionalScopes(); + const { query, remote, schema, octokit, _avatars } = await this.ensureAdditionalScopes(); let after: string | null = null; let hasNextPage = false; @@ -1123,7 +1122,7 @@ export class GitHubRepository implements vscode.Disposable { })); if (remote.isEnterprise) { - await Promise.all(teamsWithRepositoryNames.map(team => replaceAvatarUrl(team, octokit))); + await Promise.all(teamsWithRepositoryNames.map(team => _avatars.replaceAvatarUrl(team, octokit))); } orgTeams.push(...teamsWithRepositoryNames); @@ -1150,7 +1149,7 @@ export class GitHubRepository implements vscode.Disposable { async getPullRequestParticipants(pullRequestNumber: number): Promise { Logger.debug(`Fetch participants from a Pull Request`, GitHubRepository.ID); - const { query, remote, schema, octokit } = await this.ensure(); + const { query, remote, schema, octokit, _avatars } = await this.ensure(); const ret: IAccount[] = []; @@ -1167,7 +1166,7 @@ export class GitHubRepository implements vscode.Disposable { if (remote.isEnterprise) { await Promise.all( - result.data.repository.pullRequest.participants.nodes.map(node => replaceAvatarUrl(node, octokit)), + result.data.repository.pullRequest.participants.nodes.map(node => _avatars.replaceAvatarUrl(node, octokit)), ); } @@ -1230,7 +1229,7 @@ export class GitHubRepository implements vscode.Disposable { */ private _useFallbackChecks: boolean = false; async getStatusChecks(number: number): Promise<[PullRequestChecks | null, PullRequestReviewRequirement | null]> { - const { query, remote, schema, octokit } = await this.ensure(); + const { query, remote, schema, octokit, _avatars } = await this.ensure(); const captureUseFallbackChecks = this._useFallbackChecks; let result: ApolloQueryResult; try { @@ -1345,7 +1344,7 @@ export class GitHubRepository implements vscode.Disposable { email: status.creator?.email, avatarUrl: status.avatarUrl, }; - await replaceAvatarUrl(user, octokit); + await _avatars.replaceAvatarUrl(user, octokit); status.avatarUrl = user.avatarUrl; })); } diff --git a/src/github/issueModel.ts b/src/github/issueModel.ts index b8ab96aa47..f16c757a41 100644 --- a/src/github/issueModel.ts +++ b/src/github/issueModel.ts @@ -9,6 +9,7 @@ import Logger from '../common/logger'; import { Remote } from '../common/remote'; import { TimelineEvent } from '../common/timelineEvent'; import { formatError } from '../common/utils'; +import { Avatars } from './avatars'; import { OctokitCommon } from './common'; import { GitHubRepository } from './githubRepository'; import { @@ -18,7 +19,7 @@ import { UpdatePullRequestResponse, } from './graphql'; import { GithubItemStateEnum, IAccount, IMilestone, IPullRequestEditData, Issue } from './interface'; -import { parseGraphQlIssueComment, parseGraphQLTimelineEvents, replaceAvatarUrl, replaceTimelineEventAvatarUrls } from './utils'; +import { parseGraphQlIssueComment, parseGraphQLTimelineEvents } from './utils'; export class IssueModel { static ID = 'IssueModel'; @@ -34,15 +35,18 @@ export class IssueModel { public createdAt: string; public updatedAt: string; public milestone?: IMilestone; - public readonly githubRepository: GitHubRepository; - public readonly remote: Remote; - public item: TItem; public bodyHTML?: string; private _onDidInvalidate = new vscode.EventEmitter(); public onDidInvalidate = this._onDidInvalidate.event; - constructor(githubRepository: GitHubRepository, remote: Remote, item: TItem, skipUpdate: boolean = false) { + constructor( + public readonly githubRepository: GitHubRepository, + public readonly remote: Remote, + public item: TItem, + protected readonly _avatars: Avatars, + skipUpdate: boolean = false, + ) { this.githubRepository = githubRepository; this.remote = remote; this.item = item; @@ -119,7 +123,7 @@ export class IssueModel { if (issue.titleHTML) { this.titleHTML = issue.titleHTML; } - if (!this.bodyHTML || (issue.body !== this.body)) { + if (!this.bodyHTML || issue.body !== this.body) { this.bodyHTML = issue.bodyHTML; } this.html_url = issue.url; @@ -153,7 +157,9 @@ export class IssueModel { return true; } - async edit(toEdit: IPullRequestEditData): Promise<{ body: string; bodyHTML: string; title: string; titleHTML: string }> { + async edit( + toEdit: IPullRequestEditData, + ): Promise<{ body: string; bodyHTML: string; title: string; titleHTML: string }> { try { const { mutate, schema } = await this.githubRepository.ensure(); @@ -201,6 +207,7 @@ export class IssueModel { } async createIssueComment(text: string): Promise { + const _avatars = this._avatars; const { mutate, schema, remote, octokit } = await this.githubRepository.ensure(); const { data } = await mutate({ mutation: schema.AddIssueComment, @@ -215,7 +222,7 @@ export class IssueModel { const comment = parseGraphQlIssueComment(data!.addComment.commentEdge.node); if (remote.isEnterprise && comment.user) { - await replaceAvatarUrl(comment.user, octokit); + await _avatars.replaceAvatarUrl(comment.user, octokit); } return comment; @@ -223,6 +230,7 @@ export class IssueModel { async editIssueComment(comment: IComment, text: string): Promise { try { + const _avatars = this._avatars; const { mutate, schema, remote, octokit } = await this.githubRepository.ensure(); const { data } = await mutate({ @@ -238,7 +246,7 @@ export class IssueModel { const newComment = parseGraphQlIssueComment(data!.updateIssueComment.issueComment); if (remote.isEnterprise && newComment.user) { - await replaceAvatarUrl(newComment.user, octokit); + await _avatars.replaceAvatarUrl(newComment.user, octokit); } return newComment; @@ -274,7 +282,9 @@ export class IssueModel { // We don't get a nice error message from the API when setting labels fails. // Since adding labels isn't a critical part of the PR creation path it's safe to catch all errors that come from setting labels. Logger.error(`Failed to add labels to PR #${this.number}`, IssueModel.ID); - vscode.window.showWarningMessage(vscode.l10n.t('Some, or all, labels could not be added to the pull request.')); + vscode.window.showWarningMessage( + vscode.l10n.t('Some, or all, labels could not be added to the pull request.'), + ); } } @@ -290,6 +300,7 @@ export class IssueModel { async getIssueTimelineEvents(): Promise { Logger.debug(`Fetch timeline events of issue #${this.number} - enter`, IssueModel.ID); + const _avatars = this._avatars; const githubRepository = this.githubRepository; const { query, remote, schema, octokit } = await githubRepository.ensure(); @@ -306,7 +317,7 @@ export class IssueModel { const events = parseGraphQLTimelineEvents(ret); if (remote.isEnterprise) { - await replaceTimelineEventAvatarUrls(events, octokit); + await _avatars.replaceTimelineEventAvatarUrls(events, octokit); } return events; diff --git a/src/github/pullRequestModel.ts b/src/github/pullRequestModel.ts index 6fef82ed3a..edf5e3d905 100644 --- a/src/github/pullRequestModel.ts +++ b/src/github/pullRequestModel.ts @@ -18,6 +18,7 @@ import { ITelemetry } from '../common/telemetry'; import { ReviewEvent as CommonReviewEvent, EventType, TimelineEvent } from '../common/timelineEvent'; import { resolvePath, Schemes, toPRUri, toReviewUri } from '../common/uri'; import { formatError } from '../common/utils'; +import { Avatars } from './avatars'; import { OctokitCommon } from './common'; import { CredentialStore } from './credentials'; import { FolderRepositoryManager } from './folderRepositoryManager'; @@ -69,9 +70,6 @@ import { parseGraphQLReviewThread, parseGraphQLTimelineEvents, parseMergeability, - replaceAccountAvatarUrls, - replaceAvatarUrl, - replaceTimelineEventAvatarUrls, restPaginate, } from './utils'; @@ -146,9 +144,10 @@ export class PullRequestModel extends IssueModel implements IPullRe githubRepository: GitHubRepository, remote: Remote, item: PullRequest, + avatars: Avatars, isActive?: boolean, ) { - super(githubRepository, remote, item, true); + super(githubRepository, remote, item, avatars, true); this._telemetry = telemetry; this.isActive = !!isActive; @@ -344,6 +343,7 @@ export class PullRequestModel extends IssueModel implements IPullRe * Close the pull request. */ async close(): Promise { + const _avatars = this._avatars; const { octokit, remote } = await this.githubRepository.ensure(); const ret = await octokit.call(octokit.api.pulls.update, { owner: remote.owner, @@ -360,7 +360,7 @@ export class PullRequestModel extends IssueModel implements IPullRe const pr = convertRESTPullRequestToRawPullRequest(ret.data); if (remote.isEnterprise) { - await replaceAccountAvatarUrls(pr, octokit); + await _avatars.replaceAccountAvatarUrls(pr, octokit); } return pr; @@ -372,6 +372,7 @@ export class PullRequestModel extends IssueModel implements IPullRe * @param message The summary comment text. */ private async createReview(event: ReviewEvent, message?: string): Promise { + const _avatars = this._avatars; const { octokit, remote } = await this.githubRepository.ensure(); const { data } = await octokit.call(octokit.api.pulls.createReview, { @@ -385,7 +386,7 @@ export class PullRequestModel extends IssueModel implements IPullRe const reviewEvent = convertRESTReviewEvent(data); if (remote.isEnterprise) { - await replaceAvatarUrl(reviewEvent.user, octokit); + await _avatars.replaceAvatarUrl(reviewEvent.user, octokit); } return reviewEvent; @@ -398,6 +399,7 @@ export class PullRequestModel extends IssueModel implements IPullRe */ async submitReview(event?: ReviewEvent, body?: string): Promise { let pendingReviewId = await this.getPendingReviewId(); + const _avatars = this._avatars; const { mutate, schema, remote, octokit } = await this.githubRepository.ensure(); if (!pendingReviewId && (event === ReviewEvent.Comment)) { @@ -421,9 +423,9 @@ export class PullRequestModel extends IssueModel implements IPullRe if (remote.isEnterprise) { await Promise.all([ - replaceAvatarUrl(reviewEvent.user, octokit), + _avatars.replaceAvatarUrl(reviewEvent.user, octokit), ...reviewEvent.comments.map(comment => - Promise.resolve(comment.user && replaceAvatarUrl(comment.user, octokit)), + Promise.resolve(comment.user && _avatars.replaceAvatarUrl(comment.user, octokit)), ), ]); } @@ -519,6 +521,7 @@ export class PullRequestModel extends IssueModel implements IPullRe */ async deleteReview(): Promise<{ deletedReviewId: number; deletedReviewComments: IComment[] }> { const pendingReviewId = await this.getPendingReviewId(); + const _avatars = this._avatars; const { mutate, schema, remote, octokit } = await this.githubRepository.ensure(); const { data } = await mutate({ mutation: schema.DeleteReview, @@ -537,7 +540,11 @@ export class PullRequestModel extends IssueModel implements IPullRe const reviewComments = comments.nodes.map(comment => parseGraphQLComment(comment, false)); if (remote.isEnterprise) { - await Promise.all(reviewComments.map(comment => Promise.resolve(comment.user && replaceAvatarUrl(comment.user, octokit)))); + await Promise.all( + reviewComments.map(comment => + Promise.resolve(comment.user && _avatars.replaceAvatarUrl(comment.user, octokit)), + ), + ); } return { @@ -597,7 +604,7 @@ export class PullRequestModel extends IssueModel implements IPullRe return; } const pendingReviewId = await this.getPendingReviewId(); - + const _avatars = this._avatars; const { mutate, schema, remote, octokit } = await this.githubRepository.ensure(); const { data } = await mutate({ mutation: schema.AddReviewThread, @@ -632,7 +639,11 @@ export class PullRequestModel extends IssueModel implements IPullRe const newThread = parseGraphQLReviewThread(thread); if (remote.isEnterprise) { - await Promise.all(newThread.comments.map(comment => Promise.resolve(comment.user && replaceAvatarUrl(comment.user, octokit)))); + await Promise.all( + newThread.comments.map(comment => + Promise.resolve(comment.user && _avatars.replaceAvatarUrl(comment.user, octokit)), + ), + ); } this._reviewThreadsCache.push(newThread); @@ -664,6 +675,7 @@ export class PullRequestModel extends IssueModel implements IPullRe pendingReviewId = await this.startReview(commitId); } + const _avatars = this._avatars; const { mutate, schema, remote, octokit } = await this.githubRepository.ensure(); const { data } = await mutate({ mutation: schema.AddComment, @@ -685,7 +697,7 @@ export class PullRequestModel extends IssueModel implements IPullRe const newComment = parseGraphQLComment(comment, false); if (remote.isEnterprise && newComment.user) { - await replaceAvatarUrl(newComment.user, octokit); + await _avatars.replaceAvatarUrl(newComment.user, octokit); } if (isSingleComment) { @@ -729,6 +741,7 @@ export class PullRequestModel extends IssueModel implements IPullRe * @param text The new comment text */ async editReviewComment(comment: IComment, text: string): Promise { + const _avatars = this._avatars; const { mutate, schema, remote, octokit } = await this.githubRepository.ensure(); let threadWithComment = this._reviewThreadsCache.find(thread => thread.comments.some(c => c.graphNodeId === comment.graphNodeId), @@ -758,7 +771,7 @@ export class PullRequestModel extends IssueModel implements IPullRe ); if (remote.isEnterprise && newComment.user) { - await replaceAvatarUrl(newComment.user, octokit); + await _avatars.replaceAvatarUrl(newComment.user, octokit); } if (threadWithComment) { @@ -811,6 +824,7 @@ export class PullRequestModel extends IssueModel implements IPullRe */ async getReviewRequests(): Promise<(IAccount | ITeam)[]> { const githubRepository = this.githubRepository; + const _avatars = this._avatars; const { remote, query, schema, octokit } = await githubRepository.ensure(); const { data } = await query({ @@ -847,7 +861,7 @@ export class PullRequestModel extends IssueModel implements IPullRe } if (remote.isEnterprise) { - await Promise.all(reviewers.map(reviewer => replaceAvatarUrl(reviewer, octokit))); + await Promise.all(reviewers.map(reviewer => _avatars.replaceAvatarUrl(reviewer, octokit))); } return reviewers; @@ -931,6 +945,7 @@ export class PullRequestModel extends IssueModel implements IPullRe } async getReviewThreads(): Promise { + const _avatars = this._avatars; const { remote, query, schema, octokit } = await this.githubRepository.ensure(); try { const { data } = await query({ @@ -951,7 +966,7 @@ export class PullRequestModel extends IssueModel implements IPullRe reviewThreads .map(thread => thread.comments.map(comment => - Promise.resolve(comment.user && replaceAvatarUrl(comment.user, octokit)), + Promise.resolve(comment.user && _avatars.replaceAvatarUrl(comment.user, octokit)), ), ) .flat(), @@ -972,6 +987,7 @@ export class PullRequestModel extends IssueModel implements IPullRe * Get all review comments. */ async initializeReviewComments(): Promise { + const _avatars = this._avatars; const { remote, query, schema, octokit } = await this.githubRepository.ensure(); try { const { data } = await query({ @@ -991,7 +1007,9 @@ export class PullRequestModel extends IssueModel implements IPullRe }); if (remote.isEnterprise) { - await Promise.all(comments.map(comment => comment.user && replaceAvatarUrl(comment.user, octokit))); + await Promise.all( + comments.map(comment => comment.user && _avatars.replaceAvatarUrl(comment.user, octokit)), + ); } this.comments = comments; @@ -1079,6 +1097,7 @@ export class PullRequestModel extends IssueModel implements IPullRe */ async getTimelineEvents(): Promise { Logger.debug(`Fetch timeline events of PR #${this.number} - enter`, PullRequestModel.ID); + const _avatars = this._avatars; const { query, remote, schema, octokit } = await this.githubRepository.ensure(); try { @@ -1100,7 +1119,7 @@ export class PullRequestModel extends IssueModel implements IPullRe const events = parseGraphQLTimelineEvents(ret); if (remote.isEnterprise) { - await replaceTimelineEventAvatarUrls(events, octokit); + await _avatars.replaceTimelineEventAvatarUrls(events, octokit); } this.addReviewTimelineEventComments(events, reviewThreads); @@ -1304,6 +1323,7 @@ export class PullRequestModel extends IssueModel implements IPullRe PullRequestModel.ID, ); const githubRepository = this.githubRepository; + const _avatars = this._avatars; const { octokit, remote } = await githubRepository.ensure(); if (!this.base) { @@ -1316,7 +1336,7 @@ export class PullRequestModel extends IssueModel implements IPullRe const pr = convertRESTPullRequestToRawPullRequest(info.data); if (remote.isEnterprise) { - await replaceAccountAvatarUrls(pr, octokit); + await _avatars.replaceAccountAvatarUrls(pr, octokit); } this.update(pr); @@ -1522,6 +1542,7 @@ export class PullRequestModel extends IssueModel implements IPullRe } async resolveReviewThread(threadId: string): Promise { + const _avatars = this._avatars; const { mutate, schema, remote, octokit } = await this.githubRepository.ensure(); // optimistically update @@ -1560,7 +1581,7 @@ export class PullRequestModel extends IssueModel implements IPullRe if (remote.isEnterprise) { await Promise.all( thread.comments.map(comment => - Promise.resolve(comment.user && replaceAvatarUrl(comment.user, octokit)), + Promise.resolve(comment.user && _avatars.replaceAvatarUrl(comment.user, octokit)), ), ); } @@ -1571,6 +1592,7 @@ export class PullRequestModel extends IssueModel implements IPullRe } async unresolveReviewThread(threadId: string): Promise { + const _avatars = this._avatars; const { mutate, schema, remote, octokit } = await this.githubRepository.ensure(); // optimistically update @@ -1609,7 +1631,7 @@ export class PullRequestModel extends IssueModel implements IPullRe if (remote.isEnterprise) { await Promise.all( thread.comments.map(comment => - Promise.resolve(comment.user && replaceAvatarUrl(comment.user, octokit)), + Promise.resolve(comment.user && _avatars.replaceAvatarUrl(comment.user, octokit)), ), ); } diff --git a/src/github/utils.ts b/src/github/utils.ts index 83e0a8b03d..0c6037a041 100644 --- a/src/github/utils.ts +++ b/src/github/utils.ts @@ -4,9 +4,7 @@ *--------------------------------------------------------------------------------------------*/ 'use strict'; -import * as crypto from 'crypto'; import * as OctokitTypes from '@octokit/types'; -import PQueue from 'p-queue'; import * as vscode from 'vscode'; import { Repository } from '../api/api'; import { GitApiImpl } from '../api/api1'; @@ -41,7 +39,6 @@ import { User, } from './interface'; import { IssueModel } from './issueModel'; -import { LoggingOctokit } from './loggingOctokit'; import { GHPRComment, GHPRCommentThread } from './prComment'; import { PullRequestModel } from './pullRequestModel'; @@ -1107,179 +1104,6 @@ export function hasEnterpriseUri(): boolean { return !!getEnterpriseUri(); } -const GRAVATAR_STYLE_NONE = 'none'; - -function isGravatarEnabled() { - return getGravatarStyle() !== GRAVATAR_STYLE_NONE; -} - -function getGravatarStyle() { - return vscode.workspace.getConfiguration('githubPullRequests').get('defaultGravatarsStyle', GRAVATAR_STYLE_NONE); -} - -function generateGravatarUrl(gravatarId: string | undefined, size: number = 200): string | undefined { - if (!gravatarId || !isGravatarEnabled()) { - return undefined; - } - - return `https://www.gravatar.com/avatar/${gravatarId}?s=${size}&d=${getGravatarStyle()}`; -} - -// This limits the concurrent promises that fetch avatars from the Enterprise REST service -const enterpriseAvatarQueue = new PQueue({concurrency: 3}); - -// This is an in-memory cache of Enterprise avatar data URIs -const enterpriseAvatarCache: {[k: string]: Promise} = {}; - -async function getEnterpriseAvatarUrl(avatarUrl: string | undefined, octokit: LoggingOctokit): Promise { - try { - if (!avatarUrl || !hasEnterpriseUri()) { - return; - } - - const avatarUri = vscode.Uri.parse(avatarUrl, true); - const enterpriseUri = getEnterpriseUri()!; - const enterpriseAvatarRestBase = '/enterprise/avatars'; - - // static asset from enterprise does not need authentication - if (avatarUri.scheme === 'data' || avatarUri.authority === `assets.${enterpriseUri.authority}`) { - return avatarUrl; - } - - // only proxy avatars from the "avatars" sub-domain of Enterprise - if (avatarUri.authority !== `avatars.${enterpriseUri.authority}`) { - return; - } - - const cacheKey = `${avatarUri.path}?${avatarUri.query}`; - const options = {}; - const qs = new URLSearchParams(avatarUri.query); - - qs.forEach((v, k) => { - options[k] = v; - }); - - if (!(cacheKey in enterpriseAvatarCache)) { - enterpriseAvatarCache[cacheKey] = enterpriseAvatarQueue.add(() => - octokit.api.request(`GET ${enterpriseAvatarRestBase}${avatarUri.path}`, options).then( - resp => { - const dataUri = `data:${resp.headers['content-type']};base64,${Buffer.from(resp.data).toString( - 'base64', - )}`; - return dataUri; - }, - () => { - delete enterpriseAvatarCache[cacheKey]; - return; - }, - ), - ); - } - - const avatarDataUri = await enterpriseAvatarCache[cacheKey]; - if (avatarDataUri) { - return avatarDataUri; - } - } catch { - // ignore - } -} - -export async function replaceAvatarUrl(user: IAccount | ITeam, octokit: LoggingOctokit): Promise { - const origAvatarUrl = user.avatarUrl; - user.avatarUrl = undefined; - - const enterpriseAvatarUrl = await getEnterpriseAvatarUrl(origAvatarUrl, octokit); - if (enterpriseAvatarUrl) { - user.avatarUrl = enterpriseAvatarUrl; - return; - } - - if (!('login' in user)) { - return; - } - - if (user.email === undefined && user.login) { - try { - const { data } = await octokit.call(octokit.api.users.getByUsername, { - username: user.login - }); - - user.email = data.email || undefined; - } catch { - // ignore - } - } - - if (!user.email) { - return; - } - - user.avatarUrl = generateGravatarUrl(crypto.createHash('md5').update(user.email.trim().toLowerCase()).digest('hex')); -} - -export function replaceAccountAvatarUrls(pr: PullRequest, octokit: LoggingOctokit): Promise { - const promises: Promise[] = []; - promises.push(replaceAvatarUrl(pr.user, octokit)); - if (pr.assignees) { - promises.push(...pr.assignees.map(user => replaceAvatarUrl(user, octokit))); - } - if (pr.suggestedReviewers) { - promises.push(...pr.suggestedReviewers.map(user => replaceAvatarUrl(user, octokit))); - } - return Promise.all(promises); -} - -export function replaceTimelineEventAvatarUrls(events: Common.TimelineEvent[], octokit: LoggingOctokit): Promise { - const promises: Promise[] = []; - - for (const event of events) { - const type = event.event; - switch (type) { - case Common.EventType.Commented: - const commentEvent = event as Common.CommentEvent; - promises.push(replaceAvatarUrl(commentEvent.user, octokit)); - break; - case Common.EventType.Reviewed: - const reviewEvent = event as Common.ReviewEvent; - promises.push(replaceAvatarUrl(reviewEvent.user, octokit)); - break; - case Common.EventType.Committed: - const commitEv = event as Common.CommitEvent; - promises.push(replaceAvatarUrl(commitEv.author, octokit)); - break; - case Common.EventType.Merged: - const mergeEv = event as Common.MergedEvent; - promises.push(replaceAvatarUrl(mergeEv.user, octokit)); - break; - case Common.EventType.Assigned: - const assignEv = event as Common.AssignEvent; - promises.push(replaceAvatarUrl(assignEv.user, octokit)); - promises.push(replaceAvatarUrl(assignEv.actor, octokit)); - break; - case Common.EventType.HeadRefDeleted: - const deletedEv = event as Common.HeadRefDeleteEvent; - promises.push(replaceAvatarUrl(deletedEv.actor, octokit)); - break; - } - } - - return Promise.all(promises); -} - -export function replaceIssuesAvatarUrls(issues: Issue[], octokit: LoggingOctokit): Promise { - const promises: Promise[] = []; - - for (const issue of issues) { - promises.push(replaceAvatarUrl(issue.user, octokit)); - if (issue.assignees) { - promises.push(...issue.assignees.map(user => replaceAvatarUrl(user, octokit))); - } - } - - return Promise.all(promises); -} - export function getPullsUrl(repo: GitHubRepository) { return vscode.Uri.parse(`https://${repo.remote.host}/${repo.remote.owner}/${repo.remote.repositoryName}/pulls`); } diff --git a/src/test/github/folderRepositoryManager.test.ts b/src/test/github/folderRepositoryManager.test.ts index a3919c19a9..ae85473aba 100644 --- a/src/test/github/folderRepositoryManager.test.ts +++ b/src/test/github/folderRepositoryManager.test.ts @@ -21,6 +21,7 @@ import { CredentialStore } from '../../github/credentials'; import { MockExtensionContext } from '../mocks/mockExtensionContext'; import { Uri } from 'vscode'; import { GitHubServerType } from '../../common/authentication'; +import { Avatars } from '../../github/avatars'; describe('PullRequestManager', function () { let sinon: SinonSandbox; @@ -53,9 +54,10 @@ describe('PullRequestManager', function () { const protocol = new Protocol(url); const remote = new GitHubRemote('origin', url, protocol, GitHubServerType.GitHubDotCom); const rootUri = Uri.file('C:\\users\\test\\repo'); - const repository = new GitHubRepository(remote, rootUri, manager.credentialStore, telemetry); + const avatars = new Avatars(new MockExtensionContext()) + const repository = new GitHubRepository(remote, rootUri, manager.credentialStore, telemetry, avatars); const prItem = convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().build()); - const pr = new PullRequestModel(manager.credentialStore, telemetry, repository, remote, prItem); + const pr = new PullRequestModel(manager.credentialStore, telemetry, repository, remote, prItem, avatars); manager.activePullRequest = pr; assert(changeFired.called); diff --git a/src/test/github/githubRepository.test.ts b/src/test/github/githubRepository.test.ts index 493f4a99f3..9a550bc4af 100644 --- a/src/test/github/githubRepository.test.ts +++ b/src/test/github/githubRepository.test.ts @@ -15,6 +15,7 @@ import { Uri } from 'vscode'; import { MockExtensionContext } from '../mocks/mockExtensionContext'; import { GitHubManager } from '../../authentication/githubServer'; import { GitHubServerType } from '../../common/authentication'; +import { Avatars } from '../../github/avatars'; describe('GitHubRepository', function () { let sinon: SinonSandbox; @@ -40,7 +41,8 @@ describe('GitHubRepository', function () { const url = 'https://github.com/some/repo'; const remote = new GitHubRemote('origin', url, new Protocol(url), GitHubServerType.GitHubDotCom); const rootUri = Uri.file('C:\\users\\test\\repo'); - const dotcomRepository = new GitHubRepository(remote, rootUri, credentialStore, telemetry); + const avatars = new Avatars(context) + const dotcomRepository = new GitHubRepository(remote, rootUri, credentialStore, telemetry, avatars); assert(GitHubManager.isGithubDotCom(Uri.parse(remote.url).authority)); }); @@ -48,7 +50,8 @@ describe('GitHubRepository', function () { const url = 'https://github.enterprise.horse/some/repo'; const remote = new GitHubRemote('origin', url, new Protocol(url), GitHubServerType.GitHubDotCom); const rootUri = Uri.file('C:\\users\\test\\repo'); - const dotcomRepository = new GitHubRepository(remote, rootUri, credentialStore, telemetry); + const avatars = new Avatars(context) + const dotcomRepository = new GitHubRepository(remote, rootUri, credentialStore, telemetry, avatars); // assert(! dotcomRepository.isGitHubDotCom); }); }); diff --git a/src/test/github/pullRequestGitHelper.test.ts b/src/test/github/pullRequestGitHelper.test.ts index 30a44329a0..ffeba5ebb2 100644 --- a/src/test/github/pullRequestGitHelper.test.ts +++ b/src/test/github/pullRequestGitHelper.test.ts @@ -21,6 +21,7 @@ import { RefType } from '../../api/api1'; import { RepositoryBuilder } from '../builders/rest/repoBuilder'; import { MockExtensionContext } from '../mocks/mockExtensionContext'; import { GitHubServerType } from '../../common/authentication'; +import { Avatars } from '../../github/avatars'; describe('PullRequestGitHelper', function () { let sinon: SinonSandbox; @@ -49,6 +50,7 @@ describe('PullRequestGitHelper', function () { const url = 'git@github.com:owner/name.git'; const remote = new GitHubRemote('elsewhere', url, new Protocol(url), GitHubServerType.GitHubDotCom); const gitHubRepository = new MockGitHubRepository(remote, credentialStore, telemetry, sinon); + const avatars = new Avatars(context); const prItem = convertRESTPullRequestToRawPullRequest( new PullRequestBuilder() @@ -67,7 +69,7 @@ describe('PullRequestGitHelper', function () { repository.expectFetch('you', 'my-branch:pr/me/100', 1); repository.expectPull(true); - const pullRequest = new PullRequestModel(credentialStore, telemetry, gitHubRepository, remote, prItem); + const pullRequest = new PullRequestModel(credentialStore, telemetry, gitHubRepository, remote, prItem, avatars); 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 2f70f42eb8..8e564fe379 100644 --- a/src/test/github/pullRequestModel.test.ts +++ b/src/test/github/pullRequestModel.test.ts @@ -19,6 +19,7 @@ import { NetworkStatus } from 'apollo-client'; import { Resource } from '../../common/resources'; import { MockExtensionContext } from '../mocks/mockExtensionContext'; import { GitHubServerType } from '../../common/authentication'; +import { Avatars } from '../../github/avatars'; const queries = require('../../github/queries.gql'); const telemetry = new MockTelemetry(); @@ -57,12 +58,14 @@ describe('PullRequestModel', function () { let credentials: CredentialStore; let repo: MockGitHubRepository; let context: MockExtensionContext; + let avatars: Avatars; beforeEach(function () { sinon = createSandbox(); MockCommandRegistry.install(sinon); context = new MockExtensionContext(); + avatars = new Avatars(context); credentials = new CredentialStore(telemetry, context); repo = new MockGitHubRepository(remote, credentials, telemetry, sinon); Resource.initialize(context); @@ -77,21 +80,21 @@ describe('PullRequestModel', function () { it('should return `state` properly as `open`', function () { const pr = new PullRequestBuilder().state('open').build(); - const open = new PullRequestModel(credentials, telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr)); + const open = new PullRequestModel(credentials, telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr), avatars); 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(credentials, telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr)); + const open = new PullRequestModel(credentials, telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr), avatars); 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(credentials, telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr)); + const open = new PullRequestModel(credentials, telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr), avatars); assert.strictEqual(open.state, GithubItemStateEnum.Merged); }); @@ -105,6 +108,7 @@ describe('PullRequestModel', function () { repo, remote, convertRESTPullRequestToRawPullRequest(pr), + avatars, ); repo.queryProvider.expectGraphQLQuery( diff --git a/src/test/github/pullRequestOverview.test.ts b/src/test/github/pullRequestOverview.test.ts index ccd11c6e52..c5144adfe0 100644 --- a/src/test/github/pullRequestOverview.test.ts +++ b/src/test/github/pullRequestOverview.test.ts @@ -23,6 +23,7 @@ import { CredentialStore } from '../../github/credentials'; import { GitHubServerType } from '../../common/authentication'; import { GitHubRemote } from '../../common/remote'; import { CheckState } from '../../github/interface'; +import { Avatars } from '../../github/avatars'; const EXTENSION_URI = vscode.Uri.joinPath(vscode.Uri.file(__dirname), '../../..'); @@ -34,11 +35,13 @@ describe('PullRequestOverview', function () { let repo: MockGitHubRepository; let telemetry: MockTelemetry; let credentialStore: CredentialStore; + let avatars: Avatars; beforeEach(async function () { sinon = createSandbox(); MockCommandRegistry.install(sinon); context = new MockExtensionContext(); + avatars = new Avatars(context); const repository = new MockRepository(); telemetry = new MockTelemetry(); @@ -74,7 +77,7 @@ describe('PullRequestOverview', function () { }); const prItem = convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(1000).build()); - const prModel = new PullRequestModel(credentialStore, telemetry, repo, remote, prItem); + const prModel = new PullRequestModel(credentialStore, telemetry, repo, remote, prItem, avatars); await PullRequestOverviewPanel.createOrShow(EXTENSION_URI, pullRequestManager, prModel); @@ -107,7 +110,7 @@ describe('PullRequestOverview', function () { }); const prItem0 = convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(1000).build()); - const prModel0 = new PullRequestModel(credentialStore, telemetry, repo, remote, prItem0); + const prModel0 = new PullRequestModel(credentialStore, telemetry, repo, remote, prItem0, avatars); const resolveStub = sinon.stub(pullRequestManager, 'resolvePullRequest').resolves(prModel0); sinon.stub(prModel0, 'getReviewRequests').resolves([]); sinon.stub(prModel0, 'getTimelineEvents').resolves([]); @@ -120,7 +123,7 @@ describe('PullRequestOverview', function () { assert.strictEqual(panel0!.getCurrentTitle(), 'Pull Request #1000'); const prItem1 = convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(2000).build()); - const prModel1 = new PullRequestModel(credentialStore, telemetry, repo, remote, prItem1); + const prModel1 = new PullRequestModel(credentialStore, telemetry, repo, remote, prItem1, avatars); resolveStub.resolves(prModel1); sinon.stub(prModel1, 'getReviewRequests').resolves([]); sinon.stub(prModel1, 'getTimelineEvents').resolves([]); diff --git a/src/test/mocks/mockGitHubRepository.ts b/src/test/mocks/mockGitHubRepository.ts index 63f5805936..63ff9115c3 100644 --- a/src/test/mocks/mockGitHubRepository.ts +++ b/src/test/mocks/mockGitHubRepository.ts @@ -20,13 +20,15 @@ import { import { MockTelemetry } from './mockTelemetry'; import { Uri } from 'vscode'; import { LoggingOctokit, RateLogger } from '../../github/loggingOctokit'; +import { MockExtensionContext } from './mockExtensionContext'; +import { Avatars } from '../../github/avatars'; const queries = require('../../github/queries.gql'); export class MockGitHubRepository extends GitHubRepository { readonly queryProvider: QueryProvider; constructor(remote: GitHubRemote, credentialStore: CredentialStore, telemetry: MockTelemetry, sinon: SinonSandbox) { - super(remote, Uri.file('C:\\users\\test\\repo'), credentialStore, telemetry); + super(remote, Uri.file('C:\\users\\test\\repo'), credentialStore, telemetry, new Avatars(new MockExtensionContext())); this.queryProvider = new QueryProvider(sinon); diff --git a/src/test/view/prsTree.test.ts b/src/test/view/prsTree.test.ts index c17641755f..6718809c9c 100644 --- a/src/test/view/prsTree.test.ts +++ b/src/test/view/prsTree.test.ts @@ -27,6 +27,7 @@ import { GitApiImpl } from '../../api/api1'; import { RepositoriesManager } from '../../github/repositoriesManager'; import { LoggingOctokit, RateLogger } from '../../github/loggingOctokit'; import { GitHubServerType } from '../../common/authentication'; +import { Avatars } from '../../github/avatars'; describe('GitHub Pull Requests view', function () { let sinon: SinonSandbox; @@ -34,12 +35,14 @@ describe('GitHub Pull Requests view', function () { let telemetry: MockTelemetry; let provider: PullRequestsTreeDataProvider; let credentialStore: CredentialStore; + let avatars: Avatars; beforeEach(function () { sinon = createSandbox(); MockCommandRegistry.install(sinon); context = new MockExtensionContext(); + avatars = new Avatars(context); telemetry = new MockTelemetry(); provider = new PullRequestsTreeDataProvider(telemetry, context); @@ -148,7 +151,7 @@ describe('GitHub Pull Requests view', function () { }); }).pullRequest; const prItem0 = parseGraphQLPullRequest(pr0.repository.pullRequest); - const pullRequest0 = new PullRequestModel(credentialStore, telemetry, gitHubRepository, remote, prItem0); + const pullRequest0 = new PullRequestModel(credentialStore, telemetry, gitHubRepository, remote, prItem0, avatars); const pr1 = gitHubRepository.addGraphQLPullRequest(builder => { builder.pullRequest(pr => { @@ -164,7 +167,7 @@ describe('GitHub Pull Requests view', function () { }); }).pullRequest; const prItem1 = parseGraphQLPullRequest(pr1.repository.pullRequest); - const pullRequest1 = new PullRequestModel(credentialStore, telemetry, gitHubRepository, remote, prItem1); + const pullRequest1 = new PullRequestModel(credentialStore, telemetry, gitHubRepository, remote, prItem1, avatars); 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 c6d1825de8..c6f83ced8e 100644 --- a/src/test/view/reviewCommentController.test.ts +++ b/src/test/view/reviewCommentController.test.ts @@ -36,6 +36,7 @@ import { GitFileChangeModel } from '../../view/fileChangeModel'; import { WebviewViewCoordinator } from '../../view/webviewViewCoordinator'; import { GitHubServerType } from '../../common/authentication'; import { CreatePullRequestHelper } from '../../view/createPullRequestHelper'; +import { Avatars } from '../../github/avatars'; const schema = require('../../github/queries.gql'); const protocol = new Protocol('https://github.com/github/test.git'); @@ -65,6 +66,7 @@ describe('ReviewCommentController', function () { telemetry = new MockTelemetry(); const context = new MockExtensionContext(); credentialStore = new CredentialStore(telemetry, context); + const avatars = new Avatars(context); repository = new MockRepository(); repository.addRemote('origin', 'git@github.com:aaa/bbb'); @@ -91,6 +93,7 @@ describe('ReviewCommentController', function () { githubRepo, remote, convertRESTPullRequestToRawPullRequest(pr), + avatars, ); manager.activePullRequest = activePullRequest; diff --git a/webpack.config.js b/webpack.config.js index 599f17e0eb..32abef5b13 100644 --- a/webpack.config.js +++ b/webpack.config.js @@ -297,20 +297,6 @@ async function getExtensionConfig(target, mode, env) { alias: target === 'webworker' ? { - 'universal-user-agent': path.join( - __dirname, - 'node_modules', - 'universal-user-agent', - 'dist-web', - 'index.js', - ), - 'p-queue': path.join( - __dirname, - 'node_modules', - 'p-queue', - 'dist', - 'index.js', - ), 'node-fetch': 'cross-fetch', '../env/node/net': path.resolve(__dirname, 'src', 'env', 'browser', 'net'), '../env/node/ssh': path.resolve(__dirname, 'src', 'env', 'browser', 'ssh'), @@ -325,9 +311,6 @@ async function getExtensionConfig(target, mode, env) { ) } : undefined, - // : { - // 'universal-user-agent': path.join(__dirname, 'node_modules', 'universal-user-agent', 'dist-node', 'index.js'), - // }, fallback: target === 'webworker' ? { @@ -339,7 +322,8 @@ async function getExtensionConfig(target, mode, env) { 'os': require.resolve('os-browserify/browser'), "constants": require.resolve("constants-browserify"), buffer: require.resolve('buffer'), - timers: require.resolve('timers-browserify') + timers: require.resolve('timers-browserify'), + 'p-queue': require.resolve('p-queue'), } : undefined, extensions: ['.ts', '.tsx', '.js', '.jsx', '.json'], From d31153631ead9a0f0d7fd236ff03305c641d4234 Mon Sep 17 00:00:00 2001 From: Kevin Abel Date: Wed, 26 Apr 2023 16:41:44 -0500 Subject: [PATCH 4/4] Rework centralized config strings --- src/common/settingKeys.ts | 1 + src/github/avatars.ts | 9 +++++---- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/common/settingKeys.ts b/src/common/settingKeys.ts index bed8634e94..7ce447167e 100644 --- a/src/common/settingKeys.ts +++ b/src/common/settingKeys.ts @@ -27,6 +27,7 @@ export const DEFAULT_DELETION_METHOD = 'defaultDeletionMethod'; export const SELECT_LOCAL_BRANCH = 'selectLocalBranch'; export const SELECT_REMOTE = 'selectRemote'; export const REMOTES = 'remotes'; +export const DEFAULT_GRAVATAR_STYLE = 'defaultGravatarsStyle'; export const ISSUES_SETTINGS_NAMESPACE = 'githubIssues'; export const ASSIGN_WHEN_WORKING = 'assignWhenWorking'; diff --git a/src/github/avatars.ts b/src/github/avatars.ts index 0cc06f0acb..39db395ef0 100644 --- a/src/github/avatars.ts +++ b/src/github/avatars.ts @@ -10,6 +10,7 @@ import { OctokitResponse, RequestParameters, ResponseHeaders } from '@octokit/ty import PQueue from 'p-queue'; import * as vscode from 'vscode'; import Logger from '../common/logger'; +import { DEFAULT_GRAVATAR_STYLE, PR_SETTINGS_NAMESPACE } from '../common/settingKeys'; import * as Common from '../common/timelineEvent'; import { IAccount, Issue, ITeam, PullRequest } from './interface'; import { LoggingOctokit } from './loggingOctokit'; @@ -23,8 +24,8 @@ function isGravatarEnabled() { function getGravatarStyle() { return vscode.workspace - .getConfiguration('githubPullRequests') - .get('defaultGravatarsStyle', GRAVATAR_STYLE_NONE); + .getConfiguration(PR_SETTINGS_NAMESPACE) + .get(DEFAULT_GRAVATAR_STYLE, GRAVATAR_STYLE_NONE); } function generateGravatarUrl(gravatarId: string | undefined, size: number = 200): string | undefined { @@ -168,7 +169,7 @@ export class Avatars { const serverDate = headers.date ? new Date(headers.date) : new Date(); const expireAt = serverDate.setSeconds( serverDate.getSeconds() + - (cacheControlDirectives['s-maxage'] ?? cacheControlDirectives['max-age'] ?? 0), + (cacheControlDirectives['s-maxage'] ?? cacheControlDirectives['max-age'] ?? 0), ); if (expireAt - Date.now() > 0) { Logger.appendLine('Cache fresh hit', LOGGER_COMPONENT); @@ -249,7 +250,7 @@ export class Avatars { await vscode.workspace.fs.writeFile(cacheResult.uri, cacheResult.content); return convertBinaryToDataUri(cacheResult.content, cacheResult.contentType); }, - (reason: RequestError) => { + (reason: RequestError) => { if (reason.status !== 304) { Logger.warn(`REST request failed: ${reason.message}`, LOGGER_COMPONENT); return;