-
Notifications
You must be signed in to change notification settings - Fork 9
#3311 cud endpoints for parts #3360
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
#3311 cud endpoints for parts #3360
Conversation
gcooper407
left a comment
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.
Looks perfect!
| const partsRouter = express.Router(); | ||
|
|
||
| partsRouter.post( | ||
| '/:wbsNum/create', |
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.
It would really be better to have the wbs in the body here -- the path expands to /parts/:wbsNum/create which implies that wbsNum is an identifier for a part (like the param in the same location on other routes is). It also makes things more standard to avoid path params on create
| try { | ||
| const { partId } = req.params; | ||
| const deletedPart = await PartReviewService.deletePart(partId, req.currentUser, req.organization.organizationId); | ||
| res.status(200).json(deletedPart); |
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.
nit: usually it makes more sense to return a success message on delete
| const updatedPart = await prisma.part.update({ | ||
| where: { partId }, | ||
| data: { | ||
| previewImageLink: previewImageData.id |
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.
We should change this field name in the schema, it's an id, not a link
| submitter.userId === part.userCreated.userId; | ||
| if (!hasPermission) throw new AccessDeniedException('Only leadership and part creators can add a preview image'); | ||
|
|
||
| const previewImageData = await uploadFile(previewImage); |
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.
nit: you can use destructuring here
|
|
||
| if (!hasPermission) throw new AccessDeniedException('Only leadership and the part creator can delete a part'); | ||
|
|
||
| const deletedPart = await prisma.part.update({ |
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 have to add the user here too
| await resetUsers(); | ||
| }); | ||
|
|
||
| it('creates a part, updates it, and deletes it', async () => { |
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.
We need exception tests for deletion
| * @param submitter the user making the update | ||
| * @param organization the organization | ||
| */ | ||
| static async uploadPreview(previewImage: Express.Multer.File, partId: string, submitter: User, organizationId: string) { |
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.
nit: name uploadPartPreviewImage or similar to be more specific
| * @param submitter the user making the update | ||
| * @param organization the organization | ||
| */ | ||
| static async uploadPreview(previewImage: Express.Multer.File, partId: string, submitter: User, organizationId: string) { |
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.
We should enforce a size of <= 1 MB here
Changes
Create, update, and delete endpoints for parts, along with an upload preview image endpoint to upload an image as the preview for a part
Test Cases
Screenshots
Checklist
It can be helpful to check the
ChecksandFiles changedtabs.Please review the contributor guide and reach out to your Tech Lead if anything is unclear.
Please request reviewers and ping on slack only after you've gone through this whole checklist.
yarn.lockchanges (unless dependencies have changed)Closes #3311