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

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.

diff --git a/app/assets/javascripts/discourse/app/models/composer.js b/app/assets/javascripts/discourse/app/models/composer.js
index c2a7f4d..83ec5fa 100644
--- a/app/assets/javascripts/discourse/app/models/composer.js
+++ b/app/assets/javascripts/discourse/app/models/composer.js
@@ -151,8 +151,23 @@ const Composer = RestModel.extend({
 
   @discourseComputed("category")
   minimumRequiredTags(category) {
-    return category && category.minimum_required_tags > 0
-      ? category.minimum_required_tags
+    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;
   },
 
diff --git a/app/assets/javascripts/discourse/app/templates/composer.hbs b/app/assets/javascripts/discourse/app/templates/composer.hbs
index 97c78ab..c03eb28 100644
--- a/app/assets/javascripts/discourse/app/templates/composer.hbs
+++ b/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}}
diff --git a/app/assets/javascripts/select-kit/addon/components/mini-tag-chooser.js b/app/assets/javascripts/select-kit/addon/components/mini-tag-chooser.js
index 7fcbe04..bce736c 100644
--- a/app/assets/javascripts/select-kit/addon/components/mini-tag-chooser.js
+++ b/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(
diff --git a/app/assets/stylesheets/common/base/edit-category.scss b/app/assets/stylesheets/common/base/edit-category.scss
index 9069d49..5e34df8 100644
--- a/app/assets/stylesheets/common/base/edit-category.scss
+++ b/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 {
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 5d94936..177a423 100644
--- a/config/locales/server.en.yml
+++ b/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}"
diff --git a/lib/discourse_tagging.rb b/lib/discourse_tagging.rb
index 3fcd53a..4c9aa5c 100644
--- a/lib/discourse_tagging.rb
+++ b/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
diff --git a/lib/topic_creator.rb b/lib/topic_creator.rb
index a06f81f..7b0a43f 100644
--- a/lib/topic_creator.rb
+++ b/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,21 +169,12 @@ 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
+    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
 
diff --git a/spec/components/discourse_tagging_spec.rb b/spec/components/discourse_tagging_spec.rb
index ffd27a8..30e51bc 100644
--- a/spec/components/discourse_tagging_spec.rb
+++ b/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)

[... diff too long, it was truncated ...]

GitHub sha: e3b1f572

This commit appears in #12728 which was approved by SamSaffron. It was merged by martin.