Prettify phone numbers displayed in Call Log

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'

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

app/assets/javascripts/app/views/cti/caller_log.jst.eco

@@ -40,9 +40,9 @@
           <br>
         <% end %>
         <% if shown: %>
-          <small><%= item.from %></small>
+          <small><%= item.from_pretty %></small>
         <% else: %>
-          <span class="js-userNew u-clickable" href="#"><%= item.from %></span>
+          <span class="js-userNew u-clickable" href="#"><%= item.from_pretty %></span>
         <% end %>
       </td>
       <td>
@@ -66,9 +66,9 @@
           <br>
         <% end %>
         <% if shown: %>
-          <small><%= item.to %></small>
+          <small><%= item.to_pretty %></small>
         <% else: %>
-          <%= item.to %>
+          <%= item.to_pretty %>
         <% end %>
       </td>
       <td style="vertical-align: middle">

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

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

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

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

test/browser/integration_cti_notify_not_clearing_on_leftside_test.rb

@@ -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

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

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'))

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