FIX: Display assignments in user menu properly

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.

app/controllers/discourse_assign/assign_controller.rb

@@ -159,43 +159,6 @@ module DiscourseAssign
              }
     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 @@ module DiscourseAssign
       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)

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?
-
+      Assignment.find(args[:assignment_id]).create_missing_notifications!
-      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
     end
   end
 end

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?
+      %i[topic_id assigned_to_id assigned_to_type assignment_id].each do |argument|
-      raise Discourse::InvalidParameters.new(:assigned_to_id) if args[:assigned_to_id].nil?
+        raise Discourse::InvalidParameters.new(argument) if args[argument].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)
 
+      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
-          .where(
+        .for_assignment(args[:assignment_id])
-            notification_type: Notification.types[:assigned] || Notification.types[:custom],
+        .where(user: assignment.assigned_users, topic: assignment.topic)
-            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
     end
   end
-  end
 end

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 @@ class Assignment < ActiveRecord::Base
   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

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");
+    // sorting by `data.message` length to group single user assignments and
-    const content = [];
+    // group assignments, then by `created_at` to keep chronological order.
-
+    return (await super.fetchItems())
-    const notifications = data.notifications.map((n) => Notification.create(n));
+      .sortBy("notification.data.message", "notification.created_at")
-    await Notification.applyTransformations(notifications);
+      .reverse();
-    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;
   }
 }

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,7 +9,6 @@ export default {
 
   initialize(container) {
     withPluginApi("1.2.0", (api) => {
-      if (api.registerUserMenuTab) {
       const siteSettings = container.lookup("service:site-settings");
       if (!siteSettings.assign_enabled) {
         return;
@@ -16,6 +18,65 @@ export default {
       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) {
         api.registerUserMenuTab((UserMenuTab) => {
           return class extends UserMenuTab {
             id = "assign-list";

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

assets/javascripts/discourse/lib/user-menu/assign-item.js

@@ -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;
-  }
-}

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: "<p><span>%{username}</span> %{description}</p>"
-      assign_group_notification: "<p><span>%{username}</span> %{description}</p>"
       unassign:
         title: "Unassign"
         title_w_ellipsis: "Unassign..."
@@ -97,8 +95,15 @@ en:
         <br><br>
         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_group:
-      assigned_to_you: "assigned to you"
+        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:

config/routes.rb

@@ -7,7 +7,6 @@ DiscourseAssign::Engine.routes.draw do
   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

db/post_migrate/20231011152903_ensure_notifications_consistency.rb

@@ -0,0 +1,26 @@
+# 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 do |assignment|
+        next if !assignment.target || !assignment.assigned_to
+        assignment.create_missing_notifications!(mark_as_read: true)
+      end
+  end
+
+  def down
+  end
+end

lib/assigner.rb

@@ -239,11 +239,8 @@ class ::Assigner
     end
 
     @target.assignment.update!(note: note, status: status)
-
+    queue_notification(@target.assignment)
-    queue_notification(assign_to, skip_small_action_post, @target.assignment)
+    publish_assignment(@target.assignment, assign_to, note, status)
-
-    assignment = @target.assignment
-    publish_assignment(assignment, assign_to, note, status)
 
     # email is skipped, for now
 
@@ -276,31 +273,28 @@ class ::Assigner
 
     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_id: @target.assignment.assigned_to_id,
-        assigned_to_type: topic.assignment.assigned_to_type,
+        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: assign_to,
-        assigned_to_type: assigned_to_type,
+        assigned_by_user: @assigned_by,
-        assigned_by_user_id: @assigned_by.id,
+        topic: topic,
-        topic_id: topic.id,
         note: note,
         status: status,
       )
 
     first_post.publish_change_to_clients!(:revised, reload_topic: true)
-
+    queue_notification(assignment)
-    queue_notification(assign_to, skip_small_action_post, assignment)
-
     publish_assignment(assignment, assign_to, note, status)
 
     if assignment.assigned_to_user?
@@ -370,6 +364,7 @@ class ::Assigner
         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 @@ class ::Assigner
     @guardian ||= Guardian.new(@assigned_by)
   end
 
-  def queue_notification(assign_to, skip_small_action_post, assignment)
+  def queue_notification(assignment)
-    Jobs.enqueue(
+    Jobs.enqueue(:assign_notification, assignment_id: assignment.id)
-      :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,
-    )
   end
 
   def add_small_action_post(action_code, assign_to, text)

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

lib/discourse_assign/group_extension.rb

@@ -2,8 +2,11 @@
 
 module DiscourseAssign
   module GroupExtension
-    def self.prepended(base)
+    extend ActiveSupport::Concern
-      base.class_eval do
+
+    prepended do
+      has_many :assignments, as: :assigned_to
+
       scope :assignable,
             ->(user) do
               where(
@@ -21,5 +24,4 @@ module DiscourseAssign
             end
     end
   end
-  end
 end

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

plugin.rb

@@ -26,10 +26,12 @@ after_initialize do
   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 @@ after_initialize do
   require_relative "lib/topic_assigner"
 
   reloadable_patch do |plugin|
-    Group.class_eval { prepend DiscourseAssign::GroupExtension }
+    Group.prepend(DiscourseAssign::GroupExtension)
-    ListController.class_eval { prepend DiscourseAssign::ListControllerExtension }
+    ListController.prepend(DiscourseAssign::ListControllerExtension)
-    Post.class_eval { prepend DiscourseAssign::PostExtension }
+    Post.prepend(DiscourseAssign::PostExtension)
-    Topic.class_eval { prepend DiscourseAssign::TopicExtension }
+    Topic.prepend(DiscourseAssign::TopicExtension)
-    WebHook.class_eval { prepend DiscourseAssign::WebHookExtension }
+    WebHook.prepend(DiscourseAssign::WebHookExtension)
+    Notification.prepend(DiscourseAssign::NotificationExtension)
   end
 
   register_group_param(:assignable_level)
@@ -201,7 +204,8 @@ after_initialize do
       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 @@ after_initialize do
     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(
+        Jobs.enqueue(:assign_notification, assignment_id: assignment.id)
-          :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,
-        )
       end
   end
 
@@ -863,30 +859,28 @@ after_initialize do
     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_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|
-    assign_allowed_groups = SiteSetting.assign_allowed_on_groups.split("|").map(&:to_i)
+    user.notifications.for_assignment(group.assignments.select(:id)).destroy_all
-
-    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
-    end
   end
 
   on(:post_moved) do |post, original_topic_id|

spec/fabricators/assignment_fabricator.rb

@@ -9,6 +9,7 @@ Fabricator(:topic_assignment, class_name: :assignment) do
 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"

spec/jobs/regular/assign_notification_spec.rb

@@ -4,199 +4,27 @@ require "rails_helper"
 
 RSpec.describe Jobs::AssignNotification do
   describe "#execute" do
-    fab!(:user1) { Fabricate(:user, last_seen_at: 1.day.ago) }
+    subject(:execute_job) { described_class.new.execute(args) }
-    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") }
 
-    def assert_publish_topic_state(topic, user)
+    let(:args) { { assignment_id: assignment_id } }
-      message = MessageBus.track_publish("/private-messages/assigned") { yield }.first
 
-      expect(message.data[:topic_id]).to eq(topic.id)
+    context "when `assignment_id` is not provided" do
-      expect(message.user_ids).to eq([user.id])
+      let(:args) { {} }
-    end
 
-    before { assign_allowed_group.add(user1) }
+      it "raises an error" do
-
+        expect { execute_job }.to raise_error(Discourse::InvalidParameters, "assignment_id")
-    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
 
-      it "sends a high priority notification to the assignee" do
+    context "when `assignment_id` is provided" do
-        Notification.expects(:create!).with(
+      let(:assignment_id) { Fabricate(:topic_assignment).id }
-          notification_type: Notification.types[:assigned] || Notification.types[:custom],
+      let(:assignment) { stub("assignment").responds_like_instance_of(Assignment) }
-          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,
-          },
-        )
-      end
-    end
 
-    describe "Group" do
+      before { Assignment.stubs(:find).with(assignment_id).returns(assignment) }
-      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
+      it "creates missing notifications for the provided assignment" do
-        group.add(user2)
+        assignment.expects(:create_missing_notifications!)
-        group.add(user3)
+        execute_job
-        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
-
-      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
-
-        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,
-          },
-        )
       end
     end
   end

spec/jobs/regular/unassign_notification_spec.rb

@@ -22,19 +22,11 @@ RSpec.describe Jobs::UnassignNotification do
     end
 
     describe "User" do
-      it "deletes notifications" do
+      fab!(:assignment) { Fabricate(:topic_assignment, topic: topic, assigned_to: user2) }
-        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,
-          },
-        )
 
+      before { assignment.create_missing_notifications! }
+
+      it "deletes notifications" do
         expect {
           described_class.new.execute(
             {
@@ -42,6 +34,7 @@ RSpec.describe Jobs::UnassignNotification do
               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 @@ RSpec.describe Jobs::UnassignNotification do
               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 @@ RSpec.describe Jobs::UnassignNotification do
       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 @@ RSpec.describe Jobs::UnassignNotification do
               post_id: post.id,
               assigned_to_id: group.id,
               assigned_to_type: "Group",
+              assignment_id: assignment.id,
             },
           )
         }.to change { Notification.count }.by(-2)

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

spec/models/assignment_spec.rb

@@ -2,26 +2,190 @@
 
 require "rails_helper"
 
-describe Assignment do
+RSpec.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) }
-
-  fab!(:wrong_group) { Fabricate(:group) }
-
   before { SiteSetting.assign_enabled = true }
 
-  describe "#active_for_group" do
+  describe ".active_for_group" do
-    it "returns active assignments for the group" do
+    subject(:assignments) { described_class.active_for_group(group) }
-      assignment1 = Fabricate(:topic_assignment, assigned_to: group)
+
-      assignment2 = Fabricate(:post_assignment, assigned_to: group)
+    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) }
+
+    before do
       Fabricate(:post_assignment, assigned_to: group, active: false)
       Fabricate(:post_assignment, assigned_to: user1)
       Fabricate(:topic_assignment, assigned_to: wrong_group)
+    end
 
-      expect(Assignment.active_for_group(group)).to contain_exactly(assignment1, assignment2)
+    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
+
+      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

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 "Events" do
-    describe "on user_removed_from_group" 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
+        group.users << user
-        @user = Fabricate(:user)
+        Fabricate(
-        @group_a = Fabricate(:group)
+          :notification,
-        @group_a.add(@user)
+          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
+      it "removes user's notifications related to group assignments" do
-        SiteSetting.assign_allowed_on_groups = @group_a.id.to_s
+        expect { group.remove(user) }.to change { user.notifications.assigned.count }.by(-2)
-
+      end
-        Assigner.new(@topic, Discourse.system_user).assign(@user)
-        @group_a.remove(@user)
-
-        expect(Assignment.count).to eq(0)
     end
 
-      it "doesn't unassign the user if it still has access through another group" do
+    describe "on 'user_added_to_group'" do
-        @group_b = Fabricate(:group)
+      let(:group) { Fabricate(:group) }
-        @group_b.add(@user)
+      let(:user) { Fabricate(:user) }
-        SiteSetting.assign_allowed_on_groups = [@group_a.id.to_s, @group_b.id.to_s].join("|")
+      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) }
 
-        Assigner.new(@topic, Discourse.system_user).assign(@user)
+      it "creates missing notifications for added user" do
-        @group_a.remove(@user)
+        group.add(user)
-
+        [first_assignment, second_assignment].each do |assignment|
-        assignment = Assignment.first
+          expect_job_enqueued(job: Jobs::AssignNotification, args: { assignment_id: assignment.id })
-        expect(assignment.assigned_to_id).to eq(@user.id)
+        end
-        expect(assignment.assigned_by_user_id).to eq(Discourse::SYSTEM_USER_ID)
+        expect(
+          job_enqueued?(
+            job: Jobs::AssignNotification,
+            args: {
+              assignment_id: third_assignment.id,
+            },
+          ),
+        ).to eq(false)
       end
     end
   end

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

spec/requests/assign_controller_spec.rb

@@ -379,7 +379,7 @@ RSpec.describe DiscourseAssign::AssignController do
 
         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 @@ RSpec.describe DiscourseAssign::AssignController do
       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

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,
+      id: 1717,
-      title: "Howdy this a test topic!",
-      fancy_title: "Howdy this <b>my test topic</b> 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,
+      notification_type: 34,
-          flair_group_id: 45,
+      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,
       },
-      ],
-    },
-    {
-      id: 173,
-      title: "Owners elegance entrance startled spirits losing",
-      fancy_title:
-        "Owners <i>elegance entrance :car: startled</i> 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",
-      },
-      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"),
+        .href.endsWith("/t/test-poll-topic-please-bear-with-me/227"),
-      "user assign links to the first unread post (last read post + 1)"
+      "user assign links to the assigned topic"
     );
     assert.true(
       groupAssign
         .querySelector("a")
-        .href.endsWith(
+        .href.endsWith("/t/test-poll-topic-please-bear-with-me-2/228"),
-          "/t/owners-elegance-entrance-startled-spirits-losing/173/4"
+      "group assign links to the assigned topic"
-        ),
-      "group assign links to the first unread post (last read post + 1)"
     );
 
     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");