Ports - Jessica #29
Ports - Jessica #29
Conversation
…isplay additional information as per example
…user is deleted. Also add logout button.
Media RankerWhat We're Looking For
Hi Jessica! Well done! Overall, I think your project was really clean, thorough (model tests!), and often times clever. Fantastic work! I have confidence that you hit the learning goals. That being said, all of the comments I have are on the following things: ways to make your code style more consistent, and some small details that have an opportunity to be refactored. Overall, good work. Well done! |
| flash[:success] = "Successfully logged in as existing user #{user.name}" | ||
| end | ||
|
|
||
| if user.id |
There was a problem hiding this comment.
With our understanding of Rails, this if ... else checks on essentially user.nil?, which is covered above. It probably might feel more useful to bring this check on failure of saving inside of the first if after checking if user = User.create(name: name) succeeds or fails.
| end | ||
|
|
||
| def logout | ||
| user = User.find_by(id: session[:user_id]) |
There was a problem hiding this comment.
This line gets repeated twice... Maybe this is a good use case for a helper method to be pulled out, or even a controller filter (before_action)?
| class User < ApplicationRecord | ||
| has_many :votes | ||
| has_many :works, through: :votes | ||
|
|
There was a problem hiding this comment.
There is one goofy edge case not accounted for: validating on the presence of name!
| validates :title, presence: true | ||
|
|
||
| def self.top | ||
| Work.all.max_by do |work| work.votes.length end |
There was a problem hiding this comment.
For the sake of consistency, I would recommend curly braces and an explicit return:
return Work.all.max_by { |work| work.votes.length }
| def self.list_by_votes(category) | ||
| list = Work.where(category: category) | ||
| sorted_list = list.sort_by { |work| -work.votes.length } | ||
| return sorted_list |
There was a problem hiding this comment.
This is very specifically a "bonus" refactoring: I think this will be fine with just:
return list.sort_by { |work| -work.votes.length }
| @@ -0,0 +1,13 @@ | |||
| Rails.application.routes.draw do | |||
| get "homepages/index" | |||
There was a problem hiding this comment.
Interesting, I'm not sure what this syntax does... Do we need it? A lot of the tests break if I comment this out... but maybe at those points you should use root_path?
| root to: "homepages#index" # where do i want this to be | ||
| resources :users, only: [:show, :index] | ||
| resources :works do | ||
| resources :votes, only: [:create] |
There was a problem hiding this comment.
Good call on nesting this one route in here -- you're right, votes are inherently tied to works!
| get "/login", to: "users#login_form", as: "login" | ||
| post "/login", to: "users#login" | ||
| post "/logout", to: "users#logout", as: "logout" | ||
| get "/current", to: "users#current", as: "current_user" |
| has_many :works, through: :votes | ||
|
|
||
| # I added this before I decided to do a current user path. Do I need this, then? | ||
| def self.current(session) |
There was a problem hiding this comment.
From our spelunking: This method is only being used in the application layout file... which could be refactored to not use this method (called with User.current)
| it "returns an array of all objects of type if less than 10 exist" do | ||
| all_movies = Work.where(category: "movie") | ||
| top_movies = Work.top_ten("movie") | ||
| assert top_movies.length < 10 # for sake of test, must be true |
There was a problem hiding this comment.
This is not a high priority refactor, but for the sake of being consistent with non-assert style tests, this is the syntax for this:
expect(top_movies.length).must_be :<, 10
Media Ranker
Congratulations! You're submitting your assignment!
Comprehension Questions
sessionandflash? What is the difference between them?