23 put cohortid #49

Mu7ammadAbed · 2020-03-30T22:59:02Z

I created PUT method to edit an existed cohort data and I added 2 tests one for an attempt to edit exist cohort and the other non-exist cohort
Relates #23

MohammedAlghazali · 2020-03-31T09:22:09Z

server/controllers/index.js

@@ -1,7 +1,9 @@
 const router = require('express').Router();
+const {
+  cohort: { putSpecificCohort },


as I comment in PR #41

I suggest making destructure of getSpecificCohort in /database/queries/index as :

const { getSpecificCohort } = require('./cohort'); module.exports = { getSpecificCohort };

as you make in server/database/queries/cohort/index.js file,

and here in this file you can make require to getSpecificCohort as

so we get more benefit from /database/queries/index file.

MohammedAlghazali · 2020-03-31T09:23:20Z

server/controllers/index.js

-router.get('/', (req, res) => {
-  res.send('<h1>CA WIKI</h1>');
-});
+router.put('/cohort/:cohortid', validateEditCohort, putSpecificCohort);


based on issue #5 please change the route to be /cohorts/:cohortId

MohammedAlghazali · 2020-03-31T09:32:33Z

server/database/queries/cohort/putSpecificCohort.js

+exports.putSpecificCohort = (id, req) =>
+  connection.query(
+    'UPDATE cohort SET name = $2, description = $3, img_url = $4, github_link = $5 WHERE id = $1 ',
+    [id, req.name, req.description, req.img_url, req.github_link],


I suggest to make destructure for req data as:

const { name, description, img_url, github_link} = req; connection.query( 'UPDATE cohort SET name = $2, description = $3, img_url = $4, github_link = $5 WHERE id = $1 ', [id, name, description, img_url, github_link],

MohammedAlghazali · 2020-03-31T09:38:56Z

server/validation/validateEditCohort.js

+    img_url: yup.string().url().required(),
+    github_link: yup.string().url().required(),
+  });
+  const projectData = {


I suggest making destructure for req.body

MohammedAlghazali · 2020-03-31T09:40:20Z

server/validation/validateEditCohort.js

+  const schema = yup.object({
+    name: yup.string().required(),
+    description: yup.string().required(),
+    img_url: yup.string().url().required(),


I suggest using camel case to name the key here

MohammedAlghazali

Nice Work

tomduggan85 · 2020-03-31T13:49:53Z

server/database/queries/cohort/putSpecificCohort.js

+
+exports.putSpecificCohort = (id, req) =>
+  connection.query(
+    'UPDATE cohort SET name = $2, description = $3, img_url = $4, github_link = $5 WHERE id = $1 ',


@Mu7ammadAbed I added a comment to another PR which might be helpful here too:
https://github.com/GSG-G8/ca-wiki/pull/47/files#r400924612

It describes how an ideal design for PUT requests is to have them handle requests that don't contain every single argument, so frontends can choose to PUT just a new name.

ranasobeid95 · 2020-03-31T20:56:44Z

server/controllers/routes/admin/cohort/editCohort.js

+
+exports.editCohort = async (req, res) => {
+  try {
+    await editCohortSchema.validate(req.body);


If you test this editCohortSchema.validate(req.body) by default it's aborts all the validations steps once it hit the first validation error , to get all error you would just add the optional second parameter {abortEarly: false } to the call to editCohortSchema.validate(req.body) which look like as I suggested :

Suggested change

await editCohortSchema.validate(req.body);

await editCohortSchema.validate(req.body, {abortEarly: false });

ranasobeid95 · 2020-03-31T21:00:48Z

server/controllers/routes/admin/cohort/editCohort.js

+  } catch (err) {
+    res.status(400).json({ statusCode: 400, message: err.message });
+  }


Here there is two types of error Server error, Client Error so you need to handle each of them

Suggested change

} catch (err) {

res.status(400).json({ statusCode: 400, message: err.message });

}

} catch (err) {

if(err.errors){

res.status(400).json({ statusCode: 400, message: err.message });

}else {

next(err)

}

}

ranasobeid95 · 2020-03-31T21:03:22Z

server/database/queries/cohort/putSpecificCohort.js

+exports.putSpecificCohort = (id, req) =>
+  connection.query(
+    'UPDATE cohort SET name = $2, description = $3, img_url = $4, github_link = $5 WHERE id = $1 ',
+    [id, req.name, req.description, req.imgUrl, req.githubLink],


Destructuring !!

…dit project schema'

tomduggan85 · 2020-04-01T16:39:16Z

server/database/queries/cohort/putSpecificCohort.js

+const putSpecificCohort = (req) => {
+  const { name, description, imgUrl, githubLink, cohortId } = req;
+  return connection.query(
+    'UPDATE cohort SET name = $1, description = $2, img_url = $3, github_link = $4 WHERE id = $5 ',


@Mu7ammadAbed My comment here might be helpful your endpoint - you can design PUT endpoints to accept any number of arguments, instead of always expecting every argument to be provided by the frontend.

Can you provide me with a resource for it ?
I saw what you send to us yesterday and I didn't really get it, I tried so much and searched a lot without reaching any place.

Here's an example of the query syntax: https://stackoverflow.com/a/19358694
You can use a question mark in the query and then pass an object in the arguments array to match with the question mark. In your code, I think that means it could look like this:
return connection.query('UPDATE cohort SET ? WHERE id = ?', [req, req.cohortId]);

Once you do that, you should be able PUT any number / combination of the arguments (for example, only name or only img_url or both name and description together)

Hopefully that works - and if not, sorry for sending you on the wrong path. This change isn't incredibly important for this project, but maybe it would help for a future project.

tomduggan85 · 2020-04-01T16:40:50Z

server/controllers/routes/admin/cohort/editCohort.js

+      res.json({ statusCode: 200, message: 'Changed Succefully' });
+    } else {
+      res.status(404).json({
+        statusCode: 404,


@Mu7ammadAbed You could remove statusCode from the json body because you already called status(404) on the response.

tomduggan85 · 2020-04-01T16:42:41Z

server/controllers/routes/admin/cohort/editCohort.js

+    }
+  } catch (err) {
+    if (err.errors) {
+      res.status(400).json({ statusCode: 400, message: err.errors });


@Mu7ammadAbed Here also you could remove statusCode from the json body.

Add tests

2095e7b

Mu7ammadAbed added Awaiting Review This PR is ready and waiting for review Backend labels Mar 30, 2020

Mu7ammadAbed requested review from AlaaSaadeddin, ranasobeid95, rehabas and MohammedAlghazali March 30, 2020 22:59

Mu7ammadAbed added 4 commits March 31, 2020 02:03

Add new line at the end of putSpecificCohort.js file

03f4a78

define sent data as a const

f9ff730

Add data validation

10a9871

Edit beforeAll/afterAll function

9e1ff32

MohammedAlghazali requested changes Mar 31, 2020

View reviewed changes

MohammedAlghazali added Changes Requested This PR has some mandatory changes to be done in order for it to be accepted and removed Awaiting Review This PR is ready and waiting for review labels Mar 31, 2020

Mu7ammadAbed added 2 commits March 31, 2020 13:50

destructure validation + change /cohort route into /cohorts

29a7f87

Change keys name to camlCase

f9a833b

Mu7ammadAbed added Awaiting Review This PR is ready and waiting for review and removed Changes Requested This PR has some mandatory changes to be done in order for it to be accepted labels Mar 31, 2020

Edit validation schema

84eff3c

MohammedAlghazali previously approved these changes Mar 31, 2020

View reviewed changes

tomduggan85 reviewed Mar 31, 2020

View reviewed changes

rehabas previously approved these changes Mar 31, 2020

View reviewed changes

AlaaSaadeddin previously approved these changes Mar 31, 2020

View reviewed changes

ranasobeid95 reviewed Mar 31, 2020

View reviewed changes

ranasobeid95 added Changes Requested This PR has some mandatory changes to be done in order for it to be accepted and removed Awaiting Review This PR is ready and waiting for review labels Mar 31, 2020

Handle error + destructure query function

457389a

Mu7ammadAbed added 3 commits April 1, 2020 18:56

Fix Conflicts

bee0f4b

Fix Conflicts

98b875b

Move getSpecific Cohort into user route

17db212

Mu7ammadAbed dismissed MohammedAlghazali’s stale review via 17db212 April 1, 2020 16:02

Mu7ammadAbed added 2 commits April 1, 2020 19:07

Move app.use(compression()) to the top of the app.js file and edit 'E…

6cd155c

…dit project schema'

remove commented line

f9a46ad

Mu7ammadAbed requested review from tomduggan85, ranasobeid95, MohammedAlghazali, AlaaSaadeddin and rehabas April 1, 2020 16:14

MohammedAlghazali previously approved these changes Apr 1, 2020

View reviewed changes

tomduggan85 reviewed Apr 1, 2020

View reviewed changes

AlaaSaadeddin previously approved these changes Apr 1, 2020

View reviewed changes

destructure cohortID and sent data

98d40d0

Mu7ammadAbed dismissed stale reviews from AlaaSaadeddin and MohammedAlghazali via 98d40d0 April 1, 2020 18:27

Edit destructure

ff5e871

Mu7ammadAbed requested review from MohammedAlghazali and AlaaSaadeddin April 1, 2020 19:30

rehabas approved these changes Apr 1, 2020

View reviewed changes

ranasobeid95 approved these changes Apr 1, 2020

View reviewed changes

MohammedAlghazali approved these changes Apr 2, 2020

View reviewed changes

MohammedAlghazali merged commit 5b3a966 into master Apr 2, 2020

MohammedAlghazali deleted the 23-put-cohortid branch April 2, 2020 07:05

ranasobeid95 added Done and removed Awaiting Review This PR is ready and waiting for review labels Apr 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

23 put cohortid #49

23 put cohortid #49

Mu7ammadAbed commented Mar 30, 2020 •

edited by ranasobeid95

Loading

MohammedAlghazali Mar 31, 2020

MohammedAlghazali Mar 31, 2020

MohammedAlghazali Mar 31, 2020

MohammedAlghazali Mar 31, 2020

MohammedAlghazali Mar 31, 2020

MohammedAlghazali left a comment

tomduggan85 Mar 31, 2020

ranasobeid95 Mar 31, 2020

ranasobeid95 Mar 31, 2020 •

edited

Loading

ranasobeid95 Mar 31, 2020

tomduggan85 Apr 1, 2020

Mu7ammadAbed Apr 1, 2020

tomduggan85 Apr 1, 2020

tomduggan85 Apr 1, 2020

tomduggan85 Apr 1, 2020

	await editCohortSchema.validate(req.body);
	await editCohortSchema.validate(req.body, {abortEarly: false });

23 put cohortid #49

23 put cohortid #49

Conversation

Mu7ammadAbed commented Mar 30, 2020 • edited by ranasobeid95 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MohammedAlghazali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranasobeid95 Mar 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mu7ammadAbed commented Mar 30, 2020 •

edited by ranasobeid95

Loading

ranasobeid95 Mar 31, 2020 •

edited

Loading