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

Pipes - angela - api muncher #26

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

Conversation

awilson2017
Copy link

@awilson2017 awilson2017 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 used Edamam docs to figure out the structure of the API call and then Postman to parse the JSON
Describe your API Wrapper. How did you decide on the methods you created? The wrapper has methods list the recipes and then find a recipe. These allow for one location that parses the JSON in case data structure changes in the future we can just change it in one place. How I decided, I looked at the Edamam docs and saw there were two applicable queries (q= and r=) and build the methods based on that. In addition I used slack-api as an example.
Describe an edge case or failure case test you wrote for your API Wrapper. I gave the url a bogus API token and raised an ApiError when the request failed.
Explain how VCR aids in testing an API. It records the JSON data in a yml file. By recording it we can use it instead of making costly API calls for each test run.
What is the Heroku URL of your deployed application? https://fathomless-hollows-55266.herokuapp.com/

… Recipe class. still working on associated tests.
@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 some - You've got the flash reporting section in application.html.erb set up nicely, but I was not able to get it to actually display a message for me. Make sure you're detecting error cases accurately, and actually reporting problems to the user.
API Wrapper to handle the API requests yes
Controller testing Good start, especially around show, but there's still some pieces missing here. See inline.
Lib testing yes
Search Functionality yes
List Functionality yes
Show individual item functionality (link to original recipe opens in new tab) yes
Styling
Foundation Styling for responsive layout yes
List View shows 10 items at a time/pagination yes
The app is styled to create an attractive user interface yes
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 Good job overall! It seems like you've got a solid grasp on the core learning goals for this assignment.

One area worth focusing on going forward is error handling, and making sure that errors are plumbed all the way from the source (in this case an API call) to being displayed to a user. There were a few cases where there was a disconnect, and valuable information ended up being lost. Fixing this means knowing whether a method throws an exception, returns a sentinel value like nil, or returns something like an empty array, and being able to handle it appropriately wherever the method is called. This sort of information also manifests when coming up with test cases, which is a big portion of why we study testing in this course.

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

def index
@query = params[:search]

if params[:search]

Choose a reason for hiding this comment

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

Because the empty string is truthy in Ruby, even if you search for nothing you won't end up in the error handling here. Instead, you might have something like if params[:search] && params[:search] != "".


def new

end

Choose a reason for hiding this comment

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

I don't think the new action is used in your app.


unless @recipe
head :not_found
end

Choose a reason for hiding this comment

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

I like this idea, but I do not believe the code will ever get here. Your API wrapper throws an error if the recipe is not found, but here you check for a nil response. You have two options to make the connection:

  1. Use a begin/rescue block here to catch the exception
  2. Instead of throwing an error, return nil from ApiWrapper.find_recipe if the recipe can't be found.



# parse the JSON data in order to get recipe details for the show page
if data.empty?

Choose a reason for hiding this comment

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

It looks like you're missing a call to check_status here. If the API returns an error like not_authed (as opposed to no results), as is this method won't be able to detect it.

# #
#
# get recipe_path params: { search: "chicken", url: "http://www.edamam.com/ontologies/edamam.owl%23recipe_637913ec61d9da69eb451818c3293df2" + "fake-url"}
# must_respond_with :not_found

Choose a reason for hiding this comment

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

I think this test is failing because the controller action is handling the error incorrectly. See comments above.

begin # something which might raise an exception
origin_token = ApiWrapper::TOKEN
ApiWrapper::TOKEN = "bogus_token"
VCR.use_cassette("recipes") do

Choose a reason for hiding this comment

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

I like this way of testing an invalid token - much simpler than what we did in class.

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