-
Notifications
You must be signed in to change notification settings - Fork 43
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
Nkiru - Pipes- MediaRanker #30
base: master
Are you sure you want to change the base?
Conversation
Media RankerWhat We're Looking For
|
|
||
resources :votes | ||
|
||
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'm not sure you need all seven RESTful routes for votes and users. Be careful when using resources
to only generate the routes you plan to use.
|
||
<table> | ||
<h3> Albums </h3> | ||
<thead> |
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.
Is there a way you could use a view partial to DRY up repeated HTML on this page?
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.
Also, I would like to see each category in its own <section>
here.
def show | ||
find_work_by_params_id | ||
# @work = Work.find_by(id: params[:id]) | ||
# unless @work |
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 are basically using find_work_by_params_id
as a controller filter already here - the last step is to take advantage of Rails's syntactic sugar to clean up the code a bit.
def self.top_ten_albums | ||
album = Work.where(category: "album") | ||
return album.sort_by { |work| -work.votes.count }.take(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.
Watch your indentation here
describe UsersController do | ||
# it "must be a real test" do | ||
# flunk "Need real tests" | ||
# 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.
Even though we didn't ask you to test login/logout, you should at least have tests for index and show here.
|
||
#question should be what will cause the index page not to show. since it is ordered by title then if title isn't present maybe it wont show?? I wonder if I could mess with the form and that could result in html not rendering properly. perhaps! | ||
|
||
#not sure what to do with this yet, need an invalid case. do I need to check work id |
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.
This is a good question! Could be there is no failure case here, which would mean all the tests for this action result in success.
|
||
must_respond_with :not_found | ||
Work.count.must_equal start_work_count | ||
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.
What if you try to destroy a work with votes? Can you do it? What happens to the votes?
These are exactly the sorts of questions that the end-to-end style of a controller test is suitable to answer.
Media Ranker
Congratulations! You're submitting your assignment!
Comprehension Questions
session
andflash
? What is the difference between them?