Conversation
…using partial view
…create after details page is created
…ences to work and user
Media RankerWhat We're Looking For
|
| resources :works | ||
| post "/works/:id/upvote", to: "works#upvote", as: "upvote" | ||
|
|
||
| resources :users, only: [:index, :show] |
There was a problem hiding this comment.
Good work generating only the routes you needed here.
| @@ -0,0 +1,86 @@ | |||
| class WorksController < ApplicationController | |||
| before_action :find_work, only: [:show, :edit, :update, :destroy, :upvote] | |||
|
|
|||
There was a problem hiding this comment.
Good use of a controller filter to help keep this file DRY
| vote = Vote.find_by(user_id: session[:user_id], work_id: @work.id) | ||
| if vote | ||
| flash[:status] = :error | ||
| flash[:message] = "Cannot vote more than once" |
There was a problem hiding this comment.
Could you move this check to the model, possibly as a validation?
| <% ["movie", "book", "album"].each do |type| %> | ||
| <section class="top-ten-container"> | ||
| <h2>Top <%= type.capitalize%>s</h2> | ||
| <ul> |
| <% end %> | ||
| </nav> | ||
|
|
||
| <body> |
There was a problem hiding this comment.
All HTML that appears on the page should be contained inside the <body> tag.
| <% ordered_works = selected_works.sort_by do |work| %> | ||
| <% work.votes.length %> | ||
| <% end %> | ||
| <% ordered_works.reverse! %> |
There was a problem hiding this comment.
This is an awful lot of work to be doing in a view. Could you extract this to a model method? It would probably look pretty similar to your top_ten method.
| <% Work.top_ten(type).each do |work| %> | ||
| <li> | ||
| <%= link_to work.title, work_path(work) %> by <%= work.creator %> | ||
| <%= work.votes.length%> votes |
There was a problem hiding this comment.
I'm not a huge fan of you calling the model directly from the view - would it be possible to "tee up" this work in the controller?
| describe HomepagesController do | ||
| # it "must be a real test" do | ||
| # flunk "Need real tests" | ||
| # end |
There was a problem hiding this comment.
Looks like this was missed. In addition to the nominal case, the media spotlight presents and interesting edge case where there are no works in the database.
| describe "login" do | ||
| it "logs in an user" do | ||
| new_user = User.create(username: "newuser") | ||
|
|
There was a problem hiding this comment.
There are a couple of behavior modes you're not covering:
- Logging in a new user (should create that user)
- Attempting to log in with an invalid username (e.g. empty string) (should fail)
|
|
||
| it "flashes an error if user already voted" do | ||
| post login_path, params: @login_data | ||
| expect(session[:user_id]).must_equal @user.id |
There was a problem hiding this comment.
Good work getting the three interesting cases for upvote, especially since the testing logic is so tricky.
2 participants
Media Ranker
Congratulations! You're submitting your assignment!
Comprehension Questions
sessionandflash? What is the difference between them?While going through the building my index pages, I discovered that I was unable access any data that showed when a particular work was upvoted. As a result, I had to rework my models, removing my join table between works and users and creating a new vote model that had a many to one relationship with both user and work. This allowed me to access each particular votes created_at variable. It also I think made it more clear what I was doing in my html having work.votes rather than work.users to represent that votes.
Because I switched my design, a lot of my tests had to be rewritten. This is something that I am working on updating.