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

Selam Ainalem Amperes #24

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

Selam Ainalem Amperes #24

wants to merge 18 commits into from

Conversation

SelamawitA
Copy link

@SelamawitA SelamawitA 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 heavily relied on postman to play with the API as well as reading the documentation. On parts of the documentation that weren't as clear (r = uri for example) copying the examples and running the request myself in Postman made the connections and the requirements more clear.
Describe your API Wrapper. How did you decide on the methods you created? My design was driven by relying on the methods provided by the API, realistically I know that more API calls = $$ but it seemed more efficient for this project to rely on the API for calling a single recipe, list from a query, range of queries over pulling on large set of a search, storing it somewhere and having to constantly search.
Describe an edge case or failure case test you wrote for your API Wrapper. The case of a non-string type entered for a query. I added a condition to return an empty array in the event that this occurs.
Explain how VCR aids in testing an API. VCR's store calls to APIs made by tests so if tests need to be run/updated multiple times the data from the original API call can be referenced, limiting the overall cost of API usage.
What is the Heroku URL of your deployed application? https://limitless-wildwood-28723.herokuapp.com/
Provide a link to the Trello board you used https://trello.com/b/N2UFecmf/recipeapi

…tion, and added test file for recipe_api_wrapper class.
…e able to sign in with their github (attempted with google but the process kept rejecting clientID).
…s to show page of a recipe. Added flash messages (and some styling) for reminder to log-in and successful search.
…ccounted for properly in conditions in wrapper class, and made update to css styling.
@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
Semantic HTML You've got some broken HTML in your layout file
Errors are reported to the user For search results with no recipes, yes. For incorrect URIs for the show action, no.
API Wrapper to handle the API requests Check, see my notes in your code
Controller testing Check,
Lib testing Check, see my notes
Search Functionality Check
List Functionality Check
Show individual item functionality (link to original recipe opens in new tab) Check
Styling
Responsive layout The search results are nicely responsive, the show actions, not so much
List View shows 10 items at a time/pagination Check
The app is styled to create an attractive user interface The styling looks pretty good.
API Features
The App attributes Edaman Check
The VCR casettes do not contain the API key Check, nice that you're also filtering the github ID and secret.
External Resources
Link to Trello Board Yes, but I can't access it.
Link to deployed app on Heroku Check
Overall Nice work, you hit the major learning goals. You have some things to work on including through testing and making sure you handle errors, but you did overall a nice job here.

<%= javascript_include_tag "application", 'data-turbolinks-track' => true %>
<%= csrf_meta_tags %>
</head>
<nav>

Choose a reason for hiding this comment

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

This nav element should be in the body

<%end%>
</nav>

<%if params[:query].nil?%>

Choose a reason for hiding this comment

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

Maybe simply put main elements with these ids in the views for homepage and search etc.

@@ -0,0 +1,40 @@

<h2 class="item-header"><%[email protected]%></h2>
<h3 class="item-header">By <%[email protected]%></h3>

Choose a reason for hiding this comment

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

This would be an excellent point for a link back to the source of the recipe.

end

def show
@recipe = RecipeApiWrapper.a_recipe(params[:uri])

Choose a reason for hiding this comment

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

You should have some error handling for if a recipe isn't found.

return []
end
a_recipe = recipe.uri_encode
response = HTTParty.get("https://api.edamam.com/search?app_id=#{APP_ID}&app_key=#{APP_KEY}&r=#{a_recipe}")

Choose a reason for hiding this comment

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

You should do some error handling for if the API request doesn't work. What if the URI is incorrect? How do you handle it?

end
end
it 'will return [] for a recipe search that is not of string type' do
VCR.use_cassette("recipe-is-a-string") do

Choose a reason for hiding this comment

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

What about a call with a URI that's not valid?

return recipe
end

def self.limited_query(query,from,to)

Choose a reason for hiding this comment

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

This looks like a great candidate for combining with the list_of_queried_recipes method using default parameters for the from and to fields.

it 'will display index for query within a specific range' do
session[:user_id].must_equal @jay.id
VCR.use_cassette("Recipe-controller:recipe-query-range") do
get query_results_path, params: {from:"1", to: "5", query: "corn"}

Choose a reason for hiding this comment

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

You should also test with invalid from and to fields

end
end
describe 'Show' do
it 'will show details of a recipe' do

Choose a reason for hiding this comment

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

What about invalid uris

end
describe 'Logged-in User' do
before do
login(@jay)

Choose a reason for hiding this comment

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

Very good that you're doing this testing.

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