From 53ca6bd4c1282c47143143b7faee774d8043944f Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Tue, 22 May 2018 18:40:39 +0200 Subject: [PATCH 01/11] Fixed issue #2037: Restart of processes form application context fails. --- config/application.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/config/application.rb b/config/application.rb index 4fe500369..7b59d86f1 100644 --- a/config/application.rb +++ b/config/application.rb @@ -2,6 +2,9 @@ require File.expand_path('boot', __dir__) require 'rails/all' +# DO NOT REMOVE THIS LINE - see issue #2037 +Bundler.setup + # Require the gems listed in Gemfile, including any gems # you've limited to :test, :development, or :production. Bundler.require(*Rails.groups) From 0deedf3fea59ddf8eb4f3b97dba426be90e5fe7b Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Wed, 23 May 2018 08:51:41 +0200 Subject: [PATCH 02/11] Fixed failing browsertest: Mysql2::Error: Incorrect datetime value https://dev.mysql.com/doc/refman/5.7/en/datetime.html The TIMESTAMP data type is used for values that contain both date and time parts. TIMESTAMP has a range of '1970-01-01 00:00:01' UTC to '2038-01-19 03:14:07' UTC. --- test/browser/agent_ticket_overview_level0_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/browser/agent_ticket_overview_level0_test.rb b/test/browser/agent_ticket_overview_level0_test.rb index 0f2f9b038..848cd17ff 100644 --- a/test/browser/agent_ticket_overview_level0_test.rb +++ b/test/browser/agent_ticket_overview_level0_test.rb @@ -304,7 +304,7 @@ class AgentTicketOverviewLevel0Test < TestCase set( css: '.content.active .bulkAction [data-item="date"]', - value: '05/23/2088', + value: '05/23/2037', ) select( From b057155832460a16af9dd1919e48f0bd44495fa2 Mon Sep 17 00:00:00 2001 From: Ryan Lue Date: Wed, 23 May 2018 18:49:11 +0800 Subject: [PATCH 03/11] Update error message in bootstrap.rake --- lib/tasks/bootstrap.rake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/tasks/bootstrap.rake b/lib/tasks/bootstrap.rake index 99771ad14..9605bce75 100644 --- a/lib/tasks/bootstrap.rake +++ b/lib/tasks/bootstrap.rake @@ -21,7 +21,7 @@ module BootstrapRakeHelper end def add_database_config - raise Errno::ENOENT, 'contrib/database.yml not found' unless File.exist?(DB_CONFIG[:source]) + raise Errno::ENOENT, 'config/database.yml not found' unless File.exist?(DB_CONFIG[:source]) if File.exist?(DB_CONFIG[:dest]) return if FileUtils.identical?(DB_CONFIG[:source], DB_CONFIG[:dest]) From 03c586e7fc46c61e0a79e842c83f84b916855729 Mon Sep 17 00:00:00 2001 From: Ryan Lue Date: Wed, 23 May 2018 18:25:11 +0800 Subject: [PATCH 04/11] Fix sequential execution of multiple email notification triggers --- app/models/ticket.rb | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/app/models/ticket.rb b/app/models/ticket.rb index f71bc5300..60d50c1b6 100644 --- a/app/models/ticket.rb +++ b/app/models/ticket.rb @@ -806,6 +806,10 @@ perform changes on ticket def perform_changes(perform, perform_origin, item = nil, current_user_id = nil) logger.debug { "Perform #{perform_origin} #{perform.inspect} on Ticket.find(#{id})" } + article = if item.try(:key?, :article_id) + Ticket::Article.lookup(id: item[:article_id]) + end + # if the configuration contains the deletion of the ticket then # we skip all other ticket changes because they does not matter if perform['ticket.action'].present? && perform['ticket.action']['value'] == 'delete' @@ -835,8 +839,7 @@ perform changes on ticket recipients_raw = [] value_recipient.each do |recipient| if recipient == 'article_last_sender' - if item && item[:article_id] - article = Ticket::Article.lookup(id: item[:article_id]) + if article.present? if article.reply_to.present? recipients_raw.push(article.reply_to) elsif article.from.present? @@ -914,12 +917,9 @@ perform changes on ticket end # check if notification should be send because of customer emails - if item && item[:article_id] - article = Ticket::Article.lookup(id: item[:article_id]) - if article&.preferences&.fetch('is-auto-response', false) == true && article.from && article.from =~ /#{Regexp.quote(recipient_email)}/i - logger.info "Send no trigger based notification to #{recipient_email} because of auto response tagged incoming email" - next - end + if article.present? && article.preferences.fetch('is-auto-response', false) == true && article.from && article.from =~ /#{Regexp.quote(recipient_email)}/i + logger.info "Send no trigger based notification to #{recipient_email} because of auto response tagged incoming email" + next end # loop protection / check if maximal count of trigger mail has reached @@ -984,9 +984,12 @@ perform changes on ticket next end + # articles.last breaks (returns the wrong article) + # if another email notification trigger preceded this one + # (see https://github.com/zammad/zammad/issues/1543) objects = { ticket: self, - article: articles.last, + article: article || articles.last } # get subject @@ -1007,7 +1010,7 @@ perform changes on ticket (body, attachments_inline) = HtmlSanitizer.replace_inline_images(body, id) - article = Ticket::Article.create( + message = Ticket::Article.create( ticket_id: id, to: recipient_string, subject: subject, @@ -1026,7 +1029,7 @@ perform changes on ticket attachments_inline.each do |attachment| Store.add( object: 'Ticket::Article', - o_id: article.id, + o_id: message.id, data: attachment[:data], filename: attachment[:filename], preferences: attachment[:preferences], From 461505545afb5cb33f7be7cf5c1826faa717d7ed Mon Sep 17 00:00:00 2001 From: Ryan Lue Date: Wed, 23 May 2018 19:17:05 +0800 Subject: [PATCH 05/11] Revert previous fix (pending specs) --- app/models/ticket.rb | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/app/models/ticket.rb b/app/models/ticket.rb index f71bc5300..60d50c1b6 100644 --- a/app/models/ticket.rb +++ b/app/models/ticket.rb @@ -806,6 +806,10 @@ perform changes on ticket def perform_changes(perform, perform_origin, item = nil, current_user_id = nil) logger.debug { "Perform #{perform_origin} #{perform.inspect} on Ticket.find(#{id})" } + article = if item.try(:key?, :article_id) + Ticket::Article.lookup(id: item[:article_id]) + end + # if the configuration contains the deletion of the ticket then # we skip all other ticket changes because they does not matter if perform['ticket.action'].present? && perform['ticket.action']['value'] == 'delete' @@ -835,8 +839,7 @@ perform changes on ticket recipients_raw = [] value_recipient.each do |recipient| if recipient == 'article_last_sender' - if item && item[:article_id] - article = Ticket::Article.lookup(id: item[:article_id]) + if article.present? if article.reply_to.present? recipients_raw.push(article.reply_to) elsif article.from.present? @@ -914,12 +917,9 @@ perform changes on ticket end # check if notification should be send because of customer emails - if item && item[:article_id] - article = Ticket::Article.lookup(id: item[:article_id]) - if article&.preferences&.fetch('is-auto-response', false) == true && article.from && article.from =~ /#{Regexp.quote(recipient_email)}/i - logger.info "Send no trigger based notification to #{recipient_email} because of auto response tagged incoming email" - next - end + if article.present? && article.preferences.fetch('is-auto-response', false) == true && article.from && article.from =~ /#{Regexp.quote(recipient_email)}/i + logger.info "Send no trigger based notification to #{recipient_email} because of auto response tagged incoming email" + next end # loop protection / check if maximal count of trigger mail has reached @@ -984,9 +984,12 @@ perform changes on ticket next end + # articles.last breaks (returns the wrong article) + # if another email notification trigger preceded this one + # (see https://github.com/zammad/zammad/issues/1543) objects = { ticket: self, - article: articles.last, + article: article || articles.last } # get subject @@ -1007,7 +1010,7 @@ perform changes on ticket (body, attachments_inline) = HtmlSanitizer.replace_inline_images(body, id) - article = Ticket::Article.create( + message = Ticket::Article.create( ticket_id: id, to: recipient_string, subject: subject, @@ -1026,7 +1029,7 @@ perform changes on ticket attachments_inline.each do |attachment| Store.add( object: 'Ticket::Article', - o_id: article.id, + o_id: message.id, data: attachment[:data], filename: attachment[:filename], preferences: attachment[:preferences], From 3034ff7e97f356ce529c1816e34236f7692b7575 Mon Sep 17 00:00:00 2001 From: Ryan Lue Date: Wed, 23 May 2018 19:19:49 +0800 Subject: [PATCH 06/11] REALLY revert last fix --- app/models/ticket.rb | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/app/models/ticket.rb b/app/models/ticket.rb index 60d50c1b6..f71bc5300 100644 --- a/app/models/ticket.rb +++ b/app/models/ticket.rb @@ -806,10 +806,6 @@ perform changes on ticket def perform_changes(perform, perform_origin, item = nil, current_user_id = nil) logger.debug { "Perform #{perform_origin} #{perform.inspect} on Ticket.find(#{id})" } - article = if item.try(:key?, :article_id) - Ticket::Article.lookup(id: item[:article_id]) - end - # if the configuration contains the deletion of the ticket then # we skip all other ticket changes because they does not matter if perform['ticket.action'].present? && perform['ticket.action']['value'] == 'delete' @@ -839,7 +835,8 @@ perform changes on ticket recipients_raw = [] value_recipient.each do |recipient| if recipient == 'article_last_sender' - if article.present? + if item && item[:article_id] + article = Ticket::Article.lookup(id: item[:article_id]) if article.reply_to.present? recipients_raw.push(article.reply_to) elsif article.from.present? @@ -917,9 +914,12 @@ perform changes on ticket end # check if notification should be send because of customer emails - if article.present? && article.preferences.fetch('is-auto-response', false) == true && article.from && article.from =~ /#{Regexp.quote(recipient_email)}/i - logger.info "Send no trigger based notification to #{recipient_email} because of auto response tagged incoming email" - next + if item && item[:article_id] + article = Ticket::Article.lookup(id: item[:article_id]) + if article&.preferences&.fetch('is-auto-response', false) == true && article.from && article.from =~ /#{Regexp.quote(recipient_email)}/i + logger.info "Send no trigger based notification to #{recipient_email} because of auto response tagged incoming email" + next + end end # loop protection / check if maximal count of trigger mail has reached @@ -984,12 +984,9 @@ perform changes on ticket next end - # articles.last breaks (returns the wrong article) - # if another email notification trigger preceded this one - # (see https://github.com/zammad/zammad/issues/1543) objects = { ticket: self, - article: article || articles.last + article: articles.last, } # get subject @@ -1010,7 +1007,7 @@ perform changes on ticket (body, attachments_inline) = HtmlSanitizer.replace_inline_images(body, id) - message = Ticket::Article.create( + article = Ticket::Article.create( ticket_id: id, to: recipient_string, subject: subject, @@ -1029,7 +1026,7 @@ perform changes on ticket attachments_inline.each do |attachment| Store.add( object: 'Ticket::Article', - o_id: message.id, + o_id: article.id, data: attachment[:data], filename: attachment[:filename], preferences: attachment[:preferences], From 163a5f2f2613cf106966002ffc18e6da1cc2c514 Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Fri, 11 May 2018 10:47:20 +0200 Subject: [PATCH 07/11] Refactoring - DRYing up code. Use existing `.backends` method. --- app/models/import_job.rb | 10 +--------- spec/models/import_job_spec.rb | 2 +- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/app/models/import_job.rb b/app/models/import_job.rb index 16bae41c4..335fd2a06 100644 --- a/app/models/import_job.rb +++ b/app/models/import_job.rb @@ -109,15 +109,7 @@ class ImportJob < ApplicationModel # # return [nil] def self.queue_registered - import_backends = Setting.get('import_backends') - return if import_backends.blank? - - import_backends.each do |backend| - - if !backend_valid?(backend) - Rails.logger.error "Invalid import backend '#{backend}'" - next - end + backends.each do |backend| # skip backends that are not "ready" yet next if !backend.constantize.queueable? diff --git a/spec/models/import_job_spec.rb b/spec/models/import_job_spec.rb index 0f5f88f34..a99d029a4 100644 --- a/spec/models/import_job_spec.rb +++ b/spec/models/import_job_spec.rb @@ -120,7 +120,7 @@ RSpec.describe ImportJob do it 'logs errors for invalid registered backends' do allow(Setting).to receive(:get) expect(Setting).to receive(:get).with('import_backends').and_return(['InvalidBackend']) - expect(Rails.logger).to receive(:error) + expect(described_class.logger).to receive(:error) described_class.queue_registered end From c4ea22470a47b5500d84754ac51e0b1577f26dfe Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Thu, 10 May 2018 15:24:11 +0200 Subject: [PATCH 08/11] Fixes #2014 - Import job (LDAP/Exchange) hangs on scheduler restart. --- app/models/import_job.rb | 2 + app/models/scheduler.rb | 58 +++++++++++++--- spec/models/scheduler_spec.rb | 120 ++++++++++++++++++++++------------ 3 files changed, 132 insertions(+), 48 deletions(-) diff --git a/app/models/import_job.rb b/app/models/import_job.rb index 335fd2a06..4623aba5c 100644 --- a/app/models/import_job.rb +++ b/app/models/import_job.rb @@ -5,6 +5,8 @@ class ImportJob < ApplicationModel store :payload store :result + scope :running, -> { where(finished_at: nil, dry_run: false).where.not(started_at: nil) } + # Starts the import backend class based on the name attribute. # Import backend class is initialized with the current instance. # Logs the start and end time (if ended successfully) and logs diff --git a/app/models/scheduler.rb b/app/models/scheduler.rb index e3b1baa60..5aa54e7ca 100644 --- a/app/models/scheduler.rb +++ b/app/models/scheduler.rb @@ -121,9 +121,24 @@ class Scheduler < ApplicationModel raise 'This method should only get called when Scheduler.threads are initialized. Use `force: true` to start anyway.' end - log_start_finish(:info, 'Cleanup of left over locked delayed jobs') do + start_time = Time.zone.now - Delayed::Job.all.each do |job| + cleanup_delayed_jobs(start_time) + cleanup_import_jobs(start_time) + end + + # Checks for locked delayed jobs and tries to reschedule or destroy each of them. + # + # @param [ActiveSupport::TimeWithZone] after the time the cleanup was started + # + # @example + # Scheduler.cleanup_delayed_jobs(TimeZone.now) + # + # return [nil] + def self.cleanup_delayed_jobs(after) + log_start_finish(:info, "Cleanup of left over locked delayed jobs #{after}") do + + Delayed::Job.where('updated_at < ?', after).where.not(locked_at: nil).each do |job| log_start_finish(:info, "Checking left over delayed job #{job.inspect}") do cleanup_delayed(job) end @@ -131,13 +146,13 @@ class Scheduler < ApplicationModel end end - # Checks if the given job can be rescheduled or destroys it. Logs the action as warn. - # Works only for locked jobs. Jobs that are not locked are ignored and + # Checks if the given delayed job can be rescheduled or destroys it. Logs the action as warn. + # Works only for locked delayed jobs. Delayed jobs that are not locked are ignored and # should get destroyed directly. - # Checks the delayed job object for a method called .reschedule?. The memthod is called - # with the delayed job as a parameter. The result value is expected as a Boolean. If the - # result is true the lock gets removed and the delayed job gets rescheduled. If the return - # value is false it will get destroyed which is the default behaviour. + # Checks the Delayed::Job instance for a method called .reschedule?. The method is called + # with the Delayed::Job instance as a parameter. The result value is expected to be a Boolean. + # If the result is true the lock gets removed and the delayed job gets rescheduled. + # If the return value is false it will get destroyed which is the default behaviour. # # @param [Delayed::Job] job the job that should get checked for destroying/rescheduling. # @@ -181,6 +196,33 @@ class Scheduler < ApplicationModel logger.warn "#{action} locked delayed job: #{job_name}" end + # Checks for killed import jobs and marks them as finished and adds a note. + # + # @param [ActiveSupport::TimeWithZone] after the time the cleanup was started + # + # @example + # Scheduler.cleanup_import_jobs(TimeZone.now) + # + # return [nil] + def self.cleanup_import_jobs(after) + log_start_finish(:info, "Cleanup of left over import jobs #{after}") do + error = 'Interrupted by scheduler restart. Please restart manually or wait till next execution time.'.freeze + + # we need to exclude jobs that were updated at or since we started + # cleaning up (via the #reschedule? call) because they might + # were started `.delay`-ed and are flagged for restart + ImportJob.running.where('updated_at < ?', after).each do |job| + + job.update!( + finished_at: after, + result: { + error: error + } + ) + end + end + end + def self.start_job(job) # start job and return thread handle diff --git a/spec/models/scheduler_spec.rb b/spec/models/scheduler_spec.rb index aef9511cc..0f11afc74 100644 --- a/spec/models/scheduler_spec.rb +++ b/spec/models/scheduler_spec.rb @@ -99,56 +99,24 @@ RSpec.describe Scheduler do described_class.cleanup end - it 'keeps unlocked Delayed::Job-s' do - # meta :) - described_class.delay.cleanup + context 'Delayed::Job' do - expect do - simulate_threads_call - end.not_to change { - Delayed::Job.count - } - end - - context 'locked Delayed::Job' do - - it 'gets destroyed' do + it 'keeps unlocked' do # meta :) described_class.delay.cleanup - # lock job (simluates interrupted scheduler task) - locked_job = Delayed::Job.last - locked_job.update!(locked_at: Time.zone.now) - expect do simulate_threads_call - end.to change { + end.not_to change { Delayed::Job.count - }.by(-1) + } end - context 'respond to reschedule?' do + context 'locked' do - it 'gets rescheduled for positive responses' do - SpecSpace::DelayedJobBackend.reschedule = true - SpecSpace::DelayedJobBackend.delay.start - - # lock job (simluates interrupted scheduler task) - locked_job = Delayed::Job.last - locked_job.update!(locked_at: Time.zone.now) - - expect do - simulate_threads_call - end.to not_change { - Delayed::Job.count - }.and change { - Delayed::Job.last.locked_at - } - end - - it 'gets destroyed for negative responses' do - SpecSpace::DelayedJobBackend.reschedule = false - SpecSpace::DelayedJobBackend.delay.start + it 'gets destroyed' do + # meta :) + described_class.delay.cleanup # lock job (simluates interrupted scheduler task) locked_job = Delayed::Job.last @@ -160,6 +128,78 @@ RSpec.describe Scheduler do Delayed::Job.count }.by(-1) end + + context 'respond to reschedule?' do + + it 'gets rescheduled for positive responses' do + SpecSpace::DelayedJobBackend.reschedule = true + SpecSpace::DelayedJobBackend.delay.start + + # lock job (simluates interrupted scheduler task) + locked_job = Delayed::Job.last + locked_job.update!(locked_at: Time.zone.now) + + expect do + simulate_threads_call + end.to not_change { + Delayed::Job.count + }.and change { + Delayed::Job.last.locked_at + } + end + + it 'gets destroyed for negative responses' do + SpecSpace::DelayedJobBackend.reschedule = false + SpecSpace::DelayedJobBackend.delay.start + + # lock job (simluates interrupted scheduler task) + locked_job = Delayed::Job.last + locked_job.update!(locked_at: Time.zone.now) + + expect do + simulate_threads_call + end.to change { + Delayed::Job.count + }.by(-1) + end + end + end + end + + context 'ImportJob' do + + context 'affected job' do + + let(:job) { create(:import_job, started_at: 5.minutes.ago) } + + it 'finishes stuck jobs' do + + expect do + simulate_threads_call + end.to change { + job.reload.finished_at + } + end + + it 'adds an error message to the result' do + + expect do + simulate_threads_call + end.to change { + job.reload.result[:error] + } + end + end + + it "doesn't change jobs added after stop" do + + job = create(:import_job) + + expect do + simulate_threads_call + end.not_to change { + job.reload + } end end end From 4b98c61698631cad9247ee7bddd6a25aeb065dac Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Thu, 24 May 2018 14:46:22 +0200 Subject: [PATCH 09/11] Improved handling of disposition URL parameter for local URLs. --- lib/html_sanitizer.rb | 23 ++++++++++++++++++++++- test/unit/html_sanitizer_test.rb | 19 ++++++++++++++++++- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/lib/html_sanitizer.rb b/lib/html_sanitizer.rb index 8c6308a2b..f8b7a09ba 100644 --- a/lib/html_sanitizer.rb +++ b/lib/html_sanitizer.rb @@ -380,7 +380,28 @@ cleanup html string: else /[[:space:]]|\t|\n|\r/ end - string.strip.gsub(blank_regex, '').gsub(%r{/\*.*?\*/}, '').gsub(//, '').gsub(/\[.+?\]/, '').delete("\u0000") + cleaned_string = string.strip.gsub(blank_regex, '').gsub(%r{/\*.*?\*/}, '').gsub(//, '').gsub(/\[.+?\]/, '').delete("\u0000") + sanitize_attachment_disposition(cleaned_string) + end + + def self.sanitize_attachment_disposition(url) + uri = URI(url) + return url if uri.host != Setting.get('fqdn') + + params = CGI.parse(uri.query || '') + if params.key?('disposition') + params['disposition'] = 'attachment' + end + + uri.query = if params.blank? + nil + else + URI.encode_www_form(params) + end + + uri.to_s + rescue URI::InvalidURIError + url end def self.url_same?(url_new, url_old) diff --git a/test/unit/html_sanitizer_test.rb b/test/unit/html_sanitizer_test.rb index e084a63cc..9c95a0dad 100644 --- a/test/unit/html_sanitizer_test.rb +++ b/test/unit/html_sanitizer_test.rb @@ -113,9 +113,26 @@ test 123 assert_equal(HtmlSanitizer.strict('
123
'), '
123
') assert_equal(HtmlSanitizer.strict('
123
'), '
123
') assert_equal(HtmlSanitizer.strict('
123
'), '
123
') + assert_equal(HtmlSanitizer.strict('
123
'), '
123
') assert_equal(HtmlSanitizer.strict('test'), 'test') assert_equal(HtmlSanitizer.strict('test'), 'test') assert_equal(HtmlSanitizer.strict('test'), 'test') - end + api_path = Rails.configuration.api_path + http_type = Setting.get('http_type') + fqdn = Setting.get('fqdn') + attachment_url = "#{http_type}://#{fqdn}#{api_path}/ticket_attachment/239/986/1653" + attachment_url_good = "#{attachment_url}?disposition=attachment" + attachment_url_evil = "#{attachment_url}?disposition=inline" + assert_equal(HtmlSanitizer.strict("Evil link"), "Evil link") + + assert_equal(HtmlSanitizer.strict("Good link"), "Good link") + assert_equal(HtmlSanitizer.strict("No disposition"), "No disposition") + + different_fqdn_url = attachment_url_evil.gsub(fqdn, 'some.other.tld') + assert_equal(HtmlSanitizer.strict("Different FQDN"), "Different FQDN") + + attachment_url_evil_other = "#{attachment_url}?disposition=some_other" + assert_equal(HtmlSanitizer.strict("Evil link"), "Evil link") + end end From c21be6e928e0a805891a4a9019eda6cff8244c62 Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Wed, 23 May 2018 18:46:01 +0200 Subject: [PATCH 10/11] Fixed issue #2039: Import fails for ObjectManager Attribute with non-ASCII name. --- .../common/object_attribute/sanitized_name.rb | 17 ++++++++++++++++- .../object_attribute/sanitized_name_spec.rb | 16 ++++++++++++---- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/lib/sequencer/unit/import/common/object_attribute/sanitized_name.rb b/lib/sequencer/unit/import/common/object_attribute/sanitized_name.rb index 4afd75537..9096c001a 100644 --- a/lib/sequencer/unit/import/common/object_attribute/sanitized_name.rb +++ b/lib/sequencer/unit/import/common/object_attribute/sanitized_name.rb @@ -12,6 +12,7 @@ class Sequencer # model_nos # model_name # model_name + # model_name without_double_underscores.gsub(/_id(s?)$/, '_no\1') end @@ -20,6 +21,7 @@ class Sequencer # model_ids # model_name # model_name + # model_name without_spaces_and_slashes.gsub(/_{2,}/, '_') end @@ -28,7 +30,17 @@ class Sequencer # model_ids # model___name # model_name - unsanitized_name.gsub(%r{[\s\/]}, '_').underscore + # model_name + transliterated.gsub(%r{[\s\/]}, '_').underscore + end + + def transliterated + # Model ID + # Model IDs + # Model / Name + # Model Name + # Model Name + ::ActiveSupport::Inflector.transliterate(unsanitized_name, '_'.freeze) end def unsanitized_name @@ -36,6 +48,9 @@ class Sequencer # Model IDs # Model / Name # Model Name + # rubocop:disable Style/AsciiComments + # Mödel Nâmé + # rubocop:enable Style/AsciiComments raise 'Missing implementation for unsanitized_name method' end end diff --git a/spec/lib/sequencer/unit/import/common/object_attribute/sanitized_name_spec.rb b/spec/lib/sequencer/unit/import/common/object_attribute/sanitized_name_spec.rb index ea4d43c40..d471fb53d 100644 --- a/spec/lib/sequencer/unit/import/common/object_attribute/sanitized_name_spec.rb +++ b/spec/lib/sequencer/unit/import/common/object_attribute/sanitized_name_spec.rb @@ -11,7 +11,7 @@ RSpec.describe Sequencer::Unit::Import::Common::ObjectAttribute::SanitizedName, context 'sanitizes' do - it 'whitespaces' do + it 'replaces whitespaces' do provided = process do |instance| expect(instance).to receive(:unsanitized_name).and_return('model name') end @@ -19,7 +19,7 @@ RSpec.describe Sequencer::Unit::Import::Common::ObjectAttribute::SanitizedName, expect(provided[:sanitized_name]).to eq('model_name') end - it 'dashes' do + it 'replaces dashes' do provided = process do |instance| expect(instance).to receive(:unsanitized_name).and_return('model-name') end @@ -27,7 +27,7 @@ RSpec.describe Sequencer::Unit::Import::Common::ObjectAttribute::SanitizedName, expect(provided[:sanitized_name]).to eq('model_name') end - it 'ids suffix' do + it 'replaces ids suffix' do provided = process do |instance| expect(instance).to receive(:unsanitized_name).and_return('Model Ids') end @@ -35,12 +35,20 @@ RSpec.describe Sequencer::Unit::Import::Common::ObjectAttribute::SanitizedName, expect(provided[:sanitized_name]).to eq('model_nos') end - it 'id suffix' do + it 'replaces id suffix' do provided = process do |instance| expect(instance).to receive(:unsanitized_name).and_return('Model Id') end expect(provided[:sanitized_name]).to eq('model_no') end + + it 'replaces non-ASCII characters' do + provided = process do |instance| + expect(instance).to receive(:unsanitized_name).and_return('Ærøskøbing Ät Mödél') + end + + expect(provided[:sanitized_name]).to eq('a_eroskobing_at_model') + end end end From ad3362e94c2339428a658bf554b40c2dd7997553 Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Wed, 23 May 2018 18:53:07 +0200 Subject: [PATCH 11/11] Fixed issue #2040: Zendesk import: To/From extraction for system comments fails. --- .../zendesk/ticket/comment/source_based.rb | 9 +++-- .../ticket/comment/source_based_spec.rb | 39 +++++++++++++++++++ 2 files changed, 45 insertions(+), 3 deletions(-) create mode 100644 spec/lib/sequencer/unit/import/zendesk/ticket/comment/source_based_spec.rb diff --git a/lib/sequencer/unit/import/zendesk/ticket/comment/source_based.rb b/lib/sequencer/unit/import/zendesk/ticket/comment/source_based.rb index ee130de05..5932ff3a8 100644 --- a/lib/sequencer/unit/import/zendesk/ticket/comment/source_based.rb +++ b/lib/sequencer/unit/import/zendesk/ticket/comment/source_based.rb @@ -9,9 +9,12 @@ class Sequencer uses :resource def value - method_name = resource.via.channel.to_sym - return if !respond_to?(method_name, true) - send(method_name) + return if private_methods(false).exclude?(value_method_name) + send(value_method_name) + end + + def value_method_name + @value_method_name ||= resource.via.channel.to_sym end end end diff --git a/spec/lib/sequencer/unit/import/zendesk/ticket/comment/source_based_spec.rb b/spec/lib/sequencer/unit/import/zendesk/ticket/comment/source_based_spec.rb new file mode 100644 index 000000000..bb09d5659 --- /dev/null +++ b/spec/lib/sequencer/unit/import/zendesk/ticket/comment/source_based_spec.rb @@ -0,0 +1,39 @@ +require 'rails_helper' + +RSpec.describe Sequencer::Unit::Import::Zendesk::Ticket::Comment::SourceBased, sequencer: :unit do + + before(:all) do + + described_class.class_eval do + + private + + def email + 'test@example.com' + end + end + end + + def parameters_for_channel(channel) + { + resource: double( + via: double( + channel: channel + ) + ) + } + end + + context 'for resource.via.channel attribute' do + + it 'provides from existing method' do + provided = process(parameters_for_channel('email')) + expect(provided[:source_based]).to eq('test@example.com') + end + + it 'provides nil for non existing method' do + provided = process(parameters_for_channel('system')) + expect(provided[:source_based]).to be_nil + end + end +end