From d0c9f6e0b8182fea9d4ba8a530a7b0e77d838610 Mon Sep 17 00:00:00 2001 From: evazion Date: Fri, 29 Jan 2021 01:58:12 -0600 Subject: [PATCH] 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`. --- .../post_votes_component.html.erb | 4 +- app/controllers/post_votes_controller.rb | 20 +++-- app/models/comment_vote.rb | 5 +- app/models/post.rb | 43 ++++----- app/models/post_vote.rb | 39 +++----- app/policies/post_vote_policy.rb | 2 +- app/views/post_votes/create.js.erb | 2 +- test/components/post_votes_component_test.rb | 4 +- test/functional/post_votes_controller_test.rb | 82 ++++++++++++----- test/unit/post_test.rb | 85 ++++++++++-------- test/unit/post_vote_test.rb | 90 ++++++++++++------- 11 files changed, 221 insertions(+), 155 deletions(-) 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