Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Strange avatar is displayed for GitHub Enterprise #2771

Closed
think120 opened this issue Jun 11, 2021 · 16 comments
Closed

Strange avatar is displayed for GitHub Enterprise #2771

think120 opened this issue Jun 11, 2021 · 16 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug verified Verification succeeded
Milestone

Comments

@think120
Copy link

think120 commented Jun 11, 2021

  • Extension version: v0.27.0
  • VSCode Version: Code 1.57.0
  • OS: Win10 1909
  • GitHub Enterprise version: GitHub Enterprise Server 3.0.4

The avatar of the Pull Request in GitHub Enterprise is strange in "GitHub" and "GitHub Pull Request" tab, as shown in the picture below:
image

Looks like it is because in the following postion, this.pullRequestModel.userAvatarUri is used instead of new vscode.ThemeIcon('github')

iconPath: this.pullRequestModel.userAvatarUri
? this.pullRequestModel.userAvatarUri
: new vscode.ThemeIcon('github'),
};

@alexr00
Copy link
Member

alexr00 commented Jun 11, 2021

@think120 I made some changes that may fix this issue this week. Would you be willing to try the nightly build of this extension (id is github.vscode-pull-request-github-insiders)? You'll need to disable the stable extension to use the nightly build.

@alexr00 alexr00 added the info-needed Issue requires more information from poster label Jun 11, 2021
@think120
Copy link
Author

@alexr00 , I've tested the nightly build,

  1. "GitHub" tab now can display correct avatar
    image

  2. For "GitHub Pull Request" tab, still shows the strange avatar
    image

  3. In the Pull Request Description page:

  • For the Pull Request created by myself, sill shows the strange avatar
    image

  • For For the PR created by other people, correct avatar is displayed.
    image

@alexr00 alexr00 added bug Issue identified by VS Code Team member as probable bug and removed info-needed Issue requires more information from poster labels Jun 11, 2021
@alexr00 alexr00 added this to the June 2021 milestone Jun 11, 2021
@alexr00 alexr00 self-assigned this Jun 11, 2021
@alexr00
Copy link
Member

alexr00 commented Jun 11, 2021

Thanks for testing this @think120. I've found the source of the bug for the other avatar locations. There will be a new nightly build on Monday. If you could test again with that upcoming build and comment back here about whether the bug is fixed that would be very helpful!

@think120
Copy link
Author

think120 commented Jun 14, 2021

@alexr00, I've upgraded the nightly build of this extension to v2021.6.19263, but the issue 2 and 3 still exist. Does that build include your fix? Many thanks!

I've done some additional test in the "Pull Request Description" page, as shown in the picture below:
1 and 2 are commits, which show the weird avatar.
3 is "merge the pull request" which shows the correct avatar
4 is "delete the branch", which can't show any avatar

image

@alexr00
Copy link
Member

alexr00 commented Jun 15, 2021

Yes, that build has (what I thought would be) the fix. Thanks for testing this out.

@alexr00 alexr00 reopened this Jun 15, 2021
@think120
Copy link
Author

think120 commented Jun 15, 2021

@alexr00 , many thanks for your quick fix! Now, only the "delete branch" operation can't display the avatar of the user, but it is good enough for me!
image

Also noticed the avatar for the user left a comment also can't be displayed.

image

@alexr00
Copy link
Member

alexr00 commented Jun 15, 2021

@think120 It's not clear to me what could be causing that last one, since based on your other images I would expect either the weird avatar or the correct avatar. I'm going to leave this as is for now since I don't have an enterprise setup to test with. Thank you for all your testing!

@alexr00 alexr00 added the verified Verification succeeded label Jun 15, 2021
@think120
Copy link
Author

@alexr00 , I just found this issue #416, looks like #416 has the same problem as I have.

@kabel
Copy link
Contributor

kabel commented Jul 8, 2021

The weird avatars were a purposeful fallback choice for the enterprise integration. The icon was generated from the Gravatar service since unauthenticated requests to the GHE avatar service is likely disabled for most. If your GHE email didn't exist in the Gravatar service, the default was set to the "retro" generated default.

Perhaps I should have implemented this feature behind a feature flag, but now we've regressed back to having no avatars for enterprise, which make me sad.

@kabel
Copy link
Contributor

kabel commented Jul 8, 2021

I've also seen the 404 image shown in example 4. These usually come from DOM updates to the webview instead of a full refresh, so some action is inserting a DOM image node that likely assumes a GitHub.com repo instead of GHE.

@think120
Copy link
Author

think120 commented Jul 8, 2021

And that 404 image is only for "delete branch" operation.

@alexr00
Copy link
Member

alexr00 commented Jul 9, 2021

@kabel are you saying that even for enterprise users who have an avatar we now show the wrong thing? If that's the case, I'll just revert the changes I made, call it a day, and not touch enterprise again.

@think120
Copy link
Author

think120 commented Jul 9, 2021

@kabel , @alexr00, I think may be better to make the feature of using Gravatar configurable. Like in my company, no one use Gravatar and I don't think they will use company email to register Gravatar. And I think the default octocat icon is nicer than the green "retro" icon 😄

@kabel
Copy link
Contributor

kabel commented Jul 9, 2021

@alexr00

are you saying that even for enterprise users who have an avatar we now show the wrong thing?

Yes.

0.27.1:
image

0.28.0:
image


@think120

And that 404 image is only for "delete branch" operation.

Not only, but certainly one of the instances of the webview templates making the wrong assumption about the avatar URL to use. Another example is when I add a reviewer or the rendered PR comments on the Description view:
image
image

I think may be better to make the feature of using Gravatar configurable.

I agree, so I'll see if I can implement that feature (for the privacy conscious and anti-gravatar folks). I had originally planned to use the SVG icon as the Gravatar fallback instead of the "retro", but couldn't find a publicly hosted version that their proxies could access.

@think120
Copy link
Author

think120 commented Jul 9, 2021

@kabel ,

export const Avatar = ({ for: author }: { for: Partial<PullRequest['author']> }) => (
<a className="avatar-link" href={author.url}>
{author.avatarUrl ? (
<img className="avatar" src={author.avatarUrl} alt="" />
) : (
<Icon className="avatar-icon" src={require('../../resources/icons/dark/github.svg')} />
)}
</a>

This part?

@alexr00
Copy link
Member

alexr00 commented Jul 9, 2021

@kabel sorry about the enterprise avatar regression, and thank you for your interest in making Gravatar use configurable. I would love to accept a PR for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue identified by VS Code Team member as probable bug verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

3 participants