Revert "DEV: use group ids to allow assign on groups (#38)" (#40)

This reverts commit b256d462bf.
This commit is contained in:
Roman Rizzi 2019-07-16 11:59:33 -03:00 committed by GitHub
parent b256d462bf
commit 8f92c5f9bd
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 23 additions and 52 deletions

View File

@ -31,6 +31,6 @@ plugins:
client: true client: true
type: group_list type: group_list
list_type: compact list_type: compact
default: '' default: 'staff'
allow_any: false allow_any: false
refresh: true refresh: true

View File

@ -16,7 +16,8 @@ module Jobs
end end
def allowed_group_ids def allowed_group_ids
Group.assign_allowed_groups.pluck(:id).join(',') allowed_groups = SiteSetting.assign_allowed_on_groups.split('|')
Group.where(name: allowed_groups).pluck(:id).join(',')
end end
def user_ids def user_ids

View File

@ -96,7 +96,7 @@ class ::TopicAssigner
allowed_groups = SiteSetting.assign_allowed_on_groups.split('|') allowed_groups = SiteSetting.assign_allowed_on_groups.split('|')
User.human_users User.human_users
.assign_allowed .joins(:groups).where(groups: { name: allowed_groups })
.where('username_lower IN (?)', mentions.map(&:downcase)) .where('username_lower IN (?)', mentions.map(&:downcase))
.first .first
end end

View File

@ -48,29 +48,11 @@ after_initialize do
=end =end
reviewable_api_enabled = defined?(Reviewable) 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 add_to_class(:user, :can_assign?) do
@can_assign ||= @can_assign ||=
begin begin
allowed_groups = SiteSetting.assign_allowed_on_groups.split('|').compact allowed_groups = SiteSetting.assign_allowed_on_groups.split('|').compact
allowed_groups.present? && groups.where("groups.#{attribute} in (?)", allowed_groups).exists? ? allowed_groups.present? && groups.where('name in (?)', allowed_groups).exists? ?
:true : :false :true : :false
end end
@can_assign == :true @can_assign == :true
@ -80,22 +62,21 @@ after_initialize do
add_class_method(:user, :assign_allowed) do add_class_method(:user, :assign_allowed) do
allowed_groups = SiteSetting.assign_allowed_on_groups.split('|') allowed_groups = SiteSetting.assign_allowed_on_groups.split('|')
where("users.id IN ( where('users.id IN (
SELECT user_id FROM group_users SELECT user_id FROM group_users
INNER JOIN groups ON group_users.group_id = groups.id INNER JOIN groups ON group_users.group_id = groups.id
WHERE groups.#{attribute} IN (?) WHERE groups.name IN (?)
)", allowed_groups) )', allowed_groups)
end end
add_model_callback(Group, :before_update) do add_model_callback(Group, :before_update) do
if !above_min_version && name_changed? if name_changed?
SiteSetting.assign_allowed_on_groups = SiteSetting.assign_allowed_on_groups.gsub(name_was, name) SiteSetting.assign_allowed_on_groups = SiteSetting.assign_allowed_on_groups.gsub(name_was, name)
end end
end end
add_model_callback(Group, :before_destroy) do add_model_callback(Group, :before_destroy) do
to_remove = above_min_version ? id : name new_setting = SiteSetting.assign_allowed_on_groups.gsub(/#{name}[|]?/, '')
new_setting = SiteSetting.assign_allowed_on_groups.gsub(/#{to_remove}[|]?/, '')
new_setting = new_setting.chomp('|') if new_setting.ends_with?('|') new_setting = new_setting.chomp('|') if new_setting.ends_with?('|')
SiteSetting.assign_allowed_on_groups = new_setting SiteSetting.assign_allowed_on_groups = new_setting
end end

View File

@ -10,17 +10,19 @@ RSpec.describe Group do
SiteSetting.assign_enabled = true SiteSetting.assign_enabled = true
end end
let(:above_min_version) do it 'updates the site setting when the group name changes' do
current_version = ActiveRecord::Migrator.current_version SiteSetting.assign_allowed_on_groups = "#{group.name}|staff|moderators"
min_version = 201_907_081_533_31 different_name = 'different_name'
current_version >= min_version
group.update!(name: different_name)
expect(SiteSetting.assign_allowed_on_groups).to eq "#{different_name}|staff|moderators"
end end
let(:removed_group_setting) { above_min_version ? '3|4' : 'staff|moderators' } let(:removed_group_setting) { '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 it 'removes the group from the setting when the group gets destroyed' do
SiteSetting.assign_allowed_on_groups = "#{group_attribute}|#{removed_group_setting}" SiteSetting.assign_allowed_on_groups = "#{group.name}|staff|moderators"
group.destroy! group.destroy!
@ -28,7 +30,7 @@ RSpec.describe Group do
end end
it 'removes the group from the setting when this is the last one on the list' do it 'removes the group from the setting when this is the last one on the list' do
SiteSetting.assign_allowed_on_groups = "#{removed_group_setting}|#{group_attribute}" SiteSetting.assign_allowed_on_groups = "staff|moderators|#{group.name}"
group.destroy! group.destroy!
@ -36,8 +38,7 @@ RSpec.describe Group do
end end
it 'removes the group from the list when it is on the middle of the list' do it 'removes the group from the list when it is on the middle of the list' do
allowed_groups = above_min_version ? "3|#{group_attribute}|4" : "staff|#{group_attribute}|moderators" SiteSetting.assign_allowed_on_groups = "staff|#{group.name}|moderators"
SiteSetting.assign_allowed_on_groups = allowed_groups
group.destroy! group.destroy!

View File

@ -11,12 +11,6 @@ RSpec.describe DiscourseAssign::AssignController do
let(:post) { Fabricate(:post) } let(:post) { Fabricate(:post) }
let(:user2) { Fabricate(:active_user) } 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 describe 'only allow users from allowed groups' do
before { sign_in(user2) } before { sign_in(user2) }
@ -37,13 +31,7 @@ RSpec.describe DiscourseAssign::AssignController do
allowed_group = Group.find_by(name: 'everyone') allowed_group = Group.find_by(name: 'everyone')
user2.groups << allowed_group user2.groups << allowed_group
user2.groups << default_allowed_group user2.groups << default_allowed_group
defaults = if above_min_version SiteSetting.assign_allowed_on_groups = 'staff|everyone'
"#{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) TopicAssigner.new(post.topic, user).assign(user2)
get '/assign/suggestions.json' get '/assign/suggestions.json'
@ -55,7 +43,7 @@ RSpec.describe DiscourseAssign::AssignController do
it 'does not include users from disallowed groups' do it 'does not include users from disallowed groups' do
allowed_group = Group.find_by(name: 'everyone') allowed_group = Group.find_by(name: 'everyone')
user2.groups << allowed_group user2.groups << allowed_group
SiteSetting.assign_allowed_on_groups = above_min_version ? default_allowed_group.id.to_s : default_allowed_group.name SiteSetting.assign_allowed_on_groups = 'staff'
TopicAssigner.new(post.topic, user).assign(user2) TopicAssigner.new(post.topic, user).assign(user2)
get '/assign/suggestions.json' get '/assign/suggestions.json'