Skip to content

Commit 7ecff9a

Browse files
ostoliar-atlassianostoliar-atlassian
authored
AXON-323-can-not-remove-reviewer-from-PR (#676)
* AXON-323: added the functionality to remove a reviewer * AXON-323: add tests * AXON-323: update changelog * AXON-323: update changelog
1 parent 65daf13 commit 7ecff9a

File tree

4 files changed

+301
-29
lines changed

4 files changed

+301
-29
lines changed

‎CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,10 @@
44

55
## What's new in 3.8.9
66

7+
### Features
8+
9+
- Added the possibility to remove a reviewer from a PR
10+
711
### Bug Fixes
812

913
- Removed setting options that had no practical or business logic use.

‎src/bitbucket/bitbucket-cloud/pullRequests.test.ts

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1112,6 +1112,107 @@ describe('CloudPullRequestApi', () => {
11121112
});
11131113
expect(result.data.id).toBe('123');
11141114
});
1115+
1116+
it('should remove all reviewers when empty array is provided', async () => {
1117+
const mockApiResponse = {
1118+
data: {
1119+
...mockPullRequest.data,
1120+
id: '123',
1121+
author: { account_id: 'author-id', display_name: 'Author' },
1122+
participants: [],
1123+
source: {
1124+
repository: { full_name: 'test-owner/test-repo' },
1125+
branch: { name: 'feature' },
1126+
commit: { hash: 'abc123' },
1127+
},
1128+
destination: {
1129+
repository: { full_name: 'test-owner/test-repo' },
1130+
branch: { name: 'main' },
1131+
commit: { hash: 'def456' },
1132+
},
1133+
links: { html: { href: 'https://pr.url' } },
1134+
},
1135+
};
1136+
mockHttpClient.put.mockResolvedValue(mockApiResponse);
1137+
1138+
const result = await api.update(mockPullRequest, 'Test PR', 'Test summary', []);
1139+
1140+
expect(mockHttpClient.put).toHaveBeenCalledWith('/repositories/test-owner/test-repo/pullrequests/123', {
1141+
title: 'Test PR',
1142+
summary: { raw: 'Test summary' },
1143+
reviewers: [],
1144+
});
1145+
expect(result.data.id).toBe('123');
1146+
});
1147+
1148+
it('should remove specific reviewers by providing filtered reviewer list', async () => {
1149+
const mockApiResponse = {
1150+
data: {
1151+
...mockPullRequest.data,
1152+
id: '123',
1153+
author: { account_id: 'author-id', display_name: 'Author' },
1154+
participants: [],
1155+
source: {
1156+
repository: { full_name: 'test-owner/test-repo' },
1157+
branch: { name: 'feature' },
1158+
commit: { hash: 'abc123' },
1159+
},
1160+
destination: {
1161+
repository: { full_name: 'test-owner/test-repo' },
1162+
branch: { name: 'main' },
1163+
commit: { hash: 'def456' },
1164+
},
1165+
links: { html: { href: 'https://pr.url' } },
1166+
},
1167+
};
1168+
mockHttpClient.put.mockResolvedValue(mockApiResponse);
1169+
1170+
const remainingReviewers = ['reviewer1', 'reviewer3'];
1171+
const result = await api.update(mockPullRequest, 'Test PR', 'Test summary', remainingReviewers);
1172+
1173+
expect(mockHttpClient.put).toHaveBeenCalledWith('/repositories/test-owner/test-repo/pullrequests/123', {
1174+
title: 'Test PR',
1175+
summary: { raw: 'Test summary' },
1176+
reviewers: [
1177+
{ type: 'user', account_id: 'reviewer1' },
1178+
{ type: 'user', account_id: 'reviewer3' },
1179+
],
1180+
});
1181+
expect(result.data.id).toBe('123');
1182+
});
1183+
1184+
it('should handle reviewer removal when only one reviewer remains', async () => {
1185+
const mockApiResponse = {
1186+
data: {
1187+
...mockPullRequest.data,
1188+
id: '123',
1189+
author: { account_id: 'author-id', display_name: 'Author' },
1190+
participants: [],
1191+
source: {
1192+
repository: { full_name: 'test-owner/test-repo' },
1193+
branch: { name: 'feature' },
1194+
commit: { hash: 'abc123' },
1195+
},
1196+
destination: {
1197+
repository: { full_name: 'test-owner/test-repo' },
1198+
branch: { name: 'main' },
1199+
commit: { hash: 'def456' },
1200+
},
1201+
links: { html: { href: 'https://pr.url' } },
1202+
},
1203+
};
1204+
mockHttpClient.put.mockResolvedValue(mockApiResponse);
1205+
1206+
const remainingReviewers = ['reviewer1'];
1207+
const result = await api.update(mockPullRequest, 'Test PR', 'Test summary', remainingReviewers);
1208+
1209+
expect(mockHttpClient.put).toHaveBeenCalledWith('/repositories/test-owner/test-repo/pullrequests/123', {
1210+
title: 'Test PR',
1211+
summary: { raw: 'Test summary' },
1212+
reviewers: [{ type: 'user', account_id: 'reviewer1' }],
1213+
});
1214+
expect(result.data.id).toBe('123');
1215+
});
11151216
});
11161217

11171218
describe('getReviewers', () => {

‎src/bitbucket/bitbucket-server/pullRequests.test.ts

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1734,6 +1734,98 @@ describe('ServerPullRequestApi', () => {
17341734
);
17351735
expect(result.data.title).toBe('Updated Title');
17361736
});
1737+
1738+
it('should remove all reviewers when empty array is provided', async () => {
1739+
const mockUpdateResponse = {
1740+
data: {
1741+
...getPullRequestData,
1742+
title: 'Test PR',
1743+
description: 'Test description',
1744+
version: 2,
1745+
},
1746+
};
1747+
1748+
mockPut.mockResolvedValue(mockUpdateResponse);
1749+
1750+
const result = await api.update(mockPullRequest, 'Test PR', 'Test description', []);
1751+
1752+
expect(mockPut).toHaveBeenCalledWith(
1753+
'/rest/api/1.0/projects/testproject/repos/testrepo/pull-requests/123',
1754+
{
1755+
version: 1,
1756+
title: 'Test PR',
1757+
description: 'Test description',
1758+
reviewers: [],
1759+
},
1760+
{
1761+
markup: true,
1762+
avatarSize: 64,
1763+
},
1764+
);
1765+
expect(result.data.title).toBe('Test PR');
1766+
});
1767+
1768+
it('should remove specific reviewers by providing filtered reviewer list', async () => {
1769+
const mockUpdateResponse = {
1770+
data: {
1771+
...getPullRequestData,
1772+
title: 'Test PR',
1773+
description: 'Test description',
1774+
version: 2,
1775+
},
1776+
};
1777+
1778+
mockPut.mockResolvedValue(mockUpdateResponse);
1779+
1780+
const remainingReviewers = ['reviewer1', 'reviewer3'];
1781+
const result = await api.update(mockPullRequest, 'Test PR', 'Test description', remainingReviewers);
1782+
1783+
expect(mockPut).toHaveBeenCalledWith(
1784+
'/rest/api/1.0/projects/testproject/repos/testrepo/pull-requests/123',
1785+
{
1786+
version: 1,
1787+
title: 'Test PR',
1788+
description: 'Test description',
1789+
reviewers: [{ user: { name: 'reviewer1' } }, { user: { name: 'reviewer3' } }],
1790+
},
1791+
{
1792+
markup: true,
1793+
avatarSize: 64,
1794+
},
1795+
);
1796+
expect(result.data.title).toBe('Test PR');
1797+
});
1798+
1799+
it('should handle reviewer removal when only one reviewer remains', async () => {
1800+
const mockUpdateResponse = {
1801+
data: {
1802+
...getPullRequestData,
1803+
title: 'Test PR',
1804+
description: 'Test description',
1805+
version: 2,
1806+
},
1807+
};
1808+
1809+
mockPut.mockResolvedValue(mockUpdateResponse);
1810+
1811+
const remainingReviewers = ['reviewer1'];
1812+
const result = await api.update(mockPullRequest, 'Test PR', 'Test description', remainingReviewers);
1813+
1814+
expect(mockPut).toHaveBeenCalledWith(
1815+
'/rest/api/1.0/projects/testproject/repos/testrepo/pull-requests/123',
1816+
{
1817+
version: 1,
1818+
title: 'Test PR',
1819+
description: 'Test description',
1820+
reviewers: [{ user: { name: 'reviewer1' } }],
1821+
},
1822+
{
1823+
markup: true,
1824+
avatarSize: 64,
1825+
},
1826+
);
1827+
expect(result.data.title).toBe('Test PR');
1828+
});
17371829
});
17381830

17391831
describe('getComments', () => {

‎src/react/atlascode/pullrequest/Reviewers.tsx

Lines changed: 104 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,97 @@
1-
import { Avatar, Badge, Box, CircularProgress, Grid, Tooltip, Typography } from '@material-ui/core';
1+
import { Avatar, Badge, Box, CircularProgress, Grid, IconButton, Tooltip, Typography } from '@material-ui/core';
22
import CheckCircleIcon from '@material-ui/icons/CheckCircle';
3+
import CloseIcon from '@material-ui/icons/Close';
34
import { AvatarGroup } from '@material-ui/lab';
45
import React, { useCallback, useEffect, useState } from 'react';
56

67
import { BitbucketSite, Reviewer, User } from '../../../bitbucket/model';
78
import StoppedIcon from '../icons/StoppedIcon';
89
import { AddReviewers } from './AddReviewers';
10+
11+
type RemovableReviewerAvatarProps = {
12+
participant: Reviewer;
13+
onRemove?: (participant: Reviewer) => void;
14+
showRemoveButton?: boolean;
15+
};
16+
17+
const RemovableReviewerAvatar: React.FunctionComponent<RemovableReviewerAvatarProps> = ({
18+
participant,
19+
onRemove,
20+
showRemoveButton = false,
21+
}) => {
22+
const [isHovered, setIsHovered] = useState(false);
23+
24+
return (
25+
<Box position="relative" onMouseEnter={() => setIsHovered(true)} onMouseLeave={() => setIsHovered(false)}>
26+
<Badge
27+
style={{ borderWidth: '0px' }}
28+
overlap="circle"
29+
anchorOrigin={{
30+
vertical: 'top',
31+
horizontal: 'right',
32+
}}
33+
invisible={participant.status === 'UNAPPROVED'}
34+
badgeContent={
35+
participant.status === 'CHANGES_REQUESTED' ? (
36+
<Tooltip title="Requested changes">
37+
<Box bgcolor={'white'} borderRadius={'100%'}>
38+
<StoppedIcon fontSize={'small'} htmlColor={'#FFAB00'} />
39+
</Box>
40+
</Tooltip>
41+
) : (
42+
<Tooltip title="Approved">
43+
<Box bgcolor={'white'} borderRadius={'100%'}>
44+
<CheckCircleIcon fontSize={'small'} htmlColor={'#07b82b'} />
45+
</Box>
46+
</Tooltip>
47+
)
48+
}
49+
>
50+
<Tooltip title={participant.displayName}>
51+
<Avatar alt={participant.displayName} src={participant.avatarUrl} />
52+
</Tooltip>
53+
</Badge>
54+
{showRemoveButton && isHovered && onRemove && (
55+
<IconButton
56+
size="small"
57+
onClick={(e) => {
58+
e.stopPropagation();
59+
onRemove(participant);
60+
}}
61+
style={{
62+
position: 'absolute',
63+
top: -8,
64+
right: -8,
65+
backgroundColor: 'var(--vscode-button-background)',
66+
color: 'var(--vscode-button-foreground)',
67+
padding: 2,
68+
width: 20,
69+
height: 20,
70+
zIndex: 1,
71+
}}
72+
title={`Remove ${participant.displayName}`}
73+
>
74+
<CloseIcon fontSize="small" />
75+
</IconButton>
76+
)}
77+
</Box>
78+
);
79+
};
80+
981
type ReviewersProps = {
1082
site: BitbucketSite;
1183
onUpdateReviewers: (reviewers: User[]) => Promise<void>;
1284
participants: Reviewer[];
1385
isLoading: boolean;
86+
allowRemoveReviewers?: boolean;
1487
};
88+
1589
export const Reviewers: React.FunctionComponent<ReviewersProps> = ({
1690
site,
1791
onUpdateReviewers,
1892
participants,
1993
isLoading,
94+
allowRemoveReviewers = true,
2095
}) => {
2196
const [activeParticipants, setActiveParticipants] = useState<Reviewer[]>([]);
2297
const [isFetchingReviewer, setIsFetchingReviewers] = useState<boolean>(false);
@@ -34,6 +109,29 @@ export const Reviewers: React.FunctionComponent<ReviewersProps> = ({
34109
[onUpdateReviewers],
35110
);
36111

112+
const handleRemoveReviewer = useCallback(
113+
async (reviewerToRemove: Reviewer) => {
114+
if (!allowRemoveReviewers) {
115+
return;
116+
}
117+
118+
const updatedReviewers = participants
119+
.filter((p) => p.accountId !== reviewerToRemove.accountId)
120+
.map((p) => ({
121+
accountId: p.accountId,
122+
displayName: p.displayName,
123+
avatarUrl: p.avatarUrl,
124+
url: p.url,
125+
mention: p.mention,
126+
userName: p.userName,
127+
emailAddress: p.emailAddress,
128+
}));
129+
130+
await handleUpdateReviewers(updatedReviewers);
131+
},
132+
[participants, allowRemoveReviewers, handleUpdateReviewers],
133+
);
134+
37135
useEffect(() => {
38136
setActiveParticipants(
39137
participants // always show reviewers & show non-reviewers if they have approved or marked needs work
@@ -55,35 +153,12 @@ export const Reviewers: React.FunctionComponent<ReviewersProps> = ({
55153
) : (
56154
<AvatarGroup max={5}>
57155
{activeParticipants.map((participant) => (
58-
<Badge
59-
style={{ borderWidth: '0px' }}
60-
overlap="circle"
61-
anchorOrigin={{
62-
vertical: 'top',
63-
horizontal: 'right',
64-
}}
65-
invisible={participant.status === 'UNAPPROVED'}
156+
<RemovableReviewerAvatar
66157
key={participant.accountId}
67-
badgeContent={
68-
participant.status === 'CHANGES_REQUESTED' ? (
69-
<Tooltip title="Requested changes">
70-
<Box bgcolor={'white'} borderRadius={'100%'}>
71-
<StoppedIcon fontSize={'small'} htmlColor={'#FFAB00'} />
72-
</Box>
73-
</Tooltip>
74-
) : (
75-
<Tooltip title="Approved">
76-
<Box bgcolor={'white'} borderRadius={'100%'}>
77-
<CheckCircleIcon fontSize={'small'} htmlColor={'#07b82b'} />
78-
</Box>
79-
</Tooltip>
80-
)
81-
}
82-
>
83-
<Tooltip title={participant.displayName}>
84-
<Avatar alt={participant.displayName} src={participant.avatarUrl} />
85-
</Tooltip>
86-
</Badge>
158+
participant={participant}
159+
onRemove={allowRemoveReviewers ? handleRemoveReviewer : undefined}
160+
showRemoveButton={allowRemoveReviewers && participant.role === 'REVIEWER'}
161+
/>
87162
))}
88163
</AvatarGroup>
89164
)}

0 commit comments

Comments
 (0)