-
Notifications
You must be signed in to change notification settings - Fork 31
Mimmi - api #16
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?
Mimmi - api #16
Conversation
…a into the database
… to caclulate max pages in the frontend
…tch a users liked messages
MalLunBar
left a comment
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.
Amazing! Everything is super clean and very clear. I have no complaints or ideas for changes 😍
Great job!
osckli990
left a comment
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.
Didn't find much to complain about, the code is clear, readable, nicely sectioned, while being RESTful and you have validation. It's probably correct but wanted to point out matching status code with the error type like: 400 (bad request, client input) or 500 (server error).
Nice!
| // create a new user | ||
| const salt = bcrypt.genSaltSync() | ||
| const user = new User({ userName: userName.toLowerCase(), password: bcrypt.hashSync(password, salt) }) | ||
| user.save() |
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.
I think you can write this one as "await user.save()", because of the risk that the response goes out to mongo before it's saved
| } else { | ||
| res.status(401).json({ | ||
| success: false, | ||
| message: "Invalid password", |
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.
Mr. GPT pointed this one out, he had some issue with revealing that the username exists on the site, for security reasons, so it could be written as "invalid credentials" instead. idk, up to you.
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.
This is a good approach. For bad ppl trying to hack an account it is very useful to know if it is the username or the password that is wrong. So better to send msg with invalid credentials in both cases. 💡
JennieDalgren
left a comment
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 job with this project. There are a few things that seems buggy. For example when I try to send a thought as not logged in I get this error message:

Maybe not the most UX friendly.
Also, when clicking on my thoughts, I get 404

.
Other than that! Well structured code, great job with the stretch goals too. I added some small comments in the code as well.
Ping me when you have checked the issues that I pointed out. However, you are approved and with VG
| // seeding data to database | ||
| // if (process.env.RESET_DATABASE) { | ||
| // const seedDatabase = async () => { | ||
| // await Thought.deleteMany({}) | ||
| // data.forEach(thought => { | ||
| // new Thought(thought).save() | ||
| // }) | ||
| // } | ||
| // seedDatabase() | ||
| // } | ||
|
|
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.
If you don't need this it is okay to remove it. If you keep it for demo purpose you can add that in the comment
https://think-happy-api.onrender.com