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

[CRT-5] Added Classes API #52

merged 5 commits into from
Nov 5, 2024

Conversation

pierluca
Copy link
Contributor

@pierluca pierluca commented Nov 4, 2024

No description provided.

@pierluca pierluca requested a review from Tyratox November 4, 2024 11:08
Copy link
Contributor

@Tyratox Tyratox left a comment

Choose a reason for hiding this comment

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

Overall LGTM, added some small comments / questions. Also check the CI ;)

backend/src/api/classes/classes.controller.spec.ts Outdated Show resolved Hide resolved
Comment on lines 48 to 53
@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 :(

Comment on lines +41 to +43
create(klass: Prisma.ClassUncheckedCreateInput): Promise<Class> {
const checkedClass: Prisma.ClassCreateInput = {
name: klass.name,
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?

backend/src/api/classes/dto/existing-class-extended.dto.ts Outdated Show resolved Hide resolved
backend/src/api/classes/dto/existing-class.dto.ts Outdated Show resolved Hide resolved
backend/src/api/classes/dto/existing-class-extended.dto.ts Outdated Show resolved Hide resolved
Comment on lines +57 to +58
// TODO: Implement this properly when auth is available
// TODO: Only allow teachers to see their own classes
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!

backend/src/api/classes/dto/existing-class-teacher.dto.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@Tyratox Tyratox left a comment

Choose a reason for hiding this comment

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

So nice!! 😄😄

@Tyratox Tyratox merged commit ed43c35 into main Nov 5, 2024
2 checks passed
@Tyratox Tyratox deleted the feature/CRT-5_api_class branch November 5, 2024 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants