-
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
Amy Cash - Pipes - MediaRanker #25
base: master
Are you sure you want to change the base?
Conversation
Media RankerWhat We're Looking For
This is a good start, but it feels like there's some room for improvement here. It seems like you had a hard time working with more complex model relations, and building them into a sophisticated application. It would definitely be worth spending some time studying relations and practicing working with models, with the goal of being able to quickly write ruby code to pull whatever information you're interested in from the database. I also want you to be conscious about increasing the speed at which you can develop. Part of this is just experience, but there are lots of little things you can do to make life easier for your future self. An example of this is always using This was a large project with many moving parts, and we're not expecting perfection. Make sure you're getting time in bEtsy to practice the concepts you missed here, and keep up the hard work! |
resources :users | ||
|
||
resources :works | ||
resources :votes, only: [:new, :create] |
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 if this is what you intended, but this syntax will not nest your vote routes under works. To do so, you need to add a do
/end
:
resources :works do
resources :votes, only: [:new, :create]
end
<%# @details={:title=>[{:error=>:blank}]}, | ||
@messages={:title=>[" must be given"]}> %> | ||
|
||
<% if model.errors.messages.length > 0 %> |
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 probably don't need both this partial and the section in application.html.erb
to display flash errors. Pick one error reporting technique and stick with it.
<p>Find me in app/views/votes/index.html.erb</p> | ||
|
||
<p> | ||
Need View Top Media Page with Spotlight view and top ten in each 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.
I'm not sure this view is ever used.
@work.update_attributes(work_params) | ||
if @work.save | ||
redirect_to work_path(@work) | ||
return |
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.
If updating the work fails, this will not report the reasons why back to the user. Displaying this information is important to help your users understand what went wrong, but it is also an important tool for you the developer so that you can diagnose when things go wrong.
@votes = Vote.all | ||
@works = Work.all | ||
user_works = [] | ||
votes = @votes.where(user_id: user) |
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 really like that you've implemented this functionality in the model.
However, I'm a little worried about this implementation - you should be using active record relations to do some of this work for you. For example, instead of calling Vote.all
, you can get a list of a user's votes by saying user.votes
, and instead of saying @works.find(vote.work_id)
, you can say vote.work
.
Also, since this is already an instance method on User
, you shouldn't need to pass in a user, you can just use self
.
Here is a simplified version:
def list_votes
user_works = []
self.votes.each do |vote|
user_works << vote.work.title
end
return user_works
end
belongs_to :work | ||
|
||
validates :work, uniqueness: { scope: :user } | ||
validates :user, uniqueness: { scope: :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.
I like this validation. However, you only need one or the other, not both.
work_voters = [] | ||
votes = @votes.where(work_id: work) | ||
votes.each do |vote| | ||
work_voters << @users.find(vote.user_id).username |
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.
As above, you should be using AR relations here.
def create | ||
@vote = Vote.new(vote_params) | ||
@vote.save | ||
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.
You should be checking the result of save
here, and reporting it (success or failure) to the user so they know what happened.
end | ||
|
||
describe "destroy" do | ||
it "returns success and destroys the book when given a valid book ID" do |
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 the work has votes? Can you destroy it? What happens to those votes?
Media Ranker
Congratulations! You're submitting your assignment!
Comprehension Questions
session
andflash
? What is the difference between them?