diff --git a/config/settings.yml b/config/settings.yml index a0aedb0..f67447d 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 d1038e8..8739013 100644 --- a/jobs/scheduled/enqueue_reminders.rb +++ b/jobs/scheduled/enqueue_reminders.rb @@ -17,7 +17,16 @@ module Jobs def allowed_group_ids allowed_groups = SiteSetting.assign_allowed_on_groups.split('|') - Group.where(name: allowed_groups).pluck(:id).join(',') + 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 end def user_ids @@ -36,10 +45,11 @@ module Jobs ON topic_custom_fields.value::INT = user_frequency.user_id AND user_frequency.name = '#{PendingAssignsReminder::REMINDERS_FREQUENCY}' - INNER JOIN group_users ON topic_custom_fields.value::INT = group_users.user_id + 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 topics ON topics.id = topic_custom_fields.topic_id AND (topics.deleted_at IS NULL) - WHERE group_users.group_id IN (#{allowed_group_ids}) + WHERE (#{membership_condition}) AND #{frequency} > 0 AND ( last_reminder.value IS NULL OR diff --git a/lib/topic_assigner.rb b/lib/topic_assigner.rb index d48bfa0..ed83a95 100644 --- a/lib/topic_assigner.rb +++ b/lib/topic_assigner.rb @@ -18,8 +18,7 @@ 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 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 + WHERE ( #{staff_mention} ) AND tc.value IS NULL AND NOT t.closed AND t.deleted_at IS NULL GROUP BY p.topic_id SQL @@ -93,10 +92,8 @@ 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 - .joins(:groups).where(groups: { name: allowed_groups }) + .assign_allowed .where('username_lower IN (?)', mentions.map(&:downcase)) .first end @@ -116,7 +113,6 @@ 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 8b871a5..537ac01 100644 --- a/plugin.rb +++ b/plugin.rb @@ -49,6 +49,8 @@ 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 @@ -62,11 +64,18 @@ after_initialize do add_class_method(:user, :assign_allowed) do allowed_groups = SiteSetting.assign_allowed_on_groups.split('|') - 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) + + 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 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 61361d8..209cfb4 100644 --- a/spec/jobs/scheduled/enqueue_reminders_spec.rb +++ b/spec/jobs/scheduled/enqueue_reminders_spec.rb @@ -3,15 +3,24 @@ require 'rails_helper' RSpec.describe Jobs::EnqueueReminders do - let(:assign_allowed_group) { Group.find_by(name: 'staff') } + let(:assign_allowed_group) { Fabricate(:group) } 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 new file mode 100644 index 0000000..d1c76c2 --- /dev/null +++ b/spec/models/user_spec.rb @@ -0,0 +1,60 @@ +# 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