FIX: always check for a bio before sending to Akismet

FIX: always check for a bio before sending to Akismet

diff --git a/lib/discourse_akismet/users_bouncer.rb b/lib/discourse_akismet/users_bouncer.rb
index b9a58c5..c99ea97 100644
--- a/lib/discourse_akismet/users_bouncer.rb
+++ b/lib/discourse_akismet/users_bouncer.rb
@@ -4,16 +4,20 @@ module DiscourseAkismet
   class UsersBouncer
     VALID_STATUSES = %w[spam ham]
 
-    def enqueue_for_check(user)
-      return unless SiteSetting.akismet_review_users
-      profile = user.user_profile
-      return if user.trust_level > TrustLevel[0] || profile.bio_raw.blank?
+    def should_check_user?(user)
+      SiteSetting.akismet_review_users &&
+        user.trust_level === TrustLevel[0] &&
+        user.user_profile.bio_raw.present? &&
+        !Reviewable.exists?(target: user)
+    end
 
+    def enqueue_for_check(user)
+      return if !should_check_user?(user)
       Jobs.enqueue(:check_users_for_spam, user_id: user.id)
     end
 
     def check_user(client, user)
-      return if Reviewable.exists?(target: user)
+      return if !should_check_user?(user)
 
       if client.comment_check(args_for_user(user))
         spam_reporter = Discourse.system_user
diff --git a/plugin.rb b/plugin.rb
index 9a831d7..fd21089 100644
--- a/plugin.rb
+++ b/plugin.rb
@@ -46,7 +46,7 @@ after_initialize do
     register_reviewable_type ReviewableAkismetUser
 
     add_model_callback(UserProfile, :before_save) do
-      if bio_raw_changed?
+      if bio_raw_changed? && bio_raw.present?
         DiscourseAkismet::UsersBouncer.new.enqueue_for_check(user)
       end
     end
diff --git a/spec/lib/users_bouncer_spec.rb b/spec/lib/users_bouncer_spec.rb
index f21abe1..e4a8d31 100644
--- a/spec/lib/users_bouncer_spec.rb
+++ b/spec/lib/users_bouncer_spec.rb
@@ -1,117 +1,93 @@
 # frozen_string_literal: true
 
-# frozen_string_literal
-
 require 'rails_helper'
 
-RSpec.describe DiscourseAkismet::UsersBouncer do
-  describe '#enqueue_for_check' do
-    before { SiteSetting.akismet_review_users = true }
-
-    let(:user) do
-      Fabricate.build(:user, user_profile: Fabricate.build(:user_profile), trust_level: TrustLevel[0])
-    end
-
-    it 'does not enqueue for check if user trust level is higher than 0' do
-      user.trust_level = TrustLevel[1]
-
-      queued_jobs_for_check(user, 0)
-    end
-
-    it 'enqueues the user when trust level is higher than zero and bio is present' do
-      user.user_profile.bio_raw = "Let's spam"
+RSpec.describe DiscourseAkismet::UsersBouncer, if: defined?(Reviewable) do
 
-      queued_jobs_for_check(user, 1)
-    end
+  let(:user) do
+    user = Fabricate(:user, trust_level: TrustLevel[0])
+    user.user_profile.bio_raw = "I am batman"
+    user
+  end
 
-    it 'does not enqueue for check if user bio is empty' do
-      user.user_profile.bio_raw = ''
+  before { SiteSetting.akismet_review_users = true }
 
-      queued_jobs_for_check(user, 0)
-    end
+  describe "#should_check_user?" do
 
-    it 'does not enqueue for check if the setting is turned off' do
+    it "returns false when setting is disabled" do
       SiteSetting.akismet_review_users = false
-      user.user_profile.bio_raw = "Let's spam"
-
-      queued_jobs_for_check(user, 0)
-    end
-
-    def queued_jobs_for_check(to_check, expected_queued_jobs)
-      expect {
-        subject.enqueue_for_check(to_check)
-      }.to change(Jobs::CheckUsersForSpam.jobs, :size).by(expected_queued_jobs)
-    end
-  end
 
-  describe '#check_user', if: defined?(Reviewable) do
-    let(:user) do
-      Fabricate(:user, trust_level: TrustLevel[0])
+      expect(subject.should_check_user?(user)).to eq(false)
     end
 
-    before { UserAuthToken.generate!(user_id: user.id) }
-
-    it "does not create a reviewable if akismet says it's not spam" do
-      should_find_spam = false
-
-      subject.check_user(build_client(should_find_spam), user)
+    it "returns false when user is higher than TL0" do
+      user.trust_level = TrustLevel[1]
 
-      expect(ReviewableAkismetUser.count).to be_zero
+      expect(subject.should_check_user?(user)).to eq(false)
     end
 
-    it "creates a reviewable if akismet says it's spam" do
-      should_find_spam = true
+    it "returns false when user has no bio" do
+      user.user_profile.bio_raw = ""
 
-      subject.check_user(build_client(should_find_spam), user)
-      created_reviewable = ReviewableAkismetUser.last
-      created_score = ReviewableScore.last
-
-      assert_reviewable_was_created_correctly(created_reviewable)
-      assert_score_was_created(created_score)
+      expect(subject.should_check_user?(user)).to eq(false)
     end
 
-    it 'does not create a reviewable if there is another one for the same user' do
+    it "returns false if a Reviewable already exists for that user" do
       ReviewableUser.create_for(user)
-      client = mock('Akismet::Client')
-      client.expects(:comment_check).never
-
-      subject.check_user(client, user)
-
-      expect(ReviewableAkismetUser.count).to be_zero
-    end
 
-    def assert_reviewable_was_created_correctly(reviewable)
-      expect(reviewable.target).to eq user
-      expect(reviewable.created_by).to eq Discourse.system_user
-      expect(reviewable.reviewable_by_moderator).to eq true
-      expect(reviewable.payload['username']).to eq user.username
-      expect(reviewable.payload['name']).to eq user.name
-      expect(reviewable.payload['email']).to eq user.email
-      expect(reviewable.payload['bio']).to eq user.user_profile.bio_raw
+      expect(subject.should_check_user?(user)).to eq(false)
     end
 
-    def assert_score_was_created(score)
-      expect(score.user).to eq Discourse.system_user
-      expect(score.reviewable_score_type).to eq PostActionType.types[:spam]
-      expect(score.take_action_bonus).to be_zero
+    it "returns true for TL0 with a bio" do
+      expect(subject.should_check_user?(user)).to eq(true)
     end
+  end
 
-    def build_client(detect_spam)
-      mock('Akismet::Client').tap do |client|
-        client.expects(:comment_check).returns(detect_spam)
-      end
+  describe "#enqueue_for_check" do
+    it "enqueues a job when user is TL0 and bio is present" do
+      expect {
+        subject.enqueue_for_check(user)
+      }.to change {
+        Jobs::CheckUsersForSpam.jobs.size
+      }.by(1)
     end
   end
 
-  describe '#submit_feedback' do
-    let(:user) { Fabricate.build(:user) }
+  describe "#check_user" do
+    it "does not create a Reviewable if Akismet says it's not spam" do
+      expect {
+        subject.check_user(akismet(is_spam: false), user)
+      }.to_not change {
+        ReviewableAkismetUser.count
+      }
+    end
 
-    it 'validates that the status is valid' do
-      non_valid_status = 'clear'
+    it "creates a Reviewable if Akismet says it's spam" do
+      expect {
+        subject.check_user(akismet(is_spam: true), user)
+      }.to change {
+        ReviewableAkismetUser.count
+      }.by(1)
+
+      reviewable = ReviewableAkismetUser.last
+      expect(reviewable.target).to eq(user)
+      expect(reviewable.created_by).to eq(Discourse.system_user)
+      expect(reviewable.reviewable_by_moderator).to eq(true)
+      expect(reviewable.payload['username']).to eq(user.username)
+      expect(reviewable.payload['name']).to eq(user.name)
+      expect(reviewable.payload['email']).to eq(user.email)
+      expect(reviewable.payload['bio']).to eq(user.user_profile.bio_raw)
+
+      score = ReviewableScore.last
+      expect(score.user).to eq(Discourse.system_user)
+      expect(score.reviewable_score_type).to eq(PostActionType.types[:spam])
+      expect(score.take_action_bonus).to eq(0)
+    end
 
-      expect do
-        described_class.new.submit_feedback(non_valid_status, user)
-      end.to raise_error(Discourse::InvalidParameters)
+    def akismet(is_spam:)
+      mock("Akismet::Client").tap do |client|
+        client.expects(:comment_check).returns(is_spam)
+      end
     end
   end
 end

GitHub sha: 1e7d31eb