Comments count according to display rules in feed (forem#20753)

* Move calculating comments score from worker to a service

* Display displayed comments count in feed

* Recalculating displayed comments counts on article show (if needed)

* Rubocop fixes

* Fixed lambda

app/controllers/stories_controller.rb

@@ -286,7 +286,7 @@ def assign_article_show_variables
     @organization = @article.organization
     @comments_order = fetch_sort_order
 
-    @comments_count = Comments::Count.new(@article).call
+    @comments_count = Comments::Count.call(@article)
 
     if @article.collection
       @collection = @article.collection

app/queries/comments/count.rb

@@ -4,22 +4,33 @@
 
 module Comments
   class Count
-    attr_reader :article
+    def self.call(...)
+      new(...).call
+    end
 
-    def initialize(article)
+    def initialize(article, recalculate: false)
       @article = article
+      @recalculate = recalculate
     end
 
     def call
-      # comments that are not displayed at all (not even a "comment deleted" message):
-      # with the score below hiding threshold and w/o children
-      count_sql = "SELECT COUNT(id) FROM comments c1 WHERE score < ? AND commentable_id = ? " \
-                  "AND commentable_type = ? AND NOT EXISTS " \
-                  "(SELECT 1 FROM comments c2 WHERE c2.ancestry LIKE CONCAT('%/', c1.id::varchar(255)) " \
-                  "OR c2.ancestry = c1.id::varchar(255))"
-      san_count_sql = Comment.sanitize_sql([count_sql, Comment::HIDE_THRESHOLD, @article.id, "Article"])
-      hidden_comments_cnt = Comment.count_by_sql(san_count_sql)
-      article.comments_count - hidden_comments_cnt
+      if recalculate || !article.displayed_comments_count?
+        # comments that are not displayed at all (not even a "comment deleted" message):
+        # with the score below hiding threshold and w/o children
+        count_sql = "SELECT COUNT(id) FROM comments c1 WHERE score < ? AND commentable_id = ? " \
+                    "AND commentable_type = ? AND NOT EXISTS " \
+                    "(SELECT 1 FROM comments c2 WHERE c2.ancestry LIKE CONCAT('%/', c1.id::varchar(255)) " \
+                    "OR c2.ancestry = c1.id::varchar(255))"
+        san_count_sql = Comment.sanitize_sql([count_sql, Comment::HIDE_THRESHOLD, @article.id, "Article"])
+        hidden_comments_cnt = Comment.count_by_sql(san_count_sql)
+        displayed_comments_count = article.comments.count - hidden_comments_cnt
+        article.update_column(:displayed_comments_count, displayed_comments_count)
+      end
+      article.displayed_comments_count
     end
+
+    private
+
+    attr_reader :article, :recalculate
   end
 end

app/queries/homepage/articles_query.rb

@@ -4,6 +4,7 @@ class ArticlesQuery
       cached_tag_list
       comments_count
       crossposted_at
+      displayed_comments_count
       id
       organization_id
       path

app/serializers/homepage/article_serializer.rb

@@ -20,7 +20,6 @@ def self.serialized_collection_from(relation:)
     attributes(
       :class_name,
       :cloudinary_video_url,
-      :comments_count,
       :id,
       :path,
       :public_reactions_count,
@@ -31,6 +30,10 @@ def self.serialized_collection_from(relation:)
       :public_reaction_categories,
     )
 
+    # return displayed_comments_count (excluding low score comments) if it was calculated earlier
+    attribute :comments_count, (lambda do |article|
+      article.displayed_comments_count? ? article.displayed_comments_count : article.comments_count
+    end)
     attribute :video_duration_string, &:video_duration_in_minutes
     attribute :published_at_int, ->(article) { article.published_at.to_i }
     attribute :tag_list, ->(article) { article.cached_tag_list.to_s.split(", ") }

app/services/comments/calculate_score.rb

@@ -0,0 +1,32 @@
+module Comments
+  class CalculateScore
+    def self.call(...)
+      new(...).call
+    end
+
+    def initialize(comment)
+      @comment = comment
+    end
+
+    def call
+      score = BlackBox.comment_quality_score(comment)
+      score -= 500 if comment.user&.spam?
+      comment.update_columns(score: score, updated_at: Time.current)
+
+      comment.user&.touch(:last_comment_at)
+
+      # update commentable
+      commentable = comment.commentable
+
+      commentable.touch(:last_comment_at) if commentable.respond_to?(:last_comment_at)
+      Comments::Count.call(commentable, recalculate: true) if commentable.is_a?(Article)
+
+      # busting comment cache includes busting commentable cache
+      Comments::BustCacheWorker.new.perform(comment.id)
+    end
+
+    private
+
+    attr_reader :comment
+  end
+end

app/workers/comments/calculate_score_worker.rb

@@ -8,15 +8,7 @@ def perform(comment_id)
       comment = Comment.find_by(id: comment_id)
       return unless comment
 
-      score = BlackBox.comment_quality_score(comment)
-      score -= 500 if comment.user&.spam?
-      comment.update_columns(score: score, updated_at: Time.current)
-
-      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)
+      Comments::CalculateScore.call(comment)
     end
   end
 end

db/migrate/20240306121718_add_displayed_comments_count_to_articles.rb

@@ -0,0 +1,5 @@
+class AddDisplayedCommentsCountToArticles < ActiveRecord::Migration[7.0]
+  def change
+    add_column :articles, :displayed_comments_count, :integer
+  end
+end

db/schema.rb

@@ -103,6 +103,7 @@
     t.datetime "created_at", precision: nil, null: false
     t.datetime "crossposted_at", precision: nil
     t.string "description"
+    t.integer "displayed_comments_count"
     t.datetime "edited_at", precision: nil
     t.boolean "email_digest_eligible", default: true
     t.float "experience_level_rating", default: 5.0

spec/queries/comments/count_spec.rb

@@ -51,4 +51,37 @@
     count = described_class.new(article).call
     expect(count).to eq(4)
   end
+
+  context "with recalculate option" do
+    # displayed comments count = 4
+    # comments count = 5
+    before do
+      comment.update_column(:score, -500)
+      create(:comment, commentable: article, parent: comment, score: 10)
+      create(:comment, commentable: article, parent: comment, score: -490)
+      create(:comment, commentable: article, parent: comment2, score: 0)
+      article.reload
+    end
+
+    it "returns displayed_comments_count if it exists + no recalculate" do
+      article.update_column(:displayed_comments_count, 3)
+      cnt = described_class.call(article, recalculate: false)
+      expect(cnt).to eq(3)
+    end
+
+    it "recalculates if recalculate is passed", :aggregate_failures do
+      article.update_column(:displayed_comments_count, 3)
+      cnt = described_class.call(article, recalculate: true)
+      expect(cnt).to eq(4)
+      article.reload
+      expect(article.displayed_comments_count).to eq(4)
+    end
+
+    it "recalculates if no recalculate and no displayed_comments_count", :aggregate_failures do
+      cnt = described_class.call(article, recalculate: false)
+      expect(cnt).to eq(4)
+      article.reload
+      expect(article.displayed_comments_count).to eq(4)
+    end
+  end
 end

spec/services/comments/calculate_score_spec.rb

@@ -0,0 +1,40 @@
+require "rails_helper"
+
+RSpec.describe Comments::CalculateScore, type: :service do
+  let(:article) { create(:article) }
+  let(:comment) { create(:comment, commentable: article) }
+  let(:user) { instance_double(User, spam?: false) }
+
+  before do
+    allow(BlackBox).to receive(:comment_quality_score).and_return(7)
+  end
+
+  it "updates the score" do
+    described_class.call(comment)
+    comment.reload
+    expect(comment.score).to be(7)
+  end
+
+  context "when adding spam role" do
+    before do
+      comment.user.add_role(:spam)
+      comment.update_column(:updated_at, 1.day.ago)
+    end
+
+    it "updates the score and updated_at with a penalty if the user is a spammer", :aggregate_failures do
+      described_class.call(comment)
+      comment.reload
+      expect(comment.score).to be(-493)
+      expect(comment.updated_at).to be_within(1.minute).of(Time.current)
+    end
+
+    it "updates article displayed comments count" do
+      other_comment = create(:comment, commentable: article)
+      create(:comment, commentable: article, parent: other_comment)
+      described_class.call(comment)
+      article.reload
+      expect(article.comments_count).to eq(3)
+      expect(article.displayed_comments_count).to eq(2)
+    end
+  end
+end

spec/workers/comments/calculate_score_worker_spec.rb

@@ -7,29 +7,15 @@
     let(:worker) { subject }
 
     context "with comment" do
-      let(:article) { create(:article) }
-      let(:comment) { create(:comment, commentable: article) }
-      let(:root_comment) { instance_double(Comment) }
-      let(:user) { instance_double(User, spam?: false) }
+      let(:comment) { create(:comment) }
 
       before do
-        allow(BlackBox).to receive(:comment_quality_score).and_return(7)
+        allow(Comments::CalculateScore).to receive(:call)
       end
 
-      it "updates the score" do
+      it "calls CalculateScore" do
         worker.perform(comment.id)
-
-        comment.reload
-        expect(comment.score).to be(7)
-      end
-
-      it "updates the score and updated_at with a penalty if the user is a spammer", :aggregate_failures do
-        comment.user.add_role(:spam)
-        comment.update_column(:updated_at, 1.day.ago)
-        worker.perform(comment.id)
-        comment.reload
-        expect(comment.score).to be(-493)
-        expect(comment.updated_at).to be_within(1.minute).of(Time.current)
+        expect(Comments::CalculateScore).to have_received(:call).with(comment)
       end
     end