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 Abinnet #38

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

Ampers Abinnet #38

wants to merge 33 commits into from

Conversation

Abiaina
Copy link

@Abiaina Abiaina 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 went to the documents page and looked for examples online using the api, also used postman a lot
Describe your API Wrapper. How did you decide on the methods you created? My wrapper creates a new instance of recipe and there is an individual wrapper for creating many recipes and creating a single recipe instance
Describe an edge case or failure case test you wrote for your API Wrapper. Update: I tested if the wrapper was given invalid query from params and if the page or from value was also invalid. I tested that the last item on a page is also made an instance of the recipe class. Past comment: I did not finish testing, but the edge cases I explored were trying to create a new instance of recipe with limited parameters. Also Making an invalid get request using invalid query and page number(from= value)
Explain how VCR aids in testing an API. It copies responses of the api as I make requests on them for testing and helps prevent me from over using the api and exceeding my limit (this happened a lot and I had to get new keys)
What is the Heroku URL of your deployed application? Update: It works!! (but almost no css) Past comment: it doesn't work past the search page. I want to continue working on it tonight/later today. https://chicken-muncher.herokuapp.com/
Provide a link to the Trello board you used https://trello.com/b/tuGLq06T/apimuncher

Abiaina added 25 commits May 2, 2018 17:38
…heroku and changed config application from autoload to eagerload
@CheezItMan
Copy link

API Muncher

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good number of commits and good commit mesages
Comprehension questions Check
General
Rails fundamentals (RESTful routing, use of named paths) Your index action uses path /recipes/index, when it should use /recipes instead. The show action should be on path /recipes/:uri.
Semantic HTML Less div elements and more semantic HTML tags please.
Errors are reported to the user It reports a confusing message when the search results are empty. It crashes when the show action is given an invalid URI
API Wrapper to handle the API requests Check
Controller testing You have all your controller tests in test/lib/recipe_test.rb. So you have a lot of problems with them passing.
Lib testing You've mixed the Recipe class and Wrapper testing.
Search Functionality It works
List Functionality It works
Show individual item functionality (link to original recipe opens in new tab) It works, it doesn't open the source in a new tab (minor issue)
Styling
Responsive layout Minimal styling
List View shows 10 items at a time/pagination Check
The app is styled to create an attractive user interface Minimal styling
API Features
The App attributes Edaman Check
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 had the basic functionality working, but with serious problems in your testing. I'd like you to take 10 minutes to review it with me or your tutor. You really should how to setup proper tests, even if you can't write tests to cover all circumstances. I do appreciate how you stuck with working on this after the due date. That shows the dedication you'll need in this career.

must_respond_with :success
end

it "should redirect to root with invalid data in params" do

Choose a reason for hiding this comment

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

This file is for testing the lib files, not controllers.

That's why all these tests are failing.


url = "#{BASE_URL}?r=#{r}&app_id=#{APPLICATION_ID}&app_key=#{APPLICATION_KEYS}"

data = HTTParty.get(URI.encode(url))

Choose a reason for hiding this comment

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

You need an if statement to verify that the get request worked.


recipe_details = ApiMuncherWrapper.get_recipe(r)

# should this be in ApiMuncherWrapper???

Choose a reason for hiding this comment

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

Probably a bit of both. I do think that recipe_details should be nil here not false

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