From e43154c3730d5c88d9b0e462d173cc3019b7a944 Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Mon, 7 Jan 2019 10:22:00 +0100 Subject: [PATCH 1/5] Refactoring: Removed unneeded screenshot taking (to speed up tests). --- test/browser_test_helper.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/browser_test_helper.rb b/test/browser_test_helper.rb index 6cea4c82c..e96499073 100644 --- a/test/browser_test_helper.rb +++ b/test/browser_test_helper.rb @@ -2238,10 +2238,8 @@ wait untill text in selector disabppears css: '.content.active .newTicket button.js-submit', mute_log: true, ) - screenshot(browser: instance, comment: 'ticket_create_after_submit_1') sleep 1 - screenshot(browser: instance, comment: 'ticket_create_after_submit_2') 9.times do if instance.current_url.match?(/#{Regexp.quote('#ticket/zoom/')}/) assert(true, 'ticket created') From dadb8e207bcb679297448996d7e587feb7c89017 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Tue, 8 Jan 2019 23:14:47 +0100 Subject: [PATCH 2/5] Follow up for issue #2419 - Zammad Icon (Bird) on page top left disappears and does not come back - fixed test. --- .../app/controllers/agent_ticket_merge.coffee | 13 +++++++++++-- test/browser/agent_ticket_tag_test.rb | 6 ++++++ test/browser_test_helper.rb | 17 ++++------------- 3 files changed, 21 insertions(+), 15 deletions(-) diff --git a/app/assets/javascripts/app/controllers/agent_ticket_merge.coffee b/app/assets/javascripts/app/controllers/agent_ticket_merge.coffee index f894767b9..46ac8e447 100644 --- a/app/assets/javascripts/app/controllers/agent_ticket_merge.coffee +++ b/app/assets/javascripts/app/controllers/agent_ticket_merge.coffee @@ -23,17 +23,26 @@ class App.TicketMerge extends App.ControllerModal @render() ) + onShown: (e) => + super + later = => + if @tableCustomerTickets + @tableCustomerTickets.show() + if @tableRecentViewedTickets + @tableRecentViewedTickets.show() + @delay(later, 300) + content: => content = $( App.view('agent_ticket_merge')() ) - new App.TicketList( + @tableCustomerTickets = new App.TicketList( tableId: 'ticket-merge-customer-tickets' el: content.find('#ticket-merge-customer-tickets') ticket_ids: @ticket_ids_by_customer radio: true ) - new App.TicketList( + @tableRecentViewedTickets = new App.TicketList( tableId: 'ticket-merge-recent-tickets' el: content.find('#ticket-merge-recent-tickets') ticket_ids: @ticket_ids_recent_viewed diff --git a/test/browser/agent_ticket_tag_test.rb b/test/browser/agent_ticket_tag_test.rb index beee9cde9..d1686cdc2 100644 --- a/test/browser/agent_ticket_tag_test.rb +++ b/test/browser/agent_ticket_tag_test.rb @@ -103,6 +103,9 @@ class AgentTicketTagTest < TestCase browser: browser2, number: ticket3[:number], ) + empty_search( + browser: browser2, + ) # set tag #1 click( @@ -272,6 +275,9 @@ class AgentTicketTagTest < TestCase browser: browser2, number: ticket3[:number], ) + empty_search( + browser: browser2, + ) # verify tags tags_verify( diff --git a/test/browser_test_helper.rb b/test/browser_test_helper.rb index e96499073..891a92dc6 100644 --- a/test/browser_test_helper.rb +++ b/test/browser_test_helper.rb @@ -2719,18 +2719,9 @@ wait untill text in selector disabppears element.send_keys(params[:number]) sleep 3 - empty_search(browser: instance) - - # search by number again - element = instance.find_elements(css: '#global-search')[0] - element.click - element.clear - element.send_keys(params[:number]) - sleep 1 - # open ticket #instance.find_element(partial_link_text: params[:number] } ).click - instance.execute_script("$(\".js-global-search-result a:contains('#{params[:number]}') .nav-tab-icon\").first().click()") + instance.execute_script("$(\".js-global-search-result a:contains('#{params[:number]}') .nav-tab-name\").first().click()") watch_for( browser: instance, css: '.content.active .ticketZoom-header .ticket-number' @@ -2767,7 +2758,7 @@ wait untill text in selector disabppears # open ticket #instance.find_element(partial_link_text: params[:title] } ).click - instance.execute_script("$(\".js-global-search-result a:contains('#{params[:title]}') .nav-tab-icon\").click()") + instance.execute_script("$(\".js-global-search-result a:contains('#{params[:title]}') .nav-tab-name\").first().click()") sleep 1 title = instance.find_elements(css: '.content.active .ticketZoom-header .js-objectTitle')[0].text if !title.match?(/#{params[:title]}/) @@ -2854,7 +2845,7 @@ wait untill text in selector disabppears element.send_keys(params[:value]) sleep 2 #instance.find_element(partial_link_text: params[:value] } ).click - instance.execute_script("$(\".js-global-search-result a:contains('#{params[:value]}') .nav-tab-icon\").click()") + instance.execute_script("$(\".js-global-search-result a:contains('#{params[:value]}') .nav-tab-name\").first().click()") watch_for( browser: instance, css: '.content.active h1' @@ -2890,7 +2881,7 @@ wait untill text in selector disabppears sleep 3 #instance.find_element(partial_link_text: params[:value]).click - instance.execute_script("$(\".js-global-search-result a:contains('#{params[:value]}') .nav-tab-icon\").click()") + instance.execute_script("$(\".js-global-search-result a:contains('#{params[:value]}') .nav-tab-name\").first().click()") watch_for( browser: instance, css: '.content.active h1' From 6faaee3a9785e05e085f064ad8cd7080c198e903 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Mon, 14 Jan 2019 15:53:26 +0100 Subject: [PATCH 3/5] Fixed issue #2423 - Unable to update already configured email channel, verify email will not arrive. --- app/models/channel/driver/imap.rb | 60 +++++++++++++++++++++++++++---- app/models/channel/driver/pop3.rb | 22 ++++++++++-- lib/email_helper/probe.rb | 5 ++- 3 files changed, 77 insertions(+), 10 deletions(-) diff --git a/app/models/channel/driver/imap.rb b/app/models/channel/driver/imap.rb index 42c7c8e46..89ccf342c 100644 --- a/app/models/channel/driver/imap.rb +++ b/app/models/channel/driver/imap.rb @@ -145,11 +145,12 @@ example end # check how many content messages we have, for notice used - header = message_meta['RFC822.HEADER'] - if header && header !~ /x-zammad-ignore/i - content_messages += 1 - break if content_max_check < content_messages - end + headers = parse_headers(message_meta['RFC822.HEADER']) + next if messages_is_verify_message?(headers) + next if messages_is_ignore_message?(headers) + + content_messages += 1 + break if content_max_check < content_messages end if content_messages >= content_max_check content_messages = message_ids.count @@ -207,9 +208,12 @@ example message_meta = nil timeout(1.minute) do - message_meta = @imap.fetch(message_id, ['RFC822.SIZE', 'ENVELOPE', 'FLAGS', 'INTERNALDATE'])[0] + message_meta = @imap.fetch(message_id, ['RFC822.SIZE', 'ENVELOPE', 'FLAGS', 'INTERNALDATE', 'RFC822.HEADER'])[0] end + # ignore verify messages + next if !messages_is_too_old_verify?(message_meta, count, count_all) + # ignore to big messages info = too_big?(message_meta, count, count_all) if info @@ -283,6 +287,50 @@ returns private + def messages_is_too_old_verify?(message_meta, count, count_all) + headers = parse_headers(message_meta.attr['RFC822.HEADER']) + return true if !messages_is_verify_message?(headers) + return true if headers['X-Zammad-Verify-Time'].blank? + + begin + verify_time = Time.zone.parse(headers['X-Zammad-Verify-Time']) + rescue => e + Rails.logger.error e + return true + end + return true if verify_time < Time.zone.now - 30.minutes + + Rails.logger.info " - ignore message #{count}/#{count_all} - because message has a verify message" + + false + end + + def messages_is_verify_message?(headers) + return true if headers['X-Zammad-Verify'] == 'true' + + false + end + + def messages_is_ignore_message?(headers) + return true if headers['X-Zammad-Ignore'] == 'true' + + false + end + + def parse_headers(string) + return {} if string.blank? + + headers = {} + headers_pairs = string.split("\r\n") + headers_pairs.each do |pair| + key_value = pair.split(': ') + next if key_value[0].blank? + + headers[key_value[0]] = key_value[1] + end + headers + end + # rubocop:disable Metrics/ParameterLists def already_imported?(message_id, message_meta, count, count_all, keep_on_server, channel) # rubocop:enable Metrics/ParameterLists diff --git a/app/models/channel/driver/pop3.rb b/app/models/channel/driver/pop3.rb index 346f9d444..71725824f 100644 --- a/app/models/channel/driver/pop3.rb +++ b/app/models/channel/driver/pop3.rb @@ -91,7 +91,7 @@ returns next if !mail # check how many content messages we have, for notice used - if !mail.match?(/x-zammad-ignore/i) + if !mail.match?(/(X-Zammad-Ignore: true|X-Zammad-Verify: true)/) content_messages += 1 break if content_max_check < content_messages end @@ -112,7 +112,7 @@ returns mails.reverse! # check for verify message - mails.each do |m| + mails.first(2000).each do |m| mail = m.pop next if !mail @@ -137,12 +137,28 @@ returns count = 0 count_fetched = 0 notice = '' - mails.each do |m| + mails.first(2000).each do |m| count += 1 Rails.logger.info " - message #{count}/#{count_all}" mail = m.pop next if !mail + # ignore verify messages + if mail.match?(/(X-Zammad-Ignore: true|X-Zammad-Verify: true)/) + if mail =~ /X-Zammad-Verify-Time:\s(.+?)\s/ + begin + verify_time = Time.zone.parse($1) + if verify_time > Time.zone.now - 30.minutes + info = " - ignore message #{count}/#{count_all} - because it's a verify message" + Rails.logger.info info + next + end + rescue => e + Rails.logger.error e + end + end + end + # ignore to big messages max_message_size = Setting.get('postmaster_max_size').to_f real_message_size = mail.size.to_f / 1024 / 1024 diff --git a/lib/email_helper/probe.rb b/lib/email_helper/probe.rb index fcdad877a..f44404851 100644 --- a/lib/email_helper/probe.rb +++ b/lib/email_helper/probe.rb @@ -308,10 +308,13 @@ returns on fail body: "This is a Test Email of Zammad to verify if Zammad can send emails to an external address.\n\nIf you see this email, you can ignore and delete it.", } end - if subject + if subject.present? mail['X-Zammad-Test-Message'] = subject end mail['X-Zammad-Ignore'] = 'true' + mail['X-Zammad-Fqdn'] = Setting.get('fqdn') + mail['X-Zammad-Verify'] = 'true' + mail['X-Zammad-Verify-Time'] = Time.zone.now.iso8601 mail['X-Loop'] = 'yes' mail['Precedence'] = 'bulk' mail['Auto-Submitted'] = 'auto-generated' From ce27170434361029f6022fa29de15082bdac8c4d Mon Sep 17 00:00:00 2001 From: Ryan Lue Date: Mon, 14 Jan 2019 16:31:31 +0100 Subject: [PATCH 4/5] Refactoring: Capybara tests use wrong route check method. --- spec/support/capybara/common_actions.rb | 7 ++++--- spec/system/basic/authentication_spec.rb | 4 ++-- spec/system/basic/redirects_spec.rb | 8 ++++---- spec/system/setup/mail_accounts_spec.rb | 2 +- spec/system/setup/system_spec.rb | 10 +++++----- 5 files changed, 16 insertions(+), 15 deletions(-) diff --git a/spec/support/capybara/common_actions.rb b/spec/support/capybara/common_actions.rb index c51122a8f..acf85a141 100644 --- a/spec/support/capybara/common_actions.rb +++ b/spec/support/capybara/common_actions.rb @@ -109,16 +109,17 @@ module CommonActions route = Regexp.new(Regexp.quote("/##{route}")) end - options.reverse_merge!(wait: 0, url: true) + # wait 1 sec by default because Firefox is slow + options.reverse_merge!(wait: 1, url: true) - have_current_path("/##{route}", **options) + have_current_path(route, **options) end # This is a convenient wrapper method around #have_current_route # which requires no previous `expect(page).to ` call. # # @example - # expect_current_routes('login') + # expect_current_route('login') # => checks for SPA route '/#login' # def expect_current_route(route, **options) diff --git a/spec/system/basic/authentication_spec.rb b/spec/system/basic/authentication_spec.rb index 720b14451..35afff7b4 100644 --- a/spec/system/basic/authentication_spec.rb +++ b/spec/system/basic/authentication_spec.rb @@ -8,11 +8,11 @@ RSpec.describe 'Authentication', type: :system do password: 'test', ) - have_current_route 'dashboard' + expect_current_route 'dashboard' end scenario 'Logout' do logout - have_current_route 'login', wait: 2 + expect_current_route 'login', wait: 2 end end diff --git a/spec/system/basic/redirects_spec.rb b/spec/system/basic/redirects_spec.rb index bd28238a1..152cc1e42 100644 --- a/spec/system/basic/redirects_spec.rb +++ b/spec/system/basic/redirects_spec.rb @@ -4,21 +4,21 @@ RSpec.describe 'Unauthenticated redirect', type: :system, authenticated: false d scenario 'Sessions' do visit 'system/sessions' - have_current_route 'login' + expect_current_route 'login' end scenario 'Profile' do visit 'profile/linked' - have_current_route 'login' + expect_current_route 'login' end scenario 'Ticket' do visit 'ticket/zoom/1' - have_current_route 'login' + expect_current_route 'login' end scenario 'Not existing route' do visit 'not_existing' - have_current_route 'not_existing' + expect_current_route 'not_existing' end end diff --git a/spec/system/setup/mail_accounts_spec.rb b/spec/system/setup/mail_accounts_spec.rb index 32687338f..d7919d171 100644 --- a/spec/system/setup/mail_accounts_spec.rb +++ b/spec/system/setup/mail_accounts_spec.rb @@ -13,7 +13,7 @@ RSpec.describe 'Mail accounts', type: :system do # wait for verification process to finish expect(page).to have_css('.js-agent h2', text: 'Invite Colleagues', wait: 4.minutes) - have_current_route 'getting_started/agents' + expect_current_route 'getting_started/agents' end def fill_in_credentials(account) diff --git a/spec/system/setup/system_spec.rb b/spec/system/setup/system_spec.rb index 806737345..33b6b47ba 100644 --- a/spec/system/setup/system_spec.rb +++ b/spec/system/setup/system_spec.rb @@ -54,12 +54,12 @@ RSpec.describe 'System setup process', type: :system, set_up: false do # configure Email Notification expect(page).to have_css('.js-outbound h2', text: 'Email Notification') - have_current_route 'getting_started/email_notification' + expect_current_route 'getting_started/email_notification' click_on('Continue') # create email account expect(page).to have_css('.js-channel h2', text: 'Connect Channels') - have_current_route 'getting_started/channel' + expect_current_route 'getting_started/channel' click('.js-channel .btn.email') within('.js-intro') do @@ -75,7 +75,7 @@ RSpec.describe 'System setup process', type: :system, set_up: false do # wait for verification process to finish expect(page).to have_css('.js-agent h2', text: 'Invite Colleagues', wait: 2.minutes) - have_current_route 'getting_started/agents' + expect_current_route 'getting_started/agents' # invite agent1 within('.js-agent') do @@ -88,14 +88,14 @@ RSpec.describe 'System setup process', type: :system, set_up: false do expect(page).to have_css('body', text: 'Invitation sent!') # expect to still be on the same page - have_current_route 'getting_started/agents' + expect_current_route 'getting_started/agents' within('.js-agent') do click_on('Continue') end # expect Dashboard of a fresh system expect(page).to have_css('body', text: 'My Stats') - have_current_route 'clues' + expect_current_route 'clues' find(:clues_close, wait: 4).in_fixed_postion.click # verify organization and fqdn From 5356c9f9f142b60aa4552d48ec16b29a9af99ae3 Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Tue, 15 Jan 2019 13:11:07 +0100 Subject: [PATCH 5/5] Refactoring: Make asset fetch more robust for records referring to deleted records. --- app/models/application_model/can_assets.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/models/application_model/can_assets.rb b/app/models/application_model/can_assets.rb index c1f084390..5a15d9897 100644 --- a/app/models/application_model/can_assets.rb +++ b/app/models/application_model/can_assets.rb @@ -145,7 +145,9 @@ get assets of object list def assets_of_object_list(list, assets = {}) list.each do |item| require_dependency item['object'].to_filename - record = Kernel.const_get(item['object']).find(item['o_id']) + record = Kernel.const_get(item['object']).lookup(id: item['o_id']) + next if record.blank? + assets = record.assets(assets) if item['created_by_id'].present? user = User.find(item['created_by_id'])