BUGFIX: trust_level_0 group not including trust_level_1

BUGFIX: manual trust level change not adding user to groups
BUGFIX: system not in correct trust level groups

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)
+    level = TrustLevel.levels[params[:level].to_i]
-    BoostTrustLevel.new(user: @user, level: params[:level], logger: logger).save!
+    @user.change_trust_level!(level, log_action_for: current_user)
+
     render_serialized(@user, AdminUserSerializer)
   end
 

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,19 +187,29 @@ 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)
+    desired.each do |id|
-      group.group_users.build(user_id: user_id)
+      if group = find_by(id: id)
-      group.save!
+        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
 
   def self.builtin
     Enum.new(:moderators, :admins, :trust_level_1, :trust_level_2)

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

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])

lib/boost_trust_level.rb

@@ -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

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
+    Promotion.basic_met?(@user) && change_trust_level!(:basic)
-    return false if stat.topics_entered < SiteSetting.basic_requires_topics_entered
+  end
-    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
 
-    @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

spec/components/boost_trust_level_spec.rb

@@ -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

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_1].user_ids.should include user.id
-    Group[:trust_level_2].user_ids.should == [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
+    # system and user
-    g.user_count.should == 1
+    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

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

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