FEATURE: better UI for group assignments (#197)

On the group assignment page, we should be able to display
- all topics assigned to that group + to user belonging to that group
- all topics assigned directly to group
- all topics assigned to individual user

app/controllers/discourse_assign/assign_controller.rb

@@ -126,17 +126,25 @@ module DiscourseAssign
         SQL
       end
 
-      assignment_count = Topic
-        .joins("JOIN assignments a ON a.topic_id = topics.id AND a.assigned_to_id IS NOT NULL ")
+      group_assignment_count = Topic
+        .joins("JOIN assignments a ON a.topic_id = topics.id")
         .where(<<~SQL, group_id: group.id)
-          (a.assigned_to_id IN (SELECT group_users.user_id FROM group_users WHERE group_id = :group_id) AND a.assigned_to_type = 'User')
-          OR
-          (a.assigned_to_id = :group_id AND a.assigned_to_type = 'Group')
+          a.assigned_to_id = :group_id AND a.assigned_to_type = 'Group'
         SQL
-        .where("topics.deleted_at IS NULL")
         .count
 
-      render json: { members: serialize_data(members, GroupUserAssignedSerializer), assignment_count: assignment_count }
+      assignment_count = Topic
+        .joins("JOIN assignments a ON a.topic_id = topics.id")
+        .joins("JOIN group_users ON group_users.user_id = a.assigned_to_id ")
+        .where("group_users.group_id = ?", group.id)
+        .where("a.assigned_to_type = 'User'")
+        .count
+
+      render json: {
+        members: serialize_data(members, GroupUserAssignedSerializer),
+        assignment_count: assignment_count + group_assignment_count,
+        group_assignment_count: group_assignment_count
+      }
     end
 
     private

assets/javascripts/discourse-assign/controllers/group-assigned-show.js.es6

@@ -38,6 +38,7 @@ export default UserTopicsList.extend({
           order: this.order,
           ascending: this.ascending,
           search: this.search,
+          direct: this.model.params.direct,
         },
       })
       .then((result) => this.set("model", result))

assets/javascripts/discourse-assign/routes/group-assigned-show.js.es6

@@ -13,12 +13,14 @@ export default DiscourseRoute.extend({
 
   model(params) {
     let filter = null;
-    if (params.filter !== "everyone") {
-      filter = `topics/messages-assigned/${params.filter}`;
-    } else {
+    if (
+      ["everyone", this.modelFor("group").get("name")].includes(params.filter)
+    ) {
       filter = `topics/group-topics-assigned/${this.modelFor("group").get(
         "name"
       )}`;
+    } else {
+      filter = `topics/messages-assigned/${params.filter}`;
     }
     const lastTopicList = findOrResetCachedTopicList(this.session, filter);
     return lastTopicList
@@ -29,6 +31,7 @@ export default DiscourseRoute.extend({
             order: params.order,
             ascending: params.ascending,
             search: params.search,
+            direct: params.filter !== "everyone",
           },
         });
   },

assets/javascripts/discourse-assign/routes/group-assigned.js.es6

@@ -13,7 +13,10 @@ export default DiscourseRoute.extend({
       members: [],
       group: this.modelFor("group"),
     });
-    controller.group.set("assignment_count", model.assignment_count);
+    controller.group.setProperties({
+      assignment_count: model.assignment_count,
+      group_assignment_count: model.group_assignment_count,
+    });
 
     controller.findMembers(true);
   },

assets/javascripts/discourse/templates/components/group-assigned-filter.hbs

@@ -1,4 +1,4 @@
-{{#if show-avatar}}
+{{#if showAvatar}}
   {{#link-to "group.assigned.show" filter.username_lower (query-params order=order ascending=ascending search=search)}}
     <div class="assign-image">
       <a href={{filter.userPath}} data-user-card={{filter.username}}>{{avatar filter imageSize="large"}}</a>
@@ -13,6 +13,19 @@
       {{filter.assignments_count}}
     </div>
   {{/link-to}}
+{{else if groupName}}
+  {{#link-to "group.assigned.show" filter (query-params order=order ascending=ascending search=search)}}
+    <div class="assign-image">
+      {{d-icon "group-plus"}}
+    </div>
+    <div class="assign-names">
+      <div class="assign-username">{{groupName}}</div>
+    </div>
+
+    <div class="assign-count">
+      {{assignmentCount}}
+    </div>
+  {{/link-to}}
 {{else}}
   {{#link-to "group.assigned.show" filter (query-params order=order ascending=ascending search=search)}}
     <div class="assign-everyone">

assets/javascripts/discourse/templates/group/assigned.hbs

@@ -13,7 +13,7 @@
     {{/if}}
     {{#load-more selector=".activity-nav li" action=(action "loadMore")}}
       {{group-assigned-filter
-        show-avatar=false
+        showAvatar=false
         filter="everyone"
         routeType=route_type
         assignmentCount=group.assignment_count
@@ -21,9 +21,19 @@
         ascending=ascending
         order=order
       }}
+      {{group-assigned-filter
+        showAvatar=false
+        groupName=group.name
+        filter=group.name
+        routeType=route_type
+        assignmentCount=group.group_assignment_count
+        search=search
+        ascending=ascending
+        order=order
+      }}
       {{#each members as |member|}}
         {{group-assigned-filter
-          show-avatar=true
+          showAvatar=true
           filter=member
           routeType=route_type
           search=search

assets/stylesheets/assigns.scss

@@ -112,6 +112,16 @@
     align-items: center;
     padding: 0.5em 13px;
 
+    .assign-image {
+      width: 45px;
+      margin-right: 0.5em;
+      text-align: center;
+      svg {
+        width: 40px;
+        height: 40px;
+      }
+    }
+
     .assign-names {
       font-size: $font-down-1;
       grid-area: names;

db/migrate/20210908060141_add_index_to_assigned_to_id_and_type.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AddIndexToAssignedToIdAndType < ActiveRecord::Migration[6.1]
+  def change
+    add_index :assignments, [:assigned_to_id, :assigned_to_type]
+  end
+end

plugin.rb

@@ -275,33 +275,47 @@ after_initialize do
   add_to_class(:topic_query, :list_messages_assigned) do |user|
     list = default_results(include_pms: true)
 
-    list = list.where(<<~SQL, user_id: user.id)
-      topics.id IN (
+    topic_ids_sql = +<<~SQL
       SELECT topic_id FROM assignments
       LEFT JOIN group_users ON group_users.user_id = :user_id
       WHERE
         assigned_to_id = :user_id AND assigned_to_type = 'User'
-          OR
-          assigned_to_id IN (group_users.group_id) AND assigned_to_type = 'Group'
-      )
     SQL
 
+    if @options[:filter] != :direct
+      topic_ids_sql << <<~SQL
+        OR assigned_to_id IN (group_users.group_id) AND assigned_to_type = 'Group'
+      SQL
+    end
+
+    sql = "topics.id IN (#{topic_ids_sql})"
+
+    list = list.where(sql, user_id: user.id)
+
     create_list(:assigned, { unordered: true }, list)
   end
 
   add_to_class(:topic_query, :list_group_topics_assigned) do |group|
     list = default_results(include_pms: true)
 
-    list = list.where(<<~SQL, group_id: group.id)
-      topics.id IN (
+    topic_ids_sql = +<<~SQL
       SELECT topic_id FROM assignments
       WHERE (
-          assigned_to_id IN (SELECT user_id from group_users where group_id = :group_id) AND assigned_to_type = 'User'
-        ) OR (
         assigned_to_id = :group_id AND assigned_to_type = 'Group'
       )
+    SQL
+
+    if @options[:filter] != :direct
+      topic_ids_sql << <<~SQL
+        OR (
+          assigned_to_id IN (SELECT user_id from group_users where group_id = :group_id) AND assigned_to_type = 'User'
         )
       SQL
+    end
+
+    sql = "topics.id IN (#{topic_ids_sql})"
+
+    list = list.where(sql, group_id: group.id)
 
     create_list(:assigned, { unordered: true }, list)
   end
@@ -337,6 +351,7 @@ after_initialize do
     raise Discourse::InvalidAccess unless current_user.can_assign?
 
     list_opts = build_topic_list_options
+    list_opts.merge!({ filter: :direct }) if params[:direct] == "true"
     list = generate_list_for("messages_assigned", user, list_opts)
 
     list.more_topics_url = construct_url_with(:next, list_opts)
@@ -354,6 +369,7 @@ after_initialize do
     raise Discourse::InvalidAccess unless group.can_show_assigned_tab?
 
     list_opts = build_topic_list_options
+    list_opts.merge!({ filter: :direct }) if params[:direct] == "true"
     list = generate_list_for("group_topics_assigned", group, list_opts)
 
     list.more_topics_url = construct_url_with(:next, list_opts)

spec/components/topic_query_spec.rb

@@ -8,8 +8,8 @@ describe TopicQuery do
     SiteSetting.assign_enabled = true
   end
 
-  let(:user) { Fabricate(:user) }
-  let(:user2) { Fabricate(:user) }
+  fab!(:user) { Fabricate(:user) }
+  fab!(:user2) { Fabricate(:user) }
 
   include_context 'A group that is allowed to assign'
 
@@ -19,56 +19,73 @@ describe TopicQuery do
   end
 
   describe '#list_messages_assigned' do
-    before do
-      @private_message = Fabricate(:private_message_topic, user: user)
-      @topic = Fabricate(:topic, user: user)
+    fab!(:private_message) { Fabricate(:private_message_topic, user: user) }
+    fab!(:topic) { Fabricate(:topic, user: user) }
+    fab!(:group_topic) { Fabricate(:topic, user: user) }
 
-      assign_to(@private_message, user)
-      assign_to(@topic, user)
+    before do
+      assign_to(private_message, user, user)
+      assign_to(topic, user, user)
+      assign_to(group_topic, user, assign_allowed_group)
     end
 
     it 'Includes topics and PMs assigned to user' do
       assigned_messages = TopicQuery.new(user, { page: 0 }).list_messages_assigned(user).topics
 
-      expect(assigned_messages).to contain_exactly(@private_message, @topic)
+      expect(assigned_messages).to contain_exactly(private_message, topic, group_topic)
     end
 
     it 'Excludes topics and PMs not assigned to user' do
       assigned_messages = TopicQuery.new(user2, { page: 0 }).list_messages_assigned(user2).topics
 
-      expect(assigned_messages).to be_empty
+      expect(assigned_messages).to contain_exactly(group_topic)
+    end
+
+    it 'direct filter excludes group assignment' do
+      assigned_messages = TopicQuery.new(user, { page: 0, filter: :direct }).list_messages_assigned(user).topics
+
+      expect(assigned_messages).to contain_exactly(private_message, topic)
     end
 
     it 'Returns the results ordered by the bumped_at field' do
-      @topic.update(bumped_at: 2.weeks.ago)
+      topic.update(bumped_at: 2.weeks.ago)
 
       assigned_messages = TopicQuery.new(user, { page: 0 }).list_messages_assigned(user).topics
 
-      expect(assigned_messages).to eq([@private_message, @topic])
+      expect(assigned_messages).to eq([group_topic, private_message, topic])
     end
   end
 
   describe '#list_group_topics_assigned' do
-    before do
-      @private_message = Fabricate(:private_message_topic, user: user)
-      @topic = Fabricate(:topic, user: user)
 
-      assign_to(@private_message, user)
-      assign_to(@topic, user2)
+    fab!(:private_message) { Fabricate(:private_message_topic, user: user) }
+    fab!(:topic) { Fabricate(:topic, user: user) }
+    fab!(:group_topic) { Fabricate(:topic, user: user) }
+
+    before do
+      assign_to(private_message, user, user)
+      assign_to(topic, user2, user2)
+      assign_to(group_topic, user, assign_allowed_group)
     end
 
     it 'Includes topics and PMs assigned to user' do
       assigned_messages = TopicQuery.new(user, { page: 0 }).list_group_topics_assigned(assign_allowed_group).topics
 
-      expect(assigned_messages).to contain_exactly(@private_message, @topic)
+      expect(assigned_messages).to contain_exactly(private_message, topic, group_topic)
     end
 
     it 'Returns the results ordered by the bumped_at field' do
-      @topic.update(bumped_at: 2.weeks.ago)
+      topic.update(bumped_at: 2.weeks.ago)
 
       assigned_messages = TopicQuery.new(user, { page: 0 }).list_group_topics_assigned(assign_allowed_group).topics
 
-      expect(assigned_messages).to eq([@private_message, @topic])
+      expect(assigned_messages).to eq([group_topic, private_message, topic])
+    end
+
+    it 'direct filter shows only group assignments' do
+      assigned_messages = TopicQuery.new(user, { page: 0, filter: :direct }).list_group_topics_assigned(assign_allowed_group).topics
+
+      expect(assigned_messages).to contain_exactly(group_topic)
     end
   end
 
@@ -92,7 +109,7 @@ describe TopicQuery do
           Fabricate.build(:topic_allowed_user, user: user2)
         ],
       )
-      assign_to(topic, user)
+      assign_to(topic, user, user)
     end
 
     let(:group2) { Fabricate(:group) }
@@ -106,7 +123,7 @@ describe TopicQuery do
         ],
       )
 
-      assign_to(topic, user)
+      assign_to(topic, user, user)
     end
 
     before do
@@ -178,7 +195,7 @@ describe TopicQuery do
 
     it "filters topics assigned to anybody (*)" do
       assigned_topic = Fabricate(:post).topic
-      unassigned_topic = Fabricate(:topic)
+      Fabricate(:topic)
 
       TopicAssigner.new(assigned_topic, user).assign(user)
       query = TopicQuery.new(user, assigned: "*").list_latest
@@ -188,10 +205,10 @@ describe TopicQuery do
     end
   end
 
-  def assign_to(topic, user)
+  def assign_to(topic, user, assignee)
     topic.tap do |t|
       t.posts << Fabricate(:post)
-      TopicAssigner.new(t, user).assign(user)
+      TopicAssigner.new(t, user).assign(assignee)
     end
   end
 end

test/javascripts/acceptance/assign-enabled-test.js.es6

@@ -19,6 +19,7 @@ acceptance("Discourse Assign | Assign mobile", function (needs) {
       return helper.response({
         success: true,
         assign_allowed_groups: false,
+        assign_allowed_for_groups: [],
         suggestions: [
           {
             id: 19,
@@ -60,6 +61,7 @@ acceptance("Discourse Assign | Assign desktop", function (needs) {
       return helper.response({
         success: true,
         assign_allowed_groups: false,
+        assign_allowed_for_groups: [],
         suggestions: [
           {
             id: 19,

test/javascripts/components/group-assigned-filter-test.js.es6

@@ -10,7 +10,7 @@ discourseModule(
     setupRenderingTest(hooks);
 
     componentTest("displays username and name", {
-      template: hbs`{{group-assigned-filter show-avatar=true filter=filter}}`,
+      template: hbs`{{group-assigned-filter showAvatar=true filter=filter}}`,
 
       beforeEach() {
         this.set("filter", {