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 @@ 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)

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

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

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

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();
   }
 }

assets/javascripts/discourse/initializers/assign-user-menu.js

@@ -1,21 +1,76 @@
 import { withPluginApi } from "discourse/lib/plugin-api";
+import I18n from "I18n";
 import UserMenuAssignNotificationsList from "../components/user-menu/assigns-list";
 
 export default {
   name: "assign-user-menu",
 
   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 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";

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