From 16d17445a553ecb9adeb0bd8dba55cf10c0597bd Mon Sep 17 00:00:00 2001
From: Daniel Uber <djuber@gmail.com>
Date: Tue, 16 Mar 2021 14:04:13 -0500
Subject: [PATCH] reenable article rescoring when moderators flag users 
 (#12983)

* Update worker calculation and disable early return

* Enable service tests

This updates the test to look at the individual article scores
changing, rather than the sum of the articles scores changing

The scores lambda was added as a way to observe this eagerly

* Correctly handle raising the score when vomit is cleared

The initial implementation loaded the articles reactions and the
user's reactions in order to rescore. This restores that activity.

It occurs to me that article.reactions.sum(:points) +
user.reactions.sum(:points) feels like something articles should know
how to do, in which case we can move this into article (#rescore!)?
and call that instead of knowing how to handle moderation specially here.

* Move logic to Article#update_score

Having realized "sum of reaction scores" was an Article concern, I
found that we have a worker to score articles that does _exactly_ what
we're doing here.

Let's use that and allow the vomit reaction presence or absence on a
user to automatically affect the article calculations.

This is at least n + 1 and maybe worse.
---
 app/services/moderator/sink_articles.rb       |  7 ------
 app/workers/moderator/sink_articles_worker.rb |  5 +----
 spec/services/moderator/sink_articles_spec.rb | 22 ++++++++++++++++---
 3 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/app/services/moderator/sink_articles.rb b/app/services/moderator/sink_articles.rb
index 9b3722afd..feeee2d89 100644
--- a/app/services/moderator/sink_articles.rb
+++ b/app/services/moderator/sink_articles.rb
@@ -1,14 +1,7 @@
 module Moderator
   class SinkArticles
     def self.call(user_id)
-      # rubocop:disable Lint/UnreachableCode
-      # HOTFIX
-      # @mstruve: This worker is broken and needs to be fixed. It can inadvertantly
-      # bump up scores for a user's articles if they have a lot of good reactions
-      # this leads to clumps of articles from the same user in feeds
-      return
       Moderator::SinkArticlesWorker.perform_async(user_id)
-      # rubocop:enable Lint/UnreachableCode
     end
   end
 end
diff --git a/app/workers/moderator/sink_articles_worker.rb b/app/workers/moderator/sink_articles_worker.rb
index 7ac48d9ce..9b2442335 100644
--- a/app/workers/moderator/sink_articles_worker.rb
+++ b/app/workers/moderator/sink_articles_worker.rb
@@ -8,10 +8,7 @@ module Moderator
       user = User.find_by(id: user_id)
       return unless user
 
-      articles = Article.where(user: user)
-      reactions = Reaction.where(reactable: articles)
-      new_score = reactions.sum(:points) + Reaction.where(reactable: user).sum(:points)
-      articles.update_all(score: new_score)
+      Article.where(user: user).each(&:update_score)
     rescue StandardError => e
       ForemStatsClient.count("moderators.sink", 1, tags: ["action:failed", "user_id:#{user.id}"])
       Honeybadger.notify(e)
diff --git a/spec/services/moderator/sink_articles_spec.rb b/spec/services/moderator/sink_articles_spec.rb
index 9c739a3f6..6efd99c8c 100644
--- a/spec/services/moderator/sink_articles_spec.rb
+++ b/spec/services/moderator/sink_articles_spec.rb
@@ -7,16 +7,17 @@ RSpec.describe Moderator::SinkArticles, type: :service do
     create_list(:article, 3, user: user)
     user
   end
+  let(:scores) { -> { spam_user.articles.reload.pluck(:score) } }
   let(:vomit_reaction) { create(:reaction, reactable: spam_user, user: moderator, category: "vomit") }
 
-  xdescribe "#call" do
+  describe "#call" do
     it "lowers all of a user's articles' scores by 25 each if not confirmed" do
       vomit_reaction
       expect do
         sidekiq_perform_enqueued_jobs do
           described_class.call(spam_user.id)
         end
-      end.to change { spam_user.articles.sum(:score) }.from(0).to(-75)
+      end.to change(scores, :call).from([0, 0, 0]).to([-25, -25, -25])
     end
 
     it "lowers all of the user's articles' scores by 50 each if confirmed" do
@@ -25,7 +26,22 @@ RSpec.describe Moderator::SinkArticles, type: :service do
         sidekiq_perform_enqueued_jobs do
           described_class.call(spam_user.id)
         end
-      end.to change { spam_user.articles.sum(:score) }.from(0).to(-150)
+      end.to change(scores, :call).from([0, 0, 0]).to([-50, -50, -50])
+    end
+
+    context "when removing a user vomit reaction" do
+      before do
+        # pretend we had a confirmed vomit on the user
+        spam_user.articles.update(score: -50)
+      end
+
+      it "raises all of the user's articles but the moderation score" do
+        expect do
+          sidekiq_perform_enqueued_jobs do
+            described_class.call(spam_user.id)
+          end
+        end.to change(scores, :call).from([-50, -50, -50]).to([0, 0, 0])
+      end
     end
   end
 end