FIX: Recalculate scores only when approving or transitioning to pending. (#13009)

FIX: Recalculate scores only when approving or transitioning to pending. (#13009)

Recalculating a ReviewableFlaggedPost’s score after rejecting or ignoring it sets the score as 0, which means that we can’t find them after reviewing. They don’t surpass the minimum priority threshold and are hidden.

Additionally, we only want to use agreed flags when calculating the different priority thresholds.

diff --git a/app/jobs/scheduled/reviewable_priorities.rb b/app/jobs/scheduled/reviewable_priorities.rb
index cb74ead..da4d9c8 100644
--- a/app/jobs/scheduled/reviewable_priorities.rb
+++ b/app/jobs/scheduled/reviewable_priorities.rb
@@ -14,9 +14,13 @@ class Jobs::ReviewablePriorities < ::Jobs::Scheduled
   end
 
   def execute(args)
-    return unless Reviewable.where('score > 0').count >= self.class.min_reviewables
-
     min_priority_threshold = SiteSetting.reviewable_low_priority_threshold
+    reviewable_count = Reviewable.where(
+      "score > ? AND status = ?",
+      min_priority_threshold,
+      Reviewable.statuses[:approved]
+    ).count
+    return if reviewable_count < self.class.min_reviewables
 
     res = DB.query_single(<<~SQL, target_count: self.class.target_count, min_priority: min_priority_threshold)
       SELECT COALESCE(PERCENTILE_DISC(0.5) WITHIN GROUP (ORDER BY score), 0.0) AS medium,
@@ -25,7 +29,7 @@ class Jobs::ReviewablePriorities < ::Jobs::Scheduled
         SELECT r.score
         FROM reviewables AS r
         INNER JOIN reviewable_scores AS rs ON rs.reviewable_id = r.id
-        WHERE r.score >= :min_priority
+        WHERE r.score > :min_priority AND r.status = 1
         GROUP BY r.id
         HAVING COUNT(*) >= :target_count
       ) AS x
diff --git a/app/models/reviewable.rb b/app/models/reviewable.rb
index 0d6ecb6..0e8af0b 100644
--- a/app/models/reviewable.rb
+++ b/app/models/reviewable.rb
@@ -175,7 +175,13 @@ class Reviewable < ActiveRecord::Base
         reviewable.save!
       else
         reviewable = find_by(target: target)
-        reviewable.log_history(:transitioned, created_by) if old_status != statuses[:pending]
+
+        if old_status != statuses[:pending]
+          # If we're transitioning back from reviewed to pending, we should recalculate
+          # the score to prevent posts from being hidden.
+          reviewable.recalculate_score
+          reviewable.log_history(:transitioned, created_by)
+        end
       end
     end
 
@@ -584,8 +590,6 @@ class Reviewable < ActiveRecord::Base
     SQL
   end
 
-protected
-
   def recalculate_score
     # pending/agreed scores count
     sql = <<~SQL
@@ -633,6 +637,8 @@ protected
     self.score
   end
 
+protected
+
   def increment_version!(version = nil)
     version_result = nil
 
diff --git a/app/models/reviewable_flagged_post.rb b/app/models/reviewable_flagged_post.rb
index d481ec5..dada954 100644
--- a/app/models/reviewable_flagged_post.rb
+++ b/app/models/reviewable_flagged_post.rb
@@ -127,7 +127,6 @@ class ReviewableFlaggedPost < Reviewable
 
     create_result(:success, :ignored) do |result|
       result.update_flag_stats = { status: :ignored, user_ids: actions.map(&:user_id) }
-      result.recalculate_score = true
     end
   end
 
@@ -205,7 +204,6 @@ class ReviewableFlaggedPost < Reviewable
 
     create_result(:success, :rejected) do |result|
       result.update_flag_stats = { status: :disagreed, user_ids: actions.map(&:user_id) }
-      result.recalculate_score = true
     end
   end
 
diff --git a/spec/jobs/reviewable_priorities_spec.rb b/spec/jobs/reviewable_priorities_spec.rb
index 9a5c96d..65f232f 100644
--- a/spec/jobs/reviewable_priorities_spec.rb
+++ b/spec/jobs/reviewable_priorities_spec.rb
@@ -16,12 +16,13 @@ describe Jobs::ReviewablePriorities do
   fab!(:user_0) { Fabricate(:user) }
   fab!(:user_1) { Fabricate(:user) }
 
-  def create_reviewables(count)
-    (1..count).each { |i| create_with_score(i) }
+  def create_reviewables(count, status: :approved)
+    minimum_threshold = SiteSetting.reviewable_low_priority_threshold
+    (1..count).each { |i| create_with_score(minimum_threshold + i) }
   end
 
-  def create_with_score(score)
-    Fabricate(:reviewable_flagged_post).tap do |reviewable|
+  def create_with_score(score, status: :approved)
+    Fabricate(:reviewable_flagged_post, status: Reviewable.statuses[status]).tap do |reviewable|
       reviewable.add_score(user_0, PostActionType.types[:off_topic])
       reviewable.add_score(user_1, PostActionType.types[:off_topic])
       reviewable.update!(score: score)
@@ -45,10 +46,9 @@ describe Jobs::ReviewablePriorities do
     Jobs::ReviewablePriorities.new.execute({})
 
     expect_min_score(:low, SiteSetting.reviewable_low_priority_threshold)
-    expect_min_score(:medium, 8.0)
-    expect_min_score('medium', 8.0)
-    expect_min_score(:high, 13.0)
-    expect(Reviewable.score_required_to_hide_post).to eq(8.66)
+    expect_min_score(:medium, 9.0)
+    expect_min_score(:high, 14.0)
+    expect(Reviewable.score_required_to_hide_post).to eq(9.33)
   end
 
   it 'ignore negative scores when calculating priorities' do
@@ -59,9 +59,22 @@ describe Jobs::ReviewablePriorities do
     Jobs::ReviewablePriorities.new.execute({})
 
     expect_min_score(:low, SiteSetting.reviewable_low_priority_threshold)
-    expect_min_score(:medium, 8.0)
-    expect_min_score(:high, 13.0)
-    expect(Reviewable.score_required_to_hide_post).to eq(8.66)
+    expect_min_score(:medium, 9.0)
+    expect_min_score(:high, 14.0)
+    expect(Reviewable.score_required_to_hide_post).to eq(9.33)
+  end
+
+  it 'ignores non-approved reviewables' do
+    create_reviewables(Jobs::ReviewablePriorities.min_reviewables)
+    low_score = 2
+    10.times { create_with_score(low_score, status: :pending) }
+
+    Jobs::ReviewablePriorities.new.execute({})
+
+    expect_min_score(:low, SiteSetting.reviewable_low_priority_threshold)
+    expect_min_score(:medium, 9.0)
+    expect_min_score(:high, 14.0)
+    expect(Reviewable.score_required_to_hide_post).to eq(9.33)
   end
 
   def expect_min_score(priority, score)
diff --git a/spec/models/reviewable_flagged_post_spec.rb b/spec/models/reviewable_flagged_post_spec.rb
index 0712468..339a756 100644
--- a/spec/models/reviewable_flagged_post_spec.rb
+++ b/spec/models/reviewable_flagged_post_spec.rb
@@ -305,6 +305,23 @@ RSpec.describe ReviewableFlaggedPost, type: :model do
     end
   end
 
+  describe 'recalculating the reviewable score' do
+    let(:expected_score) { 8 }
+    let(:reviewable) { Fabricate(:reviewable_flagged_post, score: expected_score) }
+
+    it "doesn't recalculate the score after ignore" do
+      reviewable.perform(moderator, :ignore)
+
+      expect(reviewable.score).to eq(expected_score)
+    end
+
+    it "doesn't recalculate the score after disagree" do
+      reviewable.perform(moderator, :disagree)
+
+      expect(reviewable.score).to eq(expected_score)
+    end
+  end
+
   def assert_pm_creation_enqueued(user_id, pm_type)
     expect(Jobs::SendSystemMessage.jobs.length).to eq(1)
       job = Jobs::SendSystemMessage.jobs[0]
diff --git a/spec/models/reviewable_score_spec.rb b/spec/models/reviewable_score_spec.rb
index 1352498..0da0443 100644
--- a/spec/models/reviewable_score_spec.rb
+++ b/spec/models/reviewable_score_spec.rb
@@ -35,7 +35,7 @@ RSpec.describe ReviewableScore, type: :model do
       expect(rs.reload).to be_disagreed
       expect(rs.reviewed_by).to eq(moderator)
       expect(rs.reviewed_at).to be_present
-      expect(reviewable.score).to eq(0.0)
+      expect(reviewable.score).to eq(4.0)
     end
 
     it "increases the score by the post action type's score bonus" do

GitHub sha: d4b5a81b

This commit appears in #13009 which was approved by eviltrout. It was merged by romanrizzi.