diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index a28c7726f2b..e706cec7d56 100644 --- a/app/controllers/topics_controller.rb +++ b/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 diff --git a/app/models/topic.rb b/app/models/topic.rb index c7d943e3033..fbb7c4b3578 100644 --- a/app/models/topic.rb +++ b/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) diff --git a/app/models/topic_converter.rb b/app/models/topic_converter.rb index 3c0b9652a95..72e5691edda 100644 --- a/app/models/topic_converter.rb +++ b/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 diff --git a/spec/jobs/publish_topic_to_category_spec.rb b/spec/jobs/publish_topic_to_category_spec.rb index a0c54719402..aebf4d3c622 100644 --- a/spec/jobs/publish_topic_to_category_spec.rb +++ b/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 diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index a2afd121fec..c06ed29b7c6 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/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