-
Notifications
You must be signed in to change notification settings - Fork 3
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
#37652856-Backend api implementation of project CRUD #20
base: develop
Are you sure you want to change the base?
Conversation
OpenAPI 3.0 design Yaml and JSON files closes #6
- set content type for catchall error response
- workaround for JS Object destructuring inside an if block considered error by codacity
Added instructions to run API endpoint documentation Added link to external docs close
- Updated readme file - Added schemas and model related to project feature - Created an all model file to import into database ,prevents previous error from mongoose "cannot overrite model already compiled" - Added ApiErrorResponse class to manage api errors - Wrote tests for Project feature endpoints - Updated setupTests.js with code to run before and after test suits run - rename auth directory to user to match docs api endpoints close #14
- Updated readme file - Added schemas and model related to project feature - Created an all model file to import into database ,prevents previous error from mongoose "cannot overrite model already compiled" - Added ApiErrorResponse class to manage api errors - Wrote tests for Project feature endpoints - Updated setupTests.js with code to run before and after test suits run - rename auth directory to user to match docs api endpoints close #14
- Updated readme file - Added schemas and model related to project feature - Created an all model file to import into database ,prevents previous error from mongoose "cannot overrite model already compiled" - Added ApiErrorResponse class to manage api errors - Wrote tests for Project feature endpoints - Updated setupTests.js with code to run before and after test suits run - rename auth directory to user to match docs api endpoints close #14
- added jest options --forceExit --detectOpenHandles --maxWorkers=10 to exit jest when all test pass close #14
- fix codacy fail error close #14
- fix codacy fail error close #14
Hey @tanerochris Please could you attach screenshot for test on different endpoints? |
Hello you can clone the branch and run the test. yarn test or npm test
…On Fri, May 15, 2020, 19:56 Teneng Clifford ***@***.***> wrote:
Hey @tanerochris <https://github.com/tanerochris> Please could you attach
screenshot for test on different endpoints?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#20 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACC3JXKVWUTHWCWLQDN34ELRRWF7LANCNFSM4NBFPPPQ>
.
|
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.
Critical
- File Naming: Going forward let's adopt
camelCase
for naming files as opposed tosnake-case
that's been used. - It appears responses aren't sent for error messages thrown.
- I noticed we're using
res.json()
& having to set the header.res.send()
automatically sets the header so it's preferable. - I might need to schedule a sync with all today(21st May) on this .
* @returns {Promise<void>} | ||
*/ | ||
async function SignOutHandler({ req, res }) { | ||
if (req.method === 'DELETE') { |
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.
Nitpick
If this route is meant to specifically handle sign-out (cos the frontend should specifically send delete
request in the first place) why check the http method in the first place? Besides I believe all checking/validation should be handle by a middleware.
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.
@mernxl could you look into this.
* @returns {Promise<void>} | ||
*/ | ||
async function SignUpHandler({ req, res }) { | ||
if (req.method === 'POST') { |
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.
Nitpick
Same thing as the comment I left above.
*/ | ||
const IndexProjectHandler = async ({ req, res }) => { | ||
if (req.method === 'GET') { | ||
const limit = req.query.limit || 0; |
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.
Nitpick
A better approach would be to abstract these into a helper file or a middleware
@KaiserPhemi I will love to mention that we are using NextJS for the
project ,it is an opinionated framework so there are some aspects of the
file structure and naming convention that is dictated by the framework.
Equally handling API requests.
…On Thu, May 21, 2020, 03:46 Oluwafemi Akinwa ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Critical
- File Naming: Going forward let's adopt camelCase for naming files as
opposed to snake-case that's been used.
- It appears responses aren't sent for error messages thrown.
- I noticed we're using res.json() & having to set the header.
res.send() automatically sets the header so it's preferable.
- I might need to schedule a sync with all today(21st May) on this .
------------------------------
In pages/api/user/sign-out.js
<#20 (comment)>:
> @@ -0,0 +1,19 @@
+import Middleware from '../../../middlewares';
+/**
+ * Sets the session value to null.
+ *
+ * DELETE endpoint
+ * @param req
+ * @param res
+ * @returns {Promise<void>}
+ */
+async function SignOutHandler({ req, res }) {
+ if (req.method === 'DELETE') {
Nitpick
If this route is meant to specifically handle sign-out (cos the frontend
should specifically send delete request in the first place) why check the
http method in the first place? Besides I believe all checking/validation
should be handle by a middleware.
------------------------------
In pages/api/user/sign-up.js
<#20 (comment)>:
> +import { setUserSession } from '../../../helpers/auth-helpers';
+import Middleware from '../../../middlewares';
+import User from '../../../models/user/user.model';
+
+/**
+ * POST only
+ * Sign up a user, body must contain these two fields
+ *
+ * @param req
+ * @param req.body.email
+ * @param req.body.password
+ * @param res
+ * @returns {Promise<void>}
+ */
+async function SignUpHandler({ req, res }) {
+ if (req.method === 'POST') {
Nitpick
Same thing as the comment I left above.
------------------------------
In pages/api/projects/index.js
<#20 (comment)>:
> @@ -0,0 +1,40 @@
+import mongoose from 'mongoose';
+import Middleware from '../../../middlewares';
+
+const Project = mongoose.model('Project');
+/**
+ * @async
+ * @desc Api handles project list and search request
+ * @param {{ req: NextApiRequest, res: NextApiResponse }} prop
+ */
+const IndexProjectHandler = async ({ req, res }) => {
+ if (req.method === 'GET') {
+ const limit = req.query.limit || 0;
Nitpick
A better approach would be to abstract these into a helper file or a
middleware
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#20 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACC3JXNQTJJ5TXQGOWR4VLLRSSIY5ANCNFSM4NBFPPPQ>
.
|
Description
Developing the API backend for creating, updating and viewing of projects on voxnostra,
### How Has This Been Tested?
Tests have been written for the different endpoints
Any background context you want to provide?
### Relevant Task link (from the project board)
Card 37652856
Questions
closes Backend API implementation of Project CRUD #14