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

Rivera Leanne #47

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

Rivera Leanne #47

wants to merge 24 commits into from

Conversation

leannerivera
Copy link

API Muncher

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
How did you go about exploring the Edamam API, and how did you try querying the API? using postman and printing a lot of the data. also read docs
What is an API Wrapper? Why is it in lib? How would your project change if you needed to interact with more than one API (aka more than just the Edamam API)? it is a 'faux-model' that we create to reduce the impact of any API changes to our code. I think if more than one API was needed, you can just create a new lib file and call it like the first.
Describe your API wrapper, the methods you created in it, and why you decided to create those methods/how they were helpful the wrapper gets all the info from the api and then parses the data. it also requests data from the api using a url formula.
What was an edge case or failure case test you wrote for your API Wrapper? What was a nominal case? empty search queries, invalid single recipe search query are edge cases. returning back results of search query is a nominal.
How does VCR aid in testing an API? 'records' information from the API the first go-around so that the API 'cost' is not increased while testing.
What is the Heroku URL of your deployed application? https://chomp-crunch-munch.herokuapp.com/

@dHelmgren
Copy link

API Muncher

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good
Comprehension questions good
General
Rails fundamentals (RESTful routing, use of named paths) Yesish, see comments
Semantic HTML Good
Errors are reported to the user Could be better, see comments
API Wrapper to handle the API requests yes
Controller testing could be more robust, see comments
Lib testing TOP NOTCH
Search Functionality yes
List Functionality yes
Show individual item functionality yes
Styling
List view shows 10 items at a time and/or has pagination Good
Attractive and usable UI This is in the running for my favorite style on this project! Nice work!
API Features
The App attributes Edamam Yes, except for recipe detail page.
The VCR cassettes do not contain the API key No.
External Resources
Link to deployed app on Heroku Yes
Overall This site is really beautiful! You hit most of the learning goals, but the lack of good error handling is really what got you. Don't forget to try and break your site live!

get "recipes/", to: "recipes#index", as: "recipes_index"
get 'recipes/:uri/view', to: "recipes#show", as: "recipe_show"

get '/search', to: 'search#new', as: "search_recipes" #where searches are routed to

Choose a reason for hiding this comment

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

I'm not sure that new is a good name for this route/action. You're not really creating a new recipe, you're doing a search.

Rails.application.routes.draw do
get root "search#new"
get "recipes/", to: "recipes#index", as: "recipes_index"
get 'recipes/:uri/view', to: "recipes#show", as: "recipe_show"

Choose a reason for hiding this comment

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

Could you use resources for the index and show pages?

#example url search: "https://api.edamam.com/search?q=chicken&app_id=${YOUR_APP_ID}&app_key=${YOUR_APP_KEY}&from=0&to=3&calories=591-722&health=alcohol-free"

BASE_URL = "https://api.edamam.com/search" #what query params are attached to (?q for eng, ?r for nums)
REQUEST_URI = URI::encode("http://www.edamam.com/ontologies/edamam.owl#recipe_")

Choose a reason for hiding this comment

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

Good work distinguishing these elements from one another.

api_params["recipe"]["healthLabels"],
api_params["recipe"]["calories"],
api_params["recipe"]["totalTime"],
api_params["recipe"]["url"],

Choose a reason for hiding this comment

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

Could you dry this code by removing the recipe wrapper before passing the data into single_recipe?

before_action :find_query

def index
@recipes = EdamamApiWrapper.search_recipes(find_query).paginate(page: params[:page], per_page: 10)

Choose a reason for hiding this comment

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

You've written your API wrapper to raise an error if the API call fails, but you're not looking for an error here. You should wrap this call in a begin/rescue.

This causes your pages to break on bogus data, rather than redirect.

@recipe = EdamamApiWrapper.recipe_contents(params[:uri])

if @recipe.nil? || @recipe == false
redirect_to root_path, status: :not_found

Choose a reason for hiding this comment

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

Your redirect doesn't complete, possibly because you're explicitly setting the status code here.

require 'httparty'

class EdamamApiWrapper
#example url search: "https://api.edamam.com/search?q=chicken&app_id=${YOUR_APP_ID}&app_key=${YOUR_APP_KEY}&from=0&to=3&calories=591-722&health=alcohol-free"

Choose a reason for hiding this comment

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

Nice work not giving away your app key, and keeping handy data nearby. Others goofed on this.

http_interactions:
- request:
method: get
uri: https://api.edamam.com/search?app_id=73a32603&app_key=fc79540af556f65253707c3c2b00862e&r=http://www.edamam.com/ontologies/edamam.owl%23recipe_a53ef6c8495adcb9f2859b1e5d99e9ba

Choose a reason for hiding this comment

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

You didn't hide your app key from the cassettes here. :'(


describe "index" do

it "should get index" do

Choose a reason for hiding this comment

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

There are some interesting cases you're not covering here:

  • What happens if no search term is provided?
  • What if the search returns no results?

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