-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: change addTool to multipart data #264
base: main
Are you sure you want to change the base?
Conversation
GeekaN2
commented
Jul 2, 2024
ece521b
to
394023c
Compare
* Editor tool cover location | ||
*/ | ||
export type EditorToolCoverFileLocation = { | ||
isEditorToolCover: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe store toolId?
// body: { | ||
// $ref: 'AddEditorToolSchema', | ||
// }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove it, maybe add real validation?
if (editorTool.cover) { | ||
const coverBuffer = await editorTool.cover.toBuffer(); | ||
|
||
coverKey = await fileUploaderService.uploadFile({ | ||
data: coverBuffer, | ||
name: createFileId(), | ||
mimetype: editorTool.cover.mimetype, | ||
}, { | ||
isEditorToolCover: true, | ||
}, { | ||
userId, | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets move this logic into addTool()
/** | ||
* Update tool cover | ||
* @param editorToolId | ||
* @param cover | ||
*/ | ||
public async updateToolCover(editorToolId: EditorTool['id'], cover: EditorTool['cover']): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty docs
a07998d
to
60a0c18
Compare
title: String(editorTool.title?.value), | ||
name: String(editorTool.name?.value), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unsafe call
cover?: MultipartFile; | ||
|
||
/** | ||
* The user ID associated with the editor tool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it the user who added a tool or who updated a tool last time?
title: editorTool.title?.value ?? '', | ||
name: editorTool.name?.value ?? '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to validate all required fields or add a schema
/** | ||
* Tool cover | ||
*/ | ||
EditorToolCover = 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the numbers mean? Add description please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove all commented-out lines if it is not used
const { editorToolsService } = opts; | ||
const { editorToolsService, fileUploaderService } = opts; | ||
|
||
await fastify.register(fastifyMultipart, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the fastify.registed
should be stored here
notes.api/src/presentation/http/http-api.ts
NoteAttachment = 1, | ||
|
||
/** | ||
* Tool cover | ||
*/ | ||
EditorToolCover = 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets store this as string enum for better readability
// eslint-disable-next-line | ||
const formData = new FormData(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spicify eslint rule that you are disabling