diff --git a/app/assets/javascripts/discourse/components/basic_topic_list_component.js b/app/assets/javascripts/discourse/components/basic_topic_list_component.js index ca725e889..1ff51a95d 100644 --- a/app/assets/javascripts/discourse/components/basic_topic_list_component.js +++ b/app/assets/javascripts/discourse/components/basic_topic_list_component.js @@ -17,15 +17,22 @@ Discourse.DiscourseBasicTopicListComponent = Ember.Component.extend({ } }.property('topicList.loaded'), + _topicListChanged: function() { + this._initFromTopicList(this.get('topicList')); + }.observes('topicList'), + + _initFromTopicList: function(topicList) { + this.setProperties({ + topics: topicList.get('topics'), + sortOrder: topicList.get('sortOrder') + }); + }, + init: function() { this._super(); - var topicList = this.get('topicList'); if (topicList) { - this.setProperties({ - topics: topicList.get('topics'), - sortOrder: topicList.get('sortOrder') - }); + this._initFromTopicList(topicList); } else { // Without a topic list, we assume it's loaded always. this.set('loaded', true); diff --git a/app/assets/javascripts/discourse/components/breadcrumbs_component.js b/app/assets/javascripts/discourse/components/breadcrumbs_component.js index d1efb82ae..1b10082d2 100644 --- a/app/assets/javascripts/discourse/components/breadcrumbs_component.js +++ b/app/assets/javascripts/discourse/components/breadcrumbs_component.js @@ -1,3 +1,11 @@ +/** + A breadcrumb including category drop downs + + @class DiscourseBreadcrumbsComponent + @extends Ember.Component + @namespace Discourse + @module Discourse +**/ Discourse.DiscourseBreadcrumbsComponent = Ember.Component.extend({ classNames: ['category-breadcrumb'], tagName: 'ol', diff --git a/app/assets/javascripts/discourse/components/categorydrop_component.js b/app/assets/javascripts/discourse/components/categorydrop_component.js index f24a9959e..20c077536 100644 --- a/app/assets/javascripts/discourse/components/categorydrop_component.js +++ b/app/assets/javascripts/discourse/components/categorydrop_component.js @@ -1,3 +1,11 @@ +/** + Renders a drop down for selecting a category + + @class DiscourseCategorydropComponent + @extends Ember.Component + @namespace Discourse + @module Discourse +**/ Discourse.DiscourseCategorydropComponent = Ember.Component.extend({ classNameBindings: ['category::no-category', 'categories:has-drop'], tagName: 'li', diff --git a/app/assets/javascripts/discourse/components/heading_component.js b/app/assets/javascripts/discourse/components/heading_component.js index 80fe7a0d0..3ec8d1b7f 100644 --- a/app/assets/javascripts/discourse/components/heading_component.js +++ b/app/assets/javascripts/discourse/components/heading_component.js @@ -1,7 +1,14 @@ +/** + Renders a heading for a table with optional sorting controls. + + @class DiscourseHeadingComponent + @extends Ember.Component + @namespace Discourse + @module Discourse +**/ Discourse.DiscourseHeadingComponent = Ember.Component.extend({ tagName: 'th', - - classNameBindings: ['number:num', 'sortBy', 'iconSortClass:sorting', 'sortable'], + classNameBindings: ['number:num', 'sortBy', 'iconSortClass:sorting', 'sortable:sortable'], attributeBindings: ['colspan'], sortable: function() { diff --git a/app/assets/javascripts/discourse/models/topic_list.js b/app/assets/javascripts/discourse/models/topic_list.js index 308260dde..6317f1a8c 100644 --- a/app/assets/javascripts/discourse/models/topic_list.js +++ b/app/assets/javascripts/discourse/models/topic_list.js @@ -17,14 +17,17 @@ function finderFor(filter, params) { if (keys.length > 0) { var encoded = []; keys.forEach(function(p) { - encoded.push(p + "=" + params[p]); + var value = params[p]; + if (typeof value !== 'undefined') { + encoded.push(p + "=" + value); + } }); url += "?" + encoded.join('&'); } } return Discourse.ajax(url); - } + }; } Discourse.TopicList = Discourse.Model.extend({ diff --git a/app/controllers/list_controller.rb b/app/controllers/list_controller.rb index 446a9b651..b95b689b8 100644 --- a/app/controllers/list_controller.rb +++ b/app/controllers/list_controller.rb @@ -142,7 +142,7 @@ class ListController < ApplicationController exclude_category: (params[:exclude_category] || menu_item.try(:filter)), category: params[:category], sort_order: params[:sort_order], - sort_descending: params[:sort_order] + sort_descending: params[:sort_descending] } end diff --git a/lib/topic_query.rb b/lib/topic_query.rb index 0e58135c7..f8eda7532 100644 --- a/lib/topic_query.rb +++ b/lib/topic_query.rb @@ -19,6 +19,14 @@ class TopicQuery sort_order sort_descending).map(&:to_sym) + # Maps `sort_order` to a columns in `topics` + SORTABLE_MAPPING = { + 'likes' => 'like_count', + 'views' => 'views', + 'posts' => 'posts_count', + 'activity' => 'bumped_at' + } + def initialize(user=nil, options={}) options.assert_valid_keys(VALID_OPTIONS) @@ -147,6 +155,31 @@ class TopicQuery result end + def default_ordering(result, options) + # If we're logged in, we have to pay attention to our pinned settings + if @user + result = options[:category].blank? ? result.order(TopicQuerySQL.order_nocategory_with_pinned_sql) : + result.order(TopicQuerySQL.order_with_pinned_sql) + else + result = result.order(TopicQuerySQL.order_nocategory_basic_bumped) + end + result + end + + def apply_ordering(result, options) + sort_column = SORTABLE_MAPPING[options[:sort_order]] || 'default' + sort_dir = (options[:sort_descending] == "false") ? "ASC" : "DESC" + + # If we are sorting in the default order desc, we should consider including pinned + # topics. Otherwise, just use bumped_at. + if sort_column == 'default' + return default_ordering(result, options) if sort_dir == 'DESC' + sort_column = 'bumped_at' + end + + result.order("topics.#{sort_column} #{sort_dir}") + end + # Create results based on a bunch of default options def default_results(options={}) options.reverse_merge!(@options) @@ -170,15 +203,7 @@ class TopicQuery result = result.references(:categories) end - unless options[:unordered] - # If we're logged in, we have to pay attention to our pinned settings - if @user - result = category_id.nil? ? result.order(TopicQuerySQL.order_nocategory_with_pinned_sql) : - result.order(TopicQuerySQL.order_with_pinned_sql) - else - result = result.order(TopicQuerySQL.order_nocategory_basic_bumped) - end - end + result = apply_ordering(result, options) unless options[:unordered] result = result.listable_topics.includes(category: :topic_only_relative_url) result = result.where('categories.name is null or categories.name <> ?', options[:exclude_category]).references(:categories) if options[:exclude_category] diff --git a/spec/components/topic_query_spec.rb b/spec/components/topic_query_spec.rb index 155f0eba7..7c6acd1e6 100644 --- a/spec/components/topic_query_spec.rb +++ b/spec/components/topic_query_spec.rb @@ -59,16 +59,56 @@ describe TopicQuery do context 'a bunch of topics' do - let!(:regular_topic) { Fabricate(:topic, title: 'this is a regular topic', user: creator, bumped_at: 15.minutes.ago) } - let!(:pinned_topic) { Fabricate(:topic, title: 'this is a pinned topic', user: creator, pinned_at: 10.minutes.ago, bumped_at: 10.minutes.ago) } - let!(:archived_topic) { Fabricate(:topic, title: 'this is an archived topic', user: creator, archived: true, bumped_at: 6.minutes.ago) } - let!(:invisible_topic) { Fabricate(:topic, title: 'this is an invisible topic', user: creator, visible: false, bumped_at: 5.minutes.ago) } - let!(:closed_topic) { Fabricate(:topic, title: 'this is a closed topic', user: creator, closed: true, bumped_at: 1.minute.ago) } + let!(:regular_topic) do + Fabricate(:topic, title: 'this is a regular topic', + user: creator, + views: 100, + like_count: 66, + posts_count: 3, + bumped_at: 15.minutes.ago) + end + let!(:pinned_topic) do + Fabricate(:topic, title: 'this is a pinned topic', + user: creator, + views: 10, + like_count: 100, + posts_count: 5, + pinned_at: 10.minutes.ago, + bumped_at: 10.minutes.ago) + end + let!(:archived_topic) do + Fabricate(:topic, title: 'this is an archived topic', + user: creator, + views: 50, + like_count: 30, + posts_count: 4, + archived: true, + bumped_at: 6.minutes.ago) + end + let!(:invisible_topic) do + Fabricate(:topic, title: 'this is an invisible topic', + user: creator, + views: 1, + like_count: 5, + posts_count: 2, + visible: false, + bumped_at: 5.minutes.ago) + end + let!(:closed_topic) do + Fabricate(:topic, title: 'this is a closed topic', + user: creator, + views: 2, + like_count: 1, + posts_count: 1, + closed: true, + bumped_at: 1.minute.ago) + end + let(:topics) { topic_query.list_latest.topics } context 'list_latest' do it "returns the topics in the correct order" do - topics.map(&:title).should == [pinned_topic, closed_topic, archived_topic, regular_topic].map(&:title) + topics.map(&:id).should == [pinned_topic, closed_topic, archived_topic, regular_topic].map(&:id) end it "includes the invisible topic if you're a moderator" do @@ -78,6 +118,39 @@ describe TopicQuery do it "includes the invisible topic if you're an admin" do TopicQuery.new(admin).list_latest.topics.include?(invisible_topic).should be_true end + + context 'sort_order' do + + def ids_in_order(order, descending=true) + TopicQuery.new(admin, sort_order: order, sort_descending: descending ? 'true' : 'false').list_latest.topics.map(&:id) + end + + it "returns the topics in likes order if requested" do + ids_in_order('posts').should == [pinned_topic, archived_topic, regular_topic, invisible_topic, closed_topic].map(&:id) + end + + it "returns the topics in reverse likes order if requested" do + ids_in_order('posts', false).should == [closed_topic, invisible_topic, regular_topic, archived_topic, pinned_topic].map(&:id) + end + + it "returns the topics in likes order if requested" do + ids_in_order('likes').should == [pinned_topic, regular_topic, archived_topic, invisible_topic, closed_topic].map(&:id) + end + + it "returns the topics in reverse likes order if requested" do + ids_in_order('likes', false).should == [closed_topic, invisible_topic, archived_topic, regular_topic, pinned_topic].map(&:id) + end + + it "returns the topics in views order if requested" do + ids_in_order('views').should == [regular_topic, archived_topic, pinned_topic, closed_topic, invisible_topic].map(&:id) + end + + it "returns the topics in reverse views order if requested" do + ids_in_order('views', false).should == [invisible_topic, closed_topic, pinned_topic, archived_topic, regular_topic].map(&:id) + end + + end + end context 'after clearring a pinned topic' do