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