From 87ec058b8b0097ef35f3e88dbdca5554dfa65760 Mon Sep 17 00:00:00 2001 From: Ted Johansson Date: Fri, 10 Mar 2023 13:45:01 +0800 Subject: [PATCH] FEATURE: Configurable auto-bump cooldown (#20507) Currently the auto-bump cooldown is hard-coded to 24 hours. This change makes the highlighted 24 hours part configurable (defaulting to 24 hours), and the rest of the process remains the same. This uses the new CategorySetting model associated with Category. We decided to add this because we want to move away from custom fields due to the lack of type casting and validations, but we want to keep the loading of these optional as they are not needed for almost all of the flows. Category settings will be back-filled to all categories as part of this change, and creating a new category will now also create a category setting. --- .../app/components/edit-category-settings.hbs | 12 ++++++ .../discourse/app/models/category.js | 1 + .../discourse/app/routes/new-category.js | 1 + app/controllers/categories_controller.rb | 4 +- app/models/category.rb | 11 +++++- app/models/category_setting.rb | 22 +++++++---- app/serializers/category_serializer.rb | 13 +++++++ config/locales/client.en.yml | 1 + ...bump_cooldown_days_to_category_settings.rb | 7 ++++ ...uto_bump_cooldown_days_category_setting.rb | 26 +++++++++++++ spec/models/category_setting_spec.rb | 7 ++++ spec/models/category_spec.rb | 38 +++++++++++++++++++ .../json/category_create_response.json | 6 +++ .../json/category_update_response.json | 6 +++ 14 files changed, 146 insertions(+), 9 deletions(-) create mode 100644 db/migrate/20230301071240_add_auto_bump_cooldown_days_to_category_settings.rb create mode 100644 db/migrate/20230308042434_backfill_auto_bump_cooldown_days_category_setting.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 647ce9c3fd5..2a36e0531d3 100644 --- a/app/assets/javascripts/discourse/app/components/edit-category-settings.hbs +++ b/app/assets/javascripts/discourse/app/components/edit-category-settings.hbs @@ -193,6 +193,18 @@ @min="0" /> + +
+ + +
diff --git a/app/assets/javascripts/discourse/app/models/category.js b/app/assets/javascripts/discourse/app/models/category.js index 8b3d3d7d96c..839fa6e7d22 100644 --- a/app/assets/javascripts/discourse/app/models/category.js +++ b/app/assets/javascripts/discourse/app/models/category.js @@ -219,6 +219,7 @@ const Category = RestModel.extend({ uploaded_logo_dark_id: this.get("uploaded_logo_dark.id"), uploaded_background_id: this.get("uploaded_background.id"), allow_badges: this.allow_badges, + category_setting_attributes: this.category_setting, custom_fields: this.custom_fields, topic_template: this.topic_template, form_template_ids: this.form_template_ids, diff --git a/app/assets/javascripts/discourse/app/routes/new-category.js b/app/assets/javascripts/discourse/app/routes/new-category.js index 2fb6237719e..7dbead0fad1 100644 --- a/app/assets/javascripts/discourse/app/routes/new-category.js +++ b/app/assets/javascripts/discourse/app/routes/new-category.js @@ -31,6 +31,7 @@ export default DiscourseRoute.extend({ allow_badges: true, topic_featured_link_allowed: true, custom_fields: {}, + category_setting: {}, search_priority: SEARCH_PRIORITIES.normal, required_tag_groups: [], form_template_ids: [], diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb index 7c450962caf..0bf524f9652 100644 --- a/app/controllers/categories_controller.rb +++ b/app/controllers/categories_controller.rb @@ -258,7 +258,8 @@ class CategoriesController < ApplicationController def find_by_slug params.require(:category_slug) - @category = Category.find_by_slug_path(params[:category_slug].split("/")) + @category = + Category.includes(:category_setting).find_by_slug_path(params[:category_slug].split("/")) raise Discourse::NotFound unless @category.present? @@ -405,6 +406,7 @@ class CategoriesController < ApplicationController :read_only_banner, :default_list_filter, :reviewable_by_group_id, + category_setting_attributes: %i[auto_bump_cooldown_days], custom_fields: [custom_field_params], permissions: [*p.try(:keys)], allowed_tags: [], diff --git a/app/models/category.rb b/app/models/category.rb index c568107da45..32397ee6aa3 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -48,8 +48,12 @@ class Category < ActiveRecord::Base has_one :category_setting, dependent: :destroy + delegate :auto_bump_cooldown_days, to: :category_setting, allow_nil: true + has_and_belongs_to_many :web_hooks + accepts_nested_attributes_for :category_setting, update_only: true + validates :user_id, presence: true validates :name, @@ -96,6 +100,7 @@ class Category < ActiveRecord::Base before_save :apply_permissions before_save :downcase_email before_save :downcase_name + before_save :ensure_category_setting after_save :publish_discourse_stylesheet after_save :publish_category @@ -682,7 +687,7 @@ class Category < ActiveRecord::Base .exclude_scheduled_bump_topics .where(category_id: self.id) .where("id <> ?", self.topic_id) - .where("bumped_at < ?", 1.day.ago) + .where("bumped_at < ?", (self.auto_bump_cooldown_days || 1).days.ago) .where("pinned_at IS NULL AND NOT closed AND NOT archived") .order("bumped_at ASC") .limit(1) @@ -1040,6 +1045,10 @@ class Category < ActiveRecord::Base private + def ensure_category_setting + self.build_category_setting if self.category_setting.blank? + end + def should_update_reviewables? SiteSetting.enable_category_group_moderation? && saved_change_to_reviewable_by_group_id? end diff --git a/app/models/category_setting.rb b/app/models/category_setting.rb index 3295564ec5a..4506d634af8 100644 --- a/app/models/category_setting.rb +++ b/app/models/category_setting.rb @@ -9,19 +9,27 @@ class CategorySetting < ActiveRecord::Base greater_than_or_equal_to: 0, allow_nil: true, } + + validates :auto_bump_cooldown_days, + numericality: { + only_integer: true, + greater_than_or_equal_to: 0, + allow_nil: true, + } end # == Schema Information # # Table name: category_settings # -# id :bigint not null, primary key -# category_id :bigint not null -# require_topic_approval :boolean -# require_reply_approval :boolean -# num_auto_bump_daily :integer -# created_at :datetime not null -# updated_at :datetime not null +# id :bigint not null, primary key +# category_id :bigint not null +# require_topic_approval :boolean +# require_reply_approval :boolean +# num_auto_bump_daily :integer +# created_at :datetime not null +# updated_at :datetime not null +# auto_bump_cooldown_days :integer default(1) # Indexes # # index_category_settings_on_category_id (category_id) UNIQUE diff --git a/app/serializers/category_serializer.rb b/app/serializers/category_serializer.rb index d2bfbb16121..08b92ac6a5e 100644 --- a/app/serializers/category_serializer.rb +++ b/app/serializers/category_serializer.rb @@ -1,6 +1,13 @@ # frozen_string_literal: true class CategorySerializer < SiteCategorySerializer + class CategorySettingSerializer < ApplicationSerializer + attributes :auto_bump_cooldown_days, + :num_auto_bump_daily, + :require_reply_approval, + :require_topic_approval + end + attributes :read_restricted, :available_groups, :auto_close_hours, @@ -22,6 +29,8 @@ class CategorySerializer < SiteCategorySerializer :reviewable_by_group_name, :default_slow_mode_seconds + has_one :category_setting, serializer: CategorySettingSerializer, embed: :objects + def reviewable_by_group_name object.reviewable_by_group.name end @@ -30,6 +39,10 @@ class CategorySerializer < SiteCategorySerializer SiteSetting.enable_category_group_moderation? && object.reviewable_by_group_id.present? end + def include_category_setting? + object.association(:category_setting).loaded? + end + def group_permissions @group_permissions ||= begin diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index ec74c481de4..a55cc7b98cd 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -3740,6 +3740,7 @@ en: default_slow_mode: 'Enable "Slow Mode" for new topics in this category.' parent: "Parent Category" num_auto_bump_daily: "Number of open topics to automatically bump daily:" + auto_bump_cooldown_days: "Minimum days before bumping the same topic again:" navigate_to_first_post_after_read: "Navigate to first post after topics are read" notifications: title: "change notification level for this category" diff --git a/db/migrate/20230301071240_add_auto_bump_cooldown_days_to_category_settings.rb b/db/migrate/20230301071240_add_auto_bump_cooldown_days_to_category_settings.rb new file mode 100644 index 00000000000..0119f4dd8c4 --- /dev/null +++ b/db/migrate/20230301071240_add_auto_bump_cooldown_days_to_category_settings.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddAutoBumpCooldownDaysToCategorySettings < ActiveRecord::Migration[7.0] + def change + add_column :category_settings, :auto_bump_cooldown_days, :integer, default: 1 + end +end diff --git a/db/migrate/20230308042434_backfill_auto_bump_cooldown_days_category_setting.rb b/db/migrate/20230308042434_backfill_auto_bump_cooldown_days_category_setting.rb new file mode 100644 index 00000000000..f37c9117e24 --- /dev/null +++ b/db/migrate/20230308042434_backfill_auto_bump_cooldown_days_category_setting.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +class BackfillAutoBumpCooldownDaysCategorySetting < ActiveRecord::Migration[7.0] + def up + execute(<<~SQL) + INSERT INTO + category_settings( + category_id, + auto_bump_cooldown_days, + created_at, + updated_at + ) + SELECT + id, + 1, + NOW(), + NOW() + FROM categories + ON CONFLICT (category_id) + DO + UPDATE SET + auto_bump_cooldown_days = 1, + updated_at = NOW(); + SQL + end +end diff --git a/spec/models/category_setting_spec.rb b/spec/models/category_setting_spec.rb index fbcd6431336..c9bf1160a7d 100644 --- a/spec/models/category_setting_spec.rb +++ b/spec/models/category_setting_spec.rb @@ -9,4 +9,11 @@ RSpec.describe CategorySetting do .is_greater_than_or_equal_to(0) .allow_nil end + + it do + is_expected.to validate_numericality_of(:auto_bump_cooldown_days) + .only_integer + .is_greater_than_or_equal_to(0) + .allow_nil + end end diff --git a/spec/models/category_spec.rb b/spec/models/category_spec.rb index 73c501d1536..3157f3e4dbc 100644 --- a/spec/models/category_spec.rb +++ b/spec/models/category_spec.rb @@ -39,6 +39,10 @@ RSpec.describe Category do describe "Associations" do it { is_expected.to have_one(:category_setting).dependent(:destroy) } + it "automatically creates a category setting" do + expect { Fabricate(:category) }.to change { CategorySetting.count }.by(1) + end + it "should delete associated sidebar_section_links when category is destroyed" do category_sidebar_section_link = Fabricate(:category_sidebar_section_link) category_sidebar_section_link_2 = @@ -965,6 +969,40 @@ RSpec.describe Category do expect(Category.auto_bump_topic!).to eq(false) end + it "should not auto-bump the same topic within the cooldown" do + freeze_time + category = + Fabricate( + :category_with_definition, + num_auto_bump_daily: 2, + created_at: 1.minute.ago, + category_setting_attributes: { + auto_bump_cooldown_days: 1, + }, + ) + category.clear_auto_bump_cache! + + post1 = create_post(category: category, created_at: 15.seconds.ago) + + # no limits on post creation or category creation please + RateLimiter.enable + + time = freeze_time 1.month.from_now + + expect(category.auto_bump_topic!).to eq(true) + expect(Topic.where(bumped_at: time).count).to eq(1) + + time = freeze_time 13.hours.from_now + + expect(category.auto_bump_topic!).to eq(false) + expect(Topic.where(bumped_at: time).count).to eq(0) + + time = freeze_time 13.hours.from_now + + expect(category.auto_bump_topic!).to eq(true) + expect(Topic.where(bumped_at: time).count).to eq(1) + end + it "should not automatically bump topics with a bump scheduled" do freeze_time category = Fabricate(:category_with_definition, created_at: 1.second.ago) diff --git a/spec/requests/api/schemas/json/category_create_response.json b/spec/requests/api/schemas/json/category_create_response.json index 95b119111e9..a669e84aa74 100644 --- a/spec/requests/api/schemas/json/category_create_response.json +++ b/spec/requests/api/schemas/json/category_create_response.json @@ -160,6 +160,12 @@ ] } }, + "category_setting": { + "auto_bump_cooldown_days": 1, + "num_auto_bump_daily": null, + "require_reply_approval": null, + "require_topic_approval": null + }, "read_only_banner": { "type": [ "string", diff --git a/spec/requests/api/schemas/json/category_update_response.json b/spec/requests/api/schemas/json/category_update_response.json index 2570b4192c2..93cf445d60d 100644 --- a/spec/requests/api/schemas/json/category_update_response.json +++ b/spec/requests/api/schemas/json/category_update_response.json @@ -163,6 +163,12 @@ ] } }, + "category_setting": { + "auto_bump_cooldown_days": 1, + "num_auto_bump_daily": null, + "require_reply_approval": null, + "require_topic_approval": null + }, "read_only_banner": { "type": [ "string",