posts: allow toggling between upvotes and downvotes.

Like 9efb374ae, allow users to toggle between upvoting and downvoting a
post without raising an error or having to manually remove the vote
first. If you upvote a post, then downvote it, the upvote is
automatically removed and replaced by the downvote.

Other changes:

* Tagging a post with `upvote:self` or `downvote:self` is now silently
  ignored when the user doesn't have permission to vote, instead of
  raising an error.
* Undoing a vote that doesn't exist now does nothing instead of
  returning an error. This can happen if you open the same post in two
  tabs, undo the vote in tab 1, then try to undo the vote again in tab 2.

Changes to the /post_votes API:

* `POST /post_votes` and `DELETE /post_votes` now return a post vote
  instead of a post.
* The `score` param in `POST /post_votes` is now 1 or -1, not `up` or
  `down`.

app/components/post_votes_component/post_votes_component.html.erb

@@ -3,7 +3,7 @@
     <% if upvoted? %>
       <%= link_to upvote_icon, post_post_votes_path(post_id: post.id), class: "post-upvote-link post-unvote-link active-link", method: :delete, remote: true %>
     <% else %>
-      <%= link_to upvote_icon, post_post_votes_path(post_id: post.id, score: "up"), class: "post-upvote-link inactive-link", method: :post, remote: true %>
+      <%= link_to upvote_icon, post_post_votes_path(post_id: post.id, score: 1), class: "post-upvote-link inactive-link", method: :post, remote: true %>
     <% end %>
   <% end %>
 
@@ -13,7 +13,7 @@
     <% if downvoted? %>
       <%= link_to downvote_icon, post_post_votes_path(post_id: post.id), class: "post-downvote-link post-unvote-link active-link", method: :delete, remote: true %>
     <% else %>
-      <%= link_to downvote_icon, post_post_votes_path(post_id: post.id, score: "down"), class: "post-downvote-link inactive-link", method: :post, remote: true %>
+      <%= link_to downvote_icon, post_post_votes_path(post_id: post.id, score: -1), class: "post-downvote-link inactive-link", method: :post, remote: true %>
     <% end %>
   <% end %>
 </span>

app/controllers/post_votes_controller.rb

@@ -1,7 +1,6 @@
 class PostVotesController < ApplicationController
   skip_before_action :api_check
   respond_to :js, :json, :xml, :html
-  rescue_with PostVote::Error, status: 422
 
   def index
     @post_votes = authorize PostVote.visible(CurrentUser.user).paginated_search(params, count_pages: true)
@@ -11,16 +10,23 @@ class PostVotesController < ApplicationController
   end
 
   def create
-    @post = authorize Post.find(params[:post_id]), policy_class: PostVotePolicy
+    @post = Post.find(params[:post_id])
-    @post.vote!(params[:score])
 
-    respond_with(@post)
+    @post.with_lock do
+      @post_vote = authorize PostVote.new(post: @post, score: params[:score], user: CurrentUser.user)
+      PostVote.where(post: @post, user: CurrentUser.user).destroy_all
+      @post_vote.save
+    end
+
+    flash.now[:notice] = @post_vote.errors.full_messages.join("; ") if @post_vote.errors.present?
+    respond_with(@post_vote)
   end
 
   def destroy
-    @post = authorize Post.find(params[:post_id]), policy_class: PostVotePolicy
+    @post = Post.find(params[:post_id])
-    @post.unvote!
+    @post_vote = @post.votes.find_by(user: CurrentUser.user)
 
-    respond_with(@post)
+    authorize(@post_vote).destroy if @post_vote
+    respond_with(@post_vote)
   end
 end

app/models/comment_vote.rb

@@ -2,9 +2,8 @@ class CommentVote < ApplicationRecord
   belongs_to :comment
   belongs_to :user
 
-  validates_presence_of :score
+  validates :user_id, uniqueness: { scope: :comment_id, message: "have already voted for this comment" }
-  validates_uniqueness_of :user_id, :scope => :comment_id, :message => "have already voted for this comment"
+  validates :score, inclusion: { in: [-1, 1], message: "must be 1 or -1" }
-  validates_inclusion_of :score, :in => [-1, 1], :message => "must be 1 or -1"
 
   after_create :update_score_after_create
   after_destroy :update_score_after_destroy

app/models/post.rb

@@ -633,7 +633,8 @@ class Post < ApplicationRecord
           remove_favorite(CurrentUser.user)
 
         when /^(up|down)vote:(.+)$/i
-          vote!($1)
+          score = ($1 == "up" ? 1 : -1)
+          vote!(score, CurrentUser.user)
 
         when /^status:active$/i
           raise User::PrivilegeError unless CurrentUser.is_approver?
@@ -779,8 +780,7 @@ class Post < ApplicationRecord
 
     def add_favorite!(user)
       Favorite.add(post: self, user: user)
-      vote!("up", user) if Pundit.policy!(user, PostVote).create?
+      vote!(1, user)
-    rescue PostVote::Error
     end
 
     def delete_user_from_fav_string(user_id)
@@ -789,8 +789,7 @@ class Post < ApplicationRecord
 
     def remove_favorite!(user)
       Favorite.remove(post: self, user: user)
-      unvote!(user) if Pundit.policy!(user, PostVote).create?
+      unvote!(user)
-    rescue PostVote::Error
     end
 
     def remove_favorite(user)
@@ -871,30 +870,22 @@ class Post < ApplicationRecord
   end
 
   module VoteMethods
-    def can_be_voted_by?(user)
+    def vote!(score, voter)
-      !PostVote.exists?(:user_id => user.id, :post_id => id)
+      # Ignore vote if user doesn't have permission to vote.
+      return unless Pundit.policy!(voter, PostVote).create?
+
+      with_lock do
+        votes.destroy_by(user: voter)
+        votes.create!(user: voter, score: score)
+        reload # PostVote.create modifies our score. Reload to get the new score.
+      end
     end
 
-    def vote!(vote, voter = CurrentUser.user)
+    def unvote!(voter)
-      unless Pundit.policy!(voter, PostVote).create?
+      return unless Pundit.policy!(voter, PostVote).create?
-        raise PostVote::Error.new("You do not have permission to vote")
-      end
 
-      unless can_be_voted_by?(voter)
+      votes.destroy_by(user: voter)
-        raise PostVote::Error.new("You have already voted for this post")
+      reload
-      end
-
-      votes.create!(user: voter, vote: vote)
-      reload # PostVote.create modifies our score. Reload to get the new score.
-    end
-
-    def unvote!(voter = CurrentUser.user)
-      if can_be_voted_by?(voter)
-        raise PostVote::Error.new("You have not voted for this post")
-      else
-        votes.where(user: voter).destroy_all
-        reload
-      end
     end
   end
 

app/models/post_vote.rb

@@ -1,15 +1,12 @@
 class PostVote < ApplicationRecord
-  class Error < StandardError; end
-
   belongs_to :post
   belongs_to :user
-  attr_accessor :vote
 
-  after_initialize :initialize_attributes, if: :new_record?
+  validates :user_id, uniqueness: { scope: :post_id, message: "have already voted for this post" }
-  validates_presence_of :score
+  validates :score, inclusion: { in: [1, -1], message: "must be 1 or -1" }
-  validates_inclusion_of :score, in: [1, -1]
+
-  after_create :update_post_on_create
+  after_create :update_score_after_create
-  after_destroy :update_post_on_destroy
+  after_destroy :update_score_after_destroy
 
   scope :positive, -> { where("post_votes.score > 0") }
   scope :negative, -> { where("post_votes.score < 0") }
@@ -23,16 +20,6 @@ class PostVote < ApplicationRecord
     q.apply_default_order(params)
   end
 
-  def initialize_attributes
-    self.user_id ||= CurrentUser.id
-
-    if vote == "up"
-      self.score = 1
-    elsif vote == "down"
-      self.score = -1
-    end
-  end
-
   def is_positive?
     score > 0
   end
@@ -41,19 +28,19 @@ class PostVote < ApplicationRecord
     score < 0
   end
 
-  def update_post_on_create
+  def update_score_after_create
-    if score > 0
+    if is_positive?
-      Post.where(:id => post_id).update_all("score = score + #{score}, up_score = up_score + #{score}")
+      Post.update_counters(post_id, { score: score, up_score: score })
     else
-      Post.where(:id => post_id).update_all("score = score + #{score}, down_score = down_score + #{score}")
+      Post.update_counters(post_id, { score: score, down_score: score })
     end
   end
 
-  def update_post_on_destroy
+  def update_score_after_destroy
-    if score > 0
+    if is_positive?
-      Post.where(:id => post_id).update_all("score = score - #{score}, up_score = up_score - #{score}")
+      Post.update_counters(post_id, { score: -score, up_score: -score })
     else
-      Post.where(:id => post_id).update_all("score = score - #{score}, down_score = down_score - #{score}")
+      Post.update_counters(post_id, { score: -score, down_score: -score })
     end
   end
 

app/policies/post_vote_policy.rb

@@ -4,6 +4,6 @@ class PostVotePolicy < ApplicationPolicy
   end
 
   def destroy?
-    unbanned? && user.is_gold?
+    unbanned? && record.user == user
   end
 end

app/views/post_votes/create.js.erb

@@ -1 +1 @@
-$("span.post-votes[data-id=<%= @post.id %>]").replaceWith("<%= j render_post_votes @post, current_user: CurrentUser.user %>");
+$("span.post-votes[data-id=<%= @post.reload.id %>]").replaceWith("<%= j render_post_votes @post, current_user: CurrentUser.user %>");

test/components/post_votes_component_test.rb

@@ -34,7 +34,7 @@ class PostVotesComponentTest < ViewComponent::TestCase
 
       context "for a downvoted post" do
         should "highlight the downvote button as active" do
-          @post.vote!("down", @user)
+          @post.vote!(-1, @user)
           render_post_votes(@post, current_user: @user)
 
           assert_css(".post-upvote-link.inactive-link")
@@ -44,7 +44,7 @@ class PostVotesComponentTest < ViewComponent::TestCase
 
       context "for an upvoted post" do
         should "highlight the upvote button as active" do
-          @post.vote!("up", @user)
+          @post.vote!(1, @user)
           render_post_votes(@post, current_user: @user)
 
           assert_css(".post-upvote-link.active-link")

test/functional/post_votes_controller_test.rb

@@ -46,52 +46,94 @@ class PostVotesControllerTest < ActionDispatch::IntegrationTest
 
     context "create action" do
       should "not allow anonymous users to vote" do
-        post post_post_votes_path(post_id: @post.id), params: {:score => "up", :format => "js"}
+        post post_post_votes_path(post_id: @post.id), params: { score: 1, format: "js" }
+
         assert_response 403
         assert_equal(0, @post.reload.score)
       end
 
       should "not allow banned users to vote" do
-        @banned = create(:user)
+        post_auth post_post_votes_path(post_id: @post.id), create(:banned_user), params: { score: 1, format: "js"}
-        @ban = create(:ban, user: @banned)
+
-        post_auth post_post_votes_path(post_id: @post.id), @banned, params: {:score => "up", :format => "js"}
         assert_response 403
         assert_equal(0, @post.reload.score)
       end
 
       should "not allow members to vote" do
-        @member = create(:member_user)
+        post_auth post_post_votes_path(post_id: @post.id), create(:user), params: { score: 1, format: "js" }
-        post_auth post_post_votes_path(post_id: @post.id), @member, params: {:score => "up", :format => "js"}
+
         assert_response 403
         assert_equal(0, @post.reload.score)
       end
 
+      should "not allow invalid scores" do
+        post_auth post_post_votes_path(post_id: @post.id), @user, params: { score: 3, format: "js" }
+
+        assert_response 200
+        assert_equal(0, @post.reload.score)
+        assert_equal(0, @post.up_score)
+        assert_equal(0, @post.votes.count)
+      end
+
       should "increment a post's score if the score is positive" do
-        post_auth post_post_votes_path(post_id: @post.id), @user, params: {:score => "up", :format => "js"}
+        post_auth post_post_votes_path(post_id: @post.id), @user, params: { score: 1, format: "js" }
+
         assert_response :success
-        @post.reload
+        assert_equal(1, @post.reload.score)
-        assert_equal(1, @post.score)
+        assert_equal(1, @post.up_score)
+        assert_equal(1, @post.votes.count)
+      end
+
+      should "decrement a post's score if the score is negative" do
+        post_auth post_post_votes_path(post_id: @post.id), @user, params: { score: -1, format: "js" }
+
+        assert_response :success
+        assert_equal(-1, @post.reload.score)
+        assert_equal(-1, @post.down_score)
+        assert_equal(1, @post.votes.count)
       end
 
       context "for a post that has already been voted on" do
-        should "not create another vote" do
+        should "replace the vote" do
-          @post.vote!("up", @user)
+          @post.vote!(1, @user)
-          assert_no_difference("PostVote.count") do
+
-            post_auth post_post_votes_path(post_id: @post.id), @user, params: { score: "up", format: "js" }
+          assert_no_difference("@post.votes.count") do
-            assert_response 422
+            post_auth post_post_votes_path(post_id: @post.id), @user, params: { score: -1, format: "js" }
+
+            assert_response :success
+            assert_equal(-1, @post.reload.score)
+            assert_equal(0, @post.up_score)
+            assert_equal(-1, @post.down_score)
           end
         end
       end
     end
 
     context "destroy action" do
-      should "remove a vote" do
+      should "do nothing for anonymous users" do
-        as(@user) { create(:post_vote, post_id: @post.id, user_id: @user.id) }
+        delete post_post_votes_path(post_id: @post.id), xhr: true
 
-        assert_difference("PostVote.count", -1) do
+        assert_response 200
-          delete_auth post_post_votes_path(post_id: @post.id), @user, as: :javascript
+        assert_equal(0, @post.reload.score)
-          assert_redirected_to @post
+      end
-        end
+
+      should "do nothing if the post hasn't been voted on" do
+        delete_auth post_post_votes_path(post_id: @post.id), @user, xhr: true
+
+        assert_response :success
+        assert_equal(0, @post.reload.score)
+        assert_equal(0, @post.down_score)
+        assert_equal(0, @post.votes.count)
+      end
+
+      should "remove a vote" do
+        @post.vote!(1, @user)
+        delete_auth post_post_votes_path(post_id: @post.id), @user, xhr: true
+
+        assert_response :success
+        assert_equal(0, @post.reload.score)
+        assert_equal(0, @post.down_score)
+        assert_equal(0, @post.votes.count)
       end
     end
   end

test/unit/post_test.rb

@@ -1162,19 +1162,19 @@ class PostTest < ActiveSupport::TestCase
           context "upvote:self or downvote:self" do
             context "by a member" do
               should "not upvote the post" do
-                assert_raises PostVote::Error do
+                assert_no_difference("PostVote.count") do
-                  @post.update(:tag_string => "upvote:self")
+                  @post.update(tag_string: "upvote:self")
                 end
 
-                assert_equal(0, @post.score)
+                assert_equal(0, @post.reload.score)
               end
 
               should "not downvote the post" do
-                assert_raises PostVote::Error do
+                assert_no_difference("PostVote.count") do
-                  @post.update(:tag_string => "downvote:self")
+                  @post.update(tag_string: "downvote:self")
                 end
 
-                assert_equal(0, @post.score)
+                assert_equal(0, @post.reload.score)
               end
             end
 
@@ -1832,50 +1832,63 @@ class PostTest < ActiveSupport::TestCase
 
   context "Voting:" do
     should "not allow members to vote" do
-      @user = FactoryBot.create(:user)
+      user = create(:user)
-      @post = FactoryBot.create(:post)
+      post = create(:post)
-      as(@user) do
+
-        assert_raises(PostVote::Error) { @post.vote!("up") }
+      assert_nothing_raised { post.vote!(1, user) }
-      end
+      assert_equal(0, post.votes.count)
+      assert_equal(0, post.reload.score)
     end
 
     should "not allow duplicate votes" do
-      user = FactoryBot.create(:gold_user)
+      user = create(:gold_user)
-      post = FactoryBot.create(:post)
+      post = create(:post)
-      CurrentUser.scoped(user, "127.0.0.1") do
+
-        assert_nothing_raised {post.vote!("up")}
+      post.vote!(1, user)
-        assert_raises(PostVote::Error) {post.vote!("up")}
+      post.vote!(1, user)
-        post.reload
+
-        assert_equal(1, PostVote.count)
+      assert_equal(1, post.reload.score)
-        assert_equal(1, post.score)
+      assert_equal(1, post.votes.count)
-      end
     end
 
     should "allow undoing of votes" do
-      user = FactoryBot.create(:gold_user)
+      user = create(:gold_user)
-      post = FactoryBot.create(:post)
+      post = create(:post)
 
       # We deliberately don't call post.reload until the end to verify that
       # post.unvote! returns the correct score even when not forcibly reloaded.
-      CurrentUser.scoped(user, "127.0.0.1") do
+      post.vote!(1, user)
-        post.vote!("up")
+      assert_equal(1, post.score)
-        assert_equal(1, post.score)
+      assert_equal(1, post.up_score)
+      assert_equal(0, post.down_score)
+      assert_equal(1, post.votes.positive.count)
 
-        post.unvote!
+      post.unvote!(user)
-        assert_equal(0, post.score)
+      assert_equal(0, post.score)
+      assert_equal(0, post.up_score)
+      assert_equal(0, post.down_score)
+      assert_equal(0, post.votes.count)
 
-        assert_nothing_raised {post.vote!("down")}
+      post.vote!(-1, user)
-        assert_equal(-1, post.score)
+      assert_equal(-1, post.score)
+      assert_equal(0, post.up_score)
+      assert_equal(-1, post.down_score)
+      assert_equal(1, post.votes.negative.count)
 
-        post.unvote!
+      post.unvote!(user)
-        assert_equal(0, post.score)
+      assert_equal(0, post.score)
+      assert_equal(0, post.up_score)
+      assert_equal(0, post.down_score)
+      assert_equal(0, post.votes.count)
 
-        assert_nothing_raised {post.vote!("up")}
+      post.vote!(1, user)
-        assert_equal(1, post.score)
+      assert_equal(1, post.score)
+      assert_equal(1, post.up_score)
+      assert_equal(0, post.down_score)
+      assert_equal(1, post.votes.positive.count)
 
-        post.reload
+      post.reload
-        assert_equal(1, post.score)
+      assert_equal(1, post.score)
-      end
     end
   end
 

test/unit/post_vote_test.rb

@@ -1,46 +1,74 @@
 require 'test_helper'
 
 class PostVoteTest < ActiveSupport::TestCase
-  def setup
+  context "A PostVote" do
-    super
+    setup do
-
+      @post = create(:post)
-    @user = FactoryBot.create(:user)
-    CurrentUser.user = @user
-    CurrentUser.ip_addr = "127.0.0.1"
-
-    @post = FactoryBot.create(:post)
-  end
-
-  context "Voting for a post" do
-    should "interpret up as +1 score" do
-      vote = PostVote.create(:post_id => @post.id, :vote => "up")
-      assert_equal(1, vote.score)
     end
 
-    should "interpret down as -1 score" do
+    context "during validation" do
-      vote = PostVote.create(:post_id => @post.id, :vote => "down")
+      subject { build(:post_vote, post: @post) }
-      assert_equal(-1, vote.score)
+
+      should validate_uniqueness_of(:user_id).scoped_to(:post_id).with_message("have already voted for this post")
+      should validate_inclusion_of(:score).in_array([-1, 1]).with_message("must be 1 or -1")
     end
 
-    should "not accept any other scores" do
+    context "creating" do
-      vote = PostVote.create(:post_id => @post.id, :vote => "xxx")
+      context "an upvote" do
-      assert(vote.errors.any?)
+        should "increment the post's score" do
+          vote = create(:post_vote, post: @post, score: 1)
+
+          assert_equal(1, @post.reload.score)
+          assert_equal(1, @post.up_score)
+          assert_equal(0, @post.down_score)
+          assert_equal(1, @post.votes.positive.count)
+        end
+      end
+
+      context "a downvote" do
+        should "decrement the post's score" do
+          vote = create(:post_vote, post: @post, score: -1)
+
+          assert_equal(-1, @post.reload.score)
+          assert_equal(0, @post.up_score)
+          assert_equal(-1, @post.down_score)
+          assert_equal(1, @post.votes.negative.count)
+        end
+      end
     end
 
-    should "increase the score of the post" do
+    context "destroying" do
-      @post.votes.create(vote: "up")
+      context "an upvote" do
-      @post.reload
+        should "decrement the post's score" do
+          vote = create(:post_vote, post: @post, score: 1)
+          assert_equal(1, @post.reload.score)
+          assert_equal(1, @post.up_score)
+          assert_equal(0, @post.down_score)
+          assert_equal(1, @post.votes.count)
 
-      assert_equal(1, @post.score)
+          vote.destroy
-      assert_equal(1, @post.up_score)
+          assert_equal(0, @post.reload.score)
-    end
+          assert_equal(0, @post.up_score)
+          assert_equal(0, @post.down_score)
+          assert_equal(0, @post.votes.count)
+        end
+      end
 
-    should "decrease the score of the post when removed" do
+      context "a downvote" do
-      @post.votes.create(vote: "up").destroy
+        should "increment the post's score" do
-      @post.reload
+          vote = create(:post_vote, post: @post, score: -1)
+          assert_equal(-1, @post.reload.score)
+          assert_equal(0, @post.up_score)
+          assert_equal(-1, @post.down_score)
+          assert_equal(1, @post.votes.count)
 
-      assert_equal(0, @post.score)
+          vote.destroy
-      assert_equal(0, @post.up_score)
+          assert_equal(0, @post.reload.score)
+          assert_equal(0, @post.up_score)
+          assert_equal(0, @post.down_score)
+          assert_equal(0, @post.votes.count)
+        end
+      end
     end
   end
 end