FIX: Enforce tag group count validation before sending to review queue (PR #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.

GitHub

any mobile concerns here?

why no call TopicCreator to ask it to do the validation.

Maybe make discourse/topic_creator.rb at 32689573fa7dc3389378b8ef669a01271438d71b · discourse/discourse · GitHub a bit more reusable?

The logic there is a bit bizarre…if the tags are blank then it does the DiscourseTagging validation?? It should be doing that if the tags exist.

    if @opts[:tags].blank?
      unless @guardian.is_staff? || !guardian.can_tag?(topic)
        category = find_category

        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

If the TopicCreator was doing the right thing here I think that we would achieve the same result, will try it.

No seems fine, I changed this because on desktop it was way too cramped and it squashed the text

setup_tags isn’t actually called until later; only .valid? for TopicCreator is called from NewPostManager. That in turn only calls topic.valid? which is not yet aware of the tags.

Do you think I should just move the tag validation checks from NewPostManager and TopicCreator#create_tags into TopicCreator#valid? I think that would be the best course of action but it does clutter up that method a bit. We cannot assign tags to a topic without it being created because topic.tags= goes off and creates the associations.

Tricky…

setup_tags is the only other place in TopicCreator that does errors.add(:base, whatever) so it is an outlier. I am going to move this validation into TopicCreator#valid?

Some tests are only failing via turbo_rspec which is quite annoying because debugging is very tricky in that environment

Figured out what was wrong. guardian.can_tag? wasn’t consistent because SiteSetting.tagging_enabled was not always true

I think the JS test failures are flaky because I cannot reproduce locally, and they seem unrelated to what I changed

looking good!