Refactoring and extending article spam behavior (#15399)

* Refactoring and extending article spam behavior

Prior to this refactor, we were checking an Article's title for
spaminess.  This change now checks the Article's title and
body_markdown.

In addition, the encapsulates the regular expression implementations in
favor of asking the Settings::RateLimit class to determine if the passed
in text is "spammy".

Furthermore, instead of having lots of inline logic within the article,
this refactor introduces a `Spam::ArticleHandler` class.  This helps
tidy up the already busy Article specs by delegating the business logic
of Spam handling to it's own class.

There are further refactors for Comments and Users that would follow
this pattern, but this change is more important to get out the door.

Closes #15396

* Adding comments to clarify behavior of spam handling

* Adding missing expectation to spec

app/models/article.rb

@@ -809,27 +809,7 @@ class Article < ApplicationRecord
   end
 
   def create_conditional_autovomits
-    return unless Settings::RateLimit.spam_trigger_terms.any? do |term|
-                    Regexp.new(term.downcase).match?(title.downcase)
-                  end
-
-    Reaction.create(
-      user_id: Settings::General.mascot_user_id,
-      reactable_id: id,
-      reactable_type: "Article",
-      category: "vomit",
-    )
-
-    return unless Reaction.article_vomits.where(reactable_id: user.articles.pluck(:id)).size > 2
-
-    user.add_role(:suspended)
-    Note.create(
-      author_id: Settings::General.mascot_user_id,
-      noteable_id: user_id,
-      noteable_type: "User",
-      reason: "automatic_suspend",
-      content: "User suspended for too many spammy articles, triggered by autovomit.",
-    )
+    Spam::ArticleHandler.handle!(article: self)
   end
 
   def async_bust

app/models/reaction.rb

@@ -72,6 +72,14 @@ class Reaction < ApplicationRecord
         Reaction.where(reactable_id: reactable.id, reactable_type: class_name, user: user, category: category).any?
       end
     end
+
+    # @param user [User] the user who might be spamming the system
+    #
+    # @return [TrueClass] yup, they're spamming the system.
+    # @return [FalseClass] they're not (yet) spamming the system
+    def user_has_been_given_too_many_spammy_reactions?(user:)
+      article_vomits.where(reactable_type: "Article", reactable_id: user.articles.pluck(:id)).size > 2
+    end
   end
 
   # no need to send notification if:

app/models/settings/rate_limit.rb

@@ -21,5 +21,18 @@ module Settings
     setting :user_considered_new_days, type: :integer, default: 3
     setting :user_subscription_creation, type: :integer, default: 3
     setting :user_update, type: :integer, default: 15
+
+    # A helper function to determine if text is spammy.
+    #
+    # @param text [String] text to check for "spamminess"
+    #
+    # @return [TrueClass] if this is spammy
+    # @return [FalseClass] if this isn't spammy
+    def self.trigger_spam_for?(text:)
+      return false if spam_trigger_terms.empty?
+
+      regexp = Regexp.new("(#{spam_trigger_terms.join('|')})", true)
+      regexp.match?(text)
+    end
   end
 end

app/services/spam/article_handler.rb

@@ -0,0 +1,42 @@
+module Spam
+  module ArticleHandler
+    # Test the article for spamminess.  If it's not spammy, don't do anything.
+    #
+    # If it is spammy, escalate the situation!
+    #
+    # @param article [Article] the article to check for spamminess
+    # @param attributes [Array<Symbol>] test these attributes of the article.
+    #
+    # @note We are not blocking the spammer from submitting the
+    # article with spam content, but we are instead performing some
+    # administrative tasks to indicate that we're taking notice and
+    # even escalating.
+    def self.handle!(article:, attributes: %i[title body_markdown])
+      return :not_spam if attributes.none? do |attr|
+                            Settings::RateLimit.trigger_spam_for?(text: article.public_send(attr))
+                          end
+
+      Reaction.create!(
+        user_id: Settings::General.mascot_user_id,
+        reactable_id: article.id,
+        reactable_type: "Article",
+        category: "vomit",
+      )
+
+      return unless Reaction.user_has_been_given_too_many_spammy_reactions?(user: article.user)
+
+      # TODO: What should we do with their active session?  I think we
+      # might want to explore later raising an exception that the
+      # controller rescues and logs the user out.  Because, as written
+      # they might still be logged in but have limited abilities.
+      article.user.add_role(:suspended)
+
+      Note.create(
+        author_id: Settings::General.mascot_user_id,
+        noteable: article.user,
+        reason: "automatic_suspend",
+        content: "User suspended for too many spammy articles, triggered by autovomit.",
+      )
+    end
+  end
+end

spec/models/article_spec.rb

@@ -1033,56 +1033,10 @@ RSpec.describe Article, type: :model do
     end
 
     describe "spam" do
-      before do
-        allow(Settings::General).to receive(:mascot_user_id).and_return(user.id)
-        allow(Settings::RateLimit).to receive(:spam_trigger_terms).and_return(
-          ["yahoomagoo gogo", "testtestetest", "magoo.+magee"],
-        )
-      end
-
-      it "creates vomit reaction if possible spam" do
-        article.body_markdown = article.body_markdown.gsub(article.title, "This post is about Yahoomagoo gogo")
+      it "delegates spam handling to Spam::ArticleHandler" do
+        allow(Spam::ArticleHandler).to receive(:handle!).with(article: article).and_call_original
         article.save
-        expect(Reaction.last.category).to eq("vomit")
-        expect(Reaction.last.user_id).to eq(user.id)
-      end
-
-      it "creates vomit reaction if possible spam based on pattern" do
-        article.body_markdown = article.body_markdown.gsub(article.title, "This post is about magoo to the magee")
-        article.save
-        expect(Reaction.last.category).to eq("vomit")
-        expect(Reaction.last.user_id).to eq(user.id)
-      end
-
-      it "does not suspend user if only single vomit" do
-        article.body_markdown = article.body_markdown.gsub(article.title, "This post is about Yahoomagoo gogo")
-        article.save
-        expect(article.user.suspended?).to be false
-      end
-
-      it "suspends user with 3 comment vomits" do
-        second_article = create(:article, user: article.user)
-        third_article = create(:article, user: article.user)
-        article.body_markdown = article.body_markdown.gsub(article.title, "This post is about Yahoomagoo gogo")
-        second_article.body_markdown = second_article.body_markdown.gsub(second_article.title, "testtestetest")
-        third_article.body_markdown = third_article.body_markdown.gsub(third_article.title, "yahoomagoo gogo")
-
-        article.save
-        second_article.save
-        third_article.save
-        expect(article.user.suspended?).to be true
-        expect(Note.last.reason).to eq "automatic_suspend"
-      end
-
-      it "does not create vomit reaction if does not have matching title" do
-        article.save
-        expect(Reaction.last).to be nil
-      end
-
-      it "does not create vomit reaction if does not have pattern match" do
-        article.body_markdown = article.body_markdown.gsub(article.title, "This post is about magoo to")
-        article.save
-        expect(Reaction.last).to be nil
+        expect(Spam::ArticleHandler).to have_received(:handle!).with(article: article)
       end
     end
 

spec/models/reaction_spec.rb

@@ -13,6 +13,12 @@ RSpec.describe Reaction, type: :model do
     it { is_expected.to validate_uniqueness_of(:user_id).scoped_to(%i[reactable_id reactable_type category]) }
   end
 
+  describe ".user_has_been_given_too_many_spammy_reactions?" do
+    it "performs a valid query for the user" do
+      expect { described_class.user_has_been_given_too_many_spammy_reactions?(user: user) }.not_to raise_error
+    end
+  end
+
   describe "counter_culture" do
     context "when a reaction is created" do
       it "increments reaction count on user" do

spec/models/settings/rate_limit_spec.rb

@@ -0,0 +1,33 @@
+require "rails_helper"
+
+RSpec.describe Settings::RateLimit, type: :model do
+  describe ".trigger_spam_for?" do
+    subject { described_class.trigger_spam_for?(text: text) }
+
+    let(:text) { "text to test" }
+
+    context "when it matches a spam trigger term" do
+      before do
+        allow(described_class).to receive(:spam_trigger_terms).and_return(["to test"])
+      end
+
+      it { is_expected.to be_truthy }
+    end
+
+    context "when there are no spam trigger terms" do
+      before do
+        allow(described_class).to receive(:spam_trigger_terms).and_return([])
+      end
+
+      it { is_expected.to be_falsey }
+    end
+
+    context "when there are spam trigger terms but they don't match" do
+      before do
+        allow(described_class).to receive(:spam_trigger_terms).and_return(["in a hole in the ground"])
+      end
+
+      it { is_expected.to be_falsey }
+    end
+  end
+end

spec/services/spam/article_handler_spec.rb

@@ -0,0 +1,52 @@
+require "rails_helper"
+
+RSpec.describe Spam::ArticleHandler, type: :service do
+  describe ".handle!" do
+    subject(:handler) { described_class.handle!(article: article) }
+
+    let!(:article) { create(:article) }
+    let(:mascot_user) { create(:user) }
+
+    before do
+      allow(Settings::General).to receive(:mascot_user_id).and_return(mascot_user.id)
+    end
+
+    context "when non-spammy content" do
+      before do
+        allow(Settings::RateLimit).to receive(:trigger_spam_for?).with(text: article.title).and_return(false)
+        allow(Settings::RateLimit).to receive(:trigger_spam_for?).with(text: article.body_markdown).and_return(false)
+      end
+
+      it { is_expected.to eq(:not_spam) }
+    end
+
+    context "when first time spammy content" do
+      before do
+        allow(Settings::RateLimit).to receive(:trigger_spam_for?).with(text: article.title).and_return(true)
+        allow(Settings::RateLimit).to receive(:trigger_spam_for?).with(text: article.body_markdown).and_return(false)
+        allow(Reaction).to receive(:user_has_been_given_too_many_spammy_reactions?)
+          .with(user: article.user).and_return(false)
+      end
+
+      it "creates a reaction but does not suspend the user" do
+        expect { handler }.to change { Reaction.where(reactable: article, category: "vomit").count }.by(1)
+        expect(article.user.reload).not_to be_suspended
+      end
+    end
+
+    context "when multiple offender of spammy" do
+      before do
+        allow(Settings::RateLimit).to receive(:trigger_spam_for?).with(text: article.title).and_return(true)
+        allow(Settings::RateLimit).to receive(:trigger_spam_for?).with(text: article.body_markdown).and_return(false)
+        allow(Reaction).to receive(:user_has_been_given_too_many_spammy_reactions?)
+          .with(user: article.user).and_return(true)
+      end
+
+      it "creates a reaction, suspends the user, and creates a note for the user" do
+        expect { handler }.to change { Reaction.where(reactable: article, category: "vomit").count }.by(1)
+        expect(article.user.reload).to be_suspended
+        expect(Note.where(noteable: article.user, reason: "automatic_suspend").count).to eq(1)
+      end
+    end
+  end
+end