Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CRT-5] Added Classes API #52

Merged
merged 5 commits into from
Nov 5, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions backend/src/api/api.module.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import { Module } from "@nestjs/common";
import { APP_INTERCEPTOR } from "@nestjs/core";
import { UsersModule } from "./users/users.module";
import * as interceptors from "./interceptors";
import { UsersModule } from "./users/users.module";
import { ClassesModule } from "./classes/classes.module";

@Module({
imports: [UsersModule],
imports: [UsersModule, ClassesModule],
providers: [
{
provide: APP_INTERCEPTOR,
Expand Down
193 changes: 193 additions & 0 deletions backend/src/api/classes/classes.controller.spec.ts
pierluca marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,193 @@
import { Test, TestingModule } from "@nestjs/testing";
import { DeepMockProxy, mockDeep } from "jest-mock-extended";
import { plainToInstance } from "class-transformer";
import { PrismaClient } from "@prisma/client";
import { CoreModule } from "src/core/core.module";
import { PrismaService } from "src/prisma/prisma.service";
import { ClassesController } from "./classes.controller";
import { ClassesService, ClassExtended } from "./classes.service";
import {
CreateClassDto,
UpdateClassDto,
ExistingClassDto,
DeletedClassDto,
ExistingClassExtendedDto,
ExistingClassTeacherDto,
} from "./dto";

describe("ClassesController", () => {
let controller: ClassesController;
let prismaMock: DeepMockProxy<PrismaClient>;

beforeEach(async () => {
prismaMock = mockDeep<PrismaClient>();

const module: TestingModule = await Test.createTestingModule({
imports: [CoreModule],
controllers: [ClassesController],
providers: [
ClassesService,
{
provide: PrismaService,
useValue: prismaMock,
},
],
}).compile();

controller = module.get<ClassesController>(ClassesController);

jest.clearAllMocks();
});

it("should be defined", () => {
expect(controller).toBeDefined();
});

it("can create a class", async () => {
const dto: CreateClassDto = { name: "Test Class", teacherId: 33 };
const createdClass = { id: 1, ...dto };
prismaMock.class.create.mockResolvedValue(createdClass);

const result = await controller.create(dto);

expect(result).toEqual(plainToInstance(ExistingClassDto, createdClass));
expect(prismaMock.class.create).toHaveBeenCalledWith({
data: {
name: dto.name,
teacher: { connect: { id: dto.teacherId } },
},
});
});

it("can update a class", async () => {
const id = 3;
const dto: UpdateClassDto = { name: "Updated Class", teacherId: 34 };
const updatedClass = { id, ...dto };
prismaMock.class.update.mockResolvedValue(updatedClass);

const result = await controller.update(id, dto);

expect(result).toEqual(plainToInstance(ExistingClassDto, updatedClass));
expect(prismaMock.class.update).toHaveBeenCalledWith({
data: dto,
where: { id },
});
});

it("can delete a class", async () => {
const id = 99;
const deletedClass = { id, name: "Deleted Class", teacherId: 34 };
prismaMock.class.delete.mockResolvedValue(deletedClass);

const result = await controller.remove(id);

expect(result).toEqual(plainToInstance(DeletedClassDto, deletedClass));
expect(prismaMock.class.delete).toHaveBeenCalledWith({
where: { id },
});
});

it("can find an existing class", async () => {
const klass: ClassExtended = {
id: 1,
name: "Test Class",
sessions: [],
teacherId: 33,
teacher: { name: "Teacher" },
_count: { students: 0 },
};
prismaMock.class.findUniqueOrThrow.mockResolvedValue(klass);

const result = await controller.findOne(1);

expect(result).toBeInstanceOf(ExistingClassExtendedDto);
expect(result).toEqual(plainToInstance(ExistingClassExtendedDto, klass));
expect(prismaMock.class.findUniqueOrThrow).toHaveBeenCalledWith({
include: {
_count: {
select: { students: true },
},
sessions: {
select: { id: true },
},
teacher: {
select: { id: true, name: true },
},
},
where: { id: 1 },
});
});

it("should return not found for a non-existing class", async () => {
prismaMock.class.findUniqueOrThrow.mockRejectedValue(
new Error("Not found"),
);

const action = controller.findOne(999);
await expect(action).rejects.toThrow("Not found");
});

it("can retrieve all classes", async () => {
const classes = [
{
id: 1,
name: "Test Class",
teacherId: 5,
teacher: { id: 5, name: "Jerry Smith" },
},
{
id: 2,
name: "Another Class",
teacherId: 6,
teacher: { id: 6, name: "Summer Smith" },
},
];

prismaMock.class.findMany.mockResolvedValue(classes);

const result = await controller.findAll();

expect(prismaMock.class.findMany).toHaveBeenCalledTimes(1);
expect(Array.isArray(result)).toBe(true);
expect(
result.every((klass) => klass instanceof ExistingClassTeacherDto),
).toBe(true);
expect(result).toEqual(
classes.map((klass) => plainToInstance(ExistingClassTeacherDto, klass)),
);
});

it("can retrieve all classes for a specific teacher", async () => {
const teacherId = 5;
const classes = [
{
id: 1,
name: "Test Class",
teacherId: teacherId,
teacher: { id: teacherId, name: "Jerry Smith" },
},
{
id: 2,
name: "Another Class",
teacherId: teacherId,
teacher: { id: teacherId, name: "Jerry Smith" },
},
];

prismaMock.class.findMany.mockResolvedValue(classes);

const result = await controller.findAll(teacherId);

expect(prismaMock.class.findMany).toHaveBeenCalledWith({
where: { teacherId: teacherId },
include: { teacher: { select: { id: true, name: true } } },
});
expect(Array.isArray(result)).toBe(true);
expect(
result.every((klass) => klass instanceof ExistingClassTeacherDto),
).toBe(true);
expect(result).toEqual(
classes.map((klass) => plainToInstance(ExistingClassTeacherDto, klass)),
);
});
});
100 changes: 100 additions & 0 deletions backend/src/api/classes/classes.controller.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
import {
Body,
Controller,
Delete,
Get,
Param,
ParseIntPipe,
Patch,
Post,
Query,
} from "@nestjs/common";
import {
ApiCreatedResponse,
ApiForbiddenResponse,
ApiNotFoundResponse,
ApiOkResponse,
ApiQuery,
ApiTags,
} from "@nestjs/swagger";
import { plainToInstance } from "class-transformer";
import {
CreateClassDto,
ExistingClassDto,
UpdateClassDto,
DeletedClassDto,
ClassId,
ExistingClassTeacherDto,
ExistingClassExtendedDto,
} from "./dto";
import { ClassesService } from "./classes.service";

@Controller("classes")
@ApiTags("classes")
export class ClassesController {
constructor(private readonly classesService: ClassesService) {}

@Post()
@ApiCreatedResponse({ type: ExistingClassDto })
@ApiForbiddenResponse()
async create(
@Body() createClassDto: CreateClassDto,
): Promise<ExistingClassDto> {
const klass = await this.classesService.create(createClassDto);
return plainToInstance(ExistingClassDto, klass);
}

@Get()
@ApiQuery({ name: "teacherId", required: false, type: Number })
@ApiOkResponse({ type: ExistingClassTeacherDto, isArray: true })
async findAll(
@Query("teacherId", new ParseIntPipe({ optional: true }))
teacherId?: number,
): Promise<ExistingClassTeacherDto[]> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm it would be nice if we did not have to add these decorators twice. according to the docs, @query should suffice? (https://docs.nestjs.com/openapi/types-and-parameters)

was there something wrong with that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had issues originally, but I resolved them recently - I can revisit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just checked: without the ApiQuery decorator, it shows up as required.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see :(

// TODO: add pagination support

const classes = await this.classesService.listClassesWithTeacher({
// TODO: Implement this properly when auth is available
// TODO: Only allow teachers to see their own classes
Comment on lines +57 to +58
Copy link
Contributor

Choose a reason for hiding this comment

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

and once we do that, do we still include the teacher in the response?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd say so. Generic endpoint, same handling on the frontend, just an authz limitation.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, makes sense!

where: !!teacherId ? { teacherId } : undefined,
});

return classes.map((klass) =>
plainToInstance(ExistingClassTeacherDto, klass),
);
}

@Get(":id")
@ApiOkResponse({ type: ExistingClassExtendedDto })
@ApiForbiddenResponse()
@ApiNotFoundResponse()
async findOne(
@Param("id", ParseIntPipe) id: ClassId,
): Promise<ExistingClassExtendedDto> {
const user = await this.classesService.findByIdOrThrow(id);
return plainToInstance(ExistingClassExtendedDto, user);
}

@Patch(":id")
@ApiCreatedResponse({ type: ExistingClassDto })
@ApiForbiddenResponse()
@ApiNotFoundResponse()
async update(
@Param("id", ParseIntPipe) id: ClassId,
@Body() updateClassDto: UpdateClassDto,
): Promise<ExistingClassDto> {
const klass = await this.classesService.update(id, updateClassDto);
return plainToInstance(ExistingClassDto, klass);
}

@Delete(":id")
@ApiOkResponse({ type: DeletedClassDto })
@ApiForbiddenResponse()
@ApiNotFoundResponse()
async remove(
@Param("id", ParseIntPipe) id: ClassId,
): Promise<DeletedClassDto> {
const klass = await this.classesService.deleteById(id);
return plainToInstance(DeletedClassDto, klass);
}
}
9 changes: 9 additions & 0 deletions backend/src/api/classes/classes.module.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { Module } from "@nestjs/common";
import { ClassesController } from "./classes.controller";
import { ClassesService } from "./classes.service";

@Module({
controllers: [ClassesController],
providers: [ClassesService],
})
export class ClassesModule {}
18 changes: 18 additions & 0 deletions backend/src/api/classes/classes.service.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { Test, TestingModule } from "@nestjs/testing";
import { ClassesService } from "src/classes/services/classes.service";

describe("ClassesService", () => {
let service: ClassesService;

beforeEach(async () => {
const module: TestingModule = await Test.createTestingModule({
providers: [ClassesService],
}).compile();

service = module.get<ClassesService>(ClassesService);
});

it("should be defined", () => {
expect(service).toBeDefined();
});
});
59 changes: 59 additions & 0 deletions backend/src/api/classes/classes.service.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import { Injectable } from "@nestjs/common";
import { Class, Prisma } from "@prisma/client";
import { PrismaService } from "src/prisma/prisma.service";
import { ClassId } from "./dto";

type StudentCount = { _count: { students: number } };
type Teacher = { teacher: { name: string | null } };
type SessionIds = { sessions: { id: number }[] };

export type ClassExtended = Class & StudentCount & Teacher & SessionIds;
export type ClassWithTeacher = Class & Teacher;

@Injectable()
export class ClassesService {
constructor(private readonly prisma: PrismaService) {}

findByIdOrThrow(id: ClassId): Promise<ClassExtended> {
return this.prisma.class.findUniqueOrThrow({
where: { id },
include: {
sessions: {
select: {
id: true,
},
},
teacher: { select: { id: true, name: true } },
_count: { select: { students: true } },
},
});
}

listClassesWithTeacher(
args?: Omit<Prisma.ClassFindManyArgs, "select" | "include">,
): Promise<ClassWithTeacher[]> {
return this.prisma.class.findMany({
...args,
include: { teacher: { select: { id: true, name: true } } },
});
}

create(klass: Prisma.ClassUncheckedCreateInput): Promise<Class> {
const checkedClass: Prisma.ClassCreateInput = {
name: klass.name,
Comment on lines +41 to +43
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the difference between the checked/unchecked types here?

Copy link
Contributor Author

@pierluca pierluca Nov 4, 2024

Choose a reason for hiding this comment

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

checked: { ..., teacher: { connect: { id: XXX } } }, it verifies that the ID is valid and the row exists.

unchecked: { ..., teacherId: XXX }, inserts as-is in database and lets the thing crash if a constraint is violated.

Copy link
Contributor

Choose a reason for hiding this comment

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

so in both cases there will be an exception but once by the db and once by prisma?

teacher: { connect: { id: klass.teacherId } },
};
return this.prisma.class.create({ data: checkedClass });
}

update(id: ClassId, klass: Prisma.ClassUpdateInput): Promise<Class> {
return this.prisma.class.update({
data: klass,
where: { id },
});
}

deleteById(id: ClassId): Promise<Class> {
return this.prisma.class.delete({ where: { id } });
}
}
Loading
Loading