32 put project #47
32 put project #47
Conversation
reletes #32
| @@ -0,0 +1,21 @@ | |||
| const { deleteCohort } = require('../../../../database/queries'); | |||
There was a problem hiding this comment.
@MohammedAlghazali I think these deleteCohort changes are originally from #43, the 29-DELETE-cohort branch, maybe because you created this branch (32-PUT-project) branch from the 29-DELETE-cohort branch.
In that case, you can make a PR into the source branch (32-PUT-project -> 29-DELETE-cohort) so that other code reviewers only have to look at the PUT-related changes in github instead of seeing both sets of changes in one pull request.
Then, you could finish the DELETE pull request first, merge that into master, and then change this PR's target branch to master (so then it becomes 32-PUT-project -> master).
Let me know if that makes sense or not - it's a technique you can use to keep other unrelated changes from showing up in your PRs when others are reviewing on github.
There was a problem hiding this comment.
really I needed this idea for this branch,
yes I understand this point, it's like when you make a fork and need to make push to your repo
There was a problem hiding this comment.
Yeah - I do that workflow a lot too, where I make a new branch from an existing branch that is still being peer-reviewed, because I need the first branch's code.
So by switching the base branch in the github PR like in your screenshot, that's how you can do that workflow for yourself, but also keep your pull requests small and focused for your peers :)
| projectId, | ||
| ) => | ||
| connection.query( | ||
| 'UPDATE project SET name=$1, description=$2, img_url=$3, github_link=$4, website_link=$5, project_type=$6, cohort_id=$7 WHERE id = $8', |
There was a problem hiding this comment.
@MohammedAlghazali In general it's best if PUT endpoints can handle requests that do not provide every argument, and can merge with existing data.
For example, it would be good if I could PUT /projects/1 {name: 'updated name'} and then it updates only the name, while leaving the rest of the data, like description and img_url, set to their previous values.
This is a good design because then you can add new columns to the table and endpoint in the future, and if old clients are still doing PUT requests that don't have that new column, they won't nullify data in the mysql table.
In other words, what if you release an iOS app which is PUTting these data columns, then next month you add a new column, project_owner, and your web app can update the project_owner but people are still using the old version of the iOS app to edit projects, and that app does not know about project_owner and is still making PUT requests without project_owner - you want your endpoint to still work for those old apps.
I think you could accomplish this by passing the request body object to this function, instead of all of the destructured arguments, and then doing UPDATE project SET ?. This link has more info:
https://stackoverflow.com/questions/59959755/update-data-with-nodejs/59960177#59960177
| expect(rows[0].name).toBe('Mooooot'); | ||
| done(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
@MohammedAlghazali This test looks good! You could consider adding a test for error conditions too, or a test for validation.
MohammedAlghazali
added
Awaiting Review
MohammedAlghazali
requested review from
rehabas,
ranasobeid95,
Mu7ammadAbed and
AlaaSaadeddin
ranasobeid95
added
Done
and removed
Awaiting Review
relates #32
