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

LJ API Muncher #24

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

LJ API Muncher #24

wants to merge 8 commits into from

Conversation

elle-johnnie
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? I read through the documentation and tested it out with Postman
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)? An API Wrapper stands in place of a rails model, it creates ruby objects based off of API response json and enables the controller to collect the object data to pass it to the view.
Describe your API wrapper, the methods you created in it, and why you decided to create those methods/how they were helpful I have 2 methods in my API wrapper one to build a list of recipes based on the search params and another to query edamam for a specific recipe and it's information.
What was an edge case or failure case test you wrote for your API Wrapper? What was a nominal case? An edge case I tested was where invalid input was put into the search bar.
How does VCR aid in testing an API? The VCR records any new API requests and utilizes them for testing later, this minimizes the total requests made throughout testing.
What is the Heroku URL of your deployed application? https://ljs-munchies-app.herokuapp.com/

@dHelmgren
Copy link

API Muncher

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Not great, you should be committing smaller chunks if you can!
Comprehension questions I want to adjust your understanding of the API wrapper: It's not a replacement for a model, it's more like a foreign language interpreter. It takes data coming in from other sites and translates it into something you can use, rather than having everybody (each piece of your project) learn how to talk to the API.
General
Rails fundamentals (RESTful routing, use of named paths) yes, see comments
Semantic HTML No, your index page is divs all the way down!
Errors are reported to the user No, see comments on routes.rb
API Wrapper to handle the API requests yes
Controller testing
Lib testing
Search Functionality yes
List Functionality yes
Show individual item functionality yes
Styling
List view shows 10 items at a time and/or has pagination yes
Attractive and usable UI Yes
API Features
The App attributes Edamam Not on the index page.
The VCR cassettes do not contain the API key no. :'(
External Resources
Link to deployed app on Heroku
Overall Good work on the project! It hits the majority of the requirements, but you haven't tested the edge cases of you site the way I would hope.

Rails.application.routes.draw do
root "recipes#index"
get 'recipes/index', to: "recipes#index", as: "list_recipes"
get 'recipes/show/:find', to: "recipes#show", as: "show_recipe"

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?

api_params["ingredientLines"],
api_params["healthLabels"],
api_params["cautions"]
)

Choose a reason for hiding this comment

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

Is there a way to merge show_recipe and create_recipe?

def show
if params[:find]
@find = params[:find]
@recipe = EdamamApiWrapper.find_recipe(@find)

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.

Not doing so affects your ability to catch fail states and properly redirect the user.

http_interactions:
- request:
method: get
uri: https://api.edamam.com/search?app_id=<MUNCHER_TOKEN>&app_key=04c236eb15ab0bae3dd07c08766aa823&r=http://www.edamam.com/ontologies/edamam.owl%23recipe_7bf4a371c6884d809682a72808da7dc2

Choose a reason for hiding this comment

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

nooooooo app keeeeeyyyyyyy

it "should get home" do
# make test to
get root_path
value(response).must_be :successful?

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?


it 'can find recipe given a valid path' do
VCR.use_cassette('find_valid') do
find = "http://www.edamam.com/ontologies/edamam.owl#recipe_7bf4a371c6884d809682a72808da7dc2"

Choose a reason for hiding this comment

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

What if they give you a bad recipe?

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