Skip to content

Sockets - Kelly#38

Open
kdow wants to merge 84 commits intoAda-C11:masterfrom
kdow:master
Open

Sockets - Kelly#38
kdow wants to merge 84 commits intoAda-C11:masterfrom
kdow:master

Conversation

@kdow
Copy link
Copy Markdown

@kdow kdow commented Apr 29, 2019

Media Ranker

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Describe a custom model method you wrote. I wrote a method for get_spotlight to get the work for the media spotlight.
Describe how you approached testing that model method. What edge cases did you come up with? I tested that it returns a Work object and that it returns the work with the most votes. For edge cases, I tested that it returns the first work created in the case of a tie, that it returns the first work created if all works have no votes, and that it returns nil if there are no works.
What are session and flash? What is the difference between them? Session and flash are both hash like objects. Flash is used to send one time messages from controllers to views. Session is used to keep track of data throughout a browsing session, so it will last until the browser is closed.
What was one thing that you gained more clarity on through this assignment? How different models and controllers work together.
What is the Heroku URL of your deployed application? https://kellys-media-ranker.herokuapp.com/
Do you have any recommendations on how we could improve this project for the next cohort?

@droberts-sea
Copy link
Copy Markdown

Media Ranker

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene yes
Comprehension questions yes
General
Rails fundamentals (RESTful routing, use of named paths) yes
Views are well-organized (DRY, use of semantic HTML, use of partials) yes - great work using partials for the work lists
Errors are reported to the user yes
Business logic lives in the models yes
Models are thoroughly tested, including relations, validations and any custom logic yes - good work
Controllers are thoroughly tested, including the login/logout process and multi-step workflows like voting for a work mostly - see inline
Wave 1 - Media
Splash page shows the three media categories yes
Basic CRUD operations on media are present and functional yes - You can't delete a work that has votes! This is because you've created a link between the two tables at the database level, and deleting the work would leave behind invalid votes. You might want to check out the dependent argument to has_many.
Wave 2 - Users and Votes
Users can log in and log out yes
The ID of the current user is stored in the session yes
A user cannot vote for the same media more than once yes
All media lists are ordered by vote count yes
Splash page contains a media spotlight yes
Wave 3 - Users and Votes
Media pages contain lists of voting users no
Individual user pages and the user list are present yes
Optional - Styling
Bootstrap is used appropriately yes
Look and feel is similar to the original yes
Overall Great job overall! Your implementation matches the demo site very closely, and I would say the learning goals for this assignment were definitely met. There are a few places where things could be cleaned up, which I've tried to outline below, but in general I am quite happy with this submission. Keep up the hard work!

Comment thread config/routes.rb
root "homepages#index"

resources :works
resources :users, only: [:index, :show]
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 work only generating the routes you need here.

Comment thread app/models/vote.rb
belongs_to :user

validates :user, uniqueness: {scope: :work,
message: "User can only vote on this work one time"}
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 work getting this tricky uniqueness scope figured out!

Comment thread app/models/work.rb

def self.get_top_ten(category)
return self.sorted_works(category)[0...10]
end
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 love the way the methods in this file build on each other, each providing a small bit of functionality. This is an excellent example of functional decomposition.

</tr>
<% Work.sorted_works(category).each do |work| %>
<tr>
<td><%= work.votes.count %></td>
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 a huge fan of calling a model method directly from the view like this - would it be possible to "tee up" this data in the controller?


describe "login" do
it "will login user" do
post login_path, params: @login_data
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 have two tests here, but these are only covering one behavior mode for this controller action (logging in an existing user). I would probably combine these into one.

However, there are a couple of behavior modes you're not covering:

  • Logging in a new user (should create that user)
  • Attempting to log in with an invalid username (e.g. empty string) (should fail)

describe "logout" do
it "will logout user" do
perform_login
post logout_path
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 happens if no one is logged in?

describe "upvote" do
it "disallows guest users voting if they have not logged in" do
user = users(:kelly)
work = works(:amelie)
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 coverage for this action.

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