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

Katricia - Edges - API Muncher #42

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

Conversation

krsmith7
Copy link

@krsmith7 krsmith7 commented Nov 6, 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 read it through reading the documentation and sending a few requests in 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)? t is in lib because it does not fit the single responsibilities of Model, View, or Controller. We would have to make more than one API wrapper if we had more than Edamam.
Describe your API wrapper, the methods you created in it, and why you decided to create those methods/how they were helpful My wrapper has a self.search method and a self.find_details method. I created these because those are the functions that I want to be able to do with the data for the API.
What was an edge case or failure case test you wrote for your API Wrapper? What was a nominal case? Nominal case was for when the user search query matched results in the API.
How does VCR aid in testing an API? It creates a record of the HTTP request so that every same request does not repeat the call to the API, but references the VCR data.
What is the Heroku URL of your deployed application? https://muncherama.herokuapp.com/

…hange url for show_details from search url to show url
@droberts-sea
Copy link

API Muncher

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene yes
Comprehension questions yes
General
Rails fundamentals (RESTful routing, use of named paths) see inline
Semantic HTML yes - great work
Errors are reported to the user some - A search with no hits results in a nice message, but mangling the URI on the show page results in an exception rather than a polite error message.
API Wrapper to handle the API requests yes
Controller testing some - see inline
Lib testing some - see inline
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 yes
The VCR cassettes do not contain the API key yes
External Resources
Link to deployed app on Heroku yes
Overall Good work overall. This app is attractive and functional, and it's clear that the core learning goals around working with an API and mocking an external resource were met. I would like to see more test coverage, and there are a few inline things that could be cleaned up, but in general I'm quite happy with this submission. Keep up the hard work.

# get 'recipes/index', to: 'recipes#index', as: 'index'
# get 'recipe', to: 'recipes#show', as: 'show'

resources :recipes

Choose a reason for hiding this comment

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

Do you need all 7 RESTful routes here?

# without [0] gave "no implicit conversion of string into integar" error. why need index if only one?
return recipe
end
end

Choose a reason for hiding this comment

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

This will return nil if the API didn't send back a result, but your controller isn't set up to handle this case.


it "can get list of recipes for user search term" do
VCR.use_cassette('recipes') do
get recipes_path, params: { q: 'lemongrass'}

Choose a reason for hiding this comment

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

What if the search yields no results?

You should also be testing the show action here (success and failure cases)

describe "find details" do
it "can get the details of a single recipe" do
VCR.use_cassette("recipes") do
query = "ginger"

Choose a reason for hiding this comment

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

These are a good start, but you should have a failure case for both methods.

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