diff --git a/app/mailers/import_mailer.rb b/app/mailers/import_mailer.rb index c626ddb2e..3ab25e481 100644 --- a/app/mailers/import_mailer.rb +++ b/app/mailers/import_mailer.rb @@ -4,7 +4,8 @@ class ImportMailer < ApplicationMailer mail to: identity.email_address, subject: "Your Fizzy account import is done" end - def failed(identity) - mail to: identity.email_address, subject: "Your Fizzy account import failed" + def failed(import) + @import = import + mail to: import.identity.email_address, subject: "Your Fizzy account import failed" end end diff --git a/app/models/account/data_transfer/record_set.rb b/app/models/account/data_transfer/record_set.rb index b7955932a..8eeafbe10 100644 --- a/app/models/account/data_transfer/record_set.rb +++ b/app/models/account/data_transfer/record_set.rb @@ -1,5 +1,6 @@ class Account::DataTransfer::RecordSet class IntegrityError < StandardError; end + class ConflictError < IntegrityError; end IMPORT_BATCH_SIZE = 100 @@ -93,7 +94,7 @@ class Account::DataTransfer::RecordSet end if model.exists?(id: data["id"]) - raise IntegrityError, "#{model} record with ID #{data['id']} already exists" + raise ConflictError, "#{model} record with ID #{data['id']} already exists" end check_associations_dont_exist(data) @@ -118,7 +119,7 @@ class Account::DataTransfer::RecordSet end if associated_class.exists?(id: associated_id) - raise IntegrityError, "#{model} record references existing #{association.name} (#{associated_class}) with ID #{associated_id}" + raise ConflictError, "#{model} record references existing #{association.name} (#{associated_class}) with ID #{associated_id}" end end diff --git a/app/models/account/import.rb b/app/models/account/import.rb index 814e6b00c..14ee376ea 100644 --- a/app/models/account/import.rb +++ b/app/models/account/import.rb @@ -7,6 +7,7 @@ class Account::Import < ApplicationRecord has_one_attached :file enum :status, %w[ pending processing completed failed ].index_by(&:itself), default: :pending + enum :failure_reason, %w[ conflict invalid_export ].index_by(&:itself), prefix: :failed_due_to, scopes: false scope :expired, -> { where(completed_at: ...24.hours.ago).or(where(status: :failed, created_at: ...7.days.ago)) } @@ -26,6 +27,12 @@ class Account::Import < ApplicationRecord record_set.check(from: zip, start: last_id, callback: callback) end end + rescue Account::DataTransfer::RecordSet::ConflictError => e + mark_as_failed(:conflict) + raise e + rescue Account::DataTransfer::RecordSet::IntegrityError, ZipFile::InvalidFileError => e + mark_as_failed(:invalid_export) + raise e rescue => e mark_as_failed raise e @@ -44,6 +51,12 @@ class Account::Import < ApplicationRecord reconcile_account_storage mark_completed + rescue Account::DataTransfer::RecordSet::ConflictError => e + mark_as_failed(:conflict) + raise e + rescue Account::DataTransfer::RecordSet::IntegrityError, ZipFile::InvalidFileError => e + mark_as_failed(:invalid_export) + raise e rescue => e mark_as_failed raise e @@ -60,9 +73,9 @@ class Account::Import < ApplicationRecord ImportMailer.completed(identity, account).deliver_later end - def mark_as_failed - failed! - ImportMailer.failed(identity).deliver_later + def mark_as_failed(failure_reason = nil) + update!(status: :failed, failure_reason: failure_reason) + ImportMailer.failed(self).deliver_later end def add_importer_to_all_access_boards diff --git a/app/views/account/imports/show.html.erb b/app/views/account/imports/show.html.erb index 26dbf88d5..a6d966059 100644 --- a/app/views/account/imports/show.html.erb +++ b/app/views/account/imports/show.html.erb @@ -13,7 +13,13 @@ <%= link_to "Go to your account", landing_url(script_name: @import.account.slug), class: "btn btn--link center txt-medium" %> <% when "failed" %>

Your import failed.

-

This may be due to corrupted export data or a conflict with existing data. Please try again with a fresh export.

+ <% if @import.failed_due_to_conflict? %> +

It looks like the account you are trying to import already exists.

+ <% elsif @import.failed_due_to_invalid_export? %> +

The ZIP file isn't a Fizzy account export.

+ <% else %> +

This may be due to corrupted export data or a conflict with existing data. Please try again with a fresh export.

+ <% end %> <%= link_to "Try again", new_account_import_path, class: "btn btn--plain center txt-medium" %> <% end %> diff --git a/app/views/mailers/import_mailer/failed.html.erb b/app/views/mailers/import_mailer/failed.html.erb index c7b6d9b5c..6aa9fab0a 100644 --- a/app/views/mailers/import_mailer/failed.html.erb +++ b/app/views/mailers/import_mailer/failed.html.erb @@ -1,8 +1,11 @@

Unfortunately, we couldn't import your Fizzy account.

-

-This may be due to corrupted export data or a conflict with existing data. -Please try again with a fresh export, or reach out for help if the problem persists. -

+<% if @import.failed_due_to_conflict? %> +

It looks like the account you are trying to import already exists.

+<% elsif @import.failed_due_to_invalid_export? %> +

The ZIP file isn't a Fizzy account export.

+<% else %> +

This may be due to corrupted export data or a conflict with existing data. Please try again with a fresh export, or reach out for help if the problem persists.

+<% end %> diff --git a/app/views/mailers/import_mailer/failed.text.erb b/app/views/mailers/import_mailer/failed.text.erb index 82e3b772a..1895f0d5d 100644 --- a/app/views/mailers/import_mailer/failed.text.erb +++ b/app/views/mailers/import_mailer/failed.text.erb @@ -1,6 +1,11 @@ Unfortunately, we couldn't import your Fizzy account. -This may be due to corrupted export data or a conflict with existing data. -Please try again with a fresh export, or reach out for help if the problem persists. +<% if @import.failed_due_to_conflict? -%> +It looks like the account you are trying to import already exists. +<% elsif @import.failed_due_to_invalid_export? -%> +The ZIP file isn't a Fizzy account export. +<% else -%> +This may be due to corrupted export data or a conflict with existing data. Please try again with a fresh export, or reach out for help if the problem persists. +<% end -%> Need help? Send us an email at support@fizzy.do diff --git a/db/migrate/20260211122517_add_failure_reason_to_account_imports.rb b/db/migrate/20260211122517_add_failure_reason_to_account_imports.rb new file mode 100644 index 000000000..d7e7d6ceb --- /dev/null +++ b/db/migrate/20260211122517_add_failure_reason_to_account_imports.rb @@ -0,0 +1,5 @@ +class AddFailureReasonToAccountImports < ActiveRecord::Migration[8.2] + def change + add_column :account_imports, :failure_reason, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index a1f2a33bc..8bc5f5c7d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.2].define(version: 2026_01_21_155752) do +ActiveRecord::Schema[8.2].define(version: 2026_02_11_122517) do create_table "accesses", id: :uuid, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.datetime "accessed_at" t.uuid "account_id", null: false @@ -42,6 +42,7 @@ ActiveRecord::Schema[8.2].define(version: 2026_01_21_155752) do t.uuid "account_id" t.datetime "completed_at" t.datetime "created_at", null: false + t.string "failure_reason" t.uuid "identity_id", null: false t.string "status", default: "pending", null: false t.datetime "updated_at", null: false diff --git a/db/schema_sqlite.rb b/db/schema_sqlite.rb index dad03edde..248bf97d4 100644 --- a/db/schema_sqlite.rb +++ b/db/schema_sqlite.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.2].define(version: 2026_01_21_155752) do +ActiveRecord::Schema[8.2].define(version: 2026_02_11_122517) do create_table "accesses", id: :uuid, force: :cascade do |t| t.datetime "accessed_at" t.uuid "account_id", null: false @@ -42,6 +42,7 @@ ActiveRecord::Schema[8.2].define(version: 2026_01_21_155752) do t.uuid "account_id" t.datetime "completed_at" t.datetime "created_at", null: false + t.string "failure_reason" t.uuid "identity_id", null: false t.string "status", limit: 255, default: "pending", null: false t.datetime "updated_at", null: false diff --git a/test/mailers/import_mailer_test.rb b/test/mailers/import_mailer_test.rb index 0acff9bfb..740793353 100644 --- a/test/mailers/import_mailer_test.rb +++ b/test/mailers/import_mailer_test.rb @@ -12,4 +12,39 @@ class ImportMailerTest < ActionMailer::TestCase assert_equal "Your Fizzy account import is done", email.subject assert_match accounts(:"37s").slug, email.body.encoded end + + test "failed with no reason" do + import = Account::Import.create!(account: Current.account, identity: identities(:david), status: :failed) + email = ImportMailer.failed(import) + + assert_emails 1 do + email.deliver_now + end + + assert_equal [ "david@37signals.com" ], email.to + assert_equal "Your Fizzy account import failed", email.subject + assert_match "corrupted export data", email.body.encoded + end + + test "failed with conflict reason" do + import = Account::Import.create!(account: Current.account, identity: identities(:david), status: :failed, failure_reason: :conflict) + email = ImportMailer.failed(import) + + assert_emails 1 do + email.deliver_now + end + + assert_match "account you are trying to import already exists", email.body.encoded + end + + test "failed with invalid_export reason" do + import = Account::Import.create!(account: Current.account, identity: identities(:david), status: :failed, failure_reason: :invalid_export) + email = ImportMailer.failed(import) + + assert_emails 1 do + email.deliver_now + end + + assert_match "isn't a Fizzy account export", email.body.encoded + end end diff --git a/test/models/account/import_test.rb b/test/models/account/import_test.rb index dfce60cf3..a377f69c2 100644 --- a/test/models/account/import_test.rb +++ b/test/models/account/import_test.rb @@ -62,6 +62,67 @@ class Account::ImportTest < ActiveSupport::TestCase 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")) + + assert_raises(NoMethodError) { import.check } + + assert import.failed? + assert_nil import.failure_reason + end + + test "check sets failure_reason to invalid_export for non-Fizzy ZIP" do + target_account = Account.create!(name: "Import Test") + import = Account::Import.create!(identity: identities(:david), account: target_account) + + # Create a ZIP with no account.json + tempfile = Tempfile.new([ "bad_export", ".zip" ]) + tempfile.binmode + writer = ZipFile::Writer.new(tempfile) + writer.add_file("data/dummy.json", '{"hello": "world"}') + writer.close + tempfile.rewind + + Current.set(account: target_account) do + import.file.attach(io: tempfile, filename: "export.zip", content_type: "application/zip") + end + + assert_raises(Account::DataTransfer::RecordSet::IntegrityError) { import.check } + + assert import.failed? + assert_equal "invalid_export", import.failure_reason + ensure + tempfile&.close + tempfile&.unlink + end + + test "check sets failure_reason to conflict when records already exist" do + source_account = accounts("37s") + exporter = users(:david) + identity = exporter.identity + + 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) } + + # Import without destroying the source, so records still exist + 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 + + assert_raises(Account::DataTransfer::RecordSet::ConflictError) { import.check } + + assert import.failed? + assert_equal "conflict", import.failure_reason + ensure + export_tempfile&.close + export_tempfile&.unlink + end + private def account_digest(account) {