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

MediaRanker - Divya #26

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

MediaRanker - Divya #26

wants to merge 26 commits into from

Conversation

Naltrexone
Copy link

@Naltrexone Naltrexone commented Oct 15, 2018

Media Ranker

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Describe a custom model method you wrote. Wrote 'self.top' and 'self.most_voted' methods in 'work' model to arrange media according to votes and to find the top 10
Describe how you approached testing that model method. What edge cases did you come up with? I first tested the normal scenarios. I then tested for edge cases to check for probable mistakes. Then I tried testing for unusual edge cases. some edge cases I tested were - invalid user, duplicate username, voting without logging in etc.
What are session and flash? What is the difference between them? Session is used to keep track of the current logged in user. The value from the sessions hash can be used across the rails application while the user is logged in. Flash is used to display messages to the user regarding the status of some action performed. It lasts until the page is refreshed.
Describe a controller filter you wrote. I used the find_work filter in Works controller for show, edit, update and destroy methods. It helps DRY up the code and it helps find an instance of work by the id in params hash.
What was one thing that you gained more clarity on through this assignment? The workings of the rails application in its entirety has become clearer to me. The circuit followed in request response cycle is clearer in my mind.
What is the Heroku URL of your deployed application? https://divya-mediaranker.herokuapp.com/
Do you have any recommendations on how we could improve this project for the next cohort? A little more practice with Heroku deployment and problems we may encounter.

@CheezItMan
Copy link

Media Ranker

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good number of commits, good commit messages
Comprehension questions Check, I'm glad the response-cycle is more clear now.
General
Rails fundamentals (RESTful routing, use of named paths) You should default to using link_to and named routes instead of hardcoded anchor tags. You can also style link_to with the same classes an anchor tags.
Views are well-organized (DRY, use of semantic HTML, use of partials) You could make better use of partial views to render some of the tables.
Errors are reported to the user You're not telling the user what went wrong with flash notices, and the update action doesn't do flash notices at all.
Business logic lives in the models You have some business logic in the models, but it would make sense to have one that pulls works by category.
Models are thoroughly tested, including relations, validations and any custom logic Overall, not bad, you do need more validations added however.
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 Yes, but you printed the work title instead of the user name.
Individual user pages and the user list are present Check
Optional - Styling
Bootstrap is used appropriately Overall well done with Bootstrap
Look and feel is similar to the original Check
Overall Not too bad, but you need to try to avoid using anchor tags and hardcoded routes. You also needed more validations and their accompanying tests. You did hit most of the learning goals and demonstrated a good understanding of validations, testing and custom methods.


resources :works

resources :users, only: [:show, :index, :new, :create]

Choose a reason for hiding this comment

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

Since the sessions controller is doing the creating and the login form, do you really need the :new and :create actions here?

post '/login', to: 'sessions#create'
delete '/logout', to: 'sessions#destroy'

post 'works/:id/upvote', to: 'votes#upvote', as: 'new_vote'

Choose a reason for hiding this comment

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

You could also consider just nesting the votes#create action in the works routes.

validates :user_id, presence: true, uniqueness: {
scope: :work_id
}
validates :work_id, presence: true

Choose a reason for hiding this comment

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

Since a Vote belongs_to a Work, this requirement is there by default.

def self.top(num)
self.most_voted[0..(num-1)]
end

Choose a reason for hiding this comment

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

Having a works_by_category method would be very helpful as well.

class Work < ApplicationRecord
has_many :votes

validates :title, presence: true, uniqueness: true

Choose a reason for hiding this comment

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

no validations on categories, or publication date etc?


<header class="app-header__header">
<h1>
<a href="/">Media Ranker</a>

Choose a reason for hiding this comment

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

Instead of using anchor tags, use link_to and named routes. It makes things easier to adjust.


<% if session[:user_id] %>
<li class="nav-item app-header__nav_item">
<a class="btn btn-primary"

Choose a reason for hiding this comment

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

You don't need the anchor tag here. Since link_to generates an anchor tag, you can style it with Bootstrap like this:

<%= link_to "Logged in as #{@user.username}", user_path(@user.id), class: "btn btn-primary" %>


result = user2.valid?

result.must_equal false

Choose a reason for hiding this comment

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

You should also verify that the username field is the problem.

result.count.must_equal expected_count
end

it "it returns a smaller collection if the argument is bigger than the number of works available" do

Choose a reason for hiding this comment

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

If the test deals with the number of items, you should test the number of items.

You should also test if there are no works.

</thead>
<tbody>
<tr>
<td><a href="/users/#{@work.id}"><%[email protected]%></a></td>

Choose a reason for hiding this comment

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

You're printing the work title here not the user name.

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