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

Brandy Austin - API Muncher - Octos #42

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

Conversation

brandyaustinseattle
Copy link

@brandyaustinseattle brandyaustinseattle commented May 7, 2018

API Muncher

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
How did you go about exploring the Edamam API, how did you try querying the API? I read the documentation than tested calls in Postman.
Describe your API Wrapper. How did you decide on the methods you created? My API Wrapper gets the group of recipes based on the search term and the the recipe details for a specific recipe. I also used helper methods to format some of the resulting input before creating instances of recipe.
Describe an edge case or failure case test you wrote for your API Wrapper. I wrote a test to ensure that it dave an empty list when someone enter a space for their search term.
Explain how VCR aids in testing an API. VCR makes it so you don't have to repeatedly call the API which could be costly.
What is the Heroku URL of your deployed application? https://the-kitchen.herokuapp.com/
Provide a link to the Trello board you used https://trello.com/b/fhg2wlGL/muncher-api

@droberts-sea
Copy link

API Muncher

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene yes
Comprehension questions yes
General
Rails fundamentals (RESTful routing, use of named paths) yes
Semantic HTML yes
Errors are reported to the user yes - would like a little more detail than "something went wrong"
API Wrapper to handle the API requests yes
Controller testing yes
Lib testing yes
Search Functionality yes
List Functionality ues
Show individual item functionality (link to original recipe opens in new tab) yes (no popout link)
Styling
Responsive layout no - does not adjust to smaller screen size
List View shows 10 items at a time/pagination yes
The app is styled to create an attractive user interface yes - great work!
API Features
The App attributes Edaman yes
The VCR casettes do not contain the API key yes
External Resources
Link to Trello Board yes
Link to deployed app on Heroku yes
Overall Great job overall! Your code is well-organized and easy to read, and it's clear the learning goals of this assignment were met. I've left a few nitpicks below, but in general I am quite happy with this submission. Keep up the hard work!


get '/recipes', to: 'recipes#index', as: :index
get '/search', to: 'recipes#search', as: :search

Choose a reason for hiding this comment

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

Why not use resources for line 8?

recipe = MuncherApiWrapper.show_recipe(id)

if recipe.nil?
flash[:notice] = "Something went wrong. Please enter a new search."

Choose a reason for hiding this comment

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

This is where using an exception to report errors comes in handy - an exception contains a lot more useful info than a nil.

# #recipe_ always has to be last # delimited item in uri otherwise this breaks
label_uri_image[:id] = r["recipe"]["uri"].split("#recipe_")[-1]
label_uri_image[:image] = r["recipe"]["image"]

Choose a reason for hiding this comment

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

Good work sorting out the recipe URI!

label_uri_image = {}
label_uri_image[:label] = r["recipe"]["label"]
# #recipe_ always has to be last # delimited item in uri otherwise this breaks
label_uri_image[:id] = r["recipe"]["uri"].split("#recipe_")[-1]

Choose a reason for hiding this comment

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

You should be creating an instance of your Recipe class here. That would let the controller / views treat this like any other model, and isolate the knowledge of how the API sends data back in one place.

it "can't get the recipe's info and redirects with invalid id" do
VCR.use_cassette("chicken") do
recipe_list = MuncherApiWrapper.list_recipes("chicken")
id = recipe_list.first[:id] + "abc"

Choose a reason for hiding this comment

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

Good work nailing this error case. It might also be worthwhile to add some tests around the paging parameters:

  • What happens if they're absent?
  • Do you get different results when they change?
  • What if you send a bogus value, like a negative page number?

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