Skip to content

API Muncher - Divya#34

Open
Naltrexone wants to merge 17 commits intoAda-C10:masterfrom
Naltrexone:master
Open

API Muncher - Divya#34
Naltrexone wants to merge 17 commits intoAda-C10:masterfrom
Naltrexone:master

Conversation

@Naltrexone
Copy link

@Naltrexone Naltrexone commented Nov 5, 2018

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 initially got the ID and keys after registering on the Edamam website. I then used that information to along with the 'q' parameter on Postman to retrieve data from the API
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 is a way to wrap the dependency around API in one place. It is probably in the lib due to convention, because it is business logic but doesn't really belong to a model. If I needed to interact with more than one API, I would build more wrapper methods to interact with the API.
Describe your API wrapper, the methods you created in it, and why you decided to create those methods/how they were helpful The wrapper consists of two methods - one that makes a query and returns all related recipes and another that makes a query and returns just one recipe. Because we are not storing the data, I had to depend on creating a temporary collection of recipe objects and then making a second request to the API to return just one recipe.
What was an edge case or failure case test you wrote for your API Wrapper? What was a nominal case? A failure case is any response aside from 200. If it's not 200, the wrapper will raise an error.
How does VCR aid in testing an API? Making requests to an API can be expensive. VCR allows you to record an HTTP response and test it to avoid making repeated requests to the actual API.
What is the Heroku URL of your deployed application? https://dm-muncher.herokuapp.com/

@CheezItMan
Copy link

API Muncher

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good number of commits and good commit messages
Comprehension questions You needed to be more specific on your edge cases
General
Rails fundamentals (RESTful routing, use of named paths) Check
Semantic HTML Check, maybe a bit of overused div elements.
Errors are reported to the user You're crashing the site when weird search terms are entered. You do give a message when the id for the show page is invalid.
API Wrapper to handle the API requests Check
Controller testing Check, but better testing on the search functionality would be better, see my comments.
Lib testing MISSING you are missing wrapper tests
Search Functionality Check
List Functionality Check
Show individual item functionality Check
Styling
List view shows 10 items at a time and/or has pagination MISSING
Attractive and usable UI The styling could definitely use some improvement, but this isn't a big concern. The page isn't responsive.
API Features
The App attributes Edamam Attributed without image
The VCR cassettes do not contain the API key Check
External Resources
Link to deployed app on Heroku MISSING
Overall You hit the majority of the learning goals, but the site is definitely unpolished. Check my comments for things to improve. The highest priority would be fixing wrapper tests.

def self.search_recipes(query)
url = BASE_URL + "?q=#{query}" + "&app_id=#{ID}" + "&app_key=#{KEY}" + "&from=0" + "&to=50"
response = HTTParty.get(url)
raise_on_error(response)

Choose a reason for hiding this comment

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

If you're going to raise an error here, your controller would need to rescue the error.

it "succeeds with illogical input" do
VCR.use_cassette('recipes') do

get recipes_path, params: {query: "qwerty"}

Choose a reason for hiding this comment

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

You should also test with nonsense like $%^ or nil

@@ -0,0 +1,82 @@
require 'test_helper'

Choose a reason for hiding this comment

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

Your edamam_wrapper_test is testing Recipe instead of the wrapper.

Copy link

@CheezItMan CheezItMan left a comment

Choose a reason for hiding this comment

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

Better, I can still crash your heroku instance by typing *&^ in the search bar.

gem 'will_paginate', '~> 3.1.6'
gem 'httparty'

gem 'kaminari'

Choose a reason for hiding this comment

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

Why both gems?


it "raises StandardError for a bad recipe id" do
VCR.use_cassette('recipes') do
proc { EdamamWrapper.get_recipe(0000) }.must_raise

Choose a reason for hiding this comment

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

You should use expect instead of proc, that behavior is old-minitest and will be depreciated in the next version.

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