Fixed issue #2329 - Unable to import (update) users with email addresses als lookup index written in upper letters.

This commit is contained in:
Martin Edenhofer 2018-11-06 06:42:52 +01:00
parent 27dfeacf1d
commit 13b3b841d5
19 changed files with 289 additions and 42 deletions

View file

@ -1320,6 +1320,7 @@ class App.Import extends App.ControllerModal
params.set('try', true)
if _.isEmpty(params.get('data'))
params.delete('data')
@formDisable(e)
@ajax(
id: 'csv_import'
type: 'POST'
@ -1341,6 +1342,14 @@ class App.Import extends App.ControllerModal
return
@data = data
@update()
@formEnable(e)
error: (data) =>
details = data.responseJSON || {}
@notify
type: 'error'
msg: App.i18n.translateContent(details.error_human || details.error || 'Unable to import!')
timeout: 6000
@formEnable(e)
)
class App.ImportTryResult extends App.ControllerModal
@ -1361,10 +1370,23 @@ class App.ImportTryResult extends App.ControllerModal
import_example_url: "#{@baseUrl}/import"
result: @result
))
# check if data is processing...
if @data
result = App.view("#{@templateDirectory}/result")(
@data
)
content.find('.js-error').html(result)
if result
content.find('.js-error').removeClass('hide')
else
content.find('.js-error').addClass('hide')
content
onSubmit: (e) =>
@params.set('try', false)
@formDisable(e)
@ajax(
id: 'csv_import'
type: 'POST'
@ -1386,6 +1408,14 @@ class App.ImportTryResult extends App.ControllerModal
return
@data = data
@update()
@formEnable(e)
error: (data) =>
details = data.responseJSON || {}
@notify
type: 'error'
msg: App.i18n.translateContent(details.error_human || details.error || 'Unable to import!')
timeout: 6000
@formEnable(e)
)
class App.ImportResult extends App.ControllerModal

View file

@ -349,7 +349,12 @@ curl http://localhost/api/v1/organization/{id} -v -u #{login}:#{password} -H "Co
# @response_message 401 Invalid session.
def import_start
permission_check('admin.user')
string = params[:data] || params[:file].read.force_encoding('utf-8')
string = params[:data]
if string.blank? && params[:file].present?
string = params[:file].read.force_encoding('utf-8')
end
raise Exceptions::UnprocessableEntity, 'No source data submitted!' if string.blank?
result = Organization.csv_import(
string: string,
parse_params: {

View file

@ -186,7 +186,12 @@ curl http://localhost/api/v1/text_modules.json -v -u #{login}:#{password} -H "Co
# @response_message 401 Invalid session.
def import_start
permission_check('admin.text_module')
string = params[:data] || params[:file].read.force_encoding('utf-8')
string = params[:data]
if string.blank? && params[:file].present?
string = params[:file].read.force_encoding('utf-8')
end
raise Exceptions::UnprocessableEntity, 'No source data submitted!' if string.blank?
result = TextModule.csv_import(
string: string,
parse_params: {

View file

@ -318,8 +318,14 @@ class TicketArticlesController < ApplicationController
raise 'Only can import tickets if system is in import mode.'
end
string = params[:data]
if string.blank? && params[:file].present?
string = params[:file].read.force_encoding('utf-8')
end
raise Exceptions::UnprocessableEntity, 'No source data submitted!' if string.blank?
result = Ticket::Article.csv_import(
string: params[:file].read.force_encoding('utf-8'),
string: string,
parse_params: {
col_sep: ';',
},

View file

@ -641,7 +641,12 @@ class TicketsController < ApplicationController
raise 'Only can import tickets if system is in import mode.'
end
string = params[:data] || params[:file].read.force_encoding('utf-8')
string = params[:data]
if string.blank? && params[:file].present?
string = params[:file].read.force_encoding('utf-8')
end
raise Exceptions::UnprocessableEntity, 'No source data submitted!' if string.blank?
result = Ticket.csv_import(
string: string,
parse_params: {

View file

@ -1043,7 +1043,12 @@ curl http://localhost/api/v1/users/avatar -v -u #{login}:#{password} -H "Content
# @response_message 401 Invalid session.
def import_start
permission_check('admin.user')
string = params[:data] || params[:file].read.force_encoding('utf-8')
string = params[:data]
if string.blank? && params[:file].present?
string = params[:file].read.force_encoding('utf-8')
end
raise Exceptions::UnprocessableEntity, 'No source data submitted!' if string.blank?
result = User.csv_import(
string: string,
parse_params: {

View file

@ -20,20 +20,33 @@ returns
=end
def lookup(**attr)
raise ArgumentError, "Multiple lookup attributes given (#{attr.keys.join(', ')}), only support (#{lookup_keys.join(', ')})" if attr.many?
attr.transform_keys!(&:to_sym).slice!(*lookup_keys)
raise ArgumentError, "Valid lookup attribute required (#{lookup_keys.join(', ')})" if attr.empty?
raise ArgumentError, "Multiple lookup attributes given (#{attr.keys.join(', ')})" if attr.many?
record = cache_get(attr.values.first)
record ||= find_and_save_to_cache_by(attr)
end
private
=begin
return possible lookup keys for model
result = Model.lookup_keys
returns
[:id, :name] # or fror users [:id, :login, :email]
=end
def lookup_keys
@lookup_keys ||= %i[id name login email number] & attribute_names.map(&:to_sym)
end
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 }

View file

@ -118,6 +118,17 @@ returns
return result
end
# check if min one lookup key exists
if header.count == (header - lookup_keys.map(&:to_s)).count
errors.push "No lookup column like #{lookup_keys.map(&:to_s).join(',')} for #{new.class} found."
result = {
errors: errors,
try: try,
result: 'failed',
}
return result
end
# get payload based on csv
payload = []
rows.each do |row|
@ -172,11 +183,15 @@ returns
payload.each do |attributes|
line_count += 1
record = nil
%i[id number name login email].each do |lookup_by|
next if !attributes[lookup_by]
lookup_keys.each do |lookup_by|
next if attributes[lookup_by].blank?
params = {}
params[lookup_by] = attributes[lookup_by]
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
@ -199,6 +214,7 @@ returns
end
# create object
BulkImportInfo.enable
Transaction.execute(disable_notification: true, reset_user_id: true) do
UserInfo.current_user_id = clean_params[:updated_by_id] || clean_params[:created_by_id]
if !record || delete == true
@ -217,7 +233,7 @@ returns
record.associations_from_param(attributes)
record.save!
rescue => e
errors.push "Line #{line_count}: #{e.message}"
errors.push "Line #{line_count}: Unable to create record - #{e.message}"
next
end
else
@ -234,14 +250,20 @@ returns
record.with_lock do
record.associations_from_param(attributes)
record.update!(clean_params)
clean_params.each do |key, value|
record[key] = value
end
next if !record.changed?
record.save!
end
rescue => e
errors.push "Line #{line_count}: #{e.message}"
errors.push "Line #{line_count}: Unable to update record - #{e.message}"
next
end
end
end
BulkImportInfo.disable
records.push record
end
@ -258,7 +280,6 @@ returns
try: try,
result: result,
}
end
=begin

View file

@ -9,6 +9,9 @@ module Cti
after_commit :update_cti_logs, on: :destroy
after_commit :update_cti_logs_with_fg_optimization, on: :create
skip_callback :commit, :after, :update_cti_logs, if: -> { BulkImportInfo.enabled? }
skip_callback :commit, :after, :update_cti_logs_with_fg_optimization, if: -> { BulkImportInfo.enabled? }
=begin
Cti::CallerId.maybe_add(

View file

@ -28,6 +28,9 @@ class User < ApplicationModel
after_commit :update_caller_id
before_destroy :destroy_longer_required_objects
skip_callback :create, :after, :avatar_for_email_check, if: -> { BulkImportInfo.enabled? }
skip_callback :update, :after, :avatar_for_email_check, if: -> { BulkImportInfo.enabled? }
store :preferences
activity_stream_permission 'admin.user'

13
lib/bulk_import_info.rb Normal file
View file

@ -0,0 +1,13 @@
module BulkImportInfo
def self.enabled?
Thread.current[:bulk_import]
end
def self.enable
Thread.current[:bulk_import] = true
end
def self.disable
Thread.current[:bulk_import] = false
end
end

View file

@ -4,8 +4,8 @@ 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) { (%i[name login email number] & subject.attribute_names.map(&:to_sym)) }
let(:non_attributes) { (%i[id name login email number] - subject.attribute_names.map(&:to_sym)) }
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 } }
@ -35,7 +35,7 @@ RSpec.shared_examples 'CanLookup' do
end
it 'accepts exactly one attribute-value pair' do
expect { subject.lookup(:id => lookup_id, string_attributes.sample => 'foo') }
expect { subject.lookup(id: lookup_id, some_attribute: 'foo') }
.to raise_error(ArgumentError)
end
@ -69,5 +69,10 @@ RSpec.shared_examples 'CanLookup' do
.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

View file

@ -512,8 +512,8 @@ RSpec.describe 'Organization', type: :request, searchindex: true do
expect(json_response['records'].count).to eq(2)
expect(json_response['result']).to eq('failed')
expect(json_response['errors'].count).to eq(2)
expect(json_response['errors'][0]).to eq("Line 1: unknown attribute 'name2' for Organization.")
expect(json_response['errors'][1]).to eq("Line 2: unknown attribute 'name2' for Organization.")
expect(json_response['errors'][0]).to eq("Line 1: Unable to create record - unknown attribute 'name2' for Organization.")
expect(json_response['errors'][1]).to eq("Line 2: Unable to create record - unknown attribute 'name2' for Organization.")
# valid file try
csv_file_path = Rails.root.join('test', 'data', 'csv', 'organization_simple.csv')

View file

@ -59,8 +59,8 @@ RSpec.describe 'Text Module', type: :request do
expect(json_response['records'].count).to eq(2)
expect(json_response['result']).to eq('failed')
expect(json_response['errors'].count).to eq(2)
expect(json_response['errors'][0]).to eq("Line 1: unknown attribute 'keywords2' for TextModule.")
expect(json_response['errors'][1]).to eq("Line 2: unknown attribute 'keywords2' for TextModule.")
expect(json_response['errors'][0]).to eq("Line 1: Unable to create record - unknown attribute 'keywords2' for TextModule.")
expect(json_response['errors'][1]).to eq("Line 2: Unable to create record - unknown attribute 'keywords2' for TextModule.")
# valid file try
csv_file_path = Rails.root.join('test', 'data', 'csv', 'text_module_simple.csv')

View file

@ -872,8 +872,8 @@ RSpec.describe 'User', type: :request, searchindex: true do
expect(json_response['records'].count).to eq(2)
expect(json_response['result']).to eq('failed')
expect(json_response['errors'].count).to eq(2)
expect(json_response['errors'][0]).to eq("Line 1: unknown attribute 'firstname2' for User.")
expect(json_response['errors'][1]).to eq("Line 2: unknown attribute 'firstname2' for User.")
expect(json_response['errors'][0]).to eq("Line 1: Unable to create record - unknown attribute 'firstname2' for User.")
expect(json_response['errors'][1]).to eq("Line 2: Unable to create record - unknown attribute 'firstname2' for User.")
# valid file try
csv_file_path = Rails.root.join('test', 'data', 'csv', 'user_simple.csv')

View file

@ -45,6 +45,20 @@ class OrganizationCsvImportTest < ActiveSupport::TestCase
assert_equal('No records found in file/string for Organization.', result[:errors][0])
end
test 'verify required lookup headers' do
csv_string = "firstname;lastname;active;\nfirstname-simple-import1;lastname-simple-import1;;true\nfirstname-simple-import2;lastname-simple-import2;false\n"
result = Organization.csv_import(
string: csv_string,
parse_params: {
col_sep: ';',
},
try: true,
)
assert_equal(true, result[:try])
assert_equal('failed', result[:result])
assert_equal('No lookup column like id,name for Organization found.', result[:errors][0])
end
test 'simple import' do
csv_string = "id;name;shared;domain;domain_assignment;active;note\n;org-simple-import1;true;org-simple-import1.example.com;false;true;some note1\n;org-simple-import2;true;org-simple-import2.example.com;false;false;some note2\n"
@ -210,8 +224,8 @@ class OrganizationCsvImportTest < ActiveSupport::TestCase
assert_equal(true, result[:try])
assert_equal(2, result[:errors].count)
assert_equal('failed', result[:result])
assert_equal("Line 1: unknown attribute 'not existing' for Organization.", result[:errors][0])
assert_equal("Line 2: unknown attribute 'not existing' for Organization.", result[:errors][1])
assert_equal("Line 1: Unable to create record - unknown attribute 'not existing' for Organization.", result[:errors][0])
assert_equal("Line 2: Unable to create record - unknown attribute 'not existing' for Organization.", result[:errors][1])
assert_nil(Organization.find_by(name: 'organization-invalid-import1'))
assert_nil(Organization.find_by(name: 'organization-invalid-import2'))
@ -226,8 +240,8 @@ class OrganizationCsvImportTest < ActiveSupport::TestCase
assert_equal(false, result[:try])
assert_equal(2, result[:errors].count)
assert_equal('failed', result[:result])
assert_equal("Line 1: unknown attribute 'not existing' for Organization.", result[:errors][0])
assert_equal("Line 2: unknown attribute 'not existing' for Organization.", result[:errors][1])
assert_equal("Line 1: Unable to create record - unknown attribute 'not existing' for Organization.", result[:errors][0])
assert_equal("Line 2: Unable to create record - unknown attribute 'not existing' for Organization.", result[:errors][1])
assert_nil(Organization.find_by(name: 'organization-invalid-import1'))
assert_nil(Organization.find_by(name: 'organization-invalid-import2'))

View file

@ -49,6 +49,20 @@ class TextModuleCsvImportTest < ActiveSupport::TestCase
assert_equal('No records found in file/string for TextModule.', result[:errors][0])
end
test 'verify required lookup headers' do
csv_string = "firstname;lastname;active;\nfirstname-simple-import1;lastname-simple-import1;;true\nfirstname-simple-import2;lastname-simple-import2;false\n"
result = TextModule.csv_import(
string: csv_string,
parse_params: {
col_sep: ';',
},
try: true,
)
assert_equal(true, result[:try])
assert_equal('failed', result[:result])
assert_equal('No lookup column like id,name for TextModule found.', result[:errors][0])
end
test 'simple import' do
TextModule.create!(
name: 'nsome name1',

View file

@ -49,6 +49,20 @@ class TicketCsvImportTest < ActiveSupport::TestCase
assert_equal('No records found in file/string for Ticket.', result[:errors][0])
end
test 'verify required lookup headers' do
csv_string = "firstname;lastname;active;\nfirstname-simple-import1;lastname-simple-import1;;true\nfirstname-simple-import2;lastname-simple-import2;false\n"
result = Ticket.csv_import(
string: csv_string,
parse_params: {
col_sep: ';',
},
try: true,
)
assert_equal(true, result[:try])
assert_equal('failed', result[:result])
assert_equal('No lookup column like id,number for Ticket found.', result[:errors][0])
end
test 'simple import' do
csv_string = "id;number;title;state;priority;owner;customer;group;note\n;123456;some title1;new;2 normal;-;nicole.braun@zammad.org;Users;some note1\n;123457;some title2;closed;1 low;admin@example.com;nicole.braun@zammad.org;Users;some note2\n"
@ -173,8 +187,8 @@ class TicketCsvImportTest < ActiveSupport::TestCase
assert_equal(true, result[:try])
assert_equal(2, result[:errors].count)
assert_equal('failed', result[:result])
assert_equal("Line 1: unknown attribute 'not_existing' for Ticket.", result[:errors][0])
assert_equal("Line 2: unknown attribute 'not_existing' for Ticket.", result[:errors][1])
assert_equal("Line 1: Unable to create record - unknown attribute 'not_existing' for Ticket.", result[:errors][0])
assert_equal("Line 2: Unable to create record - unknown attribute 'not_existing' for Ticket.", result[:errors][1])
assert_nil(Ticket.find_by(number: '123456'))
assert_nil(Ticket.find_by(number: '123457'))
@ -189,8 +203,8 @@ class TicketCsvImportTest < ActiveSupport::TestCase
assert_equal(false, result[:try])
assert_equal(2, result[:errors].count)
assert_equal('failed', result[:result])
assert_equal("Line 1: unknown attribute 'not_existing' for Ticket.", result[:errors][0])
assert_equal("Line 2: unknown attribute 'not_existing' for Ticket.", result[:errors][1])
assert_equal("Line 1: Unable to create record - unknown attribute 'not_existing' for Ticket.", result[:errors][0])
assert_equal("Line 2: Unable to create record - unknown attribute 'not_existing' for Ticket.", result[:errors][1])
assert_nil(Ticket.find_by(number: '123456'))
assert_nil(Ticket.find_by(number: '123457'))

View file

@ -44,9 +44,24 @@ class UserCsvImportTest < ActiveSupport::TestCase
assert_equal('No records found in file/string for User.', result[:errors][0])
end
test 'verify required lookup headers' do
csv_string = "firstname;lastname;active;\nfirstname-simple-import1;lastname-simple-import1;;true\nfirstname-simple-import2;lastname-simple-import2;false\n"
result = User.csv_import(
string: csv_string,
parse_params: {
col_sep: ';',
},
try: true,
)
assert_equal(true, result[:try])
assert_equal('failed', result[:result])
assert_equal('No lookup column like id,login,email for User found.', result[:errors][0])
end
test 'simple import' do
csv_string = "login;firstname;lastname;email;active;\nuser-simple-import1;firstname-simple-import1;lastname-simple-import1;user-simple-import1@example.com;true\nuser-simple-import2;firstname-simple-import2;lastname-simple-import2;user-simple-import2@example.com;false\n"
count = User.count
csv_string = "login;firstname;lastname;email;active;\nuser-simple-IMPORT1;firstname-simple-import1;lastname-simple-import1;user-simple-IMPORT1@example.com ;true\nuser-simple-import2;firstname-simple-import2;lastname-simple-import2;user-simple-import2@example.com;false\n"
result = User.csv_import(
string: csv_string,
parse_params: {
@ -57,6 +72,9 @@ class UserCsvImportTest < ActiveSupport::TestCase
assert_equal(true, result[:try])
assert_equal(2, result[:records].count)
assert_equal('success', result[:result])
assert_equal(2, result[:stats][:created])
assert_equal(0, result[:stats][:updated])
assert_equal(count, User.count)
assert_nil(User.find_by(login: 'user-simple-import1'))
assert_nil(User.find_by(login: 'user-simple-import2'))
@ -71,6 +89,9 @@ class UserCsvImportTest < ActiveSupport::TestCase
assert_equal(false, result[:try])
assert_equal(2, result[:records].count)
assert_equal('success', result[:result])
assert_equal(2, result[:stats][:created])
assert_equal(0, result[:stats][:updated])
assert_equal(count + 2, User.count)
user1 = User.find_by(login: 'user-simple-import1')
assert(user1)
@ -87,6 +108,76 @@ class UserCsvImportTest < ActiveSupport::TestCase
assert_equal(user2.email, 'user-simple-import2@example.com')
assert_equal(user2.active, false)
result = User.csv_import(
string: csv_string,
parse_params: {
col_sep: ';',
},
try: false,
)
assert_equal(false, result[:try])
assert_equal(2, result[:records].count)
assert_equal('success', result[:result])
assert_equal(0, result[:stats][:created])
assert_equal(2, result[:stats][:updated])
assert_equal(count + 2, User.count)
user1_1 = user1
user1 = User.find_by(login: 'user-simple-import1')
assert(user1)
assert_equal(user1.login, 'user-simple-import1')
assert_equal(user1.firstname, 'firstname-simple-import1')
assert_equal(user1.lastname, 'lastname-simple-import1')
assert_equal(user1.email, 'user-simple-import1@example.com')
assert_equal(user1.active, true)
assert_equal(user1.updated_at, user1_1.updated_at)
user2 = user2
user2_1 = User.find_by(login: 'user-simple-import2')
assert(user2)
assert_equal(user2.login, 'user-simple-import2')
assert_equal(user2.firstname, 'firstname-simple-import2')
assert_equal(user2.lastname, 'lastname-simple-import2')
assert_equal(user2.email, 'user-simple-import2@example.com')
assert_equal(user2.active, false)
assert_equal(user2.updated_at, user2_1.updated_at)
travel 2.seconds
csv_string = "login;firstname;lastname;email;active;\n ;firstname-simple-import1;lastname-simple-import1;user-simple-IMPORT1@example.com ;true\n user-simple-import2\t;firstname-simple-import2;lastname-simple-import2;;false\n"
result = User.csv_import(
string: csv_string,
parse_params: {
col_sep: ';',
},
try: false,
)
assert_equal(false, result[:try])
assert_equal(2, result[:records].count)
assert_equal(0, result[:stats][:created])
assert_equal(2, result[:stats][:updated])
assert_equal('success', result[:result])
assert_equal(count + 2, User.count)
user1_1 = user1
user1 = User.find_by(email: 'user-simple-import1@example.com')
assert(user1)
assert_equal(user1.login, 'user-simple-import1@example.com')
assert_equal(user1.firstname, 'firstname-simple-import1')
assert_equal(user1.lastname, 'lastname-simple-import1')
assert_equal(user1.email, 'user-simple-import1@example.com')
assert_equal(user1.active, true)
assert_not_equal(user1.updated_at, user1_1.updated_at)
user2 = user2
user2_1 = User.find_by(login: 'user-simple-import2')
assert(user2)
assert_equal(user2.login, 'user-simple-import2')
assert_equal(user2.firstname, 'firstname-simple-import2')
assert_equal(user2.lastname, 'lastname-simple-import2')
assert_equal(user2.email, 'user-simple-import2@example.com')
assert_equal(user2.active, false)
assert_equal(user2.updated_at, user2_1.updated_at)
user1.destroy!
user2.destroy!
end
@ -340,8 +431,8 @@ class UserCsvImportTest < ActiveSupport::TestCase
assert_equal(true, result[:try])
assert_equal(2, result[:errors].count)
assert_equal('failed', result[:result])
assert_equal("Line 1: unknown attribute 'firstname2' for User.", result[:errors][0])
assert_equal("Line 2: unknown attribute 'firstname2' for User.", result[:errors][1])
assert_equal("Line 1: Unable to create record - unknown attribute 'firstname2' for User.", result[:errors][0])
assert_equal("Line 2: Unable to create record - unknown attribute 'firstname2' for User.", result[:errors][1])
assert_nil(User.find_by(login: 'user-invalid-import1'))
assert_nil(User.find_by(login: 'user-invalid-import2'))
@ -356,8 +447,8 @@ class UserCsvImportTest < ActiveSupport::TestCase
assert_equal(false, result[:try])
assert_equal(2, result[:errors].count)
assert_equal('failed', result[:result])
assert_equal("Line 1: unknown attribute 'firstname2' for User.", result[:errors][0])
assert_equal("Line 2: unknown attribute 'firstname2' for User.", result[:errors][1])
assert_equal("Line 1: Unable to create record - unknown attribute 'firstname2' for User.", result[:errors][0])
assert_equal("Line 2: Unable to create record - unknown attribute 'firstname2' for User.", result[:errors][1])
assert_nil(User.find_by(login: 'user-invalid-import1'))
assert_nil(User.find_by(login: 'user-invalid-import2'))