-
Notifications
You must be signed in to change notification settings - Fork 9
3157 get all parts endpoint #3354
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
3157 get all parts endpoint #3354
Conversation
chpy04
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.
LGTM
Peyton-McKee
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 great! Just move some stuff around and maybe rename the endpoint to more accurately describe what it is
| try { | ||
| const wbsNumber: WbsNumber = validateWBS(req.params.wbsNum); | ||
|
|
||
| const project: Project = await ProjectsService.getSingleProject(wbsNumber, req.organization); |
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 typically put this logic inside of the service function, the controller should typically only call one service function.
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.
Okay cool that was something I was wondering, good to know, easy fix
| * @param organizationId organization Id of the Project | ||
| * @returns all the parts from the given project | ||
| */ | ||
| static async getAllParts(projectId: string, 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.
this doesnt quite fit for a getAllParts endpoint, Id call it a getPartsForProject, or it should probably take in the wbsNum and then get a project for that and then find the part.
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.
If I do the latter, do you still want the name change?
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.
but yeah during the process I realized that for our use case, we want to get parts for a specific project (as opposed to an entire org) so I just adjusted it to that and didn't reconsider the name -- I do agree about making the name more specific
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.
yeah name change should be good
| import { validateWBS, WbsNumber } from 'shared'; | ||
|
|
||
| export default class PartReviewController { | ||
| static async getAllParts(req: Request, res: Response, next: NextFunction) { |
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.
Also rename this please
Changes
Created getAllParts endpoints that retrieves all the parts for a specified project, so that the parts can be displayed on the Parts Review page. The information that the endpoint returns includes information about tags, project, assignees, and reviewe requests, but not submissions or reviews.
GET /parts/:wbsNum
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 #3157