From bc3962cdfadf82a943db0c426b7b075f89ed848c Mon Sep 17 00:00:00 2001
From: Artur Beljajev <artur.beljajev@internet.ee>
Date: Tue, 8 Aug 2017 15:13:13 +0300
Subject: [PATCH] Improve contact phone number validation

#569
---
 app/models/contact.rb             |  2 +-
 lib/validators/e164_validator.rb  | 10 ++++++++
 spec/lib/e164_phone_number.rb     | 29 +++++++++++++++++++++++
 spec/models/contact/phone_spec.rb | 38 +++++++++++++++++++------------
 4 files changed, 63 insertions(+), 16 deletions(-)
 create mode 100644 lib/validators/e164_validator.rb
 create mode 100644 spec/lib/e164_phone_number.rb

diff --git a/app/models/contact.rb b/app/models/contact.rb
index 392ad3b98..24756d6da 100644
--- a/app/models/contact.rb
+++ b/app/models/contact.rb
@@ -22,7 +22,7 @@ class Contact < ActiveRecord::Base
   validates :name, :email, :ident, :ident_type, presence: true
   validates :street, :city, :zip, :country_code, presence: true, if: 'self.class.address_processing?'
 
-  validates :phone, presence: true, format: /\+[0-9]{1,3}\.[0-9]{1,14}?/, phone: true
+  validates :phone, presence: true, e164: true, phone: true
 
   validates :email, format: /@/
   validates :email, email_format: { message: :invalid }, if: proc { |c| c.email_changed? }
diff --git a/lib/validators/e164_validator.rb b/lib/validators/e164_validator.rb
new file mode 100644
index 000000000..ee13db1e9
--- /dev/null
+++ b/lib/validators/e164_validator.rb
@@ -0,0 +1,10 @@
+class E164Validator < ActiveModel::EachValidator
+  def validate_each(record, attribute, value)
+    validator = ActiveModel::Validations::LengthValidator.new(maximum: 17, attributes: attribute)
+    validator.validate(record)
+
+    validator = ActiveModel::Validations::FormatValidator.new(with: /\+[0-9]{1,3}\.[0-9]{1,14}?/,
+                                                              attributes: attribute)
+    validator.validate(record)
+  end
+end
diff --git a/spec/lib/e164_phone_number.rb b/spec/lib/e164_phone_number.rb
new file mode 100644
index 000000000..e8740517b
--- /dev/null
+++ b/spec/lib/e164_phone_number.rb
@@ -0,0 +1,29 @@
+# https://en.wikipedia.org/wiki/E.164
+
+RSpec.shared_examples 'e164 phone number' do
+  describe 'validation' do
+    it 'rejects invalid format' do
+      model.send("#{attribute}=", '+.1')
+      model.validate
+      expect(model.errors).to be_added(attribute, :invalid)
+    end
+
+    it 'rejects longer than max length' do
+      model.send("#{attribute}=", '1' * 18)
+      model.validate
+      expect(model.errors).to be_added(attribute, :too_long, count: 17)
+    end
+
+    it 'accepts valid format' do
+      model.send("#{attribute}=", '+123.4')
+      model.validate
+      expect(model.errors).to_not be_added(attribute, :invalid)
+    end
+
+    it 'accepts max length' do
+      model.send("#{attribute}=", '1' * 17)
+      model.validate
+      expect(model.errors).to_not be_added(attribute, :too_long, count: 17)
+    end
+  end
+end
diff --git a/spec/models/contact/phone_spec.rb b/spec/models/contact/phone_spec.rb
index 78b07d164..f084ce64b 100644
--- a/spec/models/contact/phone_spec.rb
+++ b/spec/models/contact/phone_spec.rb
@@ -1,37 +1,45 @@
 require 'rails_helper'
+require 'lib/e164_phone_number'
 
 RSpec.describe Contact do
-  describe 'phone validation', db: false do
-    let(:contact) { described_class.new }
+  let(:contact) { described_class.new }
 
+  describe 'phone', db: false do
+    it_behaves_like 'e164 phone number' do
+      let(:model) { contact }
+      let(:attribute) { :phone }
+    end
+  end
+
+  describe 'phone validation', db: false do
     it 'rejects absent' do
       contact.phone = nil
       contact.validate
-      expect(contact.errors).to have_key(:phone)
-    end
-
-    it 'rejects invalid format' do
-      contact.phone = '123'
-      contact.validate
-      expect(contact.errors).to have_key(:phone)
+      expect(contact.errors).to be_added(:phone, :blank)
     end
 
     it 'rejects all zeros in country code' do
       contact.phone = '+000.1'
       contact.validate
-      expect(contact.errors).to have_key(:phone)
+      expect(contact.errors).to be_added(:phone, :invalid)
     end
 
-    it 'rejects all zeros in phone number' do
+    it 'rejects all zeros in subscriber number' do
       contact.phone = '+123.0'
       contact.validate
-      expect(contact.errors).to have_key(:phone)
+      expect(contact.errors).to be_added(:phone, :invalid)
     end
 
-    it 'accepts valid' do
-      contact.phone = '+123.4'
+    it 'translates :blank error message' do
+      contact.phone = nil
       contact.validate
-      expect(contact.errors).to_not have_key(:phone)
+      expect(contact.errors.generate_message(:phone, :blank)).to eq('Required parameter missing - phone')
+    end
+
+    it 'translates :invalid error message' do
+      contact.phone = nil
+      contact.validate
+      expect(contact.errors.generate_message(:phone, :invalid)).to eq('Phone nr is invalid')
     end
   end
 end