Feature: Allow assign on groups (#31)

* Feature: Allow assign on groups

* Use type: group_list option

* Track group changes and update the setting accordingly. Restrict reminders frequency to assign allowed users instead of staff
This commit is contained in:
Roman Rizzi 2019-06-04 09:21:33 -03:00 committed by GitHub
parent 8536795da3
commit f312ece4a9
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
22 changed files with 241 additions and 69 deletions

View File

@ -1,13 +1,13 @@
# frozen_string_literal: true
module DiscourseAssign
class AssignController < Admin::AdminController
before_action :ensure_logged_in
class AssignController < ApplicationController
requires_login
before_action :ensure_logged_in, :ensure_assign_allowed
def suggestions
users = [current_user]
users += User
.where('admin OR moderator')
.where('users.id <> ?', current_user.id)
.joins(<<~SQL
JOIN(
@ -18,7 +18,9 @@ module DiscourseAssign
HAVING COUNT(*) < #{SiteSetting.max_assigned_topics}
) as X ON X.user_id = users.id
SQL
).order('X.last_assigned DESC')
)
.assign_allowed
.order('X.last_assigned DESC')
.limit(6)
render json: ActiveModel::ArraySerializer.new(users,
@ -88,5 +90,9 @@ module DiscourseAssign
{ error: I18n.t('discourse_assign.too_many_assigns', username: user.username, max: max) }
end
end
def ensure_assign_allowed
raise Discourse::InvalidAccess.new unless current_user.can_assign?
end
end
end

View File

@ -3,7 +3,7 @@ import { getOwner } from "discourse-common/lib/get-owner";
export default {
shouldRender(args, component) {
const needsButton =
component.currentUser && component.currentUser.get("staff");
component.currentUser && component.currentUser.get("can_assign");
return (
needsButton &&
(!component.get("site.mobileView") || args.topic.get("isPrivateMessage"))

View File

@ -1,4 +1,4 @@
{{#if currentUser.staff}}
{{#if currentUser.can_assign}}
{{#link-to 'userActivity.assigned'}}
{{d-icon "user-plus"}} {{i18n 'discourse_assign.assigned'}}
{{/link-to}}

View File

@ -1,6 +1,6 @@
export function shouldShowAssigned(args, component) {
const needsButton =
component.currentUser && component.currentUser.get("staff");
component.currentUser && component.currentUser.get("can_assign");
return (
needsButton &&
(!component.get("site.mobileView") || args.model.get("isPrivateMessage"))

View File

@ -1,5 +1,5 @@
export default {
shouldRender(args, component) {
return component.currentUser && component.currentUser.get("staff");
return component.currentUser && component.currentUser.get("can_assign");
}
};

View File

@ -3,6 +3,7 @@ import { ajax } from "discourse/lib/ajax";
import { popupAjaxError } from "discourse/lib/ajax-error";
export default Ember.Controller.extend({
taskActions: Ember.inject.service(),
assignSuggestions: function() {
ajax("/assign/suggestions").then(users => {
this.set("assignSuggestions", users);
@ -23,6 +24,7 @@ export default Ember.Controller.extend({
actions: {
assignUser(user) {
this.set("model.username", user.username);
this.set("model.allowedGroups", this.get("taskActions").allowedGroups);
this.send("assign");
},

View File

@ -17,7 +17,7 @@ function modifySelectKit(api) {
api
.modifySelectKit("topic-footer-mobile-dropdown")
.modifyContent((context, existingContent) => {
if (context.get("currentUser.staff")) {
if (context.get("currentUser.can_assign")) {
const hasAssignement = context.get("topic.assigned_to_user");
const button = {
id: ACTION_ID,
@ -31,7 +31,7 @@ function modifySelectKit(api) {
return existingContent;
})
.onSelect((context, value) => {
if (!context.get("currentUser.staff") || value !== ACTION_ID) {
if (!context.get("currentUser.can_assign") || value !== ACTION_ID) {
return;
}
@ -157,7 +157,7 @@ function initialize(api) {
});
api.addUserMenuGlyph(widget => {
if (widget.currentUser && widget.currentUser.get("staff")) {
if (widget.currentUser && widget.currentUser.get("can_assign")) {
return {
label: "discourse_assign.assigned",
className: "assigned",

View File

@ -1,7 +1,16 @@
import { ajax } from "discourse/lib/ajax";
import showModal from "discourse/lib/show-modal";
import { getOwner } from "discourse-common/lib/get-owner";
export default Ember.Service.extend({
init() {
this._super(...arguments);
this.allowedGroups = getOwner(this)
.lookup("site-settings:main")
.assign_allowed_on_groups.split("|");
},
unassign(topicId) {
return ajax("/assign/unassign", {
type: "PUT",
@ -13,7 +22,8 @@ export default Ember.Service.extend({
return showModal("assign-user", {
model: {
topic: topic,
username: topic.get("assigned_to_user.username")
username: topic.get("assigned_to_user.username"),
allowedGroups: this.allowedGroups
}
});
}

View File

@ -4,6 +4,7 @@
single=true
allowAny=false
group="staff"
groupMembersOf=model.allowedGroups
excludeCurrentUser=false
includeMentionableGroups=false
hasGroups=false

View File

@ -15,6 +15,7 @@ en:
remind_assigns: "Remind users about pending assigns."
remind_assigns_frequency: "Frequency for reminding users about assigned topics."
max_assigned_topics: "Maximum number of topics that can be assigned to a user."
assign_allowed_on_groups: "Groups that are allowed to assign topics."
discourse_assign:
assigned_to: "Topic assigned to @%{username}"
unassigned: "Topic was unassigned"

View File

@ -27,3 +27,10 @@ plugins:
client: true
default: 10
min: 1
assign_allowed_on_groups:
client: true
type: group_list
list_type: compact
default: 'staff'
allow_any: false
refresh: true

View File

@ -15,6 +15,11 @@ module Jobs
SiteSetting.remind_assigns_frequency.nil? || !SiteSetting.assign_enabled?
end
def allowed_group_ids
allowed_groups = SiteSetting.assign_allowed_on_groups.split('|')
Group.where(name: allowed_groups).pluck(:id).join(',')
end
def user_ids
global_frequency = SiteSetting.remind_assigns_frequency
frequency = ActiveRecord::Base.sanitize_sql("COALESCE(user_frequency.value, '#{global_frequency}')::INT")
@ -31,10 +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
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 (users.moderator OR users.admin)
WHERE group_users.group_id IN (#{allowed_group_ids})
AND #{frequency} > 0
AND (
last_reminder.value IS NULL OR

View File

@ -8,20 +8,20 @@ class ::TopicAssigner
ASSIGNED_BY_ID = 'assigned_by_id'
def self.backfill_auto_assign
staff_mention = User.where('moderator OR admin')
staff_mention = User.assign_allowed
.pluck('username')
.map { |name| "p.cooked ILIKE '%mention%@#{name}%'" }
.map! { |name| "p.cooked ILIKE '%mention%@#{name}%'" }
.join(' OR ')
sql = <<SQL
SELECT p.topic_id, MAX(post_number) post_number
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
GROUP BY p.topic_id
SQL
sql = <<~SQL
SELECT p.topic_id, MAX(post_number) post_number
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
GROUP BY p.topic_id
SQL
assigned = 0
puts
@ -54,7 +54,7 @@ SQL
def self.auto_assign(post, force: false)
return unless SiteSetting.assigns_by_staff_mention
if post.user && post.topic && post.user.staff?
if post.user && post.topic && post.user.can_assign?
can_assign = force || post.topic.custom_fields[ASSIGNED_TO_ID].nil?
assign_other = assign_other_passes?(post) && mentioned_staff(post)
@ -72,12 +72,14 @@ SQL
end
def self.is_last_staff_post?(post)
allowed_user_ids = User.assign_allowed.pluck(:id).join(',')
sql = <<~SQL
SELECT 1 FROM posts p
JOIN users u ON u.id = p.user_id AND (moderator OR admin)
JOIN users u ON u.id = p.user_id
WHERE p.deleted_at IS NULL AND p.topic_id = :topic_id
AND u.id IN (#{allowed_user_ids})
having max(post_number) = :post_number
SQL
args = {
@ -85,19 +87,16 @@ SQL
post_number: post.post_number
}
# TODO post 2.1 release remove
if defined?(DB)
DB.exec(sql, args) == 1
else
Post.exec_sql(sql, args).to_a.length == 1
end
DB.exec(sql, args) == 1
end
def self.mentioned_staff(post)
mentions = post.raw_mentions
if mentions.present?
User.where('moderator OR admin')
.human_users
allowed_groups = SiteSetting.assign_allowed_on_groups.split('|')
User.human_users
.joins(:groups).where(groups: { name: allowed_groups })
.where('username_lower IN (?)', mentions.map(&:downcase))
.first
end
@ -108,8 +107,8 @@ SQL
@topic = topic
end
def staff_ids
User.real.staff.pluck(:id)
def allowed_user_ids
User.assign_allowed.pluck(:id)
end
def can_assign_to?(user)
@ -143,7 +142,7 @@ SQL
topic_id: @topic.id,
assigned_to: BasicUserSerializer.new(assign_to, root: false).as_json
},
user_ids: staff_ids
user_ids: allowed_user_ids
)
publish_topic_tracking_state(@topic, assign_to.id)
@ -274,7 +273,7 @@ SQL
type: 'unassigned',
topic_id: @topic.id,
},
user_ids: staff_ids
user_ids: allowed_user_ids
)
publish_topic_tracking_state(@topic, assigned_user.id)

View File

@ -48,6 +48,34 @@ after_initialize do
=end
reviewable_api_enabled = defined?(Reviewable)
add_to_class(:user, :can_assign?) do
allowed_groups = SiteSetting.assign_allowed_on_groups.split('|')
(groups.pluck(:name) & allowed_groups).present?
end
add_to_class(:guardian, :can_assign?) { user && user.can_assign? }
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)
end
add_model_callback(Group, :before_update) do
if 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}[|]?/, '')
new_setting = new_setting.chomp('|') if new_setting.ends_with?('|')
SiteSetting.assign_allowed_on_groups = new_setting
end
# Raise an invalid access error if a user tries to act on something
# not assigned to them
DiscourseEvent.on(:before_staff_flag_action) do |args|
@ -88,8 +116,8 @@ after_initialize do
TopicList.on_preload do |topics, topic_list|
if SiteSetting.assign_enabled?
is_staff = topic_list.current_user && topic_list.current_user.staff?
allowed_access = SiteSetting.assigns_public || is_staff
can_assign = topic_list.current_user && topic_list.current_user.can_assign?
allowed_access = SiteSetting.assigns_public || can_assign
if allowed_access && topics.length > 0
users = User.where("users.id in (
@ -114,7 +142,7 @@ after_initialize do
require_dependency 'topic_query'
TopicQuery.add_custom_filter(:assigned) do |results, topic_query|
if topic_query.guardian.is_staff? || SiteSetting.assigns_public
if topic_query.guardian.can_assign? || SiteSetting.assigns_public
username = topic_query.options[:assigned]
user_id = topic_query.guardian.user.id if username == "me"
@ -202,10 +230,6 @@ after_initialize do
@assigned_to_user = assigned_to_user
end
add_to_serializer(:topic_list_item, 'include_assigned_to_user?') do
(SiteSetting.assigns_public || scope.is_staff?) && object.assigned_to_user
end
add_to_serializer(:topic_list, :assigned_messages_count) do
TopicQuery.new(object.current_user, guardian: scope, limit: false)
.private_messages_assigned_query(object.current_user)
@ -216,7 +240,7 @@ after_initialize do
options = object.instance_variable_get(:@opts)
if assigned_user = options.dig(:assigned)
scope.is_staff? ||
scope.can_assign? ||
assigned_user.downcase == scope.current_user&.username_lower
end
end
@ -225,19 +249,27 @@ after_initialize do
DiscourseAssign::Helpers.build_assigned_to_user(assigned_to_user_id, object.topic)
end
add_to_serializer(:topic_list_item, 'include_assigned_to_user?') do
(SiteSetting.assigns_public || scope.can_assign?) && object.assigned_to_user
end
add_to_serializer(:topic_view, 'include_assigned_to_user?') do
if SiteSetting.assigns_public || scope.can_assign?
# subtle but need to catch cases where stuff is not assigned
object.topic.custom_fields.keys.include?(TopicAssigner::ASSIGNED_TO_ID)
end
end
add_to_serializer(:current_user, :can_assign) do
object.can_assign?
end
add_to_class(:topic_view_serializer, :assigned_to_user_id) do
id = object.topic.custom_fields[TopicAssigner::ASSIGNED_TO_ID]
# a bit messy but race conditions can give us an array here, avoid
id && id.to_i rescue nil
end
add_to_serializer(:topic_view, 'include_assigned_to_user?') do
if SiteSetting.assigns_public || scope.is_staff?
# subtle but need to catch cases where stuff is not assigned
object.topic.custom_fields.keys.include?(TopicAssigner::ASSIGNED_TO_ID)
end
end
add_to_serializer(:flagged_topic, :assigned_to_user) do
DiscourseAssign::Helpers.build_assigned_to_user(assigned_to_user_id, object)
end
@ -318,5 +350,4 @@ after_initialize do
assigner.unassign(silent: true)
end
end
end

View File

@ -3,7 +3,8 @@
require 'rails_helper'
RSpec.describe Jobs::EnqueueReminders do
let(:user) { Fabricate(:user, admin: true) }
let(:assign_allowed_group) { Group.find_by(name: 'staff') }
let(:user) { Fabricate(:user, groups: [assign_allowed_group]) }
before do
SiteSetting.remind_assigns_frequency = RemindAssignsFrequencySiteSettings::MONTHLY_MINUTES

View File

@ -3,6 +3,8 @@
require 'rails_helper'
RSpec.describe PendingAssignsReminder do
before { SiteSetting.assign_enabled = true }
let(:user) { Fabricate(:user) }
it 'does not create a reminder if the user has 0 assigned topics' do

View File

@ -3,6 +3,9 @@
require 'rails_helper'
RSpec.describe TopicAssigner do
before { SiteSetting.assign_enabled = true }
let(:assign_allowed_group) { Group.find_by(name: 'staff') }
let(:pm_post) { Fabricate(:private_message_post) }
let(:pm) { pm_post.topic }
@ -18,6 +21,7 @@ RSpec.describe TopicAssigner do
describe 'assigning and unassigning private message' do
it 'should publish the right message' do
user = pm.allowed_users.first
user.groups << assign_allowed_group
assigner = described_class.new(pm, user)
assert_publish_topic_state(pm, user) { assigner.assign(user) }
@ -26,15 +30,16 @@ RSpec.describe TopicAssigner do
end
context "assigning and unassigning" do
before { SiteSetting.assign_enabled = true }
let(:post) { Fabricate(:post) }
let(:topic) { post.topic }
let(:moderator) { Fabricate(:moderator) }
let(:moderator2) { Fabricate(:moderator) }
let(:moderator) { Fabricate(:moderator, groups: [assign_allowed_group]) }
let(:moderator2) { Fabricate(:moderator, groups: [assign_allowed_group]) }
let(:assigner) { TopicAssigner.new(topic, moderator2) }
let(:assigner_self) { TopicAssigner.new(topic, moderator) }
it "can assign and unassign correctly" do
messages = MessageBus.track_publish("/notification-alert/#{moderator.id}") do
assigner.assign(moderator)
end
@ -87,7 +92,7 @@ RSpec.describe TopicAssigner do
context "when assigns_by_staff_mention is set to true" do
let(:system_user) { Discourse.system_user }
let(:moderator) { Fabricate(:admin, username: "modi") }
let(:moderator) { Fabricate(:admin, username: "modi", groups: [assign_allowed_group]) }
let(:post) { Fabricate(:post, raw: "Hey you @system, stay unassigned", user: moderator) }
let(:topic) { post.topic }
@ -170,7 +175,7 @@ RSpec.describe TopicAssigner do
context "unassign_on_close" do
let(:post) { Fabricate(:post) }
let(:topic) { post.topic }
let(:moderator) { Fabricate(:moderator) }
let(:moderator) { Fabricate(:moderator, groups: [assign_allowed_group]) }
let(:assigner) { TopicAssigner.new(topic, moderator) }
before do

48
spec/models/group_spec.rb Normal file
View File

@ -0,0 +1,48 @@
# frozen_string_literal: true
require 'rails_helper'
RSpec.describe Group do
describe 'Tracking changes that could affect the allow assign on groups site setting' do
let(:group) { Fabricate(:group) }
before 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"
end
let(:removed_group_setting) { 'staff|moderators' }
it 'removes the group from the setting when the group gets destroyed' do
SiteSetting.assign_allowed_on_groups = "#{group.name}|staff|moderators"
group.destroy!
expect(SiteSetting.assign_allowed_on_groups).to eq removed_group_setting
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}"
group.destroy!
expect(SiteSetting.assign_allowed_on_groups).to eq removed_group_setting
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"
group.destroy!
expect(SiteSetting.assign_allowed_on_groups).to eq removed_group_setting
end
end
end

View File

@ -4,17 +4,64 @@ require 'rails_helper'
RSpec.describe DiscourseAssign::AssignController do
let(:user) { Fabricate(:admin) }
before { SiteSetting.assign_enabled = true }
let(:default_allowed_group) { Group.find_by(name: 'staff') }
let(:user) { Fabricate(:admin, groups: [default_allowed_group]) }
let(:post) { Fabricate(:post) }
let(:user2) { Fabricate(:active_user) }
before { sign_in(user) }
describe 'only allow users from allowed groups' do
before { sign_in(user2) }
it 'filters requests where current_user is not member of an allowed group' do
SiteSetting.assign_allowed_on_groups = ''
put '/assign/assign.json', params: {
topic_id: post.topic_id, username: user2.username
}
expect(response.status).to eq(403)
end
context '#suggestions' do
before { sign_in(user) }
it 'includes users in allowed groups' do
allowed_group = Group.find_by(name: 'everyone')
user2.groups << allowed_group
user2.groups << default_allowed_group
SiteSetting.assign_allowed_on_groups = 'staff|everyone'
TopicAssigner.new(post.topic, user).assign(user2)
get '/assign/suggestions.json'
suggestions = JSON.parse(response.body).map { |u| u['username'] }
expect(suggestions).to contain_exactly(user2.username, user.username)
end
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'
TopicAssigner.new(post.topic, user).assign(user2)
get '/assign/suggestions.json'
suggestions = JSON.parse(response.body).map { |u| u['username'] }
expect(suggestions).to contain_exactly(user.username)
end
end
end
context "#suggestions" do
before { SiteSetting.max_assigned_topics = 1 }
before do
SiteSetting.max_assigned_topics = 1
sign_in(user)
end
it 'excludes other users from the suggestions when they already reached the max assigns limit' do
another_admin = Fabricate(:admin)
another_admin = Fabricate(:admin, groups: [default_allowed_group])
TopicAssigner.new(post.topic, user).assign(another_admin)
get '/assign/suggestions.json'
@ -25,6 +72,10 @@ RSpec.describe DiscourseAssign::AssignController do
end
context '#assign' do
before do
sign_in(user)
end
it 'assigns topic to a user' do
put '/assign/assign.json', params: {
topic_id: post.topic_id, username: user2.username

View File

@ -84,7 +84,8 @@ RSpec.describe TopicListSerializer do
end
describe 'as a staff' do
let(:guardian) { Guardian.new(Fabricate(:admin)) }
let(:admin) { Fabricate(:admin, groups: [Group.find_by(name: 'staff')]) }
let(:guardian) { Guardian.new(admin) }
it 'should include the right attribute' do
expect(serializer.as_json[:topic_list][:assigned_messages_count])

View File

@ -1,4 +1,4 @@
import { acceptance } from "helpers/qunit-helpers";
import { acceptance, replaceCurrentUser } from "helpers/qunit-helpers";
import { clearCallbacks } from "select-kit/mixins/plugin-api";
acceptance("Assign disabled mobile", {
@ -11,6 +11,7 @@ acceptance("Assign disabled mobile", {
});
QUnit.test("Footer dropdown does not contain button", async assert => {
replaceCurrentUser({ can_assign: true });
const menu = selectKit(".topic-footer-mobile-dropdown");
await visit("/t/internationalization-localization/280");

View File

@ -1,4 +1,4 @@
import { acceptance } from "helpers/qunit-helpers";
import { acceptance, replaceCurrentUser } from "helpers/qunit-helpers";
import { clearCallbacks } from "select-kit/mixins/plugin-api";
acceptance("Assign mobile", {
@ -11,6 +11,7 @@ acceptance("Assign mobile", {
});
QUnit.test("Footer dropdown contains button", async assert => {
replaceCurrentUser({ can_assign: true });
const menu = selectKit(".topic-footer-mobile-dropdown");
await visit("/t/internationalization-localization/280");