Merge pull request #2259 from internetee/2239-validate-contact-phone-number

Improved phone number validation

app/interactions/actions/contact_update.rb

@@ -15,7 +15,7 @@ module Actions
       maybe_update_statuses
       maybe_update_ident if ident.present?
       maybe_attach_legal_doc
-      maybe_change_email
+      maybe_change_email if new_attributes[:email].present?
       maybe_filtering_old_failed_records
       commit
     end

app/models/depp/contact.rb

@@ -16,6 +16,8 @@ module Depp
       ['Birthday', 'birthday']
     ]
 
+    validates :phone, e164: true, phone: true
+
     class << self
       attr_reader :epp_xml, :user
 
@@ -144,6 +146,8 @@ module Depp
     end
 
     def save
+      return false unless valid?
+
       hash = {
         id: { value: code },
         postalInfo: {
@@ -166,13 +170,15 @@ module Depp
 
       hash[:id] = nil if code.blank?
       create_xml = Depp::Contact.epp_xml.create(hash, extension_xml(:create))
-
       data = Depp::Contact.user.request(create_xml)
       self.id = data.css('id').text
       handle_errors(data)
     end
 
+    # rubocop:disable Metrics/MethodLength
     def update_attributes(params)
+      return false unless valid?
+
       self.ident_country_code = params[:ident_country_code]
       self.ident_type   = params[:ident_type]
       self.ident        = params[:ident]
@@ -220,6 +226,7 @@ module Depp
       data = Depp::Contact.user.request(update_xml)
       handle_errors(data)
     end
+    # rubocop:enable Metrics/MethodLength
 
     def delete
       delete_xml = Contact.epp_xml.delete(

app/views/registrar/contacts/form/_general.haml

@@ -56,7 +56,8 @@
       .col-md-3.control-label
         = f.label :email, t(:email) + '*'
       .col-md-7
-        = f.email_field :email, class: 'form-control', required: true
+        = f.email_field :email, class: 'form-control', required: true,
+          pattern: "[a-z0-9._%+-]+@[a-z0-9.-]+\.[a-z]{2,4}$"
 
     .form-group
       .col-md-3.control-label

config/locales/en.yml

@@ -377,6 +377,11 @@ en:
   activemodel:
     errors:
       models:
+        'depp/contact':
+          attributes:
+            phone:
+              invalid: "Phone number must be in +XXX.YYYYYYY format"
+              too_long: "Phone number is too long"
         'depp/user':
           attributes:
             base:

lib/validators/e164_validator.rb

@@ -5,7 +5,7 @@ class E164Validator < ActiveModel::EachValidator
     length_validator.validate(record)
 
     format_validator = ActiveModel::Validations::
-            FormatValidator.new(with: /\+[0-9]{1,3}\.[0-9]{1,14}?/,
+            FormatValidator.new(with: /\A\+[0-9]{1,3}\.[0-9]{1,14}\z/,
                                 attributes: attribute)
     format_validator.validate(record)
   end

test/helpers/phone_format_helper_test.rb

@@ -0,0 +1,37 @@
+module PhoneFormatHelperTest
+  # https://en.wikipedia.org/wiki/E.164
+  def assert_phone_format(contact)
+    contact.phone = '+.1'
+    assert contact.invalid?
+
+    contact.phone = '+123.'
+    assert contact.invalid?
+
+    contact.phone = '+1.123456789123456'
+    assert contact.invalid?
+
+    contact.phone = '+134.1234567891234'
+    assert contact.invalid?
+
+    contact.phone = '+000.1'
+    assert contact.invalid?
+
+    contact.phone = '+123.0'
+    assert contact.invalid?
+
+    contact.phone = '+1.2'
+    assert contact.valid?
+
+    contact.phone = '+123.4'
+    assert contact.valid?
+
+    contact.phone = '+1.12345678912345'
+    assert contact.valid?
+
+    contact.phone = '+134.123456789123'
+    assert contact.valid?
+
+    contact.phone = '+134.00000000'
+    assert contact.invalid?
+  end
+end

test/models/contact_test.rb

@@ -1,6 +1,9 @@
 require 'test_helper'
+require 'helpers/phone_format_helper_test'
 
 class ContactTest < ActiveJob::TestCase
+  include PhoneFormatHelperTest
+
   setup do
     @contact = contacts(:john)
     @old_validation_type = Truemail.configure.default_validation_type
@@ -102,39 +105,9 @@ class ContactTest < ActiveJob::TestCase
     assert contact.invalid?
   end
 
-  # https://en.wikipedia.org/wiki/E.164
   def test_validates_phone_format
     contact = valid_contact
-
-    contact.phone = '+.1'
-    assert contact.invalid?
-
-    contact.phone = '+123.'
-    assert contact.invalid?
-
-    contact.phone = '+1.123456789123456'
-    assert contact.invalid?
-
-    contact.phone = '+134.1234567891234'
-    assert contact.invalid?
-
-    contact.phone = '+000.1'
-    assert contact.invalid?
-
-    contact.phone = '+123.0'
-    assert contact.invalid?
-
-    contact.phone = '+1.2'
-    assert contact.valid?
-
-    contact.phone = '+123.4'
-    assert contact.valid?
-
-    contact.phone = '+1.12345678912345'
-    assert contact.valid?
-
-    contact.phone = '+134.123456789123'
-    assert contact.valid?
+    assert_phone_format(contact)
   end
 
   def test_valid_without_address_when_address_processing_id_disabled

test/models/depp_contact_test.rb

@@ -0,0 +1,14 @@
+require 'test_helper'
+require 'helpers/phone_format_helper_test'
+
+class DeppContactTest < ActiveSupport::TestCase
+  include PhoneFormatHelperTest
+
+  setup do
+    @depp_contact = Depp::Contact.new
+  end
+
+  def test_validates_phone_format
+    assert_phone_format(@depp_contact)
+  end
+end

test/system/registrar_area/contact_test.rb

@@ -0,0 +1,45 @@
+require 'application_system_test_case'
+
+class RegistrarAreaContactTest < ApplicationSystemTestCase
+  setup do
+    @registrar = registrars(:bestnames)
+    @contact = contacts(:john)
+    sign_in users(:api_bestnames)
+  end
+
+  def test_creates_contact_with_invalid_phone
+    visit registrar_contacts_path
+    click_on 'New'
+
+    fill_in 'Ident', with: @contact.ident
+    fill_in 'Name', with: @contact.name
+    fill_in 'E-mail', with: @contact.email
+    fill_in 'Phone', with: '372'
+    click_on 'Create'
+
+    assert_text 'Phone number must be in +XXX.YYYYYYY format'
+  end
+
+  def test_updates_contact_with_invalid_phone
+    depp_contact = Depp::Contact.new(
+      id: @contact.id,
+      name: @contact.name,
+      code: @contact.code,
+      email: @contact.email,
+      phone: @contact.phone,
+      ident: @contact.ident,
+      ident_type: @contact.ident_type,
+      ident_country_code: @contact.ident_country_code)
+
+    Spy.on(Depp::Contact, :find_by_id).and_return(depp_contact)
+
+    visit edit_registrar_contact_path(depp_contact.code)
+
+    assert_text "Edit: #{depp_contact.name}"
+
+    fill_in 'Phone', with: '372'
+    click_on 'Save'
+
+    assert_text 'Phone number must be in +XXX.YYYYYYY format'
+  end
+end