From 0522aabaab892dac38c7a5b07ce752a801994bf2 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Mon, 19 Mar 2018 16:14:50 +0800 Subject: [PATCH] UX: Allow user_count on groups page to be sortable. --- .../components/directory-toggle.js.es6 | 14 +++++-- .../discourse/controllers/groups.js.es6 | 9 +++-- .../discourse/routes/groups.js.es6 | 9 ++++- .../discourse/templates/groups.hbs | 9 +++-- .../stylesheets/common/base/groups.scss | 13 +++++-- app/controllers/groups_controller.rb | 5 ++- app/models/group.rb | 4 +- spec/requests/groups_controller_spec.rb | 39 +++++++++++++++++++ 8 files changed, 83 insertions(+), 19 deletions(-) diff --git a/app/assets/javascripts/discourse/components/directory-toggle.js.es6 b/app/assets/javascripts/discourse/components/directory-toggle.js.es6 index 8da529957a4..f6a9de9b800 100644 --- a/app/assets/javascripts/discourse/components/directory-toggle.js.es6 +++ b/app/assets/javascripts/discourse/components/directory-toggle.js.es6 @@ -1,16 +1,22 @@ import { iconHTML } from 'discourse-common/lib/icon-library'; import { bufferedRender } from 'discourse-common/lib/buffered-render'; +import computed from 'ember-addons/ember-computed-decorators'; export default Ember.Component.extend(bufferedRender({ tagName: 'th', classNames: ['sortable'], attributeBindings: ['title'], rerenderTriggers: ['order', 'asc'], + labelKey: null, + + @computed("field", "labelKey") + title(field, labelKey) { + if (!labelKey) { + labelKey = `directory.${this.get('field')}`; + } - title: function() { - const labelKey = 'directory.' + this.get('field'); return I18n.t(labelKey + '_long', { defaultValue: I18n.t(labelKey) }); - }.property('field'), + }, buildBuffer(buffer) { const icon = this.get('icon'); @@ -19,7 +25,7 @@ export default Ember.Component.extend(bufferedRender({ } const field = this.get('field'); - buffer.push(I18n.t('directory.' + field)); + buffer.push(I18n.t(this.get('labelKey') || `directory.${field}`)); if (field === this.get('order')) { buffer.push(iconHTML(this.get('asc') ? 'chevron-up' : 'chevron-down')); diff --git a/app/assets/javascripts/discourse/controllers/groups.js.es6 b/app/assets/javascripts/discourse/controllers/groups.js.es6 index d8ce08f3227..4c2b675f707 100644 --- a/app/assets/javascripts/discourse/controllers/groups.js.es6 +++ b/app/assets/javascripts/discourse/controllers/groups.js.es6 @@ -2,15 +2,18 @@ import { observes } from 'ember-addons/ember-computed-decorators'; export default Ember.Controller.extend({ application: Ember.inject.controller(), + queryParams: ["order", "asc"], + order: null, + asc: null, - @observes("groups.canLoadMore") + @observes("model.canLoadMore") _showFooter() { - this.set("application.showFooter", !this.get("groups.canLoadMore")); + this.set("application.showFooter", !this.get("model.canLoadMore")); }, actions: { loadMore() { - this.get('groups').loadMore(); + this.get('model').loadMore(); } } }); diff --git a/app/assets/javascripts/discourse/routes/groups.js.es6 b/app/assets/javascripts/discourse/routes/groups.js.es6 index 582ebe0f5c6..3aca55574b2 100644 --- a/app/assets/javascripts/discourse/routes/groups.js.es6 +++ b/app/assets/javascripts/discourse/routes/groups.js.es6 @@ -1,4 +1,11 @@ export default Discourse.Route.extend({ + queryParams: { + order: { refreshModel: true, replace: true }, + asc: { refreshModel: true, replace: true }, + }, + + refreshQueryWithoutTransition: true, + titleToken() { return I18n.t('groups.index.title'); }, @@ -8,6 +15,6 @@ export default Discourse.Route.extend({ }, setupController(controller, model) { - controller.set('groups', model); + controller.set('model', model); } }); diff --git a/app/assets/javascripts/discourse/templates/groups.hbs b/app/assets/javascripts/discourse/templates/groups.hbs index 296324e7737..aa5734f4679 100644 --- a/app/assets/javascripts/discourse/templates/groups.hbs +++ b/app/assets/javascripts/discourse/templates/groups.hbs @@ -1,17 +1,18 @@ {{#d-section pageClass="groups"}}

{{i18n "groups.index.title"}}

- {{#if groups}} + + {{#if model}} {{#load-more selector=".groups-table .groups-table-row" action="loadMore"}}
- + {{directory-toggle field="user_count" labelKey="groups.user_count" order=order asc=asc}} - {{#each groups as |group|}} + {{#each model as |group|}}
{{i18n "groups.user_count"}}{{i18n "groups.membership"}}
{{#link-to "group.members" group.name}} @@ -61,7 +62,7 @@ {{/load-more}} - {{conditional-loading-spinner condition=groups.loadingMore}} + {{conditional-loading-spinner condition=model.loadingMore}} {{else}}

{{i18n "groups.index.empty"}}

{{/if}} diff --git a/app/assets/stylesheets/common/base/groups.scss b/app/assets/stylesheets/common/base/groups.scss index 138510f28b4..08f346df66d 100644 --- a/app/assets/stylesheets/common/base/groups.scss +++ b/app/assets/stylesheets/common/base/groups.scss @@ -9,16 +9,23 @@ th { border-bottom: 1px solid $primary-low; - padding: 5px 0; + padding: 0.5em; text-align: left; } + .sortable { + &:hover { + background-color: $primary-low; + cursor: pointer; + } + } + tr { - border-bottom: 1px solid $primary-low; + border-bottom: 1px solid $primary-low; td { color: blend-primary-secondary(50%); - padding: 0.8em 0; + padding: 0.8em; } td.groups-user-count { diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 9f48cc843ad..0543fd514cf 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -20,8 +20,9 @@ class GroupsController < ApplicationController page_size = 30 page = params[:page]&.to_i || 0 - - groups = Group.visible_groups(current_user) + order = %w{name user_count}.delete(params[:order]) + dir = params[:asc] ? 'ASC' : 'DESC' + groups = Group.visible_groups(current_user, order ? "#{order} #{dir}" : nil) unless guardian.is_staff? # hide automatic groups from all non stuff to de-clutter page diff --git a/app/models/group.rb b/app/models/group.rb index 6c542b9b6c4..5888e0e6c51 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -81,8 +81,8 @@ class Group < ActiveRecord::Base validates :mentionable_level, inclusion: { in: ALIAS_LEVELS.values } validates :messageable_level, inclusion: { in: ALIAS_LEVELS.values } - scope :visible_groups, ->(user) { - groups = Group.order(name: :asc).where("groups.id > 0") + scope :visible_groups, Proc.new { |user, order| + groups = Group.order(order || "name ASC").where("groups.id > 0") unless user&.admin sql = <<~SQL diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index 295b91875b1..e9c9ddb6d2b 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -18,6 +18,45 @@ describe GroupsController do end end + context 'sortable' do + let!(:other_group) { Fabricate(:group, name: "zzzzzz", users: [user]) } + + %w{ + desc + asc + }.each do |order| + context "#{order} order" do + it 'should return the right response' do + is_asc = order == 'asc' + params = { order: 'name' } + params.merge!(asc: true) if is_asc + group + get "/groups.json", params: params + + expect(response.status).to eq(200) + + group_ids = [group.id, other_group.id] + group_ids.reverse! if !is_asc + + expect(JSON.parse(response.body)["groups"].map { |group| group["id"] }) + .to eq(group_ids) + end + end + end + + context 'ascending order' do + it 'should return the right response' do + group + get "/groups.json", params: { order: 'name' } + + expect(response.status).to eq(200) + + expect(JSON.parse(response.body)["groups"].map { |group| group["id"] }) + .to eq([other_group.id, group.id]) + end + end + end + it 'should return the right response' do group get "/groups.json"