From 901cee55cdeefa64cb65efa9a02a81297e416051 Mon Sep 17 00:00:00 2001 From: Bianca Nenciu Date: Wed, 3 Feb 2021 16:11:08 +0200 Subject: [PATCH] FEATURE: Improve group settings and members management (#11878) This pull requests contains a series of improvements to groups settings and member management such as: - Showing which users have set a group as primary - Moving similar settings together under Effects - Adding bulk select and actions to members page --- .../app/components/group-member-dropdown.js | 86 +++++++++++++++- .../discourse/app/controllers/group-index.js | 99 +++++++++++++++++++ .../javascripts/discourse/app/models/group.js | 1 + .../javascripts/discourse/app/models/user.js | 7 ++ .../groups-form-membership-fields.hbs | 99 +++++++++++-------- .../components/groups-form-profile-fields.hbs | 18 ---- .../discourse/app/templates/group-index.hbs | 38 +++++-- .../tests/acceptance/group-index-test.js | 21 ++++ .../group-manage-membership-test.js | 5 + .../acceptance/group-manage-profile-test.js | 4 - app/assets/stylesheets/common/base/group.scss | 59 ++++++----- app/controllers/admin/groups_controller.rb | 26 ++++- config/locales/client.en.yml | 15 +++ config/routes.rb | 1 + spec/requests/admin/groups_controller_spec.rb | 47 +++++++++ 15 files changed, 425 insertions(+), 101 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/group-member-dropdown.js b/app/assets/javascripts/discourse/app/components/group-member-dropdown.js index c59dc84ca3e..8b74a485bc2 100644 --- a/app/assets/javascripts/discourse/app/components/group-member-dropdown.js +++ b/app/assets/javascripts/discourse/app/components/group-member-dropdown.js @@ -11,7 +11,56 @@ export default DropdownSelectBoxComponent.extend({ showFullTitle: false, }, - content: computed("member.owner", function () { + contentBulk() { + const items = []; + + items.push({ + id: "removeMembers", + name: I18n.t("groups.members.remove_members"), + description: I18n.t("groups.members.remove_members_description"), + icon: "user-times", + }); + + if (this.bulkSelection.some((m) => !m.owner)) { + items.push({ + id: "makeOwners", + name: I18n.t("groups.members.make_owners"), + description: I18n.t("groups.members.make_owners_description"), + icon: "shield-alt", + }); + } + + if (this.bulkSelection.some((m) => m.owner)) { + items.push({ + id: "removeOwners", + name: I18n.t("groups.members.remove_owners"), + description: I18n.t("groups.members.remove_owners_description"), + icon: "shield-alt", + }); + } + + if (this.bulkSelection.some((m) => !m.primary)) { + items.push({ + id: "setPrimary", + name: I18n.t("groups.members.make_all_primary"), + description: I18n.t("groups.members.make_all_primary_description"), + icon: "id-card", + }); + } + + if (this.bulkSelection.some((m) => m.primary)) { + items.push({ + id: "unsetPrimary", + name: I18n.t("groups.members.remove_all_primary"), + description: I18n.t("groups.members.remove_all_primary_description"), + icon: "id-card", + }); + } + + return items; + }, + + contentSingle() { const items = [ { id: "removeMember", @@ -45,6 +94,39 @@ export default DropdownSelectBoxComponent.extend({ } } + if (this.currentUser.staff) { + if (this.member.primary) { + items.push({ + id: "removePrimary", + name: I18n.t("groups.members.remove_primary"), + description: I18n.t("groups.members.remove_primary_description", { + username: this.get("member.username"), + }), + icon: "id-card", + }); + } else { + items.push({ + id: "makePrimary", + name: I18n.t("groups.members.make_primary"), + description: I18n.t("groups.members.make_primary_description", { + username: this.get("member.username"), + }), + icon: "id-card", + }); + } + } + return items; - }), + }, + + content: computed( + "bulkSelection.[]", + "member.owner", + "member.primary", + function () { + return this.bulkSelection !== undefined + ? this.contentBulk() + : this.contentSingle(); + } + ), }); diff --git a/app/assets/javascripts/discourse/app/controllers/group-index.js b/app/assets/javascripts/discourse/app/controllers/group-index.js index b11eb199c53..847fdcece2c 100644 --- a/app/assets/javascripts/discourse/app/controllers/group-index.js +++ b/app/assets/javascripts/discourse/app/controllers/group-index.js @@ -1,6 +1,7 @@ import Controller, { inject as controller } from "@ember/controller"; import discourseComputed, { observes } from "discourse-common/utils/decorators"; import { action } from "@ember/object"; +import { ajax } from "discourse/lib/ajax"; import discourseDebounce from "discourse-common/lib/debounce"; import { gt } from "@ember/object/computed"; import { popupAjaxError } from "discourse/lib/ajax-error"; @@ -16,8 +17,11 @@ export default Controller.extend({ filterInput: null, loading: false, + isBulk: false, showActions: false, + bulkSelection: null, + @observes("filterInput") _setFilter() { discourseDebounce( @@ -51,6 +55,10 @@ export default Controller.extend({ this.model.members.length >= this.model.user_count, loading: false, }); + + if (this.refresh) { + this.set("bulkSelection", []); + } }); }, @@ -97,6 +105,68 @@ export default Controller.extend({ case "removeOwner": this.removeOwner(member); break; + case "makePrimary": + member + .setPrimaryGroup(this.model.id) + .then(() => member.set("primary", true)); + break; + case "removePrimary": + member.setPrimaryGroup(null).then(() => member.set("primary", false)); + break; + } + }, + + @action + actOnSelection(selection, actionId) { + if (!selection || selection.length === 0) { + return; + } + + switch (actionId) { + case "removeMembers": + return ajax(`/groups/${this.model.id}/members.json`, { + type: "DELETE", + data: { user_ids: selection.map((u) => u.id).join(",") }, + }).then(() => { + this.model.findMembers(this.memberParams, true); + this.set("isBulk", false); + }); + + case "makeOwners": + return ajax(`/admin/groups/${this.model.id}/owners.json`, { + type: "PUT", + data: { + group: { usernames: selection.map((u) => u.username).join(",") }, + }, + }).then(() => { + selection.forEach((s) => s.set("owner", true)); + this.set("isBulk", false); + }); + + case "removeOwners": + return ajax(`/admin/groups/${this.model.id}/owners.json`, { + type: "DELETE", + data: { + group: { usernames: selection.map((u) => u.username).join(",") }, + }, + }).then(() => { + selection.forEach((s) => s.set("owner", false)); + this.set("isBulk", false); + }); + + case "setPrimary": + case "unsetPrimary": + const primary = actionId === "setPrimary"; + return ajax(`/admin/groups/${this.model.id}/primary.json`, { + type: "PUT", + data: { + group: { usernames: selection.map((u) => u.username).join(",") }, + primary, + }, + }).then(() => { + selection.forEach((s) => s.set("primary", primary)); + this.set("isBulk", false); + }); } }, @@ -124,4 +194,33 @@ export default Controller.extend({ .catch(popupAjaxError); } }, + + @action + toggleBulkSelect() { + this.setProperties({ + isBulk: !this.isBulk, + bulkSelection: [], + }); + }, + + @action + bulkSelectAll() { + $("input.bulk-select:not(:checked)").click(); + }, + + @action + bulkClearAll() { + $("input.bulk-select:checked").click(); + }, + + @action + selectMember(member, e) { + this.set("bulkSelection", this.bulkSelection || []); + + if (e.target.checked) { + this.bulkSelection.pushObject(member); + } else { + this.bulkSelection.removeObject(member); + } + }, }); diff --git a/app/assets/javascripts/discourse/app/models/group.js b/app/assets/javascripts/discourse/app/models/group.js index 1afcac983ce..429dcd05953 100644 --- a/app/assets/javascripts/discourse/app/models/group.js +++ b/app/assets/javascripts/discourse/app/models/group.js @@ -56,6 +56,7 @@ const Group = RestModel.extend({ members.pushObjects( result.members.map((member) => { member.owner = ownerIds.has(member.id); + member.primary = member.primary_group_name === this.name; return User.create(member); }) ); diff --git a/app/assets/javascripts/discourse/app/models/user.js b/app/assets/javascripts/discourse/app/models/user.js index 61f34556f66..1077161b9b9 100644 --- a/app/assets/javascripts/discourse/app/models/user.js +++ b/app/assets/javascripts/discourse/app/models/user.js @@ -960,6 +960,13 @@ const User = RestModel.extend({ } }, + setPrimaryGroup(primaryGroupId) { + return ajax(`/admin/users/${this.id}/primary_group`, { + type: "PUT", + data: { primary_group_id: primaryGroupId }, + }); + }, + enterDoNotDisturbFor(duration) { return ajax({ url: "/do-not-disturb.json", diff --git a/app/assets/javascripts/discourse/app/templates/components/groups-form-membership-fields.hbs b/app/assets/javascripts/discourse/app/templates/components/groups-form-membership-fields.hbs index 03a6813bf2c..181d0abc76b 100644 --- a/app/assets/javascripts/discourse/app/templates/components/groups-form-membership-fields.hbs +++ b/app/assets/javascripts/discourse/app/templates/components/groups-form-membership-fields.hbs @@ -1,3 +1,45 @@ +
+ + + + + + + + + {{#if model.allow_membership_requests}} +
+ + + {{expanding-text-area name="membership-request-template" + class="group-form-membership-request-template input-xxlarge" + value=model.membership_request_template}} +
+ {{/if}} +
+ {{#if model.can_admin_group}}
@@ -41,48 +83,21 @@ {{i18n "admin.groups.manage.membership.primary_group"}} +
+
+ + + {{input value=model.title name="title" class="input-xxlarge"}} + +
+ {{i18n "admin.groups.default_title_description"}} +
+
+ +
+ {{group-flair-inputs model=model}}
{{/if}} - -
- - - - - - - - - {{#if model.allow_membership_requests}} -
- - - {{expanding-text-area name="membership-request-template" - class="group-form-membership-request-template input-xxlarge" - value=model.membership_request_template}} -
- {{/if}} -
diff --git a/app/assets/javascripts/discourse/app/templates/components/groups-form-profile-fields.hbs b/app/assets/javascripts/discourse/app/templates/components/groups-form-profile-fields.hbs index d9590bbb217..5be10c58309 100644 --- a/app/assets/javascripts/discourse/app/templates/components/groups-form-profile-fields.hbs +++ b/app/assets/javascripts/discourse/app/templates/components/groups-form-profile-fields.hbs @@ -19,20 +19,6 @@ class="input-xxlarge group-form-full-name" value=model.full_name}} - - {{#if this.currentUser.can_create_group}} -
- - - {{input value=model.title name="title" class="input-xxlarge"}} - -
- {{i18n "admin.groups.default_title_description"}} -
-
- {{/if}} {{/if}}
@@ -43,9 +29,5 @@ {{#if canEdit}} {{yield}} -
- {{group-flair-inputs model=model}} -
- {{plugin-outlet name="group-edit" args=(hash group=model)}} {{/if}} diff --git a/app/assets/javascripts/discourse/app/templates/group-index.hbs b/app/assets/javascripts/discourse/app/templates/group-index.hbs index 973f0aa4ce0..b53bdc3fc4d 100644 --- a/app/assets/javascripts/discourse/app/templates/group-index.hbs +++ b/app/assets/javascripts/discourse/app/templates/group-index.hbs @@ -23,26 +23,50 @@ {{#load-more selector=".group-members tr" action=(action "loadMore")}} - {{table-header-toggle order=order asc=asc field="username_lower" labelKey="username"}} - + + {{#if isBulk}} + + {{/if}} + {{table-header-toggle order=order asc=asc field="username_lower" labelKey="username" class="username"}} + {{table-header-toggle order=order asc=asc field="added_at" labelKey="groups.member_added"}} {{table-header-toggle order=order asc=asc field="last_posted_at" labelKey="last_post"}} {{table-header-toggle order=order asc=asc field="last_seen_at" labelKey="last_seen"}} - + {{#each model.members as |m|}} - + {{/if}} + +
{{i18n "groups.members.owner"}} + {{flat-button class="bulk-select" icon="list" action=(action "toggleBulkSelect") title="topics.bulk.toggle"}} + + {{d-button action=(action "bulkSelectAll") label="topics.bulk.select_all"}} + {{d-button action=(action "bulkClearAll") label="topics.bulk.clear_all"}} + + {{#if isBulk}} + {{group-member-dropdown + bulkSelection=bulkSelection + canAdminGroup=model.can_admin_group + onChange=(action "actOnSelection" bulkSelection) + }} + {{/if}} +
+ {{#if isBulk}} + + {{input type="checkbox" class="bulk-select" click=(action "selectMember" m)}} + {{user-info user=m skipName=skipName}} {{#if m.owner}} - - {{d-icon "shield-alt"}} - + {{d-icon "shield-alt"}} {{i18n "groups.members.owner"}}
+ {{/if}} + {{#if m.primary}} + {{i18n "groups.members.primary"}} {{/if}} 		diff --git a/app/assets/javascripts/discourse/tests/acceptance/group-index-test.js b/app/assets/javascripts/discourse/tests/acceptance/group-index-test.js index 524af365e59..1c2872c52f3 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/group-index-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/group-index-test.js @@ -6,6 +6,7 @@ import { } from "discourse/tests/helpers/qunit-helpers"; import { click, visit } from "@ember/test-helpers"; import I18n from "I18n"; +import selectKit from "discourse/tests/helpers/select-kit-helper"; import { test } from "qunit"; acceptance("Group Members - Anonymous", function () { @@ -34,6 +35,12 @@ acceptance("Group Members - Anonymous", function () { acceptance("Group Members", function (needs) { needs.user(); + needs.pretender((server, helper) => { + server.put("/admin/groups/47/owners.json", () => { + return helper.response({ success: true }); + }); + }); + test("Viewing Members as a group owner", async function (assert) { updateCurrentUser({ moderator: false, admin: false }); @@ -61,4 +68,18 @@ acceptance("Group Members", function (needs) { "it should display the right filter placehodler" ); }); + + test("Shows bulk actions", async function (assert) { + await visit("/g/discourse"); + + assert.ok(count("button.bulk-select") > 0); + await click("button.bulk-select"); + + await click("input.bulk-select:nth(0)"); + await click("input.bulk-select:nth(1)"); + + const memberDropdown = selectKit(".group-member-dropdown:first"); + await memberDropdown.expand(); + await memberDropdown.selectRowByValue("makeOwners"); + }); }); diff --git a/app/assets/javascripts/discourse/tests/acceptance/group-manage-membership-test.js b/app/assets/javascripts/discourse/tests/acceptance/group-manage-membership-test.js index 5a955e12c92..afef5e560c2 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/group-manage-membership-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/group-manage-membership-test.js @@ -50,6 +50,11 @@ acceptance("Managing Group Membership", function (needs) { "it should disable group allow_membership_request input" ); + assert.ok( + queryAll(".group-flair-inputs").length === 1, + "it should display avatar flair inputs" + ); + await click(".group-form-public-admission"); await click(".group-form-allow-membership-requests"); diff --git a/app/assets/javascripts/discourse/tests/acceptance/group-manage-profile-test.js b/app/assets/javascripts/discourse/tests/acceptance/group-manage-profile-test.js index 67785934d18..7a0ced08025 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/group-manage-profile-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/group-manage-profile-test.js @@ -24,10 +24,6 @@ acceptance("Managing Group Profile", function (needs) { test("As an admin", async function (assert) { await visit("/g/discourse/manage/profile"); - assert.ok( - queryAll(".group-flair-inputs").length === 1, - "it should display avatar flair inputs" - ); assert.ok( queryAll(".group-form-bio").length === 1, "it should display group bio input" diff --git a/app/assets/stylesheets/common/base/group.scss b/app/assets/stylesheets/common/base/group.scss index 3ecab3780be..cf5750b2676 100644 --- a/app/assets/stylesheets/common/base/group.scss +++ b/app/assets/stylesheets/common/base/group.scss @@ -107,39 +107,48 @@ table.group-members { width: 100%; table-layout: fixed; - th:first-child { - width: 30%; - text-align: left; - } - - th:last-child { - width: 5%; - } - - th.group-members-actions { - width: 5%; - } - th { text-align: center; + + &.bulk-select { + height: 30px; + width: 30px; + } + + &.bulk-select-buttons { + text-align: left; + width: 20%; + } + + &.username { + text-align: left; + width: 30%; + } + + &.bulk-select-buttons + .username { + width: 10%; + } + + &.group-members-actions { + width: 5%; + } } - tr { - .user-info { - display: block; - .avatar-flair { - color: var(--primary); - } - } + td { + color: var(--primary-medium); + padding: 0.8em 0; + text-align: center; - td:first-child { + &.avatar { text-align: left; } + } - td { - text-align: center; - color: var(--primary-medium); - padding: 0.8em 0; + .user-info { + display: block; + + .avatar-flair { + color: var(--primary); } } } diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index e3bae016792..eba7042ba10 100644 --- a/app/controllers/admin/groups_controller.rb +++ b/app/controllers/admin/groups_controller.rb @@ -125,15 +125,35 @@ class Admin::GroupsController < Admin::AdminController return can_not_modify_automatic if group.automatic guardian.ensure_can_edit_group!(group) - user = User.find(params[:user_id].to_i) - group.group_users.where(user_id: user.id).update_all(owner: false) - GroupActionLogger.new(current_user, group).log_remove_user_as_group_owner(user) + if params[:user_id].present? + users = [User.find_by(id: params[:user_id].to_i)] + elsif usernames = group_params[:usernames].presence + users = User.where(username: usernames.split(",")) + else + raise Discourse::InvalidParameters.new(:user_id) + end + + users.each do |user| + group.group_users.where(user_id: user.id).update_all(owner: false) + GroupActionLogger.new(current_user, group).log_remove_user_as_group_owner(user) + end Group.reset_counters(group.id, :group_users) render json: success_json end + def set_primary + group = Group.find_by(id: params.require(:id)) + raise Discourse::NotFound unless group + + users = User.where(username: group_params[:usernames].split(",")) + users.each { |user| guardian.ensure_can_change_primary_group!(user) } + users.update_all(primary_group_id: params[:primary] == "true" ? group.id : nil) + + render json: success_json + end + def automatic_membership_count domains = Group.get_valid_email_domains(params.require(:automatic_membership_email_domains)) group_id = params[:id] diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index a514203f011..6b4b3cfed7e 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -785,7 +785,22 @@ en: make_owner_description: "Make %{username} an owner of this group" remove_owner: "Remove as Owner" remove_owner_description: "Remove %{username} as an owner of this group" + make_primary: "Make Primary" + make_primary_description: "Make this the primary group for %{username}" + remove_primary: "Remove as Primary" + remove_primary_description: "Remove this as the primary group for %{username}" + remove_members: "Remove Members" + remove_members_description: "Remove selected users from this group" + make_owners: "Make Owners" + make_owners_description: "Make selected users owners of this group" + remove_owners: "Remove Owners" + remove_owners_description: "Remove selected users as owners of this group" + make_all_primary: "Make All Primary" + make_all_primary_description: "Make this the primary group for all selected users" + remove_all_primary: "Remove as Primary" + remove_all_primary_description: "Remove this group as primary" owner: "Owner" + primary: "Primary" forbidden: "You're not allowed to view the members." topics: "Topics" posts: "Posts" diff --git a/config/routes.rb b/config/routes.rb index 85c40920445..dd704accb53 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -95,6 +95,7 @@ Discourse::Application.routes.draw do member do put "owners" => "groups#add_owners" delete "owners" => "groups#remove_owner" + put "primary" => "groups#set_primary" end end resources :groups, except: [:create], constraints: AdminConstraint.new do diff --git a/spec/requests/admin/groups_controller_spec.rb b/spec/requests/admin/groups_controller_spec.rb index 2181cb38370..03a606e2367 100644 --- a/spec/requests/admin/groups_controller_spec.rb +++ b/spec/requests/admin/groups_controller_spec.rb @@ -157,6 +157,9 @@ RSpec.describe Admin::GroupsController do end describe '#remove_owner' do + let(:user2) { Fabricate(:user) } + let(:user3) { Fabricate(:user) } + it 'should work' do group.add_owner(user) @@ -168,6 +171,20 @@ RSpec.describe Admin::GroupsController do expect(group.group_users.where(owner: true)).to eq([]) end + it 'should work with multiple users' do + group.add_owner(user) + group.add_owner(user3) + + delete "/admin/groups/#{group.id}/owners.json", params: { + group: { + usernames: "#{user.username},#{user2.username},#{user3.username}" + } + } + + expect(response.status).to eq(200) + expect(group.group_users.where(owner: true)).to eq([]) + end + it 'returns not-found error when there is no group' do group.destroy! @@ -190,6 +207,36 @@ RSpec.describe Admin::GroupsController do end end + describe "#set_primary" do + let(:user2) { Fabricate(:user) } + let(:user3) { Fabricate(:user) } + + it 'sets with multiple users' do + user2.update!(primary_group_id: group.id) + + put "/admin/groups/#{group.id}/primary.json", params: { + group: { usernames: "#{user.username},#{user2.username},#{user3.username}" }, + primary: "true" + } + + expect(response.status).to eq(200) + expect(User.where(primary_group_id: group.id).size).to eq(3) + end + + it 'unsets with multiple users' do + user.update!(primary_group_id: group.id) + user3.update!(primary_group_id: group.id) + + put "/admin/groups/#{group.id}/primary.json", params: { + group: { usernames: "#{user.username},#{user2.username},#{user3.username}" }, + primary: "false" + } + + expect(response.status).to eq(200) + expect(User.where(primary_group_id: group.id).size).to eq(0) + end + end + describe "#bulk_perform" do fab!(:group) do Fabricate(:group,