Skip to content
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

Canaan -- carets #31

Open
wants to merge 55 commits into
base: master
Choose a base branch
from
Open

Canaan -- carets #31

wants to merge 55 commits into from

Conversation

canaanwest
Copy link

Media Ranker

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Describe a custom model method you wrote. I wrote a self.top method to find the work with the greatest number of votes across categories. It searches through all works, sorting by vote, and then selects the work with the highest vote.
Describe how you approached testing that model method. What edge cases did you come up with? I tested that model first by making sure it returned the work with the greatest votes in a situation where there was only one; the edge case I tested was when there was a tie to make sure it selected the first in the list.
Describe an edge case test you wrote for a controller In my votes controller, I tested to make sure that someone who wasn't logged in couldn't vote on a work.
What are session and flash? What is the difference between them? Session is a tool we use that keeps track of a user's interaction with a site. It persists throughout the interaction with the browser, until that browser is closed or the user is logged out. It allows us to make connection between the person using the website and that person's actions. Flash is less persistent--the information it gives to the browser only lasts through the end of the request/redirect and then disappears; it's used in this context primarily as a way to show success or failures in the actions of a user.
Describe a controller filter you wrote. I wrote a controller filter for finding a user. It did the work of finding the user by ID so that my other tests didn't have to. Very convenient!
What was one thing that you gained more clarity on through this assignment? After a great many struggles, testing both models and controllers has become easier through this assignment.
What is the Heroku URL of your deployed application https://media-ranker-caw.herokuapp.com/
Do you have any recommendations on how we could improve this project for the next cohort? I think our Foundation knowledge was not what it needed to be in order to successfully mimic the website. There was too much left us to up to figure out about foundation (our lesson on that was pretty short, and we didn't get to do any examples; it seemed more like an afterthought in class, but then was absolutely central to this project). I think the foundation stuff could be put somewhere else--like maybe before we learned testing or after we've mastered it in this context.

@CheezItMan
Copy link

Media Ranker

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good number of commit and good commit messages.
Comprehension questions Check, we'll take the Foundation feedback under advisement.
General
Rails fundamentals (RESTful routing, use of named paths) Check
Semantic HTML Check, good semantic HTML
Errors are reported to the user Trying to update a Model with a blank title doesn't result in an error message.
Business logic lives in the models Good work putting business logic in the models.
Models are thoroughly tested, including relations, validations and any custom logic Model testing is pretty good with some minor things that could be improved.
Controllers are thoroughly tested Some good tests, you're missing some negative tests in the WorksController.
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
Individual user pages and the user list are present Check
A user cannot vote for the same media more than once Check, well done
All media lists are ordered by vote count Check
Splash page contains a media spotlight Check
Media pages contain lists of voting users Check
Wave 3 - Styling
Foundation is used appropriately Well done, not perfect, but well done
Look and feel is similar to the original Pretty good, the responsiveness needs some work as under small sizes it doesn't look right.
Overall This is really good. You've got some minor things to work on, but this is well done. I left some minor notes in your code as well.

Copy link

@CheezItMan CheezItMan left a comment

Choose a reason for hiding this comment

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

Some minor notes.

greys.valid?.must_equal true
end

it "will not be true unless it has a title and category" do

Choose a reason for hiding this comment

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

Better to say,

it "is valid with both a title & category" do

let(:greys) {works(:greys)}
let(:work) {Work.new}

it "must have a title and category to be true" do

Choose a reason for hiding this comment

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

Better to say,

it "is valid with both a title & category" do

describe Vote do
let(:vote) {votes(:one)}

it "will be valid when user and work are present" do

Choose a reason for hiding this comment

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

You should have a test to make sure that the combination of user/work ids are unique.

@@ -0,0 +1,48 @@
require "test_helper"

describe WorksController do

Choose a reason for hiding this comment

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

Some negative tests for the WorksController are needed, like deleting a work that already exists, and showing a work that doesn't exist, etc.

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