Server/feat/class api#31
Conversation
…allocation; improve response formatting and examples
…and improve formatting
…ry DTO and update class response formatting
- Added `deletedAt` field to Class model and related types in Prisma schema. - Updated Class service to support soft deletion of classes. - Modified Classes controller to include archive endpoint for soft delete. - Adjusted queries to filter out soft-deleted classes. - Updated migration script to add `deletedAt` column and adjust foreign keys.
📝 WalkthroughWalkthroughThis PR revises class relationships and foreign-key actions, adds soft-delete and permanent-delete class flows, updates class response and request DTOs, introduces shared entity DTOs, and refreshes Swagger examples and documentation. ChangesClass lifecycle and API contracts
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ClassesController
participant ClassesService
participant Database
Client->>ClassesController: Request class archive or permanent deletion
ClassesController->>ClassesService: Invoke archive(id) or delete(id)
ClassesService->>Database: Update deletedAt or remove related records and class
Database-->>ClassesService: Return persistence result
ClassesService-->>ClassesController: Return operation message
ClassesController-->>Client: Return API response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 11
🧹 Nitpick comments (3)
server/src/modules/classes/dto/create-class-response.dto.ts (2)
31-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueValidation decorators on a response DTO are unnecessary.
@IsArray()and@IsOptional()are request-validation decorators fromclass-validator. SinceCreateClassResponseis a response projection (the service constructs it, not the client), these decorators add no runtime benefit and can mislead readers into thinking this DTO validates incoming payloads. Consider removing them and theclass-validatorimport.♻️ Proposed refactor
-import { ApiProperty, ApiPropertyOptional, PickType } from '`@nestjs/swagger`'; -import { IsArray, IsOptional } from 'class-validator'; +import { ApiProperty, ApiPropertyOptional, PickType } from '`@nestjs/swagger`'; import { StudentDto } from '../../students/dto/student.dto'; import { SubjectDto } from '../../subjects/dto/subject.dto'; import { TeacherDto } from '../../teachers/dto/teacher.dto'; @@ `@ApiPropertyOptional`({ type: [StudentSummaryDto] }) - `@IsArray`() - `@IsOptional`() students?: StudentSummaryDto[]; `@ApiPropertyOptional`({ type: [TeachingAllocationRefDto] }) - `@IsArray`() - `@IsOptional`() teachingAllocations?: TeachingAllocationRefDto[];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/modules/classes/dto/create-class-response.dto.ts` around lines 31 - 39, Remove the class-validator decorators from the students and teachingAllocations properties in CreateClassResponseDto, and remove the now-unused class-validator import. Keep the ApiPropertyOptional decorators and property types unchanged.
14-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove commented-out dead code.
The commented-out
idfield inTeachingAllocationRefDtoadds noise. If the field is not needed, remove it entirely; if it is needed, uncomment and implement it properly.🧹 Proposed cleanup
class TeachingAllocationRefDto { - // `@ApiProperty`({ type: String }) - - // id!: string; - `@ApiProperty`({ type: TeacherSummaryDto }) teacher!: TeacherSummaryDto;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/modules/classes/dto/create-class-response.dto.ts` around lines 14 - 15, Remove the commented-out id property and its ApiProperty annotation from TeachingAllocationRefDto unless the DTO requires this field; if required, restore both as active, properly typed declarations.server/src/modules/teaching-allocations/dto/teaching-allocation.dto.ts (1)
4-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate summary DTOs across files violate DRY.
SubjectSummaryDto,ClassSummaryDto, andTeacherSummaryDtoare defined here withidandnameproperties, whilecreate-class-response.dto.tsdefinesStudentSummaryDto,TeacherSummaryDto, andSubjectSummaryDtousingPickType. These represent the same domain entities but with different shapes (one hasname, the other only hasid). Consider extracting shared summary DTOs to a common location to prevent divergence.♻️ Proposed approach
Extract summary DTOs to a shared file, e.g.
server/src/common/dto/summary-dtos.ts:import { ApiProperty } from '`@nestjs/swagger`'; export class SubjectSummaryDto { `@ApiProperty`({ type: String, example: 'subjectid123' }) id!: string; `@ApiProperty`({ type: String, example: 'English' }) name!: string; } export class ClassSummaryDto { `@ApiProperty`({ type: String, example: 'classid123' }) id!: string; `@ApiProperty`({ type: String, example: 'Grade 10' }) name!: string; } export class TeacherSummaryDto { `@ApiProperty`({ type: String, example: 'teacherid123' }) id!: string; `@ApiProperty`({ type: String, example: 'Mrs Alice' }) name!: string; }Then import and reuse in both
teaching-allocation.dto.tsandcreate-class-response.dto.ts, usingPickTypewhere onlyidis needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/modules/teaching-allocations/dto/teaching-allocation.dto.ts` around lines 4 - 24, Extract the shared SubjectSummaryDto, ClassSummaryDto, and TeacherSummaryDto definitions into a common DTO module, then update teaching-allocation.dto.ts and create-class-response.dto.ts to import and reuse them. Preserve the existing full id/name shapes and continue using PickType in create-class-response.dto.ts where only id is exposed, removing the duplicate local declarations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/prisma/migrations/20260712084700_init/migration.sql`:
- Around line 14-20: Update the foreign-key definitions for
time_tables.teachingAllocationId, attendances.enrollmentId, and
exams.teachingAllocationId in the migration so their ON DELETE behavior remains
cascading, matching schema.prisma and the preceding cascade migration; leave the
ON UPDATE behavior unchanged.
In `@server/prisma/schema.prisma`:
- Line 179: Update the class model’s name uniqueness constraint near deletedAt
so archived records no longer block active classes with the same name. Replace
the global @@unique([name]) constraint with the project’s supported active-row
partial unique index approach, preserving uniqueness among rows where deletedAt
is null and the existing create/update behavior.
In `@server/src/modules/classes/classes.controller.ts`:
- Around line 74-96: Update the classesService.delete flow used by the
controller’s delete method to verify the class is already archived before
permanently deleting it. Enforce a deletedAt or equivalent archived-state
precondition, and reject active classes without removing the class or related
data; preserve the existing permanent-delete behavior for archived records.
In `@server/src/modules/classes/classes.service.ts`:
- Around line 113-116: Update the class creation flow around the returned
CreateClassResponse to validate academicYearId before writing and persist a
class-year association for that year. Ensure the created class is discoverable
by findAll() and reject unknown academic-year IDs instead of returning the
unpersisted association.
- Around line 247-251: Update findOne() around the effectiveAcademicYearId
lookup to throw NotFoundException when an explicitly supplied academicYearId
does not resolve through prisma.academicYear.findUnique. Preserve the existing
null behavior when the parameter is not supplied, and continue using the
resolved academic year for valid IDs.
In `@server/src/modules/classes/dto/create-class.dto.ts`:
- Around line 31-34: Align the DTO and service behavior for academicYearId:
either add `@IsOptional`() to academicYearId in the create-class DTO to preserve
active-year auto-resolution, or remove the nullish fallback from the classes
service and require the validated field directly. Keep validation and service
semantics consistent.
In `@server/src/modules/exams/dto/create-exam.dto.ts`:
- Around line 49-52: Update ExamsService.create to persist the required
CreateExamDto.academicYearId value by restoring the academicYearId mapping in
the created exam data; keep the DTO field and its validation unchanged.
In `@server/src/modules/results/dto/create-result.dto.ts`:
- Around line 29-37: Restore the create flow by uncommenting and implementing
ResultsService.create, ensuring it accepts and persists the DTO’s required
academicYearId and enrollmentId fields so the create-result endpoint functions
at runtime; otherwise remove the endpoint and its exposed route until
implementation is available.
In `@server/src/modules/students/dto/student-response.dto.ts`:
- Around line 17-20: Update the Swagger metadata for the
StudentResponseDto.class property to use ClassDto instead of EnrollmentDto,
matching its declared ClassDto | null type; remove the unused EnrollmentDto
definition if no other references require it.
In `@server/src/modules/students/dto/student.dto.ts`:
- Around line 23-24: Update the `@ApiProperty` decorator for the gender field in
the student DTO to document UserGender as a string rather than a boolean,
preferably using enum: UserGender so the OpenAPI schema exposes the valid enum
values. Keep the field type and existing API example aligned with UserGender.
In `@server/src/modules/teachers/dto/teacher.dto.ts`:
- Around line 23-24: Update the ApiProperty metadata for the gender field in the
teacher DTO to declare a string or UserGender enum schema instead of Boolean,
while preserving the existing UserGender typing and example value.
---
Nitpick comments:
In `@server/src/modules/classes/dto/create-class-response.dto.ts`:
- Around line 31-39: Remove the class-validator decorators from the students and
teachingAllocations properties in CreateClassResponseDto, and remove the
now-unused class-validator import. Keep the ApiPropertyOptional decorators and
property types unchanged.
- Around line 14-15: Remove the commented-out id property and its ApiProperty
annotation from TeachingAllocationRefDto unless the DTO requires this field; if
required, restore both as active, properly typed declarations.
In `@server/src/modules/teaching-allocations/dto/teaching-allocation.dto.ts`:
- Around line 4-24: Extract the shared SubjectSummaryDto, ClassSummaryDto, and
TeacherSummaryDto definitions into a common DTO module, then update
teaching-allocation.dto.ts and create-class-response.dto.ts to import and reuse
them. Preserve the existing full id/name shapes and continue using PickType in
create-class-response.dto.ts where only id is exposed, removing the duplicate
local declarations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b329bc8-ea04-488c-aad6-9dca0d345cb0
⛔ Files ignored due to path filters (4)
server/prisma/generated/prisma/internal/class.tsis excluded by!**/generated/**server/prisma/generated/prisma/internal/prismaNamespace.tsis excluded by!**/generated/**server/prisma/generated/prisma/internal/prismaNamespaceBrowser.tsis excluded by!**/generated/**server/prisma/generated/prisma/models/Class.tsis excluded by!**/generated/**
📒 Files selected for processing (34)
flow.txtserver/prisma/migrations/20260712073901_init/migration.sqlserver/prisma/migrations/20260712084700_init/migration.sqlserver/prisma/schema.prismaserver/src/common/formatters/class.formatter.tsserver/src/modules/academic-years/academic-years.controller.tsserver/src/modules/academic-years/dto/academic-year.dto.tsserver/src/modules/academic-years/dto/create-academic-year.dto.tsserver/src/modules/attendances/dto/attendance-response.dto.tsserver/src/modules/attendances/dto/create-attendance.dto.tsserver/src/modules/attendances/dto/query-attendance-dto.tsserver/src/modules/classes/classes.controller.tsserver/src/modules/classes/classes.service.tsserver/src/modules/classes/dto/class-response-dto.tsserver/src/modules/classes/dto/class.dto.tsserver/src/modules/classes/dto/create-class-response.dto.tsserver/src/modules/classes/dto/create-class.dto.tsserver/src/modules/classes/dto/update-class.dto.tsserver/src/modules/enrollments/dto/create-enrollment.dto.tsserver/src/modules/enrollments/dto/enrollment-response.dto.tsserver/src/modules/enrollments/dto/enrollment.dto.tsserver/src/modules/enrollments/dto/query-enrollment.dto.tsserver/src/modules/exams/dto/create-exam.dto.tsserver/src/modules/results/dto/create-result.dto.tsserver/src/modules/results/dto/query-result-dto.tsserver/src/modules/results/dto/result-response.dto.tsserver/src/modules/students/dto/student-response.dto.tsserver/src/modules/students/dto/student.dto.tsserver/src/modules/subjects/dto/subject.dto.tsserver/src/modules/teachers/dto/teacher.dto.tsserver/src/modules/teaching-allocations/dto/create-teaching-allocation.dto.tsserver/src/modules/teaching-allocations/dto/query-teaching-allocation.dto.tsserver/src/modules/teaching-allocations/dto/teaching-allocation-response.dto.tsserver/src/modules/teaching-allocations/dto/teaching-allocation.dto.ts
| ALTER TABLE "time_tables" ADD CONSTRAINT "time_tables_teachingAllocationId_fkey" FOREIGN KEY ("teachingAllocationId") REFERENCES "teaching_assignments"("id") ON DELETE RESTRICT ON UPDATE CASCADE; | ||
|
|
||
| -- AddForeignKey | ||
| ALTER TABLE "attendances" ADD CONSTRAINT "attendances_enrollmentId_fkey" FOREIGN KEY ("enrollmentId") REFERENCES "enrollments"("id") ON DELETE RESTRICT ON UPDATE CASCADE; | ||
|
|
||
| -- AddForeignKey | ||
| ALTER TABLE "exams" ADD CONSTRAINT "exams_teachingAllocationId_fkey" FOREIGN KEY ("teachingAllocationId") REFERENCES "teaching_assignments"("id") ON DELETE RESTRICT ON UPDATE CASCADE; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Keep the final foreign keys cascading.
This later migration overrides the preceding cascade migration with RESTRICT, contrary to schema.prisma. A class with attendance, exams, or timetable entries cannot be permanently deleted: deleting its enrollment/allocation fails on the dependent row and rolls back the transaction.
Proposed fix
- ON DELETE RESTRICT ON UPDATE CASCADE;
+ ON DELETE CASCADE ON UPDATE CASCADE;📝 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.
| ALTER TABLE "time_tables" ADD CONSTRAINT "time_tables_teachingAllocationId_fkey" FOREIGN KEY ("teachingAllocationId") REFERENCES "teaching_assignments"("id") ON DELETE RESTRICT ON UPDATE CASCADE; | |
| -- AddForeignKey | |
| ALTER TABLE "attendances" ADD CONSTRAINT "attendances_enrollmentId_fkey" FOREIGN KEY ("enrollmentId") REFERENCES "enrollments"("id") ON DELETE RESTRICT ON UPDATE CASCADE; | |
| -- AddForeignKey | |
| ALTER TABLE "exams" ADD CONSTRAINT "exams_teachingAllocationId_fkey" FOREIGN KEY ("teachingAllocationId") REFERENCES "teaching_assignments"("id") ON DELETE RESTRICT ON UPDATE CASCADE; | |
| ALTER TABLE "time_tables" ADD CONSTRAINT "time_tables_teachingAllocationId_fkey" FOREIGN KEY ("teachingAllocationId") REFERENCES "teaching_assignments"("id") ON DELETE CASCADE ON UPDATE CASCADE; | |
| -- AddForeignKey | |
| ALTER TABLE "attendances" ADD CONSTRAINT "attendances_enrollmentId_fkey" FOREIGN KEY ("enrollmentId") REFERENCES "enrollments"("id") ON DELETE CASCADE ON UPDATE CASCADE; | |
| -- AddForeignKey | |
| ALTER TABLE "exams" ADD CONSTRAINT "exams_teachingAllocationId_fkey" FOREIGN KEY ("teachingAllocationId") REFERENCES "teaching_assignments"("id") ON DELETE CASCADE ON UPDATE CASCADE; |
🧰 Tools
🪛 Squawk (2.59.0)
[warning] 14-14: By default new constraints require a table scan and block writes to the table while that scan occurs. Use NOT VALID with a later VALIDATE CONSTRAINT call.
(constraint-missing-not-valid)
[warning] 14-14: Adding a foreign key constraint requires a table scan and a SHARE ROW EXCLUSIVE lock on both tables, which blocks writes to each table. Add NOT VALID to the constraint in one transaction and then VALIDATE the constraint in a separate transaction.
(adding-foreign-key-constraint)
[warning] 17-17: By default new constraints require a table scan and block writes to the table while that scan occurs. Use NOT VALID with a later VALIDATE CONSTRAINT call.
(constraint-missing-not-valid)
[warning] 17-17: Adding a foreign key constraint requires a table scan and a SHARE ROW EXCLUSIVE lock on both tables, which blocks writes to each table. Add NOT VALID to the constraint in one transaction and then VALIDATE the constraint in a separate transaction.
(adding-foreign-key-constraint)
[warning] 20-20: By default new constraints require a table scan and block writes to the table while that scan occurs. Use NOT VALID with a later VALIDATE CONSTRAINT call.
(constraint-missing-not-valid)
[warning] 20-20: Adding a foreign key constraint requires a table scan and a SHARE ROW EXCLUSIVE lock on both tables, which blocks writes to each table. Add NOT VALID to the constraint in one transaction and then VALIDATE the constraint in a separate transaction.
(adding-foreign-key-constraint)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/prisma/migrations/20260712084700_init/migration.sql` around lines 14 -
20, Update the foreign-key definitions for time_tables.teachingAllocationId,
attendances.enrollmentId, and exams.teachingAllocationId in the migration so
their ON DELETE behavior remains cascading, matching schema.prisma and the
preceding cascade migration; leave the ON UPDATE behavior unchanged.
| enrollments Enrollment[] | ||
| teachingAllocations TeachingAllocation[] | ||
| announcements Announcement[] | ||
| deletedAt DateTime? |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make class-name uniqueness compatible with soft deletion.
@@unique([name]) still includes archived rows, but create() and update() only search active rows. After archiving “Grade 1A,” creating or renaming to that name reaches Prisma’s unique constraint and fails instead of treating the archived record as inactive. Replace the global constraint with an active-row partial unique index (or explicitly restore the archived class).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/prisma/schema.prisma` at line 179, Update the class model’s name
uniqueness constraint near deletedAt so archived records no longer block active
classes with the same name. Replace the global @@unique([name]) constraint with
the project’s supported active-row partial unique index approach, preserving
uniqueness among rows where deletedAt is null and the existing create/update
behavior.
| @ApiOperation({ | ||
| summary: 'Archive a class (soft delete)', | ||
| description: 'This performs a soft delete, preserving all related data.', | ||
| }) | ||
| @ApiOkResponse({ | ||
| schema: { example: { message: 'Class archived successfully' } }, | ||
| }) | ||
| @Delete(':id/archive') | ||
| async archive(@Param('id') id: string): Promise<{ message: string }> { | ||
| return this.classesService.archive(id); | ||
| } | ||
|
|
||
| @ApiOperation({ | ||
| summary: 'Permanently delete a class (destructive)', | ||
| description: | ||
| 'Warning: This action is irreversible and will delete the class and all its associated data, including enrollments, attendance, exams, and timetables.', | ||
| }) | ||
| @ApiOkResponse({ | ||
| schema: { example: { message: 'Class deleted successfully' } }, | ||
| schema: { example: { message: 'Class permanently deleted successfully' } }, | ||
| }) | ||
| @Delete(':id') | ||
| async remove(@Param('id') id: string): Promise<{ message: string }> { | ||
| return this.classesService.remove(id); | ||
| async delete(@Param('id') id: string): Promise<{ message: string }> { | ||
| return this.classesService.delete(id); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check archive and delete method implementations in ClassesService.
ast-grep run --pattern $'async archive($_) {
$$$
}' --lang typescript server/src/modules/classes/classes.service.ts
ast-grep run --pattern $'async delete($_) {
$$$
}' --lang typescript server/src/modules/classes/classes.service.tsRepository: thureinhtet99/student-os
Length of output: 799
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the service file structure first.
ast-grep outline server/src/modules/classes/classes.service.ts --view expanded || true
# Find deletedAt / archive / delete usages in the classes module.
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' \
'\bdeletedAt\b|\barchive\(|\bdelete\(' server/src/modules/classes -S || true
# Show the relevant section of the service file with line numbers.
wc -l server/src/modules/classes/classes.service.ts
sed -n '1,260p' server/src/modules/classes/classes.service.ts | cat -nRepository: thureinhtet99/student-os
Length of output: 11640
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the archive/delete implementation with line numbers.
sed -n '360,430p' server/src/modules/classes/classes.service.ts | cat -n
# Check for any class-related delete/archive helpers or state checks elsewhere.
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' \
'deletedAt: null|NotFoundException|archive\(id: string\)|delete\(id: string\)' server/src/modules/classes -S || trueRepository: thureinhtet99/student-os
Length of output: 3672
Guard permanent delete against active classes. archive already rejects archived records, but delete still hard-deletes any existing class ID, including active classes and their related data. Add a deletedAt precondition or require an explicit archived state before deleting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/modules/classes/classes.controller.ts` around lines 74 - 96,
Update the classesService.delete flow used by the controller’s delete method to
verify the class is already archived before permanently deleting it. Enforce a
deletedAt or equivalent archived-state precondition, and reject active classes
without removing the class or related data; preserve the existing
permanent-delete behavior for archived records.
| const response: CreateClassResponse = { | ||
| name: classItem.name, | ||
| academicYearId, | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Persist and validate the returned academic-year association.
A valid create request containing only name and academicYearId writes no row tied to that year, yet returns it here. Such a class is immediately excluded by findAll() because that query requires an enrollment or teaching allocation for the year; an unknown academic-year ID can also succeed. Persist a class-year association (or require an association row) and verify the academic year before writing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/modules/classes/classes.service.ts` around lines 113 - 116, Update
the class creation flow around the returned CreateClassResponse to validate
academicYearId before writing and persist a class-year association for that
year. Ensure the created class is discoverable by findAll() and reject unknown
academic-year IDs instead of returning the unpersisted association.
| const academicYear = effectiveAcademicYearId | ||
| ? await this.prisma.academicYear.findUnique({ | ||
| where: { id: effectiveAcademicYearId }, | ||
| }) | ||
| : null; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject an unknown explicit academic year.
findAll() validates a supplied academicYearId, but findOne() converts an unknown ID into academicYear: undefined and returns 200 with empty nested data. Apply the same NotFoundException check when the caller explicitly supplies this query parameter.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/modules/classes/classes.service.ts` around lines 247 - 251, Update
findOne() around the effectiveAcademicYearId lookup to throw NotFoundException
when an explicitly supplied academicYearId does not resolve through
prisma.academicYear.findUnique. Preserve the existing null behavior when the
parameter is not supplied, and continue using the resolved academic year for
valid IDs.
| @ApiProperty({ example: 'academicyearid123' }) | ||
| @IsString() | ||
| @IsNotEmpty() | ||
| academicYearId!: string; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
academicYearId is required by the DTO but commented out in the service.
CreateExamDto.academicYearId has @IsNotEmpty(), so clients must provide it. However, ExamsService.create has // academicYearId: createExamDto.academicYearId commented out, meaning the value is silently discarded. Either remove the field from the DTO or uncomment the service line to persist it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/modules/exams/dto/create-exam.dto.ts` around lines 49 - 52, Update
ExamsService.create to persist the required CreateExamDto.academicYearId value
by restoring the academicYearId mapping in the created exam data; keep the DTO
field and its validation unchanged.
| @ApiProperty({ example: 'academicyearid123' }) | ||
| @IsString() | ||
| @IsNotEmpty() | ||
| academicYearId!: string; | ||
|
|
||
| @ApiProperty({ example: 'ckx123enrollmentid' }) | ||
| @ApiProperty({ example: 'enrollmentid123' }) | ||
| @IsString() | ||
| @IsNotEmpty() | ||
| enrollmentId!: string; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
ResultsService.create is fully commented out — the endpoint is non-functional.
The DTO requires academicYearId and enrollmentId with @IsNotEmpty(), but the service's create method is entirely commented out. Any request to create a result will fail at runtime. Uncomment and implement the service method, or remove the endpoint until it's ready.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/modules/results/dto/create-result.dto.ts` around lines 29 - 37,
Restore the create flow by uncommenting and implementing ResultsService.create,
ensuring it accepts and persists the DTO’s required academicYearId and
enrollmentId fields so the create-result endpoint functions at runtime;
otherwise remove the endpoint and its exposed route until implementation is
available.
| class EnrollmentDto { | ||
| @ApiProperty({ example: 'ckx123enrollmentid' }) | ||
| @ApiProperty({ example: 'enrollmentid123' }) | ||
| id!: string; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use ClassDto for the documented class property.
This newly defined EnrollmentDto is used by @ApiProperty({ type: EnrollmentDto }) at Lines [43-44], while the property is declared as ClassDto | null. Swagger clients will therefore receive an incomplete/incorrect schema for StudentResponseDto.class. Use ClassDto there, or rename the property/type if it is actually intended to represent an enrollment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/modules/students/dto/student-response.dto.ts` around lines 17 -
20, Update the Swagger metadata for the StudentResponseDto.class property to use
ClassDto instead of EnrollmentDto, matching its declared ClassDto | null type;
remove the unused EnrollmentDto definition if no other references require it.
| @ApiProperty({ type: Boolean, example: 'MALE' }) | ||
| gender!: UserGender; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix incorrect Swagger type for gender field.
@ApiProperty({ type: Boolean, ... }) is applied to a UserGender enum field. This will generate misleading OpenAPI documentation showing gender as a boolean. The type should be String (or use enum: UserGender) to match the actual UserGender type.
🐛 Proposed fix
- `@ApiProperty`({ type: Boolean, example: 'MALE' })
+ `@ApiProperty`({ type: String, example: 'MALE', enum: UserGender })
gender!: UserGender;📝 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.
| @ApiProperty({ type: Boolean, example: 'MALE' }) | |
| gender!: UserGender; | |
| `@ApiProperty`({ type: String, example: 'MALE', enum: UserGender }) | |
| gender!: UserGender; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/modules/students/dto/student.dto.ts` around lines 23 - 24, Update
the `@ApiProperty` decorator for the gender field in the student DTO to document
UserGender as a string rather than a boolean, preferably using enum: UserGender
so the OpenAPI schema exposes the valid enum values. Keep the field type and
existing API example aligned with UserGender.
| @ApiProperty({ type: Boolean, example: 'MALE' }) | ||
| gender!: UserGender; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
gender Swagger type is Boolean but the field is a UserGender string enum.
@ApiProperty({ type: Boolean, example: 'MALE' }) will generate an incorrect OpenAPI schema showing a boolean input for a gender field. The type should be String (or use the enum directly).
🔧 Proposed fix
- `@ApiProperty`({ type: Boolean, example: 'MALE' })
+ `@ApiProperty`({ type: String, example: 'MALE', enum: UserGender })
gender!: UserGender;📝 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.
| @ApiProperty({ type: Boolean, example: 'MALE' }) | |
| gender!: UserGender; | |
| `@ApiProperty`({ type: String, example: 'MALE', enum: UserGender }) | |
| gender!: UserGender; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/modules/teachers/dto/teacher.dto.ts` around lines 23 - 24, Update
the ApiProperty metadata for the gender field in the teacher DTO to declare a
string or UserGender enum schema instead of Boolean, while preserving the
existing UserGender typing and example value.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation