From f08c6d27564b3f652f35d33a13a66a943a3a3742 Mon Sep 17 00:00:00 2001 From: Ted Johansson Date: Tue, 12 Sep 2023 09:51:49 +0800 Subject: [PATCH] DEV: Switch over category settings to new table - Part 3 (#20657) In #20135 we prevented invalid inputs from being accepted in category setting form fields on the front-end. We didn't do anything on the back-end at that time, because we were still discussing which path we wanted to take. Eventually we decided we want to move this to a new CategorySetting model. This PR moves the require_topic_approval and require_reply_approval from custom fields to the new CategorySetting model. This PR is nearly identical to #20580, which migrated num_auto_bump_daily, but since these are slightly more sensitive, they are moved after the previous one is verified. --- .../app/components/edit-category-settings.hbs | 4 +- app/controllers/categories_controller.rb | 7 +- app/models/category.rb | 20 ++--- app/models/category_setting.rb | 4 +- ...update_category_setting_approval_values.rb | 74 +++++++++++++++++++ spec/lib/new_post_manager_spec.rb | 6 +- spec/lib/post_revisor_spec.rb | 4 +- spec/lib/topic_view_spec.rb | 18 +++-- ...e_category_setting_approval_values_spec.rb | 54 ++++++++++++++ spec/models/category_spec.rb | 26 +++---- spec/requests/categories_controller_spec.rb | 8 +- spec/requests/posts_controller_spec.rb | 2 +- spec/requests/topics_controller_spec.rb | 2 +- 13 files changed, 178 insertions(+), 51 deletions(-) create mode 100644 db/migrate/20230910021213_update_category_setting_approval_values.rb create mode 100644 spec/migrations/uppdate_category_setting_approval_values_spec.rb diff --git a/app/assets/javascripts/discourse/app/components/edit-category-settings.hbs b/app/assets/javascripts/discourse/app/components/edit-category-settings.hbs index cd027fcb60b..f9482975a96 100644 --- a/app/assets/javascripts/discourse/app/components/edit-category-settings.hbs +++ b/app/assets/javascripts/discourse/app/components/edit-category-settings.hbs @@ -128,7 +128,7 @@ @@ -138,7 +138,7 @@ diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb index 7765ffc5d4a..28b312cfb8b 100644 --- a/app/controllers/categories_controller.rb +++ b/app/controllers/categories_controller.rb @@ -411,7 +411,12 @@ class CategoriesController < ApplicationController :read_only_banner, :default_list_filter, :reviewable_by_group_id, - category_setting_attributes: %i[auto_bump_cooldown_days num_auto_bump_daily], + category_setting_attributes: %i[ + auto_bump_cooldown_days + num_auto_bump_daily + require_reply_approval + require_topic_approval + ], custom_fields: [custom_field_params], permissions: [*p.try(:keys)], allowed_tags: [], diff --git a/app/models/category.rb b/app/models/category.rb index a834d3fd1ed..92e888ebd73 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -16,14 +16,8 @@ class Category < ActiveRecord::Base include AnonCacheInvalidator include HasDestroyedWebHook - REQUIRE_TOPIC_APPROVAL = "require_topic_approval" - REQUIRE_REPLY_APPROVAL = "require_reply_approval" - NUM_AUTO_BUMP_DAILY = "num_auto_bump_daily" SLUG_REF_SEPARATOR = ":" - register_custom_field_type(REQUIRE_TOPIC_APPROVAL, :boolean) - register_custom_field_type(REQUIRE_REPLY_APPROVAL, :boolean) - belongs_to :topic belongs_to :topic_only_relative_url, -> { select "id, title, slug" }, @@ -51,6 +45,12 @@ class Category < ActiveRecord::Base delegate :auto_bump_cooldown_days, :num_auto_bump_daily, :num_auto_bump_daily=, + :require_reply_approval, + :require_reply_approval=, + :require_reply_approval?, + :require_topic_approval, + :require_topic_approval=, + :require_topic_approval?, to: :category_setting, allow_nil: true @@ -671,14 +671,6 @@ 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 auto_bump_limiter return nil if num_auto_bump_daily.to_i == 0 RateLimiter.new(nil, "auto_bump_limit_#{self.id}", 1, 86_400 / num_auto_bump_daily.to_i) diff --git a/app/models/category_setting.rb b/app/models/category_setting.rb index 8c70964e567..95eb4a215d1 100644 --- a/app/models/category_setting.rb +++ b/app/models/category_setting.rb @@ -24,8 +24,8 @@ end # # id :bigint not null, primary key # category_id :bigint not null -# require_topic_approval :boolean -# require_reply_approval :boolean +# require_topic_approval :boolean default(FALSE) +# require_reply_approval :boolean default(FALSE) # num_auto_bump_daily :integer default(0) # created_at :datetime not null # updated_at :datetime not null diff --git a/db/migrate/20230910021213_update_category_setting_approval_values.rb b/db/migrate/20230910021213_update_category_setting_approval_values.rb new file mode 100644 index 00000000000..bd287840b9b --- /dev/null +++ b/db/migrate/20230910021213_update_category_setting_approval_values.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true + +class UpdateCategorySettingApprovalValues < ActiveRecord::Migration[7.0] + def up + change_column_default :category_settings, :require_topic_approval, false + change_column_default :category_settings, :require_reply_approval, false + + execute(<<~SQL) + WITH custom_fields AS ( + SELECT + category_id, + MAX( + CASE WHEN (name = 'require_topic_approval') + THEN NULLIF(value, '') ELSE NULL END + )::boolean AS require_topic_approval, + MAX( + CASE WHEN (name = 'require_reply_approval') + THEN NULLIF(value, '') ELSE NULL END + )::boolean AS require_reply_approval, + NOW() AS created_at, + NOW() AS updated_at + FROM category_custom_fields + WHERE name IN ( + 'require_topic_approval', + 'require_reply_approval' + ) + GROUP BY category_id + ) + INSERT INTO + category_settings( + category_id, + require_topic_approval, + require_reply_approval, + created_at, + updated_at + ) + SELECT * FROM custom_fields + ON CONFLICT (category_id) DO + UPDATE SET + require_topic_approval = EXCLUDED.require_topic_approval, + require_reply_approval = EXCLUDED.require_reply_approval, + updated_at = NOW() + SQL + + execute(<<~SQL) + UPDATE category_settings + SET require_topic_approval = false + WHERE require_topic_approval IS NULL; + SQL + + execute(<<~SQL) + UPDATE category_settings + SET require_reply_approval = false + WHERE require_reply_approval IS NULL; + SQL + end + + def down + change_column_default :category_settings, :require_topic_approval, nil + change_column_default :category_settings, :require_reply_approval, nil + + execute(<<~SQL) + UPDATE category_settings + SET require_topic_approval = NULL + WHERE require_topic_approval = false; + SQL + + execute(<<~SQL) + UPDATE category_settings + SET require_reply_approval = NULL + WHERE require_reply_approval = false; + SQL + end +end diff --git a/spec/lib/new_post_manager_spec.rb b/spec/lib/new_post_manager_spec.rb index 7edc8c61080..c9daea2fadb 100644 --- a/spec/lib/new_post_manager_spec.rb +++ b/spec/lib/new_post_manager_spec.rb @@ -60,7 +60,7 @@ RSpec.describe NewPostManager do tag3 = Fabricate(:tag) tag_group = Fabricate(:tag_group, tags: [tag2]) category = Fabricate(:category, tags: [tag1], tag_groups: [tag_group]) - category.custom_fields[Category::REQUIRE_TOPIC_APPROVAL] = true + category.require_topic_approval = true category.save! manager = @@ -498,7 +498,7 @@ RSpec.describe NewPostManager do context "when new topics require approval" do before do SiteSetting.tagging_enabled = true - category.custom_fields[Category::REQUIRE_TOPIC_APPROVAL] = true + category.require_topic_approval = true category.save end @@ -625,7 +625,7 @@ RSpec.describe NewPostManager do let!(:topic) { Fabricate(:topic, category: category) } before do - category.custom_fields[Category::REQUIRE_REPLY_APPROVAL] = true + category.require_reply_approval = true category.save end diff --git a/spec/lib/post_revisor_spec.rb b/spec/lib/post_revisor_spec.rb index 83a471798d3..13aa0bbabbf 100644 --- a/spec/lib/post_revisor_spec.rb +++ b/spec/lib/post_revisor_spec.rb @@ -83,7 +83,7 @@ RSpec.describe PostRevisor do 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.require_topic_approval = true new_category.save! post = create_post @@ -92,7 +92,7 @@ RSpec.describe PostRevisor do 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.require_topic_approval = false new_category.save! post.revise(post.user, category_id: new_category.id) diff --git a/spec/lib/topic_view_spec.rb b/spec/lib/topic_view_spec.rb index e05cbcebc53..d845c841238 100644 --- a/spec/lib/topic_view_spec.rb +++ b/spec/lib/topic_view_spec.rb @@ -1004,9 +1004,14 @@ RSpec.describe TopicView do describe "#reviewable_counts" do it "exclude posts queued because the category needs approval" do - category = Fabricate.build(:category, user: admin) - category.custom_fields[Category::REQUIRE_TOPIC_APPROVAL] = true - category.save! + category = + Fabricate.create( + :category, + user: admin, + category_setting_attributes: { + require_topic_approval: true, + }, + ) manager = NewPostManager.new( user, @@ -1081,13 +1086,16 @@ RSpec.describe TopicView do let(:queue_enabled) { false } context "when category is moderated" do - before { category.custom_fields[Category::REQUIRE_REPLY_APPROVAL] = true } + before do + category.require_reply_approval = true + category.save! + end it { expect(topic_view.queued_posts_enabled?).to be(true) } end context "when category is not moderated" do - it { expect(topic_view.queued_posts_enabled?).to be(nil) } + it { expect(topic_view.queued_posts_enabled?).to be(false) } end end end diff --git a/spec/migrations/uppdate_category_setting_approval_values_spec.rb b/spec/migrations/uppdate_category_setting_approval_values_spec.rb new file mode 100644 index 00000000000..4a1af1a7408 --- /dev/null +++ b/spec/migrations/uppdate_category_setting_approval_values_spec.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +require Rails.root.join("db/migrate/20230910021213_update_category_setting_approval_values.rb") + +RSpec.describe UpdateCategorySettingApprovalValues do + describe "#up" do + context "when require_topic_approval is null" do + let(:category) do + Fabricate(:category, category_setting_attributes: { require_topic_approval: nil }) + end + + it "backfills with false (new default)" do + expect { described_class.new.up }.to change { category.reload.require_topic_approval }.from( + nil, + ).to(false) + end + end + + context "when the category has no category setting" do + before do + category.category_setting.destroy! + CategoryCustomField.create!( + category: category, + name: "require_topic_approval", + value: "true", + ) + end + + let(:category) { Fabricate(:category) } + + it "backfills with the custom field value" do + expect { described_class.new.up }.to change { category.reload.category_setting }.from( + nil, + ).to(have_attributes(require_topic_approval: true)) + end + end + + context "when the category has a category setting and the custom field changed" do + before do + CategoryCustomField.create!(category: category, name: "require_topic_approval", value: true) + end + + let(:category) do + Fabricate(:category, category_setting_attributes: { require_topic_approval: false }) + end + + it "backfills with the custom field value" do + expect { described_class.new.up }.to change { + category.category_setting.reload.require_topic_approval + }.from(false).to(true) + end + end + end +end diff --git a/spec/models/category_spec.rb b/spec/models/category_spec.rb index e540433aee9..60bb83e63f9 100644 --- a/spec/models/category_spec.rb +++ b/spec/models/category_spec.rb @@ -906,22 +906,18 @@ RSpec.describe Category do describe "require topic/post approval" do fab!(:category) { Fabricate(:category_with_definition) } - describe "#require_topic_approval?" do - before do - category.custom_fields[Category::REQUIRE_TOPIC_APPROVAL] = true - category.save - end + it "delegates methods to category settings" do + expect(category).to delegate_method(:require_reply_approval).to(:category_setting) + expect(category).to delegate_method(:require_reply_approval=).with_arguments(true).to( + :category_setting, + ) + expect(category).to delegate_method(:require_reply_approval?).to(:category_setting) - 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) } + expect(category).to delegate_method(:require_topic_approval).to(:category_setting) + expect(category).to delegate_method(:require_topic_approval=).with_arguments(true).to( + :category_setting, + ) + expect(category).to delegate_method(:require_topic_approval?).to(:category_setting) end end diff --git a/spec/requests/categories_controller_spec.rb b/spec/requests/categories_controller_spec.rb index f020e0662f8..062dc86917c 100644 --- a/spec/requests/categories_controller_spec.rb +++ b/spec/requests/categories_controller_spec.rb @@ -714,8 +714,8 @@ RSpec.describe CategoriesController do end it "updates per-category settings correctly" do - category.custom_fields[Category::REQUIRE_TOPIC_APPROVAL] = false - category.custom_fields[Category::REQUIRE_REPLY_APPROVAL] = false + category.require_topic_approval = false + category.require_reply_approval = false category.navigate_to_first_post_after_read = false category.save! @@ -726,11 +726,9 @@ RSpec.describe CategoriesController do color: category.color, text_color: category.text_color, navigate_to_first_post_after_read: true, - custom_fields: { + category_setting_attributes: { require_reply_approval: true, require_topic_approval: true, - }, - category_setting_attributes: { num_auto_bump_daily: 10, }, } diff --git a/spec/requests/posts_controller_spec.rb b/spec/requests/posts_controller_spec.rb index c641f1b2dd4..6959615dadb 100644 --- a/spec/requests/posts_controller_spec.rb +++ b/spec/requests/posts_controller_spec.rb @@ -628,7 +628,7 @@ RSpec.describe PostsController do sign_in(post.user) category = Fabricate(:category) - category.custom_fields[Category::REQUIRE_TOPIC_APPROVAL] = true + category.require_topic_approval = true category.save! put "/posts/#{post.id}.json", diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index b772bf00d63..ea7db75a1eb 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -1599,7 +1599,7 @@ RSpec.describe TopicsController do end it "can not move to a category that requires topic approval" do - category.custom_fields[Category::REQUIRE_TOPIC_APPROVAL] = true + category.require_topic_approval = true category.save! put "/t/#{topic.id}.json", params: { category_id: category.id }