From 2901691e8766d7a22102e77e27a6dfb2c52ef9ae Mon Sep 17 00:00:00 2001 From: Kyle Zhao Date: Thu, 12 Jul 2018 22:51:08 -0400 Subject: [PATCH] FEATURE: per-category approval settings (#5778) - disallow moving topics to a category that requires topic approval --- .../routes/discovery-categories.js.es6 | 3 +- .../components/edit-category-settings.hbs | 14 ++++++ app/controllers/posts_controller.rb | 2 +- app/controllers/topics_controller.rb | 2 +- app/models/category.rb | 14 ++++++ config/locales/client.en.yml | 2 + lib/guardian/topic_guardian.rb | 6 +++ lib/new_post_manager.rb | 15 +++++- lib/post_revisor.rb | 2 +- spec/components/new_post_manager_spec.rb | 47 +++++++++++++++++++ spec/components/post_revisor_spec.rb | 17 +++++++ spec/models/category_spec.rb | 21 +++++++++ spec/requests/categories_controller_spec.rb | 20 ++++++++ spec/requests/posts_controller_spec.rb | 14 ++++++ spec/requests/topics_controller_spec.rb | 15 +++++- 15 files changed, 187 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/discourse/routes/discovery-categories.js.es6 b/app/assets/javascripts/discourse/routes/discovery-categories.js.es6 index ea4c03a607d..8356f6fb82e 100644 --- a/app/assets/javascripts/discourse/routes/discovery-categories.js.es6 +++ b/app/assets/javascripts/discourse/routes/discovery-categories.js.es6 @@ -124,7 +124,8 @@ const DiscoveryCategoriesRoute = Discourse.Route.extend(OpenComposer, { group_permissions: [{ group_name: everyoneName, permission_type: 1 }], available_groups: groups.map(g => g.name), allow_badges: true, - topic_featured_link_allowed: true + topic_featured_link_allowed: true, + custom_fields: {} }); showModal("edit-category", { model }); diff --git a/app/assets/javascripts/discourse/templates/components/edit-category-settings.hbs b/app/assets/javascripts/discourse/templates/components/edit-category-settings.hbs index 36e6bfc0b4c..e08271dfff4 100644 --- a/app/assets/javascripts/discourse/templates/components/edit-category-settings.hbs +++ b/app/assets/javascripts/discourse/templates/components/edit-category-settings.hbs @@ -161,4 +161,18 @@ {{/if}} +
+ +
+ +
+ +
+ {{plugin-outlet name="category-custom-settings" args=(hash category=category)}} diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index f032286b208..6cb8aa6f4b1 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -204,7 +204,7 @@ class PostsController < ApplicationController if changes[:category_id] && changes[:category_id].to_i != post.topic.category_id.to_i category = Category.find_by(id: changes[:category_id]) if category || (changes[:category_id].to_i == 0) - guardian.ensure_can_create_topic_on_category!(category) + guardian.ensure_can_move_topic_to_category!(category) else return render_json_error(I18n.t('category.errors.not_found')) end diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 758201946bb..76059440cdc 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -259,7 +259,7 @@ class TopicsController < ApplicationController if params[:category_id] && (params[:category_id].to_i != topic.category_id.to_i) category = Category.find_by(id: params[:category_id]) if category || (params[:category_id].to_i == 0) - guardian.ensure_can_create_topic_on_category!(category) + guardian.ensure_can_move_topic_to_category!(category) else return render_json_error(I18n.t('category.errors.not_found')) end diff --git a/app/models/category.rb b/app/models/category.rb index 067037e1dc9..426d901bfb2 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -7,6 +7,12 @@ class Category < ActiveRecord::Base include CategoryHashtag include AnonCacheInvalidator + REQUIRE_TOPIC_APPROVAL = 'require_topic_approval' + REQUIRE_REPLY_APPROVAL = 'require_reply_approval' + + register_custom_field_type(REQUIRE_TOPIC_APPROVAL, :boolean) + register_custom_field_type(REQUIRE_REPLY_APPROVAL, :boolean) + belongs_to :topic, dependent: :destroy belongs_to :topic_only_relative_url, -> { select "id, title, slug" }, @@ -351,6 +357,14 @@ class Category < ActiveRecord::Base [read_restricted, mapped] end + def require_topic_approval? + custom_fields[REQUIRE_TOPIC_APPROVAL] + end + + def require_reply_approval? + custom_fields[REQUIRE_REPLY_APPROVAL] + end + def allowed_tags=(tag_names_arg) DiscourseTagging.add_or_create_tags_by_name(self, tag_names_arg, unlimited: true) end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 888bd02675f..29db1a3a5e1 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -2280,6 +2280,8 @@ en: allow_badges_label: "Allow badges to be awarded in this category" edit_permissions: "Edit Permissions" add_permission: "Add Permission" + require_topic_approval: "Require moderator approval of all new topics" + require_reply_approval: "Require moderator approval of all new replies" this_year: "this year" position: "position" default_position: "Default Position" diff --git a/lib/guardian/topic_guardian.rb b/lib/guardian/topic_guardian.rb index d3b60521806..cd915f5fe35 100644 --- a/lib/guardian/topic_guardian.rb +++ b/lib/guardian/topic_guardian.rb @@ -34,6 +34,12 @@ module TopicGuardian (!category || Category.topic_create_allowed(self).where(id: category_id).count == 1) end + def can_move_topic_to_category?(category) + category = Category === category ? category : Category.find(category || SiteSetting.uncategorized_category_id) + + is_staff? || (can_create_topic_on_category?(category) && !category.require_topic_approval?) + end + def can_create_post_on_topic?(topic) # No users can create posts on deleted topics return false if topic.blank? diff --git a/lib/new_post_manager.rb b/lib/new_post_manager.rb index b6965bc221e..6fe08f93c41 100644 --- a/lib/new_post_manager.rb +++ b/lib/new_post_manager.rb @@ -82,7 +82,20 @@ class NewPostManager is_fast_typer?(manager) || matches_auto_silence_regex?(manager) || WordWatcher.new("#{manager.args[:title]} #{manager.args[:raw]}").requires_approval? || - (SiteSetting.approve_unless_staged && user.staged) + (SiteSetting.approve_unless_staged && user.staged) || + post_needs_approval_in_its_category?(manager) + end + + def self.post_needs_approval_in_its_category?(manager) + if manager.args[:topic_id].present? + cat = Category.joins(:topics).find_by(topics: { id: manager.args[:topic_id] }) + return false unless cat + cat.require_reply_approval? + elsif manager.args[:category].present? + Category.find(manager.args[:category]).require_topic_approval? + else + false + end end def self.default_handler(manager) diff --git a/lib/post_revisor.rb b/lib/post_revisor.rb index b406351256c..8148b110040 100644 --- a/lib/post_revisor.rb +++ b/lib/post_revisor.rb @@ -69,7 +69,7 @@ class PostRevisor end track_topic_field(:category_id) do |tc, category_id| - if category_id == 0 || tc.guardian.can_create_topic_on_category?(category_id) + if category_id == 0 || tc.guardian.can_move_topic_to_category?(category_id) tc.record_change('category_id', tc.topic.category_id, category_id) tc.check_result(tc.topic.change_category_to_id(category_id)) end diff --git a/spec/components/new_post_manager_spec.rb b/spec/components/new_post_manager_spec.rb index 22218a67dc5..347adbb132a 100644 --- a/spec/components/new_post_manager_spec.rb +++ b/spec/components/new_post_manager_spec.rb @@ -281,4 +281,51 @@ describe NewPostManager do end end + context 'when posting in the category requires approval' do + let(:user) { Fabricate(:user) } + let(:category) { Fabricate(:category) } + + context 'when new topics require approval' do + before do + category.custom_fields[Category::REQUIRE_TOPIC_APPROVAL] = true + category.save + end + + it 'enqueues new topics' do + manager = NewPostManager.new( + user, + raw: 'this is a new topic', + title: "Let's start a new topic!", + category: category.id + ) + + expect(manager.perform.action).to eq(:enqueued) + end + end + + context 'when new posts require approval' do + let(:topic) { Fabricate(:topic, category: category) } + + before do + category.custom_fields[Category::REQUIRE_REPLY_APPROVAL] = true + category.save + end + + it 'enqueues new posts' do + manager = NewPostManager.new(user, raw: 'this is a new post', topic_id: topic.id) + expect(manager.perform.action).to eq(:enqueued) + end + + it "doesn't blow up with invalid topic_id" do + expect do + manager = NewPostManager.new( + user, + raw: 'this is a new topic', + topic_id: 97546 + ) + expect(manager.perform.action).to eq(:create_post) + end.not_to raise_error + end + end + end end diff --git a/spec/components/post_revisor_spec.rb b/spec/components/post_revisor_spec.rb index 4734d94cec0..5fd997bd43b 100644 --- a/spec/components/post_revisor_spec.rb +++ b/spec/components/post_revisor_spec.rb @@ -62,6 +62,23 @@ describe PostRevisor do expect(post.topic.category_id).to eq(category.id) end + it 'does not revise category when the destination category requires topic approval' do + new_category = Fabricate(:category) + new_category.custom_fields[Category::REQUIRE_TOPIC_APPROVAL] = true + new_category.save! + + post = create_post + old_category_id = post.topic.category_id + + post.revise(post.user, category_id: new_category.id) + expect(post.reload.topic.category_id).to eq(old_category_id) + + new_category.custom_fields[Category::REQUIRE_TOPIC_APPROVAL] = false + new_category.save! + + post.revise(post.user, category_id: new_category.id) + expect(post.reload.topic.category_id).to eq(new_category.id) + end end context 'revise wiki' do diff --git a/spec/models/category_spec.rb b/spec/models/category_spec.rb index 51992b1ce56..688035ebc86 100644 --- a/spec/models/category_spec.rb +++ b/spec/models/category_spec.rb @@ -663,4 +663,25 @@ describe Category do end + describe 'require topic/post approval' do + let(:category) { Fabricate(:category) } + + describe '#require_topic_approval?' do + before do + category.custom_fields[Category::REQUIRE_TOPIC_APPROVAL] = true + category.save + end + + it { expect(category.reload.require_topic_approval?).to eq(true) } + end + + describe '#require_reply_approval?' do + before do + category.custom_fields[Category::REQUIRE_REPLY_APPROVAL] = true + category.save + end + + it { expect(category.reload.require_reply_approval?).to eq(true) } + end + end end diff --git a/spec/requests/categories_controller_spec.rb b/spec/requests/categories_controller_spec.rb index e3a76cacbf8..dbc7c250357 100644 --- a/spec/requests/categories_controller_spec.rb +++ b/spec/requests/categories_controller_spec.rb @@ -296,6 +296,26 @@ describe CategoriesController do expect(response.status).to eq(200) expect(UserHistory.count).to eq(5) # 2 + 3 (bootstrap mode) end + + it 'updates per-category approval settings correctly' do + category.custom_fields[Category::REQUIRE_TOPIC_APPROVAL] = false + category.custom_fields[Category::REQUIRE_REPLY_APPROVAL] = false + category.save! + + put "/categories/#{category.id}.json", params: { + name: category.name, + color: category.color, + text_color: category.text_color, + custom_fields: { + require_reply_approval: true, + require_topic_approval: true, + } + } + + category.reload + expect(category.require_topic_approval?).to eq(true) + expect(category.require_reply_approval?).to eq(true) + end end end end diff --git a/spec/requests/posts_controller_spec.rb b/spec/requests/posts_controller_spec.rb index 90090937248..22dd8c25829 100644 --- a/spec/requests/posts_controller_spec.rb +++ b/spec/requests/posts_controller_spec.rb @@ -340,6 +340,20 @@ describe PostsController do expect(response.status).not_to eq(200) expect(post.topic.category_id).not_to eq(category.id) end + + it 'can not move to a category that requires topic approval' do + post = create_post + sign_in(post.user) + + category = Fabricate(:category) + category.custom_fields[Category::REQUIRE_TOPIC_APPROVAL] = true + category.save! + + put "/posts/#{post.id}.json", params: { post: { category_id: category.id, raw: "this is a test edit to post" } } + + expect(response.status).to eq(403) + expect(post.topic.reload.category_id).not_to eq(category.id) + end end describe '#bookmark' do diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index a7df964eb25..fb9421e7505 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -671,8 +671,19 @@ RSpec.describe TopicsController do put "/t/#{topic.id}.json", params: { category_id: category.id } - expect(response.status).not_to eq(200) - expect(topic.category_id).not_to eq(category.id) + expect(response.status).to eq(403) + expect(topic.reload.category_id).not_to eq(category.id) + end + + it 'can not move to a category that requires topic approval' do + category = Fabricate(:category) + category.custom_fields[Category::REQUIRE_TOPIC_APPROVAL] = true + category.save! + + put "/t/#{topic.id}.json", params: { category_id: category.id } + + expect(response.status).to eq(403) + expect(topic.reload.category_id).not_to eq(category.id) end describe 'without permission' do