Merge pull request #4594 from tgxworld/make_group_members_sortable

FEATURE: Allow columns on group members page to be sortable.

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 });
+    }
+  }
+}));

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,

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)
     });
   },
 

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();
   }
 });

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>
 

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 {

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),

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
 

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