Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions backend/src/models/Response.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand Down
3 changes: 3 additions & 0 deletions backend/src/models/Review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
5 changes: 3 additions & 2 deletions backend/src/routes/v2/review/getReviews.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)),
}),
})

Expand Down Expand Up @@ -49,10 +50,10 @@ export const getReviews = [
async (req: Request, res: Response<GetReviewResponse>) => {
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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down
6 changes: 3 additions & 3 deletions backend/src/routes/v2/review/postReleaseReviewResponse.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down
54 changes: 54 additions & 0 deletions backend/src/services/response.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import { ObjectId } from 'mongoose'

import ResponseModel, {
Decision,
DecisionKeys,
ReactionKindKeys,
ResponseDoc,
ResponseInterface,
ResponseKind,
ResponseReaction,
Expand All @@ -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'
Expand Down Expand Up @@ -87,6 +92,55 @@ export async function updateResponseReaction(user: UserInterface, responseId: st
return response
}

export function determineOverallResponseStatus(
roles: Array<IdRoles>,
decisions: ResponseDoc,
): DecisionKeys | undefined {
if (!decisions || decisions.length === 0) {
return undefined
}

const uniqueDecisions = new Set<DecisionKeys>()
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<ResponseInterface, 'comment' | 'decision'>
export async function respondToReview(
user: UserInterface,
Expand Down
51 changes: 44 additions & 7 deletions backend/src/services/review.ts
Original file line number Diff line number Diff line change
@@ -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'
Expand All @@ -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<ReviewModelInterface[]> {
const reviews = await Review.aggregate<ReviewModelDoc>()
.match({
...(modelId && { modelId }),
...(semver && { semver }),
Expand All @@ -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<IdRoles> = 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) {
Expand All @@ -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({
Expand Down Expand Up @@ -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[]) {
Expand Down
7 changes: 7 additions & 0 deletions backend/src/types/types.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { ObjectId } from 'mongoose'

export type PartialDeep<T> = T extends object
? {
[P in keyof T]?: PartialDeep<T[P]>
Expand All @@ -24,6 +26,11 @@ export interface Role {
description?: string
}

export type IdRoles = {
id: ObjectId
roles: string[]
}

export type PermissionDetail =
| {
hasPermission: true
Expand Down
2 changes: 1 addition & 1 deletion frontend/actions/review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export function useGetReviewRequestsForUser() {
reviews: ReviewRequestInterface[]
},
ErrorInfo
>('/api/v2/reviews', fetcher)
>('/api/v2/reviews?decision=true', fetcher)

return {
mutateReviews: mutate,
Expand Down
11 changes: 11 additions & 0 deletions frontend/src/common/ReviewWithComment.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ export default function ReviewWithComment({
)}
<LoadingButton
variant='outlined'
color='warning'
onClick={() => submitForm(Decision.RequestChanges)}
loading={loading}
data-test='requestChangesReviewButton'
Expand All @@ -192,6 +193,16 @@ export default function ReviewWithComment({
</LoadingButton>
<LoadingButton
variant='contained'
color='error'
onClick={() => submitForm(Decision.Deny)}
loading={loading}
data-test='denyReviewButton'
>
Deny
</LoadingButton>
<LoadingButton
variant='contained'
color='success'
onClick={() => submitForm(Decision.Approve)}
loading={loading}
data-test='approveReviewButton'
Expand Down
34 changes: 27 additions & 7 deletions frontend/src/entry/model/reviews/ApprovalsDisplay.tsx
Original file line number Diff line number Diff line change
@@ -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 {
Expand All @@ -26,12 +27,17 @@ export default function ApprovalsDisplay({

const roleApprovals = useMemo(
() =>
dynamicRoles.reduce<ReactElement[]>((approvals, dynamicRole) => {
const hasRoleApproved = !!acceptedReviewResponses.find(
(acceptedResponse) => acceptedResponse.role === dynamicRole.id,
dynamicRoles.reduce<ReactElement[]>((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(
<Tooltip title={`${plural(acceptedReviewResponses.length, 'review')}`} key={dynamicRole.id}>
<Stack direction='row'>
<Done color='success' fontSize='small' />
Expand All @@ -44,7 +50,21 @@ export default function ApprovalsDisplay({
</Tooltip>,
)
}
return approvals
if (latestDecision.decision === 'deny') {
decisions.push(
<Tooltip title={`${plural(acceptedReviewResponses.length, 'review')}`} key={dynamicRole.id}>
<Stack direction='row'>
<ErrorOutline color='error' fontSize='small' />
<Typography variant='caption'>
{showCurrentUserResponses
? `You have denied this as a ${dynamicRole.name}`
: `Denied by ${dynamicRole.name}`}
</Typography>
</Stack>
</Tooltip>,
)
}
return decisions
}, []),
[acceptedReviewResponses, dynamicRoles, showCurrentUserResponses],
)
Expand Down
3 changes: 2 additions & 1 deletion frontend/src/entry/model/reviews/ReviewDisplay.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -32,7 +33,7 @@ export default function ReviewDisplay({

return (
<>
{orderedReviewResponses.length > 0 && orderedReviewResponses[0].decision === Decision.Approve && (
{orderedReviewResponses.length > 0 && isFinalised(orderedReviewResponses[0].decision) && (
<ApprovalsDisplay
modelId={modelId}
acceptedReviewResponses={orderedReviewResponses}
Expand Down
Loading
Loading