From 2070edf8890ce11b1ccad4ed47c85bff49ffcde6 Mon Sep 17 00:00:00 2001 From: Bianca Nenciu Date: Sat, 25 Aug 2018 00:41:03 +0200 Subject: [PATCH] FIX: Clarify User.group_locked_trust_level. * Rename User.group_locked_trust_level to User.group_granted_trust_level. * Remove the column from users table. --- app/jobs/scheduled/tl3_promotions.rb | 12 ++++--- app/models/group_user.rb | 11 ------- app/models/user.rb | 11 +++++++ app/services/user_merger.rb | 1 - ...drop_group_locked_trust_level_from_user.rb | 9 +++++ lib/promotion.rb | 6 ++-- spec/jobs/tl3_promotions_spec.rb | 33 +++++++++++-------- spec/models/group_spec.rb | 10 +++--- spec/models/user_spec.rb | 2 +- 9 files changed, 58 insertions(+), 37 deletions(-) create mode 100644 db/post_migrate/20181012123001_drop_group_locked_trust_level_from_user.rb diff --git a/app/jobs/scheduled/tl3_promotions.rb b/app/jobs/scheduled/tl3_promotions.rb index 7dd8d05db09..f494638da10 100644 --- a/app/jobs/scheduled/tl3_promotions.rb +++ b/app/jobs/scheduled/tl3_promotions.rb @@ -6,10 +6,14 @@ module Jobs def execute(args) # Demotions demoted_user_ids = [] - User.real.where( - trust_level: TrustLevel[3], - manual_locked_trust_level: nil - ).where("group_locked_trust_level IS NULL OR group_locked_trust_level < ?", TrustLevel[3]).find_each do |u| + User.real + .joins("LEFT JOIN (SELECT gu.user_id, MAX(g.grant_trust_level) AS group_granted_trust_level FROM groups g, group_users gu WHERE g.id = gu.group_id GROUP BY gu.user_id) tl ON users.id = tl.user_id") + .where( + trust_level: TrustLevel[3], + manual_locked_trust_level: nil + ) + .where("group_granted_trust_level IS NULL OR group_granted_trust_level < ?", TrustLevel[3]) + .find_each do |u| # Don't demote too soon after being promoted next if u.on_tl3_grace_period? diff --git a/app/models/group_user.rb b/app/models/group_user.rb index 330290a3fe6..57028e8f8fd 100644 --- a/app/models/group_user.rb +++ b/app/models/group_user.rb @@ -62,23 +62,12 @@ class GroupUser < ActiveRecord::Base def grant_trust_level return if group.grant_trust_level.nil? - if (user.group_locked_trust_level || 0) < group.grant_trust_level - user.update!(group_locked_trust_level: group.grant_trust_level) - end - TrustLevelGranter.grant(group.grant_trust_level, user) end def recalculate_trust_level return if group.grant_trust_level.nil? - # Find the highest level of the user's remaining groups - highest_level = GroupUser - .where(user_id: user.id) - .includes(:group) - .maximum("groups.grant_trust_level") - - user.update!(group_locked_trust_level: highest_level) Promotion.recalculate(user) end end diff --git a/app/models/user.rb b/app/models/user.rb index 6f805161e0d..21d230d6a8f 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -21,6 +21,10 @@ class User < ActiveRecord::Base include SecondFactorManager include HasDestroyedWebHook + self.ignored_columns = %w{ + group_locked_trust_level + } + has_many :posts has_many :notifications, dependent: :destroy has_many :topic_users, dependent: :destroy @@ -342,6 +346,13 @@ class User < ActiveRecord::Base find_by(username_lower: username.downcase) end + def group_granted_trust_level + GroupUser + .where(user_id: id) + .includes(:group) + .maximum("groups.grant_trust_level") + end + def enqueue_welcome_message(message_type) return unless SiteSetting.send_welcome_message? Jobs.enqueue(:send_system_message, user_id: id, message_type: message_type) diff --git a/app/services/user_merger.rb b/app/services/user_merger.rb index 1d31a802e7c..71925af8778 100644 --- a/app/services/user_merger.rb +++ b/app/services/user_merger.rb @@ -209,7 +209,6 @@ class UserMerger primary_group_id = COALESCE(t.primary_group_id, s.primary_group_id), registration_ip_address = COALESCE(t.registration_ip_address, s.registration_ip_address), first_seen_at = LEAST(t.first_seen_at, s.first_seen_at), - group_locked_trust_level = GREATEST(t.group_locked_trust_level, s.group_locked_trust_level), manual_locked_trust_level = GREATEST(t.manual_locked_trust_level, s.manual_locked_trust_level) FROM users AS s WHERE t.id = :target_user_id AND s.id = :source_user_id diff --git a/db/post_migrate/20181012123001_drop_group_locked_trust_level_from_user.rb b/db/post_migrate/20181012123001_drop_group_locked_trust_level_from_user.rb new file mode 100644 index 00000000000..0092d77e254 --- /dev/null +++ b/db/post_migrate/20181012123001_drop_group_locked_trust_level_from_user.rb @@ -0,0 +1,9 @@ +class DropGroupLockedTrustLevelFromUser < ActiveRecord::Migration[5.2] + def up + Migration::ColumnDropper.execute_drop(:posts, %i{group_locked_trust_level}) + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/lib/promotion.rb b/lib/promotion.rb index 84b56783d44..02935df148f 100644 --- a/lib/promotion.rb +++ b/lib/promotion.rb @@ -121,9 +121,11 @@ class Promotion end # Then consider the group locked level - if user.group_locked_trust_level + user_group_granted_trust_level = user.group_granted_trust_level + + unless user_group_granted_trust_level.blank? return user.update!( - trust_level: user.group_locked_trust_level + trust_level: user_group_granted_trust_level ) end diff --git a/spec/jobs/tl3_promotions_spec.rb b/spec/jobs/tl3_promotions_spec.rb index 983c1d704bc..58e39dcfd50 100644 --- a/spec/jobs/tl3_promotions_spec.rb +++ b/spec/jobs/tl3_promotions_spec.rb @@ -17,26 +17,29 @@ describe Jobs::Tl3Promotions do subject(:run_job) { described_class.new.execute({}) } it "promotes tl2 user who qualifies for tl3" do - _tl2_user = Fabricate(:user, trust_level: TrustLevel[2]) - create_qualifying_stats(_tl2_user) + tl2_user = Fabricate(:user, trust_level: TrustLevel[2]) + create_qualifying_stats(tl2_user) TrustLevel3Requirements.any_instance.stubs(:requirements_met?).returns(true) Promotion.any_instance.expects(:change_trust_level!).with(TrustLevel[3], anything).once run_job end - it "promotes a qualifying tl2 user who has a group_locked_trust_level" do - _group_locked_user = Fabricate(:user, trust_level: TrustLevel[2], group_locked_trust_level: TrustLevel[1]) - create_qualifying_stats(_group_locked_user) + it "promotes a qualifying tl2 user who has a group_granted_trust_level" do + group = Fabricate(:group, grant_trust_level: 1) + group_locked_user = Fabricate(:user, trust_level: TrustLevel[2]) + group.add(group_locked_user) + + create_qualifying_stats(group_locked_user) TrustLevel3Requirements.any_instance.stubs(:requirements_met?).returns(true) Promotion.any_instance.expects(:change_trust_level!).with(TrustLevel[3], anything).once run_job end it "doesn't promote tl1 and tl0 users who have met tl3 requirements" do - _tl1_user = Fabricate(:user, trust_level: TrustLevel[1]) - _tl0_user = Fabricate(:user, trust_level: TrustLevel[0]) - create_qualifying_stats(_tl1_user) - create_qualifying_stats(_tl0_user) + tl1_user = Fabricate(:user, trust_level: TrustLevel[1]) + tl0_user = Fabricate(:user, trust_level: TrustLevel[0]) + create_qualifying_stats(tl1_user) + create_qualifying_stats(tl0_user) TrustLevel3Requirements.any_instance.expects(:requirements_met?).never Promotion.any_instance.expects(:change_trust_level!).never run_job @@ -91,10 +94,12 @@ describe Jobs::Tl3Promotions do expect(user.reload.trust_level).to eq(TrustLevel[3]) end - it "demotes a user with a group_locked_trust_level of 2" do + it "demotes a user with a group_granted_trust_level of 2" do + group = Fabricate(:group, grant_trust_level: 2) user = nil freeze_time(4.days.ago) do - user = Fabricate(:user, trust_level: TrustLevel[3], group_locked_trust_level: TrustLevel[2]) + user = Fabricate(:user, trust_level: TrustLevel[3]) + group.add(user) end TrustLevel3Requirements.any_instance.stubs(:requirements_met?).returns(false) TrustLevel3Requirements.any_instance.stubs(:requirements_lost?).returns(true) @@ -103,10 +108,12 @@ describe Jobs::Tl3Promotions do end - it "doesn't demote user if their group_locked_trust_level is 3" do + it "doesn't demote user if their group_granted_trust_level is 3" do + group = Fabricate(:group, grant_trust_level: 3) user = nil freeze_time(4.days.ago) do - user = Fabricate(:user, trust_level: TrustLevel[3], group_locked_trust_level: TrustLevel[3]) + user = Fabricate(:user, trust_level: TrustLevel[3]) + group.add(user) end TrustLevel3Requirements.any_instance.stubs(:requirements_met?).returns(false) TrustLevel3Requirements.any_instance.stubs(:requirements_lost?).returns(true) diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 33cd25b8c0c..a0bdc24ac80 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -588,28 +588,28 @@ describe Group do # Add a group without one to consider `NULL` check g2.add(user) - expect(user.group_locked_trust_level).to be_nil + expect(user.group_granted_trust_level).to be_nil expect(user.manual_locked_trust_level).to be_nil g0.add(user) expect(user.reload.trust_level).to eq(2) - expect(user.group_locked_trust_level).to eq(2) + expect(user.group_granted_trust_level).to eq(2) expect(user.manual_locked_trust_level).to be_nil g1.add(user) expect(user.reload.trust_level).to eq(3) - expect(user.group_locked_trust_level).to eq(3) + expect(user.group_granted_trust_level).to eq(3) expect(user.manual_locked_trust_level).to be_nil g1.remove(user) expect(user.reload.trust_level).to eq(2) - expect(user.group_locked_trust_level).to eq(2) + expect(user.group_granted_trust_level).to eq(2) expect(user.manual_locked_trust_level).to be_nil g0.remove(user) user.reload expect(user.manual_locked_trust_level).to be_nil - expect(user.group_locked_trust_level).to be_nil + expect(user.group_granted_trust_level).to be_nil expect(user.trust_level).to eq(0) end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 42d73ae7073..6849785b2d2 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1378,7 +1378,7 @@ describe User do expect(user.title).to eq("bars and wats") expect(user.trust_level).to eq(1) expect(user.manual_locked_trust_level).to be_nil - expect(user.group_locked_trust_level).to eq(1) + expect(user.group_granted_trust_level).to eq(1) end end