diff --git a/app/components/post_votes_component/post_votes_component.html.erb b/app/components/post_votes_component/post_votes_component.html.erb index d0fca6863..f9186994a 100644 --- a/app/components/post_votes_component/post_votes_component.html.erb +++ b/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 %> diff --git a/app/controllers/post_votes_controller.rb b/app/controllers/post_votes_controller.rb index 1b41d4f42..1c8f8ff2f 100644 --- a/app/controllers/post_votes_controller.rb +++ b/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.vote!(params[:score]) + @post = Post.find(params[:post_id]) - 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.unvote! + @post = Post.find(params[:post_id]) + @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 diff --git a/app/models/comment_vote.rb b/app/models/comment_vote.rb index 4da57de96..b73ec8aa7 100644 --- a/app/models/comment_vote.rb +++ b/app/models/comment_vote.rb @@ -2,9 +2,8 @@ class CommentVote < ApplicationRecord belongs_to :comment belongs_to :user - validates_presence_of :score - validates_uniqueness_of :user_id, :scope => :comment_id, :message => "have already voted for this comment" - validates_inclusion_of :score, :in => [-1, 1], :message => "must be 1 or -1" + validates :user_id, uniqueness: { scope: :comment_id, message: "have already voted for this comment" } + validates :score, inclusion: { in: [-1, 1], message: "must be 1 or -1" } after_create :update_score_after_create after_destroy :update_score_after_destroy diff --git a/app/models/post.rb b/app/models/post.rb index 0640bad28..2ac522e0f 100644 --- a/app/models/post.rb +++ b/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? - rescue PostVote::Error + vote!(1, user) 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? - rescue PostVote::Error + unvote!(user) end def remove_favorite(user) @@ -871,30 +870,22 @@ class Post < ApplicationRecord end module VoteMethods - def can_be_voted_by?(user) - !PostVote.exists?(:user_id => user.id, :post_id => id) + def vote!(score, voter) + # 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) - unless Pundit.policy!(voter, PostVote).create? - raise PostVote::Error.new("You do not have permission to vote") - end + def unvote!(voter) + return unless Pundit.policy!(voter, PostVote).create? - unless can_be_voted_by?(voter) - raise PostVote::Error.new("You have already voted for this post") - 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 + votes.destroy_by(user: voter) + reload end end diff --git a/app/models/post_vote.rb b/app/models/post_vote.rb index ff01fec8d..4a571344c 100644 --- a/app/models/post_vote.rb +++ b/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_presence_of :score - validates_inclusion_of :score, in: [1, -1] - after_create :update_post_on_create - after_destroy :update_post_on_destroy + validates :user_id, uniqueness: { scope: :post_id, message: "have already voted for this post" } + validates :score, inclusion: { in: [1, -1], message: "must be 1 or -1" } + + after_create :update_score_after_create + 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 - if score > 0 - Post.where(:id => post_id).update_all("score = score + #{score}, up_score = up_score + #{score}") + def update_score_after_create + if is_positive? + 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 - if score > 0 - Post.where(:id => post_id).update_all("score = score - #{score}, up_score = up_score - #{score}") + def update_score_after_destroy + if is_positive? + 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 diff --git a/app/policies/post_vote_policy.rb b/app/policies/post_vote_policy.rb index 4e24b4895..57e357968 100644 --- a/app/policies/post_vote_policy.rb +++ b/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 diff --git a/app/views/post_votes/create.js.erb b/app/views/post_votes/create.js.erb index 1932aa3a4..70caa471c 100644 --- a/app/views/post_votes/create.js.erb +++ b/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 %>"); diff --git a/test/components/post_votes_component_test.rb b/test/components/post_votes_component_test.rb index c40e72052..df30aff6d 100644 --- a/test/components/post_votes_component_test.rb +++ b/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") diff --git a/test/functional/post_votes_controller_test.rb b/test/functional/post_votes_controller_test.rb index 65313b022..f6a498c1e 100644 --- a/test/functional/post_votes_controller_test.rb +++ b/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) - @ban = create(:ban, user: @banned) - post_auth post_post_votes_path(post_id: @post.id), @banned, params: {:score => "up", :format => "js"} + post_auth post_post_votes_path(post_id: @post.id), create(:banned_user), params: { score: 1, 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), @member, params: {:score => "up", :format => "js"} + post_auth post_post_votes_path(post_id: @post.id), create(:user), params: { score: 1, 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.score) + assert_equal(1, @post.reload.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 - @post.vote!("up", @user) - assert_no_difference("PostVote.count") do - post_auth post_post_votes_path(post_id: @post.id), @user, params: { score: "up", format: "js" } - assert_response 422 + should "replace the vote" do + @post.vote!(1, @user) + + assert_no_difference("@post.votes.count") 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(0, @post.up_score) + assert_equal(-1, @post.down_score) end end end end context "destroy action" do - should "remove a vote" do - as(@user) { create(:post_vote, post_id: @post.id, user_id: @user.id) } + should "do nothing for anonymous users" do + delete post_post_votes_path(post_id: @post.id), xhr: true - assert_difference("PostVote.count", -1) do - delete_auth post_post_votes_path(post_id: @post.id), @user, as: :javascript - assert_redirected_to @post - end + assert_response 200 + assert_equal(0, @post.reload.score) + 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 diff --git a/test/unit/post_test.rb b/test/unit/post_test.rb index c2fa690fa..8674f408b 100644 --- a/test/unit/post_test.rb +++ b/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 - @post.update(:tag_string => "upvote:self") + assert_no_difference("PostVote.count") do + @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 - @post.update(:tag_string => "downvote:self") + assert_no_difference("PostVote.count") do + @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) - @post = FactoryBot.create(:post) - as(@user) do - assert_raises(PostVote::Error) { @post.vote!("up") } - end + user = create(:user) + post = create(:post) + + assert_nothing_raised { post.vote!(1, user) } + assert_equal(0, post.votes.count) + assert_equal(0, post.reload.score) end should "not allow duplicate votes" do - user = FactoryBot.create(:gold_user) - post = FactoryBot.create(:post) - CurrentUser.scoped(user, "127.0.0.1") do - assert_nothing_raised {post.vote!("up")} - assert_raises(PostVote::Error) {post.vote!("up")} - post.reload - assert_equal(1, PostVote.count) - assert_equal(1, post.score) - end + user = create(:gold_user) + post = create(:post) + + post.vote!(1, user) + post.vote!(1, user) + + assert_equal(1, post.reload.score) + assert_equal(1, post.votes.count) end should "allow undoing of votes" do - user = FactoryBot.create(:gold_user) - post = FactoryBot.create(:post) + user = create(:gold_user) + 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!("up") - assert_equal(1, post.score) + post.vote!(1, user) + 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! - assert_equal(0, post.score) + post.unvote!(user) + 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")} - assert_equal(-1, post.score) + post.vote!(-1, user) + 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! - assert_equal(0, post.score) + post.unvote!(user) + 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")} - assert_equal(1, post.score) + post.vote!(1, user) + 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 - assert_equal(1, post.score) - end + post.reload + assert_equal(1, post.score) end end diff --git a/test/unit/post_vote_test.rb b/test/unit/post_vote_test.rb index 1934d0cb9..a53232063 100644 --- a/test/unit/post_vote_test.rb +++ b/test/unit/post_vote_test.rb @@ -1,46 +1,74 @@ require 'test_helper' class PostVoteTest < ActiveSupport::TestCase - def setup - super - - @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) + context "A PostVote" do + setup do + @post = create(:post) end - should "interpret down as -1 score" do - vote = PostVote.create(:post_id => @post.id, :vote => "down") - assert_equal(-1, vote.score) + context "during validation" do + subject { build(:post_vote, post: @post) } + + 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 - vote = PostVote.create(:post_id => @post.id, :vote => "xxx") - assert(vote.errors.any?) + context "creating" do + context "an upvote" do + 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 - @post.votes.create(vote: "up") - @post.reload + context "destroying" do + context "an upvote" do + 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) - assert_equal(1, @post.up_score) - end + vote.destroy + 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 - should "decrease the score of the post when removed" do - @post.votes.create(vote: "up").destroy - @post.reload + context "a downvote" do + should "increment 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.count) - assert_equal(0, @post.score) - assert_equal(0, @post.up_score) + vote.destroy + 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