Modules separated #7
Conversation
Reviewer's Guide by SourceryThis pull request implements a significant restructuring of the project, primarily focusing on modularizing the codebase, updating authentication and user management, and adjusting the database setup. Key changes include:
These changes aim to improve the project's structure, simplify authentication, and modernize the database approach. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @muneebhashone - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| if (!isUserExist) { | ||
| throw new Error('User already exists'); |
There was a problem hiding this comment.
issue (bug_risk): Potential bug in user existence check
The logic here seems inverted. It's throwing an error when the user doesn't exist, which is the opposite of what we'd expect for a 'user already exists' check. Please review and correct this condition.
| @@ -1,3 +1,4 @@ | |||
| import validator from 'validator'; | |||
There was a problem hiding this comment.
issue (complexity): Consider streamlining the new code to reduce complexity.
The new code introduces some complexity that could be streamlined. Here are a few suggestions:
-
Dependencies: The addition of the
validatorlibrary increases the dependency footprint. Ensure it's necessary for the project scope. -
ID Schemas: Consider unifying
mongoIdSchemaandidSchemato avoid confusion and redundancy. A single schema that handles both MongoDB and numeric IDs could simplify usage. -
Password Validation: The
passwordValidationSchemafunction embeds specific logic directly. Abstracting this into a utility function could improve clarity and reusability. -
Readability: Inline use of
refineandtransformmethods can reduce readability. Simplifying these could make the code more accessible to others.
Overall, these changes could enhance maintainability and clarity.
| const user = await createUser({ ...rest, role: 'DEFAULT_USER' }, false); | ||
|
|
||
| return user; |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| const user = await createUser({ ...rest, role: 'DEFAULT_USER' }, false); | |
| return user; | |
| return await createUser({ ...rest, role: 'DEFAULT_USER' }, false); | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
| const token = await signToken(jwtPayload); | ||
|
|
||
| return token; |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| const token = await signToken(jwtPayload); | |
| return token; | |
| return await signToken(jwtPayload); | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
| const newUser = await createUser({ | ||
| email, | ||
| username: name, | ||
| avatar: picture, | ||
| role: ROLE_ENUM.DEFAULT_USER, | ||
| password: generateRandomNumbers(4), | ||
| socialAccount: [ | ||
| { | ||
| refreshToken: refresh_token, | ||
| tokenExpiry: new Date(Date.now() + expires_in * 1000), | ||
| accountType: SOCIAL_ACCOUNT_ENUM.GOOGLE, | ||
| accessToken: access_token, | ||
| accountID: id, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| return newUser; |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| const newUser = await createUser({ | |
| email, | |
| username: name, | |
| avatar: picture, | |
| role: ROLE_ENUM.DEFAULT_USER, | |
| password: generateRandomNumbers(4), | |
| socialAccount: [ | |
| { | |
| refreshToken: refresh_token, | |
| tokenExpiry: new Date(Date.now() + expires_in * 1000), | |
| accountType: SOCIAL_ACCOUNT_ENUM.GOOGLE, | |
| accessToken: access_token, | |
| accountID: id, | |
| }, | |
| ], | |
| }); | |
| return newUser; | |
| return await createUser({ | |
| email, | |
| username: name, | |
| avatar: picture, | |
| role: ROLE_ENUM.DEFAULT_USER, | |
| password: generateRandomNumbers(4), | |
| socialAccount: [ | |
| { | |
| refreshToken: refresh_token, | |
| tokenExpiry: new Date(Date.now() + expires_in * 1000), | |
| accountType: SOCIAL_ACCOUNT_ENUM.GOOGLE, | |
| accessToken: access_token, | |
| accountID: id, | |
| }, | |
| ], | |
| }); | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
| const updatedUser = await updateUser(user._id, { | ||
| socialAccount: [ | ||
| { | ||
| refreshToken: refresh_token, | ||
| tokenExpiry: new Date(Date.now() + expires_in * 1000), | ||
| accountType: SOCIAL_ACCOUNT_ENUM.GOOGLE, | ||
| accessToken: access_token, | ||
| accountID: id, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| return updatedUser; |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| const updatedUser = await updateUser(user._id, { | |
| socialAccount: [ | |
| { | |
| refreshToken: refresh_token, | |
| tokenExpiry: new Date(Date.now() + expires_in * 1000), | |
| accountType: SOCIAL_ACCOUNT_ENUM.GOOGLE, | |
| accessToken: access_token, | |
| accountID: id, | |
| }, | |
| ], | |
| }); | |
| return updatedUser; | |
| return await updateUser(user._id, { | |
| socialAccount: [ | |
| { | |
| refreshToken: refresh_token, | |
| tokenExpiry: new Date(Date.now() + expires_in * 1000), | |
| accountType: SOCIAL_ACCOUNT_ENUM.GOOGLE, | |
| accessToken: access_token, | |
| accountID: id, | |
| }, | |
| ], | |
| }); | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
| }, | ||
| ); | ||
|
|
||
| if (!user) throw new Error('User not found'); |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!user) throw new Error('User not found'); | |
| if (!user) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
Summary by Sourcery
Separate the user and authentication logic into distinct modules, enhancing code organization and maintainability. Introduce new user management features and Google login support. Update the build and deployment configurations to accommodate these changes, including adding a PostgreSQL service. Remove obsolete code and templates to streamline the project.
New Features:
Enhancements:
Build:
Deployment:
Chores: