diff --git a/app/models/group_user.rb b/app/models/group_user.rb index 5ee988eaa70..5e6b3f7dcba 100644 --- a/app/models/group_user.rb +++ b/app/models/group_user.rb @@ -120,7 +120,7 @@ class GroupUser < ActiveRecord::Base return if group.grant_trust_level.nil? return if self.destroyed_by_association&.active_record == User # User is being destroyed, so don't try to recalculate - Promotion.recalculate(user) + Promotion.recalculate(user, use_previous_trust_level: true) end def set_category_notifications diff --git a/app/services/staff_action_logger.rb b/app/services/staff_action_logger.rb index f5c3386f15e..a4203452bb7 100644 --- a/app/services/staff_action_logger.rb +++ b/app/services/staff_action_logger.rb @@ -100,7 +100,8 @@ class StaffActionLogger UserHistory.create!(params(opts).merge( action: UserHistory.actions[:change_trust_level], target_user_id: user.id, - details: "old trust level: #{old_trust_level}\nnew trust level: #{new_trust_level}" + previous_value: old_trust_level, + new_value: new_trust_level, )) end diff --git a/db/migrate/20220818171849_move_tl_user_history_to_previous_and_new_value.rb b/db/migrate/20220818171849_move_tl_user_history_to_previous_and_new_value.rb new file mode 100644 index 00000000000..6bd3022c8c5 --- /dev/null +++ b/db/migrate/20220818171849_move_tl_user_history_to_previous_and_new_value.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +class MoveTlUserHistoryToPreviousAndNewValue < ActiveRecord::Migration[7.0] + def change + execute <<~SQL + UPDATE user_histories + SET previous_value = old_tl, + new_value = new_tl, + details = NULL + FROM ( + SELECT id user_history_id, + (REGEXP_MATCHES(details, 'old trust level: (\d+)', 'i'))[1] old_tl, + (REGEXP_MATCHES(details, 'new trust level: (\d+)', 'i'))[1] new_tl + FROM user_histories + WHERE action = 2 + ) trust_levels + WHERE user_histories.id = trust_levels.user_history_id + SQL + end +end diff --git a/lib/promotion.rb b/lib/promotion.rb index 5ac5ee519da..da0f89916fa 100644 --- a/lib/promotion.rb +++ b/lib/promotion.rb @@ -121,21 +121,12 @@ class Promotion end # Figure out what a user's trust level should be from scratch - def self.recalculate(user, performed_by = nil) - # First, use the manual locked level - unless user.manual_locked_trust_level.nil? - return user.update!( - trust_level: user.manual_locked_trust_level - ) - end - - # Then consider the group locked level - user_group_granted_trust_level = user.group_granted_trust_level - - if user_group_granted_trust_level.present? - return user.update( - trust_level: user_group_granted_trust_level - ) + def self.recalculate(user, performed_by = nil, use_previous_trust_level: false) + granted_trust_level = TrustLevel.calculate( + user, use_previous_trust_level: use_previous_trust_level + ) + if granted_trust_level.present? + return user.update(trust_level: granted_trust_level) end user.update_column(:trust_level, TrustLevel[0]) diff --git a/lib/trust_level.rb b/lib/trust_level.rb index 8c87c9ac1e3..b776056b7aa 100644 --- a/lib/trust_level.rb +++ b/lib/trust_level.rb @@ -3,33 +3,51 @@ class InvalidTrustLevel < StandardError; end class TrustLevel + def self.[](level) + raise InvalidTrustLevel if !valid?(level) - class << self - - def [](level) - raise InvalidTrustLevel if !valid?(level) - level - end - - def levels - @levels ||= Enum.new(:newuser, :basic, :member, :regular, :leader, start: 0) - end - - def valid?(level) - valid_range === level - end - - def valid_range - (0..4) - end - - def compare(current_level, level) - (current_level || 0) >= level - end - - def name(level) - I18n.t("js.trust_levels.names.#{levels[level]}") - end + level end + def self.levels + @levels ||= Enum.new(:newuser, :basic, :member, :regular, :leader, start: 0) + end + + def self.valid?(level) + valid_range === level + end + + def self.valid_range + (0..4) + end + + def self.compare(current_level, level) + (current_level || 0) >= level + end + + def self.name(level) + I18n.t("js.trust_levels.names.#{levels[level]}") + end + + def self.calculate(user, use_previous_trust_level: false) + # First, use the manual locked level + return user.manual_locked_trust_level if user.manual_locked_trust_level.present? + + # Then consider the group locked level (or the previous trust level) + granted_trust_level = user.group_granted_trust_level || 0 + previous_trust_level = use_previous_trust_level ? find_previous_trust_level(user) : 0 + + [granted_trust_level, previous_trust_level].max + end + + private + + def self.find_previous_trust_level(user) + UserHistory + .where(action: UserHistory.actions[:change_trust_level]) + .where(target_user_id: user.id) + .order(created_at: :desc) + .pluck_first(:new_value) + .to_i + end end diff --git a/spec/models/group_user_spec.rb b/spec/models/group_user_spec.rb index 16b9404b1e7..5fe790dc5fb 100644 --- a/spec/models/group_user_spec.rb +++ b/spec/models/group_user_spec.rb @@ -237,5 +237,20 @@ RSpec.describe GroupUser do expect(user.primary_group_id).to be_nil expect(user.flair_group_id).to be_nil end + + it "restores previous trust level" do + user = Fabricate(:user) + expect(user.trust_level).to eq(1) + + user.change_trust_level!(2, log_action_for: Discourse.system_user) + user.change_trust_level!(3, log_action_for: Discourse.system_user) + group.update!(grant_trust_level: 4) + + group_user = Fabricate(:group_user, group: group, user: user) + expect(user.reload.trust_level).to eq(4) + + group_user.destroy! + expect(user.reload.trust_level).to eq(3) + end end end diff --git a/spec/services/staff_action_logger_spec.rb b/spec/services/staff_action_logger_spec.rb index a5bfaef522e..b448bb0230f 100644 --- a/spec/services/staff_action_logger_spec.rb +++ b/spec/services/staff_action_logger_spec.rb @@ -132,7 +132,8 @@ RSpec.describe StaffActionLogger do it 'creates a new UserHistory record' do expect { log_trust_level_change }.to change { UserHistory.count }.by(1) - expect(UserHistory.last.details).to include "new trust level: #{new_trust_level}" + expect(UserHistory.last.previous_value).to eq(old_trust_level.to_s) + expect(UserHistory.last.new_value).to eq(new_trust_level.to_s) end end