From a2da2971affd63d637726873e2153439715f8626 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan <tgx_world@hotmail.com> Date: Wed, 7 Dec 2016 17:28:43 +0800 Subject: [PATCH] FEATURE: Allow columns on group members page to be sortable. --- .../components/group-index-toggle.js.es6 | 24 +++++++ .../discourse/controllers/group-index.js.es6 | 17 ++++- .../javascripts/discourse/models/group.js.es6 | 10 +-- .../discourse/routes/group-index.js.es6 | 3 +- .../discourse/templates/group-index.hbs | 4 +- app/assets/stylesheets/common/base/group.scss | 20 ++++++ app/controllers/groups_controller.rb | 21 +++++- spec/controllers/groups_controller_spec.rb | 13 ++-- spec/integration/groups_spec.rb | 71 +++++++++++++++++-- 9 files changed, 158 insertions(+), 25 deletions(-) create mode 100644 app/assets/javascripts/discourse/components/group-index-toggle.js.es6 diff --git a/app/assets/javascripts/discourse/components/group-index-toggle.js.es6 b/app/assets/javascripts/discourse/components/group-index-toggle.js.es6 new file mode 100644 index 00000000000..3213bc63f1a --- /dev/null +++ b/app/assets/javascripts/discourse/components/group-index-toggle.js.es6 @@ -0,0 +1,24 @@ +import { iconHTML } from 'discourse-common/helpers/fa-icon'; +import { bufferedRender } from 'discourse-common/lib/buffered-render'; + +export default Ember.Component.extend(bufferedRender({ + tagName: 'th', + classNames: ['sortable'], + rerenderTriggers: ['order', 'asc'], + + buildBuffer(buffer) { + buffer.push(I18n.t(this.get('i18nKey'))); + + if (this.get('field') === this.get('order')) { + buffer.push(iconHTML(this.get('asc') ? 'chevron-up' : 'chevron-down')); + } + }, + + click() { + if (this.get('order') === this.field) { + this.set('asc', this.get('asc') ? null : true); + } else { + this.setProperties({ order: this.field, asc: null }); + } + } +})); diff --git a/app/assets/javascripts/discourse/controllers/group-index.js.es6 b/app/assets/javascripts/discourse/controllers/group-index.js.es6 index 162ac3e8984..3ed9e32c1d6 100644 --- a/app/assets/javascripts/discourse/controllers/group-index.js.es6 +++ b/app/assets/javascripts/discourse/controllers/group-index.js.es6 @@ -1,12 +1,22 @@ import { popupAjaxError } from 'discourse/lib/ajax-error'; import Group from 'discourse/models/group'; +import { observes } from 'ember-addons/ember-computed-decorators'; export default Ember.Controller.extend({ + queryParams: ['order', 'asc'], + order: 'last_posted_at', + asc: null, loading: false, limit: null, offset: null, isOwner: Ember.computed.alias('model.is_group_owner'), + @observes('order', 'asc') + refreshMembers() { + this.get('model') && + this.get('model').findMembers({ order: this.get('order'), asc: this.get('asc') }); + }, + actions: { removeMember(user) { this.get('model').removeMember(user); @@ -25,7 +35,12 @@ export default Ember.Controller.extend({ this.set("loading", true); - Group.loadMembers(this.get("model.name"), this.get("model.members.length"), this.get("limit")).then(result => { + Group.loadMembers( + this.get("model.name"), + this.get("model.members.length"), + this.get("limit"), + { order: this.get('order'), asc: this.get('asc') } + ).then(result => { this.get("model.members").addObjects(result.members.map(member => Discourse.User.create(member))); this.setProperties({ loading: false, diff --git a/app/assets/javascripts/discourse/models/group.js.es6 b/app/assets/javascripts/discourse/models/group.js.es6 index 510149b6ba3..c05460c04cd 100644 --- a/app/assets/javascripts/discourse/models/group.js.es6 +++ b/app/assets/javascripts/discourse/models/group.js.es6 @@ -24,12 +24,12 @@ const Group = Discourse.Model.extend({ if (userCount > 0) { return userCount; } }, - findMembers() { + 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)); - return Group.loadMembers(this.get("name"), offset, this.get("limit")).then(function (result) { + return Group.loadMembers(this.get("name"), offset, this.get("limit"), params).then(function (result) { var ownerIds = {}; result.owners.forEach(owner => ownerIds[owner.id] = true); @@ -177,12 +177,12 @@ Group.reopenClass({ return ajax("/groups/" + name + ".json").then(result => Group.create(result.basic_group)); }, - loadMembers(name, offset, limit) { + loadMembers(name, offset, limit, params) { return ajax('/groups/' + name + '/members.json', { - data: { + data: Object.assign({ limit: limit || 50, offset: offset || 0 - } + }, params) }); }, diff --git a/app/assets/javascripts/discourse/routes/group-index.js.es6 b/app/assets/javascripts/discourse/routes/group-index.js.es6 index 5182e0978cf..003a4a8d9e1 100644 --- a/app/assets/javascripts/discourse/routes/group-index.js.es6 +++ b/app/assets/javascripts/discourse/routes/group-index.js.es6 @@ -1,5 +1,4 @@ export default Discourse.Route.extend({ - titleToken() { return I18n.t('groups.members'); }, @@ -11,6 +10,6 @@ export default Discourse.Route.extend({ setupController(controller, model) { this.controllerFor("group").set("showing", "members"); controller.set("model", model); - model.findMembers(); + controller.refreshMembers(); } }); diff --git a/app/assets/javascripts/discourse/templates/group-index.hbs b/app/assets/javascripts/discourse/templates/group-index.hbs index 5e1fd167ee9..93ebb0c725d 100644 --- a/app/assets/javascripts/discourse/templates/group-index.hbs +++ b/app/assets/javascripts/discourse/templates/group-index.hbs @@ -10,8 +10,8 @@ <table class='group-members'> <thead> <th></th> - <th>{{i18n 'last_post'}}</th> - <th>{{i18n 'last_seen'}}</th> + {{group-index-toggle order=order asc=asc field='last_posted_at' i18nKey='last_post'}} + {{group-index-toggle order=order asc=asc field='last_seen_at' i18nKey='last_seen'}} <th></th> </thead> diff --git a/app/assets/stylesheets/common/base/group.scss b/app/assets/stylesheets/common/base/group.scss index 2157423bd30..08f2bc37e16 100644 --- a/app/assets/stylesheets/common/base/group.scss +++ b/app/assets/stylesheets/common/base/group.scss @@ -17,13 +17,33 @@ table.group-members { width: 100%; + table-layout: fixed; th, tr { border-bottom: 1px solid dark-light-diff($primary, $secondary, 90%, -60%); } + th:first-child { + width: 60%; + } + + th:last-child { + width: 5%; + } + th { text-align: left; + padding: 5px 0px; + + i { + margin-left: 5px; + } + + &:hover { + color: $tertiary; + cursor: pointer; + text-decoration: underline; + } } tr { diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 6c449113ee8..087ba58aea7 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -72,12 +72,27 @@ class GroupsController < ApplicationController def members group = find_group(:group_id) - limit = (params[:limit] || 50).to_i + limit = (params[:limit] || 20).to_i offset = params[:offset].to_i + order = {} + + if params[:order] && %w{last_posted_at last_seen_at}.include?(params[:order]) + order.merge!({ params[:order] => params[:asc].blank? ? 'ASC' : 'DESC' }) + end + total = group.users.count - members = group.users.order('NOT group_users.owner').order(:username_lower).limit(limit).offset(offset) - owners = group.users.order(:username_lower).where('group_users.owner') + members = group.users + .order('NOT group_users.owner') + .order(order) + .order(:username_lower) + .limit(limit) + .offset(offset) + + owners = group.users + .order(order) + .order(:username_lower) + .where('group_users.owner') render json: { members: serialize_data(members, GroupUserSerializer), diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 5cd791ba183..de32060f6c2 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -53,20 +53,19 @@ describe GroupsController do expect(response).to be_success end - # Pending until we fix group truncation - skip "ensures that membership can be paginated" do + it "ensures that membership can be paginated" do 5.times { group.add(Fabricate(:user)) } - usernames = group.users.map{ |m| m['username'] }.sort + usernames = group.users.map{ |m| m.username }.sort xhr :get, :members, group_id: group.name, limit: 3 expect(response).to be_success - members = JSON.parse(response.body) - expect(members.map{ |m| m['username'] }).to eq(usernames[0..2]) + members = JSON.parse(response.body)["members"] + expect(members.map { |m| m['username'] }).to eq(usernames[0..2]) xhr :get, :members, group_id: group.name, limit: 3, offset: 3 expect(response).to be_success - members = JSON.parse(response.body) - expect(members.map{ |m| m['username'] }).to eq(usernames[3..4]) + members = JSON.parse(response.body)["members"] + expect(members.map { |m| m['username'] }).to eq(usernames[3..4]) end end diff --git a/spec/integration/groups_spec.rb b/spec/integration/groups_spec.rb index c063e7d78dc..a69c44da44c 100644 --- a/spec/integration/groups_spec.rb +++ b/spec/integration/groups_spec.rb @@ -1,12 +1,12 @@ require 'rails_helper' describe "Groups" do - let(:password) { 'somecomplicatedpassword' } - let(:email_token) { Fabricate(:email_token, confirmed: true) } - let(:user) { email_token.user } + let(:user) { Fabricate(:user) } - before do - user.update_attributes!(password: password) + def sign_in(user) + password = 'somecomplicatedpassword' + user.update!(password: password) + Fabricate(:email_token, confirmed: true, user: user) post "/session.json", { login: user.username, password: password } expect(response).to be_success end @@ -15,6 +15,7 @@ describe "Groups" do let(:group) { Fabricate(:group, name: 'test', users: [user]) } it "should return the right response" do + sign_in(user) group get "/groups/test/mentionable.json", { name: group.name } @@ -40,6 +41,7 @@ describe "Groups" do context "when user is group owner" do before do group.add_owner(user) + sign_in(user) end it "should be able update the group" do @@ -66,6 +68,7 @@ describe "Groups" do context "when user is group admin" do before do user.update_attributes!(admin: true) + sign_in(user) end it 'should be able to update the group' do @@ -78,10 +81,68 @@ describe "Groups" do context "when user is not a group owner or admin" do it 'should not be able to update the group' do + sign_in(user) + xhr :put, "/groups/#{group.id}", { group: { name: 'testing' } } expect(response.status).to eq(403) end end end + + describe 'members' do + let(:user1) do + Fabricate(:user, + last_seen_at: Time.zone.now, + last_posted_at: Time.zone.now - 1.day, + email: 'b@test.org' + ) + end + + let(:user2) do + Fabricate(:user, + last_seen_at: Time.zone.now - 1 .day, + last_posted_at: Time.zone.now, + email: 'a@test.org' + ) + end + + let(:group) { Fabricate(:group, users: [user1, user2]) } + + it "should allow members to be sorted by" do + xhr :get, "/groups/#{group.name}/members", order: 'last_seen_at', asc: true + + expect(response).to be_success + + members = JSON.parse(response.body)["members"] + + expect(members.map { |m| m["id"] }).to eq([user1.id, user2.id]) + + xhr :get, "/groups/#{group.name}/members", order: 'last_seen_at' + + expect(response).to be_success + + members = JSON.parse(response.body)["members"] + + expect(members.map { |m| m["id"] }).to eq([user2.id, user1.id]) + + xhr :get, "/groups/#{group.name}/members", order: 'last_posted_at', asc: true + + expect(response).to be_success + + members = JSON.parse(response.body)["members"] + + expect(members.map { |m| m["id"] }).to eq([user2.id, user1.id]) + end + + it "should not allow members to be sorted by columns that are not allowed" do + xhr :get, "/groups/#{group.name}/members", order: 'email' + + expect(response).to be_success + + members = JSON.parse(response.body)["members"] + + expect(members.map { |m| m["id"] }).to eq([user1.id, user2.id]) + end + end end