From 60059a7190ff70924d70213bf8970aaf89faeffc Mon Sep 17 00:00:00 2001 From: Roman Rizzi Date: Fri, 23 Apr 2021 15:34:24 -0300 Subject: [PATCH] FEATURE: A low priority filter for the review queue. (#12822) This filter hides reviewables with a score lower than the "reviewable_low_priority_threshold" setting. We only use reviewables that already met this threshold to calculate the Medium and High priority filters. --- .../discourse/app/controllers/review-index.js | 2 +- app/jobs/scheduled/reviewable_priorities.rb | 11 +++- app/models/reviewable.rb | 4 +- .../initializers/014-track-setting-changes.rb | 5 ++ config/locales/client.en.yml | 3 +- config/locales/server.en.yml | 1 + config/site_settings.yml | 4 ++ .../track_setting_changes_spec.rb | 23 ++++++++ spec/jobs/reviewable_priorities_spec.rb | 57 +++++++++++++------ 9 files changed, 87 insertions(+), 23 deletions(-) diff --git a/app/assets/javascripts/discourse/app/controllers/review-index.js b/app/assets/javascripts/discourse/app/controllers/review-index.js index dc28bf6558e..73f2c655429 100644 --- a/app/assets/javascripts/discourse/app/controllers/review-index.js +++ b/app/assets/javascripts/discourse/app/controllers/review-index.js @@ -49,7 +49,7 @@ export default Controller.extend({ @discourseComputed priorities() { - return ["low", "medium", "high"].map((priority) => { + return ["any", "low", "medium", "high"].map((priority) => { return { id: priority, name: I18n.t(`review.filters.priority.${priority}`), diff --git a/app/jobs/scheduled/reviewable_priorities.rb b/app/jobs/scheduled/reviewable_priorities.rb index 7e0f5bbd7e9..cb74ead9cf6 100644 --- a/app/jobs/scheduled/reviewable_priorities.rb +++ b/app/jobs/scheduled/reviewable_priorities.rb @@ -16,13 +16,16 @@ class Jobs::ReviewablePriorities < ::Jobs::Scheduled def execute(args) return unless Reviewable.where('score > 0').count >= self.class.min_reviewables - res = DB.query_single(<<~SQL, target_count: self.class.target_count) + min_priority_threshold = SiteSetting.reviewable_low_priority_threshold + + 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, COALESCE(PERCENTILE_DISC(0.85) WITHIN GROUP (ORDER BY score), 0.0) AS high FROM ( SELECT r.score FROM reviewables AS r INNER JOIN reviewable_scores AS rs ON rs.reviewable_id = r.id + WHERE r.score >= :min_priority GROUP BY r.id HAVING COUNT(*) >= :target_count ) AS x @@ -32,6 +35,10 @@ class Jobs::ReviewablePriorities < ::Jobs::Scheduled medium, high = res - Reviewable.set_priorities(medium: medium, high: high) + Reviewable.set_priorities( + low: min_priority_threshold, + medium: medium, + high: high + ) end end diff --git a/app/models/reviewable.rb b/app/models/reviewable.rb index cb964477f93..9b729bdf8d4 100644 --- a/app/models/reviewable.rb +++ b/app/models/reviewable.rb @@ -444,8 +444,6 @@ class Reviewable < ActiveRecord::Base to_date: nil, additional_filters: {} ) - min_score = Reviewable.min_score_for_priority(priority) - order = case sort_order when 'score_asc' 'reviewables.score ASC, reviewables.created_at DESC' @@ -488,6 +486,8 @@ class Reviewable < ActiveRecord::Base ) end + min_score = Reviewable.min_score_for_priority(priority) + if min_score > 0 && status == :pending result = result.where("reviewables.score >= ? OR reviewables.force_review", min_score) elsif min_score > 0 diff --git a/config/initializers/014-track-setting-changes.rb b/config/initializers/014-track-setting-changes.rb index fed99a618c8..1340d1defe9 100644 --- a/config/initializers/014-track-setting-changes.rb +++ b/config/initializers/014-track-setting-changes.rb @@ -48,4 +48,9 @@ DiscourseEvent.on(:site_setting_changed) do |name, old_value, new_value| if SiteSetting::WATCHED_SETTINGS.include?(name) SiteSetting.reset_cached_settings! end + + # Make sure medium and high priority thresholds were calculated. + if name == :reviewable_low_priority_threshold && Reviewable.min_score_for_priority(:medium) > 0 + Reviewable.set_priorities(low: new_value) + end end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 0cf26e86fb5..40213e18423 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -516,7 +516,8 @@ en: priority: title: "Minimum Priority" - low: "(any)" + any: "(any)" + low: "Low" medium: "Medium" high: "High" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index a05451336ba..66589a878ea 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1956,6 +1956,7 @@ en: reviewable_claiming: "Does reviewable content need to be claimed before it can be acted upon?" reviewable_default_topics: "Show reviewable content grouped by topic by default" reviewable_default_visibility: "Don't show reviewable items unless they meet this priority" + reviewable_low_priority_threshold: "The priority filter hides reviewable that doesn't meet this score unless the '(any)' filter is used." high_trust_flaggers_auto_hide_posts: "New user posts are automatically hidden after being flagged as spam by a TL3+ user" cooldown_hours_until_reflag: "How much time users will have to wait until they are able to reflag a post" diff --git a/config/site_settings.yml b/config/site_settings.yml index 5464916ada8..beacd3a5022 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1710,6 +1710,10 @@ spam: type: enum default: low enum: "ReviewablePrioritySetting" + reviewable_low_priority_threshold: + default: 1 + min: 1 + rate_limits: unique_posts_mins: 5 diff --git a/spec/initializers/track_setting_changes_spec.rb b/spec/initializers/track_setting_changes_spec.rb index 82b7963ec4d..b6e86c161a0 100644 --- a/spec/initializers/track_setting_changes_spec.rb +++ b/spec/initializers/track_setting_changes_spec.rb @@ -22,4 +22,27 @@ describe 'Setting changes' do expect(non_approved_user.reload.approved?).to eq(true) end end + + describe '#reviewable_low_priority_threshold' do + let(:new_threshold) { 5 } + + it 'sets the low priority value' do + medium_threshold = 10 + Reviewable.set_priorities(medium: medium_threshold) + + expect(Reviewable.min_score_for_priority(:low)).not_to eq(new_threshold) + + SiteSetting.reviewable_low_priority_threshold = new_threshold + + expect(Reviewable.min_score_for_priority(:low)).to eq(new_threshold) + end + + it "does nothing if the other thresholds were not calculated" do + Reviewable.set_priorities(medium: 0.0) + + SiteSetting.reviewable_low_priority_threshold = new_threshold + + expect(Reviewable.min_score_for_priority(:low)).not_to eq(new_threshold) + end + end end diff --git a/spec/jobs/reviewable_priorities_spec.rb b/spec/jobs/reviewable_priorities_spec.rb index 22276374fde..9a5c96d7332 100644 --- a/spec/jobs/reviewable_priorities_spec.rb +++ b/spec/jobs/reviewable_priorities_spec.rb @@ -6,42 +6,65 @@ describe Jobs::ReviewablePriorities do it "needs returns 0s with no existing reviewables" do Jobs::ReviewablePriorities.new.execute({}) - expect(Reviewable.min_score_for_priority(:low)).to eq(0.0) - expect(Reviewable.min_score_for_priority(:medium)).to eq(0.0) - expect(Reviewable.min_score_for_priority(:high)).to eq(0.0) + + expect_min_score(:low, 0.0) + expect_min_score(:medium, 0.0) + expect_min_score(:high, 0.0) expect(Reviewable.score_required_to_hide_post).to eq(8.33) end - fab!(:u0) { Fabricate(:user) } - fab!(:u1) { Fabricate(:user) } + fab!(:user_0) { Fabricate(:user) } + fab!(:user_1) { Fabricate(:user) } def create_reviewables(count) - (1..count).each do |i| - r = Fabricate(:reviewable_flagged_post) - r.add_score(u0, PostActionType.types[:off_topic]) - r.add_score(u1, PostActionType.types[:off_topic]) - r.update!(score: i) + (1..count).each { |i| create_with_score(i) } + end + + def create_with_score(score) + Fabricate(:reviewable_flagged_post).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) end end it "needs a minimum amount of reviewables before it calculates anything" do create_reviewables(5) + Jobs::ReviewablePriorities.new.execute({}) - expect(Reviewable.min_score_for_priority(:low)).to eq(0.0) - expect(Reviewable.min_score_for_priority(:medium)).to eq(0.0) - expect(Reviewable.min_score_for_priority(:high)).to eq(0.0) + + expect_min_score(:low, 0.0) + expect_min_score(:medium, 0.0) + expect_min_score(:high, 0.0) expect(Reviewable.score_required_to_hide_post).to eq(8.33) end it "will set priorities based on the maximum score" do create_reviewables(Jobs::ReviewablePriorities.min_reviewables) + Jobs::ReviewablePriorities.new.execute({}) - expect(Reviewable.min_score_for_priority(:low)).to eq(0.0) - expect(Reviewable.min_score_for_priority(:medium)).to eq(8.0) - expect(Reviewable.min_score_for_priority('medium')).to eq(8.0) - expect(Reviewable.min_score_for_priority(:high)).to eq(13.0) + 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) end + it 'ignore negative scores when calculating priorities' do + create_reviewables(Jobs::ReviewablePriorities.min_reviewables) + negative_score = -9 + 10.times { create_with_score(negative_score) } + + 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) + end + + def expect_min_score(priority, score) + expect(Reviewable.min_score_for_priority(priority)).to eq(score) + end end