Skip to content

Commit 0d5cca4

Browse files
akshat-khosyaakshat-khosya
committed
Implement useMemo for 'Opened by' link generation to prioritize html_urladded db schema creation in start command and Add comprehensive unit tests for Enterprise and Cloud scenarios
1 parent d7f0959 commit 0d5cca4

File tree

2 files changed

+165
-49
lines changed

2 files changed

+165
-49
lines changed

‎webapp/src/components/link_tooltip/link_tooltip.jsx‎

Lines changed: 55 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,19 @@
11
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
22
// See LICENSE.txt for license information.
33

4-
import React, {useEffect, useState} from 'react';
4+
import React, { useEffect, useMemo, useState } from 'react';
55
import PropTypes from 'prop-types';
66
import './tooltip.css';
7-
import {GitMergeIcon, GitPullRequestIcon, IssueClosedIcon, IssueOpenedIcon} from '@primer/octicons-react';
7+
import { GitMergeIcon, GitPullRequestIcon, IssueClosedIcon, IssueOpenedIcon } from '@primer/octicons-react';
88
import ReactMarkdown from 'react-markdown';
99

1010
import Client from '@/client';
1111

12-
import {getLabelFontColor, hexToRGB} from '../../utils/styles';
12+
import { getLabelFontColor, hexToRGB } from '../../utils/styles';
1313

1414
const maxTicketDescriptionLength = 160;
1515

16-
export const LinkTooltip = ({href, connected, show, theme, enterpriseURL}) => {
16+
export const LinkTooltip = ({ href, connected, show, theme, enterpriseURL }) => {
1717
const [data, setData] = useState(null);
1818
useEffect(() => {
1919
const initData = async () => {
@@ -37,12 +37,12 @@ export const LinkTooltip = ({href, connected, show, theme, enterpriseURL}) => {
3737

3838
let res;
3939
switch (type) {
40-
case 'issues':
41-
res = await Client.getIssue(owner, repo, number);
42-
break;
43-
case 'pull':
44-
res = await Client.getPullRequest(owner, repo, number);
45-
break;
40+
case 'issues':
41+
res = await Client.getIssue(owner, repo, number);
42+
break;
43+
case 'pull':
44+
res = await Client.getPullRequest(owner, repo, number);
45+
break;
4646
}
4747
if (res) {
4848
res.owner = owner;
@@ -60,6 +60,25 @@ export const LinkTooltip = ({href, connected, show, theme, enterpriseURL}) => {
6060
initData();
6161
}, [connected, data, href, show, enterpriseURL]);
6262

63+
const openedByLink = useMemo(() => {
64+
if (!data?.user?.login) {
65+
return null;
66+
}
67+
// Immediately map the html_url value when present (which should work for both Enterprise and Cloud)
68+
if (data.user.html_url) {
69+
return data.user.html_url;
70+
}
71+
72+
// Fallback to a generic enterprise URL when appropriate, handling possible trailing slashes
73+
if (enterpriseURL) {
74+
const entURL = enterpriseURL.endsWith('/') ? enterpriseURL : enterpriseURL + '/';
75+
return `${entURL}${data.user.login}`;
76+
}
77+
78+
// Assume it's GitHub cloud and fallback to the original path (unlikely to ever run unless there are breaking changes in GitHub's API
79+
return `https://github.com/${data.user.login}`;
80+
}, [data, enterpriseURL]);
81+
6382
const getIconElement = () => {
6483
const iconProps = {
6584
size: 'small',
@@ -69,32 +88,32 @@ export const LinkTooltip = ({href, connected, show, theme, enterpriseURL}) => {
6988
let icon;
7089
let color;
7190
switch (data.type) {
72-
case 'pull':
73-
icon = <GitPullRequestIcon {...iconProps}/>;
74-
75-
color = '#28a745';
76-
if (data.state === 'closed') {
77-
if (data.merged) {
78-
color = '#6f42c1';
79-
icon = <GitMergeIcon {...iconProps}/>;
80-
} else {
81-
color = '#cb2431';
91+
case 'pull':
92+
icon = <GitPullRequestIcon {...iconProps} />;
93+
94+
color = '#28a745';
95+
if (data.state === 'closed') {
96+
if (data.merged) {
97+
color = '#6f42c1';
98+
icon = <GitMergeIcon {...iconProps} />;
99+
} else {
100+
color = '#cb2431';
101+
}
82102
}
83-
}
84103

85-
break;
86-
case 'issues':
87-
color = data.state === 'open' ? '#28a745' : '#cb2431';
104+
break;
105+
case 'issues':
106+
color = data.state === 'open' ? '#28a745' : '#cb2431';
88107

89-
if (data.state === 'open') {
90-
icon = <IssueOpenedIcon {...iconProps}/>;
91-
} else {
92-
icon = <IssueClosedIcon {...iconProps}/>;
93-
}
94-
break;
108+
if (data.state === 'open') {
109+
icon = <IssueOpenedIcon {...iconProps} />;
110+
} else {
111+
icon = <IssueClosedIcon {...iconProps} />;
112+
}
113+
break;
95114
}
96115
return (
97-
<span style={{color}}>
116+
<span style={{ color }}>
98117
{icon}
99118
</span>
100119
);
@@ -116,10 +135,10 @@ export const LinkTooltip = ({href, connected, show, theme, enterpriseURL}) => {
116135
<div className='github-tooltip'>
117136
<div
118137
className='github-tooltip box github-tooltip--large github-tooltip--bottom-left p-4'
119-
style={{backgroundColor: theme.centerChannelBg, border: `1px solid ${hexToRGB(theme.centerChannelColor, '0.16')}`}}
138+
style={{ backgroundColor: theme.centerChannelBg, border: `1px solid ${hexToRGB(theme.centerChannelColor, '0.16')}` }}
120139
>
121140
<div className='header mb-1'>
122-
<span style={{color: theme.centerChannelColor}}>
141+
<span style={{ color: theme.centerChannelColor }}>
123142
{data.repo}
124143
</span>
125144
{' on '}
@@ -137,7 +156,7 @@ export const LinkTooltip = ({href, connected, show, theme, enterpriseURL}) => {
137156
href={href}
138157
target='_blank'
139158
rel='noopener noreferrer'
140-
style={{color: theme.centerChannelColor}}
159+
style={{ color: theme.centerChannelColor }}
141160
>
142161
<h5 className='mr-1'>{data.title}</h5>
143162
<span>{'#' + data.number}</span>
@@ -146,7 +165,7 @@ export const LinkTooltip = ({href, connected, show, theme, enterpriseURL}) => {
146165
<p className='opened-by'>
147166
{'Opened by '}
148167
<a
149-
href={`${enterpriseURL && enterpriseURL !== '' ? enterpriseURL : 'https://github.com'}/${data.user.login}`}
168+
href={openedByLink}
150169
target='_blank'
151170
rel='noopener noreferrer'
152171
>
@@ -183,7 +202,7 @@ export const LinkTooltip = ({href, connected, show, theme, enterpriseURL}) => {
183202
key={idx}
184203
className='label mr-1'
185204
title={label.description}
186-
style={{backgroundColor: '#' + label.color, color: getLabelFontColor(label.color)}}
205+
style={{ backgroundColor: '#' + label.color, color: getLabelFontColor(label.color) }}
187206
>
188207
<span>{label.name}</span>
189208
</span>

‎webapp/src/components/link_tooltip/link_tooltip.test.jsx‎

Lines changed: 110 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,16 @@
11
import React from 'react';
2-
import {mount} from 'enzyme';
2+
import { mount } from 'enzyme';
33

44
import Client from '@/client';
55

6-
import {LinkTooltip} from './link_tooltip';
6+
import { LinkTooltip } from './link_tooltip';
77

88
jest.mock('@/client', () => ({
99
getIssue: jest.fn(),
1010
getPullRequest: jest.fn(),
1111
}));
1212

13-
jest.mock('react-markdown', () => () => <div/>);
13+
jest.mock('react-markdown', () => () => <div />);
1414

1515
describe('LinkTooltip', () => {
1616
const baseProps = {
@@ -37,11 +37,7 @@ describe('LinkTooltip', () => {
3737
});
3838

3939
test('should fetch issue for github.com link', () => {
40-
// We need to use mount or wait for useEffect?
41-
// shallow renders the component, useEffect is a hook.
42-
// Enzyme shallow supports hooks in newer versions, but let's check if we need to manually trigger logic.
43-
// The component uses useEffect to call initData.
44-
wrapper = mount(<LinkTooltip {...baseProps}/>);
40+
wrapper = mount(<LinkTooltip {...baseProps} />);
4541
expect(Client.getIssue).toHaveBeenCalledWith('mattermost', 'mattermost-plugin-github', '1');
4642
});
4743

@@ -50,7 +46,7 @@ describe('LinkTooltip', () => {
5046
...baseProps,
5147
href: 'https://github.com/mattermost/mattermost-plugin-github/pull/2',
5248
};
53-
wrapper = mount(<LinkTooltip {...props}/>);
49+
wrapper = mount(<LinkTooltip {...props} />);
5450
expect(Client.getPullRequest).toHaveBeenCalledWith('mattermost', 'mattermost-plugin-github', '2');
5551
});
5652

@@ -60,7 +56,7 @@ describe('LinkTooltip', () => {
6056
href: 'https://github.example.com/mattermost/mattermost-plugin-github/issues/3',
6157
enterpriseURL: 'https://github.example.com',
6258
};
63-
wrapper = mount(<LinkTooltip {...props}/>);
59+
wrapper = mount(<LinkTooltip {...props} />);
6460
expect(Client.getIssue).toHaveBeenCalledWith('mattermost', 'mattermost-plugin-github', '3');
6561
});
6662

@@ -70,7 +66,7 @@ describe('LinkTooltip', () => {
7066
href: 'https://github.example.com/mattermost/mattermost-plugin-github/pull/4',
7167
enterpriseURL: 'https://github.example.com',
7268
};
73-
wrapper = mount(<LinkTooltip {...props}/>);
69+
wrapper = mount(<LinkTooltip {...props} />);
7470
expect(Client.getPullRequest).toHaveBeenCalledWith('mattermost', 'mattermost-plugin-github', '4');
7571
});
7672

@@ -80,7 +76,7 @@ describe('LinkTooltip', () => {
8076
href: 'https://github.example.com/mattermost/mattermost-plugin-github/issues/5',
8177
enterpriseURL: 'https://github.example.com/',
8278
};
83-
wrapper = mount(<LinkTooltip {...props}/>);
79+
wrapper = mount(<LinkTooltip {...props} />);
8480
expect(Client.getIssue).toHaveBeenCalledWith('mattermost', 'mattermost-plugin-github', '5');
8581
});
8682

@@ -90,7 +86,108 @@ describe('LinkTooltip', () => {
9086
href: 'https://other-github.com/mattermost/mattermost-plugin-github/issues/6',
9187
enterpriseURL: 'https://github.example.com',
9288
};
93-
wrapper = mount(<LinkTooltip {...props}/>);
89+
wrapper = mount(<LinkTooltip {...props} />);
9490
expect(Client.getIssue).not.toHaveBeenCalled();
9591
});
92+
93+
test('should use html_url for opened by link if available', async () => {
94+
Client.getIssue.mockResolvedValueOnce({
95+
id: 1,
96+
title: 'Test Issue',
97+
body: 'Description',
98+
user: {
99+
login: 'testuser',
100+
html_url: 'https://github.com/testuser/profile',
101+
},
102+
state: 'open',
103+
labels: [],
104+
created_at: '2023-01-01T00:00:00Z',
105+
});
106+
107+
wrapper = mount(<LinkTooltip {...baseProps} />);
108+
109+
await new Promise((resolve) => setTimeout(resolve, 0));
110+
wrapper.update();
111+
112+
const link = wrapper.find('.opened-by a');
113+
expect(link.exists()).toBe(true);
114+
expect(link.prop('href')).toBe('https://github.com/testuser/profile');
115+
});
116+
117+
test('should fallback to enterprise URL for opened by link if html_url missing', async () => {
118+
Client.getIssue.mockResolvedValueOnce({
119+
id: 1,
120+
title: 'Test Enterprise Issue',
121+
body: 'Description',
122+
user: {
123+
login: 'entuser',
124+
},
125+
state: 'open',
126+
labels: [],
127+
created_at: '2023-01-01T00:00:00Z',
128+
});
129+
130+
const props = {
131+
...baseProps,
132+
href: 'https://github.example.com/mattermost/mattermost-plugin-github/issues/3',
133+
enterpriseURL: 'https://github.example.com',
134+
};
135+
wrapper = mount(<LinkTooltip {...props} />);
136+
137+
await new Promise((resolve) => setTimeout(resolve, 0));
138+
wrapper.update();
139+
140+
const link = wrapper.find('.opened-by a');
141+
expect(link.exists()).toBe(true);
142+
expect(link.prop('href')).toBe('https://github.example.com/entuser');
143+
});
144+
145+
test('should handle enterprise URL with trailing slash for opened by link fallback', async () => {
146+
Client.getIssue.mockResolvedValueOnce({
147+
id: 1,
148+
title: 'Test Enterprise Issue',
149+
body: 'Description',
150+
user: {
151+
login: 'entuser',
152+
},
153+
state: 'open',
154+
labels: [],
155+
created_at: '2023-01-01T00:00:00Z',
156+
});
157+
158+
const props = {
159+
...baseProps,
160+
href: 'https://github.example.com/mattermost/mattermost-plugin-github/issues/3',
161+
enterpriseURL: 'https://github.example.com/',
162+
};
163+
wrapper = mount(<LinkTooltip {...props} />);
164+
165+
await new Promise((resolve) => setTimeout(resolve, 0));
166+
wrapper.update();
167+
168+
const link = wrapper.find('.opened-by a');
169+
expect(link.prop('href')).toBe('https://github.example.com/entuser');
170+
});
171+
172+
test('should default to github.com for opened by link if no enterpriseURL and no html_url', async () => {
173+
Client.getIssue.mockResolvedValueOnce({
174+
id: 1,
175+
title: 'Test Issue',
176+
body: 'Description',
177+
user: {
178+
login: 'clouduser',
179+
},
180+
state: 'open',
181+
labels: [],
182+
created_at: '2023-01-01T00:00:00Z',
183+
});
184+
185+
wrapper = mount(<LinkTooltip {...baseProps} />);
186+
187+
await new Promise((resolve) => setTimeout(resolve, 0));
188+
wrapper.update();
189+
190+
const link = wrapper.find('.opened-by a');
191+
expect(link.prop('href')).toBe('https://github.com/clouduser');
192+
});
96193
});

0 commit comments

Comments
 (0)