#7-User Authentication #16
Conversation
|
@mernxl fix the codacy errors |
|
The review from codacity is somewhat incorrect, well will raise an issue later. Clearly i destructured that object which is valid in JS. Well i will fix it now, raise a flag in codacity.com so we move on. |
- set content type for catchall error response
- workaround for JS Object destructuring inside an if block considered error by codacity
tanerochris
dismissed
their stale review
it was an error on my part, wrongly spelled a paramter
|
Well fixed it. 😅In case future such mistakes. |
There was a problem hiding this comment.
Critical
- The folder structure seems a bit cumbersome. This does not make it easy for other users to navigate through the project during development
- Many files aren't documented properly i.e. functions, classes & methods should carry at least the following docs:
/**
* @desc description of what the function does
* @param {type} param
*/-
All database related files or functions should be in a models folder i.e.
/modelsnot/libs -
Also, all frontend related files should be in a separate folder for easy identification cos as it is, everything seems to be not well structured.
-
Please attach screenshots of test results.
| index: true, | ||
| unique: true, | ||
| required: true, | ||
| validate: [/^(([^<>()[\]\\.,;:\s@"]+(\.[^<>()[\]\\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))$/, 'Must be a valid email address.'] |
There was a problem hiding this comment.
Nitpick
This makes the code a bit difficult to read. Why not abstract the data validation to another function or library like Joi
There was a problem hiding this comment.
Ok, I was to raise the issue of validation. A framework was supposed to be set before i worked on this task. That is to permit us carry out validations at the controller level.
There was a problem hiding this comment.
Can we leave validation currently at the database, validation at controller will be functionality update on another issue.
| * | ||
| * name, message, stack | ||
| */ | ||
| export class ErrorResponse extends Error { |
There was a problem hiding this comment.
Nitpick
I understand that this handles error messages for the API but if we can include all that in the response object in the controller, there won't be a need for this.
There was a problem hiding this comment.
@KaiserPhemi you mean like, i create an error method on the response object, like i did for .json to send json responses??
There was a problem hiding this comment.
@mernxl
Typically you'll send an API response like this:
return response.send({
message: "",
data,
})This class appears to only handle error message. What I'm saying is that, why create a class for that when I can easily send the error message as an API response like this:
return response
.status(500)
.send({
message: "Internal Server Error",
details,
});There was a problem hiding this comment.
Ok i see. Maybe i state the reasons for my doing that, you see if they are legit.
Although i know theres a down side of having to deal with the error generation and handling but the reasons for doing that are as follows.
- I wanted to reuse the error messages, status codes, so i don't have to always create new ones which may mismatch.
- Gives a general and continuous structure for all backend errors. Especially when it comes to validation errors.
|
@KaiserPhemi Thank you for your reviews. I guess the whole structure will need to be changes in another pull request then i rebase and add this changes to follow up a better structure. That was actually the structure i found so i had to flow with it. @tanerochris maybe we will have to look into the file structure, given you setup the initial one.
|
|
I noticed @tanerochris already took in the changes of this pull request in #20 pull. I guess this pull request will see a closure then, So @tanerochris will have to do the updates on this pull if thats ok. |
|
I think having an error response structure is appropriate, especially for
writing tests, also it prevents a scenario where other persons create
different error responses. In #20 I added some sort of adapter to the
ErrorResponse classes.
…On Mon, May 18, 2020 at 1:00 PM Rodrick Nfinyoh M ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In libs/api-errors.js
<#16 (comment)>:
> @@ -0,0 +1,42 @@
+/* eslint-disable max-classes-per-file */
+
+/**
+ * General Error Object, gets other props from Error which include
+ *
+ * name, message, stack
+ */
+export class ErrorResponse extends Error {
Ok i see. Maybe i state the reasons for my doing that, you see if they are
legit.
Although i know theres a down side of having to deal with the error
generation and handling but the reasons for doing that are as follows.
- I wanted to reuse the error messages, status codes, so i don't have
to always create new ones which may mismatch.
- Gives a general and continuous structure for all backend errors.
Especially when it comes to validation errors.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACC3JXPMVM6GB6PJLUQZWQDRSGHWBANCNFSM4M47L5YA>
.
--
My twitter @tanerochris
What I've built https://tchizer.com
Developer Circle Buea Lead facebook.com/groups/DevCBuea/
Whatsapp +237 678431904
|
OpenAPI 3.0 design Yaml and JSON files closes #6
Added instructions to run API endpoint documentation Added link to external docs close
- add project schema - add user schema
Description
Implementation of authentication, accessible through
/auth/{action}where action stands for the action to be carried out. These actions include;Other Notes
bcrypt.You can check up my comment on issue for more
Fixes #7
How Has This Been Tested?
I did minor testing with PostMan to see that the features. Still to in app tests
Checklist: