diff --git a/app/assets/javascripts/discourse/models/composer.js b/app/assets/javascripts/discourse/models/composer.js index 11e258a0f..432d96dc6 100644 --- a/app/assets/javascripts/discourse/models/composer.js +++ b/app/assets/javascripts/discourse/models/composer.js @@ -339,10 +339,6 @@ Discourse.Composer = Discourse.Model.extend({ reply: opts.reply || this.get("reply") || "" }); - if (!this.get('categoryId') && !Discourse.SiteSettings.allow_uncategorized_topics && Discourse.Category.list().length > 0) { - this.set('categoryId', Discourse.Category.list()[0].get('id')); - } - if (opts.postId) { this.set('loading', true); Discourse.Post.load(opts.postId).then(function(result) { diff --git a/app/assets/javascripts/discourse/views/category_chooser_view.js b/app/assets/javascripts/discourse/views/category_chooser_view.js index 83ef194b0..aba5c5afa 100644 --- a/app/assets/javascripts/discourse/views/category_chooser_view.js +++ b/app/assets/javascripts/discourse/views/category_chooser_view.js @@ -21,7 +21,11 @@ Discourse.CategoryChooserView = Discourse.ComboboxView.extend({ }, none: function() { - if (Discourse.SiteSettings.allow_uncategorized_topics || this.get('showUncategorized')) return 'category.none'; + if (!Discourse.SiteSettings.allow_uncategorized_topics) { + return 'category.choose'; + } else if (Discourse.SiteSettings.allow_uncategorized_topics || this.get('showUncategorized')) { + return 'category.none'; + } }.property('showUncategorized'), template: function(text, templateData) { diff --git a/app/assets/stylesheets/desktop/compose.scss b/app/assets/stylesheets/desktop/compose.scss index 02c2062ef..79d66f4f1 100644 --- a/app/assets/stylesheets/desktop/compose.scss +++ b/app/assets/stylesheets/desktop/compose.scss @@ -335,7 +335,7 @@ .category-input .popup-tip { width: 240px; left: 432px; - top: -7px; + top: -19px; } } diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index af21eb991..9c1068ff8 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -108,7 +108,7 @@ class TopicsController < ApplicationController success = false Topic.transaction do success = topic.save - topic.change_category(params[:category]) if success + success = topic.change_category(params[:category]) if success end # this is used to return the title to the client as it may have been diff --git a/app/models/topic.rb b/app/models/topic.rb index 978cdb377..cf0452906 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -56,6 +56,12 @@ class Topic < ActiveRecord::Base :case_sensitive => false, :collection => Proc.new{ Topic.listable_topics } } + # The allow_uncategorized_topics site setting can be changed at any time, so there may be + # existing topics with nil category. We'll allow that, but when someone tries to make a new + # topic or change a topic's category, perform validation. + attr_accessor :do_category_validation + validates :category_id, :presence => { :if => Proc.new { @do_category_validation && !SiteSetting.allow_uncategorized_topics } } + before_validation do self.sanitize_title self.title = TextCleaner.clean_title(TextSentinel.title_sentinel(title).text) if errors[:title].empty? @@ -135,6 +141,7 @@ class Topic < ActiveRecord::Base before_create do self.bumped_at ||= Time.now self.last_post_user_id ||= user_id + self.do_category_validation = true if !@ignore_category_auto_close and self.category and self.category.auto_close_days and self.auto_close_at.nil? set_auto_close(self.category.auto_close_days) end @@ -316,8 +323,8 @@ class Topic < ActiveRecord::Base def changed_to_category(cat) - return if cat.blank? - return if Category.where(topic_id: id).first.present? + return true if cat.blank? + return true if Category.where(topic_id: id).first.present? Topic.transaction do old_category = category @@ -327,12 +334,15 @@ class Topic < ActiveRecord::Base end self.category_id = cat.id - save - - CategoryFeaturedTopic.feature_topics_for(old_category) - Category.where(id: cat.id).update_all 'topic_count = topic_count + 1' - CategoryFeaturedTopic.feature_topics_for(cat) unless old_category.try(:id) == cat.try(:id) + if save + CategoryFeaturedTopic.feature_topics_for(old_category) + Category.where(id: cat.id).update_all 'topic_count = topic_count + 1' + CategoryFeaturedTopic.feature_topics_for(cat) unless old_category.try(:id) == cat.try(:id) + else + return false + end end + true end def add_moderator_post(user, text, opts={}) @@ -362,6 +372,8 @@ class Topic < ActiveRecord::Base # Changes the category to a new name def change_category(name) + self.do_category_validation = true + # If the category name is blank, reset the attribute if name.blank? if category_id.present? @@ -369,12 +381,11 @@ class Topic < ActiveRecord::Base Category.where(id: category_id).update_all 'topic_count = topic_count - 1' end self.category_id = nil - save - return + return save end cat = Category.where(name: name).first - return if cat == category + return true if cat == category changed_to_category(cat) end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index b8762586c..673d28087 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -925,6 +925,7 @@ en: category: can: 'can… ' none: '(no category)' + choose: 'Select a category…' edit: 'edit' edit_long: "Edit Category" edit_uncategorized: "Edit Uncategorized" diff --git a/spec/controllers/topics_controller_spec.rb b/spec/controllers/topics_controller_spec.rb index 7ab15c8f9..fcd9b2330 100644 --- a/spec/controllers/topics_controller_spec.rb +++ b/spec/controllers/topics_controller_spec.rb @@ -637,7 +637,7 @@ describe TopicsController do end it 'triggers a change of category' do - Topic.any_instance.expects(:change_category).with('incredible') + Topic.any_instance.expects(:change_category).with('incredible').returns(true) xhr :put, :update, topic_id: @topic.id, slug: @topic.title, category: 'incredible' end @@ -646,6 +646,24 @@ describe TopicsController do expect(response).not_to be_success end + it "returns errors with invalid categories" do + Topic.any_instance.expects(:change_category).returns(false) + xhr :put, :update, topic_id: @topic.id, slug: @topic.title, category: '' + expect(response).not_to be_success + end + + context "allow_uncategorized_topics is false" do + before do + SiteSetting.stubs(:allow_uncategorized_topics).returns(false) + end + + it "can add a category to an uncategorized topic" do + Topic.any_instance.expects(:change_category).with('incredible').returns(true) + xhr :put, :update, topic_id: @topic.id, slug: @topic.title, category: 'incredible' + response.should be_success + end + end + end end end diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 11103d163..1d9e6beb2 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -175,6 +175,38 @@ describe Topic do end + context 'category validation' do + context 'allow_uncategorized_topics is false' do + before do + SiteSetting.stubs(:allow_uncategorized_topics).returns(false) + end + + it "does not allow nil category" do + topic = Fabricate(:topic, category: nil) + topic.should_not be_valid + topic.errors[:category_id].should be_present + end + + it 'passes for topics with a category' do + Fabricate.build(:topic, category: Fabricate(:category)).should be_valid + end + end + + context 'allow_uncategorized_topics is true' do + before do + SiteSetting.stubs(:allow_uncategorized_topics).returns(true) + end + + it "passes for topics with nil category" do + Fabricate.build(:topic, category: nil).should be_valid + end + + it 'passes for topics with a category' do + Fabricate.build(:topic, category: Fabricate(:category)).should be_valid + end + end + end + context 'similar_to' do @@ -825,7 +857,18 @@ describe Topic do it "should lower the original category's topic count" do @category.topic_count.should == 0 end + end + context 'when allow_uncategorized_topics is false' do + before do + SiteSetting.stubs(:allow_uncategorized_topics).returns(false) + end + + let!(:topic) { Fabricate(:topic, category: Fabricate(:category)) } + + it 'returns false' do + topic.change_category('').should eq(false) # don't use "be_false" here because it would also match nil + end end describe 'when the category exists' do @@ -838,7 +881,6 @@ describe Topic do @topic.category_id.should be_blank @category.topic_count.should == 0 end - end end diff --git a/test/javascripts/models/composer_test.js b/test/javascripts/models/composer_test.js index b0c71193d..23de36fbf 100644 --- a/test/javascripts/models/composer_test.js +++ b/test/javascripts/models/composer_test.js @@ -186,5 +186,5 @@ test('initial category when uncategorized is allowed', function() { test('initial category when uncategorized is not allowed', function() { Discourse.SiteSettings.allow_uncategorized_topics = false; var composer = Discourse.Composer.open({action: 'createTopic', draftKey: 'asfd', draftSequence: 1}); - ok(composer.get('categoryId') !== undefined, "Not uncategorized by default"); + ok(composer.get('categoryId') === undefined, "Uncategorized by default. Must choose a category."); }); \ No newline at end of file