-
Notifications
You must be signed in to change notification settings - Fork 30
RAT API / server to HAppy HToughts #23
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
base: master
Are you sure you want to change the base?
Conversation
… lektionen igår, enklare! Ändrade test-scriptet så det använden babel.node
… Patch unLikeThought is done and tested
…d remove thought to HappyRouter
…ctoring to have controller-folder. brb.
…the Auth middlewere to post and login
This reverts commit 53f5962.
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.
Hey Casandra, so nice to finally review your code! ⭐️ Here's my thoughts about it:
General feedback
Overall, the code is well-structured, consistent, and easy to follow. The use of async/await
is consistent, the REST endpoints are clearly separated, and the error handling is well-implemented. Variable naming is clear, and the use of Mongoose’s populate()
is a good choice for retrieving related user information.
Identified issues and improvement suggestions
1. Message length validation inconsistency
- In
addThought
(Thought controller), the validation states that the message length must be between 4 and 140 characters. - In the Thought model, the
message
field has aminlength
of 5, which means the two validations are not aligned. - Recommendation: Align the validation rules in both the controller and the model to avoid confusion and ensure consistent behavior.
2. Missing validation in updateThought
- The
updateThought
function does not validate themessage
length at all. This allows updating a thought with a message that would not pass the creation validation. - Recommendation: Add the same length validation as in addThought to maintain data integrity.
3. createdBy
field set to required: false
- In the Thought model, the
createdBy
field is optional. This allows the creation of posts without a logged-in user. - Recommendation: If your application requires authentication for posting thoughts, set
required: true
for thecreatedBy
field. If anonymous posting is allowed, ensure there is proper handling in the API to manage such posts.
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.
Really good job with this project. Your code is well strcutred and easy to follow. The addons in the frontend is also super nice. I like the icons for edit and delete and also the descripiton of the app it self. You have thought about the small things as well as well structured code and testing!
Great work ⭐️
@@ -0,0 +1,124 @@ | |||
import assert from 'assert'; |
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.
Nice! ⭐
|
||
const happyRouter = express.Router(); | ||
|
||
//KOMIHÅG ATT URL börjar med /api/thoughts |
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.
stick to english in your comments, it helps the reviewer to understand ;)
type: mongoose.Schema.Types.ObjectId, | ||
ref: 'User', | ||
required: false, |
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.
great connection to the user collection here
try { | ||
const newThought = await Thought.create({ | ||
message, | ||
createdBy: req.user._id, | ||
// likes and createdAt will be set by defaults in the model | ||
}); | ||
|
||
const populatedThought = await newThought.populate( | ||
'createdBy', | ||
'_id email' | ||
); |
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.
⭐
Please include your Render link here.