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.

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

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
 

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
 

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)
+  def create_reviewables(count, status: :approved)
-    (1..count).each { |i| create_with_score(i) }
+    minimum_threshold = SiteSetting.reviewable_low_priority_threshold
+    (1..count).each { |i| create_with_score(minimum_threshold + i) }
   end
 
-  def create_with_score(score)
+  def create_with_score(score, status: :approved)
-    Fabricate(:reviewable_flagged_post).tap do |reviewable|
+    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, 9.0)
-    expect_min_score('medium', 8.0)
+    expect_min_score(:high, 14.0)
-    expect_min_score(:high, 13.0)
+    expect(Reviewable.score_required_to_hide_post).to eq(9.33)
-    expect(Reviewable.score_required_to_hide_post).to eq(8.66)
   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(:medium, 9.0)
-    expect_min_score(:high, 13.0)
+    expect_min_score(:high, 14.0)
-    expect(Reviewable.score_required_to_hide_post).to eq(8.66)
+    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)

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]

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