-
Notifications
You must be signed in to change notification settings - Fork 47
Elle's Muncher App #70
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
base: master
Are you sure you want to change the base?
Conversation
…e and yes I am on master like a casual
…agination up next
Muncher wave2
Muncher wave4
added formatting to navs and show page for responsiveness and fixed b…
Muncher heroku
added testing for wrapper and recipe.rb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ellevargas I've noted some things in your code via the pull request here, and I've also completed the Progress Report update: https://docs.google.com/document/d/14HiUfUow1QxVkw9uk41S_AW_DHf0F8EfUdWpv6ol8gk/edit
if @data != nil | ||
@data = @data.paginate(:page => params[:page], :per_page => 10) | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good use of the pagination gem.
What do you do when there are no results to a search or if it's an invalid search.
redirect_to root_path, notice: "You must search for something before we can give you recipes!" | ||
end | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work catching errors when the user doesn't give you a good search term.
<%= link_to "Muncher", root_path, alt:"Muncher Home" %> | ||
</div> | ||
</nav> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good use of Foundation to make the list page responsive!
|
||
get '/show/*uri' => 'recipes#show', format: false, as: 'show' | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not very RESTful routes.
I would suggest it be be:
root to: 'recipes#search' # make it go to the search screen
get '/recipes' => 'recipes#list' # list the recipes using get parameters
get '/recipes/:id' => 'recipes#show'
Nice use of format: false to be able to include things like periods etc in the @uri
recipes << wrapper | ||
end | ||
return recipes | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job here making sure the wrapper returns an array of Recipe objects.
test "should get list" do | ||
VCR.use_cassette("controller-list") do | ||
get :list, {query: "chicken"} | ||
assert_response :success |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also check to see if some of the search results are in there, and that the list is greater than 0.
assert_response :redirect | ||
end | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might also want to test:
- When the result has no entries.
- When the URI doesn't exist.
Otherwise good testing.
end | ||
end | ||
|
||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good testing here.
assert_equal recipe.url, "http://www.seriouseats.com/recipes/2011/07/durian-smoothie-recipe.html" | ||
assert_equal recipe.healthLabels, "" | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be cause to require some fields in a Recipe, so that might be something to test as well.
:record => :new_episodes, # record new data when we don't have it yet | ||
:match_requests_on => [:method, :uri, :body] # The http method, URI and body of a request all need to match | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job filtering out the Muncher Keys and IDs from github!
Pushed to Heroku. Live @ https://muncher-app.herokuapp.com/
Will work on more robust testing tomorrow/Tuesday after school.