From 9efb374ae5f5ca33e78f2e2c0856a661314f97f2 Mon Sep 17 00:00:00 2001 From: evazion Date: Thu, 21 Jan 2021 01:02:22 -0600 Subject: [PATCH] comments: allow swapping votes. Allow users to upvote a comment, then downvote it, without raising an error or having to manually remove the upvote first. The upvote is automatically removed and replaced by the downvote. Changes to the /comment_votes API: * `POST /comment_votes` and `DELETE /comment_votes` now return a comment vote instead of a comment. * The `score` param in `POST /comment_votes` is now 1 or -1, not `up` or `down.` --- CHANGELOG.md | 6 + .../comment_component.html.erb | 4 +- app/controllers/comment_votes_controller.rb | 22 ++-- app/models/comment.rb | 32 ------ app/models/comment_vote.rb | 18 ++- app/policies/comment_vote_policy.rb | 7 +- app/views/comment_votes/create.js.erb | 8 +- app/views/comment_votes/destroy.js.erb | 3 +- .../comment_votes_controller_test.rb | 104 ++++++++++++------ .../moderator/dashboards_controller_test.rb | 4 +- test/unit/comment_test.rb | 49 --------- test/unit/comment_vote_test.rb | 57 ++++++++++ 12 files changed, 181 insertions(+), 133 deletions(-) mode change 100644 => 120000 app/views/comment_votes/destroy.js.erb create mode 100644 test/unit/comment_vote_test.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index c1a465924..0cb22f852 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,12 @@ `creator_id`, `updater_id`, and `body` fields are hidden if you're not a moderator. +* The `POST /comment_votes` and `DELETE /comment_votes` endpoints now return a + comment vote instead of a comment. + +* The `score` param in the `POST /comment_votes` endpoint is now 1 or -1, not + `up` or `down`. + ## 2021-01-12 ### Changes diff --git a/app/components/comment_component/comment_component.html.erb b/app/components/comment_component/comment_component.html.erb index f15579acd..d931f22b7 100644 --- a/app/components/comment_component/comment_component.html.erb +++ b/app/components/comment_component/comment_component.html.erb @@ -51,7 +51,7 @@ <% elsif upvoted? %> <%= link_to "🡹", comment_comment_votes_path(comment_id: comment.id), class: "comment-upvote-link comment-unvote-link", method: :delete, remote: true %> <% else %> - <%= link_to "🡹", comment_comment_votes_path(comment_id: comment.id, score: "up"), class: "comment-upvote-link", method: :post, remote: true %> + <%= link_to "🡹", comment_comment_votes_path(comment_id: comment.id, score: "1"), class: "comment-upvote-link", method: :post, remote: true %> <% end %> <%= comment.score %> @@ -61,7 +61,7 @@ <% elsif downvoted? %> <%= link_to "🡻", comment_comment_votes_path(comment_id: comment.id), class: "comment-downvote-link comment-unvote-link", method: :delete, remote: true %> <% else %> - <%= link_to "🡻", comment_comment_votes_path(comment_id: comment.id, score: "down"), class: "comment-downvote-link", method: :post, remote: true %> + <%= link_to "🡻", comment_comment_votes_path(comment_id: comment.id, score: "-1"), class: "comment-downvote-link", method: :post, remote: true %> <% end %> <% end %> diff --git a/app/controllers/comment_votes_controller.rb b/app/controllers/comment_votes_controller.rb index 667611669..c85faf4d8 100644 --- a/app/controllers/comment_votes_controller.rb +++ b/app/controllers/comment_votes_controller.rb @@ -1,23 +1,31 @@ class CommentVotesController < ApplicationController skip_before_action :api_check respond_to :js, :json, :xml, :html - rescue_with CommentVote::Error, ActiveRecord::RecordInvalid, status: 422 def index @comment_votes = authorize CommentVote.visible(CurrentUser.user).paginated_search(params, count_pages: true) @comment_votes = @comment_votes.includes(:user, comment: [:creator, post: [:uploader]]) if request.format.html? + respond_with(@comment_votes) end def create - @comment = authorize Comment.find(params[:comment_id]), policy_class: CommentVotePolicy - @comment_vote = @comment.vote!(params[:score]) - respond_with(@comment) + @comment = Comment.find(params[:comment_id]) + + @comment.with_lock do + @comment_vote = authorize CommentVote.new(comment: @comment, score: params[:score], user: CurrentUser.user) + CommentVote.where(comment: @comment, user: CurrentUser.user).destroy_all + @comment_vote.save + end + + flash.now[:notice] = @comment_vote.errors.full_messages.join("; ") if @comment_vote.errors.present? + respond_with(@comment_vote) end def destroy - @comment = authorize Comment.find(params[:comment_id]), policy_class: CommentVotePolicy - @comment.unvote! - respond_with(@comment) + @comment_vote = authorize CommentVote.find_by!(comment_id: params[:comment_id], user: CurrentUser.user) + @comment_vote.destroy + + respond_with(@comment_vote) end end diff --git a/app/models/comment.rb b/app/models/comment.rb index 8d260fa28..49cfe11e0 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -44,39 +44,7 @@ class Comment < ApplicationRecord end end - module VoteMethods - def vote!(val, voter = CurrentUser.user) - numerical_score = (val == "up") ? 1 : -1 - vote = votes.create!(user: voter, score: numerical_score) - - if vote.is_positive? - update_column(:score, score + 1) - elsif vote.is_negative? - update_column(:score, score - 1) - end - - return vote - end - - def unvote! - vote = votes.where("user_id = ?", CurrentUser.user.id).first - - if vote - if vote.is_positive? - update_column(:score, score - 1) - else - update_column(:score, score + 1) - end - - vote.destroy - else - raise CommentVote::Error.new("You have not voted for this comment") - end - end - end - extend SearchMethods - include VoteMethods def validate_creator_is_not_limited if creator.is_comment_limited? && !do_not_bump_post? diff --git a/app/models/comment_vote.rb b/app/models/comment_vote.rb index 0df3cc952..29d6c4464 100644 --- a/app/models/comment_vote.rb +++ b/app/models/comment_vote.rb @@ -1,13 +1,15 @@ class CommentVote < ApplicationRecord - class Error < StandardError; end - 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" validate :validate_comment_can_be_down_voted 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 + def self.visible(user) if user.is_moderator? all @@ -37,6 +39,18 @@ class CommentVote < ApplicationRecord score == -1 end + def update_score_after_create + comment.with_lock do + comment.update_columns(score: comment.score + score) + end + end + + def update_score_after_destroy + comment.with_lock do + comment.update_columns(score: comment.score - score) + end + end + def self.available_includes [:comment, :user] end diff --git a/app/policies/comment_vote_policy.rb b/app/policies/comment_vote_policy.rb index 46b9bf3e7..25706185c 100644 --- a/app/policies/comment_vote_policy.rb +++ b/app/policies/comment_vote_policy.rb @@ -1,7 +1,10 @@ class CommentVotePolicy < ApplicationPolicy + def create? + unbanned? && !record.comment.is_deleted? + end + def destroy? - # XXX permissions are checked in Comment#unvote! - true + record.user_id == user.id end def can_see_votes? diff --git a/app/views/comment_votes/create.js.erb b/app/views/comment_votes/create.js.erb index b8a438cd5..3439ecfba 100644 --- a/app/views/comment_votes/create.js.erb +++ b/app/views/comment_votes/create.js.erb @@ -1,2 +1,6 @@ -var $comment = $("article#comment_<%= @comment.id %>"); -$comment.replaceWith("<%= j render_comment(@comment, current_user: CurrentUser.user) %>"); +<% if flash[:notice] %> + Danbooru.Utility.notice("<%= j flash[:notice] %>"); +<% end %> + +var $comment = $("article#comment_<%= @comment_vote.comment_id %>"); +$comment.replaceWith("<%= j render_comment(@comment_vote.comment, current_user: CurrentUser.user) %>"); diff --git a/app/views/comment_votes/destroy.js.erb b/app/views/comment_votes/destroy.js.erb deleted file mode 100644 index b8a438cd5..000000000 --- a/app/views/comment_votes/destroy.js.erb +++ /dev/null @@ -1,2 +0,0 @@ -var $comment = $("article#comment_<%= @comment.id %>"); -$comment.replaceWith("<%= j render_comment(@comment, current_user: CurrentUser.user) %>"); diff --git a/app/views/comment_votes/destroy.js.erb b/app/views/comment_votes/destroy.js.erb new file mode 120000 index 000000000..c7f6d951c --- /dev/null +++ b/app/views/comment_votes/destroy.js.erb @@ -0,0 +1 @@ +create.js.erb \ No newline at end of file diff --git a/test/functional/comment_votes_controller_test.rb b/test/functional/comment_votes_controller_test.rb index 20cf11ce3..9be3afc2c 100644 --- a/test/functional/comment_votes_controller_test.rb +++ b/test/functional/comment_votes_controller_test.rb @@ -46,43 +46,83 @@ class CommentVotesControllerTest < ActionDispatch::IntegrationTest end end - context "#create.json" do - should "create a vote" do + context "create action" do + setup do + @user = create(:user) + @comment = create(:comment) + end + + should "not allow anonymous users to vote" do + post comment_comment_votes_path(comment_id: @comment.id, score: "1"), xhr: true + assert_response 403 + end + + should "allow Members to vote" do + post_auth comment_comment_votes_path(comment_id: @comment.id, score: "1"), @user, xhr: true + assert_response :success + end + + should "create a upvote" do assert_difference("CommentVote.count", 1) do - post_auth comment_comment_votes_path(comment_id: @comment.id, score: "down"), @user, as: :json - assert_response :success - - assert_equal(@comment.id, response.parsed_body["id"]) - assert_equal(-1, response.parsed_body["score"]) + post_auth comment_comment_votes_path(comment_id: @comment.id, score: "1"), @user, xhr: true end + + assert_response :success + assert_equal(1, @comment.reload.score) end - should "fail silently on errors" do - create(:comment_vote, user: @user, comment: @comment, score: -1) - assert_difference("CommentVote.count", 0) do - post_auth comment_comment_votes_path(comment_id: @comment.id, score: "down", format: "json"), @user - assert_response 422 - - comment = JSON.parse(@response.body) - assert_equal(false, comment["success"]) - assert_equal("Validation failed: You have already voted for this comment", comment["message"]) - end - end - end - - context "#create.js" do - should "create a vote" do + should "create a downvote" do assert_difference("CommentVote.count", 1) do - post_auth comment_comment_votes_path(comment_id: @comment.id, format: "json", score: "down"), @user - assert_response :success + post_auth comment_comment_votes_path(comment_id: @comment.id, score: "-1"), @user, xhr: true end + + assert_response :success + assert_equal(-1, @comment.reload.score) end - should "fail on errors" do - create(:comment_vote, user: @user, comment: @comment, score: -1) + should "ignore duplicate votes" do + vote = create(:comment_vote, comment: @comment, user: @user, score: 1) + assert_equal(1, vote.comment.reload.score) + assert_difference("CommentVote.count", 0) do - post_auth comment_comment_votes_path(comment_id: @comment.id, :score => "down", format: "js"), @user - assert_response 422 + post_auth comment_comment_votes_path(comment_id: @comment.id, score: "1"), @user, xhr: true + end + + assert_response :success + assert_equal(1, @comment.reload.score) + end + + should "automatically undo existing votes" do + create(:comment_vote, comment: @comment, user: @user, score: -1) + assert_equal(-1, @comment.reload.score) + + assert_difference("CommentVote.count", 0) do + post_auth comment_comment_votes_path(comment_id: @comment.id, score: "1"), @user, xhr: true + end + + assert_response :success + assert_equal(1, @comment.reload.score) + end + + should "not allow voting on deleted comments" do + @comment.update!(is_deleted: true) + + assert_difference("CommentVote.count", 0) do + post_auth comment_comment_votes_path(comment_id: @comment.id, score: "1"), @user, xhr: true + end + + assert_response 403 + assert_equal(0, @comment.reload.score) + end + + should "not update the comment's updated_at or updater_id" do + assert_no_difference(["@comment.updater_id", "@comment.reload.updated_at"]) do + assert_difference("CommentVote.count", 1) do + post_auth comment_comment_votes_path(comment_id: @comment.id, score: "1"), @user, xhr: true + + assert_response :success + assert_equal(1, @comment.reload.score) + end end end end @@ -92,8 +132,8 @@ class CommentVotesControllerTest < ActionDispatch::IntegrationTest @vote = create(:comment_vote, user: @user) assert_difference("CommentVote.count", -1) do - delete_auth comment_comment_votes_path(@vote.comment), @user - assert_redirected_to @vote.comment + delete_auth comment_comment_votes_path(@vote.comment), @user, xhr: true + assert_response :success end end @@ -101,8 +141,8 @@ class CommentVotesControllerTest < ActionDispatch::IntegrationTest @vote = create(:comment_vote) assert_difference("CommentVote.count", 0) do - delete_auth comment_comment_votes_path(@vote.comment), @user - assert_response 422 + delete_auth comment_comment_votes_path(@vote.comment), @user, xhr: true + assert_response 404 end end end diff --git a/test/functional/moderator/dashboards_controller_test.rb b/test/functional/moderator/dashboards_controller_test.rb index 4657c4aa2..db5ffb8f6 100644 --- a/test/functional/moderator/dashboards_controller_test.rb +++ b/test/functional/moderator/dashboards_controller_test.rb @@ -86,9 +86,7 @@ module Moderator end @users.each do |user| - CurrentUser.as(user) do - @comment.vote!(-1) - end + create(:comment_vote, score: -1, comment: @comment, user: user) end end diff --git a/test/unit/comment_test.rb b/test/unit/comment_test.rb index 18bba141b..5b859b231 100644 --- a/test/unit/comment_test.rb +++ b/test/unit/comment_test.rb @@ -143,55 +143,6 @@ class CommentTest < ActiveSupport::TestCase end end - should "not record the user id of the voter" do - user = FactoryBot.create(:user) - post = FactoryBot.create(:post) - c1 = FactoryBot.create(:comment, :post => post) - CurrentUser.scoped(user, "127.0.0.1") do - c1.vote!("up") - c1.reload - assert_not_equal(user.id, c1.updater_id) - end - end - - should "not allow duplicate votes" do - user = FactoryBot.create(:user) - post = FactoryBot.create(:post) - c1 = FactoryBot.create(:comment, :post => post) - - assert_nothing_raised { c1.vote!("down") } - exception = assert_raises(ActiveRecord::RecordInvalid) { c1.vote!("down") } - assert_equal("Validation failed: You have already voted for this comment", exception.message) - assert_equal(1, CommentVote.count) - assert_equal(-1, CommentVote.last.score) - - c2 = FactoryBot.create(:comment, :post => post) - assert_nothing_raised { c2.vote!("down") } - assert_equal(2, CommentVote.count) - end - - should "not allow upvotes by the creator" do - user = FactoryBot.create(:user) - post = FactoryBot.create(:post) - c1 = create(:comment, post: post, creator: CurrentUser.user) - - exception = assert_raises(ActiveRecord::RecordInvalid) { c1.vote!("up") } - assert_equal("Validation failed: You cannot upvote your own comments", exception.message) - end - - should "allow undoing of votes" do - user = FactoryBot.create(:user) - post = FactoryBot.create(:post) - comment = FactoryBot.create(:comment, :post => post) - CurrentUser.scoped(user, "127.0.0.1") do - comment.vote!("up") - comment.unvote! - comment.reload - assert_equal(0, comment.score) - assert_nothing_raised {comment.vote!("down")} - end - end - should "be searchable" do c1 = FactoryBot.create(:comment, :body => "aaa bbb ccc") c2 = FactoryBot.create(:comment, :body => "aaa ddd") diff --git a/test/unit/comment_vote_test.rb b/test/unit/comment_vote_test.rb new file mode 100644 index 000000000..cf17b5afc --- /dev/null +++ b/test/unit/comment_vote_test.rb @@ -0,0 +1,57 @@ +require 'test_helper' + +class CommentVoteTest < ActiveSupport::TestCase + context "A CommentVote" do + setup do + @user = create(:user) + @comment = as(@user) { create(:comment) } + end + + context "during validation" do + subject { build(:comment_vote, comment: as(@user) { create(:comment) }) } + + should validate_uniqueness_of(:user_id).scoped_to(:comment_id).with_message("have already voted for this comment") + should validate_inclusion_of(:score).in_array([-1, 1]).with_message("must be 1 or -1") + end + + context "creating" do + context "an upvote" do + should "increment the comment's score" do + vote = create(:comment_vote, comment: @comment, score: 1) + + assert_equal(1, @comment.reload.score) + end + end + + context "a downvote" do + should "decrement the comment's score" do + vote = create(:comment_vote, comment: @comment, score: -1) + + assert_equal(-1, @comment.reload.score) + end + end + end + + context "destroying" do + context "an upvote" do + should "decrement the comment's score" do + vote = create(:comment_vote, comment: @comment, score: 1) + assert_equal(1, @comment.reload.score) + + vote.destroy + assert_equal(0, @comment.reload.score) + end + end + + context "a downvote" do + should "increment the comment's score" do + vote = create(:comment_vote, comment: @comment, score: -1) + assert_equal(-1, @comment.reload.score) + + vote.destroy + assert_equal(0, @comment.reload.score) + end + end + end + end +end