Skip to content

Selamawit Ainalem - Amperes#28

Open
SelamawitA wants to merge 28 commits intoAda-C9:masterfrom
SelamawitA:master
Open

Selamawit Ainalem - Amperes#28
SelamawitA wants to merge 28 commits intoAda-C9:masterfrom
SelamawitA:master

Conversation

@SelamawitA
Copy link
Copy Markdown

@SelamawitA SelamawitA commented Apr 14, 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 the works model to display the top ten work items. Using the sort_by and iterating through each work item I was able to re-arrange the hash of objects by the number of votes. I added the reverse method to have it in descending order and used the first() method to pull ten items from the list although now thinking about it I could have also used a range to pull the the sub-array after it had been sorted.
Describe how you approached testing that model method. What edge cases did you come up with? I tested around validations for edge cases. With cases like "must be present" i tested multiple cases where an empty value could be input like nil input,empty string, or an empty object instantiation.
What are session and flash? What is the difference between them? Session is a hash generated to track a user's data throughout their presence on a site unlike flash it continues to track their information through different views and only expires once they leave the browser, the information is consistently available. With flash, once the message is displayed it is gone.
Describe a controller filter you wrote. I wrote a filter to search for the session message, I wanted the logged in status to be available for every view page and instead of having that code repeated throughout each method on every controller. I took advantage of both the polymorphic relationship between the application_controller and the remaining controllers and the filter method to DRY up my code.
What was one thing that you gained more clarity on through this assignment? I got a better understanding of session, flash, routes, and CRUD.
What is the Heroku URL of your deployed application? https://media-list-selamawit.herokuapp.com/works
Do you have any recommendations on how we could improve this project for the next cohort? This was a really fun assignment, I think CSS is always fun but I wish I could have used that time to getting a deeper understanding of rails concepts. Maybe add more requirements/make it more in depth? With a bit more time I would have liked to updated my project with nested routes, they still confuse me!

… and test for a user to not be able to vote for the same work more than once (test not functioning properly yet.)
…Added another button that will re-direct back to list of all media
… User is alerted to a successful login by a flash message
…tus. Also updated the sessions controller instance method name.
@CheezItMan
Copy link
Copy Markdown

Media Ranker

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good git hygiene
Comprehension questions Check, I'm glad you enjoyed the CSS and while we'd like more time to get familiar with Rails concepts you'll have bEtsy and the 2 API weeks to learn more of that, CSS is important too.
General
Rails fundamentals (RESTful routing, use of named paths) Check
Semantic HTML Check
Errors are reported to the user Check, When you create a work and a validation fails it doesn't inform you which field can't be blank.
Business logic lives in the models Check
Models are thoroughly tested, including relations, validations and any custom logic Some issues with testing, some edge-cases missing and some mixing of assert-style and spec-style testing.
Wave 1 - Media
Splash page shows the three media categories Only the spotlight showing, you have a capitalization problem in your homepage view.
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
Individual user pages and the user list are present Check
A user cannot vote for the same media more than once Check
All media lists are ordered by vote count On the homepage yes, on the all media page, no.
Splash page contains a media spotlight Check
Media pages contain lists of voting users Check
Wave 3 - Styling
Foundation is used appropriately Check
Look and feel is similar to the original Looks pretty close, not perfect, but close.
Overall Not bad, there are some issues, some testing is incomplete and you mixed testing styles. You also have a bug in your homepage just because your view has the categories capitalized. These are not critical, but should be avoided in bEtsy. Let me know if you have questions. I left some comments inline in your code as well. You hit all the major learning goals, nice work.

<meta charset="utf-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />

<title><%= content_for?(:title) ? yield(:title) : "Untitled" %></title>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe change "untitled" to something for your site.

<%= csrf_meta_tags %>
</head>

<section id="intro">
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this section should be inside the body element.

Comment thread test/models/user_test.rb

it 'An empty input will violate the presence validation' do
user = User.new()
assert_not user.valid?
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're mixing assert-style and spec-style testing here.

Comment thread test/models/vote_test.rb
end

it 'must be able to display associated users' do
vote.users[0].name.must_equal "Noe Body"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since a vote should belong to only one user, this should be.

vote.user.name.must_equal "Noe Body"

Comment thread test/models/work_test.rb

end

it 'will return the highest rated work item' do
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 would suggest grouping each custom method inside it's own describe block.

I would also test the method when there are no votes in the system.

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

validates_uniqueness_of :work_id, :scope => [:user_id]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍

class HomepageController < ApplicationController
before_action :find_user
def index
@votes_hmp = Vote.all
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 should probably find each group of works by category.

<section class="top-ten">
<h3>Top Movies</h3>
<ul>
<%Work.where(category: "Movie").top_ten.each do |top_movie|%>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Here "Movie", "Book" and "Album" shouldn't be capitalized!

Also this should be in the controller not the view.

<th width="100">Upvotes</th>
</tr>
</thead>
<%Work.where(category:'album').each do |album|%>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shouldn't these be ordered by the number of votes?

session[:user_id] = @username.id
flash[:success] = "Successfully logged in as exisisting user #{@username.name} with ID #{@username.id}"
else
@username = User.create name: params[:user][:name]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Some checks to make sure that create worked properly would be better.

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