Skip to content
Merged
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
171 changes: 171 additions & 0 deletions src/api/review/review.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
Param,
Query,
NotFoundException,
BadRequestException,
InternalServerErrorException,
} from '@nestjs/common';
import {
Expand All @@ -31,6 +32,7 @@ import {
ReviewResponseDto,
ReviewItemRequestDto,
ReviewItemResponseDto,
ReviewProgressResponseDto,
mapReviewRequestToDto,
mapReviewItemRequestToDto,
} from 'src/dto/review.dto';
Expand All @@ -39,6 +41,7 @@ import { ScorecardStatus } from '../../dto/scorecard.dto';
import { LoggerService } from '../../shared/modules/global/logger.service';
import { PaginatedResponse, PaginationDto } from '../../dto/pagination.dto';
import { PrismaErrorService } from '../../shared/modules/global/prisma-error.service';
import { ResourceApiService } from '../../shared/modules/global/resource.service';

@ApiTags('Reviews')
@ApiBearerAuth()
Expand All @@ -49,6 +52,7 @@ export class ReviewController {
constructor(
private readonly prisma: PrismaService,
private readonly prismaErrorService: PrismaErrorService,
private readonly resourceApiService: ResourceApiService,
) {
this.logger = LoggerService.forRoot('ReviewController');
}
Expand Down Expand Up @@ -603,4 +607,171 @@ export class ReviewController {
});
}
}

@Get('/progress/:challengeId')
@Roles(UserRole.Admin, UserRole.Copilot, UserRole.Reviewer, UserRole.User)
@Scopes(Scope.ReadReview)
@ApiOperation({
summary: 'Get review progress for a specific challenge',
description:
'Calculate and return the review progress percentage for a challenge. Accessible to all authenticated users. | Scopes: read:review',
})
@ApiParam({
name: 'challengeId',
description: 'The ID of the challenge to calculate progress for',
example: 'challenge123',
})
@ApiResponse({
status: 200,
description: 'Review progress calculated successfully.',
type: ReviewProgressResponseDto,
})
@ApiResponse({
status: 400,
description: 'Invalid challengeId parameter.',
})
@ApiResponse({
status: 404,
description: 'Challenge not found or no data available.',
})
@ApiResponse({
status: 500,
description: 'Server error during calculation.',
})
async getReviewProgress(
@Param('challengeId') challengeId: string,
): Promise<ReviewProgressResponseDto> {
this.logger.log(
`Calculating review progress for challenge: ${challengeId}`,
);

try {
// Validate challengeId parameter
if (
!challengeId ||
typeof challengeId !== 'string' ||
challengeId.trim() === ''
) {
throw new Error('Invalid challengeId parameter');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using a more specific exception class like BadRequestException instead of throwing a generic Error for invalid challengeId to provide more context and consistency with other error handling.

}

// Get reviewers from Resource API
this.logger.debug('Fetching reviewers from Resource API');
const resources = await this.resourceApiService.getResources({
challengeId,
});

// Get resource roles to filter by reviewer role
const resourceRoles = await this.resourceApiService.getResourceRoles();

// Filter resources to get only reviewers
const reviewers = resources.filter((resource) => {
const role = resourceRoles[resource.roleId];
return role && role.name.toLowerCase().includes('reviewer');
});

const totalReviewers = reviewers.length;
this.logger.debug(
`Found ${totalReviewers} reviewers for challenge ${challengeId}`,
);

// Get submissions for the challenge
this.logger.debug('Fetching submissions for the challenge');
const submissions = await this.prisma.submission.findMany({
where: {
challengeId,
status: 'ACTIVE',
},
});

const submissionIds = submissions.map((s) => s.id);
const totalSubmissions = submissions.length;
this.logger.debug(
`Found ${totalSubmissions} submissions for challenge ${challengeId}`,
);

// Get submitted reviews for these submissions
this.logger.debug('Fetching submitted reviews');
const submittedReviews = await this.prisma.review.findMany({
where: {
submissionId: { in: submissionIds },
committed: true,
},
include: {
reviewItems: true,
},
});

const totalSubmittedReviews = submittedReviews.length;
this.logger.debug(`Found ${totalSubmittedReviews} submitted reviews`);

// Calculate progress percentage
let progressPercentage = 0;

if (totalReviewers > 0 && totalSubmissions > 0) {
const expectedTotalReviews = totalSubmissions * totalReviewers;
progressPercentage =
(totalSubmittedReviews / expectedTotalReviews) * 100;
// Round to 2 decimal places
progressPercentage = Math.round(progressPercentage * 100) / 100;
}

// Handle edge cases
if (progressPercentage > 100) {
progressPercentage = 100;
}

const result: ReviewProgressResponseDto = {
challengeId,
totalReviewers,
totalSubmissions,
totalSubmittedReviews,
progressPercentage,
calculatedAt: new Date().toISOString(),
};

this.logger.log(
`Review progress calculated: ${progressPercentage}% for challenge ${challengeId}`,
);
return result;
} catch (error) {
this.logger.error(
`Error calculating review progress for challenge ${challengeId}:`,
error,
);

if (error.message === 'Invalid challengeId parameter') {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error handling here could be improved by using specific exception classes instead of re-throwing a generic Error. For example, use BadRequestException or NotFoundException directly when catching specific errors.

throw new Error('Invalid challengeId parameter');
}

// Handle Resource API errors based on HTTP status codes
if (error.message === 'Cannot get data from Resource API.') {
const statusCode = (error as Error & { statusCode?: number })
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider checking if statusCode is undefined before comparing it to avoid potential runtime errors if statusCode is not present in the error object.

.statusCode;
if (statusCode === 400) {
throw new BadRequestException({
message: `Challenge ID ${challengeId} is not in valid GUID format`,
code: 'INVALID_CHALLENGE_ID',
});
} else if (statusCode === 404) {
throw new NotFoundException({
message: `Challenge with ID ${challengeId} was not found`,
code: 'CHALLENGE_NOT_FOUND',
});
}
}

if (error.message && error.message.includes('not found')) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message check error.message.includes('not found') might be too broad and could potentially catch unintended errors. Consider refining the condition or using a more specific error handling mechanism.

throw new NotFoundException({
message: `Challenge with ID ${challengeId} was not found or has no data available`,
code: 'CHALLENGE_NOT_FOUND',
});
}

throw new InternalServerErrorException({
message: 'Failed to calculate review progress',
code: 'PROGRESS_CALCULATION_ERROR',
});
}
}
}
45 changes: 45 additions & 0 deletions src/dto/review.dto.ts
Original file line number Diff line number Diff line change
Expand Up @@ -418,3 +418,48 @@ export function mapReviewItemRequestToDto(request: ReviewItemRequestDto) {
},
};
}

export class ReviewProgressResponseDto {
@ApiProperty({
description: 'The ID of the challenge',
example: 'challenge123',
})
@IsString()
@IsNotEmpty()
challengeId: string;

@ApiProperty({
description: 'Total number of reviewers for the challenge',
example: 2,
})
@IsNumber()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a validation decorator such as @IsInt() to ensure that totalReviewers is an integer, as fractional reviewers do not make sense.

totalReviewers: number;

@ApiProperty({
description: 'Total number of submissions for the challenge',
example: 4,
})
@IsNumber()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a validation decorator such as @IsInt() to ensure that totalSubmissions is an integer, as fractional submissions do not make sense.

totalSubmissions: number;

@ApiProperty({
description: 'Total number of submitted reviews',
example: 6,
})
@IsNumber()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a validation decorator such as @IsInt() to ensure that totalSubmittedReviews is an integer, as fractional reviews do not make sense.

totalSubmittedReviews: number;

@ApiProperty({
description: 'Review progress percentage',
example: 75.0,
})
@IsNumber()
progressPercentage: number;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a validation decorator such as @IsPositive() to ensure that progressPercentage is a positive number, as negative progress does not make sense.


@ApiProperty({
description: 'Timestamp when the progress was calculated',
example: '2025-01-15T10:30:00Z',
})
@IsDateString()
calculatedAt: string;
}
5 changes: 4 additions & 1 deletion src/shared/modules/global/resource.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,10 @@ export class ResourceApiService {
} catch (e) {
if (e instanceof AxiosError) {
this.logger.error(`Http Error: ${e.message}`, e.response?.data);
throw new Error('Cannot get data from Resource API.');
const error = new Error('Cannot get data from Resource API.');
(error as any).statusCode = e.response?.status;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using a more specific type instead of any for the error object to improve type safety and maintainability.

(error as any).originalMessage = e.response?.data?.message;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using a more specific type instead of any for the error object to improve type safety and maintainability.

throw error;
}
this.logger.error(`Data validation error: ${e}`);
throw new Error('Malformed data returned from Resource API');
Expand Down