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

Kayla Ecker - Caret #36

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

Kayla Ecker - Caret #36

wants to merge 24 commits into from

Conversation

kaylaecker
Copy link

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?
Describe your API Wrapper. How did you decide on the methods you created?
Describe an edge case or failure case test you wrote for your API Wrapper.
Explain how VCR aids in testing an API.
What is the Heroku URL of your deployed application? https://desolate-scrubland-60709.herokuapp.com/

@CheezItMan
Copy link

API Muncher

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good commit messages, more granular commits would be better.
Comprehension questions NOOOOOOOOOOOOOOOO!!
General
Rails fundamentals (RESTful routing, use of named paths) Check
Semantic HTML overall pretty good
Errors are reported to the user Check, nice!
API Wrapper to handle the API requests Check
Controller testing Yes, but no show action tests
Lib testing Yes, but your negative cases are failing.
Search Functionality Check
List Functionality Check
Show individual item functionality (link to original recipe opens in new tab) Check
Styling
Foundation Styling for responsive layout The whole site is done in 1-column setups, so it's not really responsive.
List View shows 10 items at a time/pagination Check
The app is styled to create an attractive user interface Check
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 Deployed, but it doesn't work!
Overall You need to look at the comprehension questions, and work on styling, as well as practice deploying to Heroku. See my notes in the code about issues with your tests and wrapper. You did hit most of the requirements.

url = BASE_URL + "q=" + query + ID_AND_KEY + "&to=50"
data = HTTParty.get(url)
recipes = []
unless data["hits"].empty?

Choose a reason for hiding this comment

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

You should have this check read
unless data["hits"].nil? || data["hits"].empty?

<%= flash[:message] %>
</h3>
<%end%>
</div>

Choose a reason for hiding this comment

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

Need to fix alignment a bit, plus unnecessary div elements.

<div class=name>
<%= link_to recipe.name, recipe_path(recipe.name, recipe_id: recipe.id)%>
</div>
<img src="<%= recipe.image %>", alt="<%= recipe.name %>"/>

Choose a reason for hiding this comment

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

making the image a link would be good as well.

data = HTTParty.get(url)
recipe = ""
unless data.empty?
recipe = self.create_recipe(data[0])

Choose a reason for hiding this comment

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

You should also check to see if data[0] has values

it "succeeds when there are recipes for search" do
query = "apple"
VCR.use_cassette("search") do
EdamamApiWrapper.search(query).empty?.must_equal false

Choose a reason for hiding this comment

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

This is too specific to the wrapper. Let the controller make the wrapper call, focus on using the controller.

must_respond_with :success
end
end

Choose a reason for hiding this comment

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

What about show recipe tests?

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.

3 participants