diff --git a/lib/discourse_dev/config.rb b/lib/discourse_dev/config.rb index f83eebe880e..778aeae9628 100644 --- a/lib/discourse_dev/config.rb +++ b/lib/discourse_dev/config.rb @@ -10,10 +10,11 @@ module DiscourseDev def initialize default_file_path = File.join(Rails.root, "config", "dev_defaults.yml") @file_path = File.join(Rails.root, "config", "dev.yml") - default_config = YAML.load_file(default_file_path) + # https://stackoverflow.com/questions/71332602/upgrading-to-ruby-3-1-causes-psychdisallowedclass-exception-when-using-yaml-lo + default_config = YAML.load_file(default_file_path, permitted_classes: [Date]) if File.exist?(file_path) - user_config = YAML.load_file(file_path) + user_config = YAML.load_file(file_path, permitted_classes: [Date]) else puts "I did no detect a custom `config/dev.yml` file, creating one for you where you can amend defaults." FileUtils.cp(default_file_path, file_path) diff --git a/plugins/chat/app/controllers/api/chat_channels_archives_controller.rb b/plugins/chat/app/controllers/api/chat_channels_archives_controller.rb index 759348ef4ec..ca5640e9925 100644 --- a/plugins/chat/app/controllers/api/chat_channels_archives_controller.rb +++ b/plugins/chat/app/controllers/api/chat_channels_archives_controller.rb @@ -8,35 +8,48 @@ class Chat::Api::ChatChannelsArchivesController < Chat::Api::ChatChannelsControl guardian.ensure_can_change_channel_status!(channel_from_params, :archived) raise Discourse::InvalidAccess if !existing_archive.failed? Chat::ChatChannelArchiveService.retry_archive_process(chat_channel: channel_from_params) - else - archive_params = - params - .require(:archive) - .tap do |ca| - ca.require(:type) - ca.permit(:title, :topic_id, :category_id, tags: []) - end + return render json: success_json + end - new_topic = archive_params[:type] == "new_topic" - raise Discourse::InvalidParameters if new_topic && archive_params[:title].blank? - raise Discourse::InvalidParameters if !new_topic && archive_params[:topic_id].blank? + new_topic = archive_params[:type] == "new_topic" + raise Discourse::InvalidParameters if new_topic && archive_params[:title].blank? + raise Discourse::InvalidParameters if !new_topic && archive_params[:topic_id].blank? - if !guardian.can_change_channel_status?(channel_from_params, :read_only) - raise Discourse::InvalidAccess.new(I18n.t("chat.errors.channel_cannot_be_archived")) - end + if !guardian.can_change_channel_status?(channel_from_params, :read_only) + raise Discourse::InvalidAccess.new(I18n.t("chat.errors.channel_cannot_be_archived")) + end - Chat::ChatChannelArchiveService.begin_archive_process( + begin + Chat::ChatChannelArchiveService.create_archive_process( chat_channel: channel_from_params, acting_user: current_user, - topic_params: { - topic_id: archive_params[:topic_id], - topic_title: archive_params[:title], - category_id: archive_params[:category_id], - tags: archive_params[:tags], - }, + topic_params: topic_params, ) + rescue Chat::ChatChannelArchiveService::ArchiveValidationError => err + return render json: failed_json.merge(errors: err.errors), status: 400 end render json: success_json end + + private + + def archive_params + @archive_params ||= + params + .require(:archive) + .tap do |ca| + ca.require(:type) + ca.permit(:title, :topic_id, :category_id, tags: []) + end + end + + def topic_params + @topic_params ||= { + topic_id: archive_params[:topic_id], + topic_title: archive_params[:title], + category_id: archive_params[:category_id], + tags: archive_params[:tags], + } + end end diff --git a/plugins/chat/app/models/chat_channel_archive.rb b/plugins/chat/app/models/chat_channel_archive.rb index e84cdb35e37..057af4e5bf9 100644 --- a/plugins/chat/app/models/chat_channel_archive.rb +++ b/plugins/chat/app/models/chat_channel_archive.rb @@ -13,6 +13,10 @@ class ChatChannelArchive < ActiveRecord::Base def failed? !complete? && self.archive_error.present? end + + def new_topic? + self.destination_topic_title.present? + end end # == Schema Information diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel-archive-status.js b/plugins/chat/assets/javascripts/discourse/components/chat-channel-archive-status.js index 94d27ed5a66..0f23a725e3d 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel-archive-status.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel-archive-status.js @@ -19,8 +19,11 @@ export default Component.extend({ "channel.archive_failed" ) channelArchiveFailedMessage() { + const translationKey = !this.channel.archive_topic_id + ? "chat.channel_status.archive_failed_no_topic" + : "chat.channel_status.archive_failed"; return htmlSafe( - I18n.t("chat.channel_status.archive_failed", { + I18n.t(translationKey, { completed: this.channel.archived_messages, total: this.channel.total_messages, topic_url: this._getTopicUrl(), @@ -50,6 +53,9 @@ export default Component.extend({ }, _getTopicUrl() { + if (!this.channel.archive_topic_id) { + return ""; + } return getURL(`/t/-/${this.channel.archive_topic_id}`); }, }); diff --git a/plugins/chat/config/locales/client.en.yml b/plugins/chat/config/locales/client.en.yml index 9f912699a8d..e7761687bc6 100644 --- a/plugins/chat/config/locales/client.en.yml +++ b/plugins/chat/config/locales/client.en.yml @@ -205,7 +205,8 @@ en: read_only: "Read Only" archived_header: "Channel is archived" archived: "Archived" - archive_failed: "Archive channel failed. %{completed}/%{total} messages have been archived in the destination topic. Press retry to attempt to complete the archive." + archive_failed: "Archive channel failed. %{completed}/%{total} messages have been archived. the destination topic. Press retry to attempt to complete the archive." + archive_failed_no_topic: "Archive channel failed. %{completed}/%{total} messages have been archived, the destination topic was not created. Press retry to attempt to complete the archive." archive_completed: "See the archive topic" closed_header: "Channel is closed" closed: "Closed" diff --git a/plugins/chat/config/locales/server.en.yml b/plugins/chat/config/locales/server.en.yml index 62c2229208b..9cdaf1c2d68 100644 --- a/plugins/chat/config/locales/server.en.yml +++ b/plugins/chat/config/locales/server.en.yml @@ -35,6 +35,15 @@ en: subject_template: "Chat channel archive failed" text_body_template: | Archiving the chat channel **\#%{channel_name}** has failed. %{messages_archived} messages have been archived. Partially archived messages were copied into the topic [%{topic_title}](%{topic_url}). Visit the channel at %{channel_url} to retry. + chat_channel_archive_failed_no_topic: + title: "Chat Channel Archive Failed" + subject_template: "Chat channel archive failed" + text_body_template: | + Archiving the chat channel **\#%{channel_name}** has failed. No messages have been archived. The topic was not created successfully for the following reasons: + + %{topic_validation_errors} + + Visit the channel at %{channel_url} to retry. chat: deleted_chat_username: deleted diff --git a/plugins/chat/lib/chat_channel_archive_service.rb b/plugins/chat/lib/chat_channel_archive_service.rb index 45196c1f937..0cd7a9c1d2b 100644 --- a/plugins/chat/lib/chat_channel_archive_service.rb +++ b/plugins/chat/lib/chat_channel_archive_service.rb @@ -2,9 +2,7 @@ ## # From time to time, site admins may choose to sunset a chat channel and archive -# the messages within. The main use case for this is a topic-based channel, but -# it can be used for category channels just fine. It cannot be used for DM channels -# in its current iteration. +# the messages within. It cannot be used for DM channels in its current iteration. # # To archive a channel, we mark it read_only first to prevent any further message # additions or changes, and create a record to track whether the archive topic @@ -17,9 +15,29 @@ class Chat::ChatChannelArchiveService ARCHIVED_MESSAGES_PER_POST = 100 - def self.begin_archive_process(chat_channel:, acting_user:, topic_params:) + class ArchiveValidationError < StandardError + attr_reader :errors + + def initialize(errors: []) + super + @errors = errors + end + end + + def self.create_archive_process(chat_channel:, acting_user:, topic_params:) return if ChatChannelArchive.exists?(chat_channel: chat_channel) + # Only need to validate topic params for a new topic, not an existing one. + if topic_params[:topic_id].blank? + valid, errors = + Chat::ChatChannelArchiveService.validate_topic_params( + Guardian.new(acting_user), + topic_params, + ) + + raise ArchiveValidationError.new(errors: errors) if !valid + end + ChatChannelArchive.transaction do chat_channel.read_only!(acting_user) @@ -48,6 +66,21 @@ class Chat::ChatChannelArchiveService chat_channel.chat_channel_archive end + def self.validate_topic_params(guardian, topic_params) + topic_creator = + TopicCreator.new( + Discourse.system_user, + guardian, + { + title: topic_params[:topic_title], + category: topic_params[:category_id], + tags: topic_params[:tags], + import_mode: true, + }, + ) + [topic_creator.valid?, topic_creator.errors.full_messages] + end + attr_reader :chat_channel_archive, :chat_channel, :chat_channel_title def initialize(chat_channel_archive) @@ -60,22 +93,22 @@ class Chat::ChatChannelArchiveService chat_channel_archive.update(archive_error: nil) begin - ensure_destination_topic_exists! + return if !ensure_destination_topic_exists! Rails.logger.info( "Creating posts from message batches for #{chat_channel_title} archive, #{chat_channel_archive.total_messages} messages to archive (#{chat_channel_archive.total_messages / ARCHIVED_MESSAGES_PER_POST} posts).", ) - # a batch should be idempotent, either the post is created and the + # A batch should be idempotent, either the post is created and the # messages are deleted or we roll back the whole thing. # - # at some point we may want to reconsider disabling post validations, + # At some point we may want to reconsider disabling post validations, # and add in things like dynamic resizing of the number of messages per - # post based on post length, but that can be done later + # post based on post length, but that can be done later. # - # another future improvement is to send a MessageBus message for each + # Another future improvement is to send a MessageBus message for each # completed batch, so the UI can receive updates and show a progress - # bar or something similar + # bar or something similar. chat_channel .chat_messages .find_in_batches(batch_size: ARCHIVED_MESSAGES_PER_POST) do |chat_messages| @@ -95,7 +128,7 @@ class Chat::ChatChannelArchiveService kick_all_users complete_archive rescue => err - notify_archiver(:failed, error: err) + notify_archiver(:failed, error_message: err.message) raise err end end @@ -144,29 +177,44 @@ class Chat::ChatChannelArchiveService }, ) - chat_channel_archive.update!(destination_topic: topic_creator.create) + if topic_creator.valid? + chat_channel_archive.update!(destination_topic: topic_creator.create) + else + Rails.logger.info("Destination topic for #{chat_channel_title} archive was not valid.") + notify_archiver( + :failed_no_topic, + error_message: topic_creator.errors.full_messages.join("\n"), + ) + end end - Rails.logger.info("Creating first post for #{chat_channel_title} archive.") - create_post( - I18n.t( - "chat.channel.archive.first_post_raw", - channel_name: chat_channel_title, - channel_url: chat_channel.url, - ), - ) + if chat_channel_archive.destination_topic.present? + Rails.logger.info("Creating first post for #{chat_channel_title} archive.") + create_post( + I18n.t( + "chat.channel.archive.first_post_raw", + channel_name: chat_channel_title, + channel_url: chat_channel.url, + ), + ) + end else Rails.logger.info("Topic already exists for #{chat_channel_title} archive.") end - update_destination_topic_status + if chat_channel_archive.destination_topic.present? + update_destination_topic_status + return true + end + + false end def update_destination_topic_status - # we only want to do this when the destination topic is new, not an + # We only want to do this when the destination topic is new, not an # existing topic, because we don't want to update the status unexpectedly # on an existing topic - if chat_channel_archive.destination_topic_title.present? + if chat_channel_archive.new_topic? if SiteSetting.chat_archive_destination_topic_status == "archived" chat_channel_archive.destination_topic.update!(archived: true) elsif SiteSetting.chat_archive_destination_topic_status == "closed" @@ -198,16 +246,17 @@ class Chat::ChatChannelArchiveService notify_archiver(:success) end - def notify_archiver(result, error: nil) + def notify_archiver(result, error_message: nil) base_translation_params = { channel_name: chat_channel_title, - topic_title: chat_channel_archive.destination_topic.title, - topic_url: chat_channel_archive.destination_topic.url, + topic_title: chat_channel_archive.destination_topic&.title, + topic_url: chat_channel_archive.destination_topic&.url, + topic_validation_errors: result == :failed_no_topic ? error_message : nil, } - if result == :failed + if result == :failed || result == :failed_no_topic Discourse.warn_exception( - error, + error_message, message: "Error when archiving chat channel #{chat_channel_title}.", env: { chat_channel_id: chat_channel.id, @@ -219,10 +268,17 @@ class Chat::ChatChannelArchiveService channel_url: chat_channel.url, messages_archived: chat_channel_archive.archived_messages, ) - chat_channel_archive.update(archive_error: error.message) + chat_channel_archive.update(archive_error: error_message) + message_translation_key = + case result + when :failed + :chat_channel_archive_failed + when :failed_no_topic + :chat_channel_archive_failed_no_topic + end SystemMessage.create_from_system_user( chat_channel_archive.archived_by, - :chat_channel_archive_failed, + message_translation_key, error_translation_params, ) else @@ -235,7 +291,7 @@ class Chat::ChatChannelArchiveService ChatPublisher.publish_archive_status( chat_channel, - archive_status: result, + archive_status: result != :success ? :failed : :success, archived_messages: chat_channel_archive.archived_messages, archive_topic_id: chat_channel_archive.destination_topic_id, total_messages: chat_channel_archive.total_messages, diff --git a/plugins/chat/spec/lib/chat_channel_archive_service_spec.rb b/plugins/chat/spec/lib/chat_channel_archive_service_spec.rb index 5c4c97c3926..2d79e85690e 100644 --- a/plugins/chat/spec/lib/chat_channel_archive_service_spec.rb +++ b/plugins/chat/spec/lib/chat_channel_archive_service_spec.rb @@ -12,11 +12,11 @@ describe Chat::ChatChannelArchiveService do let(:topic_params) { { topic_title: "This will be a new topic", category_id: category.id } } subject { Chat::ChatChannelArchiveService } - describe "#begin_archive_process" do + describe "#create_archive_process" do before { 3.times { Fabricate(:chat_message, chat_channel: channel) } } it "marks the channel as read_only" do - subject.begin_archive_process( + subject.create_archive_process( chat_channel: channel, acting_user: user, topic_params: topic_params, @@ -25,7 +25,7 @@ describe Chat::ChatChannelArchiveService do end it "creates the chat channel archive record to save progress and topic params" do - subject.begin_archive_process( + subject.create_archive_process( chat_channel: channel, acting_user: user, topic_params: topic_params, @@ -40,7 +40,7 @@ describe Chat::ChatChannelArchiveService do it "enqueues the archive job" do channel_archive = - subject.begin_archive_process( + subject.create_archive_process( chat_channel: channel, acting_user: user, topic_params: topic_params, @@ -56,13 +56,13 @@ describe Chat::ChatChannelArchiveService do end it "does nothing if there is already an archive record for the channel" do - subject.begin_archive_process( + subject.create_archive_process( chat_channel: channel, acting_user: user, topic_params: topic_params, ) expect { - subject.begin_archive_process( + subject.create_archive_process( chat_channel: channel, acting_user: user, topic_params: topic_params, @@ -74,7 +74,7 @@ describe Chat::ChatChannelArchiveService do new_message = Fabricate(:chat_message, chat_channel: channel) new_message.trash! channel_archive = - subject.begin_archive_process( + subject.create_archive_process( chat_channel: channel, acting_user: user, topic_params: topic_params, @@ -90,7 +90,7 @@ describe Chat::ChatChannelArchiveService do def start_archive @channel_archive = - subject.begin_archive_process( + subject.create_archive_process( chat_channel: channel, acting_user: user, topic_params: topic_params, @@ -167,6 +167,23 @@ describe Chat::ChatChannelArchiveService do ) end + it "does not continue archiving if the destination topic fails to be created" do + SiteSetting.max_emojis_in_title = 1 + + create_messages(3) && start_archive + @channel_archive.update!(destination_topic_title: "Wow this is the new title :tada: :joy:") + subject.new(@channel_archive).execute + expect(@channel_archive.reload.complete?).to eq(false) + expect(@channel_archive.reload.failed?).to eq(true) + expect(@channel_archive.archive_error).to eq("Title can't have more than 1 emoji") + + pm_topic = Topic.private_messages.last + expect(pm_topic.title).to eq( + I18n.t("system_messages.chat_channel_archive_failed.subject_template"), + ) + expect(pm_topic.first_post.raw).to include("Title can't have more than 1 emoji") + end + describe "channel members" do before do create_messages(3) diff --git a/plugins/chat/spec/requests/api/chat_channels_archives_controller_spec.rb b/plugins/chat/spec/requests/api/chat_channels_archives_controller_spec.rb index c209f5f4985..2acd568e9ec 100644 --- a/plugins/chat/spec/requests/api/chat_channels_archives_controller_spec.rb +++ b/plugins/chat/spec/requests/api/chat_channels_archives_controller_spec.rb @@ -98,6 +98,22 @@ RSpec.describe Chat::Api::ChatChannelsArchivesController do }.not_to change { ChatChannelArchive.count } end + context "when archiving to a new topic" do + it "returns validation errors if the topic is not valid" do + SiteSetting.max_emojis_in_title = 1 + new_topic_params_invalid = new_topic_params.dup + new_topic_params_invalid[:archive][ + :title + ] = "Some new topic with too many emoji :joy: :sob: :tada:" + sign_in(admin) + expect { + post "/chat/api/channels/#{channel.id}/archives", params: new_topic_params_invalid + }.not_to change { ChatChannelArchive.count } + expect(response.status).to eq(400) + expect(response.parsed_body["errors"]).to eq(["Title can't have more than 1 emoji"]) + end + end + describe "when retrying the archive process" do fab!(:channel) { Fabricate(:category_channel, chatable: category, status: :read_only) } fab!(:archive) do diff --git a/plugins/chat/spec/system/archive_channel_spec.rb b/plugins/chat/spec/system/archive_channel_spec.rb index 998045e1b57..bfbb33f3e94 100644 --- a/plugins/chat/spec/system/archive_channel_spec.rb +++ b/plugins/chat/spec/system/archive_channel_spec.rb @@ -65,6 +65,18 @@ RSpec.describe "Archive channel", type: :system, js: true do expect(page).to have_css(".chat-channel-archive-status") end + it "shows an error when the topic is invalid" do + chat.visit_channel_settings(channel_1) + click_button(I18n.t("js.chat.channel_settings.archive_channel")) + find("#split-topic-name").fill_in( + with: "An interesting topic for cats :cat: :cat2: :smile_cat:", + ) + click_button(I18n.t("js.chat.channel_archive.title")) + + expect(page).not_to have_content(I18n.t("js.chat.channel_archive.process_started")) + expect(page).to have_content("Title can't have more than 1 emoji") + end + context "when archived channels had unreads" do before { channel_1.add(current_user) }