From 51316c747f4c7fc749fe509b31624eda5c7e1eb2 Mon Sep 17 00:00:00 2001 From: Anna Buianova Date: Thu, 28 Mar 2024 15:12:05 +0300 Subject: [PATCH] Comments count according to display rules in feed (#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 | 2 +- app/queries/comments/count.rb | 33 ++++++++++----- app/queries/homepage/articles_query.rb | 1 + .../homepage/article_serializer.rb | 5 ++- app/services/comments/calculate_score.rb | 32 +++++++++++++++ .../comments/calculate_score_worker.rb | 10 +---- ...dd_displayed_comments_count_to_articles.rb | 5 +++ db/schema.rb | 1 + spec/queries/comments/count_spec.rb | 33 +++++++++++++++ .../services/comments/calculate_score_spec.rb | 40 +++++++++++++++++++ .../comments/calculate_score_worker_spec.rb | 22 ++-------- 11 files changed, 144 insertions(+), 40 deletions(-) create mode 100644 app/services/comments/calculate_score.rb create mode 100644 db/migrate/20240306121718_add_displayed_comments_count_to_articles.rb create mode 100644 spec/services/comments/calculate_score_spec.rb diff --git a/app/controllers/stories_controller.rb b/app/controllers/stories_controller.rb index 98f0eed13..18c9cd208 100644 --- a/app/controllers/stories_controller.rb +++ b/app/controllers/stories_controller.rb @@ -286,7 +286,7 @@ class StoriesController < ApplicationController @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 diff --git a/app/queries/comments/count.rb b/app/queries/comments/count.rb index 69f01b232..cceead096 100644 --- a/app/queries/comments/count.rb +++ b/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 diff --git a/app/queries/homepage/articles_query.rb b/app/queries/homepage/articles_query.rb index c25e1ec94..21ec0151b 100644 --- a/app/queries/homepage/articles_query.rb +++ b/app/queries/homepage/articles_query.rb @@ -4,6 +4,7 @@ module Homepage cached_tag_list comments_count crossposted_at + displayed_comments_count id organization_id path diff --git a/app/serializers/homepage/article_serializer.rb b/app/serializers/homepage/article_serializer.rb index 072d8565a..5b3369e56 100644 --- a/app/serializers/homepage/article_serializer.rb +++ b/app/serializers/homepage/article_serializer.rb @@ -20,7 +20,6 @@ module Homepage attributes( :class_name, :cloudinary_video_url, - :comments_count, :id, :path, :public_reactions_count, @@ -31,6 +30,10 @@ module Homepage :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(", ") } diff --git a/app/services/comments/calculate_score.rb b/app/services/comments/calculate_score.rb new file mode 100644 index 000000000..aab4b515c --- /dev/null +++ b/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 diff --git a/app/workers/comments/calculate_score_worker.rb b/app/workers/comments/calculate_score_worker.rb index 06e5d7b97..19d182f21 100644 --- a/app/workers/comments/calculate_score_worker.rb +++ b/app/workers/comments/calculate_score_worker.rb @@ -8,15 +8,7 @@ module Comments 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 diff --git a/db/migrate/20240306121718_add_displayed_comments_count_to_articles.rb b/db/migrate/20240306121718_add_displayed_comments_count_to_articles.rb new file mode 100644 index 000000000..cad3bf29d --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index afc8c3823..4d6ee8237 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -103,6 +103,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_03_06_173309) do 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 diff --git a/spec/queries/comments/count_spec.rb b/spec/queries/comments/count_spec.rb index 6c5080aad..90541fcef 100644 --- a/spec/queries/comments/count_spec.rb +++ b/spec/queries/comments/count_spec.rb @@ -51,4 +51,37 @@ RSpec.describe Comments::Count do 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 diff --git a/spec/services/comments/calculate_score_spec.rb b/spec/services/comments/calculate_score_spec.rb new file mode 100644 index 000000000..de329c847 --- /dev/null +++ b/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 diff --git a/spec/workers/comments/calculate_score_worker_spec.rb b/spec/workers/comments/calculate_score_worker_spec.rb index 6aa5e8981..af9eba116 100644 --- a/spec/workers/comments/calculate_score_worker_spec.rb +++ b/spec/workers/comments/calculate_score_worker_spec.rb @@ -7,29 +7,15 @@ RSpec.describe Comments::CalculateScoreWorker, type: :worker do 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