FIX: Find better users for automatic assignment (#370)

There were two problems with the way current automation script for automatic
assignment works:

- it tried to assign users that were not allowed to be assigned because they
were not part of groups that are allowed to use discourse-assign - fixed by
skipping users that are not a part of assign_allowed_on_groups groups

- it assigned new users - fixed by adding a new user that can skip new users

app/controllers/discourse_assign/assign_controller.rb

@@ -175,46 +175,40 @@ module DiscourseAssign
     end
 
     def user_menu_assigns
-      assign_notifications = Notification.unread_type(
+      assign_notifications =
-        current_user,
+        Notification.unread_type(current_user, Notification.types[:assigned], user_menu_limit)
-        Notification.types[:assigned],
-        user_menu_limit
-      )
 
       if assign_notifications.size < user_menu_limit
         opts = {}
-        ignored_assignment_ids = assign_notifications.filter_map do |notification|
+        ignored_assignment_ids =
-          notification.data_hash[:assignment_id]
+          assign_notifications.filter_map { |notification| notification.data_hash[:assignment_id] }
-        end
         opts[:ignored_assignment_ids] = ignored_assignment_ids if ignored_assignment_ids.present?
 
-        assigns_list = TopicQuery.new(
+        assigns_list =
-          current_user,
+          TopicQuery.new(
-          per_page: user_menu_limit - assign_notifications.size
+            current_user,
-        ).list_messages_assigned(current_user, ignored_assignment_ids: ignored_assignment_ids)
+            per_page: user_menu_limit - assign_notifications.size,
+          ).list_messages_assigned(current_user, ignored_assignment_ids: ignored_assignment_ids)
       end
 
       if assign_notifications.present?
-        serialized_notifications = ActiveModel::ArraySerializer.new(
+        serialized_notifications =
-          assign_notifications,
+          ActiveModel::ArraySerializer.new(
-          each_serializer: NotificationSerializer,
+            assign_notifications,
-          scope: guardian
+            each_serializer: NotificationSerializer,
-        )
+            scope: guardian,
+          )
       end
 
       if assigns_list
-        serialized_assigns = serialize_data(
+        serialized_assigns =
-          assigns_list,
+          serialize_data(assigns_list, TopicListSerializer, scope: guardian, root: false)[:topics]
-          TopicListSerializer,
-          scope: guardian,
-          root: false
-        )[:topics]
       end
 
       render json: {
-        notifications: serialized_notifications || [],
+               notifications: serialized_notifications || [],
-        topics: serialized_assigns || [],
+               topics: serialized_assigns || [],
-      }
+             }
     end
 
     private

config/locales/client.en.yml

@@ -126,6 +126,9 @@ en:
             min_recently_assigned_days:
               label: Min recently assigned days
               description: Defaults to 14 days.
+            skip_new_users_for_days:
+              label: Skip new users for days
+              description: Defaults to 0 days (users can be assigned immediately after signing up).
             max_recently_assigned_days:
               label: Max recently assigned days
               description: First attempt to exclude users assigned in the last `n` days. If no user left, fallbacks to `min_recently_assigned_days`. Defaults to 180 days.

lib/random_assign_utils.rb

@@ -38,6 +38,7 @@ class RandomAssignUtils
       raise_error(automation, "Group(#{group_id}) not found")
     end
 
+    assignable_user_ids = User.assign_allowed.pluck(:id)
     users_on_holiday =
       Set.new(
         User.where(
@@ -45,11 +46,15 @@ class RandomAssignUtils
         ).pluck(:id),
       )
 
+    group_users = group.group_users.joins(:user)
+    if skip_new_users_for_days = fields.dig("skip_new_users_for_days", "value").presence
+      group_users = group_users.where("users.created_at < ?", skip_new_users_for_days.to_i.days.ago)
+    end
+
     group_users_ids =
-      group
+      group_users
-        .group_users
-        .joins(:user)
         .pluck("users.id")
+        .filter { |user_id| assignable_user_ids.include?(user_id) }
         .reject { |user_id| users_on_holiday.include?(user_id) }
 
     if group_users_ids.empty?

plugin.rb

@@ -400,9 +400,7 @@ after_initialize do
     SQL
 
     where_args = { user_id: user.id }
-    if ignored_assignment_ids.present?
+    where_args[:ignored_assignment_ids] = ignored_assignment_ids if ignored_assignment_ids.present?
-      where_args[:ignored_assignment_ids] = ignored_assignment_ids
-    end
     list = list.where("topics.id IN (#{topic_ids_sql})", **where_args).includes(:allowed_users)
 
     create_list(:assigned, { unordered: true }, list)
@@ -978,6 +976,7 @@ after_initialize do
       field :minimum_time_between_assignments, component: :text
       field :max_recently_assigned_days, component: :text
       field :min_recently_assigned_days, component: :text
+      field :skip_new_users_for_days, component: :text
       field :in_working_hours, component: :boolean
       field :post_template, component: :post
 

spec/lib/random_assign_utils_spec.rb

@@ -44,7 +44,7 @@ describe RandomAssignUtils do
           automation,
         )
         expect(topic_1.posts.first.raw).to match(
-          "Attempted randomly assign a member of `@#{group_1.name}`, but no one was available.",
+          I18n.t("discourse_automation.scriptables.random_assign.no_one", group: group_1.name),
         )
       end
     end
@@ -73,7 +73,33 @@ describe RandomAssignUtils do
           automation,
         )
         expect(topic_1.posts.first.raw).to match(
-          "Attempted randomly assign a member of `@#{group_1.name}`, but no one was available.",
+          I18n.t("discourse_automation.scriptables.random_assign.no_one", group: group_1.name),
+        )
+      end
+    end
+
+    context "no users can be assigned because none are members of assign_allowed_on_groups groups" do
+      fab!(:topic_1) { Fabricate(:topic) }
+      fab!(:group_1) { Fabricate(:group) }
+      fab!(:user_1) { Fabricate(:user) }
+
+      before { group_1.add(user_1) }
+
+      it "creates post on the topic" do
+        described_class.automation_script!(
+          {},
+          {
+            "assignees_group" => {
+              "value" => group_1.id,
+            },
+            "assigned_topic" => {
+              "value" => topic_1.id,
+            },
+          },
+          automation,
+        )
+        expect(topic_1.posts.first.raw).to match(
+          I18n.t("discourse_automation.scriptables.random_assign.no_one", group: group_1.name),
         )
       end
     end
@@ -158,7 +184,7 @@ describe RandomAssignUtils do
           automation,
         )
         expect(topic_1.posts.first.raw).to match(
-          "Attempted randomly assign a member of `@#{group_1.name}`, but no one was available.",
+          I18n.t("discourse_automation.scriptables.random_assign.no_one", group: group_1.name),
         )
       end
     end
@@ -250,6 +276,59 @@ describe RandomAssignUtils do
         end
       end
     end
+
+    context "skip_new_users_for_days is set" do
+      fab!(:topic_1) { Fabricate(:topic) }
+      fab!(:post_1) { Fabricate(:post, topic: topic_1) }
+      fab!(:group_1) { Fabricate(:group) }
+      fab!(:user_1) { Fabricate(:user) }
+
+      before do
+        SiteSetting.assign_allowed_on_groups = "#{group_1.id}"
+        group_1.add(user_1)
+      end
+
+      it "creates post on the topic if all users are new" do
+        described_class.automation_script!(
+          {},
+          {
+            "assignees_group" => {
+              "value" => group_1.id,
+            },
+            "assigned_topic" => {
+              "value" => topic_1.id,
+            },
+            "skip_new_users_for_days" => {
+              "value" => "10",
+            },
+          },
+          automation,
+        )
+        expect(topic_1.posts.last.raw).to match(
+          I18n.t("discourse_automation.scriptables.random_assign.no_one", group: group_1.name),
+        )
+      end
+
+      it "assign topic if all users are old" do
+        described_class.automation_script!(
+          {},
+          {
+            "assignees_group" => {
+              "value" => group_1.id,
+            },
+            "assigned_topic" => {
+              "value" => topic_1.id,
+            },
+            "skip_new_users_for_days" => {
+              "value" => "0",
+            },
+          },
+          automation,
+        )
+
+        expect(topic_1.assignment).not_to eq(nil)
+      end
+    end
   end
 
   describe ".recently_assigned_users_ids" do

spec/requests/assign_controller_spec.rb

@@ -401,7 +401,9 @@ RSpec.describe DiscourseAssign::AssignController do
     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) { Fabricate(:post, topic: read_assigned_post_in_same_topic.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 }
@@ -416,9 +418,7 @@ RSpec.describe DiscourseAssign::AssignController do
         read_assigned_post,
         unread_assigned_post_in_same_topic,
         read_assigned_post_in_same_topic,
-      ].each do |target|
+      ].each { |target| Assigner.new(target, normal_admin).assign(user) }
-        Assigner.new(target, normal_admin).assign(user)
-      end
 
       Notification
         .where(
@@ -428,25 +428,23 @@ RSpec.describe DiscourseAssign::AssignController do
           topic_id: [
             read_assigned_topic.id,
             read_assigned_post.topic.id,
-            read_assigned_post_in_same_topic.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
+          post_number: unread_assigned_post_in_same_topic.post_number,
         )
         .update_all(read: true)
 
       Assigner.new(another_user_read_assigned_topic, normal_admin).assign(user2)
       Assigner.new(another_user_unread_assigned_topic, normal_admin).assign(user2)
-      Notification
+      Notification.where(
-        .where(
+        notification_type: Notification.types[:assigned],
-          notification_type: Notification.types[:assigned],
+        read: false,
-          read: false,
+        user_id: user2.id,
-          user_id: user2.id,
+        topic_id: another_user_read_assigned_topic,
-          topic_id: another_user_read_assigned_topic,
+      ).update_all(read: true)
-        )
-        .update_all(read: true)
     end
 
     context "when logged out" do
@@ -457,9 +455,7 @@ RSpec.describe DiscourseAssign::AssignController do
     end
 
     context "when logged in" do
-      before do
+      before { sign_in(user) }
-        sign_in(user)
-      end
 
       it "responds with 403 if the current user can't assign" do
         user.update!(admin: false)
@@ -479,11 +475,16 @@ RSpec.describe DiscourseAssign::AssignController do
         expect(response.status).to eq(200)
 
         notifications = response.parsed_body["notifications"]
-        expect(notifications.map { |n| [n["topic_id"], n["post_number"]] }).to eq([
+        expect(notifications.map { |n| [n["topic_id"], n["post_number"]] }).to eq(
-          [unread_assigned_topic.id, 1],
+          [
-          [unread_assigned_post.topic.id, unread_assigned_post.post_number],
+            [unread_assigned_topic.id, 1],
-          [unread_assigned_post_in_same_topic.topic.id, unread_assigned_post_in_same_topic.post_number]
+            [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
@@ -491,11 +492,13 @@ RSpec.describe DiscourseAssign::AssignController do
         expect(response.status).to eq(200)
 
         topics = response.parsed_body["topics"]
-        expect(topics.map { |t| t["id"] }).to eq([
+        expect(topics.map { |t| t["id"] }).to eq(
-          read_assigned_post_in_same_topic.topic.id,
+          [
-          read_assigned_post.topic.id,
+            read_assigned_post_in_same_topic.topic.id,
-          read_assigned_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