From 1a2b9435b0a17c9d06eb5e8e47cfe042a62b9853 Mon Sep 17 00:00:00 2001 From: Blake Erickson Date: Thu, 14 May 2020 20:10:59 -0600 Subject: [PATCH] DEV: Standardize table sorting verbiage (#9757) * DEV: Standardize table sorting verbiage This commit creates a common component that tables can use to make their headers sortable. This commit also standardizes on using `desc` as the default and passing in the `asc=true` flag to adjust the sorting direction. * Add deprecation warnings Adds deprecation warnings if using previous params and maintains backwards compatibility. Set the default sort value for group members to be asc. * switch group requests to use common table-header-toggle * update fixture --- .../components/admin-directory-toggle.js | 30 ------------------- .../controllers/admin-users-list-show.js | 4 +-- .../admin/routes/admin-users-list-show.js | 4 +-- .../components/admin-directory-toggle.hbs | 1 - .../admin/templates/users-list-show.hbs | 16 +++++----- .../app/components/group-index-toggle.js | 30 ------------------- ...ctory-toggle.js => table-header-toggle.js} | 0 .../discourse/app/controllers/group-index.js | 12 ++++---- .../components/group-index-toggle.hbs | 1 - ...ory-toggle.hbs => table-header-toggle.hbs} | 0 .../discourse/app/templates/group-index.hbs | 8 ++--- .../app/templates/group-requests.hbs | 4 +-- .../discourse/app/templates/users.hbs | 14 ++++----- app/controllers/groups_controller.rb | 6 +++- lib/admin_user_index_query.rb | 3 +- .../components/admin_user_index_query_spec.rb | 6 ++-- spec/requests/groups_controller_spec.rb | 12 ++++---- test/javascripts/helpers/create-pretender.js | 4 +-- 18 files changed, 49 insertions(+), 106 deletions(-) delete mode 100644 app/assets/javascripts/admin/components/admin-directory-toggle.js delete mode 100644 app/assets/javascripts/admin/templates/components/admin-directory-toggle.hbs delete mode 100644 app/assets/javascripts/discourse/app/components/group-index-toggle.js rename app/assets/javascripts/discourse/app/components/{directory-toggle.js => table-header-toggle.js} (100%) delete mode 100644 app/assets/javascripts/discourse/app/templates/components/group-index-toggle.hbs rename app/assets/javascripts/discourse/app/templates/components/{directory-toggle.hbs => table-header-toggle.hbs} (100%) diff --git a/app/assets/javascripts/admin/components/admin-directory-toggle.js b/app/assets/javascripts/admin/components/admin-directory-toggle.js deleted file mode 100644 index 093f29826b8..00000000000 --- a/app/assets/javascripts/admin/components/admin-directory-toggle.js +++ /dev/null @@ -1,30 +0,0 @@ -import Component from "@ember/component"; -import { iconHTML } from "discourse-common/lib/icon-library"; - -export default Component.extend({ - tagName: "th", - classNames: ["sortable"], - chevronIcon: null, - toggleProperties() { - if (this.order === this.field) { - this.set("ascending", this.ascending ? null : true); - } else { - this.setProperties({ order: this.field, ascending: null }); - } - }, - toggleChevron() { - if (this.order === this.field) { - let chevron = iconHTML(this.ascending ? "chevron-up" : "chevron-down"); - this.set("chevronIcon", `${chevron}`.htmlSafe()); - } else { - this.set("chevronIcon", null); - } - }, - click() { - this.toggleProperties(); - }, - didReceiveAttrs() { - this._super(...arguments); - this.toggleChevron(); - } -}); diff --git a/app/assets/javascripts/admin/controllers/admin-users-list-show.js b/app/assets/javascripts/admin/controllers/admin-users-list-show.js index 23deb83955d..9f82545dacb 100644 --- a/app/assets/javascripts/admin/controllers/admin-users-list-show.js +++ b/app/assets/javascripts/admin/controllers/admin-users-list-show.js @@ -11,7 +11,7 @@ export default Controller.extend(CanCheckEmails, { model: null, query: null, order: null, - ascending: null, + asc: null, showEmails: false, refreshing: false, listFilter: null, @@ -54,7 +54,7 @@ export default Controller.extend(CanCheckEmails, { filter: this.listFilter, show_emails: this.showEmails, order: this.order, - ascending: this.ascending, + asc: this.asc, page: this._page }) .then(result => { diff --git a/app/assets/javascripts/admin/routes/admin-users-list-show.js b/app/assets/javascripts/admin/routes/admin-users-list-show.js index 764fe4ad341..c16591d37f3 100644 --- a/app/assets/javascripts/admin/routes/admin-users-list-show.js +++ b/app/assets/javascripts/admin/routes/admin-users-list-show.js @@ -3,7 +3,7 @@ import DiscourseRoute from "discourse/routes/discourse"; export default DiscourseRoute.extend({ queryParams: { order: { refreshModel: true }, - ascending: { refreshModel: true } + asc: { refreshModel: true } }, // TODO: this has been introduced to fix a bug in admin-users-list-show @@ -18,7 +18,7 @@ export default DiscourseRoute.extend({ if (controller) { controller.setProperties({ order: transition.to.queryParams.order, - ascending: transition.to.queryParams.ascending, + asc: transition.to.queryParams.asc, query: params.filter, refreshing: false }); diff --git a/app/assets/javascripts/admin/templates/components/admin-directory-toggle.hbs b/app/assets/javascripts/admin/templates/components/admin-directory-toggle.hbs deleted file mode 100644 index 7ea697404f6..00000000000 --- a/app/assets/javascripts/admin/templates/components/admin-directory-toggle.hbs +++ /dev/null @@ -1 +0,0 @@ -{{i18n this.i18nKey}}{{chevronIcon}} diff --git a/app/assets/javascripts/admin/templates/users-list-show.hbs b/app/assets/javascripts/admin/templates/users-list-show.hbs index e96f794a678..ff66a923a29 100644 --- a/app/assets/javascripts/admin/templates/users-list-show.hbs +++ b/app/assets/javascripts/admin/templates/users-list-show.hbs @@ -23,14 +23,14 @@ {{#if model}} - {{admin-directory-toggle field="username" i18nKey="username" order=order ascending=ascending}} - {{admin-directory-toggle field="email" i18nKey="email" order=order ascending=ascending}} - {{admin-directory-toggle field="last_emailed" i18nKey="admin.users.last_emailed" order=order ascending=ascending}} - {{admin-directory-toggle field="seen" i18nKey="last_seen" order=order ascending=ascending}} - {{admin-directory-toggle field="topics_viewed" i18nKey="admin.user.topics_entered" order=order ascending=ascending}} - {{admin-directory-toggle field="posts_read" i18nKey="admin.user.posts_read_count" order=order ascending=ascending}} - {{admin-directory-toggle field="read_time" i18nKey="admin.user.time_read" order=order ascending=ascending}} - {{admin-directory-toggle field="created" i18nKey="created" order=order ascending=ascending}} + {{table-header-toggle field="username" labelKey="username" order=order asc=asc}} + {{table-header-toggle field="email" labelKey="email" order=order asc=asc}} + {{table-header-toggle field="last_emailed" labelKey="admin.users.last_emailed" order=order asc=asc}} + {{table-header-toggle field="seen" labelKey="last_seen" order=order asc=asc}} + {{table-header-toggle field="topics_viewed" labelKey="admin.user.topics_entered" order=order asc=asc}} + {{table-header-toggle field="posts_read" labelKey="admin.user.posts_read_count" order=order asc=asc}} + {{table-header-toggle field="read_time" labelKey="admin.user.time_read" order=order asc=asc}} + {{table-header-toggle field="created" labelKey="created" order=order asc=asc}} {{#if siteSettings.must_approve_users}} {{/if}} diff --git a/app/assets/javascripts/discourse/app/components/group-index-toggle.js b/app/assets/javascripts/discourse/app/components/group-index-toggle.js deleted file mode 100644 index f84b9b3d21d..00000000000 --- a/app/assets/javascripts/discourse/app/components/group-index-toggle.js +++ /dev/null @@ -1,30 +0,0 @@ -import Component from "@ember/component"; -import { iconHTML } from "discourse-common/lib/icon-library"; - -export default Component.extend({ - tagName: "th", - classNames: ["sortable"], - chevronIcon: null, - toggleProperties() { - if (this.order === this.field) { - this.set("desc", this.desc ? null : true); - } else { - this.setProperties({ order: this.field, desc: null }); - } - }, - toggleChevron() { - if (this.order === this.field) { - let chevron = iconHTML(this.desc ? "chevron-down" : "chevron-up"); - this.set("chevronIcon", `${chevron}`.htmlSafe()); - } else { - this.set("chevronIcon", null); - } - }, - click() { - this.toggleProperties(); - }, - didReceiveAttrs() { - this._super(...arguments); - this.toggleChevron(); - } -}); diff --git a/app/assets/javascripts/discourse/app/components/directory-toggle.js b/app/assets/javascripts/discourse/app/components/table-header-toggle.js similarity index 100% rename from app/assets/javascripts/discourse/app/components/directory-toggle.js rename to app/assets/javascripts/discourse/app/components/table-header-toggle.js diff --git a/app/assets/javascripts/discourse/app/controllers/group-index.js b/app/assets/javascripts/discourse/app/controllers/group-index.js index 6d661891441..c0ab592fdea 100644 --- a/app/assets/javascripts/discourse/app/controllers/group-index.js +++ b/app/assets/javascripts/discourse/app/controllers/group-index.js @@ -8,10 +8,10 @@ import { action } from "@ember/object"; export default Controller.extend({ application: controller(), - queryParams: ["order", "desc", "filter"], + queryParams: ["order", "asc", "filter"], order: "", - desc: null, + asc: true, filter: null, filterInput: null, @@ -24,7 +24,7 @@ export default Controller.extend({ this.set("filter", this.filterInput); }, 500), - @observes("order", "desc", "filter") + @observes("order", "asc", "filter") _filtersChanged() { this.findMembers(true); }, @@ -49,9 +49,9 @@ export default Controller.extend({ }); }, - @discourseComputed("order", "desc", "filter") - memberParams(order, desc, filter) { - return { order, desc, filter }; + @discourseComputed("order", "asc", "filter") + memberParams(order, asc, filter) { + return { order, asc, filter }; }, hasMembers: gt("model.members.length", 0), diff --git a/app/assets/javascripts/discourse/app/templates/components/group-index-toggle.hbs b/app/assets/javascripts/discourse/app/templates/components/group-index-toggle.hbs deleted file mode 100644 index 7ea697404f6..00000000000 --- a/app/assets/javascripts/discourse/app/templates/components/group-index-toggle.hbs +++ /dev/null @@ -1 +0,0 @@ -{{i18n this.i18nKey}}{{chevronIcon}} diff --git a/app/assets/javascripts/discourse/app/templates/components/directory-toggle.hbs b/app/assets/javascripts/discourse/app/templates/components/table-header-toggle.hbs similarity index 100% rename from app/assets/javascripts/discourse/app/templates/components/directory-toggle.hbs rename to app/assets/javascripts/discourse/app/templates/components/table-header-toggle.hbs diff --git a/app/assets/javascripts/discourse/app/templates/group-index.hbs b/app/assets/javascripts/discourse/app/templates/group-index.hbs index f917c42d76a..07d46d374b1 100644 --- a/app/assets/javascripts/discourse/app/templates/group-index.hbs +++ b/app/assets/javascripts/discourse/app/templates/group-index.hbs @@ -29,11 +29,11 @@ {{#load-more selector=".group-members tr" action=(action "loadMore")}}
{{i18n "admin.users.approved"}}
- {{group-index-toggle order=order desc=desc field="username_lower" i18nKey="username"}} + {{table-header-toggle order=order asc=asc field="username_lower" labelKey="username"}} - {{group-index-toggle order=order desc=desc field="added_at" i18nKey="groups.member_added"}} - {{group-index-toggle order=order desc=desc field="last_posted_at" i18nKey="last_post"}} - {{group-index-toggle order=order desc=desc field="last_seen_at" i18nKey="last_seen"}} + {{table-header-toggle order=order asc=asc field="added_at" labelKey="groups.member_added"}} + {{table-header-toggle order=order asc=asc field="last_posted_at" labelKey="last_post"}} + {{table-header-toggle order=order asc=asc field="last_seen_at" labelKey="last_seen"}} diff --git a/app/assets/javascripts/discourse/app/templates/group-requests.hbs b/app/assets/javascripts/discourse/app/templates/group-requests.hbs index 7ee73b8fb6d..017aaaef083 100644 --- a/app/assets/javascripts/discourse/app/templates/group-requests.hbs +++ b/app/assets/javascripts/discourse/app/templates/group-requests.hbs @@ -12,8 +12,8 @@ {{#load-more selector=".group-members tr" action=(action "loadMore")}}
{{i18n "groups.members.owner"}}
- {{group-index-toggle order=order desc=desc field="username_lower" i18nKey="username"}} - {{group-index-toggle order=order desc=desc field="requested_at" i18nKey="groups.member_requested"}} + {{table-header-toggle order=order asc=asc field="username_lower" labelKey="username"}} + {{table-header-toggle order=order asc=asc field="requested_at" labelKey="groups.member_requested"}} diff --git a/app/assets/javascripts/discourse/app/templates/users.hbs b/app/assets/javascripts/discourse/app/templates/users.hbs index cdb85f24de5..1d3450d0f8b 100644 --- a/app/assets/javascripts/discourse/app/templates/users.hbs +++ b/app/assets/javascripts/discourse/app/templates/users.hbs @@ -22,13 +22,13 @@
{{i18n "groups.requests.reason"}}
- {{directory-toggle field="likes_received" order=order asc=asc icon="heart"}} - {{directory-toggle field="likes_given" order=order asc=asc icon="heart"}} - {{directory-toggle field="topic_count" order=order asc=asc}} - {{directory-toggle field="post_count" order=order asc=asc}} - {{directory-toggle field="topics_entered" order=order asc=asc}} - {{directory-toggle field="posts_read" order=order asc=asc}} - {{directory-toggle field="days_visited" order=order asc=asc}} + {{table-header-toggle field="likes_received" order=order asc=asc icon="heart"}} + {{table-header-toggle field="likes_given" order=order asc=asc icon="heart"}} + {{table-header-toggle field="topic_count" order=order asc=asc}} + {{table-header-toggle field="post_count" order=order asc=asc}} + {{table-header-toggle field="topics_entered" order=order asc=asc}} + {{table-header-toggle field="posts_read" order=order asc=asc}} + {{table-header-toggle field="days_visited" order=order asc=asc}} {{#if showTimeRead}} {{/if}} diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 60ba5167994..bb763811d08 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -209,7 +209,11 @@ class GroupsController < ApplicationController raise Discourse::InvalidParameters.new(:limit) if limit < 0 || limit > 1000 raise Discourse::InvalidParameters.new(:offset) if offset < 0 - dir = (params[:desc] && params[:desc].present?) ? 'DESC' : 'ASC' + dir = (params[:asc] && params[:asc].present?) ? 'ASC' : 'DESC' + if params[:desc] + Discourse.deprecate(":desc is deprecated please use :asc instead", output_in_test: true) + dir = (params[:desc] && params[:desc].present?) ? 'DESC' : 'ASC' + end order = "" if params[:requesters] diff --git a/lib/admin_user_index_query.rb b/lib/admin_user_index_query.rb index 4d04e61fd29..8780aced0c4 100644 --- a/lib/admin_user_index_query.rb +++ b/lib/admin_user_index_query.rb @@ -37,7 +37,8 @@ class AdminUserIndexQuery end def custom_direction - asc = params[:ascending] + Discourse.deprecate(":ascending is deprecated please use :asc instead", output_in_test: true) if params[:ascending] + asc = params[:asc] || params[:ascending] asc.present? && asc ? "ASC" : "DESC" end diff --git a/spec/components/admin_user_index_query_spec.rb b/spec/components/admin_user_index_query_spec.rb index 971468523fc..e41c7fc068a 100644 --- a/spec/components/admin_user_index_query_spec.rb +++ b/spec/components/admin_user_index_query_spec.rb @@ -29,7 +29,7 @@ describe AdminUserIndexQuery do end it "allows custom ordering asc" do - query = ::AdminUserIndexQuery.new(order: "trust_level", ascending: true) + query = ::AdminUserIndexQuery.new(order: "trust_level", asc: true) expect(query.find_users_query.to_sql).to match("trust_level ASC") end @@ -39,7 +39,7 @@ describe AdminUserIndexQuery do end it "allows custom ordering and direction for stats" do - query = ::AdminUserIndexQuery.new(order: "topics_viewed", ascending: true) + query = ::AdminUserIndexQuery.new(order: "topics_viewed", asc: true) expect(query.find_users_query.to_sql).to match("topics_entered ASC") end end @@ -118,7 +118,7 @@ describe AdminUserIndexQuery do end it "shows nil values first with asc" do - users = ::AdminUserIndexQuery.new(order: "last_emailed", ascending: true).find_users + users = ::AdminUserIndexQuery.new(order: "last_emailed", asc: true).find_users expect(users.where('users.id > -2').count).to eq(2) expect(users.where('users.id > -2').order('users.id asc').first.username).to eq("system") diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index 8d58f3f5315..45a4db2d21f 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -489,7 +489,7 @@ describe GroupsController do 4.times { group.add(Fabricate(:user)) } usernames = group.users.map { |m| m.username }.sort - get "/groups/#{group.name}/members.json", params: { limit: 3 } + get "/groups/#{group.name}/members.json", params: { limit: 3, asc: true } expect(response.status).to eq(200) @@ -497,7 +497,7 @@ describe GroupsController do expect(members.map { |m| m['username'] }).to eq(usernames[0..2]) - get "/groups/#{group.name}/members.json", params: { limit: 3, offset: 3 } + get "/groups/#{group.name}/members.json", params: { limit: 3, offset: 3, asc: true } expect(response.status).to eq(200) @@ -505,7 +505,7 @@ describe GroupsController do expect(members.map { |m| m['username'] }).to eq(usernames[3..5]) - get "/groups/#{group.name}/members.json", params: { order: 'added_at', desc: true } + get "/groups/#{group.name}/members.json", params: { order: 'added_at' } members = response.parsed_body["members"] expect(members.last['added_at']).to eq(first_user.created_at.as_json) @@ -851,7 +851,7 @@ describe GroupsController do it "should allow members to be sorted by" do get "/groups/#{group.name}/members.json", params: { - order: 'last_seen_at', desc: true + order: 'last_seen_at' } expect(response.status).to eq(200) @@ -860,7 +860,7 @@ describe GroupsController do expect(members.map { |m| m["id"] }).to eq([user1.id, user2.id, user3.id]) - get "/groups/#{group.name}/members.json", params: { order: 'last_seen_at' } + get "/groups/#{group.name}/members.json", params: { order: 'last_seen_at', asc: true } expect(response.status).to eq(200) @@ -869,7 +869,7 @@ describe GroupsController do expect(members.map { |m| m["id"] }).to eq([user2.id, user1.id, user3.id]) get "/groups/#{group.name}/members.json", params: { - order: 'last_posted_at', desc: true + order: 'last_posted_at' } expect(response.status).to eq(200) diff --git a/test/javascripts/helpers/create-pretender.js b/test/javascripts/helpers/create-pretender.js index d5e92e55ed5..b09965fb964 100644 --- a/test/javascripts/helpers/create-pretender.js +++ b/test/javascripts/helpers/create-pretender.js @@ -620,7 +620,7 @@ export function applyDefaultHandlers(pretender) { }); } - const ascending = request.queryParams.ascending; + const asc = request.queryParams.asc; const order = request.queryParams.order; if (order) { @@ -629,7 +629,7 @@ export function applyDefaultHandlers(pretender) { }); } - if (ascending) { + if (asc) { store = store.reverse(); }
{{i18n "directory.total_rows" count=model.totalRows}}{{i18n "directory.time_read"}}