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

Jocelyn Gonzalez -- Carets #18

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

Jocelyn Gonzalez -- Carets #18

wants to merge 28 commits into from

Conversation

jocegonz
Copy link

@jocegonz jocegonz commented Nov 6, 2017

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 from Edamam and query results via Postman and my Chrome browser.
Describe your API Wrapper. How did you decide on the methods you created? My API wrapper has two methods that dictate what information needs to be accessed from the API's database. One method searches for a list of food matches, the other is to show a specific recipe from the collection of search results. I determined my methods based on what the API's documentation gave me access to.
Describe an edge case or failure case test you wrote for your API Wrapper. I tested if the wrapper would return an empty array for a bad uri.
Explain how VCR aids in testing an API. VCRs record responses from the API without having to pull information from the API every time just to test. This saves money and makes you less dependent on the API for tests in case the API went down.
What is the Heroku URL of your deployed application? https://munchsquad.herokuapp.com/

@CheezItMan
Copy link

API Muncher

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good number of commits, but some of your commit messages are a bit... odd. Three httparty commit?
Comprehension questions Check
General
Rails fundamentals (RESTful routing, use of named paths) Check
Semantic HTML Good semantic HTML
Errors are reported to the user Check
API Wrapper to handle the API requests Check
Controller testing Check, but no negative tests.
Lib testing Missing a test for your recipe class.
Search Functionality Works!
List Functionality Works!
Show individual item functionality (link to original recipe opens in new tab) BROKEN
Styling
Foundation Styling for responsive layout Nice attractive layout, you obviously spent a lot of time working on it.
List View shows 10 items at a time/pagination hi!!! pagination isn't working! what's up with that??? <-- That's my line!
The app is styled to create an attractive user interface Nicely done
API Features
The App attributes Edaman Check
The VCR casettes do not contain the API key Check
External Resources
Link to deployed app on Heroku Check
Overall Nicely done, you had trouble with pagination and didn't cover negative cases in testing, but overall pretty good.

end
return view_results
else
return []

Choose a reason for hiding this comment

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

Why would this return an empty array instead of nil? This is for an individual recipe not a collection.

end
end

it "can view the show page" do

Choose a reason for hiding this comment

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

What about negative case tests?

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

Choose a reason for hiding this comment

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

What about tests for your Recipe class?

<% @recipe.each do |r| %>
<section class="ingredients small-12 large-6 column">
<h2><%= r.label %></h2>
<p><em>Originally found in <%= link_to r.url %></em></p>

Choose a reason for hiding this comment

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

This link is broken!

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.

None yet

2 participants