FIX: Enforce tag group count validation before sending to review queue (#12728)

There is a category setting that enforces 1 or more tags must be added to a topic from a specific tag group before creating it. This validation was not being run before the topic was being sent to a review queue for categories that have that setting enabled.

There was an existing validation in `TopicCreator` but it was not correct; it was only validating when the tags did _not_ exist and also only happened on `create`. I now run the validation in `TopicCreator.valid?`

I also improved the error message shown to the user when they have not added the tags required (showing the tag names from the tag group), and changed the composer tag selector to not show "optional" if there are N tags required from a certain group.

app/assets/javascripts/discourse/app/models/composer.js

@@ -151,9 +151,24 @@ const Composer = RestModel.extend({
 
   @discourseComputed("category")
   minimumRequiredTags(category) {
-    return category && category.minimum_required_tags > 0
+    if (category) {
+      if (category.required_tag_groups) {
+        return category.min_tags_from_required_group;
+      } else {
+        return category.minimum_required_tags > 0
           ? category.minimum_required_tags
           : null;
+      }
+    }
+
+    return null;
+  },
+
+  @discourseComputed("category")
+  requiredTagGroups(category) {
+    return category && category.required_tag_groups
+      ? category.required_tag_groups
+      : null;
   },
 
   creatingTopic: equal("action", CREATE_TOPIC),

app/assets/javascripts/discourse/app/templates/composer.hbs

@@ -94,6 +94,7 @@
                     options=(hash
                       categoryId=model.categoryId
                       minimum=model.minimumRequiredTags
+                      requiredTagGroups=model.requiredTagGroups
                     )
                   }}
                   {{popup-input-tip validation=tagValidation}}

app/assets/javascripts/select-kit/addon/components/mini-tag-chooser.js

@@ -79,7 +79,10 @@ export default ComboBox.extend(TagsMixin, {
   }),
 
   modifyNoSelection() {
-    if (this.selectKit.options.minimum) {
+    if (
+      this.selectKit.options.minimum ||
+      this.selectKit.options.requiredTagGroups
+    ) {
       const minimum = parseInt(this.selectKit.options.minimum, 10);
       if (minimum > 0) {
         return this.defaultItem(

app/assets/stylesheets/common/base/edit-category.scss

@@ -123,7 +123,7 @@ div.edit-category {
 
     .select-kit {
       &.tag-chooser {
-        width: $category-settings-width;
+        width: 250px;
 
         .select-kit-filter,
         .filter-input {

config/locales/server.en.yml

@@ -4682,8 +4682,8 @@ en:
       synonym: 'Synonyms are not allowed. Use "%{tag_name}" instead.'
       has_synonyms: '"%{tag_name}" cannot be used because it has synonyms.'
     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."
+      one: "You must include at least %{count} %{tag_group_name} tag. The tags in this group are: %{tags}."
+      other: "You must include at least %{count} %{tag_group_name} tags. The tags in this group are: %{tags}."
     invalid_target_tag: "cannot be a synonym of a synonym"
     synonyms_exist: "is not allowed while synonyms exist"
   rss_by_tag: "Topics tagged %{tag}"

lib/discourse_tagging.rb

@@ -138,31 +138,32 @@ module DiscourseTagging
     false
   end
 
-  def self.validate_min_required_tags_for_category(guardian, topic, category, tags = [])
+  def self.validate_min_required_tags_for_category(guardian, model, 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))
+      model.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 = [])
+  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)
 
-      topic.errors.add(:base,
+      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
+          tag_group_name: category.required_tag_group.name,
+          tags: category.required_tag_group.tags.pluck(:name).join(", ")
         )
       )
       false

lib/topic_creator.rb

@@ -24,6 +24,15 @@ class TopicCreator
     # this allows us to add errors
     valid = topic.valid?
 
+    category = find_category
+    if category.present? && guardian.can_tag?(topic)
+      tags = @opts[:tags].present? ? Tag.where(name: @opts[:tags]) : (@opts[:tags] || [])
+
+      # both add to topic.errors
+      DiscourseTagging.validate_min_required_tags_for_category(guardian, topic, category, tags)
+      DiscourseTagging.validate_required_tags_from_group(guardian, topic, category, tags)
+    end
+
     DiscourseEvent.trigger(:after_validate_topic, topic, self)
     valid &&= topic.errors.empty?
 
@@ -160,23 +169,14 @@ class TopicCreator
   end
 
   def setup_tags(topic)
-    if @opts[:tags].blank?
-      unless @guardian.is_staff? || !guardian.can_tag?(topic)
-        category = find_category
+    return if @opts[:tags].blank?
 
-        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
-    else
     valid_tags = DiscourseTagging.tag_topic_by_names(topic, @guardian, @opts[:tags])
     unless valid_tags
       topic.errors.add(:base, :unable_to_tag)
       rollback_from_errors!(topic)
     end
   end
-  end
 
   def setup_auto_close_time(topic)
     return unless @opts[:auto_close_time].present?

spec/components/discourse_tagging_spec.rb

@@ -459,7 +459,7 @@ describe DiscourseTagging 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)
+          I18n.t("tags.required_tags_from_group", count: 1, tag_group_name: tag_group.name, tags: tag_group.tags.pluck(:name).join(", "))
         )
       end
 
@@ -467,7 +467,7 @@ describe DiscourseTagging 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)
+          I18n.t("tags.required_tags_from_group", count: 1, tag_group_name: tag_group.name, tags: tag_group.tags.pluck(:name).join(", "))
         )
       end
 

spec/components/new_post_manager_spec.rb

@@ -450,12 +450,13 @@ describe NewPostManager do
   end
 
   context 'when posting in the category requires approval' do
-    fab!(:user) { Fabricate(:user) }
-    fab!(:review_group) { Fabricate(:group) }
-    fab!(:category) { Fabricate(:category, reviewable_by_group_id: review_group.id) }
+    let!(:user) { Fabricate(:user) }
+    let!(:review_group) { Fabricate(:group) }
+    let!(:category) { Fabricate(:category, reviewable_by_group_id: review_group.id) }
 
     context 'when new topics require approval' do
       before do
+        SiteSetting.tagging_enabled = true
         category.custom_fields[Category::REQUIRE_TOPIC_APPROVAL] = true
         category.save
       end
@@ -488,10 +489,89 @@ describe NewPostManager do
         expect(result.action).to eq(:create_post)
         expect(result).to be_success
       end
+
+      context "when the category has tagging rules" do
+        context "when there is a minimum number of tags required for the category" do
+          before do
+            category.update(minimum_required_tags: 1)
+          end
+
+          it "errors when there are no tags provided" do
+            manager = NewPostManager.new(
+              user,
+              raw: 'this is a new topic',
+              title: "Let's start a new topic!",
+              category: category.id
+            )
+
+            result = manager.perform
+            expect(result.action).to eq(:enqueued)
+            expect(result.errors.full_messages).to include(I18n.t("tags.minimum_required_tags", count: category.minimum_required_tags))
+          end
+
+          it "enqueues the topic if there are tags provided" do
+            tag = Fabricate(:tag)
+            manager = NewPostManager.new(
+              user,
+              raw: 'this is a new topic',
+              title: "Let's start a new topic!",
+              category: category.id,
+              tags: tag.name
+            )
+
+            result = manager.perform
+            expect(result.action).to eq(:enqueued)
+            expect(result.reason).to eq(:category)
+          end
+        end
+
+        context "when there is a minimum number of tags required from a certain tag group for the category" do
+          let(:tag_group) { Fabricate(:tag_group) }
+          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)
+          end
+
+          it "errors when there are no tags from the group provided" do
+            manager = NewPostManager.new(
+              user,
+              raw: 'this is a new topic',
+              title: "Let's start a new topic!",
+              category: category.id
+            )
+
+            result = manager.perform
+            expect(result.action).to eq(:enqueued)
+            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,
+                tags: tag.name
+              )
+            )
+          end
+
+          it "enqueues the topic if there are tags provided" do
+            manager = NewPostManager.new(
+              user,
+              raw: 'this is a new topic',
+              title: "Let's start a new topic!",
+              category: category.id,
+              tags: [tag.name]
+            )
+
+            result = manager.perform
+            expect(result.action).to eq(:enqueued)
+            expect(result.reason).to eq(:category)
+          end
+        end
+      end
     end
 
     context 'when new posts require approval' do
-      fab!(:topic) { Fabricate(:topic, category: category) }
+      let!(:topic) { Fabricate(:topic, category: category) }
 
       before do
         category.custom_fields[Category::REQUIRE_REPLY_APPROVAL] = true

spec/components/topic_creator_spec.rb

@@ -123,9 +123,9 @@ describe TopicCreator do
         fab!(:category) { Fabricate(:category, name: "beta", minimum_required_tags: 2) }
 
         it "fails for regular user if minimum_required_tags is not satisfied" do
-          expect do
-            TopicCreator.create(user, Guardian.new(user), valid_attrs.merge(category: category.id))
-          end.to raise_error(ActiveRecord::Rollback)
+          expect(
+            TopicCreator.new(user, Guardian.new(user), valid_attrs.merge(category: category.id)).valid?
+          ).to be_falsy
         end
 
         it "lets admin create a topic regardless of minimum_required_tags" do
@@ -153,27 +153,27 @@ describe TopicCreator do
         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)
+          expect(
+            TopicCreator.new(user, Guardian.new(user), valid_attrs.merge(category: category.id)).valid?
+          ).to be_falsy
         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)
+          expect(
+            TopicCreator.new(user, Guardian.new(user), valid_attrs.merge(category: category.id, tags: ['nope'])).valid?
+          ).to be_falsy
         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)
+          expect(
+            TopicCreator.new(user, Guardian.new(user), valid_attrs.merge(category: category.id, tags: [tag1.name, tag2.name])).valid?
+          ).to be_truthy
         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)
+          expect(
+            TopicCreator.new(user, Guardian.new(admin), valid_attrs.merge(category: category.id)).valid?
+          ).to be_truthy
         end
       end
     end