Optimize and test comments cache after score change (#20640)

* Add specs for caching article comments, use cache_key_with_version

* Optimize cache busting for spammer comments, reorganized related specs

app/views/articles/_full_comment_area.html.erb

@@ -1,4 +1,4 @@
-<% cache("whole-comment-area-#{@article.id}-#{@article.last_comment_at}-#{@article.show_comments}-#{@discussion_lock&.updated_at}-#{@comments_order}-#{user_signed_in?}", expires_in: 2.hours) do %>
+<% cache("whole-comment-area-#{@article.id}-#{@article.comments.cache_key_with_version}-#{@article.show_comments}-#{@discussion_lock&.updated_at}-#{@comments_order}-#{user_signed_in?}", expires_in: 2.hours) do %>
   <section id="comments" data-follow-button-container="true" data-updated-at="<%= Time.current %>" class="text-padding mb-4 border-t-1 border-0 border-solid border-base-10">
     <% if @article.show_comments %>
       <header class="relative flex justify-between items-center mb-6">

app/workers/comments/calculate_score_worker.rb

@@ -11,7 +11,12 @@ module Comments
       score = BlackBox.comment_quality_score(comment)
       score -= 500 if comment.user&.spam?
       comment.update_columns(score: score, updated_at: Time.current)
-      comment.root.save! if !comment.is_root? && comment.root_exists?
+
+      comment.commentable.touch(:last_comment_at) if comment.commentable.respond_to?(:last_comment_at)
+      comment.user.touch(:last_comment_at) if comment.user
+
+      # busting comment cache includes busting commentable cache
+      Comments::BustCacheWorker.new.perform(comment.id)
     end
   end
 end

spec/requests/comments_with_cache_spec.rb

@@ -0,0 +1,79 @@
+# these tests were written to check the cache invalidation after adding spam role to the user
+# the cache is invalidated in Comments::CalculateScoreWorker
+# actually, the tests succeed if there is at least updating comment updated_at (in Comments::CalculateScoreWorker)
+
+require "rails_helper"
+
+RSpec.describe "ArticleCommentsWithCache" do
+  let(:user) { create(:user, :admin) }
+  let(:article) { create(:article, user: user, published: true) }
+  let(:user2) { create(:user) }
+
+  let(:parent) { create(:comment, commentable: article, user: user, body_markdown: "parent-comment") }
+
+  let(:cache_store) { ActiveSupport::Cache.lookup_store(:redis_cache_store) }
+  let(:cache) { Rails.cache }
+
+  before do
+    sign_in user
+    allow(Rails).to receive(:cache).and_return(cache_store)
+  end
+
+  def assign_spam_role
+    sidekiq_perform_enqueued_jobs do
+      Moderator::ManageActivityAndRoles.handle_user_roles(
+        admin: user,
+        user: user2,
+        user_params: {
+          note_for_current_role: "note",
+          user_status: "Spam"
+        },
+      )
+    end
+  end
+
+  describe "GET /:slug (articles)" do
+    it "busts comments cache" do
+      create(:comment, commentable: article, user: user2, body_markdown: "potential-spam-comment")
+      get article.path
+      expect(response.body).to include("potential-spam-comment")
+      assign_spam_role
+      get article.path
+      expect(response.body).not_to include("potential-spam-comment")
+    end
+
+    it "busts cache when spam comment is a child and a parent" do
+      comment = create(:comment, commentable: article, user: user2, parent: parent,
+                                 body_markdown: "potential-spam-comment")
+      create(:comment, commentable: article, user: user, body_markdown: "child-comment", parent: comment)
+
+      get article.path
+      expect(response.body).to include("potential-spam-comment")
+      expect(response.body).to include("parent-comment")
+      expect(response.body).to include("child-comment")
+
+      assign_spam_role
+
+      get article.path
+      expect(response.body).not_to include("potential-spam-comment")
+    end
+  end
+
+  describe "GET /:username/comment/:id_code (root comment path)" do
+    it "busts cache for root comment" do
+      comment = create(:comment, commentable: article, user: user2, parent: parent,
+                                 body_markdown: "potential-spam-comment")
+      create(:comment, commentable: article, user: user, body_markdown: "child-comment", parent: comment)
+
+      get parent.path
+      expect(response.body).to include("potential-spam-comment")
+      expect(response.body).to include("parent-comment")
+      expect(response.body).to include("child-comment")
+
+      assign_spam_role
+
+      get parent.path
+      expect(response.body).not_to include("potential-spam-comment")
+    end
+  end
+end

spec/workers/comments/calculate_score_worker_spec.rb

@@ -31,35 +31,6 @@ RSpec.describe Comments::CalculateScoreWorker, type: :worker do
         expect(comment.score).to be(-493)
         expect(comment.updated_at).to be_within(1.minute).of(Time.current)
       end
-
-      it "calls save on the root comment when given a descendant comment" do
-        child_comment = instance_double(Comment)
-
-        allow(root_comment).to receive(:save!)
-        allow(child_comment).to receive(:update_columns)
-        allow(child_comment).to receive_messages(is_root?: false, root_exists?: true, root: root_comment,
-                                                 user: user)
-        allow(Comment).to receive(:find_by).with(id: 1).and_return(child_comment)
-
-        worker.perform(1)
-
-        expect(child_comment).to have_received(:is_root?)
-        expect(child_comment).to have_received(:root)
-        expect(root_comment).to have_received(:save!)
-      end
-
-      it "does not call save on the root comment" do
-        allow(root_comment).to receive(:save)
-        allow(root_comment).to receive(:update_columns)
-        allow(root_comment).to receive_messages(is_root?: true, root: root_comment, user: user)
-        allow(Comment).to receive(:find_by).with(id: 1).and_return(root_comment)
-
-        worker.perform(1)
-
-        expect(root_comment).to have_received(:is_root?)
-        expect(root_comment).not_to have_received(:root)
-        expect(root_comment).not_to have_received(:save)
-      end
     end
 
     context "without comment" do