From 6b91e55283d5889bb164d540a765d55580fca00f Mon Sep 17 00:00:00 2001 From: evazion Date: Mon, 29 Mar 2021 19:40:37 -0500 Subject: [PATCH] comments: allow votes to be soft deleted. Make it so that when a user removes their own vote, the vote is soft deleted (the is_deleted flag is set) instead of hard deleted. Changes: * Add is_deleted flag to comment votes. * Relax uniqueness constraint so you can have multiple deleted votes on the same comment. You can still only have one active vote on the comment. * Add `soft_delete` method to Deletable concern. --- app/components/comment_component.rb | 4 +- app/controllers/comment_votes_controller.rb | 11 ++++-- app/logical/concerns/deletable.rb | 8 ++++ app/models/comment_vote.rb | 37 +++++++++++++++---- app/models/mod_action.rb | 2 + ...0003356_add_is_deleted_to_comment_votes.rb | 11 ++++++ db/structure.sql | 15 ++++++-- test/components/comment_component_test.rb | 11 ++++++ test/factories/comment_vote.rb | 1 + .../comment_votes_controller_test.rb | 27 ++++++++++++-- test/unit/comment_vote_test.rb | 21 ++++++++--- 11 files changed, 125 insertions(+), 23 deletions(-) create mode 100644 db/migrate/20210330003356_add_is_deleted_to_comment_votes.rb diff --git a/app/components/comment_component.rb b/app/components/comment_component.rb index 7f2f77ccc..77fbdafdc 100644 --- a/app/components/comment_component.rb +++ b/app/components/comment_component.rb @@ -28,12 +28,12 @@ class CommentComponent < ApplicationComponent def upvoted? return false if current_user.is_anonymous? - comment.votes.select(&:is_positive?).map(&:user_id).include?(current_user.id) + comment.votes.active.select(&:is_positive?).map(&:user_id).include?(current_user.id) end def downvoted? return false if current_user.is_anonymous? - comment.votes.select(&:is_negative?).map(&:user_id).include?(current_user.id) + comment.votes.active.select(&:is_negative?).map(&:user_id).include?(current_user.id) end def reported? diff --git a/app/controllers/comment_votes_controller.rb b/app/controllers/comment_votes_controller.rb index 8e280385a..4235e8492 100644 --- a/app/controllers/comment_votes_controller.rb +++ b/app/controllers/comment_votes_controller.rb @@ -13,7 +13,11 @@ class CommentVotesController < ApplicationController @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 + + CommentVote.active.where(comment: @comment, user: CurrentUser.user).each do |vote| + vote.soft_delete!(updater: CurrentUser.user) + end + @comment_vote.save end @@ -22,8 +26,9 @@ class CommentVotesController < ApplicationController end def destroy - @comment_vote = authorize CommentVote.find_by!(comment_id: params[:comment_id], user: CurrentUser.user) - @comment_vote.destroy + # XXX should find by comment vote id. + @comment_vote = authorize CommentVote.active.find_by!(comment_id: params[:comment_id], user: CurrentUser.user) + @comment_vote.soft_delete(updater: CurrentUser.user) respond_with(@comment_vote) end diff --git a/app/logical/concerns/deletable.rb b/app/logical/concerns/deletable.rb index 3606870ef..82ceb61b8 100644 --- a/app/logical/concerns/deletable.rb +++ b/app/logical/concerns/deletable.rb @@ -6,6 +6,14 @@ module Deletable scope :active, -> { where(is_deleted: false) } scope :deleted, -> { where(is_deleted: true) } scope :undeleted, -> { where(is_deleted: false) } + + define_method(:soft_delete) do |**options| + update(is_deleted: true, **options) + end + + define_method(:soft_delete!) do |**options| + update!(is_deleted: true, **options) + end end end end diff --git a/app/models/comment_vote.rb b/app/models/comment_vote.rb index b73ec8aa7..0145d5364 100644 --- a/app/models/comment_vote.rb +++ b/app/models/comment_vote.rb @@ -1,12 +1,16 @@ class CommentVote < ApplicationRecord + attr_accessor :updater + belongs_to :comment belongs_to :user - validates :user_id, uniqueness: { scope: :comment_id, message: "have already voted for this comment" } + validate :validate_vote_is_unique, if: :is_deleted_changed? validates :score, inclusion: { in: [-1, 1], message: "must be 1 or -1" } - after_create :update_score_after_create - after_destroy :update_score_after_destroy + before_create :update_score_on_create + before_save :update_score_on_delete_or_undelete, if: -> { !new_record? && is_deleted_changed? } + + deletable def self.visible(user) if user.is_moderator? @@ -19,7 +23,7 @@ class CommentVote < ApplicationRecord end def self.search(params) - q = search_attributes(params, :id, :created_at, :updated_at, :score, :comment, :user) + q = search_attributes(params, :id, :created_at, :updated_at, :score, :is_deleted, :comment, :user) q.apply_default_order(params) end @@ -31,15 +35,34 @@ class CommentVote < ApplicationRecord score == -1 end - def update_score_after_create + # allow duplicate deleted votes but not duplicate active votes + def validate_vote_is_unique + if !is_deleted? && CommentVote.active.where.not(id: id).exists?(comment_id: comment_id, user_id: user_id) + errors.add(:user, "have already voted for this comment") + end + end + + def update_score_on_create comment.with_lock do comment.update_columns(score: comment.score + score) end end - def update_score_after_destroy + def update_score_on_delete_or_undelete comment.with_lock do - comment.update_columns(score: comment.score - score) + if is_deleted_changed?(from: false, to: true) + comment.update_columns(score: comment.score - score) + + if updater != user + ModAction.log("#{updater.name} deleted comment vote ##{id} on comment ##{comment_id}", :comment_vote_delete, updater) + end + else + comment.update_columns(score: comment.score + score) + + if updater != user + ModAction.log("#{updater.name} undeleted comment vote ##{id} on comment ##{comment_id}", :comment_vote_undelete, updater) + end + end end end diff --git a/app/models/mod_action.rb b/app/models/mod_action.rb index a0c98ab16..369856c65 100644 --- a/app/models/mod_action.rb +++ b/app/models/mod_action.rb @@ -37,6 +37,8 @@ class ModAction < ApplicationRecord artist_unban: 185, comment_update: 81, comment_delete: 82, + comment_vote_delete: 92, + comment_vote_undelete: 93, forum_topic_delete: 202, forum_topic_undelete: 203, forum_topic_lock: 206, diff --git a/db/migrate/20210330003356_add_is_deleted_to_comment_votes.rb b/db/migrate/20210330003356_add_is_deleted_to_comment_votes.rb new file mode 100644 index 000000000..a98fee498 --- /dev/null +++ b/db/migrate/20210330003356_add_is_deleted_to_comment_votes.rb @@ -0,0 +1,11 @@ +class AddIsDeletedToCommentVotes < ActiveRecord::Migration[6.1] + def change + add_column :comment_votes, :is_deleted, :boolean, default: false, null: :false + change_column_null :comment_votes, :is_deleted, false + + add_index :comment_votes, :is_deleted, where: "is_deleted = TRUE" + + remove_index :comment_votes, [:user_id, :comment_id], unique: true + add_index :comment_votes, [:user_id, :comment_id], unique: true, where: "is_deleted = FALSE" + end +end diff --git a/db/structure.sql b/db/structure.sql index 4e7d2369f..bf8810cfb 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -764,7 +764,8 @@ CREATE TABLE public.comment_votes ( user_id integer NOT NULL, score integer NOT NULL, created_at timestamp without time zone NOT NULL, - updated_at timestamp without time zone NOT NULL + updated_at timestamp without time zone NOT NULL, + is_deleted boolean DEFAULT false NOT NULL ); @@ -5107,6 +5108,13 @@ CREATE INDEX index_comment_votes_on_comment_id ON public.comment_votes USING btr CREATE INDEX index_comment_votes_on_created_at ON public.comment_votes USING btree (created_at); +-- +-- Name: index_comment_votes_on_is_deleted; Type: INDEX; Schema: public; Owner: - +-- + +CREATE INDEX index_comment_votes_on_is_deleted ON public.comment_votes USING btree (is_deleted) WHERE (is_deleted = true); + + -- -- Name: index_comment_votes_on_user_id; Type: INDEX; Schema: public; Owner: - -- @@ -5118,7 +5126,7 @@ CREATE INDEX index_comment_votes_on_user_id ON public.comment_votes USING btree -- Name: index_comment_votes_on_user_id_and_comment_id; Type: INDEX; Schema: public; Owner: - -- -CREATE UNIQUE INDEX index_comment_votes_on_user_id_and_comment_id ON public.comment_votes USING btree (user_id, comment_id); +CREATE UNIQUE INDEX index_comment_votes_on_user_id_and_comment_id ON public.comment_votes USING btree (user_id, comment_id) WHERE (is_deleted = false); -- @@ -8005,6 +8013,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20210214095121'), ('20210214101614'), ('20210303195217'), -('20210310221248'); +('20210310221248'), +('20210330003356'); diff --git a/test/components/comment_component_test.rb b/test/components/comment_component_test.rb index 601c1a490..96d1899d6 100644 --- a/test/components/comment_component_test.rb +++ b/test/components/comment_component_test.rb @@ -92,5 +92,16 @@ class CommentComponentTest < ViewComponent::TestCase end end end + + context "for a comment with a deleted vote" do + should "not treat the vote as active" do + @user = create(:user) + @vote = create(:comment_vote, user: @user, comment: @comment, is_deleted: true, score: 1) + render_comment(@comment, current_user: @user) + + assert_css("article.comment[data-is-upvoted=false]") + assert_css("article.comment .comment-upvote-link.inactive-link") + end + end end end diff --git a/test/factories/comment_vote.rb b/test/factories/comment_vote.rb index 9a5ba5379..0502bba6c 100644 --- a/test/factories/comment_vote.rb +++ b/test/factories/comment_vote.rb @@ -3,5 +3,6 @@ FactoryBot.define do comment user score {1} + is_deleted { false } end end diff --git a/test/functional/comment_votes_controller_test.rb b/test/functional/comment_votes_controller_test.rb index cc3f71727..2b6e0227c 100644 --- a/test/functional/comment_votes_controller_test.rb +++ b/test/functional/comment_votes_controller_test.rb @@ -91,24 +91,28 @@ class CommentVotesControllerTest < ActionDispatch::IntegrationTest vote = create(:comment_vote, comment: @comment, user: @user, score: 1) assert_equal(1, vote.comment.reload.score) - assert_difference("CommentVote.count", 0) do + assert_difference("CommentVote.count", 1) 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) + assert_equal(1, @comment.votes.active.count) + assert_equal(1, @comment.votes.deleted.count) 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 + assert_difference("CommentVote.count", 1) 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) + assert_equal(1, @comment.votes.active.count) + assert_equal(1, @comment.votes.deleted.count) end should "not allow voting on deleted comments" do @@ -138,9 +142,10 @@ class CommentVotesControllerTest < ActionDispatch::IntegrationTest should "allow users to remove their own comment votes" do @vote = create(:comment_vote, user: @user) - assert_difference("CommentVote.count", -1) do + assert_difference("CommentVote.count", 0) do delete_auth comment_comment_votes_path(@vote.comment), @user, xhr: true assert_response :success + assert_equal(true, @vote.reload.is_deleted?) end end @@ -150,6 +155,22 @@ class CommentVotesControllerTest < ActionDispatch::IntegrationTest assert_difference("CommentVote.count", 0) do delete_auth comment_comment_votes_path(@vote.comment), @user, xhr: true assert_response 404 + assert_equal(false, @vote.reload.is_deleted?) + end + end + + context "deleting a vote on a comment that already has deleted votes" do + setup do + create(:comment_vote, comment: @comment, user: @user, score: 1, is_deleted: true) + create(:comment_vote, comment: @comment, user: @user, score: -1, is_deleted: true) + end + + should "delete the current active vote" do + @vote = create(:comment_vote, comment: @comment, user: @user) + delete_auth comment_comment_votes_path(@vote.comment), @user, xhr: true + + assert_response :success + assert_equal(true, @vote.reload.is_deleted?) end end end diff --git a/test/unit/comment_vote_test.rb b/test/unit/comment_vote_test.rb index 16117da87..03ee92c45 100644 --- a/test/unit/comment_vote_test.rb +++ b/test/unit/comment_vote_test.rb @@ -10,11 +10,10 @@ class CommentVoteTest < ActiveSupport::TestCase 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 - should "not allow creating duplicate votes" do + should "not allow creating duplicate active votes" do v1 = create(:comment_vote, comment: @comment, user: @user) v2 = build(:comment_vote, comment: @comment, user: @user) @@ -23,6 +22,16 @@ class CommentVoteTest < ActiveSupport::TestCase end end + should "allow creating duplicate deleted votes" do + v1 = create(:comment_vote, comment: @comment, user: @user) + v2 = create(:comment_vote, comment: @comment, user: @user, is_deleted: true) + v3 = create(:comment_vote, comment: @comment, user: @user, is_deleted: true) + + assert_equal(true, v1.valid?) + assert_equal(true, v2.valid?) + assert_equal(true, v3.valid?) + end + context "creating" do context "an upvote" do should "increment the comment's score" do @@ -41,14 +50,15 @@ class CommentVoteTest < ActiveSupport::TestCase end end - context "destroying" do + context "soft deleting" 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 + vote.soft_delete(updater: vote.user) assert_equal(0, @comment.reload.score) + assert_equal(true, vote.is_deleted?) end end @@ -57,8 +67,9 @@ class CommentVoteTest < ActiveSupport::TestCase vote = create(:comment_vote, comment: @comment, score: -1) assert_equal(-1, @comment.reload.score) - vote.destroy + vote.soft_delete(updater: vote.user) assert_equal(0, @comment.reload.score) + assert_equal(true, vote.is_deleted?) end end end