Merge pull request #2597 from basecamp/import-improvements

Import improvements

app/models/account/data_transfer/account_record_set.rb

@@ -31,7 +31,7 @@ class Account::DataTransfer::AccountRecordSet < Account::DataTransfer::RecordSet
       account_data = load(files.first)
       join_code_data = account_data.delete("join_code")
 
-      account.update!(name: account_data.fetch("name"))
+      account.update!(name: account_data.fetch("name"), cards_count: account_data.fetch("cards_count", 0))
       account.join_code.update!(join_code_data.slice("usage_count", "usage_limit"))
       account.join_code.update(code: join_code_data.fetch("code"))
     end

app/models/account/data_transfer/action_text/rich_text_record_set.rb

@@ -1,4 +1,4 @@
-class Account::DataTransfer::ActionTextRichTextRecordSet < Account::DataTransfer::RecordSet
+class Account::DataTransfer::ActionText::RichTextRecordSet < Account::DataTransfer::RecordSet
   ATTRIBUTES = %w[
     account_id
     body
@@ -11,16 +11,16 @@ class Account::DataTransfer::ActionTextRichTextRecordSet < Account::DataTransfer
   ].freeze
 
   def initialize(account)
-    super(account: account, model: ActionText::RichText)
+    super(account: account, model: ::ActionText::RichText)
   end
 
   private
     def records
-      ActionText::RichText.where(account: account)
+      ::ActionText::RichText.where(account: account)
     end
 
     def export_record(rich_text)
-      data = rich_text.as_json.merge("body" => convert_sgids_to_gids(rich_text.body))
+      data = rich_text.as_json.merge("body" => transform_body_for_export(rich_text.body))
       zip.add_file "data/action_text_rich_texts/#{rich_text.id}.json", data.to_json
     end
 
@@ -31,11 +31,11 @@ class Account::DataTransfer::ActionTextRichTextRecordSet < Account::DataTransfer
     def import_batch(files)
       batch_data = files.map do |file|
         data = load(file)
-        data["body"] = convert_gids_to_sgids(data["body"])
+        data["body"] = transform_body_for_import(data["body"])
         data.slice(*ATTRIBUTES).merge("account_id" => account.id)
       end
 
-      ActionText::RichText.insert_all!(batch_data)
+      ::ActionText::RichText.insert_all!(batch_data)
     end
 
     def check_record(file_path)
@@ -54,11 +54,16 @@ class Account::DataTransfer::ActionTextRichTextRecordSet < Account::DataTransfer
       check_associations_dont_exist(data)
     end
 
-    def convert_sgids_to_gids(content)
+    def transform_body_for_export(content)
       return nil if content.blank?
 
+      html = convert_sgids_to_gids(content)
+      relativize_urls(html)
+    end
+
+    def convert_sgids_to_gids(content)
       content.send(:attachment_nodes).each do |node|
-        sgid = SignedGlobalID.parse(node["sgid"], for: ActionText::Attachable::LOCATOR_NAME)
+        sgid = SignedGlobalID.parse(node["sgid"], for: ::ActionText::Attachable::LOCATOR_NAME)
 
         record = begin
           sgid&.find
@@ -75,11 +80,35 @@ class Account::DataTransfer::ActionTextRichTextRecordSet < Account::DataTransfer
       content.fragment.source.to_html
     end
 
-    def convert_gids_to_sgids(html)
-      return html if html.blank?
+    def relativize_urls(html)
+      host = Rails.application.routes.default_url_options[:host]
+      return html unless host
 
       fragment = Nokogiri::HTML.fragment(html)
 
+      fragment.css("a[href]").each do |link|
+        uri = URI.parse(link["href"]) rescue nil
+
+        if uri.respond_to?(:host) && uri.host == host
+          link["href"] = uri.path
+          link["href"] += "?#{uri.query}" if uri.query
+          link["href"] += "##{uri.fragment}" if uri.fragment
+        end
+      end
+
+      fragment.to_html
+    end
+
+    def transform_body_for_import(body)
+      return body if body.blank?
+
+      Nokogiri::HTML.fragment(body)
+        .then { convert_gids_to_sgids(it) }
+        .then { replace_account_slugs(it) }
+        .to_html
+    end
+
+    def convert_gids_to_sgids(fragment)
       fragment.css("action-text-attachment[gid]").each do |node|
         gid = GlobalID.parse(node["gid"])
 
@@ -97,6 +126,20 @@ class Account::DataTransfer::ActionTextRichTextRecordSet < Account::DataTransfer
         end
       end
 
-      fragment.to_html
+      fragment
+    end
+
+    def replace_account_slugs(fragment)
+      fragment.css("a[href]").each do |link|
+        match = link["href"].match(AccountSlug::PATH_INFO_MATCH)
+
+        if match
+          path = match.post_match.presence || "/"
+          valid_path = Rails.application.routes.recognize_path(path) rescue nil
+          link["href"] = "#{account.slug}#{path}" if valid_path
+        end
+      end
+
+      fragment
     end
 end

app/models/account/data_transfer/active_storage/blob_record_set.rb

@@ -1,9 +1,9 @@
-class Account::DataTransfer::ActiveStorageBlobRecordSet < Account::DataTransfer::RecordSet
+class Account::DataTransfer::ActiveStorage::BlobRecordSet < Account::DataTransfer::RecordSet
   def initialize(account)
     super(
       account: account,
-      model: ActiveStorage::Blob,
-      attributes: ActiveStorage::Blob.column_names - %w[service_name]
+      model: ::ActiveStorage::Blob,
+      attributes: ::ActiveStorage::Blob.column_names - %w[service_name]
     )
   end
 
@@ -13,7 +13,7 @@ class Account::DataTransfer::ActiveStorageBlobRecordSet < Account::DataTransfer:
         data = load(file)
         data.slice(*attributes).merge(
           "account_id" => account.id,
-          "service_name" => ActiveStorage::Blob.service.name
+          "service_name" => ::ActiveStorage::Blob.service.name
         )
       end
 

app/models/account/data_transfer/active_storage/file_record_set.rb

@@ -1,11 +1,11 @@
-class Account::DataTransfer::BlobFileRecordSet < Account::DataTransfer::RecordSet
+class Account::DataTransfer::ActiveStorage::FileRecordSet < Account::DataTransfer::RecordSet
   def initialize(account)
-    super(account: account, model: ActiveStorage::Blob)
+    super(account: account, model: ::ActiveStorage::Blob)
   end
 
   private
     def records
-      ActiveStorage::Blob.where(account: account)
+      ::ActiveStorage::Blob.where(account: account)
     end
 
     def export_record(blob)
@@ -23,7 +23,7 @@ class Account::DataTransfer::BlobFileRecordSet < Account::DataTransfer::RecordSe
     def import_batch(files)
       files.each do |file|
         key = File.basename(file)
-        blob = ActiveStorage::Blob.find_by(key: key, account: account)
+        blob = ::ActiveStorage::Blob.find_by(key: key, account: account)
         next unless blob
 
         zip.read(file) do |stream|
@@ -35,7 +35,7 @@ class Account::DataTransfer::BlobFileRecordSet < Account::DataTransfer::RecordSe
     def check_record(file_path)
       key = File.basename(file_path)
 
-      unless zip.exists?("data/active_storage_blobs/#{key}.json") || ActiveStorage::Blob.exists?(key: key, account: account)
+      unless zip.exists?("data/active_storage_blobs/#{key}.json") || ::ActiveStorage::Blob.exists?(key: key, account: account)
         # File exists without corresponding blob record - could be orphaned or blob not yet imported
         # We allow this since blob metadata is imported before files
       end

app/models/account/data_transfer/manifest.rb

@@ -35,10 +35,10 @@ class Account::DataTransfer::Manifest
           ::Filter, ::Webhook::DelinquencyTracker, ::Event,
           ::Notification, ::Notification::Bundle, ::Webhook::Delivery
         ),
-        Account::DataTransfer::ActiveStorageBlobRecordSet.new(account),
+        Account::DataTransfer::ActiveStorage::BlobRecordSet.new(account),
         *build_record_sets(::ActiveStorage::Attachment),
-        Account::DataTransfer::ActionTextRichTextRecordSet.new(account),
-        Account::DataTransfer::BlobFileRecordSet.new(account)
+        Account::DataTransfer::ActionText::RichTextRecordSet.new(account),
+        Account::DataTransfer::ActiveStorage::FileRecordSet.new(account)
       ]
     end
 

app/models/account/import.rb

@@ -48,6 +48,7 @@ class Account::Import < ApplicationRecord
     end
 
     add_importer_to_all_access_boards
+    reconcile_cards_count
     reconcile_account_storage
 
     mark_completed
@@ -78,6 +79,10 @@ class Account::Import < ApplicationRecord
       ImportMailer.failed(self).deliver_later
     end
 
+    def reconcile_cards_count
+      account.update_column :cards_count, [ account.cards_count, account.cards.maximum(:number).to_i ].max
+    end
+
     def add_importer_to_all_access_boards
       importer = account.users.find_by!(identity: identity)
 

test/models/account/data_transfer/action_text/rich_text_record_set_test.rb

@@ -0,0 +1,181 @@
+require "test_helper"
+
+class Account::DataTransfer::ActionText::RichTextRecordSetTest < ActiveSupport::TestCase
+  test "check rejects ActionText record referencing existing card in another account" do
+    importing_account = Account.create!(name: "Importing Account", external_account_id: 99999999)
+
+    victim_card = cards(:logo)
+    assert_not_equal importing_account.id, victim_card.account_id, "Card must belong to a different account"
+
+    # Create a malicious ActionText record that points to the victim's card
+    malicious_action_text_data = {
+      "id" => "malicious_action_text_id_12345",
+      "account_id" => importing_account.id,
+      "record_type" => "Card",
+      "record_id" => victim_card.id,
+      "name" => "description",
+      "body" => "<p>Injected content from attacker</p>",
+      "created_at" => Time.current.iso8601,
+      "updated_at" => Time.current.iso8601
+    }
+
+    tempfile = Tempfile.new([ "malicious_import", ".zip" ])
+    tempfile.binmode
+
+    writer = ZipFile::Writer.new(tempfile)
+    writer.add_file("data/action_text_rich_texts/#{malicious_action_text_data['id']}.json", malicious_action_text_data.to_json)
+    writer.close
+    tempfile.rewind
+
+    reader = ZipFile::Reader.new(tempfile)
+
+    record_set = Account::DataTransfer::ActionText::RichTextRecordSet.new(importing_account)
+
+    error = assert_raises(Account::DataTransfer::RecordSet::IntegrityError) do
+      record_set.check(from: reader)
+    end
+
+    assert_match(/references existing record.*Card.*#{victim_card.id}/i, error.message)
+  ensure
+    tempfile&.close
+    tempfile&.unlink
+    importing_account&.destroy
+  end
+
+  test "transform_body_for_import skips GIDs belonging to another account" do
+    victim_tag = tags(:web)
+    attacker_account = accounts(:initech)
+    assert_not_equal attacker_account.id, victim_tag.account_id
+
+    cross_tenant_gid = victim_tag.to_global_id.to_s
+    html = %(<action-text-attachment gid="#{cross_tenant_gid}"></action-text-attachment>)
+
+    record_set = Account::DataTransfer::ActionText::RichTextRecordSet.new(attacker_account)
+    result = record_set.send(:transform_body_for_import, html)
+
+    assert_no_match(/sgid=/, result, "Cross-tenant GID must not be converted to SGID")
+    assert_match(/gid=/, result, "Original GID should remain unconverted")
+  end
+
+  test "transform_body_for_import converts GIDs belonging to the same account" do
+    own_tag = tags(:web)
+    own_account = accounts(:"37s")
+    assert_equal own_account.id, own_tag.account_id
+
+    same_account_gid = own_tag.to_global_id.to_s
+    html = %(<action-text-attachment gid="#{same_account_gid}"></action-text-attachment>)
+
+    record_set = Account::DataTransfer::ActionText::RichTextRecordSet.new(own_account)
+    result = record_set.send(:transform_body_for_import, html)
+
+    assert_match(/sgid=/, result, "Same-account GID should be converted to SGID")
+    assert_no_match(/ gid=/, result, "GID should be removed after SGID conversion")
+  end
+
+  test "transform_body_for_import handles non-existent record GIDs gracefully" do
+    nonexistent_gid = "gid://fizzy/Tag/00000000000000000000000000"
+    html = %(<action-text-attachment gid="#{nonexistent_gid}"></action-text-attachment>)
+
+    record_set = Account::DataTransfer::ActionText::RichTextRecordSet.new(accounts(:"37s"))
+    result = record_set.send(:transform_body_for_import, html)
+
+    assert_no_match(/sgid=/, result, "Non-existent record should not produce SGID")
+  end
+
+  test "replace_account_slugs rewrites relative URLs with account slug" do
+    target_account = accounts(:"37s")
+    source_slug = "9999999"
+    target_slug = AccountSlug.encode(target_account.external_account_id)
+
+    html = %(<p>See <a href="/#{source_slug}/cards/42">card 42</a></p>)
+
+    record_set = Account::DataTransfer::ActionText::RichTextRecordSet.new(target_account)
+    result = record_set.send(:transform_body_for_import, html)
+
+    assert_includes result, "/#{target_slug}/cards/42"
+    assert_not_includes result, source_slug
+  end
+
+  test "replace_account_slugs leaves absolute URLs alone" do
+    target_account = accounts(:"37s")
+
+    html = %(<p>See <a href="https://fizzy.app/9999999/boards/7">board</a></p>)
+
+    record_set = Account::DataTransfer::ActionText::RichTextRecordSet.new(target_account)
+    result = record_set.send(:transform_body_for_import, html)
+
+    assert_includes result, "https://fizzy.app/9999999/boards/7"
+  end
+
+  test "replace_account_slugs leaves plain text alone" do
+    target_account = accounts(:"37s")
+
+    html = "<p>Nothing to rewrite here</p>"
+
+    record_set = Account::DataTransfer::ActionText::RichTextRecordSet.new(target_account)
+    result = record_set.send(:transform_body_for_import, html)
+
+    assert_equal html, result
+  end
+
+  test "relativize_urls strips instance host from absolute URLs" do
+    with_default_url_host("fizzy.example.com") do
+      record_set = Account::DataTransfer::ActionText::RichTextRecordSet.new(accounts(:"37s"))
+
+      html = %(<p>See <a href="https://fizzy.example.com/123/cards/42">card</a></p>)
+      result = record_set.send(:relativize_urls, html)
+
+      assert_includes result, %(/123/cards/42)
+      assert_not_includes result, "fizzy.example.com"
+    end
+  end
+
+  test "relativize_urls preserves query and fragment" do
+    with_default_url_host("fizzy.example.com") do
+      record_set = Account::DataTransfer::ActionText::RichTextRecordSet.new(accounts(:"37s"))
+
+      html = %(<p><a href="https://fizzy.example.com/123/cards/42?tab=comments#comment_1">link</a></p>)
+      result = record_set.send(:relativize_urls, html)
+
+      assert_includes result, "/123/cards/42?tab=comments#comment_1"
+      assert_not_includes result, "fizzy.example.com"
+    end
+  end
+
+  test "relativize_urls leaves external URLs alone" do
+    with_default_url_host("fizzy.example.com") do
+      record_set = Account::DataTransfer::ActionText::RichTextRecordSet.new(accounts(:"37s"))
+
+      html = %(<p><a href="https://github.com/some/repo">link</a></p>)
+      result = record_set.send(:relativize_urls, html)
+
+      assert_includes result, "https://github.com/some/repo"
+    end
+  end
+
+  test "relativize_urls is a no-op when host is not configured" do
+    with_default_url_host(nil) do
+      record_set = Account::DataTransfer::ActionText::RichTextRecordSet.new(accounts(:"37s"))
+
+      html = %(<p><a href="https://fizzy.example.com/123/cards/42">link</a></p>)
+      result = record_set.send(:relativize_urls, html)
+
+      assert_includes result, "https://fizzy.example.com/123/cards/42"
+    end
+  end
+
+  private
+    def with_default_url_host(host)
+      options = Rails.application.routes.default_url_options
+      had_key = options.key?(:host)
+      original = options[:host]
+      options[:host] = host
+      yield
+    ensure
+      if had_key
+        options[:host] = original
+      else
+        options.delete(:host)
+      end
+    end
+end

test/models/account/data_transfer/action_text_rich_text_record_set_test.rb

@@ -1,84 +0,0 @@
-require "test_helper"
-
-class Account::DataTransfer::ActionTextRichTextRecordSetTest < ActiveSupport::TestCase
-  test "check rejects ActionText record referencing existing card in another account" do
-    importing_account = Account.create!(name: "Importing Account", external_account_id: 99999999)
-
-    victim_card = cards(:logo)
-    assert_not_equal importing_account.id, victim_card.account_id, "Card must belong to a different account"
-
-    # Create a malicious ActionText record that points to the victim's card
-    malicious_action_text_data = {
-      "id" => "malicious_action_text_id_12345",
-      "account_id" => importing_account.id,
-      "record_type" => "Card",
-      "record_id" => victim_card.id,
-      "name" => "description",
-      "body" => "<p>Injected content from attacker</p>",
-      "created_at" => Time.current.iso8601,
-      "updated_at" => Time.current.iso8601
-    }
-
-    tempfile = Tempfile.new([ "malicious_import", ".zip" ])
-    tempfile.binmode
-
-    writer = ZipFile::Writer.new(tempfile)
-    writer.add_file("data/action_text_rich_texts/#{malicious_action_text_data['id']}.json", malicious_action_text_data.to_json)
-    writer.close
-    tempfile.rewind
-
-    reader = ZipFile::Reader.new(tempfile)
-
-    record_set = Account::DataTransfer::ActionTextRichTextRecordSet.new(importing_account)
-
-    error = assert_raises(Account::DataTransfer::RecordSet::IntegrityError) do
-      record_set.check(from: reader)
-    end
-
-    assert_match(/references existing record.*Card.*#{victim_card.id}/i, error.message)
-  ensure
-    tempfile&.close
-    tempfile&.unlink
-    importing_account&.destroy
-  end
-
-  test "convert_gids_to_sgids skips GIDs belonging to another account" do
-    victim_tag = tags(:web)
-    attacker_account = accounts(:initech)
-    assert_not_equal attacker_account.id, victim_tag.account_id
-
-    cross_tenant_gid = victim_tag.to_global_id.to_s
-    html = %(<action-text-attachment gid="#{cross_tenant_gid}"></action-text-attachment>)
-
-    record_set = Account::DataTransfer::ActionTextRichTextRecordSet.new(attacker_account)
-    result = record_set.send(:convert_gids_to_sgids, html)
-
-    assert_no_match(/sgid=/, result, "Cross-tenant GID must not be converted to SGID")
-    assert_match(/gid=/, result, "Original GID should remain unconverted")
-  end
-
-  test "convert_gids_to_sgids converts GIDs belonging to the same account" do
-    own_tag = tags(:web)
-    own_account = accounts(:"37s")
-    assert_equal own_account.id, own_tag.account_id
-
-    same_account_gid = own_tag.to_global_id.to_s
-    html = %(<action-text-attachment gid="#{same_account_gid}"></action-text-attachment>)
-
-    record_set = Account::DataTransfer::ActionTextRichTextRecordSet.new(own_account)
-    result = record_set.send(:convert_gids_to_sgids, html)
-
-    assert_match(/sgid=/, result, "Same-account GID should be converted to SGID")
-    assert_no_match(/ gid=/, result, "GID should be removed after SGID conversion")
-  end
-
-  test "convert_gids_to_sgids handles non-existent record GIDs gracefully" do
-    nonexistent_gid = "gid://fizzy/Tag/00000000000000000000000000"
-    html = %(<action-text-attachment gid="#{nonexistent_gid}"></action-text-attachment>)
-
-    record_set = Account::DataTransfer::ActionTextRichTextRecordSet.new(accounts(:"37s"))
-    result = record_set.send(:convert_gids_to_sgids, html)
-
-    assert_no_match(/sgid=/, result, "Non-existent record should not produce SGID")
-  end
-end

test/models/account/import_test.rb

@@ -62,6 +62,36 @@ class Account::ImportTest < ActiveSupport::TestCase
     export_tempfile&.unlink
   end
 
+  test "import reconciles cards count so new cards get correct numbers" do
+    source_account = accounts("37s")
+    exporter = users(:david)
+    identity = exporter.identity
+    max_card_number = source_account.cards.maximum(:number)
+
+    export = Account::Export.create!(account: source_account, user: exporter)
+    export.build
+
+    export_tempfile = Tempfile.new([ "export", ".zip" ])
+    export.file.open { |f| FileUtils.cp(f.path, export_tempfile.path) }
+
+    source_account.destroy!
+
+    target_account = Account.create_with_owner(account: { name: "Import Test" }, owner: { identity: identity, name: exporter.name })
+    import = Account::Import.create!(identity: identity, account: target_account)
+    Current.set(account: target_account) do
+      import.file.attach(io: File.open(export_tempfile.path), filename: "export.zip", content_type: "application/zip")
+    end
+
+    import.check
+    import.process
+
+    target_account.reload
+    assert_operator target_account.cards_count, :>=, max_card_number
+  ensure
+    export_tempfile&.close
+    export_tempfile&.unlink
+  end
+
   test "check sets no failure_reason for unexpected errors" do
     import = Account::Import.create!(identity: identities(:david), account: Account.create!(name: "Import Test"))