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/assets/javascripts/discourse/app/components/edit-category-settings.hbs

@@ -128,7 +128,7 @@
     <label>
       <Input
         @type="checkbox"
-        @checked={{this.category.custom_fields.require_topic_approval}}
+        @checked={{this.category.category_setting.require_topic_approval}}
       />
       {{i18n "category.require_topic_approval"}}
     </label>
@@ -138,7 +138,7 @@
     <label>
       <Input
         @type="checkbox"
-        @checked={{this.category.custom_fields.require_reply_approval}}
+        @checked={{this.category.category_setting.require_reply_approval}}
       />
       {{i18n "category.require_reply_approval"}}
     </label>

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: [],

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)

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

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

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
 

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)

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

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

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
 

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,
                 },
               }

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",

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 }