Server/feat/class academic year default#28
Conversation
- Adds AcademicYearContextService (request-scoped) that resolves the active academic year from the x-academic-year-id header, falling back to whichever AcademicYear row has isCurrent = true. Caches per request. - Exports ACADEMIC_YEAR_HEADER constant for use by consumers. - Registers AcademicYearContextModule (global) in AppModule.imports. Note: this commit touches src/app.module.ts, which other branches in this series also touch — merge/rebase carefully. Co-Authored-By: Claude <noreply@anthropic.com>
…ears module
- Service: setCurrent(id) runs in a transaction: updateMany(isCurrent: true →
false) then update(isCurrent: true). getCurrent() returns the row with
isCurrent = true.
- Controller: PATCH /academic-years/:id/set-current and GET
/academic-years/current. The static GET /current route is declared before
GET /:id to avoid Nest matching the literal "current" as an id param.
Note (not auto-applied): Prisma cannot express a partial unique index. For a
DB-level guard that only one AcademicYear has isCurrent = true, add this
manually to a future migration:
CREATE UNIQUE INDEX academic_years_one_current_idx
ON academic_years ((true)) WHERE "isCurrent" = true;
For now, setCurrent enforces the invariant at the application level inside
its transaction.
Co-Authored-By: Claude <noreply@anthropic.com>
- Refactored Teacher, Student, Parent, ParentStudent, AcademicYear, Class, Enrollment, Subject, TeachingAssignment, Timetable, Attendance, Exam, Result, and Announcement models in Prisma schema for better clarity and structure. - Changed TeachingAssignment model to TeachingAllocation and updated related references. - Enhanced AcademicYearContextService to cache active academic year ID and improved error handling for missing academic years. - Updated unit tests for AcademicYearContextService to reflect changes in logic and ensure proper functionality. - Adjusted various DTOs and service methods to accommodate new academic year handling logic. - Cleaned up imports and formatting across multiple files for consistency.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a request-scoped active academic year resolver, wires it into class defaults, exposes academic-year current-state endpoints, restructures related Prisma models, and updates student/class response shapes and supporting cleanup files. ChangesAcademic year context and module wiring
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ClassesService
participant AcademicYearContextService
participant PrismaService
ClassesService->>AcademicYearContextService: getActiveId()
AcademicYearContextService->>PrismaService: findUnique(header id) or findFirst(isCurrent)
PrismaService-->>AcademicYearContextService: academic year or null
AcademicYearContextService-->>ClassesService: resolved academicYearId
sequenceDiagram
participant AcademicYearsController
participant AcademicYearsService
participant PrismaService
AcademicYearsController->>AcademicYearsService: setCurrent(id)
AcademicYearsService->>PrismaService: findUnique(id)
PrismaService-->>AcademicYearsService: academic year or null
AcademicYearsService->>PrismaService: $transaction(updateMany, update)
PrismaService-->>AcademicYearsService: updated record
AcademicYearsService-->>AcademicYearsController: AcademicYearResponseDto
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 5
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/classes/classes.service.spec.ts (1)
1-42: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftAdd behavioral tests for
create()andfindAll()override/fallback paths.The spec only has a "should be defined" test. Coding guidelines require tests covering explicit
academicYearIdoverride andacademicYearContext.getActiveId()fallback for bothcreate()andfindAll(). TheAcademicYearContextServicemock is wired but never exercised.Additionally, the
PrismaServicemock is missingclass.findFirst, whichClassesService.create()calls to check for existing classes — anycreate()test would fail without it.🧪 Suggested test additions
{ provide: AcademicYearContextService, useValue: { getActiveId: jest.fn().mockResolvedValue('academic-year-id'), }, }, ], }).compile(); service = module.get<ClassesService>(ClassesService); }); it('should be defined', () => { expect(service).toBeDefined(); }); + describe('create', () => { + it('uses explicit academicYearId from dto when provided', async () => { + const createDto = { + name: 'Class A', + academicYearId: 'explicit-id', + }; + const mockPrisma = module.get(PrismaService); + mockPrisma.class.findFirst.mockResolvedValue(null); + mockPrisma.class.create.mockResolvedValue({ + id: 'class-1', + name: 'Class A', + academicYearId: 'explicit-id', + academicYear: { id: 'explicit-id', name: '2025-2026' }, + }); + + const result = await service.create(createDto as any); + + expect(result.id).toBe('class-1'); + expect(mockPrisma.class.findFirst).toHaveBeenCalledWith({ + where: { name: 'Class A', academicYearId: 'explicit-id' }, + }); + }); + + it('falls back to academicYearContext.getActiveId() when academicYearId omitted', async () => { + const createDto = { name: 'Class B' }; + const mockPrisma = module.get(PrismaService); + const mockCtx = module.get(AcademicYearContextService); + mockPrisma.class.findFirst.mockResolvedValue(null); + mockPrisma.class.create.mockResolvedValue({ + id: 'class-2', + name: 'Class B', + academicYearId: 'academic-year-id', + academicYear: { id: 'academic-year-id', name: '2025-2026' }, + }); + + const result = await service.create(createDto as any); + + expect(result.id).toBe('class-2'); + expect(mockCtx.getActiveId).toHaveBeenCalled(); + expect(mockPrisma.class.findFirst).toHaveBeenCalledWith({ + where: { name: 'Class B', academicYearId: 'academic-year-id' }, + }); + }); + }); + + describe('findAll', () => { + it('uses explicit academicYearId filter when provided', async () => { + const mockPrisma = module.get(PrismaService); + const mockCtx = module.get(AcademicYearContextService); + mockPrisma.class.count.mockResolvedValue(0); + mockPrisma.class.findMany.mockResolvedValue([]); + + await service.findAll({ academicYearId: 'explicit-id' } as any); + + expect(mockCtx.getActiveId).not.toHaveBeenCalled(); + expect(mockPrisma.class.findMany).toHaveBeenCalledWith( + expect.objectContaining({ + where: expect.objectContaining({ academicYearId: 'explicit-id' }), + }), + ); + }); + + it('falls back to academicYearContext.getActiveId() when filter omitted', async () => { + const mockPrisma = module.get(PrismaService); + const mockCtx = module.get(AcademicYearContextService); + mockPrisma.class.count.mockResolvedValue(0); + mockPrisma.class.findMany.mockResolvedValue([]); + + await service.findAll({} as any); + + expect(mockCtx.getActiveId).toHaveBeenCalled(); + expect(mockPrisma.class.findMany).toHaveBeenCalledWith( + expect.objectContaining({ + where: expect.objectContaining({ academicYearId: 'academic-year-id' }), + }), + ); + }); + }); });Also add
findFirstto the PrismaService mock:class: { findUnique: jest.fn(), + findFirst: jest.fn(), findMany: jest.fn(), create: jest.fn(), update: jest.fn(), delete: jest.fn(), count: jest.fn(), },🤖 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.spec.ts` around lines 1 - 42, The `ClassesService` spec only checks instantiation and does not cover the required `create()` and `findAll()` override/fallback behavior. Add tests in `classes.service.spec.ts` that exercise `ClassesService.create` and `ClassesService.findAll` with an explicit `academicYearId` as well as the fallback to `AcademicYearContextService.getActiveId()`, and assert the expected Prisma calls. Also extend the `PrismaService` mock used in `beforeEach` to include `class.findFirst`, since `ClassesService.create` relies on it and the new tests will need it.Source: Coding guidelines
🧹 Nitpick comments (4)
server/src/modules/students/dto/student-response.dto.ts (1)
41-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the commented-out
enrollmentproperty and any now-unusedEnrollmentDtoimport.If
enrollmentis being replaced byclass, drop the dead commented lines; also verifyEnrollmentDtois still needed after the decorator fix above, or remove its import to avoid an unused symbol.🤖 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 41 - 42, Remove the dead commented-out enrollment property from StudentResponseDto and clean up any now-unused EnrollmentDto import. If student enrollment is now represented elsewhere (for example by class), keep StudentResponseDto focused on the active fields and ensure the remaining decorators and imports in the dto still compile cleanly without unused symbols.server/src/modules/classes/classes.service.ts (2)
42-52: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnneeded
academicYearinclude after removingacademicYearNamefrom the response.
formatClass(inclass.formatter.ts) no longer readsclassItem.academicYear, onlyacademicYearId. Yetcreate,findAll,findOne, andupdatestillinclude: { academicYear: true }, forcing an unnecessary join on every request, including the paginatedfindAlllist.♻️ Proposed fix (repeat for each call site)
const classItem = await this.prisma.class.create({ data: { ... }, - include: { - academicYear: true, - }, });Also applies to: 73-81, 95-100, 140-144
🤖 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 42 - 52, `ClassesService` is still eagerly loading `academicYear` in `create`, `findAll`, `findOne`, and `update` even though `formatClass` in `class.formatter.ts` no longer uses it. Remove the unnecessary `include: { academicYear: true }` from each Prisma call and keep the returned shape aligned with what `formatClass` now reads (`academicYearId` only), so the service avoids the extra join on every request.
62-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant truthiness check; unreachable in the success path.
getActiveId()either resolves to a non-empty id or throws — so once execution reaches Line 67,effectiveAcademicYearIdis always truthy unlessacademicYearIdwas explicitly passed as an empty string (allowed byQueryClassDto, which lacks@IsNotEmpty()). If empty-string is meant as an intentional "no filter" override, worth a comment; otherwise this branch is dead code.🤖 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 62 - 69, The truthiness check on effectiveAcademicYearId in classes.service.ts is redundant because getActiveId() already returns a valid id or throws, so the branch only matters if QueryClassDto allows an empty academicYearId. Update the logic in ClassesService to either explicitly treat empty string as a deliberate “no filter” case with a clarifying comment, or normalize/validate academicYearId so the conditional can be removed and the where clause is always built from a real id.server/src/common/academic-year/academic-year-context.service.ts (1)
7-14: 🚀 Performance & Scalability | 🔵 TrivialRequest-scope will cascade to all consumers.
Making this provider
Scope.REQUESTforces every provider that injects it (e.g.ClassesService, and per the planTeachingAssignmentsService) to also become request-scoped, disabling singleton caching for those services and adding DI resolution overhead per request. This is inherent to the design but worth confirming is an accepted tradeoff at this scale.🤖 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/academic-year/academic-year-context.service.ts` around lines 7 - 14, The `AcademicYearContextService` is marked `Scope.REQUEST`, which will propagate request scope to every consumer such as `ClassesService` and `TeachingAssignmentsService`, increasing DI overhead and disabling singleton caching. Review whether `REQUEST` injection can be avoided or the context can be passed in another way; if request scope is truly required, make sure the dependent services are intentionally converted to request-scoped and the tradeoff is explicitly accepted in the surrounding design.
🤖 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/schema.prisma`:
- Around line 185-199: The Class Prisma model has been changed in a way that
breaks the existing year-scoped contract used by classes.service and its DTOs.
Restore the academicYearId relation fields on Class and keep the uniqueness
constraint scoped by year, not global, so the generated Prisma client still
matches the create/filter/dedupe logic in classes.service and related class
DTOs. Use the Class model symbols and its @@unique definition to update the
schema consistently with the rest of the module, or otherwise update the full
module and migration together.
In `@server/src/common/academic-year/academic-year-context.service.ts`:
- Around line 1-14: The `AcademicYearContextService` constructor is using
`Request` without importing the Express type, so the injected `REQUEST` value is
being checked against the wrong request shape. Update the
`AcademicYearContextService` import section to bring in `Request` as a type from
Express, and keep using that type for the injected `request` property so header
access via `ACADEMIC_YEAR_HEADER` type-checks correctly.
In `@server/src/modules/academic-years/academic-years.service.spec.ts`:
- Around line 83-84: The prisma.$transaction mock in
academic-years.service.spec.ts is using an any-typed callback parameter (`work:
any`), which triggers the no-unsafe-call lint error when it is invoked. Update
the mockImplementation on prisma.$transaction to type the callback with the
proper transaction function signature instead of any, so the call to
work(prisma) is type-safe and ESLint no longer flags it.
In `@server/src/modules/classes/dto/update-class.dto.ts`:
- Around line 6-17: The UpdateClassDto partial-update fields are still
documented and typed as required, even though UpdateClassDto extends
PartialType(CreateClassDto) and ClassesService.update() accepts omitted values.
Update the name and academicYearId properties in UpdateClassDto to be optional,
and replace their ApiProperty decorators with ApiPropertyOptional so the
generated schema matches PATCH behavior.
In `@server/src/modules/students/dto/student-response.dto.ts`:
- Around line 43-44: The Swagger metadata for the student response DTO is
pointing at the wrong model: the `class` property in `StudentResponseDto` is
decorated with `@ApiProperty` using `EnrollmentDto` even though the field type
is `ClassDto | null`. Update the decorator on `class!` to reference `ClassDto`
so the OpenAPI schema matches the actual response shape.
---
Outside diff comments:
In `@server/src/modules/classes/classes.service.spec.ts`:
- Around line 1-42: The `ClassesService` spec only checks instantiation and does
not cover the required `create()` and `findAll()` override/fallback behavior.
Add tests in `classes.service.spec.ts` that exercise `ClassesService.create` and
`ClassesService.findAll` with an explicit `academicYearId` as well as the
fallback to `AcademicYearContextService.getActiveId()`, and assert the expected
Prisma calls. Also extend the `PrismaService` mock used in `beforeEach` to
include `class.findFirst`, since `ClassesService.create` relies on it and the
new tests will need it.
---
Nitpick comments:
In `@server/src/common/academic-year/academic-year-context.service.ts`:
- Around line 7-14: The `AcademicYearContextService` is marked `Scope.REQUEST`,
which will propagate request scope to every consumer such as `ClassesService`
and `TeachingAssignmentsService`, increasing DI overhead and disabling singleton
caching. Review whether `REQUEST` injection can be avoided or the context can be
passed in another way; if request scope is truly required, make sure the
dependent services are intentionally converted to request-scoped and the
tradeoff is explicitly accepted in the surrounding design.
In `@server/src/modules/classes/classes.service.ts`:
- Around line 42-52: `ClassesService` is still eagerly loading `academicYear` in
`create`, `findAll`, `findOne`, and `update` even though `formatClass` in
`class.formatter.ts` no longer uses it. Remove the unnecessary `include: {
academicYear: true }` from each Prisma call and keep the returned shape aligned
with what `formatClass` now reads (`academicYearId` only), so the service avoids
the extra join on every request.
- Around line 62-69: The truthiness check on effectiveAcademicYearId in
classes.service.ts is redundant because getActiveId() already returns a valid id
or throws, so the branch only matters if QueryClassDto allows an empty
academicYearId. Update the logic in ClassesService to either explicitly treat
empty string as a deliberate “no filter” case with a clarifying comment, or
normalize/validate academicYearId so the conditional can be removed and the
where clause is always built from a real id.
In `@server/src/modules/students/dto/student-response.dto.ts`:
- Around line 41-42: Remove the dead commented-out enrollment property from
StudentResponseDto and clean up any now-unused EnrollmentDto import. If student
enrollment is now represented elsewhere (for example by class), keep
StudentResponseDto focused on the active fields and ensure the remaining
decorators and imports in the dto still compile cleanly without unused symbols.
🪄 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: 8aecb85b-ff8a-4007-8a6c-2c8e370fbfd7
📒 Files selected for processing (28)
server/AGENT.mdserver/DOCS.mdserver/package.jsonserver/prisma/schema.prismaserver/src/app.module.tsserver/src/common/academic-year/academic-year-context.module.tsserver/src/common/academic-year/academic-year-context.service.spec.tsserver/src/common/academic-year/academic-year-context.service.tsserver/src/common/formatters/class.formatter.tsserver/src/common/formatters/subject.formatter.tsserver/src/database/seeders/seed-subjects.tsserver/src/modules/academic-years/academic-years.controller.tsserver/src/modules/academic-years/academic-years.service.spec.tsserver/src/modules/academic-years/academic-years.service.tsserver/src/modules/attendances/dto/query-attendance-dto.tsserver/src/modules/classes/classes.module.tsserver/src/modules/classes/classes.service.spec.tsserver/src/modules/classes/classes.service.tsserver/src/modules/classes/dto/class-response-dto.tsserver/src/modules/classes/dto/create-class.dto.tsserver/src/modules/classes/dto/query-class-dto.tsserver/src/modules/classes/dto/update-class.dto.tsserver/src/modules/parents/parents.controller.tsserver/src/modules/students/dto/student-response.dto.tsserver/src/modules/students/students.controller.tsserver/src/modules/subjects/subjects.controller.tsserver/src/modules/teachers/teachers.controller.tsserver/src/modules/teaching-assignments/teaching-assignments.service.spec.ts
💤 Files with no reviewable changes (1)
- server/DOCS.md
| id String @id @default(cuid()) | ||
| name String | ||
| enrollments Enrollment[] | ||
| teachingAllocations TeachingAllocation[] | ||
| announcements Announcement[] | ||
| // academicYearId String | ||
| // academicYear AcademicYear @relation(fields: [academicYearId], references: [id]) | ||
| // gradeId String | ||
|
|
||
| // grade Grade @relation(fields: [gradeId], references: [id]) | ||
|
|
||
| enrollments Enrollment[] | ||
|
|
||
| teachingAssignments TeachingAssignment[] | ||
|
|
||
| announcements Announcement[] | ||
|
|
||
| // @@unique([gradeId, name]) | ||
| @@unique([name, academicYearId]) | ||
| // @@unique([name, academicYearId]) | ||
|
|
||
| @@unique([name]) | ||
| @@map("classes") | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether the Class module still references academicYearId on the Class entity.
rg -nP --type=ts 'academicYearId' server/src/modules/classes
# Confirm ClassesService create/findAll usage against Class model
ast-grep run --pattern 'this.prisma.class.$$$' --lang typescript server/src/modules/classes || true
rg -nP --type=ts 'prisma\.class\.(create|findMany|findFirst)' server/src/modules/classes -A6Repository: thureinhtet99/student-os
Length of output: 4756
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== server/prisma/schema.prisma: Class model and related AcademicYear model ==="
nl -ba server/prisma/schema.prisma | sed -n '170,235p'
echo
echo "=== server/src/modules/classes/classes.service.ts ==="
nl -ba server/src/modules/classes/classes.service.ts | sed -n '1,220p'
echo
echo "=== server/src/modules/classes/dto/create-class.dto.ts ==="
nl -ba server/src/modules/classes/dto/create-class.dto.ts | sed -n '1,120p'
echo
echo "=== server/src/modules/classes/dto/update-class.dto.ts ==="
nl -ba server/src/modules/classes/dto/update-class.dto.ts | sed -n '1,120p'
echo
echo "=== server/src/modules/classes/dto/query-class-dto.ts ==="
nl -ba server/src/modules/classes/dto/query-class-dto.ts | sed -n '1,120p'
echo
echo "=== server/src/modules/classes/dto/class-response-dto.ts ==="
nl -ba server/src/modules/classes/dto/class-response-dto.ts | sed -n '1,120p'Repository: thureinhtet99/student-os
Length of output: 282
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
files = [
"server/prisma/schema.prisma",
"server/src/modules/classes/classes.service.ts",
"server/src/modules/classes/dto/create-class.dto.ts",
"server/src/modules/classes/dto/update-class.dto.ts",
"server/src/modules/classes/dto/query-class-dto.ts",
"server/src/modules/classes/dto/class-response-dto.ts",
]
for path in files:
p = Path(path)
print(f"=== {path} ===")
if not p.exists():
print("(missing)")
continue
lines = p.read_text().splitlines()
# targeted ranges
if path.endswith("schema.prisma"):
start, end = 170, 235
elif path.endswith("classes.service.ts"):
start, end = 1, 220
else:
start, end = 1, 120
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4d} {lines[i-1]}")
print()
PYRepository: thureinhtet99/student-os
Length of output: 10260
🏁 Script executed:
rg -n "Class 1|academicYearId|@@unique\\(\\[name\\]\\)|academicYear:" server -g'!**/node_modules/**'Repository: thureinhtet99/student-os
Length of output: 50380
Restore Class.academicYearId and year-scoped uniqueness
server/src/modules/classes/classes.service.ts and the related DTOs still create, filter, and dedupe on academicYearId; removing it from Class breaks that contract once the Prisma client is regenerated. @@unique([name]) also makes class names global across all years, which conflicts with the year-scoped model used elsewhere. Keep academicYearId on Class and retain @@unique([name, academicYearId]), or update the whole module/migration together.
🤖 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` around lines 185 - 199, The Class Prisma model
has been changed in a way that breaks the existing year-scoped contract used by
classes.service and its DTOs. Restore the academicYearId relation fields on
Class and keep the uniqueness constraint scoped by year, not global, so the
generated Prisma client still matches the create/filter/dedupe logic in
classes.service and related class DTOs. Use the Class model symbols and its
@@unique definition to update the schema consistently with the rest of the
module, or otherwise update the full module and migration together.
| import { Inject, Injectable, NotFoundException, Scope } from '@nestjs/common'; | ||
| import { REQUEST } from '@nestjs/core'; | ||
| import { PrismaService } from '../../database/prisma/prisma.service.js'; | ||
|
|
||
| export const ACADEMIC_YEAR_HEADER = 'x-academic-year-id'; | ||
|
|
||
| @Injectable({ scope: Scope.REQUEST }) | ||
| export class AcademicYearContextService { | ||
| private activeAcademicYearId: string | null = null; | ||
|
|
||
| constructor( | ||
| @Inject(REQUEST) private readonly request: Request, | ||
| private readonly prisma: PrismaService, | ||
| ) {} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Request import convention and tsconfig lib settings.
rg -n "^import" server/src/common/academic-year/academic-year-context.service.ts
rg -n '"lib"' server/tsconfig*.json
rg -nP "`@Inject`\(REQUEST\)" -A2 -B2 server/srcRepository: thureinhtet99/student-os
Length of output: 823
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tsconfig files =="
git ls-files 'server/tsconfig*.json' 'server/package.json' | sed 's#^`#-` #'
echo
echo "== academic-year-context.service.ts imports =="
cat -n server/src/common/academic-year/academic-year-context.service.ts | sed -n '1,30p'
echo
echo "== tsconfig excerpts =="
for f in $(git ls-files 'server/tsconfig*.json'); do
echo "--- $f ---"
cat "$f"
echo
done
echo "== package.json type deps =="
cat server/package.json | sed -n '1,220p'Repository: thureinhtet99/student-os
Length of output: 5504
🌐 Web query:
@types/node global Request fetch types Request Headers Node 24 TypeScript
💡 Result:
As of July 8, 2026, Node.js provides a native, stable implementation of the Fetch API (via the Undici library) that is globally available without requiring imports [1][2]. TypeScript support for these globals—specifically Request, Headers, and fetch—is included automatically when you install and use the current @types/node package [3][4]. If you are writing TypeScript in a Node.js environment, the @types/node definitions for Request and Headers are automatically exposed as global types, provided your environment is correctly configured [3]. These definitions are typically resolved via undici-types within the @types/node package [3]. To ensure full compatibility and avoid type errors: 1. Installation: Ensure you have the latest @types/node installed in your project: npm install --save-dev @types/node 2. TypeScript Configuration: Ensure your tsconfig.json is configured correctly. Setting moduleResolution to "NodeNext" or "Bundler" is generally recommended for modern Node.js development to correctly resolve global types and ESM/CJS exports [4]. 3. Global Availability: Because these types are part of the global scope in @types/node, you do not need to manually import Request or Headers to use them in your type annotations; they are automatically available [3]. If you encounter conflicts (for instance, if your project also references DOM library types in tsconfig.json via "lib": ["dom"]), ensure that your TypeScript environment understands that these are the Node.js versions of the types [3]. In most standard Node.js projects, the @types/node definitions take precedence for the global objects mentioned.
Citations:
- 1: https://nodejs.org/learn/getting-started/fetch
- 2: https://blog.logrocket.com/fetch-api-node-js/
- 3: https://cdn.jsdelivr.net/npm/@types/node@20.19.8/globals.d.ts
- 4: https://www.codestudy.net/blog/how-can-i-use-native-fetch-with-node-in-typescript-node-v17-6/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== full service =="
cat -n server/src/common/academic-year/academic-year-context.service.ts
echo
echo "== other REQUEST-injected Request typings =="
rg -n "`@Inject`\\(REQUEST\\).*Request" server/src
echo
echo "== any explicit express Request imports =="
rg -n "from 'express'" server/srcRepository: thureinhtet99/student-os
Length of output: 2516
Import Express Request as a type.
Without import type { Request } from 'express';, Request resolves to Node’s global fetch type, so this.request.headers[ACADEMIC_YEAR_HEADER] doesn’t match the injected request object and fails type-checking.
🤖 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/academic-year/academic-year-context.service.ts` around
lines 1 - 14, The `AcademicYearContextService` constructor is using `Request`
without importing the Express type, so the injected `REQUEST` value is being
checked against the wrong request shape. Update the `AcademicYearContextService`
import section to bring in `Request` as a type from Express, and keep using that
type for the injected `request` property so header access via
`ACADEMIC_YEAR_HEADER` type-checks correctly.
| export class UpdateClassDto extends PartialType(CreateClassDto) { | ||
| @ApiProperty({ example: 'Grade 10' }) | ||
| @IsString() | ||
| @IsNotEmpty() | ||
| @MaxLength(200) | ||
| name!: string; | ||
|
|
||
| @ApiProperty({ example: 'ckx123academicyearid' }) | ||
| @IsString() | ||
| @IsNotEmpty() | ||
| academicYearId!: 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 list ==\n'
git ls-files 'server/src/modules/classes/**/*.{dto.ts,service.ts,*.spec.ts}'
printf '\n== create-class.dto.ts ==\n'
sed -n '1,220p' server/src/modules/classes/dto/create-class.dto.ts
printf '\n== update-class.dto.ts ==\n'
sed -n '1,220p' server/src/modules/classes/dto/update-class.dto.ts
printf '\n== classes.service.ts ==\n'
sed -n '1,260p' server/src/modules/classes/classes.service.ts
printf '\n== tests mentioning updateClassDto or PATCH ==\n'
rg -n "updateClassDto|PartialType|IsOptional|academicYearId|name!:" server/src/modules/classes -g '*.spec.ts' -g '*.ts'Repository: thureinhtet99/student-os
Length of output: 8805
🌐 Web query:
NestJS PartialType redeclare property decorators override IsOptional inherited metadata class-validator subclass property partial type
💡 Result:
When using NestJS PartialType, it is designed to automatically handle the transformation of class properties to optional, including the application of class-validator's @IsOptional decorator [1]. However, issues involving lost metadata or overridden decorators in subclasses often arise due to the interplay between manual redeclarations, the NestJS CLI plugin, and the underlying metadata reflection system [2][3][4]. Key Technical Considerations: 1. Automatic Decorator Application: The PartialType helper from @nestjs/swagger (which should be used instead of @nestjs/mapped-types when Swagger is present) includes internal logic to copy properties and apply @IsOptional decorators [1][5]. If validation is missing, it is often because the helper cannot correctly "see" the properties due to how they are defined or because the CLI plugin has generated metadata that conflicts with your manual redeclarations [2][4]. 2. Redeclaring Properties: When you redeclare a property in a subclass that uses PartialType, you may inadvertently strip inherited metadata if you do not properly re-apply the decorators [3]. Recent updates in the NestJS ecosystem (as of mid-2026) have improved how the framework handles redeclared @ApiProperty and plugin-generated metadata by ensuring child class types are respected and that inherited constraints are eagerly applied [3][4]. 3. Overwriting Behavior: If you manually apply a decorator (like @IsOptional or @ApiProperty) in your subclass, it typically overrides the inherited metadata for that specific property. If the result is that the property becomes required again, ensure that you have explicitly included @IsOptional on the redeclared property. 4. Recommended Troubleshooting Steps: - Use @nestjs/swagger's PartialType: Ensure you are importing PartialType from @nestjs/swagger, not @nestjs/mapped-types, if you are using OpenAPI/Swagger features [5]. - Check CLI Plugin Conflicts: If you are using the NestJS CLI plugin, it may generate metadata that interferes with manual decorators. If validation fails, try explicitly defining @ApiProperty and class-validator decorators on the redeclared field to ensure the desired metadata is present [2]. - Inherit Metadata Correctly: If you are encountering issues where validation rules (like @MaxLength) are missing in the subclass, verify if you are extending the class or using it as a parameter for PartialType. NestJS utilities are generally designed to carry over existing class-validator metadata, provided the property name remains the same [1]. If you find that validation is still being ignored after redeclaration, it is often helpful to verify the presence of the metadata using Reflect.getMetadata keys (such as those used by class-validator) to confirm whether your subclass successfully registered the constraints [1].
Citations:
- 1: https://github.com/nestjs/swagger/blob/master/lib/type-helpers/partial-type.helper.ts
- 2: PartialType helper function doesn't apply @IsOptional decorator nestjs/swagger#803
- 3: fix: use child class type when re-declaring an inherited @ApiProperty nestjs/swagger#3840
- 4: fix(type-helpers): eagerly apply plugin metadata properties in mapped type helpers nestjs/swagger#3822
- 5: Extending using PartialType, OmitType… doesn't copy properties decorators nestjs/mapped-types#1174
🌐 Web query:
class-validator inherited decorators subclass redeclared property PartialType IsOptional validation inheritance
💡 Result:
In class-validator, when you extend a class and redefine a property, the library is designed to merge the decorators from the parent class with those defined on the subclass property [1][2]. However, this behavior can be inconsistent in practice, particularly when interacting with NestJS mapped types like PartialType [3][4][5]. Key Considerations for Validation Inheritance and Redefinition: 1. Decorator Merging: Officially, class-validator maintains that when a property is redefined in a subclass, decorators from both the parent class and the subclass are applied [1][2]. In many cases, these decorators are chained together [6]. 2. PartialType Limitations: NestJS PartialType generates a new class where all properties from the source class are marked optional. Because it creates a new class, it can sometimes struggle to preserve or correctly merge validation metadata from the parent class's property decorators [7][8][4]. If you find that validations are missing in a class extending PartialType, it is often due to how the metadata storage is handled during the class transformation [7]. 3. IsOptional Behavior: If you include @IsOptional on a property, it effectively tells class-validator to skip other validations on that property if it is null or undefined [1][5]. If you are experiencing issues where inherited validations are "lost" or ignored, check if an inherited @IsOptional is present, as it may be short-circuiting the validation logic for that field [9][5]. 4. Troubleshooting and Workarounds: - Explicit Redefinition: If inherited decorators are not being applied as expected, explicitly redeclare the property in your subclass and add the necessary decorators again [6][9]. - Metadata Issues: If using NestJS Mapped Types, ensure you are using the versions provided by @nestjs/mapped-types or @nestjs/swagger, as they are specifically designed to handle NestJS-specific metadata [7]. - Custom Inheritance: If standard inheritance fails, some developers use custom solutions like a decorator-based inheritance helper to manually copy validation metadata from a parent class to a child class [9]. In summary, while class-validator intends for decorators to be inherited and merged upon property redefinition, real-world usage—especially with NestJS mapped types—can encounter issues where metadata is not correctly mapped. Explicitly redeclaring the property with the required decorators is often the most reliable way to ensure validation logic is enforced in subclasses [6].
Citations:
- 1: https://github.com/typestack/class-validator?tab=readme-ov-file
- 2: https://github.com/typestack/class-validator
- 3: Validation issues during overriding fields nestjs/mapped-types#1697
- 4: NestJS DTO extended with PartialType breaks validation nestjs/nest#5844
- 5: Add support for consistent inheritance typestack/class-validator#2673
- 6: fix: Inheritance and overriding fields typestack/class-validator#2609
- 7: Extending using PartialType, OmitType… doesn't copy properties decorators nestjs/mapped-types#1174
- 8: Amending properties with decorators for validation (class-validator) nestjs/mapped-types#1665
- 9: fix: overwrite validation decorators from parent class when defined in child typestack/class-validator#748
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== package versions ==\n'
rg -n '"(`@nestjs/mapped-types`|`@nestjs/swagger`|class-validator)"' package.json server/package.json pnpm-lock.yaml package-lock.json yarn.lock 2>/dev/null || true
printf '\n== classes module tests ==\n'
git ls-files 'server/src/modules/classes/**/*.spec.ts' 'server/src/modules/classes/**/*.test.ts'
printf '\n== query dto ==\n'
sed -n '1,200p' server/src/modules/classes/dto/query-class-dto.tsRepository: thureinhtet99/student-os
Length of output: 704
Use ApiPropertyOptional for these PATCH fields name and academicYearId are partial-update inputs, so @ApiProperty() makes the generated schema look required even though ClassesService.update() accepts them as omitted. Use optional property types and @ApiPropertyOptional() 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/classes/dto/update-class.dto.ts` around lines 6 - 17, The
UpdateClassDto partial-update fields are still documented and typed as required,
even though UpdateClassDto extends PartialType(CreateClassDto) and
ClassesService.update() accepts omitted values. Update the name and
academicYearId properties in UpdateClassDto to be optional, and replace their
ApiProperty decorators with ApiPropertyOptional so the generated schema matches
PATCH behavior.
| @ApiProperty({ type: EnrollmentDto, nullable: true }) | ||
| enrollment!: EnrollmentDto | null; | ||
| class!: ClassDto | null; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Swagger decorator references the wrong type.
The class property is typed ClassDto | null, but the @ApiProperty decorator still declares type: EnrollmentDto. The generated OpenAPI schema will describe the wrong shape.
🔧 Proposed fix
- `@ApiProperty`({ type: EnrollmentDto, nullable: true })
+ `@ApiProperty`({ type: ClassDto, nullable: true })
class!: ClassDto | null;📝 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: EnrollmentDto, nullable: true }) | |
| enrollment!: EnrollmentDto | null; | |
| class!: ClassDto | null; | |
| `@ApiProperty`({ type: ClassDto, nullable: true }) | |
| class!: ClassDto | 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 43 -
44, The Swagger metadata for the student response DTO is pointing at the wrong
model: the `class` property in `StudentResponseDto` is decorated with
`@ApiProperty` using `EnrollmentDto` even though the field type is `ClassDto |
null`. Update the decorator on `class!` to reference `ClassDto` so the OpenAPI
schema matches the actual response shape.
1 participant
Summary by CodeRabbit
New Features
API Changes
Bug Fixes