Skip to content

Commit cc1e212

Browse files
StanleyGoldmanStanleyGoldman
authored
Merge pull request #1121 from Microsoft/fixes/github-enterprise-user-avatar
Pull Request Tree GitHub Enterprise Avatars
2 parents 04e1a76 + 3abb635 commit cc1e212

File tree

10 files changed

+88
-31
lines changed

10 files changed

+88
-31
lines changed

‎package.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@
138138
{
139139
"id": "github-pull-requests",
140140
"title": "GitHub Pull Requests",
141-
"icon": "resources/icons/github.svg"
141+
"icon": "resources/icons/light/github.svg"
142142
}
143143
]
144144
},

‎resources/icons/dark/github.svg

+6
Loading

‎resources/icons/github.svg renamed to ‎resources/icons/light/github.svg

File renamed without changes.

‎src/common/resources.ts

+2
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ export class Resource {
2323
Ignored: context.asAbsolutePath(path.join('resources', 'icons', 'light', 'status-ignored.svg')),
2424
Conflict: context.asAbsolutePath(path.join('resources', 'icons', 'light', 'status-conflict.svg')),
2525
Comment: context.asAbsolutePath(path.join('resources', 'icons', 'comment.svg')),
26+
Avatar: context.asAbsolutePath(path.join('resources', 'icons', 'light', 'github.svg')),
2627
Fold: context.asAbsolutePath(path.join('resources', 'icons', 'light', 'fold.svg')),
2728
Description: context.asAbsolutePath(path.join('resources', 'icons', 'light', 'git-pull-request.svg'))
2829
},
@@ -36,6 +37,7 @@ export class Resource {
3637
Ignored: context.asAbsolutePath(path.join('resources', 'icons', 'dark', 'status-ignored.svg')),
3738
Conflict: context.asAbsolutePath(path.join('resources', 'icons', 'dark', 'status-conflict.svg')),
3839
Comment: context.asAbsolutePath(path.join('resources', 'icons', 'comment.svg')),
40+
Avatar: context.asAbsolutePath(path.join('resources', 'icons', 'dark', 'github.svg')),
3941
Fold: context.asAbsolutePath(path.join('resources', 'icons', 'dark', 'fold.svg')),
4042
Description: context.asAbsolutePath(path.join('resources', 'icons', 'dark', 'git-pull-request.svg'))
4143
},

‎src/github/githubRepository.ts

+58-14
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import { PRDocumentCommentProvider } from '../view/prDocumentCommentProvider';
1616
import { convertRESTPullRequestToRawPullRequest, parseGraphQLPullRequest } from './utils';
1717
import { PullRequestResponse, MentionableUsersResponse } from './graphql';
1818
const queries = require('./queries.gql');
19+
import axois, { AxiosResponse } from 'axios';
1920

2021
export const PULL_REQUEST_PAGE_SIZE = 20;
2122

@@ -30,6 +31,7 @@ export class GitHubRepository implements IGitHubRepository, vscode.Disposable {
3031
static ID = 'GitHubRepository';
3132
private _hub: GitHub | undefined;
3233
private _initialized: boolean;
34+
private _repositoryReturnsAvatar: boolean | null;
3335
private _metadata: any;
3436
private _toDispose: vscode.Disposable[] = [];
3537
public commentsController?: vscode.CommentController;
@@ -72,6 +74,7 @@ export class GitHubRepository implements IGitHubRepository, vscode.Disposable {
7274
}
7375

7476
constructor(public remote: Remote, private readonly _credentialStore: CredentialStore) {
77+
this._repositoryReturnsAvatar = remote.host.toLowerCase() === 'github.com' ? true : null;
7578
}
7679

7780
get supportsGraphQl(): boolean {
@@ -206,7 +209,29 @@ export class GitHubRepository implements IGitHubRepository, vscode.Disposable {
206209
}
207210

208211
async getPullRequests(prType: PRType, page?: number): Promise<PullRequestData | undefined> {
209-
return prType === PRType.All ? this.getAllPullRequests(page) : this.getPullRequestsForCategory(prType, page);
212+
return await (prType === PRType.All ? this.getAllPullRequests(page) : this.getPullRequestsForCategory(prType, page));
213+
}
214+
215+
public async ensureRepositoryReturnsAvatar(testAvatarUrl: string): Promise<boolean> {
216+
if (this._repositoryReturnsAvatar === null) {
217+
let response: AxiosResponse | null = null;
218+
219+
try {
220+
response = await axois({method: 'get', url: testAvatarUrl, maxRedirects: 0});
221+
} catch (err) {
222+
if(err && err instanceof Error) {
223+
response = (<any> err).response as AxiosResponse;
224+
}
225+
}
226+
227+
if (response && response.status === 200) {
228+
this._repositoryReturnsAvatar = true;
229+
}
230+
231+
this._repositoryReturnsAvatar = false;
232+
}
233+
234+
return this._repositoryReturnsAvatar;
210235
}
211236

212237
private async getAllPullRequests(page?: number): Promise<PullRequestData | undefined> {
@@ -221,6 +246,12 @@ export class GitHubRepository implements IGitHubRepository, vscode.Disposable {
221246
});
222247

223248
const hasMorePages = !!result.headers.link && result.headers.link.indexOf('rel="next"') > -1;
249+
250+
let repoReturnsAvatar: boolean = true;
251+
if (result && result.data.length > 0) {
252+
repoReturnsAvatar = await this.ensureRepositoryReturnsAvatar(result.data[0].user.avatar_url);
253+
}
254+
224255
if (!result.data) {
225256
// We really don't expect this to happen, but it seems to (see #574).
226257
// Log a warning and return an empty set.
@@ -242,7 +273,8 @@ export class GitHubRepository implements IGitHubRepository, vscode.Disposable {
242273
}
243274

244275
const item = convertRESTPullRequestToRawPullRequest(pullRequest);
245-
return new PullRequestModel(this, this.remote, item);
276+
277+
return new PullRequestModel(this, this.remote, item, repoReturnsAvatar);
246278
}
247279
)
248280
.filter(item => item !== null) as PullRequestModel[];
@@ -288,15 +320,23 @@ export class GitHubRepository implements IGitHubRepository, vscode.Disposable {
288320
});
289321

290322
const hasMorePages = !!headers.link && headers.link.indexOf('rel="next"') > -1;
291-
const pullRequests = await Promise.all(promises).then(values => {
292-
return values.map(item => {
293-
if (!item.data.head.repo) {
294-
Logger.appendLine('GitHubRepository> The remote branch for this PR was already deleted.');
295-
return null;
296-
}
297-
return new PullRequestModel(this, this.remote, convertRESTPullRequestToRawPullRequest(item.data));
298-
}).filter(item => item !== null) as PullRequestModel[];
299-
});
323+
const pullRequestResponses = await Promise.all(promises);
324+
325+
let repoReturnsAvatar = true;
326+
if (pullRequestResponses && pullRequestResponses.length > 0) {
327+
repoReturnsAvatar = await this.ensureRepositoryReturnsAvatar(pullRequestResponses[0].data.user.avatar_url);
328+
}
329+
330+
const pullRequests = pullRequestResponses.map(response => {
331+
if (!response.data.head.repo) {
332+
Logger.appendLine('GitHubRepository> The remote branch for this PR was already deleted.');
333+
return null;
334+
}
335+
336+
const item = convertRESTPullRequestToRawPullRequest(response.data,);
337+
return new PullRequestModel(this, this.remote, item, repoReturnsAvatar);
338+
}).filter(item => item !== null) as PullRequestModel[];
339+
300340
Logger.debug(`Fetch pull request catogory ${PRType[prType]} - done`, GitHubRepository.ID);
301341

302342
return {
@@ -328,9 +368,11 @@ export class GitHubRepository implements IGitHubRepository, vscode.Disposable {
328368
number: id
329369
}
330370
});
331-
332371
Logger.debug(`Fetch pull request ${id} - done`, GitHubRepository.ID);
333-
return new PullRequestModel(this, remote, parseGraphQLPullRequest(data));
372+
373+
const repoReturnsAvatar = await this.ensureRepositoryReturnsAvatar(data.repository.pullRequest.author.avatarUrl);
374+
375+
return new PullRequestModel(this, remote, parseGraphQLPullRequest(data), repoReturnsAvatar);
334376
} else {
335377
let { data } = await octokit.pullRequests.get({
336378
owner: remote.owner,
@@ -339,13 +381,15 @@ export class GitHubRepository implements IGitHubRepository, vscode.Disposable {
339381
});
340382
Logger.debug(`Fetch pull request ${id} - done`, GitHubRepository.ID);
341383

384+
const repoReturnsAvatar = await this.ensureRepositoryReturnsAvatar(data.user.avatar_url);
385+
342386
if (!data.head.repo) {
343387
Logger.appendLine('The remote branch for this PR was already deleted.', GitHubRepository.ID);
344388
return;
345389
}
346390

347391
let item = convertRESTPullRequestToRawPullRequest(data);
348-
return new PullRequestModel(this, remote, item);
392+
return new PullRequestModel(this, remote, item, repoReturnsAvatar);
349393
}
350394
} catch (e) {
351395
Logger.appendLine(`GithubRepository> Unable to fetch PR: ${e}`);

‎src/github/pullRequestManager.ts

+2-1
Original file line numberDiff line numberDiff line change
@@ -1050,7 +1050,8 @@ export class PullRequestManager {
10501050
// Create PR
10511051
let { data } = await repo.octokit.pullRequests.create(params);
10521052
const item = convertRESTPullRequestToRawPullRequest(data);
1053-
const pullRequestModel = new PullRequestModel(repo, repo.remote, item);
1053+
const repoReturnsAvatar = await repo.ensureRepositoryReturnsAvatar(item.user.avatarUrl);
1054+
const pullRequestModel = new PullRequestModel(repo, repo.remote, item, repoReturnsAvatar);
10541055

10551056
const branchNameSeparatorIndex = params.head.indexOf(':');
10561057
const branchName = params.head.slice(branchNameSeparatorIndex + 1);

‎src/github/pullRequestModel.ts

+13-11
Original file line numberDiff line numberDiff line change
@@ -28,25 +28,27 @@ export class PullRequestModel {
2828
return this.state === PullRequestStateEnum.Merged;
2929
}
3030

31-
public get userAvatar(): string {
32-
if (this.prItem) {
31+
public get userAvatar(): string | undefined {
32+
if (this.prItem && this._repositoryReturnsAvatar) {
3333
return this.prItem.user.avatarUrl;
3434
}
3535

36-
return '';
36+
return undefined;
3737
}
3838
public get userAvatarUri(): vscode.Uri | undefined {
3939
if (this.prItem) {
4040
let key = this.userAvatar;
41-
let gravatar = vscode.Uri.parse(`${key}&s=${64}`);
41+
if (key) {
42+
let uri = vscode.Uri.parse(`${key}&s=${64}`);
4243

43-
// hack, to ensure queries are not wrongly encoded.
44-
const originalToStringFn = gravatar.toString;
45-
gravatar.toString = function (skipEncoding?: boolean | undefined) {
46-
return originalToStringFn.call(gravatar, true);
47-
};
44+
// hack, to ensure queries are not wrongly encoded.
45+
const originalToStringFn = uri.toString;
46+
uri.toString = function (skipEncoding?: boolean | undefined) {
47+
return originalToStringFn.call(uri, true);
48+
};
4849

49-
return gravatar;
50+
return uri;
51+
}
5052
}
5153

5254
return undefined;
@@ -80,7 +82,7 @@ export class PullRequestModel {
8082
public head: GitHubRef;
8183
public base: GitHubRef;
8284

83-
constructor(public readonly githubRepository: GitHubRepository, public readonly remote: Remote, public prItem: PullRequest) {
85+
constructor(public readonly githubRepository: GitHubRepository, public readonly remote: Remote, public prItem: PullRequest, private _repositoryReturnsAvatar: boolean) {
8486
this.update(prItem);
8587
}
8688

‎src/github/utils.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ export function convertToVSCodeComment(comment: Comment, command: vscode.Command
3535
body: new vscode.MarkdownString(comment.body),
3636
selectCommand: command,
3737
userName: comment.user!.login,
38-
userIconPath: vscode.Uri.parse(comment.user!.avatarUrl),
38+
userIconPath: comment.user && comment.user.avatarUrl ? vscode.Uri.parse(comment.user.avatarUrl) : undefined,
3939
label: !!comment.isDraft ? 'Pending' : undefined,
4040
commentReactions: comment.reactions ? comment.reactions.map(reaction => {
4141
return { label: reaction.label, hasReacted: reaction.viewerHasReacted, count: reaction.count, iconPath: reaction.icon };

‎src/test/github/pullRequestModel.test.ts

+3-3
Original file line numberDiff line numberDiff line change
@@ -171,19 +171,19 @@ const pr: Octokit.PullRequestsGetResponse | Octokit.PullRequestsGetAllResponseIt
171171

172172
describe('PullRequestModel', () => {
173173
it('should return `state` properly as `open`', () => {
174-
const open = new PullRequestModel(repo, remote, convertRESTPullRequestToRawPullRequest(pr));
174+
const open = new PullRequestModel(repo, remote, convertRESTPullRequestToRawPullRequest(pr), true);
175175

176176
assert.equal(open.state, PullRequestStateEnum.Open);
177177
});
178178

179179
it('should return `state` properly as `closed`', () => {
180-
const open = new PullRequestModel(repo, remote, convertRESTPullRequestToRawPullRequest({ ...pr, state: 'closed' }));
180+
const open = new PullRequestModel(repo, remote, convertRESTPullRequestToRawPullRequest({ ...pr, state: 'closed' }), true);
181181

182182
assert.equal(open.state, PullRequestStateEnum.Closed);
183183
});
184184

185185
it('should return `state` properly as `merged`', () => {
186-
const open = new PullRequestModel(repo, remote, convertRESTPullRequestToRawPullRequest({ ...pr, merged: true, state: 'closed' }));
186+
const open = new PullRequestModel(repo, remote, convertRESTPullRequestToRawPullRequest({ ...pr, merged: true, state: 'closed' }), true);
187187

188188
assert.equal(open.state, PullRequestStateEnum.Merged);
189189
});

‎src/view/treeNodes/pullRequestNode.ts

+2
Original file line numberDiff line numberDiff line change
@@ -407,6 +407,8 @@ export class PRNode extends TreeNode implements CommentHandler, vscode.Commentin
407407
collapsibleState: 1,
408408
contextValue: 'pullrequest' + (this._isLocal ? ':local' : '') + (currentBranchIsForThisPR ? ':active' : ':nonactive'),
409409
iconPath: this.pullRequestModel.userAvatarUri
410+
? this.pullRequestModel.userAvatarUri
411+
: { light: Resource.icons.light.Avatar, dark: Resource.icons.dark.Avatar }
410412
};
411413
}
412414

0 commit comments

Comments
 (0)