-
Notifications
You must be signed in to change notification settings - Fork 608
Support GitHub Enterprise avatars via Enterprise REST service and optionally Gravatar #4497
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
Conversation
6cee68b
to
664eb2f
Compare
664eb2f
to
d876027
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review/questions. Based on the availability of the Enterprise REST API (as now used in GitHub Desktop), I added a bit of scope creep to this request. In c0145681df76ef688929ab21181245974db7725e the fallback logic will first check the REST API for the avatar image binary. If the API returns the image binary, it is converted to a data URI to be rendered by the webviews and treeviews. I updated the CSP meta of the webviews to allow Other comments that don't require additional feedback should now be addressed. |
I'm sorry @kabel but this PR has become too big for me to continue reviewing now and it mostly seems to be because of fetching the user email when parsing user results from the GitHub REST API. I started reviewing and left some comments, but scope of this change has just gotten to big for me to review at this time. I will try to plan time to review in the next couple months, but ideally the PR would be much smaller. Thank you again for this change. If I hear from users that gravatars are important I will definitely reference your changes to see how to enable them. |
I've opened an issue to try to gauge how much desire there is for this feature: #4517 |
154a59f
to
c65376f
Compare
c65376f
to
653da79
Compare
Sounds fair @alexr00. I've got the latest push down to a more manageable size (mostly added lines for the new Enterprise REST integration), but I'll wait for more time to become available. Having a better Enterprise experience continues to be important for me, but I can understand how avatars aren't going to be a huge win feature-wise for folks. |
d950471
to
cec122d
Compare
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](desktop/desktop#3513)), but this behavior was disabled in 13ab0a7 and 6769dd4 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 `<Avatar>` 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.
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.
Also implements HTTP caching client by saving the avatar REST headers and responses to the globalStorage.
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), but this behavior was disabled in 13ab0a7 and 6769dd4 because of end-user preference for making this feature configurable.
Recently an enterprise-only REST API was discovered that allows us to proxy the avatar image. This allows us to fetch the binary and encode it as a data URI to inject into the various views that otherwise render the avatar with standard http requests. If this API fails for some reason, Gravatar remains an optional fallback choice.
A new setting
githubPullRequests.defaultGravatarsStyle
has been added of typeenum
with values for each of the Gravatar supported fallback styles. Setting this value tonone
(the default) disables Gravatar 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
<Avatar>
is rendered, the GraphQL queries (and matching typescript interfaces) have been updated to include the necessaryemail
property for integration with Gravatar.REST provided actors are missing email. If missing email and using GitHub Enterprise and it's needed by Gravatar, make another REST API call to the user endpoint to get email. This allows a fallback avatar to be generated.
This fixes GitHub Enterprise avatars not being rendered in the tree views too.
The utility functions have been adjusted to support doing the fallback image URL generation directly after the raw results have been loaded from the normal REST and GraphQL APIs. This should be a cleaner implementation.