Changes for adding dynamic template and assign template to course #99
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds new certificate-related endpoints, DTOs, entities, and service logic for templates and course-template mappings. Expands routing, increases request body size limits, updates the certificate module to include new repositories, and implements CRUD-style flows and listings with filtering, pagination, and tenant validation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as CertificateController
participant Svc as CertificateService
participant Ext as External Template Service
participant DB as DB (Templates, CourseTemplate)
rect rgba(200,230,255,0.3)
note over Client,API: Create Template
Client->>API: POST /certificate/template {template,title,template_type?,userId} + TenantID
API->>Svc: createTemplate(template,userId,title,res,req)
Svc->>Ext: POST createTemplate(template,...)
Ext-->>Svc: templateId, type
Svc->>DB: saveTemplate(templateId, template_type/type, title, userId, req)
DB-->>Svc: upsert OK
Svc-->>API: result with template metadata
API-->>Client: 200 payload
end
rect rgba(220,255,220,0.3)
note over Client,API: Add Course-Template Mapping
Client->>API: POST /certificate/course-template {templateId,contextId,userId}
API->>Svc: addCourseTemplate(templateId,contextId,userId,res,req)
Svc->>DB: upsert course_templates by contextId
DB-->>Svc: OK
Svc-->>API: mapping result
API-->>Client: 200 payload
end
rect rgba(255,240,200,0.3)
note over Client,API: Get Templates (list)
Client->>API: POST /certificate/templates-list {filters,limit,offset}
API->>Svc: getTemplates(filters,res)
Svc->>DB: SELECT templates LEFT JOIN course_templates + filters + pagination
DB-->>Svc: rows
Svc-->>API: list + counts
API-->>Client: 200 payload
end
rect rgba(255,220,220,0.3)
note over Client,API: Get Template (detail)
Client->>API: POST /certificate/get-template {templateId}
API->>Svc: getTemplate(templateId,res)
Svc->>Ext: GET template by id
Ext-->>Svc: template data
Svc->>DB: fetch local template + course-template by templateId
DB-->>Svc: local data
Svc-->>API: merged response
API-->>Client: 200 payload
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/modules/certificate/certificate.contoller.ts (1)
1-11: Missing import for Request type.The
Requesttype is used in method parameters (lines 68, 116, 140) but is not imported fromexpress. This will cause TypeScript compilation errors.Apply this diff to add the missing import:
import { Controller, Post, Body, Res, Headers, Req, StreamableFile, BadRequestException, Put, } from '@nestjs/common'; import { ApiInternalServerErrorResponse, ApiBadRequestResponse, ApiNotFoundResponse, ApiOkResponse, } from '@nestjs/swagger'; -import { Response } from 'express'; +import { Request, Response } from 'express';
🧹 Nitpick comments (7)
src/modules/certificate/dto/add-course-template-dto.ts (1)
4-14: Consider using @IsUUID() for ID fields.If
templateId,contextId, anduserIdrepresent UUIDs (common for entity IDs), use@IsUUID()instead of@IsString()for stricter validation and better error messages.Apply this diff:
-import { IsNotEmpty, IsString } from 'class-validator'; +import { IsNotEmpty, IsUUID } from 'class-validator'; export class AddCourseTemplateDto { @IsNotEmpty() - @IsString() + @IsUUID() templateId: string; @IsNotEmpty() - @IsString() + @IsUUID() contextId: string; @IsNotEmpty() - @IsString() + @IsUUID() userId: string; }src/modules/certificate/dto/edit-template-dto.ts (1)
12-24: Consider consolidating type and template_type fields.Both
type(required) andtemplate_type(optional) use identical@IsIn(['certificate', 'badge'])validation. This duplication might be intentional for backward compatibility, but consider whether both fields are necessary.src/modules/certificate/dto/course-template-list-dto.ts (1)
13-17: Add maximum limit validation.The
limitfield lacks an upper bound, potentially allowing excessively large queries that could impact performance or enable DoS attacks.Apply this diff:
-import { IsOptional, IsString, IsNumber, Min } from 'class-validator'; +import { IsOptional, IsString, IsNumber, Min, Max } from 'class-validator'; @IsOptional() @Type(() => Number) @IsNumber() @Min(1) + @Max(100) limit?: number = 10;src/modules/certificate/certificate.contoller.ts (4)
161-161: Consider using GET instead of POST for retrieval operations.The endpoints
getTemplate,getTemplates, andgetCourseTemplateListuse POST but perform read-only operations. REST conventions recommend GET for retrieval operations.If you need to send complex filter/pagination parameters, consider:
- Using GET with query parameters
- Using GET with a request body (though non-standard, some frameworks support it)
- If POST is intentional due to large payloads, document this decision
Example refactor for
getTemplate:- @Post('get-template') + @Get('template/:templateId') async getTemplate( - @Body() getTemplateDto: GetTemplateDto, + @Param('templateId') templateId: string, @Res() response: Response, - @Headers() headers, + @TenantId() tenantId: string, ) { - const tenantId = headers['tenantid']; - if (!tenantId || !isUUID(tenantId)) { - throw new BadRequestException('Please add valid Tenant ID'); - } return this.certificateService.getTemplate( - getTemplateDto.templateId, + templateId, + tenantId, response, ); }For list endpoints with filters, GET with query parameters is still preferable:
- @Post('templates-list') + @Get('templates') async getTemplates( - @Body() getTemplatesDto: GetTemplatesDto, + @Query() getTemplatesDto: GetTemplatesDto, @Res() response: Response, - @Headers() headers, + @TenantId() tenantId: string, ) { return this.certificateService.getTemplates( getTemplatesDto, + tenantId, response, ); }Also applies to: 181-181, 198-198
117-117: Add type annotation for headers parameter.The
headersparameter lacks a type annotation, reducing type safety. Consider adding at leastanyor preferably a more specific type.async createTemplate( @Body() createTemplateDto: CreateTemplateDto, @Res() response: Response, @Req() request: Request, - @Headers() headers, + @Headers() headers: Record<string, string>, ) {Better approach: Use the custom decorator suggested earlier (
@TenantId()) to eliminate the need for the headers parameter entirely.Also applies to: 141-141, 165-165, 185-185, 202-202, 223-223
123-129: Pass entire DTOs to service methods for better encapsulation.Several endpoints extract individual fields from DTOs before passing them to service methods (
createTemplate,addCourseTemplate,getTemplate,editTemplate). This approach:
- Tightly couples the controller to service implementation details
- Makes it harder to add new DTO fields without updating multiple locations
- Is inconsistent with
getTemplatesandgetCourseTemplateListwhich correctly pass entire DTOsRefactor service methods to accept DTOs directly:
async createTemplate( @Body() createTemplateDto: CreateTemplateDto, @Res() response: Response, @Req() request: Request, + @TenantId() tenantId: string, ) { return this.certificateService.createTemplate( - createTemplateDto.template, - createTemplateDto.userId, - createTemplateDto.title, + createTemplateDto, + tenantId, response, request, ); }Then update the service method signature:
// In certificate.service.ts async createTemplate( createTemplateDto: CreateTemplateDto, tenantId: string, response: Response, request: Request, ) { // Access fields via createTemplateDto.template, createTemplateDto.userId, etc. }Also applies to: 147-153, 171-174, 229-236
121-121: Improve error message specificity.The error message "Please add valid Tenant ID" doesn't distinguish between:
- Missing tenant ID header
- Invalid UUID format
Consider a more specific message:
const tenantId = headers['tenantid']; if (!tenantId || !isUUID(tenantId)) { - throw new BadRequestException('Please add valid Tenant ID'); + throw new BadRequestException( + !tenantId + ? 'Missing required header: tenantid' + : 'Invalid tenantid header: must be a valid UUID' + ); }Or if using the suggested custom decorator/guard, improve the error message there.
Also applies to: 145-145, 169-169, 189-189, 206-206, 227-227
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (14)
src/constant/routeConfig.js(2 hunks)src/main.ts(2 hunks)src/modules/certificate/certificate.contoller.ts(3 hunks)src/modules/certificate/certificate.module.ts(1 hunks)src/modules/certificate/certificate.service.ts(3 hunks)src/modules/certificate/dto/add-course-template-dto.ts(1 hunks)src/modules/certificate/dto/course-template-list-dto.ts(1 hunks)src/modules/certificate/dto/create-template-dto.ts(1 hunks)src/modules/certificate/dto/edit-template-dto.ts(1 hunks)src/modules/certificate/dto/get-template-dto.ts(1 hunks)src/modules/certificate/dto/get-templates-dto.ts(1 hunks)src/modules/certificate/dto/get-user-certificates-dto.ts(1 hunks)src/modules/certificate/entities/course_templates.ts(1 hunks)src/modules/certificate/entities/templates.ts(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
src/constant/routeConfig.js
[error] 457-457: Expected a statement but instead found ','.
Expected a statement here.
(parse)
[error] 459-459: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 466-466: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 469-469: Expected a statement but instead found ']'.
Expected a statement here.
(parse)
[error] 470-470: Expected a statement but instead found ','.
Expected a statement here.
(parse)
[error] 472-472: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 479-479: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 482-482: Expected a statement but instead found ']'.
Expected a statement here.
(parse)
[error] 483-483: Expected a statement but instead found ','.
Expected a statement here.
(parse)
[error] 485-485: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 492-492: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 495-495: Expected a statement but instead found ']'.
Expected a statement here.
(parse)
[error] 496-496: Expected a statement but instead found ','.
Expected a statement here.
(parse)
[error] 498-498: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 505-505: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 508-508: Expected a statement but instead found ']'.
Expected a statement here.
(parse)
[error] 509-509: Expected a statement but instead found ','.
Expected a statement here.
(parse)
[error] 511-511: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 518-518: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 521-521: Expected a statement but instead found ']'.
Expected a statement here.
(parse)
🔇 Additional comments (3)
src/modules/certificate/certificate.module.ts (1)
8-18: LGTM!The entity imports and TypeOrmModule registration follow standard NestJS/TypeORM patterns.
src/constant/routeConfig.js (1)
458-522: LGTM!The new certificate template routes follow the existing pattern and structure consistently. The static analysis warnings appear to be false positives - the JSON structure is valid.
src/modules/certificate/dto/get-template-dto.ts (1)
4-6: Leave @IsString() for templateId—thetemplateIdcolumn inTemplatesis defined asvarchar(not a UUID type), so switching to@IsUUID()would reject any non-UUID strings.
| import { SwaggerModule, DocumentBuilder } from '@nestjs/swagger'; | ||
| import { AppModule } from './app.module'; | ||
| import { ValidationPipe, BadRequestException } from '@nestjs/common'; | ||
| import { json, urlencoded } from 'express'; |
There was a problem hiding this comment.
Remove unused imports.
The json and urlencoded imports from express are no longer used after introducing body-parser middleware.
Apply this diff:
-import { json, urlencoded } from 'express';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { json, urlencoded } from 'express'; |
🤖 Prompt for AI Agents
In src/main.ts around line 5, the imports "json" and "urlencoded" from 'express'
are unused after switching to body-parser; remove these unused imports by
deleting them from the import statement (or replace the import with only the
remaining used identifiers) so the file no longer imports json and urlencoded.
| app.use(bodyParser.json({ limit: '500mb' })); | ||
| app.use(bodyParser.urlencoded({ limit: '500mb', extended: true })); |
There was a problem hiding this comment.
Reduce the payload size limit and remove duplicate middleware.
The 500mb limit is excessive and poses security risks (DoS attacks, resource exhaustion). Additionally, lines 44-45 register express's built-in middleware inside the listen callback, which is ineffective (middleware registered after listen is never invoked) and redundant with the body-parser middleware here.
Apply this diff to set a reasonable limit and remove the ineffective duplicate:
- app.use(bodyParser.json({ limit: '500mb' }));
- app.use(bodyParser.urlencoded({ limit: '500mb', extended: true }));
+ app.use(bodyParser.json({ limit: '10mb' }));
+ app.use(bodyParser.urlencoded({ limit: '10mb', extended: true }));Then remove lines 44-45:
await app.listen(3000, () => {
console.log(`Server is running on port 3000`);
- app.use(json({ limit: '30mb' }));
- app.use(urlencoded({ extended: true, limit: '30mb' }));
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| app.use(bodyParser.json({ limit: '500mb' })); | |
| app.use(bodyParser.urlencoded({ limit: '500mb', extended: true })); | |
| // register body-parser with a safer payload limit | |
| app.use(bodyParser.json({ limit: '10mb' })); | |
| app.use(bodyParser.urlencoded({ limit: '10mb', extended: true })); | |
| // start the server (remove redundant middleware from here) | |
| await app.listen(3000, () => { | |
| console.log(`Server is running on port 3000`); | |
| }); |
🤖 Prompt for AI Agents
In src/main.ts around lines 25-26, the app is using bodyParser with an excessive
500mb limit and duplicate middleware is later registered ineffectively inside
the listen callback (lines 44-45); change the bodyParser limits to a reasonable
value such as '10mb' for both json and urlencoded, and remove the duplicate
express.json()/express.urlencoded() registrations at lines 44-45 (delete those
lines so middleware is only registered once before listen).
| const tenantId = headers['tenantid']; | ||
| if (!tenantId || !isUUID(tenantId)) { | ||
| throw new BadRequestException('Please add valid Tenant ID'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicated tenant validation logic.
The tenant validation logic is repeated identically across all six new endpoints. This violates the DRY principle and makes the code harder to maintain.
Consider extracting this logic into:
- A custom guard (preferred for NestJS):
// tenant-validation.guard.ts
import { Injectable, CanActivate, ExecutionContext, BadRequestException } from '@nestjs/common';
import { isUUID } from 'class-validator';
@Injectable()
export class TenantValidationGuard implements CanActivate {
canActivate(context: ExecutionContext): boolean {
const request = context.switchToHttp().getRequest();
const tenantId = request.headers['tenantid'];
if (!tenantId || !isUUID(tenantId)) {
throw new BadRequestException('Please add valid Tenant ID');
}
// Optionally attach to request for later use
request.tenantId = tenantId;
return true;
}
}Then apply the guard to endpoints:
+ @UseGuards(TenantValidationGuard)
@Post('template')
async createTemplate(
@Body() createTemplateDto: CreateTemplateDto,
@Res() response: Response,
@Req() request: Request,
- @Headers() headers,
) {
- const tenantId = headers['tenantid'];
- if (!tenantId || !isUUID(tenantId)) {
- throw new BadRequestException('Please add valid Tenant ID');
- }
return this.certificateService.createTemplate(
createTemplateDto.template,
createTemplateDto.userId,
createTemplateDto.title,
+ request.tenantId,
response,
request,
);
}- Or a custom decorator for simpler extraction:
// tenant-id.decorator.ts
import { createParamDecorator, ExecutionContext, BadRequestException } from '@nestjs/common';
import { isUUID } from 'class-validator';
export const TenantId = createParamDecorator(
(data: unknown, ctx: ExecutionContext) => {
const request = ctx.switchToHttp().getRequest();
const tenantId = request.headers['tenantid'];
if (!tenantId || !isUUID(tenantId)) {
throw new BadRequestException('Please add valid Tenant ID');
}
return tenantId;
},
);Usage:
@Post('template')
async createTemplate(
@Body() createTemplateDto: CreateTemplateDto,
@Res() response: Response,
@Req() request: Request,
- @Headers() headers,
+ @TenantId() tenantId: string,
) {
- const tenantId = headers['tenantid'];
- if (!tenantId || !isUUID(tenantId)) {
- throw new BadRequestException('Please add valid Tenant ID');
- }
return this.certificateService.createTemplate(
createTemplateDto.template,
createTemplateDto.userId,
createTemplateDto.title,
+ tenantId,
response,
request,
);
}Also applies to: 143-146, 167-170, 187-190, 204-207, 225-228
| const tenantId = headers['tenantid']; | ||
| if (!tenantId || !isUUID(tenantId)) { | ||
| throw new BadRequestException('Please add valid Tenant ID'); | ||
| } | ||
| return this.certificateService.createTemplate( | ||
| createTemplateDto.template, | ||
| createTemplateDto.userId, | ||
| createTemplateDto.title, | ||
| response, | ||
| request, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
Tenant ID validated but not used in service call.
The tenant ID is validated for format but never passed to certificateService.createTemplate(). This means:
- The validation only checks UUID format, not actual tenant context
- Multi-tenancy is not enforced at the service layer
- Templates might not be properly isolated by tenant
This pattern repeats across all new endpoints and could lead to data leakage or unauthorized access across tenants.
Please verify whether tenant-based isolation is implemented in the service layer:
🏁 Script executed:
#!/bin/bash
# Check if service methods accept tenantId parameter
ast-grep --pattern $'createTemplate($$$) {
$$$
}'
# Check for tenant-related filtering in service implementations
rg -nP -A10 'createTemplate\(' src/modules/certificate/certificate.service.tsLength of output: 1053
Enforce tenant isolation by passing tenantId
Controller validates tenantId but doesn’t forward it; service uses a static SCHEMA_ID. Update certificateService.createTemplate (signature and implementation in src/modules/certificate/certificate.service.ts) to accept tenantId and use it for tenant-specific schema operations instead of the config value.
🤖 Prompt for AI Agents
In src/modules/certificate/certificate.contoller.ts around lines 119-129, the
controller validates tenantId but does not forward it to the service; update the
call to this.certificateService.createTemplate(...) to pass the validated
tenantId as an additional argument. Then update the signature and implementation
in src/modules/certificate/certificate.service.ts (and any call sites) to accept
tenantId and use it for schema/DB operations (replace usage of static
CONFIG.SCHEMA_ID/SCHEMA constant with the passed tenantId) so all
tenant-specific operations use the provided tenantId instead of the config
value.
| let userCertQuery = this.userCourseCertificateRepository | ||
| .createQueryBuilder('uc') | ||
| .select([ | ||
| 'uc.usercertificateId', | ||
| 'uc.userId', | ||
| 'uc.contextId', | ||
| 'uc.contextType', | ||
| 'uc.tenantId', | ||
| 'uc.certificateId', | ||
| 'uc.status', | ||
| 'uc.issuedOn', | ||
| 'uc.createdOn', | ||
| 'uc.updatedOn', | ||
| 'uc.createdBy', | ||
| 'uc.updatedBy', | ||
| 'uc.certificate_type', | ||
| ]); | ||
|
|
||
| // Apply filters only if they are provided | ||
| if (userIds && userIds.length > 0) { | ||
| userCertQuery = userCertQuery.andWhere('uc.userId IN (:...userIds)', { | ||
| userIds, | ||
| }); | ||
| } | ||
|
|
||
| if (certificateIds && certificateIds.length > 0) { | ||
| userCertQuery = userCertQuery.andWhere( | ||
| 'uc.certificateId IN (:...certificateIds)', | ||
| { certificateIds }, | ||
| ); | ||
| } | ||
|
|
||
| if (courseIds && courseIds.length > 0) { | ||
| userCertQuery = userCertQuery.andWhere( | ||
| 'uc.contextId IN (:...courseIds)', | ||
| { courseIds }, | ||
| ); | ||
| } | ||
|
|
||
| if (templateIds && templateIds.length > 0) { | ||
| userCertQuery = userCertQuery.andWhere( | ||
| 'ct.templateId IN (:...templateIds)', | ||
| { templateIds }, | ||
| ); | ||
| } | ||
|
|
||
| if (certificateTypes && certificateTypes.length > 0) { | ||
| userCertQuery = userCertQuery.andWhere( | ||
| 'uc.certificate_type IN (:...certificateTypes)', | ||
| { certificateTypes }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fix missing join before filtering by template IDs
When templateIds is provided we add AND ct.templateId IN (...), but the query never joins course_templates. Any such request currently crashes with “missing FROM-clause entry for table ‘ct’”. Join the table before referencing ct so the filter works.
- if (templateIds && templateIds.length > 0) {
- userCertQuery = userCertQuery.andWhere(
- 'ct.templateId IN (:...templateIds)',
- { templateIds },
- );
- }
+ if (templateIds && templateIds.length > 0) {
+ userCertQuery = userCertQuery
+ .leftJoin('course_templates', 'ct', 'ct.contextId = uc.contextId')
+ .andWhere('ct.templateId IN (:...templateIds)', { templateIds });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let userCertQuery = this.userCourseCertificateRepository | |
| .createQueryBuilder('uc') | |
| .select([ | |
| 'uc.usercertificateId', | |
| 'uc.userId', | |
| 'uc.contextId', | |
| 'uc.contextType', | |
| 'uc.tenantId', | |
| 'uc.certificateId', | |
| 'uc.status', | |
| 'uc.issuedOn', | |
| 'uc.createdOn', | |
| 'uc.updatedOn', | |
| 'uc.createdBy', | |
| 'uc.updatedBy', | |
| 'uc.certificate_type', | |
| ]); | |
| // Apply filters only if they are provided | |
| if (userIds && userIds.length > 0) { | |
| userCertQuery = userCertQuery.andWhere('uc.userId IN (:...userIds)', { | |
| userIds, | |
| }); | |
| } | |
| if (certificateIds && certificateIds.length > 0) { | |
| userCertQuery = userCertQuery.andWhere( | |
| 'uc.certificateId IN (:...certificateIds)', | |
| { certificateIds }, | |
| ); | |
| } | |
| if (courseIds && courseIds.length > 0) { | |
| userCertQuery = userCertQuery.andWhere( | |
| 'uc.contextId IN (:...courseIds)', | |
| { courseIds }, | |
| ); | |
| } | |
| if (templateIds && templateIds.length > 0) { | |
| userCertQuery = userCertQuery.andWhere( | |
| 'ct.templateId IN (:...templateIds)', | |
| { templateIds }, | |
| ); | |
| } | |
| if (certificateTypes && certificateTypes.length > 0) { | |
| userCertQuery = userCertQuery.andWhere( | |
| 'uc.certificate_type IN (:...certificateTypes)', | |
| { certificateTypes }, | |
| ); | |
| } | |
| // Apply filters only if they are provided | |
| if (userIds && userIds.length > 0) { | |
| userCertQuery = userCertQuery.andWhere('uc.userId IN (:...userIds)', { | |
| userIds, | |
| }); | |
| } | |
| if (certificateIds && certificateIds.length > 0) { | |
| userCertQuery = userCertQuery.andWhere( | |
| 'uc.certificateId IN (:...certificateIds)', | |
| { certificateIds }, | |
| ); | |
| } | |
| if (courseIds && courseIds.length > 0) { | |
| userCertQuery = userCertQuery.andWhere( | |
| 'uc.contextId IN (:...courseIds)', | |
| { courseIds }, | |
| ); | |
| } | |
| if (templateIds && templateIds.length > 0) { | |
| userCertQuery = userCertQuery | |
| .leftJoin('course_templates', 'ct', 'ct.contextId = uc.contextId') | |
| .andWhere('ct.templateId IN (:...templateIds)', { templateIds }); | |
| } | |
| if (certificateTypes && certificateTypes.length > 0) { | |
| userCertQuery = userCertQuery.andWhere( | |
| 'uc.certificate_type IN (:...certificateTypes)', | |
| { certificateTypes }, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In src/modules/certificate/certificate.service.ts around lines 533 to 584, the
query applies a filter on ct.templateId without joining the course_templates
table first, causing a "missing FROM-clause entry for table 'ct'"; fix by adding
a join to course_templates with alias ct (joining ct.courseId to uc.contextId or
the appropriate FK) before applying the templateIds AND condition, and scope
that join to run only when templateIds are provided (use an inner join since
you’re filtering by it).
| @@ -0,0 +1,24 @@ | |||
| import { IsOptional, IsString, IsNumber, Min, Max } from 'class-validator'; | |||
There was a problem hiding this comment.
Remove unused import.
The Max import is unused.
Apply this diff:
-import { IsOptional, IsString, IsNumber, Min, Max } from 'class-validator';
+import { IsOptional, IsString, IsNumber, Min } from 'class-validator';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { IsOptional, IsString, IsNumber, Min, Max } from 'class-validator'; | |
| import { IsOptional, IsString, IsNumber, Min } from 'class-validator'; |
🤖 Prompt for AI Agents
In src/modules/certificate/dto/course-template-list-dto.ts around line 1, the
imported symbol `Max` from 'class-validator' is not used; remove `Max` from the
import list so only IsOptional, IsString, IsNumber, and Min are imported to
clean up unused imports and avoid linting errors.
| @@ -0,0 +1,18 @@ | |||
| import { IsString, IsNotEmpty, IsOptional, isString } from 'class-validator'; | |||
There was a problem hiding this comment.
Remove unused import.
The isString import (lowercase) is unused.
Apply this diff:
-import { IsString, IsNotEmpty, IsOptional, isString } from 'class-validator';
+import { IsString, IsNotEmpty, IsOptional } from 'class-validator';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { IsString, IsNotEmpty, IsOptional, isString } from 'class-validator'; | |
| import { IsString, IsNotEmpty, IsOptional } from 'class-validator'; |
🤖 Prompt for AI Agents
In src/modules/certificate/dto/create-template-dto.ts around line 1, remove the
unused lowercase isString import from the class-validator import list; update
the import statement to only include IsString, IsNotEmpty, and IsOptional (and
run lint/compile to ensure no other usages rely on isString).
| @IsString() | ||
| userId: string; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add @isnotempty() to userId.
The userId field lacks @IsNotEmpty() validation. Since it's a required field without @IsOptional(), it should validate for non-empty values.
Apply this diff:
+ @IsNotEmpty()
@IsString()
userId: string;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @IsString() | |
| userId: string; | |
| @IsNotEmpty() | |
| @IsString() | |
| userId: string; |
🤖 Prompt for AI Agents
In src/modules/certificate/dto/create-template-dto.ts around lines 16 to 17, the
userId property has @IsString() but is missing @IsNotEmpty(); add the
@IsNotEmpty() decorator above the userId field to enforce non-empty validation
(since the field is required and not optional), ensuring class-validator will
reject empty strings.
| @ApiProperty({ | ||
| description: 'Page number for pagination', | ||
| example: 1, | ||
| required: false, | ||
| default: 1 | ||
| }) | ||
| @IsOptional() | ||
| @IsNumber() | ||
| @Min(1) | ||
| page?: number = 1; | ||
|
|
||
| @ApiProperty({ | ||
| description: 'Number of records per page', | ||
| example: 10, | ||
| required: false, | ||
| default: 10, | ||
| minimum: 1, | ||
| maximum: 100 | ||
| }) | ||
| @IsOptional() | ||
| @IsNumber() | ||
| @Min(1) | ||
| @Max(100) | ||
| limit?: number = 10; |
There was a problem hiding this comment.
Add numeric transformation before validation
@IsNumber() runs on plain query-string values (e.g., '1', '10'). Without @Type(() => Number) (or implicit conversion), these validations fail and every request with page/limit query params is rejected. Please add the transformation decorator so the DTO accepts standard HTTP query values.
@IsOptional()
+ @Type(() => Number)
@IsNumber()
@Min(1)
page?: number = 1;
@@
})
@IsOptional()
+ @Type(() => Number)
@IsNumber()
@Min(1)
@Max(100)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/modules/certificate/dto/get-user-certificates-dto.ts around lines 5 to
28, the DTO applies @IsNumber() to query params but doesn't convert string query
values to numbers; add @Type(() => Number) from class-transformer above both
page and limit properties (before validation decorators) so '1'/'10' are
transformed to numbers, and update imports to include Type from
'class-transformer' if missing.
1 participant
No description provided.