From dbe13c2ed6b99988090b56a3264287239241c8ab Mon Sep 17 00:00:00 2001 From: Ryan Lue Date: Mon, 21 Oct 2019 12:44:52 +0200 Subject: [PATCH] - Fixes #2658 - CSV-Import doesn't lookup records by "name" field. - Fixes #2659 - CSV-Import doesn't (always) handle database errors correctly. Attention: This introduces a behavior change: Before (other) data was still imported on errors, now data only will get imported when no error occurs. --- app/models/application_model/can_lookup.rb | 11 +- app/models/concerns/can_csv_import.rb | 84 +++++++------- .../application_model/can_lookup_examples.rb | 109 ++++++++++++++++++ spec/models/application_model_examples.rb | 2 + .../concerns/can_csv_import_examples.rb | 52 +++++++++ spec/models/concerns/can_lookup_examples.rb | 78 ------------- spec/models/organization_spec.rb | 4 +- spec/models/ticket/article_spec.rb | 2 + spec/models/ticket_spec.rb | 4 +- spec/models/user_spec.rb | 2 - spec/requests/organization_spec.rb | 2 +- spec/requests/text_module_spec.rb | 2 +- spec/requests/user_spec.rb | 2 +- test/unit/object_cache_test.rb | 81 ------------- test/unit/organization_csv_import_test.rb | 8 +- test/unit/ticket_csv_import_test.rb | 14 +-- test/unit/user_csv_import_test.rb | 43 +++---- 17 files changed, 240 insertions(+), 260 deletions(-) create mode 100644 spec/models/application_model/can_lookup_examples.rb create mode 100644 spec/models/concerns/can_csv_import_examples.rb delete mode 100644 spec/models/concerns/can_lookup_examples.rb diff --git a/app/models/application_model/can_lookup.rb b/app/models/application_model/can_lookup.rb index 5a71f6225..fba30306b 100644 --- a/app/models/application_model/can_lookup.rb +++ b/app/models/application_model/can_lookup.rb @@ -47,11 +47,12 @@ returns private def find_and_save_to_cache_by(attr) - if !Rails.application.config.db_case_sensitive && string_key?(attr.keys.first) - where(attr).find { |r| r[attr.keys.first] == attr.values.first } - else - find_by(attr) - end.tap { |r| cache_set(attr.values.first, r) } + record = find_by(attr) + return nil if string_key?(attr.keys.first) && (record&.send(attr.keys.first) != attr.values.first) # enforce case-sensitivity on MySQL + return record if ActiveRecord::Base.connection.transaction_open? # rollbacks can invalidate cache entries + + cache_set(attr.values.first, record) + record end def string_key?(key) diff --git a/app/models/concerns/can_csv_import.rb b/app/models/concerns/can_csv_import.rb index d1a5fe7df..a71d5ddf9 100644 --- a/app/models/concerns/can_csv_import.rb +++ b/app/models/concerns/can_csv_import.rb @@ -180,41 +180,48 @@ returns csv_object_ids_ignored = @csv_object_ids_ignored || [] records = [] line_count = 0 - payload.each do |attributes| - line_count += 1 - record = nil - lookup_keys.each do |lookup_by| - next if attributes[lookup_by].blank? - params = {} - params[lookup_by] = if %i[email login].include?(lookup_by) - attributes[lookup_by].downcase - else - attributes[lookup_by] - end - record = lookup(params) - break if record - end + Transaction.execute(disable_notification: true, bulk: true) do + payload.each do |attributes| + line_count += 1 + record = nil + lookup_keys.each do |lookup_by| + next if attributes[lookup_by].blank? - if attributes[:id].present? && !record - errors.push "Line #{line_count}: unknown record with id '#{attributes[:id]}' for #{new.class}." - next - end + record = if lookup_by.in?(%i[name]) + find_by("LOWER(#{lookup_by}) = ?", attributes[lookup_by].downcase) + elsif lookup_by.in?(%i[email login]) + lookup(attributes.slice(lookup_by).transform_values!(&:downcase)) + else + lookup(attributes.slice(lookup_by)) + end - if record && csv_object_ids_ignored.include?(record.id) - errors.push "Line #{line_count}: unable to update record with id '#{attributes[:id]}' for #{new.class}." - next - end + break if record + end - begin - clean_params = association_name_to_id_convert(attributes) - rescue => e - errors.push "Line #{line_count}: #{e.message}" - next - end + if record.in?(records) + errors.push "Line #{line_count}: duplicate record found." + next + end - # create object - Transaction.execute(disable_notification: true, reset_user_id: true, bulk: true) do + if attributes[:id].present? && !record + errors.push "Line #{line_count}: unknown record with id '#{attributes[:id]}' for #{new.class}." + next + end + + if record && csv_object_ids_ignored.include?(record.id) + errors.push "Line #{line_count}: unable to update record with id '#{attributes[:id]}' for #{new.class}." + next + end + + begin + clean_params = association_name_to_id_convert(attributes) + rescue => e + errors.push "Line #{line_count}: #{e.message}" + next + end + + # create object UserInfo.current_user_id = clean_params[:updated_by_id] || clean_params[:created_by_id] if !record || delete == true stats[:created] += 1 @@ -227,8 +234,6 @@ returns clean_params[:updated_by_id] = 1 end record = new(clean_params) - next if try == true - record.associations_from_param(attributes) record.save! rescue => e @@ -237,8 +242,6 @@ returns end else stats[:updated] += 1 - next if try == true - begin csv_verify_attributes(clean_params) clean_params = param_cleanup(clean_params) @@ -261,14 +264,11 @@ returns next end end + + records.push record if record end - - records.push record - end - - result = 'success' - if errors.present? - result = 'failed' + ensure + raise ActiveRecord::Rollback if try || errors.any? end { @@ -276,7 +276,7 @@ returns records: records, errors: errors, try: try, - result: result, + result: errors.empty? ? 'success' : 'failed', } end diff --git a/spec/models/application_model/can_lookup_examples.rb b/spec/models/application_model/can_lookup_examples.rb new file mode 100644 index 000000000..912341956 --- /dev/null +++ b/spec/models/application_model/can_lookup_examples.rb @@ -0,0 +1,109 @@ +RSpec.shared_examples 'ApplicationModel::CanLookup' do + describe '.lookup_keys' do + it 'returns a subset of: id, name, login, email, number' do + expect(described_class.lookup_keys) + .to all(be_in(%i[id name login email number])) + end + + it 'only includes attributes present on the model' do + expect(described_class.lookup_keys) + .to all(be_in(described_class.attribute_names.map(&:to_sym))) + end + end + + describe '.lookup' do + around do |example| + Rails.cache.clear + example.run + Rails.cache.clear + end + + let!(:instance) { create(described_class.name.underscore) } + let(:valid_lookup_key) { (described_class.lookup_keys - [:id]).sample } + let(:invalid_lookup_key) { (described_class.attribute_names.map(&:to_sym) - described_class.lookup_keys).sample } + + it 'accepts exactly one attribute-value pair' do + expect { described_class.lookup(id: instance.id, valid_lookup_key => 'foo') } + .to raise_error(ArgumentError) + end + + it 'only accepts attributes from .lookup_keys' do + expect { described_class.lookup(invalid_lookup_key => 'foo') } + .to raise_error(ArgumentError) + end + + shared_examples 'per-attribute examples' do |attribute| + it "finds records by #{attribute} (like .find_by)" do + expect(described_class.lookup(attribute => instance.send(attribute))).to eq(instance) + end + + describe "cache storage by #{attribute}" do + context 'inside a DB transaction' do # provided by default RSpec config + it 'leaves the cache untouched' do + expect { described_class.lookup(attribute => instance.send(attribute)) } + .not_to change { described_class.cache_get(instance.send(attribute)) } + end + end + + context 'outside a DB transaction' do + before do + allow(ActiveRecord::Base.connection) + .to receive(:transaction_open?).and_return(false) + end + + context 'when called for the first time' do + it 'saves the value to the cache' do + expect(Rails.cache) + .to receive(:write) + .with("#{described_class}::#{instance.send(attribute)}", instance, { expires_in: 7.days }) + .and_call_original + + expect { described_class.lookup(attribute => instance.send(attribute)) } + .to change { described_class.cache_get(instance.send(attribute)) } + .to(instance) + end + end + + context 'when called a second time' do + before { described_class.lookup(attribute => instance.send(attribute)) } + + it 'retrieves results from cache' do + expect(Rails.cache) + .to receive(:read) + .with("#{described_class}::#{instance.send(attribute)}") + + described_class.lookup(attribute => instance.send(attribute)) + end + + if attribute != :id + context 'after it has been updated' do + let!(:old_attribute_val) { instance.send(attribute) } + let!(:new_attribute_val) { instance.send(attribute).next } + + it 'moves the record to a new cache key' do + expect { instance.update(attribute => new_attribute_val) } + .to change { described_class.cache_get(old_attribute_val) }.to(nil) + + expect { described_class.lookup({ attribute => instance.send(attribute) }) } + .to change { described_class.cache_get(new_attribute_val) }.to(instance) + end + end + end + + context 'after it has been destroyed' do + it 'returns nil' do + expect { instance.destroy } + .to change { described_class.cache_get(instance.send(attribute)) } + .to(nil) + end + end + end + end + end + end + + described_class.lookup_keys.each do |key| + include_examples 'per-attribute examples', key + end + end +end diff --git a/spec/models/application_model_examples.rb b/spec/models/application_model_examples.rb index 537aaf1bd..0949d6a64 100644 --- a/spec/models/application_model_examples.rb +++ b/spec/models/application_model_examples.rb @@ -1,11 +1,13 @@ require 'models/application_model/can_assets_examples' require 'models/application_model/can_associations_examples' require 'models/application_model/can_latest_change_examples' +require 'models/application_model/can_lookup_examples' require 'models/application_model/checks_import_examples' RSpec.shared_examples 'ApplicationModel' do |options = {}| include_examples 'ApplicationModel::CanAssets', options[:can_assets] include_examples 'ApplicationModel::CanAssociations' include_examples 'ApplicationModel::CanLatestChange' + include_examples 'ApplicationModel::CanLookup' include_examples 'ApplicationModel::ChecksImport' end diff --git a/spec/models/concerns/can_csv_import_examples.rb b/spec/models/concerns/can_csv_import_examples.rb new file mode 100644 index 000000000..2c71e65bf --- /dev/null +++ b/spec/models/concerns/can_csv_import_examples.rb @@ -0,0 +1,52 @@ +RSpec.shared_examples 'CanCsvImport' do |unique_attributes: []| + describe '.csv_import' do + let!(:params) { { string: <<~CSV, parse_params: { col_sep: ',' } } } + #{described_class.attribute_names.join(',')} + #{build(factory).attributes.values.join(',')} + CSV + + let(:factory) { described_class.name.underscore } + + context 'with duplicate entries for unique attributes' do + shared_examples 'case-insensitive duplicates' do |attribute| + let!(:params) { { string: <<~CSV, parse_params: { col_sep: ',' } } } + #{described_class.attribute_names.join(',')} + #{build(factory, attribute => 'Foo').attributes.values.join(',')} + #{build(factory, attribute => 'FOO').attributes.values.join(',')} + CSV + + it 'cancels import' do + expect { described_class.csv_import(**params) } + .not_to change(described_class, :count) + end + + it 'reports the duplicate' do + expect(described_class.csv_import(**params)) + .to include(result: 'failed') + end + end + + Array(unique_attributes).each do |attribute| + include_examples 'case-insensitive duplicates', attribute + end + end + + context 'when record creation unexpectedly fails' do + around do |example| + described_class.validate { errors.add(:base, 'Unexpected failure!') } + example.run + described_class._validate_callbacks.send(:chain).pop + described_class.send(:set_callbacks, :validate, described_class._validate_callbacks.dup) + end + + context 'during a dry-run' do + before { params.merge!(try: 'true') } + + it 'reports the failure' do + expect(described_class.csv_import(**params)) + .to include(result: 'failed') + end + end + end + end +end diff --git a/spec/models/concerns/can_lookup_examples.rb b/spec/models/concerns/can_lookup_examples.rb deleted file mode 100644 index c16847db6..000000000 --- a/spec/models/concerns/can_lookup_examples.rb +++ /dev/null @@ -1,78 +0,0 @@ -require 'rails_helper' - -RSpec.shared_examples 'CanLookup' do - describe '::lookup' do - let(:subject) { described_class } - let(:ensure_instance) { create(subject.to_s.downcase) if subject.none? } - let(:string_attributes) { subject.lookup_keys } - let(:non_attributes) { (subject.attribute_names.map(&:to_sym) - subject.lookup_keys) } - let(:lookup_id) { 1 } - let(:cache_key) { "#{subject}::#{lookup_id}" } - let(:cache_expiry_param) { { expires_in: 7.days } } - - it 'finds records by id (like ::find_by)' do - ensure_instance - - expect(subject.lookup(id: lookup_id)).to eql(subject.find_by(id: lookup_id)) - end - - it 'finds records by other attributes (like ::find_by)' do - ensure_instance - - # don't run this example on models with no valid string attributes - if string_attributes.select! { |a| subject.pluck(a).any? } - attribute_name = string_attributes.sample - attribute_value = subject.pluck(target_attribute).sample - - expect(subject.lookup(attribute_name => attribute_value)) - .to eql(subject.find_by(attribute_name => attribute_value)) - end - end - - it 'only accepts attributes that uniquely identify the record' do - expect { subject.lookup(created_by_id: 1) } - .to raise_error(ArgumentError) - end - - it 'accepts exactly one attribute-value pair' do - expect { subject.lookup(id: lookup_id, some_attribute: 'foo') } - .to raise_error(ArgumentError) - end - - it 'does not accept attributes not present in model' do - expect { subject.lookup(non_attributes.sample => 'foo') } - .to raise_error(ArgumentError) - end - - it 'saves results to cache' do - ensure_instance - allow(Rails.cache) - .to receive(:write) - .and_call_original - - subject.lookup(id: lookup_id) - - expect(Rails.cache) - .to have_received(:write) - .with(cache_key, subject.first, cache_expiry_param) - end - - it 'retrieves results from cache, if stored' do - ensure_instance - allow(Rails.cache) - .to receive(:read) - .and_call_original - - subject.lookup(id: lookup_id) - - expect(Rails.cache) - .to have_received(:read) - .with(cache_key) - end - - it 'verify lookup keys' do - expect(subject.lookup_keys).to match_array((%i[id name login email number] & subject.attribute_names.map(&:to_sym))) - end - - end -end diff --git a/spec/models/organization_spec.rb b/spec/models/organization_spec.rb index 2b2a19cb8..f91884d78 100644 --- a/spec/models/organization_spec.rb +++ b/spec/models/organization_spec.rb @@ -1,6 +1,6 @@ require 'rails_helper' require 'models/application_model_examples' -require 'models/concerns/can_lookup_examples' +require 'models/concerns/can_csv_import_examples' require 'models/concerns/has_history_examples' require 'models/concerns/has_search_index_backend_examples' require 'models/concerns/has_xss_sanitized_note_examples' @@ -8,7 +8,7 @@ require 'models/concerns/has_object_manager_attributes_validation_examples' RSpec.describe Organization, type: :model do it_behaves_like 'ApplicationModel', can_assets: { associations: :members } - it_behaves_like 'CanLookup' + it_behaves_like 'CanCsvImport', unique_attributes: 'name' it_behaves_like 'HasHistory' it_behaves_like 'HasSearchIndexBackend', indexed_factory: :organization it_behaves_like 'HasXssSanitizedNote', model_factory: :organization diff --git a/spec/models/ticket/article_spec.rb b/spec/models/ticket/article_spec.rb index 89030e3c6..840ae37a0 100644 --- a/spec/models/ticket/article_spec.rb +++ b/spec/models/ticket/article_spec.rb @@ -1,12 +1,14 @@ require 'rails_helper' require 'models/application_model_examples' require 'models/concerns/can_be_imported_examples' +require 'models/concerns/can_csv_import_examples' require 'models/concerns/has_history_examples' require 'models/concerns/has_object_manager_attributes_validation_examples' RSpec.describe Ticket::Article, type: :model do it_behaves_like 'ApplicationModel' it_behaves_like 'CanBeImported' + it_behaves_like 'CanCsvImport' it_behaves_like 'HasHistory' it_behaves_like 'HasObjectManagerAttributesValidation' diff --git a/spec/models/ticket_spec.rb b/spec/models/ticket_spec.rb index 6447f1072..0be3fe940 100644 --- a/spec/models/ticket_spec.rb +++ b/spec/models/ticket_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' require 'models/application_model_examples' require 'models/concerns/can_be_imported_examples' -require 'models/concerns/can_lookup_examples' +require 'models/concerns/can_csv_import_examples' require 'models/concerns/has_history_examples' require 'models/concerns/has_tags_examples' require 'models/concerns/has_xss_sanitized_note_examples' @@ -10,7 +10,7 @@ require 'models/concerns/has_object_manager_attributes_validation_examples' RSpec.describe Ticket, type: :model do it_behaves_like 'ApplicationModel' it_behaves_like 'CanBeImported' - it_behaves_like 'CanLookup' + it_behaves_like 'CanCsvImport' it_behaves_like 'HasHistory', history_relation_object: 'Ticket::Article' it_behaves_like 'HasTags' it_behaves_like 'HasXssSanitizedNote', model_factory: :ticket diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 143490d4a..c48892b2e 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -6,7 +6,6 @@ require 'models/concerns/has_roles_examples' require 'models/concerns/has_groups_permissions_examples' require 'models/concerns/has_xss_sanitized_note_examples' require 'models/concerns/can_be_imported_examples' -require 'models/concerns/can_lookup_examples' require 'models/concerns/has_object_manager_attributes_validation_examples' RSpec.describe User, type: :model do @@ -23,7 +22,6 @@ RSpec.describe User, type: :model do it_behaves_like 'HasXssSanitizedNote', model_factory: :user it_behaves_like 'HasGroups and Permissions', group_access_no_permission_factory: :user it_behaves_like 'CanBeImported' - it_behaves_like 'CanLookup' it_behaves_like 'HasObjectManagerAttributesValidation' describe 'Class methods:' do diff --git a/spec/requests/organization_spec.rb b/spec/requests/organization_spec.rb index f038a0a48..5c5938365 100644 --- a/spec/requests/organization_spec.rb +++ b/spec/requests/organization_spec.rb @@ -524,7 +524,7 @@ RSpec.describe 'Organization', type: :request, searchindex: true do expect(json_response).to be_a_kind_of(Hash) expect(json_response['try']).to eq(true) - expect(json_response['records'].count).to eq(2) + expect(json_response['records']).to be_empty expect(json_response['result']).to eq('failed') expect(json_response['errors'].count).to eq(2) expect(json_response['errors'][0]).to eq("Line 1: Unable to create record - unknown attribute 'name2' for Organization.") diff --git a/spec/requests/text_module_spec.rb b/spec/requests/text_module_spec.rb index 20aa19e72..31552ab87 100644 --- a/spec/requests/text_module_spec.rb +++ b/spec/requests/text_module_spec.rb @@ -55,7 +55,7 @@ RSpec.describe 'Text Module', type: :request do expect(json_response).to be_a_kind_of(Hash) expect(json_response['try']).to be_truthy - expect(json_response['records'].count).to eq(2) + expect(json_response['records']).to be_empty expect(json_response['result']).to eq('failed') expect(json_response['errors'].count).to eq(2) expect(json_response['errors'][0]).to eq("Line 1: Unable to create record - unknown attribute 'keywords2' for TextModule.") diff --git a/spec/requests/user_spec.rb b/spec/requests/user_spec.rb index 801dba0f8..bcbfc56eb 100644 --- a/spec/requests/user_spec.rb +++ b/spec/requests/user_spec.rb @@ -879,7 +879,7 @@ RSpec.describe 'User', type: :request, searchindex: true do expect(json_response).to be_a_kind_of(Hash) expect(json_response['try']).to eq(true) - expect(json_response['records'].count).to eq(2) + expect(json_response['records']).to be_empty expect(json_response['result']).to eq('failed') expect(json_response['errors'].count).to eq(2) expect(json_response['errors'][0]).to eq("Line 1: Unable to create record - unknown attribute 'firstname2' for User.") diff --git a/test/unit/object_cache_test.rb b/test/unit/object_cache_test.rb index 8ec86436a..8dd9af332 100644 --- a/test/unit/object_cache_test.rb +++ b/test/unit/object_cache_test.rb @@ -101,85 +101,4 @@ class ObjectCacheTest < ActiveSupport::TestCase assert_equal(user1.organization_ids.sort, assets[:User][user1.id]['organization_ids'].sort) end - - test 'group cache' do - - name = "object cache test #{rand(9_999_999)}" - group = Group.create!( - name: name, - updated_by_id: 1, - created_by_id: 1, - ) - group_new = Group.where(name: name).first - assert_equal(name, group_new[:name], 'verify by where') - - # lookup by name - cache_key = group_new.name.to_s - assert_nil(Group.cache_get(cache_key)) - - group_lookup_name = Group.lookup(name: group_new.name) - assert_equal(group_new.name, group_lookup_name[:name], 'verify by lookup.name') - assert(Group.cache_get(cache_key)) - - # lookup by id - cache_key = group_new.id.to_s - assert_nil(Group.cache_get(cache_key)) - - group_lookup_id = Group.lookup(id: group.id) - assert_equal(group_new.name, group_lookup_id[:name], 'verify by lookup.id') - assert(Group.cache_get(cache_key)) - - # update / check if old name caches are deleted - name_new = name + ' next' - group.name = name_new - group.save - - # lookup by name - cache_key = group.name.to_s - assert_nil(Group.cache_get(cache_key)) - - group_lookup = Group.where(name: group_new.name).first - assert_nil(group_lookup, 'verify by where name_old') - assert_nil(Group.cache_get(cache_key)) - - group_lookup = Group.where(name: group.name).first - assert_equal(name_new, group_lookup[:name], 'verify by where name_new') - assert_nil(Group.cache_get(cache_key)) - - group_lookup_name = Group.lookup(name: group_new.name) - assert_nil(group_lookup_name, 'verify by lookup.name name_old') - assert_nil(Group.cache_get(cache_key)) - - group_lookup_name = Group.lookup(name: group.name) - assert_equal(name_new, group_lookup_name[:name], 'verify by lookup.name name_new') - assert(Group.cache_get(cache_key)) - - # lookup by id - cache_key = group_new.id.to_s - assert_nil(Group.cache_get(cache_key)) - - group_lookup_id = Group.lookup(id: group.id) - assert_equal(name_new, group_lookup_id[:name], 'verify by lookup.id') - assert(Group.cache_get(cache_key)) - - group.destroy - - # lookup by name - group_lookup = Group.where(name: group_new.name).first - assert_nil(group_lookup, 'verify by where name_old') - - group_lookup = Group.where(name: group.name).first - assert_nil(group_lookup, 'verify by where name_new') - - group_lookup_name = Group.lookup(name: group_new.name) - assert_nil(group_lookup_name, 'verify by lookup.name name_old') - - group_lookup_name = Group.lookup(name: group.name) - assert_nil(group_lookup_name, 'verify by lookup.name name_new') - - # lookup by id - group_lookup_id = Group.lookup(id: group.id) - assert_nil(group_lookup_id, 'verify by lookup.id') - - end end diff --git a/test/unit/organization_csv_import_test.rb b/test/unit/organization_csv_import_test.rb index a8eaca763..2e424ca0f 100644 --- a/test/unit/organization_csv_import_test.rb +++ b/test/unit/organization_csv_import_test.rb @@ -139,12 +139,8 @@ class OrganizationCsvImportTest < ActiveSupport::TestCase assert_nil(Organization.find_by(name: 'organization-simple-invalid_id-import1')) - organization2 = Organization.find_by(name: 'organization-simple-invalid_id-import2') - assert(organization2) - assert_equal(organization2.name, 'organization-simple-invalid_id-import2') - assert_equal(organization2.active, true) - - organization2.destroy! + # any single failure will cause the entire import to be aborted + assert_nil(Organization.find_by(name: 'organization-simple-invalid_id-import2')) end test 'simple import with members' do diff --git a/test/unit/ticket_csv_import_test.rb b/test/unit/ticket_csv_import_test.rb index 679666c32..f578ef1bb 100644 --- a/test/unit/ticket_csv_import_test.rb +++ b/test/unit/ticket_csv_import_test.rb @@ -146,10 +146,8 @@ class TicketCsvImportTest < ActiveSupport::TestCase assert_nil(Ticket.find_by(number: '123456')) - ticket2 = Ticket.find_by(number: '123457') - assert(ticket2) - assert_equal(ticket2.title, 'some title2') - assert_equal(ticket2.note, 'some note2') + # any single failure will cause the entire import to be aborted + assert_nil(Ticket.find_by(number: '123457')) csv_string = "id;number;title;state;priority;owner;customer;group;note\n999999999;123456;some title1;new;2 normal;-;nicole.braun@zammad.org;Users;some note1\n;123457;some title22;closed;1 low;admin@example.com;nicole.braun@zammad.org;Users;some note22\n" @@ -166,12 +164,8 @@ class TicketCsvImportTest < ActiveSupport::TestCase assert_nil(Ticket.find_by(number: '123456')) - ticket2 = Ticket.find_by(number: '123457') - assert(ticket2) - assert_equal(ticket2.title, 'some title22') - assert_equal(ticket2.note, 'some note22') - - ticket2.destroy! + # any single failure will cause the entire import to be aborted + assert_nil(Ticket.find_by(number: '123457')) end test 'invalid attributes' do diff --git a/test/unit/user_csv_import_test.rb b/test/unit/user_csv_import_test.rb index 3b1cb1266..5668d4632 100644 --- a/test/unit/user_csv_import_test.rb +++ b/test/unit/user_csv_import_test.rb @@ -213,15 +213,8 @@ class UserCsvImportTest < ActiveSupport::TestCase assert_nil(User.find_by(login: 'user-simple-invalid_id-import1')) - user2 = User.find_by(login: 'user-simple-invalid_id-import2') - assert(user2) - assert_equal(user2.login, 'user-simple-invalid_id-import2') - assert_equal(user2.firstname, 'firstname-simple-import2') - assert_equal(user2.lastname, 'lastname-simple-import2') - assert_equal(user2.email, 'user-simple-invalid_id-import2@example.com') - assert_equal(user2.active, false) - - user2.destroy! + # any single failure will cause the entire import to be aborted + assert_nil(User.find_by(login: 'user-simple-invalid_id-import2')) end test 'simple import with read only id' do @@ -255,15 +248,8 @@ class UserCsvImportTest < ActiveSupport::TestCase assert_nil(User.find_by(login: 'user-simple-readonly_id-import1')) - user2 = User.find_by(login: 'user-simple-readonly_id-import2') - assert(user2) - assert_equal(user2.login, 'user-simple-readonly_id-import2') - assert_equal(user2.firstname, 'firstname-simple-import2') - assert_equal(user2.lastname, 'lastname-simple-import2') - assert_equal(user2.email, 'user-simple-readonly_id-import2@example.com') - assert_equal(user2.active, false) - - user2.destroy! + # any single failure will cause the entire import to be aborted + assert_nil(User.find_by(login: 'user-simple-readonly_id-import2')) end test 'simple import with roles' do @@ -392,8 +378,8 @@ class UserCsvImportTest < ActiveSupport::TestCase try: true, ) assert_equal(true, result[:try]) - assert_equal(3, result[:records].count) - assert_equal('success', result[:result]) + assert_equal(2, result[:records].count) + assert_equal('failed', result[:result]) assert_nil(User.find_by(login: 'user-duplicate-import1')) assert_nil(User.find_by(login: 'user-duplicate-import2')) @@ -407,15 +393,12 @@ class UserCsvImportTest < ActiveSupport::TestCase try: false, ) assert_equal(false, result[:try]) - assert_equal(3, result[:records].count) - assert_equal('success', result[:result]) + assert_equal(2, result[:records].count) + assert_equal('failed', result[:result]) - assert(User.find_by(login: 'user-duplicate-import1')) - assert(User.find_by(login: 'user-duplicate-import2')) + assert_nil(User.find_by(login: 'user-duplicate-import1')) + assert_nil(User.find_by(login: 'user-duplicate-import2')) assert_nil(User.find_by(login: 'user-duplicate-import3')) - - User.find_by(login: 'user-duplicate-import1').destroy! - User.find_by(login: 'user-duplicate-import2').destroy! end test 'invalid attributes' do @@ -486,7 +469,7 @@ class UserCsvImportTest < ActiveSupport::TestCase assert_nil(User.find_by(login: 'user-reference-import1')) assert_nil(User.find_by(login: 'user-reference-import2')) - assert(User.find_by(login: 'user-reference-import3')) + assert_nil(User.find_by(login: 'user-reference-import3')) assert_equal("Line 1: No lookup value found for 'organization': \"organization-reference-import1\"", result[:errors][0]) assert_equal("Line 2: No lookup value found for 'organization': \"organization-reference-import2\"", result[:errors][1]) @@ -506,7 +489,9 @@ class UserCsvImportTest < ActiveSupport::TestCase assert_equal('success', result[:result]) assert_nil(User.find_by(login: 'user-reference-import1')) assert_nil(User.find_by(login: 'user-reference-import2')) - assert(User.find_by(login: 'user-reference-import3')) + + # any single failure will cause the entire import to be aborted + assert_nil(User.find_by(login: 'user-reference-import3')) result = User.csv_import( string: csv_string,