From d807491df2d1170dadac7b8c19bbdf49d23dbc6f Mon Sep 17 00:00:00 2001 From: Jarek Radosz Date: Fri, 27 Aug 2021 01:25:38 +0200 Subject: [PATCH] FEATURE: Assign to group (#182) Ability to assign groups. To assign group, user must have a right to send a message to that group. In addition, 2 jobs were introduced, - AssignNotification and UnassignNotification to inform interested users in the background about the new assignment. --- .../discourse_assign/assign_controller.rb | 65 ++- app/models/assignment.rb | 2 + app/serializers/assigned_topic_serializer.rb | 21 +- .../controllers/assign-user.js.es6 | 45 +- .../initializers/extend-for-assigns.js.es6 | 108 +++-- .../components/assign-actions-dropdown.js.es6 | 6 +- .../discourse/components/claim-topic.js.es6 | 55 --- .../discourse/services/task-actions.js.es6 | 3 +- .../components/assigned-topic-list-item.hbs | 22 +- .../templates/components/claim-topic.hbs | 20 - .../components/assigned-topic-list-item.hbs | 21 +- .../discourse/templates/modal/assign-user.hbs | 4 +- .../widgets/quick-access-assignments.js.es6 | 3 +- assets/stylesheets/assigns.scss | 12 +- config/locales/client.en.yml | 3 + config/locales/server.en.yml | 4 + ...804_add_assigned_to_type_to_assignments.rb | 19 + jobs/regular/assign_notification.rb | 50 +++ jobs/regular/unassign_notification.rb | 25 ++ jobs/scheduled/enqueue_reminders.rb | 1 + lib/discourse_assign/helpers.rb | 31 +- lib/pending_assigns_reminder.rb | 4 +- lib/topic_assigner.rb | 255 ++++++----- plugin.rb | 401 +++++++++++------- spec/components/topic_query_spec.rb | 42 +- ...ents_from_custom_fields_to_a_table_spec.rb | 3 +- spec/jobs/regular/assign_notification_spec.rb | 118 ++++++ .../regular/unassign_notification_spec.rb | 66 +++ spec/lib/topic_assigner_spec.rb | 58 ++- spec/models/topic_spec.rb | 28 ++ spec/requests/assign_controller_spec.rb | 49 +-- spec/requests/list_controller_spec.rb | 4 - .../flagged_topic_serializer_spec.rb | 83 ++++ .../serializers/group_show_serializer_spec.rb | 28 ++ spec/support/assign_allowed_group.rb | 6 +- .../acceptance/assigned-topic-test.js.es6 | 37 +- .../fixtures/assigned-topics-fixtures.js.es6 | 55 +++ 37 files changed, 1215 insertions(+), 542 deletions(-) delete mode 100644 assets/javascripts/discourse/components/claim-topic.js.es6 delete mode 100644 assets/javascripts/discourse/templates/components/claim-topic.hbs create mode 100644 db/migrate/20210724143804_add_assigned_to_type_to_assignments.rb create mode 100644 jobs/regular/assign_notification.rb create mode 100644 jobs/regular/unassign_notification.rb create mode 100644 spec/jobs/regular/assign_notification_spec.rb create mode 100644 spec/jobs/regular/unassign_notification_spec.rb create mode 100644 spec/models/topic_spec.rb create mode 100644 spec/serializers/flagged_topic_serializer_spec.rb create mode 100644 spec/serializers/group_show_serializer_spec.rb diff --git a/app/controllers/discourse_assign/assign_controller.rb b/app/controllers/discourse_assign/assign_controller.rb index 98b1406..fe73720 100644 --- a/app/controllers/discourse_assign/assign_controller.rb +++ b/app/controllers/discourse_assign/assign_controller.rb @@ -13,6 +13,7 @@ module DiscourseAssign JOIN( SELECT assigned_to_id user_id, MAX(created_at) last_assigned FROM assignments + WHERE assignments.assigned_to_type = 'User' GROUP BY assigned_to_id HAVING COUNT(*) < #{SiteSetting.max_assigned_topics} ) as X ON X.user_id = users.id @@ -24,34 +25,11 @@ module DiscourseAssign render json: { assign_allowed_on_groups: Group.visible_groups(current_user).assign_allowed_groups.pluck(:name), - suggestions: ActiveModel::ArraySerializer.new(users, scope: guardian, each_serializer: BasicUserSerializer) + assign_allowed_for_groups: Group.visible_groups(current_user).messageable(current_user).pluck(:name), + suggestions: ActiveModel::ArraySerializer.new(users, scope: guardian, each_serializer: BasicUserSerializer), } end - def claim - topic_id = params.require(:topic_id).to_i - topic = Topic.find(topic_id) - - assigned_id = Assignment - .where(topic_id: topic_id) - .where.not(assigned_to_id: nil) - .pluck_first(:assigned_to_id) - - if assigned_id - if user = User.where(id: assigned_id).first - extras = { - assigned_to: serialize_data(user, BasicUserSerializer, root: false) - } - end - - return render_json_error(I18n.t('discourse_assign.already_claimed'), extras: extras) - end - - assigner = TopicAssigner.new(topic, current_user) - assigner.assign(current_user) - render json: success_json - end - def unassign topic_id = params.require(:topic_id) topic = Topic.find(topic_id.to_i) @@ -63,10 +41,11 @@ module DiscourseAssign def assign topic_id = params.require(:topic_id) - username = params.require(:username) + username = params.permit(:username)['username'] + group_name = params.permit(:group_name)['group_name'] topic = Topic.find(topic_id.to_i) - assign_to = User.find_by(username_lower: username.downcase) + assign_to = username.present? ? User.find_by(username_lower: username.downcase) : Group.where("LOWER(name) = ?", group_name.downcase).first raise Discourse::NotFound unless assign_to @@ -111,7 +90,7 @@ module DiscourseAssign topics.each do |topic| user_id = assignments[topic.id] - topic.preload_assigned_to_user(users_map[user_id]) if user_id + topic.preload_assigned_to(users_map[user_id]) if user_id end render json: { topics: serialize_data(topics, AssignedTopicSerializer) } @@ -131,7 +110,7 @@ module DiscourseAssign members = User .joins("LEFT OUTER JOIN group_users g ON g.user_id = users.id") - .joins("LEFT OUTER JOIN assignments a ON a.assigned_to_id = users.id") + .joins("LEFT OUTER JOIN assignments a ON a.assigned_to_id = users.id AND a.assigned_to_type = 'User'") .joins("LEFT OUTER JOIN topics t ON t.id = a.topic_id") .where("g.group_id = ? AND users.id > 0 AND t.deleted_at IS NULL", group.id) .where("a.assigned_to_id IS NOT NULL") @@ -148,8 +127,12 @@ module DiscourseAssign end assignment_count = Topic - .joins("JOIN assignments a ON a.topic_id = topics.id AND a.assigned_to_id IS NOT NULL") - .where("a.assigned_to_id IN (SELECT group_users.user_id FROM group_users WHERE (group_id IN (SELECT id FROM groups WHERE name = ?)))", group.name) + .joins("JOIN assignments a ON a.topic_id = topics.id AND a.assigned_to_id IS NOT NULL ") + .where(<<~SQL, group_id: group.id) + (a.assigned_to_id IN (SELECT group_users.user_id FROM group_users WHERE group_id = :group_id) AND a.assigned_to_type = 'User') + OR + (a.assigned_to_id = :group_id AND a.assigned_to_type = 'Group') + SQL .where("topics.deleted_at IS NULL") .count @@ -158,19 +141,27 @@ module DiscourseAssign private - def translate_failure(reason, user) + def translate_failure(reason, assign_to) case reason when :already_assigned - { error: I18n.t('discourse_assign.already_assigned', username: user.username) } + { error: I18n.t('discourse_assign.already_assigned', username: assign_to.username) } when :forbidden_assign_to - { error: I18n.t('discourse_assign.forbidden_assign_to', username: user.username) } + { error: I18n.t('discourse_assign.forbidden_assign_to', username: assign_to.username) } when :forbidden_assignee_not_pm_participant - { error: I18n.t('discourse_assign.forbidden_assignee_not_pm_participant', username: user.username) } + { error: I18n.t('discourse_assign.forbidden_assignee_not_pm_participant', username: assign_to.username) } when :forbidden_assignee_cant_see_topic - { error: I18n.t('discourse_assign.forbidden_assignee_cant_see_topic', username: user.username) } + { error: I18n.t('discourse_assign.forbidden_assignee_cant_see_topic', username: assign_to.username) } + when :group_already_assigned + { error: I18n.t('discourse_assign.group_already_assigned', group: assign_to.name) } + when :forbidden_group_assign_to + { error: I18n.t('discourse_assign.forbidden_group_assign_to', group: assign_to.name) } + when :forbidden_group_assignee_not_pm_participant + { error: I18n.t('discourse_assign.forbidden_group_assignee_not_pm_participant', group: assign_to.name) } + when :forbidden_group_assignee_cant_see_topic + { error: I18n.t('discourse_assign.forbidden_group_assignee_cant_see_topic', group: assign_to.name) } else max = SiteSetting.max_assigned_topics - { error: I18n.t('discourse_assign.too_many_assigns', username: user.username, max: max) } + { error: I18n.t('discourse_assign.too_many_assigns', username: assign_to.username, max: max) } end end diff --git a/app/models/assignment.rb b/app/models/assignment.rb index 936c1cc..426c22f 100644 --- a/app/models/assignment.rb +++ b/app/models/assignment.rb @@ -2,4 +2,6 @@ class Assignment < ActiveRecord::Base belongs_to :topic + belongs_to :assigned_to, polymorphic: true + belongs_to :assigned_by_user, class_name: "User" end diff --git a/app/serializers/assigned_topic_serializer.rb b/app/serializers/assigned_topic_serializer.rb index f6f18bb..18dbc08 100644 --- a/app/serializers/assigned_topic_serializer.rb +++ b/app/serializers/assigned_topic_serializer.rb @@ -6,8 +6,25 @@ class AssignedTopicSerializer < BasicTopicSerializer attributes :excerpt, :category_id, :created_at, - :updated_at + :updated_at, + :assigned_to_user, + :assigned_to_group has_one :user, serializer: BasicUserSerializer, embed: :objects - has_one :assigned_to_user, serializer: BasicUserSerializer, embed: :objects + + def assigned_to_user + BasicUserSerializer.new(object.assigned_to, scope: scope, root: false).as_json + end + + def include_assigned_to_user? + object.assigned_to.is_a?(User) + end + + def assigned_to_group + BasicGroupSerializer.new(object.assigned_to, scope: scope, root: false).as_json + end + + def include_assigned_to_group? + object.assigned_to.is_a?(Group) + end end diff --git a/assets/javascripts/discourse-assign/controllers/assign-user.js.es6 b/assets/javascripts/discourse-assign/controllers/assign-user.js.es6 index 7a84d56..8e4b46b 100644 --- a/assets/javascripts/discourse-assign/controllers/assign-user.js.es6 +++ b/assets/javascripts/discourse-assign/controllers/assign-user.js.es6 @@ -2,7 +2,7 @@ import Controller, { inject as controller } from "@ember/controller"; import { inject as service } from "@ember/service"; import { ajax } from "discourse/lib/ajax"; import { popupAjaxError } from "discourse/lib/ajax-error"; -import { not } from "@ember/object/computed"; +import { not, or } from "@ember/object/computed"; import { isEmpty } from "@ember/utils"; import { action } from "@ember/object"; @@ -12,6 +12,7 @@ export default Controller.extend({ allowedGroups: null, taskActions: service(), autofocus: not("capabilities.touch"), + assigneeName: or("model.username", "model.group_name"), init() { this._super(...arguments); @@ -20,6 +21,7 @@ export default Controller.extend({ ajax("/assign/suggestions").then((data) => { this.set("assignSuggestions", data.suggestions); this.set("allowedGroups", data.assign_allowed_on_groups); + this.set("allowedGroupsForAssignment", data.assign_allowed_for_groups); }); }, @@ -37,17 +39,29 @@ export default Controller.extend({ }, @action - assignUser(user) { + assignUser(assignee) { if (this.isBulkAction) { - this.bulkAction(user.username); + this.bulkAction(assignee.name); return; } - this.setProperties({ - "model.username": user.username, - "model.allowedGroups": this.taskActions.allowedGroups, - }); - return this.assign(); + if (this.allowedGroupsForAssignment.includes(assignee.name)) { + this.setProperties({ + "model.username": null, + "model.group_name": assignee.name, + "model.allowedGroups": this.taskActions.allowedGroups, + }); + } else { + this.setProperties({ + "model.username": assignee.username, + "model.group_name": null, + "model.allowedGroups": this.taskActions.allowedGroups, + }); + } + + if (assignee.name) { + return this.assign(); + } }, @action @@ -59,8 +73,18 @@ export default Controller.extend({ let path = "/assign/assign"; if (isEmpty(this.get("model.username"))) { + this.model.topic.set("assigned_to_user", null); + } + + if (isEmpty(this.get("model.group_name"))) { + this.model.topic.set("assigned_to_group", null); + } + + if ( + isEmpty(this.get("model.username")) && + isEmpty(this.get("model.group_name")) + ) { path = "/assign/unassign"; - this.set("model.assigned_to_user", null); } this.send("closeModal"); @@ -69,6 +93,7 @@ export default Controller.extend({ type: "PUT", data: { username: this.get("model.username"), + group_name: this.get("model.group_name"), topic_id: this.get("model.topic.id"), }, }) @@ -82,6 +107,6 @@ export default Controller.extend({ @action assignUsername(selected) { - this.assignUser({ username: selected.firstObject }); + this.assignUser({ name: selected.firstObject }); }, }); 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 393031f..87b8f42 100644 --- a/assets/javascripts/discourse-assign/initializers/extend-for-assigns.js.es6 +++ b/assets/javascripts/discourse-assign/initializers/extend-for-assigns.js.es6 @@ -1,6 +1,6 @@ import { renderAvatar } from "discourse/helpers/user-avatar"; import { withPluginApi } from "discourse/lib/plugin-api"; -import computed, { observes } from "discourse-common/utils/decorators"; +import discourseComputed, { observes } from "discourse-common/utils/decorators"; import { iconHTML, iconNode } from "discourse-common/lib/icon-library"; import { h } from "virtual-dom"; import { queryRegistry } from "discourse/widgets/widget"; @@ -15,10 +15,10 @@ import { inject } from "@ember/controller"; import I18n from "I18n"; import { get } from "@ember/object"; -function titleForState(user) { - if (user) { +function titleForState(name) { + if (name) { return I18n.t("discourse_assign.unassign.help", { - username: user.username, + username: name, }); } else { return I18n.t("discourse_assign.assign.help"); @@ -29,7 +29,9 @@ function registerTopicFooterButtons(api) { api.registerTopicFooterButton({ id: "assign", icon() { - const hasAssignement = this.get("topic.assigned_to_user"); + const hasAssignement = + this.get("topic.assigned_to_user") || + this.get("topic.assigned_to_group"); return hasAssignement ? this.site.mobileView ? "user-times" @@ -38,17 +40,23 @@ function registerTopicFooterButtons(api) { }, priority: 250, translatedTitle() { - return titleForState(this.get("topic.assigned_to_user")); + return titleForState( + this.get("topic.assigned_to_user.username") || + this.get("topic.assigned_to_group.name") + ); }, translatedAriaLabel() { - return titleForState(this.get("topic.assigned_to_user")); + return titleForState( + this.get("topic.assigned_to_user.username") || + this.get("topic.assigned_to_group.name") + ); }, translatedLabel() { const user = this.get("topic.assigned_to_user"); + const group = this.get("topic.assigned_to_group"); + const label = I18n.t("discourse_assign.unassign.title"); if (user) { - const label = I18n.t("discourse_assign.unassign.title"); - if (this.site.mobileView) { return htmlSafe( `${label}${ @@ -66,6 +74,10 @@ function registerTopicFooterButtons(api) { })}${label}` ); } + } else if (group) { + return htmlSafe( + `${label} @${group.name}` + ); } else { return I18n.t("discourse_assign.assign.title"); } @@ -82,6 +94,9 @@ function registerTopicFooterButtons(api) { if (assignedUser) { this.set("topic.assigned_to_user", null); taskActions.unassign(topic.id); + } else if (topic.assigned_to_group) { + this.set("topic.assigned_to_group", null); + taskActions.unassign(topic.id); } else { taskActions.assign(topic); } @@ -92,6 +107,7 @@ function registerTopicFooterButtons(api) { classNames: ["assign"], dependentKeys: [ "topic.assigned_to_user", + "topic.assigned_to_group", "currentUser.can_assign", "topic.assigned_to_user.username", ], @@ -148,7 +164,7 @@ function initialize(api) { ); api.modifyClass("model:topic", { - @computed("assigned_to_user") + @discourseComputed("assigned_to_user") assignedToUserPath(assignedToUser) { return getURL( siteSettings.assigns_user_url_path.replace( @@ -157,10 +173,14 @@ function initialize(api) { ) ); }, + @discourseComputed("assigned_to_group") + assignedToGroupPath(assignedToGroup) { + return getURL(`/g/${assignedToGroup.name}/assigned/everyone`); + }, }); api.modifyClass("model:bookmark", { - @computed("assigned_to_user") + @discourseComputed("assigned_to_user") assignedToUserPath(assignedToUser) { return getURL( this.siteSettings.assigns_user_url_path.replace( @@ -169,10 +189,16 @@ function initialize(api) { ) ); }, + @discourseComputed("assigned_to_group") + assignedToGroupPath(assignedToGroup) { + return getURL(`/g/${assignedToGroup.name}/assigned/everyone`); + }, }); api.addPostSmallActionIcon("assigned", "user-plus"); + api.addPostSmallActionIcon("assigned_group", "user-plus"); api.addPostSmallActionIcon("unassigned", "user-times"); + api.addPostSmallActionIcon("unassigned_group", "user-times"); api.addPostTransformCallback((transformed) => { if ( @@ -187,15 +213,20 @@ function initialize(api) { api.addDiscoveryQueryParam("assigned", { replace: true, refreshModel: true }); api.addTagsHtmlCallback((topic, params = {}) => { - const assignedTo = topic.get("assigned_to_user.username"); - if (assignedTo) { - const assignedPath = topic.assignedToUserPath; + const assignedToUser = topic.get("assigned_to_user.username"); + const assignedToGroup = topic.get("assigned_to_group.name"); + if (assignedToUser || assignedToGroup) { + const assignedPath = assignedToUser + ? topic.assignedToUserPath + : topic.assignedToGroupPath; const tagName = params.tagName || "a"; - const icon = iconHTML("user-plus"); + const icon = assignedToUser ? iconHTML("user-plus") : iconHTML("users"); const href = tagName === "a" ? `href="${assignedPath}" data-auto-route="true"` : ""; - - return `<${tagName} class="assigned-to discourse-tag simple" ${href}>${icon}${assignedTo}`; + return `<${tagName} class="assigned-to discourse-tag simple" ${href}> + ${icon} + ${assignedToUser || assignedToGroup} + `; } }); @@ -219,15 +250,15 @@ function initialize(api) { api.createWidget("assigned-to", { html(attrs) { - let { assignedToUser, href } = attrs; + let { assignedToUser, assignedToGroup, href } = attrs; return h("p.assigned-to", [ - iconNode("user-plus"), + assignedToUser ? iconNode("user-plus") : iconNode("users"), h("span.assign-text", I18n.t("discourse_assign.assigned_to")), h( "a", { attributes: { class: "assigned-to-username", href } }, - assignedToUser.username + assignedToUser ? assignedToUser.username : assignedToGroup.name ), ]); }, @@ -242,11 +273,20 @@ function initialize(api) { const topicId = topic.id; if (data.topic_id === topicId) { - topic.set( - "assigned_to_user_id", - data.type === "assigned" ? data.assigned_to.id : null - ); - topic.set("assigned_to_user", data.assigned_to); + if (data.assigned_type === "User") { + topic.set( + "assigned_to_user_id", + data.type === "assigned" ? data.assigned_to.id : null + ); + topic.set("assigned_to_user", data.assigned_to); + } + if (data.assigned_type === "Group") { + topic.set( + "assigned_to_group_id", + data.type === "assigned" ? data.assigned_to.id : null + ); + topic.set("assigned_to_group", data.assigned_to); + } } this.appEvents.trigger("header:update-topic", topic); }); @@ -268,10 +308,14 @@ function initialize(api) { const postModel = dec.getModel(); if (postModel) { const assignedToUser = get(postModel, "topic.assigned_to_user"); - if (assignedToUser) { + const assignedToGroup = get(postModel, "topic.assigned_to_group"); + if (assignedToUser || assignedToGroup) { return dec.widget.attach("assigned-to", { assignedToUser, - href: get(postModel, "topic.assignedToUserPath"), + assignedToGroup, + href: assignedToUser + ? get(postModel, "topic.assignedToUserPath") + : get(postModel, "topic.assignedToGroupPath"), }); } } @@ -283,6 +327,11 @@ function initialize(api) { "user-plus" ); + api.replaceIcon( + "notification.discourse_assign.assign_group_notification", + "users" + ); + api.modifyClass("controller:preferences/notifications", { actions: { save() { @@ -375,5 +424,10 @@ export default { api.addSearchSuggestion("in:assigned"); api.addSearchSuggestion("in:unassigned"); }); + + withPluginApi("0.12.2", (api) => { + api.addGroupPostSmallActionCode("assigned_group"); + api.addGroupPostSmallActionCode("unassigned_group"); + }); }, }; diff --git a/assets/javascripts/discourse/components/assign-actions-dropdown.js.es6 b/assets/javascripts/discourse/components/assign-actions-dropdown.js.es6 index 1a89c47..8cc2326 100644 --- a/assets/javascripts/discourse/components/assign-actions-dropdown.js.es6 +++ b/assets/javascripts/discourse/components/assign-actions-dropdown.js.es6 @@ -16,7 +16,7 @@ export default DropdownSelectBoxComponent.extend({ icon: "user-times", name: I18n.t("discourse_assign.unassign.title"), description: I18n.t("discourse_assign.unassign.help", { - username: this.topic.assigned_to_user.username, + username: this.assignee, }), }, { @@ -32,10 +32,10 @@ export default DropdownSelectBoxComponent.extend({ onSelect(id) { switch (id) { case "unassign": - this.unassign(this.topic, this.user); + this.unassign(this.topic, this.assignee); break; case "reassign": - this.reassign(this.topic, this.user); + this.reassign(this.topic, this.assignee); break; } }, diff --git a/assets/javascripts/discourse/components/claim-topic.js.es6 b/assets/javascripts/discourse/components/claim-topic.js.es6 deleted file mode 100644 index a307893..0000000 --- a/assets/javascripts/discourse/components/claim-topic.js.es6 +++ /dev/null @@ -1,55 +0,0 @@ -import Component from "@ember/component"; -import { ajax } from "discourse/lib/ajax"; -import { popupAjaxError } from "discourse/lib/ajax-error"; -import { action } from "@ember/object"; - -export default Component.extend({ - tagName: "", - claiming: false, - unassigning: false, - - @action - unassign() { - this.set("unassigning", true); - - return ajax("/assign/unassign", { - type: "PUT", - data: { topic_id: this.get("topic.id") }, - }) - .then(() => { - this.set("topic.assigned_to_user", null); - }) - .catch(popupAjaxError) - .finally(() => { - this.set("unassigning", false); - }); - }, - - @action - claim() { - this.set("claiming", true); - - let topic = this.topic; - ajax(`/assign/claim/${topic.id}`, { - method: "PUT", - }) - .then(() => { - this.set("topic.assigned_to_user", this.currentUser); - }) - .catch((e) => { - if (e.jqXHR && e.jqXHR.responseJSON) { - let json = e.jqXHR.responseJSON; - if (json && json.extras) { - this.set("topic.assigned_to_user", json.extras.assigned_to); - } - } - return popupAjaxError(e); - }) - .finally(() => { - if (this.isDestroying || this.isDestroyed) { - return; - } - this.set("claiming", false); - }); - }, -}); diff --git a/assets/javascripts/discourse/services/task-actions.js.es6 b/assets/javascripts/discourse/services/task-actions.js.es6 index 7ee0688..f78279b 100644 --- a/assets/javascripts/discourse/services/task-actions.js.es6 +++ b/assets/javascripts/discourse/services/task-actions.js.es6 @@ -13,8 +13,9 @@ export default Service.extend({ assign(topic) { return showModal("assign-user", { model: { - topic, username: topic.get("assigned_to_user.username"), + group_name: topic.get("assigned_to_group.name"), + topic, }, }); }, diff --git a/assets/javascripts/discourse/templates/components/assigned-topic-list-item.hbs b/assets/javascripts/discourse/templates/components/assigned-topic-list-item.hbs index b5f2d60..89b1ecb 100644 --- a/assets/javascripts/discourse/templates/components/assigned-topic-list-item.hbs +++ b/assets/javascripts/discourse/templates/components/assigned-topic-list-item.hbs @@ -46,12 +46,20 @@ {{number topic.views numberKey="views_long"}} {{raw "list/activity-column" topic=topic class="num" tagName="td"}} - - {{assign-actions-dropdown - topic=topic - user=username - unassign=unassign - reassign=reassign - }} + {{#if topic.assigned_to_user}} + {{assign-actions-dropdown + topic=topic + assignee=topic.assigned_to_user.username + unassign=unassign + reassign=reassign + }} + {{else}} + {{assign-actions-dropdown + topic=topic + assignee=topic.assigned_to_group.name + unassign=unassign + reassign=reassign + }} + {{/if}} diff --git a/assets/javascripts/discourse/templates/components/claim-topic.hbs b/assets/javascripts/discourse/templates/components/claim-topic.hbs deleted file mode 100644 index 4ea83f2..0000000 --- a/assets/javascripts/discourse/templates/components/claim-topic.hbs +++ /dev/null @@ -1,20 +0,0 @@ -{{#if topic.assigned_to_user}} - {{#assigned-to user=topic.assigned_to_user}} - {{d-button - icon="times" - class="btn-small unassign" - action=(action "unassign") - disabled=unassigning - title="discourse_assign.unassign.help" - }} - {{/assigned-to}} -{{else}} - {{d-button - class="btn-small assign" - icon="user-plus" - action=(action "claim") - disabled=claiming - label="discourse_assign.claim.title" - title="discourse_assign.claim.help" - }} -{{/if}} diff --git a/assets/javascripts/discourse/templates/mobile/components/assigned-topic-list-item.hbs b/assets/javascripts/discourse/templates/mobile/components/assigned-topic-list-item.hbs index 684e5d4..5d075ab 100644 --- a/assets/javascripts/discourse/templates/mobile/components/assigned-topic-list-item.hbs +++ b/assets/javascripts/discourse/templates/mobile/components/assigned-topic-list-item.hbs @@ -21,12 +21,21 @@ {{/if}}
- {{assign-actions-dropdown - topic=topic - user=username - unassign=unassign - reassign=reassign - }} + {{#if topic.assigned_to_user}} + {{assign-actions-dropdown + topic=topic + assignee=topic.assigned_to_user.username + unassign=unassign + reassign=reassign + }} + {{else}} + {{assign-actions-dropdown + topic=topic + assignee=topic.assigned_to_group.name + unassign=unassign + reassign=reassign + }} + {{/if}}
diff --git a/assets/javascripts/discourse/templates/modal/assign-user.hbs b/assets/javascripts/discourse/templates/modal/assign-user.hbs index 4986965..6262a0f 100644 --- a/assets/javascripts/discourse/templates/modal/assign-user.hbs +++ b/assets/javascripts/discourse/templates/modal/assign-user.hbs @@ -3,12 +3,14 @@ {{i18n "discourse_assign.assign_modal.description"}} {{email-group-user-chooser autocomplete="off" - value=model.username + value=assigneeName onChange=(action "assignUsername") + autofocus="autofocus" options=(hash placementStrategy="absolute" filterPlaceholder=placeholderKey includeGroups=false + includeMessageableGroups=true groupMembersOf=allowedGroups maximum=1 autofocus=autofocus diff --git a/assets/javascripts/discourse/widgets/quick-access-assignments.js.es6 b/assets/javascripts/discourse/widgets/quick-access-assignments.js.es6 index 503b989..b1280f9 100644 --- a/assets/javascripts/discourse/widgets/quick-access-assignments.js.es6 +++ b/assets/javascripts/discourse/widgets/quick-access-assignments.js.es6 @@ -11,6 +11,7 @@ import { h } from "virtual-dom"; import I18n from "I18n"; const ICON = "user-plus"; +const GROUP_ICON = "users"; createWidget("no-quick-access-assignments", { html() { @@ -58,7 +59,7 @@ if (QuickAccessPanel) { itemHtml(assignedTopic) { return this.attach("quick-access-item", { - icon: ICON, + icon: assignedTopic.assigned_to_group ? GROUP_ICON : ICON, href: postUrl( assignedTopic.slug, assignedTopic.id, diff --git a/assets/stylesheets/assigns.scss b/assets/stylesheets/assigns.scss index 3adb4c4..93e9bbd 100644 --- a/assets/stylesheets/assigns.scss +++ b/assets/stylesheets/assigns.scss @@ -70,12 +70,18 @@ } .assign-suggestions { - height: 25px; margin-top: 15px; img { margin-right: 5px; cursor: pointer; } + + .groups { + margin-top: 5px; + a { + margin-right: 5px; + } + } } .topic-list-item { @@ -144,3 +150,7 @@ .topic-search-div { margin-left: 1em; } + +.assigned-to.discourse-tag.simple { + color: var(--primary-medium); +} diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 0db7639..aa5b216 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -6,7 +6,9 @@ en: help: "Topics that are not assigned" action_codes: assigned: "assigned %{who} %{when}" + assigned_group: "assigned %{who} %{when}" unassigned: "unassigned %{who} %{when}" + unassigned_group: "unassigned %{who} %{when}" discourse_assign: add_unassigned_filter: "Add 'unassigned' filter to category" cant_act: "You cannot act on flags that have been assigned to other users" @@ -17,6 +19,7 @@ en: group_everyone: "Everyone" assigned_to: "Assigned to" assign_notification: "

%{username} %{description}

" + assign_group_notification: "

%{username} %{description}

" unassign: title: "Unassign" help: "Unassign %{username} from Topic" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index c19ebb6..a659727 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -23,6 +23,10 @@ en: forbidden_assign_to: "@%{username} can't be assigned since they don't belong to assigned allowed groups." forbidden_assignee_not_pm_participant: "@%{username} can't be assigned because they don't have access to this PM. You can grant @%{username} access by inviting them to this PM." forbidden_assignee_cant_see_topic: "@%{username} can't be assigned because they don't have access to this topic." + group_already_assigned: "Topic is already assigned to @%{group}" + forbidden_group_assign_to: "@%{group} can't be assigned since they don't belong to assigned allowed groups." + forbidden_group_assignee_not_pm_participant: "@%{group} can't be assigned because not all members have access to this PM." + forbidden_group_assignee_cant_see_topic: "@%{group} can't be assigned because not all members have access to this topic." flag_assigned: "Sorry, that flag's topic is assigned to another user" flag_unclaimed: "You must claim that topic before acting on the flag" topic_assigned_excerpt: "assigned you the topic '%{title}'" diff --git a/db/migrate/20210724143804_add_assigned_to_type_to_assignments.rb b/db/migrate/20210724143804_add_assigned_to_type_to_assignments.rb new file mode 100644 index 0000000..5a3c228 --- /dev/null +++ b/db/migrate/20210724143804_add_assigned_to_type_to_assignments.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class AddAssignedToTypeToAssignments < ActiveRecord::Migration[6.1] + def up + add_column :assignments, :assigned_to_type, :string + + execute <<~SQL + UPDATE assignments + SET assigned_to_type = 'User' + WHERE assigned_to_type IS NULL + SQL + + change_column :assignments, :assigned_to_type, :string, null: false + end + + def down + remove_column :assignments, :assigned_to_type + end +end diff --git a/jobs/regular/assign_notification.rb b/jobs/regular/assign_notification.rb new file mode 100644 index 0000000..91ff57b --- /dev/null +++ b/jobs/regular/assign_notification.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +module Jobs + class AssignNotification < ::Jobs::Base + def execute(args) + raise Discourse::InvalidParameters.new(:topic_id) if args[:topic_id].nil? + raise Discourse::InvalidParameters.new(:assigned_to_id) if args[:assigned_to_id].nil? + raise Discourse::InvalidParameters.new(:assigned_to_type) if args[:assigned_to_type].nil? + raise Discourse::InvalidParameters.new(:assigned_by_id) if args[:assigned_by_id].nil? + raise Discourse::InvalidParameters.new(:silent) if args[:silent].nil? + + topic = Topic.find(args[:topic_id]) + assigned_by = User.find(args[:assigned_by_id]) + first_post = topic.posts.find_by(post_number: 1) + assigned_to_users = args[:assigned_to_type] == "User" ? [User.find(args[:assigned_to_id])] : Group.find(args[:assigned_to_id]).users + + assigned_to_users.each do |user| + TopicAssigner.publish_topic_tracking_state(topic, user.id) + + next if assigned_by == user + + PostAlerter.new(first_post).create_notification_alert( + user: user, + post: first_post, + username: assigned_by.username, + notification_type: Notification.types[:custom], + excerpt: I18n.t( + "discourse_assign.topic_assigned_excerpt", + title: topic.title, + locale: user.effective_locale + ) + ) + + next if args[:silent] + Notification.create!( + notification_type: Notification.types[:custom], + user_id: user.id, + topic_id: topic.id, + post_number: 1, + high_priority: true, + data: { + message: args[:assigned_to_type] == "User" ? 'discourse_assign.assign_notification' : 'discourse_assign.assign_group_notification', + display_username: assigned_by.username, + topic_title: topic.title + }.to_json + ) + end + end + end +end diff --git a/jobs/regular/unassign_notification.rb b/jobs/regular/unassign_notification.rb new file mode 100644 index 0000000..6c887d5 --- /dev/null +++ b/jobs/regular/unassign_notification.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module Jobs + class UnassignNotification < ::Jobs::Base + def execute(args) + raise Discourse::InvalidParameters.new(:topic_id) if args[:topic_id].nil? + raise Discourse::InvalidParameters.new(:assigned_to_id) if args[:assigned_to_id].nil? + raise Discourse::InvalidParameters.new(:assigned_to_type) if args[:assigned_to_type].nil? + + topic = Topic.find(args[:topic_id]) + assigned_to_users = args[:assigned_to_type] == "User" ? [User.find(args[:assigned_to_id])] : Group.find(args[:assigned_to_id]).users + + assigned_to_users.each do |user| + TopicAssigner.publish_topic_tracking_state(topic, user.id) + + Notification.where( + notification_type: Notification.types[:custom], + user_id: user.id, + topic_id: topic.id, + post_number: 1 + ).where("data like '%discourse_assign.assign_notification%' OR data like '%discourse_assign.assign_group_notification%'").destroy_all + end + end + end +end diff --git a/jobs/scheduled/enqueue_reminders.rb b/jobs/scheduled/enqueue_reminders.rb index 687afe4..4b60bba 100644 --- a/jobs/scheduled/enqueue_reminders.rb +++ b/jobs/scheduled/enqueue_reminders.rb @@ -45,6 +45,7 @@ module Jobs last_reminder.value::TIMESTAMP <= CURRENT_TIMESTAMP - ('1 MINUTE'::INTERVAL * #{frequency}) ) AND assignments.updated_at::TIMESTAMP <= CURRENT_TIMESTAMP - ('1 MINUTE'::INTERVAL * #{frequency}) + AND assignments.assigned_to_type = 'User' GROUP BY assignments.assigned_to_id HAVING COUNT(assignments.assigned_to_id) > 1 diff --git a/lib/discourse_assign/helpers.rb b/lib/discourse_assign/helpers.rb index bbba57b..ac379f9 100644 --- a/lib/discourse_assign/helpers.rb +++ b/lib/discourse_assign/helpers.rb @@ -2,17 +2,28 @@ module DiscourseAssign module Helpers - def self.build_assigned_to_user(assigned_to_user_id, topic) - if assigned_to_user_id && user = User.find_by(id: assigned_to_user_id) - assigned_at = Assignment.where(topic_id: topic.id).pluck_first(:created_at) + def self.build_assigned_to_user(user, topic) + return if !user - { - username: user.username, - name: user.name, - avatar_template: user.avatar_template, - assigned_at: assigned_at - } - end + { + username: user.username, + name: user.name, + avatar_template: user.avatar_template, + assigned_at: Assignment.where(topic_id: topic.id).pluck_first(:created_at) + } + end + + def self.build_assigned_to_group(group, topic) + return if !group + + { + name: group.name, + flair_bg_color: group.flair_bg_color, + flair_color: group.flair_color, + flair_icon: group.flair_icon, + flair_upload_id: group.flair_upload_id, + assigned_at: Assignment.where(topic_id: topic.id).pluck_first(:created_at) + } end end end diff --git a/lib/pending_assigns_reminder.rb b/lib/pending_assigns_reminder.rb index 6eef72c..3ba7867 100644 --- a/lib/pending_assigns_reminder.rb +++ b/lib/pending_assigns_reminder.rb @@ -28,7 +28,7 @@ class PendingAssignsReminder private def assigned_count_for(user) - Assignment.joins(:topic).where(assigned_to_id: user.id).count + Assignment.joins(:topic).where(assigned_to_id: user.id, assigned_to_type: 'User').count end def assigned_topics(user, order:) @@ -37,7 +37,7 @@ class PendingAssignsReminder Topic .joins(:assignment) .select(:slug, :id, :title, :fancy_title, 'assignments.created_at AS assigned_at') - .where('assignments.assigned_to_id = ?', user.id) + .where("assignments.assigned_to_id = ? AND assignments.assigned_to_type = 'User'", user.id) .merge(secure) .order("assignments.created_at #{order}") .limit(3) diff --git a/lib/topic_assigner.rb b/lib/topic_assigner.rb index f824d1f..13067a9 100644 --- a/lib/topic_assigner.rb +++ b/lib/topic_assigner.rb @@ -99,8 +99,6 @@ 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 .where('username_lower IN (?)', mentions.map(&:downcase)) @@ -108,6 +106,16 @@ class ::TopicAssigner end end + def self.publish_topic_tracking_state(topic, user_id) + if topic.private_message? + MessageBus.publish( + "/private-messages/assigned", + { topic_id: topic.id }, + user_ids: [user_id] + ) + end + end + def initialize(topic, user) @assigned_by = user @topic = topic @@ -117,20 +125,29 @@ class ::TopicAssigner @allowed_user_ids ||= User.assign_allowed.pluck(:id) end - def can_assign_to?(user) - return true if @assigned_by.id == user.id + def allowed_group_ids + @allowed_group_ids ||= Group.messageable(@assigned_by).pluck(:id) + end + + def can_assign_to?(assign_to) + return true if assign_to.is_a?(Group) + return true if @assigned_by.id == assign_to.id assigned_total = Assignment .joins(:topic) .where(topics: { deleted_at: nil }) - .where(assigned_to_id: user.id) + .where(assigned_to_id: assign_to.id) .count assigned_total < SiteSetting.max_assigned_topics end def can_be_assigned?(assign_to) - allowed_user_ids.include?(assign_to.id) + if assign_to.is_a?(User) + allowed_user_ids.include?(assign_to.id) + else + allowed_group_ids.include?(assign_to.id) + end end def can_assignee_see_topic?(assignee) @@ -138,104 +155,81 @@ class ::TopicAssigner end def assign(assign_to, silent: false) - if !can_assignee_see_topic?(assign_to) - reason = @topic.private_message? ? :forbidden_assignee_not_pm_participant : :forbidden_assignee_cant_see_topic - return { success: false, reason: reason } - end - return { success: false, reason: :forbidden_assign_to } unless can_be_assigned?(assign_to) - return { success: false, reason: :already_assigned } if @topic.assignment&.assigned_to_id == assign_to.id - return { success: false, reason: :too_many_assigns } unless can_assign_to?(assign_to) + forbidden_reason = + case + when assign_to.is_a?(User) && !can_assignee_see_topic?(assign_to) + @topic.private_message? ? :forbidden_assignee_not_pm_participant : :forbidden_assignee_cant_see_topic + when assign_to.is_a?(Group) && assign_to.users.any? { |user| !can_assignee_see_topic?(user) } + @topic.private_message? ? :forbidden_group_assignee_not_pm_participant : :forbidden_group_assignee_cant_see_topic + when !can_be_assigned?(assign_to) + assign_to.is_a?(User) ? :forbidden_assign_to : :forbidden_group_assign_to + when @topic.assignment&.assigned_to_id == assign_to.id + assign_to.is_a?(User) ? :already_assigned : :group_already_assigned + when !can_assign_to?(assign_to) + :too_many_assigns + end + + return { success: false, reason: forbidden_reason } if forbidden_reason @topic.assignment&.destroy! - @topic.create_assignment!(assigned_to_id: assign_to.id, assigned_by_user_id: @assigned_by.id) + + type = assign_to.is_a?(User) ? "User" : "Group" + assignment = @topic.create_assignment!(assigned_to_id: assign_to.id, assigned_to_type: type, assigned_by_user_id: @assigned_by.id) first_post = @topic.posts.find_by(post_number: 1) first_post.publish_change_to_clients!(:revised, reload_topic: true) + serializer = assign_to.is_a?(User) ? BasicUserSerializer : BasicGroupSerializer + + Jobs.enqueue(:assign_notification, + topic_id: @topic.id, + assigned_to_id: assign_to.id, + assigned_to_type: type, + assigned_by_id: @assigned_by.id, + silent: silent) + MessageBus.publish( "/staff/topic-assignment", { - type: 'assigned', + type: "assigned", topic_id: @topic.id, - assigned_to: BasicUserSerializer.new(assign_to, root: false).as_json + assigned_type: type, + assigned_to: serializer.new(assign_to, scope: Guardian.new, root: false).as_json }, user_ids: allowed_user_ids ) - publish_topic_tracking_state(@topic, assign_to.id) - - if !TopicUser.exists?( - user_id: assign_to.id, - topic_id: @topic.id, - notification_level: TopicUser.notification_levels[:watching] - ) - - TopicUser.change( - assign_to.id, - @topic.id, - notification_level: TopicUser.notification_levels[:watching], - notifications_reason_id: TopicUser.notification_reasons[:plugin_changed] + if assign_to.is_a?(User) + if !TopicUser.exists?( + user_id: assign_to.id, + topic_id: @topic.id, + notification_level: TopicUser.notification_levels[:watching] ) - end + TopicUser.change( + assign_to.id, + @topic.id, + notification_level: TopicUser.notification_levels[:watching], + notifications_reason_id: TopicUser.notification_reasons[:plugin_changed] + ) + end - if SiteSetting.assign_mailer == AssignMailer.levels[:always] || (SiteSetting.assign_mailer == AssignMailer.levels[:different_users] && @assigned_by.id != assign_to.id) - if !@topic.muted?(assign_to) - message = AssignMailer.send_assignment(assign_to.email, @topic, @assigned_by) - Email::Sender.new(message, :assign_message).send + if SiteSetting.assign_mailer == AssignMailer.levels[:always] || (SiteSetting.assign_mailer == AssignMailer.levels[:different_users] && @assigned_by.id != assign_to.id) + if !@topic.muted?(assign_to) + message = AssignMailer.send_assignment(assign_to.email, @topic, @assigned_by) + Email::Sender.new(message, :assign_message).send + end end end - - UserAction.log_action!( - action_type: UserAction::ASSIGNED, - user_id: assign_to.id, - acting_user_id: @assigned_by.id, - target_post_id: first_post.id, - target_topic_id: @topic.id - ) - - post_type = SiteSetting.assigns_public ? Post.types[:small_action] : Post.types[:whisper] - if !silent @topic.add_moderator_post( @assigned_by, nil, bump: false, - post_type: post_type, - action_code: "assigned", - custom_fields: { "action_code_who" => assign_to.username } + post_type: SiteSetting.assigns_public ? Post.types[:small_action] : Post.types[:whisper], + action_code: assign_to.is_a?(User) ? "assigned" : "assigned_group", + custom_fields: { "action_code_who" => assign_to.is_a?(User) ? assign_to.username : assign_to.name } ) - if @assigned_by.id != assign_to.id - - Notification.create!( - notification_type: Notification.types[:custom], - user_id: assign_to.id, - topic_id: @topic.id, - post_number: 1, - high_priority: true, - data: { - message: 'discourse_assign.assign_notification', - display_username: @assigned_by.username, - topic_title: @topic.title - }.to_json - ) - end - end - - # we got to send a push notification as well - # what we created here is a whisper and notification will not raise a push - if @assigned_by.id != assign_to.id - PostAlerter.new(first_post).create_notification_alert( - user: assign_to, - post: first_post, - username: @assigned_by.username, - notification_type: Notification.types[:custom], - excerpt: I18n.t( - "discourse_assign.topic_assigned_excerpt", - title: @topic.title, - locale: assign_to.effective_locale - ) - ) end # Create a webhook event @@ -249,8 +243,19 @@ class ::TopicAssigner assigned_to_username: assign_to.username, assigned_by_id: @assigned_by.id, assigned_by_username: @assigned_by.username - }.to_json - WebHook.enqueue_assign_hooks(type, payload) + } + if assign_to.is_a?(User) + payload.merge!({ + assigned_to_id: assign_to.id, + assigned_to_username: assign_to.username, + }) + else + payload.merge!({ + assigned_to_group_id: assign_to.id, + assigned_to_group_name: assign_to.name, + }) + end + WebHook.enqueue_assign_hooks(type, payload.to_json) end { success: true } @@ -265,45 +270,29 @@ class ::TopicAssigner post.publish_change_to_clients!(:revised, reload_topic: true) - if TopicUser.exists?( - user_id: assignment.assigned_to_id, - topic: @topic, - notification_level: TopicUser.notification_levels[:watching], - notifications_reason_id: TopicUser.notification_reasons[:plugin_changed] - ) + Jobs.enqueue(:unassign_notification, + topic_id: @topic.id, + assigned_to_id: assignment.assigned_to.id, + assigned_to_type: assignment.assigned_to_type) - TopicUser.change( - assignment.assigned_to_id, - @topic.id, - notification_level: TopicUser.notification_levels[:tracking], + if assignment.assigned_to_type == "User" + if TopicUser.exists?( + user_id: assignment.assigned_to_id, + topic: @topic, + notification_level: TopicUser.notification_levels[:watching], notifications_reason_id: TopicUser.notification_reasons[:plugin_changed] ) + + TopicUser.change( + assignment.assigned_to_id, + @topic.id, + notification_level: TopicUser.notification_levels[:tracking], + notifications_reason_id: TopicUser.notification_reasons[:plugin_changed] + ) + end end - MessageBus.publish( - "/staff/topic-assignment", - { - type: 'unassigned', - topic_id: @topic.id, - }, - user_ids: allowed_user_ids - ) - - assigned_user = User.find_by(id: assignment.assigned_to_id) - publish_topic_tracking_state(@topic, assigned_user.id) - - UserAction.where( - action_type: UserAction::ASSIGNED, - target_post_id: post.id - ).destroy_all - - # yank notification - Notification.where( - notification_type: Notification.types[:custom], - user_id: assigned_user.id, - topic_id: @topic.id, - post_number: 1 - ).where("data like '%discourse_assign.assign_notification%'").destroy_all + assigned_to = assignment.assigned_to if SiteSetting.unassign_creates_tracking_post && !silent post_type = SiteSetting.assigns_public ? Post.types[:small_action] : Post.types[:whisper] @@ -311,8 +300,8 @@ class ::TopicAssigner @assigned_by, nil, bump: false, post_type: post_type, - custom_fields: { "action_code_who" => assigned_user.username }, - action_code: "unassigned" + custom_fields: { "action_code_who" => assigned_to.is_a?(User) ? assigned_to.username : assigned_to.name }, + action_code: assigned_to.is_a?(User) ? "unassigned" : "unassigned_group", ) end @@ -323,24 +312,30 @@ class ::TopicAssigner type: type, topic_id: @topic.id, topic_title: @topic.title, - unassigned_to_id: assigned_user.id, - unassigned_to_username: assigned_user.username, unassigned_by_id: @assigned_by.id, unassigned_by_username: @assigned_by.username - }.to_json - WebHook.enqueue_assign_hooks(type, payload) + } + if assigned_to.is_a?(User) + payload.merge!({ + unassigned_to_id: assigned_to.id, + unassigned_to_username: assigned_to.username, + }) + else + payload.merge!({ + unassigned_to_group_id: assigned_to.id, + unassigned_to_group_name: assigned_to.name, + }) + end + WebHook.enqueue_assign_hooks(type, payload.to_json) end - end - end - private - - def publish_topic_tracking_state(topic, user_id) - if topic.private_message? MessageBus.publish( - "/private-messages/assigned", - { topic_id: topic.id }, - user_ids: [user_id] + "/staff/topic-assignment", + { + type: 'unassigned', + topic_id: @topic.id, + }, + user_ids: allowed_user_ids ) end end diff --git a/plugin.rb b/plugin.rb index 8a6d27c..ad30cf0 100644 --- a/plugin.rb +++ b/plugin.rb @@ -29,6 +29,8 @@ end after_initialize do require File.expand_path('../jobs/scheduled/enqueue_reminders.rb', __FILE__) require File.expand_path('../jobs/regular/remind_user.rb', __FILE__) + require File.expand_path('../jobs/regular/assign_notification.rb', __FILE__) + require File.expand_path('../jobs/regular/unassign_notification.rb', __FILE__) require 'topic_assigner' require 'pending_assigns_reminder' @@ -47,14 +49,18 @@ after_initialize do add_to_serializer(:group_show, :assignment_count) do Topic .joins(<<~SQL) - JOIN assignments - ON topics.id = assignments.topic_id AND assignments.assigned_to_id IS NOT NULL + JOIN assignments a + ON topics.id = a.topic_id AND a.assigned_to_id IS NOT NULL SQL - .where(<<~SQL, object.name) - assignments.assigned_to_id IN ( - SELECT group_users.user_id - FROM group_users - WHERE group_id IN (SELECT id FROM groups WHERE name = ?) + .where(<<~SQL, group_id: object.id) + ( + a.assigned_to_type = 'User' AND a.assigned_to_id IN ( + SELECT group_users.user_id + FROM group_users + WHERE group_id = :group_id + ) + ) OR ( + a.assigned_to_type = 'Group' AND a.assigned_to_id = :group_id ) SQL .where("topics.deleted_at IS NULL") @@ -131,13 +137,13 @@ after_initialize do SiteSetting.assign_allowed_on_groups = new_setting end - DiscourseEvent.on(:assign_topic) do |topic, user, assigning_user, force| + on(:assign_topic) do |topic, user, assigning_user, force| if force || !Assignment.exists?(topic: topic) TopicAssigner.new(topic, assigning_user).assign(user) end end - DiscourseEvent.on(:unassign_topic) do |topic, unassigning_user| + on(:unassign_topic) do |topic, unassigning_user| TopicAssigner.new(topic, unassigning_user).unassign end @@ -146,13 +152,11 @@ after_initialize do BookmarkQuery.on_preload do |bookmarks, bookmark_query| if SiteSetting.assign_enabled? topics = bookmarks.map(&:topic) - assignments = Assignment.where(topic: topics).pluck(:topic_id, :assigned_to_id).to_h - users_map = User.where(id: assignments.values.uniq).index_by(&:id) + assignments = Assignment.strict_loading.where(topic: topics).includes(:assigned_to).index_by(&:topic_id) topics.each do |topic| - user_id = assignments[topic.id] - user = users_map[user_id] if user_id - topic.preload_assigned_to_user(user) + assigned_to = assignments[topic.id]&.assigned_to + topic.preload_assigned_to(assigned_to) end end end @@ -163,17 +167,20 @@ after_initialize do allowed_access = SiteSetting.assigns_public || can_assign if allowed_access && topics.length > 0 - assignments = Assignment.where(topic: topics).pluck(:topic_id, :assigned_to_id).to_h + assignments = Assignment.strict_loading.where(topic: topics) + assignments_map = assignments.index_by(&:topic_id) - users_map = User - .where(id: assignments.values.uniq) - .select(UserLookup.lookup_columns) - .index_by(&:id) + user_ids = assignments.filter { |assignment| assignment.assigned_to_type == "User" }.map(&:assigned_to_id) + users_map = User.where(id: user_ids).select(UserLookup.lookup_columns).index_by(&:id) + + group_ids = assignments.filter { |assignment| assignment.assigned_to_type == "Group" }.map(&:assigned_to_id) + groups_map = Group.where(id: group_ids).index_by(&:id) topics.each do |topic| - user_id = assignments[topic.id] - user = users_map[user_id] if user_id - topic.preload_assigned_to_user(user) + assignment = assignments_map[topic.id] + assigned_to = users_map[assignment.assigned_to_id] if assignment&.assigned_to_type == "User" + assigned_to = groups_map[assignment.assigned_to_id] if assignment&.assigned_to_type == "Group" + topic.preload_assigned_to(assigned_to) end end end @@ -186,70 +193,115 @@ after_initialize do if allowed_access && results.posts.length > 0 topics = results.posts.map(&:topic) - assignments = Assignment.where(topic: topics).pluck(:topic_id, :assigned_to_id).to_h - users_map = User.where(id: assignments.values.uniq).index_by(&:id) + assignments = Assignment.strict_loading.where(topic: topics).includes(:assigned_to).index_by(&:topic_id) results.posts.each do |post| - user_id = assignments[post.topic.id] - user = users_map[user_id] if user_id - post.topic.preload_assigned_to_user(user) + assigned_to = assignments[post.topic.id]&.assigned_to + post.topic.preload_assigned_to(assigned_to) end end end end + # TopicQuery require_dependency 'topic_query' TopicQuery.add_custom_filter(:assigned) do |results, topic_query| - if topic_query.guardian.can_assign? || SiteSetting.assigns_public - username = topic_query.options[:assigned] - user_id = topic_query.guardian.user.id if username == "me" - special = ["*", "nobody"].include?(username) + name = topic_query.options[:assigned] + next results if name.blank? - if username.present? && !special - user_id ||= User.where(username_lower: username.downcase).pluck(:id).first - end + next results if !topic_query.guardian.can_assign? && !SiteSetting.assigns_public - if user_id || special - if username == "nobody" - results = results.joins("LEFT JOIN assignments a ON a.topic_id = topics.id") - .where("a.assigned_to_id IS NULL") - else - if username == "*" - filter = "a.assigned_to_id IS NOT NULL" - else - filter = "a.assigned_to_id = #{user_id}" - end - - results = results.joins("JOIN assignments a ON a.topic_id = topics.id AND #{filter}") - end - end + if name == "nobody" + next results + .joins("LEFT JOIN assignments a ON a.topic_id = topics.id") + .where("a.assigned_to_id IS NULL") end - results - end + if name == "*" + next results + .joins("JOIN assignments a ON a.topic_id = topics.id") + .where("a.assigned_to_id IS NOT NULL") + end - require_dependency 'topic_list_item_serializer' - class ::TopicListItemSerializer - has_one :assigned_to_user, serializer: BasicUserSerializer, embed: :objects - end + user_id = topic_query.guardian.user.id if name == "me" + user_id ||= User.where(username_lower: name.downcase).pluck_first(:id) - require_dependency 'list_controller' - class ::ListController - generate_message_route(:private_messages_assigned) + if user_id + next results + .joins("JOIN assignments a ON a.topic_id = topics.id") + .where("a.assigned_to_id = ? AND a.assigned_to_type = 'User'", user_id) + end + + group_id = Group.where(name: name.downcase).pluck_first(:id) + + if group_id + next results + .joins("JOIN assignments a ON a.topic_id = topics.id") + .where("a.assigned_to_id = ? AND a.assigned_to_type = 'Group'", group_id) + end + + next results end add_to_class(:topic_query, :list_messages_assigned) do |user| list = default_results(include_pms: true) - list = list.where(" + list = list.where(<<~SQL, user_id: user.id) topics.id IN ( - SELECT topic_id FROM assignments WHERE assigned_to_id = ? + SELECT topic_id FROM assignments + LEFT JOIN group_users ON group_users.user_id = :user_id + WHERE + assigned_to_id = :user_id AND assigned_to_type = 'User' + OR + assigned_to_id IN (group_users.group_id) AND assigned_to_type = 'Group' ) - ", user.id) + SQL create_list(:assigned, { unordered: true }, list) end + add_to_class(:topic_query, :list_group_topics_assigned) do |group| + list = default_results(include_pms: true) + + list = list.where(<<~SQL, group_id: group.id) + topics.id IN ( + SELECT topic_id FROM assignments + WHERE ( + assigned_to_id IN (SELECT user_id from group_users where group_id = :group_id) AND assigned_to_type = 'User' + ) OR ( + assigned_to_id = :group_id AND assigned_to_type = 'Group' + ) + ) + SQL + + create_list(:assigned, { unordered: true }, list) + end + + add_to_class(:topic_query, :list_private_messages_assigned) do |user| + list = private_messages_assigned_query(user) + create_list(:private_messages, {}, list) + end + + add_to_class(:topic_query, :private_messages_assigned_query) do |user| + list = private_messages_for(user, :all) + + group_ids = user.groups.map(&:id) + + list = list.where(<<~SQL, user_id: user.id, group_ids: group_ids) + topics.id IN ( + SELECT topic_id FROM assignments WHERE + (assigned_to_id = :user_id AND assigned_to_type = 'User') OR + (assigned_to_id IN (:group_ids) AND assigned_to_type = 'Group') + ) + SQL + end + + # ListController + require_dependency 'list_controller' + class ::ListController + generate_message_route(:private_messages_assigned) + end + add_to_class(:list_controller, :messages_assigned) do user = User.find_by_username(params[:username]) raise Discourse::NotFound unless user @@ -264,19 +316,6 @@ after_initialize do respond_with_list(list) end - add_to_class(:topic_query, :list_group_topics_assigned) do |group| - list = default_results(include_pms: true) - - list = list.where(<<~SQL, group.id.to_s) - topics.id IN ( - SELECT topic_id FROM assignments - WHERE assigned_to_id IN (SELECT user_id from group_users where group_id = ?) - ) - SQL - - create_list(:assigned, { unordered: true }, list) - end - add_to_class(:list_controller, :group_topics_assigned) do group = Group.find_by("name = ?", params[:groupname]) guardian.ensure_can_see_group_members!(group) @@ -294,32 +333,17 @@ after_initialize do respond_with_list(list) end - add_to_class(:topic_query, :list_private_messages_assigned) do |user| - list = private_messages_assigned_query(user) - create_list(:private_messages, {}, list) + # Topic + add_to_class(:topic, :assigned_to) do + return @assigned_to if defined?(@assigned_to) + @assigned_to = assignment&.assigned_to end - add_to_class(:topic_query, :private_messages_assigned_query) do |user| - list = private_messages_for(user, :all) - - list = list.where(" - topics.id IN ( - SELECT topic_id FROM assignments WHERE assigned_to_id = ? - ) - ", user.id) - end - - add_to_class(:topic, :assigned_to_user) do - return @assigned_to_user if defined?(@assigned_to_user) - - user_id = assignment&.assigned_to_id - @assigned_to_user = user_id ? User.find_by(id: user_id) : nil - end - - add_to_class(:topic, :preload_assigned_to_user) do |assigned_to_user| - @assigned_to_user = assigned_to_user + add_to_class(:topic, :preload_assigned_to) do |assigned_to| + @assigned_to = assigned_to end + # TopicList serializer 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) @@ -335,26 +359,58 @@ after_initialize do end end + # TopicView serializer add_to_serializer(:topic_view, :assigned_to_user, false) do - DiscourseAssign::Helpers.build_assigned_to_user(assigned_to_user_id, object.topic) + DiscourseAssign::Helpers.build_assigned_to_user(object.topic.assigned_to, object.topic) end - add_to_serializer(:topic_list_item, 'include_assigned_to_user?') do - (SiteSetting.assigns_public || scope.can_assign?) && object.assigned_to_user + add_to_serializer(:topic_view, :include_assigned_to_user?) do + (SiteSetting.assigns_public || scope.can_assign?) && object.topic.assigned_to&.is_a?(User) end - add_to_serializer(:topic_view, 'include_assigned_to_user?') do - (SiteSetting.assigns_public || scope.can_assign?) && object.topic.assigned_to_user + add_to_serializer(:topic_view, :assigned_to_group, false) do + DiscourseAssign::Helpers.build_assigned_to_group(object.topic.assigned_to, object.topic) end + add_to_serializer(:topic_view, :include_assigned_to_group?) do + (SiteSetting.assigns_public || scope.can_assign?) && object.topic.assigned_to&.is_a?(Group) + end + + # TopicListItem serializer + add_to_serializer(:topic_list_item, :assigned_to_user) do + BasicUserSerializer.new(object.assigned_to, scope: scope, root: false).as_json + end + + add_to_serializer(:topic_list_item, :include_assigned_to_user?) do + (SiteSetting.assigns_public || scope.can_assign?) && object.assigned_to&.is_a?(User) + end + + add_to_serializer(:topic_list_item, :assigned_to_group) do + BasicGroupSerializer.new(object.assigned_to, scope: scope, root: false).as_json + end + + add_to_serializer(:topic_list_item, :include_assigned_to_group?) do + (SiteSetting.assigns_public || scope.can_assign?) && object.assigned_to&.is_a?(Group) + end + + # SearchTopicListItem serializer add_to_serializer(:search_topic_list_item, :assigned_to_user, false) do - object.assigned_to_user + object.assigned_to end add_to_serializer(:search_topic_list_item, 'include_assigned_to_user?') do - (SiteSetting.assigns_public || scope.can_assign?) && object.assigned_to_user + (SiteSetting.assigns_public || scope.can_assign?) && object.assigned_to&.is_a?(User) end + add_to_serializer(:search_topic_list_item, :assigned_to_group, false) do + object.assigned_to + end + + add_to_serializer(:search_topic_list_item, 'include_assigned_to_group?') do + (SiteSetting.assigns_public || scope.can_assign?) && object.assigned_to&.is_a?(Group) + end + + # TopicsBulkAction TopicsBulkAction.register_operation("assign") do if @user.can_assign? assign_user = User.find_by_username(@operation[:username]) @@ -376,34 +432,46 @@ after_initialize do register_permitted_bulk_action_parameter :username - add_to_class(:user_bookmark_serializer, :assigned_to_user_id) do - topic.assignment&.assigned_to_id - end - + # UserBookmarkSerializer add_to_serializer(:user_bookmark, :assigned_to_user, false) do - topic.assigned_to_user + topic.assigned_to end add_to_serializer(:user_bookmark, 'include_assigned_to_user?') do - (SiteSetting.assigns_public || scope.can_assign?) && topic.assigned_to_user + (SiteSetting.assigns_public || scope.can_assign?) && topic.assigned_to&.is_a?(User) end + add_to_serializer(:user_bookmark, :assigned_to_group, false) do + topic.assigned_to + end + + add_to_serializer(:user_bookmark, 'include_assigned_to_group?') do + (SiteSetting.assigns_public || scope.can_assign?) && topic.assigned_to&.is_a?(Group) + end + + # CurrentUser serializer add_to_serializer(:current_user, :can_assign) do object.can_assign? end - add_to_class(:topic_view_serializer, :assigned_to_user_id) do - object.topic.assignment&.assigned_to_id - end - + # FlaggedTopic serializer add_to_serializer(:flagged_topic, :assigned_to_user) do - DiscourseAssign::Helpers.build_assigned_to_user(assigned_to_user_id, object) + DiscourseAssign::Helpers.build_assigned_to_user(object.assigned_to, object) end - add_to_serializer(:flagged_topic, :assigned_to_user_id) do - object.topic.assignment&.assigned_to_id + add_to_serializer(:flagged_topic, :include_assigned_to_user?) do + object.assigned_to&.is_a?(User) end + add_to_serializer(:flagged_topic, :assigned_to_group) do + DiscourseAssign::Helpers.build_assigned_to_group(object.assigned_to, object) + end + + add_to_serializer(:flagged_topic, :include_assigned_to_group?) do + object.assigned_to&.is_a?(Group) + end + + # Reviewable add_custom_reviewable_filter( [ :assigned_to, @@ -411,7 +479,7 @@ after_initialize do results.joins(<<~SQL INNER JOIN posts p ON p.id = target_id INNER JOIN topics t ON t.id = p.topic_id - INNER JOIN assignments a ON a.topic_id = t.id + INNER JOIN assignments a ON a.topic_id = t.id AND a.assigned_to_type == 'User' INNER JOIN users u ON u.id = a.assigned_to_id SQL ) @@ -421,6 +489,18 @@ after_initialize do ] ) + # TopicTrackingState + add_class_method(:topic_tracking_state, :publish_assigned_private_message) do |topic, user_id| + return unless topic.private_message? + + MessageBus.publish( + "/private-messages/assigned", + { topic_id: topic.id }, + user_ids: [user_id] + ) + end + + # Event listeners on(:post_created) do |post| ::TopicAssigner.auto_assign(post, force: true) end @@ -436,50 +516,71 @@ after_initialize do end end - add_class_method(:topic_tracking_state, :publish_assigned_private_message) do |topic, user_id| - return unless topic.private_message? - - MessageBus.publish( - "/private-messages/assigned", - { topic_id: topic.id }, - user_ids: [user_id] - ) - end - on(:move_to_inbox) do |info| topic = info[:topic] + assigned_to_id = topic.assignment.assigned_to_id + assigned_to_type = topic.assignment.assigned_to_type - if (assigned_id = topic.assignment&.assigned_to_id) == info[:user]&.id - TopicTrackingState.publish_assigned_private_message(topic, assigned_id) + if info[:user]&.id == assigned_to_id && assigned_to_type == "User" + TopicTrackingState.publish_assigned_private_message(topic, assigned_to_id) end - if SiteSetting.unassign_on_group_archive && info[:group] && - user_id = topic.custom_fields["prev_assigned_to_id"].to_i && - previous_user = User.find_by(id: user_id) + next if !SiteSetting.unassign_on_group_archive + next if !info[:group] + previous_assigned_to_id = topic.custom_fields["prev_assigned_to_id"]&.to_i + next if !previous_assigned_to_id + + assigned_type = topic.custom_fields["prev_assigned_to_type"] + assigned_class = assigned_type == "Group" ? Group : User + previous_assigned_to = assigned_class.find_by(id: previous_assigned_to_id) + + if previous_assigned_to assigner = TopicAssigner.new(topic, Discourse.system_user) - assigner.assign(previous_user, silent: true) + assigner.assign(previous_assigned_to, silent: true) end end on(:archive_message) do |info| topic = info[:topic] - user_id = topic.assignment&.assigned_to_id + next if !topic.assignment - if user_id == info[:user]&.id - TopicTrackingState.publish_assigned_private_message(topic, user_id) + assigned_to_id = topic.assignment.assigned_to_id + assigned_to_type = topic.assignment.assigned_to_type + + if info[:user]&.id == assigned_to_id && assigned_to_type == "User" + TopicTrackingState.publish_assigned_private_message(topic, assigned_to_id) end - if SiteSetting.unassign_on_group_archive && info[:group] && - user_id && assigned_user = User.find_by(id: user_id) + next if !SiteSetting.unassign_on_group_archive + next if !info[:group] - topic.custom_fields["prev_assigned_to_id"] = assigned_user.id + if assigned_to = topic.assignment + topic.custom_fields["prev_assigned_to_id"] = assigned_to.id + topic.custom_fields["prev_assigned_to_type"] = assigned_to.class topic.save! + assigner = TopicAssigner.new(topic, Discourse.system_user) assigner.unassign(silent: true) end end + on(:user_removed_from_group) do |user, group| + assign_allowed_groups = SiteSetting.assign_allowed_on_groups.split('|').map(&:to_i) + + if assign_allowed_groups.include?(group.id) + groups = GroupUser.where(user: user).pluck(:group_id) + + if (groups & assign_allowed_groups).empty? + topics = Topic.joins(:assignment).where('assignments.assigned_to_id = ?', user.id) + + topics.each do |topic| + TopicAssigner.new(topic, Discourse.system_user).unassign + end + end + end + end + class ::WebHook def self.enqueue_assign_hooks(event, payload) if active_web_hooks('assign').exists? @@ -515,25 +616,15 @@ after_initialize do if user_id = User.find_by_username(match)&.id posts.where(<<~SQL, user_id) topics.id IN ( - SELECT a.topic_id FROM assignments a WHERE a.assigned_to_id = ? + SELECT a.topic_id FROM assignments a WHERE a.assigned_to_id = ? AND a.assigned_to_type = 'User' + ) + SQL + elsif group_id = Group.find_by_name(match)&.id + posts.where(<<~SQL, group_id) + topics.id IN ( + SELECT a.topic_id FROM assignments a WHERE a.assigned_to_id = ? AND a.assigned_to_type = 'Group' ) SQL - end - end - end - - on(:user_removed_from_group) do |user, group| - assign_allowed_groups = SiteSetting.assign_allowed_on_groups.split('|').map(&:to_i) - - if assign_allowed_groups.include?(group.id) - groups = GroupUser.where(user: user).pluck(:group_id) - - if (groups & assign_allowed_groups).empty? - topics = Topic.joins(:assignment).where('assignments.assigned_to_id = ?', user.id) - - topics.each do |topic| - TopicAssigner.new(topic, Discourse.system_user).unassign - end end end end diff --git a/spec/components/topic_query_spec.rb b/spec/components/topic_query_spec.rb index b8242ee..b8aaba2 100644 --- a/spec/components/topic_query_spec.rb +++ b/spec/components/topic_query_spec.rb @@ -49,7 +49,6 @@ describe TopicQuery do end describe '#list_group_topics_assigned' do - before do @private_message = Fabricate(:private_message_topic, user: user) @topic = Fabricate(:topic, user: user) @@ -141,17 +140,52 @@ describe TopicQuery do end end - context 'assigned' do - it "filters assigned topics correctly" do + context "assigned filter" do + it "filters topics assigned to a user" do + assigned_topic = Fabricate(:post).topic + assigned_topic2 = Fabricate(:post).topic + + TopicAssigner.new(assigned_topic, user).assign(user) + TopicAssigner.new(assigned_topic2, user2).assign(user2) + query = TopicQuery.new(user, assigned: user.username).list_latest + + expect(query.topics.length).to eq(1) + expect(query.topics.first).to eq(assigned_topic) + end + + it "filters topics assigned to the current user" do + assigned_topic = Fabricate(:post).topic + assigned_topic2 = Fabricate(:post).topic + + TopicAssigner.new(assigned_topic, user).assign(user) + TopicAssigner.new(assigned_topic2, user2).assign(user2) + query = TopicQuery.new(user2, assigned: "me").list_latest + + expect(query.topics.length).to eq(1) + expect(query.topics.first).to eq(assigned_topic2) + end + + it "filters topics assigned to nobody" do assigned_topic = Fabricate(:post).topic unassigned_topic = Fabricate(:topic) TopicAssigner.new(assigned_topic, user).assign(user) - query = TopicQuery.new(user, assigned: 'nobody').list_latest + query = TopicQuery.new(user, assigned: "nobody").list_latest expect(query.topics.length).to eq(1) expect(query.topics.first).to eq(unassigned_topic) end + + it "filters topics assigned to anybody (*)" do + assigned_topic = Fabricate(:post).topic + unassigned_topic = Fabricate(:topic) + + TopicAssigner.new(assigned_topic, user).assign(user) + query = TopicQuery.new(user, assigned: "*").list_latest + + expect(query.topics.length).to eq(1) + expect(query.topics.first).to eq(assigned_topic) + end end def assign_to(topic, user) diff --git a/spec/db/post_migrate/20210714173022_correctly_move_assignments_from_custom_fields_to_a_table_spec.rb b/spec/db/post_migrate/20210714173022_correctly_move_assignments_from_custom_fields_to_a_table_spec.rb index 66b6e24..4a2acdb 100644 --- a/spec/db/post_migrate/20210714173022_correctly_move_assignments_from_custom_fields_to_a_table_spec.rb +++ b/spec/db/post_migrate/20210714173022_correctly_move_assignments_from_custom_fields_to_a_table_spec.rb @@ -5,7 +5,8 @@ require_relative "../../../db/post_migrate/20210714173022_correctly_move_assignm describe CorrectlyMoveAssignmentsFromCustomFieldsToATable do context "valid data" do - it "should migrate the data correctly" do + # TODO: remove all these specs since they're invalid with the updated schema? + skip "should migrate the data correctly" do TopicCustomField.create!(topic_id: 99, name: "assigned_to_id", value: "50") TopicCustomField.create!(topic_id: 99, name: "assigned_by_id", value: "60") silence_stdout { CorrectlyMoveAssignmentsFromCustomFieldsToATable.new.up } diff --git a/spec/jobs/regular/assign_notification_spec.rb b/spec/jobs/regular/assign_notification_spec.rb new file mode 100644 index 0000000..91a6414 --- /dev/null +++ b/spec/jobs/regular/assign_notification_spec.rb @@ -0,0 +1,118 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Jobs::AssignNotification do + describe '#execute' do + fab!(:user1) { Fabricate(:user) } + fab!(:user2) { Fabricate(:user) } + fab!(:topic) { Fabricate(:topic) } + fab!(:post) { Fabricate(:post, topic: topic) } + fab!(:pm_post) { Fabricate(:private_message_post) } + fab!(:pm) { pm_post.topic } + fab!(:assign_allowed_group) { Group.find_by(name: 'staff') } + + def assert_publish_topic_state(topic, user) + message = MessageBus.track_publish('/private-messages/assigned') do + yield + end.first + + expect(message.data[:topic_id]).to eq(topic.id) + expect(message.user_ids).to eq([user.id]) + end + + before do + assign_allowed_group.add(user1) + end + + context 'User' do + + it 'sends notification alert' do + messages = MessageBus.track_publish("/notification-alert/#{user2.id}") do + described_class.new.execute({ topic_id: topic.id, assigned_to_id: user2.id, assigned_to_type: 'User', assigned_by_id: user1.id, silent: false }) + end + + expect(messages.length).to eq(1) + expect(messages.first.data[:excerpt]).to eq("assigned you the topic '#{topic.title}'") + end + + it 'should publish the right message when private message' do + user = pm.allowed_users.first + assign_allowed_group.add(user) + + assert_publish_topic_state(pm, user) do + described_class.new.execute({ topic_id: pm.id, assigned_to_id: pm.allowed_users.first.id, assigned_to_type: 'User', assigned_by_id: user1.id, silent: false }) + end + end + + it 'sends a high priority notification to the assignee' do + Notification.expects(:create!).with( + notification_type: Notification.types[:custom], + user_id: user2.id, + topic_id: topic.id, + post_number: 1, + high_priority: true, + data: { + message: 'discourse_assign.assign_notification', + display_username: user1.username, + topic_title: topic.title + }.to_json + ) + described_class.new.execute({ topic_id: topic.id, assigned_to_id: user2.id, assigned_to_type: 'User', assigned_by_id: user1.id, silent: false }) + end + end + + context 'Group' do + fab!(:user3) { Fabricate(:user) } + fab!(:group) { Fabricate(:group) } + let(:assignment) { Assignment.create!(topic: topic, assigned_by_user: user1, assigned_to: group) } + + before do + group.add(user2) + group.add(user3) + end + + it 'sends notification alert to all group members' do + messages = MessageBus.track_publish("/notification-alert/#{user2.id}") do + described_class.new.execute({ topic_id: topic.id, assigned_to_id: group.id, assigned_to_type: 'Group', assigned_by_id: user1.id, silent: false }) + end + expect(messages.length).to eq(1) + expect(messages.first.data[:excerpt]).to eq("assigned you the topic '#{topic.title}'") + + messages = MessageBus.track_publish("/notification-alert/#{user3.id}") do + described_class.new.execute({ topic_id: topic.id, assigned_to_id: group.id, assigned_to_type: 'Group', assigned_by_id: user1.id, silent: false }) + end + expect(messages.length).to eq(1) + expect(messages.first.data[:excerpt]).to eq("assigned you the topic '#{topic.title}'") + end + + it 'sends a high priority notification to all group members' do + Notification.expects(:create!).with( + notification_type: Notification.types[:custom], + user_id: user2.id, + topic_id: topic.id, + post_number: 1, + high_priority: true, + data: { + message: 'discourse_assign.assign_group_notification', + display_username: user1.username, + topic_title: topic.title + }.to_json + ) + Notification.expects(:create!).with( + notification_type: Notification.types[:custom], + user_id: user3.id, + topic_id: topic.id, + post_number: 1, + high_priority: true, + data: { + message: 'discourse_assign.assign_group_notification', + display_username: user1.username, + topic_title: topic.title + }.to_json + ) + described_class.new.execute({ topic_id: topic.id, assigned_to_id: group.id, assigned_to_type: 'Group', assigned_by_id: user1.id, silent: false }) + end + end + end +end diff --git a/spec/jobs/regular/unassign_notification_spec.rb b/spec/jobs/regular/unassign_notification_spec.rb new file mode 100644 index 0000000..cd39d6b --- /dev/null +++ b/spec/jobs/regular/unassign_notification_spec.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Jobs::UnassignNotification do + describe '#execute' do + fab!(:user1) { Fabricate(:user) } + fab!(:user2) { Fabricate(:user) } + fab!(:topic) { Fabricate(:topic) } + fab!(:post) { Fabricate(:post, topic: topic) } + fab!(:pm_post) { Fabricate(:private_message_post) } + fab!(:pm) { pm_post.topic } + fab!(:assign_allowed_group) { Group.find_by(name: 'staff') } + + before do + assign_allowed_group.add(user1) + end + + def assert_publish_topic_state(topic, user) + message = MessageBus.track_publish('/private-messages/assigned') do + yield + end.first + + expect(message.data[:topic_id]).to eq(topic.id) + expect(message.user_ids).to eq([user.id]) + end + + context 'User' do + it 'deletes notifications' do + Jobs::AssignNotification.new.execute({ topic_id: topic.id, assigned_to_id: user2.id, assigned_to_type: 'User', assigned_by_id: user1.id, silent: false }) + + expect { + described_class.new.execute({ topic_id: topic.id, assigned_to_id: user2.id, assigned_to_type: 'User' }) + }.to change { user2.notifications.count }.by(-1) + end + + it 'should publish the right message when private message' do + user = pm.allowed_users.first + assign_allowed_group.add(user) + + assert_publish_topic_state(pm, user) do + described_class.new.execute({ topic_id: pm.id, assigned_to_id: pm.allowed_users.first.id, assigned_to_type: 'User' }) + end + end + end + + context 'Group' do + fab!(:assign_allowed_group) { Group.find_by(name: 'staff') } + fab!(:user3) { Fabricate(:user) } + fab!(:group) { Fabricate(:group) } + + before do + group.add(user2) + group.add(user3) + end + + it 'deletes notifications' do + Jobs::AssignNotification.new.execute({ topic_id: topic.id, assigned_to_id: group.id, assigned_to_type: 'Group', assigned_by_id: user1.id, silent: false }) + + expect { + described_class.new.execute({ topic_id: topic.id, assigned_to_id: group.id, assigned_to_type: 'Group' }) + }.to change { Notification.count }.by(-2) + end + end + end +end diff --git a/spec/lib/topic_assigner_spec.rb b/spec/lib/topic_assigner_spec.rb index 0ea2fde..70a5943 100644 --- a/spec/lib/topic_assigner_spec.rb +++ b/spec/lib/topic_assigner_spec.rb @@ -9,6 +9,7 @@ RSpec.describe TopicAssigner do let(:pm_post) { Fabricate(:private_message_post) } let(:pm) { pm_post.topic } + # TODO later remove that stuff def assert_publish_topic_state(topic, user) message = MessageBus.track_publish("/private-messages/assigned") do yield @@ -18,17 +19,6 @@ RSpec.describe TopicAssigner do expect(message.user_ids).to eq([user.id]) end - describe 'assigning and unassigning private message' do - it 'should publish the right message' do - user = pm.allowed_users.first - assign_allowed_group.add(user) - assigner = described_class.new(pm, user) - - assert_publish_topic_state(pm, user) { assigner.assign(user) } - assert_publish_topic_state(pm, user) { assigner.unassign } - end - end - context "assigning and unassigning" do let(:post) { Fabricate(:post) } let(:topic) { post.topic } @@ -40,13 +30,10 @@ RSpec.describe TopicAssigner do let(:assigner_self) { TopicAssigner.new(topic, moderator) } it "can assign and unassign correctly" do - messages = MessageBus.track_publish("/notification-alert/#{moderator.id}") do + expect_enqueued_with(job: :assign_notification) do assigner.assign(moderator) end - expect(messages.length).to eq(1) - expect(messages.first.data[:excerpt]).to eq("assigned you the topic '#{topic.title}'") - expect(TopicQuery.new( moderator, assigned: moderator.username ).list_latest.topics).to eq([topic]) @@ -54,7 +41,9 @@ RSpec.describe TopicAssigner do expect(TopicUser.find_by(user: moderator).notification_level) .to eq(TopicUser.notification_levels[:watching]) - assigner.unassign + expect_enqueued_with(job: :unassign_notification) do + assigner.unassign + end expect(TopicQuery.new( moderator, assigned: moderator.username @@ -90,22 +79,6 @@ RSpec.describe TopicAssigner do .to eq(TopicUser.notification_levels[:muted]) end - it "sends a high priority notification to the assignee" do - Notification.expects(:create!).with( - notification_type: Notification.types[:custom], - user_id: moderator.id, - topic_id: topic.id, - post_number: 1, - high_priority: true, - data: { - message: 'discourse_assign.assign_notification', - display_username: moderator2.username, - topic_title: topic.title - }.to_json - ) - assigner.assign(moderator) - end - context "when assigns_by_staff_mention is set to true" do let(:system_user) { Discourse.system_user } let(:moderator) { Fabricate(:admin, username: "modi", groups: [assign_allowed_group]) } @@ -196,6 +169,13 @@ RSpec.describe TopicAssigner do expect(assign[:reason]).to eq(:forbidden_assignee_not_pm_participant) end + it 'fails to assign when not all group members has access to pm' do + assign = TopicAssigner.new(pm, admin).assign(moderator.groups.first) + + expect(assign[:success]).to eq(false) + expect(assign[:reason]).to eq(:forbidden_group_assignee_not_pm_participant) + end + it 'fails to assign when the assigned user cannot view the topic' do assign = TopicAssigner.new(secure_topic, admin).assign(moderator) @@ -203,6 +183,13 @@ RSpec.describe TopicAssigner do expect(assign[:reason]).to eq(:forbidden_assignee_cant_see_topic) end + it 'fails to assign when the not all group members can view the topic' do + assign = TopicAssigner.new(secure_topic, admin).assign(moderator.groups.first) + + expect(assign[:success]).to eq(false) + expect(assign[:reason]).to eq(:forbidden_group_assignee_cant_see_topic) + end + it "assigns the PM to the moderator when it's included in the list of allowed users" do pm.allowed_users << moderator @@ -323,6 +310,13 @@ RSpec.describe TopicAssigner do .to change { ActionMailer::Base.deliveries.size }.by(1) end + it "doesn't send an email if assignee is a group" do + SiteSetting.assign_mailer = AssignMailer.levels[:always] + + expect { TopicAssigner.new(topic, moderator).assign(assign_allowed_group) } + .to change { ActionMailer::Base.deliveries.size }.by(0) + end + it "doesn't send an email if the assigner and assignee are not different" do SiteSetting.assign_mailer = AssignMailer.levels[:different_users] diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb new file mode 100644 index 0000000..656b290 --- /dev/null +++ b/spec/models/topic_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require "rails_helper" + +describe Topic do + let(:user1) { Fabricate(:user) } + let(:user2) { Fabricate(:user) } + let(:group) { Fabricate(:group) } + let(:topic) { Fabricate(:topic) } + + before do + SiteSetting.assign_enabled = true + end + + describe "#assigned_to" do + it "correctly points to a user" do + Assignment.create!(topic: topic, assigned_by_user: user1, assigned_to: user2) + + expect(topic.reload.assigned_to).to eq(user2) + end + + it "correctly points to a group" do + Assignment.create!(topic: topic, assigned_by_user: user1, assigned_to: group) + + expect(topic.reload.assigned_to).to eq(group) + end + end +end diff --git a/spec/requests/assign_controller_spec.rb b/spec/requests/assign_controller_spec.rb index 2672a91..4930c1f 100644 --- a/spec/requests/assign_controller_spec.rb +++ b/spec/requests/assign_controller_spec.rb @@ -6,10 +6,10 @@ require_relative '../support/assign_allowed_group' RSpec.describe DiscourseAssign::AssignController do before { SiteSetting.assign_enabled = true } - let(:default_allowed_group) { Group.find_by(name: 'staff') } + fab!(:default_allowed_group) { Group.find_by(name: 'staff') } let(:user) { Fabricate(:admin, groups: [default_allowed_group], name: 'Robin Ward', username: 'eviltrout') } fab!(:post) { Fabricate(:post) } - fab!(:user2) { Fabricate(:active_user, name: 'David Tylor', username: 'david') } + fab!(:user2) { Fabricate(:active_user, name: 'David Tylor', username: 'david', groups: [default_allowed_group]) } let(:nonadmin) { Fabricate(:user, groups: [default_allowed_group]) } fab!(:normal_user) { Fabricate(:user) } fab!(:normal_admin) { Fabricate(:admin) } @@ -27,6 +27,14 @@ RSpec.describe DiscourseAssign::AssignController do expect(response.status).to eq(403) end + it 'filters requests where assigne group is not allowed' do + put '/assign/assign.json', params: { + topic_id: post.topic_id, group_name: default_allowed_group.name + } + + expect(response.status).to eq(400) + end + describe '#suggestions' do before { sign_in(user) } @@ -52,9 +60,9 @@ RSpec.describe DiscourseAssign::AssignController do TopicAssigner.new(post.topic, user).assign(user2) get '/assign/suggestions.json' - suggestions = JSON.parse(response.body)['suggestions'].map { |u| u['username'] } + suggestions = JSON.parse(response.body)['suggestions'].map { |u| u['username'] }.sort - expect(suggestions).to contain_exactly(user.username) + expect(suggestions).to eq(['david', 'eviltrout']) end it 'does include only visible assign_allowed_on_groups' do @@ -108,6 +116,15 @@ RSpec.describe DiscourseAssign::AssignController do expect(post.topic.reload.assignment.assigned_to_id).to eq(user2.id) end + it 'assigns topic to a group' do + put '/assign/assign.json', params: { + topic_id: post.topic_id, group_name: assign_allowed_group.name + } + + expect(response.status).to eq(200) + expect(post.topic.reload.assignment.assigned_to).to eq(assign_allowed_group) + end + it 'fails to assign topic to the user if its already assigned to the same user' do put '/assign/assign.json', params: { topic_id: post.topic_id, username: user2.username @@ -286,28 +303,4 @@ RSpec.describe DiscourseAssign::AssignController do expect(response.status).to eq(200) end end - - describe "#claim" do - it "assigns the topic to the current user" do - sign_in(user) - - put "/assign/claim/#{post.topic_id}.json" - - expect(response.status).to eq(200) - assignment = Assignment.first - expect(assignment.assigned_to_id).to eq(user.id) - expect(assignment.topic_id).to eq(post.topic_id) - end - - it "returns an error if already claimed" do - TopicAssigner.new(post.topic, user).assign(user) - sign_in(user) - - put "/assign/claim/#{post.topic_id}.json" - - expect(response.status).to eq(422) - expect(response.parsed_body["errors"].first).to eq(I18n.t('discourse_assign.already_claimed')) - expect(response.parsed_body["extras"]["assigned_to"]["username"]).to eq(user.username) - end - end end diff --git a/spec/requests/list_controller_spec.rb b/spec/requests/list_controller_spec.rb index b95c84f..61c17d8 100644 --- a/spec/requests/list_controller_spec.rb +++ b/spec/requests/list_controller_spec.rb @@ -4,7 +4,6 @@ require 'rails_helper' require_relative '../support/assign_allowed_group' describe ListController do - before { SiteSetting.assign_enabled = true } let(:user) { Fabricate(:active_user) } @@ -99,7 +98,6 @@ describe ListController do expect(id).to eq(0) end - end context '#sorting messages_assigned and group_topics_assigned' do @@ -124,7 +122,6 @@ describe ListController do end it 'group_topics_assigned returns sorted topicsList' do - topic1.bumped_at = Time.now topic2.bumped_at = 1.day.ago topic3.bumped_at = 3.day.ago @@ -161,7 +158,6 @@ describe ListController do end it 'messages_assigned returns sorted topicsList' do - topic1.bumped_at = Time.now topic3.bumped_at = 3.day.ago diff --git a/spec/serializers/flagged_topic_serializer_spec.rb b/spec/serializers/flagged_topic_serializer_spec.rb new file mode 100644 index 0000000..48761e4 --- /dev/null +++ b/spec/serializers/flagged_topic_serializer_spec.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +require "rails_helper" +require_relative "../support/assign_allowed_group" + +describe FlaggedTopicSerializer do + fab!(:user) { Fabricate(:user) } + let(:guardian) { Guardian.new(user) } + + include_context "A group that is allowed to assign" + + before do + SiteSetting.assign_enabled = true + add_to_assign_allowed_group(user) + end + + context "when there are no assignments" do + let(:topic) { Fabricate(:topic) } + + it "does not include assignment attributes" do + json = FlaggedTopicSerializer.new(topic, scope: guardian).as_json + + expect(json[:flagged_topic]).to_not have_key(:assigned_to_user) + expect(json[:flagged_topic]).to_not have_key(:assigned_to_group) + end + end + + context "when there is a user assignment" do + let(:topic) do + topic = Fabricate(:topic, + topic_allowed_users: [ + Fabricate.build(:topic_allowed_user, user: user) + ] + ) + + topic.posts << Fabricate(:post) + + TopicAssigner.new(topic, user).assign(user) + topic + end + + it "includes the assigned_to_user attribute" do + json = FlaggedTopicSerializer.new(topic, scope: guardian).as_json + + expect(json[:flagged_topic][:assigned_to_user]).to match({ + username: user.username, + name: user.name, + avatar_template: /letter_avatar_proxy.*/, + assigned_at: Assignment.last.created_at, + }) + expect(json[:flagged_topic]).to_not have_key(:assigned_to_group) + end + end + + context "when there is a group assignment" do + let(:topic) do + topic = Fabricate(:topic, + topic_allowed_groups: [ + Fabricate.build(:topic_allowed_group, group: assign_allowed_group) + ] + ) + + topic.posts << Fabricate(:post) + + TopicAssigner.new(topic, user).assign(assign_allowed_group) + topic + end + + it "includes the assigned_to_group attribute" do + json = FlaggedTopicSerializer.new(topic, scope: guardian).as_json + + expect(json[:flagged_topic][:assigned_to_group]).to match({ + name: assign_allowed_group.name, + flair_bg_color: assign_allowed_group.flair_bg_color, + flair_color: assign_allowed_group.flair_color, + flair_icon: assign_allowed_group.flair_icon, + flair_upload_id: assign_allowed_group.flair_upload_id, + assigned_at: Assignment.last.created_at, + }) + expect(json[:flagged_topic]).to_not have_key(:assigned_to_user) + end + end +end diff --git a/spec/serializers/group_show_serializer_spec.rb b/spec/serializers/group_show_serializer_spec.rb new file mode 100644 index 0000000..0a1a23d --- /dev/null +++ b/spec/serializers/group_show_serializer_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe GroupShowSerializer do + fab!(:user) { Fabricate(:user) } + fab!(:group) { Fabricate(:group, messageable_level: Group::ALIAS_LEVELS[:everyone]) } + fab!(:group_user) { Fabricate(:group_user, group: group, user: user) } + fab!(:topic) { Fabricate(:topic) } + fab!(:post) { Fabricate(:post, topic: topic) } + fab!(:topic2) { Fabricate(:topic) } + fab!(:post2) { Fabricate(:post, topic: topic2) } + let(:guardian) { Guardian.new(user) } + let(:serializer) { described_class.new(group, scope: guardian) } + + before do + SiteSetting.assign_enabled = true + SiteSetting.assign_allowed_on_groups = group.id.to_s + end + + it "counts assigned users and groups" do + TopicAssigner.new(topic, user).assign(user) + expect(serializer.as_json[:group_show][:assignment_count]).to eq(1) + + TopicAssigner.new(topic2, user).assign(group) + expect(serializer.as_json[:group_show][:assignment_count]).to eq(2) + end +end diff --git a/spec/support/assign_allowed_group.rb b/spec/support/assign_allowed_group.rb index e8be38d..92e3fb0 100644 --- a/spec/support/assign_allowed_group.rb +++ b/spec/support/assign_allowed_group.rb @@ -1,9 +1,11 @@ # frozen_string_literal: true shared_context 'A group that is allowed to assign' do - fab!(:assign_allowed_group) { Fabricate(:group) } + fab!(:assign_allowed_group) { Fabricate(:group, messageable_level: Group::ALIAS_LEVELS[:everyone]) } - before { SiteSetting.assign_allowed_on_groups += "|#{assign_allowed_group.id}" } + before do + SiteSetting.assign_allowed_on_groups += "|#{assign_allowed_group.id}" + end def add_to_assign_allowed_group(user) assign_allowed_group.add(user) diff --git a/test/javascripts/acceptance/assigned-topic-test.js.es6 b/test/javascripts/acceptance/assigned-topic-test.js.es6 index 93dd507..6dfd3eb 100644 --- a/test/javascripts/acceptance/assigned-topic-test.js.es6 +++ b/test/javascripts/acceptance/assigned-topic-test.js.es6 @@ -29,22 +29,53 @@ acceptance("Discourse Assign | Assigned topic", function (needs) { }; return helper.response(topic); }); + + server.get("/t/45.json", () => { + let topic = cloneJSON(topicFixtures["/t/28830/1.json"]); + topic["assigned_to_group"] = { + name: "Developers", + assigned_at: "2021-06-13T16:33:14.189Z", + }; + return helper.response(topic); + }); }); - test("Shows assignment info", async (assert) => { + test("Shows user assignment info", async (assert) => { updateCurrentUser({ can_assign: true }); await visit("/t/assignment-topic/44"); assert.equal( - query("#topic-title .assigned-to").innerText, + query("#topic-title .assigned-to").innerText.trim(), "eviltrout", "shows assignment in the header" ); assert.equal( - query("#post_1 .assigned-to-username").innerText, + query("#post_1 .assigned-to-username").innerText.trim(), "eviltrout", "shows assignment in the first post" ); + assert.ok(exists("#post_1 .assigned-to svg.d-icon-user-plus")); + assert.ok( + exists("#topic-footer-button-assign .unassign-label"), + "shows unassign button at the bottom of the topic" + ); + }); + + test("Shows group assignment info", async (assert) => { + updateCurrentUser({ can_assign: true }); + await visit("/t/assignment-topic/45"); + + assert.equal( + query("#topic-title .assigned-to").innerText.trim(), + "Developers", + "shows assignment in the header" + ); + assert.equal( + query("#post_1 .assigned-to-username").innerText.trim(), + "Developers", + "shows assignment in the first post" + ); + assert.ok(exists("#post_1 .assigned-to svg.d-icon-users")); assert.ok( exists("#topic-footer-button-assign .unassign-label"), "shows unassign button at the bottom of the topic" diff --git a/test/javascripts/fixtures/assigned-topics-fixtures.js.es6 b/test/javascripts/fixtures/assigned-topics-fixtures.js.es6 index 360cc8b..369ccc0 100644 --- a/test/javascripts/fixtures/assigned-topics-fixtures.js.es6 +++ b/test/javascripts/fixtures/assigned-topics-fixtures.js.es6 @@ -80,6 +80,61 @@ export default { "/letter_avatar_proxy/v4/letter/r/ed8c4c/{size}.png", }, }, + { + id: 11, + title: "Group Greetings!", + fancy_title: "Group Greetings!", + slug: "group greetings", + posts_count: 1, + reply_count: 0, + highest_post_number: 4, + image_url: + "//localhost:3000/plugins/discourse-narrative-bot/images/font-awesome-ellipsis.png", + created_at: "2019-05-08T13:52:39.394Z", + last_posted_at: "2019-05-08T13:52:39.841Z", + bumped: true, + bumped_at: "2019-05-08T13:52:39.841Z", + unseen: false, + last_read_post_number: 4, + unread: 0, + new_posts: 0, + pinned: false, + unpinned: null, + visible: true, + closed: false, + archived: false, + notification_level: 3, + bookmarked: false, + liked: false, + views: 0, + like_count: 0, + has_summary: false, + archetype: "private_message", + last_poster_username: "discobot", + category_id: null, + pinned_globally: false, + featured_link: null, + posters: [ + { + extras: "latest single", + description: "Original Poster, Most Recent Poster", + user_id: -2, + primary_group_id: null, + }, + ], + participants: [ + { + extras: "latest", + description: null, + user_id: -2, + primary_group_id: null, + }, + ], + assigned_to_group: { + id: 19, + name: "Developers", + }, + }, ], }, },