Skip to content

Commit 2b55f58

Browse files
chpy04chpy04
committed
#3802 if cr has requested reviewers, only they can approve
1 parent be417a9 commit 2b55f58

6 files changed

Lines changed: 250 additions & 10 deletions

File tree

‎src/backend/src/services/change-requests.services.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -280,6 +280,14 @@ export default class ChangeRequestsService {
280280
if (reviewer.userId === foundCR.submitterId)
281281
throw new AccessDeniedException("You can't review your own change request!");
282282

283+
// verify that if there are requested reviewers, the reviewer is one of them
284+
if (foundCR.requestedReviewers.length > 0) {
285+
const isRequestedReviewer = foundCR.requestedReviewers.some((user) => user.userId === reviewer.userId);
286+
if (!isRequestedReviewer) {
287+
throw new AccessDeniedException('Only requested reviewers can review this change request!');
288+
}
289+
}
290+
283291
// ScopeChange Request That Has Been Accepted Being Reviewed
284292
if (foundCR.scopeChangeRequest && accepted) {
285293
await this.reviewScopeChangeRequest(foundCR, reviewer, psId, organization);

‎src/backend/tests/unit/change-requests.test.ts‎

Lines changed: 196 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,16 @@
11
import { CR_Type, Organization, Scope_CR_Why_Type, User, WBS_Element_Status } from '@prisma/client';
22
import { createTestOrganization, createTestUser, resetUsers } from '../test-utils';
33
import ChangeRequestsService from '../../src/services/change-requests.services';
4-
import { supermanAdmin } from '../test-data/users.test-data';
4+
import {
5+
supermanAdmin,
6+
aquamanLeadership,
7+
greenlanternHead,
8+
flashAdmin,
9+
robinMember
10+
} from '../test-data/users.test-data';
511
import { ProjectProposedChangesCreateArgs, WorkPackageProposedChangesCreateArgs } from 'shared';
612
import prisma from '../../src/prisma/prisma';
13+
import { AccessDeniedException } from '../../src/utils/errors.utils';
714

815
describe('Change Request Tests', () => {
916
let orgId: string;
@@ -284,4 +291,192 @@ describe('Change Request Tests', () => {
284291
expect(wbsElement?.workPackageNumber).toEqual(14);
285292
});
286293
});
294+
295+
describe('Review Change Request with Requested Reviewers', () => {
296+
let submitterUser: User;
297+
let leadershipUser1: User;
298+
let leadershipUser2: User;
299+
let nonRequestedLeadership: User;
300+
let memberUser: User;
301+
let changeRequestId: string;
302+
303+
beforeEach(async () => {
304+
submitterUser = await createTestUser(supermanAdmin, orgId);
305+
leadershipUser1 = await createTestUser(aquamanLeadership, orgId);
306+
leadershipUser2 = await createTestUser(greenlanternHead, orgId);
307+
nonRequestedLeadership = await createTestUser(flashAdmin, orgId);
308+
memberUser = await createTestUser(robinMember, orgId);
309+
310+
const projPropChanges: ProjectProposedChangesCreateArgs = {
311+
name: 'Test Project',
312+
descriptionBullets: [],
313+
links: [],
314+
budget: 10,
315+
summary: 'Test Summary',
316+
teamIds: [],
317+
workPackageProposedChanges: []
318+
};
319+
320+
const cr = await ChangeRequestsService.createStandardChangeRequest(
321+
submitterUser,
322+
12,
323+
13,
324+
14,
325+
CR_Type.DEFINITION_CHANGE,
326+
'What is being changed',
327+
[{ type: Scope_CR_Why_Type.COMPETITION, explain: 'Why it is being changed' }],
328+
[],
329+
organization,
330+
projPropChanges,
331+
null
332+
);
333+
334+
changeRequestId = cr.crId;
335+
});
336+
337+
it('allows any leadership to review when no reviewers are requested', async () => {
338+
const reviewResult = await ChangeRequestsService.reviewChangeRequest(
339+
nonRequestedLeadership,
340+
changeRequestId,
341+
'Looks good',
342+
true,
343+
organization,
344+
null
345+
);
346+
347+
expect(reviewResult).toBe(changeRequestId);
348+
349+
const updatedCR = await prisma.change_Request.findUnique({
350+
where: { crId: changeRequestId }
351+
});
352+
353+
expect(updatedCR?.reviewerId).toBe(nonRequestedLeadership.userId);
354+
expect(updatedCR?.accepted).toBe(true);
355+
});
356+
357+
it('allows requested reviewer to review when reviewers are requested', async () => {
358+
await ChangeRequestsService.requestCRReview(
359+
submitterUser,
360+
[leadershipUser1.userId, leadershipUser2.userId],
361+
changeRequestId,
362+
organization
363+
);
364+
365+
const reviewResult = await ChangeRequestsService.reviewChangeRequest(
366+
leadershipUser1,
367+
changeRequestId,
368+
'Approved',
369+
true,
370+
organization,
371+
null
372+
);
373+
374+
expect(reviewResult).toBe(changeRequestId);
375+
376+
const updatedCR = await prisma.change_Request.findUnique({
377+
where: { crId: changeRequestId }
378+
});
379+
380+
expect(updatedCR?.reviewerId).toBe(leadershipUser1.userId);
381+
expect(updatedCR?.accepted).toBe(true);
382+
});
383+
384+
it('rejects non-requested leadership when reviewers are requested', async () => {
385+
await ChangeRequestsService.requestCRReview(
386+
submitterUser,
387+
[leadershipUser1.userId, leadershipUser2.userId],
388+
changeRequestId,
389+
organization
390+
);
391+
392+
await expect(
393+
ChangeRequestsService.reviewChangeRequest(
394+
nonRequestedLeadership,
395+
changeRequestId,
396+
'I want to review this',
397+
true,
398+
organization,
399+
null
400+
)
401+
).rejects.toThrow(AccessDeniedException);
402+
403+
await expect(
404+
ChangeRequestsService.reviewChangeRequest(
405+
nonRequestedLeadership,
406+
changeRequestId,
407+
'I want to review this',
408+
true,
409+
organization,
410+
null
411+
)
412+
).rejects.toThrow('Only requested reviewers can review this change request!');
413+
});
414+
415+
it('allows second requested reviewer to review when reviewers are requested', async () => {
416+
await ChangeRequestsService.requestCRReview(
417+
submitterUser,
418+
[leadershipUser1.userId, leadershipUser2.userId],
419+
changeRequestId,
420+
organization
421+
);
422+
423+
const reviewResult = await ChangeRequestsService.reviewChangeRequest(
424+
leadershipUser2,
425+
changeRequestId,
426+
'Approved by second reviewer',
427+
true,
428+
organization,
429+
null
430+
);
431+
432+
expect(reviewResult).toBe(changeRequestId);
433+
434+
const updatedCR = await prisma.change_Request.findUnique({
435+
where: { crId: changeRequestId }
436+
});
437+
438+
expect(updatedCR?.reviewerId).toBe(leadershipUser2.userId);
439+
expect(updatedCR?.accepted).toBe(true);
440+
});
441+
442+
it('rejects member user even when they are in requested reviewers', async () => {
443+
await ChangeRequestsService.requestCRReview(
444+
submitterUser,
445+
[leadershipUser1.userId, memberUser.userId],
446+
changeRequestId,
447+
organization
448+
);
449+
450+
await expect(
451+
ChangeRequestsService.reviewChangeRequest(
452+
memberUser,
453+
changeRequestId,
454+
'I want to review',
455+
false,
456+
organization,
457+
null
458+
)
459+
).rejects.toThrow();
460+
});
461+
462+
it('allows rejection by non-requested leadership when reviewers are requested', async () => {
463+
await ChangeRequestsService.requestCRReview(
464+
submitterUser,
465+
[leadershipUser1.userId],
466+
changeRequestId,
467+
organization
468+
);
469+
470+
await expect(
471+
ChangeRequestsService.reviewChangeRequest(
472+
nonRequestedLeadership,
473+
changeRequestId,
474+
'Rejecting this',
475+
false,
476+
organization,
477+
null
478+
)
479+
).rejects.toThrow(AccessDeniedException);
480+
});
481+
});
287482
});

‎src/frontend/src/components/ActionsMenu.tsx‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { Box } from '@mui/system';
22
import { ElementType, ReactElement, useState } from 'react';
33
import { NERButton } from './NERButton';
44
import { ArrowDropDown } from '@mui/icons-material';
5-
import { Divider, ListItemIcon, Menu, MenuItem } from '@mui/material';
5+
import { Divider, ListItemIcon, Menu, MenuItem, Tooltip } from '@mui/material';
66
import { isGuest } from 'shared';
77
import { useCurrentUser } from '../hooks/users.hooks';
88

@@ -14,6 +14,7 @@ export type ButtonInfo = {
1414
dividerTop?: boolean;
1515
component?: ElementType<any>;
1616
to?: string;
17+
tooltip?: string;
1718
};
1819

1920
interface ActionsMenuProps {
@@ -65,8 +66,7 @@ const ActionsMenu: React.FC<ActionsMenuProps> = ({ buttons, title = 'Actions' })
6566
}}
6667
>
6768
{buttons.flatMap((button, index) => {
68-
return [
69-
button.dividerTop && <Divider key={`${index}-divider`} />,
69+
const menuItem = (
7070
<MenuItem
7171
key={index}
7272
{...(button.component ? { component: button.component } : {})}
@@ -80,6 +80,17 @@ const ActionsMenu: React.FC<ActionsMenuProps> = ({ buttons, title = 'Actions' })
8080
{button.icon && <ListItemIcon>{button.icon}</ListItemIcon>}
8181
{button.title}
8282
</MenuItem>
83+
);
84+
85+
return [
86+
button.dividerTop && <Divider key={`${index}-divider`} />,
87+
button.tooltip && button.disabled ? (
88+
<Tooltip key={index} title={button.tooltip} placement="left" arrow>
89+
<span>{menuItem}</span>
90+
</Tooltip>
91+
) : (
92+
menuItem
93+
)
8394
];
8495
})}
8596
</Menu>

‎src/frontend/src/pages/ChangeRequestDetailPage/ChangeRequestActionMenu.tsx‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import { taskUserToAutocompleteOption } from '../../utils/task.utils';
2222

2323
interface ChangeRequestActionMenuProps {
2424
isUserAllowedToReview: boolean;
25+
reviewDisabledTooltip?: string;
2526
isUserAllowedToImplement: boolean;
2627
isUserAllowedToDelete: boolean;
2728
changeRequest: ChangeRequest;
@@ -31,6 +32,7 @@ interface ChangeRequestActionMenuProps {
3132

3233
const ChangeRequestActionMenu: React.FC<ChangeRequestActionMenuProps> = ({
3334
isUserAllowedToReview,
35+
reviewDisabledTooltip,
3436
isUserAllowedToImplement,
3537
isUserAllowedToDelete,
3638
changeRequest,
@@ -77,7 +79,8 @@ const ChangeRequestActionMenu: React.FC<ChangeRequestActionMenuProps> = ({
7779
title: 'Review',
7880
onClick: handleReviewOpen,
7981
disabled: !isUserAllowedToReview,
80-
icon: <ContentPasteIcon fontSize="small" />
82+
icon: <ContentPasteIcon fontSize="small" />,
83+
tooltip: reviewDisabledTooltip
8184
},
8285
{
8386
title: 'Delete',

‎src/frontend/src/pages/ChangeRequestDetailPage/ChangeRequestDetails.tsx‎

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,33 @@ const ChangeRequestDetails: React.FC = () => {
2323

2424
if (isError) return <ErrorPage message={error?.message} />;
2525

26+
// Determine if user can review and why they can't
27+
const isLeadership = !isNotLeadership(auth.user?.role);
28+
const isSubmitter = auth.user?.userId === data?.submitter.userId;
29+
const isOpen = data!.status === ChangeRequestStatus.Open;
30+
const hasRequestedReviewers = data!.requestedReviewers.length > 0;
31+
const isInRequestedReviewers = data!.requestedReviewers.some((reviewer) => reviewer.userId === auth.user?.userId);
32+
33+
const isUserAllowedToReview = isLeadership && !isSubmitter && isOpen && (!hasRequestedReviewers || isInRequestedReviewers);
34+
35+
// Generate tooltip message explaining why review is disabled
36+
let reviewDisabledTooltip: string | undefined;
37+
if (!isUserAllowedToReview) {
38+
if (!isLeadership) {
39+
reviewDisabledTooltip = 'You must be a leadership member to review change requests';
40+
} else if (isSubmitter) {
41+
reviewDisabledTooltip = 'You cannot review your own change request';
42+
} else if (!isOpen) {
43+
reviewDisabledTooltip = 'This change request is not open for review';
44+
} else if (hasRequestedReviewers && !isInRequestedReviewers) {
45+
reviewDisabledTooltip = 'Only requested reviewers can review this change request';
46+
}
47+
}
48+
2649
return (
2750
<ChangeRequestDetailsView
28-
isUserAllowedToReview={
29-
!isNotLeadership(auth.user?.role) &&
30-
auth.user?.userId !== data?.submitter.userId &&
31-
data!.status === ChangeRequestStatus.Open
32-
}
51+
isUserAllowedToReview={isUserAllowedToReview}
52+
reviewDisabledTooltip={reviewDisabledTooltip}
3353
isUserAllowedToImplement={!isGuest(auth.user?.role)}
3454
isUserAllowedToDelete={
3555
isAdmin(auth.user?.role) ||

‎src/frontend/src/pages/ChangeRequestDetailPage/ChangeRequestDetailsView.tsx‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,13 +45,15 @@ const buildDetails = (cr: ChangeRequest): ReactElement => {
4545
};
4646
interface ChangeRequestDetailsProps {
4747
isUserAllowedToReview: boolean;
48+
reviewDisabledTooltip?: string;
4849
isUserAllowedToImplement: boolean;
4950
isUserAllowedToDelete: boolean;
5051
changeRequest: ChangeRequest;
5152
}
5253

5354
const ChangeRequestDetailsView: React.FC<ChangeRequestDetailsProps> = ({
5455
isUserAllowedToReview,
56+
reviewDisabledTooltip,
5557
isUserAllowedToImplement,
5658
isUserAllowedToDelete,
5759
changeRequest
@@ -83,6 +85,7 @@ const ChangeRequestDetailsView: React.FC<ChangeRequestDetailsProps> = ({
8385
headerRight={
8486
<ChangeRequestActionMenu
8587
isUserAllowedToReview={isUserAllowedToReview}
88+
reviewDisabledTooltip={reviewDisabledTooltip}
8689
isUserAllowedToImplement={isUserAllowedToImplement}
8790
isUserAllowedToDelete={isUserAllowedToDelete}
8891
changeRequest={changeRequest}

0 commit comments

Comments
 (0)