From 71be74ffd32b39912d63c790a6547c11e96676f9 Mon Sep 17 00:00:00 2001 From: Sam Date: Wed, 1 Mar 2023 15:35:21 +1100 Subject: [PATCH] FIX: recalculating trust levels was not working (#20492) The recalculate code was never firing cause TrustLevel.calculate unconditionally returned a trust level. (albeit a wrong one) New code ensures we only bypass promotion checks for cases where trust level is locked. see: https://meta.discourse.org/t/user-trust-level-resets-to-zero-when-unlocked/255444 --- lib/promotion.rb | 24 +++++++++++++++--------- lib/trust_level.rb | 2 +- spec/lib/promotion_spec.rb | 9 +++++++++ spec/models/group_spec.rb | 16 ++++++++++------ spec/models/group_user_spec.rb | 5 +++-- 5 files changed, 38 insertions(+), 18 deletions(-) diff --git a/lib/promotion.rb b/lib/promotion.rb index b817ae64414..688751f8f88 100644 --- a/lib/promotion.rb +++ b/lib/promotion.rb @@ -133,17 +133,23 @@ class Promotion # Figure out what a user's trust level should be from scratch 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) - return user.update(trust_level: granted_trust_level) if granted_trust_level.present? + TrustLevel.calculate(user, use_previous_trust_level: use_previous_trust_level) || + TrustLevel[0] - user.update_column(:trust_level, TrustLevel[0]) + # TrustLevel.calculate always returns a value, however we added extra protection just + # in case this changes + user.update_column(:trust_level, TrustLevel[granted_trust_level]) - p = Promotion.new(user) - p.review_tl0 - p.review_tl1 - p.review_tl2 - if user.trust_level == 3 && Promotion.tl3_lost?(user) - user.change_trust_level!(2, log_action_for: performed_by || Discourse.system_user) + return if user.manual_locked_trust_level.present? + + promotion = Promotion.new(user) + + promotion.review_tl0 if granted_trust_level < TrustLevel[1] + promotion.review_tl1 if granted_trust_level < TrustLevel[2] + promotion.review_tl2 if granted_trust_level < TrustLevel[3] + + if user.trust_level == TrustLevel[3] && Promotion.tl3_lost?(user) + user.change_trust_level!(TrustLevel[2], log_action_for: performed_by || Discourse.system_user) end end end diff --git a/lib/trust_level.rb b/lib/trust_level.rb index 2589df081b9..8db2e634baf 100644 --- a/lib/trust_level.rb +++ b/lib/trust_level.rb @@ -38,7 +38,7 @@ class TrustLevel 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 + [granted_trust_level, previous_trust_level, SiteSetting.default_trust_level].max end private diff --git a/spec/lib/promotion_spec.rb b/spec/lib/promotion_spec.rb index d830480ce39..d807a309c0c 100644 --- a/spec/lib/promotion_spec.rb +++ b/spec/lib/promotion_spec.rb @@ -99,9 +99,18 @@ RSpec.describe Promotion do stat.posts_read_count = SiteSetting.tl1_requires_read_posts stat.time_read = SiteSetting.tl1_requires_time_spent_mins * 60 Promotion.recalculate(user) + + expect(user.trust_level).to eq(1) + expect(Jobs::SendSystemMessage.jobs.length).to eq(0) end + it "respects default trust level" do + SiteSetting.default_trust_level = 2 + Promotion.recalculate(user) + expect(user.trust_level).to eq(2) + end + it "can be turned off" do SiteSetting.send_tl1_welcome_message = false @result = promotion.review diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 349a2240288..da63a2e8ebd 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -253,7 +253,7 @@ RSpec.describe Group do tl3_users = Group.find(Group::AUTO_GROUPS[:trust_level_3]) tl3_users.add(user) - events = DiscourseEvent.track_events { Group.refresh_automatic_group!(:trust_level_3) } + _events = DiscourseEvent.track_events { Group.refresh_automatic_group!(:trust_level_3) } expect(GroupUser.exists?(group: tl3_users, user: user)).to eq(false) publish_event_job_args = Jobs::PublishGroupMembershipUpdates.jobs.last["args"].first @@ -267,7 +267,7 @@ RSpec.describe Group do expect(GroupUser.exists?(group: tl0_users, user: user)).to eq(false) - events = DiscourseEvent.track_events { Group.refresh_automatic_group!(:trust_level_0) } + _events = DiscourseEvent.track_events { Group.refresh_automatic_group!(:trust_level_0) } expect(GroupUser.exists?(group: tl0_users, user: user)).to eq(true) publish_event_job_args = Jobs::PublishGroupMembershipUpdates.jobs.last["args"].first @@ -641,8 +641,8 @@ RSpec.describe Group do describe "when a user has qualified for trust level 1" do fab!(:user) { Fabricate(:user, trust_level: 1, created_at: Time.zone.now - 10.years) } - fab!(:group) { Fabricate(:group, grant_trust_level: 1) } - fab!(:group2) { Fabricate(:group, grant_trust_level: 0) } + fab!(:group) { Fabricate(:group, grant_trust_level: 3) } + fab!(:group2) { Fabricate(:group, grant_trust_level: 2) } before { user.user_stat.update!(topics_entered: 999, posts_read_count: 999, time_read: 999) } @@ -650,11 +650,15 @@ RSpec.describe Group do group.add(user) group2.add(user) - expect(user.reload.trust_level).to eq(1) + expect(user.reload.trust_level).to eq(3) group.remove(user) - expect(user.reload.trust_level).to eq(0) + expect(user.reload.trust_level).to eq(2) + + group2.remove(user) + + expect(user.reload.trust_level).to eq(1) end end diff --git a/spec/models/group_user_spec.rb b/spec/models/group_user_spec.rb index 0323544f88c..896a0973721 100644 --- a/spec/models/group_user_spec.rb +++ b/spec/models/group_user_spec.rb @@ -293,15 +293,16 @@ RSpec.describe GroupUser do user = Fabricate(:user) expect(user.trust_level).to eq(1) + user.change_trust_level!(1, log_action_for: Discourse.system_user) 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) + # keep in mind that we do not restore tl3, cause reqs can be lost + expect(user.reload.trust_level).to eq(2) end end end