FIX: Comment serialization for deleted users (#154)

Serializing topics with deleted comment users results in unhandled
errors.

This change guards the user details access on `QuestionAnswerComment`
records and also cleans up post voting related records when a user is
destroyed via UserDestroyer.

app/serializers/question_answer_comment_serializer.rb

@@ -14,11 +14,11 @@ class QuestionAnswerCommentSerializer < ApplicationSerializer
   attr_accessor :comments_user_voted
 
   def name
-    object.user.name
+    object.user&.name
   end
 
   def username
-    object.user.username
+    object.user&.username
   end
 
   def user_voted

assets/javascripts/discourse/components/post-voting-comment.hbs

@@ -34,12 +34,20 @@
 
       <span class="post-voting-comment-info-separator">–</span>
 
-      <a
-        class="post-voting-comment-info-username"
-        data-user-card={{@comment.username}}
-      >
-        {{format-username @comment.username}}
-      </a>
+      {{#if @comment.username}}
+        <a
+          class="post-voting-comment-info-username"
+          data-user-card={{@comment.username}}
+        >
+          {{format-username @comment.username}}
+        </a>
+      {{else}}
+        <span
+          class="post-voting-comment-info-username post-voting-comment-info-username-deleted"
+        >
+          {{i18n "post_voting.post.post_voting_comment.user.deleted"}}
+        </span>
+      {{/if}}
 
       <span class="post-voting-comment-info-created">
         {{format-date @comment.created_at}}

assets/stylesheets/common/post-voting.scss

@@ -136,7 +136,8 @@
       margin-right: 0.3em;
     }
 
-    .post-voting-comment-info-created {
+    .post-voting-comment-info-created,
+    .post-voting-comment-info-username-deleted {
       color: var(--primary-med-or-secondary-med);
     }
 

config/locales/client.en.yml

@@ -53,6 +53,8 @@ en:
             other: Show %{count} more comments
           delete:
             confirm: "Are you sure you want to delete this comment?"
+          user:
+            deleted: "(user deleted)"
       topic:
         answer:
           label: "Answer"

lib/post_voting/vote_manager.rb

@@ -64,5 +64,42 @@ def self.publish_changes(obj, user, vote_count, direction)
         )
       end
     end
+
+    def self.bulk_remove_votes_by(user)
+      ActiveRecord::Base.transaction do
+        QuestionAnswerVote::VOTABLE_TYPES.map do |votable_type|
+          table_name = votable_type.tableize
+
+          DB.exec(
+            <<~SQL,
+            UPDATE #{table_name}
+            SET qa_vote_count = qa_vote_count - (
+              SELECT CASE
+                  WHEN direction = :up THEN 1
+                  WHEN direction = :down THEN -1
+                  ELSE 0
+              END
+              FROM question_answer_votes
+              WHERE question_answer_votes.votable_id = #{table_name}.id
+              AND question_answer_votes.votable_type = '#{votable_type}'
+              AND question_answer_votes.user_id = :user_id
+            )
+            WHERE EXISTS (
+                SELECT 1
+                FROM question_answer_votes
+                WHERE question_answer_votes.votable_id = #{table_name}.id
+                AND question_answer_votes.votable_type = '#{votable_type}'
+                AND question_answer_votes.user_id = :user_id
+            );
+          SQL
+            user_id: user.id,
+            up: QuestionAnswerVote.directions[:up],
+            down: QuestionAnswerVote.directions[:down],
+          )
+        end
+
+        QuestionAnswerVote.where(user_id: user.id).delete_all
+      end
+    end
   end
 end

plugin.rb

@@ -203,4 +203,11 @@
       self.reply_to_post_number = nil
     end
   end
+
+  register_user_destroyer_on_content_deletion_callback(
+    Proc.new do |user|
+      QuestionAnswerComment.where(user_id: user.id).delete_all
+      PostVoting::VoteManager.bulk_remove_votes_by(user)
+    end,
+  )
 end

spec/components/post_voting/vote_manager_spec.rb

@@ -109,4 +109,53 @@
       expect(QuestionAnswerVote.exists?(id: vote_3.id)).to eq(false)
     end
   end
+
+  describe ".bulk_remove_votes_by" do
+    it "removes all votes by a user" do
+      other_user_1 = Fabricate(:user)
+      other_user_2 = Fabricate(:user)
+
+      comment_1 = Fabricate(:post_voting_comment, post: post)
+      comment_2 = Fabricate(:post_voting_comment, post: post)
+      comment_3 = Fabricate(:post_voting_comment, post: topic_post)
+
+      vote_1 = PostVoting::VoteManager.vote(post, user, direction: down)
+      vote_2 = PostVoting::VoteManager.vote(topic_post, user, direction: up)
+      vote_3 = PostVoting::VoteManager.vote(comment_1, user, direction: up)
+      vote_4 = PostVoting::VoteManager.vote(comment_2, user, direction: up)
+
+      vote_5 = PostVoting::VoteManager.vote(post, other_user_1, direction: down)
+      vote_6 = PostVoting::VoteManager.vote(topic_post, other_user_1, direction: up)
+      vote_7 = PostVoting::VoteManager.vote(comment_1, other_user_1, direction: up)
+      vote_8 = PostVoting::VoteManager.vote(comment_3, other_user_2, direction: up)
+
+      expect(QuestionAnswerVote.exists?(id: [vote_1.id, vote_2.id, vote_3.id, vote_4.id])).to eq(
+        true,
+      )
+      expect(user.question_answer_votes.count).to eq(4)
+      expect(QuestionAnswerVote.count).to eq(8)
+
+      expect(post.qa_vote_count).to eq(-2)
+      expect(topic_post.qa_vote_count).to eq(2)
+      expect(comment_1.qa_vote_count).to eq(2)
+      expect(comment_2.qa_vote_count).to eq(1)
+      expect(comment_3.qa_vote_count).to eq(1)
+
+      PostVoting::VoteManager.bulk_remove_votes_by(user)
+
+      expect(QuestionAnswerVote.exists?(id: [vote_1.id, vote_2.id, vote_3.id, vote_4.id])).to eq(
+        false,
+      )
+      expect(QuestionAnswerVote.exists?(id: [vote_5.id, vote_6.id, vote_7.id, vote_8.id])).to eq(
+        true,
+      )
+      expect(QuestionAnswerVote.count).to eq(4)
+
+      expect(post.reload.qa_vote_count).to eq(-1)
+      expect(topic_post.reload.qa_vote_count).to eq(1)
+      expect(comment_1.reload.qa_vote_count).to eq(1)
+      expect(comment_2.reload.qa_vote_count).to eq(0)
+      expect(comment_3.reload.qa_vote_count).to eq(1)
+    end
+  end
 end

spec/serializers/question_answer/comment_serializer_spec.rb

@@ -13,28 +13,46 @@
     PostVoting::VoteManager.vote(comment, post.user)
   end
 
-  it "returns the right attributes for an anonymous user" do
-    serializer = described_class.new(comment, scope: Guardian.new)
-    serilized_comment = serializer.as_json[:question_answer_comment]
-
-    expect(serilized_comment[:id]).to eq(comment.id)
-    expect(serilized_comment[:created_at]).to eq_time(comment.created_at)
-    expect(serilized_comment[:post_voting_vote_count]).to eq(1)
-    expect(serilized_comment[:cooked]).to eq(comment.cooked)
-    expect(serilized_comment[:name]).to eq(comment.user.name)
-    expect(serilized_comment[:username]).to eq(comment.user.username)
+  context "with a comment user" do
+    it "returns the right attributes for an anonymous user" do
+      serializer = described_class.new(comment, scope: Guardian.new)
+      serilized_comment = serializer.as_json[:question_answer_comment]
+
+      expect(serilized_comment[:id]).to eq(comment.id)
+      expect(serilized_comment[:created_at]).to eq_time(comment.created_at)
+      expect(serilized_comment[:post_voting_vote_count]).to eq(1)
+      expect(serilized_comment[:cooked]).to eq(comment.cooked)
+      expect(serilized_comment[:name]).to eq(comment.user.name)
+      expect(serilized_comment[:username]).to eq(comment.user.username)
+    end
+
+    it "returns the right attributes for logged in user" do
+      serializer = described_class.new(comment, scope: Guardian.new(post.user))
+      serilized_comment = serializer.as_json[:question_answer_comment]
+
+      expect(serilized_comment[:id]).to eq(comment.id)
+      expect(serilized_comment[:created_at]).to eq_time(comment.created_at)
+      expect(serilized_comment[:post_voting_vote_count]).to eq(1)
+      expect(serilized_comment[:cooked]).to eq(comment.cooked)
+      expect(serilized_comment[:name]).to eq(comment.user.name)
+      expect(serilized_comment[:username]).to eq(comment.user.username)
+      expect(serilized_comment[:user_voted]).to eq(true)
+    end
   end
 
-  it "returns the right attributes for logged in user" do
-    serializer = described_class.new(comment, scope: Guardian.new(post.user))
-    serilized_comment = serializer.as_json[:question_answer_comment]
-
-    expect(serilized_comment[:id]).to eq(comment.id)
-    expect(serilized_comment[:created_at]).to eq_time(comment.created_at)
-    expect(serilized_comment[:post_voting_vote_count]).to eq(1)
-    expect(serilized_comment[:cooked]).to eq(comment.cooked)
-    expect(serilized_comment[:name]).to eq(comment.user.name)
-    expect(serilized_comment[:username]).to eq(comment.user.username)
-    expect(serilized_comment[:user_voted]).to eq(true)
+  context "with a deleted comment user" do
+    before do
+      comment.user.destroy
+      comment.reload
+    end
+
+    it "does not fail to serialize" do
+      serializer = described_class.new(comment, scope: Guardian.new(post.user))
+      serilized_comment = serializer.as_json[:question_answer_comment]
+
+      expect(serilized_comment[:id]).to eq(comment.id)
+      expect(serilized_comment[:name]).to be_nil
+      expect(serilized_comment[:username]).to be_nil
+    end
   end
 end

test/javascripts/acceptance/post-voting-test.js

@@ -87,8 +87,8 @@ function postVotingEnabledTopicResponse() {
     {
       id: 6,
       user_id: 12345678,
-      name: "Some Name 4 ",
-      username: "someusername4",
+      name: null,
+      username: null,
       created_at: "2022-01-12T08:21:54.175Z",
       cooked: "<p>Test comment 6</p>",
       post_voting_vote_count: 0,
@@ -231,6 +231,12 @@ acceptance("Discourse Post Voting - anon user", function (needs) {
       6,
       "displays the right number of comments after loading more"
     );
+
+    assert
+      .dom(
+        "#post_2 .post-voting-comments #post-voting-comment-6 .post-voting-comment-info-username"
+      )
+      .hasText(I18n.t("post_voting.post.post_voting_comment.user.deleted"));
   });
 
   test("adding a comment", async function (assert) {