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

Media Ranker - Danielle #32

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

Media Ranker - Danielle #32

wants to merge 36 commits into from

Conversation

danimetz
Copy link

Media Ranker

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Describe a custom model method you wrote. I created a method in the works model to determine what the "Spotlight" media was. It is a class method that returns the piece of work that has the highest number of votes.
Describe how you approached testing that model method. What edge cases did you come up with? The edge cases I came up with were what if there was a tie? What if none of the works have votes yet.
What are session and flash? What is the difference between them? We use flash to send one-time messages from our controllers to our views. Session is similar to flash except that the data is stored in the cookies and will not be gone after the user clicks somewhere else. The session data will be gone once the browser is closed.
Describe a controller filter you wrote. I wrote a two controller filters one in the application controller to determine the current user from all pages, this was necessary in order to keep track of the users votes during their session. I also created a find_work controller filter in the works controller to DRY up some of the methods in the works controller.
What was one thing that you gained more clarity on through this assignment? I gained better clarity on tests, and using session and flash messages.
What is the Heroku URL of your deployed application? https://media-ranker-dani.herokuapp.com/
Do you have any recommendations on how we could improve this project for the next cohort? Overall I enjoyed it! The CSS took way longer than I was anticipating. It was a good way to really nail down how session and flash work.

… for select, and the second for html options. So all you need is to give default empty options as first param after list of items and then add your class to html_options. This fixed the category drop down to be the entire width of the container
@CheezItMan
Copy link

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'm glad you gained some clarity with tests.
General
Rails fundamentals (RESTful routing, use of named paths) Not all your routes are needed, check the UsersController.
Views are well-organized (DRY, use of semantic HTML, use of partials) You could make better use of partials, and you don't have flash notices on update actions for WorksController.
Business logic lives in the models You've got some business logic methods here. Nice work.
Models are thoroughly tested, including relations, validations and any custom logic You do have some bugs in testing relationships. You also have some spotty testing in your custom methods. I realize the code is almost a copy-paste, but you can't test like that will always be the case.
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 Very nice work with Bootstrap
Look and feel is similar to the original Very good job imitating the original
Overall Overall, nice work, you hit all the learning goals. Areas for improvement include some testing, and using partials where you can. Still, you did a good job.

Rails.application.routes.draw do
root 'works#main'
resources :works
post 'works/:id/upvote', to: 'works#upvote', as: 'upvote'

Choose a reason for hiding this comment

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

The indentation doesn't actually mean anything here as it's not technically nested inside.

resources :works
post 'works/:id/upvote', to: 'works#upvote', as: 'upvote'

resources :users

Choose a reason for hiding this comment

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

Do you need all the CRUD route for users? What about :edit and :update?

vote = work.votes

# Assert
expect(vote).must_be_instance_of Vote

Choose a reason for hiding this comment

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

because vote here is actually a collection of votes, this test failed. If you wanted to test the type you could loop through the collection and check that each instance is a Vote.

Also since a Work has many votes, it doesn't have a foreign key in it.

Instead expect(vote.work_id).must_equal work.id

end

describe 'Media Page - Methods' do
it 'return top 10 of a given category' do

Choose a reason for hiding this comment

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

You're not throughly testing all the methods here. Just top_movies for 10 items and more than 10 and top_books for less than 10. You don't test each.

end
end

describe "Validations" do

Choose a reason for hiding this comment

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

No user validations?

What about tests for date_joined

class User < ApplicationRecord
has_many :votes
has_many :works, through: :votes

Choose a reason for hiding this comment

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

No validations? What about username?

def update
if @work && @work.update(work_params)
redirect_to work_path(@work.id)
elsif @book

Choose a reason for hiding this comment

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

No flash.now notices?

</tr>
</thead>
<tbody>
<% @works.each do |work| %>

Choose a reason for hiding this comment

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

Notice that you do a very similar table here a lot. You could DRY this out with a partial.

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