From 312278c2d675ab7183024021ee60886882175f2b Mon Sep 17 00:00:00 2001 From: Ryan Lue Date: Mon, 28 May 2018 14:42:04 +0800 Subject: [PATCH] Prettify phone numbers displayed in Call Log --- Gemfile | 3 + Gemfile.lock | 4 +- .../app/views/cti/caller_log.jst.eco | 8 +- app/models/cti/log.rb | 39 +++-- script/build/test_slice_tests.sh | 24 +-- spec/factories/cti/log.rb | 9 ++ spec/models/cti/log.rb | 46 ++++++ ...ti_notify_not_clearing_on_leftside_test.rb | 65 -------- test/browser/integration_cti_test.rb | 147 ++++++++++++++++++ ...de_test.rb => integration_sipgate_test.rb} | 8 +- test/browser_test_helper.rb | 65 ++++---- 11 files changed, 284 insertions(+), 134 deletions(-) create mode 100644 spec/factories/cti/log.rb create mode 100644 spec/models/cti/log.rb delete mode 100644 test/browser/integration_cti_notify_not_clearing_on_leftside_test.rb create mode 100644 test/browser/integration_cti_test.rb rename test/browser/{integration_sipgate_notify_not_clearing_on_leftside_test.rb => integration_sipgate_test.rb} (91%) diff --git a/Gemfile b/Gemfile index c5aa43639..7fd34a003 100644 --- a/Gemfile +++ b/Gemfile @@ -100,6 +100,9 @@ gem 'browser' gem 'icalendar' gem 'icalendar-recurrence' +# feature - phone number formatting +gem 'telephone_number' + # integrations gem 'clearbit' gem 'net-ldap' diff --git a/Gemfile.lock b/Gemfile.lock index 79d661a52..a25f5978c 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -413,6 +413,7 @@ GEM sqlite3 (1.3.13) telegramAPI (1.4.2) rest-client (~> 2.0, >= 2.0.2) + telephone_number (1.3.0) term-ansicolor (1.6.0) tins (~> 1.0) test-unit (3.2.6) @@ -542,6 +543,7 @@ DEPENDENCIES sprockets sqlite3 telegramAPI + telephone_number test-unit therubyracer twitter @@ -557,4 +559,4 @@ RUBY VERSION ruby 2.4.4p296 BUNDLED WITH - 1.16.1 + 1.16.2 diff --git a/app/assets/javascripts/app/views/cti/caller_log.jst.eco b/app/assets/javascripts/app/views/cti/caller_log.jst.eco index b5faa1a7f..d00c71e4a 100644 --- a/app/assets/javascripts/app/views/cti/caller_log.jst.eco +++ b/app/assets/javascripts/app/views/cti/caller_log.jst.eco @@ -40,9 +40,9 @@
<% end %> <% if shown: %> - <%= item.from %> + <%= item.from_pretty %> <% else: %> - <%= item.from %> + <%= item.from_pretty %> <% end %> @@ -66,9 +66,9 @@
<% end %> <% if shown: %> - <%= item.to %> + <%= item.to_pretty %> <% else: %> - <%= item.to %> + <%= item.to_pretty %> <% end %> diff --git a/app/models/cti/log.rb b/app/models/cti/log.rb index af35c5cc7..edb99b8d4 100644 --- a/app/models/cti/log.rb +++ b/app/models/cti/log.rb @@ -246,19 +246,12 @@ returns list = Cti::Log.order('created_at DESC, id DESC').limit(60) # add assets - assets = {} - list.each do |item| - next if !item.preferences - %w[from to].each do |direction| - next if !item.preferences[direction] - item.preferences[direction].each do |caller_id| - next if !caller_id['user_id'] - user = User.lookup(id: caller_id['user_id']) - next if !user - assets = user.assets(assets) - end - end - end + assets = list.map(&:preferences) + .map { |p| p.slice(:from, :to) } + .map(&:values).flatten + .map { |caller_id| caller_id[:user_id] }.compact + .map { |user_id| User.lookup(id: user_id) }.compact + .each.with_object({}) { |user, a| user.assets(a) } { list: list, @@ -392,5 +385,25 @@ optional you can put the max oldest chat entries as argument true end + # adds virtual attributes when rendering #to_json + # see http://api.rubyonrails.org/classes/ActiveModel/Serialization.html + def attributes + virtual_attributes = { + 'from_pretty' => from_pretty, + 'to_pretty' => to_pretty, + } + + super.merge(virtual_attributes) + end + + def from_pretty + parsed = TelephoneNumber.parse(from&.sub(/^\+?/, '+')) + parsed.send(parsed.valid? ? :international_number : :original_number) + end + + def to_pretty + parsed = TelephoneNumber.parse(to&.sub(/^\+?/, '+')) + parsed.send(parsed.valid? ? :international_number : :original_number) + end end end diff --git a/script/build/test_slice_tests.sh b/script/build/test_slice_tests.sh index 4a30d2fdf..f705f0c0f 100755 --- a/script/build/test_slice_tests.sh +++ b/script/build/test_slice_tests.sh @@ -65,8 +65,8 @@ if [ "$LEVEL" == '1' ]; then # test/browser/maintenance_session_message_test.rb # test/browser/manage_test.rb # test/browser/monitoring_test.rb - rm test/browser/integration_sipgate_notify_not_clearing_on_leftside_test.rb - rm test/browser/integration_cti_notify_not_clearing_on_leftside_test.rb + rm test/browser/integration_sipgate_test.rb + rm test/browser/integration_cti_test.rb rm test/browser/preferences_language_test.rb rm test/browser/preferences_permission_check_test.rb rm test/browser/preferences_token_access_test.rb @@ -139,8 +139,8 @@ elif [ "$LEVEL" == '2' ]; then rm test/browser/maintenance_session_message_test.rb rm test/browser/manage_test.rb rm test/browser/monitoring_test.rb - rm test/browser/integration_sipgate_notify_not_clearing_on_leftside_test.rb - rm test/browser/integration_cti_notify_not_clearing_on_leftside_test.rb + rm test/browser/integration_sipgate_test.rb + rm test/browser/integration_cti_test.rb rm test/browser/preferences_language_test.rb rm test/browser/preferences_permission_check_test.rb rm test/browser/preferences_token_access_test.rb @@ -213,8 +213,8 @@ elif [ "$LEVEL" == '3' ]; then rm test/browser/maintenance_session_message_test.rb rm test/browser/manage_test.rb rm test/browser/monitoring_test.rb - rm test/browser/integration_sipgate_notify_not_clearing_on_leftside_test.rb - rm test/browser/integration_cti_notify_not_clearing_on_leftside_test.rb + rm test/browser/integration_sipgate_test.rb + rm test/browser/integration_cti_test.rb rm test/browser/preferences_language_test.rb rm test/browser/preferences_permission_check_test.rb rm test/browser/preferences_token_access_test.rb @@ -287,8 +287,8 @@ elif [ "$LEVEL" == '4' ]; then rm test/browser/maintenance_session_message_test.rb rm test/browser/manage_test.rb rm test/browser/monitoring_test.rb - rm test/browser/integration_sipgate_notify_not_clearing_on_leftside_test.rb - rm test/browser/integration_cti_notify_not_clearing_on_leftside_test.rb + rm test/browser/integration_sipgate_test.rb + rm test/browser/integration_cti_test.rb rm test/browser/preferences_language_test.rb rm test/browser/preferences_permission_check_test.rb rm test/browser/preferences_token_access_test.rb @@ -360,8 +360,8 @@ elif [ "$LEVEL" == '5' ]; then rm test/browser/maintenance_session_message_test.rb rm test/browser/manage_test.rb rm test/browser/monitoring_test.rb - rm test/browser/integration_sipgate_notify_not_clearing_on_leftside_test.rb - rm test/browser/integration_cti_notify_not_clearing_on_leftside_test.rb + rm test/browser/integration_sipgate_test.rb + rm test/browser/integration_cti_test.rb rm test/browser/preferences_language_test.rb rm test/browser/preferences_permission_check_test.rb rm test/browser/preferences_token_access_test.rb @@ -436,8 +436,8 @@ elif [ "$LEVEL" == '6' ]; then rm test/browser/maintenance_session_message_test.rb rm test/browser/manage_test.rb rm test/browser/monitoring_test.rb - # rm test/browser/integration_sipgate_notify_not_clearing_on_leftside_test.rb - # rm test/browser/integration_cti_notify_not_clearing_on_leftside_test.rb + # rm test/browser/integration_sipgate_test.rb + # rm test/browser/integration_cti_test.rb # test/browser/preferences_language_test.rb # test/browser/preferences_permission_check_test.rb # test/browser/preferences_token_access_test.rb diff --git a/spec/factories/cti/log.rb b/spec/factories/cti/log.rb new file mode 100644 index 000000000..c2f3b0cdd --- /dev/null +++ b/spec/factories/cti/log.rb @@ -0,0 +1,9 @@ +FactoryBot.define do + factory :cti_log, class: 'cti/log' do + direction { %w[in out].sample } + state { %w[newCall answer hangup].sample } + from '4930609854180' + to '4930609811111' + call_id { (Cti::Log.pluck(:call_id).max || '0').next } # has SQL UNIQUE constraint + end +end diff --git a/spec/models/cti/log.rb b/spec/models/cti/log.rb new file mode 100644 index 000000000..519b1b978 --- /dev/null +++ b/spec/models/cti/log.rb @@ -0,0 +1,46 @@ +require 'rails_helper' + +RSpec.describe Cti::Log do + subject { create(:cti_log, **factory_attributes) } + let(:factory_attributes) { {} } + + context 'with complete, E164 international numbers' do + let(:factory_attributes) { { from: '4930609854180', to: '4930609811111' } } + + describe '#from_pretty' do + it 'gives the number in prettified format' do + expect(subject.from_pretty).to eq('+49 30 609854180') + end + end + + describe '#to_pretty' do + it 'gives the number in prettified format' do + expect(subject.to_pretty).to eq('+49 30 609811111') + end + end + end + + context 'with private network numbers' do + let(:factory_attributes) { { from: '007', to: '008' } } + + describe '#from_pretty' do + it 'gives the number unaltered' do + expect(subject.from_pretty).to eq('007') + end + end + + describe '#to_pretty' do + it 'gives the number unaltered' do + expect(subject.to_pretty).to eq('008') + end + end + end + + describe '#to_json' do + let(:virtual_attributes) { %w[from_pretty to_pretty] } + + it 'includes virtual attributes' do + expect(subject.as_json).to include(*virtual_attributes) + end + end +end diff --git a/test/browser/integration_cti_notify_not_clearing_on_leftside_test.rb b/test/browser/integration_cti_notify_not_clearing_on_leftside_test.rb deleted file mode 100644 index ce222186e..000000000 --- a/test/browser/integration_cti_notify_not_clearing_on_leftside_test.rb +++ /dev/null @@ -1,65 +0,0 @@ -require 'browser_test_helper' - -# Regression test for #2017 - -class IntegrationCtiNotifyNotClearingOnLeftsideTest < TestCase - setup do - if !ENV['CTI_TOKEN'] - raise "ERROR: Need CTI_TOKEN - hint CTI_TOKEN='some_token'" - end - - end - - def test_notify_badge - id = rand(99_999_999) - - @browser = browser_instance - login( - username: 'master@example.com', - password: 'test', - url: browser_url, - ) - - click(css: 'a[href="#manage"]') - click(css: 'a[href="#system/integration"]') - click(css: 'a[href="#system/integration/cti"]') - - switch( - css: '.content.active .js-switch', - type: 'on' - ) - - watch_for( - css: 'a[href="#cti"]' - ) - - click(css: 'a[href="#cti"]') - - # simulate cti callbacks - - url = URI.join(browser_url, "api/v1/cti/#{ENV['CTI_TOKEN']}") - params = { direction: 'in', from: '491715000002', to: '4930600000000', callId: "4991155921769858278-#{id}", cause: 'busy' } - Net::HTTP.post_form(url, params.merge(event: 'newCall')) - Net::HTTP.post_form(url, params.merge(event: 'hangup')) - - watch_for( - css: '.js-phoneMenuItem .counter', - value: '1' - ) - - click(css: '.content.active .table-checkbox label') - - watch_for_disappear( - css: '.js-phoneMenuItem .counter' - ) - - click(css: 'a[href="#manage"]') - click(css: 'a[href="#system/integration"]') - click(css: 'a[href="#system/integration/cti"]') - - switch( - css: '.content.active .js-switch', - type: 'off' - ) - end -end diff --git a/test/browser/integration_cti_test.rb b/test/browser/integration_cti_test.rb new file mode 100644 index 000000000..71edf2a00 --- /dev/null +++ b/test/browser/integration_cti_test.rb @@ -0,0 +1,147 @@ +require 'browser_test_helper' + +class IntegrationCtiTest < TestCase + setup do + if !ENV['CTI_TOKEN'] + raise "ERROR: Need CTI_TOKEN - hint CTI_TOKEN='some_token'" + end + + end + + # Regression test for #2017 + def test_nav_menu_notification_badge_clears + id = rand(99_999_999) + + @browser = browser_instance + login( + username: 'master@example.com', + password: 'test', + url: browser_url, + ) + + click(css: 'a[href="#manage"]') + click(css: 'a[href="#system/integration"]') + click(css: 'a[href="#system/integration/cti"]') + + switch( + css: '.content.active .js-switch', + type: 'on' + ) + + watch_for(css: 'a[href="#cti"]') + + click(css: 'a[href="#cti"]') + + call_counter = @browser.find_elements(css: '.js-phoneMenuItem .counter') + .first&.text.to_i + + # simulate cti callbacks + url = URI.join(browser_url, "api/v1/cti/#{ENV['CTI_TOKEN']}") + params = { + direction: 'in', + from: '491715000002', + to: '4930600000000', + callId: "4991155921769858278-#{id}", + cause: 'busy' + } + Net::HTTP.post_form(url, params.merge(event: 'newCall')) + Net::HTTP.post_form(url, params.merge(event: 'hangup')) + + watch_for( + css: '.js-phoneMenuItem .counter', + value: (call_counter + 1).to_s + ) + + click(css: '.content.active .table-checkbox label', all: true) + + watch_for_disappear( + css: '.js-phoneMenuItem .counter' + ) + + click(css: 'a[href="#manage"]') + click(css: 'a[href="#system/integration"]') + click(css: 'a[href="#system/integration/cti"]') + + switch( + css: '.content.active .js-switch', + type: 'off' + ) + end + + # Regression test for #2018 + def test_e164_numbers_displayed_in_prettified_format + id = rand(99_999_999) + + @browser = browser_instance + login( + username: 'master@example.com', + password: 'test', + url: browser_url, + ) + + click(css: 'a[href="#manage"]') + click(css: 'a[href="#system/integration"]') + click(css: 'a[href="#system/integration/cti"]') + + switch( + css: '.content.active .js-switch', + type: 'on' + ) + + watch_for( + css: 'a[href="#cti"]' + ) + + click(css: 'a[href="#cti"]') + + # simulate cti callbacks... + url = URI.join(browser_url, "api/v1/cti/#{ENV['CTI_TOKEN']}") + + # ...for private network number + params = { + direction: 'in', + from: '007', + to: '008', + callId: "4991155921769858278-#{id}", + cause: 'busy' + } + Net::HTTP.post_form(url, params.merge(event: 'newCall')) + Net::HTTP.post_form(url, params.merge(event: 'hangup')) + + # ...for e164 number + params = { + direction: 'in', + from: '4930609854180', + to: '4930609811111', + callId: "4991155921769858278-#{id.next}", + cause: 'busy' + } + Net::HTTP.post_form(url, params.merge(event: 'newCall')) + Net::HTTP.post_form(url, params.merge(event: 'hangup')) + + # view caller log + click(css: 'a[href="#cti"]') + + # assertion: private network numbers appear verbatim + match( + css: '.js-callerLog', + value: '007', + ) + + match( + css: '.js-callerLog', + value: '008', + ) + + # assertion: E164 numbers appear prettified + match( + css: '.js-callerLog', + value: '+49 30 609854180', + ) + + match( + css: '.js-callerLog', + value: '+49 30 609811111', + ) + end +end diff --git a/test/browser/integration_sipgate_notify_not_clearing_on_leftside_test.rb b/test/browser/integration_sipgate_test.rb similarity index 91% rename from test/browser/integration_sipgate_notify_not_clearing_on_leftside_test.rb rename to test/browser/integration_sipgate_test.rb index 7f8ee8cfc..e30794e2d 100644 --- a/test/browser/integration_sipgate_notify_not_clearing_on_leftside_test.rb +++ b/test/browser/integration_sipgate_test.rb @@ -1,9 +1,8 @@ require 'browser_test_helper' -# Regression test for #2017 - -class IntegrationSipgateNotifyNotClearingOnLeftsideTest < TestCase - def test_notify_badge +class IntegrationSipgateTest < TestCase + # Regression test for #2017 + def test_nav_menu_notification_badge_clears id = rand(99_999_999) @browser = browser_instance @@ -29,7 +28,6 @@ class IntegrationSipgateNotifyNotClearingOnLeftsideTest < TestCase click(css: 'a[href="#cti"]') # simulate sipgate callbacks - url = URI.join(browser_url, 'api/v1/sipgate/in') params = { direction: 'in', from: '491715000002', to: '4930600000000', callId: "4991155921769858278-#{id}", cause: 'busy' } Net::HTTP.post_form(url, params.merge(event: 'newCall')) diff --git a/test/browser_test_helper.rb b/test/browser_test_helper.rb index 3bf22a0bc..b55b53f91 100644 --- a/test/browser_test_helper.rb +++ b/test/browser_test_helper.rb @@ -409,43 +409,40 @@ class TestCase < Test::Unit::TestCase log('click', params) instance = params[:browser] || @browser - if params[:css] - - begin - element = instance.find_elements(css: params[:css])[0] - return if !element && params[:only_if_exists] == true - #if element - # instance.action.move_to(element).release.perform - #end - element.click - rescue => e - sleep 0.5 - - # just try again - log('click', { rescure: true }) - element = instance.find_elements(css: params[:css])[0] - return if !element && params[:only_if_exists] == true - #if element - # instance.action.move_to(element).release.perform - #end - raise "No such element '#{params[:css]}'" if !element - element.click - end - + if params.include?(:css) + param_key = :css + find_element_key = :css else + param_key = :text + find_element_key = :partial_link_text sleep 0.5 - begin - instance.find_elements(partial_link_text: params[:text])[0].click - rescue => e - sleep 0.5 - - # just try again - log('click', { rescure: true }) - element = instance.find_elements(partial_link_text: params[:text])[0] - raise "No such element '#{params[:text]}'" if !element - element.click - end end + + begin + elements = instance.find_elements(find_element_key => params[param_key]) + .tap { |e| e.slice!(1..-1) unless params[:all] } + + if elements.empty? + return if params[:only_if_exists] == true + raise "No such element '#{params[param_key]}'" + end + + # a clumsy substitute for elements.each(&:click) + # (we need to refresh element references after each element.click + # because if clicks alter page content, + # subsequent element.clicks will raise a StaleElementReferenceError) + elements.length.times do |i| + instance.find_elements(find_element_key => params[param_key])[i].try(:click) + end + rescue => e + raise e if (fail_count ||= 0).positive? + + fail_count += 1 + log('click', { rescure: true }) + sleep 0.5 + retry + end + sleep 0.2 if !params[:fast] sleep params[:wait] if params[:wait] end