-
Notifications
You must be signed in to change notification settings - Fork 48
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
LJ: nodes : media ranker #47
base: master
Are you sure you want to change the base?
Conversation
…es for works and users
Media RankerWhat We're Looking For
|
Rails.application.routes.draw do | ||
root "works#index" | ||
|
||
resources :users |
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.
I don't think you need all 7 restful routes for users
- only index
and show
are needed for this project.
<% if cat_list.empty? %> | ||
<h4>No <%=cat.pluralize%> have been added to the database.</h4> | ||
<% else %> | ||
<% cat_list.each do |item| %> |
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.
Nice work on this! very DRY and clean
has_many :users, through: :votes | ||
|
||
# validation | ||
CATS = %w(album movie book) |
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.
Nice work here, this keeps your code very clean.
get votes_create_url | ||
value(response).must_be :success? | ||
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 should be testing the uniqueness constraint here! Specific test cases I'd want to see:
- A user can have votes for two different works
- A work can have votes from two different users
- A user cannot vote for the same work twice
end | ||
|
||
def self.spotlight? | ||
spot = self.all.sample |
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.
Gotta sort them too!
# pub year must be number in year range 0 - 2018 | ||
validates :publication_year, numericality: true, inclusion: 0..2018 | ||
|
||
def self.top?(category) |
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.
the question mark here has a semantic meaning that you aren't using correctly. A question mark at the end of a method signature implies that it returns a boolean.
delete work_url(work) | ||
}.must_change "Work.count", -1 | ||
|
||
must_redirect_to works_path |
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's a lot of interesting test cases for your custom Work
methods missing here. For top?
, I would wonder:
- 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?
Similarly for your 'sample' method, I would ask:
- 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?
Media Ranker
Congratulations! You're submitting your assignment!
Comprehension Questions
session
andflash
? What is the difference between them?