From 2f998abc6cded74a442dfe4165ee3998b65b672f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Guitaut?= Date: Wed, 6 Sep 2023 18:10:31 +0200 Subject: [PATCH] FIX: Display assignments in user menu properly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, we display a mix of topics and notifications in the user menu assignments tab. This has a number of issues like having to maintain hard to understand code instead of simply relying on the notifications system we have, we can’t display more than one assignment per topic and it’s not clear if an assignment is for a topic or a post nor if it’s a group assignment or an individual one. This patch addresses those issues by relying on the notifications system we’re using for most of the other user menu tabs instead of a custom implementation. This led to some heavy refactoring but it was worthwhile as things are a bit more normalized and easier to reason about. Instead of serializing topics with different attributes to access various assignments, we now simply return a notification for each existing assignment. The UI changed a bit: tooltips are now explaining if the assignment is for a topic or a post and if it’s for an individual or a group. Icons are also properly set (so no more individual icon for group assignments) and the assigned group name is displayed. The background jobs signatures changed a bit (`assignment_id` is now needed for the unassign job) so when deploying this patch, it’s expected to have some jobs failing. That’s why a post-migration has been written to handle the creation of missing notifications and the deletion of extra notifications too. --- .../discourse_assign/assign_controller.rb | 41 --- app/jobs/regular/assign_notification.rb | 73 +---- app/jobs/regular/unassign_notification.rb | 36 +-- app/models/assignment.rb | 28 +- .../components/user-menu/assigns-list.js | 30 +- .../initializers/assign-user-menu.js | 79 +++++- .../initializers/extend-for-assigns.js | 2 +- .../discourse/lib/user-menu/assign-item.js | 59 ---- config/locales/client.en.yml | 13 +- config/routes.rb | 1 - ...152903_ensure_notifications_consistency.rb | 23 ++ lib/assigner.rb | 42 +-- lib/discourse_assign/create_notification.rb | 103 +++++++ lib/discourse_assign/group_extension.rb | 24 +- .../notification_extension.rb | 15 + plugin.rb | 54 ++-- spec/fabricators/assignment_fabricator.rb | 1 + spec/jobs/regular/assign_notification_spec.rb | 198 +------------- .../regular/unassign_notification_spec.rb | 35 +-- .../create_notification_spec.rb | 140 ++++++++++ spec/models/assignment_spec.rb | 190 ++++++++++++- spec/plugin_spec.rb | 69 +++-- .../ensure_notifications_consistency_spec.rb | 66 +++++ spec/requests/assign_controller_spec.rb | 136 +--------- .../acceptance/assigns-tab-user-menu-test.js | 256 +++--------------- 25 files changed, 798 insertions(+), 916 deletions(-) delete mode 100644 assets/javascripts/discourse/lib/user-menu/assign-item.js create mode 100644 db/post_migrate/20231011152903_ensure_notifications_consistency.rb create mode 100644 lib/discourse_assign/create_notification.rb create mode 100644 lib/discourse_assign/notification_extension.rb create mode 100644 spec/lib/discourse_assign/create_notification_spec.rb create mode 100644 spec/post_migrations/ensure_notifications_consistency_spec.rb diff --git a/app/controllers/discourse_assign/assign_controller.rb b/app/controllers/discourse_assign/assign_controller.rb index 842b318f..846faff0 100644 --- a/app/controllers/discourse_assign/assign_controller.rb +++ b/app/controllers/discourse_assign/assign_controller.rb @@ -159,43 +159,6 @@ def group_members } end - def user_menu_assigns - assign_notifications = - Notification.unread_type(current_user, Notification.types[:assigned], user_menu_limit) - - if assign_notifications.size < user_menu_limit - opts = {} - ignored_assignment_ids = - assign_notifications.filter_map { |notification| notification.data_hash[:assignment_id] } - opts[:ignored_assignment_ids] = ignored_assignment_ids if ignored_assignment_ids.present? - - assigns_list = - TopicQuery.new( - current_user, - per_page: user_menu_limit - assign_notifications.size, - ).list_messages_assigned(current_user, ignored_assignment_ids) - end - - if assign_notifications.present? - serialized_notifications = - ActiveModel::ArraySerializer.new( - assign_notifications, - each_serializer: NotificationSerializer, - scope: guardian, - ) - end - - if assigns_list - serialized_assigns = - serialize_data(assigns_list, TopicListSerializer, scope: guardian, root: false)[:topics] - end - - render json: { - notifications: serialized_notifications || [], - topics: serialized_assigns || [], - } - end - private def translate_failure(reason, assign_to) @@ -261,10 +224,6 @@ def ensure_assign_allowed raise Discourse::InvalidAccess.new unless current_user.can_assign? end - def user_menu_limit - UsersController::USER_MENU_LIST_LIMIT - end - def recent_assignees User .where("users.id <> ?", current_user.id) diff --git a/app/jobs/regular/assign_notification.rb b/app/jobs/regular/assign_notification.rb index a2ada45f..2bae14dd 100644 --- a/app/jobs/regular/assign_notification.rb +++ b/app/jobs/regular/assign_notification.rb @@ -3,79 +3,8 @@ module Jobs class AssignNotification < ::Jobs::Base def execute(args) - raise Discourse::InvalidParameters.new(:topic_id) if args[:topic_id].nil? - raise Discourse::InvalidParameters.new(:post_id) if args[:post_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(:assignment_id) if args[:assignment_id].nil? - - if args[:skip_small_action_post].nil? - raise Discourse::InvalidParameters.new(:skip_small_action_post) - end - - topic = Topic.find(args[:topic_id]) - post = Post.find(args[:post_id]) - assigned_by = User.find(args[:assigned_by_id]) - assigned_to = - ( - if args[:assigned_to_type] == "User" - User.find(args[:assigned_to_id]) - else - Group.find(args[:assigned_to_id]) - end - ) - assigned_to_users = args[:assigned_to_type] == "User" ? [assigned_to] : assigned_to.users - - assigned_to_users.each do |user| - Assigner.publish_topic_tracking_state(topic, user.id) - - next if assigned_by == user - - assigned_to_user = args[:assigned_to_type] == "User" - - PostAlerter.new(post).create_notification_alert( - user: user, - post: post, - username: assigned_by.username, - notification_type: Notification.types[:assigned] || Notification.types[:custom], - excerpt: - I18n.t( - ( - if assigned_to_user - "discourse_assign.topic_assigned_excerpt" - else - "discourse_assign.topic_group_assigned_excerpt" - end - ), - title: topic.title, - group: assigned_to.name, - locale: user.effective_locale, - ), - ) - - next if args[:skip_small_action_post] - Notification.create!( - notification_type: Notification.types[:assigned] || Notification.types[:custom], - user_id: user.id, - topic_id: topic.id, - post_number: post.post_number, - high_priority: true, - data: { - message: - ( - if assigned_to_user - "discourse_assign.assign_notification" - else - "discourse_assign.assign_group_notification" - end - ), - display_username: assigned_to_user ? assigned_by.username : assigned_to.name, - topic_title: topic.title, - assignment_id: args[:assignment_id], - }.to_json, - ) - end + Assignment.find(args[:assignment_id]).create_missing_notifications! end end end diff --git a/app/jobs/regular/unassign_notification.rb b/app/jobs/regular/unassign_notification.rb index cdbe4cf7..6db3f140 100644 --- a/app/jobs/regular/unassign_notification.rb +++ b/app/jobs/regular/unassign_notification.rb @@ -3,34 +3,18 @@ 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 = - ( - if args[:assigned_to_type] == "User" - [User.find(args[:assigned_to_id])] - else - Group.find(args[:assigned_to_id]).users - end - ) - - assigned_to_users.each do |user| - Assigner.publish_topic_tracking_state(topic, user.id) + %i[topic_id assigned_to_id assigned_to_type assignment_id].each do |argument| + raise Discourse::InvalidParameters.new(argument) if args[argument].nil? + end - Notification - .where( - notification_type: Notification.types[:assigned] || Notification.types[:custom], - user_id: user.id, - topic_id: topic.id, - ) - .where( - "data like '%discourse_assign.assign_notification%' OR data like '%discourse_assign.assign_group_notification%'", - ) - .destroy_all + assignment = Assignment.new(args.slice(:topic_id, :assigned_to_id, :assigned_to_type)) + assignment.assigned_users.each do |user| + Assigner.publish_topic_tracking_state(assignment.topic, user.id) end + Notification + .for_assignment(args[:assignment_id]) + .where(user: assignment.assigned_users, topic: assignment.topic) + .destroy_all end end end diff --git a/app/models/assignment.rb b/app/models/assignment.rb index f89a6f8b..36a2ae98 100644 --- a/app/models/assignment.rb +++ b/app/models/assignment.rb @@ -15,7 +15,9 @@ class Assignment < ActiveRecord::Base ) end - scope :active_for_group, ->(group) { where(assigned_to: group, active: true) } + scope :active_for_group, ->(group) { active.where(assigned_to: group) } + scope :active, -> { where(active: true) } + scope :inactive, -> { where(active: false) } before_validation :default_status @@ -38,11 +40,31 @@ def self.status_enabled? end def assigned_to_user? - assigned_to_type == "User" + assigned_to.is_a?(User) end def assigned_to_group? - assigned_to_type == "Group" + assigned_to.is_a?(Group) + end + + def assigned_users + Array.wrap(assigned_to.try(:users) || assigned_to) + end + + def post + return target.posts.find_by(post_number: 1) if target.is_a?(Topic) + target + end + + def create_missing_notifications!(mark_as_read: false) + assigned_users.each do |user| + next if user.notifications.for_assignment(self).exists? + DiscourseAssign::CreateNotification.call( + assignment: self, + user: user, + mark_as_read: mark_as_read || assigned_by_user == user, + ) + end end private diff --git a/assets/javascripts/discourse/components/user-menu/assigns-list.js b/assets/javascripts/discourse/components/user-menu/assigns-list.js index 3bf26bc4..f652cbeb 100644 --- a/assets/javascripts/discourse/components/user-menu/assigns-list.js +++ b/assets/javascripts/discourse/components/user-menu/assigns-list.js @@ -1,10 +1,5 @@ import UserMenuNotificationsList from "discourse/components/user-menu/notifications-list"; -import { ajax } from "discourse/lib/ajax"; -import UserMenuNotificationItem from "discourse/lib/user-menu/notification-item"; -import Notification from "discourse/models/notification"; -import Topic from "discourse/models/topic"; import I18n from "I18n"; -import UserMenuAssignItem from "../../lib/user-menu/assign-item"; import UserMenuAssignsListEmptyState from "./assigns-list-empty-state"; export default class UserMenuAssignNotificationsList extends UserMenuNotificationsList { @@ -56,25 +51,10 @@ export default class UserMenuAssignNotificationsList extends UserMenuNotificatio } async fetchItems() { - const data = await ajax("/assign/user-menu-assigns.json"); - const content = []; - - const notifications = data.notifications.map((n) => Notification.create(n)); - await Notification.applyTransformations(notifications); - notifications.forEach((notification) => { - content.push( - new UserMenuNotificationItem({ - notification, - currentUser: this.currentUser, - siteSettings: this.siteSettings, - site: this.site, - }) - ); - }); - - const topics = data.topics.map((t) => Topic.create(t)); - await Topic.applyTransformations(topics); - content.push(...topics.map((assign) => new UserMenuAssignItem({ assign }))); - return content; + // sorting by `data.message` length to group single user assignments and + // group assignments, then by `created_at` to keep chronological order. + return (await super.fetchItems()) + .sortBy("notification.data.message", "notification.created_at") + .reverse(); } } diff --git a/assets/javascripts/discourse/initializers/assign-user-menu.js b/assets/javascripts/discourse/initializers/assign-user-menu.js index 125aea99..6f82ee8e 100644 --- a/assets/javascripts/discourse/initializers/assign-user-menu.js +++ b/assets/javascripts/discourse/initializers/assign-user-menu.js @@ -1,4 +1,7 @@ +import { htmlSafe } from "@ember/template"; import { withPluginApi } from "discourse/lib/plugin-api"; +import { emojiUnescape } from "discourse/lib/text"; +import I18n from "I18n"; import UserMenuAssignNotificationsList from "../components/user-menu/assigns-list"; export default { @@ -6,16 +9,74 @@ export default { initialize(container) { withPluginApi("1.2.0", (api) => { + const siteSettings = container.lookup("service:site-settings"); + if (!siteSettings.assign_enabled) { + return; + } + + const currentUser = api.getCurrentUser(); + if (!currentUser?.can_assign) { + return; + } + + if (api.registerNotificationTypeRenderer) { + api.registerNotificationTypeRenderer( + "assigned", + (NotificationItemBase) => { + return class extends NotificationItemBase { + get linkTitle() { + if (this.isGroup()) { + return I18n.t( + `user.assigned_to_group.${this.postOrTopic()}`, + { + group_name: this.notification.data.display_username, + } + ); + } + return I18n.t(`user.assigned_to_you.${this.postOrTopic()}`); + } + + get icon() { + return this.isGroup() ? "group-plus" : "user-plus"; + } + + get label() { + if (!this.isGroup()) { + return ""; + } + return this.notification.data.display_username; + } + + get description() { + return htmlSafe( + emojiUnescape( + I18n.t( + `user.assignment_description.${this.postOrTopic()}`, + { + topic_title: this.notification.fancy_title, + post_number: this.notification.post_number, + } + ) + ) + ); + } + + isGroup() { + return ( + this.notification.data.message === + "discourse_assign.assign_group_notification" + ); + } + + postOrTopic() { + return this.notification.post_number === 1 ? "topic" : "post"; + } + }; + } + ); + } + if (api.registerUserMenuTab) { - const siteSettings = container.lookup("service:site-settings"); - if (!siteSettings.assign_enabled) { - return; - } - - const currentUser = api.getCurrentUser(); - if (!currentUser?.can_assign) { - return; - } api.registerUserMenuTab((UserMenuTab) => { return class extends UserMenuTab { id = "assign-list"; diff --git a/assets/javascripts/discourse/initializers/extend-for-assigns.js b/assets/javascripts/discourse/initializers/extend-for-assigns.js index de5fecf5..76600a8d 100644 --- a/assets/javascripts/discourse/initializers/extend-for-assigns.js +++ b/assets/javascripts/discourse/initializers/extend-for-assigns.js @@ -1,3 +1,4 @@ +import { getOwner } from "@ember/application"; import { htmlSafe } from "@ember/template"; import { isEmpty } from "@ember/utils"; import { h } from "virtual-dom"; @@ -7,7 +8,6 @@ import { withPluginApi } from "discourse/lib/plugin-api"; import { registerTopicFooterDropdown } from "discourse/lib/register-topic-footer-dropdown"; import { escapeExpression } from "discourse/lib/utilities"; import RawHtml from "discourse/widgets/raw-html"; -import { getOwner } from "discourse-common/lib/get-owner"; import getURL from "discourse-common/lib/get-url"; import { iconHTML, iconNode } from "discourse-common/lib/icon-library"; import discourseComputed from "discourse-common/utils/decorators"; diff --git a/assets/javascripts/discourse/lib/user-menu/assign-item.js b/assets/javascripts/discourse/lib/user-menu/assign-item.js deleted file mode 100644 index c3aa55d9..00000000 --- a/assets/javascripts/discourse/lib/user-menu/assign-item.js +++ /dev/null @@ -1,59 +0,0 @@ -import { htmlSafe } from "@ember/template"; -import { emojiUnescape } from "discourse/lib/text"; -import UserMenuBaseItem from "discourse/lib/user-menu/base-item"; -import { postUrl } from "discourse/lib/utilities"; -import I18n from "I18n"; - -const ICON = "user-plus"; -const GROUP_ICON = "group-plus"; - -export default class UserMenuAssignItem extends UserMenuBaseItem { - constructor({ assign }) { - super(...arguments); - this.assign = assign; - } - - get className() { - return "assign"; - } - - get linkHref() { - return postUrl( - this.assign.slug, - this.assign.id, - (this.assign.last_read_post_number || 0) + 1 - ); - } - - get linkTitle() { - if (this.assign.assigned_to_group) { - return I18n.t("user.assigned_to_group", { - group_name: - this.assign.assigned_to_group.full_name || - this.assign.assigned_to_group.name, - }); - } else { - return I18n.t("user.assigned_to_you"); - } - } - - get icon() { - if (this.assign.assigned_to_group) { - return GROUP_ICON; - } else { - return ICON; - } - } - - get label() { - return null; - } - - get description() { - return htmlSafe(emojiUnescape(this.assign.fancy_title)); - } - - get topicId() { - return this.assign.id; - } -} diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 19387912..80b68b5f 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -32,8 +32,6 @@ en: # assign_post_to_multiple used in list form, example: "Assigned topic to username0, [#2 to username1], [#10 to username2]" assign_post_to_multiple: "#%{post_number} to %{username}" assigned_to_w_ellipsis: "Assigned to..." - assign_notification: "

%{username} %{description}

" - assign_group_notification: "

%{username} %{description}

" unassign: title: "Unassign" title_w_ellipsis: "Unassign..." @@ -97,8 +95,15 @@ en:

To assign a topic or message to yourself or to someone else, look for the %{icon} assign button at the bottom. dismiss_assigned_tooltip: "Mark all unread assign notifications as read" - assigned_to_group: "assigned to %{group_name}" - assigned_to_you: "assigned to you" + assigned_to_group: + post: "post assigned to %{group_name}" + topic: "topic assigned to %{group_name}" + assigned_to_you: + post: "post assigned to you" + topic: "topic assigned to you" + assignment_description: + post: "%{topic_title} (#%{post_number})" + topic: "%{topic_title}" admin: web_hooks: assign_event: diff --git a/config/routes.rb b/config/routes.rb index 56fd94a0..ef974f3d 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -7,7 +7,6 @@ get "/suggestions" => "assign#suggestions" get "/assigned" => "assign#assigned" get "/members/:group_name" => "assign#group_members" - get "/user-menu-assigns" => "assign#user_menu_assigns" end Discourse::Application.routes.draw do diff --git a/db/post_migrate/20231011152903_ensure_notifications_consistency.rb b/db/post_migrate/20231011152903_ensure_notifications_consistency.rb new file mode 100644 index 00000000..ddcdc547 --- /dev/null +++ b/db/post_migrate/20231011152903_ensure_notifications_consistency.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +class EnsureNotificationsConsistency < ActiveRecord::Migration[7.0] + def up + Notification + .assigned + .joins( + "LEFT OUTER JOIN assignments ON assignments.id = ((notifications.data::jsonb)->'assignment_id')::int", + ) + .where(assignments: { id: nil }) + .or(Assignment.inactive) + .destroy_all + + Assignment + .active + .left_joins(:topic) + .where.not(topics: { id: nil }) + .find_each { |assignment| assignment.create_missing_notifications!(mark_as_read: true) } + end + + def down + end +end diff --git a/lib/assigner.rb b/lib/assigner.rb index b2c053f0..eecf536a 100644 --- a/lib/assigner.rb +++ b/lib/assigner.rb @@ -239,11 +239,8 @@ def update_details(assign_to, note, status, skip_small_action_post: false) end @target.assignment.update!(note: note, status: status) - - queue_notification(assign_to, skip_small_action_post, @target.assignment) - - assignment = @target.assignment - publish_assignment(assignment, assign_to, note, status) + queue_notification(@target.assignment) + publish_assignment(@target.assignment, assign_to, note, status) # email is skipped, for now @@ -276,31 +273,28 @@ def assign(assign_to, note: nil, skip_small_action_post: false, status: nil) skip_small_action_post = skip_small_action_post || no_assignee_change?(assign_to) - if topic.assignment.present? + if @target.assignment Jobs.enqueue( :unassign_notification, topic_id: topic.id, - assigned_to_id: topic.assignment.assigned_to_id, - assigned_to_type: topic.assignment.assigned_to_type, + assigned_to_id: @target.assignment.assigned_to_id, + assigned_to_type: @target.assignment.assigned_to_type, + assignment_id: @target.assignment.id, ) + @target.assignment.destroy! end - @target.assignment&.destroy! - assignment = @target.create_assignment!( - assigned_to_id: assign_to.id, - assigned_to_type: assigned_to_type, - assigned_by_user_id: @assigned_by.id, - topic_id: topic.id, + assigned_to: assign_to, + assigned_by_user: @assigned_by, + topic: topic, note: note, status: status, ) first_post.publish_change_to_clients!(:revised, reload_topic: true) - - queue_notification(assign_to, skip_small_action_post, assignment) - + queue_notification(assignment) publish_assignment(assignment, assign_to, note, status) if assignment.assigned_to_user? @@ -370,6 +364,7 @@ def unassign(silent: false, deactivate: false) topic_id: topic.id, assigned_to_id: assignment.assigned_to.id, assigned_to_type: assignment.assigned_to_type, + assignment_id: assignment.id, ) assigned_to = assignment.assigned_to @@ -461,17 +456,8 @@ def guardian @guardian ||= Guardian.new(@assigned_by) end - def queue_notification(assign_to, skip_small_action_post, assignment) - Jobs.enqueue( - :assign_notification, - topic_id: topic.id, - post_id: topic_target? ? first_post.id : @target.id, - assigned_to_id: assign_to.id, - assigned_to_type: assign_to.is_a?(User) ? "User" : "Group", - assigned_by_id: @assigned_by.id, - skip_small_action_post: skip_small_action_post, - assignment_id: assignment.id, - ) + def queue_notification(assignment) + Jobs.enqueue(:assign_notification, assignment_id: assignment.id) end def add_small_action_post(action_code, assign_to, text) diff --git a/lib/discourse_assign/create_notification.rb b/lib/discourse_assign/create_notification.rb new file mode 100644 index 00000000..171195ba --- /dev/null +++ b/lib/discourse_assign/create_notification.rb @@ -0,0 +1,103 @@ +# frozen_string_literal: true + +module DiscourseAssign + class CreateNotification + class UserAssignment + attr_reader :assignment + + def initialize(assignment) + @assignment = assignment + end + + def excerpt_key + "discourse_assign.topic_assigned_excerpt" + end + + def notification_message + "discourse_assign.assign_notification" + end + + def display_username + assignment.assigned_by_user.username + end + end + + class GroupAssignment < UserAssignment + def excerpt_key + "discourse_assign.topic_group_assigned_excerpt" + end + + def notification_message + "discourse_assign.assign_group_notification" + end + + def display_username + assignment.assigned_to.name + end + end + + def self.call(...) + new(...).call + end + + attr_reader :assignment, :user, :mark_as_read, :assignment_type + alias mark_as_read? mark_as_read + + delegate :topic, + :post, + :assigned_by_user, + :assigned_to, + :created_at, + :updated_at, + :assigned_to_user?, + :id, + to: :assignment, + private: true + delegate :excerpt_key, + :notification_message, + :display_username, + to: :assignment_type, + private: true + + def initialize(assignment:, user:, mark_as_read:) + @assignment = assignment + @user = user + @mark_as_read = mark_as_read + @assignment_type = + "#{self.class}::#{assignment.assigned_to.class}Assignment".constantize.new(assignment) + end + + def call + Assigner.publish_topic_tracking_state(topic, user.id) + unless mark_as_read? + PostAlerter.new(post).create_notification_alert( + user: user, + post: post, + username: assigned_by_user.username, + notification_type: Notification.types[:assigned], + excerpt: + I18n.t( + excerpt_key, + title: topic.title, + group: assigned_to.name, + locale: user.effective_locale, + ), + ) + end + user.notifications.assigned.create!( + created_at: created_at, + updated_at: updated_at, + topic: topic, + post_number: post.post_number, + high_priority: true, + read: mark_as_read?, + data: { + message: notification_message, + display_username: display_username, + topic_title: topic.title, + assignment_id: id, + }.to_json, + ) + end + end +end diff --git a/lib/discourse_assign/group_extension.rb b/lib/discourse_assign/group_extension.rb index 923f5a7b..66b311a2 100644 --- a/lib/discourse_assign/group_extension.rb +++ b/lib/discourse_assign/group_extension.rb @@ -2,12 +2,15 @@ module DiscourseAssign module GroupExtension - def self.prepended(base) - base.class_eval do - scope :assignable, - ->(user) do - where( - "assignable_level in (:levels) OR + extend ActiveSupport::Concern + + prepended do + has_many :assignments, as: :assigned_to + + scope :assignable, + ->(user) do + where( + "assignable_level in (:levels) OR ( assignable_level = #{Group::ALIAS_LEVELS[:members_mods_and_admins]} AND id in ( SELECT group_id FROM group_users WHERE user_id = :user_id) @@ -15,11 +18,10 @@ def self.prepended(base) assignable_level = #{Group::ALIAS_LEVELS[:owners_mods_and_admins]} AND id in ( SELECT group_id FROM group_users WHERE user_id = :user_id AND owner IS TRUE) )", - levels: alias_levels(user), - user_id: user&.id, - ) - end - end + levels: alias_levels(user), + user_id: user&.id, + ) + end end end end diff --git a/lib/discourse_assign/notification_extension.rb b/lib/discourse_assign/notification_extension.rb new file mode 100644 index 00000000..ddb7e258 --- /dev/null +++ b/lib/discourse_assign/notification_extension.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module DiscourseAssign + module NotificationExtension + extend ActiveSupport::Concern + + prepended do + scope :assigned, -> { where(notification_type: Notification.types[:assigned]) } + scope :for_assignment, + ->(assignment) do + assigned.where("((data::jsonb)->'assignment_id')::int IN (?)", assignment) + end + end + end +end diff --git a/plugin.rb b/plugin.rb index 5a82c950..171535ae 100644 --- a/plugin.rb +++ b/plugin.rb @@ -26,10 +26,12 @@ module ::DiscourseAssign require_relative "app/jobs/regular/unassign_notification" require_relative "app/jobs/scheduled/enqueue_reminders" require_relative "lib/assigner" + require_relative "lib/discourse_assign/create_notification" require_relative "lib/discourse_assign/discourse_calendar" require_relative "lib/discourse_assign/group_extension" require_relative "lib/discourse_assign/helpers" require_relative "lib/discourse_assign/list_controller_extension" + require_relative "lib/discourse_assign/notification_extension" require_relative "lib/discourse_assign/post_extension" require_relative "lib/discourse_assign/topic_extension" require_relative "lib/discourse_assign/web_hook_extension" @@ -38,11 +40,12 @@ module ::DiscourseAssign require_relative "lib/topic_assigner" reloadable_patch do |plugin| - Group.class_eval { prepend DiscourseAssign::GroupExtension } - ListController.class_eval { prepend DiscourseAssign::ListControllerExtension } - Post.class_eval { prepend DiscourseAssign::PostExtension } - Topic.class_eval { prepend DiscourseAssign::TopicExtension } - WebHook.class_eval { prepend DiscourseAssign::WebHookExtension } + Group.prepend(DiscourseAssign::GroupExtension) + ListController.prepend(DiscourseAssign::ListControllerExtension) + Post.prepend(DiscourseAssign::PostExtension) + Topic.prepend(DiscourseAssign::TopicExtension) + WebHook.prepend(DiscourseAssign::WebHookExtension) + Notification.prepend(DiscourseAssign::NotificationExtension) end register_group_param(:assignable_level) @@ -201,7 +204,8 @@ module ::DiscourseAssign allowed_access = SiteSetting.assigns_public || can_assign if allowed_access && topics.length > 0 - assignments = Assignment.strict_loading.where(topic: topics, active: true).includes(:target) + assignments = + Assignment.strict_loading.active.where(topic: topics).includes(:target, :assigned_to) assignments_map = assignments.group_by(&:topic_id) user_ids = @@ -836,20 +840,12 @@ module ::DiscourseAssign next if !info[:group] Assignment - .where(topic_id: topic.id, active: false) + .inactive + .where(topic: topic) .find_each do |assignment| next unless assignment.target assignment.update!(active: true) - Jobs.enqueue( - :assign_notification, - topic_id: topic.id, - post_id: assignment.target_type.is_a?(Topic) ? topic.first_post.id : assignment.target.id, - assigned_to_id: assignment.assigned_to_id, - assigned_to_type: assignment.assigned_to_type, - assigned_by_id: assignment.assigned_by_user_id, - skip_small_action_post: true, - assignment_id: assignment.id, - ) + Jobs.enqueue(:assign_notification, assignment_id: assignment.id) end end @@ -863,32 +859,30 @@ module ::DiscourseAssign next if !info[:group] Assignment - .where(topic_id: topic.id, active: true) + .active + .where(topic: topic) .find_each do |assignment| assignment.update!(active: false) Jobs.enqueue( :unassign_notification, topic_id: topic.id, - assigned_to_id: assignment.assigned_to.id, + assigned_to_id: assignment.assigned_to_id, assigned_to_type: assignment.assigned_to_type, + assignment_id: assignment.id, ) 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 { |topic| Assigner.new(topic, Discourse.system_user).unassign } - end + on(:user_added_to_group) do |user, group, automatic:| + group.assignments.active.find_each do |assignment| + Jobs.enqueue(:assign_notification, assignment_id: assignment.id) end end + on(:user_removed_from_group) do |user, group| + user.notifications.for_assignment(group.assignments.select(:id)).destroy_all + end + on(:post_moved) do |post, original_topic_id| assignment = Assignment.where(topic_id: original_topic_id, target_type: "Post", target_id: post.id).first diff --git a/spec/fabricators/assignment_fabricator.rb b/spec/fabricators/assignment_fabricator.rb index 5a3e6a44..9dfa97fd 100644 --- a/spec/fabricators/assignment_fabricator.rb +++ b/spec/fabricators/assignment_fabricator.rb @@ -9,6 +9,7 @@ end Fabricator(:post_assignment, class_name: :assignment) do + transient :post topic { |attrs| attrs[:post]&.topic || Fabricate(:topic) } target { |attrs| attrs[:post] || Fabricate(:post, topic: attrs[:topic]) } target_type "Post" diff --git a/spec/jobs/regular/assign_notification_spec.rb b/spec/jobs/regular/assign_notification_spec.rb index ed85e824..4ecbd57f 100644 --- a/spec/jobs/regular/assign_notification_spec.rb +++ b/spec/jobs/regular/assign_notification_spec.rb @@ -4,199 +4,27 @@ RSpec.describe Jobs::AssignNotification do describe "#execute" do - fab!(:user1) { Fabricate(:user, last_seen_at: 1.day.ago) } - fab!(:user2) { Fabricate(:user, last_seen_at: 1.day.ago) } - fab!(:topic) { Fabricate(:topic, title: "Basic topic title") } - 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") } + subject(:execute_job) { described_class.new.execute(args) } - def assert_publish_topic_state(topic, user) - message = MessageBus.track_publish("/private-messages/assigned") { yield }.first + let(:args) { { assignment_id: assignment_id } } - expect(message.data[:topic_id]).to eq(topic.id) - expect(message.user_ids).to eq([user.id]) - end - - before { assign_allowed_group.add(user1) } - - describe "User" do - it "sends notification alert" do - messages = - MessageBus.track_publish("/notification-alert/#{user2.id}") do - described_class.new.execute( - { - topic_id: topic.id, - post_id: post.id, - assigned_to_id: user2.id, - assigned_to_type: "User", - assigned_by_id: user1.id, - skip_small_action_post: false, - assignment_id: 1093, - }, - ) - end - - expect(messages.length).to eq(1) - expect(messages.first.data[:excerpt]).to eq( - I18n.t("discourse_assign.topic_assigned_excerpt", title: 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, - post_id: pm_post.id, - assigned_to_id: pm.allowed_users.first.id, - assigned_to_type: "User", - assigned_by_id: user1.id, - skip_small_action_post: false, - assignment_id: 9023, - }, - ) - end - end + context "when `assignment_id` is not provided" do + let(:args) { {} } - it "sends a high priority notification to the assignee" do - Notification.expects(:create!).with( - notification_type: Notification.types[:assigned] || 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, - assignment_id: 3194, - }.to_json, - ) - described_class.new.execute( - { - topic_id: topic.id, - post_id: post.id, - assigned_to_id: user2.id, - assigned_to_type: "User", - assigned_by_id: user1.id, - skip_small_action_post: false, - assignment_id: 3194, - }, - ) + it "raises an error" do + expect { execute_job }.to raise_error(Discourse::InvalidParameters, "assignment_id") end end - describe "Group" do - fab!(:user3) { Fabricate(:user, last_seen_at: 1.day.ago) } - fab!(:user4) { Fabricate(:user, suspended_till: 1.year.from_now) } - fab!(:group) { Fabricate(:group, name: "Developers") } - let(:assignment) do - Assignment.create!(topic: topic, assigned_by_user: user1, assigned_to: group) - end - - before do - group.add(user2) - group.add(user3) - group.add(user4) - 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, - post_id: post.id, - assigned_to_id: group.id, - assigned_to_type: "Group", - assigned_by_id: user1.id, - skip_small_action_post: false, - assignment_id: 7839, - }, - ) - end - expect(messages.length).to eq(1) - expect(messages.first.data[:excerpt]).to eq( - I18n.t( - "discourse_assign.topic_group_assigned_excerpt", - title: topic.title, - group: group.name, - ), - ) - - messages = - MessageBus.track_publish("/notification-alert/#{user3.id}") do - described_class.new.execute( - { - topic_id: topic.id, - post_id: post.id, - assigned_to_id: group.id, - assigned_to_type: "Group", - assigned_by_id: user1.id, - skip_small_action_post: false, - assignment_id: 7763, - }, - ) - end - expect(messages.length).to eq(1) - expect(messages.first.data[:excerpt]).to eq( - I18n.t( - "discourse_assign.topic_group_assigned_excerpt", - title: topic.title, - group: group.name, - ), - ) - - messages = - MessageBus.track_publish("/notification-alert/#{user4.id}") do - described_class.new.execute( - { - topic_id: topic.id, - post_id: post.id, - assigned_to_id: group.id, - assigned_to_type: "Group", - assigned_by_id: user1.id, - skip_small_action_post: false, - assignment_id: 8883, - }, - ) - end - expect(messages.length).to eq(0) - end + context "when `assignment_id` is provided" do + let(:assignment_id) { Fabricate(:topic_assignment).id } + let(:assignment) { stub("assignment").responds_like_instance_of(Assignment) } - it "sends a high priority notification to all group members" do - [user2, user3, user4].each do |user| - Notification.expects(:create!).with( - notification_type: Notification.types[:assigned] || Notification.types[:custom], - user_id: user.id, - topic_id: topic.id, - post_number: 1, - high_priority: true, - data: { - message: "discourse_assign.assign_group_notification", - display_username: group.name, - topic_title: topic.title, - assignment_id: 9429, - }.to_json, - ) - end + before { Assignment.stubs(:find).with(assignment_id).returns(assignment) } - described_class.new.execute( - { - topic_id: topic.id, - post_id: post.id, - assigned_to_id: group.id, - assigned_to_type: "Group", - assigned_by_id: user1.id, - skip_small_action_post: false, - assignment_id: 9429, - }, - ) + it "creates missing notifications for the provided assignment" do + assignment.expects(:create_missing_notifications!) + execute_job end end end diff --git a/spec/jobs/regular/unassign_notification_spec.rb b/spec/jobs/regular/unassign_notification_spec.rb index 14facff1..352a1001 100644 --- a/spec/jobs/regular/unassign_notification_spec.rb +++ b/spec/jobs/regular/unassign_notification_spec.rb @@ -22,19 +22,11 @@ def assert_publish_topic_state(topic, user) end describe "User" do - it "deletes notifications" do - Jobs::AssignNotification.new.execute( - { - topic_id: topic.id, - post_id: post.id, - assigned_to_id: user2.id, - assigned_to_type: "User", - assigned_by_id: user1.id, - skip_small_action_post: false, - assignment_id: 4519, - }, - ) + fab!(:assignment) { Fabricate(:topic_assignment, topic: topic, assigned_to: user2) } + + before { assignment.create_missing_notifications! } + it "deletes notifications" do expect { described_class.new.execute( { @@ -42,6 +34,7 @@ def assert_publish_topic_state(topic, user) post_id: post.id, assigned_to_id: user2.id, assigned_to_type: "User", + assignment_id: assignment.id, }, ) }.to change { user2.notifications.count }.by(-1) @@ -58,6 +51,7 @@ def assert_publish_topic_state(topic, user) post_id: pm_post.id, assigned_to_id: pm.allowed_users.first.id, assigned_to_type: "User", + assignment_id: 4519, }, ) end @@ -68,25 +62,17 @@ def assert_publish_topic_state(topic, user) fab!(:assign_allowed_group) { Group.find_by(name: "staff") } fab!(:user3) { Fabricate(:user) } fab!(:group) { Fabricate(:group) } + fab!(:assignment) do + Fabricate(:topic_assignment, topic: topic, assigned_to: group, assigned_by_user: user1) + end before do group.add(user2) group.add(user3) + assignment.create_missing_notifications! end it "deletes notifications" do - Jobs::AssignNotification.new.execute( - { - topic_id: topic.id, - post_id: post.id, - assigned_to_id: group.id, - assigned_to_type: "Group", - assigned_by_id: user1.id, - skip_small_action_post: false, - assignment_id: 9281, - }, - ) - expect { described_class.new.execute( { @@ -94,6 +80,7 @@ def assert_publish_topic_state(topic, user) post_id: post.id, assigned_to_id: group.id, assigned_to_type: "Group", + assignment_id: assignment.id, }, ) }.to change { Notification.count }.by(-2) diff --git a/spec/lib/discourse_assign/create_notification_spec.rb b/spec/lib/discourse_assign/create_notification_spec.rb new file mode 100644 index 00000000..3010251c --- /dev/null +++ b/spec/lib/discourse_assign/create_notification_spec.rb @@ -0,0 +1,140 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe DiscourseAssign::CreateNotification do + describe ".call" do + subject(:create_notification) do + described_class.call(assignment: assignment, user: user, mark_as_read: mark_as_read) + end + + let(:assignment) { Fabricate(:topic_assignment, topic: post.topic, assigned_to: assigned_to) } + let(:post) { Fabricate(:post) } + let(:mark_as_read) { false } + let(:alerter) { stub_everything("alerter").responds_like_instance_of(PostAlerter) } + + before { PostAlerter.stubs(:new).returns(alerter) } + + context "when assigned to a single user" do + let(:assigned_to) { Fabricate(:user) } + let(:user) { assigned_to } + + it "publishes topic tracking state" do + Assigner.expects(:publish_topic_tracking_state).with(assignment.topic, user.id) + create_notification + end + + context "when `mark_as_read` is false" do + let(:excerpt) do + I18n.t( + "discourse_assign.topic_assigned_excerpt", + title: post.topic.title, + group: user.name, + locale: user.effective_locale, + ) + end + + it "creates a notification alert" do + alerter.expects(:create_notification_alert).with( + user: user, + post: post, + username: assignment.assigned_by_user.username, + notification_type: Notification.types[:assigned], + excerpt: excerpt, + ) + create_notification + end + end + + context "when `mark_as_read` is true" do + let(:mark_as_read) { true } + + it "does not create a notification alert" do + alerter.expects(:create_notification_alert).never + create_notification + end + end + + it "creates a notification" do + expect { create_notification }.to change { Notification.count }.by(1) + expect(Notification.assigned.last).to have_attributes( + created_at: assignment.created_at, + updated_at: assignment.updated_at, + user: user, + topic: post.topic, + post_number: post.post_number, + high_priority: true, + read: mark_as_read, + data_hash: { + message: "discourse_assign.assign_notification", + display_username: assignment.assigned_by_user.username, + topic_title: post.topic.title, + assignment_id: assignment.id, + }, + ) + end + end + + context "when assigned to a group" do + let(:assigned_to) { Fabricate(:group) } + let(:user) { Fabricate(:user) } + + before { assigned_to.users << user } + + it "publishes topic tracking state" do + Assigner.expects(:publish_topic_tracking_state).with(assignment.topic, user.id) + create_notification + end + + context "when `mark_as_read` is false" do + let(:excerpt) do + I18n.t( + "discourse_assign.topic_group_assigned_excerpt", + title: post.topic.title, + group: assigned_to.name, + locale: user.effective_locale, + ) + end + + it "creates a notification alert" do + alerter.expects(:create_notification_alert).with( + user: user, + post: post, + username: assignment.assigned_by_user.username, + notification_type: Notification.types[:assigned], + excerpt: excerpt, + ) + create_notification + end + end + + context "when `mark_as_read` is true" do + let(:mark_as_read) { true } + + it "does not create a notification alert" do + alerter.expects(:create_notification_alert).never + create_notification + end + end + + it "creates a notification" do + expect { create_notification }.to change { Notification.count }.by(1) + expect(Notification.assigned.last).to have_attributes( + created_at: assignment.created_at, + updated_at: assignment.updated_at, + user: user, + topic: post.topic, + post_number: post.post_number, + high_priority: true, + read: mark_as_read, + data_hash: { + message: "discourse_assign.assign_group_notification", + display_username: assigned_to.name, + topic_title: post.topic.title, + assignment_id: assignment.id, + }, + ) + end + end + end +end diff --git a/spec/models/assignment_spec.rb b/spec/models/assignment_spec.rb index a223aa6e..2c257bb3 100644 --- a/spec/models/assignment_spec.rb +++ b/spec/models/assignment_spec.rb @@ -2,26 +2,190 @@ require "rails_helper" -describe Assignment do - fab!(:group) { Fabricate(:group) } - fab!(:user1) { Fabricate(:user) } - fab!(:user2) { Fabricate(:user) } - fab!(:group_user1) { Fabricate(:group_user, user: user1, group: group) } - fab!(:group_user1) { Fabricate(:group_user, user: user2, group: group) } +RSpec.describe Assignment do + before { SiteSetting.assign_enabled = true } - fab!(:wrong_group) { Fabricate(:group) } + describe ".active_for_group" do + subject(:assignments) { described_class.active_for_group(group) } - before { SiteSetting.assign_enabled = true } + let!(:group) { Fabricate(:group) } + let!(:user1) { Fabricate(:user) } + let!(:user2) { Fabricate(:user) } + let!(:group_user1) { Fabricate(:group_user, user: user1, group: group) } + let!(:group_user2) { Fabricate(:group_user, user: user2, group: group) } + let!(:wrong_group) { Fabricate(:group) } + let!(:assignment1) { Fabricate(:topic_assignment, assigned_to: group) } + let!(:assignment2) { Fabricate(:post_assignment, assigned_to: group) } - describe "#active_for_group" do - it "returns active assignments for the group" do - assignment1 = Fabricate(:topic_assignment, assigned_to: group) - assignment2 = Fabricate(:post_assignment, assigned_to: group) + before do Fabricate(:post_assignment, assigned_to: group, active: false) Fabricate(:post_assignment, assigned_to: user1) Fabricate(:topic_assignment, assigned_to: wrong_group) + end + + it "returns active assignments for the group" do + expect(assignments).to contain_exactly(assignment1, assignment2) + end + end + + describe "#assigned_users" do + subject(:assigned_users) { assignment.assigned_users } + + let(:assignment) { Fabricate.build(:topic_assignment, assigned_to: assigned_to) } + + context "when assigned to a group" do + let(:assigned_to) { Fabricate.build(:group) } + + context "when group is empty" do + it "returns an empty collection" do + expect(assigned_users).to be_empty + end + end + + context "when group is not empty" do + before { assigned_to.users = Fabricate.build_times(2, :user) } + + it "returns users from that group" do + expect(assigned_users).to eq(assigned_to.users) + end + end + end + + context "when assigned to a user" do + let(:assigned_to) { Fabricate.build(:user) } + + it "returns that user" do + expect(assigned_users).to eq([assigned_to]) + end + end + end + + describe "#post" do + subject(:post) { assignment.post } + + context "when target is a topic" do + let!(:initial_post) { Fabricate(:post) } + let(:assignment) { Fabricate.build(:topic_assignment, topic: target) } + let(:target) { initial_post.topic } + + it "returns the first post of that topic" do + expect(post).to eq(initial_post) + end + end + + context "when target is a post" do + let(:assignment) { Fabricate.build(:post_assignment) } + + it "returns that post" do + expect(post).to eq(assignment.target) + end + end + end + + describe "#create_missing_notifications!" do + subject(:create_missing_notifications) do + assignment.create_missing_notifications!(mark_as_read: mark_as_read) + end + + let(:assignment) do + Fabricate(:topic_assignment, assigned_to: assigned_to, assigned_by_user: assigned_by_user) + end + let(:mark_as_read) { false } + let(:assigned_by_user) { Fabricate(:user) } + + context "when assigned to a user" do + let(:assigned_to) { Fabricate(:user) } + + context "when notification already exists for that user" do + before do + Fabricate( + :notification, + notification_type: Notification.types[:assigned], + user: assigned_to, + data: { assignment_id: assignment.id }.to_json, + ) + end + + it "does nothing" do + DiscourseAssign::CreateNotification.expects(:call).never + create_missing_notifications + end + end + + context "when notification does not exist yet" do + context "when `mark_as_read` is true" do + let(:mark_as_read) { true } + + it "creates the missing notification" do + DiscourseAssign::CreateNotification.expects(:call).with( + assignment: assignment, + user: assigned_to, + mark_as_read: true, + ) + create_missing_notifications + end + end + + context "when `mark_as_read` is false" do + context "when user is the one that assigned" do + let(:assigned_by_user) { assigned_to } + + it "creates the missing notification" do + DiscourseAssign::CreateNotification.expects(:call).with( + assignment: assignment, + user: assigned_to, + mark_as_read: true, + ) + create_missing_notifications + end + end + + context "when user is not the one that assigned" do + it "creates the missing notification" do + DiscourseAssign::CreateNotification.expects(:call).with( + assignment: assignment, + user: assigned_to, + mark_as_read: false, + ) + create_missing_notifications + end + end + end + end + end + + context "when assigned to a group" do + let(:assigned_to) { Fabricate(:group) } + let(:users) { Fabricate.times(3, :user) } + let(:assigned_by_user) { users.last } + + before do + assigned_to.users = users + Fabricate( + :notification, + notification_type: Notification.types[:assigned], + user: users.first, + data: { assignment_id: assignment.id }.to_json, + ) + end - expect(Assignment.active_for_group(group)).to contain_exactly(assignment1, assignment2) + it "creates missing notifications for group users" do + DiscourseAssign::CreateNotification + .expects(:call) + .with(assignment: assignment, user: users.first, mark_as_read: false) + .never + DiscourseAssign::CreateNotification.expects(:call).with( + assignment: assignment, + user: users.second, + mark_as_read: false, + ) + DiscourseAssign::CreateNotification.expects(:call).with( + assignment: assignment, + user: users.last, + mark_as_read: true, + ) + create_missing_notifications + end end end end diff --git a/spec/plugin_spec.rb b/spec/plugin_spec.rb index 6faddfed..1537b1ff 100644 --- a/spec/plugin_spec.rb +++ b/spec/plugin_spec.rb @@ -2,38 +2,57 @@ require "rails_helper" -describe DiscourseAssign do +RSpec.describe DiscourseAssign do before { SiteSetting.assign_enabled = true } - describe "events" do - describe "on user_removed_from_group" do + describe "Events" do + describe "on 'user_removed_from_group'" do + let(:group) { Fabricate(:group) } + let(:user) { Fabricate(:user) } + let(:first_assignment) { Fabricate(:topic_assignment, assigned_to: group) } + let(:second_assignment) { Fabricate(:post_assignment, assigned_to: group) } + before do - @topic = Fabricate(:post).topic - @user = Fabricate(:user) - @group_a = Fabricate(:group) - @group_a.add(@user) + group.users << user + Fabricate( + :notification, + notification_type: Notification.types[:assigned], + user: user, + data: { assignment_id: first_assignment.id }.to_json, + ) + Fabricate( + :notification, + notification_type: Notification.types[:assigned], + user: user, + data: { assignment_id: second_assignment.id }.to_json, + ) end - it "unassigns the user" do - SiteSetting.assign_allowed_on_groups = @group_a.id.to_s - - Assigner.new(@topic, Discourse.system_user).assign(@user) - @group_a.remove(@user) - - expect(Assignment.count).to eq(0) + it "removes user's notifications related to group assignments" do + expect { group.remove(user) }.to change { user.notifications.assigned.count }.by(-2) end + end - it "doesn't unassign the user if it still has access through another group" do - @group_b = Fabricate(:group) - @group_b.add(@user) - SiteSetting.assign_allowed_on_groups = [@group_a.id.to_s, @group_b.id.to_s].join("|") - - Assigner.new(@topic, Discourse.system_user).assign(@user) - @group_a.remove(@user) - - assignment = Assignment.first - expect(assignment.assigned_to_id).to eq(@user.id) - expect(assignment.assigned_by_user_id).to eq(Discourse::SYSTEM_USER_ID) + describe "on 'user_added_to_group'" do + let(:group) { Fabricate(:group) } + let(:user) { Fabricate(:user) } + let!(:first_assignment) { Fabricate(:topic_assignment, assigned_to: group) } + let!(:second_assignment) { Fabricate(:post_assignment, assigned_to: group) } + let!(:third_assignment) { Fabricate(:topic_assignment, assigned_to: group, active: false) } + + it "creates missing notifications for added user" do + group.add(user) + [first_assignment, second_assignment].each do |assignment| + expect_job_enqueued(job: Jobs::AssignNotification, args: { assignment_id: assignment.id }) + end + expect( + job_enqueued?( + job: Jobs::AssignNotification, + args: { + assignment_id: third_assignment.id, + }, + ), + ).to eq(false) end end end diff --git a/spec/post_migrations/ensure_notifications_consistency_spec.rb b/spec/post_migrations/ensure_notifications_consistency_spec.rb new file mode 100644 index 00000000..c69d8164 --- /dev/null +++ b/spec/post_migrations/ensure_notifications_consistency_spec.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +require "rails_helper" +require Rails.root.join( + "plugins/discourse-assign/db/post_migrate/20231011152903_ensure_notifications_consistency", + ) + +# As this post migration is calling app code, we want to ensure its behavior +# won’t change over time. +RSpec.describe EnsureNotificationsConsistency do + describe "#up" do + subject(:migrate) { described_class.new.up } + + context "when notification targeting a non-existing assignment exists" do + let(:post) { Fabricate(:post) } + let!(:notifications) do + Fabricate( + :notification, + notification_type: Notification.types[:assigned], + post: post, + data: { assignment_id: 1 }.to_json, + ) + end + + it "deletes it" do + expect { migrate }.to change { Notification.count }.by(-1) + end + end + + context "when notification targeting an inactive assignment exists" do + let(:post) { Fabricate(:post) } + let(:assignment) { Fabricate(:topic_assignment, topic: post.topic, active: false) } + let!(:notifications) do + Fabricate( + :notification, + notification_type: Notification.types[:assigned], + post: post, + data: { assignment_id: assignment.id }.to_json, + ) + end + + it "deletes it" do + expect { migrate }.to change { Notification.count }.by(-1) + end + end + + context "when some active assignments exist" do + let(:post) { Fabricate(:post) } + let(:group) { Fabricate(:group) } + let!(:assignment) { Fabricate(:topic_assignment, topic: post.topic, assigned_to: group) } + let!(:inactive_assignment) { Fabricate(:post_assignment, post: post, active: false) } + let!(:assignment_with_deleted_topic) { Fabricate(:topic_assignment) } + + before do + group.users << Fabricate(:user) + assignment_with_deleted_topic.topic.trash! + end + + context "when notifications are missing" do + it "creates them" do + expect { migrate }.to change { Notification.assigned.count }.by(1) + end + end + end + end +end diff --git a/spec/requests/assign_controller_spec.rb b/spec/requests/assign_controller_spec.rb index 35870d21..5b938b2c 100644 --- a/spec/requests/assign_controller_spec.rb +++ b/spec/requests/assign_controller_spec.rb @@ -379,7 +379,7 @@ def assign_user_to_post it "doesn't include members with no assignments" do sign_in(admin) - allowed_group.add(non_admin_staff) + allowed_group.users << non_admin_staff get "/assign/members/#{allowed_group.name}.json" expect(response.status).to eq(200) @@ -458,138 +458,4 @@ def assign_user_to_post expect(response.status).to eq(200) end end - - describe "#user_menu_assigns" do - fab!(:non_member_admin) { Fabricate(:admin) } - - fab!(:unread_assigned_topic) { Fabricate(:post).topic } - fab!(:read_assigned_topic) { Fabricate(:post).topic } - - fab!(:unread_assigned_post) { Fabricate(:post, topic: Fabricate(:post).topic) } - fab!(:read_assigned_post) { Fabricate(:post, topic: Fabricate(:post).topic) } - - fab!(:read_assigned_post_in_same_topic) { Fabricate(:post, topic: Fabricate(:post).topic) } - fab!(:unread_assigned_post_in_same_topic) do - Fabricate(:post, topic: read_assigned_post_in_same_topic.topic) - end - - fab!(:another_user_unread_assigned_topic) { Fabricate(:post).topic } - fab!(:another_user_read_assigned_topic) { Fabricate(:post).topic } - - before do - Jobs.run_immediately! - - [ - unread_assigned_topic, - read_assigned_topic, - unread_assigned_post, - read_assigned_post, - unread_assigned_post_in_same_topic, - read_assigned_post_in_same_topic, - ].each { |target| Assigner.new(target, non_member_admin).assign(admin) } - - Notification - .where( - notification_type: Notification.types[:assigned], - read: false, - user_id: admin.id, - topic_id: [ - read_assigned_topic.id, - read_assigned_post.topic.id, - read_assigned_post_in_same_topic.topic.id, - ], - ) - .where.not( - topic_id: read_assigned_post_in_same_topic.topic.id, - post_number: unread_assigned_post_in_same_topic.post_number, - ) - .update_all(read: true) - - Assigner.new(another_user_read_assigned_topic, non_member_admin).assign(allowed_user) - Assigner.new(another_user_unread_assigned_topic, non_member_admin).assign(allowed_user) - Notification.where( - notification_type: Notification.types[:assigned], - read: false, - user_id: allowed_user.id, - topic_id: another_user_read_assigned_topic, - ).update_all(read: true) - end - - context "when logged out" do - it "responds with 403" do - get "/assign/user-menu-assigns.json" - expect(response.status).to eq(403) - end - end - - context "when logged in" do - before { sign_in(admin) } - - it "responds with 403 if the current user can't assign" do - admin.update!(admin: false) - admin.group_users.where(group_id: staff_group.id).destroy_all - get "/assign/user-menu-assigns.json" - expect(response.status).to eq(403) - end - - it "responds with 404 if the assign_enabled setting is disabled" do - SiteSetting.assign_enabled = false - get "/assign/user-menu-assigns.json" - expect(response.status).to eq(404) - end - - it "sends an array of unread assigned notifications" do - get "/assign/user-menu-assigns.json" - expect(response.status).to eq(200) - - notifications = response.parsed_body["notifications"] - expect(notifications.map { |n| [n["topic_id"], n["post_number"]] }).to match_array( - [ - [unread_assigned_topic.id, 1], - [unread_assigned_post.topic.id, unread_assigned_post.post_number], - [ - unread_assigned_post_in_same_topic.topic.id, - unread_assigned_post_in_same_topic.post_number, - ], - ], - ) - end - - it "responds with an array of assigned topics that are not associated with any of the unread assigned notifications" do - get "/assign/user-menu-assigns.json" - expect(response.status).to eq(200) - - topics = response.parsed_body["topics"] - expect(topics.map { |t| t["id"] }).to eq( - [ - read_assigned_post_in_same_topic.topic.id, - read_assigned_post.topic.id, - read_assigned_topic.id, - ], - ) - end - - it "fills up the remaining of the UsersController::USER_MENU_LIST_LIMIT limit with assigned topics" do - stub_const(UsersController, "USER_MENU_LIST_LIMIT", 3) do - get "/assign/user-menu-assigns.json" - end - expect(response.status).to eq(200) - - notifications = response.parsed_body["notifications"] - expect(notifications.size).to eq(3) - topics = response.parsed_body["topics"] - expect(topics.size).to eq(0) - - stub_const(UsersController, "USER_MENU_LIST_LIMIT", 4) do - get "/assign/user-menu-assigns.json" - end - expect(response.status).to eq(200) - - notifications = response.parsed_body["notifications"] - expect(notifications.size).to eq(3) - topics = response.parsed_body["topics"] - expect(topics.size).to eq(1) - end - end - end end diff --git a/test/javascripts/acceptance/assigns-tab-user-menu-test.js b/test/javascripts/acceptance/assigns-tab-user-menu-test.js index f330e038..d80ff309 100644 --- a/test/javascripts/acceptance/assigns-tab-user-menu-test.js +++ b/test/javascripts/acceptance/assigns-tab-user-menu-test.js @@ -1,6 +1,5 @@ import { click, currentURL, visit } from "@ember/test-helpers"; import { test } from "qunit"; -import { withPluginApi } from "discourse/lib/plugin-api"; import { acceptance, query, @@ -20,179 +19,32 @@ const USER_MENU_ASSIGN_RESPONSE = { created_at: "2022-08-11T21:32:32.404Z", post_number: 1, topic_id: 227, - fancy_title: "Test poll topic please bear with me", + fancy_title: "Test poll topic please bear with me :heart:", slug: "test-poll-topic-please-bear-with-me", data: { message: "discourse_assign.assign_notification", display_username: "tony", - topic_title: "Test poll topic please bear with me", + topic_title: "Test poll topic please bear with me :heart:", assignment_id: 2, }, }, - ], - topics: [ - { - id: 209, - title: "Howdy this a test topic!", - fancy_title: "Howdy this my test topic with emoji :heart:!", - slug: "howdy-this-a-test-topic", - posts_count: 1, - reply_count: 0, - highest_post_number: 1, - image_url: null, - created_at: "2022-03-10T20:09:25.772Z", - last_posted_at: "2022-03-10T20:09:25.959Z", - bumped: true, - bumped_at: "2022-03-10T20:09:25.959Z", - archetype: "regular", - unseen: false, - last_read_post_number: 2, - unread: 0, - new_posts: 0, - unread_posts: 0, - pinned: false, - unpinned: null, - visible: true, - closed: false, - archived: false, - notification_level: 3, - bookmarked: false, - liked: false, - thumbnails: null, - tags: [], - tags_descriptions: {}, - views: 11, - like_count: 7, - has_summary: false, - last_poster_username: "osama", - category_id: 1, - pinned_globally: false, - featured_link: null, - assigned_to_user: { - id: 1, - username: "osama", - name: "Osama.OG", - avatar_template: "/letter_avatar_proxy/v4/letter/o/f05b48/{size}.png", - assign_icon: "user-plus", - assign_path: "/u/osama/activity/assigned", - }, - posters: [ - { - extras: "latest single", - description: "Original Poster, Most Recent Poster", - user_id: 1, - primary_group_id: 45, - flair_group_id: 45, - }, - ], - }, { - id: 173, - title: "Owners elegance entrance startled spirits losing", - fancy_title: - "Owners elegance entrance :car: startled spirits losing", - slug: "owners-elegance-entrance-startled-spirits-losing", - posts_count: 7, - reply_count: 0, - highest_post_number: 7, - image_url: null, - created_at: "2021-07-11T04:50:17.029Z", - last_posted_at: "2021-12-24T17:21:03.418Z", - bumped: true, - bumped_at: "2021-12-24T17:21:03.418Z", - archetype: "regular", - unseen: false, - last_read_post_number: 3, - unread: 0, - new_posts: 0, - unread_posts: 0, - pinned: false, - unpinned: null, - visible: true, - closed: false, - archived: false, - notification_level: 1, - bookmarked: false, - liked: false, - thumbnails: null, - tags: ["music", "job-application"], - tags_descriptions: {}, - views: 23, - like_count: 24, - has_summary: false, - last_poster_username: "ambrose.bradtke", - category_id: 1, - pinned_globally: false, - featured_link: null, - assigned_to_group: { - id: 45, - automatic: false, - name: "Team", - user_count: 4, - mentionable_level: 99, - messageable_level: 99, - visibility_level: 0, - primary_group: true, - title: "", - grant_trust_level: null, - incoming_email: null, - has_messages: true, - flair_url: null, - flair_bg_color: "", - flair_color: "", - bio_raw: "", - bio_cooked: null, - bio_excerpt: null, - public_admission: true, - public_exit: true, - allow_membership_requests: false, - full_name: "", - default_notification_level: 3, - membership_request_template: "", - members_visibility_level: 0, - can_see_members: true, - can_admin_group: true, - publish_read_state: true, - assign_icon: "group-plus", - assign_path: "/g/Team/assigned/everyone", + id: 1717, + user_id: 1, + notification_type: 34, + read: true, + high_priority: true, + created_at: "2022-08-11T21:32:32.404Z", + post_number: 1, + topic_id: 228, + fancy_title: "Test poll topic please bear with me 2 :ok_hand:", + slug: "test-poll-topic-please-bear-with-me-2", + data: { + message: "discourse_assign.assign_group_notification", + display_username: "Team", + topic_title: "Test poll topic please bear with me 2 :ok_hand:", + assignment_id: 3, }, - posters: [ - { - extras: null, - description: "Original Poster", - user_id: 26, - primary_group_id: null, - flair_group_id: null, - }, - { - extras: null, - description: "Frequent Poster", - user_id: 16, - primary_group_id: null, - flair_group_id: null, - }, - { - extras: null, - description: "Frequent Poster", - user_id: 22, - primary_group_id: null, - flair_group_id: null, - }, - { - extras: null, - description: "Frequent Poster", - user_id: 12, - primary_group_id: null, - flair_group_id: null, - }, - { - extras: "latest", - description: "Most Recent Poster", - user_id: 13, - primary_group_id: null, - flair_group_id: null, - }, - ], }, ], }; @@ -250,9 +102,9 @@ acceptance("Discourse Assign | user menu", function (needs) { let requestBody; needs.pretender((server, helper) => { - server.get("/assign/user-menu-assigns.json", () => { + server.get("/notifications", () => { if (forceEmptyState) { - return helper.response({ notifications: [], topics: [] }); + return helper.response({ notifications: [] }); } else { return helper.response(USER_MENU_ASSIGN_RESPONSE); } @@ -321,7 +173,9 @@ acceptance("Discourse Assign | user menu", function (needs) { await click(".d-header-icons .current-user"); await click("#user-menu-button-assign-list"); - const notifications = queryAll("#quick-access-assign-list .notification"); + const notifications = queryAll( + "#quick-access-assign-list .notification.unread" + ); assert.strictEqual( notifications.length, 1, @@ -336,7 +190,7 @@ acceptance("Discourse Assign | user menu", function (needs) { "the notification is of type assigned" ); - const assigns = queryAll("#quick-access-assign-list .assign"); + const assigns = queryAll("#quick-access-assign-list .assigned"); assert.strictEqual(assigns.length, 2, "there are 2 assigns"); const userAssign = assigns[0]; @@ -353,59 +207,44 @@ acceptance("Discourse Assign | user menu", function (needs) { assert.true( userAssign .querySelector("a") - .href.endsWith("/t/howdy-this-a-test-topic/209/3"), - "user assign links to the first unread post (last read post + 1)" + .href.endsWith("/t/test-poll-topic-please-bear-with-me/227"), + "user assign links to the assigned topic" ); assert.true( groupAssign .querySelector("a") - .href.endsWith( - "/t/owners-elegance-entrance-startled-spirits-losing/173/4" - ), - "group assign links to the first unread post (last read post + 1)" + .href.endsWith("/t/test-poll-topic-please-bear-with-me-2/228"), + "group assign links to the assigned topic" ); assert.strictEqual( userAssign.textContent.trim(), - "Howdy this my test topic with emoji !", + "Test poll topic please bear with me", "user assign contains the topic title" ); assert.ok( userAssign.querySelector(".item-description img.emoji"), "emojis are rendered in user assign" ); - assert.strictEqual( - userAssign.querySelector(".item-description b").textContent.trim(), - "my test topic", - "user assign topic title is trusted" - ); assert.strictEqual( groupAssign.textContent.trim().replaceAll(/\s+/g, " "), - "Owners elegance entrance startled spirits losing", + "Team Test poll topic please bear with me 2", "group assign contains the topic title" ); assert.ok( - groupAssign.querySelector(".item-description i img.emoji"), + groupAssign.querySelector(".item-description img.emoji"), "emojis are rendered in group assign" ); - assert.strictEqual( - groupAssign - .querySelector(".item-description i") - .textContent.trim() - .replaceAll(/\s+/g, " "), - "elegance entrance startled", - "group assign topic title is trusted" - ); assert.strictEqual( userAssign.querySelector("a").title, - I18n.t("user.assigned_to_you"), + I18n.t("user.assigned_to_you.topic"), "user assign has the right title" ); assert.strictEqual( groupAssign.querySelector("a").title, - I18n.t("user.assigned_to_group", { group_name: "Team" }), + I18n.t("user.assigned_to_group.topic", { group_name: "Team" }), "group assign has the right title" ); }); @@ -466,37 +305,6 @@ acceptance("Discourse Assign | user menu", function (needs) { ); }); - test("assigns tab applies model transformations", async function (assert) { - withPluginApi("0.1", (api) => { - api.registerModelTransformer("notification", (notifications) => { - notifications.forEach((notification) => { - notification.fancy_title = `notificationModelTransformer ${notification.fancy_title}`; - }); - }); - api.registerModelTransformer("topic", (topics) => { - topics.forEach((topic) => { - topic.fancy_title = `topicModelTransformer ${topic.fancy_title}`; - }); - }); - }); - - await visit("/"); - await click(".d-header-icons .current-user"); - await click("#user-menu-button-assign-list"); - - const notification = query("#quick-access-assign-list ul li.notification"); - assert.strictEqual( - notification.textContent.replace(/\s+/g, " ").trim(), - "tony notificationModelTransformer Test poll topic please bear with me" - ); - - const assign = query("#quick-access-assign-list ul li.assign"); - assert.strictEqual( - assign.textContent.replace(/\s+/g, " ").trim(), - "topicModelTransformer Howdy this my test topic with emoji !" - ); - }); - test("renders the confirmation modal when dismiss assign notifications", async function (assert) { await visit("/"); await click(".d-header-icons .current-user");