diff --git a/app/assets/javascripts/discourse/app/components/edit-category-tags.js b/app/assets/javascripts/discourse/app/components/edit-category-tags.js index f9dc2d914ba..b12a980ed7c 100644 --- a/app/assets/javascripts/discourse/app/components/edit-category-tags.js +++ b/app/assets/javascripts/discourse/app/components/edit-category-tags.js @@ -1,8 +1,29 @@ import { and, empty } from "@ember/object/computed"; import { buildCategoryPanel } from "discourse/components/edit-category-panel"; +import { action, set } from "@ember/object"; export default buildCategoryPanel("tags", { allowedTagsEmpty: empty("category.allowed_tags"), allowedTagGroupsEmpty: empty("category.allowed_tag_groups"), disableAllowGlobalTags: and("allowedTagsEmpty", "allowedTagGroupsEmpty"), + + @action + onTagGroupChange(rtg, valueArray) { + // A little strange, but we're using a multi-select component + // to select a single tag group. This action takes the array + // and extracts the first value in it. + set(rtg, "name", valueArray[0]); + }, + + @action + addRequiredTagGroup() { + this.category.required_tag_groups.pushObject({ + min_count: 1, + }); + }, + + @action + deleteRequiredTagGroup(rtg) { + this.category.required_tag_groups.removeObject(rtg); + }, }); diff --git a/app/assets/javascripts/discourse/app/controllers/composer.js b/app/assets/javascripts/discourse/app/controllers/composer.js index e45d761a98f..c57d70fa75d 100644 --- a/app/assets/javascripts/discourse/app/controllers/composer.js +++ b/app/assets/javascripts/discourse/app/controllers/composer.js @@ -1435,17 +1435,12 @@ export default Controller.extend({ tagValidation(category, tags, lastValidatedAt) { const tagsArray = tags || []; if (this.site.can_tag_topics && !this.currentUser.staff && category) { - if ( - category.minimum_required_tags > tagsArray.length || - (category.required_tag_groups && - category.min_tags_from_required_group > tagsArray.length) - ) { + // category.minimumRequiredTags incorporates both minimum_required_tags, and required_tag_groups + if (category.minimumRequiredTags > tagsArray.length) { return EmberObject.create({ failed: true, reason: I18n.t("composer.error.tags_missing", { - count: - category.minimum_required_tags || - category.min_tags_from_required_group, + count: category.minimumRequiredTags, }), lastShownAt: lastValidatedAt, }); diff --git a/app/assets/javascripts/discourse/app/models/category.js b/app/assets/javascripts/discourse/app/models/category.js index b5b5a751a9c..cab4fea6421 100644 --- a/app/assets/javascripts/discourse/app/models/category.js +++ b/app/assets/javascripts/discourse/app/models/category.js @@ -35,21 +35,13 @@ const Category = RestModel.extend({ } }, - @on("init") - setupRequiredTagGroups() { - if (this.required_tag_group_name) { - this.set("required_tag_groups", [this.required_tag_group_name]); - } - }, - - @discourseComputed( - "required_tag_groups", - "min_tags_from_required_group", - "minimum_required_tags" - ) + @discourseComputed("required_tag_groups", "minimum_required_tags") minimumRequiredTags() { - if (this.required_tag_groups) { - return this.min_tags_from_required_group; + if (this.required_tag_groups?.length > 0) { + return this.required_tag_groups.reduce( + (sum, rtg) => sum + rtg.min_count, + 0 + ); } else { return this.minimum_required_tags > 0 ? this.minimum_required_tags : null; } @@ -200,7 +192,8 @@ const Category = RestModel.extend({ const url = id ? `/categories/${id}` : "/categories"; return ajax(url, { - data: { + contentType: "application/json", + data: JSON.stringify({ name: this.name, slug: this.slug, color: this.color, @@ -234,11 +227,7 @@ const Category = RestModel.extend({ ? this.allowed_tag_groups : null, allow_global_tags: this.allow_global_tags, - required_tag_group_name: - this.required_tag_groups && this.required_tag_groups.length > 0 - ? this.required_tag_groups[0] - : null, - min_tags_from_required_group: this.min_tags_from_required_group, + required_tag_groups: this.required_tag_groups, sort_order: this.sort_order, sort_ascending: this.sort_ascending, topic_featured_link_allowed: this.topic_featured_link_allowed, @@ -255,7 +244,7 @@ const Category = RestModel.extend({ reviewable_by_group_name: this.reviewable_by_group_name, read_only_banner: this.read_only_banner, default_list_filter: this.default_list_filter, - }, + }), type: id ? "PUT" : "POST", }); }, diff --git a/app/assets/javascripts/discourse/app/templates/components/edit-category-tags.hbs b/app/assets/javascripts/discourse/app/templates/components/edit-category-tags.hbs index 13d1031a4bd..b093ec89e67 100644 --- a/app/assets/javascripts/discourse/app/templates/components/edit-category-tags.hbs +++ b/app/assets/javascripts/discourse/app/templates/components/edit-category-tags.hbs @@ -18,6 +18,7 @@ {{tag-group-chooser id="category-allowed-tag-groups" tagGroups=category.allowed_tag_groups + onChange=(action (mut category.allowed_tag_groups)) }} {{#link-to "tagGroups" class="manage-tag-groups"}}{{i18n "category.manage_tag_groups_link"}}{{/link-to}} @@ -34,23 +35,33 @@
- {{i18n "category.required_tag_group_description"}} + {{i18n "category.required_tag_group.description"}}
-
- - {{text-field value=category.min_tags_from_required_group id="category-min-tags-from-group" type="number" min="1"}} -
-
- - {{tag-group-chooser - id="category-required-tag-group" - tagGroups=category.required_tag_groups - options=(hash - maximum=1 - filterPlaceholder="category.tag_group_selector_placeholder" - ) - }} +
+ {{#each category.required_tag_groups as |rtg|}} +
+ {{text-field value=rtg.min_count type="number" min="1"}} + {{tag-group-chooser + tagGroups=(if rtg.name (array rtg.name) (array)) + onChange=(action "onTagGroupChange" rtg) + options=(hash + maximum=1 + filterPlaceholder="category.required_tag_group.placeholder" + ) + }} + {{d-button + label="category.required_tag_group.delete" + action=(action "deleteRequiredTagGroup" rtg) + icon="trash-alt" + class="delete-required-tag-group"}} +
+ {{/each}} + {{d-button + label="category.required_tag_group.add" + action=(action "addRequiredTagGroup") + icon="plus" + class="add-required-tag-group"}}
diff --git a/app/assets/javascripts/discourse/tests/acceptance/category-edit-test.js b/app/assets/javascripts/discourse/tests/acceptance/category-edit-test.js index 606daae3980..45275bed6b9 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/category-edit-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/category-edit-test.js @@ -1,5 +1,7 @@ import { acceptance, + count, + exists, queryAll, visible, } from "discourse/tests/helpers/qunit-helpers"; @@ -11,7 +13,7 @@ import { test } from "qunit"; acceptance("Category Edit", function (needs) { needs.user(); - needs.settings({ email_in: true }); + needs.settings({ email_in: true, tagging_enabled: true }); test("Editing the category", async function (assert) { await visit("/c/bug"); @@ -70,6 +72,31 @@ acceptance("Category Edit", function (needs) { ); }); + test("Editing required tag groups", async function (assert) { + await visit("/c/bug/edit/tags"); + + assert.ok(exists(".required-tag-groups")); + assert.strictEqual(count(".required-tag-group-row"), 0); + + await click(".add-required-tag-group"); + assert.strictEqual(count(".required-tag-group-row"), 1); + + await click(".add-required-tag-group"); + assert.strictEqual(count(".required-tag-group-row"), 2); + + await click(".delete-required-tag-group"); + assert.strictEqual(count(".required-tag-group-row"), 1); + + const tagGroupChooser = selectKit( + ".required-tag-group-row .tag-group-chooser" + ); + await tagGroupChooser.expand(); + await tagGroupChooser.selectRowByValue("TagGroup1"); + + await click("#save-category"); + assert.strictEqual(count(".required-tag-group-row"), 1); + }); + test("Index Route", async function (assert) { await visit("/c/bug/edit"); assert.strictEqual( diff --git a/app/assets/javascripts/discourse/tests/acceptance/composer-tags-test.js b/app/assets/javascripts/discourse/tests/acceptance/composer-tags-test.js index eeb74e7dace..1ed58bca8e2 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/composer-tags-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/composer-tags-test.js @@ -74,8 +74,7 @@ acceptance("Composer - Tags", function (needs) { await fillIn(".d-editor-input", "this is the *content* of a post"); Category.findById(2).setProperties({ - required_tag_groups: ["support tags"], - min_tags_from_required_group: 1, + required_tag_groups: [{ name: "support tags", min_count: 1 }], }); const categoryChooser = selectKit(".category-chooser"); diff --git a/app/assets/javascripts/discourse/tests/fixtures/site-fixtures.js b/app/assets/javascripts/discourse/tests/fixtures/site-fixtures.js index a28ca8800f6..cfc12850afe 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/site-fixtures.js +++ b/app/assets/javascripts/discourse/tests/fixtures/site-fixtures.js @@ -366,6 +366,7 @@ export default { can_edit: true, show_subcategory_list: false, default_view: "latest", + required_tag_groups: [], }, { id: 17, diff --git a/app/assets/javascripts/discourse/tests/fixtures/tag-group-fixtures.js b/app/assets/javascripts/discourse/tests/fixtures/tag-group-fixtures.js new file mode 100644 index 00000000000..07eb46530cc --- /dev/null +++ b/app/assets/javascripts/discourse/tests/fixtures/tag-group-fixtures.js @@ -0,0 +1,8 @@ +export default { + "/tag_groups/filter/search": { + results: [ + { name: "TagGroup1", tag_names: ["alpha", "bravo", "charlie"] }, + { name: "TagGroup2", tag_names: ["delta", "echo"] }, + ], + }, +}; diff --git a/app/assets/javascripts/discourse/tests/helpers/create-pretender.js b/app/assets/javascripts/discourse/tests/helpers/create-pretender.js index e362f54a620..158d2e10b90 100644 --- a/app/assets/javascripts/discourse/tests/helpers/create-pretender.js +++ b/app/assets/javascripts/discourse/tests/helpers/create-pretender.js @@ -356,7 +356,7 @@ export function applyDefaultHandlers(pretender) { ); pretender.put("/categories/:category_id", (request) => { - const category = parsePostData(request.requestBody); + const category = JSON.parse(request.requestBody); category.id = parseInt(request.params.category_id, 10); if (category.email_in === "duplicate@example.com") { @@ -1121,6 +1121,10 @@ export function applyDefaultHandlers(pretender) { ], }); }); + + pretender.get("/tag_groups/filter/search", () => + response(fixturesByUrl["/tag_groups/filter/search"]) + ); } export function resetPretender() { diff --git a/app/assets/javascripts/discourse/tests/unit/models/category-test.js b/app/assets/javascripts/discourse/tests/unit/models/category-test.js index 27c4193c801..e8e88e67e38 100644 --- a/app/assets/javascripts/discourse/tests/unit/models/category-test.js +++ b/app/assets/javascripts/discourse/tests/unit/models/category-test.js @@ -227,8 +227,7 @@ module("Unit | Model | category", function () { let foo = store.createRecord("category", { id: 1, slug: "foo", - required_tag_groups: ["bar"], - min_tags_from_required_group: 2, + required_tag_groups: [{ name: "bar", min_count: 2 }], }); assert.equal(foo.minimumRequiredTags, 2); @@ -259,7 +258,7 @@ module("Unit | Model | category", function () { foo = store.createRecord("category", { id: 5, slug: "foo", - min_tags_from_required_group: 2, + required_tag_groups: [], }); assert.equal(foo.minimumRequiredTags, null); diff --git a/app/assets/javascripts/select-kit/addon/components/tag-group-chooser.js b/app/assets/javascripts/select-kit/addon/components/tag-group-chooser.js index 5abf91625a2..8d2b8b999a2 100644 --- a/app/assets/javascripts/select-kit/addon/components/tag-group-chooser.js +++ b/app/assets/javascripts/select-kit/addon/components/tag-group-chooser.js @@ -28,12 +28,6 @@ export default MultiSelectComponent.extend(TagsMixin, { .map((t) => this.defaultItem(t, t)); }), - actions: { - onChange(value) { - this.set("tagGroups", value); - }, - }, - search(query) { const data = { q: query, diff --git a/app/assets/stylesheets/common/base/edit-category.scss b/app/assets/stylesheets/common/base/edit-category.scss index 0fcbd518230..45148d3798d 100644 --- a/app/assets/stylesheets/common/base/edit-category.scss +++ b/app/assets/stylesheets/common/base/edit-category.scss @@ -162,6 +162,23 @@ div.edit-category { .category-default-slow-mode-seconds { width: 200px; } + + .required-tag-groups { + .required-tag-group-row { + display: flex; + gap: 0.5em; + + > * { + margin: 0; + } + + input[type="number"] { + width: 4em; + } + + margin-bottom: 1em; + } + } } .category-permissions-table { diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb index 746bc2d3aec..204c6193d47 100644 --- a/app/controllers/categories_controller.rb +++ b/app/controllers/categories_controller.rb @@ -312,7 +312,7 @@ class CategoriesController < ApplicationController if SiteSetting.tagging_enabled params[:allowed_tags] = params[:allowed_tags].presence || [] if params[:allowed_tags] params[:allowed_tag_groups] = params[:allowed_tag_groups].presence || [] if params[:allowed_tag_groups] - params[:required_tag_group_name] = params[:required_tag_group_name].presence || '' if params[:required_tag_group_name] + params[:required_tag_groups] = params[:required_tag_groups].presence || [] if params[:required_tag_groups] end if SiteSetting.enable_category_group_moderation? @@ -351,8 +351,6 @@ class CategoriesController < ApplicationController :navigate_to_first_post_after_read, :search_priority, :allow_global_tags, - :required_tag_group_name, - :min_tags_from_required_group, :read_only_banner, :default_list_filter, :reviewable_by_group_id, @@ -360,8 +358,13 @@ class CategoriesController < ApplicationController permissions: [*p.try(:keys)], allowed_tags: [], allowed_tag_groups: [], + required_tag_groups: [:name, :min_count] ) + if result[:required_tag_groups] && !result[:required_tag_groups].is_a?(Array) + raise Discourse::InvalidParameters.new(:required_tag_groups) + end + result end end diff --git a/app/models/category.rb b/app/models/category.rb index 6a0e0d5f338..3702a03bd6f 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -5,10 +5,11 @@ class Category < ActiveRecord::Base 'none' ] - # TODO(2020-11-18): remove - self.ignored_columns = %w{ - suppress_from_latest - } + self.ignored_columns = [ + :suppress_from_latest, # TODO(2020-11-18): remove + :required_tag_group_id, # TODO(2023-04-01): remove + :min_tags_from_required_group, # TODO(2023-04-01): remove + ] include Searchable include Positionable @@ -56,7 +57,6 @@ class Category < ActiveRecord::Base validates :num_featured_topics, numericality: { only_integer: true, greater_than: 0 } validates :search_priority, inclusion: { in: Searchable::PRIORITIES.values } - validates :min_tags_from_required_group, numericality: { only_integer: true, greater_than: 0 } validate :parent_category_validator validate :email_in_validator @@ -103,7 +103,8 @@ class Category < ActiveRecord::Base has_many :tags, through: :category_tags has_many :category_tag_groups, dependent: :destroy has_many :tag_groups, through: :category_tag_groups - belongs_to :required_tag_group, class_name: 'TagGroup' + + has_many :category_required_tag_groups, -> { order(order: :asc) }, dependent: :destroy belongs_to :reviewable_by_group, class_name: 'Group' @@ -639,8 +640,14 @@ class Category < ActiveRecord::Base self.tag_groups = TagGroup.where(name: group_names).all.to_a end - def required_tag_group_name=(group_name) - self.required_tag_group = group_name.blank? ? nil : TagGroup.where(name: group_name).first + def required_tag_groups=(required_groups) + map = Array(required_groups).map.with_index { |rg, i| [rg["name"], { min_count: rg["min_count"].to_i, order: i }] }.to_h + tag_groups = TagGroup.where(name: map.keys) + + self.category_required_tag_groups = tag_groups.map do |tag_group| + attrs = map[tag_group.name] + CategoryRequiredTagGroup.new(tag_group: tag_group, **attrs) + end.sort_by(&:order) end def downcase_email @@ -1044,8 +1051,6 @@ end # search_priority :integer default(0) # allow_global_tags :boolean default(FALSE), not null # reviewable_by_group_id :integer -# required_tag_group_id :integer -# min_tags_from_required_group :integer default(1), not null # read_only_banner :string # default_list_filter :string(20) default("all") # allow_unlimited_owner_edits_on_first_post :boolean default(FALSE), not null diff --git a/app/models/category_required_tag_group.rb b/app/models/category_required_tag_group.rb new file mode 100644 index 00000000000..50d9ef6ab6a --- /dev/null +++ b/app/models/category_required_tag_group.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +class CategoryRequiredTagGroup < ActiveRecord::Base + belongs_to :category + belongs_to :tag_group + + validates :min_count, numericality: { only_integer: true, greater_than: 0 } + + after_commit do + Site.clear_cache + end +end + +# == Schema Information +# +# Table name: category_required_tag_groups +# +# id :bigint not null, primary key +# category_id :bigint not null +# tag_group_id :bigint not null +# min_count :integer default(1), not null +# order :integer default(1), not null +# created_at :datetime not null +# updated_at :datetime not null +# +# Indexes +# +# idx_category_required_tag_groups (category_id,tag_group_id) UNIQUE +# diff --git a/app/models/site.rb b/app/models/site.rb index 4442a12e272..12353dbd5f6 100644 --- a/app/models/site.rb +++ b/app/models/site.rb @@ -65,7 +65,7 @@ class Site # corresponding ActiveRecord callback to clear the categories cache. Discourse.cache.fetch(categories_cache_key, expires_in: 30.minutes) do categories = Category - .includes(:uploaded_logo, :uploaded_background, :tags, :tag_groups, :required_tag_group) + .includes(:uploaded_logo, :uploaded_background, :tags, :tag_groups, category_required_tag_groups: :tag_group) .joins('LEFT JOIN topics t on t.id = categories.topic_id') .select('categories.*, t.slug topic_slug') .order(:position) diff --git a/app/serializers/category_required_tag_group_serializer.rb b/app/serializers/category_required_tag_group_serializer.rb new file mode 100644 index 00000000000..c74c5d61e9f --- /dev/null +++ b/app/serializers/category_required_tag_group_serializer.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class CategoryRequiredTagGroupSerializer < ApplicationSerializer + attributes :name, :min_count + + def name + object.tag_group.name + end +end diff --git a/app/serializers/site_category_serializer.rb b/app/serializers/site_category_serializer.rb index 8634ba38740..1d29b7c7af0 100644 --- a/app/serializers/site_category_serializer.rb +++ b/app/serializers/site_category_serializer.rb @@ -5,10 +5,10 @@ class SiteCategorySerializer < BasicCategorySerializer attributes :allowed_tags, :allowed_tag_groups, :allow_global_tags, - :min_tags_from_required_group, - :required_tag_group_name, :read_only_banner + has_many :category_required_tag_groups, key: :required_tag_groups, embed: :objects + def include_allowed_tags? SiteSetting.tagging_enabled end @@ -29,7 +29,7 @@ class SiteCategorySerializer < BasicCategorySerializer SiteSetting.tagging_enabled end - def required_tag_group_name - object.required_tag_group&.name + def include_required_tag_groups? + SiteSetting.tagging_enabled end end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 7dfa54e5161..69f65db27c9 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -3324,10 +3324,11 @@ en: tag_groups_placeholder: "(Optional) list of allowed tag groups" manage_tag_groups_link: "Manage tag groups" allow_global_tags_label: "Also allow other tags" - tag_group_selector_placeholder: "(Optional) Tag group" - required_tag_group_description: "Require new topics to have tags from a tag group:" - min_tags_from_required_group_label: "Num Tags:" - required_tag_group_label: "Tag group:" + required_tag_group: + description: "Require new topics to have tags from tag groups:" + delete: "Delete" + add: "Add required tag group" + placeholder: "select tag group..." topic_featured_link_allowed: "Allow featured links in this category" delete: "Delete Category" create: "New Category" diff --git a/db/migrate/20220401130745_create_category_required_tag_groups.rb b/db/migrate/20220401130745_create_category_required_tag_groups.rb new file mode 100644 index 00000000000..1d0bfbb4270 --- /dev/null +++ b/db/migrate/20220401130745_create_category_required_tag_groups.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +class CreateCategoryRequiredTagGroups < ActiveRecord::Migration[6.1] + def up + create_table :category_required_tag_groups do |t| + t.bigint :category_id, null: false + t.bigint :tag_group_id, null: false + t.integer :min_count, null: false, default: 1 + t.integer :order, null: false, default: 1 + t.timestamps + end + + add_index :category_required_tag_groups, [:category_id, :tag_group_id], name: "idx_category_required_tag_groups", unique: true + + execute <<~SQL + INSERT INTO category_required_tag_groups + (category_id, tag_group_id, min_count, updated_at, created_at) + SELECT c.id, c.required_tag_group_id, c.min_tags_from_required_group, NOW(), NOW() + FROM categories c + WHERE c.required_tag_group_id IS NOT NULL + SQL + end + + def down + drop_table :category_required_tag_groups + end +end diff --git a/db/post_migrate/20220401140745_drop_category_required_tag_group_columns.rb b/db/post_migrate/20220401140745_drop_category_required_tag_group_columns.rb new file mode 100644 index 00000000000..fb7abd769cc --- /dev/null +++ b/db/post_migrate/20220401140745_drop_category_required_tag_group_columns.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +class DropCategoryRequiredTagGroupColumns < ActiveRecord::Migration[6.1] + DROPPED_COLUMNS ||= { + categories: %i{ + required_tag_group_id + min_tags_from_required_group + } + } + + def up + DROPPED_COLUMNS.each do |table, columns| + Migration::ColumnDropper.execute_drop(table, columns) + end + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/lib/discourse_tagging.rb b/lib/discourse_tagging.rb index 28fa7fe0592..77487c23159 100644 --- a/lib/discourse_tagging.rb +++ b/lib/discourse_tagging.rb @@ -155,24 +155,27 @@ module DiscourseTagging end def self.validate_required_tags_from_group(guardian, model, category, tags = []) - if !guardian.is_staff? && - category && - category.required_tag_group && - (tags.length < category.min_tags_from_required_group || - category.required_tag_group.tags.where("tags.id in (?)", tags.map(&:id)).count < category.min_tags_from_required_group) + return true if guardian.is_staff? || category.nil? - model.errors.add(:base, - I18n.t( - "tags.required_tags_from_group", - count: category.min_tags_from_required_group, - tag_group_name: category.required_tag_group.name, - tags: category.required_tag_group.tags.order(:id).pluck(:name).join(", ") + success = true + category.category_required_tag_groups.each do |crtg| + if tags.length < crtg.min_count || + crtg.tag_group.tags.where("tags.id in (?)", tags.map(&:id)).count < crtg.min_count + + success = false + + model.errors.add(:base, + I18n.t( + "tags.required_tags_from_group", + count: crtg.min_count, + tag_group_name: crtg.tag_group.name, + tags: crtg.tag_group.tags.order(:id).pluck(:name).join(", ") + ) ) - ) - false - else - true + end end + + success end def self.validate_category_restricted_tags(guardian, model, category, tags = []) @@ -363,12 +366,16 @@ module DiscourseTagging # or for staff when # - there are more available tags than the query limit # - and no search term has been included - filter_required_tags = category&.required_tag_group && (filter_for_non_staff || (term.blank? && category&.required_tag_group&.tags.size >= opts[:limit].to_i)) - - if opts[:for_input] && filter_required_tags - required_tag_ids = category.required_tag_group.tags.pluck(:id) - if (required_tag_ids & selected_tag_ids).size < category.min_tags_from_required_group - builder.where("id IN (?)", required_tag_ids) + required_tag_ids = nil + if opts[:for_input] && category&.category_required_tag_groups.present? && (filter_for_non_staff || term.blank?) + category.category_required_tag_groups.each do |crtg| + group_tags = crtg.tag_group.tags.pluck(:id) + next if (group_tags & selected_tag_ids).size >= crtg.min_count + if filter_for_non_staff || group_tags.size >= opts[:limit].to_i + required_tag_ids = group_tags + builder.where("id IN (?)", required_tag_ids) + end + break end end diff --git a/spec/integration/category_tag_spec.rb b/spec/integration/category_tag_spec.rb index 6319f0e6393..6e3399846e6 100644 --- a/spec/integration/category_tag_spec.rb +++ b/spec/integration/category_tag_spec.rb @@ -96,7 +96,7 @@ describe "category tag restrictions" do context 'required tags from tag group' do fab!(:tag_group) { Fabricate(:tag_group, tags: [tag1, tag3]) } - before { category_with_tags.update!(required_tag_group: tag_group, min_tags_from_required_group: 1) } + before { category_with_tags.update!(category_required_tag_groups: [CategoryRequiredTagGroup.new(tag_group: tag_group, min_count: 1)]) } it "search only returns the allowed tags" do expect_same_tag_names(filter_allowed_tags(for_input: true, category: category_with_tags), [tag1]) @@ -130,7 +130,7 @@ describe "category tag restrictions" do context 'required tags from tag group' do fab!(:tag_group) { Fabricate(:tag_group, tags: [tag1, tag3]) } - before { category_with_tags.update!(required_tag_group: tag_group, min_tags_from_required_group: 1) } + before { category_with_tags.update!(category_required_tag_groups: [CategoryRequiredTagGroup.new(tag_group: tag_group, min_count: 1)]) } it "search only returns the allowed tags" do expect_same_tag_names(filter_allowed_tags(for_input: true, category: category_with_tags), [tag1, tag3]) @@ -190,7 +190,7 @@ describe "category tag restrictions" do context 'required tags from tag group' do fab!(:tag_group) { Fabricate(:tag_group, tags: [tag1, tag3]) } - before { category.update!(required_tag_group: tag_group, min_tags_from_required_group: 1) } + before { category.update!(category_required_tag_groups: [CategoryRequiredTagGroup.new(tag_group: tag_group, min_count: 1)]) } it "search only returns the allowed tags" do expect_same_tag_names(filter_allowed_tags(for_input: true, category: category), [tag1]) @@ -224,7 +224,7 @@ describe "category tag restrictions" do context 'required tags from tag group' do fab!(:tag_group) { Fabricate(:tag_group, tags: [tag1, tag3]) } - before { category.update!(required_tag_group: tag_group, min_tags_from_required_group: 1) } + before { category.update!(category_required_tag_groups: [CategoryRequiredTagGroup.new(tag_group: tag_group, min_count: 1)]) } it "search only returns the allowed tags" do expect_same_tag_names(filter_allowed_tags(for_input: true, category: category), [tag1, tag3]) @@ -320,7 +320,7 @@ describe "category tag restrictions" do context 'required tags from tag group' do fab!(:tag_group) { Fabricate(:tag_group, tags: [tag1, tag2]) } - fab!(:category) { Fabricate(:category, required_tag_group: tag_group, min_tags_from_required_group: 1) } + fab!(:category) { Fabricate(:category, category_required_tag_groups: [CategoryRequiredTagGroup.new(tag_group: tag_group, min_count: 1)]) } it "search only returns the allowed tags" do tag_group_with_parent = Fabricate(:tag_group, parent_tag_id: tag1.id, tags: [tag3, tag4]) diff --git a/spec/lib/discourse_tagging_spec.rb b/spec/lib/discourse_tagging_spec.rb index 8c5a3ebc008..11b97e8ba1d 100644 --- a/spec/lib/discourse_tagging_spec.rb +++ b/spec/lib/discourse_tagging_spec.rb @@ -129,7 +129,7 @@ describe DiscourseTagging do context 'with required tags from tag group' do fab!(:tag_group) { Fabricate(:tag_group, tags: [tag1, tag2]) } - fab!(:category) { Fabricate(:category, required_tag_group: tag_group, min_tags_from_required_group: 1) } + fab!(:category) { Fabricate(:category, category_required_tag_groups: [ CategoryRequiredTagGroup.new(tag_group: tag_group, min_count: 1) ]) } it "returns the required tags if none have been selected" do tags = DiscourseTagging.filter_allowed_tags(Guardian.new(user), @@ -151,7 +151,7 @@ describe DiscourseTagging do end it "returns required tags if not enough are selected" do - category.update!(min_tags_from_required_group: 2) + category.category_required_tag_groups.first.update!(min_count: 2) tags = DiscourseTagging.filter_allowed_tags(Guardian.new(user), for_input: true, category: category, @@ -171,6 +171,36 @@ describe DiscourseTagging do expect(sorted_tag_names(tags)).to eq(sorted_tag_names([tag1, tag2, tag3])) end + it "handles multiple required tag groups in sequence" do + tag4 = Fabricate(:tag) + tag_group_2 = Fabricate(:tag_group, tags: [tag4]) + CategoryRequiredTagGroup.create!(category: category, tag_group: tag_group_2, min_count: 1, order: 2) + + category.reload + + # In the beginning, show tags for tag_group + tags = DiscourseTagging.filter_allowed_tags(Guardian.new(user), + for_input: true, + category: category, + ).to_a + expect(sorted_tag_names(tags)).to eq(sorted_tag_names([tag1, tag2])) + + # Once a tag_group tag has been selected, move on to tag_group_2 tags + tags = DiscourseTagging.filter_allowed_tags(Guardian.new(user), + for_input: true, + category: category, + selected_tags: [tag1.name], + ).to_a + expect(sorted_tag_names(tags)).to eq(sorted_tag_names([tag4])) + + # Once all requirements are satisfied, show all tags + tags = DiscourseTagging.filter_allowed_tags(Guardian.new(user), + for_input: true, + category: category, + selected_tags: [tag1.name, tag4.name], + ).to_a + expect(sorted_tag_names(tags)).to eq(sorted_tag_names([tag2, tag3])) + end end context 'with many required tags in a tag group' do @@ -179,7 +209,7 @@ describe DiscourseTagging do fab!(:tag6) { Fabricate(:tag, name: "T6") } fab!(:tag7) { Fabricate(:tag, name: "T7") } fab!(:tag_group) { Fabricate(:tag_group, tags: [tag1, tag2, tag4, tag5, tag6, tag7]) } - fab!(:category) { Fabricate(:category, required_tag_group: tag_group, min_tags_from_required_group: 1) } + fab!(:category) { Fabricate(:category, category_required_tag_groups: [CategoryRequiredTagGroup.new(tag_group: tag_group, min_count: 1)]) } it "returns required tags for staff by default" do tags = DiscourseTagging.filter_allowed_tags(Guardian.new(admin), @@ -500,8 +530,7 @@ describe DiscourseTagging do before do tag_group.tags = [tag1, tag2] category.update( - required_tag_group: tag_group, - min_tags_from_required_group: 1 + category_required_tag_groups: [CategoryRequiredTagGroup.new(tag_group: tag_group, min_count: 1)], ) end diff --git a/spec/lib/new_post_manager_spec.rb b/spec/lib/new_post_manager_spec.rb index 532a6f1008c..1fd2785cce8 100644 --- a/spec/lib/new_post_manager_spec.rb +++ b/spec/lib/new_post_manager_spec.rb @@ -577,7 +577,7 @@ describe NewPostManager do let(:tag) { Fabricate(:tag) } before do TagGroupMembership.create(tag: tag, tag_group: tag_group) - category.update(min_tags_from_required_group: 1, required_tag_group_id: tag_group.id) + category.update(category_required_tag_groups: [CategoryRequiredTagGroup.new(tag_group: tag_group, min_count: 1)]) end it "errors when there are no tags from the group provided" do @@ -593,8 +593,8 @@ describe NewPostManager do expect(result.errors.full_messages).to include( I18n.t( "tags.required_tags_from_group", - count: category.min_tags_from_required_group, - tag_group_name: category.required_tag_group.name, + count: category.category_required_tag_groups.first.min_count, + tag_group_name: category.category_required_tag_groups.first.tag_group.name, tags: tag.name ) ) diff --git a/spec/lib/post_revisor_spec.rb b/spec/lib/post_revisor_spec.rb index ba4aac79e3d..eaeb480873d 100644 --- a/spec/lib/post_revisor_spec.rb +++ b/spec/lib/post_revisor_spec.rb @@ -1123,7 +1123,7 @@ describe PostRevisor do fab!(:tag2) { Fabricate(:tag) } fab!(:tag3) { Fabricate(:tag) } fab!(:tag_group) { Fabricate(:tag_group, tags: [tag1, tag2]) } - fab!(:category) { Fabricate(:category, name: "beta", required_tag_group: tag_group, min_tags_from_required_group: 1) } + fab!(:category) { Fabricate(:category, name: "beta", category_required_tag_groups: [CategoryRequiredTagGroup.new(tag_group: tag_group, min_count: 1)]) } before do post.topic.update(category: category) diff --git a/spec/lib/topic_creator_spec.rb b/spec/lib/topic_creator_spec.rb index c5bdf02f96f..028e74acbc4 100644 --- a/spec/lib/topic_creator_spec.rb +++ b/spec/lib/topic_creator_spec.rb @@ -159,7 +159,7 @@ describe TopicCreator do context 'required tag group' do fab!(:tag_group) { Fabricate(:tag_group, tags: [tag1]) } - fab!(:category) { Fabricate(:category, name: "beta", required_tag_group: tag_group, min_tags_from_required_group: 1) } + fab!(:category) { Fabricate(:category, name: "beta", category_required_tag_groups: [CategoryRequiredTagGroup.new(tag_group: tag_group, min_count: 1)]) } it "when no tags are not present" do expect( diff --git a/spec/requests/api/schemas/json/category_create_response.json b/spec/requests/api/schemas/json/category_create_response.json index 47af86d8c81..07dcd958599 100644 --- a/spec/requests/api/schemas/json/category_create_response.json +++ b/spec/requests/api/schemas/json/category_create_response.json @@ -128,9 +128,6 @@ ] }, - "min_tags_from_required_group": { - "type": "integer" - }, "allowed_tags": { "type": "array" }, @@ -140,10 +137,25 @@ "allow_global_tags": { "type": "boolean" }, - "required_tag_group_name": { - "type": [ - "string", - "null" + "required_tag_groups": { + "type": "array", + "items": [ + { + "type": "object", + "additionalProperties": false, + "properties": { + "name": { + "type": "string" + }, + "min_count": { + "type": "integer" + } + }, + "required": [ + "name", + "min_count" + ] + } ] }, "read_only_banner": { @@ -267,8 +279,7 @@ "minimum_required_tags", "navigate_to_first_post_after_read", "custom_fields", - "min_tags_from_required_group", - "required_tag_group_name", + "required_tag_groups", "read_only_banner", "available_groups", "auto_close_hours", diff --git a/spec/requests/api/schemas/json/category_update_response.json b/spec/requests/api/schemas/json/category_update_response.json index 26d9aa86fb6..aca53a35a9a 100644 --- a/spec/requests/api/schemas/json/category_update_response.json +++ b/spec/requests/api/schemas/json/category_update_response.json @@ -131,9 +131,6 @@ ] }, - "min_tags_from_required_group": { - "type": "integer" - }, "allowed_tags": { "type": "array" }, @@ -143,10 +140,25 @@ "allow_global_tags": { "type": "boolean" }, - "required_tag_group_name": { - "type": [ - "string", - "null" + "required_tag_groups": { + "type": "array", + "items": [ + { + "type": "object", + "additionalProperties": false, + "properties": { + "name": { + "type": "string" + }, + "min_count": { + "type": "integer" + } + }, + "required": [ + "name", + "min_count" + ] + } ] }, "read_only_banner": { @@ -270,8 +282,7 @@ "minimum_required_tags", "navigate_to_first_post_after_read", "custom_fields", - "min_tags_from_required_group", - "required_tag_group_name", + "required_tag_groups", "read_only_banner", "available_groups", "auto_close_hours", diff --git a/spec/requests/api/schemas/json/site_response.json b/spec/requests/api/schemas/json/site_response.json index 69d5c299772..db4d9b64695 100644 --- a/spec/requests/api/schemas/json/site_response.json +++ b/spec/requests/api/schemas/json/site_response.json @@ -593,13 +593,25 @@ "allow_global_tags": { "type": "boolean" }, - "min_tags_from_required_group": { - "type": "integer" - }, - "required_tag_group_name": { - "type": [ - "string", - "null" + "required_tag_groups": { + "type": "array", + "items": [ + { + "type": "object", + "additionalProperties": false, + "properties": { + "name": { + "type": "string" + }, + "min_count": { + "type": "integer" + } + }, + "required": [ + "name", + "min_count" + ] + } ] }, "read_only_banner": { @@ -655,8 +667,7 @@ "allowed_tags", "allowed_tag_groups", "allow_global_tags", - "min_tags_from_required_group", - "required_tag_group_name", + "required_tag_groups", "read_only_banner", "uploaded_logo", "uploaded_background", diff --git a/spec/requests/categories_controller_spec.rb b/spec/requests/categories_controller_spec.rb index fdaf5802e65..790a7ab2741 100644 --- a/spec/requests/categories_controller_spec.rb +++ b/spec/requests/categories_controller_spec.rb @@ -493,8 +493,10 @@ describe CategoriesController do }, minimum_required_tags: "", allow_global_tags: 'true', - required_tag_group_name: tag_group.name, - min_tags_from_required_group: 2 + required_tag_groups: [{ + name: tag_group.name, + min_count: 2 + }] } expect(response.status).to eq(200) @@ -509,8 +511,9 @@ describe CategoriesController do expect(category.custom_fields).to eq("dancing" => "frogs") expect(category.minimum_required_tags).to eq(0) expect(category.allow_global_tags).to eq(true) - expect(category.required_tag_group_id).to eq(tag_group.id) - expect(category.min_tags_from_required_group).to eq(2) + expect(category.category_required_tag_groups.count).to eq(1) + expect(category.category_required_tag_groups.first.tag_group.id).to eq(tag_group.id) + expect(category.category_required_tag_groups.first.min_count).to eq(2) end it 'logs the changes correctly' do @@ -558,19 +561,19 @@ describe CategoriesController do it "can remove required tag group" do SiteSetting.tagging_enabled = true - category.update!(required_tag_group: Fabricate(:tag_group)) + category.update!(category_required_tag_groups: [ CategoryRequiredTagGroup.new(tag_group: Fabricate(:tag_group)) ]) put "/categories/#{category.id}.json", params: { name: category.name, color: category.color, text_color: category.text_color, allow_global_tags: 'false', min_tags_from_required_group: 1, - required_tag_group_name: '' + required_tag_groups: [] } expect(response.status).to eq(200) category.reload - expect(category.required_tag_group).to be_nil + expect(category.category_required_tag_groups).to be_empty end it "does not update other fields" do @@ -581,7 +584,7 @@ describe CategoriesController do category.update!( allowed_tags: ["hello", "world"], allowed_tag_groups: [tag_group_1.name], - required_tag_group_name: tag_group_2.name, + category_required_tag_groups: [ CategoryRequiredTagGroup.new(tag_group: tag_group_2) ], custom_fields: { field_1: 'hello', field_2: 'hello' } ) @@ -590,7 +593,7 @@ describe CategoriesController do category.reload expect(category.tags.pluck(:name)).to contain_exactly("hello", "world") expect(category.tag_groups.pluck(:name)).to contain_exactly(tag_group_1.name) - expect(category.required_tag_group).to eq(tag_group_2) + expect(category.category_required_tag_groups.first.tag_group).to eq(tag_group_2) expect(category.custom_fields).to eq({ 'field_1' => 'hello', 'field_2' => 'hello' }) put "/categories/#{category.id}.json", params: { allowed_tags: [], custom_fields: { field_1: nil } } @@ -598,15 +601,15 @@ describe CategoriesController do category.reload expect(category.tags).to be_blank expect(category.tag_groups.pluck(:name)).to contain_exactly(tag_group_1.name) - expect(category.required_tag_group).to eq(tag_group_2) + expect(category.category_required_tag_groups.first.tag_group).to eq(tag_group_2) expect(category.custom_fields).to eq({ 'field_2' => 'hello' }) - put "/categories/#{category.id}.json", params: { allowed_tags: [], allowed_tag_groups: [], required_tag_group_name: nil, custom_fields: { field_1: 'hi', field_2: nil } } + put "/categories/#{category.id}.json", params: { allowed_tags: [], allowed_tag_groups: [], required_tag_groups: [], custom_fields: { field_1: 'hi', field_2: nil } } expect(response.status).to eq(200) category.reload expect(category.tags).to be_blank expect(category.tag_groups).to be_blank - expect(category.required_tag_group).to eq(nil) + expect(category.category_required_tag_groups).to eq([]) expect(category.custom_fields).to eq({ 'field_1' => 'hi' }) end end diff --git a/spec/serializers/site_serializer_spec.rb b/spec/serializers/site_serializer_spec.rb index 4481338ed66..51e657f54d7 100644 --- a/spec/serializers/site_serializer_spec.rb +++ b/spec/serializers/site_serializer_spec.rb @@ -35,14 +35,14 @@ describe SiteSerializer do category.tags << tag category.tag_groups << tag_group - category.update!(required_tag_group: tag_group_2) + category.update!(category_required_tag_groups: [CategoryRequiredTagGroup.new(tag_group: tag_group_2, min_count: 1)]) serialized = described_class.new(Site.new(guardian), scope: guardian, root: false).as_json c1 = serialized[:categories].find { |c| c[:id] == category.id } expect(c1[:allowed_tags]).to contain_exactly(tag.name) expect(c1[:allowed_tag_groups]).to contain_exactly(tag_group.name) - expect(c1[:required_tag_group_name]).to eq(tag_group_2.name) + expect(c1[:required_tag_groups]).to eq([{ name: tag_group_2.name, min_count: 1 }]) end it "returns correct notification level for categories" do