Skip to content

Ampers: Nora Peters#35

Open
npeters5 wants to merge 74 commits intoAda-C9:masterfrom
npeters5:master
Open

Ampers: Nora Peters#35
npeters5 wants to merge 74 commits intoAda-C9:masterfrom
npeters5:master

Conversation

@npeters5
Copy link
Copy Markdown

Media Ranker

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Describe a custom model method you wrote. I wrote a number of class methods in my work model, including one called self.top_ranked. This method is called in my welcomes controller, and sorts collections of Work instances (by category) in order from most votes to least votes.
Describe how you approached testing that model method. What edge cases did you come up with? I'm still working on it. So for I'm only testing nominal cases. Also having trouble changing the methods to test for the presence of votes before trying to group and sort them by vote.
What are session and flash? What is the difference between them? Session and flash are both hash-like objects in Rails. Session allows you to persist data (such as id of current user) between HTTP requests without necessarily using the database. Flash behaves the same way, except that its data is persisted for only one HTTP request.
Describe a controller filter you wrote. In the application controller I wrote a find_user method, and I implemented a before_action controller filter in my votes controller, before all actions. This way, the current session's user gets connected to the vote when its created.
What was one thing that you gained more clarity on through this assignment? I gained a lot more clarity on git branching and merging, building some model logic, session and flash and controller filters, and of course testing but I have a long way to go to understand it better.
What is the Heroku URL of your deployed application? https://np-media-ranker.herokuapp.com/
Do you have any recommendations on how we could improve this project for the next cohort? I liked this project a lot - it was challenging and fun. However since a lot of us are struggling with writing tests, I might try to spend more lecture time on it or something.

npeters5 added 30 commits April 9, 2018 14:15
…te button. implements vote_count method in work model.
…_to upvote button on works#index view with post request
…ow controller action and view + #pretty_date app helper method
npeters5 added 28 commits April 13, 2018 14:51
@CheezItMan
Copy link
Copy Markdown

Media Ranker

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good number of commits and good commit messages
Comprehension questions Check, I'll bring up doing more review on tests with Dee
General
Rails fundamentals (RESTful routing, use of named paths) Some issues here, mostly unused routes for users.
Semantic HTML Check, very nice work!
Errors are reported to the user Some attempt is made here, but when trying to edit or create a work validation errors are not showing. Instead for create I'm getting a ObjectId. I posted a solution inline in your code.
Business logic lives in the models Very clever use of the left join.
Models are thoroughly tested, including relations, validations and any custom logic Missing some edge cases, I left notes in your code.
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
Individual user pages and the user list are present Check
A user cannot vote for the same media more than once Check, but the error message for duplicate votes is awkward.
All media lists are ordered by vote count On the all media page they're not in order by vote count!
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
Overall Well done, some small issues on edge-case testing and some bugs in reporting errors, but overall pretty good, I left notes in your code. Slack me if you have questions.

<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.

Just a suggestion, changing "untitled" to a title for your site.

Comment thread config/routes.rb
post '/login', to: 'sessions#create', as: 'login'
delete '/login', to: 'sessions#destroy', as: 'logout'

resources :users
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do you need destroy, edit, update actions for the users controller?

session[:user_id] = @user.id
flash[:success] = "Welcome back #{@user.username}"
else
@user = User.create username: params[:user][:username]
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 need to check if the create method worked! I tried to log in with a blank username and it said, "Weclome"!

<section class="flash">
<% flash.each do |name, message| %>
<section class="callout <%= name %>">
<% if name == :alert %>
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 should be

<% if name == "alert" %>

Comment thread app/models/vote.rb

validates :user_id, presence: true
validates :work_id, presence: true
validates :work_id, uniqueness: { 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.

along with the line below, this is redundant.

Comment thread app/models/work.rb
end

def self.top_ranked
left_joins(:votes)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🥇

Comment thread test/models/user_test.rb
user.must_respond_to :username
users(:chad).username.must_equal "Chad"

user.valid?.must_equal false
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 also test the username uniqueness.

Comment thread test/models/vote_test.rb
votes(:vote_five).user.id.must_equal todd.id
end

it 'must have one work' 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.

You should also test that you can't create duplicate votes.

Comment thread test/models/work_test.rb
work.valid?.must_equal false
work.errors.must_include :publication_year

work = Work.create(category: 'book', title: 'work', publication_year: 1999)
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 also try this with a 5 digit publication year.

Comment thread test/models/work_test.rb
end

describe "#top_ranked" do
it "must return the work with highest number of votes" 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.

You should also test and see what happens when there are no works and when there are no 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