Skip to content

Commit ca6acc3

Browse files
walker-seanwalker-sean
authored
Merge pull request #3829 from Northeastern-Electric-Racing/#3802-specific-reviewers-on-crs
#3802 if cr has requested reviewers, only they can approve
2 parents 01f68cc + 584c25e commit ca6acc3

6 files changed

Lines changed: 259 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: 205 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
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 { supermanAdmin, aquamanLeadership, greenlanternHead, flashAdmin, robinMember } from '../test-data/users.test-data';
55
import { ProjectProposedChangesCreateArgs, WorkPackageProposedChangesCreateArgs } from 'shared';
66
import prisma from '../../src/prisma/prisma';
7+
import { AccessDeniedException } from '../../src/utils/errors.utils';
78

89
describe('Change Request Tests', () => {
910
let orgId: string;
@@ -284,4 +285,207 @@ describe('Change Request Tests', () => {
284285
expect(wbsElement?.workPackageNumber).toEqual(14);
285286
});
286287
});
288+
289+
describe('Review Change Request with Requested Reviewers', () => {
290+
let submitterUser: User;
291+
let leadershipUser1: User;
292+
let leadershipUser2: User;
293+
let nonRequestedLeadership: User;
294+
let memberUser: User;
295+
let changeRequestId: string;
296+
297+
beforeEach(async () => {
298+
// Use the existing user from the main beforeEach
299+
submitterUser = user;
300+
301+
// Create users with User_Settings that include slackId (needed for requestCRReview)
302+
leadershipUser1 = await createTestUser(aquamanLeadership, orgId, {
303+
id: 'aquaman-settings',
304+
userId: '',
305+
defaultTheme: 'DARK' as any,
306+
slackId: 'slack-aquaman'
307+
});
308+
309+
leadershipUser2 = await createTestUser(greenlanternHead, orgId, {
310+
id: 'greenlantern-settings',
311+
userId: '',
312+
defaultTheme: 'DARK' as any,
313+
slackId: 'slack-greenlantern'
314+
});
315+
316+
nonRequestedLeadership = await createTestUser(flashAdmin, orgId, {
317+
id: 'flash-settings',
318+
userId: '',
319+
defaultTheme: 'DARK' as any,
320+
slackId: 'slack-flash'
321+
});
322+
323+
memberUser = await createTestUser(robinMember, orgId);
324+
325+
// Create a simple change request with a proposed solution
326+
const cr = await ChangeRequestsService.createStandardChangeRequest(
327+
submitterUser,
328+
12,
329+
13,
330+
14,
331+
CR_Type.ISSUE,
332+
'What is being changed',
333+
[{ type: Scope_CR_Why_Type.COMPETITION, explain: 'Why it is being changed' }],
334+
[
335+
{
336+
description: 'Proposed solution',
337+
scopeImpact: 'Low impact',
338+
timelineImpact: 0,
339+
budgetImpact: 0
340+
}
341+
],
342+
organization,
343+
null,
344+
null
345+
);
346+
347+
changeRequestId = cr.crId;
348+
});
349+
350+
it('allows any leadership to review when no reviewers are requested', async () => {
351+
const reviewResult = await ChangeRequestsService.reviewChangeRequest(
352+
nonRequestedLeadership,
353+
changeRequestId,
354+
'Looks good',
355+
false,
356+
organization,
357+
null
358+
);
359+
360+
expect(reviewResult).toBe(changeRequestId);
361+
362+
const updatedCR = await prisma.change_Request.findUnique({
363+
where: { crId: changeRequestId }
364+
});
365+
366+
expect(updatedCR?.reviewerId).toBe(nonRequestedLeadership.userId);
367+
expect(updatedCR?.accepted).toBe(false);
368+
});
369+
370+
it('allows requested reviewer to review when reviewers are requested', async () => {
371+
await ChangeRequestsService.requestCRReview(
372+
submitterUser,
373+
[leadershipUser1.userId, leadershipUser2.userId],
374+
changeRequestId,
375+
organization
376+
);
377+
378+
const reviewResult = await ChangeRequestsService.reviewChangeRequest(
379+
leadershipUser1,
380+
changeRequestId,
381+
'Approved',
382+
false,
383+
organization,
384+
null
385+
);
386+
387+
expect(reviewResult).toBe(changeRequestId);
388+
389+
const updatedCR = await prisma.change_Request.findUnique({
390+
where: { crId: changeRequestId }
391+
});
392+
393+
expect(updatedCR?.reviewerId).toBe(leadershipUser1.userId);
394+
expect(updatedCR?.accepted).toBe(false);
395+
});
396+
397+
it('rejects non-requested leadership when reviewers are requested', async () => {
398+
await ChangeRequestsService.requestCRReview(
399+
submitterUser,
400+
[leadershipUser1.userId, leadershipUser2.userId],
401+
changeRequestId,
402+
organization
403+
);
404+
405+
await expect(
406+
ChangeRequestsService.reviewChangeRequest(
407+
nonRequestedLeadership,
408+
changeRequestId,
409+
'I want to review this',
410+
true,
411+
organization,
412+
null
413+
)
414+
).rejects.toThrow(AccessDeniedException);
415+
416+
await expect(
417+
ChangeRequestsService.reviewChangeRequest(
418+
nonRequestedLeadership,
419+
changeRequestId,
420+
'I want to review this',
421+
true,
422+
organization,
423+
null
424+
)
425+
).rejects.toThrow('Only requested reviewers can review this change request!');
426+
});
427+
428+
it('allows second requested reviewer to review when reviewers are requested', async () => {
429+
await ChangeRequestsService.requestCRReview(
430+
submitterUser,
431+
[leadershipUser1.userId, leadershipUser2.userId],
432+
changeRequestId,
433+
organization
434+
);
435+
436+
const reviewResult = await ChangeRequestsService.reviewChangeRequest(
437+
leadershipUser2,
438+
changeRequestId,
439+
'Approved by second reviewer',
440+
false,
441+
organization,
442+
null
443+
);
444+
445+
expect(reviewResult).toBe(changeRequestId);
446+
447+
const updatedCR = await prisma.change_Request.findUnique({
448+
where: { crId: changeRequestId }
449+
});
450+
451+
expect(updatedCR?.reviewerId).toBe(leadershipUser2.userId);
452+
expect(updatedCR?.accepted).toBe(false);
453+
});
454+
455+
it('rejects member user from being requested as a reviewer', async () => {
456+
// requestCRReview should fail when trying to add a non-leadership user
457+
await expect(
458+
ChangeRequestsService.requestCRReview(
459+
submitterUser,
460+
[leadershipUser1.userId, memberUser.userId],
461+
changeRequestId,
462+
organization
463+
)
464+
).rejects.toThrow(AccessDeniedException);
465+
466+
await expect(
467+
ChangeRequestsService.requestCRReview(
468+
submitterUser,
469+
[leadershipUser1.userId, memberUser.userId],
470+
changeRequestId,
471+
organization
472+
)
473+
).rejects.toThrow('The following user(s) are not leadership: Dick Grayson');
474+
});
475+
476+
it('allows rejection by non-requested leadership when reviewers are requested', async () => {
477+
await ChangeRequestsService.requestCRReview(submitterUser, [leadershipUser1.userId], changeRequestId, organization);
478+
479+
await expect(
480+
ChangeRequestsService.reviewChangeRequest(
481+
nonRequestedLeadership,
482+
changeRequestId,
483+
'Rejecting this',
484+
false,
485+
organization,
486+
null
487+
)
488+
).rejects.toThrow(AccessDeniedException);
489+
});
490+
});
287491
});

‎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)