FIX: Sum assignments for group and group users (#482)

Use the assignment count rather than the number of topics assigned for the number of assignments in the nav

app/controllers/discourse_assign/assign_controller.rb

@@ -125,7 +125,7 @@ module DiscourseAssign
 
       guardian.ensure_can_see_group_members!(group)
 
-      members =
+      users_with_assignments_count =
         User
           .joins("LEFT OUTER JOIN group_users g ON g.user_id = users.id")
           .joins(
@@ -140,25 +140,22 @@ module DiscourseAssign
           .limit(limit)
           .offset(offset)
 
-      members = members.where(<<~SQL, pattern: "%#{params[:filter]}%") if params[:filter]
+      users_with_assignments_count =
+        users_with_assignments_count.where(<<~SQL, pattern: "%#{params[:filter]}%") if params[
           users.name ILIKE :pattern OR users.username_lower ILIKE :pattern
         SQL
-
+        :filter
-      group_assignments =
+      ]
-        Topic
+      group_assignments_count = Assignment.active_for_group(group).count
-          .joins("JOIN assignments a ON a.topic_id = topics.id")
+      users_assignments_count =
-          .where(<<~SQL, group_id: group.id)
+        users_with_assignments_count.reduce(0) do |sum, assignment|
-          a.assigned_to_id = :group_id AND a.assigned_to_type = 'Group' AND a.active
+          sum + assignment.assignments_count
-        SQL
+        end
-          .pluck(:topic_id)
-
-      assignments =
-        TopicQuery.new(current_user).group_topics_assigned_results(group).pluck("topics.id")
 
       render json: {
-               members: serialize_data(members, GroupUserAssignedSerializer),
+               members: serialize_data(users_with_assignments_count, GroupUserAssignedSerializer),
-               assignment_count: (assignments | group_assignments).count,
+               assignment_count: users_assignments_count + group_assignments_count,
-               group_assignment_count: group_assignments.count,
+               group_assignment_count: group_assignments_count,
              }
     end
 

app/models/assignment.rb

@@ -15,6 +15,8 @@ class Assignment < ActiveRecord::Base
           )
         }
 
+  scope :active_for_group, ->(group) { where(assigned_to: group, active: true) }
+
   before_validation :default_status
 
   validate :validate_status, if: -> { SiteSetting.enable_assign_status }

spec/fabricators/assignment_fabricator.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+Fabricator(:topic_assignment, class_name: :assignment) do
+  topic
+  target { |attrs| attrs[:topic] }
+  target_type "Topic"
+  assigned_by_user { Fabricate(:user) }
+  assigned_to { Fabricate(:user) }
+end
+
+Fabricator(:post_assignment, class_name: :assignment) do
+  topic { |attrs| attrs[:post]&.topic || Fabricate(:topic) }
+  target { |attrs| attrs[:post] || Fabricate(:post, topic: attrs[:topic]) }
+  target_type "Post"
+  assigned_by_user { Fabricate(:user) }
+  assigned_to { Fabricate(:user) }
+end

spec/models/assignment_spec.rb

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+describe Assignment do
+  fab!(:group) { Fabricate(:group) }
+  fab!(:user1) { Fabricate(:user) }
+  fab!(:user2) { Fabricate(:user) }
+  fab!(:group_user1) { Fabricate(:group_user, user: user1, group: group) }
+  fab!(:group_user1) { Fabricate(:group_user, user: user2, group: group) }
+
+  fab!(:wrong_group) { Fabricate(:group) }
+
+  before { SiteSetting.assign_enabled = true }
+
+  describe "#active_for_group" do
+    it "returns active assignments for the group" do
+      assignment1 = Fabricate(:topic_assignment, assigned_to: group)
+      assignment2 = Fabricate(:post_assignment, assigned_to: group)
+      Fabricate(:post_assignment, assigned_to: group, active: false)
+      Fabricate(:post_assignment, assigned_to: user1)
+      Fabricate(:topic_assignment, assigned_to: wrong_group)
+
+      expect(Assignment.active_for_group(group)).to contain_exactly(assignment1, assignment2)
+    end
+  end
+end

spec/requests/assign_controller_spec.rb

@@ -364,56 +364,98 @@ RSpec.describe DiscourseAssign::AssignController do
   describe "#group_members" do
     include_context "with group that is allowed to assign"
 
-    fab!(:post1) { Fabricate(:post) }
-    fab!(:post2) { Fabricate(:post) }
-    fab!(:post3) { Fabricate(:post) }
-
     before do
       add_to_assign_allowed_group(user2)
       add_to_assign_allowed_group(user)
 
-      Assigner.new(post1.topic, user).assign(user)
+      topic = Fabricate(:topic)
-      Assigner.new(post2.topic, user).assign(user2)
+      post_in_same_topic = Fabricate(:post, topic: topic)
-      Assigner.new(post3.topic, user).assign(user)
+
+      Fabricate(:post_assignment, assigned_to: user, target: topic, assigned_by_user: user)
+      Fabricate(
+        :topic_assignment,
+        assigned_to: user,
+        target: post_in_same_topic,
+        assigned_by_user: user,
+      )
+      Fabricate(:topic_assignment, assigned_to: user2, assigned_by_user: user)
+      Fabricate(:topic_assignment, assigned_to: user, assigned_by_user: user)
+
+      Fabricate(:topic_assignment, assigned_to: assign_allowed_group, assigned_by_user: user)
+      Fabricate(:post_assignment, assigned_to: assign_allowed_group, assigned_by_user: user)
     end
 
-    it "list members order by assignments_count" do
+    describe "members" do
-      sign_in(user)
+      describe "without filter" do
+        it "list members ordered by the number of assignments" do
+          sign_in(user)
 
-      get "/assign/members/#{get_assigned_allowed_group_name}.json"
+          get "/assign/members/#{get_assigned_allowed_group_name}.json"
-      expect(response.status).to eq(200)
+          members = JSON.parse(response.body)["members"]
-      expect(JSON.parse(response.body)["members"].map { |m| m["id"] }).to match_array(
+
-        [user.id, user2.id],
+          expect(response.status).to eq(200)
-      )
+          expect(members[0]).to include({ "id" => user.id, "assignments_count" => 3 })
+          expect(members[1]).to include({ "id" => user2.id, "assignments_count" => 1 })
+        end
+
+        it "doesn't include members with no assignments" do
+          sign_in(user)
+          add_to_assign_allowed_group(non_admin)
+
+          get "/assign/members/#{get_assigned_allowed_group_name}.json"
+          expect(response.status).to eq(200)
+          expect(JSON.parse(response.body)["members"].map { |m| m["id"] }).to_not include(
+            non_admin.id,
+          )
+        end
+      end
+
+      describe "with filter" do
+        it "returns members as according to filter" do
+          sign_in(user)
+
+          get "/assign/members/#{get_assigned_allowed_group_name}.json", params: { filter: "a" }
+          expect(response.status).to eq(200)
+          expect(JSON.parse(response.body)["members"].map { |m| m["id"] }).to match_array(
+            [user.id, user2.id],
+          )
+
+          get "/assign/members/#{get_assigned_allowed_group_name}.json", params: { filter: "david" }
+          expect(response.status).to eq(200)
+          expect(JSON.parse(response.body)["members"].map { |m| m["id"] }).to match_array(
+            [user2.id],
+          )
+
+          get "/assign/members/#{get_assigned_allowed_group_name}.json",
+              params: {
+                filter: "Taylor",
+              }
+          expect(response.status).to eq(200)
+          expect(JSON.parse(response.body)["members"].map { |m| m["id"] }).to match_array(
+            [user2.id],
+          )
+        end
+      end
     end
 
-    it "doesn't include members with no assignments" do
+    describe "assignment_count" do
-      sign_in(user)
+      it "returns the total number of assignments for group users and the group" do
-      add_to_assign_allowed_group(non_admin)
+        sign_in(user)
 
-      get "/assign/members/#{get_assigned_allowed_group_name}.json"
+        get "/assign/members/#{get_assigned_allowed_group_name}.json"
-      expect(response.status).to eq(200)
+
-      expect(JSON.parse(response.body)["members"].map { |m| m["id"] }).to match_array(
+        expect(JSON.parse(response.body)["assignment_count"]).to eq(6)
-        [user.id, user2.id],
+      end
-      )
     end
 
-    it "returns members as according to filter" do
+    describe "group_assignment_count" do
-      sign_in(user)
+      it "returns the number of assignments assigned to the group" do
+        sign_in(user)
 
-      get "/assign/members/#{get_assigned_allowed_group_name}.json", params: { filter: "a" }
+        get "/assign/members/#{get_assigned_allowed_group_name}.json"
-      expect(response.status).to eq(200)
-      expect(JSON.parse(response.body)["members"].map { |m| m["id"] }).to match_array(
-        [user.id, user2.id],
-      )
 
-      get "/assign/members/#{get_assigned_allowed_group_name}.json", params: { filter: "david" }
+        expect(JSON.parse(response.body)["group_assignment_count"]).to eq(2)
-      expect(response.status).to eq(200)
+      end
-      expect(JSON.parse(response.body)["members"].map { |m| m["id"] }).to match_array([user2.id])
-
-      get "/assign/members/#{get_assigned_allowed_group_name}.json", params: { filter: "Taylor" }
-      expect(response.status).to eq(200)
-      expect(JSON.parse(response.body)["members"].map { |m| m["id"] }).to match_array([user2.id])
     end
 
     it "404 error to non-group-members" do