diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 2103515b5..f4ab08ed9 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -1,6 +1,5 @@ require_dependency 'user_destroyer' require_dependency 'admin_user_index_query' -require_dependency 'boost_trust_level' class Admin::UsersController < Admin::AdminController @@ -111,8 +110,9 @@ class Admin::UsersController < Admin::AdminController def trust_level guardian.ensure_can_change_trust_level!(@user) - logger = StaffActionLogger.new(current_user) - BoostTrustLevel.new(user: @user, level: params[:level], logger: logger).save! + level = TrustLevel.levels[params[:level].to_i] + @user.change_trust_level!(level, log_action_for: current_user) + render_serialized(@user, AdminUserSerializer) end diff --git a/app/models/group.rb b/app/models/group.rb index b4365ccf6..29864b0af 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -17,6 +17,7 @@ class Group < ActiveRecord::Base :admins => 1, :moderators => 2, :staff => 3, + :registered_users => 10, :trust_level_1 => 11, :trust_level_2 => 12, :trust_level_3 => 13, @@ -24,6 +25,8 @@ class Group < ActiveRecord::Base :trust_level_5 => 15 } + AUTO_GROUP_IDS = Hash[*AUTO_GROUPS.to_a.reverse] + ALIAS_LEVELS = { :nobody => 0, :only_admins => 1, @@ -88,7 +91,9 @@ class Group < ActiveRecord::Base when :staff "SELECT u.id FROM users u WHERE u.moderator OR u.admin" when :trust_level_1, :trust_level_2, :trust_level_3, :trust_level_4, :trust_level_5 - "SELECT u.id FROM users u WHERE u.trust_level = #{id-10}" + "SELECT u.id FROM users u WHERE u.trust_level >= #{id-10}" + when :registered_users + "SELECT u.id FROM users u" end @@ -182,17 +187,27 @@ class Group < ActiveRecord::Base group_ids end + def self.desired_trust_level_groups(trust_level) + trust_group_ids.keep_if do |id| + id == AUTO_GROUPS[:registered_users] || (trust_level + 10) >= id + end + end def self.user_trust_level_change!(user_id, trust_level) - name = "trust_level_#{trust_level}".to_sym + desired = desired_trust_level_groups(trust_level) + undesired = trust_group_ids - desired - GroupUser.where(group_id: trust_group_ids, user_id: user_id).delete_all + GroupUser.where(group_id: undesired, user_id: user_id).delete_all - if group = lookup_group(name) - group.group_users.build(user_id: user_id) - group.save! - else - refresh_automatic_group!(name) + desired.each do |id| + if group = find_by(id: id) + unless GroupUser.where(group_id: id, user_id: user_id).exists? + group.group_users.create!(user_id: user_id) + end + else + name = AUTO_GROUP_IDS[trust_level] + refresh_automatic_group!(name) + end end end diff --git a/app/models/user.rb b/app/models/user.rb index 93d4b4ba9..bf2b4b10a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -9,6 +9,7 @@ require_dependency 'user_name_suggester' require_dependency 'pretty_text' require_dependency 'url_helper' require_dependency 'letter_avatar' +require_dependency 'promotion' class User < ActiveRecord::Base include Roleable @@ -75,6 +76,7 @@ class User < ActiveRecord::Base after_create :create_email_token after_create :create_user_stat after_create :create_user_profile + after_create :ensure_in_trust_level_group after_save :refresh_avatar before_destroy do @@ -440,18 +442,6 @@ class User < ActiveRecord::Base admin end - def change_trust_level!(level) - raise "Invalid trust level #{level}" unless TrustLevel.valid_level?(level) - self.trust_level = TrustLevel.levels[level] - transaction do - self.save! - self.user_profile.recook_bio - self.user_profile.save! - Group.user_trust_level_change!(self.id, self.trust_level) - BadgeGranter.update_badges(self, trust_level: trust_level) - end - end - def guardian Guardian.new(self) end @@ -479,6 +469,10 @@ class User < ActiveRecord::Base save end + def change_trust_level!(level, opts=nil) + Promotion.new(self).change_trust_level!(level, opts) + end + def treat_as_new_topic_start_date duration = new_topic_duration_minutes || SiteSetting.new_topic_duration_minutes [case duration @@ -641,6 +635,10 @@ class User < ActiveRecord::Base UserProfile.create(user_id: id) end + def ensure_in_trust_level_group + Group.user_trust_level_change!(id, trust_level) + end + def create_user_stat stat = UserStat.new(new_since: Time.now) stat.user_id = id diff --git a/db/fixtures/005_users.rb b/db/fixtures/005_users.rb index 652245cdb..10da01df4 100644 --- a/db/fixtures/005_users.rb +++ b/db/fixtures/005_users.rb @@ -21,3 +21,5 @@ User.seed do |u| u.email_private_messages = false u.trust_level = TrustLevel.levels[:elder] end + +Group.user_trust_level_change!(-1 ,TrustLevel.levels[:elder]) diff --git a/lib/boost_trust_level.rb b/lib/boost_trust_level.rb deleted file mode 100644 index 87ea55f13..000000000 --- a/lib/boost_trust_level.rb +++ /dev/null @@ -1,45 +0,0 @@ -require_dependency 'promotion' - -class BoostTrustLevel - - def initialize(args) - @user = args[:user] - @level = args[:level].to_i - @promotion = Promotion.new(@user) - @trust_levels = TrustLevel.levels - @logger = args[:logger] - end - - def save! - previous_level = @user.trust_level - success = if @level < @user.trust_level - demote! - else - @user.update_attributes!(trust_level: @level) - end - @logger.log_trust_level_change(@user, previous_level, @level) - BadgeGranter.update_badges(@user, trust_level: @level) - success - end - - protected - - def demote! - current_trust_level = @user.trust_level - @user.update_attributes!(trust_level: @level) - if @promotion.review - @user.update_attributes!(trust_level: current_trust_level) - raise Discourse::InvalidAccess.new, I18n.t('trust_levels.change_failed_explanation', - user_name: @user.name, - new_trust_level: trust_level_lookup(@level), - current_trust_level: trust_level_lookup(current_trust_level)) - else - true - end - end - - def trust_level_lookup(level) - @trust_levels.key(level).id2name - end - -end diff --git a/lib/promotion.rb b/lib/promotion.rb index 656f1121b..f4cf69b1e 100644 --- a/lib/promotion.rb +++ b/lib/promotion.rb @@ -7,6 +7,7 @@ class Promotion @user = user end + # Review a user for a promotion. Delegates work to a review_#{trust_level} method. # Returns true if the user was promoted, false otherwise. def review @@ -22,18 +23,52 @@ class Promotion end def review_newuser - stat = @user.user_stat - return false if stat.topics_entered < SiteSetting.basic_requires_topics_entered - return false if stat.posts_read_count < SiteSetting.basic_requires_read_posts - return false if (stat.time_read / 60) < SiteSetting.basic_requires_time_spent_mins + Promotion.basic_met?(@user) && change_trust_level!(:basic) + end - @user.change_trust_level!(:basic) + def review_basic + Promotion.regular_met?(@user) && change_trust_level!(:regular) + end + + def change_trust_level!(level, opts = {}) + raise "Invalid trust level #{level}" unless TrustLevel.valid_level?(level) + + old_level = @user.trust_level + new_level = TrustLevel.levels[level] + + if new_level < old_level + next_up = TrustLevel.levels[new_level+1] + key = "#{next_up}_met?" + if self.class.respond_to?(key) && self.class.send(key, @user) + raise Discourse::InvalidAccess.new, I18n.t('trust_levels.change_failed_explanation', + user_name: @user.name, + new_trust_level: new_level, + current_trust_level: old_level) + end + end + + admin = opts && opts[:log_action_for] + + @user.trust_level = new_level + @user.bio_raw_will_change! # So it can get re-cooked based on the new trust level + + @user.transaction do + if admin + StaffActionLogger.new(admin).log_trust_level_change(@user, old_level, new_level) + end + @user.save! + @user.user_profile.recook_bio + @user.user_profile.save! + Group.user_trust_level_change!(@user.id, @user.trust_level) + BadgeGranter.update_badges(@user, trust_level: @user.trust_level) + end true end - def review_basic - stat = @user.user_stat + + def self.regular_met?(user) + stat = user.user_stat return false if stat.topics_entered < SiteSetting.regular_requires_topics_entered return false if stat.posts_read_count < SiteSetting.regular_requires_read_posts return false if (stat.time_read / 60) < SiteSetting.regular_requires_time_spent_mins @@ -42,7 +77,15 @@ class Promotion return false if stat.likes_given < SiteSetting.regular_requires_likes_given return false if stat.topic_reply_count < SiteSetting.regular_requires_topic_reply_count - @user.change_trust_level!(:regular) + true + end + + def self.basic_met?(user) + stat = user.user_stat + return false if stat.topics_entered < SiteSetting.basic_requires_topics_entered + return false if stat.posts_read_count < SiteSetting.basic_requires_read_posts + return false if (stat.time_read / 60) < SiteSetting.basic_requires_time_spent_mins + return true end end diff --git a/spec/components/boost_trust_level_spec.rb b/spec/components/boost_trust_level_spec.rb deleted file mode 100644 index 83884e22f..000000000 --- a/spec/components/boost_trust_level_spec.rb +++ /dev/null @@ -1,59 +0,0 @@ -require 'spec_helper' -require 'boost_trust_level' - -describe BoostTrustLevel do - - let(:user) { Fabricate(:user, trust_level: TrustLevel.levels[:newuser]) } - let(:logger) { StaffActionLogger.new(Fabricate(:admin)) } - - - it "should upgrade the trust level of a user" do - boostr = BoostTrustLevel.new(user: user, level: TrustLevel.levels[:basic], logger: logger) - boostr.save!.should be_true - user.trust_level.should == TrustLevel.levels[:basic] - end - - it "should log the action" do - StaffActionLogger.any_instance.expects(:log_trust_level_change).with(user, TrustLevel.levels[:newuser], TrustLevel.levels[:basic]).once - boostr = BoostTrustLevel.new(user: user, level: TrustLevel.levels[:basic], logger: logger) - boostr.save! - end - - describe "demotions" do - - context "for a user that has not done the requisite things to attain their trust level" do - - before do - # scenario: admin mistakenly promotes user's trust level - user.update_attributes(trust_level: TrustLevel.levels[:basic]) - end - - it "should demote the user and log the action" do - StaffActionLogger.any_instance.expects(:log_trust_level_change).with(user, TrustLevel.levels[:basic], TrustLevel.levels[:newuser]).once - boostr = BoostTrustLevel.new(user: user, level: TrustLevel.levels[:newuser], logger: logger) - boostr.save!.should be_true - user.trust_level.should == TrustLevel.levels[:newuser] - end - end - - context "for a user that has done the requisite things to attain their trust level" do - - before do - stat = user.user_stat - stat.topics_entered = SiteSetting.basic_requires_topics_entered + 1 - stat.posts_read_count = SiteSetting.basic_requires_read_posts + 1 - stat.time_read = SiteSetting.basic_requires_time_spent_mins * 60 - user.save! - user.update_attributes(trust_level: TrustLevel.levels[:basic]) - end - - it "should not demote the user and not log the action" do - StaffActionLogger.any_instance.expects(:log_trust_level_change).never - boostr = BoostTrustLevel.new(user: user, level: TrustLevel.levels[:newuser], logger: logger) - expect { boostr.save! }.to raise_error(Discourse::InvalidAccess, "You attempted to demote #{user.name} to 'newuser'. However their trust level is already 'basic'. #{user.name} will remain at 'basic'") - user.trust_level.should == TrustLevel.levels[:basic] - end - - end - end -end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index ca067f1c1..0a79fa9d6 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -2,6 +2,14 @@ require 'spec_helper' describe Group do + # UGLY but perf is horrible with this callback + before do + User.set_callback(:create, :after, :ensure_in_trust_level_group) + end + after do + User.skip_callback(:create, :after, :ensure_in_trust_level_group) + end + describe "validation" do let(:group) { build(:group) } @@ -71,19 +79,21 @@ describe Group do it "Correctly updates automatic trust level groups" do user = Fabricate(:user) + Group[:registered_users].user_ids.should include user.id + user.change_trust_level!(:basic) - Group[:trust_level_1].user_ids.should == [user.id] + Group[:trust_level_1].user_ids.should include user.id user.change_trust_level!(:regular) - Group[:trust_level_1].user_ids.should == [] - Group[:trust_level_2].user_ids.should == [user.id] + Group[:trust_level_1].user_ids.should include user.id + Group[:trust_level_2].user_ids.should include user.id user2 = Fabricate(:coding_horror) user2.change_trust_level!(:regular) - Group[:trust_level_2].user_ids.sort.should == [user.id, user2.id].sort + Group[:trust_level_2].user_ids.sort.should == [-1, user.id, user2.id].sort end it "Correctly updates all automatic groups upon request" do @@ -106,9 +116,16 @@ describe Group do g.users.count.should == 2 g.user_count.should == 2 + + g = groups.find{|g| g.id == Group::AUTO_GROUPS[:trust_level_1]} + # admin, system and user + g.users.count.should == 3 + g.user_count.should == 3 + g = groups.find{|g| g.id == Group::AUTO_GROUPS[:trust_level_2]} - g.users.count.should == 1 - g.user_count.should == 1 + # system and user + g.users.count.should == 2 + g.user_count.should == 2 end @@ -136,7 +153,6 @@ describe Group do it "correctly destroys groups" do - original_count = GroupUser.count g = Fabricate(:group) u1 = Fabricate(:user) g.add(u1) @@ -145,7 +161,7 @@ describe Group do g.destroy User.where(id: u1.id).count.should == 1 - GroupUser.count.should == original_count + GroupUser.where(group_id: g.id).count.should == 0 end @@ -167,4 +183,20 @@ describe Group do group.id.should == Group[group.name.to_sym].id end + it "can find desired groups correctly" do + Group.desired_trust_level_groups(2).sort.should == [10,11,12] + end + + + it "correctly handles trust level changes" do + user = Fabricate(:user, trust_level: 2) + Group.user_trust_level_change!(user.id, 2) + + user.groups.map(&:name).sort.should == ["registered_users","trust_level_1", "trust_level_2"] + + Group.user_trust_level_change!(user.id, 0) + user.reload + user.groups.map(&:name).sort.should == ["registered_users"] + end + end diff --git a/spec/services/badge_granter_spec.rb b/spec/services/badge_granter_spec.rb index 68b94bd38..8c8efaf1b 100644 --- a/spec/services/badge_granter_spec.rb +++ b/spec/services/badge_granter_spec.rb @@ -1,5 +1,4 @@ require 'spec_helper' -require_dependency 'boost_trust_level' describe BadgeGranter do @@ -73,12 +72,11 @@ describe BadgeGranter do context "update_badges" do let(:user) { Fabricate(:user) } - let(:logger) { StaffActionLogger.new(Fabricate(:admin)) } it "grants and revokes trust level badges" do user.change_trust_level!(:elder) UserBadge.where(user_id: user.id, badge_id: Badge.trust_level_badge_ids).count.should eq(4) - BoostTrustLevel.new(user: user, level: 1, logger: logger).save! + user.change_trust_level!(:basic) UserBadge.where(user_id: user.id, badge_id: 1).first.should_not be_nil UserBadge.where(user_id: user.id, badge_id: 2).first.should be_nil end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 51861518c..682b37768 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -57,6 +57,9 @@ Spork.prefork do config.before(:suite) do + # Ugly, but needed until we have a user creator + User.skip_callback(:create, :after, :ensure_in_trust_level_group) + DiscoursePluginRegistry.clear if ENV['LOAD_PLUGINS'] != "1" Discourse.current_user_provider = TestCurrentUserProvider