FIX: display validation errors when converting topics (#27064)

When converting a PM to a public topic (and vice versa), if there was a validation error (like a topic already used, or a tag required or not allowed) the error message wasn't bubbled up nor shown to the user.

This fix ensures we properly stop the conversion whenever a validation error happens and bubble up the errors back to the user so they can be informed.

Internal ref - t/128795

app/controllers/topics_controller.rb

@@ -1146,18 +1146,18 @@ class TopicsController < ApplicationController
   def convert_topic
     params.require(:id)
     params.require(:type)
+
     topic = Topic.find_by(id: params[:id])
     guardian.ensure_can_convert_topic!(topic)
 
-    if params[:type] == "public"
-      converted_topic =
+    topic =
+      if params[:type] == "public"
         topic.convert_to_public_topic(current_user, category_id: params[:category_id])
-    else
-      converted_topic = topic.convert_to_private_message(current_user)
-    end
-    render_topic_changes(converted_topic)
-  rescue ActiveRecord::RecordInvalid => ex
-    render_json_error(ex)
+      else
+        topic.convert_to_private_message(current_user)
+      end
+
+    topic.valid? ? render_topic_changes(topic) : render_json_error(topic)
   end
 
   def reset_bump_date

app/models/topic.rb

@@ -185,7 +185,7 @@ class Topic < ActiveRecord::Base
   rate_limit :limit_private_messages_per_day
 
   validates :title,
-            if: Proc.new { |t| t.new_record? || t.title_changed? },
+            if: Proc.new { |t| t.new_record? || t.title_changed? || t.category_id_changed? },
             presence: true,
             topic_title_length: true,
             censored_words: true,
@@ -1817,18 +1817,11 @@ class Topic < ActiveRecord::Base
   end
 
   def convert_to_public_topic(user, category_id: nil)
-    public_topic = TopicConverter.new(self, user).convert_to_public_topic(category_id)
-    Tag.update_counters(public_topic.tags, { public_topic_count: 1 }) if !category.read_restricted
-    add_small_action(user, "public_topic") if public_topic
-    public_topic
+    TopicConverter.new(self, user).convert_to_public_topic(category_id)
   end
 
   def convert_to_private_message(user)
-    read_restricted = category.read_restricted
-    private_topic = TopicConverter.new(self, user).convert_to_private_message
-    Tag.update_counters(private_topic.tags, { public_topic_count: -1 }) if !read_restricted
-    add_small_action(user, "private_topic") if private_topic
-    private_topic
+    TopicConverter.new(self, user).convert_to_private_message
   end
 
   def update_excerpt(excerpt)

app/models/topic_converter.rb

@@ -10,36 +10,41 @@ class TopicConverter
 
   def convert_to_public_topic(category_id = nil)
     Topic.transaction do
-      category_id =
-        if category_id
-          category_id
-        elsif SiteSetting.allow_uncategorized_topics
-          SiteSetting.uncategorized_category_id
-        else
-          Category
-            .where(read_restricted: false)
-            .where.not(id: SiteSetting.uncategorized_category_id)
-            .order("id asc")
-            .pick(:id)
-        end
+      category_id ||=
+        SiteSetting.uncategorized_category_id if SiteSetting.allow_uncategorized_topics
+
+      @category = Category.find_by(id: category_id) if category_id
+      @category ||=
+        Category
+          .where(read_restricted: false)
+          .where.not(id: SiteSetting.uncategorized_category_id)
+          .first
 
       PostRevisor.new(@topic.first_post, @topic).revise!(
         @user,
-        category_id: category_id,
+        category_id: @category.id,
         archetype: Archetype.default,
       )
 
+      raise ActiveRecord::Rollback if !@topic.valid?
+
       update_user_stats
       update_post_uploads_secure_status
+      add_small_action("public_topic")
+      Tag.update_counters(@topic.tags, { public_topic_count: 1 }) if !@category.read_restricted
+
       Jobs.enqueue(:topic_action_converter, topic_id: @topic.id)
       Jobs.enqueue(:delete_inaccessible_notifications, topic_id: @topic.id)
-      watch_topic(topic)
+
+      watch_topic(@topic)
     end
+
     @topic
   end
 
   def convert_to_private_message
     Topic.transaction do
+      was_public = !@topic.category.read_restricted
       @topic.update_category_topic_count_by(-1) if @topic.visible
 
       PostRevisor.new(@topic.first_post, @topic).revise!(
@@ -48,15 +53,20 @@ class TopicConverter
         archetype: Archetype.private_message,
       )
 
+      raise ActiveRecord::Rollback if !@topic.valid?
+
       add_allowed_users
       update_post_uploads_secure_status
+      add_small_action("private_topic")
+      Tag.update_counters(@topic.tags, { public_topic_count: -1 }) if was_public
       UserProfile.remove_featured_topic_from_all_profiles(@topic)
 
       Jobs.enqueue(:topic_action_converter, topic_id: @topic.id)
       Jobs.enqueue(:delete_inaccessible_notifications, topic_id: @topic.id)
 
-      watch_topic(topic)
+      watch_topic(@topic)
     end
+
     @topic
   end
 
@@ -86,21 +96,21 @@ class TopicConverter
     #
     # Changes user_stats (post_count) by the number of posts in the topic.
     # First post, hidden posts and non-regular posts are ignored.
-    DB.exec(<<~SQL)
-    UPDATE user_stats
-    SET post_count = post_count #{operation} X.count
-    FROM (
-      SELECT
-        us.user_id,
-        COUNT(*) AS count
-      FROM user_stats us
-      INNER JOIN posts ON posts.topic_id = #{@topic.id.to_i} AND posts.user_id = us.user_id
-      WHERE posts.post_number > 1
-      AND NOT posts.hidden
-      AND posts.post_type = #{Post.types[:regular].to_i}
-      GROUP BY us.user_id
-    ) X
-    WHERE X.user_id = user_stats.user_id
+    DB.exec <<~SQL
+      UPDATE user_stats
+      SET post_count = post_count #{operation} X.count
+      FROM (
+        SELECT
+          us.user_id,
+          COUNT(*) AS count
+        FROM user_stats us
+        INNER JOIN posts ON posts.topic_id = #{@topic.id.to_i} AND posts.user_id = us.user_id
+        WHERE posts.post_number > 1
+        AND NOT posts.hidden
+        AND posts.post_type = #{Post.types[:regular].to_i}
+        GROUP BY us.user_id
+      ) X
+      WHERE X.user_id = user_stats.user_id
     SQL
   end
 
@@ -139,4 +149,8 @@ class TopicConverter
   def update_post_uploads_secure_status
     DB.after_commit { Jobs.enqueue(:update_topic_upload_security, topic_id: @topic.id) }
   end
+
+  def add_small_action(action_code)
+    DB.after_commit { @topic.add_small_action(@user, action_code) }
+  end
 end

spec/jobs/publish_topic_to_category_spec.rb

@@ -72,12 +72,12 @@ RSpec.describe Jobs::PublishTopicToCategory do
 
       now = freeze_time
 
-      message =
-        MessageBus
-          .track_publish do
-            described_class.new.execute(topic_timer_id: topic.public_topic_timer.id)
-          end
-          .last
+      messages =
+        MessageBus.track_publish do
+          described_class.new.execute(topic_timer_id: topic.public_topic_timer.id)
+        end
+
+      expect(messages.any? { |m| m.data[:reload_topic] && m.data[:refresh_stream] }).to eq(true)
 
       topic.reload
       expect(topic.category).to eq(another_category)
@@ -87,9 +87,6 @@ RSpec.describe Jobs::PublishTopicToCategory do
       %w[created_at bumped_at updated_at last_posted_at].each do |attribute|
         expect(topic.public_send(attribute)).to eq_time(now)
       end
-
-      expect(message.data[:reload_topic]).to be_present
-      expect(message.data[:refresh_stream]).to be_present
     end
 
     it "does nothing if the user can't see the PM" do

spec/requests/topics_controller_spec.rb

@@ -4613,7 +4613,7 @@ RSpec.describe TopicsController do
       end
 
       context "with success" do
-        it "returns success" do
+        it "returns success and the new url" do
           sign_in(admin)
           put "/t/#{topic.id}/convert-topic/public.json?category_id=#{category.id}"
 
@@ -4627,6 +4627,20 @@ RSpec.describe TopicsController do
           expect(result["url"]).to be_present
         end
       end
+
+      context "with some errors" do
+        it "returns the error messages" do
+          Fabricate(:topic, title: topic.title, category: category)
+
+          sign_in(admin)
+          put "/t/#{topic.id}/convert-topic/public.json?category_id=#{category.id}"
+
+          expect(response.status).to eq(422)
+          expect(response.parsed_body["errors"][0]).to end_with(
+            I18n.t("errors.messages.has_already_been_used"),
+          )
+        end
+      end
     end
   end