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

Ampers: Nora Peters #32

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

Ampers: Nora Peters #32

wants to merge 53 commits into from

Conversation

npeters5
Copy link

@npeters5 npeters5 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 the API docs carefully and tested out a few different requests (q vs. r params) using Postman.
Describe your API Wrapper. How did you decide on the methods you created? I created two methods in my RecipeApiWrapper: self.find_recipes to use the 'q' parameter based on user's search terms, to populate index view; and self.get_recipe to make another api request using the 'r' query parameter to look up a specific recipe based on the object's URI, and returning a new Recipe object to display in the show view.
Describe an edge case or failure case test you wrote for your API Wrapper. I wrote a test to check that find_recipes returned an empty array if there was no match for the search terms provided.
Explain how VCR aids in testing an API. Calls to APIs can be expensive, and if the API is down, we're down. VCR helps reduce this external dependency when testing.
What is the Heroku URL of your deployed application? https://api-nosher.herokuapp.com/
Provide a link to the Trello board you used https://trello.com/b/VyU1uMaR/api-muncher-nora

NOTE: I keep getting the error: "uninitialized constant Minitest::Rails::TestUnit (NameError)". I tried all of the things mentioned in Slack and tried troubleshooting a bunch on my own, but I still can't get my tests to run.

npeters5 added 30 commits May 2, 2018 07:46
…oning of this app but casually left out by Rails...cool.
@CheezItMan
Copy link

API Muncher

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good number of commits and good commit messages
Comprehension questions Check,
General
Rails fundamentals (RESTful routing, use of named paths) Check, but it would be better if the search results went to the path /search
Semantic HTML Check
Errors are reported to the user No errors reported when there are no search results. The show page does show a message when the URI is wrong.
API Wrapper to handle the API requests Check
Controller testing Lot of issues with controller testing, for one thing you're making calls to the wrapper instead of just using the controller in these tests. You shouldn't be accessing the wrapper directly here. See my inline notes.
Lib testing Lots of problems here. See my inline notes.
Search Functionality Check
List Functionality Check
Show individual item functionality (link to original recipe opens in new tab) Check
Styling
Responsive layout Responsive both on the index and show pages.
List View shows 10 items at a time/pagination Check
The app is styled to create an attractive user interface
API Features
The App attributes Edaman MISSING
The VCR cassettes do not contain the API key Check
External Resources
Link to Trello Board Check
Link to deployed app on Heroku Check
Overall You did well with the API, but you have some serious problems with your testing. Please review my notes and let me know where I can clarify. You won't have any more opportunities for Rails testing and like you to take some time and see if you can see where you went wrong and how to do the testing properly. I'm happy to go through it with you if needed. You hit the API goals for the project, and had trouble with the testing, but you did well with the most visible portions of the project. I hope you enjoyed Rails and learn to love JavaScript starting next week!

Copy link

@CheezItMan CheezItMan left a comment

Choose a reason for hiding this comment

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

The most obvious thing is you missed the "monkey" patch which we had to include from the SlackAPi project. That might explain your testing problems.


require "minitest/rails"
require "minitest/reporters" # for Colorized output

Choose a reason for hiding this comment

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

You need to have this in here:

# MonkeyPatch for This version of Rails :(
if ActionPack::VERSION::STRING >= "5.2.0"
  Minitest::Rails::TestUnit = Rails::TestUnit
end

@@ -0,0 +1,17 @@
<h3>Results for: "<%= @search_terms %>"</h3>
<section>
<div>

Choose a reason for hiding this comment

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

Not sure this div is really needed here.

@recipes_list = []
end

encoded_search_terms = URI.encode(search_terms)

Choose a reason for hiding this comment

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

There's no checking to ensure that search_terms is a string and it's non-empty.


def index
@search_terms = params[:search_terms]
@recipes = RecipeApiWrapper.find_recipes(@search_terms).paginate(page: params[:page], per_page: 10)

Choose a reason for hiding this comment

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

You need some sort of check to ensure that @search_terms is not null. The same applies to params[:page]

it "can get recipes index" do

VCR.use_cassette("recipes") do
get recipes_path

Choose a reason for hiding this comment

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

This test is failing because you're not setting a param here for this path.

      get recipes_path, params: {search_terms: query}

get recipes_path

query = "chicken"
RecipeApiWrapper.find_recipes(query)

Choose a reason for hiding this comment

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

Why are you making API Wrapper calls directly here? This should be only making controller calls.

get recipe_path("7bf4a371c6884d809682a72808da7dc2")

recipe_id = "7bf4a371c6884d809682a72808da7dc2"
RecipeApiWrapper.get_recipe(recipe_id)

Choose a reason for hiding this comment

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

Again, this is a controller test, not a test of the wrapper.

VCR.use_cassette("get_recipe") do
RecipeApiWrapper.find_recipes("chicken")
recipe_id = "7bf4a371c6884d809682a72808da7dc2"
@recipe = RecipeApiWrapper.find_recipe(recipe_id)

Choose a reason for hiding this comment

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

Should be:

@recipe = RecipeApiWrapper.get_recipe(recipe_id)


it "Returns empty array if there are no query matches" do
VCR.use_cassette("recipes2") do
query = 2429384725434.229384687

Choose a reason for hiding this comment

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

shouldn't this be in quotes?

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