From 27cbc065635e1becda307d6684a5aae39c7a1065 Mon Sep 17 00:00:00 2001 From: Neil Lalonde Date: Fri, 16 May 2014 11:33:44 -0400 Subject: [PATCH] Add fixed_category_positions site setting to handle whether categories are ordered by specified positions or by activity. --- .../controllers/discovery/categories.js.es6 | 4 ++ .../controllers/edit-category.js.es6 | 18 ++----- .../discovery/categories.js.handlebars | 2 +- .../modal/edit_category.js.handlebars | 15 +++--- app/controllers/categories_controller.rb | 8 +-- app/models/category_list.rb | 13 +++-- app/models/concerns/positionable.rb | 13 +++-- config/locales/server.en.yml | 1 + config/site_settings.yml | 3 ++ ...131022045114_add_uncategorized_category.rb | 2 +- .../20140120155706_add_lounge_category.rb | 4 +- .../20140122043508_add_meta_category.rb | 4 +- .../20140227201005_add_staff_category.rb | 4 +- ...111_init_fixed_category_positions_value.rb | 15 ++++++ spec/components/category_list_spec.rb | 50 ++++++++++++------- spec/components/concern/positionable_spec.rb | 29 +---------- .../controllers/categories_controller_spec.rb | 7 --- 17 files changed, 91 insertions(+), 101 deletions(-) create mode 100644 db/migrate/20140515220111_init_fixed_category_positions_value.rb diff --git a/app/assets/javascripts/discourse/controllers/discovery/categories.js.es6 b/app/assets/javascripts/discourse/controllers/discovery/categories.js.es6 index 97db88d5468..68bb362f5dc 100644 --- a/app/assets/javascripts/discourse/controllers/discovery/categories.js.es6 +++ b/app/assets/javascripts/discourse/controllers/discovery/categories.js.es6 @@ -32,6 +32,10 @@ export default Discourse.DiscoveryController.extend({ return Discourse.User.currentProp('staff'); }.property(), + canOrder: function() { + return this.get('canEdit') && Discourse.SiteSettings.fixed_category_positions; + }.property('Discourse.SiteSettings.fixed_category_positions'), + moveCategory: function(categoryId, position){ this.get('model.categories').moveCategory(categoryId, position); }, diff --git a/app/assets/javascripts/discourse/controllers/edit-category.js.es6 b/app/assets/javascripts/discourse/controllers/edit-category.js.es6 index a9c2feffa02..6d56b61f6cb 100644 --- a/app/assets/javascripts/discourse/controllers/edit-category.js.es6 +++ b/app/assets/javascripts/discourse/controllers/edit-category.js.es6 @@ -12,7 +12,6 @@ export default Discourse.ObjectController.extend(Discourse.ModalFunctionality, { 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) { @@ -29,7 +28,6 @@ export default Discourse.ObjectController.extend(Discourse.ModalFunctionality, { onShow: function() { this.changeSize(); this.titleChanged(); - this.set('defaultPosition', this.get('position') === null); }, changeSize: function() { @@ -116,6 +114,10 @@ export default Discourse.ObjectController.extend(Discourse.ModalFunctionality, { return !this.get('isUncategorized') && this.get('id'); }.property('isUncategorized', 'id'), + showPositionInput: function() { + return Discourse.SiteSettings.fixed_category_positions; + }.property('Discourse.SiteSettings.fixed_category_positions'), + actions: { selectGeneral: function() { @@ -148,17 +150,6 @@ export default Discourse.ObjectController.extend(Discourse.ModalFunctionality, { 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'), @@ -166,7 +157,6 @@ export default Discourse.ObjectController.extend(Discourse.ModalFunctionality, { 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/discovery/categories.js.handlebars b/app/assets/javascripts/discourse/templates/discovery/categories.js.handlebars index 9d4a4114acf..9224959e968 100644 --- a/app/assets/javascripts/discourse/templates/discovery/categories.js.handlebars +++ b/app/assets/javascripts/discourse/templates/discovery/categories.js.handlebars @@ -7,7 +7,7 @@ {{i18n categories.latest}} {{i18n categories.topics}} {{i18n categories.posts}} - {{#if canEdit}}{{/if}} + {{#if canOrder}}{{/if}} 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 31755892822..7915feea087 100644 --- a/app/assets/javascripts/discourse/templates/modal/edit_category.js.handlebars +++ b/app/assets/javascripts/discourse/templates/modal/edit_category.js.handlebars @@ -123,15 +123,12 @@ {{/if}} -
- - {{textField value=position disabled=defaultPosition class="position-input"}} -  {{i18n or}}  - -
+ {{#if showPositionInput}} +
+ + {{textField value=position class="position-input"}} +
+ {{/if}} {{/unless}} diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb index 41d8a051e3b..025f03adf65 100644 --- a/app/controllers/categories_controller.rb +++ b/app/controllers/categories_controller.rb @@ -50,19 +50,19 @@ class CategoriesController < ApplicationController def create guardian.ensure_can_create!(Category) + position = category_params.delete(:position) + @category = Category.create(category_params.merge(user: current_user)) return render_json_error(@category) unless @category.save - @category.move_to(category_params[:position].to_i) if category_params[:position] + @category.move_to(position.to_i) if position render_serialized(@category, CategorySerializer) end def update guardian.ensure_can_edit!(@category) json_result(@category, serializer: CategorySerializer) { |cat| - if category_params[:position] - category_params[:position] == 'default' ? cat.use_default_position : cat.move_to(category_params[:position].to_i) - end + cat.move_to(category_params[:position].to_i) if category_params[:position] if category_params.key? :email_in and category_params[:email_in].length == 0 # properly null the value so the database constrain doesn't catch us category_params[:email_in] = nil diff --git a/app/models/category_list.rb b/app/models/category_list.rb index f1e679cb62d..93855357dc9 100644 --- a/app/models/category_list.rb +++ b/app/models/category_list.rb @@ -57,16 +57,19 @@ class CategoryList @categories = Category .includes(:featured_users, subcategories: [:topic_only_relative_url]) .secured(@guardian) - .order('position asc') - .order('COALESCE(categories.posts_week, 0) DESC') - .order('COALESCE(categories.posts_month, 0) DESC') - .order('COALESCE(categories.posts_year, 0) DESC') - .to_a + if SiteSetting.fixed_category_positions + @categories = @categories.order('position ASC').order('id ASC') + else + @categories = @categories.order('COALESCE(categories.posts_week, 0) DESC') + .order('COALESCE(categories.posts_month, 0) DESC') + .order('COALESCE(categories.posts_year, 0) DESC') + end if latest_post_only? @categories = @categories.includes(:latest_post => {:topic => :last_poster} ) end + @categories = @categories.to_a subcategories = {} to_delete = Set.new @categories.each do |c| diff --git a/app/models/concerns/positionable.rb b/app/models/concerns/positionable.rb index db0f92472ae..335b8ce0acb 100644 --- a/app/models/concerns/positionable.rb +++ b/app/models/concerns/positionable.rb @@ -1,6 +1,12 @@ module Positionable extend ActiveSupport::Concern + included do + before_save do + self.position ||= self.class.count + end + end + def move_to(position_arg) position = [[position_arg, 0].max, self.class.count - 1].min @@ -27,11 +33,4 @@ module Positionable SET position = :position WHERE id = :id", {id: id, position: position} end - - def use_default_position - self.exec_sql " - UPDATE #{self.class.table_name} - SET POSITION = null - WHERE id = :id", {id: id} - end end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 873bc04e7e6..097498bc429 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -620,6 +620,7 @@ en: max_image_width: "Maximum allowed width of images in a post" max_image_height: "Maximum allowed height of images in a post" category_featured_topics: "Number of topics displayed per category on the /categories page. After changing this value, it takes up to 15 minutes for the categories page to update." + fixed_category_positions: "If checked, you will be able to arrange categories into a fixed order. If unchecked, categories are listed in order of activity." add_rel_nofollow_to_user_content: "Add rel nofollow to all submitted user content, except for internal links (including parent domains) changing this requires you update all your baked markdown with: \"rake posts:rebake\"" exclude_rel_nofollow_domains: "A pipe-delimited list of domains where nofollow is not added (tld.com will automatically allow sub.tld.com as well)" diff --git a/config/site_settings.yml b/config/site_settings.yml index 221685f1cb9..d85439503ce 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -78,6 +78,9 @@ basic: category_featured_topics: client: true default: 3 + fixed_category_positions: + client: true + default: false topics_per_page: 30 posts_per_page: client: true diff --git a/db/migrate/20131022045114_add_uncategorized_category.rb b/db/migrate/20131022045114_add_uncategorized_category.rb index f44e851efc4..126f9b7ed0e 100644 --- a/db/migrate/20131022045114_add_uncategorized_category.rb +++ b/db/migrate/20131022045114_add_uncategorized_category.rb @@ -9,7 +9,7 @@ class AddUncategorizedCategory < ActiveRecord::Migration result = execute "INSERT INTO categories (name,color,slug,description,text_color, user_id, created_at, updated_at, position) - VALUES ('#{name}', 'AB9364', 'uncategorized', '', 'FFFFFF', -1, now(), now(), 1 ) + VALUES ('#{name}', 'AB9364', 'uncategorized', '', 'FFFFFF', -1, now(), now(), 0 ) RETURNING id " category_id = result[0]["id"].to_i diff --git a/db/migrate/20140120155706_add_lounge_category.rb b/db/migrate/20140120155706_add_lounge_category.rb index 4d7127c0b15..95ac270a6ea 100644 --- a/db/migrate/20140120155706_add_lounge_category.rb +++ b/db/migrate/20140120155706_add_lounge_category.rb @@ -13,8 +13,8 @@ class AddLoungeCategory < ActiveRecord::Migration end result = execute "INSERT INTO categories - (name, color, text_color, created_at, updated_at, user_id, slug, description, read_restricted) - VALUES ('#{name}', 'EEEEEE', '652D90', now(), now(), -1, '#{Slug.for(name)}', '#{description}', true) + (name, color, text_color, created_at, updated_at, user_id, slug, description, read_restricted, position) + VALUES ('#{name}', 'EEEEEE', '652D90', now(), now(), -1, '#{Slug.for(name)}', '#{description}', true, 3) RETURNING id" category_id = result[0]["id"].to_i diff --git a/db/migrate/20140122043508_add_meta_category.rb b/db/migrate/20140122043508_add_meta_category.rb index 7de9e648494..d8f5dbaa8fb 100644 --- a/db/migrate/20140122043508_add_meta_category.rb +++ b/db/migrate/20140122043508_add_meta_category.rb @@ -8,8 +8,8 @@ class AddMetaCategory < ActiveRecord::Migration name = I18n.t('meta_category_name') if Category.exec_sql("SELECT 1 FROM categories where name ilike '#{name}'").count == 0 result = execute "INSERT INTO categories - (name, color, text_color, created_at, updated_at, user_id, slug, description, read_restricted) - VALUES ('#{name}', '808281', 'FFFFFF', now(), now(), -1, '#{Slug.for(name)}', '#{description}', true) + (name, color, text_color, created_at, updated_at, user_id, slug, description, read_restricted, position) + VALUES ('#{name}', '808281', 'FFFFFF', now(), now(), -1, '#{Slug.for(name)}', '#{description}', true, 1) RETURNING id" category_id = result[0]["id"].to_i diff --git a/db/migrate/20140227201005_add_staff_category.rb b/db/migrate/20140227201005_add_staff_category.rb index 8494a0f2d5f..441dbaef3af 100644 --- a/db/migrate/20140227201005_add_staff_category.rb +++ b/db/migrate/20140227201005_add_staff_category.rb @@ -7,8 +7,8 @@ class AddStaffCategory < ActiveRecord::Migration name = I18n.t('staff_category_name') if Category.exec_sql("SELECT 1 FROM categories where name ilike '#{name}'").count == 0 result = execute "INSERT INTO categories - (name, color, text_color, created_at, updated_at, user_id, slug, description, read_restricted) - VALUES ('#{name}', '283890', 'FFFFFF', now(), now(), -1, '#{Slug.for(name)}', '#{description}', true) + (name, color, text_color, created_at, updated_at, user_id, slug, description, read_restricted, position) + VALUES ('#{name}', '283890', 'FFFFFF', now(), now(), -1, '#{Slug.for(name)}', '#{description}', true, 2) RETURNING id" category_id = result[0]["id"].to_i diff --git a/db/migrate/20140515220111_init_fixed_category_positions_value.rb b/db/migrate/20140515220111_init_fixed_category_positions_value.rb new file mode 100644 index 00000000000..96610b505f3 --- /dev/null +++ b/db/migrate/20140515220111_init_fixed_category_positions_value.rb @@ -0,0 +1,15 @@ +class InitFixedCategoryPositionsValue < ActiveRecord::Migration + def up + # Look at existing categories to determine if positions have been specified + result = Category.exec_sql("SELECT count(*) FROM categories WHERE position IS NOT NULL") + + # Greater than 4 because uncategorized, meta, staff, lounge all have positions by default + if result[0]['count'].to_i > 4 + execute "INSERT INTO site_settings (name, data_type, value, created_at, updated_at) VALUES ('fixed_category_positions', 5, 't', now(), now())" + end + end + + def down + execute "DELETE FROM site_settings WHERE name = 'fixed_category_positions'" + end +end diff --git a/spec/components/category_list_spec.rb b/spec/components/category_list_spec.rb index 02f7030a605..5c394bb8e99 100644 --- a/spec/components/category_list_spec.rb +++ b/spec/components/category_list_spec.rb @@ -89,29 +89,41 @@ describe CategoryList do 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] + context 'fixed_category_positions is enabled' do + before do + SiteSetting.stubs(:fixed_category_positions).returns(true) + end + + it "returns categories in specified order" do + cat1, cat2 = Fabricate(:category, position: 1), Fabricate(:category, position: 0) + category_ids.should == [cat2.id, cat1.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) + first_three = category_ids[0,3] # The order is not deterministic + first_three.should include(cat1.id) + first_three.should include(cat2.id) + first_three.should include(cat4.id) + category_ids[-1].should == cat3.id + end 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 + context 'fixed_category_positions is disabled' do + before do + SiteSetting.stubs(:fixed_category_positions).returns(false) + end - it "default always at the end" do - cat1, cat2, cat3 = Fabricate(:category, position: 0), Fabricate(:category, position: 2), Fabricate(:category, position: nil) - category_ids.should == [cat1.id, cat2.id, cat3.id] - end + it "returns categories in order of activity" do + cat1 = Fabricate(:category, position: 0, posts_week: 1, posts_month: 1, posts_year: 1) + cat2 = Fabricate(:category, position: 1, posts_week: 2, posts_month: 1, posts_year: 1) + category_ids.should == [cat2.id, cat1.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) - first_three = category_ids[0,3] # The order is not deterministic - first_three.should include(cat1.id) - first_three.should include(cat2.id) - first_three.should include(cat4.id) - category_ids[-1].should == cat3.id + it "returns categories in order of id when there's no activity" do + cat1, cat2 = Fabricate(:category, position: 1), Fabricate(:category, position: 0) + category_ids.should == [cat1.id, cat2.id] + end end end diff --git a/spec/components/concern/positionable_spec.rb b/spec/components/concern/positionable_spec.rb index 8da6afb86f3..da5ae19a6c5 100644 --- a/spec/components/concern/positionable_spec.rb +++ b/spec/components/concern/positionable_spec.rb @@ -45,34 +45,7 @@ describe Positionable do item = TestItem.new item.id = 7 item.save - 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] + item.position.should == 5 end end end diff --git a/spec/controllers/categories_controller_spec.rb b/spec/controllers/categories_controller_spec.rb index f6cb6a71987..3ccc461b823 100644 --- a/spec/controllers/categories_controller_spec.rb +++ b/spec/controllers/categories_controller_spec.rb @@ -165,13 +165,6 @@ describe CategoriesController do @category.color.should == "ff0" @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