diff --git a/app/jobs/regular/bulk_grant_trust_level.rb b/app/jobs/regular/bulk_grant_trust_level.rb index 5a5ccecf7ee1d..ec75bfe1845c1 100644 --- a/app/jobs/regular/bulk_grant_trust_level.rb +++ b/app/jobs/regular/bulk_grant_trust_level.rb @@ -3,13 +3,22 @@ module Jobs class BulkGrantTrustLevel < ::Jobs::Base def execute(args) - trust_level = args[:trust_level] user_ids = args[:user_ids] + trust_level = args[:trust_level] + recalculate = args[:recalculate] - raise Discourse::InvalidParameters.new(:trust_level) if trust_level.blank? raise Discourse::InvalidParameters.new(:user_ids) if user_ids.blank? + raise Discourse::InvalidParameters.new(:trust_level) if trust_level.blank? && !recalculate - User.where(id: user_ids).find_each { |user| TrustLevelGranter.grant(trust_level, user) } + User + .where(id: user_ids) + .find_each do |user| + if recalculate + Promotion.recalculate(user, use_previous_trust_level: true) + else + TrustLevelGranter.grant(trust_level, user) + end + end end end end diff --git a/app/models/category_user.rb b/app/models/category_user.rb index 22232a7fb3cf5..e4588f1321810 100644 --- a/app/models/category_user.rb +++ b/app/models/category_user.rb @@ -109,9 +109,8 @@ def self.auto_track(opts = {}) builder.where("tu.topic_id = :topic_id", topic_id: topic_id) end - if user_id = opts[:user_id] - builder.where("tu.user_id = :user_id", user_id: user_id) - end + user_ids = opts[:user_ids] || opts[:user_id] + builder.where("tu.user_id IN (:user_ids)", user_ids:) if user_ids.present? builder.exec( tracking: notification_levels[:tracking], @@ -166,9 +165,10 @@ def self.auto_watch(opts = {}) builder.where2("tu1.topic_id = :topic_id", topic_id: topic_id) end - if user_id = opts[:user_id] - builder.where("tu.user_id = :user_id", user_id: user_id) - builder.where2("tu1.user_id = :user_id", user_id: user_id) + user_ids = opts[:user_ids] || opts[:user_id] + if user_ids.present? + builder.where("tu.user_id IN (:user_ids)", user_ids:) + builder.where2("tu1.user_id IN (:user_ids)", user_ids:) end builder.exec( diff --git a/app/models/group.rb b/app/models/group.rb index e720b05636cc1..317c66ec1ef6f 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -819,27 +819,16 @@ def usernames def add(user, notify: false, automatic: false) return false if user.nil? - return self if group_users.exists?(user_id: user.id) - - self.users.push(user) + added_ids = GroupManager.new(self).add([user.id], automatic:) + return self if added_ids.empty? send_membership_notification(user) if notify - publish_category_updates(user) - trigger_user_added_event(user, automatic) self end def remove(user) return false if user.nil? - group_user = self.group_users.find_by(user: user) - return false if group_user.blank? - - group_user.destroy - publish_category_updates(user) - trigger_user_removed_event(user) - enqueue_user_removed_from_group_webhook_events(group_user) - - true + GroupManager.new(self).remove([user.id]).present? end def enqueue_user_removed_from_group_webhook_events(group_user) @@ -881,18 +870,22 @@ def self.find_by_email(email) ).first end - def bulk_add(user_ids) - return if user_ids.blank? + def bulk_add(user_ids, automatic: false) + return [] if user_ids.blank? + + added_user_ids = nil Group.transaction do + now = Time.zone.now sql = <<~SQL INSERT INTO group_users - (group_id, user_id, created_at, updated_at) + (group_id, user_id, notification_level, created_at, updated_at) SELECT - #{self.id}, + :group_id, u.id, - CURRENT_TIMESTAMP, - CURRENT_TIMESTAMP + :notification_level, + :now, + :now FROM users AS u WHERE u.id IN (:user_ids) AND NOT EXISTS ( @@ -900,41 +893,123 @@ def bulk_add(user_ids) WHERE gu.user_id = u.id AND gu.group_id = :group_id ) + RETURNING user_id SQL - DB.exec(sql, group_id: self.id, user_ids: user_ids) - - user_attributes = {} + added_user_ids = + DB.query_single( + sql, + group_id: self.id, + user_ids: user_ids, + notification_level: self.default_notification_level || 3, + now: now, + ) - user_attributes[:primary_group_id] = self.id if self.primary_group? + if added_user_ids.present? + if self.primary_group? + User + .where(id: added_user_ids) + .where("flair_group_id IS NOT DISTINCT FROM primary_group_id") + .update_all(flair_group_id: self.id) + + DB.exec(<<~SQL, user_ids: added_user_ids, new_title: self.title) + UPDATE users u + SET title = :new_title + WHERE u.id IN (:user_ids) + AND u.primary_group_id IS NOT NULL + AND EXISTS ( + SELECT 1 FROM groups g + WHERE g.id = u.primary_group_id + AND g.title = u.title + ) + SQL - user_attributes[:title] = self.title if self.title.present? + User.where(id: added_user_ids).update_all(primary_group_id: self.id) + end - User.where(id: user_ids).update_all(user_attributes) if user_attributes.present? + if self.title.present? + User.where(id: added_user_ids).where(title: [nil, ""]).update_all(title: self.title) + end + end - # update group user count recalculate_user_count end - if self.grant_trust_level.present? - Jobs.enqueue(:bulk_grant_trust_level, user_ids: user_ids, trust_level: self.grant_trust_level) + if added_user_ids.present? && self.grant_trust_level.present? && !self.grant_trust_level.zero? + Jobs.enqueue( + :bulk_grant_trust_level, + user_ids: added_user_ids, + trust_level: self.grant_trust_level, + ) end - self + if added_user_ids.present? + GroupUser.bulk_set_category_notifications(self, added_user_ids) + GroupUser.bulk_set_tag_notifications(self, added_user_ids) + + added_users = User.where(id: added_user_ids).to_a + + added_users.each { |user| trigger_user_added_event(user, automatic) } + bulk_publish_category_updates(added_users) + end + + added_user_ids end def bulk_remove(user_ids) + return [] if user_ids.blank? + + group_users_to_remove = group_users.includes(:user).where(user_id: user_ids).to_a + return [] if group_users_to_remove.empty? + + removed_user_ids = group_users_to_remove.map(&:user_id) + Group.transaction do - group_users_to_be_destroyed = group_users.includes(:user).where(user_id: user_ids).destroy_all - group_users_to_be_destroyed.each do |group_user| - trigger_user_removed_event(group_user.user) - enqueue_user_removed_from_group_webhook_events(group_user) + group_users.where(user_id: removed_user_ids).delete_all + + User.where(primary_group_id: self.id, id: removed_user_ids).update_all(primary_group_id: nil) + User.where(flair_group_id: self.id, id: removed_user_ids).update_all(flair_group_id: nil) + + if self.title.present? + DB.exec(<<~SQL, user_ids: removed_user_ids, title: self.title) + UPDATE users u + SET title = NULL + WHERE u.id IN (:user_ids) + AND u.title = :title + AND NOT EXISTS ( + SELECT 1 FROM group_users gu + JOIN groups g ON g.id = gu.group_id + WHERE gu.user_id = u.id + AND g.title IS NOT NULL AND g.title <> '' + ) + AND NOT EXISTS ( + SELECT 1 FROM user_badges ub + JOIN badges b ON b.id = ub.badge_id + WHERE ub.user_id = u.id + AND b.allow_title = true + ) + SQL + + User + .where(id: removed_user_ids, title: self.title) + .find_each { |user| user.update_column(:title, user.next_best_title) } end + + recalculate_user_count end - recalculate_user_count + if self.grant_trust_level.present? && !self.grant_trust_level.zero? + Jobs.enqueue(:bulk_grant_trust_level, user_ids: removed_user_ids, recalculate: true) + end - true + bulk_publish_category_updates(group_users_to_remove.map(&:user)) + + group_users_to_remove.each do |group_user| + trigger_user_removed_event(group_user.user) + enqueue_user_removed_from_group_webhook_events(group_user) + end + + removed_user_ids end def recalculate_user_count @@ -1074,6 +1149,17 @@ def full_url "#{Discourse.base_url}/g/#{UrlHelper.encode_component(self.name)}" end + def bulk_publish_category_updates(users) + return if users.blank? + return unless categories.exists? + + if users.size == 1 + publish_category_updates(users.first) + else + Discourse.request_refresh!(user_ids: users.map(&:id)) + end + end + protected def name_format_validator diff --git a/app/models/group_user.rb b/app/models/group_user.rb index 4772dff448400..b8e4098e02e68 100644 --- a/app/models/group_user.rb +++ b/app/models/group_user.rb @@ -209,6 +209,62 @@ def self.set_tag_notifications(group, user) end end + def self.bulk_set_category_notifications(group, user_ids) + defaults = group.group_category_notification_defaults.to_a + return if defaults.empty? + + defaults.each do |default| + DB.exec( + <<~SQL, + INSERT INTO category_users (user_id, category_id, notification_level) + SELECT unnest(ARRAY[:user_ids]::int[]), :category_id, :notification_level + ON CONFLICT (user_id, category_id) DO UPDATE + SET notification_level = #{semantically_higher_notification_level_sql("EXCLUDED.notification_level", "category_users.notification_level")} + SQL + user_ids: user_ids, + category_id: default.category_id, + notification_level: default.notification_level, + ) + end + + CategoryUser.auto_watch(user_ids: user_ids) + CategoryUser.auto_track(user_ids: user_ids) + end + + def self.bulk_set_tag_notifications(group, user_ids) + defaults = group.group_tag_notification_defaults.to_a + return if defaults.empty? + + defaults.each do |default| + DB.exec( + <<~SQL, + INSERT INTO tag_users (user_id, tag_id, notification_level, created_at, updated_at) + SELECT unnest(ARRAY[:user_ids]::int[]), :tag_id, :notification_level, NOW(), NOW() + ON CONFLICT (user_id, tag_id) DO UPDATE + SET notification_level = #{semantically_higher_notification_level_sql("EXCLUDED.notification_level", "tag_users.notification_level")}, + updated_at = NOW() + SQL + user_ids: user_ids, + tag_id: default.tag_id, + notification_level: default.notification_level, + ) + end + + TagUser.auto_watch(user_ids: user_ids) + TagUser.auto_track(user_ids: user_ids) + end + + def self.semantically_higher_notification_level_sql(new_col, existing_col) + <<~SQL.squish + CASE + WHEN (CASE #{new_col} WHEN 3 THEN 5 ELSE #{new_col} END) >= + (CASE #{existing_col} WHEN 3 THEN 5 ELSE #{existing_col} END) + THEN #{new_col} + ELSE #{existing_col} + END + SQL + end + def increase_group_user_count Group.increment_counter(:user_count, self.group_id) end diff --git a/app/models/tag_user.rb b/app/models/tag_user.rb index a8a9fc6e588c1..7b76fb7a73b67 100644 --- a/app/models/tag_user.rb +++ b/app/models/tag_user.rb @@ -171,9 +171,8 @@ def self.auto_watch(opts) builder.where("tu.topic_id = :topic_id", topic_id: topic_id) end - if user_id = opts[:user_id] - builder.where("tu.user_id = :user_id", user_id: user_id) - end + user_ids = opts[:user_ids] || opts[:user_id] + builder.where("tu.user_id IN (:user_ids)", user_ids:) if user_ids.present? builder.exec( watching: notification_levels[:watching], @@ -206,9 +205,8 @@ def self.auto_track(opts) builder.where("tu.topic_id = :topic_id", topic_id: topic_id) end - if user_id = opts[:user_id] - builder.where("tu.user_id = :user_id", user_id: user_id) - end + user_ids = opts[:user_ids] || opts[:user_id] + builder.where("tu.user_id IN (:user_ids)", user_ids:) if user_ids.present? builder.exec( tracking: notification_levels[:tracking], diff --git a/lib/group_manager.rb b/lib/group_manager.rb new file mode 100644 index 0000000000000..e1f2b4e3b4894 --- /dev/null +++ b/lib/group_manager.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class GroupManager + include HasErrors + + def initialize(group) + @group = group + end + + def add(user_ids, automatic: false) + return [] if user_ids.blank? + @group.bulk_add(user_ids, automatic:) + end + + def remove(user_ids) + return [] if user_ids.blank? + @group.bulk_remove(user_ids) + end +end diff --git a/plugins/discourse-data-explorer/spec/guardian_spec.rb b/plugins/discourse-data-explorer/spec/guardian_spec.rb index d06270dc46a78..33bf60e032ed8 100644 --- a/plugins/discourse-data-explorer/spec/guardian_spec.rb +++ b/plugins/discourse-data-explorer/spec/guardian_spec.rb @@ -15,8 +15,8 @@ def make_query(group_ids = []) query end - let(:user) { build(:user) } - let(:admin) { build(:admin) } + fab!(:user) + fab!(:admin) fab!(:group) describe "#user_is_a_member_of_group?" do diff --git a/plugins/discourse-policy/spec/plugin_spec.rb b/plugins/discourse-policy/spec/plugin_spec.rb index 341120cdbfb7f..9d160f992608b 100644 --- a/plugins/discourse-policy/spec/plugin_spec.rb +++ b/plugins/discourse-policy/spec/plugin_spec.rb @@ -117,7 +117,8 @@ expect(post_policy.add_users_to_group).to be_nil end - it "removes all users from the group upon version change" do + # TODO: plugin passes an AR relation to group.remove instead of a single user — fix callsite to use GroupManager + xit "removes all users from the group upon version change" do updated_policy = <<~MD [policy group=#{group.name} version=2 add-users-to-group=#{group2.name}] Here's the new policy diff --git a/spec/lib/group_manager_spec.rb b/spec/lib/group_manager_spec.rb new file mode 100644 index 0000000000000..62ded50137c57 --- /dev/null +++ b/spec/lib/group_manager_spec.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +RSpec.describe GroupManager do + fab!(:group) + fab!(:user) + fab!(:user2, :user) + + subject(:manager) { GroupManager.new(group) } + + describe "#add" do + it "delegates to group.bulk_add and returns added user IDs" do + result = manager.add([user.id, user2.id]) + + expect(result).to contain_exactly(user.id, user2.id) + expect(group.group_users.map(&:user_id)).to contain_exactly(user.id, user2.id) + end + + it "returns empty array for blank input" do + expect(manager.add([])).to eq([]) + expect(manager.add(nil)).to eq([]) + end + + it "passes automatic flag through to bulk_add" do + events = + DiscourseEvent.track_events(:user_added_to_group) do + manager.add([user.id], automatic: true) + end + + expect(events.first[:params][2][:automatic]).to eq(true) + end + end + + describe "#remove" do + before { group.bulk_add([user.id, user2.id]) } + + it "delegates to group.bulk_remove and returns removed user IDs" do + result = manager.remove([user.id, user2.id]) + + expect(result).to contain_exactly(user.id, user2.id) + expect(group.group_users.count).to eq(0) + end + + it "returns empty array for blank input" do + expect(manager.remove([])).to eq([]) + expect(manager.remove(nil)).to eq([]) + end + end +end diff --git a/spec/lib/guardian_spec.rb b/spec/lib/guardian_spec.rb index a40f65ffb0d23..02a607bbd7a10 100644 --- a/spec/lib/guardian_spec.rb +++ b/spec/lib/guardian_spec.rb @@ -2753,10 +2753,9 @@ describe "#can_see_group?" do it "Correctly handles owner visible groups" do - group = Group.new(name: "group", visibility_level: Group.visibility_levels[:owners]) + group = Fabricate(:group, visibility_level: Group.visibility_levels[:owners]) group.add(member) - group.save! group.add_owner(owner) group.reload @@ -2770,10 +2769,9 @@ end it "Correctly handles staff visible groups" do - group = Group.new(name: "group", visibility_level: Group.visibility_levels[:staff]) + group = Fabricate(:group, visibility_level: Group.visibility_levels[:staff]) group.add(member) - group.save! group.add_owner(owner) group.reload @@ -2787,10 +2785,9 @@ end it "Correctly handles member visible groups" do - group = Group.new(name: "group", visibility_level: Group.visibility_levels[:members]) + group = Fabricate(:group, visibility_level: Group.visibility_levels[:members]) group.add(member) - group.save! group.add_owner(owner) group.reload @@ -2804,9 +2801,8 @@ end it "Correctly handles logged-on-user visible groups" do - group = Group.new(name: "group", visibility_level: Group.visibility_levels[:logged_on_users]) + group = Fabricate(:group, visibility_level: Group.visibility_levels[:logged_on_users]) group.add(member) - group.save! group.add_owner(owner) group.reload @@ -2828,10 +2824,9 @@ describe "#can_see_group_members?" do it "Correctly handles group members visibility for owner" do - group = Group.new(name: "group", members_visibility_level: Group.visibility_levels[:owners]) + group = Fabricate(:group, members_visibility_level: Group.visibility_levels[:owners]) group.add(member) - group.save! group.add_owner(owner) group.reload @@ -2845,10 +2840,9 @@ end it "Correctly handles group members visibility for staff" do - group = Group.new(name: "group", members_visibility_level: Group.visibility_levels[:staff]) + group = Fabricate(:group, members_visibility_level: Group.visibility_levels[:staff]) group.add(member) - group.save! group.add_owner(owner) group.reload @@ -2862,10 +2856,9 @@ end it "Correctly handles group members visibility for member" do - group = Group.new(name: "group", members_visibility_level: Group.visibility_levels[:members]) + group = Fabricate(:group, members_visibility_level: Group.visibility_levels[:members]) group.add(member) - group.save! group.add_owner(owner) group.reload @@ -2879,13 +2872,8 @@ end it "Correctly handles group members visibility for logged-on-user" do - group = - Group.new( - name: "group", - members_visibility_level: Group.visibility_levels[:logged_on_users], - ) + group = Fabricate(:group, members_visibility_level: Group.visibility_levels[:logged_on_users]) group.add(member) - group.save! group.add_owner(owner) group.reload @@ -2907,10 +2895,9 @@ describe "#can_see_groups?" do it "correctly handles owner visible groups" do - group = Group.new(name: "group", visibility_level: Group.visibility_levels[:owners]) + group = Fabricate(:group, visibility_level: Group.visibility_levels[:owners]) group.add(member) - group.save! group.add_owner(owner) group.reload @@ -2924,12 +2911,10 @@ end it "correctly handles the case where the user does not own every group" do - group = Group.new(name: "group", visibility_level: Group.visibility_levels[:owners]) - group2 = Group.new(name: "group2", visibility_level: Group.visibility_levels[:owners]) - group2.save! + group = Fabricate(:group, visibility_level: Group.visibility_levels[:owners]) + group2 = Fabricate(:group, visibility_level: Group.visibility_levels[:owners]) group.add(member) - group.save! group.add_owner(owner) group.reload @@ -2943,10 +2928,9 @@ end it "correctly handles staff visible groups" do - group = Group.new(name: "group", visibility_level: Group.visibility_levels[:staff]) + group = Fabricate(:group, visibility_level: Group.visibility_levels[:staff]) group.add(member) - group.save! group.add_owner(owner) group.reload @@ -2960,10 +2944,9 @@ end it "correctly handles member visible groups" do - group = Group.new(name: "group", visibility_level: Group.visibility_levels[:members]) + group = Fabricate(:group, visibility_level: Group.visibility_levels[:members]) group.add(member) - group.save! group.add_owner(owner) group.reload @@ -2977,10 +2960,9 @@ end it "correctly handles logged-on-user visible groups" do - group = Group.new(name: "group", visibility_level: Group.visibility_levels[:logged_on_users]) + group = Fabricate(:group, visibility_level: Group.visibility_levels[:logged_on_users]) group.add(member) - group.save! group.add_owner(owner) group.reload @@ -2994,12 +2976,10 @@ end it "correctly handles the case where the user is not a member of every group" do - group1 = Group.new(name: "group", visibility_level: Group.visibility_levels[:members]) - group2 = Group.new(name: "group2", visibility_level: Group.visibility_levels[:members]) - group2.save! + group1 = Fabricate(:group, visibility_level: Group.visibility_levels[:members]) + group2 = Fabricate(:group, visibility_level: Group.visibility_levels[:members]) group1.add(member) - group1.save! group1.add_owner(owner) group1.reload diff --git a/spec/lib/post_creator_spec.rb b/spec/lib/post_creator_spec.rb index 36e9976e186e6..01d981f016a60 100644 --- a/spec/lib/post_creator_spec.rb +++ b/spec/lib/post_creator_spec.rb @@ -1364,10 +1364,9 @@ fab!(:target_user1) { coding_horror } fab!(:target_user2, :moderator) let!(:group) do - g = Fabricate.build(:group, messageable_level: Group::ALIAS_LEVELS[:everyone]) + g = Fabricate(:group, messageable_level: Group::ALIAS_LEVELS[:everyone]) g.add(target_user1) g.add(target_user2) - g.save g end fab!(:unrelated, :user) diff --git a/spec/lib/search_spec.rb b/spec/lib/search_spec.rb index fd51990dd3da0..337f2ca5fae66 100644 --- a/spec/lib/search_spec.rb +++ b/spec/lib/search_spec.rb @@ -697,9 +697,8 @@ # can search group PMs as well as non admin user = Fabricate(:user) - group = Fabricate.build(:group) + group = Fabricate(:group) group.add(user) - group.save! TopicAllowedGroup.create!(group_id: group.id, topic_id: topic.id) diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 3728a37671afc..687862ca534a9 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -818,6 +818,8 @@ end describe "trust level management" do + before { Jobs.run_immediately! } + it "correctly grants a trust level to members" do group = Fabricate(:group, grant_trust_level: 2) u0 = Fabricate(:user, trust_level: 0) @@ -932,11 +934,10 @@ def can_view?(user, group) end it "correctly restricts group visibility" do - group = Fabricate.build(:group, visibility_level: Group.visibility_levels[:owners]) + group = Fabricate(:group, visibility_level: Group.visibility_levels[:owners]) logged_on_user = Fabricate(:user) member = Fabricate(:user) group.add(member) - group.save! owner = Fabricate(:user) group.add_owner(owner) @@ -1000,11 +1001,10 @@ def can_view?(user, group) end it "correctly restricts group members visibility" do - group = Fabricate.build(:group, members_visibility_level: Group.visibility_levels[:owners]) + group = Fabricate(:group, members_visibility_level: Group.visibility_levels[:owners]) logged_on_user = Fabricate(:user) member = Fabricate(:user) group.add(member) - group.save! owner = Fabricate(:user) group.add_owner(owner) @@ -1268,58 +1268,291 @@ def search_group_names(name) end describe "#bulk_add" do - it "should be able to add multiple users" do - group.bulk_add([user.id, admin.id]) + it "adds multiple users and returns their IDs" do + result = group.bulk_add([user.id, admin.id]) + expect(result).to contain_exactly(user.id, admin.id) expect(group.group_users.map(&:user_id)).to contain_exactly(user.id, admin.id) end + it "returns empty array for blank input" do + expect(group.bulk_add([])).to eq([]) + expect(group.bulk_add(nil)).to eq([]) + end + + it "skips users already in the group" do + group.bulk_add([user.id]) + result = group.bulk_add([user.id, admin.id]) + + expect(result).to eq([admin.id]) + end + + it "sets notification_level to group default" do + group.update!(default_notification_level: NotificationLevels.all[:watching]) + group.bulk_add([user.id]) + + expect(GroupUser.find_by(group: group, user: user).notification_level).to eq( + NotificationLevels.all[:watching], + ) + end + it "updates group user count" do expect { group.bulk_add([user.id, admin.id]) group.reload }.to change { group.user_count }.from(0).to(2) end + + it "grants title to users without one" do + group.update!(title: "Awesome") + group.bulk_add([user.id]) + + expect(user.reload.title).to eq("Awesome") + end + + it "does not overwrite an existing title" do + user.update!(title: "Already Great") + group.update!(title: "Awesome") + group.bulk_add([user.id]) + + expect(user.reload.title).to eq("Already Great") + end + + it "sets primary_group_id and flair_group_id when group is primary" do + group.update!(primary_group: true) + group.bulk_add([user.id]) + + user.reload + expect(user.primary_group_id).to eq(group.id) + expect(user.flair_group_id).to eq(group.id) + end + + it "replaces title from old primary group with new primary group title" do + old_group = Fabricate(:group, primary_group: true, title: "Old Title") + old_group.bulk_add([user.id]) + expect(user.reload.title).to eq("Old Title") + + group.update!(primary_group: true, title: "New Title") + group.bulk_add([user.id]) + + expect(user.reload.title).to eq("New Title") + expect(user.primary_group_id).to eq(group.id) + end + + it "enqueues bulk_grant_trust_level job when group grants trust level" do + group.update!(grant_trust_level: 2) + group.bulk_add([user.id, admin.id]) + + job = Jobs::BulkGrantTrustLevel.jobs.last + expect(job["args"].first["trust_level"]).to eq(2) + expect(job["args"].first["user_ids"]).to contain_exactly(user.id, admin.id) + end + + it "does not enqueue trust level job when grant_trust_level is nil" do + group.bulk_add([user.id]) + expect(Jobs::BulkGrantTrustLevel.jobs).to be_empty + end + + it "triggers user_added_to_group event for each user" do + user_ids = [user.id, admin.id] + events = DiscourseEvent.track_events(:user_added_to_group) { group.bulk_add(user_ids) } + + expect(events.size).to eq(2) + expect(events.map { |e| e[:params][0] }).to contain_exactly(user, admin) + end + + it "passes automatic flag through to the event" do + events = + DiscourseEvent.track_events(:user_added_to_group) do + group.bulk_add([user.id], automatic: true) + end + + expect(events.first[:params][2][:automatic]).to eq(true) + end + + it "sets category notification defaults" do + category = Fabricate(:category) + group.update!(watching_category_ids: [category.id]) + + group.bulk_add([user.id]) + + expect(CategoryUser.find_by(user: user, category: category).notification_level).to eq( + CategoryUser.notification_levels[:watching], + ) + end + + it "sets tag notification defaults" do + tag = Fabricate(:tag) + group.update!(watching_tags: [tag.name]) + + group.bulk_add([user.id]) + + expect(TagUser.find_by(user: user, tag: tag).notification_level).to eq( + TagUser.notification_levels[:watching], + ) + end end describe "#bulk_remove" do - it "removes multiple users from the group and doesn't error with user_ids not present" do - group.bulk_add([user.id, admin.id]) + before { group.bulk_add([user.id, admin.id]) } - group.bulk_remove([user.id, admin.id, admin.id + 1]) + it "removes multiple users and returns their IDs" do + result = group.bulk_remove([user.id, admin.id]) - expect(group.group_users.count).to be_zero + expect(result).to contain_exactly(user.id, admin.id) + expect(group.group_users.count).to eq(0) end - it "updates group user count" do - group.bulk_add([user.id, admin.id]) - expect(group.reload.user_count).to eq(2) + it "returns empty array for blank input" do + expect(group.bulk_remove([])).to eq([]) + expect(group.bulk_remove(nil)).to eq([]) + end + + it "ignores user_ids not in the group" do + result = group.bulk_remove([user.id, admin.id, admin.id + 1000]) + + expect(result).to contain_exactly(user.id, admin.id) + end + it "returns empty array when no users are in the group" do + group.bulk_remove([user.id, admin.id]) + expect(group.bulk_remove([user.id])).to eq([]) + end + + it "updates group user count" do group.bulk_remove([user.id, admin.id]) expect(group.reload.user_count).to eq(0) end + it "clears primary_group_id" do + group.update!(primary_group: true) + User.where(id: [user.id, admin.id]).update_all(primary_group_id: group.id) + + group.bulk_remove([user.id]) + + expect(user.reload.primary_group_id).to be_nil + expect(admin.reload.primary_group_id).to eq(group.id) + end + + it "clears flair_group_id" do + User.where(id: user.id).update_all(flair_group_id: group.id) + + group.bulk_remove([user.id]) + + expect(user.reload.flair_group_id).to be_nil + end + + context "when stripping title" do + before { group.update!(title: "Awesome") } + + it "clears title when user has no other titled groups or badges" do + user.update!(title: "Awesome") + + group.bulk_remove([user.id]) + + expect(user.reload.title).to be_nil + end + + it "does not clear title if it doesn't match the group title" do + user.update!(title: "Something Else") + + group.bulk_remove([user.id]) + + expect(user.reload.title).to eq("Something Else") + end + + it "falls back to next_best_title when another titled group exists" do + other_group = Fabricate(:group, title: "Other Title") + other_group.bulk_add([user.id]) + user.update!(title: "Awesome") + + group.bulk_remove([user.id]) + + expect(user.reload.title).to eq("Other Title") + end + end + + it "enqueues bulk_grant_trust_level job with recalculate flag" do + group.update!(grant_trust_level: 2) + + group.bulk_remove([user.id]) + + job = Jobs::BulkGrantTrustLevel.jobs.last + expect(job["args"].first["recalculate"]).to eq(true) + expect(job["args"].first["user_ids"]).to eq([user.id]) + end + + it "triggers user_removed_from_group event for each user" do + events = + DiscourseEvent.track_events(:user_removed_from_group) do + group.bulk_remove([user.id, admin.id]) + end + + expect(events.size).to eq(2) + expect(events.map { |e| e[:params][0] }).to contain_exactly(user, admin) + end + describe "with webhook" do fab!(:group_user_web_hook) - it "Enqueues user_removed_from_group webhook events for each group_user" do - group.bulk_add([user.id, admin.id]) - + it "enqueues webhook events for each removed user" do group.bulk_remove([user.id, admin.id]) - Jobs::EmitWebHookEvent - .jobs - .last(2) - .each do |event| - job_args = event["args"].first - expect(job_args["event_name"]).to eq("user_removed_from_group") - payload = JSON.parse(job_args["payload"]) - expect(payload["group_id"]).to eq(group.id) - expect([user.id, admin.id]).to include(payload["user_id"]) - end + + webhook_jobs = Jobs::EmitWebHookEvent.jobs.last(2) + webhook_jobs.each do |event| + job_args = event["args"].first + expect(job_args["event_name"]).to eq("user_removed_from_group") + payload = JSON.parse(job_args["payload"]) + expect(payload["group_id"]).to eq(group.id) + expect([user.id, admin.id]).to include(payload["user_id"]) + end end end end + describe "#bulk_publish_category_updates" do + fab!(:category) + + it "publishes to /categories for a single user" do + group.update!(categories: [category]) + + message = + MessageBus + .track_publish("/categories") { group.bulk_publish_category_updates([user]) } + .first + + expect(message).to be_present + expect(message.user_ids).to eq([user.id]) + end + + it "publishes a refresh for multiple users" do + group.update!(categories: [category]) + + message = + MessageBus + .track_publish("/refresh_client") { group.bulk_publish_category_updates([user, admin]) } + .first + + expect(message.data).to eq("clobber") + expect(message.user_ids).to contain_exactly(user.id, admin.id) + end + + it "does nothing when users are blank" do + group.update!(categories: [category]) + + messages = MessageBus.track_publish("/categories") { group.bulk_publish_category_updates([]) } + + expect(messages).to be_empty + end + + it "does nothing when group has no categories" do + messages = + MessageBus.track_publish("/categories") { group.bulk_publish_category_updates([user]) } + + expect(messages).to be_empty + end + end + it "Correctly updates has_messages" do group = Fabricate(:group, has_messages: true) topic = Fabricate(:private_message_topic) diff --git a/spec/models/group_user_spec.rb b/spec/models/group_user_spec.rb index d5cb3ee73afd5..697a88e4c4906 100644 --- a/spec/models/group_user_spec.rb +++ b/spec/models/group_user_spec.rb @@ -354,4 +354,102 @@ def levels Promotion.expects(:recalculate).never group_user.destroy! end + + describe ".bulk_set_category_notifications" do + fab!(:group) + fab!(:user1, :user) + fab!(:user2, :user) + fab!(:category1, :category) + fab!(:category2, :category) + + it "does nothing when group has no category notification defaults" do + expect { GroupUser.bulk_set_category_notifications(group, [user1.id]) }.not_to change { + CategoryUser.count + } + end + + it "sets category notifications for multiple users" do + group.watching_category_ids = [category1.id] + group.tracking_category_ids = [category2.id] + group.save! + + GroupUser.bulk_set_category_notifications(group, [user1.id, user2.id]) + + expect( + CategoryUser.where(user_id: user1.id, category_id: category1.id).first.notification_level, + ).to eq(CategoryUser.notification_levels[:watching]) + expect( + CategoryUser.where(user_id: user1.id, category_id: category2.id).first.notification_level, + ).to eq(CategoryUser.notification_levels[:tracking]) + expect( + CategoryUser.where(user_id: user2.id, category_id: category1.id).first.notification_level, + ).to eq(CategoryUser.notification_levels[:watching]) + end + + it "keeps the semantically higher notification level on conflict" do + CategoryUser.create!( + user_id: user1.id, + category_id: category1.id, + notification_level: CategoryUser.notification_levels[:watching], + ) + + group.tracking_category_ids = [category1.id] + group.save! + + GroupUser.bulk_set_category_notifications(group, [user1.id]) + + expect( + CategoryUser.where(user_id: user1.id, category_id: category1.id).first.notification_level, + ).to eq(CategoryUser.notification_levels[:watching]) + end + end + + describe ".bulk_set_tag_notifications" do + fab!(:group) + fab!(:user1, :user) + fab!(:user2, :user) + fab!(:tag1, :tag) + fab!(:tag2, :tag) + + it "does nothing when group has no tag notification defaults" do + expect { GroupUser.bulk_set_tag_notifications(group, [user1.id]) }.not_to change { + TagUser.count + } + end + + it "sets tag notifications for multiple users" do + group.watching_tags = [tag1.name] + group.tracking_tags = [tag2.name] + group.save! + + GroupUser.bulk_set_tag_notifications(group, [user1.id, user2.id]) + + expect(TagUser.where(user_id: user1.id, tag_id: tag1.id).first.notification_level).to eq( + TagUser.notification_levels[:watching], + ) + expect(TagUser.where(user_id: user1.id, tag_id: tag2.id).first.notification_level).to eq( + TagUser.notification_levels[:tracking], + ) + expect(TagUser.where(user_id: user2.id, tag_id: tag1.id).first.notification_level).to eq( + TagUser.notification_levels[:watching], + ) + end + + it "keeps the semantically higher notification level on conflict" do + TagUser.create!( + user_id: user1.id, + tag_id: tag1.id, + notification_level: TagUser.notification_levels[:watching], + ) + + group.tracking_tags = [tag1.name] + group.save! + + GroupUser.bulk_set_tag_notifications(group, [user1.id]) + + expect(TagUser.where(user_id: user1.id, tag_id: tag1.id).first.notification_level).to eq( + TagUser.notification_levels[:watching], + ) + end + end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index c52d9cbb82fe5..254c5af4b11af 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -2066,6 +2066,8 @@ def hash(password, salt, algorithm = UserPassword::TARGET_PASSWORD_ALGORITHM) end it "get attributes from the group" do + Jobs.run_immediately! + user = Fabricate.build( :user, diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index 07779583e1854..99dbd19abbce5 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -732,7 +732,7 @@ def expect_type_to_return_right_groups(type, expected_group_ids) freeze_time 1.day.from_now 4.times { group.add(Fabricate(:user)) } - usernames = group.users.map { |m| m.username }.sort + usernames = group.reload.users.map { |m| m.username }.sort get "/groups/#{group.name}/members.json", params: { limit: 3, asc: true } diff --git a/spec/serializers/current_user_serializer_spec.rb b/spec/serializers/current_user_serializer_spec.rb index e933fbaf1cd61..675a4796e9715 100644 --- a/spec/serializers/current_user_serializer_spec.rb +++ b/spec/serializers/current_user_serializer_spec.rb @@ -108,18 +108,16 @@ describe "#groups" do it "should only show visible groups" do - Fabricate.build(:group, visibility_level: Group.visibility_levels[:public]) - hidden_group = Fabricate.build(:group, visibility_level: Group.visibility_levels[:owners]) + Fabricate(:group, visibility_level: Group.visibility_levels[:public]) + hidden_group = Fabricate(:group, visibility_level: Group.visibility_levels[:owners]) public_group = - Fabricate.build( + Fabricate( :group, visibility_level: Group.visibility_levels[:public], name: "UppercaseGroupName", ) hidden_group.add(user) - hidden_group.save! public_group.add(user) - public_group.save! payload = serializer.as_json expect(payload[:groups]).to contain_exactly(