Space - Becca#39
Open
RebeccaRoach wants to merge 59 commits into
Open
Conversation
…ented get_category method in Work model.
… model to find spotlight media.
…on on works controller.
…l to render flash[:error_messages] from controller.
…works local to _work_table.
Css/styles
Media RankerFunctional Requirements: Manual Testing
Major Learning Goals/Code Review
Previous Rails learning, Building Complex Model Logic, DRYing up Rails Code
Testing Rails Apps
Overall Feedback
Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
SummaryNicely done, this is really outstanding work. You hit all the learning goals. Well done. Take a look at my comments and let me know what questions you have. |
CheezItMan
reviewed
May 31, 2020
|
|
||
| resources :users, only: [:index, :show, :new] | ||
|
|
||
| get '/homepages', to: 'homepages#index', as: 'homepages' |
There was a problem hiding this comment.
Since you have the root path going to homepages#index, why do you need this additional route?
| expect(Vote.all.count).must_equal @previous_vote_count | ||
| end | ||
|
|
||
| it "is invalid for a user to vote more than once on the same work" do |
| require "test_helper" | ||
|
|
||
| describe HomepagesController do | ||
| # it "does a thing" do |
Comment on lines
+4
to
+6
| def record_not_found | ||
| render file: "#{Rails.root}/public/404.html" , status: 404 | ||
| end |
| else | ||
| flash[:error] = "A problem occurred: Could not create #{@work.category}" | ||
| flash[:error_messages] = @work.errors.messages | ||
| render :new |
There was a problem hiding this comment.
You should also include a status code
Suggested change
| render :new | |
| render :new, status: :bad_request |
| else | ||
| flash[:error] = "A problem occurred: Could not update #{@work.category}" | ||
| flash[:error_messages] = @work.errors.messages | ||
| render :edit |
There was a problem hiding this comment.
Suggested change
| render :edit | |
| render :edit, status: :bad_request |
Comment on lines
+15
to
+17
| return self.where(category: category_name).includes(:votes).max_by(num_category_works) { | ||
| |work| work.votes.count | ||
| } |
There was a problem hiding this comment.
Just noting it's better to sort the works at the database-level with things like .order as Postgres is more efficient at filtering and sorting records than Ruby/Rails are.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Media Ranker
Congratulations! You're submitting your assignment!
Comprehension Questions
most_recent_vote_datecustom model instance method on the works model to use in the spotlight method in case of a tie for top voted work. Themost_recent_vote_datemethod returns the most recent date on which a user voted, which made it possible to implement tie-breaking logic per the demo site.votes.ymlfile for two different votes on the same work. This allowed me to see that the most recent vote date was returned instead of the earlier one. As an edge case, I tested what would happen if a work did not have any votes, in which case I return an old dummy date (May 20, 1965). Doing this allowed me to still do the comparison of dates even if a work didn't have a vote, vs. trying to compare a date with nil.sessionandflash? What is the difference between them?