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

Arnau's MongoDB API #485

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Arnau's MongoDB API #485

wants to merge 3 commits into from

Conversation

vidalhuix
Copy link

Render link

here

Solo

[github-vidalhuix]

Copy link

@AntonellaMorittu AntonellaMorittu left a comment

Choose a reason for hiding this comment

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

Hey Arnau, your code is well-written and covers the essential aspects of building a RESTful API, I see no issues in the code but I couldn't test all the endpoints, I gave an internal error back after trying several times. I can't see from the code how was the DB populated, did you test it and did you manage to get the right responses back?Regardless, your implementation almost meets the basic requirements: fix the /books/:id endpoint and you passed! 🥂

mongoose.connect(mongoUrl);
//This will connect us to the Data Base
const mongoUrl = process.env.MONGO_URL || "mongodb://localhost/books";
mongoose.connect(mongoUrl, { useNewUrlParser: true, useUnifiedTopology: true });

Choose a reason for hiding this comment

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

Hey! did you know that with new Mongoose version you could just write mongoose.connect(mongoUrl)? 👍

Copy link
Author

Choose a reason for hiding this comment

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

No i didn't, thank's! I feel like I have to improve the documentation part, I'm still green on looking for it.

mongoose.Promise = Promise;

//Here we create a Book modell
const Book = mongoose.model("Book", {
bookID: Number,

Choose a reason for hiding this comment

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

You can remove bookId from the Schema: Mongoose automatically assigns a unique _id field to each document, so you don't need to define the field in your schema, you can even remove it from the data object

app.get("/books/:id", async (req, res) => {
const { id } = req.params;
try {
const book = await Book.findOne({ bookID: id });

Choose a reason for hiding this comment

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

you're trying to find the _id value accessing it with bookID and that's why it's not working. It should look like:
const book = await Book.findOne({ _id: id });

Copy link
Author

Choose a reason for hiding this comment

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

The datajson cames with a "bookID" and I got confused, nice you noticed, now it's working:)

@vidalhuix
Copy link
Author

Thank's for the code review Antonella! The other end point that wasn't working is books/rating, I just copied and pasted this hard code without understanding it.
Nice to hear that I passed😊

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