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");