From 15bcfcd1825b39c6980eb9c62335b96b32d6e1e4 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Tue, 20 Mar 2018 15:50:46 +0800 Subject: [PATCH] UX: Allow users to filter by different group types on groups page. --- .../discourse/controllers/groups.js.es6 | 17 ++++- .../discourse/routes/groups.js.es6 | 27 ++++--- .../discourse/templates/groups.hbs | 10 ++- .../stylesheets/common/base/groups.scss | 7 +- app/assets/stylesheets/desktop/group.scss | 1 - app/assets/stylesheets/mobile/groups.scss | 2 +- app/controllers/groups_controller.rb | 35 ++++++++- app/models/group.rb | 10 +++ config/locales/client.en.yml | 9 ++- spec/requests/groups_controller_spec.rb | 74 ++++++++++++++++++- 10 files changed, 167 insertions(+), 25 deletions(-) diff --git a/app/assets/javascripts/discourse/controllers/groups.js.es6 b/app/assets/javascripts/discourse/controllers/groups.js.es6 index 4009ba91afb..ab876339ae3 100644 --- a/app/assets/javascripts/discourse/controllers/groups.js.es6 +++ b/app/assets/javascripts/discourse/controllers/groups.js.es6 @@ -1,13 +1,24 @@ -import { observes } from 'ember-addons/ember-computed-decorators'; import debounce from 'discourse/lib/debounce'; +import { default as computed, observes } from 'ember-addons/ember-computed-decorators'; export default Ember.Controller.extend({ application: Ember.inject.controller(), - queryParams: ["order", "asc", "filter"], + queryParams: ["order", "asc", "filter", "type"], order: null, asc: null, filter: "", - filterInput: "", + type: "", + + @computed("model.extras.type_filters") + types(typeFilters) { + const types = []; + + typeFilters.forEach(type => { + types.push({ id: type, name: I18n.t(`groups.index.${type}_groups`) }); + }); + + return types; + }, @observes("filterInput") _setFilter: debounce(function() { diff --git a/app/assets/javascripts/discourse/routes/groups.js.es6 b/app/assets/javascripts/discourse/routes/groups.js.es6 index fa6cb85b365..55681ff90bd 100644 --- a/app/assets/javascripts/discourse/routes/groups.js.es6 +++ b/app/assets/javascripts/discourse/routes/groups.js.es6 @@ -1,21 +1,26 @@ export default Discourse.Route.extend({ - queryParams: { - order: { refreshModel: true, replace: true }, - asc: { refreshModel: true, replace: true }, - filter: { refreshModel: true } - }, - - refreshQueryWithoutTransition: true, - titleToken() { return I18n.t('groups.index.title'); }, + queryParams: { + order: { refreshModel: true, replace: true }, + asc: { refreshModel: true, replace: true }, + filter: { refreshModel: true }, + type: { refreshModel: true, replace: true }, + }, + + refreshQueryWithoutTransition: true, + model(params) { - return this.store.findAll('group', params); + this._params = params; + return this.store.findAll("group", params); }, setupController(controller, model) { - controller.set('model', model); - } + controller.setProperties({ + model, + filterInput: this._params.filter + }); + }, }); diff --git a/app/assets/javascripts/discourse/templates/groups.hbs b/app/assets/javascripts/discourse/templates/groups.hbs index 1bf0ddce3b2..7cc273fe78f 100644 --- a/app/assets/javascripts/discourse/templates/groups.hbs +++ b/app/assets/javascripts/discourse/templates/groups.hbs @@ -3,7 +3,13 @@ {{text-field value=filterInput placeholderKey="groups.filter_name" - class="group-filter-name no-blur"}} + class="groups-name-filter no-blur"}} + + {{combo-box value=type + content=types + clearable=true + none="groups.index.all_groups" + class="groups-type-filter"}} {{#if model}} {{#conditional-loading-spinner condition=model.loading}} @@ -56,7 +62,7 @@ showLogin='showLogin'}} {{d-button icon="ban" - label=(if group.automatic 'groups.automatic_group' 'groups.closed_group') + label=(if group.automatic 'groups.automatic_group' 'groups.close_group') disabled=true}} {{/group-membership-button}} diff --git a/app/assets/stylesheets/common/base/groups.scss b/app/assets/stylesheets/common/base/groups.scss index c9580a23110..0ac9ce93d38 100644 --- a/app/assets/stylesheets/common/base/groups.scss +++ b/app/assets/stylesheets/common/base/groups.scss @@ -5,8 +5,7 @@ } } -.group-filter-name { - display: inline-block; +.groups-name-filter, .groups-type-filter { float: right; } @@ -34,6 +33,10 @@ padding: 0.8em; } + td.groups-info { + width: 50%; + } + td.group-user-status { i { color: $primary; diff --git a/app/assets/stylesheets/desktop/group.scss b/app/assets/stylesheets/desktop/group.scss index a2d9b2f0cf2..e279256e5c3 100644 --- a/app/assets/stylesheets/desktop/group.scss +++ b/app/assets/stylesheets/desktop/group.scss @@ -1,4 +1,3 @@ - .group-nav { li { float: left; diff --git a/app/assets/stylesheets/mobile/groups.scss b/app/assets/stylesheets/mobile/groups.scss index 27f39ed92ea..1f733f425fd 100644 --- a/app/assets/stylesheets/mobile/groups.scss +++ b/app/assets/stylesheets/mobile/groups.scss @@ -3,7 +3,7 @@ margin-top: 20px; } - .group-filter-name { + .groups-name-filter { margin-top: 20px; } } diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 4e38158bee2..ae8cb37030d 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -13,6 +13,29 @@ class GroupsController < ApplicationController skip_before_action :preload_json, :check_xhr, only: [:posts_feed, :mentions_feed] skip_before_action :check_xhr, only: [:show] + TYPE_FILTERS = { + my: Proc.new { |groups, current_user| + raise Discourse::NotFound unless current_user + Group.member_of(groups, current_user) + }, + owner: Proc.new { |groups, current_user| + raise Discourse::NotFound unless current_user + Group.owner_of(groups, current_user) + }, + public: Proc.new { |groups| + groups.where(public_admission: true, automatic: false) + }, + close: Proc.new { |groups| + groups.where( + public_admission: false, + automatic: false + ) + }, + automatic: Proc.new { |groups| + groups.where(automatic: true) + } + } + def index unless SiteSetting.enable_group_directory? raise Discourse::InvalidAccess.new(:enable_group_directory) @@ -28,9 +51,12 @@ class GroupsController < ApplicationController groups = Group.search_groups(filter, groups: groups) end + type_filters = TYPE_FILTERS.keys + unless guardian.is_staff? # hide automatic groups from all non stuff to de-clutter page groups = groups.where(automatic: false) + type_filters.delete(:automatic) end count = groups.count @@ -40,6 +66,10 @@ class GroupsController < ApplicationController Group.preload_custom_fields(groups, Group.preloaded_custom_field_names) end + if type = params[:type]&.to_sym + groups = TYPE_FILTERS[type].call(groups, current_user) + end + if current_user group_users = GroupUser.where(group: groups, user: current_user) user_group_ids = group_users.pluck(:group_id) @@ -52,8 +82,11 @@ class GroupsController < ApplicationController user_group_ids: user_group_ids || [], owner_group_ids: owner_group_ids || [] ), + extras: { + type_filters: current_user ? type_filters : type_filters - [:my, :owner] + }, total_rows_groups: count, - load_more_groups: groups_path(page: page + 1) + load_more_groups: groups_path(page: page + 1, type: type), ) end diff --git a/app/models/group.rb b/app/models/group.rb index 5888e0e6c51..90bb845f63f 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -561,6 +561,16 @@ class Group < ActiveRecord::Base STAFF_GROUPS.include?(self.name.to_sym) end + def self.member_of(groups, user) + groups.joins( + "LEFT JOIN group_users gu ON gu.group_id = groups.id + ").where("gu.user_id = ?", user.id) + end + + def self.owner_of(groups, user) + self.member_of(groups, user).where("gu.owner") + end + protected def name_format_validator diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 31806ea65cd..7630e5088db 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -435,7 +435,7 @@ en: filter_name: "filter by group name" message: "Message" automatic_group: Automatic Group - closed_group: Closed Group + close_group: Close Group is_group_user: "You are a member of this group" is_group_owner: "You are an owner of this group" allow_membership_requests: "Allow users to send membership requests to group owners" @@ -454,6 +454,13 @@ en: index: title: "Groups" empty: "There are no visible groups." + all_groups: "All Groups" + owner_groups: "Groups I am an owner of" + close_groups: "Close Groups" + public_groups: "Public Groups" + automatic_groups: "Automatic Groups" + public_groups: "Public Groups" + my_groups: "My Groups" title: one: "Group" other: "Groups" diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index b30e1f519c4..30246d76d3e 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -5,7 +5,7 @@ describe GroupsController do let(:group) { Fabricate(:group, users: [user]) } describe '#index' do - let!(:staff_group) do + let(:staff_group) do Fabricate(:group, name: '0000', visibility_level: Group.visibility_levels[:staff]) end @@ -69,6 +69,7 @@ describe GroupsController do it 'should return the right response' do group + staff_group get "/groups.json" expect(response).to be_success @@ -81,15 +82,23 @@ describe GroupsController do expect(group_ids).to_not include(staff_group.id) expect(response_body["load_more_groups"]).to eq("/groups?page=1") expect(response_body["total_rows_groups"]).to eq(1) + + expect(response_body["extras"]["type_filters"].map(&:to_sym)).to eq( + described_class::TYPE_FILTERS.keys - [:my, :owner, :automatic] + ) end context 'viewing as an admin' do - it 'should display automatic groups' do - admin = Fabricate(:admin) + let(:admin) { Fabricate(:admin) } + + before do sign_in(admin) group.add(admin) group.add_owner(admin) + end + it 'should return the right response' do + staff_group get "/groups.json" expect(response).to be_success @@ -104,6 +113,65 @@ describe GroupsController do expect(group_ids).to include(group.id, staff_group.id) expect(response_body["load_more_groups"]).to eq("/groups?page=1") expect(response_body["total_rows_groups"]).to eq(10) + + expect(response_body["extras"]["type_filters"].map(&:to_sym)).to eq( + described_class::TYPE_FILTERS.keys + ) + end + + context 'filterable by type' do + def expect_type_to_return_right_groups(type, expected_group_ids) + get "/groups.json", params: { type: type } + + expect(response.status).to eq(200) + + response_body = JSON.parse(response.body) + group_ids = response_body["groups"].map { |g| g["id"] } + + expect(group_ids).to contain_exactly(*expected_group_ids) + end + + describe 'my groups' do + it 'should return the right response' do + expect_type_to_return_right_groups('my', [group.id]) + end + end + + describe 'owner groups' do + it 'should return the right response' do + group2 = Fabricate(:group) + group3 = Fabricate(:group) + group2.add_owner(admin) + + expect_type_to_return_right_groups('owner', [group.id, group2.id]) + end + end + + describe 'automatic groups' do + it 'should return the right response' do + expect_type_to_return_right_groups( + 'automatic', + Group::AUTO_GROUP_IDS.keys - [0] + ) + end + end + + describe 'public groups' do + it 'should return the right response' do + group2 = Fabricate(:group, public_admission: true) + + expect_type_to_return_right_groups('public', [group2.id]) + end + end + + describe 'close groups' do + it 'should return the right response' do + group2 = Fabricate(:group, public_admission: false) + group3 = Fabricate(:group, public_admission: true) + + expect_type_to_return_right_groups('close', [group.id, group2.id]) + end + end end end end