From f3b402ffd56af1a5b1bb23e271d45db26f599785 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Thu, 22 Mar 2018 13:42:46 +0800 Subject: [PATCH] UX: Allow users to filter members on group page. * Only admins are allowed to filter users by email. --- .../discourse/controllers/group-index.js.es6 | 33 ++++++++++--- .../javascripts/discourse/models/group.js.es6 | 6 +-- .../discourse/routes/group-index.js.es6 | 10 +++- .../discourse/templates/group-index.hbs | 4 ++ app/controllers/groups_controller.rb | 12 ++++- app/models/user.rb | 19 ++++++++ config/locales/client.en.yml | 2 + lib/admin_user_index_query.rb | 14 +----- spec/models/user_spec.rb | 37 +++++++++++++++ spec/requests/groups_controller_spec.rb | 47 +++++++++++++++++++ .../acceptance/group-index-test.js.es6 | 12 +++++ 11 files changed, 171 insertions(+), 25 deletions(-) diff --git a/app/assets/javascripts/discourse/controllers/group-index.js.es6 b/app/assets/javascripts/discourse/controllers/group-index.js.es6 index 85155d8a318..83c6b9f3e88 100644 --- a/app/assets/javascripts/discourse/controllers/group-index.js.es6 +++ b/app/assets/javascripts/discourse/controllers/group-index.js.es6 @@ -1,9 +1,10 @@ import { popupAjaxError } from 'discourse/lib/ajax-error'; import Group from 'discourse/models/group'; import { default as computed, observes } from 'ember-addons/ember-computed-decorators'; +import debounce from 'discourse/lib/debounce'; export default Ember.Controller.extend({ - queryParams: ['order', 'desc'], + queryParams: ['order', 'desc', 'filter'], order: '', desc: null, loading: false, @@ -11,15 +12,26 @@ export default Ember.Controller.extend({ offset: null, isOwner: Ember.computed.alias('model.is_group_owner'), showActions: false, + filter: null, + filterInput: null, - @observes('order', 'desc') + @observes("filterInput") + _setFilter: debounce(function() { + this.set("filter", this.get("filterInput")); + }, 500), + + @observes('order', 'desc', 'filter') refreshMembers() { this.set('loading', true); + const model = this.get('model'); - this.get('model') && - this.get('model') - .findMembers({ order: this.get('order'), desc: this.get('desc') }) - .finally(() => this.set('loading', false)); + if (model) { + model.findMembers({ + order: this.get('order'), + desc: this.get('desc'), + filter: this.get('filter'), + }).finally(() => this.set('loading', false)); + } }, @computed('model.members') @@ -32,6 +44,15 @@ export default Ember.Controller.extend({ return this.currentUser && this.currentUser.canManageGroup(model); }, + @computed + filterPlaceholder() { + if (this.currentUser && this.currentUser.admin) { + return "groups.members.filter_placeholder_admin"; + } else { + return "groups.members.filter_placeholder"; + } + }, + actions: { toggleActions() { this.toggleProperty("showActions"); diff --git a/app/assets/javascripts/discourse/models/group.js.es6 b/app/assets/javascripts/discourse/models/group.js.es6 index b1780826591..34b5e752314 100644 --- a/app/assets/javascripts/discourse/models/group.js.es6 +++ b/app/assets/javascripts/discourse/models/group.js.es6 @@ -33,13 +33,13 @@ const Group = RestModel.extend({ findMembers(params) { if (Em.isEmpty(this.get('name'))) { return ; } - const self = this, offset = Math.min(this.get("user_count"), Math.max(this.get("offset"), 0)); + const offset = Math.min(this.get("user_count"), Math.max(this.get("offset"), 0)); - return Group.loadMembers(this.get("name"), offset, this.get("limit"), params).then(function (result) { + return Group.loadMembers(this.get("name"), offset, this.get("limit"), params).then(result => { var ownerIds = {}; result.owners.forEach(owner => ownerIds[owner.id] = true); - self.setProperties({ + this.setProperties({ user_count: result.meta.total, limit: result.meta.limit, offset: result.meta.offset, diff --git a/app/assets/javascripts/discourse/routes/group-index.js.es6 b/app/assets/javascripts/discourse/routes/group-index.js.es6 index faac0525b0e..4d37501f302 100644 --- a/app/assets/javascripts/discourse/routes/group-index.js.es6 +++ b/app/assets/javascripts/discourse/routes/group-index.js.es6 @@ -3,13 +3,19 @@ export default Discourse.Route.extend({ return I18n.t('groups.members.title'); }, - model() { + model(params) { + this._params = params; return this.modelFor("group"); }, setupController(controller, model) { this.controllerFor("group").set("showing", "members"); - controller.set("model", model); + + controller.setProperties({ + model, + filterInput: this._params.filter + }); + controller.refreshMembers(); } }); diff --git a/app/assets/javascripts/discourse/templates/group-index.hbs b/app/assets/javascripts/discourse/templates/group-index.hbs index 59cba2ab528..e3d578ac962 100644 --- a/app/assets/javascripts/discourse/templates/group-index.hbs +++ b/app/assets/javascripts/discourse/templates/group-index.hbs @@ -1,3 +1,7 @@ +{{text-field value=filterInput + placeholderKey=filterPlaceholder + class="group-username-filter no-blur"}} + {{#if hasMembers}} {{#load-more selector=".group-members tr" action="loadMore"}} diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index df2b3f82b1c..70d219afc09 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -186,8 +186,8 @@ class GroupsController < ApplicationController end users = group.users.human_users - total = users.count + members = users .order('NOT group_users.owner') .order(order) @@ -200,6 +200,16 @@ class GroupsController < ApplicationController .order(username_lower: dir) .where('group_users.owner') + if (filter = params[:filter]).present? + if current_user&.admin + owners = owners.filter_by_username_or_email(filter) + members = members.filter_by_username_or_email(filter) + else + owners = owners.filter_by_username(filter) + members = members.filter_by_username(filter) + end + end + render json: { members: serialize_data(members, GroupUserSerializer), owners: serialize_data(owners, GroupUserSerializer), diff --git a/app/models/user.rb b/app/models/user.rb index a068355fe8a..9cbb5eaae69 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -159,6 +159,25 @@ class User < ActiveRecord::Base scope :not_suspended, -> { where('suspended_till IS NULL OR suspended_till <= ?', Time.zone.now) } scope :activated, -> { where(active: true) } + scope :filter_by_username, ->(filter) do + where('username_lower ILIKE ?', filter) + end + + scope :filter_by_username_or_email, ->(filter) do + if filter =~ /.+@.+/ + # probably an email so try the bypass + if user_id = UserEmail.where("lower(email) = ?", filter.downcase).pluck(:user_id).first + return where('users.id = ?', user_id) + end + end + + joins(:primary_email) + .where( + 'username_lower ILIKE :filter OR lower(user_emails.email) ILIKE :filter', + filter: "%#{filter}%" + ) + end + module NewTopicDuration ALWAYS = -1 LAST_VISIT = -2 diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 0a60ca08d5a..a91ab5cdc8e 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -466,6 +466,8 @@ en: activity: "Activity" members: title: "Members" + filter_placeholder_admin: "username or email" + filter_placeholder: "username" remove_member: "Remove Member" remove_member_description: "Remove %{username} from this group" make_owner: "Make Owner" diff --git a/lib/admin_user_index_query.rb b/lib/admin_user_index_query.rb index ed2a0b5f55f..339066c645b 100644 --- a/lib/admin_user_index_query.rb +++ b/lib/admin_user_index_query.rb @@ -101,18 +101,6 @@ class AdminUserIndexQuery end end - def filter_by_user_with_bypass(filter) - if filter =~ /.+@.+/ - # probably an email so try the bypass - user_id = UserEmail.where("lower(email) = ?", filter.downcase).pluck(:user_id).first - if user_id - return @query.where('users.id = ?', user_id) - end - end - - @query.where('username_lower ILIKE :filter OR user_emails.email ILIKE :filter', filter: "%#{params[:filter]}%") - end - def filter_by_search if params[:email].present? return @query.where('user_emails.email = ?', params[:email].downcase) @@ -124,7 +112,7 @@ class AdminUserIndexQuery if ip = IPAddr.new(filter) rescue nil @query.where('ip_address <<= :ip OR registration_ip_address <<= :ip', ip: ip.to_cidr_s) else - filter_by_user_with_bypass(filter) + @query.filter_by_username_or_email(filter) end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index dc7ffc18434..17268fa7a11 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1650,4 +1650,41 @@ describe User do expect(inactive.active).to eq(true) end end + + describe '#filter_by_username' do + it 'should be able to filter by username' do + username = 'someuniqueusername' + user.update!(username: username) + + expect(User.filter_by_username(username)) + .to eq([user]) + + expect(User.filter_by_username('UNiQuE')) + .to eq([user]) + end + end + + describe '#filter_by_username_or_email' do + it 'should be able to filter by email' do + email = 'veryspecialtest@discourse.org' + user.update!(email: email) + + expect(User.filter_by_username_or_email(email)) + .to eq([user]) + + expect(User.filter_by_username_or_email('veryspeCiaLtest')) + .to eq([user]) + end + + it 'should be able to filter by username' do + username = 'someuniqueusername' + user.update!(username: username) + + expect(User.filter_by_username_or_email(username)) + .to eq([user]) + + expect(User.filter_by_username_or_email('UNiQuE')) + .to eq([user]) + end + end end diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index b6bf99bd07a..494d1499899 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -403,6 +403,53 @@ describe GroupsController do expect(members.map { |m| m["id"] }) .to contain_exactly(user1.id, user2.id, user3.id) end + + describe 'filterable' do + describe 'as a normal user' do + it "should not allow members to be filterable by email" do + email = 'uniquetest@discourse.org' + user1.update!(email: email) + + get "/groups/#{group.name}/members.json", params: { filter: email } + + expect(response.status).to eq(200) + members = JSON.parse(response.body)["members"] + expect(members).to eq([]) + end + end + + describe 'as an admin' do + before do + sign_in(Fabricate(:admin)) + end + + it "should allow members to be filterable by username" do + email = 'uniquetest@discourse.org' + user1.update!(email: email) + + [email.upcase, 'QUEtes'].each do |filter| + get "/groups/#{group.name}/members.json", params: { filter: filter } + + expect(response.status).to eq(200) + members = JSON.parse(response.body)["members"] + expect(members.map { |m| m["id"] }).to contain_exactly(user1.id) + end + end + + it "should allow members to be filterable by email" do + username = 'uniquetest' + user1.update!(username: username) + + [username.upcase, 'QUEtes'].each do |filter| + get "/groups/#{group.name}/members.json", params: { filter: filter } + + expect(response.status).to eq(200) + members = JSON.parse(response.body)["members"] + expect(members.map { |m| m["id"] }).to contain_exactly(user1.id) + end + end + end + end end describe "#edit" do diff --git a/test/javascripts/acceptance/group-index-test.js.es6 b/test/javascripts/acceptance/group-index-test.js.es6 index fd22b18d0da..caa0a3b8d9d 100644 --- a/test/javascripts/acceptance/group-index-test.js.es6 +++ b/test/javascripts/acceptance/group-index-test.js.es6 @@ -13,6 +13,12 @@ QUnit.test("Viewing Members as anon user", assert => { count('.group-member-dropdown') === 0, 'it does not allow anon user to manage group members' ); + + assert.equal( + find('.group-username-filter').attr('placeholder'), + I18n.t('groups.members.filter_placeholder'), + 'it should display the right filter placehodler' + ); }); }); @@ -27,5 +33,11 @@ QUnit.test("Viewing Members as an admin user", assert => { count('.group-member-dropdown') > 0, 'it allows admin user to manage group members' ); + + assert.equal( + find('.group-username-filter').attr('placeholder'), + I18n.t('groups.members.filter_placeholder_admin'), + 'it should display the right filter placehodler' + ); }); });