Don't merge - review comments

kiwi/Gemfile

@@ -53,3 +53,4 @@ group :development do
   gem 'web-console', '~> 2.0'
 end
 
+gem 'simplecov', :require => false, :group => :test

kiwi/Gemfile.lock

@@ -59,6 +59,7 @@ GEM
     database_cleaner (1.5.1)
     debug_inspector (0.0.2)
     diff-lcs (1.2.5)
+    docile (1.1.5)
     erubis (2.7.0)
     execjs (2.6.0)
     factory_girl (4.5.0)
@@ -160,6 +161,11 @@ GEM
       rdoc (~> 4.0)
     shoulda-matchers (2.8.0)
       activesupport (>= 3.0.0)
+    simplecov (0.11.1)
+      docile (~> 1.1.0)
+      json (~> 1.8)
+      simplecov-html (~> 0.10.0)
+    simplecov-html (0.10.0)
     slop (3.6.0)
     sprockets (3.5.2)
       concurrent-ruby (~> 1.0)
@@ -206,6 +212,7 @@ DEPENDENCIES
   sass-rails (~> 5.0)
   sdoc (~> 0.4.0)
   shoulda-matchers
+  simplecov
   uglifier (>= 1.3.0)
   web-console (~> 2.0)
 

kiwi/app/controllers/answers_controller.rb

@@ -11,6 +11,7 @@ def create
       if @answer.save
         redirect_to question_path(id: @answer.question_id)
       else
+        # Errors are lost on redirect (it's a new request)
         @errors = @answer.errors.full_messages
         redirect_to question_path(id: @answer.question_id)
       end

kiwi/app/controllers/application_controller.rb

@@ -3,14 +3,18 @@ class ApplicationController < ActionController::Base
   # For APIs, you may want to use :null_session instead.
   protect_from_forgery with: :exception
 
+  # This might help
+  # before_action :authorize_user! only: [:new, :create, :update, :destroy]
+  # for exceptions, use skip_before_action in particular controllers where necessary
+
   helper_method :current_user, :logged_in?
 
   def current_user
     @user ||= User.find session[:user_id] if session[:user_id]
   end
 
   def authorize_user!
-    redirect_to new_admin_session_path unless current_user.present?
+    redirect_to new_admin_session_path unless current_user
   end
 
   def login user

kiwi/app/controllers/comments_controller.rb

@@ -9,6 +9,23 @@ def new
       @comment = @commentable.comments.new
   end
 
+=begin
+
+question_comments POST /questions/:question_id/comments(.:format) comments#create
+answer_comments   POST /answers/:answer_id/comments(.:format) comments#create
+
+you could do something like 
+  http://stackoverflow.com/questions/23088709/finding-parent-in-rails-polymorphic-association
+
+def find_commentable
+ resource, id = request.path.split('/')[1, 2]
+ @commentable = resource.singularize.classify.constantize.find(id)
+end
+
+It's a bit clever-clever but makes life simpler if you ever make something else commentable
+
+=end
+
   def create
     if params[:question_id]
       question = Question.find_by(id: params[:question_id])
@@ -31,7 +48,7 @@ def edit
 
   def update
     @comment = Comment.find_by(id: params[:id])
-    if @comment.update_attributes(comment_params)
+    if @comment.update(content: params[:comment][:content])
       redirect_to_question_path
     else
       render :edit
@@ -47,7 +64,8 @@ def destroy
   private
 
     def comment_params
-      params.require(:comment).permit(:content, :commentable_type, :commentable_id, :user_id)
+      # Don't accept user_id  as a param from the browser - can be forged
+      params.require(:comment).permit(:content, :commentable_type, :commentable_id).merge(user: current_user)
     end
 
     def redirect_to_question_path

kiwi/app/controllers/questions_controller.rb

@@ -6,6 +6,7 @@ def index
     elsif params[:sort] == "recent"
       @questions = Question.all.order(:created_at)
     elsif params[:sort] == "rate"
+      # TODO: add a scope on question for this
       @questions = Question.all.sort_by { |question| question.sum_of_votes }.reverse
     else
       @questions = Question.all
@@ -28,8 +29,7 @@ def create
 
   def show
     @question = Question.includes(:answers, :user).find(params[:id])
-    @question.increment(:views)
-    @question.save
+    @question.increment(:views).save
     @answers = @question.sort_answers_by_votes
     @answer = Answer.new
   end

kiwi/app/controllers/votes_controller.rb

@@ -2,7 +2,7 @@ class VotesController < ApplicationController
 
   def create
     @vote = current_user.votes.new(vote_params)
-    if !!params[:question_id]
+    if params[:question_id]
       @question = Question.find(params[:question_id])
       if [email protected]_by(user_id: current_user)
         @vote.votable_type = "Question"
@@ -16,7 +16,7 @@ def create
         @vote.save
         redirect_to question_path(id: @vote.votable_id)
       end
-    elsif !!params[:answer_id]
+    elsif params[:answer_id]
       @answer = Answer.find_by(id: params[:answer_id])
       if [email protected]_by(user_id: current_user)
         @vote.votable_type = "Answer"
@@ -39,6 +39,7 @@ def vote_params
     params.require(:vote).permit(:direction,:user_id)
   end
 
+
 # def vote_type
 #   if !!params[:question_id]
 #       @vote_type = {votable_type: "Question" , votable_id: params[:question_id]}

kiwi/app/models/answer.rb

@@ -9,4 +9,8 @@ class Answer < ActiveRecord::Base
 
   validates_presence_of :content, :user, :question
 
+  #make answer finding a concern of answer
+  def self.best_for_question question
+    find_by(id: question.best_answer_id)
+  end
 end

kiwi/app/models/concerns/votable.rb

@@ -1,10 +1,12 @@
+#I'd rename this to avoid confusion with the association name
 module Votable
+  #Let the database do this stuff
   def sum_of_votes
-    self.votes.inject(0){|sum, vote| sum += vote.direction}
+    votes.sum(:direction)
   end
 
-  def voted?
-    self.votes.find_by(user_id: user_id)
+  def already_voted?(user)
+    self.votes.where(user: user).count > 0
   end
 
 end

kiwi/app/models/question.rb

@@ -3,17 +3,25 @@ class Question < ActiveRecord::Base
 
   belongs_to :user
   has_many :answers
+  has_many :answer_votes, through: :answers, source: :votes
   has_many :comments, as: :commentable
   has_many :votes, as: :votable
 
-  validates_presence_of :title, :content, :user_id
+  #validate object in preference to id
+  validates_presence_of :title, :content, :user
+
+  def comments_by_recency
+    comments.order(created_at: :desc)
+  end
 
   def best_answer
-    Answer.find_by(id: self.best_answer_id)
+    #move the knowledge of the internals of Answer into Answer
+    Answer.best_for_question(self)
   end
 
   def sort_answers_by_votes
-    self.answers.sort_by { |answer| answer.sum_of_votes }.reverse
+    # We can do this (honestly....) in AR
+    answers.joins(:votes).group(:id).order('sum(votes.direction)')
   end
 
 end

kiwi/app/models/user.rb

@@ -7,8 +7,9 @@ class User < ActiveRecord::Base
 
   validates :name, presence: true, length: {in: 8..20}
   validates :email, presence: true
-
-  validates_format_of :email, with: /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\Z/i, :on => :create
+
+  #Be sure you really only want to validate on create
+  validates_format_of :email, with: /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\Z/i
 
 
 end

kiwi/app/views/answers/_answer_show.html.erb

@@ -5,10 +5,10 @@
       <li><%= answer.sum_of_votes %></li>
       <li id="down-vote"><%= link_to '&#x25BC'.html_safe, answer_votes_path(answer, vote: {direction: '-1'}), method: :post %></li>
       <%# binding.pry %>
-      <% if @question.best_answer_id == answer.id %>
+      <% if answer.question.best_answer_id == answer.id %>
         <li>BEST!</li>
       <% else %>
-        <% if @question.user == current_user && !@question.best_answer_id %>
+        <% if answer.question.user == current_user && !answer.question.best_answer_id %>
           <li>
             <%= link_to "Best?", best_answer_path(answer.id), method: :patch %>
           </li>

kiwi/app/views/comments/edit.html.erb

@@ -1,6 +1,6 @@
 <h2>Edit your comment</h2>
 
-<%= form_for @comment do |f| %>
+<%= form_for [@comment.commentable, @comment] do |f| %>
   <fieldset id="form">
 
     <div class="input-options">

kiwi/app/views/questions/show.html.erb

@@ -23,8 +23,8 @@
   <% if [email protected]? %>
     <div class="comments">
       <h5>Comments:</h5>
-      <% @question.comments.order(:created_at).each do |comment| %>
-        <p class="comment"><%= comment.content %> &bull; <span class="user-time"><%= comment.user.name %> <%= std_format_date(comment.created_at) %></span>  <% if current_user == comment.user %> &bull; <%= link_to "Edit", edit_comment_path(comment.id) %> &bull; <%= link_to "Delete",comment_path(comment.id), method: :delete, data: {confirm: 'Are you sure?'}  %><% end %></p>
+      <% @question.comments_by_recency.each do |comment| %>
+        <p class="comment"><%= comment.content %> &bull; <span class="user-time"><%= comment.user.name %> <%= std_format_date(comment.created_at) %></span>  <% if comment.owned_by?(current_user) %> &bull; <%= link_to "Edit", edit_comment_path(comment.id) %> &bull; <%= link_to "Delete",comment_path(comment.id), method: :delete, data: {confirm: 'Are you sure?'}  %><% end %></p>
       <% end %>
     </div>
   <% end %>
@@ -37,7 +37,7 @@
 <% end %>
 
 
-<% if !@answers.empty? %>
+<% if @answers.any? %>
   <div class="answers-list">
     <h4>Answers: <%= @answers.count %></h4>
     <% if @question.best_answer_id %>

kiwi/config/routes.rb

@@ -6,19 +6,19 @@
 
 
  resources :questions do
-  resources :comments, only: [:new, :create]
+  resources :comments, only: [:new, :create, :update]
   resources :answers, only: [:create]
   resources :votes, only: [ :create, :destroy ]
  end
 
  resources :answers do
-  resources :comments, only: [:new, :create]
+  resources :comments, only: [:new, :create, :update]
   resources :votes, only: [:create, :destroy]
   patch 'best', on: :member
  end
 
 
- resources :comments, only: [ :edit, :update, :destroy ]
+ resources :comments, only: [ :edit, :destroy ]
 
   resources :users, only: [ :new, :show, :create ]
   resources :sessions, only: [ :new, :create, :destroy ]

kiwi/spec/rails_helper.rb

@@ -7,7 +7,8 @@
 require 'rspec/rails'
 require "capybara/rspec"
 # Add additional requires below this line. Rails is not loaded until this point!
-
+require 'simplecov'
+SimpleCov.start 'rails'
 # Requires supporting ruby files with custom matchers and macros, etc, in
 # spec/support/ and its subdirectories. Files matching `spec/**/*_spec.rb` are
 # run as spec files by default. This means that files in spec/support that end