From 456e40a70967b0473ceb52ae1fbc20c11fed4e2f Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Tue, 24 Apr 2018 13:29:15 -0400 Subject: [PATCH] FIX: Don't allow a user to become TL3 if they've ever been penalized Previously the code would only check if they were *currently* suspended or silenced. --- .../admin/models/admin-user.js.es6 | 10 ++-- .../admin/models/tl3-requirements.js.es6 | 56 +++++++++++-------- .../routes/admin-user-tl3-requirements.js.es6 | 2 +- .../admin/templates/user-tl3-requirements.hbs | 12 ++++ app/models/trust_level3_requirements.rb | 47 ++++++++++++++++ app/serializers/penalty_counts_serializer.rb | 11 ++++ .../trust_level3_requirements_serializer.rb | 5 ++ config/locales/client.en.yml | 2 + spec/models/trust_level3_requirements_spec.rb | 45 +++++++++++++++ 9 files changed, 160 insertions(+), 30 deletions(-) create mode 100644 app/serializers/penalty_counts_serializer.rb diff --git a/app/assets/javascripts/admin/models/admin-user.js.es6 b/app/assets/javascripts/admin/models/admin-user.js.es6 index 99aec6aaf71..d9a5c6ca34e 100644 --- a/app/assets/javascripts/admin/models/admin-user.js.es6 +++ b/app/assets/javascripts/admin/models/admin-user.js.es6 @@ -5,7 +5,6 @@ import { propertyNotEqual } from 'discourse/lib/computed'; import { popupAjaxError } from 'discourse/lib/ajax-error'; import ApiKey from 'admin/models/api-key'; import Group from 'discourse/models/group'; -import TL3Requirements from 'admin/models/tl3-requirements'; import { userPath } from 'discourse/lib/url'; const wrapAdmin = user => user ? AdminUser.create(user) : null; @@ -472,11 +471,12 @@ const AdminUser = Discourse.User.extend({ }); }, - tl3Requirements: function() { - if (this.get('tl3_requirements')) { - return TL3Requirements.create(this.get('tl3_requirements')); + @computed('tl3_requirements') + tl3Requirements(requirements) { + if (requirements) { + return this.store.createRecord('tl3Requirements', requirements); } - }.property('tl3_requirements'), + }, @computed('suspended_by') suspendedBy: wrapAdmin, diff --git a/app/assets/javascripts/admin/models/tl3-requirements.js.es6 b/app/assets/javascripts/admin/models/tl3-requirements.js.es6 index ac27b095a7b..6dc655b1ced 100644 --- a/app/assets/javascripts/admin/models/tl3-requirements.js.es6 +++ b/app/assets/javascripts/admin/models/tl3-requirements.js.es6 @@ -1,11 +1,15 @@ -const TL3Requirements = Discourse.Model.extend({ - days_visited_percent: function() { - return Math.round((this.get('days_visited') * 100) / this.get('time_period')); - }.property('days_visited', 'time_period'), +import computed from 'ember-addons/ember-computed-decorators'; - min_days_visited_percent: function() { - return Math.round((this.get('min_days_visited') * 100) / this.get('time_period')); - }.property('min_days_visited', 'time_period'), +export default Discourse.Model.extend({ + @computed('days_visited', 'time_period') + days_visited_percent(daysVisited, timePeriod) { + return Math.round((daysVisited * 100) / timePeriod); + }, + + @computed('min_days_visited', 'time_period') + min_days_visited_percent(minDaysVisited, timePeriod) { + return Math.round((minDaysVisited * 100) / timePeriod); + }, met: function() { return { @@ -21,22 +25,26 @@ const TL3Requirements = Discourse.Model.extend({ likes_received: this.get('num_likes_received') >= this.get('min_likes_received'), likes_received_days: this.get('num_likes_received_days') >= this.get('min_likes_received_days'), likes_received_users: this.get('num_likes_received_users') >= this.get('min_likes_received_users'), - level_locked: this.get('trust_level_locked') + level_locked: this.get('trust_level_locked'), + silenced: this.get('penalty_counts.silenced') === 0, + suspended: this.get('penalty_counts.suspended') === 0 }; - }.property('days_visited', 'min_days_visited', - 'num_topics_replied_to', 'min_topics_replied_to', - 'topics_viewed', 'min_topics_viewed', - 'posts_read', 'min_posts_read', - 'num_flagged_posts', 'max_flagged_posts', - 'topics_viewed_all_time', 'min_topics_viewed_all_time', - 'posts_read_all_time', 'min_posts_read_all_time', - 'num_flagged_by_users', 'max_flagged_by_users', - 'num_likes_given', 'min_likes_given', - 'num_likes_received', 'min_likes_received', - 'num_likes_received', 'min_likes_received', - 'num_likes_received_days', 'min_likes_received_days', - 'num_likes_received_users', 'min_likes_received_users', - 'trust_level_locked') + }.property( + 'days_visited', 'min_days_visited', + 'num_topics_replied_to', 'min_topics_replied_to', + 'topics_viewed', 'min_topics_viewed', + 'posts_read', 'min_posts_read', + 'num_flagged_posts', 'max_flagged_posts', + 'topics_viewed_all_time', 'min_topics_viewed_all_time', + 'posts_read_all_time', 'min_posts_read_all_time', + 'num_flagged_by_users', 'max_flagged_by_users', + 'num_likes_given', 'min_likes_given', + 'num_likes_received', 'min_likes_received', + 'num_likes_received', 'min_likes_received', + 'num_likes_received_days', 'min_likes_received_days', + 'num_likes_received_users', 'min_likes_received_users', + 'trust_level_locked', + 'penalty_counts.silenced', + 'penalty_counts.suspended' + ) }); - -export default TL3Requirements; diff --git a/app/assets/javascripts/admin/routes/admin-user-tl3-requirements.js.es6 b/app/assets/javascripts/admin/routes/admin-user-tl3-requirements.js.es6 index 40c874eaaa1..cceaf9fd478 100644 --- a/app/assets/javascripts/admin/routes/admin-user-tl3-requirements.js.es6 +++ b/app/assets/javascripts/admin/routes/admin-user-tl3-requirements.js.es6 @@ -1,5 +1,5 @@ export default Discourse.Route.extend({ - model: function() { + model() { return this.modelFor('adminUser'); } }); diff --git a/app/assets/javascripts/admin/templates/user-tl3-requirements.hbs b/app/assets/javascripts/admin/templates/user-tl3-requirements.hbs index 1a848710a33..1ac192e1926 100644 --- a/app/assets/javascripts/admin/templates/user-tl3-requirements.hbs +++ b/app/assets/javascripts/admin/templates/user-tl3-requirements.hbs @@ -96,6 +96,18 @@ {{model.tl3Requirements.num_likes_received_users}} {{model.tl3Requirements.min_likes_received_users}} + + {{i18n 'admin.user.tl3_requirements.silenced'}} + {{check-icon model.tl3Requirements.met.silenced}} + {{model.tl3Requirements.penalty_counts.silenced}} + 0 + + + {{i18n 'admin.user.tl3_requirements.suspended'}} + {{check-icon model.tl3Requirements.met.suspended}} + {{model.tl3Requirements.penalty_counts.suspended}} + 0 + diff --git a/app/models/trust_level3_requirements.rb b/app/models/trust_level3_requirements.rb index 37785e32a37..ac63e5ae6aa 100644 --- a/app/models/trust_level3_requirements.rb +++ b/app/models/trust_level3_requirements.rb @@ -2,6 +2,19 @@ # the Leader (3) trust level. class TrustLevel3Requirements + class PenaltyCounts + attr_reader :silenced, :suspended + + def initialize(row) + @silenced = row['silence_count'] || 0 + @suspended = row['suspend_count'] || 0 + end + + def total + @silenced + @suspended + end + end + include ActiveModel::Serialization LOW_WATER_MARK = 0.9 @@ -29,6 +42,7 @@ class TrustLevel3Requirements (!@user.suspended?) && (!@user.silenced?) && + penalty_counts.total == 0 && days_visited >= min_days_visited && num_topics_replied_to >= min_topics_replied_to && topics_viewed >= min_topics_viewed && @@ -48,6 +62,7 @@ class TrustLevel3Requirements @user.suspended? || @user.silenced? || + penalty_counts.total > 0 || days_visited < min_days_visited * LOW_WATER_MARK || num_topics_replied_to < min_topics_replied_to * LOW_WATER_MARK || topics_viewed < min_topics_viewed * LOW_WATER_MARK || @@ -78,6 +93,38 @@ class TrustLevel3Requirements @user.user_visits.where("visited_at > ? and posts_read > 0", time_period.days.ago).count end + def penalty_counts + args = { + user_id: @user.id, + silence_user: UserHistory.actions[:silence_user], + unsilence_user: UserHistory.actions[:unsilence_user], + suspend_user: UserHistory.actions[:suspend_user], + unsuspend_user: UserHistory.actions[:unsuspend_user] + } + + sql = <<~SQL + SELECT SUM( + CASE + WHEN action = :silence_user THEN 1 + WHEN action = :unsilence_user THEN -1 + ELSE 0 + END + ) AS silence_count, + SUM( + CASE + WHEN action = :suspend_user THEN 1 + WHEN action = :unsuspend_user THEN -1 + ELSE 0 + END + ) AS suspend_count + FROM user_histories AS uh + WHERE uh.target_user_id = :user_id + AND uh.action IN (:silence_user, :unsilence_user, :suspend_user, :unsuspend_user) + SQL + + PenaltyCounts.new(UserHistory.exec_sql(sql, args).first) + end + def min_days_visited SiteSetting.tl3_requires_days_visited end diff --git a/app/serializers/penalty_counts_serializer.rb b/app/serializers/penalty_counts_serializer.rb new file mode 100644 index 00000000000..68d1641f14b --- /dev/null +++ b/app/serializers/penalty_counts_serializer.rb @@ -0,0 +1,11 @@ +class PenaltyCountsSerializer < ApplicationSerializer + attributes :silenced, :suspended + + def silenced + object.silenced + end + + def suspended + object.suspended + end +end diff --git a/app/serializers/trust_level3_requirements_serializer.rb b/app/serializers/trust_level3_requirements_serializer.rb index 7d489236a7d..371619ccc05 100644 --- a/app/serializers/trust_level3_requirements_serializer.rb +++ b/app/serializers/trust_level3_requirements_serializer.rb @@ -1,4 +1,9 @@ +require_dependency 'penalty_counts_serializer' + class TrustLevel3RequirementsSerializer < ApplicationSerializer + + has_one :penalty_counts, embed: :object, serializer: PenaltyCountsSerializer + attributes :time_period, :requirements_met, :requirements_lost, diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index fe27fd69118..6ef8662b25f 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -3667,6 +3667,8 @@ en: likes_received: "Likes Received" likes_received_days: "Likes Received: unique days" likes_received_users: "Likes Received: unique users" + suspended: "Suspended (all time)" + silenced: "Silenced (all time)" qualifies: "Qualifies for trust level 3." does_not_qualify: "Doesn't qualify for trust level 3." will_be_promoted: "Will be promoted soon." diff --git a/spec/models/trust_level3_requirements_spec.rb b/spec/models/trust_level3_requirements_spec.rb index b98641a3258..6a7546efa58 100644 --- a/spec/models/trust_level3_requirements_spec.rb +++ b/spec/models/trust_level3_requirements_spec.rb @@ -4,6 +4,7 @@ describe TrustLevel3Requirements do let(:user) { Fabricate.build(:user) } subject(:tl3_requirements) { described_class.new(user) } + let(:moderator) { Fabricate(:moderator) } before do described_class.clear_cache @@ -14,6 +15,38 @@ describe TrustLevel3Requirements do end describe "requirements" do + + describe "penalty_counts" do + + it "returns if the user has ever been silenced" do + expect(tl3_requirements.penalty_counts.silenced).to eq(0) + expect(tl3_requirements.penalty_counts.total).to eq(0) + UserSilencer.new(user, moderator).silence + expect(tl3_requirements.penalty_counts.silenced).to eq(1) + expect(tl3_requirements.penalty_counts.total).to eq(1) + UserSilencer.new(user, moderator).unsilence + expect(tl3_requirements.penalty_counts.silenced).to eq(0) + expect(tl3_requirements.penalty_counts.total).to eq(0) + end + + it "returns if the user has ever been suspended" do + user.save + + expect(tl3_requirements.penalty_counts.suspended).to eq(0) + expect(tl3_requirements.penalty_counts.total).to eq(0) + + UserHistory.create(target_user_id: user.id, action: UserHistory.actions[:suspend_user]) + + expect(tl3_requirements.penalty_counts.suspended).to eq(1) + expect(tl3_requirements.penalty_counts.total).to eq(1) + + UserHistory.create(target_user_id: user.id, action: UserHistory.actions[:unsuspend_user]) + + expect(tl3_requirements.penalty_counts.suspended).to eq(0) + expect(tl3_requirements.penalty_counts.total).to eq(0) + end + end + it "time_period uses site setting" do SiteSetting.tl3_time_period = 80 expect(tl3_requirements.time_period).to eq(80) @@ -366,6 +399,18 @@ describe TrustLevel3Requirements do expect(tl3_requirements.requirements_met?).to eq(false) end + it "are not met if previously silenced" do + user.save + UserHistory.create(target_user_id: user.id, action: UserHistory.actions[:silence_user]) + expect(tl3_requirements.requirements_met?).to eq(false) + end + + it "are not met if previously suspended" do + user.save + UserHistory.create(target_user_id: user.id, action: UserHistory.actions[:suspend_user]) + expect(tl3_requirements.requirements_met?).to eq(false) + end + it "are lost if not enough likes received on different days" do tl3_requirements.stubs(:num_likes_received_days).returns(4) expect(tl3_requirements.requirements_lost?).to eq(true)