FIX: delete system generated message when user_export record is deleted (#7595)

FIX: system generated message for user export should be closed by default

app/jobs/onceoff/clean_up_user_export_topics.rb

@@ -0,0 +1,36 @@
+# frozen_string_literal: true
+
+module Jobs
+  class CleanUpUserExportTopics < Jobs::Onceoff
+    def execute_onceoff(args)
+      translated_keys = []
+      I18n.available_locales.each do |l|
+        translated_keys << I18n.with_locale(l.to_sym) { I18n.t("system_messages.csv_export_succeeded.subject_template") }
+      end
+      translated_keys = translated_keys.uniq
+
+      slugs = []
+      translated_keys.each do |k|
+        slugs << "%-#{Slug.for(k.gsub('[%{export_title}]', ''))}"
+      end
+      # "[%{export_title}] 資料匯出已完成" gets converted to "%-topic", do not match that slug.
+      slugs = slugs.reject { |s| s == "%-topic" }
+
+      topics = Topic.with_deleted.where(<<~SQL, slugs, 2.days.ago)
+        slug LIKE ANY(ARRAY[?]) AND
+        archetype = 'private_message' AND
+        subtype = 'system_message' AND
+        posts_count = 1 AND
+        created_at < ? AND
+        user_id = -1
+      SQL
+
+      topics.each do |t|
+        Topic.transaction do
+          t.posts.first.destroy!
+          t.destroy!
+        end
+      end
+    end
+  end
+end

app/jobs/regular/export_csv_file.rb

@@ -87,7 +87,12 @@ module Jobs
       end
 
     ensure
-      notify_user(download_link, file_name, file_size, export_title)
+      post = notify_user(download_link, file_name, file_size, export_title)
+      if user_export.present? && post.present?
+        topic = post.topic
+        user_export.update_columns(topic_id: topic.id)
+        topic.update_status('closed', true, Discourse.system_user)
+      end
     end
 
     def user_archive_export
@@ -384,8 +389,9 @@ module Jobs
     end
 
     def notify_user(download_link, file_name, file_size, export_title)
+      post = nil
       if @current_user
-        if download_link.present?
+        post = if download_link.present?
           SystemMessage.create_from_system_user(
             @current_user,
             :csv_export_succeeded,
@@ -398,6 +404,7 @@ module Jobs
           SystemMessage.create_from_system_user(@current_user, :csv_export_failed)
         end
       end
+      post
     end
   end
 end

app/models/topic_custom_field.rb

@@ -18,5 +18,6 @@ end
 # Indexes
 #
 #  index_topic_custom_fields_on_topic_id_and_name  (topic_id,name)
+#  index_topic_custom_fields_on_value              (value) UNIQUE WHERE ((name)::text = 'commit hash'::text)
 #  topic_custom_fields_value_key_idx               (value,name) WHERE ((value IS NOT NULL) AND (char_length(value) < 400))
 #

app/models/user_export.rb

@@ -3,6 +3,7 @@
 class UserExport < ActiveRecord::Base
   belongs_to :user
   belongs_to :upload, dependent: :destroy
+  belongs_to :topic, dependent: :destroy
 
   around_destroy :ignore_missing_post_uploads
 
@@ -14,7 +15,14 @@ class UserExport < ActiveRecord::Base
 
   def self.remove_old_exports
     UserExport.where('created_at < ?', 2.days.ago).find_each do |user_export|
-      user_export.destroy!
+      UserExport.transaction do
+        begin
+          Post.where(topic_id: user_export.topic_id).find_each { |p| p.destroy! }
+          user_export.destroy!
+        rescue => e
+          Rails.logger.warn("Failed to remove user_export record with id #{user_export.id}: #{e.message}\n#{e.backtrace.join("\n")}")
+        end
+      end
     end
   end
 
@@ -34,4 +42,5 @@ end
 #  created_at :datetime         not null
 #  updated_at :datetime         not null
 #  upload_id  :integer
+#  topic_id   :integer
 #

db/migrate/20190523093215_add_topic_id_to_user_exports.rb

@@ -0,0 +1,9 @@
+class AddTopicIdToUserExports < ActiveRecord::Migration[5.2]
+  def up
+    add_column :user_exports, :topic_id, :integer
+  end
+
+  def down
+    remove_column :user_exports, :topic_id
+  end
+end

spec/jobs/clean_up_user_export_topics_spec.rb

@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe Jobs::CleanUpUserExportTopics do
+  fab!(:user) { Fabricate(:user) }
+
+  it 'should delete ancient user export system messages' do
+    post_en = SystemMessage.create_from_system_user(
+      user,
+      :csv_export_succeeded,
+      download_link: "http://example.com/download",
+      file_name: "xyz_en.gz",
+      file_size: "55",
+      export_title: "user_archive"
+    )
+    topic_en = post_en.topic
+    topic_en.update!(created_at: 5.days.ago)
+
+    I18n.locale = :fr
+    post_fr = SystemMessage.create_from_system_user(
+      user,
+      :csv_export_succeeded,
+      download_link: "http://example.com/download",
+      file_name: "xyz_fr.gz",
+      file_size: "56",
+      export_title: "user_archive"
+    )
+    topic_fr = post_fr.topic
+    topic_fr.update!(created_at: 5.days.ago)
+
+    described_class.new.execute_onceoff({})
+
+    expect(Topic.with_deleted.exists?(id: topic_en.id)).to eq(false)
+    expect(Topic.with_deleted.exists?(id: topic_fr.id)).to eq(false)
+  end
+end

spec/jobs/export_csv_file_spec.rb

@@ -15,12 +15,14 @@ describe Jobs::ExportCsvFile do
       begin
         Jobs::ExportCsvFile.new.execute(user_id: user.id, entity: "user_archive")
 
-        expect(user.topics_allowed.last.title).to eq(I18n.t(
+        system_message = user.topics_allowed.last
+        expect(system_message.title).to eq(I18n.t(
           "system_messages.csv_export_succeeded.subject_template",
           export_title: "User Archive"
         ))
-
-        expect(user.topics_allowed.last.first_post.raw).to include("user-archive-john_doe-")
+        expect(system_message.first_post.raw).to include("user-archive-john_doe-")
+        expect(system_message.id).to eq(UserExport.last.topic_id)
+        expect(system_message.closed).to eq(true)
       ensure
         user.uploads.each(&:destroy!)
       end

spec/models/user_export_spec.rb

@@ -8,18 +8,23 @@ RSpec.describe UserExport do
   describe '.remove_old_exports' do
     it 'should remove the right records' do
       csv_file_1 = Fabricate(:upload, created_at: 3.days.ago)
+      topic_1 = Fabricate(:topic, created_at: 3.days.ago)
+      post_1 = Fabricate(:post, topic: topic_1)
       export = UserExport.create!(
         file_name: "test",
         user: user,
         upload_id: csv_file_1.id,
+        topic_id: topic_1.id,
         created_at: 3.days.ago
       )
 
       csv_file_2 = Fabricate(:upload, created_at: 1.day.ago)
+      topic_2 = Fabricate(:topic, created_at: 1.day.ago)
       export2 = UserExport.create!(
         file_name: "test2",
         user: user,
         upload_id: csv_file_2.id,
+        topic_id: topic_2.id,
         created_at: 1.day.ago
       )
 
@@ -29,8 +34,11 @@ RSpec.describe UserExport do
 
       expect(UserExport.exists?(id: export.id)).to eq(false)
       expect(Upload.exists?(id: csv_file_1.id)).to eq(false)
+      expect(Topic.with_deleted.exists?(id: topic_1.id)).to eq(false)
+      expect(Post.with_deleted.exists?(id: post_1.id)).to eq(false)
       expect(UserExport.exists?(id: export2.id)).to eq(true)
       expect(Upload.exists?(id: csv_file_2.id)).to eq(true)
+      expect(Topic.exists?(id: topic_2.id)).to eq(true)
     end
   end