Skip to content

Milestone3 and Milestone3.5 - #67

Open
lallen15 wants to merge 6 commits into
hack4impact-calpoly:mainfrom
lallen15:main
Open

Milestone3 and Milestone3.5#67
lallen15 wants to merge 6 commits into
hack4impact-calpoly:mainfrom
lallen15:main

Conversation

@lallen15

@lallen15 lallen15 commented Nov 9, 2022

Copy link
Copy Markdown

Milestone: Milestone 3 and Milestone3.5

Developer: Lauren Allen

Pull Request Summary

To turn in milestone 3 and updated to turn in milestone 3.5

Pull Request Checklist

  • Code is neat, readable, and works
  • Comments are appropriate
  • The commit message follows our guidelines
  • The milestone number is specified
  • The developer name is specified
  • The summary is completed

Reviewer Checklist - PULL REQUEST REVIEWER ONLY

IMPORTANT: The rest of the sections in this checklist should only be filled out by authorized pull request reviewers. If you are the individual template contributor, do not fill out the rest of the fields or check the boxes.

NOTE: Milestones can only be completed when all boxes are checked.

  • The code is fully reviewed
  • Meaningful feedback is given
  • Comment that you have reviewed the code
  • This box is checked

@lallen15 lallen15 changed the title Milestone3 Milestone3 and Milestone3.5 Nov 18, 2022
@sfwathen
sfwathen requested review from sfwathen and removed request for sfwathen December 13, 2022 04:24
@sfwathen

Copy link
Copy Markdown
Collaborator

Hi Lauren,
It looks like you pushed your node_modules file on your backend with your milestone 4 submission. Can you resubmit that with a gitignore so node_modules isn't included. Same as how we did it for the frontend in milestone 3. I can give you feedback once done.

@sfwathen sfwathen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hey Lauren,
Sorry for the late response, I reached out to you on slack but you don't seem to have seen it. Could I have you resubmit milestone 4 after testing all of your routes and making sure they work. I ran into a few errors in your backend code and was unable to get a response from your get recipe by name route and your two put routes. If you have trouble testing please do reach out, postman should be a serious help though.
Additionally, your milestone 3.5 is not working 100%, particularly with the external recipes from Ryan's api. I gave some suggestions on what's going wrong and highly suggest you look back at the milestone 3.5 doc to get that working correctly. That being said, I would prioritize milestone 4 and 5 beforehand. Getting the external recipes working is not technically a requirement of milestone 5 so I would prioritize getting everything working with 4 then 5 so that you can get closer to being put on a project team!
Let me know if you have any questions.

Thanks,
Sam

Comment thread backend/index.ts
Comment on lines +25 to +41
app.post("/recipe", async (req, res) => {
const { name, image, description, ingredients, instructions} = req.body
let recipe = new Recipe({
name,
image,
description,
ingredients,
instructions
})
try {
recipe = await recipe.save()
res.send(`Recipe for ${name} added to collection`)
} catch(error: any) {
res.status(500).send(error.message)
console.log(`error is ${error.message}`)
}
});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this can and should go in your routes/recipe.ts file

Comment thread backend/index.ts
Comment on lines +43 to +70
router.put("/:name/ingredient", async (req, res) => {
const name = req.params.name;
const ingredient = req.body.newIngredient;
const recipes = await Recipe.findOne({name: name});
if (recipes){
recipes.ingredients = [...recipes.ingredients, ingredient];
await recipes.save();
res.send("Ingredient added");
}
else{
res.send("Failed to add new ingredient");
}
});

router.put("/:name/instruction", async (req, res) => {
const name = req.params.name;
const instruction = req.body.newInstruction;
const recipes = await Recipe.findOne({name: name});
if (recipes){
recipes.instructions = [...recipes.instructions, instruction];
await recipes.save();
res.send("Instruction added");
}
else{
res.send("Failed to add new instruction");
}
});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

these can and should go in your routes/recipe.ts file

Comment thread backend/index.ts
const express = require("express"); // 1. includes Express
const app: Express = express(); // 2. initializes Express
app.use(express.json());
const mongoose = require('mongoose')

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

use the import syntax instead of require

Comment thread backend/index.ts

import recipeRoutes from "./routes/recipe";

app.use("/recipe", recipeRoutes)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

by adding "/recipe as the route here it will prepend it to all of the routes in the file. So if you put it here you should leave it off in all of the routes in the file. Otherwise you need to go to localhost:3001/recipe/recipe/... to access any of the routes, which is redundant

Comment thread backend/index.ts
const mongoose = require('mongoose')
const connection_url = 'mongodb+srv://newUser:newPassword@cluster0.66l23vd.mongodb.net/RecipesDB?retryWrites=true&w=majority'

import recipeRoutes from "./routes/recipe";

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

put all imports at the top of your file

}

function RecipePage() {
const [externalRecipes, setExternalRecipes] = useState<Recipe[]>(recipeData)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ideally, we don't want to fetch all of our recipes when we go to each individual recipe page, instead we want to only grab the one who's name is specified in the urlParams. This is why in the docs we have a useEffect with a conditional inside. We either want to make a call to the database if we are finding an external recipe, or search through our local data using .find if its local.

useEffect(() => {
fetch("https://localhost:3001/recipe")
.then((res) => res.json())
.then((data) => setExternalRecipes([...externalRecipes, ...data]))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

if we instead had a useState variable to hold the single recipe for the page, we could call that setRecipe function with the result of your call to the find recipe by name route you made on the backend.

let param = useParams()
console.log(param.name)

let recipe = externalRecipes.find((r) => r.name === param.name)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

we should do this inside of the conditional of the useEffect, as shown in the milestone3.5 docs. Because useEffect is asynchronous these lines of code are running before externalRecipes is updated, which is why the pages of your external recipes are throwing errors.

Comment on lines +32 to +38
const [allIngredients, setAllIngredients] = useState(
recipe ? recipe.ingredients : []
)

const [allInstructions, setAllInstructions] = useState(
recipe ? recipe.ingredients : []
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Always put all the useState declarations at the top of the function together.

Comment on lines +3 to +7
interface Recipe {
name: string
image: string
description: string
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

you may have noticed that you're used this interface or something similar in several places in your code. Instead of repeating all of that code, we can put this in a separate file, call it types.ts or something similar, then import this interface wherever you need it!

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