GH-367 Render tooltip for Enterprise installations #949
Conversation
This adds support for rendering tooltips for links from Enterprise GitHub installations by checking the configured Enterprise Base URL. It also adds unit tests to verify the URL parsing logic for both SaaS and Enterprise links.
|
Hello @akshat-khosya, Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here. |
avasconcelos114
left a comment
avasconcelos114
left a comment
There was a problem hiding this comment.
Thank you for the contribution! 🎊
I left a couple of comments with regards to one of the links in the tooltip as well as testing related to it. Let me know if you have any questions about them!
| {'Opened by '} | ||
| <a | ||
| href={`https://github.com/${data.user.login}`} | ||
| href={`${enterpriseURL && enterpriseURL !== '' ? enterpriseURL : 'https://github.com'}/${data.user.login}`} |
There was a problem hiding this comment.
It seems that for enterprise installations we will end up with the Opened By link sending users directly to the enterpriseURL URL instead of what would be the user's profile
It feels like we could have a useMemo-wrapped function that attempts to first map to the user's html_url property, which both the Get issue and Get Pull Request endpoints should return. With a fallback values to each base URL for good measure
Something that would look like this
const openedByLink = useMemo(() => {
if (!data?.user?.login) {
return null;
}
// Immediately map the html_url value when present (which should work for both Enterprise and Cloud)
if (data.user.html_url) {
return data.user.html_url;
}
// Fallback to a generic enterprise URL when appropriate, handling possible trailing slashes
if (enterpriseURL) {
const entURL = enterpriseURL.endsWith('/') ? enterpriseURL : enterpriseURL + '/';
return `${entURL}${data.user.login}`;
}
// Assume it's GitHub cloud and fallback to the original path (unlikely to ever run unless there are breaking changes in GitHub's API
return `https://github.com/${data.user.login}`;
}, [data, enterpriseURL]);This way we can also safely render the link based on openedByLink's value in line 145 instead of data?.user?.login
|
|
||
| jest.mock('react-markdown', () => () => <div/>); | ||
|
|
||
| describe('LinkTooltip', () => { |
There was a problem hiding this comment.
Great job with the tests! They are a much appreciated addition
Could we also have a couple of tests that confirm that the "Opened by" link's href attribute changes accordingly based on the mapped props and component state?
| // We need to use mount or wait for useEffect? | ||
| // shallow renders the component, useEffect is a hook. | ||
| // Enzyme shallow supports hooks in newer versions, but let's check if we need to manually trigger logic. | ||
| // The component uses useEffect to call initData. |
There was a problem hiding this comment.
The comments here aren't particularly necessary, they can be safely removed
avasconcelos114
added
2: Dev Review
3 participants
Summary
This adds support for rendering tooltips for links from Enterprise GitHub installations by checking the configured Enterprise Base URL. It also adds unit tests to verify the URL parsing logic for both SaaS and Enterprise links.
Ticket Link
#367