Katrina - Edges - Media Ranker #31
Conversation
Media RankerWhat We're Looking For
|
|
|
||
| validates :user_id, presence: true | ||
| validates :work_id, presence: true | ||
| validates_uniqueness_of :work_id, scope: :user_id |
There was a problem hiding this comment.
Nice work figuring out how to restrict voting via validations.
| CATEGORIES = ["album", "book", "movie"] | ||
|
|
||
| validates :title, presence: true | ||
| validates :category, inclusion: { in: CATEGORIES } |
There was a problem hiding this comment.
I love that you stored the list of categories in a constant!
app/controllers/works_controller.rb
Outdated
| class WorksController < ApplicationController | ||
| before_action :find_work, only:[:show, :edit, :update, :upvote] | ||
| before_action :find_top_media, only:[:welcome] | ||
| before_action :find_all_works, only:[:index] |
There was a problem hiding this comment.
If a controller filter applies to only one action, then it doesn't DRY things up because that code wasn't repeated anyway. But, it does make the flow of the code harder to follow. I would probably keep this work in the action.
|
|
||
| if @work.nil? | ||
| head :not_found | ||
| end |
There was a problem hiding this comment.
This code is repeated several times in this controller. Would it make sense to put this check inside the find_work controller filter?
| <h4><strong>Books</strong></h4> | ||
|
|
||
| <table class="table "> | ||
| <thead> |
There was a problem hiding this comment.
You have the same code to show a list of works repeated 3 times. Could you use a view partial or a loop to DRY this up?
|
|
||
| resources :users, only:[:index, :new, :show] do | ||
| resources :votes, except:[:edit, :destroy] | ||
| end |
There was a problem hiding this comment.
You include nested routes for votes under both works and users. However, I don't think that any of these routes are used by this application. You should remove them.
|
|
||
| it 'is valid when a user_id is unique for votes for the same work' do | ||
| first_vote = votes(:vote_one) | ||
| second_vote = votes(:vote_three) |
There was a problem hiding this comment.
Nice work figuring out all the interesting test cases for the uniqueness validation here.
| it 'returns the 10 works in descending order based on votes' do | ||
| works = Work.top_media('book') | ||
|
|
||
| expect( works.first ).must_equal works(:potter) |
There was a problem hiding this comment.
It is not obvious to me that this test is actually checking what it's name says. Is there something you could do to show the work of arranging this test? It could even be something like renaming the potter fixture work_with_the_most_votes, since that's what's interesting about it.
| describe 'top_media' do | ||
|
|
||
| it 'returns only 10 works' do | ||
| works = Work.top_media('book') |
There was a problem hiding this comment.
There are a couple other interesting cases I would like you to consider here:
- What if there are no works of that category?
- What if there are less than 10 works?
- What if there's a tie for last place, e.g. works 9, 10 and 11 all have 0 votes?
| describe 'spotlight' do | ||
|
|
||
| it 'returns the work with the most amount of votes' do | ||
| most_votes = Work.spotlight |
There was a problem hiding this comment.
Similar to the top_media method above, you're missing some cases here.
- What happens if there are no works?
- What happens if there are works but no votes?
- What happens if two works have the same number of votes?
Media Ranker
Congratulations! You're submitting your assignment!
Comprehension Questions
sessionandflash? What is the difference between them?