diff --git a/app/controllers/discourse_assign/assign_controller.rb b/app/controllers/discourse_assign/assign_controller.rb index 8e141ab..dfae899 100644 --- a/app/controllers/discourse_assign/assign_controller.rb +++ b/app/controllers/discourse_assign/assign_controller.rb @@ -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 diff --git a/assets/javascripts/discourse-assign/connectors/topic-footer-main-buttons-before-create/assign-button.js.es6 b/assets/javascripts/discourse-assign/connectors/topic-footer-main-buttons-before-create/assign-button.js.es6 index ea78d64..e14a552 100644 --- a/assets/javascripts/discourse-assign/connectors/topic-footer-main-buttons-before-create/assign-button.js.es6 +++ b/assets/javascripts/discourse-assign/connectors/topic-footer-main-buttons-before-create/assign-button.js.es6 @@ -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")) diff --git a/assets/javascripts/discourse-assign/connectors/user-activity-bottom/assigned-list.hbs b/assets/javascripts/discourse-assign/connectors/user-activity-bottom/assigned-list.hbs index a84d53e..545f12b 100644 --- a/assets/javascripts/discourse-assign/connectors/user-activity-bottom/assigned-list.hbs +++ b/assets/javascripts/discourse-assign/connectors/user-activity-bottom/assigned-list.hbs @@ -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}} diff --git a/assets/javascripts/discourse-assign/connectors/user-messages-nav/assigned-messages.js.es6 b/assets/javascripts/discourse-assign/connectors/user-messages-nav/assigned-messages.js.es6 index a7ba007..9853ce3 100644 --- a/assets/javascripts/discourse-assign/connectors/user-messages-nav/assigned-messages.js.es6 +++ b/assets/javascripts/discourse-assign/connectors/user-messages-nav/assigned-messages.js.es6 @@ -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")) diff --git a/assets/javascripts/discourse-assign/connectors/user-preferences-notifications/remind-assigns-frequency.js.es6 b/assets/javascripts/discourse-assign/connectors/user-preferences-notifications/remind-assigns-frequency.js.es6 index 3ff2ebb..4db0626 100644 --- a/assets/javascripts/discourse-assign/connectors/user-preferences-notifications/remind-assigns-frequency.js.es6 +++ b/assets/javascripts/discourse-assign/connectors/user-preferences-notifications/remind-assigns-frequency.js.es6 @@ -1,5 +1,5 @@ export default { shouldRender(args, component) { - return component.currentUser && component.currentUser.get("staff"); + return component.currentUser && component.currentUser.get("can_assign"); } }; diff --git a/assets/javascripts/discourse-assign/controllers/assign-user.js.es6 b/assets/javascripts/discourse-assign/controllers/assign-user.js.es6 index da6bcae..13692df 100644 --- a/assets/javascripts/discourse-assign/controllers/assign-user.js.es6 +++ b/assets/javascripts/discourse-assign/controllers/assign-user.js.es6 @@ -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"); }, diff --git a/assets/javascripts/discourse-assign/initializers/extend-for-assigns.js.es6 b/assets/javascripts/discourse-assign/initializers/extend-for-assigns.js.es6 index 638031c..adf540e 100644 --- a/assets/javascripts/discourse-assign/initializers/extend-for-assigns.js.es6 +++ b/assets/javascripts/discourse-assign/initializers/extend-for-assigns.js.es6 @@ -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", diff --git a/assets/javascripts/discourse/services/task-actions.js.es6 b/assets/javascripts/discourse/services/task-actions.js.es6 index 41bf387..ae25601 100644 --- a/assets/javascripts/discourse/services/task-actions.js.es6 +++ b/assets/javascripts/discourse/services/task-actions.js.es6 @@ -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 } }); } diff --git a/assets/javascripts/discourse/templates/modal/assign-user.hbs b/assets/javascripts/discourse/templates/modal/assign-user.hbs index 149cf1c..b2ad776 100644 --- a/assets/javascripts/discourse/templates/modal/assign-user.hbs +++ b/assets/javascripts/discourse/templates/modal/assign-user.hbs @@ -4,6 +4,7 @@ single=true allowAny=false group="staff" + groupMembersOf=model.allowedGroups excludeCurrentUser=false includeMentionableGroups=false hasGroups=false diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 184c4a1..6cb18f7 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -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" diff --git a/config/settings.yml b/config/settings.yml index c83e154..a0aedb0 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -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 diff --git a/jobs/scheduled/enqueue_reminders.rb b/jobs/scheduled/enqueue_reminders.rb index dcfee89..d1038e8 100644 --- a/jobs/scheduled/enqueue_reminders.rb +++ b/jobs/scheduled/enqueue_reminders.rb @@ -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 diff --git a/lib/topic_assigner.rb b/lib/topic_assigner.rb index 9eee36f..d48bfa0 100644 --- a/lib/topic_assigner.rb +++ b/lib/topic_assigner.rb @@ -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 = < 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 diff --git a/spec/jobs/scheduled/enqueue_reminders_spec.rb b/spec/jobs/scheduled/enqueue_reminders_spec.rb index 6bae3d7..61361d8 100644 --- a/spec/jobs/scheduled/enqueue_reminders_spec.rb +++ b/spec/jobs/scheduled/enqueue_reminders_spec.rb @@ -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 diff --git a/spec/lib/pending_assigns_reminder_spec.rb b/spec/lib/pending_assigns_reminder_spec.rb index 6193490..a8373be 100644 --- a/spec/lib/pending_assigns_reminder_spec.rb +++ b/spec/lib/pending_assigns_reminder_spec.rb @@ -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 diff --git a/spec/lib/topic_assigner_spec.rb b/spec/lib/topic_assigner_spec.rb index 30ce5b2..69a5e6e 100644 --- a/spec/lib/topic_assigner_spec.rb +++ b/spec/lib/topic_assigner_spec.rb @@ -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 diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb new file mode 100644 index 0000000..a269856 --- /dev/null +++ b/spec/models/group_spec.rb @@ -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 diff --git a/spec/requests/assign_controller_spec.rb b/spec/requests/assign_controller_spec.rb index f2c425b..d4fb776 100644 --- a/spec/requests/assign_controller_spec.rb +++ b/spec/requests/assign_controller_spec.rb @@ -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 diff --git a/spec/serializers/topic_list_serializer_spec.rb b/spec/serializers/topic_list_serializer_spec.rb index adaa563..b614acb 100644 --- a/spec/serializers/topic_list_serializer_spec.rb +++ b/spec/serializers/topic_list_serializer_spec.rb @@ -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]) diff --git a/test/javascripts/acceptance/assign-disabled-test.js.es6 b/test/javascripts/acceptance/assign-disabled-test.js.es6 index 808ad55..35ed1da 100644 --- a/test/javascripts/acceptance/assign-disabled-test.js.es6 +++ b/test/javascripts/acceptance/assign-disabled-test.js.es6 @@ -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"); diff --git a/test/javascripts/acceptance/assign-enabled-test.js.es6 b/test/javascripts/acceptance/assign-enabled-test.js.es6 index 59bcaf7..0f5e9da 100644 --- a/test/javascripts/acceptance/assign-enabled-test.js.es6 +++ b/test/javascripts/acceptance/assign-enabled-test.js.es6 @@ -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");