Server/feat/student api#34
Conversation
…s, and TeachingAllocation models
- Enhanced CreateUserDto with strong password validation and updated property examples. - Modified CreateStudentDto to include academicYearId and classId, while removing unnecessary properties. - Updated student formatting to include additional fields such as phone, address, and dateOfBirth. - Adjusted student response DTO to reflect new properties and ensure optional fields are correctly defined. - Refactored student service to utilize active academic year context and streamline student creation logic. - Improved seeding scripts for students and teachers to incorporate academic year handling. - Updated tests for students and teachers to align with new DTO structures and service logic.
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughStudent contracts, service flows, response formatting, academic-year handling, seeders, account identifiers, parent-module removal, alignment guidance, schema comments, and mobile student creation APIs are updated. ChangesStudent module alignment
Mobile student creation API
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant StudentsService
participant AcademicYearContextService
participant PrismaService
Client->>StudentsService: Submit student payload
StudentsService->>AcademicYearContextService: Resolve active academic year when needed
StudentsService->>PrismaService: Create student, enrollment, and parent records
PrismaService-->>StudentsService: Return created relations
StudentsService-->>Client: Return formatted student response
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/src/modules/students/students.controller.ts (1)
35-77: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd missing Swagger error response,
@ApiParam, and@ApiQuerydecorators.Per coding guidelines, the controller must use the complete Swagger decorator set including error
@ApiResponsedecorators and@ApiParam/@ApiQuerywhere applicable. Currently missing:
@ApiNotFoundResponseonfindOne,update,remove(service throwsNotFoundException)@ApiBadRequestResponseoncreate,update(validation decorators on DTOs)@ApiUnauthorizedResponse/@ApiForbiddenResponsewhere applicable@ApiParam({ name: 'id', ... })onfindOne,update,remove@ApiQueryforclassId,gender,search,page,limitonfindAllAs per coding guidelines: "Use the complete Swagger decorator set, including
@ApiOperation, success and relevant error@ApiResponsedecorators, and@ApiParamor@ApiQuerywhere applicable."🤖 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/students.controller.ts` around lines 35 - 77, Add the missing Swagger metadata in the students controller: apply ApiBadRequestResponse to create and update, ApiNotFoundResponse to findOne, update, and remove, and the applicable ApiUnauthorizedResponse and ApiForbiddenResponse decorators. Add ApiParam describing the id route parameter on findOne, update, and remove, and add ApiQuery decorators for classId, gender, search, page, and limit on findAll, preserving the existing operations and success responses.Source: Coding guidelines
server/src/modules/students/students.service.ts (1)
332-332: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
ParentRelationship.GUARDIANinstead of string literal.
updateusesrelationship: 'GUARDIAN'(line 332) whilecreateusesParentRelationship.GUARDIAN(line 124). Use the enum consistently to avoid breakage if the value changes.♻️ Proposed fix
- relationship: 'GUARDIAN', + relationship: ParentRelationship.GUARDIAN,🤖 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/students.service.ts` at line 332, Update the relationship assignment in the students service update flow to use ParentRelationship.GUARDIAN instead of the 'GUARDIAN' string literal, matching the create flow and preserving the existing relationship value.
🧹 Nitpick comments (5)
server/src/common/dto/create-user.dto.ts (1)
87-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove commented-out
rolefield.The commented-out
roleproperty is dead code. SinceCreateUserDtono longer exposesroleand services set it internally, this scaffolding should be deleted.♻️ Proposed fix
- // `@ApiProperty`({ - // type: String, - // example: 'STUDENT', - // }) - // `@IsNotEmpty`() - // `@IsEnum`(UserRole) - // role!: UserRole; }🤖 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/common/dto/create-user.dto.ts` around lines 87 - 93, Remove the commented-out role property block, including its ApiProperty, IsNotEmpty, and IsEnum annotations, from CreateUserDto; leave the active DTO fields unchanged.server/src/database/seeders/seed-students.ts (1)
12-13: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
getActiveId()is called outside the try/catch in both seeders, and theif (!effectiveAcademicYearId)guard is dead code.AcademicYearContextService.getActiveId()returnsPromise<string>and throwsNotFoundExceptionon failure — it never returns a falsy value, so the null guard is unreachable. Moving the call inside the try block ensures errors are caught and logged consistently.
server/src/database/seeders/seed-students.ts#L12-L13,L23-L26: MovegetActiveId()inside the try block and remove the deadif (!effectiveAcademicYearId)guard.server/src/database/seeders/seed-teachers.ts#L12-L13,L23-L26: Apply the same change.🤖 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/database/seeders/seed-students.ts` around lines 12 - 13, Move the getActiveId() call into the try block in both seedStudents and seedTeachers, then remove the unreachable !effectiveAcademicYearId guards while preserving the existing seeding and error-logging flow. Apply this in server/src/database/seeders/seed-students.ts at lines 12-13 and 23-26, and server/src/database/seeders/seed-teachers.ts at lines 12-13 and 23-26.server/src/modules/students/students.service.ts (2)
216-216: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd return type annotation to
findOne.All other methods declare explicit return types (
Promise<StudentResponseDto>,Promise<PaginatedResponseDto<...>>).findOnelacks one.♻️ Proposed fix
- async findOne(id: string) { + async findOne(id: string): Promise<StudentResponseDto> {🤖 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/students.service.ts` at line 216, Add an explicit Promise-based return type annotation to the findOne method, matching the StudentResponseDto return type used by the service’s other methods.
190-202: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftUse Prisma
selectinstead ofincludefor read operations.
findAll(lines 190-202),findOne(lines 219-231), and the final fetch inupdate(lines 372-384) useincludewith nestedselectfor relations. Per coding guidelines, reads should useselectrecursively so returned data directly matches the response DTO shape and avoids over-fetching scalar fields.As per coding guidelines: "Use Prisma
selectrecursively for reads instead ofinclude, so returned data directly matches the response DTO shape."♻️ Example refactor for `findAll`
const students = await this.prisma.student.findMany({ where, skip: (page - 1) * limit, take: limit, orderBy: { user: { name: 'asc' } }, - include: { - user: true, - parents: { - include: { - parent: { select: { id: true } }, - }, - }, - enrollments: { - include: { - class: { select: { id: true } }, - }, - }, + select: { + id: true, + studentNumber: true, + phone: true, + address: true, + gender: true, + dateOfBirth: true, + userId: true, + user: { select: { id: true, name: true, email: true, image: true, setPasswordToken: true, setPasswordTokenExpires: true, resetPasswordToken: true, resetPasswordTokenExpires: true, lastLoginAt: true } }, + parents: { select: { parent: { select: { id: true } } } }, + enrollments: { select: { class: { select: { id: true } } } }, }, });Also applies to: 219-231, 372-384
🤖 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/students.service.ts` around lines 190 - 202, Replace the Prisma include trees in findAll, findOne, and the final fetch within update with recursive select objects. Select only the scalar fields and nested relation fields required by the response DTO, including the existing user, parents.parent.id, and enrollments.class.id data, and remove the corresponding include usage.Source: Coding guidelines
server/src/modules/students/students.controller.spec.ts (1)
11-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReset mock call history between tests.
mockStudentsServicemethods accumulate calls across tests withoutjest.clearAllMocks()inbeforeEach. Not an immediate issue withtoHaveBeenCalledWith, but will cause false positives iftoHaveBeenCalledTimesassertions are added later.♻️ Proposed fix
beforeEach(async () => { + jest.clearAllMocks(); const module: TestingModule = await Test.createTestingModule({🤖 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/students.controller.spec.ts` around lines 11 - 17, Reset mock call history before each test by adding jest.clearAllMocks() to the existing beforeEach setup in the student controller spec, covering all methods on mockStudentsService without changing their implementations.
🤖 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 `@mobile/services/student-service.ts`:
- Around line 42-46: Update createStudent to serialize payload into a JSON
string before passing it as the body to apiRequest, ensuring the POST request
supplies a valid fetch BodyInit while preserving the existing endpoint and
method.
- Line 24: Update createStudent to serialize its student-create payload as JSON
before passing it to apiRequest, or reuse an existing centralized serialization
path in apiRequest; ensure the request body is no longer sent as a plain object.
In `@server/AGENT.md`:
- Around line 142-150: Update the Deliverable instructions in server/AGENT.md to
remove the directive to delete the file after merge. Preserve the durable
repository guidance and retain the remaining PR requirements, including the
squashed PR and findings note.
In `@server/src/common/dto/create-user.dto.ts`:
- Around line 32-39: Update the Swagger example in the password property of
create-user.dto.ts to a value that satisfies `@IsStrongPassword`(), such as a
password containing uppercase and lowercase letters, numbers, and a symbol;
leave the validation decorators unchanged.
In `@server/src/common/formatters/student.formatter.ts`:
- Around line 17-18: Update the student formatter’s classId mapping to select
the enrollment associated with the active academic year before reading class.id,
rather than blindly using student.enrollments[0]. Reuse the existing
academic-year context or filtering convention, and preserve null when no
matching enrollment exists.
In `@server/src/database/seeders/seed-students.ts`:
- Line 11: Replace the request-scoped AcademicYearContextService resolution in
seed-students.ts:11 and seed-teachers.ts:11 with a seeding-safe approach: read
the current academic year directly through PrismaService, or explicitly provide
a synthetic request/context when invoking the seeders. Ensure both seeder paths
work under NestFactory.createApplicationContext(...) without an HTTP request.
In `@server/src/modules/students/dto/create-student.dto.ts`:
- Around line 19-22: Update the academicYearId property in CreateStudentDto to
be optional for validation and TypeScript construction, preserving its existing
string validation when provided. Keep the ApiPropertyOptional decorator and the
StudentsService.create fallback expression using createStudentDto.academicYearId
?? await this.academicYearContext.getActiveId().
In `@server/src/modules/students/dto/student-response.dto.ts`:
- Around line 67-97: Align the serialized response contract for
setPasswordTokenExpires, resetPasswordTokenExpires, and lastLoginAt with their
Swagger documentation by typing them as string | null and converting each Date
value to an ISO string in the student formatter, matching the existing
dateOfBirth handling. Preserve null values and update the DTO fields and
formatter mappings consistently.
In `@server/src/modules/students/students.controller.spec.ts`:
- Around line 37-76: Add rejection tests in the create, findAll, findOne,
update, and remove describe blocks that mock the corresponding
mockStudentsService method to reject with relevant NotFoundException or
ConflictException instances, then assert controller.create, findAll, findOne,
update, and remove propagate those exceptions with rejects.toThrow. Keep
academic-year fallback coverage in students.service.spec.ts.
In `@server/src/modules/students/students.service.spec.ts`:
- Around line 76-80: Update both createStudentDto objects in the relevant
student service tests to include a valid gender value matching UserGender; keep
the existing fields unchanged and rely on the DTO’s optional academicYearId for
the first test.
- Around line 37-41: Update the $transaction mock in the students service test
to avoid the callback’s any return type and satisfy
`@typescript-eslint/no-unsafe-return`. Use a type-safe callback/result signature
that preserves the mocked transaction behavior of invoking the callback with
prisma.
In `@server/src/modules/students/students.service.ts`:
- Around line 99-101: Update the create and update methods in StudentsService so
academicYearContext.getActiveId() is resolved only when classId is provided;
preserve an explicitly supplied createStudentDto.academicYearId without calling
getActiveId, and avoid any academic-year lookup when no enrollment data is being
processed.
- Line 304: Remove the commented-out dead code at
server/src/modules/students/students.service.ts:304,
server/src/modules/students/dto/student-response.dto.ts:40, and
server/src/modules/students/students.module.ts:11. Preserve behavior by
uncommenting the date-time format or StudentsService export only if their
current consumers require them; otherwise delete the comments.
In `@server/src/modules/teachers/dto/create-teacher.dto.ts`:
- Around line 14-17: Update the create-teacher DTO’s academicYearId property to
match TeachersService.create(): either wire the value into the create flow, or
remove it from the request contract; if retained without create-flow usage, make
it optional with `@IsOptional`() and an optional TypeScript property so clients
are not required to send a discarded value.
---
Outside diff comments:
In `@server/src/modules/students/students.controller.ts`:
- Around line 35-77: Add the missing Swagger metadata in the students
controller: apply ApiBadRequestResponse to create and update,
ApiNotFoundResponse to findOne, update, and remove, and the applicable
ApiUnauthorizedResponse and ApiForbiddenResponse decorators. Add ApiParam
describing the id route parameter on findOne, update, and remove, and add
ApiQuery decorators for classId, gender, search, page, and limit on findAll,
preserving the existing operations and success responses.
In `@server/src/modules/students/students.service.ts`:
- Line 332: Update the relationship assignment in the students service update
flow to use ParentRelationship.GUARDIAN instead of the 'GUARDIAN' string
literal, matching the create flow and preserving the existing relationship
value.
---
Nitpick comments:
In `@server/src/common/dto/create-user.dto.ts`:
- Around line 87-93: Remove the commented-out role property block, including its
ApiProperty, IsNotEmpty, and IsEnum annotations, from CreateUserDto; leave the
active DTO fields unchanged.
In `@server/src/database/seeders/seed-students.ts`:
- Around line 12-13: Move the getActiveId() call into the try block in both
seedStudents and seedTeachers, then remove the unreachable
!effectiveAcademicYearId guards while preserving the existing seeding and
error-logging flow. Apply this in server/src/database/seeders/seed-students.ts
at lines 12-13 and 23-26, and server/src/database/seeders/seed-teachers.ts at
lines 12-13 and 23-26.
In `@server/src/modules/students/students.controller.spec.ts`:
- Around line 11-17: Reset mock call history before each test by adding
jest.clearAllMocks() to the existing beforeEach setup in the student controller
spec, covering all methods on mockStudentsService without changing their
implementations.
In `@server/src/modules/students/students.service.ts`:
- Line 216: Add an explicit Promise-based return type annotation to the findOne
method, matching the StudentResponseDto return type used by the service’s other
methods.
- Around line 190-202: Replace the Prisma include trees in findAll, findOne, and
the final fetch within update with recursive select objects. Select only the
scalar fields and nested relation fields required by the response DTO, including
the existing user, parents.parent.id, and enrollments.class.id data, and remove
the corresponding include usage.
🪄 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: feb2c019-fd10-4d25-bfdd-3a81a3591545
📒 Files selected for processing (20)
mobile/services/student-service.tsserver/AGENT.mdserver/MODULE_ALIGNMENT.mdserver/prisma/schema.prismaserver/src/common/dto/create-user.dto.tsserver/src/common/formatters/student.formatter.tsserver/src/common/types/student.type.tsserver/src/database/seeders/seed-students.tsserver/src/database/seeders/seed-teachers.tsserver/src/modules/admins/admins.service.tsserver/src/modules/students/dto/create-student.dto.tsserver/src/modules/students/dto/query-student-dto.tsserver/src/modules/students/dto/student-response.dto.tsserver/src/modules/students/students.controller.spec.tsserver/src/modules/students/students.controller.tsserver/src/modules/students/students.module.tsserver/src/modules/students/students.service.spec.tsserver/src/modules/students/students.service.tsserver/src/modules/teachers/dto/create-teacher.dto.tsserver/src/modules/teachers/teachers.service.ts
💤 Files with no reviewable changes (2)
- server/MODULE_ALIGNMENT.md
- server/prisma/schema.prisma
| email: string; | ||
| password: string; | ||
| gender: StudentGender; | ||
| academicYearId: string; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and map their structure.
git ls-files 'mobile/services/student-service.ts' 'server/services/*student*' 'server/controllers/*student*' 'server/**/students*' 'mobile/**/api*' | sed -n '1,200p'
echo
echo '--- student-service.ts outline ---'
ast-grep outline mobile/services/student-service.ts --view expanded
echo
echo '--- nearby lines in mobile/services/student-service.ts ---'
sed -n '1,220p' mobile/services/student-service.ts
echo
echo '--- search for createStudent / academicYearId usage ---'
rg -n "createStudent|academicYearId|academicYearContext|getActiveId|RequestInit|body:" mobile server -SRepository: thureinhtet99/student-os
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- mobile/services/api.ts ---'
sed -n '1,220p' mobile/services/api.ts
echo
echo '--- server/src/modules/students/dto/create-student.dto.ts ---'
sed -n '1,220p' server/src/modules/students/dto/create-student.dto.ts
echo
echo '--- server/src/modules/students/students.service.ts (create path) ---'
sed -n '1,180p' server/src/modules/students/students.service.ts
echo
echo '--- server/src/modules/students/students.service.spec.ts (academicYearId cases) ---'
sed -n '70,180p' server/src/modules/students/students.service.spec.tsRepository: thureinhtet99/student-os
Length of output: 10287
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- students controller ---'
sed -n '1,220p' server/src/modules/students/students.controller.ts
echo
echo '--- server bootstrap / validation pipe search ---'
rg -n "ValidationPipe|app.useGlobalPipes|skipMissingProperties|whitelist|forbidNonWhitelisted" server/src -S
echo
echo '--- CreateStudentDto references / generated docs hints ---'
rg -n "CreateStudentDto|academicYearId" server/src/modules/students -SRepository: thureinhtet99/student-os
Length of output: 5622
Serialize the student-create payload before sending it.
createStudent passes a plain object as body, but fetch expects serialized JSON here. Stringify the payload before calling apiRequest (or handle serialization centrally in apiRequest).
🤖 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 `@mobile/services/student-service.ts` at line 24, Update createStudent to
serialize its student-create payload as JSON before passing it to apiRequest, or
reuse an existing centralized serialization path in apiRequest; ensure the
request body is no longer sent as a plain object.
| export async function createStudent(payload: CreateStudentPayload): Promise<Student> { | ||
| return apiRequest<Student>('/students', { | ||
| method: 'POST', | ||
| body: payload, | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Serialize the POST body before calling fetch.
apiRequest forwards body directly to fetch, but this passes a plain object instead of a JSON BodyInit. The request can fail TypeScript validation or arrive at the server as invalid JSON.
return apiRequest<Student>('/students', {
method: 'POST',
- body: payload,
+ body: JSON.stringify(payload),
});📝 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.
| export async function createStudent(payload: CreateStudentPayload): Promise<Student> { | |
| return apiRequest<Student>('/students', { | |
| method: 'POST', | |
| body: payload, | |
| }); | |
| export async function createStudent(payload: CreateStudentPayload): Promise<Student> { | |
| return apiRequest<Student>('/students', { | |
| method: 'POST', | |
| body: JSON.stringify(payload), | |
| }); | |
| } |
🤖 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 `@mobile/services/student-service.ts` around lines 42 - 46, Update
createStudent to serialize payload into a JSON string before passing it as the
body to apiRequest, ensuring the POST request supplies a valid fetch BodyInit
while preserving the existing endpoint and method.
| ## Deliverable | ||
|
|
||
| Once complete on the current branch, open a squashed PR targeting the | ||
| `development` branch that brings `students` to parity with | ||
| `academic-years`/`classes`, plus a short findings note (in the PR | ||
| description) for anything you deliberately left unchanged because it needed | ||
| a schema decision rather than a pure refactor. | ||
|
|
||
| Once merged, delete this file. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Do not delete server/AGENT.md after merge.
Removing this file would discard persistent repository guidance needed for future students-module changes. Keep it as durable guidance, or move only the temporary task checklist into the PR description.
🤖 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/AGENT.md` around lines 142 - 150, Update the Deliverable instructions
in server/AGENT.md to remove the directive to delete the file after merge.
Preserve the durable repository guidance and retain the remaining PR
requirements, including the squashed PR and findings note.
| @ApiProperty({ | ||
| type: String, | ||
| example: 'password123', | ||
| }) | ||
| @IsString() | ||
| @IsStrongPassword() | ||
| @IsNotEmpty() | ||
| password!: string; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File context ==\n'
git ls-files server/src/common/dto/create-user.dto.ts
wc -l server/src/common/dto/create-user.dto.ts
cat -n server/src/common/dto/create-user.dto.ts | sed -n '1,120p'
printf '\n== class-validator version refs ==\n'
rg -n '"class-validator"|from '\''class-validator'\''|from \"class-validator\"' -S package.json server package-lock.json yarn.lock pnpm-lock.yaml 2>/dev/null || true
printf '\n== StrongPassword usages ==\n'
rg -n "`@IsStrongPassword`\(" server -S || true
printf '\n== Nearby DTO patterns ==\n'
rg -n "`@ApiProperty`\\(|example:" server/src/common/dto -S || trueRepository: thureinhtet99/student-os
Length of output: 5154
Use a strong-password-compliant example
password123 doesn't satisfy @IsStrongPassword(), so copying the Swagger sample will trigger a 400. Change it to something like Password123!.
🤖 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/common/dto/create-user.dto.ts` around lines 32 - 39, Update the
Swagger example in the password property of create-user.dto.ts to a value that
satisfies `@IsStrongPassword`(), such as a password containing uppercase and
lowercase letters, numbers, and a symbol; leave the validation decorators
unchanged.
| parentId: student.parents?.[0]?.parent?.id ?? null, | ||
| classId: student.enrollments?.[0]?.class?.id ?? null, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how reference modules (academic-years, classes) handle enrollment scoping.
rg -n 'academicYearId|enrollments' server/src/modules/academic-years/ server/src/modules/classes/ --type ts -C3Repository: thureinhtet99/student-os
Length of output: 22399
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the formatter and the student query paths that populate `student.enrollments`.
for f in \
server/src/common/formatters/student.formatter.ts \
server/src/modules/students/students.service.ts \
server/src/modules/students/students.controller.ts \
server/src/modules/students/dto/*.ts \
server/src/modules/enrollments/*.ts
do
[ -f "$f" ] && echo "### $f" && sed -n '1,260p' "$f" && echo
doneRepository: thureinhtet99/student-os
Length of output: 23373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether student queries ever scope enrollments by academic year or impose a stable order.
rg -n 'academicYearId|enrollments:\s*{|orderBy:.*enrollments|formatStudent\(' server/src/modules/students server/src/common -g '*.ts' -C3
# Inspect the Prisma model for enrollment ordering fields that might make `[0]` deterministic.
rg -n 'model Enrollment|createdAt|updatedAt|academicYearId|studentId|classId' prisma -g '*.prisma' -C3Repository: thureinhtet99/student-os
Length of output: 345
classId should be scoped to the active academic year
student.enrollments?.[0]?.class?.id reads the first enrollment from an unfiltered relation, so a student with multiple enrollments can return the wrong class. Scope enrollments to the intended academic year, or make this fallback explicit if the ambiguity is intentional.
🤖 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/common/formatters/student.formatter.ts` around lines 17 - 18,
Update the student formatter’s classId mapping to select the enrollment
associated with the active academic year before reading class.id, rather than
blindly using student.enrollments[0]. Reuse the existing academic-year context
or filtering convention, and preserve null when no matching enrollment exists.
| $transaction: jest | ||
| .fn() | ||
| .mockImplementation((callback: (tx: PrismaService) => any) => | ||
| callback(prisma), | ||
| ), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
ESLint no-unsafe-return — $transaction mock returns any.
The callback type (tx: PrismaService) => any causes callback(prisma) to return any, triggering @typescript-eslint/no-unsafe-return.
🐛 Proposed fix
$transaction: jest
.fn()
- .mockImplementation((callback: (tx: PrismaService) => any) =>
+ .mockImplementation((callback: (tx: PrismaService) => unknown) =>
callback(prisma),
),📝 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.
| $transaction: jest | |
| .fn() | |
| .mockImplementation((callback: (tx: PrismaService) => any) => | |
| callback(prisma), | |
| ), | |
| $transaction: jest | |
| .fn() | |
| .mockImplementation((callback: (tx: PrismaService) => unknown) => | |
| callback(prisma), | |
| ), |
🧰 Tools
🪛 GitHub Actions: Server CI / 1_Lint.txt
[error] 40-40: @typescript-eslint/no-unsafe-return: Unsafe return of a value of type any
🪛 GitHub Actions: Server CI / Lint
[error] 40-40: ESLint @typescript-eslint/no-unsafe-return: Unsafe return of a value of type any.
🪛 GitHub Check: Lint
[failure] 40-40:
Unsafe return of a value of type any
🤖 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/students.service.spec.ts` around lines 37 - 41,
Update the $transaction mock in the students service test to avoid the
callback’s any return type and satisfy `@typescript-eslint/no-unsafe-return`. Use
a type-safe callback/result signature that preserves the mocked transaction
behavior of invoking the callback with prisma.
Sources: Linters/SAST tools, Pipeline failures
| update: { | ||
| email: updateStudentDto.email?.trim(), | ||
| role: updateStudentDto.role, | ||
| // role: updateStudentDto.role, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove commented-out dead code across the students module. Per coding guidelines, dead code should be removed from the students module. Three commented-out lines remain in this PR:
server/src/modules/students/students.service.ts#L304: Delete// role: updateStudentDto.role.server/src/modules/students/dto/student-response.dto.ts#L40: Delete// format: 'date-time'— if date-time format is needed, uncomment it; otherwise remove it.server/src/modules/students/students.module.ts#L11: Delete// exports: [StudentsService]— if the export is needed by the seeder, uncomment it; otherwise remove it.
As per coding guidelines: "Remove dead code, TODO scaffolding, and unused imports from the students module."
📍 Affects 3 files
server/src/modules/students/students.service.ts#L304-L304(this comment)server/src/modules/students/dto/student-response.dto.ts#L40-L40server/src/modules/students/students.module.ts#L11-L11
🤖 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/students.service.ts` at line 304, Remove the
commented-out dead code at server/src/modules/students/students.service.ts:304,
server/src/modules/students/dto/student-response.dto.ts:40, and
server/src/modules/students/students.module.ts:11. Preserve behavior by
uncommenting the date-time format or StudentsService export only if their
current consumers require them; otherwise delete the comments.
Source: Coding guidelines
| @ApiPropertyOptional({ type: String, example: 'academicYearId' }) | ||
| @IsString() | ||
| @IsNotEmpty() | ||
| academicYearId!: string; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- file map ---'
git ls-files 'server/src/modules/teachers/**' | sort
printf '\n%s\n' '--- outline create-teacher.dto.ts ---'
ast-grep outline server/src/modules/teachers/dto/create-teacher.dto.ts --view expanded || true
printf '\n%s\n' '--- outline teachers.service.ts ---'
ast-grep outline server/src/modules/teachers/teachers.service.ts --view expanded || true
printf '\n%s\n' '--- relevant lines in create-teacher.dto.ts ---'
cat -n server/src/modules/teachers/dto/create-teacher.dto.ts
printf '\n%s\n' '--- relevant lines in teachers.service.ts ---'
cat -n server/src/modules/teachers/teachers.service.tsRepository: thureinhtet99/student-os
Length of output: 11933
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- search academicYearId ---'
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' --glob '!coverage' 'academicYearId' .
printf '\n%s\n' '--- teachers controller create route ---'
cat -n server/src/modules/teachers/teachers.controller.tsRepository: thureinhtet99/student-os
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- AGENT conventions ---'
sed -n '1,120p' server/AGENT.md
printf '\n%s\n' '--- class create DTO pattern ---'
cat -n server/src/modules/classes/dto/create-class.dto.ts
printf '\n%s\n' '--- teacher controller create endpoint ---'
cat -n server/src/modules/teachers/teachers.controller.tsRepository: thureinhtet99/student-os
Length of output: 10241
Make academicYearId optional or remove it from teacher creation.
TeachersService.create() never uses this field, so the DTO currently forces clients to send a value that gets dropped, while Swagger already marks it optional. Keep it only if you wire it into the create flow; otherwise make it @IsOptional()/?: string or remove it from the request contract.
🤖 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/create-teacher.dto.ts` around lines 14 - 17,
Update the create-teacher DTO’s academicYearId property to match
TeachersService.create(): either wire the value into the create flow, or remove
it from the request contract; if retained without create-flow usage, make it
optional with `@IsOptional`() and an optional TypeScript property so clients are
not required to send a discarded value.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/modules/students/students.service.spec.ts (1)
82-87: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winFix TS2741 pipeline failure by casting to
CreateStudentDto.The pipeline is failing because
academicYearIdis required byCreateStudentDtobut omitted here to test the fallback behavior. Cast the object to bypass the type error. Additionally, use theUserGenderenum instead of a raw string literal to match the DTO types perfectly.🐛 Proposed fix
- const createStudentDto: CreateStudentDto = { + const createStudentDto = { name: 'Test Student', email: 'test@student.com', password: 'password123', - gender: 'MALE', - }; + gender: UserGender.MALE, + } as CreateStudentDto;🤖 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/students.service.spec.ts` around lines 82 - 87, Update the createStudentDto fixture in the relevant student service spec to cast the object as CreateStudentDto while intentionally omitting academicYearId for the fallback test, and replace the raw gender string with the UserGender enum value.Source: Pipeline failures
♻️ Duplicate comments (1)
server/src/modules/students/students.service.spec.ts (1)
44-46: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winESLint
no-unsafe-return—$transactionmock returnsany.The callback type
(tx: PrismaService) => anycausescallback(prisma)to returnany, triggering@typescript-eslint/no-unsafe-return.🐛 Proposed fix
$transaction: jest .fn() - .mockImplementation((callback: (tx: PrismaService) => any) => + .mockImplementation((callback: (tx: PrismaService) => unknown) => callback(prisma), ),🤖 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/students.service.spec.ts` around lines 44 - 46, Update the $transaction mock implementation to avoid the explicitly unsafe any return type: type the callback with the appropriate transaction callback return type (or infer it from the mocked method) and preserve the callback(prisma) return behavior without triggering `@typescript-eslint/no-unsafe-return`.
🤖 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.
Outside diff comments:
In `@server/src/modules/students/students.service.spec.ts`:
- Around line 82-87: Update the createStudentDto fixture in the relevant student
service spec to cast the object as CreateStudentDto while intentionally omitting
academicYearId for the fallback test, and replace the raw gender string with the
UserGender enum value.
---
Duplicate comments:
In `@server/src/modules/students/students.service.spec.ts`:
- Around line 44-46: Update the $transaction mock implementation to avoid the
explicitly unsafe any return type: type the callback with the appropriate
transaction callback return type (or infer it from the mocked method) and
preserve the callback(prisma) return behavior without triggering
`@typescript-eslint/no-unsafe-return`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99866b46-34e1-450e-aacb-34391d12b4d4
📒 Files selected for processing (9)
server/src/common/formatters/student.formatter.tsserver/src/common/formatters/user.formatter.tsserver/src/modules/academic-years/academic-years.service.spec.tsserver/src/modules/students/dto/create-student.dto.tsserver/src/modules/students/dto/student-response.dto.tsserver/src/modules/students/dto/update-student.dto.tsserver/src/modules/students/students.service.spec.tsserver/src/modules/students/students.service.tsserver/src/modules/teachers/teachers.service.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- server/src/common/formatters/student.formatter.ts
- server/src/modules/students/students.service.ts
- server/src/modules/students/dto/create-student.dto.ts
- server/src/modules/students/dto/student-response.dto.ts
… parent handling in DTOs and services
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/src/modules/students/dto/student-response.dto.ts (1)
43-49: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLeftover commented-out
format: 'date-time'scaffolding.As per coding guidelines: "Remove dead code, leftover scaffolding TODOs, and unused imports from the students module." This commented line was flagged before and remains unaddressed.
🧹 Proposed fix
`@ApiPropertyOptional`({ example: '2005-08-24T00:00:00.000Z', type: String, - // format: 'date-time', nullable: true, })🤖 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 43 - 49, Remove the commented-out format property from the ApiPropertyOptional decorator for dateOfBirth, leaving the active example, type, and nullable metadata unchanged.Source: Coding guidelines
server/src/modules/students/students.service.ts (1)
255-259: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
classselect omitsnameinfindOneandupdate, so the response always drops the class name.Both queries use
class: { select: { id: true } }, butstudent.formatter.tsreads.class.nameto build the response. Sincenameisn't selected,class.nameis alwaysundefined(dropped entirely on JSON serialization →class: {}), unlikefindAll, which correctly selects{ id: true, name: true }.🐛 Proposed fix
- class: { select: { id: true } }, + class: { select: { id: true, name: true } },(apply to both
findOne's include andupdate's finalfindUniqueOrThrowinclude)Also applies to: 441-456
🤖 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/students.service.ts` around lines 255 - 259, Update the enrollment class selections in both the findOne query and the update method’s final findUniqueOrThrow query to select both id and name. Keep the existing include structure and align these selections with the findAll query so student.formatter.ts receives class.name.
♻️ Duplicate comments (2)
server/src/modules/students/dto/create-student.dto.ts (1)
40-43: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
academicYearIdstill required — fallback in service remains dead code.Unresolved from previous review:
@IsNotEmpty()with no@IsOptional()makes this mandatory at validation time, socreateStudentDto.academicYearId ?? await this.academicYearContext.getActiveId()instudents.service.ts(line 118) can never reach the fallback. As per coding guidelines,academicYearIdmust resolve asdto.academicYearId ?? await this.academicYearContext.getActiveId(), which requires the DTO field to be optional.🐛 Proposed fix
`@ApiPropertyOptional`({ type: String, example: 'academicYearId' }) `@IsString`() + `@IsOptional`() - `@IsNotEmpty`() - academicYearId!: string; + academicYearId?: string;🤖 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/create-student.dto.ts` around lines 40 - 43, Make academicYearId optional in the create-student DTO by adding the appropriate optional validation decorator alongside `@IsString`() and `@IsNotEmpty`(). Preserve the existing students service resolution so academicYearId continues to use the DTO value or fall back to academicYearContext.getActiveId().Source: Coding guidelines
server/src/modules/students/dto/student-response.dto.ts (1)
92-98: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Date | nullfields still don't match the serialized (string) output.Unresolved:
setPasswordTokenExpires,resetPasswordTokenExpires, andlastLoginAtare typedDate | nulland documented withtype: String, butstudent.formatter.tsreturns the rawDateobjects for these (unlikedateOfBirth, which is converted via.toISOString()). Serialized JSON will contain date strings while the TS contract saysDate.♻️ Proposed fix
- setPasswordTokenExpires?: Date | null; + setPasswordTokenExpires?: string | null;- resetPasswordTokenExpires?: Date | null; + resetPasswordTokenExpires?: string | null;- lastLoginAt?: Date | null; + lastLoginAt?: string | null;Corresponding
student.formatter.tschanges needed (see companion comment there).Also applies to: 108-114, 116-122
🤖 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 92 - 98, Update the Student response DTO fields setPasswordTokenExpires, resetPasswordTokenExpires, and lastLoginAt to use the serialized string-or-null type matching their String API documentation. Update the corresponding student.formatter.ts mapping so each Date value is converted to an ISO string while preserving null values, consistent with dateOfBirth.
🤖 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/src/common/formatters/student.formatter.ts`:
- Around line 26-30: Update the student formatter’s class mapping to select the
enrollment matching the active/target academic year before reading its class
name, rather than always using enrollments[0]. Reuse the same year-selection
logic or value already used elsewhere in the formatter, and preserve the null
result when no matching enrollment or class exists.
In `@server/src/modules/students/dto/student-response.dto.ts`:
- Around line 73-82: Update the ApiPropertyOptional decorator for the class
field in the student response DTO to use Swagger type Object instead of String,
matching its actual { name: string } | null shape and the parent field’s
configuration.
In `@server/src/modules/students/dto/update-student.dto.ts`:
- Around line 6-19: Flag the missing parent relationship selector in
UpdateStudentDto as a product/schema decision rather than modifying this DTO.
Coordinate with students.service.ts update, which currently assumes GUARDIAN,
and do not implement implicit many-to-many or relationship changes in this pass.
In `@server/src/modules/students/students.service.ts`:
- Around line 158-170: Replace the top-level include-based Prisma reads in the
students service methods create, findAll, findOne, and update with recursive
select projections matching StudentResponseDto. Define the required nested
selections for user, parents.parent, enrollments.class, and all response fields,
removing broad include usage while preserving the existing query behavior and
returned DTO shape.
- Around line 331-333: Update the parent lookup and subsequent create, clear,
and update handling in the students service’s update flow around
existingParentStudent to avoid assuming ParentRelationship.GUARDIAN when
relationships may be FATHER or MOTHER. Flag this implicit many-to-many
relationship ambiguity for a schema/product decision, and coordinate the change
with update-student.dto.ts because it lacks a relationship selector; do not
apply an ad hoc relationship-specific patch.
---
Outside diff comments:
In `@server/src/modules/students/dto/student-response.dto.ts`:
- Around line 43-49: Remove the commented-out format property from the
ApiPropertyOptional decorator for dateOfBirth, leaving the active example, type,
and nullable metadata unchanged.
In `@server/src/modules/students/students.service.ts`:
- Around line 255-259: Update the enrollment class selections in both the
findOne query and the update method’s final findUniqueOrThrow query to select
both id and name. Keep the existing include structure and align these selections
with the findAll query so student.formatter.ts receives class.name.
---
Duplicate comments:
In `@server/src/modules/students/dto/create-student.dto.ts`:
- Around line 40-43: Make academicYearId optional in the create-student DTO by
adding the appropriate optional validation decorator alongside `@IsString`() and
`@IsNotEmpty`(). Preserve the existing students service resolution so
academicYearId continues to use the DTO value or fall back to
academicYearContext.getActiveId().
In `@server/src/modules/students/dto/student-response.dto.ts`:
- Around line 92-98: Update the Student response DTO fields
setPasswordTokenExpires, resetPasswordTokenExpires, and lastLoginAt to use the
serialized string-or-null type matching their String API documentation. Update
the corresponding student.formatter.ts mapping so each Date value is converted
to an ISO string while preserving null values, consistent with dateOfBirth.
🪄 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: 129826b6-e35b-40b2-8425-323f9dad7136
📒 Files selected for processing (18)
server/src/app.module.tsserver/src/common/formatters/parent.formatter.tsserver/src/common/formatters/student.formatter.tsserver/src/database/seeders/seed-students.tsserver/src/modules/parents/dto/create-parent.dto.tsserver/src/modules/parents/dto/parent-response.dto.tsserver/src/modules/parents/dto/query-parent-dto.tsserver/src/modules/parents/dto/update-parent.dto.tsserver/src/modules/parents/parents.controller.spec.tsserver/src/modules/parents/parents.controller.tsserver/src/modules/parents/parents.module.tsserver/src/modules/parents/parents.service.spec.tsserver/src/modules/parents/parents.service.tsserver/src/modules/students/dto/create-student.dto.tsserver/src/modules/students/dto/student-response.dto.tsserver/src/modules/students/dto/update-student.dto.tsserver/src/modules/students/students.service.spec.tsserver/src/modules/students/students.service.ts
💤 Files with no reviewable changes (11)
- server/src/modules/parents/parents.module.ts
- server/src/modules/parents/dto/update-parent.dto.ts
- server/src/modules/parents/parents.controller.spec.ts
- server/src/modules/parents/dto/create-parent.dto.ts
- server/src/modules/parents/parents.service.spec.ts
- server/src/modules/parents/dto/parent-response.dto.ts
- server/src/modules/parents/parents.controller.ts
- server/src/common/formatters/parent.formatter.ts
- server/src/modules/parents/parents.service.ts
- server/src/modules/parents/dto/query-parent-dto.ts
- server/src/app.module.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- server/src/database/seeders/seed-students.ts
- server/src/modules/students/students.service.spec.ts
| class: student.enrollments?.[0]?.class | ||
| ? { | ||
| id: student.enrollments[0].class.id, | ||
| name: student.enrollments[0].class.name, | ||
| name: student.enrollments?.[0].class.name, | ||
| } | ||
| : null, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
class still taken from unfiltered enrollments[0].
Unresolved from a previous review (previously about .id, now .name — same underlying defect): a student's enrollments relation isn't scoped to the active/target academic year, so a student with enrollments across multiple years returns an arbitrary 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/src/common/formatters/student.formatter.ts` around lines 26 - 30,
Update the student formatter’s class mapping to select the enrollment matching
the active/target academic year before reading its class name, rather than
always using enrollments[0]. Reuse the same year-selection logic or value
already used elsewhere in the formatter, and preserve the null result when no
matching enrollment or class exists.
| @ApiPropertyOptional({ | ||
| type: String, | ||
| example: { | ||
| name: 'Grade 10', | ||
| }, | ||
| nullable: true, | ||
| }) | ||
| class?: { | ||
| name: string; | ||
| } | null; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
class Swagger type says String but the actual shape is an object.
Same pattern as parent above (which correctly uses type: Object); class should too, since the value is { name: string } | null.
🐛 Proposed fix
`@ApiPropertyOptional`({
- type: String,
+ type: Object,
example: {
name: 'Grade 10',
},
nullable: true,
})📝 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.
| @ApiPropertyOptional({ | |
| type: String, | |
| example: { | |
| name: 'Grade 10', | |
| }, | |
| nullable: true, | |
| }) | |
| class?: { | |
| name: string; | |
| } | null; | |
| `@ApiPropertyOptional`({ | |
| type: Object, | |
| example: { | |
| name: 'Grade 10', | |
| }, | |
| nullable: true, | |
| }) | |
| class?: { | |
| name: string; | |
| } | null; |
🤖 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 73 -
82, Update the ApiPropertyOptional decorator for the class field in the student
response DTO to use Swagger type Object instead of String, matching its actual {
name: string } | null shape and the parent field’s configuration.
| @ApiPropertyOptional({ type: String, example: 'John Doe Sr.', nullable: true }) | ||
| @IsString() | ||
| @IsOptional() | ||
| parentName?: string | null; | ||
|
|
||
| @ApiPropertyOptional({ type: String, example: '1234567890', nullable: true }) | ||
| @IsString() | ||
| @IsOptional() | ||
| parentPhone?: string | null; | ||
|
|
||
| @ApiPropertyOptional({ type: String, example: '123 Main St', nullable: true }) | ||
| @IsString() | ||
| @IsOptional() | ||
| parentAddress?: string | null; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
No field to target a non-GUARDIAN parent relationship on update.
Unlike create-student.dto.ts (which carries parent_student_relationship: FATHER/MOTHER/GUARDIAN), this DTO only exposes flat parentName/parentPhone/parentAddress with no relationship selector. See companion finding in students.service.ts#update, which hardcodes the GUARDIAN relationship when looking up/creating the parent record. As per coding guidelines, this repo pass should not "implement... implicit many-to-many fixes... during this pass; flag such findings for a schema decision instead" — flagging for a product/schema decision rather than proposing a DTO diff here.
🤖 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/update-student.dto.ts` around lines 6 - 19,
Flag the missing parent relationship selector in UpdateStudentDto as a
product/schema decision rather than modifying this DTO. Coordinate with
students.service.ts update, which currently assumes GUARDIAN, and do not
implement implicit many-to-many or relationship changes in this pass.
Source: Coding guidelines
| const existingParentStudent = await tx.parentStudent.findFirst({ | ||
| where: { studentId: id, relationship: ParentRelationship.GUARDIAN }, | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Parent update/lookup hardcodes ParentRelationship.GUARDIAN, ignoring FATHER/MOTHER relations set at creation.
create() supports FATHER/MOTHER/GUARDIAN via parent_student_relationship, but update()'s existingParentStudent lookup filters strictly on relationship: ParentRelationship.GUARDIAN. For a student whose parent was created as FATHER or MOTHER, this lookup finds nothing — so an update creates a second, duplicate parentStudent/parent pair (as GUARDIAN) instead of updating the existing one, and a "clear parent" request silently no-ops instead of deleting the FATHER/MOTHER record. As per coding guidelines, this class of "implicit many-to-many" gap should be flagged for a schema/product decision rather than patched ad hoc — see companion note on update-student.dto.ts (no relationship selector exists there either).
[
Also applies to: 352-364, 370-377
🤖 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/students.service.ts` around lines 331 - 333,
Update the parent lookup and subsequent create, clear, and update handling in
the students service’s update flow around existingParentStudent to avoid
assuming ParentRelationship.GUARDIAN when relationships may be FATHER or MOTHER.
Flag this implicit many-to-many relationship ambiguity for a schema/product
decision, and coordinate the change with update-student.dto.ts because it lacks
a relationship selector; do not apply an ad hoc relationship-specific patch.
1 participant
Summary by CodeRabbit
New Features
Improvements
Changes