Conversation
… minitest/rails - unsure how to debug
…w says stack too deep
…ulation of current work data into form. tested user model validations
…ction in sessions
…cation.html.erb to include how and where flash messages will render on page
merging work done on votes
… and display all work based on top vote on works index view
…er controller, work controller. Provided valid publication date for works fixture.
Media RankerWhat We're Looking For
Hi Amber-Lynn! Good work on this project -- I think that there are a lot of really good clean parts of your code (a lot of use of Enumerable methods in clean ways!) and a few places that I think could get better. In general, one of my biggest concerns is the variations from your project submission compared to the original. A lot of this project was around taking the original and trying to mimic it, but I saw some differences between them in regards to use flow. Why do I need to make a new user on a separate form? In the original, if a user didn't exist at log-in, then the site would make a new user. In the submission, if the user doesn't exist at log-in, then I have to go to a new form to make a new user. Also, after editing a work, where should it go? In the original, after editing a work, then the site redirects you to the work detail page. In the submission, after editing a work, the site redirects you to the user page. Lastly, what should happen after you delete a work? In the original, after deleting a work, then the site redirects you to the home page. In the submission, after deleting a work, the site redirects you to the work detail page, which, because the work doesn't exist anymore, is broken! My other concern with your code submission are the bits of code that seem to be unused, but still are here. In general, if code is unused, we'd prefer you delete it. That being said, I think your project hits most of the learning goals-- the code that was there and worked was good. It mostly misses some things around partials, some weird areas of code/buggy code, and missing some alignment with mimicking the original site's user flow |
| has_many :votes | ||
| has_many :users, through: :votes | ||
|
|
||
| CATEGORIES = %w(movie book album) |
| validates :user_id, presence: true | ||
| validates :work_id, presence: true | ||
|
|
||
| validate :one_vote_per_user, on: :create |
|
|
||
| validates :title, presence: true, uniqueness: true | ||
| validates :creator, presence: true | ||
| validates :publication_year, presence: true, :numericality => {:less_than => Date.today.year, :greater_than => 1700} |
There was a problem hiding this comment.
Nice adding this validation... But did the original site have this validation on it?
|
|
||
| def self.show_by_vote(category) | ||
| Work.all.sort_by { |work| -work.votes.count }.select { |work| work.category == category} | ||
| end |
There was a problem hiding this comment.
Really nice method! Nice use of enumerable methods to take this logic and make it concise
| <div class="list-inline"> | ||
| <div class="p-3 mb-2 bg-info text-white"><%= link_to "View all media", works_path, class: "all_works-link" %></div> | ||
| <div class="text-white"><%= link_to "Add a new work", new_work_path, class: "new_work-link" %></div> | ||
| <div cclass="text-white"><%= link_to "View all users", users_path, class: "new_work-link" %></div> |
| <%= yield %> | ||
| </main> | ||
|
|
||
| <footer>Footer Info</footer> |
There was a problem hiding this comment.
Was there a footer on the original site?
| @@ -0,0 +1,37 @@ | |||
| <div class="container"> | |||
| locals: { | ||
| model: @work | ||
| } | ||
| %> |
There was a problem hiding this comment.
Does this form need to also render the partial of itself?
| <%= f.select :category, Work::CATEGORIES %> | ||
| <%= f.submit action_type, class: "work btn" %> | ||
| <% end %> | ||
| </div> |
There was a problem hiding this comment.
You have an end on line 23 that ends the form_with block that started on line seven... And you have a div that starts on line 11 and ends on line 24. These aren't neatly nested with each other -- you should be mindful that your end should be outside of anything that it includes fully, otherwise it isn't proper syntax and you'll get negative side-effects
| <%= f.select :category, Work::CATEGORIES %> | ||
| <%= f.submit action_name, class: "work btn" %> | ||
| <% end %> | ||
| </div> |
There was a problem hiding this comment.
You end up having essentially a form duplicate to the edit form... Why didn't you use the partial? Would that have also worked?
| post '/works/:id/upvote', to: 'works#upvote', as: 'upvote' | ||
|
|
||
| resources :users, except: [:edit, :update, :destroy] do | ||
| end |
There was a problem hiding this comment.
if there's no nesting, do you need the do ... end bit?
|
|
||
| post '/sessions/logout', to: 'sessions#logout', as: 'logout' | ||
|
|
||
| # get '/works', to: 'works#index', as: 'all_works' |
There was a problem hiding this comment.
Feel free to delete your comments before submission!
| describe "self.show_by_vote(category)" do | ||
| it "returns sorted movies in descending order based on votes" do | ||
| @books = Work.show_by_vote("book") | ||
| book_result = @books.length.times do |i| |
There was a problem hiding this comment.
Since you don't use the variable book_result, you're probably okay to just make the above line @books.length.times do |i| without the variable assignment so that you don't have any unused variables
| works = Work.all | ||
|
|
||
| result = works.each do |work| | ||
| @top_work.votes.count >= work.votes.count |
|
|
||
| result = works.each do |work| | ||
| @top_work.votes.count >= work.votes.count | ||
| return true |
There was a problem hiding this comment.
Should this "return true"? return will make the loop stop after one iteration.
2 participants
Media Ranker
Congratulations! You're submitting your assignment!
Comprehension Questions
sessionandflash? What is the difference between them?