Michelle - Edges - MediaRanker #35
Conversation
… re: logging in and out
… and edited user view a little to see the user info
…ndex views and especially in Work model
Media RankerWhat We're Looking For
Great job overall! Your implementation matches the demo site very closely, and I would say the learning goals for this assignment were definitely met. The big place where I see room for improvement is with model testing, particularly coming up with edge cases for your custom methods, so be sure to practice this on bEtsy, and feel free to ask an instructor to review your test coverage before it's due. You left a lot of questions inline, which I like. Make sure to review our code on MediaRanker Revisited, I think it addresses many of these. In general I am quite happy with this submission. Keep up the hard work! |
|
|
||
| def count_votes | ||
| return self.votes.length | ||
| end |
There was a problem hiding this comment.
I like that you put this method in ApplicationRecord - good use of inheritance.
| belongs_to :work | ||
| belongs_to :user | ||
|
|
||
| validates :user, presence: true, uniqueness: { scope: :work, message: "You may only vote on this work once."} |
There was a problem hiding this comment.
Good work getting this tricky uniqueness scope figured out!
| validates :title, presence: true, uniqueness: {scope: :category, message: "This category already knows about that title." } | ||
| # TODO: validates :publication_year --> how to get year only?? | ||
|
|
||
| # TODO: necessary???? validates_inclusion_of :category, :in => CATEGORIES |
There was a problem hiding this comment.
I would say the inclusion validation would be important. If we've built our views correctly then it shouldn't be possible for a user to send in an invalid category, but it would be a good safety check for yourself to make sure you've wired things up correctly. For example, if you misspelled "ablum" in the category dropdown menu, this would catch it.
| def self.sort_works(work_category) | ||
| cat_works_array = categorize_works(work_category) | ||
| works_by_vote = cat_works_array.sort_by{|work| work.count_votes}.reverse | ||
| return works_by_vote |
There was a problem hiding this comment.
I love how the methods in this file build upon one another, each adding a small amount of functionality. This is an excellent example of functional decomposition.
| class WelcomeController < ApplicationController | ||
|
|
||
| def index | ||
| @albums = Work.gen_top_ten_works('album') |
There was a problem hiding this comment.
I think that this action would probably fit in the works controller. It's not wrong here, but it is an extra file to pay attention to.
| def show | ||
| if @work = Work.find_by(id: params[:id]) | ||
| return @work | ||
| else |
There was a problem hiding this comment.
You repeat this code of finding the work and checking the result many times in this controller. This might be a good place to use a filter.
| <h2 class="top-ten__header"> | ||
| Top Movies | ||
| </h2> | ||
| <% @movies.each do |movie| %> |
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?
| describe Vote do | ||
| let(:vote) { votes(:user1_book1) } | ||
|
|
||
| it "must belong to a user" do |
There was a problem hiding this comment.
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
|
|
||
| describe 'sort works' do | ||
|
|
||
| it 'sorts works by most to least votes' do |
There was a problem hiding this comment.
There's a lot of interesting test cases for your custom Work methods missing here. For top_work, 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 category and top ten methods, 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
sessionandflash? What is the difference between them?