Katricia - Edges - MediaRanker #34
Conversation
…work and user foreign keys
Media RankerWhat We're Looking For
|
| belongs_to :work | ||
|
|
||
| validates :user_id, presence: true | ||
| validates :work_id, presence: true, uniqueness: { scope: :user_id } |
There was a problem hiding this comment.
Good work getting this scope figured out!
| def self.get_top_work | ||
| works = Work.all.to_a | ||
| highest= works.sort_by{|item| item.votes_count}.reverse | ||
| return highest.first |
There was a problem hiding this comment.
Instead of reversing and taking the first, you could take the last.
| def self.get_top_list(category) | ||
| category_works = Work.get_category_media(category).to_a | ||
| # sorted = category_works.sort_by{|item| item.votes_count}.reverse | ||
| # return sorted[0..9] |
There was a problem hiding this comment.
Good job reusing existing functionality here!
| redirect_to work_path(id: @vote.work.id) | ||
| else | ||
| flash[:notice] = "A problem occurred: Could not upvote: user has already voted for this work" | ||
| redirect_back fallback_location: :work_path |
There was a problem hiding this comment.
This error message assumes that the only way the vote could fail to save is if it was a duplicate. The way your application currently works that is true, but if you were to continue to work on this you might introduce another validation, which would then require this code to change. A better approach would be to read errors.messages (and set it in the validation if needed).
If you want to think of it from a POODR point of view, we could say that this code knows too much about how your model works.
| <strong><%column.capitalize %> </strong><%= message %> | ||
| <% end %> | ||
| <% end %> | ||
| <% end %> |
There was a problem hiding this comment.
Watch your indentation here. I want to recognize that the Atom auto-indenter doesn't work well for ERB, but that doesn't mean we don't have to make our code easy to follow.
If you needed to isolate this bit so it's not affected by the auto-indenter, you might use a view partial.
| <section> | ||
| <% Work::WORK_CATEGORIES.each do |category| %> | ||
| <h4 class="black-hdg"><%= category.capitalize%>s</h4> | ||
| <table class="table"> |
There was a problem hiding this comment.
Great work DRYing up this view code!
| root 'top_works#index' | ||
|
|
||
| resources :users | ||
|
|
There was a problem hiding this comment.
Do you really need all 7 RESTful routes for users? Remember to be careful about using resources, and only generate the routes you need.
|
|
||
| it "is valid if work is not unique" do | ||
| vote2 = votes(:two) | ||
| vote4 = votes(:four) |
There was a problem hiding this comment.
Good job on these test cases for the vote validation. There's a lot of interesting cases here, and I think you've found them all.
| it "sorts list in order of votes" do | ||
| @works.first.title.must_equal works(:memento).title | ||
| end | ||
| end |
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 memento fixture work_with_the_most_votes, since that's what's interesting about it.
| # add more fixtures so can test with over ten category works? | ||
|
|
||
| # describe get_top_work | ||
| # must return work with most votes |
There was a problem hiding this comment.
I would be interested in what the rest of these tests look like.
One approach might be to create works and votes in a loop.
Media Ranker
Congratulations! You're submitting your assignment!
Comprehension Questions
sessionandflash? What is the difference between them?