Feat/829 pdf backend #937
Feat/829 pdf backend #937
Conversation
fbarreta
force-pushed
the
feat/829-pdf-backend
branch
from
13d5df7 to
6cd5d2e
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
I took a look at your PR. I know its a WIP but just a few things I noticed:
lemme know what you think |
| @@ -16,6 +17,7 @@ import { RecreationResourceModule } from "@/recreation-resource/recreation-resou | |||
| AuthModule, | |||
| TerminusModule, | |||
| RecreationResourceModule, | |||
| ResourceDocsModule, | |||
There was a problem hiding this comment.
it'd be better to have this imported under RecreationResourceModule
| declare const window: any; | ||
| const NodeFormData = | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| typeof window === "undefined" ? require("form-data") : null; |
There was a problem hiding this comment.
what is this for? window will always be undefined right?
There was a problem hiding this comment.
This is a technique used to test, because running vitest with form data we can instantiate while testing.
| description: "Doc title", | ||
| example: "Campbell river site map", | ||
| }) | ||
| @IsNotEmpty() |
There was a problem hiding this comment.
shoud we add length and regex constraints here?
| @Get(":rec_resource_id/docs/:ref_id") | ||
| @ApiOperation({ | ||
| summary: "Get one document resource by reference ID", | ||
| operationId: "getDocumentResourceById", |
There was a problem hiding this comment.
should we name it getDocumentByResourceId to be consistent with the one above?
| }) | ||
| @ApiResponse({ | ||
| status: 404, | ||
| description: "Recreation Resource document not found", |
There was a problem hiding this comment.
nit
should be "Recreation Resource not found"
| } | ||
| if (file) { | ||
| if (!allowedTypes.includes(file.mimetype)) { | ||
| throw new HttpException("File Type not allowed", 501); |
There was a problem hiding this comment.
is 501 appropriate here. isnt this a user error
| await uploadFile(ref_id, file); | ||
| const files = await getResourcePath(ref_id); | ||
| url = this.getOriginalFilePath(files); | ||
| docToUpdate["url"] = url; |
There was a problem hiding this comment.
when you update the original, do all the other image variants keep the same url?
There was a problem hiding this comment.
That was actually a good insight ! I assumed that changing the file, the URL would change ... but that was not the case.
| url = this.getOriginalFilePath(files); | ||
| docToUpdate["url"] = url; | ||
| } | ||
| const result = await this.prisma.$transaction([ |
There was a problem hiding this comment.
transaction is not necessary here
| if (resource === null) { | ||
| throw new HttpException("Recreation Resource not found", 404); | ||
| } | ||
| const ref_id = await createResource(); |
There was a problem hiding this comment.
all these dam api are called one after the other. if anyone of them fails, will it emit logs that show which one has failed?
| async getById( | ||
| rec_resource_id: string, | ||
| ref_id: string, | ||
| ): Promise<RecreationResourceDocDto | null> { |
There was a problem hiding this comment.
will this ever return null?
Controller, services + dam API interaction to list, create, update and delete PDF resources.