From 341adc93a49e3c98b8cd5063d7dd40ad83f88f58 Mon Sep 17 00:00:00 2001 From: Neil Lalonde Date: Mon, 16 Dec 2013 15:13:43 -0500 Subject: [PATCH] Allow categories with null position, which means sort them based on activity. Mix absolutely positioned (position is not null) categories with null position categories. --- .../controllers/edit_category_controller.js | 14 ++++++ .../modal/edit_category.js.handlebars | 7 ++- app/controllers/categories_controller.rb | 4 +- app/models/category.rb | 2 +- app/models/category_list.rb | 39 ++++++++++------ config/locales/client.en.yml | 1 + ...57_make_position_nullable_in_categories.rb | 10 ++++ lib/concern/positionable.rb | 46 ++++++++++--------- spec/components/category_list_spec.rb | 33 ++++++++++++- spec/components/concern/positionable_spec.rb | 32 +++++++++++-- .../controllers/categories_controller_spec.rb | 9 +++- 11 files changed, 154 insertions(+), 43 deletions(-) create mode 100644 db/migrate/20131216164557_make_position_nullable_in_categories.rb diff --git a/app/assets/javascripts/discourse/controllers/edit_category_controller.js b/app/assets/javascripts/discourse/controllers/edit_category_controller.js index 3e73bfdbb35..7dcf4d7f93b 100644 --- a/app/assets/javascripts/discourse/controllers/edit_category_controller.js +++ b/app/assets/javascripts/discourse/controllers/edit_category_controller.js @@ -12,6 +12,7 @@ Discourse.EditCategoryController = Discourse.ObjectController.extend(Discourse.M securitySelected: Ember.computed.equal('selectedTab', 'security'), settingsSelected: Ember.computed.equal('selectedTab', 'settings'), foregroundColors: ['FFFFFF', '000000'], + defaultPosition: false, parentCategories: function() { return Discourse.Category.list().filter(function (c) { @@ -22,6 +23,7 @@ Discourse.EditCategoryController = Discourse.ObjectController.extend(Discourse.M onShow: function() { this.changeSize(); this.titleChanged(); + this.set('defaultPosition', this.get('position') === null); }, changeSize: function() { @@ -127,6 +129,17 @@ Discourse.EditCategoryController = Discourse.ObjectController.extend(Discourse.M this.get('model').removePermission(permission); }, + toggleDefaultPosition: function() { + this.toggleProperty('defaultPosition'); + }, + + disableDefaultPosition: function() { + this.set('defaultPosition', false); + Em.run.schedule('afterRender', function() { + this.$('.position-input').focus(); + }); + }, + saveCategory: function() { var self = this, model = this.get('model'), @@ -134,6 +147,7 @@ Discourse.EditCategoryController = Discourse.ObjectController.extend(Discourse.M this.set('saving', true); model.set('parentCategory', parentCategory); + if (this.get('defaultPosition')) { model.set('position', 'default'); } self.set('saving', false); this.get('model').save().then(function(result) { diff --git a/app/assets/javascripts/discourse/templates/modal/edit_category.js.handlebars b/app/assets/javascripts/discourse/templates/modal/edit_category.js.handlebars index b8a4f76ef45..6057ca5700b 100644 --- a/app/assets/javascripts/discourse/templates/modal/edit_category.js.handlebars +++ b/app/assets/javascripts/discourse/templates/modal/edit_category.js.handlebars @@ -100,7 +100,12 @@
- {{textField value=position}} + {{textField value=position disabled=defaultPosition class="position-input"}} +  {{i18n or}}  +
diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb index 4a8fc6539ca..8b9d0c1e421 100644 --- a/app/controllers/categories_controller.rb +++ b/app/controllers/categories_controller.rb @@ -60,7 +60,9 @@ class CategoriesController < ApplicationController def update guardian.ensure_can_edit!(@category) json_result(@category, serializer: CategorySerializer) { |cat| - cat.move_to(category_params[:position].to_i) if category_params[:position] + if category_params[:position] + category_params[:position] == 'default' ? cat.use_default_position : cat.move_to(category_params[:position].to_i) + end category_params.delete(:position) cat.update_attributes(category_params) } diff --git a/app/models/category.rb b/app/models/category.rb index cf639d33eec..2aae060a439 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -364,7 +364,7 @@ end # post_count :integer default(0), not null # latest_post_id :integer # latest_topic_id :integer -# position :integer not null +# position :integer # parent_category_id :integer # # Indexes diff --git a/app/models/category_list.rb b/app/models/category_list.rb index 0c8a7dc3700..df862a4cef1 100644 --- a/app/models/category_list.rb +++ b/app/models/category_list.rb @@ -53,23 +53,36 @@ class CategoryList # Find a list of all categories to associate the topics with def find_categories - @categories = Category - .includes(:featured_users) - .secured(@guardian) - .order('position ASC') + @absolute_position_categories = Category + .includes(:featured_users) + .secured(@guardian) + .where('position IS NOT NULL') + .order('position ASC') + @default_position_categories = Category + .includes(:featured_users) + .secured(@guardian) + .where('position IS NULL') + .order('COALESCE(categories.posts_week, 0) DESC') + .order('COALESCE(categories.posts_month, 0) DESC') + .order('COALESCE(categories.posts_year, 0) DESC') if latest_post_only? - @categories = @categories.includes(:latest_post => {:topic => :last_poster} ) - # else - # # This is only for the old 2-column layout. - # # Use this when we support "organic" positioning of some/all categories. - # @categories = @categories - # .order('COALESCE(categories.topics_week, 0) DESC') - # .order('COALESCE(categories.topics_month, 0) DESC') - # .order('COALESCE(categories.topics_year, 0) DESC') + @absolute_position_categories = @absolute_position_categories.includes(:latest_post => {:topic => :last_poster} ) + @default_position_categories = @default_position_categories.includes(:latest_post => {:topic => :last_poster} ) end - @categories = @categories.to_a + @default_position_categories = @default_position_categories.to_a + @categories = [] + index = 0 + @absolute_position_categories.to_a.each do |c| + if c.position > index + @categories.push(*(@default_position_categories.shift(c.position - index))) + end + @categories << c + index = c.position + 1 if c.position >= index # handles duplicate position values + end + @categories.push *@default_position_categories # Whatever is left is put on the end + subcategories = {} to_delete = Set.new diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 2e564140bba..65745f6f12f 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -1007,6 +1007,7 @@ en: add_permission: "Add Permission" this_year: "this year" position: "position" + default_position: "Default Position" parent: "Parent Category" flagging: diff --git a/db/migrate/20131216164557_make_position_nullable_in_categories.rb b/db/migrate/20131216164557_make_position_nullable_in_categories.rb new file mode 100644 index 00000000000..9b7dc619ac3 --- /dev/null +++ b/db/migrate/20131216164557_make_position_nullable_in_categories.rb @@ -0,0 +1,10 @@ +class MakePositionNullableInCategories < ActiveRecord::Migration + def up + change_column :categories, :position, :integer, null: true + end + + def down + execute "update categories set position=0 where position is null" + change_column :categories, :position, :integer, null: false + end +end diff --git a/lib/concern/positionable.rb b/lib/concern/positionable.rb index ec97a4442dc..fdeaf2f6fa9 100644 --- a/lib/concern/positionable.rb +++ b/lib/concern/positionable.rb @@ -2,36 +2,38 @@ module Concern module Positionable extend ActiveSupport::Concern - included do - before_save do - self.position ||= self.class.count + def move_to(position_arg) + + position = [[position_arg, 0].max, self.class.count - 1].min + + if self.position.nil? or position > self.position + self.exec_sql " + UPDATE #{self.class.table_name} + SET position = position - 1 + WHERE position > :current_position and position <= :new_position", + {current_position: self.position, new_position: position} + elsif position < self.position + self.exec_sql " + UPDATE #{self.class.table_name} + SET position = position + 1 + WHERE position >= :new_position and position < :current_position", + {current_position: self.position, new_position: position} + else + # Not moving to a new position + return end - end - - def move_to(position) - - self.exec_sql " - UPDATE #{self.class.table_name} - SET position = position - 1 - WHERE position > :position AND position > 0", {position: self.position} self.exec_sql " UPDATE #{self.class.table_name} SET position = :position WHERE id = :id", {id: id, position: position} + end + def use_default_position self.exec_sql " - UPDATE #{self.class.table_name} t - SET position = x.position - 1 - FROM ( - SELECT i.id, row_number() - OVER(ORDER BY i.position asc, - CASE WHEN i.id = :id THEN 0 ELSE 1 END ASC) AS position - FROM #{self.class.table_name} i - WHERE i.position IS NOT NULL - ) x - WHERE x.id = t.id AND t.position <> x.position - 1 - ", {id: id} + UPDATE #{self.class.table_name} + SET POSITION = null + WHERE id = :id", {id: id} end end end diff --git a/spec/components/category_list_spec.rb b/spec/components/category_list_spec.rb index 97f8f8d177b..cc9e7163530 100644 --- a/spec/components/category_list_spec.rb +++ b/spec/components/category_list_spec.rb @@ -4,11 +4,11 @@ require 'category_list' describe CategoryList do let(:user) { Fabricate(:user) } + let(:admin) { Fabricate(:admin) } let(:category_list) { CategoryList.new(Guardian.new user) } context "security" do it "properly hide secure categories" do - admin = Fabricate(:admin) user = Fabricate(:user) cat = Fabricate(:category) @@ -73,4 +73,35 @@ describe CategoryList do end + describe 'category order' do + let(:category_ids) { CategoryList.new(Guardian.new(admin)).categories.map(&:id) - [SiteSetting.uncategorized_category_id] } + + before do + uncategorized = Category.find(SiteSetting.uncategorized_category_id) + uncategorized.position = 100 + uncategorized.save + end + + it "returns topics in specified order" do + cat1, cat2 = Fabricate(:category, position: 1), Fabricate(:category, position: 0) + category_ids.should == [cat2.id, cat1.id] + end + + it "returns default order categories" do + cat1, cat2 = Fabricate(:category, position: nil), Fabricate(:category, position: nil) + category_ids.should include(cat1.id) + category_ids.should include(cat2.id) + end + + it "mixes default order categories with absolute position categories" do + cat1, cat2, cat3 = Fabricate(:category, position: 0), Fabricate(:category, position: 2), Fabricate(:category, position: nil) + category_ids.should == [cat1.id, cat3.id, cat2.id] + end + + it "handles duplicate position values" do + cat1, cat2, cat3, cat4 = Fabricate(:category, position: 0), Fabricate(:category, position: 0), Fabricate(:category, position: nil), Fabricate(:category, position: 0) + category_ids.should == [cat1.id, cat2.id, cat4.id, cat3.id] + end + end + end diff --git a/spec/components/concern/positionable_spec.rb b/spec/components/concern/positionable_spec.rb index fa8c3f8b151..82895ae75d5 100644 --- a/spec/components/concern/positionable_spec.rb +++ b/spec/components/concern/positionable_spec.rb @@ -37,8 +37,7 @@ describe Concern::Positionable do TestItem.find(3).move_to(1) positions.should == [0,3,1,2,4] - # this is somewhat odd, but when there is not positioning - # not much we can do + # this is somewhat odd, but when there is no such position, not much we can do TestItem.find(1).move_to(5) positions.should == [0,3,2,4,1] @@ -47,7 +46,34 @@ describe Concern::Positionable do item = TestItem.new item.id = 7 item.save - item.position.should == 5 + item.position.should be_nil + end + + it "can set records to have null position" do + 5.times do |i| + Topic.exec_sql("insert into test_items(id,position) values(#{i}, #{i})") + end + + TestItem.find(2).use_default_position + TestItem.find(2).position.should be_nil + + TestItem.find(1).move_to(4) + TestItem.order('id ASC').pluck(:position).should == [0,4,nil,2,3] + end + + it "can maintain null positions when moving things around" do + 5.times do |i| + Topic.exec_sql("insert into test_items(id,position) values(#{i}, null)") + end + + TestItem.find(2).move_to(0) + TestItem.order('id asc').pluck(:position).should == [nil,nil,0,nil,nil] + TestItem.find(0).move_to(4) + TestItem.order('id asc').pluck(:position).should == [4,nil,0,nil,nil] + TestItem.find(2).move_to(1) + TestItem.order('id asc').pluck(:position).should == [4,nil,1,nil,nil] + TestItem.find(0).move_to(1) + TestItem.order('id asc').pluck(:position).should == [1,nil,2,nil,nil] end end end diff --git a/spec/controllers/categories_controller_spec.rb b/spec/controllers/categories_controller_spec.rb index 8af52341c92..6c167b8745c 100644 --- a/spec/controllers/categories_controller_spec.rb +++ b/spec/controllers/categories_controller_spec.rb @@ -105,6 +105,8 @@ describe CategoriesController do describe "logged in" do + let(:valid_attrs) { {id: @category.id, name: "hello", color: "ff0", text_color: "fff"} } + before do @user = log_in(:moderator) @category = Fabricate(:category, user: @user) @@ -146,7 +148,6 @@ describe CategoriesController do describe "success" do it "updates the group correctly" do - readonly = CategoryGroup.permission_types[:readonly] create_post = CategoryGroup.permission_types[:create_post] @@ -167,7 +168,13 @@ describe CategoriesController do @category.color.should == "ff0" @category.hotness.should == 2 @category.auto_close_hours.should == 72 + end + it "can set category to use default position" do + xhr :put, :update, valid_attrs.merge(position: 'default') + response.should be_success + @category.reload + @category.position.should be_nil end end end