diff --git a/config/settings.yml b/config/settings.yml index f67447d..a0aedb0 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -30,7 +30,7 @@ plugins: assign_allowed_on_groups: client: true type: group_list - default: '' list_type: compact + default: 'staff' allow_any: false refresh: true diff --git a/jobs/scheduled/enqueue_reminders.rb b/jobs/scheduled/enqueue_reminders.rb index 8739013..d1038e8 100644 --- a/jobs/scheduled/enqueue_reminders.rb +++ b/jobs/scheduled/enqueue_reminders.rb @@ -17,16 +17,7 @@ module Jobs def allowed_group_ids allowed_groups = SiteSetting.assign_allowed_on_groups.split('|') - return [] if allowed_groups.empty? - - Group.where(name: allowed_groups).pluck(:id) - end - - def membership_condition - group_ids = allowed_group_ids.join(',') - condition = 'users.admin OR users.moderator' - condition += " OR group_users.group_id IN (#{group_ids})" if group_ids.present? - condition + Group.where(name: allowed_groups).pluck(:id).join(',') end def user_ids @@ -45,11 +36,10 @@ module Jobs ON topic_custom_fields.value::INT = user_frequency.user_id AND user_frequency.name = '#{PendingAssignsReminder::REMINDERS_FREQUENCY}' - INNER JOIN users ON topic_custom_fields.value::INT = users.id - LEFT OUTER JOIN group_users ON topic_custom_fields.value::INT = group_users.user_id + INNER JOIN group_users ON topic_custom_fields.value::INT = group_users.user_id INNER JOIN topics ON topics.id = topic_custom_fields.topic_id AND (topics.deleted_at IS NULL) - WHERE (#{membership_condition}) + WHERE group_users.group_id IN (#{allowed_group_ids}) AND #{frequency} > 0 AND ( last_reminder.value IS NULL OR diff --git a/lib/topic_assigner.rb b/lib/topic_assigner.rb index ed83a95..d48bfa0 100644 --- a/lib/topic_assigner.rb +++ b/lib/topic_assigner.rb @@ -18,7 +18,8 @@ class ::TopicAssigner FROM posts p JOIN topics t ON t.id = p.topic_id LEFT JOIN topic_custom_fields tc ON tc.name = '#{ASSIGNED_TO_ID}' AND tc.topic_id = p.topic_id - WHERE ( #{staff_mention} ) AND tc.value IS NULL AND NOT t.closed AND t.deleted_at IS NULL + WHERE p.user_id IN (SELECT id FROM users WHERE moderator OR admin) AND + ( #{staff_mention} ) AND tc.value IS NULL AND NOT t.closed AND t.deleted_at IS NULL GROUP BY p.topic_id SQL @@ -92,8 +93,10 @@ class ::TopicAssigner def self.mentioned_staff(post) mentions = post.raw_mentions if mentions.present? + allowed_groups = SiteSetting.assign_allowed_on_groups.split('|') + User.human_users - .assign_allowed + .joins(:groups).where(groups: { name: allowed_groups }) .where('username_lower IN (?)', mentions.map(&:downcase)) .first end @@ -113,6 +116,7 @@ class ::TopicAssigner assigned_total = TopicCustomField .joins(:topic) + .where(topics: { deleted_at: nil }) .where(name: ASSIGNED_TO_ID) .where(value: user.id) .count diff --git a/plugin.rb b/plugin.rb index 537ac01..8b871a5 100644 --- a/plugin.rb +++ b/plugin.rb @@ -49,8 +49,6 @@ after_initialize do reviewable_api_enabled = defined?(Reviewable) add_to_class(:user, :can_assign?) do - return true if staff? - @can_assign ||= begin allowed_groups = SiteSetting.assign_allowed_on_groups.split('|').compact @@ -64,18 +62,11 @@ after_initialize do add_class_method(:user, :assign_allowed) do allowed_groups = SiteSetting.assign_allowed_on_groups.split('|') - - if allowed_groups.present? - real.where(' - users.admin OR users.moderator OR - users.id IN ( - SELECT user_id FROM group_users - INNER JOIN groups ON group_users.group_id = groups.id - WHERE groups.name IN (?) - )', allowed_groups) - else - real.where('users.admin OR users.moderator') - end + where('users.id IN ( + SELECT user_id FROM group_users + INNER JOIN groups ON group_users.group_id = groups.id + WHERE groups.name IN (?) + )', allowed_groups) end add_model_callback(Group, :before_update) do diff --git a/spec/jobs/scheduled/enqueue_reminders_spec.rb b/spec/jobs/scheduled/enqueue_reminders_spec.rb index 209cfb4..61361d8 100644 --- a/spec/jobs/scheduled/enqueue_reminders_spec.rb +++ b/spec/jobs/scheduled/enqueue_reminders_spec.rb @@ -3,24 +3,15 @@ require 'rails_helper' RSpec.describe Jobs::EnqueueReminders do - let(:assign_allowed_group) { Fabricate(:group) } + let(:assign_allowed_group) { Group.find_by(name: 'staff') } let(:user) { Fabricate(:user, groups: [assign_allowed_group]) } before do - SiteSetting.assign_allowed_on_groups = assign_allowed_group.name SiteSetting.remind_assigns_frequency = RemindAssignsFrequencySiteSettings::MONTHLY_MINUTES SiteSetting.assign_enabled = true end describe '#execute' do - it 'enqueues a reminder when the user is an admin' do - SiteSetting.assign_allowed_on_groups = '' - admin = Fabricate(:admin) - assign_multiple_tasks_to(admin) - - assert_reminders_enqueued(1) - end - it 'does not enqueue reminders when there are no assigned tasks' do assert_reminders_enqueued(0) end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb deleted file mode 100644 index d1c76c2..0000000 --- a/spec/models/user_spec.rb +++ /dev/null @@ -1,60 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -RSpec.describe User do - let(:group) { Fabricate(:group) } - - before do - SiteSetting.assign_enabled = true - SiteSetting.assign_allowed_on_groups = group.name - end - - describe '.assign_allowed' do - it 'retrieves the user when is a member of an allowed group' do - user = Fabricate(:user) - group.add(user) - - expect(User.assign_allowed).to include(user) - end - - it "doesn't retrieve the user when is not a member of an allowed group" do - non_assign_allowed_user = Fabricate(:user) - - expect(User.assign_allowed).not_to include(non_assign_allowed_user) - end - - it 'retrieves the user if is an admin' do - admin = Fabricate(:admin) - - expect(User.assign_allowed).to include(admin) - end - - it 'retrieves the user if is an moderator' do - moderator = Fabricate(:moderator) - - expect(User.assign_allowed).to include(moderator) - end - end - - describe '#can_assign?' do - it 'allows member of allowed groups to assign' do - user = Fabricate.build(:user) - group.add(user) - - expect(user.can_assign?).to eq true - end - - it "doesn't allow non allowed users to assign" do - user = Fabricate.build(:user) - - expect(user.can_assign?).to eq false - end - - it 'allows staff members to assign' do - admin = Fabricate.build(:admin) - - expect(admin.can_assign?).to eq true - end - end -end