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

LJ: nodes : media ranker #47

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

Conversation

elle-johnnie
Copy link

@elle-johnnie elle-johnnie commented Oct 15, 2018

Media Ranker

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Describe a custom model method you wrote. I created a method in my works model to return the top 10 works. The category of the work is passed in as parameter in the method the method then collects all the works within that category orders them by their votes_count value in descending order then returns the first 10 objects within that array.
Describe how you approached testing that model method. What edge cases did you come up with? An edge case is an instance where no works are within that category and the view reflects no works found by that category. A nominal case is that it returns a list with the highest ranking vote count as the first item in the list. Another nominal case verifies that 10 items are returned.
What are session and flash? What is the difference between them? Session stores login data for a user in the browser's cookies and keeps track of them per session. Flash notices are one time messages sent to the user typically indicating success, failures, or warnings in the view.
Describe a controller filter you wrote. I utilized a find filter to get a work by its id. This filter was run before create, update, show, and destroy methods in the works controller.
What was one thing that you gained more clarity on through this assignment? I learned a little bit about caching as well as using Factory Bot to generate test data, which was pretty cool.
What is the Heroku URL of your deployed application? https://dashboard.heroku.com/apps/ljs-mediaranker-app
Do you have any recommendations on how we could improve this project for the next cohort? I struggled a lot with matching the styling using the copy paste method - once I reverted back to styling via bootstrap approximations I found it easier. It was hard to get motivated to just copy and paste. -- Things still to learn: I have lingering questions about database optimization - loading data always seems a bit slow so it makes me think I could be implementing methods that speed up data retrieval and queries. It would be good to have a class discussion on this.

@dHelmgren
Copy link

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) Mostly, see comment!
Views are well-organized (DRY, use of semantic HTML, use of partials) yes
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 You only tested your CRUD, but you wrote custom logic that needed validation!
Wave 1 - Media
Splash page shows the three media categories yes
Basic CRUD operations on media are present and functional yes
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 no, splash page is not, but you have it working on the all media page.
Splash page contains a media spotlight Yes
Wave 3 - Users and Votes
Media pages contain lists of voting users yes
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 Yeah, great work!
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!

Rails.application.routes.draw do
root "works#index"

resources :users

Choose a reason for hiding this comment

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

I don't think you need all 7 restful routes for users - only index and show are needed for this project.

<% if cat_list.empty? %>
<h4>No <%=cat.pluralize%> have been added to the database.</h4>
<% else %>
<% cat_list.each do |item| %>

Choose a reason for hiding this comment

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

Nice work on this! very DRY and clean

has_many :users, through: :votes

# validation
CATS = %w(album movie book)

Choose a reason for hiding this comment

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

Nice work here, this keeps your code very clean.

get votes_create_url
value(response).must_be :success?
end

Choose a reason for hiding this comment

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

You should be testing the uniqueness constraint here! Specific test cases I'd want to see:

  • A user can have votes for two different works
  • A work can have votes from two different users
  • A user cannot vote for the same work twice

end

def self.spotlight?
spot = self.all.sample

Choose a reason for hiding this comment

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

Gotta sort them too!

# pub year must be number in year range 0 - 2018
validates :publication_year, numericality: true, inclusion: 0..2018

def self.top?(category)

Choose a reason for hiding this comment

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

the question mark here has a semantic meaning that you aren't using correctly. A question mark at the end of a method signature implies that it returns a boolean.

delete work_url(work)
}.must_change "Work.count", -1

must_redirect_to works_path

Choose a reason for hiding this comment

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

There's a lot of interesting test cases for your custom Work methods missing here. For top?, I would wonder:

  • What happens if there are no works?
  • What happens if there are works but no votes?
  • What happens if two works have the same number of votes?

Similarly for your 'sample' method, I would ask:

  • What if there are no works of that category?
  • What if there are less than 10 works?
  • What if there's a tie for last place, e.g. works 9, 10 and 11 all have 0 votes?

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