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.
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.
setup_tags isn’t actually called until later; only
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
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.
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
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