Revert "Staff is always allowed to assign (#36)"

Revert “Staff is always allowed to assign (#36)”

This reverts commit bb200f5fbd68ef2bf27175b0bc25dc378d793b5d. Unfortunately that commit broke assign

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

GitHub sha: e4be0f02

1 Like