FIX: Restore trust level when leaving group (#17954)

If a user was granted a trust level, joined a group that granted a trust
level and left the group, the trust level was reset. This commit tries
to restore the last known trust level before joining the group by
looking into staff logs.

This commit also migrates old :change_trust_level user history records
to use previous_value and new_value fields.

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

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
 

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

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

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

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

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