-
Notifications
You must be signed in to change notification settings - Fork 5
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
Feature/ideation endpoints v2 #20
Conversation
I also recommend changing the resource to plural form, like |
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'd like to see the methods in the controller and service more descriptive. I have no idea what create and update is supposed to refer to. Also use the same name in the controller and service (that is the convention).
Remember that we have to support editing project idea and increasing/decreasing votes. In a way, both could be applied to a method called "update", hence why it's not clear to me.
fixed updatedAt fix projectIdea field to camel case
yarn add class-validator
validators added
implemented ProjectIdea from schema updated projectIdeaVotes property
adding userId to body when possible
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.
all endpoint works but all errors are just the default 500 server error. I'm not sure how much of these error handling are required for this.
Also, should a vote be automatically added when a person contribute to an idea? I mean the person should "like" that idea if they posted it?
voteCount is not needed as mentioned it can be obtained by the length of projectIdeaVotes
"projectIdeaVotes": [
{
"id": 2,
"voyageTeamMemberId": 5,
"projectIdeaId": 16,
"createdAt": "2023-10-24T13:47:44.394Z",
"updatedAt": "2023-10-24T13:47:44.394Z",
"votedBy": {
"id": 5,
"userId": "dd532498-16f7-4d32-b054-e0ed5518cb11",
"voyageTeamId": 3,
"voyageRoleId": 7,
"statusId": 4,
"hrPerSprint": 15,
"createdAt": "2023-10-24T13:30:54.444Z",
"updatedAt": "2023-10-24T13:30:54.444Z",
"member": {
"id": "dd532498-16f7-4d32-b054-e0ed5518cb11",
"avatar": "https://gravatar.com/avatar/3bfaef00e02a22f99e17c66e7a9fdd31?s=400&d=monsterid&r=x",
"firstName": "Larry",
"lastName": "Castro"
}
}
}
use private keyword https://www.typescriptlang.org/docs/handbook/2/classes.html#private Also I saw you wrote comment to denote them but that's not necessary. It's common convention to put private methods either at the top of the file or at the bottom. I have no preference to either. |
Good point. I think we should be consistent and just add a vote for themself for everything in this project. Editing this as I thought about it, let's worry about error handling for this pr specifically later as it's very high priority to get this merged. |
sorry pressed the wrong button |
sorry posted this but forgot to say anything about it, for this endpoint, I believe all these are not needed "id": 5,
"userId": "dd532498-16f7-4d32-b054-e0ed5518cb11",
"voyageTeamId": 3,
"voyageRoleId": 7,
"statusId": 4,
"hrPerSprint": 15,
"createdAt": "2023-10-24T13:30:54.444Z",
"updatedAt": "2023-10-24T13:30:54.444Z", |
private added to helper methods create ideation also adds creators vote delete ideation also removed creators vote
Which endpoint is this for? |
return votes I think, should only need first name, last name and avatar, maybe id |
The only endpoint that returns all that information is getIdeationsByVoyageTeam. I've removed the votedBy info from the response section ( info from the voyageTeamMember table). This is the updated current return: I removed a few objects so it wasn't as long as it was so some { may be off. |
remove voyageTeamMember information from Get query results
making description and vision required per Figma
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 good, more error handling can be added later
/api/v1/teams/{teamId}/ideation-vote should have the project idea id in the paramter and not the body. Other than that, this looks good to merge. Great job on this Sarah :) |
removed projectIdeaId requirement from create ideationVote added dto to deleteIdeation and deleteIdeationVote to move userId to body from parameter
updating for userId to be in body and add Dto files verify ids are uniform
update createIdeationVote endpoint parameters remove userId from endpoint and add to body for deleteIdeation and deleteIdeationVote
Both delete endpoints use a body for userId and ideation vote endpoint has ideationId moved to a parameter. |
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 - great job taking in all comments and incorporating them into your code.
I tested endpoints locally.
Endpoints should be plural - we will take care of that in another task
Description
implemented service layer of the ideation GitHub issue
Issue link
#16
Fixes #16
Type of change for the service layer of ideation
How Has This Been Tested?
I tested using Swagger
SM- Also tested via swagger
Checklist: