Conversation
Media RankerWhat We're Looking For
|
|
|
||
| resources :users, | ||
| only: [:index, :show] | ||
| end |
There was a problem hiding this comment.
Nice work limiting the routes for users to be just the ones you need!
| <% flash.each do |name, message| %> | ||
| <% if message.class == Array %> | ||
| <div class="alert-warning"> | ||
| <% message.each do |msg| %> |
There was a problem hiding this comment.
I might move this flash block outside of the nav. 1) it doesn't actually affect navigation, but 2) it's placement is not easy to read.
| <%= render partial: "top_ten", locals: { #render top albums from _top_ten.html.erb | ||
| work_category: "album", | ||
| data: @albums | ||
| } |
There was a problem hiding this comment.
Nice job using view partials here, but can you think of a way to further DRY this code?
| <section class="spotlight"> | ||
| <h2 class="spotlight__header"> | ||
| <span class="spotlight__header--prefix">Media Spotlight</span> | ||
| <%= link_to @best_work.title, work_path(@best_work), class: "spotlight__link-to" %> by <%= @best_work.creator %> |
There was a problem hiding this comment.
You don't define a @best_work, so this ERB doesn't run. The place to do this would be in the controller that directs the user to the main page.
| @@ -0,0 +1,5 @@ | |||
| class Vote < ApplicationRecord | |||
| belongs_to :user | |||
| belongs_to :work | |||
There was a problem hiding this comment.
You can't delete a work that has votes! This is because you've created a link between the two tables at the database level, and deleting the work would leave behind invalid votes. You might want to check out the dependent argument to has_many.
| def upvote(user) | ||
| @vote = Vote.new #create a new vote | ||
| @vote.work_id = self.id #a work's id is assigned vote's foreign key, work_id | ||
| @vote.user_id = user.id #a user's id is assigned to vote's foreign key, user_id |
There was a problem hiding this comment.
This method doesn't validate whether or not the user is valid, which means if you try to upvote without validating the user, this method breaks!
|
|
||
| #Assert | ||
| expect(vote).must_be_instance_of User | ||
| expect(user.id).must_equal vote.user_id |
There was a problem hiding this comment.
I would be trying to break the uniqueness property here rather than in the tests for works. 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
| #Assert | ||
| expect(@vote).must_equal false | ||
| expect(work.errors.messages).wont_include: user_id | ||
|
|
There was a problem hiding this comment.
There's a lot of interesting test cases for your custom Work methods missing here. 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?
| # end | ||
|
|
||
| def self.highest_albums #call self.top_10 method for album category | ||
| top_ten("album") #displays up to 10 albums |
There was a problem hiding this comment.
You should use symbols instead of strings here because your categories don't show up properly when they aren't capitalized the same way. That is, you have "albums" here and "Albums" in your seeds, so some of your work never shows up.
| unless @vote.valid? #if vote is not valid... | ||
| puts "#{@vote.errors.messages}" #display error messages | ||
| end | ||
| @vote.save #save vote so user cannot duplicate vote for a work |
There was a problem hiding this comment.
This method should add votes to list of votes for a work. Your vote count doesn't get updated properly, so your sorts never work.
2 participants
Media Ranker
Congratulations! You're submitting your assignment!
Comprehension Questions
sessionandflash? What is the difference between them?