From 62d87f0084001febe3612dcfba57f797fa07107f Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Thu, 9 Sep 2021 12:18:18 +1000 Subject: [PATCH] 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 --- .../discourse_assign/assign_controller.rb | 22 +++++-- .../controllers/group-assigned-show.js.es6 | 1 + .../routes/group-assigned-show.js.es6 | 9 ++- .../routes/group-assigned.js.es6 | 5 +- .../components/group-assigned-filter.hbs | 15 ++++- .../discourse/templates/group/assigned.hbs | 14 +++- assets/stylesheets/assigns.scss | 10 +++ ...41_add_index_to_assigned_to_id_and_type.rb | 7 ++ plugin.rb | 50 +++++++++----- spec/components/topic_query_spec.rb | 65 ++++++++++++------- .../acceptance/assign-enabled-test.js.es6 | 2 + .../group-assigned-filter-test.js.es6 | 2 +- 12 files changed, 146 insertions(+), 56 deletions(-) create mode 100644 db/migrate/20210908060141_add_index_to_assigned_to_id_and_type.rb diff --git a/app/controllers/discourse_assign/assign_controller.rb b/app/controllers/discourse_assign/assign_controller.rb index 7df479a..22c54e9 100644 --- a/app/controllers/discourse_assign/assign_controller.rb +++ b/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 diff --git a/assets/javascripts/discourse-assign/controllers/group-assigned-show.js.es6 b/assets/javascripts/discourse-assign/controllers/group-assigned-show.js.es6 index bdc5dfc..93d19b5 100644 --- a/assets/javascripts/discourse-assign/controllers/group-assigned-show.js.es6 +++ b/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)) diff --git a/assets/javascripts/discourse-assign/routes/group-assigned-show.js.es6 b/assets/javascripts/discourse-assign/routes/group-assigned-show.js.es6 index 7e6acde..c5f436c 100644 --- a/assets/javascripts/discourse-assign/routes/group-assigned-show.js.es6 +++ b/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", }, }); }, diff --git a/assets/javascripts/discourse-assign/routes/group-assigned.js.es6 b/assets/javascripts/discourse-assign/routes/group-assigned.js.es6 index 8d2e7b8..89f0ade 100644 --- a/assets/javascripts/discourse-assign/routes/group-assigned.js.es6 +++ b/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); }, diff --git a/assets/javascripts/discourse/templates/components/group-assigned-filter.hbs b/assets/javascripts/discourse/templates/components/group-assigned-filter.hbs index ccaa9cf..ad2dd4a 100644 --- a/assets/javascripts/discourse/templates/components/group-assigned-filter.hbs +++ b/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)}}
{{avatar filter imageSize="large"}} @@ -13,6 +13,19 @@ {{filter.assignments_count}}
{{/link-to}} +{{else if groupName}} + {{#link-to "group.assigned.show" filter (query-params order=order ascending=ascending search=search)}} +
+ {{d-icon "group-plus"}} +
+
+
{{groupName}}
+
+ +
+ {{assignmentCount}} +
+ {{/link-to}} {{else}} {{#link-to "group.assigned.show" filter (query-params order=order ascending=ascending search=search)}}
diff --git a/assets/javascripts/discourse/templates/group/assigned.hbs b/assets/javascripts/discourse/templates/group/assigned.hbs index 1ab424b..926d3ac 100644 --- a/assets/javascripts/discourse/templates/group/assigned.hbs +++ b/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 diff --git a/assets/stylesheets/assigns.scss b/assets/stylesheets/assigns.scss index 93e9bbd..6285792 100644 --- a/assets/stylesheets/assigns.scss +++ b/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; diff --git a/db/migrate/20210908060141_add_index_to_assigned_to_id_and_type.rb b/db/migrate/20210908060141_add_index_to_assigned_to_id_and_type.rb new file mode 100644 index 0000000..d11dfc8 --- /dev/null +++ b/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 diff --git a/plugin.rb b/plugin.rb index ebc3a51..c2c4dd9 100644 --- a/plugin.rb +++ b/plugin.rb @@ -275,34 +275,48 @@ 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 ( - 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' - ) + 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' 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 ( - 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' - ) + topic_ids_sql = +<<~SQL + SELECT topic_id FROM assignments + WHERE ( + 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) diff --git a/spec/components/topic_query_spec.rb b/spec/components/topic_query_spec.rb index b8aaba2..5cf0205 100644 --- a/spec/components/topic_query_spec.rb +++ b/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 diff --git a/test/javascripts/acceptance/assign-enabled-test.js.es6 b/test/javascripts/acceptance/assign-enabled-test.js.es6 index 930719e..0d7aaa1 100644 --- a/test/javascripts/acceptance/assign-enabled-test.js.es6 +++ b/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, diff --git a/test/javascripts/components/group-assigned-filter-test.js.es6 b/test/javascripts/components/group-assigned-filter-test.js.es6 index e6927df..8804903 100644 --- a/test/javascripts/components/group-assigned-filter-test.js.es6 +++ b/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", {