Give more detail about why the import failed
This commit is contained in:
@@ -4,7 +4,8 @@ class ImportMailer < ApplicationMailer
|
|||||||
mail to: identity.email_address, subject: "Your Fizzy account import is done"
|
mail to: identity.email_address, subject: "Your Fizzy account import is done"
|
||||||
end
|
end
|
||||||
|
|
||||||
def failed(identity)
|
def failed(import)
|
||||||
mail to: identity.email_address, subject: "Your Fizzy account import failed"
|
@import = import
|
||||||
|
mail to: import.identity.email_address, subject: "Your Fizzy account import failed"
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|||||||
@@ -1,5 +1,6 @@
|
|||||||
class Account::DataTransfer::RecordSet
|
class Account::DataTransfer::RecordSet
|
||||||
class IntegrityError < StandardError; end
|
class IntegrityError < StandardError; end
|
||||||
|
class ConflictError < IntegrityError; end
|
||||||
|
|
||||||
IMPORT_BATCH_SIZE = 100
|
IMPORT_BATCH_SIZE = 100
|
||||||
|
|
||||||
@@ -93,7 +94,7 @@ class Account::DataTransfer::RecordSet
|
|||||||
end
|
end
|
||||||
|
|
||||||
if model.exists?(id: data["id"])
|
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
|
end
|
||||||
|
|
||||||
check_associations_dont_exist(data)
|
check_associations_dont_exist(data)
|
||||||
@@ -118,7 +119,7 @@ class Account::DataTransfer::RecordSet
|
|||||||
end
|
end
|
||||||
|
|
||||||
if associated_class.exists?(id: associated_id)
|
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
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|||||||
@@ -7,6 +7,7 @@ class Account::Import < ApplicationRecord
|
|||||||
has_one_attached :file
|
has_one_attached :file
|
||||||
|
|
||||||
enum :status, %w[ pending processing completed failed ].index_by(&:itself), default: :pending
|
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)) }
|
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)
|
record_set.check(from: zip, start: last_id, callback: callback)
|
||||||
end
|
end
|
||||||
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
|
rescue => e
|
||||||
mark_as_failed
|
mark_as_failed
|
||||||
raise e
|
raise e
|
||||||
@@ -44,6 +51,12 @@ class Account::Import < ApplicationRecord
|
|||||||
reconcile_account_storage
|
reconcile_account_storage
|
||||||
|
|
||||||
mark_completed
|
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
|
rescue => e
|
||||||
mark_as_failed
|
mark_as_failed
|
||||||
raise e
|
raise e
|
||||||
@@ -60,9 +73,9 @@ class Account::Import < ApplicationRecord
|
|||||||
ImportMailer.completed(identity, account).deliver_later
|
ImportMailer.completed(identity, account).deliver_later
|
||||||
end
|
end
|
||||||
|
|
||||||
def mark_as_failed
|
def mark_as_failed(failure_reason = nil)
|
||||||
failed!
|
update!(status: :failed, failure_reason: failure_reason)
|
||||||
ImportMailer.failed(identity).deliver_later
|
ImportMailer.failed(self).deliver_later
|
||||||
end
|
end
|
||||||
|
|
||||||
def add_importer_to_all_access_boards
|
def add_importer_to_all_access_boards
|
||||||
|
|||||||
@@ -13,7 +13,13 @@
|
|||||||
<%= link_to "Go to your account", landing_url(script_name: @import.account.slug), class: "btn btn--link center txt-medium" %>
|
<%= link_to "Go to your account", landing_url(script_name: @import.account.slug), class: "btn btn--link center txt-medium" %>
|
||||||
<% when "failed" %>
|
<% when "failed" %>
|
||||||
<p class="txt-medium txt-negative">Your import failed.</p>
|
<p class="txt-medium txt-negative">Your import failed.</p>
|
||||||
<p class="txt-small">This may be due to corrupted export data or a conflict with existing data. Please try again with a fresh export.</p>
|
<% if @import.failed_due_to_conflict? %>
|
||||||
|
<p class="txt-small">It looks like the account you are trying to import already exists.</p>
|
||||||
|
<% elsif @import.failed_due_to_invalid_export? %>
|
||||||
|
<p class="txt-small">The ZIP file isn't a Fizzy account export.</p>
|
||||||
|
<% else %>
|
||||||
|
<p class="txt-small">This may be due to corrupted export data or a conflict with existing data. Please try again with a fresh export.</p>
|
||||||
|
<% end %>
|
||||||
<%= link_to "Try again", new_account_import_path, class: "btn btn--plain center txt-medium" %>
|
<%= link_to "Try again", new_account_import_path, class: "btn btn--plain center txt-medium" %>
|
||||||
<% end %>
|
<% end %>
|
||||||
</div>
|
</div>
|
||||||
|
|||||||
@@ -1,8 +1,11 @@
|
|||||||
<p class="subtitle">Unfortunately, we couldn't import your Fizzy account.</p>
|
<p class="subtitle">Unfortunately, we couldn't import your Fizzy account.</p>
|
||||||
|
|
||||||
<p>
|
<% if @import.failed_due_to_conflict? %>
|
||||||
This may be due to corrupted export data or a conflict with existing data.
|
<p>It looks like the account you are trying to import already exists.</p>
|
||||||
Please try again with a fresh export, or reach out for help if the problem persists.
|
<% elsif @import.failed_due_to_invalid_export? %>
|
||||||
</p>
|
<p>The ZIP file isn't a Fizzy account export.</p>
|
||||||
|
<% else %>
|
||||||
|
<p>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.</p>
|
||||||
|
<% end %>
|
||||||
|
|
||||||
<p class="footer">Need help? <%= mail_to "support@fizzy.do", "Send us an email" %>.</p>
|
<p class="footer">Need help? <%= mail_to "support@fizzy.do", "Send us an email" %>.</p>
|
||||||
|
|||||||
@@ -1,6 +1,11 @@
|
|||||||
Unfortunately, we couldn't import your Fizzy account.
|
Unfortunately, we couldn't import your Fizzy account.
|
||||||
|
|
||||||
This may be due to corrupted export data or a conflict with existing data.
|
<% if @import.failed_due_to_conflict? -%>
|
||||||
Please try again with a fresh export, or reach out for help if the problem persists.
|
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
|
Need help? Send us an email at support@fizzy.do
|
||||||
|
|||||||
@@ -0,0 +1,5 @@
|
|||||||
|
class AddFailureReasonToAccountImports < ActiveRecord::Migration[8.2]
|
||||||
|
def change
|
||||||
|
add_column :account_imports, :failure_reason, :string
|
||||||
|
end
|
||||||
|
end
|
||||||
Generated
+2
-1
@@ -10,7 +10,7 @@
|
|||||||
#
|
#
|
||||||
# It's strongly recommended that you check this file into your version control system.
|
# 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|
|
create_table "accesses", id: :uuid, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t|
|
||||||
t.datetime "accessed_at"
|
t.datetime "accessed_at"
|
||||||
t.uuid "account_id", null: false
|
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.uuid "account_id"
|
||||||
t.datetime "completed_at"
|
t.datetime "completed_at"
|
||||||
t.datetime "created_at", null: false
|
t.datetime "created_at", null: false
|
||||||
|
t.string "failure_reason"
|
||||||
t.uuid "identity_id", null: false
|
t.uuid "identity_id", null: false
|
||||||
t.string "status", default: "pending", null: false
|
t.string "status", default: "pending", null: false
|
||||||
t.datetime "updated_at", null: false
|
t.datetime "updated_at", null: false
|
||||||
|
|||||||
+2
-1
@@ -10,7 +10,7 @@
|
|||||||
#
|
#
|
||||||
# It's strongly recommended that you check this file into your version control system.
|
# 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|
|
create_table "accesses", id: :uuid, force: :cascade do |t|
|
||||||
t.datetime "accessed_at"
|
t.datetime "accessed_at"
|
||||||
t.uuid "account_id", null: false
|
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.uuid "account_id"
|
||||||
t.datetime "completed_at"
|
t.datetime "completed_at"
|
||||||
t.datetime "created_at", null: false
|
t.datetime "created_at", null: false
|
||||||
|
t.string "failure_reason"
|
||||||
t.uuid "identity_id", null: false
|
t.uuid "identity_id", null: false
|
||||||
t.string "status", limit: 255, default: "pending", null: false
|
t.string "status", limit: 255, default: "pending", null: false
|
||||||
t.datetime "updated_at", null: false
|
t.datetime "updated_at", null: false
|
||||||
|
|||||||
@@ -12,4 +12,39 @@ class ImportMailerTest < ActionMailer::TestCase
|
|||||||
assert_equal "Your Fizzy account import is done", email.subject
|
assert_equal "Your Fizzy account import is done", email.subject
|
||||||
assert_match accounts(:"37s").slug, email.body.encoded
|
assert_match accounts(:"37s").slug, email.body.encoded
|
||||||
end
|
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
|
end
|
||||||
|
|||||||
@@ -62,6 +62,67 @@ class Account::ImportTest < ActiveSupport::TestCase
|
|||||||
export_tempfile&.unlink
|
export_tempfile&.unlink
|
||||||
end
|
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
|
private
|
||||||
def account_digest(account)
|
def account_digest(account)
|
||||||
{
|
{
|
||||||
|
|||||||
Reference in New Issue
Block a user