From b256d462bf58d05c4db9f8ce9f70bde72aeb6f13 Mon Sep 17 00:00:00 2001 From: Roman Rizzi Date: Tue, 16 Jul 2019 11:33:04 -0300 Subject: [PATCH] DEV: use group ids to allow assign on groups (#38) * DEV: use group ids to allow assign on groups * FIX: Dinamically sets the default value to support older versions --- config/settings.yml | 2 +- jobs/scheduled/enqueue_reminders.rb | 3 +-- lib/topic_assigner.rb | 2 +- plugin.rb | 31 ++++++++++++++++++++----- spec/models/group_spec.rb | 21 ++++++++--------- spec/requests/assign_controller_spec.rb | 16 +++++++++++-- 6 files changed, 52 insertions(+), 23 deletions(-) diff --git a/config/settings.yml b/config/settings.yml index a0aedb0..17bdc10 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -31,6 +31,6 @@ plugins: client: true type: group_list list_type: compact - default: 'staff' + default: '' allow_any: false refresh: true diff --git a/jobs/scheduled/enqueue_reminders.rb b/jobs/scheduled/enqueue_reminders.rb index ed364f9..52d0aa4 100644 --- a/jobs/scheduled/enqueue_reminders.rb +++ b/jobs/scheduled/enqueue_reminders.rb @@ -16,8 +16,7 @@ module Jobs end def allowed_group_ids - allowed_groups = SiteSetting.assign_allowed_on_groups.split('|') - Group.where(name: allowed_groups).pluck(:id).join(',') + Group.assign_allowed_groups.pluck(:id).join(',') end def user_ids diff --git a/lib/topic_assigner.rb b/lib/topic_assigner.rb index 563fbed..6372e46 100644 --- a/lib/topic_assigner.rb +++ b/lib/topic_assigner.rb @@ -96,7 +96,7 @@ class ::TopicAssigner 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 diff --git a/plugin.rb b/plugin.rb index 8b871a5..92533d9 100644 --- a/plugin.rb +++ b/plugin.rb @@ -48,11 +48,29 @@ after_initialize do =end reviewable_api_enabled = defined?(Reviewable) + # TODO: Remove this once 2.4 becomes the new stable. + current_version = ActiveRecord::Migrator.current_version + min_version = 201_907_081_533_31 + above_min_version = current_version >= min_version + + # Dinamically sets the default value, supports older versions. + default_allowed_group_value = above_min_version ? Group::AUTO_GROUPS[:staff] : 'staff' + allowed_group_values = SiteSetting.assign_allowed_on_groups.split('|') + allowed_group_values << default_allowed_group_value if allowed_group_values.empty? + SiteSetting.assign_allowed_on_groups = allowed_group_values.join('|') + + attribute = above_min_version ? 'id' : 'name' + + add_class_method(:group, :assign_allowed_groups) do + allowed_groups = SiteSetting.assign_allowed_on_groups.split('|') + where("groups.#{attribute} IN (?)", allowed_groups) + end + add_to_class(:user, :can_assign?) do @can_assign ||= begin allowed_groups = SiteSetting.assign_allowed_on_groups.split('|').compact - allowed_groups.present? && groups.where('name in (?)', allowed_groups).exists? ? + allowed_groups.present? && groups.where("groups.#{attribute} in (?)", allowed_groups).exists? ? :true : :false end @can_assign == :true @@ -62,21 +80,22 @@ after_initialize do add_class_method(:user, :assign_allowed) do allowed_groups = SiteSetting.assign_allowed_on_groups.split('|') - where('users.id IN ( + 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) + WHERE groups.#{attribute} IN (?) + )", allowed_groups) end add_model_callback(Group, :before_update) do - if name_changed? + if !above_min_version && name_changed? SiteSetting.assign_allowed_on_groups = SiteSetting.assign_allowed_on_groups.gsub(name_was, name) end end add_model_callback(Group, :before_destroy) do - new_setting = SiteSetting.assign_allowed_on_groups.gsub(/#{name}[|]?/, '') + to_remove = above_min_version ? id : name + new_setting = SiteSetting.assign_allowed_on_groups.gsub(/#{to_remove}[|]?/, '') new_setting = new_setting.chomp('|') if new_setting.ends_with?('|') SiteSetting.assign_allowed_on_groups = new_setting end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index a269856..16241da 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -10,19 +10,17 @@ RSpec.describe Group do SiteSetting.assign_enabled = true end - it 'updates the site setting when the group name changes' do - SiteSetting.assign_allowed_on_groups = "#{group.name}|staff|moderators" - different_name = 'different_name' - - group.update!(name: different_name) - - expect(SiteSetting.assign_allowed_on_groups).to eq "#{different_name}|staff|moderators" + let(:above_min_version) do + current_version = ActiveRecord::Migrator.current_version + min_version = 201_907_081_533_31 + current_version >= min_version end - let(:removed_group_setting) { 'staff|moderators' } + let(:removed_group_setting) { above_min_version ? '3|4' : 'staff|moderators' } + let(:group_attribute) { above_min_version ? group.id : group.name } it 'removes the group from the setting when the group gets destroyed' do - SiteSetting.assign_allowed_on_groups = "#{group.name}|staff|moderators" + SiteSetting.assign_allowed_on_groups = "#{group_attribute}|#{removed_group_setting}" group.destroy! @@ -30,7 +28,7 @@ RSpec.describe Group do end it 'removes the group from the setting when this is the last one on the list' do - SiteSetting.assign_allowed_on_groups = "staff|moderators|#{group.name}" + SiteSetting.assign_allowed_on_groups = "#{removed_group_setting}|#{group_attribute}" group.destroy! @@ -38,7 +36,8 @@ RSpec.describe Group do end it 'removes the group from the list when it is on the middle of the list' do - SiteSetting.assign_allowed_on_groups = "staff|#{group.name}|moderators" + allowed_groups = above_min_version ? "3|#{group_attribute}|4" : "staff|#{group_attribute}|moderators" + SiteSetting.assign_allowed_on_groups = allowed_groups group.destroy! diff --git a/spec/requests/assign_controller_spec.rb b/spec/requests/assign_controller_spec.rb index d4fb776..bd964db 100644 --- a/spec/requests/assign_controller_spec.rb +++ b/spec/requests/assign_controller_spec.rb @@ -11,6 +11,12 @@ RSpec.describe DiscourseAssign::AssignController do let(:post) { Fabricate(:post) } let(:user2) { Fabricate(:active_user) } + let(:above_min_version) do + current_version = ActiveRecord::Migrator.current_version + min_version = 201_907_081_533_31 + current_version >= min_version + end + describe 'only allow users from allowed groups' do before { sign_in(user2) } @@ -31,7 +37,13 @@ RSpec.describe DiscourseAssign::AssignController do allowed_group = Group.find_by(name: 'everyone') user2.groups << allowed_group user2.groups << default_allowed_group - SiteSetting.assign_allowed_on_groups = 'staff|everyone' + defaults = if above_min_version + "#{default_allowed_group.id}|#{allowed_group.id}" + else + "#{default_allowed_group.name}|#{allowed_group.name}" + end + + SiteSetting.assign_allowed_on_groups = defaults TopicAssigner.new(post.topic, user).assign(user2) get '/assign/suggestions.json' @@ -43,7 +55,7 @@ RSpec.describe DiscourseAssign::AssignController do it 'does not include users from disallowed groups' do allowed_group = Group.find_by(name: 'everyone') user2.groups << allowed_group - SiteSetting.assign_allowed_on_groups = 'staff' + SiteSetting.assign_allowed_on_groups = above_min_version ? default_allowed_group.id.to_s : default_allowed_group.name TopicAssigner.new(post.topic, user).assign(user2) get '/assign/suggestions.json'