diff --git a/backend/src/models/Response.ts b/backend/src/models/Response.ts index 153f3a85dc..2f4bfa2e43 100644 --- a/backend/src/models/Response.ts +++ b/backend/src/models/Response.ts @@ -5,6 +5,7 @@ export const Decision = { RequestChanges: 'request_changes', Approve: 'approve', Undo: 'undo', + Deny: 'deny', } as const export type DecisionKeys = (typeof Decision)[keyof typeof Decision] diff --git a/backend/src/models/Review.ts b/backend/src/models/Review.ts index 9dae5eaa83..44a130ab36 100644 --- a/backend/src/models/Review.ts +++ b/backend/src/models/Review.ts @@ -2,6 +2,7 @@ import { model, Schema } from 'mongoose' import MongooseDelete, { SoftDeleteDocument } from 'mongoose-delete' import { ReviewKind, ReviewKindKeys } from '../types/enums.js' +import { DecisionKeys } from './Response.js' // This interface stores information about the properties on the base object. // It should be used for plain object representations, e.g. for sending to the @@ -16,6 +17,8 @@ export interface ReviewInterface { createdAt: Date updatedAt: Date + + decision?: DecisionKeys } // The doc type includes all values in the plain interface, as well as all the diff --git a/backend/src/routes/v2/review/getReviews.ts b/backend/src/routes/v2/review/getReviews.ts index 7a5a7cb832..6743b9648f 100644 --- a/backend/src/routes/v2/review/getReviews.ts +++ b/backend/src/routes/v2/review/getReviews.ts @@ -17,6 +17,7 @@ export const getReviewsSchema = z.object({ accessRequestId: z.string().optional(), kind: z.nativeEnum(ReviewKind).optional(), mine: strictCoerceBoolean(z.boolean().optional().default(true)), + decision: strictCoerceBoolean(z.boolean().optional().default(false)), }), }) @@ -49,10 +50,10 @@ export const getReviews = [ async (req: Request, res: Response) => { req.audit = AuditInfo.SearchReviews const { - query: { mine, modelId, semver, accessRequestId, kind }, + query: { mine, modelId, semver, accessRequestId, kind, decision }, } = parse(req, getReviewsSchema) - const reviews = await findReviews(req.user, mine, modelId, semver, accessRequestId, kind) + const reviews = await findReviews(req.user, mine, decision, modelId, semver, accessRequestId, kind) await audit.onSearchReviews(req, reviews) res.setHeader('x-count', reviews.length) diff --git a/backend/src/routes/v2/review/postAccessRequestReviewResponse.ts b/backend/src/routes/v2/review/postAccessRequestReviewResponse.ts index 30eda1a0fb..2792db8060 100644 --- a/backend/src/routes/v2/review/postAccessRequestReviewResponse.ts +++ b/backend/src/routes/v2/review/postAccessRequestReviewResponse.ts @@ -17,12 +17,12 @@ const staticProperties = z.object({ const optionalComment = z.object({ comment: z.string().optional(), - decision: z.enum(getEnumValues(Decision)).exclude([Decision.RequestChanges]), + decision: z.enum(getEnumValues(Decision)).exclude([Decision.RequestChanges, Decision.Deny]), }) const mandatoryComment = z.object({ - comment: z.string().min(1, 'A comment must be supplied when requesting changes'), - decision: z.literal(Decision.RequestChanges), + comment: z.string().min(1, 'A comment must be supplied when requesting changes or denying a request'), + decision: z.enum(getEnumValues(Decision)).exclude([Decision.Approve, Decision.Undo]), }) export const postAccessRequestReviewResponseSchema = z.object({ diff --git a/backend/src/routes/v2/review/postReleaseReviewResponse.ts b/backend/src/routes/v2/review/postReleaseReviewResponse.ts index d309749ca1..d4284dfd10 100644 --- a/backend/src/routes/v2/review/postReleaseReviewResponse.ts +++ b/backend/src/routes/v2/review/postReleaseReviewResponse.ts @@ -17,12 +17,12 @@ const staticProperties = z.object({ const optionalComment = z.object({ comment: z.string().optional(), - decision: z.enum(getEnumValues(Decision)).exclude([Decision.RequestChanges]), + decision: z.enum(getEnumValues(Decision)).exclude([Decision.RequestChanges, Decision.Deny]), }) const mandatoryComment = z.object({ - comment: z.string().min(1, 'A comment must be supplied when requesting changes'), - decision: z.literal(Decision.RequestChanges), + comment: z.string().min(1, 'A comment must be supplied when requesting changes or denying a request'), + decision: z.enum(getEnumValues(Decision)).exclude([Decision.Approve, Decision.Undo]), }) export const postReleaseReviewResponseSchema = z.object({ diff --git a/backend/src/services/response.ts b/backend/src/services/response.ts index 94ea33f16a..9828663881 100644 --- a/backend/src/services/response.ts +++ b/backend/src/services/response.ts @@ -1,6 +1,10 @@ +import { ObjectId } from 'mongoose' + import ResponseModel, { Decision, + DecisionKeys, ReactionKindKeys, + ResponseDoc, ResponseInterface, ResponseKind, ResponseReaction, @@ -9,6 +13,7 @@ import { ReviewDoc } from '../models/Review.js' import { UserInterface } from '../models/User.js' import { WebhookEvent } from '../models/Webhook.js' import { ReviewKind, ReviewKindKeys } from '../types/enums.js' +import { IdRoles } from '../types/types.js' import { toEntity } from '../utils/entity.js' import { Forbidden, InternalError, NotFound } from '../utils/error.js' import { getAccessRequestById } from './accessRequest.js' @@ -87,6 +92,55 @@ export async function updateResponseReaction(user: UserInterface, responseId: st return response } +export function determineOverallResponseStatus( + roles: Array, + decisions: ResponseDoc, +): DecisionKeys | undefined { + if (!decisions || decisions.length === 0) { + return undefined + } + + const uniqueDecisions = new Set() + decisions.filter((d) => { + if (Decision.Undo != d.decision) { + uniqueDecisions.add(d.decision) + } + }) + + if (uniqueDecisions.has(Decision.Deny)) { + return Decision.Deny + } + + if (uniqueDecisions.has(Decision.RequestChanges)) { + return Decision.RequestChanges + } + + if (uniqueDecisions.has(Decision.Approve) && decisions.every((d) => Decision.Approve === d.decision)) { + return Decision.Approve + } + + return undefined +} + +export async function getUniqueResponseStatus(_user: UserInterface, reviewIdRoles: IdRoles[]) { + // Given the IDs for reviews (parent IDs), gather the current responses for each of the roles + const parentIds = reviewIdRoles.map((r) => r.id as ObjectId) + const responses: Array<{ _id: { parentId: string; role: string }; latestResponse: ResponseDoc }> = + await ResponseModel.aggregate() + .match({ + parentId: { $in: parentIds }, + decision: { $ne: Decision.Undo }, + }) + .sort({ parentId: 1, role: 1, updatedAt: -1 }) + .group({ _id: { parentId: '$parentId', role: '$role' }, latestResponse: { $first: '$$ROOT' } }) + + if (!responses) { + throw NotFound('Request responses not found', { parentIds }) + } + + return responses +} + export type ReviewResponseParams = Pick export async function respondToReview( user: UserInterface, diff --git a/backend/src/services/review.ts b/backend/src/services/review.ts index 61876cc096..5132bc643d 100644 --- a/backend/src/services/review.ts +++ b/backend/src/services/review.ts @@ -1,3 +1,5 @@ +import { ObjectId } from 'mongoose' + import authentication from '../connectors/authentication/index.js' import { ModelAction, ReviewRoleAction } from '../connectors/authorisation/actions.js' import authorisation from '../connectors/authorisation/index.js' @@ -8,29 +10,35 @@ import Review, { ReviewDoc, ReviewInterface } from '../models/Review.js' import ReviewRoleModel, { ReviewRoleInterface } from '../models/ReviewRole.js' import { UserInterface } from '../models/User.js' import { ReviewKind, ReviewKindKeys } from '../types/enums.js' +import { IdRoles } from '../types/types.js' import { BadReq, Forbidden, InternalError, NotFound } from '../utils/error.js' import { handleDuplicateKeys } from '../utils/mongo.js' import log from './log.js' import { getModelById } from './model.js' +import { determineOverallResponseStatus, getUniqueResponseStatus } from './response.js' import { requestReviewForAccessRequest, requestReviewForRelease } from './smtp/smtp.js' // This should be replaced by using the dynamic schema const requiredRoles = { release: ['mtr', 'msro'], - accessRequest: ['msro'], + access: ['msro'], } -export const allReviewRoles = [...new Set(requiredRoles.release.concat(requiredRoles.accessRequest))] +export const allReviewRoles = [...new Set(requiredRoles.release.concat(requiredRoles.access))] + +type ReviewModelDoc = ReviewDoc & { model: ModelDoc } +type ReviewModelInterface = ReviewInterface & { model: ModelInterface } export async function findReviews( user: UserInterface, mine: boolean, + decision: boolean = false, modelId?: string, semver?: string, accessRequestId?: string, kind?: string, -): Promise<(ReviewInterface & { model: ModelInterface })[]> { - const reviews = await Review.aggregate() +): Promise { + const reviews = await Review.aggregate() .match({ ...(modelId && { modelId }), ...(semver && { semver }), @@ -50,7 +58,36 @@ export async function findReviews( ModelAction.View, ) - return reviews.filter((_, i) => auths[i].success) + const validReviews = reviews.filter((_, i) => auths[i].success) + + // Default is to not calculate a decision for each review, which can be expensive + if (!decision) { + return validReviews + } + + // For each review the user can see, get the list of required roles + const reviewIdRoles: Array = validReviews.map((reviewWithModel: ReviewDoc & { model: ModelDoc }) => { + return { + // Review ID or 'parent' ID + id: reviewWithModel._id as ObjectId, + // Based on the model, and the type of review, provide the list of role entities that should be providing responses + roles: getRoleEntities(requiredRoles[reviewWithModel.kind], reviewWithModel.model.collaborators).map( + (roleEntity) => roleEntity.role, + ), + } + }) + + // Get decisions for the reviews + const decisions = await getUniqueResponseStatus(user, reviewIdRoles) + + return validReviews.map((review) => { + const decisionsForReview = decisions.find((s) => s._id.parentId == review._id) + if (decisionsForReview) { + const decision = determineOverallResponseStatus(reviewIdRoles, decisionsForReview.latestResponse) + review.decision = decision + } + return review + }) } export async function createReleaseReviews(model: ModelDoc, release: ReleaseDoc) { @@ -74,7 +111,7 @@ export async function createReleaseReviews(model: ModelDoc, release: ReleaseDoc) } export async function createAccessRequestReviews(model: ModelDoc, accessRequest: AccessRequestDoc) { - const roleEntities = getRoleEntities(requiredRoles.accessRequest, model.collaborators) + const roleEntities = getRoleEntities(requiredRoles.access, model.collaborators) const createReviews = roleEntities.map((roleInfo) => { const review = new Review({ @@ -162,7 +199,7 @@ export async function findReviewsForAccessRequests(accessRequestIds: string[]) { const reviews: ReviewDoc[] = await Review.find({ accessRequestId: accessRequestIds, }) - return reviews.filter((review) => requiredRoles.accessRequest.includes(review.role)) + return reviews.filter((review) => requiredRoles.access.includes(review.role)) } function getRoleEntities(roles: string[], collaborators: CollaboratorEntry[]) { diff --git a/backend/src/types/types.ts b/backend/src/types/types.ts index 13f3354b97..dae4d60efd 100644 --- a/backend/src/types/types.ts +++ b/backend/src/types/types.ts @@ -1,3 +1,5 @@ +import { ObjectId } from 'mongoose' + export type PartialDeep = T extends object ? { [P in keyof T]?: PartialDeep @@ -24,6 +26,11 @@ export interface Role { description?: string } +export type IdRoles = { + id: ObjectId + roles: string[] +} + export type PermissionDetail = | { hasPermission: true diff --git a/frontend/actions/review.ts b/frontend/actions/review.ts index e74b764fe0..69832c6b99 100644 --- a/frontend/actions/review.ts +++ b/frontend/actions/review.ts @@ -18,7 +18,7 @@ export function useGetReviewRequestsForUser() { reviews: ReviewRequestInterface[] }, ErrorInfo - >('/api/v2/reviews', fetcher) + >('/api/v2/reviews?decision=true', fetcher) return { mutateReviews: mutate, diff --git a/frontend/src/common/ReviewWithComment.tsx b/frontend/src/common/ReviewWithComment.tsx index 22f37e34a7..756d0b82c0 100644 --- a/frontend/src/common/ReviewWithComment.tsx +++ b/frontend/src/common/ReviewWithComment.tsx @@ -184,6 +184,7 @@ export default function ReviewWithComment({ )} submitForm(Decision.RequestChanges)} loading={loading} data-test='requestChangesReviewButton' @@ -192,6 +193,16 @@ export default function ReviewWithComment({ submitForm(Decision.Deny)} + loading={loading} + data-test='denyReviewButton' + > + Deny + + submitForm(Decision.Approve)} loading={loading} data-test='approveReviewButton' diff --git a/frontend/src/entry/model/reviews/ApprovalsDisplay.tsx b/frontend/src/entry/model/reviews/ApprovalsDisplay.tsx index ccf36a6d1d..c4b25ddf37 100644 --- a/frontend/src/entry/model/reviews/ApprovalsDisplay.tsx +++ b/frontend/src/entry/model/reviews/ApprovalsDisplay.tsx @@ -1,10 +1,11 @@ -import { Done } from '@mui/icons-material' +import { Done, ErrorOutline } from '@mui/icons-material' import { Stack, Tooltip, Typography } from '@mui/material' import { useGetModelRoles } from 'actions/model' import { ReactElement, useMemo } from 'react' import Loading from 'src/common/Loading' import MessageAlert from 'src/MessageAlert' import { ResponseInterface } from 'types/types' +import { isFinalised } from 'utils/reviewUtils' import { plural } from 'utils/stringUtils' interface ApprovalsDisplayProps { @@ -26,12 +27,17 @@ export default function ApprovalsDisplay({ const roleApprovals = useMemo( () => - dynamicRoles.reduce((approvals, dynamicRole) => { - const hasRoleApproved = !!acceptedReviewResponses.find( - (acceptedResponse) => acceptedResponse.role === dynamicRole.id, + dynamicRoles.reduce((decisions, dynamicRole) => { + const finalisedDecisions = acceptedReviewResponses.filter( + (r) => isFinalised(r.decision) && r.role === dynamicRole.id, ) - if (hasRoleApproved) { - approvals.push( + const latestDecision = finalisedDecisions.at(0) + if (!latestDecision || !latestDecision.decision) { + return decisions + } + + if (latestDecision.decision === 'approve') { + decisions.push( @@ -44,7 +50,21 @@ export default function ApprovalsDisplay({ , ) } - return approvals + if (latestDecision.decision === 'deny') { + decisions.push( + + + + + {showCurrentUserResponses + ? `You have denied this as a ${dynamicRole.name}` + : `Denied by ${dynamicRole.name}`} + + + , + ) + } + return decisions }, []), [acceptedReviewResponses, dynamicRoles, showCurrentUserResponses], ) diff --git a/frontend/src/entry/model/reviews/ReviewDisplay.tsx b/frontend/src/entry/model/reviews/ReviewDisplay.tsx index 1b9123401c..0ca49d6dee 100644 --- a/frontend/src/entry/model/reviews/ReviewDisplay.tsx +++ b/frontend/src/entry/model/reviews/ReviewDisplay.tsx @@ -5,6 +5,7 @@ import ApprovalsDisplay from 'src/entry/model/reviews/ApprovalsDisplay' import { Decision, ResponseInterface } from 'types/types' import { sortByCreatedAtDescending } from 'utils/arrayUtils' import { fromEntity } from 'utils/entityUtils' +import { isFinalised } from 'utils/reviewUtils' import { plural } from 'utils/stringUtils' export interface ReviewDisplayProps { @@ -32,7 +33,7 @@ export default function ReviewDisplay({ return ( <> - {orderedReviewResponses.length > 0 && orderedReviewResponses[0].decision === Decision.Approve && ( + {orderedReviewResponses.length > 0 && isFinalised(orderedReviewResponses[0].decision) && ( {response.decision === Decision.Approve && 'has approved'} + + {response.decision === Decision.Deny && 'has denied'} + {response.decision === Decision.RequestChanges && 'has requested changes'} {response.decision === Decision.Undo && 'has undone their review'} {response.decision === Decision.Approve && } + {response.decision === Decision.Deny && } {response.decision === Decision.RequestChanges && } diff --git a/frontend/src/reviews/ReviewsList.tsx b/frontend/src/reviews/ReviewsList.tsx index c74bd227ea..314978f949 100644 --- a/frontend/src/reviews/ReviewsList.tsx +++ b/frontend/src/reviews/ReviewsList.tsx @@ -1,14 +1,13 @@ import { List } from '@mui/material' -import { useGetResponses } from 'actions/response' import { useGetReviewRequestsForUser } from 'actions/review' -import { useGetCurrentUser } from 'actions/user' import { memoize } from 'lodash-es' -import { useCallback, useEffect, useState } from 'react' +import { useEffect, useState } from 'react' import Loading from 'src/common/Loading' import Paginate from 'src/common/Paginate' import MessageAlert from 'src/MessageAlert' import ReviewItem from 'src/reviews/ReviewItem' import { ReviewListStatus, ReviewListStatusKeys, ReviewRequestInterface } from 'types/types' +import { isFinalised } from 'utils/reviewUtils' type ReviewsListProps = { kind: 'release' | 'access' @@ -17,36 +16,19 @@ type ReviewsListProps = { export default function ReviewsList({ kind, status }: ReviewsListProps) { const { reviews, isReviewsLoading, isReviewsError } = useGetReviewRequestsForUser() - const { responses, isResponsesLoading, isResponsesError } = useGetResponses(reviews.map((review) => review._id)) - const { currentUser, isCurrentUserLoading, isCurrentUserError } = useGetCurrentUser() const [filteredReviews, setFilteredReviews] = useState([]) - const approvedByUser = useCallback( - (review: ReviewRequestInterface) => { - return ( - currentUser && - responses.find( - (response) => - response.parentId === review._id && - response.entity === `user:${currentUser.dn}` && - response.decision === 'approve', - ) - ) - }, - [currentUser, responses], - ) - useEffect(() => { if (status === ReviewListStatus.ARCHIVED) { setFilteredReviews( - reviews.filter((filteredReview) => filteredReview.kind === kind && approvedByUser(filteredReview)), + reviews.filter((filteredReview) => filteredReview.kind === kind && isFinalised(filteredReview.decision)), ) } else { setFilteredReviews( - reviews.filter((filteredReview) => filteredReview.kind === kind && !approvedByUser(filteredReview)), + reviews.filter((filteredReview) => filteredReview.kind === kind && !isFinalised(filteredReview.decision)), ) } - }, [reviews, kind, approvedByUser, status]) + }, [reviews, kind, status]) const ReviewListItem = memoize(({ data, index }) => ( } - if (isResponsesError) { - return - } - - if (isCurrentUserError) { - return - } - return ( <> - {(isCurrentUserLoading || isReviewsLoading || isResponsesLoading) && } + {isReviewsLoading && } { diff --git a/frontend/types/types.ts b/frontend/types/types.ts index e79bb35889..e7a79b31b9 100644 --- a/frontend/types/types.ts +++ b/frontend/types/types.ts @@ -517,6 +517,7 @@ export const Decision = { RequestChanges: 'request_changes', Approve: 'approve', Undo: 'undo', + Deny: 'deny', } as const export type DecisionKeys = (typeof Decision)[keyof typeof Decision] @@ -543,6 +544,7 @@ export type ReviewRequestInterface = { kind: 'release' | 'access' createdAt: string updatedAt: string + decision?: DecisionKeys } & PartialReviewRequestInterface export interface InferenceInterface { diff --git a/frontend/utils/reviewUtils.ts b/frontend/utils/reviewUtils.ts index 310c980824..48af74aab4 100644 --- a/frontend/utils/reviewUtils.ts +++ b/frontend/utils/reviewUtils.ts @@ -1,11 +1,17 @@ import { groupBy } from 'lodash-es' -import { ResponseInterface, ReviewRequestInterface } from 'types/types' +import { DecisionKeys, ResponseInterface, ReviewRequestInterface } from 'types/types' import { sortByCreatedAtAscending } from 'utils/arrayUtils' interface GroupedReviewResponse { [user: string]: ResponseInterface[] } +export const finalisedDecisions: Array = ['approve', 'deny'] + +export function isFinalised(decision: DecisionKeys | undefined) { + return decision && finalisedDecisions.includes(decision) +} + export function latestReviewsForEachUser(reviews: ReviewRequestInterface[], responses: ResponseInterface[]) { const latestReviewResponses: ResponseInterface[] = [] reviews.forEach((review) => {