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

Laura MediaRanker #49

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

Conversation

lauracodecreations
Copy link

Media Ranker

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Describe a custom model method you wrote. I create the joining_date method to define the date by calling the created_at attribute, and in addition, I formatted the date.
Describe how you approached testing that model method. What edge cases did you come up with? I plan to continue working on this. I did test my work model validations
What are session and flash? What is the difference between them? session allows users to log in, flash is a message that flashes the user if the
Describe a controller filter you wrote. not yet
What was one thing that you gained more clarity on through this assignment? how to debug errors in rails when the 'raise' does not work, how to create a custom method in the model, and how to test validations.
What is the Heroku URL of your deployed application? https://media-app-2018.herokuapp.com/users/1
Do you have any recommendations on how we could improve this project for the next cohort? maybe learning how to use pry to test the params in the rails app while is running would be helpful for other students when debugging syntax errors. A tutor taught me how to use pry to stop the rails in a method and test if the syntax is correct (the webpage will not change move unless I exit pry). This helped a lot when debugging errors because it was more efficient than writing all info from params in the rails console.

@dHelmgren
Copy link

Media Ranker

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good
Comprehension questions Yes
General
Rails fundamentals (RESTful routing, use of named paths) Generally good, see comments
Views are well-organized (DRY, use of semantic HTML, use of partials) Mostly true, but see comments!
Errors are reported to the user Yes
Business logic lives in the models Business logic is missing.
Models are thoroughly tested, including relations, validations and any custom logic Tests could be more robust.
Wave 1 - Media
Splash page shows the three media categories No
Basic CRUD operations on media are present and functional Mostly, but there are a few bugs, see comments.
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, but see comments
All media lists are ordered by vote count No.
Splash page contains a media spotlight No.
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 No, but not important
Overall The parts of the project that you were able to finish work very well and are cleverly designed, but it feels to me like you ran out of time. If there is anything I would suggest that you practice, it's writing business logic similar to the Top Ten code that was required for this project. Keep up the good work!

get 'users/create'
get 'users/new'
get 'users/show'
get 'users/destroy'

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 these routes for users - only index and show are needed for this project.

<% @album.each do |work| %>
<tr>
<td><%=work.votes.length%></td>
<td><%= link_to "#{work.title}", work_path(work.id) %></td><td>

Choose a reason for hiding this comment

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

You have the same code to show a list of works repeated 3 times. Could you use a view partial or a loop to DRY this up?

@@ -0,0 +1,6 @@
class Vote < ApplicationRecord
validates :user, uniqueness: { scope: :user_id,
message: "should happen once per user" }

Choose a reason for hiding this comment

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

I think you misinterpreted the request we had. We wanted person to be able to vote for more than one work, but we didn't want users to be able to vote for a single work more than once.

Put another way, if I upvote DAMN by Kendrick Lamar, I can still upvote Titanic, but I can't vote for DAMN again.

@@ -0,0 +1,8 @@
class Work < ApplicationRecord
has_many :votes
CATEGORIES = %w(album book movie)

Choose a reason for hiding this comment

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

Nice job with this, you can actually leverage this EVEN MORE! Think about what this gives you on your works/index page!


it "must be valid" do
value(vote).must_be :valid?
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

expect(work).must_equal :valid?
end
end

Choose a reason for hiding this comment

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

This is where I would expect tests for the code that picks your top 10 works to live.

For implementing those 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 you category and top ten methods, 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