Skip to content

Ports - Kim#28

Open
Kimberly-Fasbender wants to merge 170 commits intoAda-C11:masterfrom
Kimberly-Fasbender:master
Open

Ports - Kim#28
Kimberly-Fasbender wants to merge 170 commits intoAda-C11:masterfrom
Kimberly-Fasbender:master

Conversation

@Kimberly-Fasbender
Copy link
Copy Markdown

Media Ranker

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Describe a custom model method you wrote. In the works model I created a custom method named top_ten that returns the top ten works for a particular category with the highest number of votes.
Describe how you approached testing that model method. What edge cases did you come up with? I made sure it was returning no more than 10 works, and if there were no works it was returning an empty array. I also tested to make sure that they were in the correct order, highest to lowest.
What are session and flash? What is the difference between them? session and flash are both hashes. Session stores information in cookies and the hash persists until those cookies are deleted. Flash only persists through the next HTTP request/response cycle. Session we used to store the user's id number, to tell who was logged in. Flash we used to give the user messages about whether certain actions they were taking were working/not working/not allowed, etc.
What was one thing that you gained more clarity on through this assignment? The trello board can be a life saver. It's hard to keep track of everything. Especially what things still need testing/styling/etc.
What is the Heroku URL of your deployed application? https://media-ranker-kfas.herokuapp.com
Do you have any recommendations on how we could improve this project for the next cohort? MORE TIME! lol. I feel like I could work on this project for another week or two. I know that's not really a feasible suggestion, but it just seemed like it was really slow moving work.

@CheezItMan
Copy link
Copy Markdown

Media Ranker

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good number of commits and good commit messages
Comprehension questions Check, I know time was tight this week.
General
Rails fundamentals (RESTful routing, use of named paths) Check
Views are well-organized (DRY, use of semantic HTML, use of partials) Check
Errors are reported to the user Check
Business logic lives in the models Check
Models are thoroughly tested, including relations, validations and any custom logic Check, see small notes about testing
Controllers are thoroughly tested, including the login/logout process and multi-step workflows like voting for a work Check, see my notes on testing
Wave 1 - Media
Splash page shows the three media categories Check
Basic CRUD operations on media are present and functional Check
Wave 2 - Users and Votes
Users can log in and log out Check
The ID of the current user is stored in the session Check
A user cannot vote for the same media more than once Check
All media lists are ordered by vote count Check
Splash page contains a media spotlight Check
Wave 3 - Users and Votes
Media pages contain lists of voting users Check
Individual user pages and the user list are present Check
Optional - Styling
Bootstrap is used appropriately Check, really nice styling
Look and feel is similar to the original Check
Overall Great work, I found small things to adjust, but you did a truly outstanding job here.

Comment thread test/models/work_test.rb
expect(albums[1].votes.length).must_equal 1
end

it "returns at most 10 items" do
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really good test! 💯

Comment thread test/models/work_test.rb
expect(Work.spotlight.votes.length).must_equal 1

aj.works << harry2
kim.works << harry2
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what this line is accomplishing.

Comment thread test/models/vote_test.rb
@@ -0,0 +1,9 @@
require "test_helper"

describe Vote do
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You really should have a test that ensures that the combination of User and Work is unique, so that the same user can't vote on the same work.

Comment thread app/models/work.rb
validates :creator, presence: true

def self.top_ten(category)
top_works = self.top_voted(category)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good use of one method in another

Comment thread app/models/work.rb
def self.spotlight
works = self.all

spotlight = works.sort_by { |work| work.votes.length }
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note, this works, but it's more efficient to use the database to sort them, using .order

end

def update
work = Work.find_by(id: params[:id])
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about if you don't find the work?

@work = Work.find_by(id: params[:work_id])
@user = User.find_by(id: session[:user_id])

if !@user
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic would be great as a model method. I suggest creating an upvote method in the Work model.

"#{work_hash[:work][:category]}!"
end

it "will return a bad request when asked to update with invalid data" do
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also need a test with an invalid work_id

describe VotesController do
let(:work) { works(:harry_one) }

describe "create" do
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good set of tests!

Comment thread config/routes.rb
Rails.application.routes.draw do
root to: "homepages#index"

resources :works do
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, no extra routes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants