From d777844ed623f126ecd941d2b159f973e8e78b8d Mon Sep 17 00:00:00 2001 From: Neil Lalonde Date: Wed, 30 Oct 2019 14:49:00 -0400 Subject: [PATCH] FEATURE: categories can require topics have a tag from a tag group In a category's settings, the Tags tab has two new fields to specify the number of tags that must be added to a topic from a tag group. When creating a new topic, an error will be shown to the user if the requirement isn't met. --- .../discourse/models/category.js.es6 | 11 +++++ .../components/edit-category-tags.hbs | 20 ++++++++ app/assets/stylesheets/common/base/modal.scss | 16 +++++-- app/controllers/categories_controller.rb | 2 + app/models/category.rb | 7 +++ app/serializers/site_category_serializer.rb | 8 +++- config/locales/client.en.yml | 6 ++- config/locales/server.en.yml | 3 ++ ...30_add_required_tag_group_to_categories.rb | 8 ++++ lib/discourse_tagging.rb | 47 +++++++++++++++---- lib/topic_creator.rb | 6 +-- spec/components/discourse_tagging_spec.rb | 46 ++++++++++++++++++ spec/components/post_revisor_spec.rb | 31 ++++++++++++ spec/components/topic_creator_spec.rb | 29 ++++++++++++ spec/requests/categories_controller_spec.rb | 7 ++- 15 files changed, 226 insertions(+), 21 deletions(-) create mode 100644 db/migrate/20191030155530_add_required_tag_group_to_categories.rb diff --git a/app/assets/javascripts/discourse/models/category.js.es6 b/app/assets/javascripts/discourse/models/category.js.es6 index ee5101074c7..48f78e1ec09 100644 --- a/app/assets/javascripts/discourse/models/category.js.es6 +++ b/app/assets/javascripts/discourse/models/category.js.es6 @@ -31,6 +31,13 @@ const Category = RestModel.extend({ } }, + @on("init") + setupRequiredTagGroups() { + if (this.required_tag_group_name) { + this.set("required_tag_groups", [this.required_tag_group_name]); + } + }, + @computed availablePermissions() { return [ @@ -127,6 +134,10 @@ const Category = RestModel.extend({ allowed_tags: this.allowed_tags, allowed_tag_groups: this.allowed_tag_groups, allow_global_tags: this.allow_global_tags, + required_tag_group_name: this.required_tag_groups + ? this.required_tag_groups[0] + : null, + min_tags_from_required_group: this.min_tags_from_required_group, sort_order: this.sort_order, sort_ascending: this.sort_ascending, topic_featured_link_allowed: this.topic_featured_link_allowed, diff --git a/app/assets/javascripts/discourse/templates/components/edit-category-tags.hbs b/app/assets/javascripts/discourse/templates/components/edit-category-tags.hbs index a0af5fb2fb9..4d27d0c101d 100644 --- a/app/assets/javascripts/discourse/templates/components/edit-category-tags.hbs +++ b/app/assets/javascripts/discourse/templates/components/edit-category-tags.hbs @@ -24,3 +24,23 @@
{{i18n 'category.tags_tab_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 + maximum=1 + filterPlaceholder="category.tag_group_selector_placeholder" + }} +
+
diff --git a/app/assets/stylesheets/common/base/modal.scss b/app/assets/stylesheets/common/base/modal.scss index ced3985e74c..f68600c08bc 100644 --- a/app/assets/stylesheets/common/base/modal.scss +++ b/app/assets/stylesheets/common/base/modal.scss @@ -268,11 +268,14 @@ section.field { padding: 0.25em 0; margin-bottom: 5px; - } - - section.field .field-item { - display: inline-block; - margin-right: 10px; + &.with-items { + display: flex; + align-items: flex-start; + } + .field-item { + display: inline-block; + margin-right: 10px; + } } // password reset modal @@ -446,6 +449,9 @@ } } } + #category-min-tags-from-group { + width: 100px; + } } .incoming-email-modal { diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb index fafa90948e0..5e660f19816 100644 --- a/app/controllers/categories_controller.rb +++ b/app/controllers/categories_controller.rb @@ -313,6 +313,8 @@ class CategoriesController < ApplicationController :navigate_to_first_post_after_read, :search_priority, :allow_global_tags, + :required_tag_group_name, + :min_tags_from_required_group, custom_fields: [params[:custom_fields].try(:keys)], permissions: [*p.try(:keys)], allowed_tags: [], diff --git a/app/models/category.rb b/app/models/category.rb index e692de65f9b..4e2ec97a93e 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -51,6 +51,7 @@ 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 @@ -93,6 +94,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' + belongs_to :reviewable_by_group, class_name: 'Group' scope :latest, -> { order('topic_count DESC') } @@ -555,6 +558,10 @@ 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 ? TagGroup.where(name: group_name).first : nil + end + def downcase_email self.email_in = (email_in || "").strip.downcase.presence end diff --git a/app/serializers/site_category_serializer.rb b/app/serializers/site_category_serializer.rb index ed02fa6190e..31549fd8a03 100644 --- a/app/serializers/site_category_serializer.rb +++ b/app/serializers/site_category_serializer.rb @@ -4,7 +4,9 @@ class SiteCategorySerializer < BasicCategorySerializer attributes :allowed_tags, :allowed_tag_groups, - :allow_global_tags + :allow_global_tags, + :min_tags_from_required_group, + :required_tag_group_name def include_allowed_tags? SiteSetting.tagging_enabled @@ -26,4 +28,8 @@ class SiteCategorySerializer < BasicCategorySerializer SiteSetting.tagging_enabled end + def required_tag_group_name + object.required_tag_group&.name + end + end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index e7f56164854..a431052d317 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -2606,10 +2606,14 @@ en: tags_allowed_tags: "Restrict these tags to this category:" tags_allowed_tag_groups: "Restrict these tag groups to this category:" tags_placeholder: "(Optional) list of allowed tags" - tags_tab_description: "Tags and tag groups specified here will only be available in this category and other categories that also specify them. They won't be available for use in other categories." + tags_tab_description: "Tags and tag groups specified above will only be available in this category and other categories that also specify them. They won't be available for use in other categories." tag_groups_placeholder: "(Optional) list of allowed tag groups" manage_tag_groups_link: "Manage tag groups here." 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:" topic_featured_link_allowed: "Allow featured links in this category" delete: "Delete Category" create: "New Category" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 969cd73aefc..b0aa5591066 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -4344,6 +4344,9 @@ en: restricted_to: one: '"%{tag_name}" is restricted to the "%{category_names}" category' other: '"%{tag_name}" is restricted to the following categories: %{category_names}' + required_tags_from_group: + one: "You must include at least %{count} %{tag_group_name} tag." + other: "You must include at least %{count} %{tag_group_name} tags." rss_by_tag: "Topics tagged %{tag}" finish_installation: diff --git a/db/migrate/20191030155530_add_required_tag_group_to_categories.rb b/db/migrate/20191030155530_add_required_tag_group_to_categories.rb new file mode 100644 index 00000000000..eaed95a70eb --- /dev/null +++ b/db/migrate/20191030155530_add_required_tag_group_to_categories.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class AddRequiredTagGroupToCategories < ActiveRecord::Migration[6.0] + def change + add_column :categories, :required_tag_group_id, :integer, null: true + add_column :categories, :min_tags_from_required_group, :integer, null: false, default: 1 + end +end diff --git a/lib/discourse_tagging.rb b/lib/discourse_tagging.rb index e8bb6b96ecc..de6592c2654 100644 --- a/lib/discourse_tagging.rb +++ b/lib/discourse_tagging.rb @@ -88,11 +88,8 @@ module DiscourseTagging tags = tags + Tag.where(id: missing_parent_tag_ids).all end - # validate minimum required tags for a category - if !guardian.is_staff? && category && category.minimum_required_tags > 0 && tags.length < category.minimum_required_tags - topic.errors.add(:base, I18n.t("tags.minimum_required_tags", count: category.minimum_required_tags)) - return false - end + return false unless validate_min_required_tags_for_category(guardian, topic, category, tags) + return false unless validate_required_tags_from_group(guardian, topic, category, tags) if tags.size == 0 topic.errors.add(:base, I18n.t("tags.forbidden.invalid", count: new_tag_names.size)) @@ -101,11 +98,8 @@ module DiscourseTagging topic.tags = tags else - # validate minimum required tags for a category - if !guardian.is_staff? && category && category.minimum_required_tags > 0 - topic.errors.add(:base, I18n.t("tags.minimum_required_tags", count: category.minimum_required_tags)) - return false - end + return false unless validate_min_required_tags_for_category(guardian, topic, category) + return false unless validate_required_tags_from_group(guardian, topic, category) topic.tags = [] end @@ -114,6 +108,39 @@ module DiscourseTagging true end + def self.validate_min_required_tags_for_category(guardian, topic, category, tags = []) + if !guardian.is_staff? && + category && + category.minimum_required_tags > 0 && + tags.length < category.minimum_required_tags + + topic.errors.add(:base, I18n.t("tags.minimum_required_tags", count: category.minimum_required_tags)) + false + else + true + end + end + + def self.validate_required_tags_from_group(guardian, topic, 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) + + topic.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 + ) + ) + false + else + true + end + end + # Options: # term: a search term to filter tags by name # category: a Category to which the object being tagged belongs diff --git a/lib/topic_creator.rb b/lib/topic_creator.rb index dfbaf74d298..5f4aa7f16e9 100644 --- a/lib/topic_creator.rb +++ b/lib/topic_creator.rb @@ -147,10 +147,10 @@ class TopicCreator def setup_tags(topic) if @opts[:tags].blank? unless @guardian.is_staff? || !guardian.can_tag?(topic) - # Validate minimum required tags for a category category = find_category - if category.present? && category.minimum_required_tags > 0 - topic.errors.add(:base, I18n.t("tags.minimum_required_tags", count: category.minimum_required_tags)) + + if !DiscourseTagging.validate_min_required_tags_for_category(@guardian, topic, category) || + !DiscourseTagging.validate_required_tags_from_group(@guardian, topic, category) rollback_from_errors!(topic) end end diff --git a/spec/components/discourse_tagging_spec.rb b/spec/components/discourse_tagging_spec.rb index d269ffd5eca..8aeb04f9aee 100644 --- a/spec/components/discourse_tagging_spec.rb +++ b/spec/components/discourse_tagging_spec.rb @@ -236,6 +236,52 @@ describe DiscourseTagging do expect(topic.reload.tags.map(&:name)).to contain_exactly(*[parent_tag, common].map(&:name)) end end + + context "enforces required tags from a tag group" do + fab!(:category) { Fabricate(:category) } + fab!(:tag_group) { Fabricate(:tag_group) } + fab!(:topic) { Fabricate(:topic, category: category) } + + before do + tag_group.tags = [tag1, tag2] + category.update( + required_tag_group: tag_group, + min_tags_from_required_group: 1 + ) + end + + it "when no tags are present" do + valid = DiscourseTagging.tag_topic_by_names(topic, Guardian.new(user), []) + expect(valid).to eq(false) + expect(topic.errors[:base]&.first).to eq( + I18n.t("tags.required_tags_from_group", count: 1, tag_group_name: tag_group.name) + ) + end + + it "when tags are not part of the tag group" do + valid = DiscourseTagging.tag_topic_by_names(topic, Guardian.new(user), [tag3.name]) + expect(valid).to eq(false) + expect(topic.errors[:base]&.first).to eq( + I18n.t("tags.required_tags_from_group", count: 1, tag_group_name: tag_group.name) + ) + end + + it "when requirement is met" do + valid = DiscourseTagging.tag_topic_by_names(topic, Guardian.new(user), [tag1.name]) + expect(valid).to eq(true) + valid = DiscourseTagging.tag_topic_by_names(topic, Guardian.new(user), [tag1.name, tag2.name]) + expect(valid).to eq(true) + valid = DiscourseTagging.tag_topic_by_names(topic, Guardian.new(user), [tag2.name, tag3.name]) + expect(valid).to eq(true) + end + + it "lets staff ignore the restriction" do + valid = DiscourseTagging.tag_topic_by_names(topic, Guardian.new(admin), []) + expect(valid).to eq(true) + valid = DiscourseTagging.tag_topic_by_names(topic, Guardian.new(admin), [tag3.name]) + expect(valid).to eq(true) + end + end end describe '#tags_for_saving' do diff --git a/spec/components/post_revisor_spec.rb b/spec/components/post_revisor_spec.rb index 598fb686d5d..8a8b284007a 100644 --- a/spec/components/post_revisor_spec.rb +++ b/spec/components/post_revisor_spec.rb @@ -810,6 +810,37 @@ describe PostRevisor do end end + context "required tag group" do + fab!(:tag1) { Fabricate(:tag) } + 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) } + + before do + post.topic.update(category: category) + end + + it "doesn't allow removing all tags from the group" do + post.topic.tags = [tag1, tag2] + result = subject.revise!(user, raw: "lets totally update the body", tags: []) + expect(result).to eq(false) + end + + it "allows removing some tags" do + post.topic.tags = [tag1, tag2, tag3] + result = subject.revise!(user, raw: "lets totally update the body", tags: [tag1.name]) + expect(result).to eq(true) + expect(post.reload.topic.tags.map(&:name)).to eq([tag1.name]) + end + + it "allows admins to remove the tags" do + post.topic.tags = [tag1, tag2, tag3] + result = subject.revise!(admin, raw: "lets totally update the body", tags: []) + expect(result).to eq(true) + expect(post.reload.topic.tags.size).to eq(0) + end + end end context "cannot create tags" do diff --git a/spec/components/topic_creator_spec.rb b/spec/components/topic_creator_spec.rb index 347719adf8d..c475a08e92f 100644 --- a/spec/components/topic_creator_spec.rb +++ b/spec/components/topic_creator_spec.rb @@ -126,6 +126,35 @@ describe TopicCreator do expect(topic).to be_valid end end + + 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) } + + it "when no tags are not present" do + expect do + TopicCreator.create(user, Guardian.new(user), valid_attrs.merge(category: category.id)) + end.to raise_error(ActiveRecord::Rollback) + end + + it "when tags are not part of the tag group" do + expect do + TopicCreator.create(user, Guardian.new(user), valid_attrs.merge(category: category.id, tags: ['nope'])) + end.to raise_error(ActiveRecord::Rollback) + end + + it "when requirement is met" do + topic = TopicCreator.create(user, Guardian.new(user), valid_attrs.merge(category: category.id, tags: [tag1.name, tag2.name])) + expect(topic).to be_valid + expect(topic.tags.length).to eq(2) + end + + it "lets staff ignore the restriction" do + topic = TopicCreator.create(user, Guardian.new(admin), valid_attrs.merge(category: category.id)) + expect(topic).to be_valid + expect(topic.tags.length).to eq(0) + end + end end context 'personal message' do diff --git a/spec/requests/categories_controller_spec.rb b/spec/requests/categories_controller_spec.rb index 36072d01f2e..cb1c4d9b086 100644 --- a/spec/requests/categories_controller_spec.rb +++ b/spec/requests/categories_controller_spec.rb @@ -349,6 +349,7 @@ describe CategoriesController do it "updates attributes correctly" do readonly = CategoryGroup.permission_types[:readonly] create_post = CategoryGroup.permission_types[:create_post] + tag_group = Fabricate(:tag_group) put "/categories/#{category.id}.json", params: { name: "hello", @@ -364,7 +365,9 @@ describe CategoriesController do "dancing" => "frogs" }, minimum_required_tags: "", - allow_global_tags: 'true' + allow_global_tags: 'true', + required_tag_group_name: tag_group.name, + min_tags_from_required_group: 2 } expect(response.status).to eq(200) @@ -379,6 +382,8 @@ 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) end it 'logs the changes correctly' do