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

Karinna Iniguez - API Muncher - Octos #22

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

Conversation

karinnainiguez
Copy link

@karinnainiguez karinnainiguez commented May 7, 2018

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 through the documentation in Edamam and once I had an idea of the requirements in a query, I tested a few different things in Postman.
Describe your API Wrapper. How did you decide on the methods you created? The Edamam api has two different ways of searching. Either with a query, or with the recipe id. So, I made a method for searching with both of those methods. My index controller action uses the method .search(query) to search based on a word or phrase. My show controller action uses the .singlesearch(id) method to search for one specific recipe.
Describe an edge case or failure case test you wrote for your API Wrapper. I wrote a test for the case that the search does not return any "hits". In that case, I made the business decision to have it return nil instead of a search.
Explain how VCR aids in testing an API. When using an API, we do not want to rely on the API being available for testing. A way to reduce the number of requests sent to the API, and allow for offline testing, is to record previous responses from the API. VCR does that by recording the response it gets from the API in a 'cassette'. When the tests are run again, if the search query is the same as it previously was, it will not have to go out to the API< it can use the recorded version of the response.
What is the Heroku URL of your deployed application? https://yummmmster.herokuapp.com/
Provide a link to the Trello board you used https://trello.com/b/7MnmzK3D/api-muncher

@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) yes
Semantic HTML yes
Errors are reported to the user no - when I mangle the recipe URI, I get an exception rather than a polite error message
API Wrapper to handle the API requests yes
Controller testing yes
Lib testing yes
Search Functionality yes
List Functionality yes
Show individual item functionality (link to original recipe opens in new tab) yes (doesn't pop out into new tab)
Styling
Responsive layout yes
List View shows 10 items at a time/pagination yes
The app is styled to create an attractive user interface yes - looks great!
API Features
The App attributes Edaman yes
The VCR casettes do not contain the API key yes
External Resources
Link to Trello Board yes
Link to deployed app on Heroku yes
Overall Great work overall! Your code is well-organized and easy to read, and it's clear the core learning goals of the assignment were met.

The one piece that's missing is error handling for the recipe show workflow - from top to bottom your app lacks a story on how to figure out the API call didn't work and report the error to the user. This impacts your views, controller, lib files and tests.

Other than that, I'm quite happy with this submission. Keep up the hard work!


root 'recipes#search'
get 'recipes', to: 'recipes#index', as: 'results'
get 'recipe/:id', to: 'recipes#show', as: '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?

@@ -0,0 +1,25 @@
<header>
<%= render partial: 'search_form' %>
</header>

Choose a reason for hiding this comment

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

Since you include the search form on every page, it might make sense to put it in application.html.erb.


<section class="search-form">
<h1><%= link_to "YUMMMMSTER", root_path %></h1>
<%= form_tag results_path, method: :get do %>

Choose a reason for hiding this comment

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

Good use of a view partial to DRY this up.

recipe = Recipe.new(response[0])
return recipe
else

Choose a reason for hiding this comment

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

You should probably have something in this else clause - return nil or throw an exception.


def show
@recipe = EdamamApiWrapper.single_search(params[:id])
end

Choose a reason for hiding this comment

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

What if the search fails? Your page should probably report the failure to the user.

it "responds with success for search with no results" do
VCR.use_cassette("recipes") do
query = "something that doesnt exist"
get results_path, params: {query: query}

Choose a reason for hiding this comment

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

Good job catching this error case!

Choose a reason for hiding this comment

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

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?

describe "show" do
it "responds with success" do
VCR.use_cassette("recipes") do
id = "67b2867627c159e4f5bbaf2431b3ccb6"

Choose a reason for hiding this comment

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

What if you give it a bogus URI?

it "returns single recipe if searching single" do
VCR.use_cassette("recipes") do
id = "67b2867627c159e4f5bbaf2431b3ccb6"
response = EdamamApiWrapper.single_search(id)

Choose a reason for hiding this comment

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

What happens for an invalid ID?

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