Skip to content
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

Crud #2

Open
merged 4 commits into
base: master
Choose a base branch
from
Apr 2, 2020
Open

Crud #2

merged 4 commits into from
Apr 2, 2020

Conversation

mj0730
Copy link
Contributor

@mj0730 mj0730 commented Apr 2, 2020

Pull request for CRUD API. Readme contains current info on how/what each route does

@mj0730 mj0730 merged commit 8fbb2cd into master Apr 2, 2020
Copy link

@AshbyGunter AshbyGunter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice. Clean and organized.
Notes on first API apply to latter ones as well, re: console statement.

Comment on lines +20 to +22
### CRUD API
To create a new user in the database:
send POST request to domain/api/users with the user name in the body of the request with the name field (name: username)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this require anything in the body, or does it only need the name in the header?

Comment on lines +13 to +19
const fakeUser = (userName) => {
let randomNumber = Math.floor(Math.random() * 100 + 1);
return {
name: userName,
imageUrl: `https://hackreactoramazonfrontendcapstone.s3-us-west-2.amazonaws.com/${randomNumber}.jpeg`
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth moving this out of index at some point.
A lot of places use a fairly blank default image for new users - having something like that mike make sense if a picture has be attached. Something like that could be stored locally and could improve time down the line.

Comment on lines +31 to +32
To delete all reviews for a property listing:
send DELETE request to domain/api/rentals/id where id is the number id of the listing

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like the focused use of README - didn't try to rewrite, just stayed on point with putting in what had to be put in.

Comment on lines +43 to +48
let userName = req.body.name;
User.insertMany(fakeUser(userName))
.then(data => console.log(`Inserted ${data} into database`))
.then(res.sendStatus(200))
.catch(err => console.log(`ERROR createing user: ${err}`));
})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clear, simple, and easy to follow.
Curious why insertMany over a single method like .save?
Recommend dropping the console log for success once you're confident this works. Could send a success message to client?
Also recommend changing the error entry to console.error
Nice use of backticks and string templates!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants