From d4b5a81b05609c32e003228df5c9dd2cea80a595 Mon Sep 17 00:00:00 2001 From: Roman Rizzi Date: Mon, 10 May 2021 14:09:04 -0300 Subject: [PATCH] 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 | 10 ++++-- app/models/reviewable.rb | 12 +++++-- app/models/reviewable_flagged_post.rb | 2 -- spec/jobs/reviewable_priorities_spec.rb | 35 ++++++++++++++------- spec/models/reviewable_flagged_post_spec.rb | 17 ++++++++++ spec/models/reviewable_score_spec.rb | 2 +- 6 files changed, 58 insertions(+), 20 deletions(-) diff --git a/app/jobs/scheduled/reviewable_priorities.rb b/app/jobs/scheduled/reviewable_priorities.rb index cb74ead9cf6..da4d9c8148c 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 0d6ecb6d662..0e8af0bcb02 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 d481ec55adc..dada954b57a 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 9a5c96d7332..65f232fee1a 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 07124680d87..339a7569a3d 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 13524980fb4..0da0443ad20 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